-
Notifications
You must be signed in to change notification settings - Fork 272
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support blake algorithms for file hashing #993
Support blake algorithms for file hashing #993
Conversation
Using securesystemslib.settings.HASH_ALGORITHMS is undersirable, because it binds tuf to an implementation detail of the underlying library. In this specific instance of file hashing algorithms it's even more undesirable because it's overloading the intended use of the setting which is "algorithm(s) [...] used to generate key IDs". Add a new setting tuf.settings.FILE_HASH_ALGORITHMS, with a default value of ['sha256', 'sha512'] (that matches the current value of securesystemslib.settings.HASH_ALGORITHMS), to be used for file hashing operations in tuf. Signed-off-by: Joshua Lock <[email protected]>
Timestamp.json includes a METAFILES entry for snapshot.json. METAFILES includes HASHES: "HASHES is the dictionary that specifies one or more hashes, including the cryptographic hash function. For example: { "sha256": HASH, ... }." We've been hard-coding this to a single sha256 hash, as that's the default algorithms argument of securesystemlib.util.get_file_details() -- this feels wrong. Change to using the new tuf.settings.FILE_HASH_ALGORITHMS setting. Signed-off-by: Joshua Lock <[email protected]>
(I closed PR #991 because I didn't want that branch name to live forever in the git history, sorry for the noise) Note: this change has been tested with the simple script in this GitHub gist: test_blake.py which requires the corresponding change in secure-systems-lab/securesystemslib#218 @lukpueh shared a list of places to check for use of hashing algorithms:
In order to unblock @woodruffw's PEP 458 implementation could we land this and create an issue to address client updater in a follow-on PR? |
@joshuagl: Have you already identified any issues in the client updater resulting from this PR? |
I have not. The only thing I've spotted so far is the related issue that I have been able to make a simple modification to |
@@ -102,6 +102,9 @@ | |||
# the securesystemslib external library. | |||
DEFAULT_HASH_ALGORITHM = 'sha256' | |||
|
|||
# The hashing algorithms used to compute file hashes | |||
FILE_HASH_ALGORITHMS = ['sha256', 'sha512'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't apply directly to this PR, but could someone on the TUF team comment on best practices re: configuring tuf.settings
?
After this PR, Warehouse will probably do something like this:
import tuf.settings
# ...
tuf.settings.FILE_HASH_ALGORITHMS = ['blake2b']
is this the intended configuration pattern? It feels slightly wrong to modify a third-party module global like this, but it doesn't look like any other interfaces for configuring TUF are exposed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it feels wrong, but I fear in some cases it is the only way to configure TUF. We should fix this at some point.
This looks good as a short term fix, but in the long term I would prefer changing the requirements for keyid calculation as discussed here: #848. As currently implemented, the client has to hard code the same hash algorithm for keyids as the metadata which does not leave much room for flexibility, especially because the keyid does not need to be globally unique. |
Thanks @mnm678. This is very much intended as a short-term fix to unblock PEP 458 implementation. @lukpueh mentioned #848 and some other issues around cohesion between securesystemslib and tuf when we discussed this quick fix. I'm absolutely interested in contributing to discussions and solutions towards long-term fixes too. |
I agree with both of you. I think a good long-term fix might be specifying the targets hashing and keyid calculation (not necessarily hashing) algorithms in the |
I think an even simpler long term solution would be to keep the keyid calculation independent of the client. The client only needs to know that keyids are unique within a metadata file, not how they are determined. But we can have a meeting/discussion about this elsewhere. |
Quick question, @joshuagl, regarding:
Are you referring to the updater ... ? (btw. the I couldn't find any repo tooling that helps prefixing target files with their hash digests. 🤷♂ |
Never mind, just found it: |
Regarding
Rather looks like python-tuf does not include hashes (only version) in snapshot metadata. See repository_lib.generate_snapshot_metadata. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Seems like this could be worthy of an issue so that we can remember to add to python-tuf? |
Fixes issue #: N/A
Description of the changes being introduced by the pull request:
Please verify and check that the pull request fulfills the following
requirements: