Re: AD review of draft-ietf-httpbis-http2bis-05

[Cory Benfield <[e-mail]>]

Thanks for the detailed review Francesca. A few responses inline.

On Wed, 10 Nov 2021 at 21:26, Francesca Palombini
<[e-mail]> wrote:
>
> Thank you very much to the authors and the wg for another well written document.
>
>
>
> I don't have major comments. I have some minor comments, and a couple of nits or request for clarifications, which you can address together with any other last call comments. The requests for adding references to specific sections are there to hopefully make it easier for a reader to navigate the different specifications referenced, I obviously think they would improve readability but feel free to take or leave as you please.
>
>
>
> Additionally, before starting the last call, I want to bring your and the wg's attention to the first 2 points, related to references.
>
>
>
> I have opened a github issue with the text below: https://github.com/httpwg/http2-spec/issues/974.
>
> Francesca
>
>
>
> 1. -----
>
>
>
> FP: Is the working group aware that RFC 793bis is in IESG evaluation (https://datatracker.ietf.org/doc/draft-ietf-tcpm-rfc793bis/ ) ? Was the choice of having a normative reference to 793 conscious, in order to avoid any delay that might come from publication of draft-ietf-tcpm-rfc793bis? (Just checking this was considered)
>
>
>
> 2. -----
>
>
>
>    for HTTP/2 over TLS.  The general TLS usage guidance in [TLSBCP]
>
>    SHOULD be followed, with some additional restrictions that are
>
>    specific to HTTP/2.
>
>
>
> FP: Given this requirement, I would have expected to see [TLSBCP] normatively referenced, rather than informatively.
>
>
>
> 3. -----
>
>
>
>    layer.  The frame and stream layers are tailored to the needs of the
>
>    HTTP protocol and server push.
>
>
>
> FP: I would think the server push is part of the HTTP protocol, which makes this formulation "HTTP protocol and server push" confusing.
>
>
>
> 4. -----
>
>
>
>    Type:  The 8-bit type of the frame.  The frame type determines the
>
>
>
>    Flags:  An 8-bit field reserved for boolean flags specific to the
>
>
>
> FP: I would find a reference to the IANA registry useful.
>
>
>
> 5. -----
>
>
>
>    implementation of flow control can be difficult.  When using flow
>
>    control, the receiver MUST read from the TCP receive buffer in a
>
>    timely fashion.  Failure to do so could lead to a deadlock when
>
>
>
> FP: "When using flow control" might be rephrased to indicate "when flow control limits are lower than the maximum" (or something of the sort), since if I understand correctly the capability is always used, it is just the window size that changes (effectively implementing control of flow). Also "timely fashion" - I know that it is probably hard, but it would be nice to have a more precise qualification, or at least a hint of what is considered timely.
>
>
>
> 6. -----
>
>
>
>    stream that it successfully received from its peer.  The GOAWAY frame
>
>    includes an error code that indicates why the connection is
>
>
>
> FP: "error code" - here as well I would have liked a reference to the IANA registry.
>
>
>
> 7. -----
>
>
>
>    Exclusive:  A single-bit flag.  This field is only present if the
>
>       PRIORITY flag is set.
>
>
>
>    Stream Dependency:  A 31-bit stream identifier.  This field is only
>
>       present if the PRIORITY flag is set.
>
>
>
>    Weight:  An unsigned 8-bit integer.  This field is only present if
>
>       the PRIORITY flag is set.
>
>
>
> FP: I would have expected to see some definition of how the fields are used. If this is defined somewhere else, a reference would be good.
>
>
>
> 8. -----
>
>
>
>    SETTINGS Frame {
>
>      Length (24),
>
>      Type (8) = 4,
>
>
>
>      Unused Flags (7),
>
>      ACK Flag (1),
>
>
>
>      Reserved (1),
>
>      Stream Identifier (31),
>
>
>
>      Setting (48) ...,
>
>    }
>
>
>
>    Setting {
>
>      Identifier (16),
>
>      Value (32),
>
>    }
>
>
>
> FP: Is there any reason why the Stream Identifier line is not:
>
>      Stream Identifier (31) = 0,
>
>
>
> 9. -----
>
>
>
>       a server does include a value it MUST be 0.  A client MUST treat
>
>       receipt of a SETTINGS frame with SETTINGS_ENABLE_PUSH set to 1 as
>
>       a connection error (Section 5.4.1) of type PROTOCOL_ERROR.
>
>
>
> FP: This is just my curiosity: what is the reason for this stronger requirement - I would think it shouldn't be a problem for the sender if it wants to advertise that it would permit/support server push. What am I missing?

This MUST applies only to clients receiving the setting. If the client
received a SETTINGS frame with SETTINGS_ENABLE_PUSH set to 1, the
server would be advertising support for receiving pushes. This
document forbids servers from receiving pushes: PUSH_PROMISE frames
may only be sent to clients, they contain requests and not responses,
and so on. As a result, this document forbids servers from claiming
they support receiving pushes.

>
>
>
> 10. -----
>
>
>
>       A value of 0 for SETTINGS_MAX_CONCURRENT_STREAMS SHOULD NOT be
>
>       treated as special by endpoints.  A zero value does prevent the
>
>
>
> FP: When is it ok that the 0 value is treated as special?
>
>
>
> 11. -----
>
>
>
>    set.  Upon receiving a SETTINGS frame with the ACK flag set, the
>
>    sender of the altered settings can rely on the value having been
>
>    applied.
>
>
>
> FP: nit s/value/values. Also I believe this could be misunderstood - can it be made more precise on the fact that only the values that are present in the received frame with the ACK flag set (and not those that might have been ignored because not understood) have been applied.
>
>
>
> 12. -----
>
>
>
>   A receiver MUST treat the receipt of a WINDOW_UPDATE frame with an
>
>    flow-control window increment of 0 as a stream error (Section 5.4.2)
>
>
>
> FP: nit s/an/a
>
>
>
> 13. -----
>
>
>
>    FLOW_CONTROL_ERROR (0x3):  The endpoint detected that its peer
>
>       violated the flow-control protocol.
>
>
>
>    STREAM_CLOSED (0x5):  The endpoint received a frame after a stream
>
>       was half-closed.
>
>
>
> FP: would be good to add a reference to the relevant sections.
>
>
>
> 14. -----
>
>
>
>    set after receiving the HEADERS frame that opens a request or after
>
>    receiving a final (non-informational) status code MUST treat the
>
>
>
> FP: Where is a "non-informational status code" defined?
>
>
>
> 15. -----
>
>
>
> FP: Are the following two sentences in Section 8.1.1. in contradiction?
>
>
>
>    request or response.  Malformed requests or responses that are
>
>    detected MUST be treated as a stream error (Section 5.4.2) of type
>
>    PROTOCOL_ERROR.
>
>
>
>    on the remainder of the request being correct.  A server or
>
>    intermediary MAY use RST_STREAM -- with a code other than
>
>    REFUSED_STREAM -- to abort a stream if a malformed request or
>
>    response is received.
>
>
>
> FP: In section 5.4.2. I read:
>
>
>
>    An endpoint that detects a stream error sends a RST_STREAM frame
>
>
>
> So the first sentence above implies RST_STREAM MUST be sent, while the second sentence states RST_STREAM MAY be sent.
>
>
>
> 16. -----
>
>
>
>    their definitions in Sections Section 5.1 of [5.1] and Section 5.5 of
>
>    [5.5] of [HTTP] respectively and treat messages that contain
>
>
>
> FP: References need fixing.
>
>
>
> 17. -----
>
>
>
>      from the control data of the original request, unless the the
>
>
>
> FP: nit Remove one "the"
>
>
>
> 18. -----
>
>
>
>       Note that request targets for CONNECT or asterisk-form OPTIONS
>
>       requests never include authority information.
>
>
>
> FP: Please add a reference to 7.1 of [HTTP] as this is the first time "asterisk-form OPTION" appear in this document.
>
>
>
> 19. -----
>
>
>
>    Advertising a SETTINGS_MAX_CONCURRENT_STREAMS value of zero disables
>
>    server push by preventing the server from creating the necessary
>
>    streams.  This does not prohibit a server from sending PUSH_PROMISE
>
>    frames; clients need to reset any promised streams that are not
>
>    wanted.
>
>
>
> FP: Do these two sentences contradict each other? In the first sentence I am reading that the server can't create the necessary stream to send the PUSH_PROMISE frame, in the second sentence I read that it can?

Sadly no. § 5.1.2 says "Streams in the reserved state do not count
toward the stream limit". Setting SETTINGS_MAX_CONCURRENT_STREAMS to 0
as a client prevents the server from actually _opening_ streams, but
sending PUSH_PROMISE frames doesn't open them, it merely reserves
them. As a result it functionally disables server push, but it does
not forbid the server from sending the frames, it just prevents them
being useful.

>
>
>
> 20. -----
>
>
>
>    on a DATA frame is treated as being equivalent to the TCP FIN bit.  A
>
>
>
> FP: Can a reference be added to the section where the TCP FIN bit is defined?
>
>
>
> 21. -----
>
>
>
>      Content-Type: image/jpeg   ==>     - END_STREAM
>
>      Content-Length: 123                + END_HEADERS
>
>
>
> FP: I think it would be good to add a sentence about the meaning of - and + (which I understand to be flag set or not set) in section 2.2.
>
>
>
> 22. -----
>
>
>
>    treated as delimiters in other HTTP versions.  An intermediary that
>
>    translates an HTTP/2 request or response MUST validate fields
>
>    according to the rules in Section 8.2 roles before translating a
>
>    message to another HTTP version.  Translating a field that includes
>
>
>
> FP: is "roles" supposed to be there?
>
>
>
> 23. -----
>
>
>
>    The CONNECT method can be used to create disproportionate load on an
>
>    proxy, since stream creation is relatively inexpensive when compared
>
>
>
> FP: nit s/an/a
>
>
>
> 24. -----
>
>
>
> FP: I think it would be good to keep the 3rd paragraph in Section 11 instead of asking the RFC Editor to remove it, just to keep a trace of the registries that have been defined in 7540, since those registries will now reference this document, but this document does not contain all the definitions of the different fields.
>
>
>
> 25. ----
>
>
>
>    Comments:  Obsolete; see Section 11.1
>
>
>
> FP: I would suggest to be explicit and add "of this document" (unless links can be maintained in IANA registries Comments fields).