Opened 7 years ago

Closed 6 years ago

#6757 closed defect (fixed)

ClientPreferIPv6ORPort not honoured when running with bridges

Reported by: ln5 Owned by:
Priority: Medium Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version: Tor: 0.2.4.1-alpha
Severity: Keywords: ipv6 tor-client
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Since get_configured_bridge_by_routerinfo() returns the same bridge
regardless of which routerinfo we pass to it, the first found
(seemingly the one configured first in the config file).

One effect of this is that rewrite_node_address_for_bridge() will
prefer whatever is listed first. Looking at ClientPreferIPv6 wouldn't
make sense here since we come here once (only one routerinfo is
received), unless we start calling it for every configured
bridge. That doesn't seem like the right thing though.

Note that the idea of having ipv6_preferred as a property of the
node_t is that we'd like to be able to change its value, per relay,
when we learn about how we have been in touch with the relay.

Child Tickets

Change History (14)

comment:1 Changed 7 years ago by arma

Component: - Select a componentTor Bridge

comment:2 Changed 7 years ago by nickm

Milestone: Tor: 0.2.4.x-final

comment:3 Changed 7 years ago by ln5

Also, in proposed patch set for #5535 we never reset
node_t->ipv6_preferred once it's been set (in
rewrite_node_address_for_bridge()).

comment:4 in reply to:  description ; Changed 7 years ago by ln5

Component: Tor BridgeTor Client
Keywords: ipv6 added
Status: newneeds_review
Version: Tor: 0.2.4.1-alpha

Replying to ln5:

Since get_configured_bridge_by_routerinfo() returns the same bridge
regardless of which routerinfo we pass to it, the first found
(seemingly the one configured first in the config file).

Please ignore that sentence. It's both confusing and pointless.

In 0.2.4.x, clients running with bridges need to set both
ClientUseIPv6 and ClientPreferIPv6ORPort in order to connect over
IPv6. This is regression from 0.2.3.x.

Based on the assumption that we want i) a Bridge line with an IPv6
address to imply ClientUseIPv6=1 and b) a Bridge line with an IPv6
address for a given bridge listed before an IPv4 address for the same
bridge to imply ClientPreferIPv6ORPort=1, I suggest two changes:

  1. Set and reset node_t.ipv6_preferred in

rewrite_node_address_for_bridge() based on address family of the
_bridge_ entry (i.e. first configured IP address matching newly
arrived router) and the existence of an IPv6 address in the
_routerinfo_.

  1. have node_get_pref_ipv6_orport() return an IPv6 address when

UseBridges is set, not only when ClientUseIPv6 is set

Please review branch bug6757 in my public repo.

comment:5 in reply to:  4 ; Changed 7 years ago by nickm

Needs more careful review than I can give it in the time before I need to head to the airport today.

Replying to ln5:

Based on the assumption that we want i) a Bridge line with an IPv6
address to imply ClientUseIPv6=1 and b) a Bridge line with an IPv6
address for a given bridge listed before an IPv4 address for the same
bridge to imply ClientPreferIPv6ORPort=1, I suggest two changes:

Hm. I'm hoping that ClientUseIPv6 and ClientPreferIPv6ORPort in this context are meant bridge-specific, right? That is, if a bridge has an Ipv6 address listed first, we don't necessarily want to prefer IPv6 for *all* bridges.

comment:6 in reply to:  5 Changed 7 years ago by ln5

Replying to nickm:

Replying to ln5:

Based on the assumption that we want i) a Bridge line with an IPv6
address to imply ClientUseIPv6=1 and b) a Bridge line with an IPv6
address for a given bridge listed before an IPv4 address for the same
bridge to imply ClientPreferIPv6ORPort=1, I suggest two changes:

Hm. I'm hoping that ClientUseIPv6 and ClientPreferIPv6ORPort in this context are meant bridge-specific, right? That is, if a bridge has an Ipv6 address listed first, we don't necessarily want to prefer IPv6 for *all* bridges.

ClientUseIPv6 is not bridge-specific. It's meant to mean that a client
might use IPv6 for connecting to first hop, be it a bridge or not. I
don't think that a user should have to set that if she's configured an
IPv6 address as a bridge.

With this patch, ClientPreferIPv6ORPort is not considered at all when
it comes to picking address family for a bridge with both.

comment:7 Changed 6 years ago by nickm

Keywords: tor-client added

comment:8 Changed 6 years ago by nickm

Component: Tor ClientTor

comment:9 Changed 6 years ago by nickm

Okay, that does look like an improvement to me then. Tested enough IYO to merge?

comment:10 Changed 6 years ago by ln5

Status: needs_reviewneeds_revision

Thanks!

Hmm, I haven't tested the non-bridge relay case thoroughly. I will do
that tomorrow and comment again when done. Setting needs_revision for
now.

comment:11 Changed 6 years ago by ln5

We _might_ want to backport this part to 0.2.3.x:

    node->ipv6_preferred = (tor_addr_family(&bridge->addr) == AF_INET6 &&
                            !tor_addr_is_null(&node->ri->ipv6_addr));

since 0.2.3.x does

    ri->ipv6_preferred = tor_addr_family(&bridge->addr) == AF_INET6;

and will thus potentially prefer an address which is not in the
descriptor of the bridge. I think that this can happen, f.ex. if the
bridge withdraws its IPv6 address and uploads a new descriptor
(matched on digest in get_configured_bridge_by_orports_digest()) and
probably more.

Regardless, it seems like the safe and sane thing to do.

comment:12 in reply to:  10 Changed 6 years ago by ln5

Status: needs_revisionneeds_review

This patch has been tested this patch on a local test network with an
assert triggering if we ever call node_ipv6_preferred() if we're a
server.

I think this can be merged to master.

If you think that the part discussed in the previous comment should go
into 0.2.3.x, please let me know and I'll prepare a patch and a
changes file for 0.2.3.x.

comment:13 Changed 6 years ago by nickm

Merged into master.

If you think that the part discussed in the previous comment should go into 0.2.3.x, please let me know and I'll prepare a patch and a changes file for 0.2.3.x.

I think it's okay to leave this one unfixed in 0.2.3; people who want to use a lot of IPv6 stuff should be running 0.2.4. Or is the impact worse than I'm seeing?

comment:14 in reply to:  13 Changed 6 years ago by ln5

Resolution: fixed
Status: needs_reviewclosed

Replying to nickm:

Merged into master.

If you think that the part discussed in the previous comment should go into 0.2.3.x, please let me know and I'll prepare a patch and a changes file for 0.2.3.x.

I think it's okay to leave this one unfixed in 0.2.3; people who want to use a lot of IPv6 stuff should be running 0.2.4. Or is the impact worse than I'm seeing?

It's not too bad. Let's leave it.

Thanks.

Note: See TracTickets for help on using tickets.