Opened 7 years ago
Last modified 3 years ago
#7478 needs_revision enhancement
Allow routersets to include/exclude nodes by IPv6 address
Reported by: | nickm | Owned by: | |
---|---|---|---|
Priority: | High | Milestone: | Tor: unspecified |
Component: | Core Tor/Tor | Version: | |
Severity: | Normal | Keywords: | tor-client, ipv6 |
Cc: | Actual Points: | ||
Parent ID: | Points: | 1 | |
Reviewer: | mikeperry | Sponsor: |
Description
Right now, routerset_contains_*() and friends only looks at a single tor_addr_t. But nodes now can have more than one address.
The simple rule here would be to have a node considered to be a member of a routerset if *any* of its addresses is contained in the routerset. That would make ExcludeNodes work right, but could get surprising behavior with ExitNodes and the like.
A less simple rule would be to have slightly different semantics for 'include' and 'exclude' routersets, but I don't much like that option: it also seems likely to surprise.
The least simple option would be to treat a node as partially in a set if one of its addresses is included and another isn't. That's a worst-case option IMO: it's likely to be complex and error-prone.
Child Tickets
Change History (50)
comment:1 Changed 7 years ago by
Milestone: | Tor: 0.2.4.x-final → Tor: 0.2.5.x-final |
---|---|
Type: | defect → enhancement |
comment:2 Changed 6 years ago by
Milestone: | Tor: 0.2.5.x-final → Tor: 0.2.6.x-final |
---|
comment:3 Changed 5 years ago by
Milestone: | Tor: 0.2.6.x-final → Tor: 0.2.??? |
---|---|
Summary: | routerset membership tests should consider nodes' addr6 fields → Allow routersets to include/exclude nodes by IPv6 address |
comment:4 Changed 4 years ago by
Milestone: | Tor: 0.2.??? → Tor: 0.2.8.x-final |
---|
comment:5 Changed 4 years ago by
Points: | → small |
---|
comment:6 Changed 4 years ago by
Cc: | teor added |
---|---|
Priority: | normal → major |
Now that #17060 is fixed, this bug is more urgent.
comment:7 Changed 4 years ago by
Severity: | → Blocker |
---|---|
Status: | new → needs_review |
See branch ticket-7478
at https://github.com/fergus-dall/tor.git
comment:8 Changed 4 years ago by
Severity: | Blocker → Normal |
---|
comment:9 follow-up: 10 Changed 4 years ago by
Status: | needs_review → needs_revision |
---|
Thanks for this patch, looks great!
One nitpick: the standard naming convention for IPv6 addresses is ipv6_addr and ipv6_orport. It will help other developers if routerset_contains() uses those argument names for the new arguments.
comment:10 Changed 4 years ago by
Status: | needs_revision → needs_review |
---|
Replying to teor:
One nitpick: the standard naming convention for IPv6 addresses is ipv6_addr and ipv6_orport. It will help other developers if routerset_contains() uses those argument names for the new arguments.
Fixed
comment:11 Changed 4 years ago by
Great! Let's get this merged!
(Thanks for this, it's an important improvement to our IPv6 support.)
nickm, I think this has OK semantics for ExcludeNodes (either address matching excludes), and ExitNodes (either address matching includes).
comment:12 Changed 4 years ago by
Keywords: | TorCoreTeam201602 added |
---|---|
Owner: | set to teor |
Status: | needs_review → assigned |
comment:13 Changed 4 years ago by
Status: | assigned → needs_review |
---|
comment:14 follow-up: 15 Changed 4 years ago by
So, do I need to bug someone about this or something?
comment:15 Changed 4 years ago by
Replying to fergus:
So, do I need to bug someone about this or something?
I scheduled it for review/merge in February, hopefully that does the trick.
comment:16 follow-up: 17 Changed 4 years ago by
Bugfix offers to modify IPv4 function into combined one. Maybe, it's better to add IPv6 function?
comment:17 Changed 4 years ago by
Replying to bugzilla:
Bugfix offers to modify IPv4 function into combined one. Maybe, it's better to add IPv6 function?
We want a combined function because we want to check if a routerset contains a router. Each router has an IPv4 address, and may have an IPv6 address as well. Having two separate functions would require callers to remember to call both, which is error-prone.
comment:18 follow-up: 19 Changed 4 years ago by
@teor:
Sounds reasonable.
Does router have only one IPv4 and/or only one IPv6?
comment:19 Changed 4 years ago by
Replying to bugzilla:
@teor:
Sounds reasonable.
Does router have only one IPv4 and/or only one IPv6?
A router can declare many addresses, but only one IPv4 and an optional IPv6 are published in the consensus and microdescriptors.
comment:20 follow-up: 21 Changed 4 years ago by
Review:
src/or/routerset.c
@@ -275,10 +280,14 @@ routerset_contains_router(const routerset_t *set, const routerinfo_t *ri,
@@ -292,10 +301,14 @@ routerset_contains_routerstatus(const routerset_t *set,
Transform addr
to const pointer (as ipv6_addr
) or vice versa for uniformity.
src/test/test_routerset.c
@1111: int
Avoid int
and other non-fixed-width data types.
comment:21 follow-up: 22 Changed 4 years ago by
Replying to bugzilla:
Review:
src/or/routerset.c
@@ -275,10 +280,14 @@ routerset_contains_router(const routerset_t *set, const routerinfo_t *ri,
@@ -292,10 +301,14 @@ routerset_contains_routerstatus(const routerset_t *set,
Transform
addr
to const pointer (asipv6_addr
) or vice versa for uniformity.
We can't make this change to the patch, because addr requires locally allocated storage, but ipv6_addr does not.
src/test/test_routerset.c
@1111: int
Avoid
int
and other non-fixed-width data types.
This change may cause compiler warnings on some platforms due to casting the int return value to the fixed-width variable. The width of int is irrelevant when it's used as a boolean.
comment:22 follow-up: 23 Changed 4 years ago by
Replying to teor:
We can't make this change to the patch, because addr requires locally allocated storage, but ipv6_addr does not.
N/A transforming ipv6_addr into the same type as addr? No problem.
This change may cause compiler warnings on some platforms due to casting the int return value to the fixed-width variable. The width of int is irrelevant when it's used as a boolean.
It's a general suggestion for the entire project. Is bool
prohibited in Tor?
comment:23 Changed 4 years ago by
Replying to bugzilla:
Replying to teor:
This change may cause compiler warnings on some platforms due to casting the int return value to the fixed-width variable. The width of int is irrelevant when it's used as a boolean.
It's a general suggestion for the entire project. Is
bool
prohibited in Tor?
Then it's one for the proposals process, or a technical / style guide discussion, not this particular ticket.
comment:24 Changed 4 years ago by
I started reviewing this and I have a question - In routerset_contains_router() the behavior is changed with respect to 0.0.0.0 IPv4 addresses. Previously, we let those addresses be handled by compare_tor_addr_to_addr_policy() which has logic to still accept those addresses in some cases (since 0-addrs are treated as wildcards in some places in the code, but not others, depending mostly on the port in use). Now we shortcut that and always say they are not present in the set. A similar issue may exist for 0-addr IPv6 addresses..
I am not sure if this actually matters yet, since I haven't read all of the related branches of code yet. Right now, my next place to look is exit policy checks against 0.0.0.0:*, but there may be other cases in the current codebase or in future code that may trip up with this change, expecting wildcard behavior to be preserved in some cases but not others. Noting this now because it is my EOD and maybe someone has a good explanation. Maybe we're actually fixing a bug here. But otherwise, I recommend holding off on merge until we know for sure. At the very lest, this is something that calls detailed commenting, IMO.
In any event, I will dig deeper tomorrow.
comment:25 Changed 4 years ago by
Status: | needs_review → needs_information |
---|
comment:26 follow-up: 27 Changed 4 years ago by
Milestone: | Tor: 0.2.8.x-final → Tor: 0.2.9.x-final |
---|---|
Status: | needs_information → needs_revision |
As IPv6 client support is experimental, I don't think we need this in 0.2.8.
Let's revise this patch so it handles 0.0.0.0 like it used to, and handles ::
(IPv6 all zeroes) the same as 0.0.0.0.
comment:27 Changed 4 years ago by
Replying to teor:
Let's revise this patch so it handles 0.0.0.0 like it used to, and handles
::
(IPv6 all zeroes) the same as 0.0.0.0.
That won't work. If a node doesn't have an IPv6 address, then both ipv6_addr
and ipv6_orport
will be zero, which compare_tor_addr_to_addr_policy
always rejects (placing it in the routerset) and similarly with a hypothetical future node that doesn't have an IPv4 address.
Both routerinfo_t
and routerstatus_t
contain their addresses directly instead of using pointers, so there's no way to distinguish between "NULL because this node lacks an address of this type" and "NULL because this node's address and port are unknown". I think we just have to treat the second case as a bug.
I've changed the short circuit to only occur when both the IP address and port are zero and added a comment to that effect. A rebased branch is available as ticket-7478-2
at https://github.com/fergus-dall/tor.git
comment:28 Changed 4 years ago by
Status: | needs_revision → needs_review |
---|
comment:29 Changed 4 years ago by
Keywords: | TorCoreTeam201603 added; TorCoreTeam201602 removed |
---|
Looks good - I think this addresses mikeperry's comment, but I'd like him to have another review before we send it on to Nick.
comment:30 Changed 4 years ago by
Keywords: | TorCoreTeam201604 added; TorCoreTeam201603 removed |
---|
comment:31 Changed 4 years ago by
Reviewer: | → mikeperry |
---|
Setting mike to reviewer based on teor's comment:29.
comment:32 Changed 4 years ago by
Keywords: | TorCoreTeam201605 TorCoreTeam-postponed-201604 added; TorCoreTeam201604 removed |
---|
April is over; calling these april tickets postponed into may.
comment:33 Changed 4 years ago by
Keywords: | review-group-1 added |
---|
comment:34 Changed 4 years ago by
Points: | small → 1 |
---|
comment:35 Changed 4 years ago by
Sorry for the delay, and thank you for your patience, fergus.
This may be an improvement, but I am still wondering about the "reject *:*" exitpolicy case. In that case, both the port and the IP would be 0. But I think that 0:0 address is never used as arguments to this function, but is just in the set itself. If that is true, then I think we're all good here.
comment:36 Changed 4 years ago by
Keywords: | review-group-2 added; review-group-1 removed |
---|
Everything in review-group-1 has had at least 1 review.
comment:37 Changed 4 years ago by
Begin review of branch at https://github.com/fergus-dall/tor/commits/ticket-7478-2:
3ed42323dbe225cb8cc6b26eac8b9f535065f1cc:
- I believe this code is correct, but I'd feel better about it if it made
routerset_contains_router() call routerset_contains() twice and
||
the results rather than adding a second addr/orport to routerset_contains(), for the sake of keeping interfaces clean. This approach will generalize much more cleanly if we ever have cases where routers have more than two addresses to check as well.
0e4a2063d6e476d91386a67c633899052ffd708a:
- New unit tests for routerset_contains(); looks okay.
276c53829c50197491ceb18f917c2adc36cc499d:
- This just renames some parameters
9c4a7c174955666240dfa9605d9e5ffa64a2dbc0:
- Adds null checks to routerset_contains(). This is important given the new interface to make it possible to disable checking one set of args, but I really prefer keeping routerset_contains() clean and calling multiple times when there's more than one address to check.
6dd42e85ea26d3b026417c8b3088c7b539bfadac:
- Unit test fixups after 9c4a7c174955666240dfa9605d9e5ffa64a2dbc0
End code review
Summary: I think this patch will work but I don't really like crufting up
routerset_contains() this way - but I also don't have a totally good feel
about routerset_contains() in general. Perhaps we should have a simpler
routerset_contains_addr() or something like that for clarity, since we have
other queries against a routerset like routerset_contains_router().
comment:38 Changed 4 years ago by
Status: | needs_review → needs_revision |
---|
comment:39 Changed 4 years ago by
The routerset_contains_*
functions are all just wrappers around routerset_contains
that pull the information from different data structures. routerset_contains
itself isn't in the public part of the header file or called by anything else.
An issue I have just noticed is that the extend_info_t
struct only has the address being connected to, but it needs to be checked against ExcludeNodes, which could cause it to be used if only the other address is excluded. I'm not really sure what to do about that.
comment:40 Changed 4 years ago by
Keywords: | TorCoreTeam201605 removed |
---|
Remove "TorCoreTeam201605" keyword. The time machine is broken.
comment:41 Changed 3 years ago by
Keywords: | review-group-3 added; review-group-2 removed |
---|
These have all had at least one review during review-group-2.
comment:42 Changed 3 years ago by
Cc: | teor removed |
---|---|
Owner: | teor deleted |
Status: | needs_revision → assigned |
I don't have time to revise this in 0.2.9.
comment:43 Changed 3 years ago by
Status: | assigned → needs_revision |
---|
comment:44 Changed 3 years ago by
Keywords: | nickm-deferred-20160905 added |
---|---|
Milestone: | Tor: 0.2.9.x-final → Tor: 0.2.??? |
Deferring many tickets that are in needs_revision with no progress noted for a while, where I think they could safely wait till 0.3.0 or later.
Please feel free to move these back to 0.2.9 if you finish the revisions soon.
comment:46 Changed 3 years ago by
Keywords: | tor-03-unspecified-201612 added |
---|---|
Milestone: | Tor: 0.3.??? → Tor: unspecified |
Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.
comment:47 Changed 3 years ago by
Keywords: | tor-03-unspecified-201612 removed |
---|
Remove an old triaging keyword.
comment:48 Changed 3 years ago by
Keywords: | nickm-deferred-20160905 removed |
---|
comment:49 Changed 3 years ago by
Keywords: | TorCoreTeam-postponed-201604 removed |
---|
comment:50 Changed 3 years ago by
Keywords: | review-group-3 removed |
---|
It appears not to be possible to actually pass an IPv6 address into a routerset. So we don't need to worry about users thinking that they're excluding a node that they aren't, which would be the bad behavior here. Deferring to 0.2.5, marking as enhancement.