Negotiator violates the http.RoundTripper interface contract.

[dihedron]

From http.RoundTripper documentation:

type RoundTripper interface {
// RoundTrip executes a single HTTP transaction, returning
// a Response for the provided Request.
//
// RoundTrip should not attempt to interpret the response. In
// particular, RoundTrip must return err == nil if it obtained
// a response, regardless of the response's HTTP status code.
// A non-nil err should be reserved for failure to obtain a
// response. Similarly, RoundTrip should not attempt to
// handle higher-level protocol details such as redirects,
// authentication, or cookies.
//
// RoundTrip should not modify the request, except for
// consuming and closing the Request's Body.
RoundTrip(*Request) (*Response, error)
}

[boumenot]

Based on the text, you are correct. Do you have a suggestion on how to fix this?

[dihedron]

In the RoundTripper docs the Go folks seem to suggest that one should keep this logic at the http.Client level. I'm exploring the possibility of creating a wrapper - possibly with an embedded http.Client - and reimplementing the relevant methods. I would like to keep the handshaking logic under the hood, without the user even noticing just like you did because that's really cool.
What's your advice? I know that's quite preliminary analysis, but am I missing something big?
Anyway if it works in my hobby project, I promise I'll fork your repo and push my experiments for you to evaluate. Yet I have so little time to spend on coding that I'm sure someone will do it faster and far better! :-)

[boumenot]

/cc: @paulmey

[paulmey]

I never looked at the source of http.Roundtripper... 😢
I guess this package clearly violates those 'should's. I don't see how we could keep it transparent at the http.Client level, since that is a struct and not an interface, so it cannot be implemented outside of its package...
The point of this package was to explicitly provide this functionality transparently to other software. We us it in Packer to be able to use NTLM for WinRM connections. An alternative could be to pick one of the more composable http client frameworks out there and tailor it to that?
Let me know if you think of something we could do here.

[dihedron]

I studied a little in the meanwhile; an update follows. Feel free to ignore me: your code just works, yet I'll point out some issues which may lead to problems further on down the line.

Most implementations I've seen these days use the "Session" approach, i.e. a wrapper object embedding an http.Client and some form of Authenticator; for instance OpenStack clients do so to authenticate against KeyStone, and so does this other SSPI project here on GitHub.

I understand your requirements as a need to use a plain http.Client in order not to break your clients. To me this excludes recourse to other "composable HTTP clients": I looked at a few of them but they are different beasts altogether and your clients would have at least to switch package. Not seamless, really.

I agree you cannot enrich the net/http package itself but maybe you can define an interface in your package that is modeled after the standard http.Client AND satisfied also by an authenticating wrapper? You let http.Client unknowingly implement your interface placing your interface at the top of the hierarchy instead of the other way around. With this form of ducktyping, your users would be able to plug a plain http.Client or a ntlmssp/SSPSession according to their needs with minimal changes in their code (limited to their method signatures having type Session interface {} instead of http.Client).

I went a bit further exploring the Authenticator scenario and I see another problem here. How would the Authenticator interact with the Client across the many submits required to go to a corporate server (401) possibly via an authenticating proxy (407)? I can see why the Go folks moved this responsibility out of the http.Client: I read RFC4559 (SPNEGO) and it looks like there's a problem with POST and PUT requests which "should not send the user data (=payload) until the authentication has completed": this scenario involves quite a bit of caching/copying of the request.Body and Headers (which you already do) for resubmitting, far beyond the responsibility of a single roundtrip. It's OK for a simple GET request but how can you transparently and efficiently submit some huge amount of data, such as a massive multipart file upload, or a stream, without breaking out of the http.Client Do() method contract? If you're interested in designing this general purpose solution together please advise, otherwise this is the point where you can close the issue. It's OK for me.

My next move is sniffing some real browsers to see how they behave with respect to this POST/PUT issue.

[paulmey]

I agree with most of your comments. Most authentication schemes are at the session (vs the request) level. I can understand why it's a bad idea to hide this concept in a 'http.RoundTripper' interface.
IMHO, the proper way of going about this is would be to change the HTTP infrastructure in https://github.com/masterzen/winrm to not use bare 'http.Client's or 'http.Transport's, but some composable abstraction first. And then transform the code here into a component that can be used in that new infrastructure. Thoughts?

[mwhooker]

I've been investigating this issue hashicorp/packer#5257 and I've come to the same conclusion as this ticket. It seems that the round tripper is creating a response that violates assumptions that the rest of the http library makes.

I think I need to stop working on this for now, but if I have any brilliant ideas I will make a PR