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

New connection IDs are mandatory for intentional migration #2414

Merged
merged 12 commits into from
Mar 11, 2019

Conversation

martinthomson
Copy link
Member

Based on #2386, which should merge soon.

Closes #2413.

martinthomson and others added 4 commits January 30, 2019 12:25
Co-Authored-By: martinthomson <mt@lowentropy.net>
I could have sworn we agreed to this previously, but I also can't find
where we captured it.  So here it is.  This was originally an editorial
factoring of text, but I guess it needs to be a design issue now.

Closes #2413.
@martinthomson martinthomson added design An issue that affects the design of the protocol; resolution requires consensus. -transport labels Feb 4, 2019

Clients MAY move to a new connection ID at any time based on
implementation-specific concerns. For example, after a period of network
inactivity NAT rebinding might occur when the client begins sending data again.
inactivity, where NAT rebinding might occur when the client begins sending data
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
inactivity, where NAT rebinding might occur when the client begins sending data
inactivity, NAT rebinding may cause an unintentional change in path when the client begins sending data

that packet numbers cannot be used to correlate activity. This does not prevent
other properties of packets, such as timing and size, from being used to
correlate activity.
An endpoint MUST use a new connection ID if it initiates connection migration,
Copy link
Contributor

Choose a reason for hiding this comment

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

I would move the conditional clause to the beginning, otherwise it's not clear what the "This" in the next sentence refers to.

"Unless the peer has selected a zero-length connection ID, an endpoint MUST use ..."

Also, how about adding something about what the endpoint needs to do if it has nothing from the peer? Something like: "If no new connection IDs are available, the endpoint MUST NOT migrate. The endpoint MAY close the connection, but MUST NOT send any packets on a new path with the same connection ID as that used on an earlier path." I can't find any text that says as much. I wonder if this is a new non-editorial issue, since we currently do not have normative text about this (maybe I just can't find it yet).

Copy link
Member Author

Choose a reason for hiding this comment

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

I've just updated the text below to cover the MUST NOT migrate case (I used "cannot" before).

migrate. If a peer chooses to receive packets with zero-length connection IDs,
an endpoint can always migrate. To ensure that migration is possible and
packets sent on different paths cannot be correlated, endpoints SHOULD provide
new connection IDs before peers migrate.
Copy link
Contributor

Choose a reason for hiding this comment

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

This text is good, but still doesn't have normatives, and I think we should have MUST NOT.

changes.
An endpoint cannot migrate to a new path if it does not have a connection ID to
use on that path. An endpoint that exhausts available connection IDs cannot
migrate. If a peer chooses to receive packets with zero-length connection IDs,
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 it makes sense to put this 3rd sentence first, and then say, "Otherwise, an endpoint cannot migrate..."

The current text reads in a somewhat self-contradictory way.

connection IDs before peers migrate.

If a peer chooses to receive packets with zero-length connection IDs, an
endpoint can always migrate - zero-length connection IDs provide no significant
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
endpoint can always migrate - zero-length connection IDs provide no significant
endpoint can always migrate, since zero-length connection IDs provide no significant

Copy link
Contributor

@MikeBishop MikeBishop left a comment

Choose a reason for hiding this comment

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

As noted in #2413, this PR permits something that's currently prohibited intentionally -- migrating with a zero-length CID. Do we have consensus to make that change?

If there's no CID and the packet is coming from an unknown remote address, how does the server know what keys to use to remove packet protection in that case? We've previously said we need to avoid trial decryption, and that seems like the only option if migration is permitted.

@mikkelfj
Copy link
Contributor

mikkelfj commented Feb 8, 2019

I think this strikes a good balance. It isn't practical to forbid migration without CID when NAT's do it behind your back, but allowing it for intentional migration would work against networks attempting to move towards safer modes of operation in the future, some which might only make sense if QUIC does indeed require new CID's on migration.

At some point with IPv6, NAT rebinding might be seen the same way as older insecure WLAN standards.

Endpoints MAY move to a new connection ID at any time.

