[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #5810 [Stem]: Implement verification of server descriptor
#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: needs_review => needs_revision
Comment:
> It will only occur if the data
> in descriptor_archive.tar.bz2 were invalid..
Oops. Iirc I messed with descriptor data at a few points to either make
the tests more interesting or obscure people's email address. Feel free to
replace fabrications with real descriptor data. You can probably find some
in your 'stem/test/data/cached-consensus' file. If not then let me know
and I'll fill in the gaps.
> ... will hang if construction of the RelayDescriptor raises an
exception.. as happens
> with the above patch.
Ewww. Any idea why? I'd expect the exception to be propagated through to
the above parse_file() caller, and iirc there's tests for this through the
'stem.descriptor.reader'. Does this hang when parsing invalid data without
your changes or is it a new regression?
========================================
If you replace 'self._digest' with self.digest() at...
{{{
+ # The local digest is stored in hex so need to encode the decrypted
digest
+ digest_hex = digest.encode('hex')
+ if digest_hex != self._digest:
+ log.warn("Decrypted digest does not match local digest")
+ raise ValueError("Decrypted digest does not match local digest")
}}}
Then you can drop the digest() call and comment from...
{{{
+ # validate the descriptor if required
+ if validate:
+ # ensure the digest of the descriptor has been calculated
+ self.digest()
+ self._validate_content()
}}}
========================================
{{{
+ def digest(self):
+ # Digest is calculated from everything in the
+ # descriptor except the router-signature.
+ raw_descriptor = str(self)
+ start_token = "router "
}}}
This isn't how a lazy loading method work (self._digest is essentially
never used and you're always recalculating it). You're saving self._digest
then returning that so any reason not to do something like the following?
{{{
def digest(self):
if self._digest is None:
... present code...
return self._digest
}}}
========================================
{{{
+ log.warn("unable to calculate digest for descriptor")
+ raise ValueError("unable to calculate digest for descriptor")
}}}
I still think that we should drop all of these logging calls. Maybe do the
logging in parse_file() Instead so you log for all validation failures
rather than just these new ones?
========================================
{{{
+ :raises a ValueError if signature does not match content,
}}}
Missing parentheses and extra comma. Probably better as ":raises:
ValueError if signature does not match the content".
========================================
{{{
+ if not self.signature:
+ log.warn("Signature missing")
+ raise ValueError("Signature missing")
}}}
This check isn't really necessary now that this method is private. By this
point we know that we have a valid descriptor object (except for this
check), so the signature attribute must exist. That said, it's fine to
leave in if you want.
========================================
> + key_as_string = ''.join(self.signing_key.split('\n')[1:4])
Ah, didn't catch that you're doing a '\n' split and '' join so the key
content all resides on a single line. Is that important? If so then please
note it in the above comment.
========================================
{{{
+ # if we have a fingerprint then check that our fingerprint is a hash
of
+ # our signing key
+ if self.fingerprint:
}}}
If you do end up filing a ticket with tor to make the fingerprint
mandatory then please add a link to it in the comment here. Ticket links
can be truncated, for instance "https://trac.torproject.org/5810"; works
for this ticket (no need to include the "projects/tor/ticket/"
boilerplate).
========================================
{{{
+ log.notice("No fingerprint for this descriptor")
}}}
This log message doesn't contain enough information to be useful. If, say,
1% of descriptors are missing fingerprints then making a 'GETINFO desc
/all-recent' call would result in dozens of these messages without saying
*what* is missing a fingerprint. We should either drop this to an INFO
level message and include more information or drop the log message
entirely. Personally I favor the later until Nick says if fingerprints
should be mandatory or not.
========================================
{{{
+ log.info("Descriptor verified.")
}}}
We definitely don't want this. A single 'GETINFO desc/all-recent' would
flood our INFO level logs with this message, which really isn't helpful.
========================================
{{{
+ if not stem.prereq.is_crypto_available():
+ return
+ else:
}}}
No need for the else indentation block here.
========================================
{{{
+ decrypted_int = pow(sig_as_long, public_exponent ,modulus)
}}}
Typo with placement of a space.
========================================
{{{
+ if IS_CRYPTO_AVAILABLE == None:
}}}
Oops, my bad. I don't always remember but None checks should use identity
comparison...
http://www.python.org/dev/peps/pep-0290/#testing-for-none
========================================
{{{
- self.assertEquals("2C7B27BEAB04B4E2459D89CA6D5CD1CC5F95A689",
desc.digest())
+ self.assertEquals("2C7B27BEAB04B4E2459D89CA6D5CD1CC5F95A689",
desc.digest().upper())
}}}
I'm a little inclined toward having digest() provide an uppercase value
since every other hex thing I'm aware of in tor (fingerprints for
instance) are uppercase. Is there any reason you can think of to make it
lowercase?
========================================
{{{
+def sign_descriptor_content(desc_content):
...
}}}
Yikes! Very, very nice. Mind adding some pydocs?
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/5810#comment:25>
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