refactor: Track 4-tuples instead of only the remote per path. #264
Conversation
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/quinn/pr/264/docs/iroh_quinn/ Last updated: 2025-12-22T15:00:52Z |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #264 +/- ##
=======================================
Coverage 76.66% 76.66%
=======================================
Files 83 83
Lines 23347 23440 +93
=======================================
+ Hits 17898 17971 +73
- Misses 5449 5469 +20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
matheus23
force-pushed
the
matheus23/4-tuples
branch
from
aea85bb to
84fe7f1
Compare
matheus23
force-pushed
the
matheus23/4-tuples
branch
from
ddef817 to
4f4b8b7
Compare
And add a regression test from proptests
divagant-martian
left a comment
divagant-martian
left a comment
There was a problem hiding this comment.
very partial review so that this can advance a bit. I'm a fan of the direction, but I can't live with the naming of addresses. Left suggestions
| rem_cid: ConnectionId, | ||
| remote: SocketAddr, | ||
| local_ip: Option<IpAddr>, | ||
| addresses: FourTuple, |
There was a problem hiding this comment.
no a fan of the field name, it's very unespecific. What about the unoriginal foutuple or maybe better, network_path
There was a problem hiding this comment.
I love network_path, and thought about it as well.
An original version had tuple: FourTuple, but I changed that to addresses: FourTuple once I saw that that's what the existing code used. (I'm just explaining myself for now reason 🙃 )
Will change it to network_path!
There was a problem hiding this comment.
love it! yes, I know quinn used the addresses one but I'm glad we agree on doing it better 😎
| /// false)` if was opened. | ||
| /// | ||
| /// [`open_path`]: Connection::open_path | ||
| // TODO(matheus23): Adjust docs |
There was a problem hiding this comment.
will this be part of this PR?
| match self | ||
| .paths | ||
| .iter() | ||
| .find(|(_id, path)| addresses.is_probably_same_path(&path.data.addresses)) |
There was a problem hiding this comment.
lines like this make think network_path would work well
|
|
||
| /// Returns the path's remote socket address | ||
| pub fn path_remote_address(&self, path_id: PathId) -> Result<SocketAddr, ClosedPath> { | ||
| pub fn path_addresses(&self, path_id: PathId) -> Result<FourTuple, ClosedPath> { |
There was a problem hiding this comment.
docs need to be adjusted
| } | ||
|
|
||
| /// Check if the 4-tuple path (as in RFC9000 Path, not multipath path) had already been validated. | ||
| fn find_open_path_on_addresses(&self, addresses: FourTuple) -> Option<(&PathId, &PathState)> { |
There was a problem hiding this comment.
suggestion:
find_path_id_on_network_path or find_network_paths_path_id
There was a problem hiding this comment.
It's more than the network path. So perhaps find_path_on_network_path. Will change 👍
| fn find_open_path_on_addresses(&self, addresses: FourTuple) -> Option<(&PathId, &PathState)> { | ||
| self.paths.iter().find(|(path_id, path_state)| { | ||
| path_state.data.validated | ||
| // Would this use the same network path, if addresses were used to send right now? |
| ?remote, | ||
| path_remote = ?self.path(path_id).map(|p| p.remote), | ||
| %addresses, | ||
| %known_path.addresses, |
There was a problem hiding this comment.
what's the field name of this when logged?
There was a problem hiding this comment.
What do you mean? The log line will look something like this:
... addresses=(::1, [::1]:4433) known_path.addresses=(::1, [::ffff, 1.1.1.1]:4433)
There was a problem hiding this comment.
yes that was the question, I didn't know it was going to end up being known_path.addresses. I'm fairly certain @flub will have opinions here 😅
| ); | ||
| return; | ||
| } | ||
| if known_path.addresses.local_ip.is_some() |
There was a problem hiding this comment.
any chance this code can be encapsulated somewhere?, in the path itself partially, maybe?
| } | ||
| if let Some(token) = params.stateless_reset_token { | ||
| let remote = self.path_data(path_id).remote; | ||
| // TODO(matheus23): Reset token for a remote, or for a 4-tuple? |
There was a problem hiding this comment.
reset tokens are per CID, which is per remote, not network path afaiu. I might be wrong, need to check (part of why review is partial)
| path.path_responses.push(number, challenge.0, remote); | ||
| if remote == path.remote { | ||
| path.path_responses.push(number, challenge.0, addresses); | ||
| if addresses == path.addresses { |
There was a problem hiding this comment.
hmm I think this is correct, but a bit tricky. Random comment here that simply means I need to check a bit more this for me to feel confident about this, given what we do track
Description
The main change of this PR is removing
PathData::remotein favor ofPathData::addresseswhich is of typeFourTupleinstead ofSocketAddr.FourTupleis pretty much this struct:(A follow-up should change
local_ipto be anOption<SocketAddr>instead ofIpAddr.)We then track path validation by full 4-tuple instead of by remote address. This is more correct, as seen in the regression test in #258.
Breaking Changes
quinn_proto::DatagramEventInnernow stores aaddresses: FourTupleinstead of aremote: SocketAddr.quinn_proto::Endpoint::handlenow expects aFourTupleinstead ofSocketAddr.quinn_proto::Connection::open_pathandopen_path_ensurenow take aFourTupleinstead ofSocketAddr.quinn_proto::Connection::local_ipwas removed. Look at the paths instead.(A follow-up should also update the quinn API to use full
FourTuples to make it possible to short-circuit path validation.)Notes & open questions
The biggest hurdle in this I can see right now has to do with path validation and path migration on the client side:
dst_ip. This IP address was completely ignored though. And the client kept assuming the path is validated, because it didn't see the path "changing". (All while the server triggered a migration and validated the new network path.)Another hurdle is the fact that this means all APIs like
open_path_ensurenow need to take full 4-tuples, so we can actually distinguish whether we already have a path on that exact network path or not. Again, looking at the remote only is not enough to identify the path!However, this extends into iroh work as well: Now iroh would need to be aware of which local address to use for which path, which means it needs to do something similar to ICE's address pairing, which is somewhere we don't necessarily want to go.
So. Does this mean we'll stay in this half-broken world where we track path validation by remote address only instead of 4-tuple, or do we fully fix it in the future?
We should probably make sure we know which edge cases we can hit in which we're not properly tracking the path's validity. The tricky thing is that some of these things are only happening with active on-path attackers, which RFC 9000 cares about, but is really hard to reason about/test/detect in the real world.