Opened 8 years ago

Last modified 6 weeks ago

#4363 needs_revision enhancement

Dirauths should save a copy of a consensus that didn't get enough signatures

Reported by: Sebastian Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-dirauth, auditability, save, dump, 035-removed-20180711, 034-unreached-backport-maybe, 033-unreached-backport-maybe
Cc: Actual Points:
Parent ID: Points: 1
Reviewer: mikeperry Sponsor:

Description

Basically right now when a dirauth doesn't get the consensus it generated signed, we don't know what kind of consensus that dirauth wanted because it isn't valid (not enough signatures). We could save a copy so we can investigate

Child Tickets

Change History (23)

comment:1 Changed 8 years ago by arma

Sounds good to me

comment:2 Changed 8 years ago by nickm

Milestone: Tor: 0.2.3.x-final
Type: defectenhancement

Probably not hard to do; definitely worth looking into soon.

comment:3 Changed 8 years ago by nickm

Milestone: Tor: 0.2.3.x-finalTor: unspecified

comment:4 Changed 7 years ago by nickm

Keywords: tor-auth added

comment:5 Changed 7 years ago by nickm

Component: Tor Directory AuthorityTor

comment:6 Changed 2 years ago by dgoulet

Keywords: tor-dirauth added; tor-auth removed

Turns out that tor-auth is for directory authority so make it clearer with tor-dirauth

comment:7 Changed 2 years ago by nickm

Keywords: auditability save dump added
Points: 1
Severity: Normal

comment:8 Changed 2 years ago by nickm

See also #4539

comment:9 Changed 15 months ago by arma

Here's the hack I'm using on moria1 today:

diff --git a/src/or/dirauth/dirvote.c b/src/or/dirauth/dirvote.c
index 1643990..b832b51 100644
--- a/src/or/dirauth/dirvote.c
+++ b/src/or/dirauth/dirvote.c
@@ -3351,6 +3351,16 @@ dirvote_compute_consensuses(void)
       pending[flav].body = consensus_body;
       pending[flav].consensus = consensus;
       n_generated++;
+
+      /* write it out to disk too, for dir auth debugging purposes */
+      {
+      char *filename;
+      tor_asprintf(&filename, "my-consensus-%s",
+                   networkstatus_get_flavor_name(flav));
+      write_str_to_file(get_datadir_fname(filename), consensus_body, 0);
+      tor_free(filename);
+      }
+
       consensus_body = NULL;
       consensus = NULL;
     }

comment:10 Changed 15 months ago by teor

Milestone: Tor: unspecifiedTor: 0.3.5.x-final
Status: newneeds_review

comment:11 Changed 15 months ago by teor

Keywords: 033-backport-maybe 034-backport-maybe added

#4539 makes authorities write mismatching signatures to disk, it isn't as useful as the consensus, but we might want to fix it in the same release.

Also, I wonder if we want to backport this patch?
(All public non-bridge authorities are on 033 or later.)

Last edited 15 months ago by teor (previous) (diff)

comment:12 Changed 15 months ago by asn

Reviewer: mikeperry

comment:13 Changed 15 months ago by arma

Note that my hack placement writes out the consensus with one signature on it (our own). I could imagine a different placement of this code that writes out the consensus with as many sigs as we were able to place on it -- wherever it is that we check to see whether there are enough sigs.

I'm also not clear that I've picked the best possible file names. And maybe dir auths want to do this behavior by default, or maybe it should be gated by a torrc option.

Here be bike sheds. :)

comment:14 Changed 15 months ago by teor

I suggest that we:

  • write out the consensus with no signatures, as early as possible (in case we crash parsing or signing it)
  • write out our own signature
  • write out any signatures we get (good or bad) to a file like the existing v3-status-votes, but for signatures

We should think about how to handle the disk DoS risk for the final file, because anyone can attempt to sign a consensus, and we would put the signature in that file. Maybe we should limit the size of the file?

comment:15 Changed 15 months ago by mikeperry

Status: needs_reviewneeds_revision

Is the only patch here armadev's comment? If so, this ticket is not ready for review.

Besides following our github+travis flow, in terms of the code, I think we should try to save more than just one failure. And definitely make the filename clear that it is a failed consensus.

comment:16 Changed 15 months ago by nickm

Keywords: 035-removed-20180711 added
Milestone: Tor: 0.3.5.x-finalTor: unspecified

Removing needs_revision tickets from 0.3.5 that seem to be stalled. Please move back if they are under active revision or discussion.

comment:17 Changed 7 months ago by teor

These open, non-merge_ready tickets can not get backported to 0.3.3, because 0.3.3 is now unsupported.

comment:18 Changed 7 months ago by teor

Keywords: 033-backport-unreached added

Hmm, I guess they should still get 033-backport-unreached

comment:19 Changed 3 months ago by nickm

Removing 034-backport from all open tickets: 034 has reached EOL.

comment:20 Changed 6 weeks ago by teor

Keywords: 034-unreached-backport-maybe added; 034-backport-maybe removed

0.3.4 is unsupported, we won't be backporting anything else to it.

comment:21 Changed 6 weeks ago by teor

Keywords: 033-unreached-backport added; 033-backport-unreached removed

Fix 033-unreached-backport spelling.

comment:22 Changed 6 weeks ago by teor

Keywords: 033-unreached-backport-maybe added; 033-backport-maybe removed

Remove outdated 033-backport-maybe

comment:23 Changed 6 weeks ago by teor

Keywords: 033-unreached-backport removed
Note: See TracTickets for help on using tickets.