Opened 3 years ago

Closed 3 months ago

#11045 closed enhancement (implemented)

Check consensus signatures

Reported by: atagar Owned by: nickm
Priority: High Milestone:
Component: Core Tor/Stem Version:
Severity: Normal Keywords: descriptor
Cc: atagar Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

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!

Child Tickets

Attachments (2)

0001-Refactoring-for-checking-consensus-signatures.patch (12.0 KB) - added by NickHopper 3 years ago.
Code that checks consensus signatures
0002-Fixed-descriptor-validation-bug-introduced-in-refact.patch (6.6 KB) - added by NickHopper 3 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 3 years ago by atagar

Resolution: wontfix
Status: newclosed

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.

comment:2 Changed 3 years ago by nickm

Resolution: wontfix
Status: closedreopened

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.

comment:3 Changed 3 years ago by atagar

Parent ID: #11480
Status: reopenedneeds_information

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

comment:4 Changed 3 years ago by atagar

Parent ID: #11480

Oh wait, trac relationships go the opposite direction.

Changed 3 years ago by NickHopper

Code that checks consensus signatures

comment:5 Changed 3 years ago by NickHopper

Status: needs_informationneeds_review

Hi Damian,

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

comment:6 Changed 3 years ago by NickHopper

Cc: atagar added

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

comment:8 Changed 3 years ago by atagar

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

comment:9 Changed 3 years ago by NickHopper

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

comment:10 Changed 3 years ago by atagar

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.

comment:11 Changed 3 years ago by atagar

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.

comment:12 Changed 3 years ago by atagar

Status: needs_reviewneeds_revision

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

comment:13 Changed 3 years ago by atagar

Keywords: descriptor added
Priority: minormajor

comment:14 Changed 3 years ago by nickm_mobile

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?

comment:15 Changed 3 years ago by 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?

comment:16 in reply to:  15 Changed 3 years ago by asn

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!

comment:17 Changed 3 years ago by atagar

Thanks and sorry for the late reply!

No problem - thanks for taking this over!

comment:18 Changed 2 years ago by atagar

Just checked with asn and his hands are still full with other things. Patch welcome if someone wants to take this on!

When I added hidden service descriptor validation I did some refactoring for signature validation which might benefit this too.

comment:19 Changed 3 months ago by atagar

Severity: Normal

Tyler and I have been working on this, and just merged support for checking signatures...

https://gitweb.torproject.org/stem.git/commit/?id=8c84eea

Not gonna resolve this just yet. Tyler is adding the ability to sign descriptors, which will let us add unit tests.

comment:20 Changed 3 months ago by atagar

Resolution: implemented
Status: needs_revisionclosed

Thanks to teor we now have unit test coverage for this too! \o/

https://gitweb.torproject.org/stem.git/commit/?id=68afdd6
https://gitweb.torproject.org/stem.git/commit/?id=2f92235

Finally resolving. :P

Note: See TracTickets for help on using tickets.