Refactor signature code #5

awwad · 2017-04-14T19:50:31Z

Separate low-level signing code from encoding and formatting code once and for all. (:

This makes the code more readable and less confusing, and also makes life a lot easier for Uptane upstream.

This Uptane PR should be applied in tandem.

I was originally going to wait until after the Great Merge of the TUF Forks for this, but after more lost time debugging, I got pretty sick of all of the optional arguments I had to add to tuf.keys, and the edge cases requiring coordination.... It was time to do this.

tuf.keys.create_signature and tuf.keys.verify_signature should have no awareness of the encodings of the data fed to them. They should receive bytes, sign those or validate the signatures on those, and return signatures or validity booleans. They shouldn't need to deal with ASN.1 or DER or JSON or even encoding things as utf-8. Future commits will capture those needs in higher level code.

Allow code to expect tuf.keys.create_signature to take bytes without lots of special flags.

that will deal with metadata encoding to sig.py. These can be called from code that would previously have had to worry about ASN.1/DER/JSON/utf-8.

repository_lib functions and sig.get_signature_status can now forget all about encoding and format and use the new functions in sig.

vladimir-v-diaz · 2017-04-17T18:43:13Z

tuf/asn1_codec.py

@@ -278,13 +278,7 @@ def convert_signed_metadata_to_der(
    # Now sign the metadata. (This signs a cryptographic hash of the metadata.)


I like how you refer to a specific function when discussing what is returned and given as input (see line 281). In contrast, the comment that starts on line 278 uses words like this, which might lead to confusion.

vladimir-v-diaz · 2017-04-17T18:50:05Z

tuf/keys.py

-      utf-8 encode the data, because it cannot necessarily be encoded as utf-8
-      and in any case is already encoded.
-
+      This should be bytes(). If it is a string, it should already be encoded


What happens if the string isn't in bytes? What error should the caller expect?

At the moment, in Python2, it works (is encoded automatically), and in Python3, lower-level crypto libraries (e.g. PyNaCl) are likely to choke on it, which results in an error reraised as a tuf.CryptoError (e.g. An "ed25519" signature could not be created with PyNaCl.initializer for ctype 'unsigned char *' must be a bytes or list or tuple, not str)

I want to produce a reliable error message in both cases, not allow it to work in one Python version and not another. That can result in mistakes or accidental incompatibilities in code using tuf.keys.

I'm not sure how best to do that, though. It's unpythonic to do this:
if python2 and isinstance(data, unicode) or python3 and not isinstance(data, bytes)

After meeting in-person to discuss this issue, we settled on:

tuf.formats.DATA_SCHEMA.check_match(data)

vladimir-v-diaz · 2017-04-17T18:54:52Z

tuf/sig.py

+    key_dict, data,
+    metadata_format=tuf.conf.METADATA_FORMAT):
+  """
+  Higher level function that wraps tuf.keys.create_signature, and works


Where are the labels? Return type, exceptions raises, etc.

We should also keep up the habit of updating the units tests and maintaining code coverage. The pull request to upstream will fail if code coverage is less than a certain percent.

I agree in general. Fixing testing on this fork is a task that I do not think is worth the energy right now, as it would be time-consuming and require re-working in the merge anyway. (I could be wrong - lmk if you disagree.)

As for the labels, the docstring directs one to the lower level create_signature docstring... but you're entirely right; this is inadequate. I'll add proper documentation.

create_signature and verify_signature only take binary data as their 'data' argument, so this commit checks that argument for the correct format.

Non-binary data must raise tuf.FormatError when provided to create_signature or verify_signature. Also fixes test_keys tests for new expectations: binary only.

sign_over_metadata and verify_signature_over_metadata

and have testing use the new sign_over_metadata function. (Testing will already use the other new function, verify_signature_over_metadata, because it is called indirectly by tuf.sig.verify.) tuf.keys only deals with binary data now.

awwad · 2017-04-18T20:18:05Z

I think that handles all your comments. I also fixed test_keys and test_sig to work with the new tuf.keys and tuf.sig code, since it was easy enough and provided a bit of reassurance. I also expanded testing slightly to cover binary data expectations in tuf.keys.

vladimir-v-diaz · 2017-04-18T20:40:24Z

tests/test_keys.py

+    # Ensure that tuf.FormatError is raised if non-binary data is passed to
+    # verify_signature:
+    self.assertRaises(tuf.FormatError, KEYS.verify_signature, self.rsakey_dict,
+        rsa_signature, u'unicode_string')


It's helpful to incorporate these edits to the unit tests, will be easier when porting to TUF.

awwad · 2017-04-24T14:27:17Z

Comments addressed, I think. Could you take a final look?

vladimir-v-diaz · 2017-04-24T15:36:24Z

tuf/sig.py

@@ -485,19 +485,6 @@ def verify_signature_over_metadata(

    See tuf.keys.verify_signature for lower level details.

-    >>> data = 'The quick brown fox jumps over the lazy dog'


If it's easy and doable, we should include doctests.

vladimir-v-diaz · 2017-04-24T15:36:42Z

Okay, I'm done reviewing this PR.

test_repository_lib fix: Some time ago in PR #5 #5 I changed signature code such that tuf.keys.create_signature and verify_signature accept already-encoded bytes instead of assuming what the encodings should look like, and moved encoding up a bit in the stack (often to tuf.sig). This test wasn't updated at that point, and now it is being updated to deal with this (by encoding first, then calling the tuf.keys.create_signature function). test_mix_and_match_attack fix: A fix to the way that role files are retained if validation fails (possibly from #13 ) broke this test, I think, though I'm not certain. In any event, this demonstrates passable behavior for now, with the role info loaded correctly. These tests in the TUF fork are not regarded as critical, given that testing occurs at the Uptane level and Uptane will be migrating to the main TUF repository when possible; however, these fixes may be helpful in the interim. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

awwad added 5 commits April 14, 2017 14:58

Restore Python3 JSON support (in tandem with Uptane changes)

6eeb452

Refactor signature-related code, part 2

f709bc1

Allow code to expect tuf.keys.create_signature to take bytes without lots of special flags.

Refactor sig-related part3: Add high-level signature functions

a099872

that will deal with metadata encoding to sig.py. These can be called from code that would previously have had to worry about ASN.1/DER/JSON/utf-8.

Refactor sig-related part4: Use the new functions

6e4a2a6

repository_lib functions and sig.get_signature_status can now forget all about encoding and format and use the new functions in sig.

awwad requested a review from vladimir-v-diaz April 14, 2017 19:50

This was referenced Apr 14, 2017

Refactor signature code uptane/obsolete-reference-implementation#75

Merged

Descriptive json error #6

Merged

Sane asn1 property limits #7

Closed

vladimir-v-diaz requested changes Apr 17, 2017

View reviewed changes

awwad added 4 commits April 18, 2017 15:51

Check for binary data in signature-related call arguments

0869436

create_signature and verify_signature only take binary data as their 'data' argument, so this commit checks that argument for the correct format.

Fix and add to test_keys, ensuring right error raised if bad data

c0ac9a9

Non-binary data must raise tuf.FormatError when provided to create_signature or verify_signature. Also fixes test_keys tests for new expectations: binary only.

DOC: Improve docstrings for new functions in tuf.sig

573a6ae

sign_over_metadata and verify_signature_over_metadata

Fix testing for tuf.sig after tuf.keys changes

34b6dcc

and have testing use the new sign_over_metadata function. (Testing will already use the other new function, verify_signature_over_metadata, because it is called indirectly by tuf.sig.verify.) tuf.keys only deals with binary data now.

vladimir-v-diaz reviewed Apr 18, 2017

View reviewed changes

awwad added 2 commits April 18, 2017 16:42

DOC: Remove accidental example of signature creation

be2b8e5

DOC: Clarify some signature-related comments

2805840

vladimir-v-diaz reviewed Apr 24, 2017

View reviewed changes

awwad merged commit 36dbb7b into develop Apr 24, 2017

awwad mentioned this pull request Jul 24, 2018

Update two tests to match changes made to Uptane's TUF fork #16

Merged

awwad deleted the refactor_signature_code branch March 4, 2019 19:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor signature code #5

Refactor signature code #5

awwad commented Apr 14, 2017 •

edited

Loading

vladimir-v-diaz Apr 17, 2017

awwad Apr 18, 2017

vladimir-v-diaz Apr 17, 2017

awwad Apr 18, 2017 •

edited

Loading

vladimir-v-diaz Apr 18, 2017

vladimir-v-diaz Apr 17, 2017

vladimir-v-diaz Apr 17, 2017

awwad Apr 18, 2017

awwad commented Apr 18, 2017

vladimir-v-diaz Apr 18, 2017

awwad commented Apr 24, 2017

vladimir-v-diaz Apr 24, 2017

vladimir-v-diaz commented Apr 24, 2017

		@@ -278,13 +278,7 @@ def convert_signed_metadata_to_der(
		# Now sign the metadata. (This signs a cryptographic hash of the metadata.)

		@@ -485,19 +485,6 @@ def verify_signature_over_metadata(

		See tuf.keys.verify_signature for lower level details.

		>>> data = 'The quick brown fox jumps over the lazy dog'

Refactor signature code #5

Refactor signature code #5

Conversation

awwad commented Apr 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awwad Apr 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awwad commented Apr 18, 2017

Choose a reason for hiding this comment

awwad commented Apr 24, 2017

Choose a reason for hiding this comment

vladimir-v-diaz commented Apr 24, 2017

awwad commented Apr 14, 2017 •

edited

Loading

awwad Apr 18, 2017 •

edited

Loading