Subject: Re: [quicwg/base-drafts] Limit RCID state (#3547)

From: martinduke <[email-masked]>
Date: Wed, 01 Apr 2020 10:12:48 -0700

@martinduke commented on this pull request.

> @@ -1069,6 +1069,15 @@ to cease using the connection IDs when requested can result in connection
 failures, as the issuing endpoint might be unable to continue using the
 connection IDs with the active connection.
 
+An endpoint SHOULD limit the number of outstanding RETIRE_CONNECTION_ID frames
+to bound the necessary state. In order to allow a peer to retire all previously
+issued connection IDs, the limit on the number of outstanding
+RETIRE_CONNECTION_IDs SHOULD be at least the active_connection_id_limit. An

@kazuho I believe my psuedocode indicates that the current version of the PR doesn't add more state for various values of "maximum RCIDs inflight" as long as it's small enough that you can fit it all in one QUIC packet. If the RCIDs are spread over multiple packets inflight then keeping track of RCIDs requires a scoreboard.

The exact threshold to throw an error is indeed not needed in the spec. However @ianswett, rightfully, requested a MAY around the error to set expectations for debugging.

I do think we should change the word "outstanding" to "in flight" just to make it a bit clearer.

-- 
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/3547#discussion_r401776620

---

From: ianswett <[email-masked]>
Date: Wed, 01 Apr 2020 11:56:57 -0700

@ianswett commented on this pull request.

in flight or unacknowledged would both be fine with me.  One negative of "in flight" is that if you're congestion blocked, you can't yet put the RCID in flight.

-- 
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/3547#discussion_r401838811

---

From: martinduke <[email-masked]>
Date: Wed, 01 Apr 2020 12:23:19 -0700

unacknowledged is 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/3547#discussion_r401853603

---

From: martinduke <[email-masked]>
Date: Wed, 01 Apr 2020 12:26:00 -0700

"unacknowledged" would also include long-lost packets, technically. I agree with the congestion control observation, but I'm not sure how it impacts the behavior. I'll _always_ withhold RCIDs that are cwnd-blocked; it's a restriction on RCIDs in flight independent of this one.

-- 
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/3547#discussion_r401855144

---

From: martinduke <[email-masked]>
Date: Wed, 01 Apr 2020 16:49:06 -0700

@martinduke pushed 1 commit.

6332cedcf7026fdcc1e0c36bca395d718344bae1  s/outstanding/in flight

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3547/files/33548fd46adf3885542bbbe87ee2498bd2b92d75..6332cedcf7026fdcc1e0c36bca395d718344bae1

---

From: martinduke <[email-masked]>
Date: Wed, 01 Apr 2020 16:53:44 -0700

beca69c7c49847bc0d473c681139bfaa9e882f57  update the rest of the text

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3547/files/6332cedcf7026fdcc1e0c36bca395d718344bae1..beca69c7c49847bc0d473c681139bfaa9e882f57

---

From: Kazuho Oku <[email-masked]>
Date: Wed, 01 Apr 2020 16:54:26 -0700

@kazuho commented on this pull request.

@martinduke I agree that when restricted by CWND, we cannot send a frame. Therefore my view is that it is difficult or confusing to call a frame that is not yet being set as "inflight" or "outstanding." Both words are wrong IMO.

That the reason why my text above talks about "state retained for retiring connection IDs."

> The exact threshold to throw an error is indeed not needed in the spec. However @ianswett, rightfully, requested a MAY around the error to set expectations for debugging.

While I am not against making such a suggestion using MAY in text, if we are to make a suggestion, I think we'd need to be very cautious so that the limit would not be too small.

-- 
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/3547#discussion_r401976201

---

From: martinduke <[email-masked]>
Date: Wed, 01 Apr 2020 17:02:43 -0700

OK -- there has been an enormous amount of discussion. The current state of the PR is that all RCIDs will eventually be sent, but if there are more than active_connection_id_limit, some may be delayed by an RTT or more. If it gets out of hand the RCID sender can throw a connection error.

I believe this reflects what @martinthomson articulated a few dozen messages ago, and in the thread above I have pseudocode for a lightweight implementation of this (i.e. essentially no additional resources for an arbitrarily large backlog of RCIDs).

The advantage of this approach is that it is lightweight and is very subtle change to behavior that will not will not affect interoperability at all or break the existing contract of a Retire Prior To.

There are two alternatives that have some traction:
1) Just bite the bullet and do #3553. This breaks interoperability but is a clean architectural change that addresses the root of the problem. I would be very comfortable with this, and prefer it to other things that break the existing semantics.
2) @kazuho's "leaky" approach that allows endpoints to simply not send some RCIDs at all if there are too many pending. I used to prefer this, but given my existence proof above I'm not as excited about it anymore. It breaks the contract that is in draft-27 instead of merely bending it.

