Skip to content
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

x/net/http2: does not conform to RFC strictly #16046

Closed
changlidong opened this issue Jun 13, 2016 · 4 comments
Closed

x/net/http2: does not conform to RFC strictly #16046

changlidong opened this issue Jun 13, 2016 · 4 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@changlidong
Copy link

I builded a HTTP2 Server Demo by x/net/http2 of Golang and found some behaviors that does not conform to RFC7540 strictly. I’d like to separate it into four categories: Wrong ErrorCode, Wrong ErrorType, WrongFrame and Ignoring.

  • Wrong ErrorCode
    this means client receives an unExpected ErrorCode from the server according to the rfc.
    e.g. when an endpoint receives a HEADERS Frame when the stream is in half closed (remote) state, the server response a stream error of code PROTOCOL_ERROR. While rfc has said that "it MUST respond with a stream error of code STREAM_CLOSED"(rfc 7540, section 5.1).
  • Wrong ErrorType
    this means the server is expected to response a connection error, but actually it responses a stream error instead, or vice versa.
    e.g. when sending a DATA Frame in a stream that is in idle state, the server responds with an RST_STREAM. while rfc has ruled that "it MUST treat it as a connection error of type PROTOCOL_ERROR"(rfc 7540, section 5.1).
  • Wrong Frame
    this is the case that when sending a HEADERS frame that contains the connection-specific header field, we expect to receive a RST_STREAM Frame from server, because rfc has ruled that "An endpoint MUST NOT generate an HTTP/2 message containing connection-specific header fields; any message containing connection-specific header fields MUST be treated as malformed"(rfc 7540, section 8.1.2.2) and "Malformed requests or responses that are detected MUST be treated as a stream error of type PROTOCOL_ERROR"(rfc 7540, section 8.1.2.6). but actually the client receives a DATA frame, the payload of which is "request header "Connection" is not valid in HTTP/2”.
  • Ignoring
    this means that the server seems to ignore some frames under certain conditions. for example, when sending a Headers Frame that depend on itself , the server does not seems to be affected and responds with a normal DATA Frame. while rfc has said that "A stream cannot depend on itself. An endpoint MUST treat this as a stream error of type PROTOCOL_ERROR"(rfc 7540, section 5.3.1).

all case code can be found from here.
one more question, any plans to fix these problems in the near future?

@robpike robpike added this to the Go1.8 milestone Jun 13, 2016
@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 5, 2016
@gopherbot
Copy link

CL https://golang.org/cl/31736 mentions this issue.

gopherbot pushed a commit to golang/net that referenced this issue Oct 21, 2016
Per RFC 7540, section 5.1 discussing the "idle" state: "Receiving any
frame other than HEADERS or PRIORITY on a stream in this state MUST be
treated as a connection error (Section 5.4.1) of type PROTOCOL_ERROR."x

Updates golang/go#16046

Change-Id: Ie0696059e76a092e483aea5ee017d9729339d309
Reviewed-on: https://go-review.googlesource.com/31736
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@gopherbot
Copy link

CL https://golang.org/cl/31858 mentions this issue.

gopherbot pushed a commit to golang/net that referenced this issue Oct 24, 2016
Fix spec bug: Section 5.3.1 says that self-dependencies should be
treated as a stream error of type PROTOCOL_ERROR.

Updates golang/go#16046

Change-Id: I8b5dc8808943dc96aac0c543c7032fa989ab9e0b
Reviewed-on: https://go-review.googlesource.com/31858
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@bradfitz
Copy link
Contributor

Wrong ErrorCode

If an endpoint receives additional frames, other than WINDOW_UPDATE, PRIORITY, or RST_STREAM, for a stream that is in this state, it MUST respond with a stream error (Section 5.4.2) of type STREAM_CLOSED.

But the spec on HEADERS says otherwise, which we implement:

        // [...] The identifier of a newly established stream MUST be
    // numerically greater than all streams that the initiating
    // endpoint has opened or reserved. [...]  An endpoint that
    // receives an unexpected stream identifier MUST respond with
    // a connection error (Section 5.4.1) of type PROTOCOL_ERROR.
    if id <= sc.maxStreamID {
        return ConnectionError(ErrCodeProtocol)
    }

In any case, please file a separate bug about this. Many small, independent bugs are easier than one big bug.

Wrong ErrorType

this means the server is expected to response a connection error, but actually it responses a stream error instead, or vice versa.
e.g. when sending a DATA Frame in a stream that is in idle state, the server responds with an RST_STREAM. while rfc has ruled that "it MUST treat it as a connection error of type PROTOCOL_ERROR"(rfc 7540, section 5.1).

Fixed by 41c5c5cb12965112d2e298342c73e6b3e045a14a

Wrong Frame

"An endpoint MUST NOT generate an HTTP/2 message containing connection-specific header fields; any message containing connection-specific header fields MUST be treated as malformed"(rfc 7540, section 8.1.2.2) and "Malformed requests or responses that are detected MUST be treated as a stream error of type PROTOCOL_ERROR"(rfc 7540, section 8.1.2.6). but actually the client receives a DATA frame, the payload of which is "request header "Connection" is not valid in HTTP/2”.

Hmm, the GFE does it the way we do. As long as we reject it, it seems
non-critical. I'll ask the GFE team why they did what they did.

Ignoring

A stream cannot depend on itself. An endpoint MUST treat this as a stream error of type PROTOCOL_ERROR"(rfc 7540, section 5.3.1)."

Fixed by 65dfc08770ce66f74becfdff5f8ab01caef4e946

I'm going to close this bug for now.

Please file separate bugs for anything else you find. Again, I'd prefer many small bugs instead of 1 big bug with separate issues.

Thanks!

@changlidong
Copy link
Author

sorry about that. I'll deal with it these days. much appreciate your work

@golang golang locked and limited conversation to collaborators Oct 25, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants