search

netty / netty

Netty project - an event-driven asynchronous network application framework
http://netty.io
Apache License 2.0
33.53k stars 15.96k forks source link

testsuite-http2's h2spec test is failing RFC 9113 compliance, but failing tests are hidden by the configuration #13630

Open KingMob opened 1 year ago

KingMob commented 1 year ago

Expected behavior

A minimal Netty HTTP/2 server should follow RFC 9113, and pass h2spec compliance (barring reasonable disagreement over any ambiguity). In particular, it should pass the 8.1.2.3 specs, which specify that if required pseudo-headers are missing, it should be a failure.

Relevant RFC sections

From RFC 9113 § 8.1.1:

A malformed request or response is one that is an otherwise valid sequence of HTTP/2 frames but is invalid due to the presence of extraneous frames, prohibited fields or pseudo-header fields, the absence of mandatory pseudo-header fields, the inclusion of uppercase field names, or invalid field names and/or values

From RFC 9113 § 8.3.1-8.3.2:

All HTTP/2 requests MUST include exactly one valid value for the ":method", ":scheme", and ":path" pseudo-header fields, unless they are CONNECT requests (Section 8.5). An HTTP request that omits mandatory pseudo-header fields is malformed

and

For HTTP/2 responses, a single ":status" pseudo-header field is defined that carries the HTTP status code field (see Section 15 of [HTTP]). This pseudo-header field MUST be included in all responses, including interim responses; otherwise, the response is malformed

Actual behavior

Netty fails to check for the presence of mandatory pseudo-headers.

NB: Netty correctly checks the pseudo-headers' values if present, but not the presence/absence of the pseudo-headers themselves.

Steps to reproduce

  1. cd testsuite-http2
  2. mvn test

h2spec will run, and list, the failures in the console output, but because they're excluded in the pom.xml, the test "succeeds".

h2spec console output:

  8. HTTP Message Exchanges
    8.1. HTTP Request/Response Exchange
      8.1.2. HTTP Header Fields
        8.1.2.3. Request Pseudo-Header Fields
          × 1: Sends a HEADERS frame with empty ":path" pseudo-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:24, flags:0x01, stream_id:1)
          × 2: Sends a HEADERS frame that omits ":method" pseudo-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:24, flags:0x01, stream_id:1)
          × 3: Sends a HEADERS frame that omits ":scheme" pseudo-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:24, flags:0x01, stream_id:1)
          × 4: Sends a HEADERS frame that omits ":path" pseudo-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:24, flags:0x01, stream_id:1)

