Trac issueshttps://gitlab.torproject.org/legacy/trac/-/issues2020-06-13T15:53:47Zhttps://gitlab.torproject.org/legacy/trac/-/issues/34412Additonal unit tests for functions related to ipv6 self-test2020-06-13T15:53:47ZNick MathewsonAdditonal unit tests for functions related to ipv6 self-testSee #33222 and #34200 for lists of functions that did not get unit tests as part of the #34200 merge.See #33222 and #34200 for lists of functions that did not get unit tests as part of the #34200 merge.Tor: 0.4.5.x-finalhttps://gitlab.torproject.org/legacy/trac/-/issues/34200Refactor tor's circuit path node selection checks2020-06-13T15:53:32ZteorRefactor tor's circuit path node selection checksIn #33222, we added an extra "can extend over IPv6" check to tor's circuit path node selection code.
To make sure it's applied consistently, I did a refactor of that code, so all those checks are in one function.In #33222, we added an extra "can extend over IPv6" check to tor's circuit path node selection code.
To make sure it's applied consistently, I did a refactor of that code, so all those checks are in one function.Tor: 0.4.5.x-finalteorteorhttps://gitlab.torproject.org/legacy/trac/-/issues/34137Make sure inform_testing_reachability() reports the correct ports2020-06-13T15:53:29ZteorMake sure inform_testing_reachability() reports the correct portsIn #33222, we added IPv6 ORPort support to inform_testing_reachability(). But that function checks the ORPorts and DirPort itself, rather than logging the reachability checks that were actually launched.
We'd like to pass flags so that ...In #33222, we added IPv6 ORPort support to inform_testing_reachability(). But that function checks the ORPorts and DirPort itself, rather than logging the reachability checks that were actually launched.
We'd like to pass flags so that it logs the actual reachability tests that are being run. (Rather than re-checking the relay's own routerinfo, which may have changed since the most recent reachability checks were launched.)
But inform_testing_reachability() is called when:
* the first circuit finishes building, or
* tor is reconfigured, and some circuits have already finished building.
So we need to do a bit of a refactor.
The refactor should preserve this behaviour:
* don't log until after the first circuit has finished building (rather than logging as soon as we start building reachability circuits)
And introduce this new behaviour:
* log the ports that were tested recently, rather than the ports that are currently available.
As an alternative, we could move some of the logging into the functions that actually launch the checks. And elevate some of those logs to notice level. (Note that these checks can be launched from at least 4 different locations in tor's code.)Tor: unspecifiedhttps://gitlab.torproject.org/legacy/trac/-/issues/34069Allow extend_info to contain both IPv4 and IPv6 ORPorts2020-06-13T15:53:20ZteorAllow extend_info to contain both IPv4 and IPv6 ORPortsTo send dual-stack EXTEND2 cells in #33222, we need to have IPv4 and IPv6 addresses in extend_info_t.To send dual-stack EXTEND2 cells in #33222, we need to have IPv4 and IPv6 addresses in extend_info_t.Tor: 0.4.5.x-finalhttps://gitlab.torproject.org/legacy/trac/-/issues/34068Decide how to handle control port events for IPv6 reachability self-tests2020-06-13T15:53:19ZteorDecide how to handle control port events for IPv6 reachability self-testsThe control spec has two reachability self-test events.
Here is how they are specified:
```
CHECKING_REACHABILITY
"ORADDRESS=IP:port"
"DIRADDRESS=IP:port"
We're going to start testing the reachability of our extern...The control spec has two reachability self-test events.
Here is how they are specified:
```
CHECKING_REACHABILITY
"ORADDRESS=IP:port"
"DIRADDRESS=IP:port"
We're going to start testing the reachability of our external OR port
or directory port.
REACHABILITY_SUCCEEDED
"ORADDRESS=IP:port"
"DIRADDRESS=IP:port"
We successfully verified the reachability of our external OR port or
directory port (depending on which of ORADDRESS or DIRADDRESS is
given.)
```
And here is what tor actually sends:
```
CHECKING_REACHABILITY ORADDRESS=IPv4:port
CHECKING_REACHABILITY DIRADDRESS=IPv4:port
REACHABILITY_SUCCEEDED ORADDRESS=IPv4:port
REACHABILITY_SUCCEEDED DIRADDRESS=IPv4:port
```
When we add IPv6 reachability events, we could break some (buggy) control parsers with:
```
CHECKING_REACHABILITY ORADDRESS=[IPv6]:port
REACHABILITY_SUCCEEDED ORADDRESS=[IPv6]:port
```
How should we handle this change?Tor: unspecifiedhttps://gitlab.torproject.org/legacy/trac/-/issues/34067Separate tor's IPv4 and IPv6 reachability flags2020-06-13T15:53:19ZteorSeparate tor's IPv4 and IPv6 reachability flagsIn #33222, I make tor build circuits to IPv4 and IPv6 ORPorts.
But we also need to detect IPv4 and IPv6 reachability separately.In #33222, I make tor build circuits to IPv4 and IPv6 ORPorts.
But we also need to detect IPv4 and IPv6 reachability separately.Tor: 0.4.5.x-finalteorteorhttps://gitlab.torproject.org/legacy/trac/-/issues/34065Make routerset_contains_router() support IPv62020-06-13T15:53:18ZteorMake routerset_contains_router() support IPv6In router_should_check_reachability(), we test to see if the current relay is in ExcludeNodes.
But ExcludeNodes doesn't support IPv6, so if a user excludes our IPv6 address, we'll just ignore that setting.
This is not a required task.
...In router_should_check_reachability(), we test to see if the current relay is in ExcludeNodes.
But ExcludeNodes doesn't support IPv6, so if a user excludes our IPv6 address, we'll just ignore that setting.
This is not a required task.
But we should probably be explicit about ignoring IPv6 in the man page.Tor: 0.4.4.x-finalNeel Chauhanneel@neelc.orgNeel Chauhanneel@neelc.orghttps://gitlab.torproject.org/legacy/trac/-/issues/34037Make chutney check tor's logs for reachability self-test success2020-06-13T15:52:08ZteorMake chutney check tor's logs for reachability self-test successThis ticket is an alternative to #33582 or #33222.
Instead of fixing bridge descriptor uploads, we can check bridge logs to make sure that reachability self-tests have succeeded.
For consistency, we should also do the same checks for r...This ticket is an alternative to #33582 or #33222.
Instead of fixing bridge descriptor uploads, we can check bridge logs to make sure that reachability self-tests have succeeded.
For consistency, we should also do the same checks for relays.
We can only do these tests on authorities, relays, and bridges that are configured with `AssumeReachable 0`. Chutney's current defaults are:
* directory authorities: 1
* bridge authorities: 1
* relays: 0
* bridges: 0
* clients: clients never perform reachability self-tests
Some custom chutney networks may set `AssumeReachable 1` for relays and bridges. So we should make it easy for them to disable these checks.https://gitlab.torproject.org/legacy/trac/-/issues/33919Write unit tests for channel_matches_target_addr_for_extend()2020-06-13T15:53:10ZteorWrite unit tests for channel_matches_target_addr_for_extend()In #33817, we modify channel_matches_target_addr_for_extend() to take an IPv4 and an IPv6 address.
We also modify channel_get_for_extend() to take an IPv4 and an IPv6 address. It has existing tests, but they ignore IP addresses. (And mo...In #33817, we modify channel_matches_target_addr_for_extend() to take an IPv4 and an IPv6 address.
We also modify channel_get_for_extend() to take an IPv4 and an IPv6 address. It has existing tests, but they ignore IP addresses. (And mock the matches_target() method, which is called by channel_matches_target_addr_for_extend().)
It would be useful to have tests that check that a channel matches, if its IP address matches the IPv4 or IPv6 address. But it's not urgent. (And the actual changes to the function are pretty trivial.)
The setup for these tasks is a bit tricky, we need to set up a channel_tls_t and an associated connection_t with a real_addr. (Note that #33898 may change the name of real_addr.)
This task is not urgent or important. Anyone can do this task.Tor: 0.4.4.x-finalhttps://gitlab.torproject.org/legacy/trac/-/issues/33918Stop truncating IPv6 addresses in channel logs2020-06-13T15:53:10ZteorStop truncating IPv6 addresses in channel logschannel_tls_get_remote_desc_method() formats IP addresses and ports, but its buffer is only 32 characters. IPv6 addresses can be up to 41 characters long, and the port is an extra 6 characters.channel_tls_get_remote_desc_method() formats IP addresses and ports, but its buffer is only 32 characters. IPv6 addresses can be up to 41 characters long, and the port is an extra 6 characters.Tor: 0.4.4.x-finalteorteorhttps://gitlab.torproject.org/legacy/trac/-/issues/33917Make IF_BUG_ONCE() support ALL_BUGS_ARE_FATAL2020-06-13T15:53:09ZteorMake IF_BUG_ONCE() support ALL_BUGS_ARE_FATAL... and DISABLE_ASSERTS_IN_UNIT_TESTS.
It's the only assertion/bug macro that doesn't support these debugging modes. Let's fix that.... and DISABLE_ASSERTS_IN_UNIT_TESTS.
It's the only assertion/bug macro that doesn't support these debugging modes. Let's fix that.Tor: 0.4.4.x-finalteorteorhttps://gitlab.torproject.org/legacy/trac/-/issues/33901Allow IPv6 extend cells2020-06-13T15:53:08ZteorAllow IPv6 extend cellsAs part of #33817, we want to allow IPv6 extend cells.
We want clients and relays to send:
* dual-stack EXTEND2 cells
* IPv6-only EXTEND2 cells
We want relays to parse:
* dual-stack EXTEND2 cells
* IPv6-only EXTEND2 cellsAs part of #33817, we want to allow IPv6 extend cells.
We want clients and relays to send:
* dual-stack EXTEND2 cells
* IPv6-only EXTEND2 cells
We want relays to parse:
* dual-stack EXTEND2 cells
* IPv6-only EXTEND2 cellsTor: 0.4.4.x-finalteorteorhttps://gitlab.torproject.org/legacy/trac/-/issues/33900Check for invalid zero IPv4 addresses and ports in extend cells2020-06-13T15:53:07ZteorCheck for invalid zero IPv4 addresses and ports in extend cellsWhen we send and parse extend cells, we check that their IPv4 address field is not AF_UNSPEC.
But we should also check for zero IPv4 addresses and zero ports. (Which are both invalid.)
Code and points in #33817, I just needed a separat...When we send and parse extend cells, we check that their IPv4 address field is not AF_UNSPEC.
But we should also check for zero IPv4 addresses and zero ports. (Which are both invalid.)
Code and points in #33817, I just needed a separate bug number.Tor: 0.4.4.x-finalteorteorhttps://gitlab.torproject.org/legacy/trac/-/issues/33899Allow IPv6 addresses to be canonical2020-06-13T15:53:06ZteorAllow IPv6 addresses to be canonicalIn #17604, we allowed IPv6 connections to set is_canonical_to_peer. But we did not let them set is_canonical.
So channel_tls_process_netinfo_cell() handles IPv6 addresses, but connection_or_check_canonicity() does not.
This inconsisten...In #17604, we allowed IPv6 connections to set is_canonical_to_peer. But we did not let them set is_canonical.
So channel_tls_process_netinfo_cell() handles IPv6 addresses, but connection_or_check_canonicity() does not.
This inconsistency could be one cause of the logging issues in #24841.
We should allow IPv6 connections to be canonical. We should also do some cleanup of the related connection address logging in #33898.Tor: 0.4.4.x-finalteorteorhttps://gitlab.torproject.org/legacy/trac/-/issues/33898Stop modifying addr on connections, and delete real_addr2020-06-13T15:53:06ZteorStop modifying addr on connections, and delete real_addrIn connection_or_check_canonicity(), we overwrite conn.addr with the canonical address of the relay in the consensus. That makes accurate logging impossible.
And so we also need channel.real_addr, to store the actual address of the remo...In connection_or_check_canonicity(), we overwrite conn.addr with the canonical address of the relay in the consensus. That makes accurate logging impossible.
And so we also need channel.real_addr, to store the actual address of the remote peer. And for some reason, we also have conn.address, a string copy of the peer's canonical address and port.
See:
https://github.com/torproject/tor/blob/7f9eaec538b7d01e0d1b130dc4cf2ec634252d46/src/core/or/connection_or.c#L920
This is... a mess.
Here's the high-level interface I'd like to see:
* use a function to format a connection or channel addresses for loogging
* use exactly as many address and port variables as needed in connection and channel (no extras!)
* qualify each address and port variable's name with its purpose
For example, here's one possible design:
* delete addr, port, address, and real_addr
* add remote_ap, a tor_addr_port_t that is the remote address and port of the TCP connection to the remote peer
* implement connection_describe(), which:
* formats remote_ap,
* formats is_canonical (and any other useful info), and
* calls node_describe() to format the canonical IPv4 and IPv6 addresses and ports of the remote peer.
We may also need separate fields for reverse proxied addresses, see the comment at:
https://github.com/torproject/tor/blob/7517e1b5d31aada1f594c2594737a231d9d8e116/src/core/or/channeltls.c#L1339
If we need separate variables or functions for channels, we can use a similar design. (But ideally, re-using as many functions and variables as possible.)
This is important for Sponsor 55: getting accurate connection information will make diagnosing bugs much easier.Tor: 0.4.5.x-finalNick MathewsonNick Mathewsonhttps://gitlab.torproject.org/legacy/trac/-/issues/33880Confusing "Your relay has a very large number of connections to other relays"...2020-06-13T15:53:02ZRoger DingledineConfusing "Your relay has a very large number of connections to other relays" relay messageA new relay operator reports this complaint in their logs, showing up hourly:
```
Your relay has a very large number of connections to other relays. Is your
outbound address the same as your relay address? Found 13 connections to 8
relay...A new relay operator reports this complaint in their logs, showing up hourly:
```
Your relay has a very large number of connections to other relays. Is your
outbound address the same as your relay address? Found 13 connections to 8
relays. Found 13 current canonical connections, in 0 of which we were a
non-canonical peer. 5 relays had more than 1 connection, 0 had more than 2, and
0 had more than 4 connections.
```
I checked, and their outbound address was the same as their relay address.
Upon investigation, it looks like the redundant connections are to directory authorities.
My theory is that the change from #17592 (which went into 0.3.1.1-alpha, commit d5a151a0) is responsible: while before that canonical relay-to-relay connections would expire after either side first reached its randomized "15 to 22.5 minute" timeout, now they expire after either side reaches its "45 to 75 minute" timeout. And since directory authorities test reachability every 1280 seconds (around 21.3 minutes), that means it is expected that most relays will have duplicate canonical connections with directory authorities.
Possible fixes:
(A) Change the notice-level log to make it clearer that it's not scary, or at least it's not actionable. Maybe that means making it info-level so nobody will see it. Probably not the best option, assuming there *are* cases where we do want relay operators to hear it.
(B) In channel_check_for_duplicates(), change `MIN_RELAY_CONNECTIONS_TO_WARN 5` to a high enough number that even if we have 2 canonical conns per authority, plus a bit more, the log message still doesn't trigger.
(C) In channel_check_for_duplicates(), skip over connections to directory authorities in some way, since we know they will be special.
(D) Make connections to or from directory authorities expire quicker, on the theory that they don't really need the same level of padding protection as other connections.
(E) Your idea here?
I'd be fine with any of B,C,D. Whichever one can be done with an easy, short, and non-invasive patch is my favorite. Maybe that's "B, raise it to 30"? That would make the message trigger when we have connections to more than 30 relays and also we have more than 45 connections open. Or we could pick the more conservative "raise it to 40", on the theory that small numbers are more likely to have edge cases and less indicative of major network problems anyway.
And while we're at it, it might be smart to say in the log message what action we want the relay operator take, e.g. "Please report:".Tor: unspecifiedNick MathewsonNick Mathewsonhttps://gitlab.torproject.org/legacy/trac/-/issues/33860Finish test_onionskin_answer()2020-06-13T15:53:00ZteorFinish test_onionskin_answer()In #33633, we finished unit tests for test_extend() and helpers.
But we didn't have time to finish test_onionskin_answer().
Let's try to test each of the cases of each `if` statement in onionskin_answer(). It's ok to mock the functions...In #33633, we finished unit tests for test_extend() and helpers.
But we didn't have time to finish test_onionskin_answer().
Let's try to test each of the cases of each `if` statement in onionskin_answer(). It's ok to mock the functions that are called by onionskin_answer().Tor: unspecifiedNick MathewsonNick Mathewsonhttps://gitlab.torproject.org/legacy/trac/-/issues/33826Add a testing-only option that turns off IPv4 extends2020-06-13T15:52:55ZteorAdd a testing-only option that turns off IPv4 extendsTo test that relays perform IPv6 extends correctly, we should turn off all IPv4 extends in a test network.
The option should be called ExtendAllowIPv4Addresses, and it should be documented and implemented like ExtendAllowIPv6Addresses i...To test that relays perform IPv6 extends correctly, we should turn off all IPv4 extends in a test network.
The option should be called ExtendAllowIPv4Addresses, and it should be documented and implemented like ExtendAllowIPv6Addresses in #33818.
We might also need an ExtendByIPv4ORPort option, similar to ExtendByIPv6ORPort.Tor: unspecifiedhttps://gitlab.torproject.org/legacy/trac/-/issues/33825Make Environ handle "in" and "get()" like a dict2020-06-13T13:31:49ZteorMake Environ handle "in" and "get()" like a dictSome standard Python dict code doesn't work on chutney's Environ class:
```
is_in_env = 'foo' in self._env
value_or_none = self._env.get('foo')
```
"in" should return a boolean, and "get()" should return the value (or None).
But instea...Some standard Python dict code doesn't work on chutney's Environ class:
```
is_in_env = 'foo' in self._env
value_or_none = self._env.get('foo')
```
"in" should return a boolean, and "get()" should return the value (or None).
But instead, when the key is missing, sometimes they throw a KeyError. (It seems to happen in certain contexts, but not others.)
We should work out if Environ is missing some of the required dict implementation functions. Or if there is some issue with Environ's lookup code.
Then we should implement unit tests, to make sure we don't break Environ in future.https://gitlab.torproject.org/legacy/trac/-/issues/33819Make clients and bridges send IPv6 extends by default in Tor 0.4.52020-06-13T15:52:54ZteorMake clients and bridges send IPv6 extends by default in Tor 0.4.5If #33818 is included in Tor 0.4.4, the relays will send IPv6 extends by default. But clients and bridges won't, for anonymity reasons.
By the time 0.4.5 is released, a large number of clients and bridges will be on 0.4.4. So it will be...If #33818 is included in Tor 0.4.4, the relays will send IPv6 extends by default. But clients and bridges won't, for anonymity reasons.
By the time 0.4.5 is released, a large number of clients and bridges will be on 0.4.4. So it will be safe for clients and bridge to send IPv6 extends.
Here's what we need to do to change the default:
* set `ExtendByIPv6ORPort=1` in the consensus, and
* set `ExtendByIPv6ORPort 1` as the default in Tor.
To avoid confusion, clients and bridges will send IPv6 extends, but they won't initiate outbound IPv6 connections to relays. For more details, see #33818 and ExtendAllowIPv6Addresses.Tor: 0.4.5.x-final