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

On Mon, Jan 26, 2015 at 6:55 PM, Martin Thomson <martin.thomson@gmail.com>
wrote:

> Thanks for the review Richard,
>
> https://github.com/http2/http2-spec/pull/709


I <3 PRs for DISCUSS resolution.



> On 21 January 2015 at 22:14, 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".
>
> I think that Mark explained this adequately.
>
> Your suggestion is one of the different startup variants that was
> considered and rejected.


OK, thanks.  That's what I needed.



> The conclusion being that there needs to be
> *exactly one* way start any given transport, regardless of scheme.
> (We can discuss why the prior knowledge thing remains, if you like.)
>
> Having the startup path vary based on scheme like that seems to invite
> the sort of layering confusion you want to avoid though.
>
> > (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.
>
> Fixed throughout.
>
> > (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:
> [...]
> > NEW: "For TCP connections without TLS, this depends on the host having
> > resolved to the same IP address."
>
> Sold.  No harm in anticipating future needs.
>
> > (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])."
>
> Yes, though I dropped the HPKP reference.  I considered adding a 2818
> or 6125 reference, but decided to avoid both and also avoid implying
> too narrow a scope for the checks.
>
> > (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).
>
> MUST-ied.  As in
> - <xref target="authority"/>) are never cached or used.
> + <xref target="authority"/>) MUST NOT be used or cached.


This still seems like burying the lede.  How about the proposed change in
8.2.1?  It seems like the point that the server must be authoritative is
important enough to put up front.  This seems like it's pretty parallel to
the checks on ":method", so borrowing text from that:

"The server MUST include a value in the ":authority" header field for which
the server is authoritative (see Section 10.1).  If a client receives
a PUSH_PROMISE for a resource for which the server is not authoritative, it
MUST respond with a stream error (Section 5.4.2) of type PROTOCOL_ERROR."

(Or maybe STREAM_REFUSED?  Since these requests should not be processed at
all by the client.)


>
> > 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."
>
> Check the PR here.
>
> > Section 2.1: "across a virtual channel"
> > What is a virtual channel?  Can this phrase just be deleted?
>
> Baleeted.
>
> > 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.)
>
> I think that I might just rely on tools.ietf.org to provide that
> little bit of history.  If that's OK with you.
>
> I mean, technically, we should be registering all these identifiers.
>
> > 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.
>
> The exclusive thing was a relatively recent addition.  An explanation
> would be retconning.
>
> The actual and original reason is what I shall term "MSB-phobia": the
> fear associated with 32- or 64-bit numbers that use the most
> significant bit.  Languages without unsigned variants of the fixed
> sized integers, combined with an unwillingness to deal with the
> occasional rollover into negative values (or a fear that doing so
> might trigger a bug), causes some people to avoid using the bit
> entirely.
>
> In any case, we've avoided providing such explanations in the past,
> preferring to stick with facts.
>
> > 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
> > "/".
>
> Done.
>
> > Section 8.1.2.3: "_:authority_"
> > Remove the underscores?
>
> Stephen noticed it too.
>
> Did you know that <spanx> defaults to <spanx style="emph">.  Well, you
> do now.  And you will never get those brain cells back either.
>