Last Call Review of draft-ietf-httpbis-http2-16
review-ietf-httpbis-http2-16-genart-lc-davies-2015-03-16-00

Request Review of draft-ietf-httpbis-http2
Requested rev. no specific revision (document currently at 17)
Type Last Call Review
Team General Area Review Team (Gen-ART) (genart)
Deadline 2015-01-20
Requested 2015-01-15
Other Reviews Genart Last Call review of -16 by Elwyn Davies (diff)
Secdir Last Call review of -16 by Chris Lonvick (diff)
Review State Completed
Reviewer Elwyn Davies
Review review-ietf-httpbis-http2-16-genart-lc-davies-2015-03-16
Posted at http://www.ietf.org/mail-archive/web/gen-art/current/msg11258.html
Reviewed rev. 16 (document currently at 17)
Review result Ready with Nits
Draft last updated 2015-03-16
Review completed: 2015-03-16

Review
review-ietf-httpbis-http2-16-genart-lc-davies-2015-03-16

I've updated the pull request.  You can see the changes there.

Note that the reason I'm pushing back hard on text additions is that
it's hard to ensure that I've correctly maintained the intent of text
that has already been extensively reviewed.  Deletions are easier, but
only slightly.  If you feel strongly about anything I've not acted on,
feel free to raise it.

On 21 January 2015 at 09:36, Elwyn Davies <elwynd at dial.pipex.com> wrote:
> It's the WG's choice to limit transports to TCP : so be it.  I'd have liked
> to allow alternatives such as SCTP but that is a personal view.

The only reason we did so is that no one offered to do the work.

> How about adding "subject to limited constraints" to the Section 5 bullet.

The statement is still correct without it.

> I have done a bit of homework on this including having a look at
> draft-pironti-tls-length-hiding-01,
> so I hope I understand this a little better now.  Practically, I think it
> would be helpful to move the last paragraph of s6.1 and combine it with the
> second para that introduces the idea of padding, making the security
> implications more upfront.  I think I am correct in saying that padding
> would be pointless if not using TLS: I think it would be worth saying this
> in s6.1.

Sounds reasonable.

> I think an (informative) reference to draft-pironti-tls-length-hiding-01 in
> s10.7 would be useful - I think it covers the 'redundant padding could be
> counterproductive' story in the 2nd sentence and also the statements in the
> last para of s10.7.

I think that I would, if it weren't for the relative levels involved
here.  The idea here is to include a clear signal that padding isn't
necessarily easy and then leave it to implementers to learn that this
warning was perhaps a little strong.  Better that than a false sense
of security.

>> I don't think this is a problem.  Most proposals that have been
>> floated use a reciprocal SETTINGS frame to signal that the value was
>> understood.  The only failure mode there occurs when there is a
>> collision between two extensions that results in two conflicting views
>> of the setting semantics.  Of course, that is equally possible with a
>> rejection-based scheme.
>
> Sorry, but I don't understand your logic here.  Let me try again:
>
> If (say) the client asks for a SETTINGS value from some future extension
> that the server doesn't understand, my interpretation is that the server
> will ignore the value it doesn't understand while doing the ones that it
> does understand and say 'ACK' to the client. I assume that the client is
> then entitled to believe that the server has interpreted its requested
> SETTINGS as expected and the client can do whatever the extension was
> intended to allow.  Depending on what the ignored SETTINGS are, this may
> have bad consequences during later operations.
>
> Accordingly, I think that the SETTINGS extension needs some way to tell the
> requesting endpoint that the responding endpoint couldn't action (part of)
> its requests.  The requester can then act accordingly, terminating the
> session or avoiding actions that exploit whatever setting couldn't be
> actioned.

Let me try again.  The ACK only provides an indication that the
settings that were understood were acted upon.  It doesn't provide
proof-positive that an extension was understood.  Another mechanism is
required for that.  One potential solution is to include a value for
the same setting in a separate SETTINGS frame.

>>> s3.5, paras 2-5: It would be worth explaining that what is being done
>>> here
>>> is to send what would be a method request for the PRI method which will
>>> not
>>> be recognized by well-implemented HTTP/1.x servers.  The PRI method is
>>> then
>>> reserved for HTTP/1.1 so that it can't be accidentally implemented later
>>> (see Section 11.6).
>>
>> We've discussed providing explanatory text of this nature in the
>> working group on a few occasions, but avoided it.  In this case, the
>> main beneficiaries of that information are people who aren't reading
>> and implementing this specification, so we decided against more
>> verbiage to that effect.
>
> Well.. with my gen-art hat firmly in place, part of the object of RFCs is
> that they shouldn't be totally opaque to those who are reading them, even
> those who aren't actually experts implementing the protocol.  My suggestion
> for an explanation would help those.. I didn't immediately twig why the
> string was structured as it is.

The right place for this discussion is the working group list.  If you
feel strongly enough.  I'm trying very hard to avoid (re)opening
issues.

> <<snip>>
>>>
>>> s5.1, next to last para; s5.5, para 3, s6.2, END_HEADERS, para 2:
>>> s5.1 says:
>>> OLD:
>>> Frame of unknown types are ignored.
[...]
>> I have little concern that the feature
>> will be accidentally missed.
>
> Perhaps not but I spent some time worrying about the inconsistency so other
> may..
> can we compromise on:
> NEW:
> Frames of unknown types are ignored except when they occur in CONTINUATION
> frame sequences (see Sections 5.5, 6.2 and 6.6).

My inclination is to remove the sentence entirely rather than fix it.

>>> s5.1.1, last para/s3.4, last para, s9.1, para 3:  Having to open a new
>>> connection when stream ids run out interacts with the point made in s3.4
>>> that there is no guarantee that the new connection will support HTTP/2.
>>> It
>>> might be worth pointing this out in s5.1.1 and s9.1 - it means the client
>>> has to be able to switch back to HTTP/1.1 in the very rare case that this
>>> happens. Thought: Would it be useful for a server to have a SETTINGS flag
>>> that guarantees that it would do HTTP/2 on all subsequent connections?
>>
>> Yes, and the fallback isn't necessarily pretty.  At least not for
>> performance.  I think that the view is that this is a low risk
>> scenario and not worthy of special attention.
>
> Hmm.. On the other hand, in the rare case that it does happen it might bite
> the client nastily if not catered for.  I think it is worth a short sentence
> in s5.1.1 at least.

If I were to do anything, I'd have thought that Sec3 was more
appropriate, but I couldn't see anywhere there.

One virtue of HTTP is that implementations are generally stateless.
That extends to the transport/protocol mappings.  This error doesn't
arise because maintaining state about whether a server previously
supported X is actually harder than just running the same logic every
time.  With ALPN that's free.

>>> s6.2, END_STREAM flag, 2nd para: s/A HEADERS frame carries/A HEADERS
>>> frame
>>> can carry/
>>
>> It always carries the flag, it is whether it is set that matters.
>
> In fact, isn't the first sentence of s6.2, para 2 redundant?  The previous
> para just said it signals stream end.  I think you can delete it completely.

I think that I got the text in question.  Yeah.