Socket rebind: drain old socket #1801
Conversation
If I disable the anti-amplification check, then the server sends stream frames much sooner. Since our client only sends ACKs (which results it a low amount of data), the only way to avoid the blocking of the anti-amplification check is to pre-validate the path. So we need to implement #1772 after all. This is the packet log without the anti-amplification check: |
There was a problem hiding this comment.
Thanks for the PR! This generally makes sense. Minor note, the diff will be a bit more readable if you set #1800's branch as the target, or just rebase onto main if the changes don't actually interact.
I'm wondering if it might be cleaner to use a single unchanging socket bound to a wildcard IP, and have each outgoing datagram set its source IP address explicitly. This would avoid the need for two separate receive paths, and allow connections to operate concurrently on multiple addresses on the same endpoint. It would break support for active migration on platforms which don't support IPV6_PKTINFO or equivalent, but most platforms do. This would probably be a larger change, so perhaps best left for future work even if it sounds good.
I haven't known that's possible. Thanks.
I'll probably go this way.
I'm OK with this. But does it eliminate use-cases when the client does not want to receive traffic on a specific network interface due to security reasons? Hm. Maybe the client could filter incoming packets based on their destination address. And what if the client wants to move from port A to port B? We used to test migration with moving from 127.0.0.1:A to 127.0.0.1:B. Although this might not be relevant in practice.
If there are multiple socket to listen to, one can use the At any rate, maybe it is enough for this pull request to query the two sockets in sequence. |
Good question. This can be solved at the host firewall level, but people are accustomed to specifying a narrow listen address automatically getting you strong isolation without any extra effort, so this could be a footgun. Packets from unexpected peers do get dropped quickly, but this would arguably remove a layer of defense-in-depth.
Another good question. It's not obvious to me if there's a use case here, but maybe if there's weird firewall rules, or the application is actively trying to break linkability? We're getting this at the cost of the capability to have different connections on different paths in the same endpoint, but an applications can always choose to have multiple endpoints. It sounds like sticking with multiple sockets along the lines drafted here is the safer bet, even if it is a little ugly. We may want to revisit this for multipath in the future when concurrent outgoing paths are more important.
Correct. The underlying async runtime is responsible for notifying us when any operation that we got a |
nemethf
force-pushed
the
planned_migration
branch
2 times, most recently
from
9ec0666
to
aa30d49
Compare
|
I force-pushed a new version, which eliminates the code duplication of the previous version as requested. Due to my limited rust knowledge I struggled with the refactorization, and the result is probably not idiomatic rust. Just like upstream main version, it does not call |
nemethf
force-pushed
the
planned_migration
branch
from
aa30d49
to
d1290a4
Compare
nemethf
force-pushed
the
planned_migration
branch
2 times, most recently
from
e101abe
to
1ae0e4b
Compare
nemethf
force-pushed
the
planned_migration
branch
from
1ae0e4b
to
aa30d49
Compare
|
Still needs a rebase onto current main. If you're in it, can you change the order of the methods to have |
nemethf
force-pushed
the
planned_migration
branch
from
aa30d49
to
3702193
Compare
I've just done it. However, I just wasn't able to move
Sorry, I don't know what happened, I seem to pushed an old version. |
There was a problem hiding this comment.
What if the endpoint has multiple outgoing connections to multiple servers? Should we hold on to abandoned_socket until we've authenticated a post-migration packet from each live connection?
This PR is an improvement over the status quo regardless, so it's okay to leave that for future work, but something to consider.
Unfortunately I haven't thought about multiple connections. If a connection is successfully migrated to the new socket, I think the client should not accept packets from the old socket for that connection to avoid potential malicious activity. So handling multiple outgoing connections will be a bit complex. Instead of force-pushing a new version, I pushed separate commits to address your review points. I think it is easier to understand them this way. I can merge into one commit in the end if necessary. As an additional improvement it would be good not to create |
There was a problem hiding this comment.
Thanks, I think the end state of this looks good now. I think rewriting history as follows, with roughly the same end state, would provide the greatest clarity to support review for correctness:
- Introduce
PollState, movingdrive_recvonto it (plus a socket argument) without changing behavior - Reintroduce a method on
Stateand move the logic to start/end therecv_limitercycle into it - Introduce
abandoned_socketand associated logic
If that's beyond what you're comfortable with/motivated for, let me know and I can have a go at it myself.
Instead of force-pushing a new version, I pushed separate commits to address your review points.
Quinn is large enough that we can't usually keep the state of every open PR memorized, so it's much easier to re-review from scratch than to try to judge incremental revisions based on half-remembered context. As a bonus, this makes it easier for new reviewers to get involved in a PR, and allows more useful history to be merged.
This workflow does make it harder to see how a specific comment was addressed. A better code review tool would address both requirements, but we make do.
As an additional improvement it would be good not to create iovs with each poll_socket() call, but I was not able to factor this out into PollState::new().
That would be out of scope for this PR anyway, no worries.
These steps sound logical, but I just cannot implement step 1 without step 2. So it would be great if you cloud take it over. Thanks. |
Ralith
force-pushed
the
planned_migration
branch
4 times, most recently
from
8193b11
to
0ec3a0e
Compare
|
Split up the changes as described, and addressed my own outstanding feedback. I still need to do a final once-over for correctness, though. |
Ralith
force-pushed
the
planned_migration
branch
3 times, most recently
from
a256d78
to
451104e
Compare
|
Did a little more readability cleanup and satisfied myself as far as correctness goes. |
Ralith
force-pushed
the
planned_migration
branch
3 times, most recently
from
b4d30d7
to
9bc790d
Compare
During a planned/active connection migration allow the client to receive trafic via the old, abandoned socket until the first packet arrives on the socket.
Ralith
force-pushed
the
planned_migration
branch
from
9bc790d
to
fa155ea
Compare
During a planned connection migration allow the client to receive
trafic via the old, abandoned socket until the first packet arrives on
the new socket.
I wrote this PR in a "cargo cult"-style without really knowing what I do. It seems to work as the client starts to acknowledge stream frames of the old path on the new path. However, the server does not send stream frames right after it notices the new path even if I modify the server to process acknowledgements when there's an outstanding path challenge here.
Packet No. 1828, below, is the first packet arriving to the server on the new path. The server sends path-challenges in 1891 (allowing linkability) and 1893. The path validation for the new path arrives in 1910. The server sends the first stream frame on the new path in 1913 with a repeated path challenge, so I assume it haven't processed the path response frame yet.
I'd like to see stream frames as early as packet 1893. What should I do? Thank you.
This is the server-side packet trace: