Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Migration with zero-length CID is inadvisable #3563
Migration with zero-length CID is inadvisable #3563
Changes from 1 commit
455fb97
caf949d
492c093
3722988
8ec6c3a
7d5089a
d29840a
b400f09
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 the first reason makes sense here. If the server routes incoming packets using source addresses, then it should send the disable_migration transport parameter. That's true regardless of connection ID length. We shouldn't make clients worry about about how the server does its routing.
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 specifying the client reaction to the situation when disable_active_migration is not set, that's all. Mostly the second reason applies here, as you are right that the server is responsible for that, but it doesn't mean that it is untrue.
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 the spec should encourage clients to build workarounds to broken servers that don't send the right transport parameters. If disable_migration isn't sent by the server, then the client needs to be able to safely assume that the server will try to make routing on this connection work across migrations.
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.
These are both things that might be the case in some situations, but also have situations where they might work.
That said, it does seem like this first one is a bit different from the second one -- for the first one there's always going to be some risk that your packets don't get to the right place, hence the encouragement to probe the new path first.
Yes, that could be a server that was supposed to set the TP, but even if it wasn't, having a zero length CID isn't really the thing that says "be careful here", and saying SHOULD NOT migrate in such a case seems a bit excessive. If we want to say "be careful, if someone screwed up their routing for whatever reason (which might correlate more with zero length CIDs) you might not hear anything back" then that's not unreasonable, but I think that's already kind of described in the whole path validation process -- you want to make sure the packets really got to the person with whom you've got 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.
What Eric said. This alone doesn't really motivate the SHOULD NOT; it is just a reason that you might come to regret ignoring it; what matters is the other point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DavidSchinazi : I don't think the spec is encouraging building around broken behavior. The actions are the same at the client whether the rationale is #1 or #2. The rationale simply explains the SHOULD NOT, and is not a concern for the client. Can you perhaps suggest a text change to clarify what you would like to see 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.
Here is a text proposal that addresses my concerns:
This provides the same normative guidance, but makes it clear to client implementors that the only motivation for this recommendation is linkability.
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 David. I think that is clearer and have tweaked it. I'm not sure that the last statement is entirely correct, because - at least in theory - there are systems with properties similar to connection IDs. However, I'm OK with adding that on the assumption that an endpoint will need to be aware of the scheme so that they can participate in 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.
In contrast to my comment on the first part, saying SHOULD NOT do trivially linkable things seems reasonable -- by default, don't, and if you've got some reason to believe that you don't care or can avoid that linkability, then that's on you.