Feature Request: Support for KMS KeyRings / CryptoKeys

[mrparkers]

Hi everyone,

I was looking into using Terraform to create my next GCP project and I noticed that there are no resources for Google's KMS, which I use in most of my projects.

I was wondering if there was already a plan to add support for these resources. If not, I'd like to take a stab at submitting a PR to implement them (if there are no objections).

Relevant links:

• KMS REST reference
• cloudkms Go package reference

[mayakacz]

Hi,
I'm the PM on Cloud KMS. That would be a great addition. If you have any implementation questions please let us know!

[danawillow]

Hey @mrparkers, my team was planning on taking this on in Q4 but if you'd like to do it go for it! Feel free to reach out if you have any questions, and keep me updated on your progress 😃

[mrparkers]

Hi @mayakacz and @danawillow, thanks for the replies!

I did have one question about the implementation, and I wasn't able to find an example in the codebase that answered my question. Since it doesn't look like you can delete a KeyRing or a CryptoKey using the KMS API, what is the expected behavior for the Destroy function for these resources? My guess is that I would just throw an error that explains that the resource cannot be destroyed, but I wasn't able to find an example to affirm this.

Thanks!

[danawillow]

Oh, that's a good question. I don't like the idea of throwing an error since that means you can never do a terraform destroy on your full configuration. I think we could just remove it from state on destroy (i.e. do nothing), and make sure that if you try to create something that already exists it doesn't throw an error. I wonder what happens if you just don't implement a delete function at all?
I don't know whether that would be allowed, but you could try! And we should also be really clear in the docs that this is different behavior from other resources because KeyRings and CryptoKeys can never be deleted.

