[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 atagar):

 Great. Please swap this ticket back to 'Needs code/patch review' when
 you've pushed the revisions.

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

 Gotcha. Since this is being used for validation it makes more sense for us
 to raise a ValueError, like the other validation issues do.

 > Do you guys mind custom exceptions ?

 Custom exceptions are fine if there's a good reason for it (see
 stem/__init__.py and stem/connection.py for some examples of places that
 we have custom exceptions).

 However, in this case the ServerDescriptor is documented as raising a
 ValueError when the content is invalid. You can add a custom exception for
 these issues if you want, but please have it subclass ValueError.

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

 Our other validation errors (... of which there's a ton) simply raise an
 exception so we should probably do that here too. I can see some
 advantages to logging the issue as well, but if we did that then it should
 probably be done for all validation issues.

 > I was thinking about adding some code to create valid signed
 descriptors.

 Ah ha. Agreed, that would definitely be better.

 > I'd prefer not to return anything from the function. I'm not sure that
 the digest needs to be publicly accessible?

 As I understand it this function is chiefly so that we can determine if
 the descriptor's digest matches what the network status says that it
 should be...

 https://gitweb.torproject.org/stem.git/blob/HEAD:/stem/descriptor/server_descriptor.py#l257
 https://gitweb.torproject.org/stem.git/blob/HEAD:/stem/descriptor/router_status_entry.py#l248

 However, in that case the ServerDescriptor's digest() method and
 RouterStatusEntryV3's digest attribute would ideally have the same
 encoding. It looks like ServerDescriptor was using a hex encoding and
 RouterStatusEntryV3 was simply storing the base64 value. Here's a router
 status entry example...

 {{{
 r caerSidi p1aag7VwarGxqctS7/fS0y5FU+s wyCIDl1crtM3lkLDEFFpR8+V8oM
 2012-11-17 05:17:27 71.35.143.   230 9001 0
 s Fast Named Running Stable Valid
 v Tor 0.2.2.35
 w Bandwidth=39
 p reject 1-65535
 }}}

 My vote would be for both of them to provide the hex value, though I could
 be persuaded otherwise.

 Cheers! -Damian

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