Refinements to previous checkin after code review/feedback

cf https://trac.torproject.org/projects/tor/ticket/5810

Removed most of the logging code
_digest function now returns the digest in uppercase hex
digest value is now calculated once & cached for evermore.
moved key string manipulation code to a separate function as it is used
 more than once, cf _get_key_bytes()
reverted change to test/integ/descriptor/server_descriptor as _digest now returns uppercase hex
added some documentation to _digest()
added some documentation to sign_descriptor_content()

stem/descriptor/server_descriptor.py

@@ -596,121 +596,106 @@ def __init__(self, raw_contents, validate = True, annotations = None):
 
     # 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 "
-    sig_token = "\nrouter-signature\n"
-    start = raw_descriptor.find(start_token)
-    sig_start = raw_descriptor.find(sig_token)
-    end = sig_start + len(sig_token)
-    if start >= 0 and sig_start > 0 and end > start:
-      for_digest = raw_descriptor[start:end]
-      digest_hash = hashlib.sha1(for_digest)
-      self._digest = digest_hash.hexdigest()
-    else:
-      log.warn("unable to calculate digest for descriptor")
-      raise ValueError("unable to calculate digest for descriptor")
+    """
+    Get the digest for this descriptor.
+    If the digest has not already been calculated it will be done inline.
+    :raises: ValueError if the digest canot be calculated
+    :returns: the digest string encoded in uppercase hex
+    """
+
+    if self._digest is None:
+      # Digest is calculated from everything in the
+      # descriptor except the router-signature.
+      raw_descriptor = str(self)
+      start_token = "router "
+      sig_token = "\nrouter-signature\n"
+      start = raw_descriptor.find(start_token)
+      sig_start = raw_descriptor.find(sig_token)
+      end = sig_start + len(sig_token)
+      if start >= 0 and sig_start > 0 and end > start:
+        for_digest = raw_descriptor[start:end]
+        digest_hash = hashlib.sha1(for_digest)
+        self._digest = digest_hash.hexdigest().upper()
+      else:
+        raise ValueError("unable to calculate digest for descriptor")
 
     return self._digest
 
   def _validate_content(self):
     """
-    Validates that our content matches our signature.
-    
-    :raises a ValueError if signature does not match content,
+    Validates that the descriptor content matches the signature.
+    :raises: ValueError if the signature does not match the content
     """
 
-    if not self.signature:
-      log.warn("Signature missing")
-      raise ValueError("Signature missing")
-
-    # strips off the '-----BEGIN RSA PUBLIC KEY-----' header and corresponding footer
-    key_as_string = ''.join(self.signing_key.split('\n')[1:4])
+    key_as_bytes = RelayDescriptor._get_key_bytes(self.signing_key)
 
-    # calculate the signing key hash
-    key_as_der = base64.b64decode(key_as_string)
-    key_der_as_hash = hashlib.sha1(key_as_der).hexdigest()
-
-    # if we have a fingerprint then check that our fingerprint is a hash of
-    # our signing key
+    # ensure the fingerprint is a hash of the signing key
     if self.fingerprint:
+      # calculate the signing key hash
+      key_der_as_hash = hashlib.sha1(key_as_bytes).hexdigest()
       if key_der_as_hash != self.fingerprint.lower():
-        log.warn("Hash of our signing key doesn't match our fingerprint. Signing key hash: %s, fingerprint: %s" % (key_der_as_hash, self.fingerprint.lower()))
+        log.warn("Signing key hash: %s != fingerprint: %s" % (key_der_as_hash, self.fingerprint.lower()))
         raise ValueError("Fingerprint does not match hash")
-    else:
-      log.notice("No fingerprint for this descriptor")
 
-    try:
-      self._verify_descriptor(key_as_der)
-      log.info("Descriptor verified.")
-    except ValueError, e:
-      log.warn("Failed to verify descriptor: %s" % e)
-      raise e
+    self._verify_descriptor(key_as_bytes)
 
   def _verify_descriptor(self, key_as_der):
     if not stem.prereq.is_crypto_available():
       return
-    else:
-      from Crypto.Util import asn1
-      from Crypto.Util.number import bytes_to_long, long_to_bytes
-
-      # get the ASN.1 sequence
-      seq = asn1.DerSequence()
-      seq.decode(key_as_der)
-      modulus = seq[0]
-      public_exponent = seq[1] #should always be 65537
-
-      # convert the descriptor signature to an int before decrypting it
-      sig_as_string = ''.join(self.signature.split('\n')[1:4])
-      sig_as_bytes = base64.b64decode(sig_as_string)
-      sig_as_long = bytes_to_long(sig_as_bytes)
-
-      # use the public exponent[e] & the modulus[n] to decrypt the int
-      decrypted_int = pow(sig_as_long, public_exponent ,modulus)
-      # block size will always be 128 for a 1024 bit key
-      blocksize = 128
-      # convert the int to a byte array.
-      decrypted_bytes = long_to_bytes(decrypted_int, blocksize)
-
-      ############################################################################
-      ## The decrypted bytes should have a structure exactly along these lines.
-      ## 1 byte  - [null '\x00']
-      ## 1 byte  - [block type identifier '\x01'] - Should always be 1
-      ## N bytes - [padding '\xFF' ]
-      ## 1 byte  - [separator '\x00' ]
-      ## M bytes - [message]
-      ## Total   - 128 bytes
-      ## More info here http://www.ietf.org/rfc/rfc2313.txt
-      ##                esp the Notes in section 8.1
-      ############################################################################
-      try:
-        if decrypted_bytes.index('\x00\x01') != 0:
-          log.warn("Verification failed, identifier missing")
-          raise ValueError("Verification failed, identifier missing")
-      except ValueError:
-        log.warn("Verification failed, Malformed data")
-        raise ValueError("Verification failed, Malformed data")
-
-      try:
-        identifier_offset = 2
-        # Find the separator
-        seperator_index = decrypted_bytes.index('\x00', identifier_offset)
-      except ValueError:
-        log.warn("Verification failed, seperator not found")
-        raise ValueError("Verification failed, seperator not found")
-
-      digest = decrypted_bytes[seperator_index+1:]
-      # 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")
+
+    from Crypto.Util import asn1
+    from Crypto.Util.number import bytes_to_long, long_to_bytes
+
+    # get the ASN.1 sequence
+    seq = asn1.DerSequence()
+    seq.decode(key_as_der)
+    modulus = seq[0]
+    public_exponent = seq[1] # should always be 65537
+
+    sig_as_bytes = RelayDescriptor._get_key_bytes(self.signature)
+    # convert the descriptor signature to an int
+    sig_as_long = bytes_to_long(sig_as_bytes)
+    # use the public exponent[e] & the modulus[n] to decrypt the int
+    decrypted_int = pow(sig_as_long, public_exponent, modulus)
+    # block size will always be 128 for a 1024 bit key
+    blocksize = 128
+    # convert the int to a byte array.
+    decrypted_bytes = long_to_bytes(decrypted_int, blocksize)
+
+    ############################################################################
+    ## The decrypted bytes should have a structure exactly along these lines.
+    ## 1 byte  - [null '\x00']
+    ## 1 byte  - [block type identifier '\x01'] - Should always be 1
+    ## N bytes - [padding '\xFF' ]
+    ## 1 byte  - [separator '\x00' ]
+    ## M bytes - [message]
+    ## Total   - 128 bytes
+    ## More info here http://www.ietf.org/rfc/rfc2313.txt
+    ##                esp the Notes in section 8.1
+    ############################################################################
+    try:
+      if decrypted_bytes.index('\x00\x01') != 0:
+        raise ValueError("Verification failed, identifier missing")
+    except ValueError:
+      raise ValueError("Verification failed, Malformed data")
+
+    try:
+      identifier_offset = 2
+      # find the separator
+      seperator_index = decrypted_bytes.index('\x00', identifier_offset)
+    except ValueError:
+      raise ValueError("Verification failed, seperator not found")
+
+    digest = decrypted_bytes[seperator_index+1:]
+    # The local digest is stored in uppercase hex;
+    #  - so decode it from hex
+    #  - and convert it to lower case
+    local_digest = self.digest().lower().decode('hex')
+    if digest != local_digest:
+      raise ValueError("Decrypted digest does not match local digest")
 
   def _parse(self, entries, validate):
     entries = dict(entries) # shallow copy since we're destructive
@@ -746,6 +731,19 @@ def __cmp__(self, other):
       return 1
 
     return str(self).strip() > str(other).strip()
+
+  @staticmethod
+  def _get_key_bytes(key_string):
+    # Remove the newlines from the key string & strip off the
+    # '-----BEGIN RSA PUBLIC KEY-----' header and
+    # '-----END RSA PUBLIC KEY-----' footer
+    key_as_string = ''.join(key_string.split('\n')[1:4])
+
+    # get the key representation in bytes
+    key_bytes = base64.b64decode(key_as_string)
+
+    return key_bytes
+
 
 class BridgeDescriptor(ServerDescriptor):
   """

stem/prereq.py

@@ -62,7 +62,7 @@ def is_python_27():
 def is_crypto_available():
   global IS_CRYPTO_AVAILABLE
 
-  if IS_CRYPTO_AVAILABLE == None:
+  if IS_CRYPTO_AVAILABLE is None:
     try:
       from Crypto.PublicKey import RSA
       from Crypto.Util import asn1

test/integ/descriptor/server_descriptor.py

@@ -86,7 +86,7 @@ def test_metrics_descriptor(self):
     self.assertEquals(expected_signing_key, desc.signing_key)
     self.assertEquals(expected_signature, desc.signature)
     self.assertEquals([], desc.get_unrecognized_lines())
-    self.assertEquals("2C7B27BEAB04B4E2459D89CA6D5CD1CC5F95A689", desc.digest().upper())
+    self.assertEquals("2C7B27BEAB04B4E2459D89CA6D5CD1CC5F95A689", desc.digest())
 
   def test_old_descriptor(self):
     """

test/mocking.py

@@ -788,6 +788,16 @@ def get_network_status_document_v3(attr = None, exclude = (), authorities = None
     return stem.descriptor.networkstatus.NetworkStatusDocumentV3(desc_content, validate = True)
 
 def sign_descriptor_content(desc_content):
+  """
+  Add a valid signature to the supplied descriptor string.
+  If the python-crypto library is available the function will generate a key
+  pair, and use it to sign the descriptor string. Any existing fingerprint,
+  signing-key or router-signature data will be overwritten.
+  If crypto is unavailable the code will return the unaltered descriptor
+  string.
+  :param string desc_content: the descriptor string to sign
+  :returns: a descriptor string, signed if crypto available, unaltered otherwise
+  """
 
   if not stem.prereq.is_crypto_available():
     return desc_content
@@ -839,9 +849,10 @@ def sign_descriptor_content(desc_content):
       ft_end = desc_content.find("\n", ft_start+1)
       desc_content = desc_content[:ft_start]+new_fp+desc_content[ft_end:]
 
-    # calculate the new digest for the descriptor
+    # create a temporary object to use to calculate the digest
     tempDesc = stem.descriptor.server_descriptor.RelayDescriptor(desc_content, validate=False)
-    new_digest_hex = tempDesc.digest()
+    # calculate the new digest for the descriptor
+    new_digest_hex = tempDesc.digest().lower()
     # remove the hex encoding
     new_digest = new_digest_hex.decode('hex')