-
Notifications
You must be signed in to change notification settings - Fork 50
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
GCPSigner import #480
GCPSigner import #480
Conversation
This is for setting up the signing support for this key: import_() should be called once per key only, and the public key should be stored.
This is useful on initial signer setup: After import_(), the application will want to store in application configuration: * private key uri * the public key keyid
Get them to match the format produced from the GCP key content
Don't use the google-cloud-kms module API during import: Build the lookup table for keytype/scheme inside a method.
Instead of being a constructor, import_() now returns the private key URI and the public Key. This makes sense as * it's still trivial to construct the Signer if needed * In many cases we don't actually use the Signer at import time * This works around the problem that a Signer instance might need a SecretsHandler * This setup likely works for key generation as well
The actual keys are ecdsa so we need to depend on cryptography
Marking ready for review |
There is no signature to conform to here but since the methods do work in a similar way, stanrdaize as much as possible: * same name * same return value Returning the private key uri is not super useful for HSM at this point (as the URI is a static string) but will be if e.g. slot is made user configurable.
Intent is to make GCP and HSM imports as similar as possible.
I'll add a couple of commits that try to "standardize" GCP and HSM "key creation / import": these change HSMSigner mostly but I think it makes some sense as part of this PR: the goal is to keep the offered Signer APIs coherent, even if there is no specific signature that the signers are implementing.
The last one, keyid change, is a separate commit so it can be dropped if you don't like it. My reasons for including it:
Neither signer has been released yet so API changes are fine. |
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. Please comment on priv_key_uri
signer attribute and I'll approve. Other inline comments are non-blockers.
"rsa-pkcs1v15-sha512", | ||
), | ||
} | ||
return keytypes_and_schemes[algorithm] |
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.
Is it worth creating a ticket to test these all schemes?
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 suppose that would be nice -- it will require creating all of these keys in the KMS though
private key uri is now available as return value from import(): it is not needed as attribute.
interesting, I broke the kms test. Investigating... |
Test still failing. Could it be that you have to recalculate the keyid?
|
The more specific keytypes may get deprecated at some point.
Okay: the test was failing as it should since I changed the key content. I fixed the test content to match as suggested, test now passes: https://github.com/jku/securesystemslib/actions/runs/3894855218/jobs/6649416664 |
Implement import (loading the whole key, including public key, from the KMS) for GCPSigner.
This is the implementation of #466 for this specific Signer -- in my opinion this can be merged: if we decide to change something in #466 that affects this Signer we can do that later.
At this point GCPSigner is fully usable: I don't think it needs anything more.
Please verify and check that the pull request fulfils the following requirements: