Skip to content
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

TP to disable migration #1447

Merged
merged 4 commits into from
Jun 26, 2018
Merged

TP to disable migration #1447

merged 4 commits into from
Jun 26, 2018

Conversation

MikeBishop
Copy link
Contributor

Fixes #1271.

@MikeBishop MikeBishop added design An issue that affects the design of the protocol; resolution requires consensus. -transport labels Jun 15, 2018
@pravb
Copy link

pravb commented Jun 15, 2018

Looks good to me but I wonder if we should make this transport parameter agnostic of client / server (i.e. who initiates the connection first) and just refer to a peer disabling migration. If we want to address server migration and peer-to-peer, then we may need to make it apply in both directions. @MikeBishop you may want to add @ianswett as a reviewer as he also requested this TP.

Copy link
Member

@nibanks nibanks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @pravb here. I don't think we should make this server specific.


: The server does not support migration of connections to a different IP
address. Clients MUST NOT send any packets, including probing packets, from an
IP address other than that used to perform the handshake. This parameter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems mildly ambiguous -- isn't there a scenario where the client doesn't necessarily have control over that IP? I'm not saying that that's the common case, but it could happen that the client doesn't just have a different port after a NAT rebinding, but that it actually shows up as coming from a different address entirely depending on how things are routed between NATs, especially after inactivity.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think most of my objection here relates to terminology. Perhaps:

The server does not support connection migration ({{migration}}). 
Clients MUST NOT send any packets, including those intended for 
path validation ({{migrate-validate}}), from a local endpoint other
than that used to perform the handshake. ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that one of the motives of having this option is to allow running QUIC servers behind a load balancer that distributes the load based on the 5-tuple.

Assuming that is true, it is necessary to minimize the chance of client rebinding to not only a different IP address but to a different port as well.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kazuho yes 5-tuple or 3-tuple i.e. only IP addresses. Load balancers can operate in both modes for UDP (the latter for handling IP fragmented UDP flows). In the latter case port changes will not cause an issue. Regardless as I said if the server indicates no support for migration then automatically turning on keep-alives should be recommended to deal with NAT rebinding.

@martinthomson
Copy link
Member

I really don't understand this PR. If you don't want someone to use a new connection ID, then don't give them one. I'd be happier saying that endpoints MUST NOT knowingly use a new IP address without a new connection ID.

@pravb
Copy link

pravb commented Jun 15, 2018

@martinthomson are we ruling out use of multiple connection IDs on the same address? That seems too restrictive. I always prefer explicit mechanisms compared to inferred behavior for most flexibility. I also like this PR because it allows the client to automatically turn on keep-alives to protect against NAT rebinding when the server states that it is not capable of supporting migration.

@nibanks
Copy link
Member

nibanks commented Jun 15, 2018

@martinthomson if you go the route of depending on CID availability, it doesn't necessarily give you the indication that even NAT rebinding won't work and that the client should use a more aggressive keep alive timer for long standing connections. I personally prefer the explicit design of this PR.

@martinthomson
Copy link
Member

@pravb, I didn't say anything about multiple connection IDs on the same address. I'm not sure how it has any bearing on this, other than to say that if you don't provide connection IDs, this won't happen. But here's the thing I don't get: if you are routing based solely on 5-tuple, why use connection IDs at all?

If we want a keep-alive function, then we should explicitly build one.

@MikeBishop
Copy link
Contributor Author

Without connection IDs, you're limited to a single session per server per client IP, which aligns poorly with multiple clients behind a NAT. So simply omitting CIDs isn't the best choice.

Not supplying additional CIDs is certainly an option - pick one and use it for the lifetime of the connection. But that goes against our existing advice to rotate CIDs after a period of quiescence.

Keeping the mechanics of CID choice and the client's ability to change its source address separate feels cleanest to me.

@MikeBishop
Copy link
Contributor Author

As to polarity - in this version of QUIC, at least, clients initiate all migrations. The most a server can do is suggest a different IP address to which the client should migrate, which the client can opt not to do. If a client doesn't support migration, it need not tell the server -- it just doesn't migrate.

Should a future / different version of QUIC enable spontaneous server migration or provide a mechanism to force peer migration, it would make sense for this parameter to be bilateral in that version.

Copy link
Member

@nibanks nibanks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@ianswett ianswett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Not giving out connection Ids seems clever, but if that's the only mechanism for disabling migration, then it would seem to discourage people for giving them out for other reasons.

Copy link

@pravb pravb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good since we don't need to handle polarity in this QUIC version.

Copy link
Contributor

@janaiyengar janaiyengar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment inline. Also, I would suggest (as I did in my comment on the issue) that: (i) a server should be allowed to detect a client violation of this TP and close the connection with PROTOCOL_VIOLATION (or a new error if this doesn't sit well with folks), (ii) the text should make it clear that a NAT rebinding may still result in path validation.

@@ -1289,6 +1290,13 @@ preferred_address (0x0004):
: The server's Preferred Address is used to effect a change in server address at
the end of the handshake, as described in {{preferred-address}}.

disable_migration (0x0009):

: The server does not support connection migration ({{migration}}). Clients MUST
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's true that we only support client-driven migrations, but I think there's value in keeping most of the text agnostic to polarity. Is there a reason to disallow this TP in the opposite direction? It'll be a NO-OP, but we don't need to disallow it, I think.

@erickinnear
Copy link
Contributor

@janaiyengar Minor clarification of (ii) above: NAT rebinding may result in a full migration, not just path validation, right? (With the current text indicating that servers are free to recognize that it's just a NAT rebinding and behave accordingly.)

In other words, I think the operative intent here is that the client not initiate migration, with a note that just because the client isn't sending from a different endpoint that the server may still see NAT rebinding.

@janaiyengar
Copy link
Contributor

@erickinnear Yes, though I wouldn't call it a "full migration", because all you have is a server potentially responding to what seems like a migration. Basically Section 6.8.3 applies.

parameter during the handshake. An endpoint which has sent this transport
parameter, but detects that a peer has nonetheless migrated to a different
network MAY treat this as a connection error of type INVALID_MIGRATION.
However, note that not all changes of peer address are intentional migrations.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really think we should mention NAT rebinding here and how disable_migration doesn't mean the endpoint doesn't need to handle NAT rebinding.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! If you send the parameter, you still need to implement a response to path challenge frames, for example. I'll comment on the issue with more thoughts, but the text should at least mention that you still need to be prepared for that to happen.

@rpaulo
Copy link
Contributor

rpaulo commented Jun 23, 2018

The latest addition looks good to me, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport design An issue that affects the design of the protocol; resolution requires consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants