Opened 5 years ago

Closed 3 years ago

Last modified 3 years ago

#13953 closed enhancement (implemented)

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

Reported by: s7r Owned by: teor
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version: Tor: 0.2.5.10
Severity: Normal Keywords: review-group-7, TorCoreTeam201608
Cc: Actual Points: 1.0
Parent ID: #17782 Points: 3
Reviewer: Sponsor:

Description (last modified by nickm)

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.

Child Tickets

Change History (34)

comment:1 Changed 5 years ago by nickm

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

comment:2 Changed 5 years ago by nickm

Milestone: Tor: 0.2.6.x-finalTor: 0.2.???
Type: defectenhancement

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

comment:3 Changed 4 years ago by teor

Parent ID: #17782
Severity: Normal

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

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 teor

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

comment:6 Changed 4 years ago by nickm

Milestone: Tor: 0.2.8.x-finalTor: 0.2.9.x-final

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.

comment:7 Changed 4 years ago by nickm

Points: medium

comment:8 Changed 4 years ago by isabela

Milestone: Tor: 0.2.9.x-finalTor: 0.2.???

tickets market to be removed from milestone 029

comment:9 Changed 4 years ago by teor

Keywords: 029-proposed added

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

comment:10 Changed 4 years ago by 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.

comment:11 Changed 4 years ago by 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.

comment:12 Changed 4 years ago by 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.

comment:13 Changed 4 years ago by 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.

comment:14 in reply to:  13 Changed 4 years ago by 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
Last edited 4 years ago by teor (previous) (diff)

comment:15 Changed 4 years ago by nickm

Keywords: 029-nickm-says-no added

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

comment:16 Changed 4 years ago by nickm

Keywords: 029-proposed 029-nickm-says-no removed

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.

comment:17 Changed 4 years ago by nickm

Description: modified (diff)
Milestone: Tor: 0.2.???Tor: 0.2.9.x-final

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.

comment:18 Changed 4 years ago by 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.

comment:19 Changed 4 years ago by teor

Actual Points: 3 hours
Status: newneeds_review

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.

comment:20 Changed 4 years ago by nickm

Owner: set to teor
Status: needs_reviewassigned

comment:21 Changed 4 years ago by isabela

Points: medium3

comment:22 Changed 3 years ago by teor

Status: assignedneeds_review

comment:23 Changed 3 years ago by 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.

Last edited 3 years ago by s7r (previous) (diff)

comment:24 Changed 3 years ago by 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.
Version 1, edited 3 years ago by s7r (previous) (next) (diff)

comment:25 Changed 3 years ago by nickm

Keywords: review-group-7 added

comment:26 in reply to:  24 Changed 3 years ago by teor

Actual Points: 3 hours0.5
Status: needs_reviewneeds_revision

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.

comment:27 Changed 3 years ago by 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.

Last edited 3 years ago by s7r (previous) (diff)

comment:28 Changed 3 years ago by teor

Keywords: TorCoreTeam201608 added
Status: needs_revisionmerge_ready

(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

comment:29 Changed 3 years ago by teor

Actual Points: 0.51.0
Status: merge_readyneeds_review

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.

comment:30 Changed 3 years ago by s7r

Status: needs_reviewmerge_ready

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

comment:31 in reply to:  30 Changed 3 years ago by teor

Status: merge_readyneeds_review

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.

comment:32 Changed 3 years ago by s7r

Status: needs_reviewmerge_ready

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.

comment:33 Changed 3 years ago by nickm

Resolution: implemented
Status: merge_readyclosed

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

comment:34 Changed 3 years ago by 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.

Note: See TracTickets for help on using tickets.