I hope that covers the decision space. I would like to put this issue out of its misery.

-- 
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/3547#issuecomment-607547785

---

From: martinduke <[email-masked]>
Date: Wed, 01 Apr 2020 17:11:54 -0700

Perhaps I'm missing something obvious. The endpoint can say "I will only have 10 RCIDs in flight at a time". So if the peer makes me send 20, it'll take me at least 2 RTTs to resolve. Now obviously it can take longer if I don't have enough cwnd, but that's orthogonal to this and no change to the spec. Yes?

I'm not sure why an endpoint would choose a threshold that would result in spurious aborts, but if there's some limit you would suggest I'm happy to add 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/3547#discussion_r401981466

---

From: Kazuho Oku <[email-masked]>
Date: Wed, 01 Apr 2020 17:18:02 -0700

@kazuho requested changes on this pull request.

+An endpoint SHOULD limit the number of in flight RETIRE_CONNECTION_ID frames
+to bound the necessary state. In order to minimize delay in common situations,
+the limit on the number of in flight RETIRE_CONNECTION_IDs SHOULD be at least
+the active_connection_id_limit. An endpoint MAY choose to treat having too many
+connection IDs in need of retirement as a connection error of type
+CONNECTION_ID_LIMIT_ERROR.

If we are to suggest that endpoints MAY immediately close the connection (an action that leads to loss of HTTP requests / responses), we should give a specific advice on when it would be safe to do so. The only number we provide in this paragraph is `active_connection_id_limit`, but we agree that it is too small to be used for closing the connection. We should be clear about that.

+An endpoint SHOULD limit the number of in flight RETIRE_CONNECTION_ID frames
+to bound the necessary state. In order to minimize delay in common situations,
+the limit on the number of in flight RETIRE_CONNECTION_IDs SHOULD be at least
+the active_connection_id_limit. An endpoint MAY choose to treat having too many

For the first two sentences, I think we should better use phrases like "state required to track the CIDs being retired." I also think that we should provide guidance on choosing the limit, which should be greater than active_connection_id_limit (see #3509).

"in flight" is confusing, as people might read it as if it excludes sequence number of retired CIDs that are to be sent but cannot be sent immediately. There is no guarantee that RCID frames for all the sequence numbers to be retired can be sent immediately as the frame is under the control of CC. Note that there would be an incentive for people to consider that "in flight" here excludes those yet to be sent, because is the next setence we talk about "having too many in need of retirement." That gives the impression that "in flight" means something narrower.

-- 
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/3547#pullrequestreview-386027439

---

From: martinduke <[email-masked]>
Date: Wed, 01 Apr 2020 17:19:28 -0700

I'm happy to include whatever minimum you suggest.

-- 
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/3547#discussion_r401983687

---

From: martinduke <[email-masked]>
Date: Wed, 01 Apr 2020 17:20:31 -0700

it does exclude ones that can't be sent; the point is, even if there is space in the cwnd I can choose to send fewer to simplify the implementation.

-- 
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/3547#discussion_r401983951

---

From: Kazuho Oku <[email-masked]>
Date: Wed, 01 Apr 2020 17:23:05 -0700

> The endpoint can say "I will only have 10 RCIDs in flight at a time".

Yes. That's possible, but having that limitation does not mitigate the attack.

> So if the peer makes me send 20, it'll take me at least 2 RTTs to resolve. Now obviously it can take longer if I don't have enough cwnd, but that's orthogonal to this and no change to the spec.

The attack is that peer might make you send more than 20 or more. And we need to either provide a guidance on when an endpoint can start leaking retirements or close the connection.

However, the only SHOULD being added in this PR is something unrelated to the attack. And the location of the text gives the false impression that the limit in that sentence is the threshold for mitigating the 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/3547#discussion_r401984660

---

From: Martin Thomson <[email-masked]>
Date: Wed, 01 Apr 2020 17:34:48 -0700

@martinthomson commented on this pull request.

I could live with saying "at least twice the active_connection_id_limit" in the first sentence.

-- 
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/3547#discussion_r401987997

---

From: Martin Thomson <[email-masked]>
Date: Wed, 01 Apr 2020 17:42:21 -0700

```suggestion
When retiring a connection ID, an endpoint remembers a sequence number and
maybe a packet number for a RETIRE_CONNECTION_ID frame.  An endpoint SHOULD
limit the state it commits to remembering connection IDs that need to be
retired to at least twice the active_connection_id_limit. An endpoint MAY
choose to treat having too many connection IDs in need of retirement as a
connection error of type CONNECTION_ID_LIMIT_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/3547#pullrequestreview-386041069

---

From: Kazuho Oku <[email-masked]>
Date: Wed, 01 Apr 2020 17:51:07 -0700

If it excludes those that can't be sent, then such an endpoint is vulnerable to the attack. Unless the amount of state that is used to track those that can't be sent is bounded. We need to recommend endpoints to limit _this_ state (see https://github.com/quicwg/base-drafts/pull/3547#discussion_r401984660), not the number of RCID frames sent at once.

-- 
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/3547#discussion_r401992408

---

From: Kazuho Oku <[email-masked]>
Date: Wed, 01 Apr 2020 18:13:01 -0700

This suggestion looks good to me. I think we might want to clarify why the recommendation is at least twice (e.g., so that endpoints can track retirements it issued and those requested by the peer even under packet loss), but that's merely an editorial preference.

-- 
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/3547#discussion_r401998245

---

From: martinduke <[email-masked]>
Date: Tue, 07 Apr 2020 16:16:28 -0700

I have received feedback from the list and the editors that "the lag" is the way to go. Unless that consensus is reversed, I believe that @martinthomson 's suggestion is too easily misinterpreted as allowing "the leak".  While I believe this can be fixed, I'll make a new suggestion that might be a better basis to making progress on what now is an editorial problem.

-- 
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/3547#discussion_r405168193

---

From: martinduke <[email-masked]>
Date: Tue, 07 Apr 2020 16:21:42 -0700

I'm reluctant to blow away the whole PR with brand new text, so in this comment I will propose four entirely new paragraphs that IMO says what needs to be said. Either the current PR, Martin's revision, or this proposal could serve as the basis of the eventual change:

If a peer sends large numbers of NEW_CONNECTION_ID frames that increase Retire
Prior To, and/or acks of packets that contain RETIRE_CONNECTION_ID are lost, the
state required at the RETIRE_CONNECTION_ID sender can grow without regard to its
active_connection_id_limit. Therefore, endpoints SHOULD take steps to bound the
state associated with needed RETIRE_CONNECTION_ID frames while ensuring that it
eventually transmits all required RETIRE_CONNECTION_ID frames.

For example, it might limit voluntarily retirement of sequence numbers if it has
not received enough acknowledgments of packets containing previous retirements.
It might also restrict the RETIRE_CONNECTION_ID frames in flight to a single
packet in order to simplify tracking of what is in flight, what needs retransmission,
and what has been acknowledged.

An endpoint MAY treat having too many connection IDs to retire as a connection
error of type CONNECTION_ID_LIMIT_ERROR. The threshold for this error SHOULD
be at least twice the endpoint's advertised active_connection_id_limit.

Endpoints SHOULD NOT issue updates of the Retire Prior To field prior to
receiving all of the RETIRE_CONNECTION_ID frames for the previous update to
Retire Prior To.

****

If people much prefer this or MT's revision, I'll update the PR accordingly.

-- 
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/3547#issuecomment-610667224

---

From: Martin Thomson <[email-masked]>
Date: Tue, 07 Apr 2020 17:21:19 -0700

```suggestion
When retiring a connection ID, an endpoint remembers a sequence number and maybe
a packet number for a RETIRE_CONNECTION_ID frame.  An endpoint SHOULD limit the
state it commits by limiting the number of connection IDs that it tracks for
retirement to at least twice the active_connection_id_limit. An endpoint MUST
NOT forget a connection ID without retiring it, though MAY choose to treat
having too many connection IDs in need of retirement as a connection error of
type CONNECTION_ID_LIMIT_ERROR.
```

I think that this is better than including a lot more words.  I've tweaked the last sentence to include a prohibition on forgetting connection IDs.

-- 
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/3547#discussion_r405188285

---

From: Kazuho Oku <[email-masked]>
Date: Tue, 07 Apr 2020 18:43:35 -0700

I'm fine with @martinthomson's proposed text. 

It gives a recommendation, and also provides a dedicated error code to be used in such case. When a well-behaving endpoint receives that error code, it can blame that the peer is doing something wrong.

I think that this is the minimal fix that we need to apply, and I think that it is a reasonable way of making progress at late stage. If we are to apply a proper fix, my preference still goes to adopting #3553. It removes the need for "lagging," therefore, provides us having less complexity in the long run, at the cost of requiring a wire-format change.

-- 
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/3547#discussion_r405210341

---

From: martinduke <[email-masked]>
Date: Tue, 07 Apr 2020 19:10:07 -0700

495a97423b8b0bb6411f14a533408a6cfbc7fb29  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/3547/files/beca69c7c49847bc0d473c681139bfaa9e882f57..495a97423b8b0bb6411f14a533408a6cfbc7fb29

---

From: martinduke <[email-masked]>
Date: Tue, 07 Apr 2020 19:11:41 -0700

I took @martinthomson 's suggested text. I would prefer a little advice on how to implement the lag, but I can live with this text.

-- 
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/3547#issuecomment-610712177

---

From: Kazuho Oku <[email-masked]>
Date: Tue, 07 Apr 2020 19:55:45 -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/3547#pullrequestreview-389620749

---

From: ianswett <[email-masked]>
Date: Wed, 08 Apr 2020 06:57:17 -0700

@ianswett approved this pull request.

One suggested clarification, but LGTM.

> +When retiring a connection ID, an endpoint remembers a sequence number and maybe
+a packet number for a RETIRE_CONNECTION_ID frame.  An endpoint SHOULD limit the
+state it commits by limiting the number of connection IDs that it tracks for

```suggestion
An endpoint SHOULD limit the
state it commits to retiring connection IDs using RETIRE_CONNECTION_ID by limiting the number of connection IDs that it tracks 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/3547#pullrequestreview-389997923

---

From: martinduke <[email-masked]>
Date: Wed, 08 Apr 2020 14:15:54 -0700

54e2a1207f926fc32ce23769368a06bbabfd0de1  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/3547/files/495a97423b8b0bb6411f14a533408a6cfbc7fb29..54e2a1207f926fc32ce23769368a06bbabfd0de1

---

From: ianswett <[email-masked]>
Date: Wed, 08 Apr 2020 16:35:05 -0700

> @@ -1069,9 +1069,8 @@ to cease using the connection IDs when requested can result in connection
 
-When retiring a connection ID, an endpoint remembers a sequence number and maybe
-a packet number for a RETIRE_CONNECTION_ID frame.  An endpoint SHOULD limit the
-state it commits by limiting the number of connection IDs that it tracks for
+An endpoint SHOULD limit the
+state it commits to retiring connection IDs using RETIRE_CONNECTION_ID by limiting the number of connection IDs that it tracks for

```suggestion
state it commits to retiring connection IDs using RETIRE_CONNECTION_ID by
limiting the number of connection IDs that it tracks 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/3547#pullrequestreview-390400218

---

From: ianswett <[email-masked]>
Date: Wed, 08 Apr 2020 16:35:37 -0700

@ianswett pushed 1 commit.

2dea527f716acf2f8a424dd300bd395839b6991d  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/3547/files/54e2a1207f926fc32ce23769368a06bbabfd0de1..2dea527f716acf2f8a424dd300bd395839b6991d

---

From: ianswett <[email-masked]>
Date: Wed, 08 Apr 2020 16:37:03 -0700

> +An endpoint SHOULD limit the
+state it commits to retiring connection IDs using RETIRE_CONNECTION_ID by
+limiting the number of connection IDs that it tracks for

```suggestion
An endpoint SHOULD limit the state it commits to retiring connection IDs
using RETIRE_CONNECTION_ID by limiting the number of connection IDs that it tracks 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/3547#pullrequestreview-390400900

---

From: ianswett <[email-masked]>
Date: Wed, 08 Apr 2020 16:38:06 -0700

+state it commits to retiring connection IDs using RETIRE_CONNECTION_ID by
+limiting the number of connection IDs that it tracks for
+retirement to at least twice the active_connection_id_limit. An endpoint MUST
+NOT forget a connection ID without retiring it, though MAY choose to treat
+having too many connection IDs in need of retirement as a connection error of
+type CONNECTION_ID_LIMIT_ERROR.

```suggestion
An endpoint SHOULD limit the state it commits to retiring connection IDs
using RETIRE_CONNECTION_ID by limiting the number of connection IDs that
it tracks for retirement to at least twice the active_connection_id_limit. An
endpoint MUST NOT forget a connection ID without retiring it, though MAY
choose to treat having too many connection IDs in need of retirement as a
connection error of type CONNECTION_ID_LIMIT_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/3547#pullrequestreview-390401240

---

From: ianswett <[email-masked]>
Date: Wed, 08 Apr 2020 16:38:12 -0700

4393b6dfc35064cc79493021ac6b80d460ad175f  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/3547/files/2dea527f716acf2f8a424dd300bd395839b6991d..4393b6dfc35064cc79493021ac6b80d460ad175f

---

From: Martin Thomson <[email-masked]>
Date: Wed, 08 Apr 2020 17:39:45 -0700

@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/3547#pullrequestreview-390421255

---

From: martinduke <[email-masked]>
Date: Wed, 08 Apr 2020 17:41:45 -0700

f436727acd7dcd49d613cac8ec032a9934c3f6ef  fixed line endings

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3547/files/4393b6dfc35064cc79493021ac6b80d460ad175f..f436727acd7dcd49d613cac8ec032a9934c3f6ef