-
Notifications
You must be signed in to change notification settings - Fork 205
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Better describe the use of Preferred Address #3354
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not totally sold on dissociating the CID from the preferred address, but the rest of this looks like a net improvement. I'll address that one the issue.
Co-Authored-By: Mike Bishop <mbishop@evequefou.be>
Co-Authored-By: Mike Bishop <mbishop@evequefou.be>
…en the serever uses TP.preferred_address
@MikeBishop Thank you for your suggestions. Based on the discussion on #3353, I've pushed ea7c750, that states that a client SHOULD retire either of the two CIDs when the server uses TP.preferred_address. |
draft-ietf-quic-transport.md
Outdated
|
||
When the client finishes migrating to the preferred address, it retires the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not currently required anywhere that I can find, do you want to make this a "SHOULD retire" as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a keen observation. I had assume that we'd state that:
- a connection ID that was used on one path MUST NOT be used on a path created by migration
- a connection ID that becomes unusable SHOULD be retired
But surprisingly, we did not. As these requirements are common to ordinary connection migration and migration by preferred address, I've changed the sections that are shared by the two.
Co-Authored-By: Eric Kinnear <32474881+erickinnear@users.noreply.github.com>
c283314
to
dc1c5f7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this. This text certainly needed a few refinements.
active connection ID that has not been previously used by the peer. | ||
An endpoint MUST use a new connection ID when it initiates connection migration | ||
as described in {{initiating-migration}} or {{preferred-address}}, or when it | ||
probes a new network path as described in {{probing}}, and MUST NOT reuse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The run-on sentence here could be two sentences.
probes a new network path as described in {{probing}}, and MUST NOT reuse | |
probes a new network path as described in {{probing}}. Endpoints MUST NOT reuse |
...but I'm not seeing what the new content here does. It appears to just be a restatement of the previous clause.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with the existing text is that while it says that a new connection ID MUST be used when "initiating" a migration, or probing, it does not forbid reuse of a CID (that has previously used on a different path) once the path is being validated.
That's is why I added the "MUST NOT" without splitting the sentence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "MUST NOT" stands on its own in that case:
An endpoint MUST NOT use a connection ID on a given network path if it has used the same connection ID to send packets on another network path.
The intended meaning of "new" in this text is to imply "previously unused", which might be more clearly stated as:
An endpoint MUST use a connection ID that it has not previously used when it initiates connection migration ({{initiating-migration}}), including using a preferred address ({{preferred-address}}). Similarly, an endpoint MUST use an unused connection when probing a new network path ({{probing}}). An endpoint MUST use a previously-unused connection ID in response to a change in the address of a peer if the packet with the new peer address uses an active connection ID that has not been previously used by the peer. An endpoint MUST NOT [...as above].
This covers all the existing reasons to use a previously unused connection ID. You could restate as a list instead, I guess:
An endpoint MUST use a new connection ID when it:
- migrates to a new network path ({{initiating-migration}}), including migrating to a server's preferred address ({{preferred-address}});
- probes a new network ({{probing}}); or,
- sends a packet on a new network path as a result of responding to the migration of a peer, where the peer uses a connection ID that has not been used on another path.
In all cases, an endpoint MUST NOT use a connection ID on a given network path if it has used the same connection ID to send packets on another network path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do have this text in 5.1.2, which I see you also touched:
As discussed in Section 9.5, each connection ID MUST be used on packets sent from only one local address. An endpoint that migrates away from a local address SHOULD retire all connection IDs used on that address once it no longer plans to use that address.
Is the missing piece that it should also be used for only one remote address, but this is the only scenario in which the server changes address?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. IIUC, the requirements are:
- A client MUST NOT reuse a CID across multiple paths (being identified by the 5 tuple).
- A server MUST NOT reuse a CID across multiple path, with the exception being that it MAY use a CID that has been used on one path on another path, if that new path is opened by a packet sent by a client using DCID for the old path.
We can combine these statements into one, and say that: an endpoint MUST NOT reuse a CID across multiple paths, with the exception being that it MAY use a CID that has been used on one path on another path, if that new path is opened by a packet sent by a client using DCID for the old path.
I think that is the most precise definition I can think of. If the above sounds fine, I can work on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Writing down the results of a discussion in ZRH about stating this in terms of endpoints (not client/server) and addresses (not paths):
- An endpoint MUST NOT reuse a CID when sending from more than one local address
- An endpoint MUST NOT reuse a CID when sending to more than one destination address
With the following exceptions/additions:
- An endpoint SHOULD NOT respond to a newly seen incoming CID with an outgoing CID that it has used before (should since it may not have a new one to use)
- An endpoint MAY skip switching if it has strong reason to believe that the remote endpoint didn't try to change (i.e. if CID didn't change and only port changed, it's likely NAT rebinding), although this exception is only necessary if it doesn't have a new one to use
(Modulo some wording tweaks)
I think this is the current state of the text, just rearranging to collapse the separate requirements into one cluster that's hopefully easy to understand. Will incorporate this into the SPA PR if that seems reasonable to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erickinnear I think "MUST NOT" should be "MUST NOT deliberately" because NAT rebinding.
longer plans to use that address. | ||
packets sent from only one local address, and MUST NOT be used across multiple | ||
paths that are opened intentionally. An endpoint SHOULD retire connection IDs | ||
as they become unusable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new text is less accurate. It is potentially always possible to reuse connection IDs, but what matters is whether the endpoint forsees use of the connection ID. The original text is probably not as good as it could be, but that seems unrelated to the stated goal of the PR.
As this is a restatement of the original text in another section, I would prefer to keep this terse. In fact, I would not use normative language and instead say:
As discussed in {{blah}}, endpoints limit the use of a connection ID to a single network path where possible. Endpoints SHOULD retire connection IDs when no longer actively using the network path on which the connection ID was used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd tend to agree here.
I think the normative text in question is:
Section 9.5 Privacy Implications of Connection Migration
...
An endpoint MUST use a new connection ID if it initiates connection migration as
described in {{initiating-migration}} or probes a new network path as described
in {{probing}}. An endpoint MUST use a new connection ID in response to a
change in the address of a peer if the packet with the new peer address uses an
active connection ID that has not been previously used by the peer.
This seems to me like:
each connection ID MUST be used on packets sent from only one local address
still has value as normative text (although it would likely be better all in one place).
Not sure the new text improves on what's there now, does:
, and MUST NOT be used across multiple paths that are opened intentionally.
add anything to clarity of this point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see later some of this is covered :)
draft-ietf-quic-transport.md
Outdated
Similarly, a client that continues using the original server address SHOULD | ||
retire the connection ID provided by the preferred_address transport parameter. | ||
Retirement of either of these connection IDs notifies the server of the | ||
address the client has chosen. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This creates a much stronger inference than I think we ever formally documented, though it is a reasonable one for sure. If your intent is that the server never see data on the old path, then say that instead:
Once a client has migrated successfully to a network path that uses a server preferred address, it MUST NOT migrate to a path using the address that was used during connection establishment.
Otherwise, the inference you are giving the server to make here is without any support. Once you say this, you can drop this text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah though not a "MUST NOT", that is already stated in the previous paragraph. That said, I think it would be better to merge the two paragraphs, as having two paragraphs describing what a client should do went migration succeeds (or fails) is confusing.
PTAL at commit f268ac4.
Co-Authored-By: Martin Thomson <mt@lowentropy.net>
Co-Authored-By: Martin Thomson <mt@lowentropy.net>
…succeeds (or fails)
@martinthomson Thank you for the review. I think I've made necessary changes, or have responded to your comments. Please take a look and let me know what you think. |
active connection ID that has not been previously used by the peer. | ||
An endpoint MUST use a new connection ID when it initiates connection migration | ||
as described in {{initiating-migration}} or {{preferred-address}}, or when it | ||
probes a new network path as described in {{probing}}, and MUST NOT reuse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "MUST NOT" stands on its own in that case:
An endpoint MUST NOT use a connection ID on a given network path if it has used the same connection ID to send packets on another network path.
The intended meaning of "new" in this text is to imply "previously unused", which might be more clearly stated as:
An endpoint MUST use a connection ID that it has not previously used when it initiates connection migration ({{initiating-migration}}), including using a preferred address ({{preferred-address}}). Similarly, an endpoint MUST use an unused connection when probing a new network path ({{probing}}). An endpoint MUST use a previously-unused connection ID in response to a change in the address of a peer if the packet with the new peer address uses an active connection ID that has not been previously used by the peer. An endpoint MUST NOT [...as above].
This covers all the existing reasons to use a previously unused connection ID. You could restate as a list instead, I guess:
An endpoint MUST use a new connection ID when it:
- migrates to a new network path ({{initiating-migration}}), including migrating to a server's preferred address ({{preferred-address}});
- probes a new network ({{probing}}); or,
- sends a packet on a new network path as a result of responding to the migration of a peer, where the peer uses a connection ID that has not been used on another path.
In all cases, an endpoint MUST NOT use a connection ID on a given network path if it has used the same connection ID to send packets on another network path.
migrate to the preferred address, it MUST continue sending all future packets to | ||
the server's original IP address, and retire the connection ID provided by the | ||
preferred_address transport parameter. Retirement of either of these connection | ||
IDs notifies the server of the address the client has chosen. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me try to be clearer about my objection to this last sentence.
This implies a semantic to the retirement of connection IDs that is not already defined. It says that in addition to releasing the resource, the server can say definitively that those other network paths won't be used. But this is misleading because retiring CID 1 does not prevent CID 4 from being used on that path. Nothing says that the connection IDs sent in NEW_CONNECTION_ID have to be used on one or other path. Better to keep the requirement where it is: don't migrate back if you use a preferred address.
Yes, that means that servers can't be sure of behaviour of clients here, but they can use the destination address to confirm acceptance of the preferred address or not. That should suffice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you want to state that in the issue (i.e. #3353), as we've been discussing there about having this requirement? Or maybe it deserves a separate issue, as it might be considered a design change, while other changes proposed in this PR can be seen as editorial.
@kazuho, it seems like this has stalled. What do you want to do with this? |
Digging through history here here, this was going to resolve #3353, however we had discussed replacing this particular set of text and with something a bit more condensed. We got some of that editorial text here via #3497. #3353 is a design issue and the proposal from ZRH was to close with no normative changes, simply clarify the text via editorial changes. From the comment in #3353, @martinthomson filed an issue (is that #3560?) to cover editorial changes, but that's been marked design and has PR #3565 covering those changes, which seem good and necessary. I'll go through #3353 (the original) and send a PR for anything covered there that's not already in via the editorial change #3497, which should mean we've covered all our bases here and the result of #3353 will be "no design changes to be made, editorial text clarified via #3497 and #3589". (Edit: this is now done, #3589) |
This PR changes the following. FWIW, I think that the new text has been our intent when we introduced the concept of retiring CIDs.
closes #3353.