Opened 4 years ago

Closed 3 years ago

#18963 closed defect (fixed)

Download authority certificates even under blackholed authorities or fallbacks

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version: Tor: 0.2.8.1-alpha
Severity: Normal Keywords: 028-backport
Cc: Actual Points:
Parent ID: Points: small
Reviewer: arma Sponsor:

Description

Our fix for #18816 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:

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).

Child Tickets

Change History (19)

comment:1 Changed 4 years ago by teor

Status: newneeds_revision

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

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

comment:2 Changed 4 years ago by teor

Keywords: must-fix-before-028-rc added
Milestone: Tor: 0.2.???Tor: 0.2.8.x-final
Parent ID: #18816
Points: mediumsmall
Status: needs_revisionneeds_review

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.

comment:3 Changed 4 years ago by 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.

comment:4 in reply to:  3 Changed 4 years ago by teor

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.

comment:5 Changed 4 years ago by teor

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

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

comment:6 Changed 4 years ago by nickm

Status: needs_reviewneeds_revision

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?

comment:7 in reply to:  6 Changed 4 years ago by teor

Status: needs_revisionneeds_review

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

comment:8 Changed 4 years ago by nickm

Owner: set to teor
Status: needs_reviewassigned

comment:9 Changed 4 years ago by nickm

Reviewer: nickm
Status: assignedneeds_review

comment:10 Changed 4 years ago by nickm

Keywords: review-group-1 added

comment:11 Changed 4 years ago by nickm

Reviewer: nickmarma

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

comment:12 Changed 4 years ago by teor

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

comment:13 Changed 4 years ago by teor

Please see my branch bug18963-remember-v2

comment:14 Changed 4 years ago by nickm

Parent ID: #18816

comment:15 Changed 4 years ago by nickm

Keywords: 029-nickm-unsure added

comment:16 Changed 4 years ago by teor

Keywords: 029-teor-yes 028-teor-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.

comment:17 Changed 4 years ago by nickm

Keywords: 028-backport added; must-fix-before-028-rc 029-proposed review-group-1 029-nickm-unsure 029-teor-yes 028-teor-unsure removed
Milestone: Tor: 0.2.8.x-finalTor: 0.2.9.x-final

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

comment:18 Changed 4 years ago by nickm

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

Merged to 0.2.9; leaving for consideration for 0.2.8 backport.

comment:19 Changed 3 years ago by nickm

Milestone: Tor: 0.2.8.x-finalTor: 0.2.9.x-final
Resolution: fixed
Status: needs_reviewclosed

Out of caution, not backporting.

Note: See TracTickets for help on using tickets.