Refactor signature code #75
Conversation
The VVM receiving wrapper added for DER support should have a conditional around the code for taking the binary data out of the XML-RPC object used in case DER data is transfered via XML-RPC, so that it doesn't break JSON processing if we're using JSON.
When checking a signature, make sure to only pass is_binary_data to tuf.keys.verify_signature if the data is binary - if we're using DER in other words.
that will deal with metadata encoding. These can be called from code that would previously have had to worry about ASN.1/DER/JSON/utf-8
Have sig.sign_signable use the new signing function in common so that it has a purpose.
instead of both DER and JSON when DER is requested.
uptane/common.py
Outdated
| @@ -27,6 +44,17 @@ def sign_signable(signable, keys_to_sign_with): | |||
| keys_to_sign_with: | |||
| A list whose elements must conform to tuf.formats.ANYKEY_SCHEMA. | |||
|
|
|||
| datatype: | |||
There was a problem hiding this comment.
What are the supported datatypes?
There was a problem hiding this comment.
'json' and 'der', as the comment says (:
At first, I added a tuf.conf value SUPPORTED_METADATA_FORMATS = ['json', 'der'], but then pulled that out.... Think I should?
There was a problem hiding this comment.
For the datatype argument?
The full text doesn't mention the supported datatypes.
datatype:
The type of data signable['signed'] represents. This is necessary because
if we're signing over ASN.1/DER, we need to convert to ASN.1/DER first,
and conversion to ASN.1/DER varies by type. This value doesn't matter if
signing is occuring over JSON.
uptane/common.py
Outdated
| 'json' or 'der'. Determines what the signature will be over. | ||
| Should generally be left to the default except when testing different | ||
| encodings or otherwise intentionally signing a different format. | ||
|
|
||
| Returns: | ||
|
|
||
| A signable object (tuf.formats.SIGNABLE_SCHEMA), but with the signatures |
There was a problem hiding this comment.
Is this correct? There is an empty return statement on line 100 (of this function). This function appears to modify the signable object, rather than return a signable object. We should make it clearer in the field of this fact, although it does somewhat vaguely state it.
There was a problem hiding this comment.
Nope, good catch: that was from a bad older design. Fixing docstring.
| # so we don't have to do this silly wrapping in an empty signable. | ||
| data = asn1_codec.convert_signed_metadata_to_der( | ||
| {'signed': data, 'signatures': []}, only_signed=True, datatype=datatype) | ||
| data = hashlib.sha256(data).digest() |
There was a problem hiding this comment.
In the future, we should consider making this a configurable option instead of fixed to sha256.
There was a problem hiding this comment.
Agreed. This particular hashing I see as a temporary measure, though: it remains here because some lower-level signing modules in TUF are signing hashes, and others aren't. (Incidentally, sha256 is hard-coded in some of those, too.) The other original motivation for this (avoiding some encoding issues) is no longer a concern.
Wherever the hashing ends up happening, it should be configurable.
There was a problem hiding this comment.
An attempt was made in the past to make hashing algorithms configurable. Although at the time it didn't seem doable, due to prepended sha512 digests and filename length restrictions on some systems, a later change to the specification allowed for configurable hashing algorithms in all of TUF. The change was that version numbers are now prepended to all metadata filenames, rather than digests.
After version numbers were added to filenames, Root and Snapshot were the only metadata required to have digests prepended, but they too have now been modified to only prepend version numbers. In addition, multiple hashing algorithms are supported for KEYIDs
Presently, I think the hashing algorithm for digest can be made configurable.
There was a problem hiding this comment.
We can make this change in Uptane in a separate pull request.
sign_over_metadata and verify_signature_over_metadata.
|
Comments addressed (along with some changes made to respond to your comments from the TUF PR which are equally applicable here). |
uptane/common.py
Outdated
| added to its 'signatures' list. | ||
| <Exceptions> | ||
| tuf.FormatError if the provided key is not the correct format or lacks a | ||
| private element, or if the signing key type is not the . |
There was a problem hiding this comment.
Text appears to be missing here.
|
I had one or two additional review comments. Other than those, this LGTM. |
and add some explanation for SUPPORTED_KEY_TYPES in asn1_codec
Separate low-level signing code from encoding and formatting code once and for all. (:
This makes the code more readable and less confusing, and will hopefully saving a good bit of encoding debugging time....
This TUF fork PR should be applied in tandem (so the build may fail, since it won't do that).