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

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#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:                
-------------------------+--------------------------------------------------
Changes (by atagar):

  * status:  new => needs_revision


Comment:

 > This whitespace problem is a bugfix. [I've split it out from the updated
 descriptor]

 Merged.

 > I've added more info on the signature verification in the code comments
 this time, and you can read more about it here:
 > ...

 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

 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()?

 > +    #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
 }}}

 > +    #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.

 > 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])
 }}}

 > #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?

 > +      #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()
 }}}

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

 > 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?

 > +    ##################
 > +    ##PROPER MESSING!!
 > +    ##################

 I'm not sure what this means.

 Cheers! -Damian

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/5810#comment:20>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs