Subject: Re: HTTP/2 GREASE, Results, and Implications

From: Bence Béky <[email-masked]>
Date: Fri, 1 Nov 2019 07:43:51 -0400

Hi Willy,

Yes, I agree with you.  My current plan is to make Chrome send a frame of
reserved type
* on stream 0, after the SETTINGS frame;
* on non-zero streams, after each HEADERS and DATA frame that does not have
the END_STREAM flag set;
* or alternatively, after each HEADERS and DATA frame, and if the stream
should be closed, then send an empty DATA frame with END_STREAM after the
reserved frame.

I think this conforms to the state transition requirements you summarized.
Thank you for the guidance.  I'll keep the list posted.

Cheers,

Bence

On Thu, Oct 31, 2019 at 4:35 PM Willy Tarreau <[email-masked]> wrote:

> On Thu, Oct 31, 2019 at 01:08:53PM -0400, Bence Béky wrote:
> > Thanks, Willy, for pointing out the different sections of RFC7540
> > concerning unknown frame types.  It seems to me that for the past few
> days
> > Chrome (on certain channels) has been sending frames on half-closed
> > (remote) streams.
>
> Bence, I saw that you updated the issue regarding this point. If
> you're going to be stricter on the states to respect the (confusing)
> wording of the spec, then you need to be very careful about other states
> for completeness:
>
>   - closed: An endpoint MUST NOT send frames other than PRIORITY on a
> closed
>     stream.
>   - reserved (remote): An endpoint MUST NOT send any type of frame other
>     than RST_STREAM, WINDOW_UPDATE, or PRIORITY in this state.
>   - idle: Receiving any frame other than HEADERS or PRIORITY on a stream in
>     this state MUST be treated as a connection error.
>
> I think it's reasonable to assume that only these transitions (in addition
> to the already mentioned half-closed) face such restrictions. This means
> that any new non-negociated frame type will still be allowed for the
> connection (stream 0) and open streams. This sounds quite reasonable and
> should not risk to trigger the consistency issues in the spec's wording.
>
> And at first glance if that's the case I don't need to patch my code,
> so we can expect that other implementations faced similar issues and
> that the limitations above remain acceptable (but an errata will have
> to be filed to keep track of this).
>
> Hoping this helps,
> Willy
>

---

From: Willy Tarreau <[email-masked]>
Date: Fri, 1 Nov 2019 14:19:03 +0100

> I think this conforms to the state transition requirements you summarized.

Yes that sounds good.

> Thank you for the guidance.  I'll keep the list posted.

Thanks!
Willy

---

From: Stefan Eissing <[email-masked]>
Date: Mon, 4 Nov 2019 09:56:13 +0100

Thanks for mitigating this. 

> Am 01.11.2019 um 14:19 schrieb Willy Tarreau <[email-masked]>:
>

---

From: Bence Béky <[email-masked]>
Date: Wed, 6 Nov 2019 15:03:18 -0500

Hi,

I added two simple command line flags to Chrome to trigger sending greased
HTTP/2 elements, and they are available since version 80.0.3960.0 (not on
Linux quite yet).

--http2-grease-settings adds one settings parameter with a reserved
identifier and random value to every SETTINGS frame sent (one per HTTP/2
connection).

--http2-grease-frame-type sends one frame of reserved type and random short
payload after every SETTINGS and HEADERS frame, including HEADERS frames
with END_STREAM flag set.  This is what caused problems apparently with
Cloudflare and Akamai.

Cheers,

Bence

---

From: Willy Tarreau <[email-masked]>
Date: Wed, 6 Nov 2019 21:24:11 +0100

Hi Bence,

> HTTP/2 elements, and they are available since version 80.0.3960.0 (not on

OK, thanks for letting us know.

> with END_STREAM flag set.  This is what caused problems apparently with

And haproxy, given that this violates the rule of no frame type other than
the few allowed one in half-closed remote state. The problem with allowing
new frame types during forbidden states is that we could make it way
harder to implement these new frame types later without risking to see
them behave differently depending on implementations. I suspect that if
we want to go down that route we'll need to either reserve certain frame
types as carrying no payload for streams and others as possibly having
some :-/ In addition, frame flags are defined per-frame, eventhough we
all know they are the same. But new frames exhibiting an END_HEADERS
flag or an END_STREAM flag cannot be completely ignored. Frames
designating an idle stream ID which are not HEADERS frame will also
possibly create a stream or not and cause issues. I think that at the
moment the only way we have to continue to ignore frames is :

  - ignore the unknown ones on stream 0
  - ignore the unknown ones on non-idle, non-half-closed(remote) streams
  - respect existing rules for existing states (i.e. reject what doesn't
    match certain types for idle, half-closed, reserved.
  - and likely define how to deal with known flags in the remaining
    cases.

Just my two cents.

Cheers,
Willy

---

From: Tom Bergan <[email-masked]>
Date: Fri, 15 Nov 2019 07:10:31 -0800

I would like to revisit the ambiguity that Willy pointed out in this
thread. On the one hand, we have this statement, which appears in Sections
4.1 and 5.5:

1. Implementations MUST discard frames that have unknown or unsupported
types.

On the other hand, we have statements like the following, which appear in
Section 5.1:

2a. idle: Receiving any frame other than HEADERS or PRIORITY on a stream in
this state MUST be treated as a connection error.

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

If this ambiguity is resolved in favor of Section 5.1, then it becomes
difficult or impossible to define a new priority scheme in an HTTP/2
extension. In particular:

- It might be desirable to send a NEW_PRIORITY frame before HEADERS, as a
replacement for priority information that is built into HEADERS. This
conflicts with (2a).

- It might be desirable to send a NEW_PRIORITY frame after END_STREAM, so
the client can reprioritize a lengthy response. This conflicts with (2b).

For this reason, I propose that this ambiguity should be resolved in favor
of Section 5.5. We should release an errata which clarifies that the rules
in Section 5.1 do not apply to frames of unknown type. If an extension
defines a new frame type, it must explicitly enumerate the states in which
that frame can be sent and received. Willy also raised a concern about new
frame types that include END_STREAM. I think this is covered by Section
5.5, which says "Extensions that could change the semantics of existing
protocol components MUST be negotiated before being used." If a new frame
type can end the stream, this adds an edge to the state diagram in Section
5.1, therefore, support for the new frame type must be negotiated before it
can be used.

Thoughts?

---

From: Bence Béky <[email-masked]>
Date: Fri, 15 Nov 2019 10:17:57 -0500

Hi Tom,

Thank you for raising this issue.  I agree with the requirements that you
outline with respect to the NEW_PRIORITY frame.

Just a comment that in case it is not possible to resolve the issue in a
way that's favorable to the priority use case (for example, if the
timeframe for updating all deployments in not acceptable for the desired
timeframe of launching priorities), then another option would be to send
priority-related frames on stream 0, and encode the stream ID they refer to
within the frame payload.  Sure that's a waste of four bytes per frame, but
at least it should work (as long as implementations are correctly
discarding unknown frames on stream 0).

Bence

On Fri, Nov 15, 2019 at 10:10 AM Tom Bergan <[email-masked]> wrote:

> 2a. idle: Receiving any frame other than HEADERS or PRIORITY on a stream
> in this state MUST be treated as a connection error.
>

---

From: Willy Tarreau <[email-masked]>
Date: Fri, 15 Nov 2019 16:59:13 +0100

Hi Tom,

first, thanks for reloading this.

> 1. Implementations MUST discard frames that have unknown or unsupported
(...)
> in Section 5.1 do not apply to frames of unknown type.

If we do this, we should really change the wording to never use "other than"
and instead explicitly mention the list of frame types that are not permitted
in each state.

> If an extension

I'm fine with this point. However I do have a real concern regarding
deployed code now based on the fact that the way this ambiguity is present
can have caused solid roots to be set inside some software. For example
when trying to address the issue in haproxy, I noticed that we've used
bit fields of permitted frame types to more easily match what's allowed
or not. When working around them I figured that changing tests constructed
this way to think in terms of exclusion instead wasn't always as riskfree
as I initially imagined, and if we relax certain rules we need to be sure
that (almost) all implementations will accept to revisit their code and
take such risks in maintenance versions to allow smooth updates of deployed
components, or we really risk the same type of fragmentation that we've
known with HTTP/1.1.

I have been wondering if we could classify frame types into 2 or 3
categories :
  - those permitted on stream 0
  - those permitted on an open stream
  - those permitted after END_STREAM or on an idle stream

I even suspect that the last one should be cut into two. The question
was whether we could have imagined simplifying the handling of extra
frame types by saying that only certain ranges of frame types will be
permitted in certain situations which would allow to better handle
extensions in the future, but after checking in my code I'm not
convinced anymore that this could really help. Thus I think that your
point about any frame type adding a new arc on the state graph should
only be used through negotiation is probably the only option.

But I still do have some concerns about our ability to extend the
protocol even without this. What if a new frame expects an ACK for
example ? Probably this one will also require an extension. And for
those plugged on the stream without the ability to really participate
(typically a firewall or a wireshark dissector), maybe declaring that
the ACK flag is always the same bit 0 and that any frame type holding
an ACK should be ignored regardless of the context could make sense.

Just a few thoughts.

Willy

---

From: Willy Tarreau <[email-masked]>
Date: Fri, 15 Nov 2019 17:02:15 +0100

Hi Bence,

> discarding unknown frames on stream 0).

