[Gen-art] Gen-art last call review of draft-ietf-httpbis-http2-16

Elwyn Davies <elwynd@dial.pipex.com> Mon, 19 January 2015 10:37 UTC

Return-Path: <elwynd@dial.pipex.com>
X-Original-To: gen-art@ietfa.amsl.com
Delivered-To: gen-art@ietfa.amsl.com
Received: from localhost (ietfa.amsl.com [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 089AC1B2A1C; Mon, 19 Jan 2015 02:37:28 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -99.199
X-Spam-Level:
X-Spam-Status: No, score=-99.199 tagged_above=-999 required=5 tests=[BAYES_50=0.8, HTML_MESSAGE=0.001, USER_IN_WHITELIST=-100] autolearn=ham
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id fy8kcd2k52uU; Mon, 19 Jan 2015 02:37:22 -0800 (PST)
Received: from auth.a.painless.aa.net.uk (a.painless.aa.net.uk [IPv6:2001:8b0:0:30::51bb:1e33]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 186051B2A1A; Mon, 19 Jan 2015 02:37:22 -0800 (PST)
Received: from brdgfw.folly.org.uk ([81.187.254.242] helo=[192.168.0.139]) by a.painless.aa.net.uk with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.77) (envelope-from <elwynd@dial.pipex.com>) id 1YD9hu-0008UG-2t; Mon, 19 Jan 2015 10:37:18 +0000
Message-ID: <54BCDE62.9090902@dial.pipex.com>
Date: Mon, 19 Jan 2015 10:37:22 +0000
From: Elwyn Davies <elwynd@dial.pipex.com>
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0
MIME-Version: 1.0
To: General area reviewing team <gen-art@ietf.org>
Content-Type: multipart/alternative; boundary="------------030409050203080202090404"
Archived-At: <http://mailarchive.ietf.org/arch/msg/gen-art/50PKgEm3XNQOlDdtfjNIFM2XAXU>
Cc: draft-ietf-httpbis-http2.all@tools.ietf.org, IETF Discussion List <ietf@ietf.org>
Subject: [Gen-art] Gen-art last call review of draft-ietf-httpbis-http2-16
X-BeenThere: gen-art@ietf.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: "GEN-ART: General Area Review Team" <gen-art.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/gen-art>, <mailto:gen-art-request@ietf.org?subject=unsubscribe>
List-Archive: <http://www.ietf.org/mail-archive/web/gen-art/>
List-Post: <mailto:gen-art@ietf.org>
List-Help: <mailto:gen-art-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/gen-art>, <mailto:gen-art-request@ietf.org?subject=subscribe>
X-List-Received-Date: Mon, 19 Jan 2015 10:37:28 -0000

I am the assigned Gen-ART reviewer for this draft. For background on
Gen-ART, please see the FAQ at

<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

Please resolve these comments along with any other Last Call comments
you may receive.

Document: draft-ietf-httpbis-http2-16.txt
Reviewer: Elwyn Davies
Review Date:  2015-01-18
IETF LC End Date: 2015-01-14
IESG Telechat date: 2015-01-22

Summary:
Almost ready.  A well written document with just a few nits really. I am 
slightly surprised (having not been following httpbis in detail) that 
HTTP/2 is so tightly wedded to TCP - this is doubtless pragmatic but it 
adds to the ossification of the Internet and makes me slightly 
suspicious that it is an attempt to really confirm HTTP/2 as the waist 
of the neo-Internet.  Can't we ever use any other transport?  A couple 
of minor issues: (1) I am not sure that I see the (security) value of 
the padding mechanisms specified, but I am not an expert so I will defer 
to the security experts, and (2) the extension method for SETTINGS seems 
to be flawed.  Apologies for the slightly delayed review.

Major Issues:
s3, para 1: Just checking: HTTP/2 is defined to run over TCP connections 
only.  I take it that this is something that the WG has formally decided 
upon.  Is there something that stops HTTP/2 running over any other 
reliable byte stream protocol with in-order delivery?  Could there be a 
more general statement?

Minor Issues:
s4.3, next to last para; s5, bullet #1:
(Just a bit more than a nit)
s4.3 says:
> Header blocks MUST be
>     transmitted as a contiguous sequence of frames, with no interleaved
>     frames of any other type or from any other stream.
s5 says:
>     o  A single HTTP/2 connection can contain multiple concurrently open
>        streams, with either endpoint interleaving frames from multiple
>        streams.
If s4.3 is correct (which multiple repetitions elsewhere indicate that 
it is) then the bullet in s5 needs to reflect the constraint in s4.3 as 
they are currently inconsistent.

I am not quite sure whether the last para of s4.3 implies that the 
client must wait until
it has the whole header block before trying to decompress or whether 
decompression
might happen as fragments are received - please clarify this in the text.

s6.1 and s6.2: I am dubious about the value of the padding story in DATA 
and HEADERS frames, even given the caveats in s10.7.  An attacker can 
use the header to deduce the padding section.  It seems a bit odd to say 
that you can add arbitrary padding and then insist it is all zeroes.  
However, I am not an expert in this area and will defer to security experts.

s6.5.3: If an endpoint sends a SETTINGS value that the receiving 
endpoint doesn't understand (for an extension, say), the receiver will 
ignore it but still send an ACK.  This leaves the sending endpoint 
unaware that the other endpoint didn't understand it.  I suspect that 
this makes the extension mechanism for settings effectively useless.  I 
note that s6.5 says that implementations MUST support the values defined 
in the draft so that the ACK mechanism works fine for the base system, 
but any extension of SETTINGS cannot be used to limit the behaviour of 
the receiving endpoint because the sender can't rely on the receiver 
actioning the limitation; the only useful things might be to expand a 
limit that the sender would otherwise have to conform to.  It would be 
possible to alter the spec so that the receiver can send back any values 
it didn't understand with a frame with ACK set - not exactly negotiation 
but just rejection.

Editorial/Nits:
Abstract:
> allowing multiple
>     concurrent messages on the same connection.
Technically the *messages* are not on the connection concurrently. How 
about:
      allowing a server to simultaneously process multiple requests 
submitted via a single
     connection with arbitrary ordering of responses.

s2, last para:
> allowing many requests to be compressed into one TCP packet.
At this point in the document, nothing has been said about TCP.
Better:
    allowing many requests to be compressed into one transport 
connection packet.

s2.2: I think definitions or references or deprecations for 
message/message body/message payload/message headers/payload data and  
entity/entity headers/entity body are needed.  With HTTP/2 the 
distinction between message body and entity body is no longer needed I 
think.  It appears that message body is not currently used but message 
payload, payload data and entity body are.  I think that it might be 
useful to stick to message payload in the body of the draft and add a 
note in terminology to indicate that message payload covers both message 
body and entity body in HTTP/1.x and stress that there is only one encoding.

s3.2.1, para 4:
OLD:
production for "token68"
NEW:
production for "token68", allowing all the characters in a base64url string,

s3.4, para 3: s/sever/server/

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

s4.1, Figure 1 (and other figures): The frame header layout doesn't 
match the usual conventions for protocol component specifications where 
each 32 bit 'word' is filled out.  I'm not sure how strictly we would 
want to adhere to this convention... consult an AD.

s4.1: Would performance be helped by aligning the length and stream 
identifiers on
32 bit boundaries?

s4.1, Flags:  For the avoidance of doubt it would be good to label the 
flag bits with the numbers that are used later in the document.

s4.1, Stream Identifier:  s5.1 and s5.1.1 say the identifiers are 
integers - this should be reflected here.  Also consistency between s4.1 
and s5.1.1 should have the reserved id as either 0 or 0x0 (OK, this is 
really picky!)

s5, bullets #2 and #5:
>     o  Streams can be established and used unilaterally or shared by
>        either the client or server.
>     o  Streams are identified by an integer.  Stream identifiers are
>        assigned to streams by the endpoint initiating the stream.
[Is there a risk of a race condition in which one end allocates a stream 
id and sends using it and the other end allocates the same id for a 
different purpose while the first frame is in transit?  Maybe clients 
should always use odd numbered streams and servers even numbered .. or 
is this a use for the reserved bit to be used by servers?]

OK.. I correctly identified the possibility of race conditions.. Please 
add a pointer to s5.1.1 which tells you to do what I just proposed.   
Another pointer in s4.1 definition of stream id would also help.

s5.1:
OLD:
    half closed (local):
       A stream that is in the "half closed (local)" state cannot be used
       for sending frames.  Only WINDOW_UPDATE, PRIORITY and RST_STREAM
       frames can be sent in this state.
NEW:
    half closed (local):
       A stream that is in the "half closed (local)" state cannot be used
       for sending frames with the exceptions of  WINDOW_UPDATE,
       PRIORITY and RST_STREAM frames.
Also: Would it be worth saying that any type of frame can be received in 
this state?

s5.1:
>     half closed (remote):
>        A stream that is "half closed (remote)" is no longer being used by
>        the peer to send frames.  In this state, an endpoint is no longer
>        obligated to maintain a receiver flow control window if it
>        performs flow control.
Needs a pointer to the section where the flow control window is discussed.

s5.1, next to last para:
>     In the absence of more specific guidance elsewhere in this document,
>     implementations SHOULD treat the receipt of a frame that is not
>     expressly permitted in the description of a state as a connection
>     error (Section 5.4.1  <http://tools.ietf.org/html/draft-ietf-httpbis-http2-16#section-5.4.1>) of type PROTOCOL_ERROR.
There are not many other sorts of frames, so I think it would help 
(possibly even essential, since there doesn't seem to be anywhere else 
this is stated) to explicitly specify what sorts of frames can be 
expected to be received in the 'active' states (Open, Half closed(local) 
at least)

s5.1, next to last para: s/Frame of unknown/Frames of unknown/

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.

s5.5 and s6.2 qualify this by saying that frames of unknown types inside 
HEADER frame sequences have to be treated as connection errors.  The two 
sections need to be synced up.  Maybe:
NEW:
Frames of unknown types are ignored except when they occur in HEADER 
frame sequences when they have to be treated as a connection error of 
type PROTOCOL_ERROR (see Sections 5.5 and 6.2).

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?

  s5.3.1, para 1: To be absolutely explicit: s/on another stream/on 
exactly one other stream/

s6.2, END_STREAM flag, 2nd para: s/A HEADERS frame carries/A HEADERS 
frame can carry/

s6.2;s6.6: The wording on padding should be synchronized with that in 
s6.1 or a note added to s6.2 to refer readers to the padding comments in 
s6.1 similar to that in s6.6.  Synching 6.2 and s6.6 would be sensible.

s6.3, Weight: Should be explicitly said to be an (unsigned) integer.

s6.5.2, SETTINGS_HEADER_TABLE_SIZE: Should have a reference to 
[COMPRESSION].

s8.1.3: For clarity, the convention "+ END_HEADERS"/"- END_HEADERS" 
should be explained.

s8.1.3, last example: It might be worth noting that there could be 
significant time between the two responses being sent.

s9.1.1, para 1: s/to an origin servers/to an origin server/

s10.3, para 2: s/translater verbatim/translated verbatim/

s10.5.1, para 1:
OLD:
     For this
    an other reasons, such as ensuring cache correctness, means that an
    endpoint might need to buffer the entire header block.
NEW:
    This ordering and other reasons, such as ensuring cache correctness, 
mean that an
    endpoint might need to buffer the entire header block.

s10.5.1, para 2: s/This setting specific/This setting is specific/