Re: Richard Barnes' Discuss on draft-ietf-httpbis-http2-16: (with DISCUSS and COMMENT)

On Thu, Jan 22, 2015 at 1:57 AM, Mark Nottingham <mnot@mnot.net> wrote:

> Hi Richard,
>
> My .02 below.
>
> > On 22 Jan 2015, at 5:14 pm, Richard Barnes <rlb@ipv.sx> wrote:
> >
> > (1) Section 3.2: "A server MUST ignore a "h2" token in an Upgrade header
> > field."
> > What is the reasoning behind this exclusion?  This seems to unnecessarily
> > rule out the use of TLS, especially given that the server can opt out by
> > choosing "h2c".
>
> We chose ALPN as the interoperable way to upgrade a TLS connection to h2;
> doing Upgrade inside TLS makes no sense, because ALPN support is required,
> and is much more efficient (since the entire connection has to stop for the
> Upgrade dance). I.e., Upgrade is designed for plaintext HTTP only, and use
> elsewhere is a good sign that there's a buggy client.


Different problem.  You're talking about how you upgrade a TLS connection
once you're making one.  I'm talking about what you do when you start with
an "http" URI.

The path I'm asking about is roughly as follows:
1. Client: Upgrade: h2, h2c
2. Server: Upgrade: TLS/1.2, h2
3. Client: ClientHello with ALPN, ...

(Maybe include a client preamble to help with middleboxes.) It seems like
that provides a path to get from an "http" URI to TLS, which is possible in
h2 by virtue of the :scheme pseudo-header.  You can get to "http" over TLS
via connection reuse; why would you forbid going there directly?

One of my favorite things about this spec is that it disentangles the
concepts of transport and scheme.  The two places I've called out in this
DISCUSS (here and below) are the only ones where there still seems to be
confusion, so it seems worth ironing them out so that the separation can be
clean.



>
> > (2) Figure 1 seems really confusing.  If the reader notices the phrase "9
> > octets of the frame header", he'll probably come to the right conclusion,
> > but it also seems likely that some readers will infer from the layout
> > that the header is 12 octets long, with the fields aligned to word
> > boundaries.  Just eliminating the header with the bit positions would
> > help a lot.  Likewise for the figures in Section 6.
>
>  Seems reasonable.
>
>
> > (3) Section 9.1.1: "For "http" resources..."
> > This seems to imply that requests for "http" resources can only be
> > carried over bare TCP, which seems wrong given the presence of the
> > ":scheme" pseudo-header.  Proposed text:
> >
> > OLD: "For "http" resources, this depends on the host having resolved to
> > the same IP address."
> > NEW: "For TCP connections without TLS, this depends on the host having
> > resolved to the same IP address."
>
> Within the scope of this specification, I think the current text is
> correct; further specifications (including HTTP OppSec, if it wants to) can
> modify this behaviour when they're in effect.


I actually think that this spec does a pretty good job of separating
"cleartext/TLS" from "http/https".  The only points of confusion are here
and the point mentioned above.  Regardless of whether we do OppSec, it
seems worth being clear about this distinction.



> > (4) Section 9.1.1: "For "https" resources..."
> > The salient requirement here is that the certificate provided by the
> > server MUST pass any checks that the client would have done if it were
> > initiating the connection afresh.  In addition to the name check here,
> > that would include things like HPKP.  Suggested text:
> >
> > OLD: "For "https" resources, connection reuse additionally depends on
> > having a certificate that is valid for the host in the URI."
> > NEW: "For "https" resources, connection reuse additionally depends on
> > having a certificate that is valid for the host in the URI.  The
> > certificate presented by the server MUST satisfy any checks that the
> > client would perform when forming a new TLS connection for the host in
> > the URI (e.g. HPKP checks [HPKP])."
>
> Seems reasonable. I'm not sure I'd use HPKP as the reference here...
>
>
> > (5) Section 10.4: "Pushed responses for which an origin server is not
> > authoritative (see Section 10.1) are never cached or used."
> > This seems like a rather important point, for which I can't find any
> > normative text.  It seems like in Section 8.2.1, the client should be
> > REQUIRED to verify that the ":authority" field in the PUSH_PROMISE
> > contains a value for which the client would have been willing to re-use
> > the connection (as specified in Section 9.1).
>
> Agreed.
>
>
> > ----------------------------------------------------------------------
> > COMMENT:
> > ----------------------------------------------------------------------
> >
> > Section 2.1:
> > The definitions of "client" and "server" here are a bit lean.  For
> > example, one might read them and conclude that the client and server
> > roles are independent of who sends requests and responses.  It would be
> > good to clarify these roles, updating the definition in RFC 7230: "An
> > HTTP "client" is a program that establishes a connection to a server for
> > the purpose of sending one or more HTTP requests.  An HTTP "server" is a
> > program that accepts connections in order to service HTTP requests by
> > sending HTTP responses."
> >
> > Section 2.1: "across a virtual channel"
> > What is a virtual channel?  Can this phrase just be deleted?
> >
> > Section 3.1: "CREF1: RFC Editor's Note:"
> > It seems like it could be useful to leave in a variant of this note,
> > describing the variant identifiers used by pre-RFC versions of the
> > protocol.  (And thus removing the RFC 2119 language.)
> >
> > Section 3.4: "the sever MUST send"
> >
> > Section 4.1: "R: A reserved 1-bit field."
> > I was mystified by the purpose of this field until I realized that it's
> > only there because stream IDs are 31 bits (to make room for the Exclusive
> > flag, I guess).  Might help this read more smoothly to note something to
> > that effect here.
> >
> > Section 5.1:
> > When I initially read Figure 2, I thought that "H/", "ES/", etc.
> > designated types of frames (or frames + flags).  If, as they appear, they
> > indicate alternatives, it would be clearer to add a space before the
> > "/".
> >
> > Section 8.1.2.3: "_:authority_"
> > Remove the underscores?
> >
> > Section 10.3: "if they are translater verbatim"
> >
> >
>
> --
> Mark Nottingham   https://www.mnot.net/
>
>