x/net/http2: Transport hangs on malformed response headers #16572
Comments
|
@icing Hmm, let me add some detail to this. Running the same version of Go on a different OS ( This output shows Go receiving an END_STREAM on a empty DATA frame and producing the output regardless. Do you have the output with the |
|
Yeah, I can't reproduce. And the code looks fine, and we have tests for this, too. @icing, the |
bradfitz
added
the
NeedsInvestigation
bradfitz
changed the title
|
Hmm, interesting... The same GET with curl: produces |
|
How nghttp client sees it: |
|
There looks like an error happening when Go is receiving your HEADERS frame. Something in the validation code seems confused. |
|
Correct, error is reported before DATA is read. Hmm. Let me know if I can provide anything else useful. |
|
Yeah, it doesn't like your headers. I've mailed out https://go-review.googlesource.com/25401 to add more logging about why. Can you patch that in? The relevant diff is: diff --git a/http2/frame.go b/http2/frame.go
index bd50d09..6769907 100644
--- a/http2/frame.go
+++ b/http2/frame.go
@@ -1419,6 +1419,9 @@ func (fr *Framer) readMetaFrame(hf *HeadersFrame) (*MetaHeadersFrame, error) {
hdec.SetEmitEnabled(true)
hdec.SetMaxStringLength(fr.maxHeaderStringLen())
hdec.SetEmitFunc(func(hf hpack.HeaderField) {
+ if VerboseLogs && logFrameReads {
+ log.Printf("http2: decoded hpack field %+v", hf)
+ }
if !httplex.ValidHeaderFieldValue(hf.Value) {
invalid = headerFieldValueError(hf.Value)
}
@@ -1477,10 +1480,16 @@ func (fr *Framer) readMetaFrame(hf *HeadersFrame) (*MetaHeadersFrame, error) {
}
if invalid != nil {
fr.errDetail = invalid
+ if VerboseLogs {
+ log.Printf("http2: invalid header: %v", invalid)
+ }
return nil, StreamError{mh.StreamID, ErrCodeProtocol}
}
if err := mh.checkPseudos(); err != nil {
fr.errDetail = err
+ if VerboseLogs {
+ log.Printf("http2: invalid pseudo headers: %v", err)
+ }
return nil, StreamError{mh.StreamID, ErrCodeProtocol}
}
return mh, nilI have an Apache mod_h2 docker container now, though, but without CGI. I'll try and reproduce too. |
|
I am using the prebuilt package for rc4. Can I get that in without building go locally? (gonoob) |
To help debug golang/go#16572 Change-Id: Ia154faedd243a06a4110f6e6a4885b7cd0a04e1f Reviewed-on: https://go-review.googlesource.com/25401 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
|
@icing, I just submitted it. You can now run: And then re-run your client program. (using code that looks like #16519 (comment)) |
|
(That is, you can keep using the prebuilt rc4 package, but don't use its built-in copy of http2, use the golang.org/x/net/http2 one.... think of those as mod_http2 vs mod_h2, respectively) |
|
Of course, the problem lies in the python code... ^^ If I remove the leading space from the content-type header, go is fine with it. Interestingly, nghttp seems to just silently omit this header and report nothing suspicious. Another reason to test with different clients... I think the choking by go on such headers is just fine. I'll add an issue for mod_h2 to trim headers before sending them on a h2 stream, since they might come from unreliable sources, such as a python script... |
|
Sorry for all the excitement. |
|
The reason why nghttp2 ignores these header fields is that there are so many misguided header field in the internet, and just treating them hard error breaks web browsing if nghttp2 is used as forward proxy. |
|
There is still problem here. Go client just hangs in this case. It should terminate stream, or connection instead of freezing. |
I think that's kinda wrong. Misguided header fields can only exist on the Internet if too-forgiving implementations allow them to exist. And so far that's only been the case for often-ill-defined HTTP/1. With HTTP/2 we get a clean start with defined rules. I think we should follow the rules, and make it the job of the HTTP/1<->HTTP/2 proxies to make the policies decisions on what to strip. But on the wire, HTTP/2 is clear about what is permitted.
Indeed. I'm looking into that now. The stream error should be reported to the user. |
|
Agreed with @tatsuhiro-t here: not only should the stream error be reported to the user, but a RST_STREAM frame should be emitted, which it isn't being. |
|
@tatsuhiro-t whatever the proxy does, it would be good if the library, by default, will not accept such headers. The decision to discard/trim/fail on such headers should be done by the user of nghttp2, IMHO. |
Well, probably I had few words ,but I totally agree with you. And what nghttp2 does is what you described. The bad header field is just ignored and stripped. |
|
I think it is not stripped, but sent onwards to the poor go client. ;-) |
That's not what it sounds like. I disagree it should be ignored and stripped. I think it should be an error, as it's not a valid HTTP header field. The HTTP/2 spec references the HTTP/1 spec which says a header-field is a token and token is https://tools.ietf.org/html/rfc7230#section-3.2.6 which doesn't include spaces. |
|
@icing Stripping header is occurred when nghttp2 received header fields. For transmission, nghttp2 does not check header fields, and rather it is application's job to make sure that header is all right. We decided to do this because application most likely validates what it sends, and validating them in nghttp2 could be redundant. But I'm ok to add those checks to nghttp2. |
|
@tatsuhiro-t it would be nice and lazy people like myself, who do not check enough, find out earlier. |
|
@bradfitz I'd like to make it error.. but when we deployed HTTP/2 forward proxy on public internet, I occasionally hit those sites that send invalid header fields, and they just stop working if we make it error and tear down the stream. |
|
@icing Right, let's discuss it in nghttp2 issue thread |
|
It could be optional: hyper-h2 is growing flags that will let users optionally turn off header validation, but it defaults to "strict". nghttp2 could do this as well. |
|
@tatsuhiro-t, then nghttp2 is part of the problem, if you saw that garbage on h2, since it contributes to the idea that garbage is okay. If you only saw the garbage on h1, then it's the proxy's job to clean or ignore things before translation to h2. Where is the nghttp2 bug? |
|
Well, let's stay constructive. I think we all start testing more interop between different implementations and that is a good thing. I am about to add go version >= 1.7 tests to my regression tests and would like to add hyper too. If a public facing Apache mod_h2 with some trivial config would help you, let me know. Can be arranged. |
|
Yeah, I want to do more interop testing too. I'll start a separate thread. I've fixed this Go issue in https://golang.org/cl/25402 |
|
I also have a patch for nghttp2 to reproduce this issue 100%, and the proposed patch fixed this hanging issue. Thank you. |
|
CL https://golang.org/cl/25402 mentions this issue. |
Sorry, I didn't mean to be come across as adversarial or disparage either @tatsuhiro-t or nghttp2. I was just trying to argue the point that if a protocol is new and is trying to be strict (as HTTP/2 is, especially compared to super-sloppy HTTP/1), implementations of the protocol must also be super strict, especially in the early days, otherwise all the strict wording in the spec is useless and a de-facto reality emerges and all of our implementations will forever have to deal with everybody's corner cases. And I don't think we would enjoy that. I haven't looked at the nghttp2 or Apache APIs and where the responsibility boundaries are, but I'm just very anxious about the idea of a library deciding to hide garbage from upper layers, when the garbage is specified in the specs as illegal. Go's http2 package is strict by default, but the Framer has AllowIllegalReads and AllowIllegalWrites (https://godoc.org/golang.org/x/net/http2#Framer) specifically to allow people to write testing tools to generate and accept garbage. But we never hide garbage from the user. We just return errors instead, hoping that causes push-back to the other side to be stricter. |
|
Totally agree. mod_h2 has to translate between the worlds as there is tons of legacy stuff out there running in the server and it needs to be more strict in that.
|
|
Never mind, and I understand that my argument for interoperability for bad header is poor because HTTP/2 implementation is brand-new anyway, and I was probably confused by the late night, sorry about that. Anyway, the discussion is valuable for me, and give me a good insight that the header field handling of nghttp2 could be improved. I created issue nghttp2/nghttp2#642 to review our current behaviour, and we could go with zero tolerance rule. Probably, the best thing as library is give a choice for the application about how to handle these headers. Will see. Thank you. |
Done. The change (currently out for review) now sends RST_STREAM back in this case too, with new tests. |
bradfitz
removed
the
NeedsInvestigation
bradfitz
changed the title
|
Nice! 🍪 |
If the Transport got a stream error on the response headers, it was never unblocking the client. Previously, Response.Body reads would be aborted with the stream error, but RoundTrip itself would never unblock. The Transport now also sends a RST_STREAM to the server when we encounter a stream error. Also, add a "Cause" field to StreamError with additional detail. The old code was just returning the detail, without the stream error header. Fixes golang/go#16572 Change-Id: Ibecedb5779f17bf98c32787b68eb8a9b850833b3 Reviewed-on: https://go-review.googlesource.com/25402 Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Andrew Gerrand <adg@golang.org>
Please answer these questions before submitting your issue. Thanks!
go version)?go1.7rc4
go env)?OS X ( 15.6.0 Darwin Kernel Version 15.6.0: Thu Jun 23 18:25:34 PDT 2016; root:xnu-3248.60.10~1/RELEASE_X86_64 x86_64)
Tested the http2 implementation of go against Apache mod_h2, using a simple GET on a python CGI.
The output from the CGI.
Go did hang and did not produce any results. The difference between a stuck GET and a successful one seems to be the EOF handling. On most responses Apache sets the EOF flag on the last DATA frame. However with CGI processes, the EOF is only encountered when the last DATA has already been written. In this case, Apache send a last DATA frame with length 0 and EOF=1. This seems to be ignored by the go implemenation, keeping it waiting.
Additionally to that, Apache will close the connection after a timeout, but the go client still will not return although the connection was properly closed.
The text was updated successfully, but these errors were encountered: