Subject: Re: Working Group Last Call: draft-ietf-httpbis-http2-14 and draft-ietf-httpbis-header-compression-09

From: Ilari Liusvaara <[email-masked]>
Date: Sun, 3 Aug 2014 17:03:55 +0300

On Fri, Aug 01, 2014 at 10:54:32AM +1000, Mark Nottingham wrote:
>
> The editors have published new drafts of HTTP/2 and HPACK:
>   http://tools.ietf.org/html/draft-ietf-httpbis-http2-14
>   http://tools.ietf.org/html/draft-ietf-httpbis-header-compression-09
> 
> ... and our issues list is currently empty. As such, this message begins
> Working Group Last Call on these documents.

Some misc. comments from a readthrough.

Main spec:

> 3.5.  HTTP/2 Connection Preface

> Clients and servers MUST terminate the TCP connection if either peer
> does not begin with a valid connection preface.  A GOAWAY frame
> (Section 6.8) can be omitted if it is clear that the peer is not
> using HTTP/2.

Under what cicrumstances one can deduce from invalid preface that
the peer is using HTTP/2? If ALPN says so?

Also, what GOAWAY code (not that it matters much)? PROTOCOL_ERROR?

> 4.3.  Header Compression and Decompression

> Header compression is stateful, using a single compression context
> for the entire connection.

