Opened 7 years ago

Last modified 5 months 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, 033-backport-maybe, 034-backport-maybe, 035-removed-20180711
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 (16)

comment:1 Changed 7 years ago by arma

Sounds good to me

comment:2 Changed 7 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 7 years ago by nickm

Milestone: Tor: 0.2.3.x-finalTor: unspecified

comment:4 Changed 6 years ago by nickm

Keywords: tor-auth added

comment:5 Changed 6 years ago by nickm

Component: Tor Directory AuthorityTor

comment:6 Changed 19 months 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 19 months ago by nickm

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

comment:8 Changed 19 months ago by nickm

See also #4539

comment:9 Changed 6 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 6 months ago by teor

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

comment:11 Changed 6 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 6 months ago by teor (previous) (diff)

comment:12 Changed 6 months ago by asn

Reviewer: mikeperry

comment:13 Changed 6 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 6 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 5 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 5 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.

Note: See TracTickets for help on using tickets.