Subject: HTTP/2 client behaviour on receiving illegal PUSH_PROMISE frames

From: Alan Egerton <[email-masked]>
Date: Tue, 22 Sep 2020 18:26:51 +0100

I'm a little unclear exactly how clients should behave upon receiving
PUSH_PROMISE frames in certain illegal states.  In particular,
sections 5.1 and 6.6 of RFC 7540 appear to state contradictory
requirements:

+------------------------------+-----------------+-----------------+
| Client state                 |   Section 5.1   |   Section 6.6   |
+------------------------------+-----------------+-----------------+
| half-closed(remote)          |       S:SC      |       C:PE      |
|                              |                 |                 |
| closed (RST_STREAM received) |       S:SC      |       C:PE      |
| closed (END_STREAM received) |       C:SC      |       C:PE      |
| closed (RST_STREAM sent)     |    Ignore/P:?   |   Handle/C:PE   |
+------------------------------+-----------------+-----------------+
KEY
S: = Stream error
C: = Connection error
P: = Reset promised stream (error type not stated)
:SC = STREAM_CLOSED
:PE = PROTOCOL_ERROR

So, which is correct?  And where RST_STREAM has been sent, should the
PUSH_PROMISE be ignored or handled (beyond sending RST_STREAM for the
promised stream, in which case what should be the error type)?

The relevant extracts from Section 5.1 are:

In "half-closed(remote)":

> 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.

In "closed":

> An endpoint that receives any frame other than PRIORITY after
> receiving a RST_STREAM MUST treat that as a stream error (Section
> 5.4.2) of type STREAM_CLOSED. Similarly, an endpoint that receives any
> frames after receiving a frame with the END_STREAM flag set MUST treat
> that as a connection error (Section 5.4.1) of type STREAM_CLOSED,
> unless the frame is permitted as described below.

and

> An endpoint MUST ignore frames that it receives on closed streams
> after it has sent a RST_STREAM frame.
>
> [deletia]
>
> An endpoint might receive a PUSH_PROMISE frame after it sends
> RST_STREAM. PUSH_PROMISE causes a stream to become "reserved" even if
> the associated stream has been reset. Therefore, a RST_STREAM is
> needed to close an unwanted promised stream.

And from Section 6.6 PUSH_PROMISE:

> A receiver MUST treat the receipt of a PUSH_PROMISE on a stream that
> is neither "open" nor "half-closed (local)" as a connection error
> (Section 5.4.1) of type PROTOCOL_ERROR. However, an endpoint that has
> sent RST_STREAM on the associated stream MUST handle PUSH_PROMISE
> frames that might have been created before the RST_STREAM frame is
> received and processed.

-- Alan

---

From: Lucas Pardue <[email-masked]>
Date: Tue, 22 Sep 2020 19:17:35 +0100

The confusion is possibly due to the reports in errata 4535 [1] and
duplicate 4645 [2],

[1] - https://www.rfc-editor.org/errata/eid4535
[2] - https://www.rfc-editor.org/errata/eid4645

---

From: Alan Egerton <[email-masked]>
Date: Tue, 22 Sep 2020 20:38:02 +0100

Thanks Lucas, but I don't think either of those reports are quite the same:
they both appear to concern the state transition from "idle" state on
sending a PUSH_PROMISE (and that the spec can be misread as describing
transitions from that state on receiving such frames); whereas I am
concerned with the correct error handling on receiving an erroneous
PUSH_PROMISE in the "half-closed(remote)" or "closed" states.

-- Alan

---

From: Lucas Pardue <[email-masked]>
Date: Tue, 22 Sep 2020 21:09:05 +0100

On Tue, Sep 22, 2020 at 8:38 PM Alan Egerton <[email-masked]> wrote:

> Thanks Lucas, but I don't think either of those reports are quite the
> same: they both appear to concern the state transition from "idle" state on
oops I meant to say "possibly related" because this is about the handling
of push promises with respect to stream lifecycle. My 2c:

I might be squinting at the state machine wrong but I don't think it is
practically possible for the client to have a request stream in a
half-closed (remote) and receive a PUSH_PROMISE. Because the only way to
get a stream in that state is for a server to respond to a request with
END_STREAM set, before the client has sent END_STREAM or RST_STREAM. This
is an early response, which is allowed. But the server shouldn't be trying
to promise things after it closed the stream, that's a plain error.
Similarly, a server sending PUSH_PROMISE after RST_STREAM is also an error.

The odd case is when a client and server have a race about the stream being
closed due to the client sending RST_STREAM in the open state. "Closed
because I said so" is a bit different to "Closed because you said so". The
statement in 6.6 about "MUST handle PUSH_PROMISES" is trying to wiggle out
of the race condition.

Cheers
Lucas

---

From: Alan Egerton <[email-masked]>
Date: Tue, 22 Sep 2020 21:42:17 +0100

Hi Lucas,

The situations I've described are absolutely illegal PUSH_PROMISE
frames—but sections 5.1 and 6.6 appear to disagree over what error handling
is appropriate in those situations.

-- Alan

On Tue, Sep 22, 2020 at 9:09 PM Lucas Pardue <[email-masked]>
wrote:

> The odd case is when a client and server have a race about the stream
> being closed due to the client sending RST_STREAM in the open state.
> "Closed because I said so" is a bit different to "Closed because you said
> so". The statement in 6.6 about "MUST handle PUSH_PROMISES" is trying to
> wiggle out of the race condition.
>
> Cheers
> Lucas
>

---

From: Lucas Pardue <[email-masked]>
Date: Tue, 22 Sep 2020 22:03:17 +0100