This seems bit misleading to me. There are two contexts per connection,
one per direction (but perhaps the other would be "decompression
context").

> A receiver MUST terminate the connection with a
> connection error (Section 5.4.1) of type COMPRESSION_ERROR if it does
> not decompress a header block.

I think this is problematic. Hidden limit (no requirement to handle
HPACK blocks up to very large size, no way to signal actual limits)
that is connection-killer if exceeded.

Since it is was decided that signaling HPACK block limits was a bad
idea, either:
- Require being able to decompressing large HPACK blocks for effects
  on header table (so one can gracefully deal with large headers).
- Reset incoming HPACK state (can be done using SETTINGS) on error
  and continue with connection.

> 5.1.  Stream States

Idle state description seems to be missing text about receiving anything
except HEADERS or PUSH_PROMISE being a protocol error.

State "reserved (local):"

> The endpoint can send a HEADERS frame.  This causes the stream
> to open in a "half closed (remote)" state.

Except if HEADERS sets END_STREAM (e.g. pushing cache updates via HEAD),
then the transition is directly to closed state.

State "reserved (remote):"

> Receiving a HEADERS frame causes the stream to transition to
> "half closed (local)".

Same as above, END_STREAM being set causes transition to closed.

> 6.6.  PUSH_PROMISE

> Since PUSH_PROMISE reserves a stream, ignoring a PUSH_PROMISE frame
> causes the stream state to become indeterminate.  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.

There is also the RST_STREAM race (as noted in state description)
that can cause PUSH_PROMISE to be recevied in closed state.

> 6.8.  GOAWAY

How to handle invalid GOAWAY frames (plen < 8)? GOAWAY(PROTOCOL_ERROR)?

> 8.1.2.3.  Request Header Fields

> ":scheme" is not restricted to "http" and "https" schemed URIs.  A
> proxy or gateway can translate requests for non-HTTP schemes,
> enabling the use of HTTP to interact with non-HTTP services.

I think that there should be RST_STREAM error code for unknown :scheme,
so that endpoints receiving unknown values can gracefully reject
those requests.

With known schemes, the endpoint is in much better position to respond
to potentially malformed requests (or unknown :authority).

Receiving indication of "scheme unknown" is much cleaner to deal with
than receiving confused HTTP reply (or worse, connection error).

Also, I think that if :scheme is "https" and the underlying connection
is not secure, then the server MUST reply with
RST_STREAM(INADEQUATE_SECURITY).

> To ensure that the HTTP/1.1 request line can be reproduced
> accurately, this header field MUST be omitted when translating
> from an HTTP/1.1 request that has a request target in origin or
> asterisk form (see [RFC7230], Section 5.3).  Clients that generate
> HTTP/2 requests directly SHOULD instead omit the "Host" header
> field.  An intermediary that converts an HTTP/2 request to
> HTTP/1.1 MUST create a "Host" header field if one is not present
> in a request by copying the value of the ":authority" header
> field.

I can't parse this properly. But I note that Host: header in HTTP/2
requests is troublesome if the server has to do anything with it,
due to the fact that it can be anywhere in the header block.

Also, what happens if both Host and :authority are present?

> 8.1.2.4.  Response Header Fields

> HTTP/2 does not define a way to carry the version or reason phrase
> that is included in an HTTP/1.1 status line.

Reason phrases are sometimes useful with 4XX and 5XX responses.
Could be added by including OPTIONAL :reason response header.

> 8.1.3.  Examples

>     GET /resource HTTP/1.1           HEADERS
>     Host: example.org          ==>     + END_STREAM
>     Accept: image/jpeg                 + END_HEADERS
>                                          :method = GET
>                                          :scheme = https
>                                          :path = /resource
>                                          host = example.org
>                                          accept = image/jpeg

This hits the ordering issue I noted above (host can appear anywhere,
whereas :authority is very near the start).

> 8.2.2.  Push Responses

> This stream becomes
> "half closed" to the client (Section 5.1) after the initial HEADERS
> frame is sent.

Unless HEADERS has END_STREAM, then it is closed.

> 9.2.2.  TLS Cipher Suites

> Ephemeral
> key exchange MUST have a minimum size of 2048 bits for DHE or
> security level of 128 bits for ECDHE.

How is "security level" defined? I can come up with multiple defintions,
and those are quite different (some fail NIST P-256).

> Clients MAY advertise support of cipher suites that are prohibited by
> the above restrictions in order to allow for connection to servers
> that do not support HTTP/2.  This enables a fallback to protocols
> without these constraints without the additional latency imposed by
> using a separate connection for fallback.

If connecting to typical TLS server without special HTTP/2 support at
TLS level(!), then one better put all the HTTP/2 ciphersuites at beginning
of the list, because many TLS servers will pick the first one off of the
list, and if it isn't compatible with HTTP/2, the connection will fail
(GOAWAY(INADEQUATE_SECURITY)).

Worse, some servers have ciphersuite perferences. If server prefers
some non-HTTP/2-compatible cipher, does not treat HTTP/2 specially
and clent sends that ciphersuite, again, the connection is going to fail.

Hpack spec:

> 8.1.2.  Mitigation

> An encoder without good knowledge of the provenance of header fields
> might instead introduce a penalty for bad guesses, such that attempts
> to guess a header field value results in all values being removed
> from consideration in all future requests, effectively preventing
> further guesses.

If one has to consider re-installation, this can potentially take
lots of memory, since "blacklisted" values must be kept even beyond
flushing out of compression state.

> Implementations might also choose to protect certain header fields
> that are known to be highly valued, such as the Authorization or
> Cookie header fields, by disabling or further limiting compression.

For most authorization schemes (all but the most insecure ones),
indexing Authorization values is useless anyway, as those will never
be repeated (and the Authorization header name is in static table).

-Ilari

---

From: Mike Bishop <[email-masked]>
Date: Mon, 4 Aug 2014 18:25:49 +0000

3.5 - I don't think it's so much that you *can* infer they're HTTP/2....  You can choose to throw the GOAWAY at them in hopes they see and understand it, or you can omit it because you think it's likely they won't.  Since the GOAWAY frame will likely be racing with a TCP RST, it effectively doesn't matter -- the frame will get lost when the RST gets there.

4.3 - I think this is an implementation decision.  You SHOULD always gracefully handle decompression so your context remains valid, even if you choose to reject the request because the output would be too large for you to process.  However, if for some reason you're not able to do so, this error code gives you a way to signal why you're closing the connection.  Of course, again, it races with the TCP RST and may well never be seen by the remote endpoint.

-----Original Message-----
From: Ilari Liusvaara [mailto:[email-masked]] 
Sent: Sunday, August 3, 2014 7:04 AM
To: Mark Nottingham
Cc: HTTP Working Group
Subject: Re: Working Group Last Call: draft-ietf-httpbis-http2-14 and draft-ietf-httpbis-header-compression-09

On Fri, Aug 01, 2014 at 10:54:32AM +1000, Mark Nottingham wrote:
> ... and our issues list is currently empty. As such, this message 
> begins Working Group Last Call on these documents.

> (Section 6.8) can be omitted if it is clear that the peer is not using 
> HTTP/2.

Under what cicrumstances one can deduce from invalid preface that the peer is using HTTP/2? If ALPN says so?

> Header compression is stateful, using a single compression context for 
> the entire connection.

This seems bit misleading to me. There are two contexts per connection, one per direction (but perhaps the other would be "decompression context").

> A receiver MUST terminate the connection with a connection error 
> (Section 5.4.1) of type COMPRESSION_ERROR if it does not decompress a 
> header block.

I think this is problematic. Hidden limit (no requirement to handle HPACK blocks up to very large size, no way to signal actual limits) that is connection-killer if exceeded.

Since it is was decided that signaling HPACK block limits was a bad idea, either:

> 5.1.  Stream States

Idle state description seems to be missing text about receiving anything except HEADERS or PUSH_PROMISE being a protocol error.

> The endpoint can send a HEADERS frame.  This causes the stream to open 
> in a "half closed (remote)" state.

Except if HEADERS sets END_STREAM (e.g. pushing cache updates via HEAD), then the transition is directly to closed state.

> Receiving a HEADERS frame causes the stream to transition to "half 
> closed (local)".

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

There is also the RST_STREAM race (as noted in state description) that can cause PUSH_PROMISE to be recevied in closed state.

> proxy or gateway can translate requests for non-HTTP schemes, enabling 
> the use of HTTP to interact with non-HTTP services.

I think that there should be RST_STREAM error code for unknown :scheme, so that endpoints receiving unknown values can gracefully reject those requests.

With known schemes, the endpoint is in much better position to respond to potentially malformed requests (or unknown :authority).

Receiving indication of "scheme unknown" is much cleaner to deal with than receiving confused HTTP reply (or worse, connection error).

Also, I think that if :scheme is "https" and the underlying connection is not secure, then the server MUST reply with RST_STREAM(INADEQUATE_SECURITY).

> To ensure that the HTTP/1.1 request line can be reproduced accurately, 
> this header field MUST be omitted when translating from an HTTP/1.1 
> request that has a request target in origin or asterisk form (see 
> [RFC7230], Section 5.3).  Clients that generate
> HTTP/2 requests directly SHOULD instead omit the "Host" header field.  
> An intermediary that converts an HTTP/2 request to
> HTTP/1.1 MUST create a "Host" header field if one is not present in a 
> request by copying the value of the ":authority" header field.

I can't parse this properly. But I note that Host: header in HTTP/2 requests is troublesome if the server has to do anything with it, due to the fact that it can be anywhere in the header block.

>     Accept: image/jpeg                 + END_HEADERS

This hits the ordering issue I noted above (host can appear anywhere, whereas :authority is very near the start).

> Ephemeral
> key exchange MUST have a minimum size of 2048 bits for DHE or security 
> level of 128 bits for ECDHE.

How is "security level" defined? I can come up with multiple defintions, and those are quite different (some fail NIST P-256).

> using a separate connection for fallback.

If connecting to typical TLS server without special HTTP/2 support at TLS level(!), then one better put all the HTTP/2 ciphersuites at beginning of the list, because many TLS servers will pick the first one off of the list, and if it isn't compatible with HTTP/2, the connection will fail (GOAWAY(INADEQUATE_SECURITY)).

Worse, some servers have ciphersuite perferences. If server prefers some non-HTTP/2-compatible cipher, does not treat HTTP/2 specially and clent sends that ciphersuite, again, the connection is going to fail.

> to guess a header field value results in all values being removed from 
> consideration in all future requests, effectively preventing further 
> guesses.

If one has to consider re-installation, this can potentially take lots of memory, since "blacklisted" values must be kept even beyond flushing out of compression state.

> Cookie header fields, by disabling or further limiting compression.

For most authorization schemes (all but the most insecure ones), indexing Authorization values is useless anyway, as those will never be repeated (and the Authorization header name is in static table).

-Ilari

---

From: Mark Nottingham <[email-masked]>
Date: Wed, 20 Aug 2014 11:45:01 +1000

Just a reminder; WGLC ends in a bit less than two weeks.

We've gathered a few issues which are listed at:
  https://github.com/http2/http2-spec/issues
... along with a few pull requests:
  https://github.com/http2/http2-spec/pulls

Note that some editorial fixes have already made their way into the main document:
  http://http2.github.io/http2-spec/

Cheers,

On 1 Aug 2014, at 10:54 am, Mark Nottingham <[email-masked]> wrote:

> Everyone,
> ... and our issues list is currently empty. As such, this message begins Working Group Last Call on these documents.
> 
> Please review the documents carefully, and comment on this list. One of the goals of WGLC is to assess consensus on the document, so expressing your support (or lack thereof) as well as implementation status / plans is important.
> 
> If you find editorial issues, you can raise them directly on github <https://github.com/http2/http2-spec/issues> (ideally with a pull request), or directly to the editors.
> 
> WGLC will end on 1 September.
> 
> At the NYC interim, we discussed a plan for doing interop before closing WGLC, based upon draft-13/-08. However, there have been a number of changes in the wire format for draft-14/-09, and it would be good to see interop for them. After discussing this with most of the implementers that indicated interest in NYC, we're going to call -14/-09 an interop draft as well.
> 
> Cheers,
> 
> --
> Mark Nottingham   https://www.mnot.net/
> 
> 

--
Mark Nottingham   https://www.mnot.net/

---

From: "Roy T. Fielding" <[email-masked]>
Date: Fri, 29 Aug 2014 13:36:37 -0700

I have been reading this over the past few weeks (on various planes
and now at home) and am frankly surprised by the number of non-editorial
comments I have amassed.  I will try to sort them into individual
suggestions for the list.

....Roy