#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 )
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
Milestone: | Tor: 0.2.5.x-final → Tor: 0.2.6.x-final |
---|
comment:2 Changed 5 years ago by
Milestone: | Tor: 0.2.6.x-final → Tor: 0.2.??? |
---|---|
Type: | defect → enhancement |
comment:3 Changed 4 years ago by
Parent ID: | → #17782 |
---|---|
Severity: | → Normal |
We'll need to fix this to get #17782 working correctly.
comment:4 Changed 4 years ago by
Milestone: | Tor: 0.2.??? → Tor: 0.2.8.x-final |
---|
comment:5 Changed 4 years ago by
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
Milestone: | Tor: 0.2.8.x-final → Tor: 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
Points: | → medium |
---|
comment:8 Changed 4 years ago by
Milestone: | Tor: 0.2.9.x-final → Tor: 0.2.??? |
---|
tickets market to be removed from milestone 029
comment:9 Changed 4 years ago by
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
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
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
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 follow-up: 14 Changed 4 years ago by
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 Changed 4 years ago by
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
comment:15 Changed 4 years ago by
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
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
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
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
Actual Points: | → 3 hours |
---|---|
Status: | new → needs_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
Owner: | set to teor |
---|---|
Status: | needs_review → assigned |
comment:21 Changed 4 years ago by
Points: | medium → 3 |
---|
comment:22 Changed 3 years ago by
Status: | assigned → needs_review |
---|
comment:23 Changed 3 years ago by
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.
comment:24 follow-up: 26 Changed 3 years ago by
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:
- We are not breaking any existing torrcs.
- We avoid an additional warning in the log file which could confuse users who don't know so much about networking stuff.
comment:25 Changed 3 years ago by
Keywords: | review-group-7 added |
---|
comment:26 Changed 3 years ago by
Actual Points: | 3 hours → 0.5 |
---|---|
Status: | needs_review → needs_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, andOutboundBindAddress == or-address
, unless manually configured by user otherwise. This requires that the ORPort line to be used must not contain aNoAdvertise
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:
- We are not breaking any existing torrcs.
- 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
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.
comment:28 Changed 3 years ago by
Keywords: | TorCoreTeam201608 added |
---|---|
Status: | needs_revision → merge_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
Actual Points: | 0.5 → 1.0 |
---|---|
Status: | merge_ready → needs_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 follow-up: 31 Changed 3 years ago by
Status: | needs_review → merge_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 bug13953
on https://github.com/gits7r/tor.git
comment:31 Changed 3 years ago by
Status: | merge_ready → needs_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
bug13953
on 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
Status: | needs_review → merge_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
Resolution: | → implemented |
---|---|
Status: | merge_ready → closed |
assert -> tor_assert. Otherwise looks pretty safe. Merging; let's see if it helps!
comment:34 Changed 3 years ago by
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.
Nice to fix; but will take some rewiring. Better not destablize this code in 0.2.6