Allow routersets to include/exclude nodes by IPv6 address

[nickm]

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.

[nickm]

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.

[nickm]

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

[fergus]

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

[teor]

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.

[fergus]

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

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

[fergus]

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

[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.

[bugzilla]

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

[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.

[bugzilla]

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

[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.

[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.

[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.

[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?

[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.

[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.

[teor]

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.

[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

[teor]

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.

[nickm]

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

[nickm]

April is over; calling these april tickets postponed into may.

[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.

[nickm]

Everything in review-group-1 has had at least 1 review.

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

[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.

[nickm]

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

[nickm]

These have all had at least one review during review-group-2.

[teor]

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

[nickm]

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.

[teor]

Milestone renamed

[nickm]

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

[nickm]

Remove an old triaging keyword.

[gaba]

Remove MikePerry from reviewer from all this old tickets.