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

Add support for softkms and cloudkms uris on requests #240

Merged
merged 7 commits into from
May 25, 2023
Merged

Conversation

maraino
Copy link
Contributor

@maraino maraino commented May 18, 2023

Description

This PR allows referring a key using a URIs like "sofkms:/path/to/key.pem" or "cloudkms:projects/p/locations/l/keyRings/k/cryptoKeys/c/cryptoKeyVersions/1".

TBD:

  • Should we use a URI format with key=value pairs? Should we support both?
    • "sofkms:path=/path/to/key.pem"
    • "cloudkms:resource=projects/p/locations/l/keyRings/k/cryptoKeys/c/cryptoKeyVersions/1".
  • Should we return the URI in the responses? For both KMSs?

@hslatman What do you think?

@maraino maraino requested review from hslatman and removed request for hslatman May 18, 2023 23:46
@maraino maraino changed the title Add support for softkms uris on requests Add support for softkms and cloudkms uris on requests May 18, 2023
kms/softkms/softkms.go Outdated Show resolved Hide resolved
@hslatman
Copy link
Member

In terms of options: I think it's nice if softkms:/path/to/file works. It's quick and simple, and looks most like our current paths. I think file= could be optional. And if there are other fields, they must have the key=value format. So if there's only one property, for softkms it'll be the filepath.

I would say that returning the scheme in the URI in the responses makes sense. That way it's self-contained. If the scheme were to be excluded from the response, it could happen that the value from a response would be used with a different KMS, I guess? With the full URI returned, it could be passed on to the next operation more easily.

For the TPM KMS I've implemented returning the full URI too. I might add support for not having to specify the name=<value> to address the key, and for tpmkms:<value> to be enough to identify the key. Will push something soon.

With the above changes, I think we can still support the current kms configuration separately from the URIs? Or is your intention to replace that?

@maraino
Copy link
Contributor Author

maraino commented May 19, 2023

With the above changes, I think we can still support the current kms configuration separately from the URIs? Or is your intention to replace that?

I want to support both. I think I will return URIs in cloudkms too (all others do that) but probably leave the softkms as the filename. Then we can manage that case in generic kms. Sounds that ok?

@hslatman
Copy link
Member

Sounds good!

This PR allows referring a key using a uri like "sofkms:path/to/key"
This commit allows to pass urls like cloudkms:projects/foo/... on
cloudkms requests.
This commit allow urls with the resource attribute in cloudkms and the
path attribute in softkms. In cloudkms the create key method will return
the key names as cloudkms:projects/foo/locations/...
This commit adds supports for any crypto.Signer in softkms.GetPublicKey
as long as the key is not encrypted. It also adds support for x25519
keys.
@maraino maraino marked this pull request as ready for review May 24, 2023 01:12
@maraino
Copy link
Contributor Author

maraino commented May 24, 2023

@hslatman This should be ready now.

@maraino maraino requested a review from hslatman May 24, 2023 01:13
@@ -190,9 +190,12 @@ func (k *CloudKMS) CreateKey(req *apiv1.CreateKeyRequest) (*apiv1.CreateKeyRespo

var crytoKeyName string

// resource is the plain Gloud KMS resource name
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// resource is the plain Gloud KMS resource name
// resource is the plain Google Cloud KMS resource name

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed with ecd8cc5

// - projects/id/locations/global/keyRings/ring/cryptoKeys/root-key/cryptoKeyVersions/1
// - cloudkms:resource=projects/id/locations/global/keyRings/ring/cryptoKeys/root-key/cryptoKeyVersions/1
// - cloudkms:projects/id/locations/global/keyRings/ring/cryptoKeys/root-key/cryptoKeyVersions/1
func resourceName(name string) string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a thought I had: when other people use our kms package, they may want to have this same logic available to them when handling certain requests/responses. We could expose these helpers in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main reason for this method is to support backward compatibility. The CreateKey method now returns the URI that you should use, in this case, cloudkms:projects/id/...

@maraino maraino requested a review from hslatman May 24, 2023 19:25
@maraino maraino merged commit 80680d7 into master May 25, 2023
@maraino maraino deleted the softkms-uris branch May 25, 2023 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants