-
Notifications
You must be signed in to change notification settings - Fork 204
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
A more complete connection migration #1012
Conversation
Rewrite of Connection Migration logic
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 writing this up. It's a pretty solid design overall (not nearly as prescriptive as ICE, which is good). I think that there are a number of details that need more meat on them to support the new features that this adds. I've tried to highlight them.
I also think that the structure of this is unfortunate. I would prefer to see a generic description of path validation that is role-agnostic. I realize that we do have a fundamentally asymmetric protocol and I'm not suggesting that this be perfectly symmetric, but this is unnecessarily asymmetric and the structure you have chosen has what appears to be errors in symmetry. There are some generic requirements that are only in one or other of the client or server sections where they should (at least from my brief review) be shared. Some of the differences seem like errors, but they could be intentional. If there are intentional differences (like the fact that you have chosen to allow only clients to migrate here), then those asymmetries should be more explicit.
There needs to be a more explicit mention of NEW_CONNECTION_ID in relation to the client-initiated path probe. If the client uses a new connection ID for that probe, what does that do to packet numbers on the existing path? That could be a hard problem given the current design (this is something I hope to address with packet number obfuscation/encryption, so you might decide not to address that and instead leave a TODO for packet numbers).
Finally, I'm not quite done with reviewing this. I want to do a gap analysis between this and STUN to see if this is a reasonable substitute (because you are effectively replicating STUN capabilities now, albeit in a more integrated and minimal fashion).
draft-ietf-quic-transport.md
Outdated
|
||
PATH_CHALLENGE ({{frame-path-challenge}}) and PATH_RESPONSE | ||
({{frame-path-response}}) frames MAY be used for validating that an endpoint | ||
owns an address, or to verify the PMTU to/from an 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.
This section doesn't talk about verifying PMTU from.
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 client is verifying PMTU from its new 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.
I mean, this talks about verification toward the peer (outward bound), not from the peer (inbound).
draft-ietf-quic-transport.md
Outdated
|
||
### Client Behavior {#migration-client} | ||
|
||
A client triggers connection migration to a new address by sending a |
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 hope that this isn't a trigger for migration. The path verification might be a prerequisite for a conscious migration by a client, but it's not what we should be using to trigger the switch on the server end.
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.
Oops -- yes it doesn't. Will fix.
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.
Perhaps "When a client intends to migrate to a new address, it prepares for the migration by...."
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 really need to separate involuntary NAT rebinding and voluntary migration.
draft-ietf-quic-transport.md
Outdated
failure. | ||
|
||
|
||
#### Responding to a Server's Validation |
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 would prefer that we talk about the different types of migration rather than have this talk about client and server behaviour separately. Having this text here is hard to contextualize because it comes before any of the text about what might have happened to cause it to be necessary.
Also, I think that initiating and responding to validation is a generic thing that could be described independent of the different use cases/scenarios.
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.
So I chose to structure it like this because it's easier to see how to implement such a thing. If it's useful to contextualize, I can add more text in the common text up at the top of this section.
Validation can be described separately, but in this description, it's only used for the server to validate a client's new address.
draft-ietf-quic-transport.md
Outdated
A client may receive a PATH_CHALLENGE from a server that is validating the | ||
client's ownership of its address, as described in {{migration-server}}. The | ||
client MUST respond to this frame by echoing the data from the server's | ||
PATH_CHALLENGE frame in a PATH_RESPONSE frame. |
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 doesn't say what path the response follows. It's important here to note that the client doesn't need to change its addressing info in any way when responding.
draft-ietf-quic-transport.md
Outdated
{{migration-server}}). | ||
|
||
A client migrating to a new network might use a new connection ID for packets | ||
sent from the new address, see {{migration-linkability}} for further discussion. |
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.
There's nothing here about the use of connection ID and packet numbers for the probes on the new 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.
Probes are just packets, and are not treated any differently, so they follow the same rules for connection ID and packet numbers as specified in the linkability section.
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.
There's an assumption in the current design that connection IDs are used in series. If a client wants to send on a new path, then it will want to use a new connection ID. But it will then send on the original path, and that will need a different connection ID. It can't use the original connection ID, or the one that it used on the path it was probing with the current design.
With the potential need for multiple probes on the new path, interleaved with continuing packets on the old path, this will consume far more connection IDs than our current design really allows for. Or it will be linkable.
This is where I think packet number encryption/obfuscation will help. It will allow us to switch back to the old connection ID without having to worry about gaps in packet numbers. I think that we will need to fix that.
That doesn't make me think that this is a broken design, only that it reveals that our current design for unlinkability across paths is not suitable any more.
draft-ietf-quic-transport.md
Outdated
The server sends the PATH_RESPONSE in a packet that is padded to no more than | ||
the size of the client's packet carrying the PATH_CHALLENGE. The server SHOULD | ||
pad the packet to the same size as the client's packet, unless the server has | ||
reasonable assurance of a smaller PMTU. |
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.
Why does the server pad its PATH_RESPONSE, but the client does not? Also, this smaller PMTU seems confusing, even if it isn't explicitly wrong. We definitely need a 1200 in here somewhere.
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.
Security consideration: if we allow PATH RESPONSE packets to be larger than the PATH CHALLENGE packet that triggered them, we enable a reflection DOD attack.
draft-ietf-quic-transport.md
Outdated
address. The server MUST send all subsequent packets to this address, with the | ||
following exception. PATH_CHALLENGE, PATH_RESPONSE, and PADDING frames are used | ||
for probing a network path, and receiving only these frames MUST NOT cause the | ||
server to commit to a new 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.
I think that this can be simpler. Any packet containing PATH_CHALLENGE is ignored.
You do allow STREAM frames with that packet in earlier text (and STREAM frames aren't a terrible waste of octets, so I can imagine people using the bandwidth for that rather than padding, even if the probability of loss is that much greater). But this text would cause the recipient to latch.
draft-ietf-quic-transport.md
Outdated
subject to the congestion controller's limits. | ||
|
||
A PATH_CHALLENGE frame used for validation MUST contain at least 12 randomly | ||
generated octets {{?RFC4086}}, making the payload sufficiently hard to guess. |
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 a generic requirement, but you have it here in the server-specific section.
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.
Seconded. Shouldn't this simply be in the definition of the PATH_CHALLENGE frame?
draft-ietf-quic-transport.md
Outdated
|
||
Data: | ||
|
||
: This variable-length field contains arbitrary data. |
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.
Can we make this a fixed length?
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 already have a minimum. Is there a scenario in which a server would want it to be longer?
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 look at it this way. We have a certain dependency on the connection ID for path verification during the handshake and that is only 64 bits. So, maybe we can reasonably say that 64 bits is enough here as well.
draft-ietf-quic-transport.md
Outdated
FRAME_ERROR, indicating the PONG frame (that is, 0x10d). If the content of a | ||
PONG frame does not match the content of a PING frame previously sent by the | ||
endpoint, the endpoint MAY generate a connection error of type UNSOLICITED_PONG. | ||
A PATH_RESPONSE frame MUST NOT elicit acknowledgements. |
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 don't think that this is a necessary requirement. Given that it's the packet that generates the acknowledgments, that PADDING causes ACKs to be sent, and that this will never fill an entire packet, I think that you can drop this line.
draft-ietf-quic-transport.md
Outdated
@@ -3841,7 +3941,8 @@ Issue and pull request numbers are listed with a leading octothorp. | |||
|
|||
## Since draft-ietf-quic-transport-08 | |||
|
|||
No significant changes. | |||
- Added PATH_CHALLENGE and PATH_RESPONSE frames, removed PING with Data, removed | |||
PONG frame and rewrote connection migration (#000) |
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 should be filled in with the correct number now
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 the write-up -- I think it helps move in the right direction. I agree with Martin, though -- you have a lot more asymmetry here than seems necessary. We also have issues open around server-side migration, and this doesn't allow for that, even though it seems like it could do so with relatively little additional work.
draft-ietf-quic-transport.md
Outdated
A server validates a client's address by sending a PATH_CHALLENGE frame | ||
containing a payload that is hard to guess to that address. The server | ||
considers the address to be valid when a PATH_RESPONSE frame containing the same | ||
payload is received from the client. |
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.
Does it have to come back from a particular address? Either way, perhaps say so.
draft-ietf-quic-transport.md
Outdated
|
||
### Client Behavior {#migration-client} | ||
|
||
A client triggers connection migration to a new address by sending a |
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.
Perhaps "When a client intends to migrate to a new address, it prepares for the migration by...."
draft-ietf-quic-transport.md
Outdated
### Client Behavior {#migration-client} | ||
|
||
A client triggers connection migration to a new address by sending a | ||
PATH_CHALLENGE as a probe or by switching to sending all packets to the new |
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.
to => from. If you take the proposed change above, perhaps expand this to talk about unintended/unexpected address changes, in which case the client will just keep sending from whatever address it has to work with.
draft-ietf-quic-transport.md
Outdated
A client triggers connection migration to a new address by sending a | ||
PATH_CHALLENGE as a probe or by switching to sending all packets to the new | ||
address. The client SHOULD do PMTU verification before considering migration | ||
complete. The client also participates in an address validation that the server |
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.
an => any
draft-ietf-quic-transport.md
Outdated
|
||
#### Verifying the PMTU | ||
|
||
When the client intends to use a new local address, it MAY establish server |
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.
Or realizes that it is using a new path? But how does it do that, based on receipt of a PATH_CHALLENGE from the server?
draft-ietf-quic-transport.md
Outdated
new address as though the address were not validated (see {{migrate-validate}}). | ||
A server in the closing state MAY instead choose to discard packets received | ||
from a new source address. | ||
new address as though the address were not validated (see |
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 know this isn't new text, but why "as though the address were" rather than "since the address is not"?
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.
Good point, fixed.
draft-ietf-quic-transport.md
Outdated
|
||
Endpoints can use PATH_CHALLENGE frames (type=0x0e) to check reachability to the | ||
peer, to verify a new path's PMTU, and to perform address validation during | ||
connection migration. |
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.
Is PATH_CHALLENGE useful for PMTU validation once the address is known to be good? Why couldn't you simply rely on ACKs to tell you whether the packet got through or not?
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, I think that the requirement for a minimum size on probes is a requirement on the probing packet, not a property of any particular frame that it contains.
draft-ietf-quic-transport.md
Outdated
|
||
Data: | ||
|
||
: This variable-length field contains arbitrary data. |
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 already have a minimum. Is there a scenario in which a server would want it to be longer?
draft-ietf-quic-transport.md
Outdated
|
||
|
||
The sender of this frame MUST include at least one octet of data in the Data | ||
field. |
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.
A larger minimum is provided above (eight octets, I think?).
draft-ietf-quic-transport.md
Outdated
PATH_CHALLENGE frame containing an empty payload MUST generate a connection | ||
error of type FRAME_ERROR, indicating the PATH_CHALLENGE frame (that is, 0x0e). | ||
|
||
A PATH_CHALLENGE frame MUST NOT elicit acknowledgements; the corresponding |
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.
Frames don't get acknowledged; packets do. Does this mean that a packet containing a PATH_CHALLENGE MUST NOT be acknowledged? That's going to play merry hob with the loss recovery if you start putting actual data in there (padding with STREAM).
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.
But frames do elicit acknowledgments. Generally. ACK doesn't. The "MUST NOT" isn't appropriate; it seems like the intent is to say that it doesn't elicit acknowledgment (that is, it's not an interoperability requirement, it's just a property).
I don't think that it makes sense to have special acknowledgment rules for PATH_CHALLENGE though. Acknowledging PATH_CHALLENGE doesn't contribute to path validation, but on the other hand explicitly excluding it creates another exception. I don't see any reason to suppress acknowledgments in this case.
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 have a feeling that this text about the composition of challenge response is repeated multiple times. Can we simplify?
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.
@martinthomson : I'm going to try to unify the mechanisms and make this client/server agnostic, which might resolve some issues. We did that because it was easier to reason about in the earlier designs, but the current version actually lends itself nicely to symmetry, I think. At any rate, let me try that first.
draft-ietf-quic-transport.md
Outdated
|
||
### Client Behavior {#migration-client} | ||
|
||
A client triggers connection migration to a new address by sending a |
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.
Oops -- yes it doesn't. Will fix.
draft-ietf-quic-transport.md
Outdated
{{migration-server}}). | ||
|
||
A client migrating to a new network might use a new connection ID for packets | ||
sent from the new address, see {{migration-linkability}} for further discussion. |
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.
Probes are just packets, and are not treated any differently, so they follow the same rules for connection ID and packet numbers as specified in the linkability section.
draft-ietf-quic-transport.md
Outdated
failure. | ||
|
||
|
||
#### Responding to a Server's Validation |
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.
So I chose to structure it like this because it's easier to see how to implement such a thing. If it's useful to contextualize, I can add more text in the common text up at the top of this section.
Validation can be described separately, but in this description, it's only used for the server to validate a client's new address.
I have mixed feelings about this. First, I am not sure that this belongs to the frame layer. The path challenge is in many ways similar to a client initial packet. It requires the same kind of padding, for the same reason. And it carries similar content restrictions such as "no other frame allowed". A new packet type might be more appropriate than a frame type. Second, this probably requires a three ways handshake, not just a question response. And it would be better served with a symmetric structure -- Then, there maybe a need for more attributes than nonces, especially if we want to handle NAT rebinding. Like, echoing the address from where this was sent. |
I guess my uneasiness stems from the PR's scope - supporting both involuntary transition like NAT remapping and voluntary transition like make before break. The latter addresses maybe 80% of the multi path issues, and I don't like addressing that without completing the multi path work. Sine full multipath is indeed out of scope for V1, how about simplifying drastically and only addressing involuntary NAT rebinding? |
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 think this is not quite ready yet. We should separate the NAT Rebinding and the network migration scenario. We should be explicit about the "per path" nature of congestion and PMTU. We should consider encrypting the sequence number.
draft-ietf-quic-transport.md
Outdated
Note that the connection ID may change during the connection via the use of | ||
NEW_CONNECTION_ID frames ({{frame-new-connection-id}}) and connection migration | ||
({{migration}}). | ||
|
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 connection ID SHOULD change during the connection.
In fact, it is debatable whether the client should engage in connection migration if the server has not provided new connection identifiers via the use of NEW_CONNECTION_ID frames. Maybe it should use close/resume instead.
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 discussed further below, in the linkability discussion. There's no normative language in this section, this is only a statement of fact.
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 prefer:
The connection ID could change over the lifetime of a connection, especially in response to connection migration (cite). NEW_CONNECTION_ID frames (cite) are used to provide new values.
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.
done.
draft-ietf-quic-transport.md
Outdated
QUIC connections are identified by 64-bit connection IDs. QUIC's connection IDs | ||
allow connections to survive changes to the client's address (client's IP | ||
address and/or port), such as those caused by a client migrating to a new | ||
network or a NAT rebinding changing the client's public 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.
I think we should really distinguish between NAT rebinding and connection migration. The clients can generally not anticipate that a NAT rebinding will occur, and are thus likely to continue using the same connection ID as before the rebinding. In contrast, clients can generally control the decision to start migration to a new network, and should use a new connection ID when doing that.
draft-ietf-quic-transport.md
Outdated
|
||
### Client Behavior {#migration-client} | ||
|
||
A client triggers connection migration to a new address by sending a |
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 really need to separate involuntary NAT rebinding and voluntary migration.
draft-ietf-quic-transport.md
Outdated
The server sends the PATH_RESPONSE in a packet that is padded to no more than | ||
the size of the client's packet carrying the PATH_CHALLENGE. The server SHOULD | ||
pad the packet to the same size as the client's packet, unless the server has | ||
reasonable assurance of a smaller PMTU. |
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.
Security consideration: if we allow PATH RESPONSE packets to be larger than the PATH CHALLENGE packet that triggered them, we enable a reflection DOD attack.
draft-ietf-quic-transport.md
Outdated
received for a prior one. | ||
|
||
Servers MUST ignore a PATH_CHALLENGE frame from a client that is carried in a | ||
packet smaller than 1200 octets in size. |
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. Security consideration as to why?
draft-ietf-quic-transport.md
Outdated
a denial of service attack against an unsuspecting victim. | ||
|
||
When validation of an address fails, the server SHOULD revert back to using the | ||
last validated client address, absent which the server MUST close the connection |
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.
Basically, starting a new path should reset the congestion control algorithm to ts starting condition. Just say so.
draft-ietf-quic-transport.md
Outdated
For instance, an endpoint may run a separate alarm when a PATH_CHALLENGE is | ||
sent, which is disarmed when the corresponding PATH_RESPONSE is received. If the | ||
alarm fires before the PATH_RESPONSE is received, the endpoint might send a new | ||
PATH_CHALLENGE, and restart the alarm, but for a longer period of time. |
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 see the discussion of the congestion control window, but I do not see an equivalent discussion of flow control and the MAX_DATA frame. Yet if the client migrates to a slow connection, MAX_DATA is going to be way too large. Where is that discussed?
@@ -1541,112 +1731,6 @@ number. "packet_number_secret" is derived from the TLS key exchange, | |||
as described in Section 5.6 of {{QUIC-TLS}}. |
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 packet-number-gap trick only works if both the client and the server do it, and if both refrain from sending any packet on the old connection in parallel with the new connection. That seems very fragile. We should really consider encrypting the packet sequence number.
Note that all this discussion only applies to voluntary migration. It does not apply to NAT rebinding. If the client cannot anticipate the NAT rebinding, it will not be able to switch to a new connection ID or to insert a gap in the sequence numbers.
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.
See issue #1034. The packet number gap is a complex mechanism that does not actually protect privacy, because client and server apply the same gap.
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 agree with Christian, this seems fragile, but that's for another issue and discussion
draft-ietf-quic-transport.md
Outdated
PATH_CHALLENGE frame containing an empty payload MUST generate a connection | ||
error of type FRAME_ERROR, indicating the PATH_CHALLENGE frame (that is, 0x0e). | ||
|
||
A PATH_CHALLENGE frame MUST NOT elicit acknowledgements; the corresponding |
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 have a feeling that this text about the composition of challenge response is repeated multiple times. Can we simplify?
draft-ietf-quic-transport.md
Outdated
generate a connection error of type FRAME_ERROR, indicating the PATH_RESPONSE | ||
frame (that is, 0x0e). If the content of a PATH_RESPONSE frame does not match | ||
the content of a PATH_CHALLENGE frame previously sent by the endpoint, the | ||
endpoint MAY generate a connection error of type UNSOLICITED_PATH_RESPONSE. |
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.
Did we say before that path challenges do not need to be acked? Presumably, some servers will want to repeat them. Should the repeated challenge have exactly the same content as the original challenge?
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 have a few more small suggestions, but I think this is close to done.
draft-ietf-quic-transport.md
Outdated
@@ -1487,8 +1487,8 @@ network. This section describes the protocol for migrating a connection to a | |||
new client address. | |||
|
|||
Migrating a connection to a new server address is left for future work. If a | |||
client receives packets from a new server address that are decryptable, the | |||
client MAY discard these packets. | |||
client receives packets in the connection from a new server address, the client |
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.
nit: I don't think "in the connection" is necessary, since you begin the sentence with "Migrating a connection..."
draft-ietf-quic-transport.md
Outdated
@@ -1477,35 +1481,240 @@ connection if the integrity check fails with a PROTOCOL_VIOLATION error code. | |||
|
|||
## Connection Migration {#migration} |
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.
If this is client specific, should it be called Client Connection Migration, at least for the time being?
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.
SGTM. done.
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 seeing the change.
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.
...except that it isn't. The title of the section is still "Connection Migration."
draft-ietf-quic-transport.md
Outdated
new client address. | ||
|
||
Migrating a connection to a new server address is left for future work. If a | ||
client receives packets in the connection from a new server address, the client |
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.
nit: I don't think "in the connection" is necessary, since you begin the sentence with "Migrating a connection..."
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 different sentence... I think it's still useful because if the packets don't belong to this connection, the rest of the sentence doesn't apply.
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 in favor of Ian's change, because if packets don't map to a connection they'll be discarded anyway. The extra words just sound awkward. However, it's not technically incorrect, so this is purely wordsmithing.
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.
Ok, removed.
draft-ietf-quic-transport.md
Outdated
The endpoint MAY send additional PATH_CHALLENGE frames to handle packet loss, | ||
but is subject to the following limit to avoid excessive network load: an | ||
endpoint SHOULD NOT send more than one PATH_CHALLENGE frame in 200ms. This | ||
limit is approximately equivalent to the network load due to a new connection, |
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 agree that we should avoid being specific, which is one of the reasons I'm not a fan of saying 200ms here, and would prefer text such as: "an endpoint SHOULD NOT send a PATH_CHALLENGE more frequently than it would a client INITIAL, ensuring that connection migration is no more costly to a new path than establishing a new connection."
draft-ietf-quic-transport.md
Outdated
client's possession of the new address. | ||
|
||
Until a client's address is deemed valid, the server MUST limit the rate at | ||
which it sends data to this address. The server MUST NOT send more than 4 * MSS |
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 see why you chose 4*MSS. In this document, 2*Min CWND seems preferable to a specific value to me.
I think it's worth clarifying that the RTO is likely based on default RTT/default RTO in this case, since if you haven't received a PATH_RESPONSE, I would expect you either have no estimate or a somewhat unverified estimate of RTT.
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.
Again, I appreciate that this targets a particular use case, but describing the basic mechanisms in general terms would be appreciated. Role agnostic descriptions of the core mechanisms would be valuable if we ever need to migrate a server, and maybe also if we wanted to do something like was proposed in #560.
Some of that has happened, but it's not very consistent.
draft-ietf-quic-transport.md
Outdated
|
||
Path validation is used by a server to confirm the client's ownership of a | ||
new address and by a client to establish reachability to the server from a new | ||
local 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.
Path validation is used by the migrating endpoint to verify reachability of its peer from a new local address. Path validation is used by the peer to verify that the migrating endpoint is able to receive packets sent to that address. That is, that the packets the peer receives do not carry a spoofed source 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.
Done (with some mods).
draft-ietf-quic-transport.md
Outdated
To start path validation, an endpoint sends a PATH_CHALLENGE frame containing | ||
a payload that is hard to guess on the path to be validated. | ||
|
||
On receiving a PATH_CHALLENGE frame, the peer MUST immediately respond by |
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.
s/the peer/an endpoint/
"immediately" here implies no ack delay or pacing. I think that you need to explicitly say which (if either) of these is being excluded.
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.
Ack delay does not apply, since it's not an ack that's being sent. Though this made me realize that I hadn't said anything about limiting the number of responses, so I've added some text (and moved some text around) limiting the rate at which responses are sent.
draft-ietf-quic-transport.md
Outdated
echoing the data contained in the PATH_CHALLENGE frame in a PATH_RESPONSE frame. | ||
The PATH_RESPONSE MUST be sent on the same path: from the same local address on | ||
which the PATH_CHALLENGE was received, to the same remote address from which the | ||
PATH_CHALLENGE was received. |
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.
Is this same-path requirement solely to ensure that reachability is symmetric? Is it to ensure that the frames can be used to measure RTT on the new path? I think that you should say why here.
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.
It was meant to establish reachability in both directions. I've written it down now.
draft-ietf-quic-transport.md
Outdated
the corresponding PATH_CHALLENGE was sent. If a PATH_RESPONSE frame is received | ||
from a different remote address than the one to which the PATH_CHALLENGE was | ||
sent, path validation is considered to have failed, even if the data matches | ||
that sent in the PATH_CHALLENGE. |
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 can't rationalize this requirement at all. Can you explain why you think that this is necessary?
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.
Perhaps this needs to be more explicitly stated, but bi-directional reachability is what is desired when doing connection migration to a new address. The test is incomplete if it only tests one direction. I've also added a sentence up above making this explicit.
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.
Worth noting that one-way reachability might be a thing once we get to multipath. If client->server has two valid paths, but server->client only one, so be it. But I don't object to saying path validation has to test both directions when we're only doing migration.
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, that would be quite a different deal. You'd want to be very careful when thinking about reachability in the multipath case, but as you say, that's for a different day.
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 agree that requiring bi-directional reachability in this version is a good step, we can always remove that when defining true multipath.
draft-ietf-quic-transport.md
Outdated
the corresponding PATH_CHALLENGE was sent. If a PATH_RESPONSE frame is received | ||
on a different local address than the one from which the PATH_CHALLENGE was | ||
sent, path validation is considered to have failed, even if the data matches | ||
that sent in the PATH_CHALLENGE. |
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.
Same as above.
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.
done
draft-ietf-quic-transport.md
Outdated
If a non-probing packet is received from a new client address but with a packet | ||
number that is not the largest seen thus far, the server must process the packet | ||
as usual, but it MUST ignore this client address under the assumption that it is | ||
not the client's most recent 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.
Some explanation as to why here (the client might have migrated twice), would be good.
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.
added some text
draft-ietf-quic-transport.md
Outdated
|
||
After verifying a new client address, the server SHOULD update any address | ||
validation tokens ({{address-validation}}) that it has issued to the client if | ||
those are no longer valid based on the changed 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.
What does update mean? Does that mean sending them again?
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.
Sending new ones that validate the new 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.
updated text
draft-ietf-quic-transport.md
Outdated
client's possession of the new address. | ||
|
||
Until a client's address is deemed valid, the server MUST limit the rate at | ||
which it sends data to this address. The server MUST NOT send more than 4 * MSS |
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.
Everything that Ian said, except that I don't understand why 2xMinCWND. Seems equally arbitrary. MinCWND on the assumption of a default RT[TO] would be more defensible.
draft-ietf-quic-transport.md
Outdated
|
||
There may be apparent reordering at the receiver when an endpoint sends data and | ||
probes from/to multiple addresses during the migration period, since the two | ||
resulting paths may have different rountrip times. A receiver of packets on |
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.
roudtrip
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 'd' is redundant anyways, nobody pronounces it :-)
... though fixing english is not in charter, so fixing text
draft-ietf-quic-transport.md
Outdated
new address as though the address were not validated (see {{migrate-validate}}). | ||
A server in the closing state MAY instead choose to discard packets received | ||
from a new source address. | ||
new address since the address is not validated (see {{migrate-validate}}). A |
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.
s/since/until
s/not //
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.
done
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 also found possibly-stale comments. Submitting these, then I'll do another read-through for current state.
draft-ietf-quic-transport.md
Outdated
the corresponding PATH_CHALLENGE was sent. If a PATH_RESPONSE frame is received | ||
on a different local address than the one from which the PATH_CHALLENGE was | ||
sent, path validation is considered to have failed, even if the data matches | ||
that sent in the PATH_CHALLENGE. |
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 feel like these two paragraphs could be combined.
draft-ietf-quic-transport.md
Outdated
client address, it MUST close the connection silently and discard any further | ||
packets received from the client for this connection. | ||
|
||
We note that receipt of packets with higher packet numbers from the legitimate |
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've mostly avoided using the first-person plural in these specs. Can we rephrase? Simply saying that "Receipt of packets...." should be sufficient.
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.
done
draft-ietf-quic-transport.md
Outdated
|
||
A client SHOULD reset its congestion controller and roundtrip time estimator | ||
prior to sending any non-probing packets from a new local 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.
And the same below for servers, as well.
draft-ietf-quic-transport.md
Outdated
|
||
There may be apparent reordering at the receiver when an endpoint sends data and | ||
probes from/to multiple addresses during the migration period, since the two | ||
resulting paths may have different rountrip times. A receiver of packets on |
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.
round trip
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.
Addressed comments, PTAL.
draft-ietf-quic-transport.md
Outdated
@@ -1477,35 +1481,240 @@ connection if the integrity check fails with a PROTOCOL_VIOLATION error code. | |||
|
|||
## Connection Migration {#migration} |
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.
SGTM. done.
draft-ietf-quic-transport.md
Outdated
new client address. | ||
|
||
Migrating a connection to a new server address is left for future work. If a | ||
client receives packets in the connection from a new server address, the client |
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 different sentence... I think it's still useful because if the packets don't belong to this connection, the rest of the sentence doesn't apply.
draft-ietf-quic-transport.md
Outdated
|
||
Path validation is used by a server to confirm the client's ownership of a | ||
new address and by a client to establish reachability to the server from a new | ||
local 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.
Done (with some mods).
draft-ietf-quic-transport.md
Outdated
To start path validation, an endpoint sends a PATH_CHALLENGE frame containing | ||
a payload that is hard to guess on the path to be validated. | ||
|
||
On receiving a PATH_CHALLENGE frame, the peer MUST immediately respond by |
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.
Ack delay does not apply, since it's not an ack that's being sent. Though this made me realize that I hadn't said anything about limiting the number of responses, so I've added some text (and moved some text around) limiting the rate at which responses are sent.
draft-ietf-quic-transport.md
Outdated
echoing the data contained in the PATH_CHALLENGE frame in a PATH_RESPONSE frame. | ||
The PATH_RESPONSE MUST be sent on the same path: from the same local address on | ||
which the PATH_CHALLENGE was received, to the same remote address from which the | ||
PATH_CHALLENGE was received. |
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.
It was meant to establish reachability in both directions. I've written it down now.
draft-ietf-quic-transport.md
Outdated
|
||
After verifying a new client address, the server SHOULD update any address | ||
validation tokens ({{address-validation}}) that it has issued to the client if | ||
those are no longer valid based on the changed 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.
updated text
draft-ietf-quic-transport.md
Outdated
client's possession of the new address. | ||
|
||
Until a client's address is deemed valid, the server MUST limit the rate at | ||
which it sends data to this address. The server MUST NOT send more than 4 * MSS |
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.
added text and set the rate to min cwnd per rto.
draft-ietf-quic-transport.md
Outdated
client address, it MUST close the connection silently and discard any further | ||
packets received from the client for this connection. | ||
|
||
We note that receipt of packets with higher packet numbers from the legitimate |
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.
done
draft-ietf-quic-transport.md
Outdated
|
||
There may be apparent reordering at the receiver when an endpoint sends data and | ||
probes from/to multiple addresses during the migration period, since the two | ||
resulting paths may have different rountrip times. A receiver of packets on |
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 'd' is redundant anyways, nobody pronounces it :-)
... though fixing english is not in charter, so fixing text
draft-ietf-quic-transport.md
Outdated
new address as though the address were not validated (see {{migrate-validate}}). | ||
A server in the closing state MAY instead choose to discard packets received | ||
from a new source address. | ||
new address since the address is not validated (see {{migrate-validate}}). A |
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.
done
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.
Looking good. Most of these are strictly wordsmithing.
draft-ietf-quic-transport.md
Outdated
@@ -1477,35 +1481,240 @@ connection if the integrity check fails with a PROTOCOL_VIOLATION error code. | |||
|
|||
## Connection Migration {#migration} |
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.
...except that it isn't. The title of the section is still "Connection Migration."
draft-ietf-quic-transport.md
Outdated
new client address. | ||
|
||
Migrating a connection to a new server address is left for future work. If a | ||
client receives packets in the connection from a new server address, the client |
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 in favor of Ian's change, because if packets don't map to a connection they'll be discarded anyway. The extra words just sound awkward. However, it's not technically incorrect, so this is purely wordsmithing.
draft-ietf-quic-transport.md
Outdated
An endpoint MAY send additional PATH_CHALLENGE frames to handle packet loss, | ||
but is subject to the following limit to avoid excessive network load: an | ||
endpoint SHOULD NOT send a PATH_CHALLENGE more frequently than it would a client | ||
INITIAL, ensuring that connection migration is no more load on a new path than |
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.
Packet names are title-cased, not all-caps. Also, I'd drop the colon lead-up.
...but to avoid excessive network load, an endpoint SHOULD NOT... an Initial packet. This ensures that....
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.
Fixed.
draft-ietf-quic-transport.md
Outdated
|
||
On receiving a PATH_CHALLENGE frame, an endpoint MUST respond immediately by | ||
echoing the data contained in the PATH_CHALLENGE frame in a PATH_RESPONSE frame, | ||
with the following stipulation. Since a PATH_CHALLENGE may be sent from a |
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.
"might be" - there's no permission involved here.
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, right. I'm always confused between may and might. This is a good way to think about it. Done.
draft-ietf-quic-transport.md
Outdated
Path validation tests both directions of a path. To ensure reachability in both | ||
directions, the PATH_RESPONSE MUST be sent on the same path as the triggering | ||
PATH_CHALLENGE: from the same local address on which the PATH_CHALLENGE was | ||
received, to the same remote address from which the PATH_CHALLENGE was received. |
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.
and port, I presume?
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.
an address is defined above as IP and/or port
draft-ietf-quic-transport.md
Outdated
|
||
### Initiating Connection Migration {#initiating-migration} | ||
|
||
A client MAY initiate connection migration to a new local address in one of two |
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 "MAY" seems superfluous, particularly since the first way also has a MAY. I'd either lower-case it, "can", or "A client initiates...." Also, I don't know that I'd frame these as two "ways" of migrating. Ultimately, the client migrates by sending data from the new address/port. A client MAY probe for reachability first, if it has the opportunity.
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.
Good suggestions. I've made restructured a bit so that it flows as you suggest instead of "two ways".
draft-ietf-quic-transport.md
Outdated
server MUST revert to using the last validated client address when validation of | ||
a new client address fails. If the server has no state about the last validated | ||
client address, it MUST close the connection silently and discard any further | ||
packets received from the client for this connection. |
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.
How does this tie in to the closure/draining states described elsewhere? Does the server enter draining? Can it send Stateless Reset?
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.
Good q. I've changed it so that the server basically discards all connection state, leaving subsequent packets to be treated generically. That is, the server could send Stateless Resets in response. I think this is reasonable.
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, Mike. Addressed your comments, PTAL.
draft-ietf-quic-transport.md
Outdated
new client address. | ||
|
||
Migrating a connection to a new server address is left for future work. If a | ||
client receives packets in the connection from a new server address, the client |
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.
Ok, removed.
draft-ietf-quic-transport.md
Outdated
An endpoint MAY send additional PATH_CHALLENGE frames to handle packet loss, | ||
but is subject to the following limit to avoid excessive network load: an | ||
endpoint SHOULD NOT send a PATH_CHALLENGE more frequently than it would a client | ||
INITIAL, ensuring that connection migration is no more load on a new path than |
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.
Fixed.
draft-ietf-quic-transport.md
Outdated
|
||
On receiving a PATH_CHALLENGE frame, an endpoint MUST respond immediately by | ||
echoing the data contained in the PATH_CHALLENGE frame in a PATH_RESPONSE frame, | ||
with the following stipulation. Since a PATH_CHALLENGE may be sent from a |
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, right. I'm always confused between may and might. This is a good way to think about it. Done.
draft-ietf-quic-transport.md
Outdated
Path validation tests both directions of a path. To ensure reachability in both | ||
directions, the PATH_RESPONSE MUST be sent on the same path as the triggering | ||
PATH_CHALLENGE: from the same local address on which the PATH_CHALLENGE was | ||
received, to the same remote address from which the PATH_CHALLENGE was received. |
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.
an address is defined above as IP and/or port
draft-ietf-quic-transport.md
Outdated
the corresponding PATH_CHALLENGE was sent. If a PATH_RESPONSE frame is received | ||
from a different remote address than the one to which the PATH_CHALLENGE was | ||
sent, path validation is considered to have failed, even if the data matches | ||
that sent in the PATH_CHALLENGE. |
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, that would be quite a different deal. You'd want to be very careful when thinking about reachability in the multipath case, but as you say, that's for a different day.
draft-ietf-quic-transport.md
Outdated
|
||
### Initiating Connection Migration {#initiating-migration} | ||
|
||
A client MAY initiate connection migration to a new local address in one of two |
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.
Good suggestions. I've made restructured a bit so that it flows as you suggest instead of "two ways".
draft-ietf-quic-transport.md
Outdated
server MUST revert to using the last validated client address when validation of | ||
a new client address fails. If the server has no state about the last validated | ||
client address, it MUST close the connection silently and discard any further | ||
packets received from the client for this connection. |
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.
Good q. I've changed it so that the server basically discards all connection state, leaving subsequent packets to be treated generically. That is, the server could send Stateless Resets in response. I think this is reasonable.
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.
Latest changes look great! A few minor nits and other comments, but nothing major.
Initial packet, ensuring that connection migration is no more load on a new path | ||
than establishing a new connection. | ||
|
||
The endpoint MUST use fresh random data in every PATH_CHALLENGE frame so that 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.
Here we indicate that this should be random data, but above we just said "hard to guess" -- I know we took out the note about what's considered hard to guess, but perhaps we should still include a note in the above paragraph that it's random?
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've added "random" to the sentence above
draft-ietf-quic-transport.md
Outdated
the corresponding PATH_CHALLENGE was sent. If a PATH_RESPONSE frame is received | ||
from a different remote address than the one to which the PATH_CHALLENGE was | ||
sent, path validation is considered to have failed, even if the data matches | ||
that sent in the PATH_CHALLENGE. |
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 agree that requiring bi-directional reachability in this version is a good step, we can always remove that when defining true multipath.
draft-ietf-quic-transport.md
Outdated
time than the original. Again, to avoid excessive network load, an endpoint | ||
SHOULD NOT send more PATH_CHALLENGE frames than it would a client INITIAL, | ||
ensuring that connection migration is no more load on a new path than | ||
establishing a new connection. |
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.
Good to have this reminder so everything is in one place when implementing
draft-ietf-quic-transport.md
Outdated
|
||
The endpoint may receive packets containing other frames during this period, but | ||
a PATH_RESPONSE frame with appropriate data is required for the path validation | ||
to succeed. |
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.
Does this mean packets on the new path or on the old path? What are we trying to indicate here?
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 meant to say other packets might be received on the new path, but they are not adequate for validation. Fixed up sentence.
draft-ietf-quic-transport.md
Outdated
|
||
If it has the opportunity, a client MAY probe for server reachability from a | ||
new local address using path validation {{migrate-validate}} prior to migrating | ||
the connection to it. Failure of path validation simply means that the new |
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.
nit: to it
-> to the new local 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.
done
draft-ietf-quic-transport.md
Outdated
performing with other addresses on the expectation that those validations are | ||
likely to fail. Abandoning path validation primarily means ceasing subsequent | ||
transmissions of the PATH_CHALLENGE frame. An endpoint MUST ignore any | ||
subsequently received PATH_RESPONSE frames from abandoned addresses. |
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 could mean the client ends up spending some additional time if it wants to re-establish reachability on the other addresses / maintain that ahead of time (like is best for performance). Probably okay, but worth considering.
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 below that this allows us to avoid issues where someone tries to migrate us and we grab it back. Looks fine in that case.
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.
Hm. I don't think we need a MUST ignore here. Changing to a MAY.
draft-ietf-quic-transport.md
Outdated
|
||
Thus, on receiving and decrypting a packet from an unvalidated address, the | ||
server MUST immediately validate the client's new address to confirm the | ||
client's possession of the new 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.
I worry that people may be confused by the perceived conflict between this and:
A server MAY skip validation of a client address if it has been seen recently or
if the server has reasonable certainty that the new address is the result of a
NAT rebinding.
Can we resolve that somehow?
Perhaps we could say above:
A server MAY consider validation to be complete for a client address that has been
seen recently or if the server has reasonable certainty that the new address is the
result of a NAT rebinding.
Or something to that effect (maybe A server MAY deem a client address to be valid
to echo the next paragraph here).
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.
Good point. I've rephrased some things here and added an explicit sentence calling out that a server SHOULD NOT apply this limit when it is not validating the client address.
draft-ietf-quic-transport.md
Outdated
its congestion controller and round-trip time estimator prior to sending any | ||
further non-probing packets. | ||
|
||
An endpoint MUST NOT restore its send rate unless it is reasonably sure that 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.
Nit: restore
-> return to
?
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.
fixed up sentence too
@@ -1541,112 +1731,6 @@ number. "packet_number_secret" is derived from the TLS key exchange, | |||
as described in Section 5.6 of {{QUIC-TLS}}. |
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 agree with Christian, this seems fragile, but that's for another issue and discussion
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, Eric, PTAL.
draft-ietf-quic-transport.md
Outdated
|
||
The endpoint may receive packets containing other frames during this period, but | ||
a PATH_RESPONSE frame with appropriate data is required for the path validation | ||
to succeed. |
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 meant to say other packets might be received on the new path, but they are not adequate for validation. Fixed up sentence.
draft-ietf-quic-transport.md
Outdated
|
||
If it has the opportunity, a client MAY probe for server reachability from a | ||
new local address using path validation {{migrate-validate}} prior to migrating | ||
the connection to it. Failure of path validation simply means that the new |
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.
done
draft-ietf-quic-transport.md
Outdated
performing with other addresses on the expectation that those validations are | ||
likely to fail. Abandoning path validation primarily means ceasing subsequent | ||
transmissions of the PATH_CHALLENGE frame. An endpoint MUST ignore any | ||
subsequently received PATH_RESPONSE frames from abandoned addresses. |
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.
Hm. I don't think we need a MUST ignore here. Changing to a MAY.
draft-ietf-quic-transport.md
Outdated
|
||
Thus, on receiving and decrypting a packet from an unvalidated address, the | ||
server MUST immediately validate the client's new address to confirm the | ||
client's possession of the new 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.
Good point. I've rephrased some things here and added an explicit sentence calling out that a server SHOULD NOT apply this limit when it is not validating the client address.
draft-ietf-quic-transport.md
Outdated
its congestion controller and round-trip time estimator prior to sending any | ||
further non-probing packets. | ||
|
||
An endpoint MUST NOT restore its send rate unless it is reasonably sure that 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.
fixed up sentence too
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.
Small editorial nits, but the content LGTM.
draft-ietf-quic-transport.md
Outdated
@@ -1477,35 +1481,240 @@ connection if the integrity check fails with a PROTOCOL_VIOLATION error code. | |||
|
|||
## Connection Migration {#migration} |
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 seeing the change.
draft-ietf-quic-transport.md
Outdated
To start path validation, an endpoint sends a PATH_CHALLENGE frame containing | ||
a payload that is hard to guess on the path to be validated. | ||
Path validation is used by the migrating endpoint to verify reachability of | ||
its peer from a new local address. Path validation is also used by the peer to |
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.
nit:s/its/the or a
draft-ietf-quic-transport.md
Outdated
An endpoint SHOULD abandon path validation after sending some number of | ||
PATH_CHALLENGE frames or after some time has passed. When setting this timer, | ||
implementations are cautioned that the new path could have a longer round-trip | ||
time than the original. Again, to avoid excessive network load, an endpoint |
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.
nit: "Again," is unnecessary.
@@ -3599,7 +3629,7 @@ entire round trip. | |||
For smooth operation of the congestion controller, it is generally considered | |||
best to not let the sender go into quiescence if avoidable. To avoid blocking a | |||
sender, and to reasonably account for the possibiity of loss, a receiver should | |||
send a MAX_DATA or MAX_STREAM_DATA frame at least two roundtrips before it | |||
send a MAX_DATA or MAX_STREAM_DATA frame at least two round trips before 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.
nit: Should it be round-trips as it is elsewhere?
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.
It's "round-trip time" or "round trip" elsewhere (I think, I'll make it consistent separately)
draft-ietf-quic-transport.md
Outdated
PATH_CHALLENGE frames or after some time has passed. When setting this timer, | ||
implementations are cautioned that the new path could have a longer round-trip | ||
time than the original. Again, to avoid excessive network load, an endpoint | ||
SHOULD NOT send more PATH_CHALLENGE frames than it would a client INITIAL, |
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.
Same issue with "Initial" as at ~1519. BTW, double check that you actually need near-identical text both places?
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.
Good point -- I don't think I need it here. The earlier text is adequate. Removed.
I am looking at a stateless implementation of challenge/response. Not hard, but the 8 bytes challenge/response is a bit short. |
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.
@martinthomson : can you take a look? I'd like to merge tomorrow (Friday), but would like to confirm that you're fine with this.
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 still too one-sided for me sadly. I would like this to say "endpoint" and "peer" everywhere it says "client" and "server", except for one paragraph that outlines why these mechanisms are only supported for client migration (i.e., because we don't want to deal with glare).
I also retain concerns about structure. I find this structure hard to deal with because - though the requirements are all there and correct - they are distributed in a way that makes working through the process difficult.
I think that you want:
- validation (this section is good)
- migration
2.1. probing
2.2. migration at the initiating end
2.2.2. voluntary migration
2.2.1. involuntary migration
2.2. migration at the responding end
2.3. congestion/loss concerns
I'm happy to propose a remix of this text that follows that structure, but that will have to wait until next week.
draft-ietf-quic-transport.md
Outdated
Path validation is used by the migrating endpoint to verify reachability of | ||
a peer from a new local address. Path validation is also used by the peer to | ||
verify that the migrating endpoint is able to receive packets sent to that | ||
address. That is, that the packets received from the migrating endpoint do |
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.
"sent to the migrated address" perhaps? The "that" is a bit hard to follow.
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.
replaced with "sent to its new address". I'm not sure if "its" is confusing; let me know if it is.
draft-ietf-quic-transport.md
Outdated
frames and MAY silently discard PATH_CHALLENGE frames that would cause it to | ||
respond at a higher rate. | ||
|
||
Path validation tests both directions of a path. To ensure reachability in both |
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.
"Path validation tests that packets can be both sent to and received from a peer."
The implication I got from reading this text was that a single validation process (CHALLENGE/RESPONSE) was sufficient for both peers to be satisfied that the path is good. That's not the case: both endpoints need to do their own challenge.
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.
done.
draft-ietf-quic-transport.md
Outdated
A client migrating to a new local address SHOULD use a new connection ID for | ||
packets sent from that address, see {{migration-linkability}} for further | ||
discussion. | ||
|
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.
Please avoid normative language then.
proof of the server's reachability from the new address. Note that since | ||
acknowledgments may be received on any path, return reachability on the new path | ||
is not established. To establish return reachability on the new path, a client | ||
MAY concurrently initiate path validation {{migrate-validate}} on the new 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.
This paragraph has too many ideas for me. Here's a breakdown:
You want to say that a client can initiate migration to a new address at any time. You also should point out that the basic functions are potentially usable for server migration, but that is not supported due to not having a means of avoid glare.
The point about paths being validated (for both server and client) belongs elsewhere, likely in the introduction of the validation section.
Same again for the point about acknowledgments. Receiving data (including acknowledgments) does not provide proof of return routeability (too low entropy).
So that leaves one idea in the first paragraph: who can do it, and what it is for.
Move the bit about concurrent path validation to a new paragraph.
Overall, I think that needs to be crisper than this and structuring this differently might help.
Give the migrating endpoint three sections:
-
Speculative probing. Why you might do it, and how you do it. Maybe also what the reaction should be, so that this is self-contained.
-
The voluntary migration an endpoint makes when it is aware of what it is doing. This happens when an endpoint is aware that it is using a new address. In this case, we should mandate path validation and a new congestion state (absent some heuristics about continuity). Path validation might be concurrent with the change, particularly if the old path is gone, so this might appear to be involuntary at the server, but this is logically distinct.
-
The involuntary migration (NAT rebinding) needs a section of its own too.
The peer of the migrating endpoint reacts differently, because the concurrent validation looks like involuntary migration to it. Note - as you do - that it can tell between a speculative probe and actual migration by the presence of frames other than PATH_CHALLENGE in packets on the new path. Then say that speculative probes don't trigger migration. After that, the only thing you need to say is that send rate is limited until the path challenge is successful, and the congestion state is reset (absent continuity heuristics again - repetition of this theme suggests a section that you can point to and include some of the nuance) after 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.
Let's chat about this tomorrow.
validation of the address of the spurious migration to be abandoned. | ||
|
||
|
||
### Loss Detection and Congestion Control |
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 section is good. I worry a little that the pattern of acknowledgments triggered by this will leak information about what is going on, but I don't have a good solution for that here.
draft-ietf-quic-transport.md
Outdated
in {{QUIC-RECOVERY}}). In the absence of this limit, the server risks being | ||
used for a denial of service attack against an unsuspecting victim. Note that | ||
since the server will not have any round-trip time estimates to the new address, | ||
the RTO period is likely to be estimated based on the default initial RTT (see |
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.
"SHOULD be" not "likely to be"
I think that this text belongs in the section that deals with handling of migrations by the non-migrating peer.
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.
done. I've simply moved this and the section below into the section above (Responding to Connection Migration).
|
||
Note that the endpoint might receive packets containing other frames on the new | ||
path, but a PATH_RESPONSE frame with appropriate data is required for path | ||
validation to succeed. |
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.
You might also hint here that a path challenge might be abandoned for other reasons. Primarily, this happens if a path challenge is initiated as a result of a path migration and another path migration occurs before it completes.
You also need to talk about failure. If this timer pops, then the path is invalid and cannot be used further.
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.
done.
draft-ietf-quic-transport.md
Outdated
packets. | ||
|
||
|
||
### Path Validation {#migrate-validate} |
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 think that you should promote this section one level and put it before connection migration generally.
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.
done. I've redone the front matter in this section to flow better as a standalone.
draft-ietf-quic-transport.md
Outdated
frames, as appropriate. For instance, an endpoint may pad a packet carrying a | ||
PATH_CHALLENGE for PMTU discovery, or an endpoint may bundle a PATH_RESPONSE | ||
with its own PATH_CHALLENGE. | ||
|
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.
Please add a paragraph disclaiming any suitability for UDP NAT traversal. Though these mechanisms might be effective for the creation of NAT bindings that support NAT traversal, the expectation is that one or other peer (typically the server) is able to receive packets without first having sent a packet on that path. Effective NAT traversal needs additional synchronization mechanisms that are not provided here.
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.
Added, PTAL.
* Create new branch for migration PR (#1012) because unicorns are not nice. * remove overview. again. * sync * fix rebase * Martin's comments * fix ref * comments
Closes #880.
This PR describes a more complete connection migration design. Among other small bits, this design includes the following: