Subject: [quicwg/base-drafts] Editorial suggestions from Benjamin Kaduk (transport) (#4597)

From: kaduk <[email-masked]>
Date: Wed, 06 Jan 2021 13:27:13 -0800

I made some proposed tweaks in my local copy as I was doing my IESG review.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Editorial suggestions from IESG Evaluation

-- File Changes --

    M draft-ietf-quic-transport.md (155)

-- Patch Links --

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

---

From: kaduk <[email-masked]>
Date: Wed, 06 Jan 2021 13:35:49 -0800

@kaduk commented on this pull request.

> @@ -1684,7 +1686,9 @@ retry_source_connection_id transport parameter.
 
 The values provided by a peer for these transport parameters MUST match the
 values that an endpoint used in the Destination and Source Connection ID fields
-of Initial packets that it sent. Including connection ID values in transport
+of Initial packets that it sent (and received, for servers). Endpoints MUST
+validate that received transport parameters match received Connection ID values.
+Including connection ID values in transport

I'm calling this "editorial" even though it adds a new MUST, because I believe the intent was already that you do this, and so the text is just reiterating an existing expectation.

> @@ -4739,8 +4747,9 @@ server MAY send multiple Initial packets.  The cryptographic key exchange could
 require multiple round trips or retransmissions of this data.
 
 The payload of an Initial packet includes a CRYPTO frame (or frames) containing
-a cryptographic handshake message, ACK frames, or both.  PING, PADDING, and
-CONNECTION_CLOSE frames of type 0x1c are also permitted.  An endpoint that
+a cryptographic handshake message, ACK frames, CONNECTION_CLOSE frames of type
+0x1c, or some combination thereof.  PING and PADDING frames are also permitted.
+An endpoint that

This is pretty awkward.  The idea is that the old text implies that it is invalid to have an Initial with neither a CRYPTO nor an ACK.  But IIUC it's valid to send CONNECTION_CLOSE and nothing else...

> @@ -5065,8 +5075,8 @@ Destination Connection ID:
 
 Packet Number:
 
-: The packet number field is 1 to 4 bytes long. The packet number has
-  confidentiality protection separate from packet protection, as described in
+: The packet number field is 1 to 4 bytes long. The packet number is protected
+  using header protection; see

Trying to use consistent language for each field covered by header protection.

> @@ -5257,7 +5267,8 @@ initial_max_stream_data_uni (0x07):
 initial_max_streams_bidi (0x08):
 
 : The initial maximum bidirectional streams parameter is an integer value that
-  contains the initial maximum number of bidirectional streams the peer is
+  contains the initial maximum number of bidirectional streams the endpoint
+  that receives this transport parameter is

We were inconsistent about how we described the two endpoints in these paragraphs.  I'm not sure I caught all instances, though, but the "the endpoint that receives this transport parameter" seems like a good clear description.

> @@ -5437,7 +5449,7 @@ application protocol wishes to prevent the connection from timing out; see
 Receivers send ACK frames (types 0x02 and 0x03) to inform senders of packets
 they have received and processed. The ACK frame contains one or more ACK Ranges.
 ACK Ranges identify acknowledged packets. If the frame type is 0x03, ACK frames
-also contain the sum of QUIC packets with associated ECN marks received on the
+also contain the cumulative count of QUIC packets with associated ECN marks received on the

(you can't take a sum of QUIC packets; there is no addition operation defined for them.)

> @@ -5492,15 +5504,14 @@ ACK Delay:
 
 ACK Range Count:
 
-: A variable-length integer specifying the number of Gap and ACK Range fields in
+: A variable-length integer specifying the number of ACK Range fields in

This whole section was a bit awkward.  I tried to operate on the principle that the Ack Range is the compound structure, and that Gap and the Ack Range Length are the member fields within it.  The resulting text isn't very pretty, though...

> @@ -6520,8 +6531,8 @@ of the security properties of QUIC depend on the TLS handshake providing these
 properties. Any attack on the TLS handshake could affect QUIC.
 
 Any attack on the TLS handshake that compromises the secrecy or uniqueness
-of session keys affects other security guarantees provided by QUIC that depends
-on these keys. For instance, migration ({{migration}}) depends on the efficacy
+of session keys, or the authentication of the participating peers, affects other security guarantees provided by QUIC that depend
+on those keys. For instance, migration ({{migration}}) depends on the efficacy

I'm open to better phrasing suggestions here, too.

 An endpoint that acknowledges packets it has not received might cause a
 congestion controller to permit sending at rates beyond what the network
 supports.  An endpoint MAY skip packet numbers when sending packets to detect
 this behavior.  An endpoint can then immediately close the connection with a
 connection error of type PROTOCOL_VIOLATION; see {{immediate-close}}.
+Note that introducing artificial gaps might increase the size of the
+returned ACK packets in a manner that scales roughly llinearly with the

Oops, "llinearly" typo.
```suggestion
returned ACK packets in a manner that scales roughly linearly with the
```

> @@ -7540,7 +7559,7 @@ Description:
 
 The initial contents of this registry are shown in {{iana-error-table}}.
 
-| Value | Error                     | Description                   | Specification   |
+| Value | Code                      | Description                   | Specification   |

(The prose says this field is 'Code', not 'Error'.)

-- 
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/4597#pullrequestreview-563047920

---

From: Martin Thomson <[email-masked]>
Date: Wed, 06 Jan 2021 17:06:13 -0800

@martinthomson requested changes on this pull request.

I've some concerns with some of the changes here, but there are a lot of good ones.  Some of these are unnecessary however.  Can you revert or fix those changes I've identified?  Thanks.

> +{{QUIC-RECOVERY}}, and the use of cryptographic mechanisms, including the use
+of TLS for key negotiation {{QUIC-TLS}}.

This sentence structure is really odd.

```suggestion
{{QUIC-RECOVERY}}, and the use of TLS and other cryptographic mechanisms {{QUIC-TLS}}.
```

> @@ -1529,7 +1531,7 @@ Handshake (CRYPTO)
 1-RTT (*)              ---------->
                        <---------- 1-RTT (HANDSHAKE_DONE,*)
 
-1-RTT (*)              <=========>                1-RTT (*)
+1-RTT (                <=========>                1-RTT

```suggestion
1-RTT                  <=========>                    1-RTT
```

> @@ -1529,7 +1531,7 @@ Handshake (CRYPTO)
 1-RTT (*)              ---------->
                        <---------- 1-RTT (HANDSHAKE_DONE,*)

```suggestion
                       <----------   1-RTT (HANDSHAKE_DONE)
```

 A client MUST change the Destination Connection ID it uses for sending packets
-in response to only the first received Initial or Retry packet.  A server MUST
+in response to only the first received Initial and/or Retry packet(s).  A server MUST

```suggestion
in response to only the first received Initial or Retry packet.  A server MUST
```
I don't accept the use of "and/or" in any document I edit.

> +more reliable if relatively few packets are sent or if packet loss (that might
+be deliberately triggered by an attacker) coincides

```suggestion
more reliable if relatively few packets are sent or if packet loss coincides
```
I prefer this without the extra illumination.

> @@ -3613,16 +3621,15 @@ connection can usually appear in packets in any packet number space, whereas
 those associated with transferring data can only appear in the application
 data packet number space:
 
-- PADDING, PING, and CRYPTO frames MAY appear in any packet number space.
+- PADDING and PING frames MAY appear in any packet number space.

This change is invalid.  CRYPTO is permitted in every packet number space.  It is only prohibited from 0-RTT.

```suggestion
- PADDING, PING, and CRYPTO frames MAY appear in any packet number space.
```

> +- ACK frames MAY appear in any packet number space other than 0-RTT, but can only acknowledge
+  packets that appeared in that packet number space.

Again, 0-RTT is not a packet number space.

```suggestion
- ACK frames MAY appear in any packet number space, but can only acknowledge
  packets that appeared in that packet number space.  However, as noted below,
  0-RTT packets cannot contain ACK frames.
```

> @@ -4195,7 +4203,7 @@ meet size constraints; the endpoint MAY however discard such datagrams.
 
 A client MUST expand the payload of all UDP datagrams carrying Initial packets
 to at least the smallest allowed maximum datagram size of 1200 bytes by adding
-PADDING frames to the Initial packet or by coalescing the Initial packet; see
+PADDING frames to the Initial packet and/or by coalescing the Initial packet; see

```suggestion
PADDING frames to the Initial packet or by coalescing the Initial packet; see
```

See above.

> +server; a client MUST NOT process those bits other than to verify the Retry
+Integrity Tag.  In addition to the fields from the

I don't think that this is a useful clarification.  Can you revert please?

> +salt) but cannot observe the server's transport parameters as sent to
+the client and cannot influence
+parameter negotiation.  (The attacker could make its own connection to
+the server and observe the transport parameters sent on that
+connection.)

I don't think that this adds much value and so I would prefer not to take this change.

 Connection IDs are unencrypted but integrity protected in all packets.
 
-This version of QUIC does not incorporate a version negotiation mechanism;
+This version of QUIC does not incorporate a complete or secure version negotiation mechanism;

This is unnecessary.

> @@ -6613,7 +6627,7 @@ ensures that valid packets are only generated by endpoints that possess the
 key material established during the handshake; see {{handshake}} and
 {{handshake-properties}}. Similarly, any active attacker that observes packets
 and attempts to insert new data or modify existing data in those packets should
-not be able to generate packets deemed valid by the receiving endpoint.
+not be able to generate non-Initial packets deemed valid by the receiving endpoint.

```suggestion
not be able to generate packets deemed valid by the receiving endpoint,
other than Initial packets.
```

> @@ -6658,7 +6672,7 @@ An on-path attacker can:
 - Delay packets
 - Reorder packets
 - Drop packets
-- Split and merge datagrams along packet boundaries
+- Split and merge datagrams along packet boundaries (though header protection obfsucates the location of packet boundaries)

Unnecessary change.

+returned ACK packets in a manner that scales roughly llinearly with the
+number of gaps introduced, so fewer, larger, gaps may be more efficient.

Unnecessary change.

I'm OK with this.

Please revert this.  I don't think that it's an improvement.

 Packet Number:

This is a good change.

The structure doesn't seem ideal, but this is OK with me and I can't see anything better.

-- 
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/4597#pullrequestreview-563140403

---

From: kaduk <[email-masked]>
Date: Sat, 09 Jan 2021 21:00:12 -0800

@kaduk pushed 1 commit.

2dd11b946d975a37a1503d276e9eb2beae82d0d9  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/4597/files/5206f03421c2aeb95f726fd532c042a9ce40a74c..2dd11b946d975a37a1503d276e9eb2beae82d0d9

---

From: kaduk <[email-masked]>
Date: Sat, 09 Jan 2021 21:08:20 -0800

@kaduk pushed 1 commit.

d1a7c14e40b0321db0fb96a2eedf3cb0bb4ce5c7  Manually revert chunks not done via github

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4597/files/2dd11b946d975a37a1503d276e9eb2beae82d0d9..d1a7c14e40b0321db0fb96a2eedf3cb0bb4ce5c7

---

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

I think I made all the requested changes.  I will make a pass through the diff to check that I didn't introduce any weirdness as a result.

Done.

 1-RTT (*)              ---------->

Oops, not sure how I messed that up.

I feel duly embarrassed at having missed that fact in my pre-PR-submission review (for all occurrences).

Okay.  I think we have ample precedent for "process" being a flexible term...

Okay; reverted.

Okay; reverted.

 - Delay packets
 - Reorder packets
 - Drop packets

Reverted

Reverted.

-- 
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/4597#pullrequestreview-564828938

---

From: kaduk <[email-masked]>
Date: Sat, 09 Jan 2021 21:15:36 -0800

> @@ -6863,7 +6872,7 @@ Servers SHOULD provide mitigations for this attack by limiting the usage and
 lifetime of address validation tokens; see {{validate-future}}.

-## Optimistic ACK Attack
+## Optimistic ACK Attack (#optimistic-ack-attack}

```suggestion
## Optimistic ACK Attack {#optimistic-ack-attack}
```

-- 
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/4597#pullrequestreview-564829749

---

From: kaduk <[email-masked]>
Date: Sat, 09 Jan 2021 21:15:44 -0800

@kaduk pushed 1 commit.

e786b9693fa71671a14e8cd77f2d464bbab3860d  Update draft-ietf-quic-transport.md

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

---

From: kaduk <[email-masked]>
Date: Sat, 09 Jan 2021 21:19:24 -0800

I would like some help with the CI issues (I don't currently have a CircleCI account and would prefer to not have to read the T&C just for 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/4597#issuecomment-757418696

---

From: Lucas Pardue <[email-masked]>
Date: Sat, 09 Jan 2021 21:48:42 -0800

@kaduk addiction the failure is simply line length. If you hard wrap everything to 80 characters all should be good.

-- 
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/4597#issuecomment-757421909

---

From: kaduk <[email-masked]>
Date: Sat, 09 Jan 2021 22:09:41 -0800

@kaduk pushed 1 commit.

c0c070d5ca97d1053050e8e16a3e911f4b6949ba  Rewrap to 80 columns

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

---

From: kaduk <[email-masked]>
Date: Sat, 09 Jan 2021 22:10:07 -0800

@LPardue ah, thanks.

-- 
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/4597#issuecomment-757423912

---

From: Martin Thomson <[email-masked]>
Date: Sun, 10 Jan 2021 18:06:35 -0800

@martinthomson approved 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/4597#pullrequestreview-564929162

---

From: Jana Iyengar <[email-masked]>
Date: Sun, 10 Jan 2021 22:39:32 -0800

@janaiyengar approved this pull request.

A couple of nits, which I'll commit, but otherwise lgtm.

> @@ -5293,8 +5303,8 @@ disable_active_migration (0x0c):
 
 : The disable active migration transport parameter is included if the endpoint
   does not support active connection migration ({{migration}}) on the address
-  being used during the handshake.  When a peer sets this transport parameter,
-  an endpoint MUST NOT use a new local address when sending to the address that
+  being used during the handshake.  An endpoint that receives this transport
+  parameter MUST NOT use a a new local address when sending to the address that

```suggestion
  parameter MUST NOT use a new local address when sending to the address that
```

 ACK Range Count:

This is a good improvement -- thanks!

-- 
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/4597#pullrequestreview-565092321

---

From: Jana Iyengar <[email-masked]>
Date: Sun, 10 Jan 2021 22:39:47 -0800

@janaiyengar pushed 1 commit.

a8f69911ef8919e7b1e9bc8b6ef0d8910fdca4a0  Update draft-ietf-quic-transport.md

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

---

From: Jana Iyengar <[email-masked]>
Date: Sun, 10 Jan 2021 22:43:28 -0800

aee31f6a0afab57de8be42eb3393a15e8feb527f  Merge branch 'master' into transport-iesg-eval

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

---

From: Jana Iyengar <[email-masked]>
Date: Sun, 10 Jan 2021 22:46:07 -0800

cf5e04d93af79955633239ebf2feaf64d5f437fa  lint

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

---

From: Jana Iyengar <[email-masked]>
Date: Sun, 10 Jan 2021 22:47:39 -0800

Merged #4597 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/4597#event-4189169293