Move _get_keyid helper to Signer base class and use in implementations #557
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This method can be used prior to creating a Key instance, in order to generate a default keyid, which is a required Key argument. Since this currently mostly happens in the import_ methods of different Signer implementations, it feels right to implement this on the Signer base class. But other places would be okay too. Note that the computed keyids are different from ``securesystemslib.keys._get_keyid()``, which includes a legacy field when producing the hash. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
The import_ method of GCPSigner and HSMSigner used to assign a default keyid to the imported key, computed with the legacy _get_keyid function, which does overly complicated and intransparent mangling of the input data. This commit uses a new simple _get_keyid helper on the Signer base class. NOTE: keyid computation is not public API, and verification of existing signatures with existing keys is not affected. If this does disrupt existing users, who expect a certain value of a given key, I'd rather annoy them now, where the signer API is still experimental. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
jku
approved these changes
Mar 31, 2023
There was a problem hiding this comment.
LGTM.
I still hope we can move to free form keyids in the future -- but even in that case it probably makes sense to have a "default" that should just work for the cases where the application is not interested in doing the extra work of uniquely naming the keys.
Agreed. And I think the signer API already allows you to use free form keyids. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signer base class now provides a static
_get_keyidmethod which may be used in theimport_orgeneratemethods of Signer implementations.This PR also changes GCPSigner in HSMSigner to use the new method instead of the legacy
_get_keyid, which does overly complicated and intransparent mangling of the input data, and thus produces different hashes.Note that keyid computation is not public API, and verification of existing signatures with existing keys is not affected. If this does disrupt existing users, who expect a certain default keyid for a give key, I'd rather annoy them now, when the signer API is still experimental.