[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:  new     
 Priority:  normal       |      Milestone:          
Component:  Stem         |        Version:          
 Keywords:  descriptors  |         Parent:          
   Points:               |   Actualpoints:          
-------------------------+--------------------------------------------------
Changes (by atagar):

  * status:  needs_review => new


Comment:

 > Hey, we're getting there..
 > Thanks for the code reviews..

 My pleasure. Thanks for these patches! This has been a task that I've been
 dreading. ;)

 Pushed, with some very minor whitespace tweaks...

 https://gitweb.torproject.org/stem.git/commitdiff/e0095fbe54759c45cbf6d1b120d2b17b47a0ec21
 https://gitweb.torproject.org/stem.git/commitdiff/5da6b9790da266f96258c7c6d6a439ca2ef06529
 https://gitweb.torproject.org/stem.git/commitdiff/d82a70a4fb874ca295c1644e3c77f24afddcbf06

 > I only saw the hang problem when I was testing with invalid data.
 > The existing data is fine. The problem seems to be related to how
 > the threads handle an exception. The RelayDescriptor.init() raising an
 exception
 > is new behaviour I guess..
 >  http://stackoverflow.com/questions/2829329/catch-a-threads-exception-
 in-the-caller-thread-in-python
 > I think this can be addressed as a separate problem.

 The reference to that link puzzled me for a sec but now I think that I see
 what you mean - you think that the DescriptorReader worker thread is the
 issue? If so then this looks like a likely culprit...

 https://gitweb.torproject.org/stem.git/blob/HEAD:/stem/descriptor/reader.py#l486

 The earlier parse_file() call catches ValueErrors but this one doesn't.
 Hence invalid descriptor in an archive might cause problems. Does adding
 the following fix it?

 {{{
 except ValueError, exc:
   self._notify_skip_listeners(target, ParsingFailure(exc))
 }}}

 Do you have a repro for this? I just ran the integ tests but they passed.

 > I've removed the logging calls, except the one that prints info on
 > the fingerprint mismatch.
 > I'm a big fan of logging but I'll defer to the norms of the project :)

 Thanks. Part of the trouble with logging in the descriptor parsing is that
 the validation is actually partly used ot figure out if a file *is* a
 specific descriptor type. Ie, if it can be parsed as a server descriptor
 then it is a server descriptor. Hence the parser seeing invalid data
 doesn't necessarily indicate a problem.

 I love logging too, but it loses its usefulness if there's many false
 positives.

 > It's quite clear that warnings & errors are not that widely used as you
 don't
 > see anything when running the unit tests with log level WARN/ERR.

 That's only because I haven't encountered many things yet that seem to
 warrant them. I'm trying to keep with tor's pattern of logging which is...

 ERROR = Something critical is broken, and impairing functionality. The
 user should be worried.
 WARN = Something's wrong but we're carrying on. The user should know.
 NOTICE = Information that isn't necessarily a problem, but the user should
 be informed.
 INFO = High level runtime information.
 DEBUG = Low level runtime information.
 TRACE = Request/reply logging. Tor doesn't have this, but it's a handy
 level to have.

 I'm generally happy for things to be at INFO level or lower, but things
 NOTICE or above should warrant end user's attention. Hence I'm far pickier
 on those. If you find something that should have additional logging then
 please do add it. I haven't payed as much attention to logging as I
 probably should.

 > ps: some other "== None" occurences..

 Shame on me. I'm sure there's quite a few "!= None" occurrences too - I
 haven't corrected those yet.

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