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 nickm

Milestone: Tor: 0.2.4.x-finalTor: 0.2.5.x-final
Type: defectenhancement

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.

comment:2 Changed 6 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.6.x-final

comment:3 Changed 5 years ago by nickm

Milestone: Tor: 0.2.6.x-finalTor: 0.2.???
Summary: routerset membership tests should consider nodes' addr6 fieldsAllow routersets to include/exclude nodes by IPv6 address

comment:4 Changed 4 years ago by nickm

Milestone: Tor: 0.2.???Tor: 0.2.8.x-final

comment:5 Changed 4 years ago by nickm

Points: small

comment:6 Changed 4 years ago by nickm

Cc: teor added
Priority: normalmajor

Now that #17060 is fixed, this bug is more urgent.

comment:7 Changed 4 years ago by fergus

Severity: Blocker
Status: newneeds_review

See branch ticket-7478 at https://github.com/fergus-dall/tor.git

comment:8 Changed 4 years ago by fergus

Severity: BlockerNormal

comment:9 Changed 4 years ago by teor

Status: needs_reviewneeds_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 in reply to:  9 Changed 4 years ago by fergus

Status: needs_revisionneeds_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 teor

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 teor

Keywords: TorCoreTeam201602 added
Owner: set to teor
Status: needs_reviewassigned

comment:13 Changed 4 years ago by teor

Status: assignedneeds_review

comment:14 Changed 4 years ago by fergus

So, do I need to bug someone about this or something?

comment:15 in reply to:  14 Changed 4 years ago by teor

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 Changed 4 years ago by bugzilla

Bugfix offers to modify IPv4 function into combined one. Maybe, it's better to add IPv6 function?

comment:17 in reply to:  16 Changed 4 years ago by teor

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 Changed 4 years ago by bugzilla

@teor:
Sounds reasonable.
Does router have only one IPv4 and/or only one IPv6?

comment:19 in reply to:  18 Changed 4 years ago by teor

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 Changed 4 years ago by 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 (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 in reply to:  20 ; Changed 4 years ago by teor

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 (as ipv6_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 in reply to:  21 ; Changed 4 years ago by bugzilla

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 in reply to:  22 Changed 4 years ago by teor

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 mikeperry

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 mikeperry

Status: needs_reviewneeds_information

comment:26 Changed 4 years ago by teor

Milestone: Tor: 0.2.8.x-finalTor: 0.2.9.x-final
Status: needs_informationneeds_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 in reply to:  26 Changed 4 years ago by fergus

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 fergus

Status: needs_revisionneeds_review

comment:29 Changed 4 years ago by teor

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 nickm

Keywords: TorCoreTeam201604 added; TorCoreTeam201603 removed

comment:31 Changed 4 years ago by nickm

Reviewer: mikeperry

Setting mike to reviewer based on teor's comment:29.

comment:32 Changed 4 years ago by nickm

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 nickm

Keywords: review-group-1 added

comment:34 Changed 4 years ago by isabela

Points: small1

comment:35 Changed 4 years ago by mikeperry

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 nickm

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 andrea

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().

Last edited 4 years ago by dgoulet (previous) (diff)

comment:38 Changed 4 years ago by dgoulet

Status: needs_reviewneeds_revision

comment:39 Changed 4 years ago by fergus

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.

Last edited 4 years ago by fergus (previous) (diff)

comment:40 Changed 4 years ago by nickm

Keywords: TorCoreTeam201605 removed

Remove "TorCoreTeam201605" keyword. The time machine is broken.

comment:41 Changed 3 years ago by nickm

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 teor

Cc: teor removed
Owner: teor deleted
Status: needs_revisionassigned

I don't have time to revise this in 0.2.9.

comment:43 Changed 3 years ago by teor

Status: assignedneeds_revision

comment:44 Changed 3 years ago by nickm

Keywords: nickm-deferred-20160905 added
Milestone: Tor: 0.2.9.x-finalTor: 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:45 Changed 3 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:46 Changed 3 years ago by nickm

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 nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:48 Changed 3 years ago by nickm

Keywords: nickm-deferred-20160905 removed

comment:49 Changed 3 years ago by nickm

Keywords: TorCoreTeam-postponed-201604 removed

comment:50 Changed 3 years ago by nickm

Keywords: review-group-3 removed
Note: See TracTickets for help on using tickets.