Subject: Minor points in draft 12

From: "Richard Wheeldon (rwheeldo)" <[email-masked]>
Date: Sat, 3 May 2014 06:47:16 +0000

Hi,

I finally got round to reading draft 12 end-to-end. Overall, it's an impressive piece of work. Congrats to all involved. However, I noticed a few minor places where there are potential ambiguities, places where the language could be a bit clearer and bits that confuse me:

"Pad High:  Padding size high bits.  This field is only present if the PAD_HIGH flag is set."
"Pad Low:  Padding size low bits.  This field is only present if the PAD_LOW flag is set."
"E: A single bit flag indicates that the stream dependency is exclusive, see Section 5.3.  This field is optional and is only present if the PRIORITY flag is set."
It's a bit ambiguous just from the text if the fields are there but set to all zero or missing completely from the byte stream? If the bytes were missing it would make no sense for a single bit to be so.

"The client makes an HTTP/1.1 request that includes an Upgrade header field identifying HTTP/2 with the "h2c" token."
What should happen if the client sends an "h2" upgrade? Ignore or error?

"The maximum concurrent streams setting is specific to each endpoint and applies only to the peer that receives the setting."
I'm assuming this is a hop-by-hop setting which an intermediary can change but it's not 100% clear from the text. It makes sense to me to have them independent - e.g. If the browser can support more streams than the server but the proxy can serve more streams by serving content from a cache. Alternatively a proxy could support a higher degree of concurrency to the server by combining requests from multiple clients if those clients are requesting static, PII-free resources from a server (e.g. a CDN).

"Endpoints MUST NOT exceed the limit set by their peer."
Presumably it would be an error if they did? I'm guessing PROTOCOL_ERROR?

"END_SEGMENT (0x2):"
As discussed elsewhere on the list, the term segment is used before it is defined. Since the doc later talks about "TCP segments" this becomes doubly confusing.

"The maximum size of a frame payload varies by frame type."
It would be good if these were laid out explicitly at some point, rather than simply implied by reading the protocol docs. It might encourage folks to write better validation code earlier.

"DATA frames MAY also contain arbitrary padding."
Padding with what? Zeroes or random noise? Before compression or after? The former answers would be simpler, the latter probably more secure.

"SETTINGS_HEADER_TABLE_SIZE ... The initial value is 4,096 bytes."
Shouldn't min and max values be defined for this as well?

"Upon receiving a SETTINGS frame with the ACK flag set, the sender of the altered parameters can rely upon their application."
I'm tempted to think a line mentioning what to do in the interim might be helpful - i.e. to assume the previous values. I can imagine some very subtle and evil bugs introduced if this is not done (e.g. for header compression table sizes).

"R: A single reserved bit." (under PUSH_PROMISE)
"The payload of a WINDOW_UPDATE frame is one reserved bit"
Might be worth adding the usual "always set to zero, always ignore" blurb for completeness.

"A receiver MUST treat the receipt of any other type of frame or a frame on a different stream as a connection error"
Maybe I'm missing something, but why the restriction on a frame on a different stream? I don't believe there's anywhere else in the protocol that such a restriction is made.

"Recipients of PUSH_PROMISE frames can choose to reject promised streams by returning a RST_STREAM referencing the promised stream identifier back to the sender of the PUSH_PROMISE."
With what error code? REFUSED_STREAM?

"The PUSH_PROMISE frame modifies the connection state as defined in Section 4.3. A PUSH_PROMISE frame modifies the connection state in two ways. "
Surely that first sentence is redundant?

"8.1.2. Examples... :method"
I think it would be better if the concept of these pseudo-headers (currently in 8.1.3) was introduced before they were used in examples.

"host=example.org"
I don't understand why this example uses a "host" header when the following one uses ":authority".

"The authority MUST NOT include the deprecated "userinfo" subcomponent for "http" or "https" schemed URIs."
So what do we do with the userinfo? In particular, I'm thinking of cases for HTTP/2 to HTTP/1.1 or FTP-over-HTTP.

"CONNECT ... Frame types other than DATA or stream management frames (RST_STREAM, WINDOW_UPDATE, and PRIORITY) MUST NOT be sent on a connected stream, and MUST be treated as a stream error (Section 5.4.2) if received."
What happens if a client receives a HEADER frame? Does it need to be minimally processed (for maintaining compression state)?

Plus there are a few typos I noticed:

"Until the remote peer receives and processes the frame bearing the END_STREAM flag, it might send frame of any of these types"
frames not frame

"Long-lived connections can result in endpoint exhausting the available range of stream identifiers."
Should be "an endpoint" or "the endpoint"

"GZIP compression ... GZip compression"
Case inconsistency

' "half-closed (local)", or "half closed (remote)" '
To hyphenate or not to hyphenate, that is the question. (this is more irritating than the others in that it makes it harder to search for - in chrome at least)

Regards,

Richard

---

From: Martin Thomson <[email-masked]>
Date: Mon, 19 May 2014 09:38:44 -0700

Hi Richard,

Sorry about taking so long to get around to this.

https://github.com/http2/http2-spec/commit/77c47588c3180f24487a623c8e5b4d90853fe420

> It's a bit ambiguous just from the text if the fields are there but set to all zero or missing completely from the byte stream? If the bytes were missing it would make no sense for a single bit to…

The bytes are omitted entirely.  I'm not sure how to make this any
clearer.  If you have any suggestions, that would help.

> What should happen if the client sends an "h2" upgrade? Ignore or error?

That's really a matter for HTTPbis p1, but I've added text requiring
that servers ignore "h2" in upgrade and "h2c" in ALPN.

Because I think that this needs to be discussed, here's the text I've added:

          A server SHOULD 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 <xref
          target="discover-https"/>.
and
          HTTP/2 over TLS uses the "h2" application token.  The "h2c"
token MUST NOT be used.

I don't think we can totally prohibit the use of "h2" in Upgrade,
though we need to be pretty strong.

> I'm assuming this is a hop-by-hop setting which an intermediary can change but it's not 100% clear from the text. It makes sense to me to have them independent - e.g. If the browser can support mor…

All settings are hop-by-hop because they pertain to a connection.

> Presumably it would be an error if they did? I'm guessing PROTOCOL_ERROR?

I moved the statement a little...

            Endpoints MUST NOT exceed the limit set by their peer.  An
endpoint that receives a
            <x:ref>HEADERS</x:ref> frame that causes their advertised
concurrent stream limit to be
            exceeded MUST treat this as a <xref
target="StreamErrorHandler">stream error</xref>.

Note that stream errors can be turned into connection errors at the
discretion of the implementation.

> As discussed elsewhere on the list, the term segment is used before it is defined. Since the doc later talks about "TCP segments" this becomes doubly confusing.

An open issue that we don't really have a good solution for just yet.
Maybe because I've never been particularly enamoured with the idea of
segments.

> It would be good if these were laid out explicitly at some point, rather than simply implied by reading the protocol docs. It might encourage folks to write better validation code earlier.

It is.  Each frame type that stipulates a maximum other than the
global maximum lays it out.

> Padding with what? Zeroes or random noise? Before compression or after? The former answers would be simpler, the latter probably more secure.

"Padding octets MUST be set to zero when sending and ignored when receiving."

The point about compression is valid though.  It happens after compression.

> Shouldn't min and max values be defined for this as well?

We talked about that.  Minimum is zero.  Some folks were concerned
that a receiver could effectively mount a denial of service by setting
a maximum to 2^32-1.  But the only thing that does in practice is to
deny access to the static table.

> I'm tempted to think a line mentioning what to do in the interim might be helpful - i.e. to assume the previous values. I can imagine some very subtle and evil bugs introduced if this is not done (…

I think that the implication is strong enough here.  Happy to take
text suggestions, of course.

> Might be worth adding the usual "always set to zero, always ignore" blurb for completeness.

That's in the lead-in.  I want to avoid unnecessary repetition.

> Maybe I'm missing something, but why the restriction on a frame on a different stream? I don't believe there's anywhere else in the protocol that such a restriction is made.

Yes, CONTINUATION is special and its intentional.

> With what error code? REFUSED_STREAM?

Whatever they like.

> Surely that first sentence is redundant?

Removed.

> I think it would be better if the concept of these pseudo-headers (currently in 8.1.3) was introduced before they were used in examples.

