Skip to content
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

Merged
merged 11 commits into from
Jan 11, 2023
Merged

GCPSigner import #480

merged 11 commits into from
Jan 11, 2023

Conversation

jku
Copy link
Collaborator

@jku jku commented Dec 12, 2022

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:

  • 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

@jku
Copy link
Collaborator Author

jku commented Dec 12, 2022

@jku jku changed the title Gcp import GCPSigner import Dec 12, 2022
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
@jku jku marked this pull request as ready for review January 2, 2023 12:27
@jku
Copy link
Collaborator Author

jku commented Jan 2, 2023

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.
@jku
Copy link
Collaborator Author

jku commented Jan 11, 2023

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.

  • use the same method name import_()
  • same return values: priv key uri and public key
  • and finally, handle keyid creation internally

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:

  • It's true that callers should have the opportunity to set keyids, but I think they do have that: they can just set the keyid of the key to whatever they want after getting it from import_()
  • providing a reasonable default still seems nice (the implementation may not be pretty but now it's an implementation detail that we can change without API changing)

Neither signer has been released yet so API changes are fine.

Copy link
Member

@lukpueh lukpueh left a 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.

securesystemslib/signer/_gcp_signer.py Outdated Show resolved Hide resolved
securesystemslib/signer/_gcp_signer.py Outdated Show resolved Hide resolved
"rsa-pkcs1v15-sha512",
),
}
return keytypes_and_schemes[algorithm]
Copy link
Member

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?

Copy link
Collaborator Author

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.
@jku
Copy link
Collaborator Author

jku commented Jan 11, 2023

interesting, I broke the kms test. Investigating...

@lukpueh
Copy link
Member

lukpueh commented Jan 11, 2023

Test still failing. Could it be that you have to recalculate the keyid?

"23b650fcc538c0c6d423d0886448172fd51d81f43ef749851a0a84978cdcd8e0",

tests/check_kms_signers.py Outdated Show resolved Hide resolved
The more specific keytypes may get deprecated at some point.
@jku
Copy link
Collaborator Author

jku commented Jan 11, 2023

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

@lukpueh lukpueh merged commit 86b44c0 into secure-systems-lab:master Jan 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants