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: HTTP/2 conformance failures using h2spec #25023

Closed
elcore opened this issue Apr 23, 2018 · 25 comments
Closed

x/net/http2: HTTP/2 conformance failures using h2spec #25023

elcore opened this issue Apr 23, 2018 · 25 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@elcore
Copy link

elcore commented Apr 23, 2018

What version of Go are you using (go version)?

go version 1.10.1 linux/arm64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

GOARCH="arm64"
GOBIN=""
GOEXE=""
GOHOSTARCH="arm64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/XXXX/go"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/linux_arm64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build393280655=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"

What did you do?

I'm using the h2spec program (https://github.com/summerwind/h2spec) and running the following command:

./h2spec -S -t -h caddyserver.com -p 443

What did you expect to see?

146 tests, 146 passed, 0 skipped, 0 failed

What did you see instead?

Failures: 

Hypertext Transfer Protocol Version 2 (HTTP/2)
  4. HTTP Frames
    4.2. Frame Size
      × 3: Sends a large size HEADERS frame that exceeds the SETTINGS_MAX_FRAME_SIZE
        -> The endpoint MUST respond with a connection error of type FRAME_SIZE_ERROR.
           Expected: GOAWAY Frame (Error Code: FRAME_SIZE_ERROR)
                     Connection closed
             Actual: DATA Frame (length:0, flags:0x01, stream_id:1)

  5. Streams and Multiplexing
    5.1. Stream States
      × 11: closed: Sends a DATA frame
        -> The endpoint MUST treat this as a connection error of type STREAM_CLOSED.
           Expected: GOAWAY Frame (Error Code: STREAM_CLOSED)
                     Connection closed
             Actual: RST_STREAM Frame (length:4, flags:0x00, stream_id:1)

  6. Frame Definitions
    6.9. WINDOW_UPDATE
      6.9.1. The Flow-Control Window
        × 3: Sends multiple WINDOW_UPDATE frames increasing the flow control window to above 2^31-1 on a stream
          -> The endpoint MUST sends a RST_STREAM frame with a FLOW_CONTROL_ERROR code.
             Expected: RST_STREAM Frame (Error Code: FLOW_CONTROL_ERROR)
               Actual: RST_STREAM Frame (length:4, flags:0x00, stream_id:1)

      6.9.2. Initial Flow-Control Window Size
        × 2: Sends a SETTINGS frame for window size to be negative
          -> The endpoint MUST track the negative flow-control window.
             Expected: DATA Frame (length:1, flags:0x00, stream_id:1)
               Actual: Timeout

  8. HTTP Message Exchanges
    8.1. HTTP Request/Response Exchange
      8.1.2. HTTP Header Fields
        8.1.2.2. Connection-Specific Header Fields
          × 1: Sends a HEADERS frame that contains the connection-specific header field
            -> The endpoint MUST respond with a stream error of type PROTOCOL_ERROR.
               Expected: GOAWAY Frame (Error Code: PROTOCOL_ERROR)
                         RST_STREAM Frame (Error Code: PROTOCOL_ERROR)
                         Connection closed
                 Actual: DATA Frame (length:51, flags:0x01, stream_id:1)
          × 2: Sends a HEADERS frame that contains the TE header field with any value other than "trailers"
            -> The endpoint MUST respond with a stream error of type PROTOCOL_ERROR.
               Expected: GOAWAY Frame (Error Code: PROTOCOL_ERROR)
                         RST_STREAM Frame (Error Code: PROTOCOL_ERROR)
                         Connection closed
                 Actual: DATA Frame (length:53, flags:0x01, stream_id:1)

        8.1.2.6. Malformed Requests and Responses
          × 1: Sends a HEADERS frame with the "content-length" header field which does not equal the DATA frame payload length
            -> The endpoint MUST treat this as a stream error of type PROTOCOL_ERROR.
               Expected: GOAWAY Frame (Error Code: PROTOCOL_ERROR)
                         RST_STREAM Frame (Error Code: PROTOCOL_ERROR)
                         Connection closed
                 Actual: Timeout
          × 2: Sends a HEADERS frame with the "content-length" header field which does not equal the sum of the multiple DATA frames payload length
            -> The endpoint MUST treat this as a stream error of type PROTOCOL_ERROR.
               Expected: GOAWAY Frame (Error Code: PROTOCOL_ERROR)
                         RST_STREAM Frame (Error Code: PROTOCOL_ERROR)
                         Connection closed
                 Actual: WINDOW_UPDATE Frame (length:4, flags:0x00, stream_id:0)

HPACK: Header Compression for HTTP/2
  4. Dynamic Table Management
    4.2. Maximum Table Size
      × 1: Sends a dynamic table size update at the end of header block
        -> The endpoint MUST treat this as a decoding error.
           Expected: GOAWAY Frame (Error Code: COMPRESSION_ERROR)
                     Connection closed
             Actual: DATA Frame (length:0, flags:0x01, stream_id:1)

146 tests, 137 passed, 0 skipped, 9 failed

As a Caddy contributor, I'm redirecting the original issue caddyserver/caddy#2132 here because it's an upstream issue

cc @mholt @bazzadp

@agnivade agnivade changed the title HTTP/2 conformance - h2spec failures net/http: HTTP/2 conformance failures using h2spec Apr 24, 2018
@agnivade agnivade added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 24, 2018
@agnivade agnivade added this to the Go1.11 milestone Apr 24, 2018
@agnivade
Copy link
Contributor

I see similar failures even without strict mode. Only difference is that in my runs 6.9.1 passes but 5.1x6 fails.

ping @bradfitz.

@meirf
Copy link
Contributor

meirf commented May 4, 2018

Running against http2.golang.org gives same success grade 137/146, but slightly different failures. (Though, I'm guessing caddyserver.com is using a newer version.)

In terms of priority:

  • As bazzadp noted in the caddy counterpart issue, "most of these tests are about how to handle bad client requests".
  • google.com has a lower success grade: 135/146.

Probably best to keep this as one issue which each CL Updates. Maybe also add help wanted.

@bradfitz bradfitz added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels May 4, 2018
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 4, 2018
@bradfitz bradfitz changed the title net/http: HTTP/2 conformance failures using h2spec x/net/http2: HTTP/2 conformance failures using h2spec May 4, 2018
@bradfitz
Copy link
Contributor

bradfitz commented May 4, 2018

These would be nice to fix but I don't think any of them are critical. It's nice that we do better than google.com at least. :-)

I agree let's just keep this one bug for all of them.

/cc @tombergan

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/111675 mentions this issue: x/net/http2: correct overflow protection

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/111676 mentions this issue: x/net/http2: a closed stream cannot receive data

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/111677 mentions this issue: x/net/http2: headers cannot be received on half closed streams

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/111678 mentions this issue: x/net/http2: send GOAWAY when headers exceed the max frame size

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/111679 mentions this issue: x/net/http2: receiving too much data is a protocol error

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/111680 mentions this issue: x/net/http2: reject connection-level headers with a protocol error

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/111681 mentions this issue: x/net/http2: dynamic table updates must occur first

gopherbot pushed a commit to golang/net that referenced this issue May 7, 2018
Dynamic table size updates must occur at the beginning of the first
header block.

Updates golang/go#25023

Change-Id: I7fd4f191da0a97cab26666545191460a6f6c1433
Reviewed-on: https://go-review.googlesource.com/111681
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@bradfitz bradfitz modified the milestones: Go1.11, Unplanned May 22, 2018
gopherbot pushed a commit to golang/net that referenced this issue May 22, 2018
Updates golang/go#25023

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

Change https://golang.org/cl/121415 mentions this issue: http2: make Server send GOAWAY if Handler sets "Connection: close" header

