Self-test reachability test - Listen address from ORPort is ignored, it uses default address unless specified via Address argument

[s7r]

Tor: 0.2.5.10
OS: Debian Wheezy 64 bit
Installation: apt-get from deb.torproject.org

Configuration of server (one gigabit network card):
em0: IP1
em0:0 IP2
em0:1 IP3
em0:2 IP4
em0:3 IP5

I tried to run multiple Tor instances to saturate the CPU of the server. I configured instances with different pid, datadirecotry and logfile as manual recommends, and specified the different IP as follows in each config file:

tor1.cfg
ORPort IP1:port
DirPort IP1:port
OutboundBindAddress IP1

tor2.cfg
ORPort IP2:port
DirPort IP2:port
OutboundBindAddress IP2

etc. so on

The first Tor instance started just fine, but the following ones didn't publish server descriptor because self reachability test did not pass. Why? They were all thinking they should be reachable on IP1 (def spend a huge amount of time on this.ault IP, em0 interface). Obviously that IP did not had the requried ports open for the additonal Tor instances so the test could not possibly pass.

Fix:
I have added in each config file:
Address IP1
Address IP2 and so on

After that it did the self reachability test on the correct IP address, and it passed of course, so server descriptor was published.

Tor should know to prase the IP address from ORPort and/or DirPort and make tests on that, obviously that's the address where the port should be open. Caution at -noadvertise and -nolisten options when this bug is inspected.

[nickm]

Nice to fix; but will take some rewiring. Better not destablize this code in 0.2.6

[teor]

We'll need to fix this to get #17782 working correctly.

[teor]

See https://trac.torproject.org/projects/tor/ticket/17782#comment:9 for a description of issues involved in reachability self-testing.

[nickm]

It is impossible that we will fix all 226 currently open 028 tickets before 028 releases. Time to move some out. This is my second pass through the "new" and tickets, looking for things to move to 0.2.9.

[isabela]

tickets market to be removed from milestone 029

[teor]

I think it's a shame we won't fix this in 0.2.9, it's still confusing users, and may well cause reachability issues - see #18819.
(This likely also affects the DirPort listen address.)

[dgoulet]

I +1 teor's comment here. This imo can be serious issue that is outbound traffic on the wrong IP is _not_ good for multiple reasons. So I would like to see this one fixed and even backported if needed.

[nickm]

I wonder if there's a minimal "fix" that just logs what we're testing, why we're testing that and not something else, and what to do if you want us to test something different.

[teor]

But we're not testing the address that the ORPort or DirPort are listening on!

There's a minimal fix that changes the order of address usage from:

• Address config option
• auto-discovered address via various methods

To:

• ORPort/DirPort address (if present)
• Address config option
• auto-discovered address via various methods

Bonus points for logging how we got the address, and what to do to change it.

[teor]

I think this can be simplified to:

• Test the ORPort IP and port (that's about to be) advertised in the descriptor
• Test the DirPort IP and port (that's about to be) advertised in the descriptor

For bonus points, we should re-test it when it changes, but I think that's #17782.

[teor]

Replying to teor:

I think this can be simplified to:

• Test the ORPort IP and port (that's about to be) advertised in the descriptor
• Test the DirPort IP and port (that's about to be) advertised in the descriptor

For bonus points, we should re-test it when it changes, but I think that's #17782.

Oops. Relay descriptors only have one address in their "router" line.

From dir-spec.txt:
"router" nickname address ORPort SOCKSPort DirPort NL

So the ORPort and DirPort need to be on the same IP address.

We should log a warning if the following addresses don't match:

• the IPv4 address used in the descriptor
  • resolve_my_address()
• an IPv4 address belonging to an ORPort that matches the ORPort we're about to advertise
  • router_get_advertised_or_port()
  • skip ORPort addresses with NoAdvertise set
• an IPv4 address belonging to a DirPort that matches the DirPort we're about to advertise
  • router_get_advertised_dir_port()
  • skip DirPort addresses with NoAdvertise set

For bonus points, and the risk of breaking existing torrcs, we could modify resolve_my_address to use:

• Address
• an IPv4 address belonging to an ORPort that matches the ORPort we're about to advertise
• an IPv4 address belonging to a DirPort that matches the ORPort we're about to advertise
• the other ways we currently try and guess our address

[nickm]

Marking these tickets as ones I propose we do not include in 029.

[nickm]

Nobody objected to these, so they aren't going into 029 milestone. Please re-add 029-proposed if you disagree, and let me know why.

[nickm]

Oh wait, teor said he thinks this one should go in, and that the code to detect a mismatch and warn will be simple.

If it's really a simple solution, let's do that in 0.2.9, but let's watch out for scope-creep.

[teor]

When building a descriptor:

• warn if the ORPort address on the advertised IPv4 ORPort doesn't match the IPv4 address in the descriptor
• warn if the DirPort address on the advertised IPv4 DirPort doesn't match the IPv4 address in the descriptor

In both cases, tell the user to set "Address" to the correct IPv4 address.

[teor]

My first attempt at this is in my branch bug13953 on ​https://github.com/teor2345/tor.git

It could do with some testing, and maybe some unit tests.

[s7r]

tested teor's branch and it works in practice. Behind NAT it is not a problem, because Tor guesses the public IP address and does the test on that address regardless if it's not configured with NoListen. It does this with teor's bug13953 branch where I've tested many combinations (didn't test on master or other branches).

It becomes a problem when there are multiple public IP addresses on the same server, because Tor will only check the primary (default) address unless differently configured with Address. That is why I suggest changing the second part of the log message to be more explicit. Something like:

Please configure the matching IPv4 address for this router as "Address <IPv4 address>" in the torrc configuration file if you have multiple public IP addresses. If you are behind a NAT and have the right ports forwarded you can ignore this warning or, to remove it, use 2 ORPort lines with options NoListen (for the public IPv4 address line) and NoAdvertise (for the internal NAT IPv4 address line).

The rest looks good to me.

[s7r]

On second thought, won't be possible to make this warning automatically go away with this simple logic:

If the address explicitly configured with ORPort, if and only if, is a public IP address - let's call this or-address' (and it also matches the DirPort address?), Tor will automatically assume Address == or-address and OutboundBindAddress == or-address. This requires that the ORPort line to be used must not contain a NoAdvertise option. I think this could be a sane default, since it's clearly the intent of any user that specially configures an ORPort <public IP address>:<port> line in torrc.

This will ensure:

1. We are not breaking any existing torrcs.
2. We avoid an additional warning in the log file which could confuse users who don't know so much about networking stuff.

[teor]

Replying to s7r:

On second thought, won't be possible to make this warning automatically go away with this simple logic:

If the address explicitly configured with ORPort, if and only if, is a public IP address - let's call this or-address (and it also matches the DirPort address?), Tor will automatically assume Address == or-address, unless manually configured by user otherwise, and OutboundBindAddress == or-address, unless manually configured by user otherwise. This requires that the ORPort line to be used must not contain a NoAdvertise option. I think this could be a sane default, since it's clearly the intent of any user that specially configures an ORPort <public IP address>:<port> line in torrc.

This will ensure:

1. We are not breaking any existing torrcs.
2. We avoid an additional warning in the log file which could confuse users who don't know so much about networking stuff.

I was concerned about the possible negative impacts of this change to existing configurations, which is why I chose to just add a log message. And as nickm said in comment 17, if we want to modify Address guessing order, we should do it in the next release (see #19919).

That said, I agree we could change the log message as suggested in comment 23 to make it clearer.

[s7r]

Agreed. Besides the log message there's nothing else that needs change in bug13953 branch.

I have applied the change in commit 4d2b3164ec922916d01d6772ef86b7041e7c7d78 and opened a pull request on your branch.

[teor]

(Some people don't have github, so it's hard for us to swap pull requests on Trac.)

The latest branch is:
bug13953 on ​​https://github.com/gits7r/tor.git

On review, here's some things we could change:

• make the log message a static const char * with "ORPort" and "DirPort" replaced with "%sPort"
• the OR and Dir code is almost identical - we could put it in its own function to avoid duplication, and pass the inputs as parameters
• add a unit test

[teor]

Please see my branch bug13953 on ​https://github.com/teor2345/tor.git

I've removed the duplicate code via refactoring, which means the string is only used once.

I also reworded the log message to make it clearer that:

• if the relay has a static IPv4 address, use 'Address' (the IPv4 address in the descriptor)
• if the relay is behind NAT, use 'ORPort <Port> NoListen' for the external port (the IP address on a NoListen port is ignored, only the port is put in the descriptor), and 'ORPort <Port> NoAdvertise' for the internal port (binding to all addresses is the most functional default, as NAT often implies DHCP).

This covers the most common causes of this issue: multiple static public IPv4, static IPv4 with NAT, and dynamic IPv4 with NAT.

I didn't write a unit test - if we want to unit test router_build_fresh_descriptor, we should do that in a separate ticket.

s7r, if you're happy with this, please put it into merge_ready for nickm to look at.

[s7r]

Nice to remove the duplicate code. I am happy with it, but I think we should reword the log message:

[...]If you have a static public IPv4 address, set 'Address <IPv4>'[...]

If there's just a static IP address, this setting is unnecessary, Tor will just discover and use it. The setting is necessary when there are multiple public IP addresses and the user wants to use a certain one. Also substitute IPv4 with IP, simpler and should cover IPv6 as well in the future.

Latest branch that includes teor's refactoring and the reworded log message is bug13953on ​https://github.com/gits7r/tor.git

[teor]

Replying to s7r:

Nice to remove the duplicate code. I am happy with it, but I think we should reword the log message:

[...]If you have a static public IPv4 address, set 'Address <IPv4>'[...]

If there's just a static IP address, this setting is unnecessary, Tor will just discover and use it.

No, that's not always true. Sometimes Tor's automatic address discovery fails, in particular, when it is not allowed to enumerate the IP addresses on the machine. Sometimes it works, but gets conflicting results (#17782, #17765).

The setting is necessary when there are multiple public IP addresses and the user wants to use a certain one.

Yes, it's necessary in this case, but also necessary in other cases (which are somewhat unpredictable). So I'd rather log a message that encourages relay operators to set Address whenever they have a static IPv4, even if they sometimes don't need to, than have some relays stop working because Address wasn't set.

Also substitute IPv4 with IP, simpler and should cover IPv6 as well in the future.

No, Address only takes an IPv4 address. The advertised IPv6 OR address is the address of the first IPv6 ORPort. It's not possible to use an IPv6 address with Address.

Latest branch that includes teor's refactoring and the reworded log message is bug13953on ​https://github.com/gits7r/tor.git

You also added:
and 'OutboundBindAddress <IP>

I think it's ok to include it, as long as you're sure that pushing all Tor traffic through the relay IP is what most relay operators want. (I think it probably is.)

I've merged that change in to:
[bug13953 00557a5] fixup! fixup! fixup! fixup! Reword the router_check_descriptor_address_port_consistency log message
on ​​https://github.com/teor2345/tor.git

I also tried to cut down the log message a bit more.

[s7r]

Ok (I thought it will be possible in the future to use IPv6 with Address). In this case I think it pretty much covers it.

[nickm]

assert -> tor_assert. Otherwise looks pretty safe. Merging; let's see if it helps!

[teor]

I just logged #19988: Warn when Port addresses have no effect, as a follow-up to this ticket, in case we still see problems with these kinds of issues when operators switch to 0.2.9.