Just to confirm @mayakacz, is this intentional based on how KMS works that these can't be deleted or is it just that it can't be done via the API (looks like gcloud doesn't have delete functionality either)

[mrparkers]

Okay, I'll go with your suggestion of removing it from state and allowing it to be "re-created" to add it back.

Speaking of the Destroy function, any advice for how I would go about making an acceptance test that doesn't pollute my test project with a bunch of undeletable KeyRings and CryptoKeys? Would it make sense to spin up a new project for the resources to be created in so it could be destroyed when the test is finished?

[mayakacz]

Hey @danawillow, yes that's correct, KeyRings and CryptoKeys can never be deleted - allowing for complete history of how these resources were used.
terraform destroy should destroy all the CryptoKeyVersions for any created resources. See docs on destroying key versions.

@mrparkers Yes that's a good idea. We recommend you run Cloud KMS in its own project, so a good way of avoiding this would indeed be to spin up a new project with Cloud KMS resources, then destroy that project when done.

[mrparkers]

@danawillow @mayakacz I created a PR in my fork for creating a KeyRing resource with a basic acceptance test. Please take a look when you have time and let me know if you have any suggestions: mrparkers#1

I'm going to work on another acceptance test that ensures you can re-create KeyRings with the same name after removing them from state.

[danawillow]

Instead of a PR to your fork, can you send it as a PR to this repo?

[mrparkers]

@danawillow Sure thing. I opened #518

[mrparkers]

I'm going to re-use this issue to talk about CryptoKey resources, since the title includes them as well. As with KeyRing, I'd like to make a PR to add support for this resource, unless someone else was planning on doing this already.

At the moment, I'm struggling a little bit with what I think this resource should look like when it's managed by terraform. Creating the CryptoKey should be simple enough, since all the user would need to specify would be its name. If I only cared about this, the resource could look something like this:

resource "google_kms_key_ring" "key_ring" {
  name     = "key-ring"
  location = "us-central1"
}

resource "google_kms_crypto_key" "crypto_key" {
  key_ring = "${google_kms_key_ring.key_ring.id}"
  name     = "crypto-key"
}

A CryptoKey is similar to the KeyRing resource in the sense that a CryptoKey also cannot be deleted, so I'd probably take the same approach as I did with the KeyRing resource.

However, it starts to get confusing (at least for me) when you factor in the ability to "rotate" the CryptoKey by creating a new CryptoKeyVersion. This can be done manually, by creating a new CryptoKeyVersion through the API, or it can be done automatically, by specifying a nextRotationTime on the CryptoKey, which tells GCP to create a new CryptoKeyVersion and set it as the default when the nextRotationTime comes around. It will also update the nextRotationTime if rotationPeriod was specified when creating the CryptoKey.

So yeah, I'm really not sure how this functionality can be captured in a terraform resource. At first, I thought that the CryptoKeyVersion could be its own terraform resource, but in my mind, that only really works if you're doing manual rotations. I have no idea how automatic rotation would work with this, since the user would never be creating their own CryptoKeyVersion resource in terraform.

A CryptoKeyVersion also contains state, which determines whether or not the version is enabled, disabled, pending destruction, or destroyed. This could easily be managed if the user were to create a CryptoKeyVersion resource in terraform, but I can't wrap my head around how it would be managed with a CryptoKeyVersion that was created via automatic rotation.

@danawillow, @mayakacz Any advice here? If we come to the consensus that a CryptoKeyVersion should get its own terraform resource, then most of these questions could potentially be shelved until I start working on that. I could also start working on CryptoKey support with manual rotation only, and add automatic rotation in a future PR. Let me know what you guys think.

Thanks!

Some helpful reading material:

1. Object hierarchy documentation
2. CryptoKey REST API documentation
3. CryptoKeyVersion REST API documentation

[tragiclifestories]

@mrparkers I've been poking at this too. I think the RotationPeriod param should definitely be supported, but is there a use case for having resources representing key versions? That seems a little odd to me - it's seems more that this is a matter of the key's internal state, and indeed Google's KMS is designed so callers can be indifferent to which version of a key a file was encrypted with.

I'd be interested to see if anyone has a use case for it. I certainly (or would like ... ) the key rotation params as the ease of key rotation is one of the big selling points for the service for me. I'd started work on a PR before I saw your comment but will pause if you're working on it.

[mrparkers]

is there a use case for having resources representing key versions? That seems a little odd to me - it's seems more that this is a matter of the key's internal state

Yeah, that's exactly what I was thinking. The only reason I can see a CryptoKeyVersion resource being useful is if you wanted to disable, enable, or destroy a version. Also, if there was a CryptoKeyVersion resource, then manual rotation would be as easy as creating a new resource, and removing the old one when you wanted it to be destroyed.

I also agree that RotationPeriod is definitely important, I just wasn't sure how to accommodate for this and the ability to do manual rotation.

I'd started work on a PR before I saw your comment but will pause if you're working on it.

I haven't started any work on this, I just started thinking about it last night since I have some free time. If you want to take this on, feel free. I also wanted to work on the KMS data source requested in #495, so I could do that instead.

[tragiclifestories]

Sounds good - I'll take this one.

[danawillow]

I'm not a KMS expert, but from looking around at the docs I definitely think the initial version of this should support automatic rotation. Manual can probably come later- right now a CryptoKeyVersion resource makes the most sense to me, but we also might be able to just do it as part of the CryptoKey resource depending on how that ends up looking.

[tragiclifestories]

Hi all, we're having a bit of trouble acceptance testing this on account of the required environment variables: what should GOOGLE_XPN_HOST_PROJECT be set to?

And should this really be mandatory for running any acceptance tests - it only seems to be used in about three of them?

[mrparkers]

@tragiclifestories If you're only running the KMS tests, you can set that to anything and it won't matter.

[tragiclifestories]

Thanks - thought as much.

[amfarrell]

@mrparkers I'm working with @tragiclifestories on adding CryptoKeys. We are attempting to run your test and having some trouble.

env | grep GOOGLE
GOOGLE_CREDENTIALS={ <The json contents of a credential file>
GOOGLE_REGION=us-central1
GOOGLE_PROJECT=707783349504
GOOGLE_XPN_HOST_PROJECT=omega-winter-121006
GOOGLE_BILLING_ACCOUNT=<redacted 18-character billing account>
GOOGLE_ORG=goroshfarrell-household

go test github.com/terraform-providers/terraform-provider-google github.com/terraform-providers/terraform-provider-google/google github.com/terraform-providers/terraform-provider-google/scripts -run TestAccGoogleKmsKeyRing_basic -v`

From the root of this repository, I get the following

?   	github.com/terraform-providers/terraform-provider-google	[no test files]
=== RUN   TestAccGoogleKmsKeyRing_basic
--- FAIL: TestAccGoogleKmsKeyRing_basic (1.47s)
	testing.go:434: Step 0 error: Error applying: 1 error(s) occurred:

		* google_project.acceptance: 1 error(s) occurred:

		* google_project.acceptance: Error creating project terraform-ldbmjbyj2w (terraform-ldbmjbyj2w): googleapi: Error 400: field [Project.parent] has issue [Parent id must be numeric.], badRequest.
FAIL
FAIL	github.com/terraform-providers/terraform-provider-google/google	1.494s
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-google/scripts	0.032s [no tests to run]

It seems like the key section is:
Error 400: field [Project.parent] has issue [Parent id must be numeric.]

Do you happen to know what a project's "Parent id" is, since https://www.terraform.io/docs/providers/google/r/google_project.html doesn't indicate any parent attribute of a project?

[mrparkers]

@amfarrell I believe that your issue is that the GOOGLE_ORG environment variable needs to be set to the ID of the organization, not its name.

[amfarrell]

It looks like based on https://cloud.google.com/resource-manager/docs/creating-managing-organization I don't actually have an organisation ID yet and I need to sign up for either G-suite or Cloud Identity. However, in signing up for Cloud Identity, I get stuck at this screen

Do you know of a way to get an organisation ID without either G-Suite or Cloud Identity, or were you already signed up for one of these?

[mrparkers]

I was already signed up for an organization (via my personal domain).

You probably don't need an organization to run these tests, but I can't say for sure, since I've only ever used GCP within the context of an organization.

If you want to run these tests without an organization, you can comment out a few lines in the test file. This function generates the terraform definition for the test, and you can remove the parts containing the organization ID to try and create a test project without an organization.

Alternatively, you can comment out the entire project altogether and update the project fields in the google_project_services and google_kms_key_ring resources so the test runs within a project you have already created. The reason the tests don't do this is because the KMS KeyRing resource cannot be deleted, but if you're okay with polluting your pre-exiting project with a bunch of undeletable resources, then feel free. I had to do this while running my acceptance tests locally because I hit my limit on the amount of projects I could create on the free trial.

[tam7t]

@mrparkers

Any advice here? If we come to the consensus that a CryptoKeyVersion should get its own terraform resource, then most of these questions could potentially be shelved until I start working on that. I could also start working on CryptoKey support with manual rotation only, and add automatic rotation in a future PR. Let me know what you guys think.

I think that a CryptoKey resource with an optional field for specifying the rotationPeriod would be the most user friendly interface (at least initially). It would be less flexible in that the entire state of your CryptoKeyVersions would not be managed by terraform, but it would be possible to do out of band manual rotations, take advantage of automatic rotation, and wouldn't necessarily prevent the addition of a CryptoKeyVersion resource in the future.

[tragiclifestories]

See #692 for me and @amfarrell's take on the first version of the CryptoKey resource.

[rosbo]

Closing.

We now have resources to manage kms entities: google_kms_key_ring and google_kms_crypto_key. We also have resources to manage IAM for crypto keys and key rings.
And the next release of this provider will include a new google_kms_secret data source

Please open a separate issue if you want support for a new feature for KMS.

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!