Check consensus signatures

Hi Nick, during the Iceland dev meeting you expressed an interest in expanding stem to check consensus signatures as part of work related to guards.

Stem has a soft dependency on the pycrypto module. If it's present then stem presently validates server descriptor fingerprints and digests. Patches are certainly welcome to expand stem's ability to validate descriptor documents. If you have any questions then let me know!

added component::archived/stem descriptor owner::nickm priority::high resolution::implemented severity::normal status::closed type::enhancement labels

Hi Nick. I never heard anything more about this so I'm gonna hazard to guess you don't need this any longer. Feel free to reopen if this is something you'd like to add to Stem after all.

Trac:
Resolution: N/A to wontfix
Status: new to closed

This is something that we're going to need eventually, but not until we implement proposal 236 ("The move to a single guard node"). We'll need it to validate historical consensus data when we're bootstrapping our initial values for the "how much time has this node spent as a guard" weighting value.

Trac:
Status: closed to reopened
Resolution: wontfix to N/A

Flagging this as being blocked by #11480 (moved) (we don't actually have a 'pending related item' or 'blocked' status in trac, so going for the next best thing).

Trac:
Status: reopened to needs_information
Parent: N/A to #11480 (moved)

Oh wait, trac relationships go the opposite direction.

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

Trac:
0001-Refactoring-for-checking-consensus-signatures.patch

Code that checks consensus signatures

Hi Damian,

Here's the patch I sent over email last week (after you added pgp block validation), applied to the descriptor classes.

Trac:
Status: needs_information to needs_review

Trac:
Cc: N/A to atagar

I'll be on vacation 7/19 to 7/28. I'll try to review it before then but authority issues among other things are on my current todo list. If I don't get to this before my trip then it'll be my top priority for afterward - sorry in advance about the delay.

Hi Nick, I took a quick peek to see if I could apply this before heading on my trip but ran into issues. Did you run the unit tests? I'm getting seventeen failures plus static checker issues...

% patch -p1 < ~/Desktop/0001-Refactoring-for-checking-consensus-signatures.patch
patching file stem/descriptor/__init__.py
patching file stem/descriptor/networkstatus.py
patching file stem/descriptor/server_descriptor.py

% ./run_tests.py --unit
...
STATIC CHECKS
* /home/atagar/Desktop/stem/stem/descriptor/__init__.py
  line 383  - line has trailing whitespace
  line 394  - W293 W293 blank line contains whitespace
  line 396  - E303 E303 too many blank lines (4)
  line 420  - W291 W291 trailing whitespace
  line 619  - E302 E302 expected 2 blank lines, found 1

* /home/atagar/Desktop/stem/stem/descriptor/networkstatus.py
  line 607  - line has trailing whitespace
  line 624  - W293 W293 blank line contains whitespace
  line 626  - E303 E303 too many blank lines (5)
  line 640  - E261 E261 at least two spaces before inline comment
  line 653  - W291 W291 trailing whitespace
  line 1467 - line has trailing whitespace
  line 1471 - line has trailing whitespace
  line 1485 - line has trailing whitespace
  line 1488 - W293 W293 blank line contains whitespace
  line 1492 - E303 E303 too many blank lines (4)
  line 1513 - W291 W291 trailing whitespace
  line 1547 - W291 W291 trailing whitespace
  line 1581 - W291 W291 trailing whitespace

* /home/atagar/Desktop/stem/stem/descriptor/server_descriptor.py
  line 36   - 'codecs' imported but unused
  line 729  - W293 W293 blank line contains whitespace
  line 764  - W293 W293 blank line contains whitespace
  line 779  - E231 E231 missing whitespace after ','
  line 787  - E231 E231 missing whitespace after ','
  line 794  - E231 E231 missing whitespace after ','

TESTING FAILED (9 seconds)
  [UNIT TEST] test_archived_bz2 (test.unit.descriptor.reader.TestDescriptorReader) ... FAIL
  [UNIT TEST] test_archived_gzip (test.unit.descriptor.reader.TestDescriptorReader) ... FAIL
  [UNIT TEST] test_archived_uncompressed (test.unit.descriptor.reader.TestDescriptorReader) ... FAIL
  [UNIT TEST] test_basic_example (test.unit.descriptor.reader.TestDescriptorReader) ... FAIL
  [UNIT TEST] test_multiple_runs (test.unit.descriptor.reader.TestDescriptorReader) ... FAIL
  [UNIT TEST] test_persistence_path (test.unit.descriptor.reader.TestDescriptorReader) ... FAIL
  [UNIT TEST] test_skip_nondescriptor_contents (test.unit.descriptor.reader.TestDescriptorReader) ... FAIL
  [UNIT TEST] test_can_iterate_multiple_times (test.unit.descriptor.remote.TestDescriptorDownloader) ... FAIL
  [UNIT TEST] test_query_download (test.unit.descriptor.remote.TestDescriptorDownloader) ... FAIL
  [UNIT TEST] test_cr_in_contact_line (test.unit.descriptor.server_descriptor.TestServerDescriptor) ... ERROR
  [UNIT TEST] test_metrics_descriptor (test.unit.descriptor.server_descriptor.TestServerDescriptor) ... ERROR
  [UNIT TEST] test_metrics_descriptor_multiple (test.unit.descriptor.server_descriptor.TestServerDescriptor) ... ERROR
  [UNIT TEST] test_negative_uptime (test.unit.descriptor.server_descriptor.TestServerDescriptor) ... ERROR
  [UNIT TEST] test_non_ascii_descriptor (test.unit.descriptor.server_descriptor.TestServerDescriptor) ... ERROR
  [UNIT TEST] test_old_descriptor (test.unit.descriptor.server_descriptor.TestServerDescriptor) ... ERROR
  [UNIT TEST] test_with_tarfile_object (test.unit.descriptor.server_descriptor.TestServerDescriptor) ... ERROR
  [UNIT TEST] test_with_tarfile_path (test.unit.descriptor.server_descriptor.TestServerDescriptor) ... ERROR

Please make sure the unit tests, integ tests, and pyflakes/pep8 all pass...

https://stem.torproject.org/faq.html#how-do-i-run-the-tests

No rush. As mentioned previously I won't be able to get to this again until after the 28th. :)

Trac:
0002-Fixed-descriptor-validation-bug-introduced-in-refact.patch

OK, second patch fixes the unit tests (integ tests also pass) and makes pyflakes/pep8 happy (insofar as it doesn't complain about code I've touched.)

Hi Nick. I apologize for failing to get back to you for so long. Usually code reviews are my top priority (after all, I love getting contributions from people wanting to help!), but a hardware failure followed by deadlines for work have been distracting me of late.

I sunk a large chunk of today into revising this patch but there's still a little more to go...

https://gitweb.torproject.org/user/atagar/stem.git/shortlog/refs/heads/consensus_validation

In particular I'm a little worried about some 'bytes => unicode => bytes' conversions that I suspect will cause us to have issues for some inputs when using python 3.x. I'll try to finish this up next week and get you something for you to review.

Oops! Think I forgot to ask: Nick, are you ok with your Stem contributions being in the public domain? Here's why I need to ask.

Hi Nick, sunk in a few more hours and now have a patch I'm really itching to merge in my consensus_validation branch. You should be able to try it out with something like...

% git remote add atagar git://git.torproject.org/user/atagar/stem.git
% git fetch atagar
% git checkout atagar/consensus_validation

The code now looks good to me. Trouble is it's not actually used at all, and when it is it understandably breaks our tests really badly.

See the RelayDescriptor for an example. Its init method calls self._validate_content() so we always verify its integrity. Our unit tests then do a couple approaches to account for this...

1. They generate validly signed descriptor content with the sign_descriptor_content() function.

2. When making test data we mocked out the _verify_digest() method.

So there's a couple things that need to happen before we merge this.

1. The KeyCertificate's init method should call check_certificate() without a date to verify its integrity. That's trivial - the trick will be getting the tests to pass.

2. We need tests for verify_consensus(). Presently it has zero coverage so it might be completely broken right now and we wouldn't have a clue.

Ball is now back in your court. I've invested quite a bit of time into this but it's now out of the realm of code cleanup and back to needing some missing bits.

Trac:
Status: needs_review to needs_revision

Trac:
Priority: minor to major
Keywords: N/A deleted, descriptor added

Hmm. It looks like tor will want this on an 026 timeframe for asn's guardiness scripts. Is the current status that this is unlikely to ever get fixed unless I fix it, or is the someone else who'd like to try fixing my patch up?

Hi Nick. I can step in if needed, though I'd rather not necessarily do all things when it comes to Stem (turns out I don't scale too well, and besides which crypto validation is something I don't know too well and are likely to bugger up).

George, if this functionality is important to you for your guardiness script? If so, would you mind taking this over?

Replying to atagar:

Hi Nick. I can step in if needed, though I'd rather not necessarily do all things when it comes to Stem (turns out I don't scale too well, and besides which crypto validation is something I don't know too well and are likely to bugger up).

George, if this functionality is important to you for your guardiness script? If so, would you mind taking this over?

Hello, I can take this over, but I probably won't have time to do it anytime soon. I can start doing it after I have finished all other tasks of the guardfraction project.

Thanks and sorry for the late reply!