[tor-bugs] #31369 [Core Tor/Stem]: HSv3 descriptor support in stem [decoding]

[Tor Bug Tracker & Wiki]

#31369: HSv3 descriptor support in stem [decoding]
-----------------------------------------+--------------------------------
 Reporter:  asn                          |          Owner:  atagar
     Type:  defect                       |         Status:  reopened
 Priority:  Medium                       |      Milestone:
Component:  Core Tor/Stem                |        Version:
 Severity:  Normal                       |     Resolution:
 Keywords:  tor-hs onionbalance scaling  |  Actual Points:  2.5
Parent ID:  #26768                       |         Points:  4
 Reviewer:                               |        Sponsor:  Sponsor27-must
-----------------------------------------+--------------------------------

Comment (by asn):

 Replying to [comment:15 atagar]:
 > Perfect, thanks asn!
 >
 > > But yes, IMO we should have a generic certificate validate (or
 verify?) function that just verifies that the certificate correctly signs
 the included key using the intended public key
 >
 > Agreed. Thought I just had: lets use isinstance checks here. That is to
 say...
 >
 > {{{
 > def validate(self, descriptor):
 >   if isinstance(descriptor,
 stem.descriptor.server_descriptor.RelayDescriptor):
 >     # server descriptor specific things
 >   elif isinstance(descriptor,
 stem.descriptor.hidden_service.HiddenServiceDescriptorV3):
 >     # hidden service specific things
 >   else:
 >     raise ValueError('ED25519 certificate validation only presently
 supports server and hidden service descriptors, not %s' %
 type(descriptor).__name__)
 > }}}
 >

 I'm fine with doing this if you prefer this approach. That said, IMO it's
 a layer violation: It shouldn't be the certificate's job to validate the
 descriptor; it should be the descriptor's job to validate the certificate.
 So IMO, the certificate codebase should be simple (just validate the sig
 with a public key and make sure it's not expired), and the complicated
 stuff should happen at the HS subsystem.

 If you think we should go with this `isintance()` approach anyhow, that's
 fine with me.


 > > Great! I see that you have disabled the checksum verification but this
 actually passes for me.
 >
 > Interesting! Yes, likely is a pysha3 vs python 3.6 difference. I'll look
 into it.
 >
 > > Would you be interested in digging more there; that is parsing the
 middle and inner layers of the descriptor?
 >
 > Yup! At this point I should have everything I need to run with this.
 Many thanks asn!
 >
 > Give me a bit and hopefully I'll have a mergeable branch for you to take
 a peek at.
 >
 > > In the meanwhile, I will be working on the encoding part, if you don't
 think that's too chaotic to do.
 >
 > Nope. Feel free, but I won't be able to give much significant thought to
 it until this end is wrapped up.
 >
 > > Commit 14a44b1c6e1438abdf5687a1c468536d88481f81 killed a few XXXs I
 had added to remind myself in the future (so that we don't forget them
 before merging).
 >
 > If that's the case would you mind this before encoding so we can get
 this branch into a mergeable state?
 >
 > To be clear at this point my plan is...
 >
 > 1. Productionize hsv3_crypto.py. This will probably include some
 refactoring.
 > 2. Add parsing support for the other layers.
 > 3. Unit test decryption and parsing.
 > 4. Consider merging the branch.
 >
 > If there's any additional verifications we'd care to get in prior to
 merging it would be preferable (but not a big whoop) to get them in during
 this.
 >
 > > FYI the encoding methods will also need an extra argument during
 descriptor encoding
 >
 > Gotcha. That's perfectly fine. Descriptor creation can take additional
 arguments. For example server_descriptor.py has...
 >
 > {{{
 > def content(cls, attr = None, exclude = (), sign = False, signing_key =
 None, exit_policy = None):
 > }}}
 >
 > > BTW, any plans on putting this stuff into github so that I can do some
 inline comments?
 >
 > Not at present. I use my personal TPO repo for in flight branches
 (GitHub is simply a mirror of the official repo). That said, I'm leaning
 toward migrating to GitHub when the rest of tor moves to GitLab so in the
 future I'll likely fully be on that platform.
 >
 > > I'm splitting this ticket into two.
 >
 > Perfect! Thanks.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/31369#comment:16>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online