Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#15642 closed defect (implemented)

Disable default fallback directories when DirAuthorities, AlternateDirAuthority, or FallbackDir are set

Reported by: teor Owned by: teor
Priority: Low Milestone: Tor: 0.2.7.x-final
Component: Core Tor/Tor Version: Tor: 0.2.4.7-alpha
Severity: Keywords: tor-client tor-relay
Cc: nickm, isabella Actual Points:
Parent ID: #15228 Points:
Reviewer: Sponsor:

Description (last modified by teor)

Currently, when we set DirAuthorities or AlternateDirAuthority, we don't add the default directory authorities. But, (as long as FallbackDir isn't set) we do add the default fallback directories.

It doesn't make sense to me to add the default fallback directories when we have custom DirAuthorities or AlternateDirAuthority. I think we should only add the default fallback directories when other directories are also set to their defaults.

However, the list of default fallback directories is currently NULL, so this issue currently has no effect.

I also can't imagine any scenarios where it would be useful to set an AlternateDirAuthority or DirAuthorities, and still get the default FallbackDir.

I can imagine this causing similar issues to #13163, where the default authorities were added to a custom set of authorities in some circumstances.

I'll create a patch to fix this, but it won't actually change tor's observable behaviour until we add directories to the default fallback directory list.

Bugfix on 90f6071d8dc0 in 0.2.4.7-alpha.

Child Tickets

Change History (14)

comment:1 Changed 4 years ago by teor

Description: modified (diff)

Edit: make explanation clearer (I hope)

comment:2 Changed 4 years ago by teor

Status: newneeds_review

Please see:

Branch: bug-15642-default-fallback
Repository: https://github.com/teor2345/tor.git
Commit: 8d61672

I'm still trying to get my head around how to do unit tests for this issue and #13163.
The code here is a little more complex than I'm used to writing tor unit tests for, so I think I will have to mock some of the functions.

comment:3 Changed 4 years ago by cypherpunks

Component: - Select a componentTor

comment:4 Changed 4 years ago by teor

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

This shouldn't be in 0.2.7

comment:5 Changed 4 years ago by teor

Owner: set to teor
Status: needs_reviewassigned

I'm working on unit tests for #13163 with these changes, likely ready in time for 0.2.7, but the changes here are not urgent at all (they don't change user-visible behaviour).

comment:6 Changed 4 years ago by teor

Status: assignedneeds_review

There are a few minor spacing issues in this patch, along with a redundant conditional. I'll fix these with the unit test branch.

Last edited 4 years ago by teor (previous) (diff)

comment:7 Changed 4 years ago by teor

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

I've created a new branch with unit tests for the new behaviour in consider_adding_dir_servers (all 10 valid cases):

Branch: bug-15642-v2-fallback-unit-tests
Repository:https://github.com/teor2345/tor.git
Commits:

  • Fix to default fallback directories behaviour
  • Unit tests for consider_adding_dir_servers based on fixed default fallback directories behaviour. These are extensive because they cover each of 10 valid cases. There may be ways to abstract some of these shared code, but I feel it would make the tests less clear.

Shifting this to 0.2.7 based on #13163's unit tests being in 0.2.7. Feel free to kick it out again.

comment:8 Changed 4 years ago by teor

Fixed up spacing issues in an additional commit.

comment:9 Changed 4 years ago by teor

Status: needs_reviewneeds_information

See also https://lists.torproject.org/pipermail/tor-dev/2015-April/008682.html
in which I say:

I am also concerned that this general area of the code lacks unit tests, which it might be wise to include before we effectively activate it for the first time.
...
there's currently no coverage for the function that adds fallback directories. (In fact, I mock it in my unit tests, because I need it to do *something* so I know if it has been called or not.)
...
The function which loads fallback directories currently loads from a string array inside the function, so it would need to be modified to load from a signed file. I support the security benefits of signed fallback directories enough to write client code and unit tests for it, but I'm not sure how the code for the authorities would work - is the proposal to sign a section of the consensus, and output it as a separate file?

If so, we would either need to backport, and/or wait until a majority of the authorities update to tor versions with the feature. And perhaps a majority of clients as well, controlled by a consensus parameter? (Otherwise, using any entry in the file itself would allow clients to effectively be partitioned from the rest of the network by their behaviour.)

While I'm making a list, do we need to modify the existing proposal which describes fallback directories?

Is this change proposed for 0.2.7?
Or all currently supported releases?

Also:

Do we need a new configuration option to give the location of the (signed) Fallback Directories file?
How should this interact with the existing FallbackDir option? Cumulative?

I'm happy to open a new issue for these questions/changes, once we have some idea what we'd like to do about them.

Version 0, edited 4 years ago by teor (next)

comment:10 Changed 4 years ago by nickm

I think making the file signed is a different ticket, and I don't really understand the threat model for it.

comment:11 Changed 4 years ago by teor

Cc: isabella added
Status: needs_informationneeds_review

I will split off populating the directory authorities list and the signed file into other tickets.

This bug needs to be fixed and is suitable for review.

Branch: bug-15642-v3-fallback-unit-tests
Repository: ​​https://github.com/teor2345/tor.git

Commits:

  • Fix to default fallback directories behaviour
  • Unit tests for consider_adding_dir_servers based on fixed default fallback directories behaviour. These are extensive because they cover each of 10 valid cases. There may be ways to abstract some of these shared code, but I feel it would make the tests less clear.

In 0.2.7 per nickm.

Isabella, can you please add the right keywords for 0.2.7?

comment:12 Changed 4 years ago by teor

Split:
#15774 - Signed Fallback Directory File
#15775 - Add Fallback Directory List to add_default_fallback_dir_servers()

comment:13 Changed 4 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Wow, what a big unit test! The code looks solid, though, so merging.

comment:14 Changed 4 years ago by teor

Parent ID: #15228

Marking the parent as #15228, so we know that the fallback directories list will only work in 0.2.7

Note: See TracTickets for help on using tickets.