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

  
  
    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

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