Subject: Re: [quicwg/base-drafts] Allow longer CIDs (#2749)

From: martinduke <[email-masked]>
Date: Sat, 01 Jun 2019 00:44:04 -0700

@martinduke pushed 1 commit.

f54a7f48c114bf7636d012bbbe326c1b87e87299  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/2749/files/63e7dd65909b908c084d366a90c13804faa7e159..f54a7f48c114bf7636d012bbbe326c1b87e87299

---

From: martinduke <[email-masked]>
Date: Sat, 01 Jun 2019 00:44:14 -0700

364f5a6e5b47ffac16bd0ee9f90a91e982541519  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/2749/files/f54a7f48c114bf7636d012bbbe326c1b87e87299..364f5a6e5b47ffac16bd0ee9f90a91e982541519

---

From: Martin Thomson <[email-masked]>
Date: Sat, 01 Jun 2019 03:11:01 -0700

martinthomson commented on this pull request.

> @@ -3577,6 +3588,10 @@ connection IDs gives clients some assurance that the server received the packet
 and that the Version Negotiation packet was not generated by an off-path
 attacker.
 
+As future versions of QUIC may support Connection IDs larger than the version 1
+limit, Version Negotiation Packets will sometimes exceed the Connection ID

s/MAY/could - it's not the place of this specification to give permission to break its own rules.

-- 
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/2749#discussion_r289599978

---

From: David Schinazi <[email-masked]>
Date: Mon, 03 Jun 2019 14:19:40 +0000 (UTC)

This CL also needs to change the encoding of the Original Destination Connection ID Length (ODCIL) in Retry packets. It is currently encoded as the four least significant bits of the first byte and that won't fit anymore. I'd suggest moving ODCIL to be a full byte between the Source Connection ID and the Original Destination Connection ID. I would then mark the four least significant bits of the first byte of Retry packets as unused similar to Version Negotiation packets.

```
 0                   1                   2                   3
 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
+-+-+-+-+-+-+-+-+
|1|1| 3 | Unused|
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|                         Version (32)                          |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|    DCIL (8)   |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|               Destination Connection ID (0..384)            ...
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|    SCIL (8)   |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|                 Source Connection ID (0..384)               ...
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|   ODCIL (8)   |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|          Original Destination Connection ID (0..384)        ...
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|                        Retry Token (*)                      ...
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
```

-- 
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/2749#issuecomment-498276578

---

From: martinduke <[email-masked]>
Date: Mon, 03 Jun 2019 12:34:28 -0700

9bbe79c075e910f1969706f1fcc0170d3c0ff23c  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/2749/files/364f5a6e5b47ffac16bd0ee9f90a91e982541519..9bbe79c075e910f1969706f1fcc0170d3c0ff23c

---

From: martinduke <[email-masked]>
Date: Mon, 03 Jun 2019 12:34:34 -0700

82cd0084816735ed3aaf243072b05ee1665d7be6  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/2749/files/9bbe79c075e910f1969706f1fcc0170d3c0ff23c..82cd0084816735ed3aaf243072b05ee1665d7be6

---

From: martinduke <[email-masked]>
Date: Mon, 03 Jun 2019 12:39:20 -0700

@martinduke pushed 3 commits.

ec6a2f13a2671a77b6f5aa4f04f941211ba1ff4f  Addressed David's comment
044f0de49809e96483a2efa6e0cdb789b91b7aad  Merge branch 'master' of https://github.com/martinduke/base-drafts
a5dd08a22bd8d74970e06b86ce1d0d83962175e3  fixed jana's formatting issues

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2749/files/82cd0084816735ed3aaf243072b05ee1665d7be6..a5dd08a22bd8d74970e06b86ce1d0d83962175e3

---

From: martinduke <[email-masked]>
Date: Tue, 04 Jun 2019 08:31:20 -0700

@DavidSchinazi Nice catch. It should be fixed now.

-- 
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/2749#issuecomment-498722596

---

From: David Schinazi <[email-masked]>
Date: Tue, 04 Jun 2019 10:11:45 -0700

DavidSchinazi commented on this pull request.

 ODCIL:
 
-: The four least-significant bits of the first byte of a Retry packet are not
-  protected as they are for other packets with the long header, because Retry
-  packets don't contain a protected payload.  These bits instead encode the
-  length of the Original Destination Connection ID field.  The length uses the
-  same encoding as the DCIL and SCIL fields.
+: These bits the length of the Original Destination Connection ID field.  The

Typo `These bits the length`

How about mimicking the SCIL field description?
```
ODCIL:

  The byte following the Source Connection ID contains the length in bytes
  of the Original Destination Connection ID field that follows it.  This length
  is encoded as a 8-bit unsigned integer.  In QUIC version 1, this value
  MUST NOT exceed 48 bytes.
```

-- 
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/2749#pullrequestreview-245583997

---

From: ianswett <[email-masked]>
Date: Tue, 04 Jun 2019 12:29:41 -0700

ianswett commented on this pull request.

> @@ -1588,15 +1588,15 @@ A token SHOULD be constructed for the server to easily distinguish it from
 tokens that are sent in Retry packets as they are carried in the same field.
 
 The token MUST NOT include information that would allow it to be linked by an
-on-path observer to the connection on which it was issued.  For example, it
+on-path observer to the connection on which it was issued. For example, it

```suggestion
on-path observer to the connection on which it was issued.  For example, it
```

-- 
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/2749#pullrequestreview-245655412

---

From: ianswett <[email-masked]>
Date: Tue, 04 Jun 2019 12:29:53 -0700

>  cannot include the connection ID or addressing information unless the values are
 encrypted.
 
 Unlike the token that is created for a Retry packet, there might be some time
 between when the token is created and when the token is subsequently used.
 Thus, a token SHOULD have an expiration time, which could be either an explicit
 expiration time or an issued timestamp that can be used to dynamically calculate
-the expiration time.  A server can store the expiration time or include it in an
+the expiration time. A server can store the expiration time or include it in an

```suggestion
the expiration time.  A server can store the expiration time or include it in an
```

-- 
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/2749#pullrequestreview-245655508

---

From: ianswett <[email-masked]>
Date: Tue, 04 Jun 2019 12:33:12 -0700

> @@ -2345,7 +2345,7 @@ following layout:
  0                   1                   2                   3
  0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-|0|1|               Unpredictable Bits (182..)                ...
+|0|1|               Unpredictable Bits (422..)                ...

So we're increasing the min size of a stateless reset token?  I'd really rather not, since that increases the chances of that being a DoS vector.  Keeping the ratio of incoming to outgoing bytes ~1:1 seems important for DoS.  As a server operator, I don't want to not respond to a client's PING only packet with a stateless reset just because it's too small, but I also don't want to respond to every small packet I don't have state for.

-- 
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/2749#pullrequestreview-245657113

---

From: ianswett <[email-masked]>
Date: Tue, 04 Jun 2019 12:35:18 -0700

Mostly LG, but I'm pretty uneasy about increasing the minimum size of a stateless reset, since it puts servers in a worse posture when responding to small packets that have no associated state.

-- 
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/2749#pullrequestreview-245658081

---

From: MikkelFJ <[email-masked]>
Date: Tue, 04 Jun 2019 14:02:30 -0700

mikkelfj commented on this pull request.

  0                   1                   2                   3

I still think we should allow responding to smaller packets at some random factor rather than not responding at all. This does not mean we should not try to keep resets small though.

-- 
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/2749#discussion_r290495871

---

From: Martin Thomson <[email-masked]>
Date: Tue, 04 Jun 2019 15:41:52 -0700

> @@ -2368,7 +2368,7 @@ of the datagram contain a Stateless Reset Token.
 
 To entities other than its intended recipient, a stateless reset will appear
 to be a packet with a short header.  For the packet to appear as valid, the
-Unpredictable Bits field needs to include at least 182 bits of data (or 23
+Unpredictable Bits field needs to include at least 422 bits of data (or 53
 bytes, less the two fixed bits).  This is intended to allow for a Destination
 Connection ID of the maximum length permitted, with a minimal packet number, and
 payload.  The Stateless Reset Token corresponds to the minimum expansion of the

A bit further down, there is a 40 and 39 that need to be changed to 70 and 69.

-- 
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/2749#pullrequestreview-245738228

---

From: ianswett <[email-masked]>
Date: Tue, 04 Jun 2019 16:05:57 -0700

0                   1                   2                   3

#2770 filed as a separate issue for 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/2749#discussion_r290529878

---

From: martinduke <[email-masked]>
Date: Wed, 05 Jun 2019 12:47:56 -0700

martinduke commented on this pull request.

  0                   1                   2                   3

OK, if you agree that this issue is non-blocking I'll proceed.

-- 
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/2749#discussion_r290904792

---

From: martinduke <[email-masked]>
Date: Thu, 06 Jun 2019 12:08:46 -0700

d60188e312f34326720c5453f0f156ab2efaa291  Fixed nits from David, Ian, MT

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

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 06 Jun 2019 13:37:57 -0700

janaiyengar commented on this pull request.

+As future versions of QUIC may support Connection IDs larger than the version 1
+limit, Version Negotiation Packets MAY carry Connection IDs that are longer than

```suggestion
limit, Version Negotiation Packets could carry Connection IDs that are longer than
```
@martinthomson pointed out that this isn't the spec where we need to allow larger CIDs to happen. This merely needs to note that it could happen.

-- 
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/2749#pullrequestreview-246802450

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 06 Jun 2019 13:38:27 -0700

janaiyengar approved this pull request.

One change, but looks good otherwise.

-- 
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/2749#pullrequestreview-246802727

---

From: martinduke <[email-masked]>
Date: Thu, 06 Jun 2019 13:40:42 -0700

b21444e25156141077fe8ad246841ca64bb2202e  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/2749/files/d60188e312f34326720c5453f0f156ab2efaa291..b21444e25156141077fe8ad246841ca64bb2202e

---

From: Mark Nottingham <[email-masked]>
Date: Thu, 06 Jun 2019 22:58:57 -0700

~~~
3592: Line is 82 characters; limit is 80
limit, Version Negotiation Packets could carry Connection IDs that are longer than
Makefile:25: recipe for target 'lint' failed
~~~

@martinduke can you fix that up?

-- 
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/2749#issuecomment-499766945

---

From: martinduke <[email-masked]>
Date: Fri, 07 Jun 2019 08:17:53 -0700

ca74837d27795daaa195e7d48a24f5bcf4bc55bc  Fixed formatting error

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

---

From: Kazuho Oku <[email-masked]>
Date: Wed, 12 Jun 2019 17:53:14 -0700

kazuho 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/2749#pullrequestreview-249083998

---

From: Martin Thomson <[email-masked]>
Date: Wed, 12 Jun 2019 17:58:13 -0700

martinthomson approved this pull request.

Thanks for writing this up.  It looks good.

Don't feel that you need to comply with the request below, but if you don't I'll feel obligated to do it myself...

> @@ -143,11 +143,13 @@ version-specific semantics are marked with an X.
-|DCIL(4)|SCIL(4)|
+|    DCIL (8)   |

Maybe not for this PR, but something to consider...

"DCIL" was chosen to fit in the small space available in the diagram.  The new field is a little wider and could fit `DCID Len (8)` comfortably.

-- 
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/2749#pullrequestreview-249084524

---

From: martinduke <[email-masked]>
Date: Tue, 25 Jun 2019 09:38:59 -0700

cf619677cbbedd4e18d8171c03dca222c40b350a  v1 CID limit of 20 Bytes + MT's nit

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

---

From: ianswett <[email-masked]>
Date: Tue, 25 Jun 2019 09:45:59 -0700

ianswett approved this pull request.

> -is also 0 bytes in length.  Non-zero encoded lengths are increased by 3 to get
-the full length of the connection ID; the final value is therefore either 0 or
-between 4 and 18 bytes in length (inclusive).  For example, an byte with the
-value 0xe0 describes a 17 byte Destination Connection ID and a zero byte Source
-Connection ID.
-
-The connection ID lengths are followed by two connection IDs.  The connection
-ID associated with the recipient of the packet (the Destination Connection ID)
-is followed by the connection ID associated with the sender of the packet (the
-Source Connection ID).
+The next byte contains the length in bytes of the destination Connection ID
+(see {{connection-id}}) that follows.  The length is encoded as an 8-bit
+unsigned integer. This is followed by the destination Connection ID itself,
+associated with the receiver of the packet.
+
+The next two fields are the Connection ID length for the source and the source

```suggestion
The next two fields are the length in bytes of the source Connection ID and the source
```

-- 
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/2749#pullrequestreview-254134250

---

From: Mike Bishop <[email-masked]>
Date: Tue, 25 Jun 2019 15:16:17 -0700

MikeBishop 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/2749#pullrequestreview-254298714

---

From: Martin Thomson <[email-masked]>
Date: Tue, 25 Jun 2019 15:17:01 -0700

+As future versions of QUIC may support Connection IDs larger than the version 1
+limit, Version Negotiation Packets could carry Connection IDs that are longer

```suggestion
limit, Version Negotiation packets could carry Connection IDs that are longer
```

-- 
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/2749#pullrequestreview-254298177

---

From: Martin Thomson <[email-masked]>
Date: Tue, 25 Jun 2019 15:18:51 -0700

@martinthomson pushed 1 commit.

f65c731f24d43e23e416de87103871c1f13a4806  Merge branch 'master' into master

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

---

From: martinduke <[email-masked]>
Date: Tue, 25 Jun 2019 15:22:32 -0700

43ad86c009e578a857ec71d0563574282bc07625  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/2749/files/f65c731f24d43e23e416de87103871c1f13a4806..43ad86c009e578a857ec71d0563574282bc07625

---

From: martinduke <[email-masked]>
Date: Tue, 25 Jun 2019 15:24:05 -0700

I am happy to commit this, but it's 'outdated'. Can you update?

-- 
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/2749#discussion_r297418208