Subject: [quicwg/base-drafts] enforce the active_connection_id_limit (#3201)

From: Marten Seemann <[email-masked]>
Date: Wed, 06 Nov 2019 18:16:21 -0800

Fixes #3193.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * forbid exceeding the peer&#39;s active_connection_id_limit
  * add an error code for exceeding the connection ID limit

-- File Changes --

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

-- Patch Links --

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

---

From: ianswett <[email-masked]>
Date: Thu, 07 Nov 2019 06:46:59 -0800

ianswett 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/3201#pullrequestreview-313390713

---

From: Mike Bishop <[email-masked]>
Date: Thu, 07 Nov 2019 07:34:47 -0800

MikeBishop requested changes on this pull request.

I think we need to fix #3197 at the same time, or this creates broken corner-cases.

-- 
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/3201#pullrequestreview-313427633

---

From: Eric Kinnear <[email-masked]>
Date: Thu, 07 Nov 2019 10:32:00 -0800

erickinnear approved this pull request.

Looks good! Let's definitely also resolve #3197 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/3201#pullrequestreview-313547668

---

From: Marten Seemann <[email-masked]>
Date: Thu, 07 Nov 2019 19:34:38 -0800

@marten-seemann pushed 1 commit.

8ed69b0c6393dca4a876ee73c866b69a7a2a3e75  set a default and minimum value for active_connection_id_limit

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3201/files/1e286a042ef691a58f38c6b63b6339fdf9b9d14b..8ed69b0c6393dca4a876ee73c866b69a7a2a3e75

---

From: Marten Seemann <[email-masked]>
Date: Thu, 07 Nov 2019 19:44:10 -0800

@erickinnear @MikeBishop As discussed in #3197 I added a minimum value of 2 for the TP, since values below this don't make any sense. 
Having an invalid default value seems wrong to me, so I also made 2 the default value for the TP. People might disagree if this a reasonable value.

-- 
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/3201#issuecomment-551371943

---

From: Marten Seemann <[email-masked]>
Date: Thu, 07 Nov 2019 23:55:14 -0800

8354804a2a30580fd1b7deadb19f3baf3ad68fd8  mandate the processing order of NEW_CONNECTION_ID frame fields

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3201/files/8ed69b0c6393dca4a876ee73c866b69a7a2a3e75..8354804a2a30580fd1b7deadb19f3baf3ad68fd8

---

From: Mike Bishop <[email-masked]>
Date: Fri, 08 Nov 2019 09:26:59 -0800

MikeBishop commented on this pull request.

> @@ -999,8 +999,10 @@ retired are considered active; any active connection ID can be used.
 An endpoint SHOULD ensure that its peer has a sufficient number of available and
 unused connection IDs.  Endpoints store received connection IDs for future use
 and advertise the number of connection IDs they are willing to store with the
-active_connection_id_limit transport parameter.  An endpoint SHOULD NOT provide
-more connection IDs than the peer's limit.
+active_connection_id_limit transport parameter.  An endpoint MUST NOT provide
+more connection IDs than the peer's limit.  An endpoint that receives more
+connection IDs than its advertised active_connection_id_limit MUST close the
+connection with an error of type CONNECTION_ID_LIMIT_ERROR.

"MUST error" requires tracking.  It's possible that an implementation might decide to drop some CIDs and reduce the number it keeps without closing the connection.  "SHOULD/MAY error" provides the same deterrent effect for the issuer's misbehavior without placing a requirement on the receiver.

-- 
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/3201#pullrequestreview-314246383

---

From: Marten Seemann <[email-masked]>
Date: Fri, 08 Nov 2019 23:05:30 -0800

marten-seemann commented on this pull request.

We're requiring other limit checks (e.g. for STREAM_LIMIT_ERROR and for FLOW_CONTROL_ERROR), so I'd prefer to keep this a requirement as well.

I don't think dropping should be regarded as acceptable behavior (although you can't prevent implementations from doing it), since a CID issued by an endpoint consumes state on that endpoint as well.

-- 
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/3201#discussion_r344432267

---

From: Kazuho Oku <[email-masked]>
Date: Sun, 10 Nov 2019 21:30:06 -0800

kazuho commented on this pull request.

This text indicates that the number of "active_connection_id_limit" means the number of "stored" CIDs "for future use." It might be better to address this now. See #3200.

-- 
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/3201#pullrequestreview-314701136

---

From: Marten Seemann <[email-masked]>
Date: Sat, 16 Nov 2019 20:11:00 -0800

912634dbeddd6dbe910a05694828db4b2fc67b3a  include the handshake and SPA CIDs in the connection ID limit

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3201/files/8354804a2a30580fd1b7deadb19f3baf3ad68fd8..912634dbeddd6dbe910a05694828db4b2fc67b3a

---

From: Marten Seemann <[email-masked]>
Date: Sat, 16 Nov 2019 20:14:15 -0800

@kazuho I updated the PR such that the active_connection_id_limit now included the handshake and SPA CID, which should make it a fix for #3200 now. PTAL.

-- 
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/3201#discussion_r347117166

---

From: Marten Seemann <[email-masked]>
Date: Sat, 16 Nov 2019 21:13:14 -0800

a11afffde6f7d8a1663baf21877b1b87449c2f41  rephrase when to supply new CIDs

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3201/files/912634dbeddd6dbe910a05694828db4b2fc67b3a..a11afffde6f7d8a1663baf21877b1b87449c2f41

---

From: Eric Kinnear <[email-masked]>
Date: Sat, 16 Nov 2019 21:18:55 -0800

Latest changes look 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/3201#pullrequestreview-318001317

---

From: Marten Seemann <[email-masked]>
Date: Sat, 16 Nov 2019 21:30:41 -0800

2473c8b0a7b3834498c159403a6dfca75daaa41e  Merge remote-tracking branch 'upstream/master' into enforce-active-connection-id-limit

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3201/files/a11afffde6f7d8a1663baf21877b1b87449c2f41..2473c8b0a7b3834498c159403a6dfca75daaa41e

---

From: Kazuho Oku <[email-masked]>
Date: Sat, 16 Nov 2019 23:43:48 -0800

> @@ -4590,8 +4595,11 @@ preferred_address (0x000d):
 active_connection_id_limit (0x000e):
 
 : The maximum number of connection IDs from the peer that an endpoint is willing
-  to store. This value includes only connection IDs sent in NEW_CONNECTION_ID
-  frames. If this parameter is absent, a default of 0 is assumed.
+  to store. This value includes connection IDs sent in NEW_CONNECTION_ID frames,
+  the connection ID used during the handshake as well as the connection ID
+  provided in the preferred_address transport parameter.
+  If the active_connection_id_limit parameter is absent, a default of 2 is
+  assumed. Values below 2 are invalid.

The default and minimal values of 2 look odd, when using a zero-length CID.

Maybe rephrase to:
> Unless a zero-length connection ID is being used, the value of the active_connection_id_limit parameter MUST be no less than 2. If the active_connection_id_limit parameter is absent, a default of 2 is
   assumed.
> 
> When a zero-length connection ID is being used, the active_connection_id_limit parameter MUST NOT be sent.

The rationale behind the last sentence is that it disputable if we consider CIDL=0 as the absence of CID or as the use of zero-length CID. The value of active_connection_id_limit becomes zero or one based on the view you choose. Hence my preference to require absence of the TP when a zero-length CID is used.

-- 
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/3201#pullrequestreview-318005163

---

From: Marten Seemann <[email-masked]>
Date: Sun, 17 Nov 2019 23:59:54 -0800

750caeac14e33b7bfa362d54d4804f6e2be69e5b  no active_connection_id_limit when using zero-length CIDs

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3201/files/2473c8b0a7b3834498c159403a6dfca75daaa41e..750caeac14e33b7bfa362d54d4804f6e2be69e5b

---

From: Marten Seemann <[email-masked]>
Date: Mon, 18 Nov 2019 00:00:06 -0800

Thanks for catching this. I updated the PR.

-- 
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/3201#discussion_r347238838

---

From: Jana Iyengar <[email-masked]>
Date: Mon, 18 Nov 2019 01:24:43 -0800

janaiyengar commented on this pull request.

I'm in agreement with @MikeBishop -- I would suggest changing this to a SHOULD error. This is different than the flow control errors, since an endpoint does not have to store all connection IDs that have been received.

-- 
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/3201#discussion_r347270154

---

From: Kazuho Oku <[email-masked]>
Date: Mon, 18 Nov 2019 01:31:09 -0800

I might suggest having a design discussion in the issue (i.e. #3193) rather than here. There has been certain amount of discussion in that issue, and it seems to me that we have preference for MUST NOT.

-- 
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/3201#discussion_r347273316

---

From: Eric Kinnear <[email-masked]>
Date: Mon, 18 Nov 2019 01:33:01 -0800

erickinnear commented on this pull request.

I'm not sure that I'm against the SHOULD, but I think the idea behind enforcing this was that an endpoint would indeed now need to store every CID it receives, and if it's not willing to store X amount, then it shouldn't have advertised a limit of X.

And even if you *still* didn't store them all, you technically don't need to store them to keep track of how many you've been issued, which you'll probably need in order to cope with getting Retire Prior To anyways.

-- 
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/3201#discussion_r347274181

---

From: Kazuho Oku <[email-masked]>
Date: Mon, 18 Nov 2019 01:33:18 -0800

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/3201#pullrequestreview-318195357

---

From: Jana Iyengar <[email-masked]>
Date: Mon, 18 Nov 2019 01:33:33 -0800

janaiyengar requested changes on this pull request.

A few comments. I think the enforcement is useful, but requiring it is not necessary.

> @@ -1033,10 +1036,12 @@ longer plans to use that address.
 
 An endpoint can cause its peer to retire connection IDs by sending a
 NEW_CONNECTION_ID frame with an increased Retire Prior To field.  Upon receipt,
-the peer MUST retire the corresponding connection IDs using RETIRE_CONNECTION_ID
-frames.  Failure to retire the connection IDs within approximately one PTO can
-cause packets to be delayed, lost, or cause the original endpoint to send a
-stateless reset in response to a connection ID it can no longer route correctly.
+the peer MUST first retire the corresponding connection IDs using
+NEW_CONNECTION_ID frames and then add the newly provided connection ID to the

```suggestion
RETIRE_CONNECTION_ID frames and then add the newly provided connection ID to the
```

> @@ -4590,8 +4595,14 @@ preferred_address (0x000d):
 
 : The maximum number of connection IDs from the peer that an endpoint is willing
-  to store. This value includes only connection IDs sent in NEW_CONNECTION_ID
-  frames. If this parameter is absent, a default of 0 is assumed.
+  to store. This value includes connection IDs sent in NEW_CONNECTION_ID frames,
+  the connection ID used during the handshake as well as the connection ID
+  provided in the preferred_address transport parameter.
+  Unless a zero-length connection ID is being used, the value of the
+  active_connection_id_limit parameter MUST be no less than 2. If this
+  transport parameter is absent, a default of 2 is assumed.
+  When a zero-length connection ID is being used, the active_connection_id_limit
+  parameter MUST NOT be sent.

I think this change is unnecessary and detrimental. Why is this useful?

-- 
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/3201#pullrequestreview-318191803

---

From: Marten Seemann <[email-masked]>
Date: Mon, 18 Nov 2019 09:36:12 +0000 (UTC)

Can you elaborate which part you find detrimental?

-- 
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/3201#discussion_r347275680

---

From: Marten Seemann <[email-masked]>
Date: Mon, 18 Nov 2019 01:37:42 -0800

569b660eb85bf55bf7a226037da73057afd86ddf  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/3201/files/750caeac14e33b7bfa362d54d4804f6e2be69e5b..569b660eb85bf55bf7a226037da73057afd86ddf

---

From: Kazuho Oku <[email-masked]>
Date: Mon, 18 Nov 2019 01:37:58 -0800

@janaiyengar PTAL at https://github.com/quicwg/base-drafts/pull/3201#discussion_r347122022.

-- 
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/3201#discussion_r347276619

---

From: Eric Kinnear <[email-masked]>
Date: Mon, 18 Nov 2019 01:42:14 -0800

(Would you rather it was ignored rather than not sent?)

-- 
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/3201#discussion_r347278701

---

From: Jana Iyengar <[email-masked]>
Date: Mon, 18 Nov 2019 17:43:40 -0800

Having chatted about this with @erickinnear and @kazuho, I'm ok with this set of changes. A few suggestions.

I'm ok with this being a MUST. I would suggest that you explicitly add a note saying that an endpoint is expected to store enough information all received CIDs to be able to generate this error. It might be redundant, but that's ok.

> @@ -999,12 +999,15 @@ retired are considered active; any active connection ID can be used.
 An endpoint SHOULD ensure that its peer has a sufficient number of available and
 unused connection IDs.  Endpoints store received connection IDs for future use
 and advertise the number of connection IDs they are willing to store with the
-active_connection_id_limit transport parameter.  An endpoint SHOULD NOT provide
-more connection IDs than the peer's limit.
-
-An endpoint SHOULD supply a new connection ID when it receives a packet with a
-previously unused connection ID or when the peer retires one, unless providing
-the new connection ID would exceed the peer's limit.  An endpoint MAY limit the
+active_connection_id_limit transport parameter.  An endpoint MUST NOT provide
+more connection IDs than the peer's limit.  An endpoint that receives more
+connection IDs than its advertised active_connection_id_limit MUST close the
+connection with an error of type CONNECTION_ID_LIMIT_ERROR.
+
+An endpoint SHOULD supply a new connection ID when the peer retires a connection
+ID. If an endpoint provided fewer connection IDs than the peer's
+active_connection_id_limit, it MAY supply a new connection ID when it receives
+a packet with a previously unused connection ID.  An endpoint MAY limit the

Is this MAY necessary? It's certainly not disallowed. I understand how this could be useful, but it seems like a purely server-side optimization, which doesn't need to be specified. Alternatively, how about something like: "An endpoint can but is not required to send as many connection IDs as the peer can store."

Suggested edit: "This value includes the connection ID received during the handshake, that received in the preferred_address transport parameter, and those received in NEW_CONNECTION_ID frames."

I said detrimental because we used to not encode values on the wire that were illegal ... but we've walked away from that in other places too :-/ I'm ok with 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/3201#pullrequestreview-318734402

---

From: Christian Huitema <[email-masked]>
Date: Mon, 18 Nov 2019 18:54:21 -0800

huitema commented on this pull request.

I don't think this shhould be MUST. For every other such action, the paradigm is "the sender MUST NOT do this silly stuff, the responder MAY close with connection 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/3201#discussion_r347710308

---

From: Marten Seemann <[email-masked]>
Date: Mon, 18 Nov 2019 18:57:11 -0800

@huitema That's not correct:
> Endpoints MUST NOT exceed the limit set by their peer. An endpoint that receives a frame with a stream ID exceeding the limit it has sent MUST treat this as a connection error of type STREAM_LIMIT_ERROR 

and 
>  An endpoint MUST terminate a connection with a FLOW_CONTROL_ERROR error if it receives more data than the maximum data value that it has sent

-- 
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/3201#discussion_r347710819

---

From: Marten Seemann <[email-masked]>
Date: Mon, 18 Nov 2019 23:41:55 -0800

This sentence goes back to @kazuho's issue https://github.com/quicwg/base-drafts/issues/2436.

-- 
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/3201#discussion_r347768589

---

From: Marten Seemann <[email-masked]>
Date: Mon, 18 Nov 2019 23:45:37 -0800

e60e94e5aafe83d4e64bb8d86a1ae68e81eff4ad  apply Jana's suggestion

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3201/files/569b660eb85bf55bf7a226037da73057afd86ddf..e60e94e5aafe83d4e64bb8d86a1ae68e81eff4ad

---

From: Marten Seemann <[email-masked]>
Date: Mon, 18 Nov 2019 23:46:29 -0800

Applied. Thank you.

-- 
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/3201#discussion_r347769960

---

From: Martin Thomson <[email-masked]>
Date: Wed, 27 Nov 2019 17:00:09 -0800

I'm seeing some people asking for changes, but I am happy with Marten's responses to those.  I'm going to mark the associated issues as `proposal-ready`.

-- 
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/3201#issuecomment-559300430

---

From: Christian Huitema <[email-masked]>
Date: Fri, 29 Nov 2019 13:26:41 -0800

@marten-seemann The examples that you quote are cases where there is no reasonable way to continue the connection. I don't think that excess number of NEW CONNECTION ID is in the same class -- the receiving peer could also just drop them without retiring them.

-- 
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/3201#discussion_r352246398

---

From: Marten Seemann <[email-masked]>
Date: Fri, 29 Nov 2019 18:24:06 -0800

@huitema That doesn’t work well with Retire Prior To, since you still MUST retire that connection ID. So you need to track at least the sequence number of the CID.
It seems like you’re trying to build a way for a peer to (silently) revoke the limit it announced in its transport parameters. Building such a mechanism is possible (we’d need something like a MAX_CID frame), but we decided to not do this, mostly because we assumed that the number of requested connection IDs will be small (O(10)). If that argument still holds, I don’t think it’s too much to ask for to keep track of O(10) CID sequence numbers.

-- 
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/3201#discussion_r352262691