HttpConnectionHandler should send a GO_AWAY frame with PROTOCOL_ERROR… #12985

vietj · 2022-11-10T23:12:46Z

… when it receives an unknown stream instead of resetting the stream.

Motivation:

When an HTTP/2 connection process an unnkown stream it should throw a GO_AWAY frame and close the connection as per RFC 7540. The current implementation instead sends a RST frame.

Modifications:

Change the HttpConnectionHandler behavior to throw an error instead of writing a rst frame, the error will trigger a GO_AWAY frame to be sent and then close the connection appropriately.

Result:

HttpConnectionHandler should send a GO_AWAY frame with PROTOCOL_ERROR when it receives an unknown stream and fix #12974

… when it receives an unknown stream instead of resetting the stream. Motivation: When an HTTP/2 connection process an unnkown stream it should throw a GO_AWAY frame and close the connection as per RFC 7540. The current implementation instead sends a RST frame. Modifications: Change the HttpConnectionHandler behavior to throw an error instead of writing a rst frame, the error will trigger a GO_AWAY frame to be sent and then close the connection appropriately. Result: HttpConnectionHandler should send a GO_AWAY frame with PROTOCOL_ERROR when it receives an unknown stream and fix netty#12974

normanmaurer · 2022-11-11T10:06:23Z

@vietj can you link the relevant section in the RFC ?

Is this your interpretation of https://datatracker.ietf.org/doc/html/rfc7540#section-5.1:

