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

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 legacy/trac#13163 (moved), 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 90f6071d in 0.2.4.7-alpha.

changed milestone to %Tor: 0.2.7.x-final in legacy/trac

added component::core tor/tor in Legacy / Trac milestone::Tor: 0.2.7.x-final in Legacy / Trac owner::teor in Legacy / Trac parent::15228 in Legacy / Trac priority::low in Legacy / Trac resolution::implemented in Legacy / Trac status::closed in Legacy / Trac tor-client in Legacy / Trac tor-relay in Legacy / Trac type::defect in Legacy / Trac version::tor 0.2.4.7-alpha in Legacy / Trac labels

Edit: make explanation clearer (I hope)

Trac:
Description: 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. However, the list of default fallback directories is currently NULL, so this issue has no effect.

But I can't imagine too many 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 legacy/trac#13163 (moved), 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 90f6071d in 0.2.4.7-alpha.

to

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 legacy/trac#13163 (moved), 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 90f6071d in 0.2.4.7-alpha.

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 legacy/trac#13163 (moved). 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.

Trac:
Status: new to needs_review

Trac:
Component: - Select a component to Tor

This shouldn't be in 0.2.7

Trac:
Milestone: Tor: 0.2.7.x-final to Tor: 0.2.8.x-final

I'm working on unit tests for legacy/trac#13163 (moved) 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).

Trac:
Owner: N/A to teor
Status: needs_review to assigned

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

Trac:
Status: assigned to needs_review

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 legacy/trac#13163 (moved)'s unit tests being in 0.2.7. Feel free to kick it out again.

Trac:
Milestone: Tor: 0.2.8.x-final to Tor: 0.2.7.x-final

Fixed up spacing issues in an additional commit.

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.

Trac:
Status: needs_review to needs_information

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

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?

Trac:
Cc: nickm to nickm, isabella
Status: needs_information to needs_review

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

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

Trac:
Resolution: N/A to implemented
Status: needs_review to closed

Marking the parent as legacy/trac#15228 (moved), so we know that the fallback directories list will only work in 0.2.7

Trac:
Parent: N/A to legacy/trac#15228 (moved)

closed

mentioned in issue legacy/trac#15774 (moved)

mentioned in issue legacy/trac#15775 (moved)

moved from legacy/trac#15642 (moved)