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

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

On 21 January 2015 at 19:55, Pete Resnick <presnick@qti.qualcomm.com> wrote:
> 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.

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.

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

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.

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

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


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

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

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

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.

> 5.4.3: I'm not clear on how to implement a "MUST assume". What is it that
> 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.

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

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.

Received on Monday, 26 January 2015 23:00:38 UTC