Refactor format_metadata_to_key #227

MVrachev · 2020-04-03T12:10:47Z

Fixes issue #:

Description of the changes being introduced by the pull request:

Refactor format_metadata_to_key so that it won't depend
on settings.HASH_ALGORITHMS.
There are couple of code snippets in tuf where in order to provide
a custom list of hash algorithms, tuf is temporary changing
settings.HASH_ALGORITHMS and then returning it to its formal state.

This could lead to problems in future and one should
be able to control hash algorithms used for generating keyids in f
ormat_metadata_to_key() without having to change
package level settings in securesystemslib.settings.

Please verify and check that the pull request fulfils the following
requirements:

The code follows the Code Style Guidelines
Tests have been added for the bug fix or new feature
Docs have been added for the bug fix or new feature

coveralls · 2020-04-03T12:13:31Z

Coverage increased (+0.001%) to 98.944% when pulling 5908bfb on MVrachev:refactor-format-metadata-to-key into df57705 on secure-systems-lab:master.

This pr enables tuf to use changes made in secure-systems-lab/securesystemslib#227 Don't merge before the above pr is merged! Because of issue secure-systems-lab/securesystemslib#227 I had to checkout to the last commit which doesn't had that issue and make the current changes based on it. Signed-off-by: Martin Vrachev <[email protected]>

joshuagl

Thanks for the patch @MVrachev. I've made some minor suggestions on the code.

It's worth noting that this PR conflicts with #225

securesystemslib/interface.py

securesystemslib/keys.py

lukpueh

Thanks for the patch, @MVrachev! This will make the interface a lot cleaner. Please resolve the git conflict and fix the test (see inline).

Also, be aware that default values in functions are only evaluated once when the function definition is evaluated. This has bitten me before (see #202). As a consequence, constructs like these won't be possible anymore.

# The default arg is evaluated together with the function definition on first import:
# format_metadata_to_key(key_metadata, hash_algorithms=['sha256', 'sha512'])
import securesystemslib.keys
import securesystemslib.settings

securesystemslib.settings.HASH_ALGORITHMS = ["blake2"]

format_metadata_to_key(key_metadata)
# the default for hash_algorithms still is ['sha256', 'sha512'] and not ["blake2"]

We shouldn't use constructs like these anyway and you already have a fix for TUF in the queue (theupdateframework/python-tuf#1016, thanks!), but given that this interface was used like this before, we maybe want to mention that backwards incompatibility in the CHANGELOG for the next release (mostly a note to myself).

tests/test_keys.py

securesystemslib/keys.py

joshuagl

Thanks for making the suggested changes, Martin. Could you address the remaining minor nit?

securesystemslib/keys.py

joshuagl · 2020-05-15T09:56:29Z

This change looks good, thank you.

I would like to hold off on merging it until we have a solid release out to support tuf changes related to the Warehouse work for pep 458, because I anticipate that this will be the first of several API changes as we work to remove the settings module (re: #219) and I would like us to include those in one release rather than spread them over multiple releases.

We just had the 0.15.0 release with abstract files & directories support. Next we'll want a release with the HSM support in #229. Then we'll have all of the features required of PEP 458 and can start landing changes like this towards #219.

It would be really nice if we tested format_metadata_to_key with non-default parameters too, do you think you could work on a patch for that in this PR @MVrachev ?

joshuagl · 2020-05-15T10:13:26Z

Also, be aware that default values in functions are only evaluated once when the function definition is evaluated. This has bitten me before (see #202). As a consequence, constructs like these won't be possible anymore.
# The default arg is evaluated together with the function definition on first import:
# format_metadata_to_key(key_metadata, hash_algorithms=['sha256', 'sha512'])
import securesystemslib.keys
import securesystemslib.settings

securesystemslib.settings.HASH_ALGORITHMS = ["blake2"]

format_metadata_to_key(key_metadata)
# the default for hash_algorithms still is ['sha256', 'sha512'] and not ["blake2"]

I've been wondering what the right pattern for this is. Maybe something like:

def format_metadata_to_key(key_metadata, default_keyid=None,
  keyid_hash_algorithms=None):
  # docstrings and stuff ...
  if keyid_hash_algorithms is None:
    keyid_hash_algorithms = securesystemslib.settings.HASH_ALGORITHMS
  # ... rest of the function

That way we don't bind the default value at function evaluation and we allow users to continue with the current pattern of assigning securesystemslib.settings.HASH_ALGORITHMS, making this change less of an API break. We're also a little bit cleaner in our intent to deprecate the settings module as we don't encode it in the interface (by using a variable from the module as a default value).

trishankatdatadog · 2020-05-15T20:49:12Z

I've been wondering what the right pattern for this is. Maybe something like:

Good idea, I like it!

MVrachev · 2020-05-18T10:42:38Z

I rebased and addressed Joshua's comments. I changed the following:

Renamed has_algorithms to keyid_hash_algorithms
Use the approach proposed from Joshua Lock about handling the default value of keyid_hash_algorithms
Added a new commit adding unit tests using nondefault hash algorithms.

joshuagl

Thanks @MVrachev. This is looking good. I made some minor style and layout suggestions for the tests to keep them readable and maintainable, would you be able to address?

tests/test_keys.py

joshuagl · 2020-05-18T21:46:19Z

tests/test_keys.py

+        FORMAT_ERROR_MSG)
+
+    self.assertEqual(None,
+        securesystemslib.formats.KEY_SCHEMA.check_match(rsakey_dict_from_meta),


Same question on matches() vs check_match() here.

tests/test_keys.py

joshuagl

This looks good, thanks @MVrachev. One minor question.

joshuagl · 2020-06-02T11:11:53Z

tests/test_keys.py

-    del self.rsakey_dict['keyid']
+    # Copying self.rsakey_dict so that rsakey_dict remains
+    # unchanged during and after this test execution.
+    test_rsakey_dict = copy.deepcopy(self.rsakey_dict)


Nit: I think copy would suffice here?

Yes, you are right.

I overdo it a little to make sure the right values were copied but forgot to try with copy.copy.

I made the change and amended.

lukpueh

Thanks, @MVrachev! Please consider addressing my two minor style comments. Otherwise this looks good to me.

lukpueh · 2020-06-03T13:01:27Z

securesystemslib/keys.py

@@ -474,7 +474,8 @@ def format_keyval_to_metadata(keytype, scheme, key_value, private=False):



-def format_metadata_to_key(key_metadata, default_keyid=None):
+def format_metadata_to_key(key_metadata, default_keyid=None,
+  keyid_hash_algorithms=None):


Minor nit: Please use double-indentation (4 spaces) for line continuation.

lukpueh · 2020-06-03T13:02:45Z

tests/test_keys.py

+
+    # Call format_metadata_to_key by using custom value for keyid_hash_algorithms
+    rsakey_dict_from_meta_custom, junk = KEYS.format_metadata_to_key(test_rsakey_dict,
+      keyid_hash_algorithms=['sha384'])


See comment above about line continuation.

MVrachev · 2020-06-08T15:43:58Z

Fixed the indentation.

Refactor format_metadata_to_key so that it won't depend on settings.HASH_ALGORITHMS. There are couple of code snippets in tuf where in order to provide a custom list of hash algorithms, tuf is temporary changing settings.HASH_ALGORITHMS and then returning it to its formal state. This could lead to problems in future and one should be able to control hash algorithms used for generating keyids in f ormat_metadata_to_key() without having to change package level settings in securesystemslib.settings. Closes: secure-systems-lab#220 Signed-off-by: Martin Vrachev <[email protected]>

Signed-off-by: Martin Vrachev <[email protected]>

MVrachev · 2020-06-26T16:40:14Z

Thanks for merging that pr @joshuagl!
I will update the corresponding pr theupdateframework/python-tuf#1016 in TUF next week.

In commit b7a15fdee7dee899c098b01fe64d604635b2b132 or pr secure-systems-lab/securesystemslib#227 in securesystemslib I change the function arguments of the format_metadata_to_key function in securesystemslib/keys.py to add the opportunity to use custom keyid hash algorithms without chainging the securesystemslib.settings.HASH_ALGORITHMS variable. With this commit, I make use of the above changes in tuf. Signed-off-by: Martin Vrachev <[email protected]>

MVrachev mentioned this pull request Apr 3, 2020

Refactor to use changed format metadata to key theupdateframework/python-tuf#1016

Merged

3 tasks

joshuagl reviewed Apr 3, 2020

View reviewed changes

securesystemslib/interface.py Outdated Show resolved Hide resolved

securesystemslib/interface.py Outdated Show resolved Hide resolved

securesystemslib/keys.py Outdated Show resolved Hide resolved

MVrachev force-pushed the refactor-format-metadata-to-key branch from 4dad937 to e28d198 Compare April 6, 2020 10:28

joshuagl reviewed Apr 6, 2020

View reviewed changes

securesystemslib/keys.py Outdated Show resolved Hide resolved

lukpueh requested changes Apr 10, 2020

View reviewed changes

tests/test_keys.py Outdated Show resolved Hide resolved

MVrachev force-pushed the refactor-format-metadata-to-key branch from c4ca76a to 6796cb9 Compare April 14, 2020 14:19

joshuagl reviewed Apr 16, 2020

View reviewed changes

securesystemslib/keys.py Outdated Show resolved Hide resolved

MVrachev force-pushed the refactor-format-metadata-to-key branch from 6796cb9 to 5b4d843 Compare April 16, 2020 11:23

joshuagl requested changes Apr 27, 2020

View reviewed changes

securesystemslib/keys.py Outdated Show resolved Hide resolved

MVrachev force-pushed the refactor-format-metadata-to-key branch from 5b4d843 to 19fb72a Compare April 27, 2020 14:16

MVrachev force-pushed the refactor-format-metadata-to-key branch 2 times, most recently from a8d18d8 to 110c168 Compare May 18, 2020 10:41

joshuagl reviewed May 18, 2020

View reviewed changes

MVrachev force-pushed the refactor-format-metadata-to-key branch from 110c168 to d64b25c Compare May 21, 2020 16:55

joshuagl approved these changes Jun 2, 2020

View reviewed changes

MVrachev force-pushed the refactor-format-metadata-to-key branch from d64b25c to a3006e0 Compare June 2, 2020 17:39

lukpueh approved these changes Jun 3, 2020

View reviewed changes

MVrachev force-pushed the refactor-format-metadata-to-key branch 2 times, most recently from f9d3f6a to 9aab0d1 Compare June 8, 2020 15:43

MVrachev added 2 commits June 25, 2020 13:21

Add a test case for non default hash algorithms

5908bfb

Signed-off-by: Martin Vrachev <[email protected]>

MVrachev force-pushed the refactor-format-metadata-to-key branch from 9aab0d1 to 5908bfb Compare June 25, 2020 10:23

joshuagl merged commit 7531747 into secure-systems-lab:master Jun 25, 2020

MVrachev deleted the refactor-format-metadata-to-key branch June 30, 2020 16:07

lukpueh mentioned this pull request Aug 20, 2020

PoC(tap13): User Selection of the Top-Level Target Files Through Mapping Metadata theupdateframework/python-tuf#1103

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor format_metadata_to_key #227

Refactor format_metadata_to_key #227

MVrachev commented Apr 3, 2020 •

edited

Loading

coveralls commented Apr 3, 2020 •

edited

Loading

joshuagl left a comment

lukpueh left a comment

joshuagl left a comment

joshuagl commented May 15, 2020

joshuagl commented May 15, 2020

trishankatdatadog commented May 15, 2020

MVrachev commented May 18, 2020 •

edited

Loading

joshuagl left a comment

joshuagl May 18, 2020

joshuagl left a comment

joshuagl Jun 2, 2020

MVrachev Jun 2, 2020

lukpueh left a comment

lukpueh Jun 3, 2020

lukpueh Jun 3, 2020

MVrachev commented Jun 8, 2020

MVrachev commented Jun 26, 2020 •

edited

Loading

Refactor format_metadata_to_key #227

Refactor format_metadata_to_key #227

Conversation

MVrachev commented Apr 3, 2020 • edited Loading

coveralls commented Apr 3, 2020 • edited Loading

joshuagl left a comment

Choose a reason for hiding this comment

lukpueh left a comment

Choose a reason for hiding this comment

joshuagl left a comment

Choose a reason for hiding this comment

joshuagl commented May 15, 2020

joshuagl commented May 15, 2020

trishankatdatadog commented May 15, 2020

MVrachev commented May 18, 2020 • edited Loading

joshuagl left a comment

Choose a reason for hiding this comment

joshuagl May 18, 2020

Choose a reason for hiding this comment

joshuagl left a comment

Choose a reason for hiding this comment

joshuagl Jun 2, 2020

Choose a reason for hiding this comment

MVrachev Jun 2, 2020

Choose a reason for hiding this comment

lukpueh left a comment

Choose a reason for hiding this comment

lukpueh Jun 3, 2020

Choose a reason for hiding this comment

lukpueh Jun 3, 2020

Choose a reason for hiding this comment

MVrachev commented Jun 8, 2020

MVrachev commented Jun 26, 2020 • edited Loading

MVrachev commented Apr 3, 2020 •

edited

Loading

coveralls commented Apr 3, 2020 •

edited

Loading

MVrachev commented May 18, 2020 •

edited

Loading

MVrachev commented Jun 26, 2020 •

edited

Loading