[tor-bugs] #5810 [Stem]: Implement verification of server descriptor

[Tor Bug Tracker & Wiki]

#5810: Implement verification of server descriptor
-------------------------+--------------------------------------------------
 Reporter:  reganeet     |          Owner:  reganeet      
     Type:  enhancement  |         Status:  needs_revision
 Priority:  normal       |      Milestone:                
Component:  Stem         |        Version:                
 Keywords:  descriptors  |         Parent:                
   Points:               |   Actualpoints:                
-------------------------+--------------------------------------------------

Comment(by eoinof):

 Replying to [comment:20 atagar]:
 > Wow, looks great! First, are you ok for your stem patches (past and
 future) to be in the public domain? Here's the reason why I need to ask...
 >
 >
 https://trac.torproject.org/projects/tor/wiki/doc/stem#CopyrightforPatches
 >
 Yeah, I was aware of that, I have no problem for them to be in the public
 domain.

 > Besides that just a few questions and suggestions...
 >
 > > +from Crypto.Util import asn1
 > > +from Crypto.Util.number import bytes_to_long, long_to_bytes
 >
 > If these are only available by default on debian based distros then we
 should have this in a try/catch for an ImportError. Actually, since this
 is replacing the rsa module maybe we should make a method for this like
 the stem.prereq.is_rsa_available()?
 I'm not sure exactly what platforms include the lib so I'll the put the
 prereq check back in.

 >
 > > +    #Ensure the digest of the descriptor has been calculated
 >
 > Minor thing but please have a space between the # and the start of the
 comment. It makes it easier to read. The convention that I've been using
 for comments is to do properly puncuated sentences if it spans mupltiple
 sentences...
 >
 > {{{
 > # Configuration options that are fetched by a special key. The keys are
 > # lowercase to make case insensitive lookups easier.
 > }}}
 >
 > ... and do all lowercase without punctuation if it's a single
 sentence...
 >
 > {{{
 > # unchangeable GETINFO parameters
 > }}}
 I'll update the comments to follow this convention.

 >
 > > +    #Validate the descriptor if required.
 > > +    if validate and not self.is_valid():
 > > +      log.error("Descriptor info not valid")
 > > +      raise ValueError("Invalid data")
 >
 > This provides precious little information to the caller. Rather than
 having is_valid() that returns a boolean maybe it should be a
 validate_content() method that raises a ValueError? Then your present
 log.warn() calls could instead raise a ValueError with a useful message.
 I think I based this structure on the previous comments in the ticket. If
 you prefer exceptions I can change the code to use them. Do you guys mind
 custom exceptions ? I'd be inclined to leave the log messages in as well,
 but I'll stick with whatever your preferred style is for the code..

 >
 > > key_as_string = ''.join(self.signing_key.split('\n')[1:4])
 >
 > This took me a couple minutes to figure out. Maybe rename it and add a
 comment? Also, we should end on -1 in case the size of the key content
 ever changes.
 >
 > {{{
 > # strips off the '-----BEGIN RSA PUBLIC KEY-----' header and
 corresponding footer
 >
 > key_content = ''.join(self.signing_key.split('\n')[1:-1])
 > }}}
 Ok, It's sometimes hard to know what parts need explaining :)

 >
 > > #TODO - what is the purpose of allowing a NULL fingerprint ?
 >
 > Because the tor spec says that it isn't mandatory...
 >
 > https://gitweb.torproject.org/torspec.git/blob/HEAD:/dir-spec.txt#l411
 >
 > I'm not entirely clear but maybe it's optional due to being first
 introduced in tor verison 0.1.0.6? If so then that reason no longer makes
 sense (0.1.0.6 has been dead for years). Wanna file a tor ticket to ask
 Nick if this should be made a mandatory field?
 Ah, ok.. I'll remove the TODO so.. I'll look into filing a tor ticket
 about the fingerprint once I've finished all the stuff here.

 >
 > > +      #FIXME - stopgap measure until tests are fixed.
 > > +      #return False
 >
 > I think that you misunderstood my suggestion about the unit tests. The
 unit tests are trying to check various aspects of how server descriptors
 are parsed and validated. To do this they need to be able to craft custom
 descriptors and those custom descriptors will not be valid according to
 _verify_descriptor(). Personally I don't think that this is a bug.
 >
 > To account for this the *unit tests* should mock _verify_descriptor() to
 simply return that the descriptor is valid. This does not require a hack
 in the RelayDescriptor class. That is to say, the unit tests should
 have...
 >
 > {{{
 > def setUp(self):
 >
 mocking.mock(stem.descriptor.server_descriptor.RelayDescriptor._verify_descriptor,
 mocking.return_true())
 >
 > def tearDown(self):
 >   mocking.revert_mocking()
 > }}}
 >
 I was thinking about adding some code to create valid signed descriptors.
 This would mean the unit tests would work, and would be exercising more of
 the code base. The only changes necessary would be in cases where the unit
 test got a descriptor, and then changed some of it's contents.. in this
 case you'd need to add a new function call sign_descriptor(..)

 > > +      log.warn("unable to calculate digest for descriptor")
 > > +      #TODO should we raise here ?
 >
 > Sure, feel free. We will only hit that condition if we were constructed
 from grossly invalid content and the user set 'validate' to False when we
 were constructed. At this point the caller deserves an exception. ;)
 Ok, I'll change this.

 >
 > > def digest(self):
 > > ...
 >
 > It looks like you're making two changes here...
 >
 > 1. We're stripping content prior to the "router " prefix. This is a good
 point, I forgot about annotations or that callers might allow for invalid
 content.
 >
 > 2. Rather than returning the hexdigest() you're returning the digest().
 Is this desirable?

 I'd prefer not to return anything from the function. I'm not sure that the
 digest needs to be publicly accessible? The reason I changed the code was
 that the decrypted signature[digest] is not in hex so I decided that
 rather than converting both the local digest, and the decrypted digest to
 hex I'd just leave them as they are.

 >
 > > +    ##################
 > > +    ##PROPER MESSING!!
 > > +    ##################
 In an ideal world I'd much prefer to use a crypto lib, rather than rolling
 my own. So I added this to note my disapproval.. I'll remove it.

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