...
In the absence of more specific guidance elsewhere in this document,
   implementations SHOULD treat the receipt of a frame that is not
   expressly permitted in the description of a state as a connection
   error ([Section 5.4.1](https://datatracker.ietf.org/doc/html/rfc7540#section-5.4.1)) of type PROTOCOL_ERROR.  Note that PRIORITY can
   be sent and received in any stream state.  Frames of unknown types
   are ignored.

vietj · 2022-11-11T11:58:03Z

The part I'm referring to in https://datatracker.ietf.org/doc/html/rfc7540#section-5.1.1 is:

An endpoint that receives an unexpected stream identifier MUST respond with a connection error ([Section 5.4.1](https://datatracker.ietf.org/doc/html/rfc7540#section-5.4.1)) of type PROTOCOL_ERROR.

normanmaurer · 2022-11-11T12:08:50Z

@vietj got it... Can you add this as a comment to the code ?

codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java

…nnectionHandler.java

normanmaurer · 2022-11-11T15:48:53Z

@vietj I added the comment

normanmaurer · 2022-11-11T15:49:05Z

/cc @ejona86

vietj · 2022-11-11T17:31:45Z

@vietj I added the comment

thanks

hyperxpro

Thanks for the fix. :)

idelpivnitskiy

Thanks @vietj!

normanmaurer · 2022-11-15T07:36:30Z

@vietj can you check the test failure. This seems to be related:

Http2ConnectionRoundtripTest.headersUsingHigherValuedStreamIdPreventsUsingLowerStreamId

ejona86 · 2022-11-15T16:29:18Z

codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionRoundtripTest.java

@@ -146,7 +146,7 @@ public void teardown() throws Exception {
    }

    @Test
-    public void inflightFrameAfterStreamResetShouldNotMakeConnectionUnusable() throws Exception {


Uhh.... this is a really important case. It seems like this change is overzealous and confusing what the spec is saying.

From RFC 7540§5.1.1 (Note that the language is the same in RFC 9113§5.1.1):

The identifier of a newly established stream MUST be numerically
greater than all streams that the initiating endpoint has opened or
reserved. This governs streams that are opened using a HEADERS frame
and streams that are reserved using PUSH_PROMISE. An endpoint that
receives an unexpected stream identifier MUST respond with a
connection error (Section 5.4.1) of type PROTOCOL_ERROR.

That is talking about newly-established streams. The error checking above is not limiting itself to HEADERS and PUSH_PROMISES.

Also, Netty forgets about streams after it sends RST_STREAM, so there is a problem of accidentally considering trailers as creating a "new stream" even though they were racing.

So I'm arguing that this change is ignoring the RFC, specifically RFC 7540§5.4.2 (same language in RFC 9113):

A RST_STREAM is the last frame that an endpoint can send on a stream.
The peer that sends the RST_STREAM frame MUST be prepared to receive
any frames that were sent or enqueued for sending by the remote peer.
These frames can be ignored, except where they modify connection
state (such as the state maintained for header compression
(Section 4.3) or flow control).

Basically, what I'm saying is that with the current "memory" of Netty, it is difficult to do both things at once, and we are currently handling it the better way. Handling the RST_STREAM race is something that happens in practice with conforming implementations. Handling the "creating new stream with lower ID" only happens with a broken remote implementation. To be fully compliant would increase memory usage, and that is an option, but is complicated and helps almost nobody.

We could get partially compliant by just checking this case for PUSH_PROMISE. HEADERS are hard because of HTTP 1xx responses and trailers. I don't see a way to reliably detect if HEADERS are for a now-closed stream or are creating a new stream, without remembering detailed closed stream states. But in any case, any checks along those lines should be in DefaultHttp2ConnectionHandler, not in onStreamError(), because onStreamError() clearly needs to handle cases like racy DATA frames received after a RST_STREAM.

@vietj please check the comments of @ejona86

I'll have a look today @normanmaurer

if I understand correctly @ejona86 you are saying that when a stream id is received and not actually known by the state machine (because it does not retain such information and because the client is allowed to increase the stream id sequence as its will), we don't know whether that stream actually ever existed or it existed but is now closed

Any update /cc @ejona86

ping @ejona86 :-)

you are saying that when a stream id is received and not actually known by the state machine (because it does not retain such information and because the client is allowed to increase the stream id sequence as its will), we don't know whether that stream actually ever existed or it existed but is now closed

Correct. And the code assumes it is the more likely case today, that the stream did exist.

rsvoboda · 2022-12-09T14:53:48Z

Hi @vietj. Seems this PR fell through the cracks.

vietj · 2022-12-15T15:22:00Z

Hi @vietj. Seems this PR fell through the cracks.

it is not clear whether it can be fixed in Netty, I asked a clarification to @ejona86

vietj force-pushed the issue-12974 branch from 6a68d22 to cd00916 Compare November 11, 2022 09:32

normanmaurer requested a review from idelpivnitskiy November 11, 2022 10:01

normanmaurer reviewed Nov 11, 2022

View reviewed changes

codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java Show resolved Hide resolved

normanmaurer approved these changes Nov 11, 2022

View reviewed changes

Update codec-http2/src/main/java/io/netty/handler/codec/http2/Http2Co…

6189504

…nnectionHandler.java

normanmaurer added this to the 4.1.86.Final milestone Nov 11, 2022

hyperxpro approved these changes Nov 12, 2022

View reviewed changes

vietj mentioned this pull request Nov 13, 2022

6 failed tests in h2spec (conformance testing tool for HTTP/2) run quarkusio/quarkus#28997

Open

idelpivnitskiy approved these changes Nov 14, 2022

View reviewed changes

ejona86 requested changes Nov 15, 2022

View reviewed changes

normanmaurer modified the milestones: 4.1.86.Final, 4.1.87.Final Dec 12, 2022

normanmaurer modified the milestones: 4.1.87.Final, 4.1.88.Final Jan 12, 2023

normanmaurer modified the milestones: 4.1.88.Final, 4.1.89.Final, 4.1.90.Final Feb 13, 2023

normanmaurer modified the milestones: 4.1.90.Final, 4.1.91.Final Mar 14, 2023

normanmaurer modified the milestones: 4.1.94.Final, 4.1.95.Final Jun 19, 2023

normanmaurer modified the milestones: 4.1.95.Final, 4.1.96.Final, 4.1.97.Final Jul 20, 2023

normanmaurer modified the milestones: 4.1.97.Final, 4.1.98.Final Aug 23, 2023

normanmaurer modified the milestones: 4.1.98.Final, 4.1.99.Final Sep 21, 2023

normanmaurer modified the milestones: 4.1.99.Final, 4.1.100.Final Sep 28, 2023

normanmaurer modified the milestones: 4.1.100.Final, 4.1.101.Final Oct 10, 2023

normanmaurer modified the milestones: 4.1.101.Final, 4.1.102.Final Nov 9, 2023

normanmaurer modified the milestones: 4.1.102.Final, 4.1.104.Final, 4.1.105.Final Dec 13, 2023

normanmaurer modified the milestones: 4.1.105.Final, 4.1.106.Final, 4.1.107.Final Jan 16, 2024

normanmaurer modified the milestones: 4.1.107.Final, 4.1.108.Final Feb 12, 2024

normanmaurer modified the milestones: 4.1.108.Final, 4.1.109.Final Mar 21, 2024

normanmaurer modified the milestones: 4.1.109.Final, 4.1.110.Final Apr 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HttpConnectionHandler should send a GO_AWAY frame with PROTOCOL_ERROR… #12985

HttpConnectionHandler should send a GO_AWAY frame with PROTOCOL_ERROR… #12985

vietj commented Nov 10, 2022

normanmaurer commented Nov 11, 2022

vietj commented Nov 11, 2022

normanmaurer commented Nov 11, 2022

normanmaurer commented Nov 11, 2022

normanmaurer commented Nov 11, 2022

vietj commented Nov 11, 2022

hyperxpro left a comment

idelpivnitskiy left a comment

normanmaurer commented Nov 15, 2022

ejona86 Nov 15, 2022

ejona86 Nov 15, 2022 •

edited

normanmaurer Nov 16, 2022

vietj Nov 17, 2022

vietj Nov 18, 2022 •

edited

normanmaurer Dec 13, 2022

vietj Feb 6, 2023

ejona86 Feb 6, 2023

rsvoboda commented Dec 9, 2022

vietj commented Dec 15, 2022

HttpConnectionHandler should send a GO_AWAY frame with PROTOCOL_ERROR… #12985

Are you sure you want to change the base?

HttpConnectionHandler should send a GO_AWAY frame with PROTOCOL_ERROR… #12985

Conversation

vietj commented Nov 10, 2022

normanmaurer commented Nov 11, 2022

vietj commented Nov 11, 2022

normanmaurer commented Nov 11, 2022

normanmaurer commented Nov 11, 2022

normanmaurer commented Nov 11, 2022

vietj commented Nov 11, 2022

hyperxpro left a comment

Choose a reason for hiding this comment

idelpivnitskiy left a comment

Choose a reason for hiding this comment

normanmaurer commented Nov 15, 2022

ejona86 Nov 15, 2022

Choose a reason for hiding this comment

ejona86 Nov 15, 2022 • edited

Choose a reason for hiding this comment

normanmaurer Nov 16, 2022

Choose a reason for hiding this comment

vietj Nov 17, 2022

Choose a reason for hiding this comment

vietj Nov 18, 2022 • edited

Choose a reason for hiding this comment

normanmaurer Dec 13, 2022

Choose a reason for hiding this comment

vietj Feb 6, 2023

Choose a reason for hiding this comment

ejona86 Feb 6, 2023

Choose a reason for hiding this comment

rsvoboda commented Dec 9, 2022

vietj commented Dec 15, 2022

ejona86 Nov 15, 2022 •

edited

vietj Nov 18, 2022 •

edited