An endpoint MUST use a new connection ID if it initiates connection migration,
unless the peer has selected a zero-length connection ID. Using a new
Copy link
Contributor

Choose a reason for hiding this comment

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

Above we say you can't migrate if you use a zero-length connection ID, so I don't understand the purpose of "unless the peer has selected a zero-length connection ID."

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, trimmed it down.

: If both endpoints change connection ID in response to seeing a change in
connection ID from their peer, then this can trigger an infinite sequence of
changes.
An endpoint MUST NOT intentionally migrate to a new path if it does not have a
Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence seems duplicative of the paragraph above starting with "An endpoint MUST use a new connection ID ..."

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.

One small suggestion, but I think this LG.

the same connection on different networks. Header protection ensures
that packet numbers cannot be used to correlate activity. This does not prevent
other properties of packets, such as timing and size, from being used to
Endpoints MAY move to a new connection ID at any time.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: move to may not be clear. How about "Endpoints MAY change the Destination Connection ID they send in short header packets at any time." assuming that statement is correct.

Copy link
Contributor

@erickinnear erickinnear left a comment

Choose a reason for hiding this comment

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

Looks like good clarification, just one minor wording comment

@@ -1853,7 +1853,9 @@ An endpoint also MUST NOT initiate connection migration if the peer sent the
`disable_migration` transport 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.
INVALID_MIGRATION. Similarly, an endpoint MUST NOT migrate if its peer supplies
Copy link
Contributor

Choose a reason for hiding this comment

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

It would probably be worth indicating that this is intentional migration, just to keep everything clear. Elsewhere I think we've said an endpoint must not "initiate" migration, since of course its peer may think it migrated due to a NAT.

@larseggert
Copy link
Member

This seems to have consensus, tagging as such and reassigning.

@larseggert larseggert added the has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list. label Feb 13, 2019
@mnot
Copy link
Member

mnot commented Feb 18, 2019

Removing has-consensus tag as it needs to be canvassed on-list (and attached to an issue, not a PR).

@mnot mnot removed the has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list. label Feb 18, 2019
@larseggert
Copy link
Member

I'm sorry if I misunderstood the new process, but I don't think your PR says that an issue must be discussed on the mailing list? Doesn't it say that the discussion can happen on the list or on the issue?

@mnot
Copy link
Member

mnot commented Feb 19, 2019

@larseggert discussion can happen in either place, but we've always checked with the list before declaring consensus (which is what has-consensus records). In the currently operative process, that's happening (admittedly sporadically) after the drafts are published.

@janaiyengar
Copy link
Contributor

@mnot @larseggert : The associated issue is #2413 and there's been a mailing list discussion. There seems to be some disagreement on what needs to happen next.

Specifically, some folks think this PR is an editorial one, and some want to open up the issue as a design issue for discussion.

@mnot
Copy link
Member

mnot commented Feb 19, 2019

#2413 is a design issue. If this is just editorial and no one disputes that, you can apply it as that, but it can't close #2413.

@martinthomson
Copy link
Member Author

#2413 is only marked design because there is debate, therefore, I conclude that we need chairs to help us. I requested that close to 2 weeks ago.

@MikeBishop
Copy link
Contributor

The requirement previously existed, but was accidentally dropped during one of the CID rewrites. The underlying question is whether we had consensus to remove the requirement previously -- if so, then reversing ourselves requires consensus. If not, then it's just fixing an accidental deletion.

@ianswett
Copy link
Contributor

If the PR that removed it doesn't have a note about removing it, I'd consider it an accidental deletion(which is what I suspect).

@erickinnear
Copy link
Contributor

I thought the overall requirement for changing CIDs was still in the text, so it doesn't seem like the issue is around adding/removing a requirement, but rather the rest of the accompanying text here?
Otherwise, not sure how this isn't entirely editorial, since the main requirement is already in place.

@martinthomson martinthomson merged commit 2575888 into master Mar 11, 2019
@martinthomson martinthomson deleted the new-path-new-cid branch March 11, 2019 22:52
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

8 participants