Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#13163 closed defect (fixed)

Alternate{Dir,Bridge}Authority aren't sufficient for fully internal TestingTorNetwork

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.2.7.x-final
Component: Core Tor/Tor Version: Tor: 0.2.7
Severity: Keywords: needs-test, tor-config, tor-authority, tor-test-network, 025-backport, 027-triaged-1-in, SponsorS
Cc: nickm Actual Points:
Parent ID: Points: medium/large
Reviewer: Sponsor:

Description

I had several issues getting chutney working on OS X 10.9 with tor 0.2.6.?-alpha. I've diagnosed and patched most of them in #13161. But I can't seem to stop the chutney bridges* flavours accessing the global tor network rather than the chutney network.

tor git d6b2a1709d28c656dadc019fb24145e6ac400771 + #13161
chutney git 60b2d18d81904e6a71a352dfa8b5cb73f4e04003 + #13161

Issue Description

Despite being configured as a test network, the chutney bridges* flavours seem to be ignoring TestingTorNetwork, and instead using the global Tor network. Which is really strange, because they all have TestNetwork on in common.i.

I can't work out why this is - perhaps it has something to do with AlternateBridgeAuthority, as that's the only real difference I can see.

I'm not sure what's happening here, and whether it's a bug in tor or chutney. (All other flavours work fine.)

I've attached example torrc files generated by chutney, one of each type. These include changes from #13161.

Child Tickets

Attachments (13)

bridges (570 bytes) - added by teor 5 years ago.
bridges network flavour from chutney
bridges+ipv6 (687 bytes) - added by teor 5 years ago.
bridges+ipv6 network flavour from chutney
torrc (2.0 KB) - added by teor 5 years ago.
chutney bridges flavour authority torrc
torrc.2 (1.5 KB) - added by teor 5 years ago.
chutney bridges flavour bridge authority torrc
torrc.3 (1.4 KB) - added by teor 5 years ago.
chutney bridges flavour relay torrc
torrc.4 (1.2 KB) - added by teor 5 years ago.
chutney bridges flavour bridge torrc
torrc.5 (1.3 KB) - added by teor 5 years ago.
chutney bridges flavour client torrc
torrc.6 (1.4 KB) - added by teor 5 years ago.
chutney bridges flavour bridge client torrc
torrc.7 (1.8 KB) - added by teor 5 years ago.
torrc file when AlternateBridgeAuthority is not present
01-AlternateAuthorities-alway-use-default-authorities.patch (628 bytes) - added by teor 5 years ago.
Avoid using default directories when both Alternate{Dir,Bridge}Authority flags are set
02-AlternateAuthorities-restrict-to-declared-type.patch (1011 bytes) - added by teor 5 years ago.
Optional - restrict alternate authorities declared as dir to dir types, and bridge to bridge type (is this even ok?)
03-UseBridges-check-bridge-type-bit.patch (1.4 KB) - added by teor 5 years ago.
Have UseBridges check the bridge bit, rather than checking equality (all other comparisons check bits)
04-dirinfo-type-expand-comments-explicit-constants.patch (4.8 KB) - added by teor 5 years ago.
Expand various comments about NO_DIRINFO so that each function where it is used is clearly documented

Download all attachments as: .zip

Change History (39)

Changed 5 years ago by teor

Attachment: bridges added

bridges network flavour from chutney

Changed 5 years ago by teor

Attachment: bridges+ipv6 added

bridges+ipv6 network flavour from chutney

Changed 5 years ago by teor

Attachment: torrc added

chutney bridges flavour authority torrc

Changed 5 years ago by teor

Attachment: torrc.2 added

chutney bridges flavour bridge authority torrc

Changed 5 years ago by teor

Attachment: torrc.3 added

chutney bridges flavour relay torrc

Changed 5 years ago by teor

Attachment: torrc.4 added

chutney bridges flavour bridge torrc

Changed 5 years ago by teor

Attachment: torrc.5 added

chutney bridges flavour client torrc

Changed 5 years ago by teor

Attachment: torrc.6 added

chutney bridges flavour bridge client torrc

comment:1 Changed 5 years ago by teor

I don't see that there's much point in attaching the torrc templates, but I'm happy to provide them.

comment:2 Changed 5 years ago by teor

Keywords: tor-authority tor-test-network added; chutney removed
Summary: Chutney bridges* flavours use global Tor consensusAlternateBridgeAuthority causes tor to ignore TestingTorNetwork

I removed the AlternateBridgeAuthority line, and the entire network functioned perfectly as a test network.

That is, if an AlternateBridgeAuthority line is present in a torrc, it cancels the effect of TestingTorNetwork for every torrc in which it is present (and not just for the bridge authority itself).

This is clearly an issue with tor, that could occur even if chutney wasn't being used.

Changed 5 years ago by teor

Attachment: torrc.7 added

torrc file when AlternateBridgeAuthority is not present

comment:3 Changed 5 years ago by teor

Keywords: chutney added
Summary: AlternateBridgeAuthority causes tor to ignore TestingTorNetworkchutney: Alternate{Dir,Bridge}Authority aren't sufficient for fully internal TestingTorNetwork

Actually, this issue may be due to chutney's switch from DirAuthority to Alternate{Dir,Bridge}Authority when bridges are in use.

Is this the expected behaviour of AlternateDirAuthority?

comment:4 Changed 5 years ago by teor

Keywords: tor-config added; chutney removed
Summary: chutney: Alternate{Dir,Bridge}Authority aren't sufficient for fully internal TestingTorNetworkAlternate{Dir,Bridge}Authority aren't sufficient for fully internal TestingTorNetwork

So it's a tor bug:
parse_dir_authority_line() treats a type of 0 as "any type", and documents this usage
add_default_trusted_dir_authorities() treats a type of 0 as "any type", but is silent on this usage
consider_adding_dir_servers() treats a type of 0 (NO_DIRINFO) as "no types", but calls add_default_trusted_dir_authorities(), which treats 0 (NO_DIRINFO) as "any type". This is why the test network is escaping localhost.

There are two alternative patches:

  1. I could patch the usage of NO_DIRINFO in consider_adding_dir_servers().
  2. I could make all functions respect NO_DIRINFO as NO_DIRINFO, and replace 0 with ALL_DIRINFO where that is what is meant. This is far more intrusive.

Changed 5 years ago by teor

Avoid using default directories when both Alternate{Dir,Bridge}Authority flags are set

Changed 5 years ago by teor

Optional - restrict alternate authorities declared as dir to dir types, and bridge to bridge type (is this even ok?)

Changed 5 years ago by teor

Have UseBridges check the bridge bit, rather than checking equality (all other comparisons check bits)

Changed 5 years ago by teor

Expand various comments about NO_DIRINFO so that each function where it is used is clearly documented

comment:5 Changed 5 years ago by teor

Status: newneeds_review

I created patches to implement option 1 - the usage of NO_DIRINFO in consider_adding_dir_servers(). In many other functions, NO_DIRINFO is used to mean "don't care" or "all".

01-AlternateAuthorities-alway-use-default-authorities.patch

This patch fixes the issue where networks that use both AlternateDirAuthority and AlternateBridgeAuthority still have the default authorities.

02-AlternateAuthorities-restrict-to-declared-type.patch

This patch restricts the types of alternate authorities to the type they are declared as - for exampe, a bridge authority will only be of bridge type. This might not be ok - it would stop bridge authorities being asked for microdescriptors, for example.

03-UseBridges-check-bridge-type-bit.patch​

A few lines of code in directory_get_from_dirserver() in directory.c use type == BRIDGE_DIRINFO rather than type & BRIDGE_DIRINFO. This patch fixes them so they use bitwise and like all other dirinfo checks.

04-dirinfo-type-expand-comments-explicit-constants.patch

Passing NO_DIRINFO to various functions is sometimes left undocumented. This patch expands the documentation around NO_DIRINFO. It also changes zeroes to NO_DIRINFO where appropriate. An unrelated by nearby comment is repositioned near its code.

comment:6 Changed 5 years ago by teor

Status: needs_reviewneeds_revision

I'll redo this with git format-patch

comment:7 Changed 5 years ago by nickm

Milestone: Tor: 0.2.6.x-final

Any updates?

comment:8 Changed 5 years ago by teor

3rd on my list after #13161 and #13162 - it's hard to test this one without them

comment:9 Changed 5 years ago by teor

I don't like the risk involved in 02, so I'm going to skip it when I create a branch on github.

02 restricts the types of alternate authorities to the type they are declared as - for exampe, a bridge authority will only be of bridge type. This might not be ok - it may stop bridge authorities being asked for microdescriptors, for example.

I don't know enough to understand the effects of 02, and I'm not sure it's a net gain, even if it works.

comment:10 Changed 5 years ago by teor

Status: needs_revisionneeds_review

Ready for review:

Repository: ​​​​https://github.com/teor2345/tor.git

Branch: bug-13163-AlternateAuthorities-type-handling

be2135319c02a6f7b39c8ba68286967e14c68bf2 Stop using default authorities with both Alternate Dir and Bridge Authority
Includes 01 and changes file

7b0825b3fd9ad426872414dba8838b5f3f524eb7 Improve DIRINFO flags' usage comments
Includes 04 and changes file

2d499f63addfcf86c0ef644cfe4cc37c01e0e118 Bitwise check BRIDGE_DIRINFO
Includes 03 and changes file

As noted above, I skipped 02 because I didn't like, and couldn't predict, the potential effects.

comment:11 Changed 5 years ago by teor

Some of the changes in #13161 may be required to get this working, depending on your platform, tor version, and chutney version.

comment:12 Changed 5 years ago by teor

I'm seeing the following warning in my tor logs, probably because of #13161 or #13163.

"[Warning] const routerstatus_t *directory_pick_generic_dirserver(dirinfo_type_t, int, uint8_t)(): Bug: Called when we have UseBridges set."

Can someone help me work out what I've done wrong or incompletely?

comment:13 Changed 5 years ago by teor

Fix in branch: bug-13163-AlternateAuthorities-type-handling-fixed
Repository: ​​​​​https://github.com/teor2345/tor.git

A missing "!" had inverted a test.

Thanks to nickm and the #tor-dev patch review workshop.

comment:14 Changed 5 years ago by teor

The warnings in bridge mode are now gone.

comment:15 Changed 5 years ago by nickm

Looks good to me. Do any bugs produced by having used == rather than a bitwise check actually cause problems in 0.2.5? If so we should consider a backport.

comment:16 Changed 5 years ago by nickm

Keywords: 025-backport added
Milestone: Tor: 0.2.6.x-finalTor: 0.2.5.x-final

Merged to master; marked for possible backport.

comment:17 Changed 5 years ago by teor

I can't say - it looks like the equality checks made it impossible to download microdescriptors or extra info from a bridge directory. But only if the codepaths that download them use those particular checks. I don't know enough to say.

comment:18 Changed 5 years ago by teor

nickm, re: "Do any bugs produced by having used == rather than a bitwise check actually cause problems in 0.2.5? If so we should consider a backport."

I suspect that the assert that arma mentions in #1776 when running a client with UseMicrodescriptors 0 and a public relay as a bridge has been fixed by the changes in:
2d499f63addfcf86c0ef644cfe4cc37c01e0e118 Bitwise check BRIDGE_DIRINFO
but I haven't actually seen it happen with the old code, so I can't be sure it was this particular fix.

comment:19 Changed 5 years ago by teor

Keywords: needs-test added

The following commits could do with unit tests:
be2135319c02a6f7b39c8ba68286967e14c68bf2 Stop using default authorities with both Alternate Dir and Bridge Authority
2d499f63addfcf86c0ef644cfe4cc37c01e0e118 Bitwise check BRIDGE_DIRINFO
Includes 03 and changes file

comment:20 Changed 5 years ago by teor

Owner: set to teor
Status: needs_reviewassigned

comment:21 Changed 5 years ago by nickm

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

No backport to 0.2.5 for anything but the most critical issues at this point. Unit tests can be merged at any point though.

comment:22 Changed 5 years ago by nickm

Keywords: 027-triaged-1-in added

Marking more tickets as triaged-in for 0.2.7

comment:23 Changed 5 years ago by teor

I'm working on this with the changes in #15642, likely ready in time for 0.2.7.

comment:24 Changed 5 years ago by teor

Resolution: fixed
Status: assignedclosed

Unit tests for #15642 supersede these unit tests.

comment:25 Changed 5 years ago by teor

I can't see how to test the following commit, or that it would be particularly useful. But I'd happy to write unit tests if we work out how and what to test:

2d499f63addfcf86c0ef644cfe4cc37c01e0e118 Bitwise check BRIDGE_DIRINFO
Includes 03 and changes file

comment:26 Changed 5 years ago by isabela

Keywords: SponsorS added
Points: medium/large
Version: Tor: unspecifiedTor: 0.2.7
Note: See TracTickets for help on using tickets.