My read is that 6.6 is a specialisation of the error conditions described
in 5.1, therefore it might help to mention that in 5.1. Others might
disagree.

As for RST_STREAM of the unwanted push, I'd say this is just as described
in 8.2.2; either CANCEL or REFUSED_STREAM.

---

From: Mike Bishop <[email-masked]>
Date: Tue, 22 Sep 2020 22:10:15 +0000

Section 6.6 says that you C:P-E (using your notation) for receipt of PUSH_PROMISE when they can’t have legally been sent; it has an exception to account for a race condition, and says that you “MUST handle” them.  So it doesn’t mandate anything in particular beyond handling as normal in that race condition.  I’d echo most of Lucas’s points; it doesn’t need to specify the error code for RST_STREAM, because you’re “handling” it normally.  If you don’t want it, use the error code you would routinely use for unwanted pushes.

However, you’re correct that receiving a PUSH_PROMISE in “half-closed (remote)” isn’t one of these race conditions; that means that the exception doesn’t apply in that case and the requirement to C:P-E applies in that scenario. 

It’s a bit odd that we require both a stream error and a connection error triggered by the same frame, but the ordering of those doesn’t matter; the connection is closed at the end either way.  I suppose my lawyerly answer is that a requirement to generate a stream error doesn’t preclude also treating it as a connection error, so this is merely a bit awkward and not outright wrong.

From: Lucas Pardue <[email-masked]> 
Sent: Tuesday, September 22, 2020 5:03 PM
To: Alan Egerton <[email-masked]>
Cc: HTTP Working Group <[email-masked]>
Subject: Re: HTTP/2 client behaviour on receiving illegal PUSH_PROMISE frames

My read is that 6.6 is a specialisation of the error conditions described in 5.1, therefore it might help to mention that in 5.1. Others might disagree.

As for RST_STREAM of the unwanted push, I'd say this is just as described in 8.2.2; either CANCEL or REFUSED_STREAM.

On Tue, 22 Sep 2020, 21:42 Alan Egerton, <[email-masked] <mailto:[email-masked]> > wrote:

Hi Lucas,

The situations I've described are absolutely illegal PUSH_PROMISE frames—but sections 5.1 and 6.6 appear to disagree over what error handling is appropriate in those situations.

-- Alan

On Tue, Sep 22, 2020 at 9:09 PM Lucas Pardue <[email-masked] <mailto:[email-masked]> > wrote:

On Tue, Sep 22, 2020 at 8:38 PM Alan Egerton <[email-masked] <mailto:[email-masked]> > wrote:

Thanks Lucas, but I don't think either of those reports are quite the same: they both appear to concern the state transition from "idle" state on sending a PUSH_PROMISE (and that the spec can be misread as describing transitions from that state on receiving such frames); whereas I am concerned with the correct error handling on receiving an erroneous PUSH_PROMISE in the "half-closed(remote)" or "closed" states.

-- Alan

oops I meant to say "possibly related" because this is about the handling of push promises with respect to stream lifecycle. My 2c:

I might be squinting at the state machine wrong but I don't think it is practically possible for the client to have a request stream in a half-closed (remote) and receive a PUSH_PROMISE. Because the only way to get a stream in that state is for a server to respond to a request with END_STREAM set, before the client has sent END_STREAM or RST_STREAM. This is an early response, which is allowed. But the server shouldn't be trying to promise things after it closed the stream, that's a plain error. Similarly, a server sending PUSH_PROMISE after RST_STREAM is also an error.

The odd case is when a client and server have a race about the stream being closed due to the client sending RST_STREAM in the open state. "Closed because I said so" is a bit different to "Closed because you said so". The statement in 6.6 about "MUST handle PUSH_PROMISES" is trying to wiggle out of the race condition. 

Cheers

Lucas

---

From: Alan Egerton <[email-masked]>
Date: Wed, 23 Sep 2020 02:03:57 +0100

Ok, thanks for the clarification.  I hadn't considered that the spec might
be requiring multiple errors be raised/handled.

-- Alan

On Tue, Sep 22, 2020 at 11:10 PM Mike Bishop <[email-masked]> wrote:

> Section 6.6 says that you C:P-E (using your notation) for receipt of
> PUSH_PROMISE when they can’t have legally been sent; it has an exception to
> account for a race condition, and says that you “MUST handle” them.  So it
> doesn’t mandate anything in particular beyond handling as normal in that
> race condition.  I’d echo most of Lucas’s points; it doesn’t need to
> specify the error code for RST_STREAM, because you’re “handling” it
> normally.  If you don’t want it, use the error code you would routinely use
> for unwanted pushes.
>
>
>
> However, you’re correct that receiving a PUSH_PROMISE in “half-closed
> (remote)” isn’t one of these race conditions; that means that the exception
> doesn’t apply in that case and the requirement to C:P-E applies in that
> scenario.
>
>
>
> It’s a bit odd that we require *both* a stream error *and *a connection
> error triggered by the same frame, but the ordering of those doesn’t
> matter; the connection is closed at the end either way.  I suppose my
> lawyerly answer is that a requirement to generate a stream error doesn’t
> preclude also treating it as a connection error, so this is merely a bit
> awkward and not outright wrong.
>
>
>
> *From:* Lucas Pardue <[email-masked]>
> *Sent:* Tuesday, September 22, 2020 5:03 PM
> *To:* Alan Egerton <[email-masked]>
> *Cc:* HTTP Working Group <[email-masked]>
> *Subject:* Re: HTTP/2 client behaviour on receiving illegal PUSH_PROMISE
> frames
>
>
>