Subject: [quicwg/base-drafts] Editorial suggestions from Benjamin Kaduk's IESG review (#4767)

From: kaduk <[email-masked]>
Date: Tue, 19 Jan 2021 16:33:07 -0800

I split a few things out into separate commits in case they need to be excised/reverted.

May be relevant for #4763 .
You can view, comment on, or merge this pull request online at:

  https://github.com/quicwg/base-drafts/pull/4767

-- Commit Summary --

  * Is &quot;schemed&quot; even a real adjective?
  * Mention control stream earlier
  * Generic editorial suggestions noticed during IESG review
  * remove leading hex 0s from IANA error codes table

-- File Changes --

    M draft-ietf-quic-http.md (106)

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/4767.patch
https://github.com/quicwg/base-drafts/pull/4767.diff

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/4767

---

From: kaduk <[email-masked]>
Date: Tue, 19 Jan 2021 16:40:48 -0800

@kaduk commented on this pull request.

> @@ -159,7 +159,8 @@ multiplexing, and flow control.  Discovery of an HTTP/3 endpoint is described in
 Within each stream, the basic unit of HTTP/3 communication is a frame
 ({{frames}}).  Each frame type serves a different purpose.  For example, HEADERS
 and DATA frames form the basis of HTTP requests and responses
-({{request-response}}).
+({{request-response}}).  Frames that apply to the entire connection are
+conveyed on a dedicated control stream.

This subtly hints that control messages are well ordered across each other, in contrast to native QUIC control messages.  I got a bit worried about `MAX_PUSH_ID` on my initial read-through as a result, and knowing about the existence of the control stream earlier would have helped.

> @@ -556,7 +558,8 @@ extension could negotiate a modification of this restriction; see
 Pseudo-header fields are only valid in the context in which they are defined.
 Pseudo-header fields defined for requests MUST NOT appear in responses;
 pseudo-header fields defined for responses MUST NOT appear in requests.
-Pseudo-header fields MUST NOT appear in trailers.  Endpoints MUST treat a
+Pseudo-header fields MUST NOT appear in trailer sections.

IIUC the "trailers" vernacular refers to trailer fields, not trailer sections.  The preceding change was made since I was already looking at "trailers" usage, though the existing text doesn't seem incorrect in its own right.

> @@ -1150,9 +1156,8 @@ error of type H3_CLOSED_CRITICAL_STREAM.  Connection errors are described in
 
 A pair of unidirectional streams is used rather than a single bidirectional
 stream.  This allows either peer to send data as soon as it is able.  Depending
-on whether 0-RTT is enabled on the QUIC connection, either client or server
-might be able to send stream data first after the cryptographic handshake
-completes.

With 0-RTT the client can send data before the cryptographic handshake completes.

