-
Notifications
You must be signed in to change notification settings - Fork 68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Negotiator violates the http.RoundTripper interface contract. #9
Comments
Based on the text, you are correct. Do you have a suggestion on how to fix this? |
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. |
/cc: @paulmey |
I never looked at the source of http.Roundtripper... 😢 |
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. |
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. |
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 |
From http.RoundTripper documentation:
The text was updated successfully, but these errors were encountered: