Subject: Pete Resnick's No Objection on draft-ietf-httpbis-http2-16: (with COMMENT)

From: "Pete Resnick" <[email-masked]>
Date: Wed, 21 Jan 2015 19:55:46 -0800

Pete Resnick has entered the following ballot position for
draft-ietf-httpbis-http2-16: No Objection

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)

Please refer to http://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.

The document, along with other ballot positions, can be found here:
http://datatracker.ietf.org/doc/draft-ietf-httpbis-http2/

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

3.2 says:

   A server MUST ignore a "h2" token in an Upgrade header field.
   Presence of a token with "h2" implies HTTP/2 over TLS, which is
   instead negotiated as described in Section 3.3.

And 3.3 says:

   HTTP/2 over TLS uses the "h2" application token.  The "h2c" token
   MUST NOT be sent by a client or selected by a server.

Why isn't the presence of an "h2" Upgrade token on a clear text
connection, or an "h2c" application token on a TLS connection, grounds
for slamming the connection? Seems like something nefarious might be
going on in either case. Seems like "MUST NOT send, MUST be a connection
error if received" seems like the right thing to do.

4.2 says:

   An endpoint MUST send a FRAME_SIZE_ERROR error if a frame exceeds the
   size defined in SETTINGS_MAX_FRAME_SIZE, any limit defined for the
   frame type, or it is too small to contain mandatory frame data.

But later 5.1 says:

      Receiving any frames other than [blah blah blah]
      on a stream in this state MUST be treated as a connection error
      (Section 5.4.1) of type PROTOCOL_ERROR.

The MUSTs in there appear contradictory. If I get a frame with the wrong
type for my current state that is *also* a bogus size, is there a
requirement that I do PROTOCOL_ERROR, or FRAME_SIZE_ERROR, or is the
choice of error code not really a requirement at all? I suspect that the
"MUST be treated as a connection error" is the key and *not* the
particular error code. I would re-word to simply say, "The
FRAME_SIZE_ERROR error is sent when a frame exceeds..." in 4.2. In 5.1
and elsewhere, you could say something like:

      Receiving any frames other than [blah blah blah]
      on a stream in this state MUST be treated as a connection error
      (Section 5.4.1). Error type PROTOCOL_ERROR can be used for this
      condition.

I just think we'll regret when the protocol police come around saying,
"But it said you MUST use such-and-so error code, and he didn't, so it's
fine if my implementation does X", where X is a thoroughly idiotic
thing.

4.3:

Earlier in the section, you say:

   A complete header block consists of either:

   o  a single HEADERS or PUSH_PROMISE frame, with the END_HEADERS flag
      set, or

   o  a HEADERS or PUSH_PROMISE frame with the END_HEADERS flag cleared
      and one or more CONTINUATION frames, where the last CONTINUATION
      frame has the END_HEADERS flag set.

So you've defined that the last frame (or the only frame if there's no
continuation) has the END_HEADERS set. Cool. But then later you make a
point to say:

   The last frame in
   a sequence of HEADERS or CONTINUATION frames MUST have the
   END_HEADERS flag set.  The last frame in a sequence of PUSH_PROMISE
   or CONTINUATION frames MUST have the END_HEADERS flag set.

I don't get the MUSTs. What is the situation that I need to look out for
here? Is there a circumstance where an implementation might think it's OK
to send the last frame without END_HEADERS set? Seems to me those two
sentences can be deleted, but maybe I'm missing something.

Also unnecessarily MUSTy:

OLD
   An
   endpoint receiving HEADERS, PUSH_PROMISE or CONTINUATION frames MUST
   reassemble header blocks and perform decompression even if the frames
   are to be discarded.
NEW
   Hence, an endpoint receiving HEADERS, PUSH_PROMISE or CONTINUATION
   frames needs to reassemble header blocks and perform decompression
   even if the frames are to be discarded.
END

5.1:

   open:
      [...]

      From this state either endpoint can send a frame with an
      END_STREAM flag set, which causes the stream to transition into
      one of the "half closed" states: [...].

      Either endpoint can send a RST_STREAM frame from this state,
      causing it to transition immediately to "closed".

You should probably define the silly state of a RST_STREAM frame with and
END_STREAM flag set on it. I presume that you immediately go to "closed",
but if you implemented it in a goofy way, you may end up in "half
closed".

A clarifying bit:

OLD
   half closed (local):
      A stream that is in the "half closed (local)" state cannot be used
      for sending frames.  Only WINDOW_UPDATE, PRIORITY and RST_STREAM
      frames can be sent in this state.
NEW
   half closed (local):
      A stream that is in the "half closed (local)" state cannot be used
      for sending frames other than WINDOW_UPDATE, PRIORITY and
      RST_STREAM.
END

Also:

      If an endpoint receives additional frames for a stream that is in
      this state, other than WINDOW_UPDATE, PRIORITY or RST_STREAM, it
      MUST respond with a stream error (Section 5.4.2) of type
      STREAM_CLOSED.

I think it would be good to introduce some language somewhere, maybe here
(and in other places throughout section 6 referring to stream errors) or
maybe in 5.4, that says that you MUST respond with a stream error *unless
the frame in question would also constitute a connection error, in which
case you MUST respond with a connection error*.

5.4.3: I'm not clear on how to implement a "MUST assume". What is it that
the implementation MUST do?

8.2.2: I was actually a little surprised to see the
SETTINGS_MAX_CONCURRENT_STREAMS suggestion. I would have thought that
using window size would be much more obvious. Any reason you chose to
discuss one instead of the other?

11.3: No advice or criteria to use for the expert reviewer?

---

From: Amos Jeffries <[email-masked]>
Date: Thu, 22 Jan 2015 22:14:40 +1300

COMMENT:
> connection, or an "h2c" application token on a TLS connection,
> grounds for slamming the connection? Seems like something nefarious
> might be going on in either case. Seems like "MUST NOT send, MUST
> be a connection error if received" seems like the right thing to
> do.
> 
> 4.2 says:
> 
> An endpoint MUST send a FRAME_SIZE_ERROR error if a frame exceeds
> the size defined in SETTINGS_MAX_FRAME_SIZE, any limit defined for
> the frame type, or it is too small to contain mandatory frame
> data.
> 
> But later 5.1 says:
> 
> Receiving any frames other than [blah blah blah] on a stream in
> this state MUST be treated as a connection error (Section 5.4.1) of
> type PROTOCOL_ERROR.
> 
> The MUSTs in there appear contradictory. If I get a frame with the
> wrong type for my current state that is *also* a bogus size, is
> there a requirement that I do PROTOCOL_ERROR, or FRAME_SIZE_ERROR,
> or is the choice of error code not really a requirement at all? I
> suspect that the "MUST be treated as a connection error" is the key
> and *not* the particular error code. I would re-word to simply say,
> "The FRAME_SIZE_ERROR error is sent when a frame exceeds..." in
> 4.2. In 5.1 and elsewhere, you could say something like:
> 
> Receiving any frames other than [blah blah blah] on a stream in
> this state MUST be treated as a connection error (Section 5.4.1).
> Error type PROTOCOL_ERROR can be used for this condition.
> 
> I just think we'll regret when the protocol police come around
> saying, "But it said you MUST use such-and-so error code, and he
> didn't, so it's fine if my implementation does X", where X is a
> thoroughly idiotic thing.

Section 7 definition for PROTOCOL_ERROR states explicitly "This error
is for use when a more specific error code is not available."

FRAME_SIZE_ERROR is more specific than PROTOCOL_ERROR.

> o  a single HEADERS or PUSH_PROMISE frame, with the END_HEADERS
> flag set, or
> 
> o  a HEADERS or PUSH_PROMISE frame with the END_HEADERS flag
> cleared and one or more CONTINUATION frames, where the last
> CONTINUATION frame has the END_HEADERS flag set.
> 
> So you've defined that the last frame (or the only frame if there's
> no continuation) has the END_HEADERS set. Cool. But then later you
> make a point to say:
> 
> The last frame in a sequence of HEADERS or CONTINUATION frames MUST
> have the END_HEADERS flag set.  The last frame in a sequence of
> PUSH_PROMISE or CONTINUATION frames MUST have the END_HEADERS flag
> set.
> 
> I don't get the MUSTs. What is the situation that I need to look
> out for here? Is there a circumstance where an implementation might
> think it's OK to send the last frame without END_HEADERS set? Seems
> to me those two sentences can be deleted, but maybe I'm missing
> something.

You are missing the fact that CONTINUATION blocks the entire
connection. Up to 2^31-2 other client and server streams are hung and
waiting for the frame containing END_HEADERS to be received. It is
well worth repeating MUSTs to prevent accidents in that state.

Potentially that means a worst-case where an entire ISP or CDN network
can be hung waiting on a single remote attacker to complete an
un-ending sequence of CONTINUATION frames. Think "Slow Loris" attack
on steroids.

Now you begin to realise why there is a WG minority faction crying out
to drop CONTINUATION entirely. Sadly we were a minority and overuled
because while this is a possibility it is unlikely to be a problem for
sane implementations that buffer and do not act on the frames in any
way until END_HEADERS has been received.

> OLD An endpoint receiving HEADERS, PUSH_PROMISE or CONTINUATION
> frames MUST reassemble header blocks and perform decompression even
> if the frames are to be discarded. NEW Hence, an endpoint receiving
> HEADERS, PUSH_PROMISE or CONTINUATION frames needs to reassemble
> header blocks and perform decompression even if the frames are to
> be discarded. END
> 

The MUST here are to prevent bad/naive implementations ignoring the
requirement on grounds of non-normative text and corrupting the HPACK
compression state when they opt to drop CONTINUATION frames.

Now you begin to realise that emitting a RST_STREAM to abort a stream
involving CONTINUATION attacks does not actually resolve the DoS
issues. The recipient is forced to wait until all frames of the attack
have been received, or to abandon the entire connection and all its
active streams.

> 5.1:
> 
> open: [...]
> 
> From this state either endpoint can send a frame with an END_STREAM
> flag set, which causes the stream to transition into one of the
> "half closed" states: [...].
> 
> You should probably define the silly state of a RST_STREAM frame
> with and END_STREAM flag set on it. I presume that you immediately
> go to "closed", but if you implemented it in a goofy way, you may
> end up in "half closed".
> 
> A clarifying bit:
> 
> OLD half closed (local): A stream that is in the "half closed
> (local)" state cannot be used for sending frames.  Only
> WINDOW_UPDATE, PRIORITY and RST_STREAM frames can be sent in this
> state. NEW half closed (local): A stream that is in the "half
> closed (local)" state cannot be used for sending frames other than
> WINDOW_UPDATE, PRIORITY and RST_STREAM. END
> 
> Also:
> 
> I think it would be good to introduce some language somewhere,
> maybe here (and in other places throughout section 6 referring to
> stream errors) or maybe in 5.4, that says that you MUST respond
> with a stream error *unless the frame in question would also
> constitute a connection error, in which case you MUST respond with
> a connection error*.
> 
> 5.4.3: I'm not clear on how to implement a "MUST assume". What is
> it that the implementation MUST do?
> 
> SETTINGS_MAX_CONCURRENT_STREAMS suggestion. I would have thought
> that using window size would be much more obvious. Any reason you
> chose to discuss one instead of the other?

Restricting the window space affects DATA frames in response to the
explicit client request message. Also, HEADERS and CONTINUATION frames
are not flow controlled so restricting window alone would not in any
way prevent the server from emitting a large amount of those frames on
pushed messages (neither requests nor responses).

Restricting via SETTINGS_MAX_CONCURRENT_STREAMS prevents the server
from emitting any frames at all for the unwanted (yet) streams.

> 11.3: No advice or criteria to use for the expert reviewer?

Amos Jeffries
Squid Software Foundation

---

From: Martin Thomson <[email-masked]>
Date: Mon, 26 Jan 2015 15:00:11 -0800

For you sir:
https://github.com/http2/http2-spec/pull/708

> error if received" seems like the right thing to do.

That's hard to enforce.  For the same reason that some of the
requirements on TLS use aren't entirely bullet proof in the same way,
that requirement would make it hard to comply in some deployments.

We have to assume that the enforcement point is ignorant of the
semantics of each identifier.  A TLS implementation that is separate
from the HTTP protocol implementation won't necessarily know that
"h2c" isn't valid and so won't be able to slam the connection when it
sees it.  At best, it will pick protocols based on a maybe-ordered
whitelist that it has been provided with.

The same is also potentially true for an upgrade, even though those
are usually less constrained.

> fine if my implementation does X", where X is a thoroughly idiotic

Cunning.

I've had some experience with these nasty error code priority cases.
Your suggestion to avoid mandates of any kind is a devious way of
avoiding being an error-code king-maker.

That said, I have no problem with this.  If this were implemented in a
quantum computer that could simultaneously evaluate all the error
conditions, it might be a problem.  Real computers tend to encounter
errors one at a time, at which point the implementer is free to
blithely ignore others.

In the end, both error conditions end with a connection error.
Someone is being sent in to fix a bug in either case.

>    or CONTINUATION frames MUST have the END_HEADERS flag set.

Yes, "MUST" is easy to overuse, I'll fix that.

> but if you implemented it in a goofy way, you may end up in "half

There is no END_STREAM flag for a RST_STREAM frame.  So that's safe.

> case you MUST respond with a connection error*.

Sending a stream error does not absolve you of the need to send a
connection error.

Note that we also have:
"An endpoint can end a connection at any time. In particular, an
endpoint MAY choose to treat a stream error as a connection error. "

A statement that seems to be getting a lot of work.

> the implementation MUST do?

Right you are:

            If the TCP connection is closed or reset while streams
remain in open or half closed
            states, then the affected streams cannot be automatically
retried (see <xref
            target="Reliability"/> for details).

The assume thing might have been correct if you squinted hard enough;
but devilishly hard to test.

> discuss one instead of the other?

Using flow control only partially addresses the state commitment
problem.  You actually have to use both, but the concurrent streams
limit is more effective.  Either way, you are exposed to the state
cost of holding all the promised *request* header fields.  The
concurrent stream limit reduces exposure to response header fields,
and the flow controlled data that might come with a response.

Firefox actually does use flow control here, in close to the way you
might have imagined.  The downside, which we don't have a lot of data
on right now, is that you need to send WINDOW_UPDATE for all the
streams you actually want.  That can cause stalling if you aren't
careful.  It also wastes bytes all over (a small flow control window
would stall regular requests unless you send WINDOW_UPDATE immediately
after each request - which Firefox does).

> 11.3: No advice or criteria to use for the expert reviewer?

"Don't be evil" ?

Honestly, in this case, I think that
https://tools.ietf.org/html/rfc5226#section-3.3 covers it.