(Theoretically, RFC 9113 § 8.1.1 means HEADERS/DATA frames can be sent after getting a malformed frame. However, that's only allowed prior to sending a GOAWAY or RST_STREAM: "For malformed requests, a server MAY send an HTTP response prior to closing or resetting the stream." h2spec reports the final frame it received before it timed out or the conn closed, which means no RST_STREAM or GOAWAY was ever sent.)

  1. Delete (or comment out) the 8.1.2.3 exclusions in pom.xml, like so:
<!--            <excludeSpec>8.1.2.3 - Sends a HEADERS frame with empty ":path" pseudo-header field</excludeSpec>-->
<!--            <excludeSpec>8.1.2.3 - Sends a HEADERS frame that omits ":method" pseudo-header field</excludeSpec>-->
<!--            <excludeSpec>8.1.2.3 - Sends a HEADERS frame that omits ":scheme" pseudo-header field</excludeSpec>-->
<!--            <excludeSpec>8.1.2.3 - Sends a HEADERS frame that omits ":path" pseudo-header field</excludeSpec>-->
<!--            <excludeSpec>8.1.2.3 - Sends a HEADERS frame with duplicated ":method" pseudo-header field</excludeSpec>-->
<!--            <excludeSpec>8.1.2.3 - Sends a HEADERS frame with duplicated ":method" pseudo-header field</excludeSpec>-->
<!--            <excludeSpec>8.1.2.3 - Sends a HEADERS frame with duplicated ":scheme" pseudo-header field</excludeSpec>-->
  1. Rerun mvn test

Now we see mvn test failing, but we also see a second subproblem: the h2spec-maven-plugin doesn't report what h2spec itself does:

[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] [id: 0xc787b448, L:/[0:0:0:0:0:0:0:0]:52717] INACTIVE
[INFO] [id: 0xc787b448, L:/[0:0:0:0:0:0:0:0]:52717] UNREGISTERED
[INFO] Total time:  18.449 s
[INFO] Finished at: 2023-09-21T14:48:43+07:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal com.github.madgnome:h2spec-maven-plugin:0.6:h2spec (default) on project netty-testsuite-http2:
[ERROR] Failed test cases:
[ERROR]     [8.1.2.3 - Sends a HEADERS frame that omits ":method" pseudo-header field] failed
[ERROR]  Expected: GOAWAY Frame (Error Code: PROTOCOL_ERROR)
[ERROR]  Actual: RST_STREAM Frame (Error Code: PROTOCOL_ERROR)
[ERROR]
[ERROR]     [8.1.2.3 - Sends a HEADERS frame that omits ":scheme" pseudo-header field] failed
[ERROR]  Expected: GOAWAY Frame (Error Code: PROTOCOL_ERROR)
[ERROR]  Actual: RST_STREAM Frame (Error Code: PROTOCOL_ERROR)
[ERROR]
[ERROR]     [8.1.2.3 - Sends a HEADERS frame that omits ":path" pseudo-header field] failed
[ERROR]  Expected: GOAWAY Frame (Error Code: PROTOCOL_ERROR)
[ERROR]  Actual: RST_STREAM Frame (Error Code: PROTOCOL_ERROR)

The h2spec-maven-plugin claims the final frame was a RST_STREAM, while h2spec itself earlier claimed it was a DATA frame. I think this is a separate bug, which masks the underlying issue by making it look like netty is actually detecting the error and just disagreeing over what error frame to send, but that's not correct. Netty's version of h2spec-maven-plugin is a bit old. If we switch to a newer fork, the org.mortbay.jetty h2spec-maven-plugin, version 1.0.10, the h2spec-maven-plugin and h2spec agree on what the final frame is: a DATA frame.

Manual check

I also double-checked this behavior by hand on the example multiplex "Hello World" server, and ran h2spec against it in verbose mode, which lists the frames received. h2spec never gets a single RST_STREAM or GOAWAY frame from the server, just a sequence of normal frames:

      8.1.2. HTTP Header Fields
        8.1.2.3. Request Pseudo-Header Fields
               [send] SETTINGS Frame (length:6, flags:0x00, stream_id:0)
               [recv] SETTINGS Frame (length:6, flags:0x00, stream_id:0)
               [send] SETTINGS Frame (length:0, flags:0x01, stream_id:0)
               [recv] SETTINGS Frame (length:0, flags:0x01, stream_id:0)
               [send] HEADERS Frame (length:14, flags:0x05, stream_id:1)
               [recv] HEADERS Frame (length:1, flags:0x04, stream_id:1)
               [recv] DATA Frame (length:24, flags:0x01, stream_id:1)
               [recv] Timeout

Minimal yet complete reproducer code (or URL to code)

n/a

Netty version

4.1.98.Final and main branch SHA 391e019cd4fee699d8812d6837689601cb3daecd

JVM version (e.g. java -version)

openjdk version "17.0.8" 2023-07-18 OpenJDK Runtime Environment Temurin-17.0.8+7 (build 17.0.8+7) OpenJDK 64-Bit Server VM Temurin-17.0.8+7 (build 17.0.8+7, mixed mode, sharing)

OS version (e.g. uname -a)

Darwin Lappy.local 22.6.0 Darwin Kernel Version 22.6.0: Wed Jul 5 22:21:56 PDT 2023; root:xnu-8796.141.3~6/RELEASE_X86_64 x86_64

KingMob commented 1 year ago

Update: based on a conversation on an h2spec issue, there's some ambiguity on whether just sending an HTTP response and closing the stream is also acceptable.

I've asked the RFC 9113 authors for clarification. Even if that's true, and h2spec has a bug, I didn't see that as the rationale in why the tests are excluded. If so, that decision should be documented.

Even if a response+end-of-stream turns out to be acceptable for stream errors, I would still suggest Netty handle it, to prevent putting the burden of missing pseudo-headers on implementers.

Regardless, it doesn't fix the subproblem of the out-of-date h2spec plugin reporting differently from h2spec.

KingMob commented 1 year ago

The HTTP working group weighed in at https://lists.w3.org/Archives/Public/ietf-http-wg/2023JulSep/0183.html

They think a response+close is a defensible interpretation, if not ideal, and instead recommend a 4xx response+RST_STREAM.