Subject: [quicwg/base-drafts] Editorial suggestions from Benjamin Kaduk (invariants, TLS) (#4473)

From: kaduk <[email-masked]>
Date: Mon, 04 Jan 2021 20:53:11 -0800

I made some proposed tweaks in my local copy as I was doing my IESG review.
This pull request contains the suggetsions for -invariants and -tls (sorry if that is not the preferred workflow; there&#39;s just one change to invariants and it&#39;s in its own commit, so it will be easy to separate if needed).
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * invariants: clarify authentication requirements for version negotiation
  * TLS does not have a &quot;Handshake Layer&quot;
  * Drop the &quot;Perfect&quot; from PFS
  * Editorial suggestions from IESG Evaluation

-- File Changes --

    M draft-ietf-quic-invariants.md (2)
    M draft-ietf-quic-tls.md (83)

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/4473.patch
https://github.com/quicwg/base-drafts/pull/4473.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/4473

---

From: kaduk <[email-masked]>
Date: Mon, 04 Jan 2021 21:01:21 -0800

@kaduk commented on this pull request.

> @@ -344,7 +344,7 @@ requires that middleboxes retain state for every connection ID they see.
 
 The Version Negotiation packet described in this document is not
 integrity-protected; it only has modest protection against insertion by off-path
-attackers.  An endpoint MUST authenticate the contents of a Version Negotiation
+attackers.  An endpoint MUST authenticate the semantic content of a Version Negotiation

(some discussion of the motivation for this is in the commit message)

> @@ -154,7 +154,7 @@ Internally, TLS is a layered protocol, with the structure shown in
 
 ~~~~
           +-------------+------------+--------------+---------+
-Handshake |             |            |  Application |         |
+Content   |             |            |  Application |         |

I really don't think it's appropriate to lump Application Data and the Alert Protocol into something called the "Handshake Layer".  I'm not tied to "Content Layer", but "Handshake Layer" is not accurate.

> @@ -681,9 +684,9 @@ Endpoints that use session resumption might need to remember some information
 about the current connection when creating a resumed connection. TLS requires
 that some information be retained; see Section 4.6.1 of {{!TLS13}}. QUIC itself
 does not depend on any state being retained when resuming a connection, unless
-0-RTT is also used; see {{enable-0rtt}} and Section 7.4.1 of

This renders as "Section 4.6.1 and Section 7.4.1 of [QUIC-TRANSPORT]"; swapping the order makes it clear which section reference is in this document.

> @@ -720,10 +723,15 @@ notably those caused by the potential exposure to replay attack; see {{replay}}.
 
 ### Enabling 0-RTT {#enable-0rtt}
 
-To communicate their willingness to process 0-RTT data, servers send a
-NewSessionTicket message that contains the early_data extension with a
-max_early_data_size of 0xffffffff.  The TLS max_early_data_size parameter is not
-used in QUIC.  The amount of data that the client can send in 0-RTT is
+The TLS "early_data" extension in the NewSessionTicket message is defined
+to convey (in the "max_early_data_size" parameter) the amount of TLS 0-RTT
+data the server is willing to accept.  QUIC does not use TLS 0-RTT data;
+instead, QUIC has its own 0-RTT stream.  Accordingly, the
+"max_early_data_size" parameter is repurposed to hold a sentinel value
+0xffffffff to indicate that the server is willing to accept QUIC 0-RTT data;
+to indicate that the server does not accept 0-RTT data, the "early_data"
+extension is omitted from the NewSessionTicket.

(This all might become moot depending on the resolution of my discuss point.)

-- 
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/4473#pullrequestreview-561514186

---

From: Martin Thomson <[email-masked]>
Date: Tue, 05 Jan 2021 19:17:20 -0800

@martinthomson approved this pull request.

Thanks Ben, this is mostly good.  I have a few suggestions, but I am planning to accept this with those (and some cleanup) assuming that we don't get better advice on "Content Layer".

> @@ -403,8 +406,8 @@ acquires handshake bytes before sending its first packet.  A QUIC server starts
 the process by providing TLS with the client's handshake bytes.
 
 At any time, the TLS stack at an endpoint will have a current sending
-encryption level and receiving encryption level. Encryption levels determine
-the packet type and keys that are used for protecting data.
+encryption level and receiving encryption level. TLS Encryption levels determine

```suggestion
encryption level and receiving encryption level. TLS encryption levels determine
```

```suggestion
data the server is willing to accept.  QUIC does not use TLS 0-RTT data.
QUIC uses 0-RTT packets to carry early data.  Accordingly, the
```

> @@ -2142,9 +2151,9 @@ Because the header uses a 4-byte packet number encoding, the first 16 bytes of
 the protected payload is sampled, then applied to the header:
 
 ~~~
-sample = d1b1c98dd7689fb8ec11d242b123dc9b
+sampled_data = d1b1c98dd7689fb8ec11d242b123dc9b

```suggestion
sampled_bytes = d1b1c98dd7689fb8ec11d242b123dc9b
```

-mask = AES-ECB(hp, sample)[0..4]
+mask = AES-ECB(hp, sampled_data)[0..4]

```suggestion
mask = AES-ECB(hp, sampled_bytes)[0..4]
```

> @@ -2050,7 +2059,7 @@ included. All values are shown in hexadecimal.
 
 ## Keys
 
-The labels generated during the execution of the HKDF-Expand-Label function and
+The labels (Hkdf.label) generated during the execution of the HKDF-Expand-Label function and

I don't think that this is right.  I think that you mean:

```suggestion
The labels generated during the execution of the HKDF-Expand-Label function (that is, HkdfLabel.label) and part of the value
```

> @@ -2221,7 +2230,7 @@ As a result, after protection, the header protection sample is taken starting
 from the third protected byte:
 
 ~~~
-sample = 2cd0991cd25b0aac406a5816b6394100
+sampled_data = 2cd0991cd25b0aac406a5816b6394100

```suggestion
sampled_bytes = 2cd0991cd25b0aac406a5816b6394100
```

> @@ -2300,7 +2309,7 @@ The resulting ciphertext is the minimum size possible. One byte is skipped to
 produce the sample for header protection.
 
 ~~~
-sample = 5e5cd55c41f69080575d7999c25a5bfb
+sampled_data = 5e5cd55c41f69080575d7999c25a5bfb

```suggestion
sampled_bytes = 5e5cd55c41f69080575d7999c25a5bfb
```

 ~~~~

@ekr, do you have any opinion on this?

-- 
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/4473#pullrequestreview-562295941

---

From: Mike Bishop <[email-masked]>
Date: Wed, 06 Jan 2021 08:28:47 -0800

@MikeBishop commented on this pull request.

This might even warrant an "...of this document" to make it explicit.

-- 
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/4473#discussion_r552779054

---

From: kaduk <[email-masked]>
Date: Wed, 06 Jan 2021 08:36:31 -0800

## Keys

Oops, yes, the HkdfLabel bit got garbled in transmission from brain to fingers.
Your suggestion looks fine; I just wanted to reiterate that it's the whole HkdfLabel.label (including the "tls13 " prefix) that we show, not just the argument to HKDF-Expand-Label.

-- 
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/4473#discussion_r552786795

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 06 Jan 2021 19:42:45 -0800

At a high level, I'm ok with the editorial changes, but I'd like to push back against the changes to variable and field names at a minimum. We've had a lot of eyes go over this so far, and changing names risks introducing inconsistencies or omissions that we can't afford to do at this late stage.

-- 
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/4473#issuecomment-755863473

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 06 Jan 2021 19:43:10 -0800

@janaiyengar requested changes on this pull request.

-- 
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/4473#pullrequestreview-563191927

---

From: kaduk <[email-masked]>
Date: Wed, 06 Jan 2021 22:58:34 -0800

> At a high level, I'm ok with the editorial changes, but I'd like to push back against the changes to variable and field names at a minimum. We've had a lot of eyes go over this so far, and changing…

Just to check: this is basically just the `sample`->`sampled_bytes` change?
The immediate impetus for that proposal is from the way this chunk renders in the text version:

```
   Header protection uses the output of the packet
   protection AEAD to derive "sample", and then encrypts the header
   field using a pseudorandom function (PRF) as follows:
```

In the HTML and PDF formats there's different formatting, but this one makes it look like the quotes are surrounding the literal output of the derivation process, as opposed to a name for the output of the derivation process. 

That issue could probably be avoided with just a local rewording in this one instance (though I do think that "sampled_bytes" is an objectively better name for what it is).

-- 
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/4473#issuecomment-755926869

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 06 Jan 2021 23:12:07 -0800

> Just to check: this is basically just the sample->sampled_bytes change?

Yes, that and the Content Layer changes. My contention is that we're unlikely to get a lot of eyes over these documents, and I am objecting to cosmetic changes that have high risk at this late stage.

-- 
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/4473#issuecomment-755932395

---

From: kaduk <[email-masked]>
Date: Thu, 07 Jan 2021 15:43:29 -0800

> Yes, that and the Content Layer changes. My contention is that we're unlikely to get a lot of eyes over these documents, and I am objecting to cosmetic changes that have high risk at this late stag…

Hmm, not-Handshake-Layer is basically the thing I care about the most in this PR.  That specific discussion seemed quite localized.  (The word "layer" only appears 16 times in the document, which seems feasible to exhaustively check for a "Handshake" prefix.)

-- 
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/4473#issuecomment-756454036

---

From: kaduk <[email-masked]>
Date: Mon, 11 Jan 2021 10:50:31 -0800

@kaduk pushed 2 commits.

aa4f3b2f6d797ce383c8647a918b30da322ff13f  Revert sampled_data/sampled_bytes
d1febff0b960e1ed23b3f850d42cfaf136580869  Alernate approach for discussing sampled bytes

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

---

From: kaduk <[email-masked]>
Date: Mon, 11 Jan 2021 10:52:15 -0800

I reverted the `sampled_bytes`/`sampled_data` stuff and proposed an alternative local rewording in the prose that doesn't affect the variable name or pseudocode.

I did not revert the "Handshake Layer"-->"Content Layer" change, and would still like to hear what @ekr thinks about that topic.

-- 
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/4473#issuecomment-758151111

---

From: Martin Thomson <[email-masked]>
Date: Mon, 11 Jan 2021 19:30:39 -0800

@martinthomson pushed 1 commit.

338b9c77394520acb038876c9ef561a37e672baa  fixup

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4473/files/d1febff0b960e1ed23b3f850d42cfaf136580869..338b9c77394520acb038876c9ef561a37e672baa

---

From: Martin Thomson <[email-masked]>
Date: Mon, 11 Jan 2021 19:31:07 -0800

00cf25a5bbd386535c8d5ec230dcc1ddd5fe58c0  fewer semi-trailers

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4473/files/338b9c77394520acb038876c9ef561a37e672baa..00cf25a5bbd386535c8d5ec230dcc1ddd5fe58c0

---

From: Martin Thomson <[email-masked]>
Date: Mon, 11 Jan 2021 19:31:42 -0800

73be063fa6d8749a781532ca86790b3f9eca0a54  HkdfLabel...

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4473/files/00cf25a5bbd386535c8d5ec230dcc1ddd5fe58c0..73be063fa6d8749a781532ca86790b3f9eca0a54

---

From: Martin Thomson <[email-masked]>
Date: Mon, 11 Jan 2021 19:38:56 -0800

@martinthomson pushed 2 commits.

bcf28d9891539d30f98cb5e0aa11a791696e7265  Merge branch 'master' into pr4473
52b5d69d2f691e2f051a879dc0a675ddda406fd3  Wrap

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4473/files/73be063fa6d8749a781532ca86790b3f9eca0a54..52b5d69d2f691e2f051a879dc0a675ddda406fd3

---

From: Martin Thomson <[email-masked]>
Date: Tue, 12 Jan 2021 16:08:29 -0800

Merged #4473 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/4473#event-4198699953