[tor-dev] Tor AF independence patch - first big step to Tor IPv6

[Nick Mathewson]

On Wed, Jun 8, 2011 at 8:19 AM, Jeroen Massar <jeroen at unfix.org> wrote:
> Hi,
>
> As this is World IPv6 day, let me present the first big step to Tor
> IPv6: the Address Family independence Patch ;)
>
> https://unfix.org/projects/ipv6/tor/tor-af-independent.diff
>
> it is diff against a recent git checkout and should apply more or less
> cleanly.

Hi!

Do you remember which git checkout it was?  I can't find one that it
applies cleanly to.

 [...]
> Note also that this is not a 'true' AF indepencence patch as in that
> case we would have to swap toraddr_t with a sockaddr_storage structure,
> which, when recompiled, would be true AF independent. In the case though
> that ever a new IP-alike protocol arises and then we still use BSD style
> sockets, this patch should make it easy to use that new address family
> too, but don't hold your breath ;)

Yeah, I wouldn't hold my breath for a shift from sockaddr_storage from
tor_addr_t.  The point of tor_addr_t is to be small; it's about 20
bytes (as opposed to the ~128 bytes used for a sockaddr_storage).  We
allocate a pretty huge number of addresses, and using a needlessly
large type here could waste a few megabytes  on a busy relay.

 [...]
> A question there also becomes, do we want to show a Tor node as separate
> IPv4 and IPv6 routers, or are they to be seen as one, if it is one, we
> require the above ORMultiport, so that we can have multiple IP addresses
> and ports on a single node.

It's one router; it has to be.  After all, it needs to be able

 [...]
> And maybe, it could be useful to have a special branch on torproject's
> git server for this, as it is quite a bit of patch ;)

This should definitely get a git repository someplace.  In the future,
if it's possible, please try to do stuff like this as a series of
patches rather than as one huge patch.  Stuff this big is hard to
merge and hard to review.

Some initial thoughts:

* I think that the code in tor_addr_parse_reverse_lookup_name is
wrong: it should call tor_addr_from_ipv4h, not _ipv4n.  The 'h' means
"host order", and you're constructing the address in host order.

* In tor_addr_from_loopback, I really don't like the type-punning from
array-of-uint32 to array-of-byte.

* The tor_inet_pton and tor_inet_ntop functions were meant to be
platform-independent clones of inet_pton and inet_ntop; now they do
something different.  Not sure that's what we want.

* tor_addr_mask_get_bits looks like it'd be broken for anything but an
IPv4 address.  That's fine -- we don't expect other masks to be used
except for IPv4 -- but it should probably check its input.

* tor_addr_from_null() seems to be used in someplaces where af_unspec
would make more sense than af_inet.

* I think tor_addr_protocol and friends can get the boot unless we
actually find a platform we care about where AF_* is not the same as
PF_*.  As near as I can tell, there are no platforms like that.
Instead of making our address-family-to-protocol-family code generic,
we should just axe it.

* Some of the new code in tor_inet_pton is duplicated from elsewhere,
and has the same bug.  We try to avoid copy-paste code.  (Also, the
reason to compare the sscanf output to 4 is to see if there is an
extra character or no.)

* In some places (I noticed this in
get_configured_bridge_by_addr_port) you've replaced !tor_addr_compare
with !tor_addr_eq.  But that's backwards; compare returns 0 when stuff
is equal and eq returns 1 when it's equal.  There are a bunches of
cases like this.  If this was a patch series instead of a great big
patch, then I could just fix that one patch.  As it is, I don't dare
even think about merging it, because it has this error scattered
throughout.

* Changing resolve_my_addr to return an arbitrary address type will
break lots of stuff; it needs to return only IPv4 addresses until the
rest of the code learns how to handle them.  After all, these are the
addresses we advertise.   Same with last_interface_ip, I think.  In
general, lots of functions that keep track of "my address" or "my last
address" will instead need to keep track of "my addresses" plural.

* I think that removing the "if (last_resolved_addr != *addr_out) {"
in resolve_my_address() is wrong.  Previously, the second block would
get invoked when we changed from "no address" to addr_out.  Now it
only gets called when we change from "some address".  I see later that
we initialize our "last_resolved_addr" to loopback: why?  Can that
really be safe?

* The virtaddress_entry_t change feels weird; I'll need to come back
and revisit that.  The conversion in parse_virtual_addr_network etc
also look half-finisheed: it seems like it can only handle ipv4
addresses.

* All of the connection_ap_handshake_socks_resolved changes seem to
imply that it's trying to handle looking up and connecting to IPv6
addresses already.  Odd!

* Converting generate_v2_networkstatus_opinion(), and other functions
that generate directory objects, seems dangerous to me.  As it stands,
they must never output an IPv6 address in any field where clients
currently only accept IPv4 addresses.  As such, using the generic
functions here seems potentially error-prone.

* Our eventdns.c was forked from Libevent's, which was necessary for a
time, but is not a long-term good idea.  I'd like to avoid changes in
eventdns.c that don't correspond to changes in Libevent's eventdns.c .

* The geoip.c file should IMO just be ignored for now; we don't have a
source of IPv6 geoip information.  If we did, we'd probably want a
more optimal representation than one that made every geoip_entry_t
become 5 times as long.  That's an extra 5MB to store the IPv4 geoip
table with this representation!

* In router_pick_published_address, can it really now pick an IPv6
address?  That would cause trouble with existing clients.  Here too,
we can't treat all AFs as equivalent, since existing clients only
allow IPv4.

* All of the routerparse.c changes need to be tighter: we can't start
allowing IPv6 addresses in places that previously supported IPv4
addresses, since old clients won't accept them.

And that's it for my comments on v1 -- looks like a good beginning!
Do you have time to clean this stuff up soon, or shall I start hacking
on it?  I'd like to get IPv6 support into 0.2.3.x if at all possible.

yrs,
-- 
Nick