Yeah, I moved the section down below the definition of header fields.

> I don't understand why this example uses a "host" header when the following one uses ":authority".

That's a mistake.

> So what do we do with the userinfo? In particular, I'm thinking of cases for HTTP/2 to HTTP/1.1 or FTP-over-HTTP.

Strip it.  That's what you are required to do today.  It's used to
construct authentication parameters.

> What happens if a client receives a HEADER frame? Does it need to be minimally processed (for maintaining compression state)?

Yes.  The normal processing rules apply.

> Plus there are a few typos I noticed:

Got 'em thanks.

---

From: "Richard Wheeldon (rwheeldo)" <[email-masked]>
Date: Tue, 20 May 2014 11:27:29 +0000

> Sorry about taking so long to get around to this.

No prob. We're all busy folks.

> > It's a bit ambiguous just from the text if the fields are there but set to al zero or missing completely from the byte stream? If the bytes were missing i would make no sense for a single bit to be so.
> 
> The bytes are omitted entirely.  I'm not sure how to make this any clearer.  If
> you have any suggestions, that would help.

How about, instead of:
"E: A single bit flag indicates that the stream dependency is exclusive, see Section 5.3.  This field is optional and is only present if the PRIORITY flag is set."
Put:	
"E: A single bit flag indicates that the stream dependency is exclusive, see Section 5.3.  This field is optional and is only valid if the PRIORITY flag is set. Otherwise, it must be set to zero" or similar. It's just the idea of a single bit missing from the stream that makes the other text confusing.

(snip a lot of changes that I'm perfectly happy with)

> > It would be good if these were laid out explicitly at some point, rather than simply implied by reading the protocol docs.
> >It might encourage folks to write better validation code earlier.
> 
> It is.  Each frame type that stipulates a maximum other than the global
> maximum lays it out.

I meant in one place as a table or similar. Currently it's distributed throughout the doc.

(snip more changes)

> > "A receiver MUST treat the receipt of any other type of frame or a frame
> on a different stream as a connection error"
> > Maybe I'm missing something, but why the restriction on a frame on a
> different stream? I don't believe there's anywhere else in the protocol that
> such a restriction is made.
> 

OK. Then I'm definitely missing something. Is there a link that explains the intent?

---

From: Mike Bishop <[email-masked]>
Date: Tue, 20 May 2014 13:57:45 +0000

But Richard, the E field *isn't* zero if PRIORITY isn't set.  It is *absent.*  Not sent.  Go straight to the next field while parsing.

And it's not that a single bit is absent -- there's a thirty-two-bit segment that is present or absent based on the presence of the PRIORITY flag, and that thirty-two-bit segment contains E (1 bit) and Stream Dependency (31 bits).  Rather than introduce a new parceling type, Martin has simply indicated that both these fields are present/absent based on that flag.

-----Original Message-----
From: Richard Wheeldon (rwheeldo) [mailto:[email-masked]] 
Sent: Tuesday, May 20, 2014 4:27 AM
To: Martin Thomson
Cc: HTTP Working Group
Subject: RE: Minor points in draft 12

How about, instead of:

(snip more changes)

> different stream? I don't believe there's anywhere else in the 
> protocol that such a restriction is made.
>

---

From: Martin Thomson <[email-masked]>
Date: Tue, 20 May 2014 07:54:52 -0700

I think that Mike answered the first.  And I'm reluctant to add more
tabulation of what is basically a secondary piece of information.

> OK. Then I'm definitely missing something. Is there a link that explains the intent?

The basic idea is that header blocks can exceed the size of a single
frame.  But we need to ensure that they are processed as a single
contiguous block because they modify/access the connection global
header compression state.  The was we have agreed to guarantee
exclusive access to that state is to force header blocks to be
contiguous.

---

From: "Richard Wheeldon (rwheeldo)" <[email-masked]>
Date: Wed, 21 May 2014 12:23:32 +0000

Got it. Thanks.

-----Original Message-----
From: Richard Wheeldon (rwheeldo) [mailto:[email-masked]]
Sent: Tuesday, May 20, 2014 4:27 AM
To: Martin Thomson
Cc: HTTP Working Group
Subject: RE: Minor points in draft 12

How about, instead of:

(snip more changes)