Request to Retire Locally Issued CIDs

[nibanks]

Adds a new (optional) field, 'Retire Prior To' (placeholder name), to the NEW_CONNECTION_ID frame, which allows a sender to request that its peer immediately retire all connection IDs up to a certain sequence number. Once the peer acknowledges this packet, it has at most 3 PTO to follow through or risk the sender closing the connection.

Fixes #2645.

[ianswett]

I'd like to make it a protocol violation for the gap between the CID sequence number and the "Retire Prior To" value being more than the active_connection_id_limit specified in the transport params. Previously people said implementing that was too difficult, but having both values in a single frame make it pretty easy to get right and detect when it's violated.

@marten-seemann

[martinthomson]

I really don't like the fact that this is a field on NEW_CONNECTION_ID rather than a new frame type that just carries the proposed "Retire Prior To" field. This change renumbers a bunch of frame types just to make this part of a frame that already exists.

The cost of a separate frame type is that you don't have an atomic assurance that a connection ID is available. That's OK. I mean, this PR doesn't even require that Sequence Number be equal to or greater than Retire Prior To, so clearly that isn't a concern that is top-of-mind. I would prefer to instead reiterate that the recipient of a "RETIRE_ALL_CONNECTION_IDS_WITH_SEQUENCE_NUMBERS_BEFORE" frame can't retire connection IDs until it has a connection ID that it can use (which is basically what we say elsewhere anyway).

[nibanks]

First, I don't see the frame type rearrange as a very large change at all. Second, I do think the atomic assurance is important, and it's just an oversight that we aren't explicitly require Sequence Number to be greater. I didn't think of that scenario.

Without the atomic assurance, I worry about the scenario where an endpoint retires all the CIDs without replacing any. It gets a lot more complicated if you have separate frames. What conditions are considered protocol violations?

In order to replace all existing CIDs, the order of the frames in a single packet starts to make a difference. You have to make sure you have the NEW_CONNECTION_ID frame followed by the RETIRE_ALL_CONNECTIONS_IDS_WITH_SEQUENCE_NUMBER_BEFORE frame.

[kazuho]

I think I agree with @martinthomson that we should refrain from renumbering frames (again). One of the issues regarding the proposed renumbering is that the two CONNECTION_CLOSE frame types would not have the same MSB bits; it could be a concern for certain stacks.

OTOH, it is also my view that we do not need a new frame nor a new frame type.

How about just making the "Retire Prior To" a mandatory field of the NEW_CONNECTION_ID frame? The value can be set to zero when retirement is not necessary. We do not expect a lot of NEW_CONNECTION_ID to be inflight; spending one additional byte is IMO a non-issue.

[ianswett]

+1 to @kazuho suggestion of making it mandatory and not renumbering frame types.

[nibanks]

Thanks @ianswett @martinthomson @kazuho for your feedback. I have made some changes, making the Retire Prior To field required, so we only have one frame again, added more 2119 language, cleaned up the text on ignoring reductions in the Retire Prior To field, and made it a protocol violation to have Retire Prior To greater than or equal to Sequence Number.

@ianswett on your request to require the difference between the Retire Prior To field and Sequence Number field to be no greater than active_connection_id_limit, that would mean the sender has to keep track of both what's the highest retired sequence number retired by the peer and what it wants locally, and the max of those two values in this field. I'm not sure the extra complexity for keep track of that is worth the cost. What benefit do you see in that requirement?

@martinthomson on the topic of the handshake and preferred address CIDs, I thought we already had text that implicitly assigned them sequence numbers. Is that not the case? What text beyond that would you like me to add?

[erickinnear]

Thanks for writing this up!
Specific to this text (see additional discussion on the issue):

I don't think that we need any requirement about the distance between RetirePriorTo/SequenceNumber active_connection_id_limit. Setting a super high value would be a bit nonsensical, so some non-normative text explaining why doing that is a complete waste of everyone's time would be good, but it doesn't actually break anything. Saying "hey this is silly, consider that limit when you set this" is probably good enough just to make implementors aware of the foot gun.

To the point about handshake and preferred address CIDs, I think it would be nice to have some text just as a reminder along the lines of "don't forget these have sequence numbers too" to help folks avoid confusion.

[marten-seemann]

I agree that atomicity is important here, if we want to avoid the situation where all connections IDs are retired at the same time. Adding the field to the NEW_CONNECTION_ID frame as @kazuho suggested seems to be the most straightforward way to do so.

I'm not a big fan of the 3 PTO requirement here. I don't see why it shouldn't be possible to retire the connections immediately when receiving the frame. There's a DoS risk here: if an attacker delays a packet with a connection ID that was retired afterwards, it can cause either a stateless reset or even a PROTOCOL_VIOLATION. The better way to enforce the retiring of the connection IDs is to look at packet numbers: The old connection IDs MUST NOT be used in packets with higher packet numbers than the packet that acknowledged the NEW_CONNECTION_ID frame.

[kazuho]

@marten-seemann

The better way to enforce the retiring of the connection IDs is to look at packet numbers: The old connection IDs MUST NOT be used in packets with higher packet numbers than the packet that acknowledged the NEW_CONNECTION_ID frame.

Am I correct in understanding that this is the way key update works? Assuming that is the case, my +1 goes here.

[marten-seemann]

Am I correct in understanding that this is the way key update works? Assuming that is the case, my +1 goes here.

I think that's an implementation decision. You could certainly enforce this by requiring that PN(packet sent with old connection IDs) < PN(first packet received with new connection ID). This doesn't stop a peer from acknowledging the NEW_CONNECTION_ID frame, but never acting on it though.

Another way to implement this would be to enforce that PN(packet sent with old connection ID) < PN(ACK for NEW_CONNECTION_ID frame). This would be a somewhat stricter way, since it doesn't allow the peer to ignore the frame (without inducing artificial packet loss).

[mikkelfj]

The better way to enforce the retiring of the connection IDs is to look at packet numbers: The old connection IDs MUST NOT be used in packets with higher packet numbers than the packet that acknowledged the NEW_CONNECTION_ID frame.

I think this solves the problem since I really dislike that a delayed packet, 3PTO or otherwise, can take down a connection.

However, I also dislike that this is coupled strongly to ACK behaviour. I'm not sure how key updates work but assuming it does not use ACK, ACK handling can run asynchronously in a separate task, and indeed this is how I would architect a high performance implementation.

Then one could send a dedicated frame to acknowledge receiving the retire frame, but that is pointless since any packet using the new CID would have that effect.

So for the time being I don't have a good answer to this.

Altogether, if this facility opens up to DoS attacks and prevents ACK handling from running asynchronously, I think it does more harm than good and should at least be moved to v2 giving more time to consider the implications.

[MikeBishop]

This is straddling the line between "request" (5037) and "require" (998). Pick one. Given that you're going to kill the connection if they don't comply, "request" doesn't seem like a strong enough statement here. But the issuer only MAY kill the connection if the peer doesn't comply, so.... 🤷‍♀️

If connection destruction is the intended result, go stronger. You SHOULD set the timer and blow up the connection if it expires; you MUST retire the CIDs upon receipt of an increased RPT field.

[MikeBishop]

@ianswett, the problem is that the sequence can be sparse. The client isn't required to use the CIDs in order any more, IIRC; so you could have four outstanding CIDs, but your four are sequence numbers (2, 127, 324, 325). Nick wants to expire everything prior to 300 and he's tired of waiting for you to use and discard the two early ones.

[nibanks]

@MikeBishop I'm all for making it immediate and adding stronger language. That's exactly what I wanted to do in the first place. Initially, I was getting push back though, so I relaxed it a bit. I agree it's a bit too wishy-washy in its current form. Unless anyone has reasons why they can't or shouldn't update immediately I will go ahead and strengthen the text.

[MikeBishop]

I think the main reason not to be totally immediate is when you have some distributed processing generating packets. You might not be able to guarantee that every thread/process generating packets can stop using those CIDs immediately on receipt of the signal that the issuer wants them gone. However, we already went through that with the RETIRE_CID discussion -- sending RETIRE is supposed to be the signal that you've hit your sync point and will not use them any more.

[ianswett]

Some small suggestions, but this LG overall.

[ianswett]

Some more small suggestions, but looking close.

[erickinnear]

Thanks for writing this up!
This seems like a pretty good start if we're adding a "Please Retire" field to a NEW_CONNECTION_ID. A few comments and some wording suggestions. I think we also need the requirement to replace all of the retired ones, otherwise we risk closing down to just a single CID every time we retire.

[erickinnear]

Looking good, I think we're getting there!

[erickinnear]

This is looking good (modulo the one extra paragraph at the end)!

[erickinnear]

Latest changes are looking great, thanks for driving this @nibanks!
Let's merge and we can update any further editorial nits when we next do a pass.

[martinthomson]

I had this comment sitting in a buffer.