> @@ -1221,8 +1226,8 @@ HTTP/2 and HTTP/3 frames is provided in {{h2-frames}}.
 | Reserved       | Yes            | Yes            | Yes         | {{frame-reserved}}       |
 {: #stream-frame-mapping title="HTTP/3 Frames and Stream Type Overview"}
 
-Certain frames can only occur as the first frame of a particular stream type;
-these are indicated in {{stream-frame-mapping}} with a (1).  Specific guidance
+The SETTINGS frame can only occur as the first frame of a Control stream; this
+is indicated in {{stream-frame-mapping}} with a (1).  Specific guidance

I assume there was more than one such frame at some earlier time.

> @@ -1256,7 +1261,8 @@ Each frame's payload MUST contain exactly the fields identified in its
 description.  A frame payload that contains additional bytes after the
 identified fields or a frame payload that terminates before the end of the
 identified fields MUST be treated as a connection error of type
-H3_FRAME_ERROR; see {{errors}}.
+H3_FRAME_ERROR; see {{errors}}.  In particular, redundant length encodings MUST
+be verified to be self-consistent.

We do call this out in Section 10.8 already, so I don't mind if we skip this.  It might be worth reiterating, though, given how many vulnerabilities this sort of thing has caused.

> @@ -1339,7 +1345,7 @@ error of type H3_FRAME_UNEXPECTED.
 CANCEL_PUSH Frame {
   Type (i) = 0x3,
   Length (i),
-  Push ID (..),
+  Push ID (..8),

I was a bit torn on this one, since the unrestricted form matches the generic frame spec more clearly.  OTOH, we have other frame specifications that just put their own specific stuff here, so I figured I would see what we thought.

> -| H3_INTERNAL_ERROR                 | 0x0102     | Internal error                           | {{http-error-codes}}   |
-| H3_STREAM_CREATION_ERROR          | 0x0103     | Stream creation error                    | {{http-error-codes}}   |
-| H3_CLOSED_CRITICAL_STREAM         | 0x0104     | Critical stream was closed               | {{http-error-codes}}   |
-| H3_FRAME_UNEXPECTED               | 0x0105     | Frame not permitted in the current state | {{http-error-codes}}   |
-| H3_FRAME_ERROR                    | 0x0106     | Frame violated layout or size rules      | {{http-error-codes}}   |
-| H3_EXCESSIVE_LOAD                 | 0x0107     | Peer generating excessive load           | {{http-error-codes}}   |
-| H3_ID_ERROR                       | 0x0108     | An identifier was used incorrectly       | {{http-error-codes}}   |
-| H3_SETTINGS_ERROR                 | 0x0109     | SETTINGS frame contained invalid values  | {{http-error-codes}}   |
-| H3_MISSING_SETTINGS               | 0x010a     | No SETTINGS frame received               | {{http-error-codes}}   |
-| H3_REQUEST_REJECTED               | 0x010b     | Request not processed                    | {{http-error-codes}}   |
-| H3_REQUEST_CANCELLED              | 0x010c     | Data no longer needed                    | {{http-error-codes}}   |
-| H3_REQUEST_INCOMPLETE             | 0x010d     | Stream terminated early                  | {{http-error-codes}}   |
-| H3_MESSAGE_ERROR                  | 0x010e     | Malformed message                        | {{http-error-codes}}   |
-| H3_CONNECT_ERROR                  | 0x010f     | TCP reset or error on CONNECT request    | {{http-error-codes}}   |
-| H3_VERSION_FALLBACK               | 0x0110     | Retry over HTTP/1.1                      | {{http-error-codes}}   |
+| H3_NO_ERROR                       | 0x100      | No error                                 | {{http-error-codes}}   |

As noted in the commit message, this table was the only place in the document that I noticed leading hex zeros.

> @@ -2395,8 +2401,8 @@ frame of the control stream, and thereafter cannot change.  This eliminates many
 corner cases around synchronization of changes.
 
 Some transport-level options that HTTP/2 specifies via the SETTINGS frame are
-superseded by QUIC transport parameters in HTTP/3.  The HTTP-level options that
-are retained in HTTP/3 have the same value as in HTTP/2.  The superseded
+superseded by QUIC transport parameters in HTTP/3.  The HTTP-level setting that
+is retained in HTTP/3 has the same value as in HTTP/2.  The superseded

(As above, I assume there used to be more.)

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/4767#pullrequestreview-571778669

---

From: Martin Thomson <[email-masked]>
Date: Tue, 19 Jan 2021 18:48:38 -0800

@martinthomson commented on this pull request.

I've identified what I think is something worth discussing further.  I'll open an issue for that.

> +For a host that is an IP address, the client
 MUST verify that the address appears as an iPAddress in the subjectAltName field
 of the certificate.

Can we fix this sentence to parallel the previous?

> A host identified by an IP address MUST be validated using an iPAddress subjectAltName in a certificate; see {{ref}}.

> @@ -300,9 +301,9 @@ component of the URL.
 If a server presents a valid certificate and proof that it controls the
 corresponding private key, then a client will accept a secured TLS session with
 that server as being authoritative for all origins with the "https" scheme and a
-host identified in the certificate.  The host must be listed either as the CN
-field of the certificate subject or as a dNSName in the subjectAltName field of
-the certificate; see {{!RFC6125}}.  For a host that is an IP address, the client
+host identified in the certificate.  A host identified by DNS name MUST be
+validated either as a CN-ID or a DNS-ID in the certificate; see {{!RFC6125}}.

IMPORTANT: I'm not sure that we need to elevate CN-ID to MUST.  In doing so, we invalidate what CABF has done: 7.1.4.2.2 clause a of the Baseline Requirements marks subject:commonName as deprecated and discouraged.  My understanding (which I can confirm) is that browsers do not use CN-ID (concretely, the CN field of the certificate subject) and so we might want to just mention the dNSName type of subjectAltName here.  We can *mention* that some clients might use subject:commonName, but that this is a deprecated practice.

I realize that this likely breaks new ground for the RFC series, and it likely needs more disucsion, but maybe we can keep up with the times with this document.

(I apologize for not having caught this earlier; I think that this is copied from elsewhere and that too needs a refresh.)

> +wire, data (HTTP frames)
+is framed into QUIC STREAM frames, but this framing is invisible to

I think that use of "data" is confusing, but I might prefer:

```suggestion
wire, HTTP frames are
framed into QUIC STREAM frames, but this framing is invisible to
```

> @@ -2337,9 +2343,9 @@ of their frame payload.
 Other than these issues, frame type HTTP/2 extensions are typically portable to
 QUIC simply by replacing Stream 0 in HTTP/2 with a control stream in HTTP/3.
 HTTP/3 extensions will not assume ordering, but would not be harmed by ordering,
-and would be portable to HTTP/2 in the same manner.
+and would be portable to HTTP/2 in the reverse manner.

or just:

```suggestion
and are expected to be portable to HTTP/2.
```

The subtlety of the hint is appropriate.  We don't necessarily need ordering for something (MAX_PUSH_ID included), but we benefit from it for some uses.  We don't need to be clear about what has ordering requirements and what doesn't, but this is helpful.

If this is a forward reference, then include that reference;

```suggestion
be verified to be self-consistent; see {{frame-parsing}}.
```

 CANCEL_PUSH Frame {
   Type (i) = 0x3,
   Length (i),
-  Push ID (..),
+  Push ID (..8),

```suggestion
  Push ID (i),
```

This is just a varint.  I don't know why this was `(..)` rather than `(i)`.  Probably my fault.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/4767#pullrequestreview-571822237

---

From: kaduk <[email-masked]>
Date: Tue, 19 Jan 2021 19:32:25 -0800

@kaduk pushed 1 commit.

fbb31637f416c7fc0230024a06d3bac57e7524b2  Make forward-reference a real forward-reference.

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4767/files/f39a89f9f3cbd313d44afbb4ff00de5a0a089bc4..fbb31637f416c7fc0230024a06d3bac57e7524b2

---

From: kaduk <[email-masked]>
Date: Tue, 19 Jan 2021 19:34:48 -0800

It also (IIUC) doesn't preclude use of non-STREAM QUIC frames for future HTTP work (datagram?).

Good call; I wrote this before I got that far in the doc to know there was a forward reference to make.

 CANCEL_PUSH Frame {
   Type (i) = 0x3,
   Length (i),
-  Push ID (..),
+  Push ID (..8),

Good point; this is better.

Sure, I'll try to remember to do that (but it may not happen until after the telechat).

Yes, I noticed that as well but was not about to slip it into an "editorial" PR :)

Thanks for filing the issue; I will reference it from my ballot position.
There was another point relating to certificate validation that took me a while to collect my thoughts on and write up, and I want to go over the writeup again before balloting, so I didn't get a chance to mention this topic yet.

That looks better than what I have, but "foo frames are framed into" is also a bit awkward, so I'll leave it open for a bit.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/4767#discussion_r560650371

---

From: Lucas Pardue <[email-masked]>
Date: Wed, 20 Jan 2021 05:02:59 -0800

@LPardue commented on this pull request.

> +An implementation MUST ignore the Value for any SETTINGS identifier it does
 not understand.

This sentence reads weird with or without the suggestion, identifier and value are independent parts of a Setting. The text before the drawing uses "parameter" a bit. Perhaps it is clearer to say something like 

"An implementation MUST ignore parameters with an unknown identifier"?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/4767#pullrequestreview-572197000

---

From: Mike Bishop <[email-masked]>
Date: Thu, 21 Jan 2021 08:12:55 -0800

@MikeBishop approved this pull request.

Mostly looks fine.  For the suggestions and open conversations, you can either accept them and I'll merge, or I can take over the PR.  Up to you.

Can we just leave this text alone in the editorial PR, since there are already issue(s) tracking it?

> @@ -1408,7 +1414,7 @@ SETTINGS Frame {
 ~~~~~~~~~~~~~~~
 {: #fig-ext-settings title="SETTINGS Frame"}
 
-An implementation MUST ignore the contents for any SETTINGS identifier it does
+An implementation MUST ignore the Value for any SETTINGS identifier it does

```suggestion
An implementation MUST ignore any parameter with an identifier it does
```
@LPardue's suggestion looks reasonable.

How about...
```suggestion
wire, the stream data containing HTTP frames
is carried by QUIC STREAM frames, but this framing is invisible to
```
I want to be careful to avoid the implication that HTTP frames need to be contained within a single STREAM frame.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/4767#pullrequestreview-573485570

---

From: Mike Bishop <[email-masked]>
Date: Thu, 21 Jan 2021 08:14:32 -0800

@MikeBishop commented on this pull request.

> @@ -909,7 +913,7 @@ peer will assume that the connection has been closed.  HTTP/3 implementations
 will need to open a new HTTP/3 connection for new requests if the existing
 connection has been idle for longer than the idle timeout negotiated during the
 QUIC handshake, and SHOULD do so if approaching the idle timeout; see Section
-10.1 of {{QUIC-TRANSPORT}}.
+10.1.1 of {{QUIC-TRANSPORT}}.

```suggestion
10.1 of {{QUIC-TRANSPORT}}.
```
10.1.1 deals with transport-level liveness testing before sending data, which isn't contemplated here.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/4767#pullrequestreview-573494966

---

From: kaduk <[email-masked]>
Date: Thu, 21 Jan 2021 08:47:23 -0800

Okay, will attempt to revert.

FWIW I proposed 10.1.1 specifically for "SHOULD [use a new connection] if approaching the idle timeout", which I did not see discussed elsewhere in the section.  But I will revert it back.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/4767#discussion_r562029501

---

From: kaduk <[email-masked]>
Date: Thu, 21 Jan 2021 08:48:55 -0800

@kaduk pushed 1 commit.

a81f9bf532901ab77418153809981b2721b8f290  Apply suggestions from code review

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4767/files/fbb31637f416c7fc0230024a06d3bac57e7524b2..a81f9bf532901ab77418153809981b2721b8f290

---

From: kaduk <[email-masked]>
Date: Thu, 21 Jan 2021 08:50:30 -0800

@MikeBishop do we need to propagate this to the issue tracking the previous text so it gets considered there?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/4767#discussion_r562036239

---

From: kaduk <[email-masked]>
Date: Thu, 21 Jan 2021 08:53:27 -0800

@kaduk pushed 1 commit.

397f6740ac41ddaacb66b678b55ab06a65fb5d19  revert certificate validation tweaks

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4767/files/a81f9bf532901ab77418153809981b2721b8f290..397f6740ac41ddaacb66b678b55ab06a65fb5d19

---

From: Mike Bishop <[email-masked]>
Date: Fri, 22 Jan 2021 12:37:24 -0800

I think by delegating to SEMANTICS and Martin's incoming PR there in #4808, we don't need this text in HTTP/3.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/4767#discussion_r562897131

---

From: Mike Bishop <[email-masked]>
Date: Fri, 22 Jan 2021 12:39:49 -0800

Merged #4767 into master.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/4767#event-4240314300