Actually I think I like this and probably we should have done this as well
for WINDOW_UPDATE (it's always easy to say afterwards). Because such frames
which may reference closed streams are really more related to the connection's
health than to the stream itself in fact and that would make more sense in
the way they are processed.

Cheers,
Willy

---

From: Tom Bergan <[email-masked]>
Date: Fri, 15 Nov 2019 11:35:13 -0800

On Fri, Nov 15, 2019 at 7:59 AM Willy Tarreau <[email-masked]> wrote:

> I do have a real concern regarding

It's important to work through these implementation issues. If
implementations are difficult to change, then we'll probably need to take
Bence's suggestion, or something similar.

That said, I don't fully understand the problem here. Can you elaborate?
Assuming the implementation throws away unknown frames immediately, as
suggested by Section 5.5, only the known frames are left. At that point, it
shouldn't matter whether the bit mask describes permitted frames or
forbidden frames -- both approaches are equivalent. FWIW, the Go H2 library
(client
<https://github.com/golang/net/blob/42ef8dbebecb1aa62a4f25c93e117a1de4d9f4fc/http2/transport.go#L1783>,
server
<https://github.com/golang/net/blob/42ef8dbebecb1aa62a4f25c93e117a1de4d9f4fc/http2/server.go#L1430>)
and Chrome both "throw away unknown frames immediately" (although Chrome
has a small bug; crbug.com/1025311).

> example ? Probably this one will also require an extension.
>

I agree. If a new frame type requires an ACK, that needs to be negotiated.

---

From: Willy Tarreau <[email-masked]>
Date: Fri, 15 Nov 2019 21:38:15 +0100

> Bence's suggestion, or something similar.

Regardless of these issues, Bence's proposal sounds appealing to me for
a better layering.

> forbidden frames -- both approaches are equivalent.

Oh it's very simple to understand: due to the rules restricting the frame
types depending on the stream state, the frames are matched against the
stream state early during the processing. In fact proceeding as you
suggest would make the code simpler, it's just that it's changing the
logic, and while I do see how to adapt mine for this (with limited risks
of regressions when it comes to backport this to deployed maintenance
branches), it's easily imaginable that and equivalent level of complexity
(not high, just moderate) can affect other deployed implementations,
causing some reluctance to changing existing code in field.

I *am* personally willing to make the change if there is consensus on
this. I even hacked a minimal patch for this after Mike's report. I'm
just not assuming that everyone will do so in an eyeblink.

Another option could be to refrain from sending new frame types until
a setting has been advertised by the other end. I just find this a bit
sad. That's exactly what the minor version in the protocol was for. We
could have had HTTP/2.1 advertised over ALPN to instantly match both
ends, but now we have to do without this possibility.

Willy

---

From: Dmitri Tikhonov <[email-masked]>
Date: Fri, 15 Nov 2019 15:49:33 -0500

On Fri, Nov 15, 2019 at 09:38:15PM +0100, Willy Tarreau wrote:
> I just find this a bit sad. That's exactly what the minor version
> in the protocol was for. We could have had HTTP/2.1 advertised over
> ALPN to instantly match both ends, but now we have to do without
> this possibility.

It would be curious to dig up the mail thread (or the GitHub issue)
where this design decision was discussed.

  - Dmitri.

---

From: Stefan Eissing <[email-masked]>
Date: Mon, 18 Nov 2019 10:04:54 +0100

+1

> Am 15.11.2019 um 16:59 schrieb Willy Tarreau <[email-masked]>:
> 
> Thus I think that your