gopherbot pushed a commit to golang/net that referenced this issue Jun 28, 2018
Correct overflow detection when going negative.

Updates golang/go#25023

Change-Id: Ic2ddb7ee757f081d1826bfbbfd884e2b7e819335
Reviewed-on: https://go-review.googlesource.com/111675
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
gopherbot pushed a commit to golang/net that referenced this issue Jun 28, 2018
…ader

In Go's HTTP/1.x Server, a "Connection: close" response from a handler results
in the TCP connection being closed.

In HTTP/2, a "Connection" header is illegal and we weren't previously
handling it, generating malformed responses to clients. (This is one
of our violations listed in golang/go#25023)

There was also a feature request in golang/go#20977 for a way for
HTTP/2 handlers to close the connection after the response. Since we
already close the connection for "Connection: close" for HTTP/1.x, do
the same for HTTP/2 and map it to a graceful GOAWAY errcode=NO
response.

Updates golang/go#25023 (improves 8.1.2.2. Connection-Specific Header Fields)
Updates golang/go#20977 (fixes after vendor into std)

Change-Id: Iefb33ea73be616052533080c63b54ae679b1d154
Reviewed-on: https://go-review.googlesource.com/121415
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
gopherbot pushed a commit to golang/net that referenced this issue Jul 10, 2018
Headers received on a half closed remote stream must respond with a
stream error of type STREAM_CLOSED.

Updates golang/go#25023

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

Change https://golang.org/cl/122877 mentions this issue: net/http: update bundled http2

gopherbot pushed a commit that referenced this issue Jul 10, 2018
Updates http2 to x/net/http2 git rev 292b43b for:

    http2: reject incoming HEADERS in Server on half-closed streams
    https://golang.org/cl/111677

Updates #25023

Change-Id: I479ae9b5b899fb0202e6301d02535fb6aeb4997a
Reviewed-on: https://go-review.googlesource.com/122877
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

Can we get an updated summary of where we're at on this bug using the latest (master) code?

gopherbot pushed a commit to golang/net that referenced this issue Jul 10, 2018
Data sent on a closed stream is treated as a connection error of type
STREAM_CLOSED.

Updates golang/go#25023

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

elcore commented Jul 10, 2018

Sure

Failures:

Hypertext Transfer Protocol Version 2 (HTTP/2)
  4. HTTP Frames
    4.2. Frame Size
      × 3: Sends a large size HEADERS frame that exceeds the SETTINGS_MAX_FRAME_SIZE
        -> The endpoint MUST respond with a connection error of type FRAME_SIZE_ERROR.
           Expected: GOAWAY Frame (Error Code: FRAME_SIZE_ERROR)
                     Connection closed
             Actual: DATA Frame (length:14, flags:0x01, stream_id:1)

  8. HTTP Message Exchanges
    8.1. HTTP Request/Response Exchange
      8.1.2. HTTP Header Fields
        8.1.2.2. Connection-Specific Header Fields
          × 1: Sends a HEADERS frame that contains the connection-specific header field
            -> The endpoint MUST respond with a stream error of type PROTOCOL_ERROR.
               Expected: GOAWAY Frame (Error Code: PROTOCOL_ERROR)
                         RST_STREAM Frame (Error Code: PROTOCOL_ERROR)
                         Connection closed
                 Actual: DATA Frame (length:51, flags:0x01, stream_id:1)
          × 2: Sends a HEADERS frame that contains the TE header field with any value other than "trailers"
            -> The endpoint MUST respond with a stream error of type PROTOCOL_ERROR.
               Expected: GOAWAY Frame (Error Code: PROTOCOL_ERROR)
                         RST_STREAM Frame (Error Code: PROTOCOL_ERROR)
                         Connection closed
                 Actual: DATA Frame (length:53, flags:0x01, stream_id:1)

HPACK: Header Compression for HTTP/2
  4. Dynamic Table Management
    4.2. Maximum Table Size
      × 1: Sends a dynamic table size update at the end of header block
        -> The endpoint MUST treat this as a decoding error.
           Expected: GOAWAY Frame (Error Code: COMPRESSION_ERROR)
                     Connection closed
             Actual: DATA Frame (length:14, flags:0x01, stream_id:1)

Finished in 14.6203 seconds
146 tests, 142 passed, 0 skipped, 4 failed

@bradfitz
Copy link
Contributor

Okay, at this point we have few enough, let's file separate bugs for them, with titles of the form:

   x/net/http2: h2spec violation 8.1.2.2

@fraenkel, you want to file those?

IIRC, one of those @fraenkel already identified as a bug in the h2spec test. If so, let's file the bug anyway just for reference and we can contact the h2spec authors to get it fixed.

@fraenkel
Copy link
Contributor

HTTP 8.1.2.x is https://go-review.googlesource.com/c/net/+/111680

HTTP 4.2.3 is an incorrect test, it is using the wrong frame size, see https://go-review.googlesource.com/c/net/+/111678

HPACK 4.2.1 I need to investigate. We have a fix in place but either its not being triggered or it could be a sequencing issue.

Will go do some digging...

@fraenkel
Copy link
Contributor

fraenkel commented Jul 11, 2018

Hmm, I just ran the latest h2specs with Go 1.10.3 + latest http2 and Go 1.11(tip) and got different results than above as well as between the 2 releases and between runs. I will see anywhere from 3 - 6 failures.
Running with GODEBUG creates more consistent failures.

@fraenkel
Copy link
Contributor

The most errors I see are below. HTTP/2 6.10, HTTP/2 8.1.2.2.1 and HTTP/2 8.1.2.2.2 always occur.
HTTP/2 8.1.2.2.x are fixed with the CL above.

Failures: 

Hypertext Transfer Protocol Version 2 (HTTP/2)
  5. Streams and Multiplexing
    5.1. Stream States
      × 5: half closed (remote): Sends a DATA frame
        -> The endpoint MUST respond with a stream error of type STREAM_CLOSED.
           Expected: GOAWAY Frame (Error Code: STREAM_CLOSED)
                     RST_STREAM Frame (Error Code: STREAM_CLOSED)
                     Connection closed
             Actual: DATA Frame (length:42, flags:0x01, stream_id:1)
      × 6: half closed (remote): Sends a HEADERS frame
        -> The endpoint MUST respond with a stream error of type STREAM_CLOSED.
           Expected: GOAWAY Frame (Error Code: STREAM_CLOSED)
                     RST_STREAM Frame (Error Code: STREAM_CLOSED)
                     Connection closed
             Actual: DATA Frame (length:42, flags:0x01, stream_id:1)

  6. Frame Definitions
    6.1. DATA
      × 2: Sends a DATA frame on the stream that is not in "open" or "half-closed (local)" state
        -> The endpoint MUST respond with a stream error of type STREAM_CLOSED.
           Expected: GOAWAY Frame (Error Code: STREAM_CLOSED)
                     RST_STREAM Frame (Error Code: STREAM_CLOSED)
                     Connection closed
             Actual: DATA Frame (length:42, flags:0x01, stream_id:1)

    6.10. CONTINUATION
      × 1: Sends multiple CONTINUATION frames preceded by a HEADERS frame
        -> The endpoint must accept the frame.
           Expected: HEADERS Frame (stream_id:1)
             Actual: Connection closed

  8. HTTP Message Exchanges
    8.1. HTTP Request/Response Exchange
      8.1.2. HTTP Header Fields
        8.1.2.2. Connection-Specific Header Fields
          × 1: Sends a HEADERS frame that contains the connection-specific header field
            -> The endpoint MUST respond with a stream error of type PROTOCOL_ERROR.
               Expected: GOAWAY Frame (Error Code: PROTOCOL_ERROR)
                         RST_STREAM Frame (Error Code: PROTOCOL_ERROR)
                         Connection closed
                 Actual: DATA Frame (length:51, flags:0x01, stream_id:1)
          × 2: Sends a HEADERS frame that contains the TE header field with any value other than "trailers"
            -> The endpoint MUST respond with a stream error of type PROTOCOL_ERROR.
               Expected: GOAWAY Frame (Error Code: PROTOCOL_ERROR)
                         RST_STREAM Frame (Error Code: PROTOCOL_ERROR)
                         Connection closed
                 Actual: DATA Frame (length:53, flags:0x01, stream_id:1)


@fraenkel
Copy link
Contributor

I have created
#26321 - 8.1.2.2 (which is has a CL)
#26322 - 5.1, I believe is an h2spec issue
#26323 - 6.1.2, also an h2spec issue
#26324 - 6.10.1 (needs investigation)

The others I do not see. I am using a simple h2 server, not caddy.

@fraenkel
Copy link
Contributor

When I switched my handler to return no body, 2 more failures appeared and the causes for the others changed.
#26326 generic 2/2 & 2/3
#26327 http/2 5.1.2

@bradfitz
Copy link
Contributor

@fraenkel, thanks. I'm going to close this meta bug, but if you or others open other h2spec bugs, feel free to reference this bug in the new bug, or reference the new bug from this bug so they're all linked together. (Or we can search for "h2spec", too.)

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/123515 mentions this issue: net/http: update bundled http2

gopherbot pushed a commit that referenced this issue Jul 12, 2018
Updates bundled x/net/http2 to git rev cffdcf67 for:

    http2: use GetBody unconditionally on Transport retry, when available
    https://golang.org/cl/123476

    http2: a closed stream cannot receive data
    https://golang.org/cl/111676

Updates #25009
Updates #25023

Change-Id: I84f50cc50c0fa5a3c34f0037a9cb1ef468e5f0d9
Reviewed-on: https://go-review.googlesource.com/123515
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
Contributor

Change https://golang.org/cl/127355 mentions this issue: vendor: update golang.org/x/net/http2/hpack

gopherbot pushed a commit that referenced this issue Aug 1, 2018
Updates bundled golang.org/x/net/http2/hpack to x/net git rev 22bb95c5e for:

   http2/hpack: lazily build huffman table on first use
   https://golang.org/cl/127275

   http2/hpack: reduce memory for huffman decoding table
   https://golang.org/cl/127235

   http2/hpack: dynamic table updates must occur first
   https://golang.org/cl/111681

And a typo & gofmt CL.

Updates #25023

Change-Id: I7027fdb4982305aa671d811fe87f61e5df0f8e0e
Reviewed-on: https://go-review.googlesource.com/127355
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Andrew Bonventre <andybons@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/153977 mentions this issue: http2: Revert a closed stream cannot receive data

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/153978 mentions this issue: http2/hpack: track the beginning of a header block

gopherbot pushed a commit to golang/net that referenced this issue Dec 13, 2018
dynamic table size updates must occur at the beginning of the first
header block. The original fix, golang/go#25023, guaranteed it was at
the beginning of the very first block. The Close method implicitly
marked the end of the current header. We now document the Close behavior
and can track when we are at the beginning of the first block.

Updates golang/go#29187

Change-Id: I83ec39546527cb17d7de8a88ec417a46443d2baa
Reviewed-on: https://go-review.googlesource.com/c/153978
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
gopherbot pushed a commit to golang/net that referenced this issue Jan 19, 2019
This reverts CL 111676 for golang/go#25023.

Reason for revert: The code change no longer issued a WindowUpdate which
is required when processing data. The original issue found in golang/go#25023
is not present after the revert.

Updates golang/go#28204

Change-Id: Iadbb63d50ca06df1281e699b9ef13181d0593f80
Reviewed-on: https://go-review.googlesource.com/c/153977
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@golang golang locked and limited conversation to collaborators Dec 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants