Download authority certificates even under blackholed authorities or fallbacks

Our fix for #18816 (moved) is still not great if a significant number of fallbacks are blocked or blackholed. There are a few options to deal with this:

• do what we do with the consensus, and try multiple, simultaneous connections to both authorities and fallback directories, use the first one that succeeds, and close the rest,
• if the connection to a fallback fails, try an authority (this still doesn't help with blackholed fallbacks),
• or any of the other options arma mentions in #18816 (moved):

Longer term (0.2.9 and later), I think we should explore a) having directory_get_from_dirserver() notice that there are tls conns established to dir mirrors that we just recently used (and prefer them), or b) trying to explicitly remember the dir mirror that gave us the consensus and re-use it, and/or c) designing a piggy-back mechanism so we can ask for "the certs that go with this consensus" when we're fetching a consensus and we know we will want the certs for it too (thus saving a round-trip).

changed milestone to %Tor: 0.2.9.x-final

added 028-backport component::core tor/tor milestone::Tor: 0.2.9.x-final owner::teor points::small priority::medium resolution::fixed reviewer::arma severity::normal status::closed type::defect version::tor 0.2.8.1-alpha labels

Please see my branch bug18963 on https://github.com/teor2345/tor.git for an attempt to fix this issue by trying both an authority and a fallback.

This still isn't great if multiple fallbacks or authorities are blackholed.

And it's a significant code change, it probably shouldn't go in 0.2.8.

Edit: please ignore this branch

Trac:
Status: new to needs_revision

Please see my branch bug18963-remember on ​https://github.com/teor2345/tor.git for a much better fix. It remembers the directory we downloaded the consensus or certificates from, and re-uses it to download future certificates.

06d05cb Fetch certificates from the same directory as the consensus ff122a2 Fetch certificates from the same directory as previous certificates (Optional, but I think it's a good idea.)

This works well if multiple fallbacks or authorities are blackholed, because we've already found one that isn't.

This might have minor security implications, if we fetch the consensus and its certificates from the same directory, it can feed us a consistently wrong view of the world.

It's quite a simple code change (much of it it comments or argument-passing), I'd like to see it go in 0.2.8, so we achieve the goal of the fallback directory feature.

Trac:
Status: needs_revision to needs_review
Milestone: Tor: 0.2.??? to Tor: 0.2.8.x-final
Parent: N/A to #18816 (moved)
Points: medium to small
Keywords: N/A deleted, must-fix-before-028-rc added

Did you forget to push that branch? I don't see it on your github repo; I only see the bug19683 branch that you say not to merge.

Replying to nickm:

Did you forget to push that branch? I don't see it on your github repo; I only see the bug19683 branch that you say not to merge.

Pushed now.

(My branch bug18963-remember is based on my branch bug18816) Edit: digit transposition

Shadowing bug:

+  /* Look up the routerstatus for the dir_hint  */
+  const routerstatus_t *rs = NULL;
+
+  if (dir_hint) {
+    /* First try the consensus routerstatus, then the fallback
+     * routerstatus */
+    const routerstatus_t *rs = router_get_consensus_status_by_id(dir_hint);

That inner declaration of rs shouldn't be a declaration.

Other than that, looks good. One thing I would like to make sure I understand, though: what is it that makes us -not- retry the same directory server forever here? Is it the fact that if that server at some point refuses to give us a certificate we asked for, we will then try to download it with dir_hint set to NULL?

Trac:
Status: needs_review to needs_revision

Replying to nickm:

Shadowing bug: {{{

• /* Look up the routerstatus for the dir_hint */
• const routerstatus_t *rs = NULL;
• if (dir_hint) {
• /* First try the consensus routerstatus, then the fallback

• * routerstatus */

• const routerstatus_t *rs = router_get_consensus_status_by_id(dir_hint); }}}

That inner declaration of rs shouldn't be a declaration.

NM1: We should turn on -Wshadow or something :-) 67662ec fixup! Fetch certificates from the same directory as the consensus

Other than that, looks good. One thing I would like to make sure I understand, though: what is it that makes us -not- retry the same directory server forever here? Is it the fact that if that server at some point refuses to give us a certificate we asked for, we will then try to download it with dir_hint set to NULL?

Yes, the logic is as follows:

• when we successfully download a consensus, and we need certificates to validate it, download certificates from the same directory
• as long as there are no failures when downloading certificates, and we keep getting at least one new authority certificate, download other certificates from the same directory
• otherwise, try a random directory

Added a comment explaining that in: 67662ec fixup! Fetch certificates from the same directory as the consensus

Don't retry the same source_dir if any certificate is bad: a6c2bcd fixup! Fetch certificates from the same directory as previous certificates

Only retry the same source_dir as long as it delivers at least one authority certificate: dafbf46 fixup! fixup! Fetch certificates from the same directory as previous certificates

Trac:
Status: needs_revision to needs_review

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

Trac:
Status: assigned to needs_review
Reviewer: N/A to nickm

Trac:
Keywords: N/A deleted, review-group-1 added

I like this, but I want Roger to have a look too if he has a chance.

Trac:
Reviewer: nickm to arma

I just reverted a commit in #18816 (moved), I need to make sure this is rebased.

Please see my branch bug18963-remember-v2

Trac:
Parent: #18816 (moved) to N/A

Trac:
Keywords: N/A deleted, 029-nickm-unsure added

This would be great in 0.2.9, nice to have in 0.2.8, but not essential (the 0.2.8 behaviour without this is no worse than 0.2.7).

It's worth noting that if we don't merge this in 0.2.8, users with blackholed fallbacks will end up downloading a consensus, but failing (or experiencing delay) when downloading certificates. I think this is not ideal, but it is tolerable if we don't want to add more code to 0.2.8.

Trac:
Keywords: N/A deleted, 028-teor-unsure, 029-teor-yes added

Calling this in for 0.2.9, and not essential (but possibly backportable) to 0.2.8.

Trac:
Keywords: must-fix-before-028-rc, 029-teor-yes, 029-nickm-unsure, review-group-1, 028-teor-unsure, 029-proposed deleted, 028-backport added
Milestone: Tor: 0.2.8.x-final to Tor: 0.2.9.x-final

Merged to 0.2.9; leaving for consideration for 0.2.8 backport.

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

Out of caution, not backporting.

Trac:
Resolution: N/A to fixed
Status: needs_review to closed
Milestone: Tor: 0.2.8.x-final to Tor: 0.2.9.x-final

closed

mentioned in issue #18987 (moved)

mentioned in issue #19530 (moved)

mentioned in issue #24740 (moved)

moved to tpo/core/tor#18963 (closed)

mentioned in issue tpo/core/tor#19530 (closed)