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 customer-managed encryption key (KMS) support to GCS backend #31786

Merged
merged 16 commits into from
Oct 4, 2022

Conversation

SarahFrench
Copy link
Member

@SarahFrench SarahFrench commented Sep 14, 2022

Description

Closes #24967

Currently the gcs backend allows users the option of supplying an encryption key from the client side, customer-supplied encryption keys, which is used as an additional layer of encryption in addition to Google-managed encryption keys. If a key isn't supplied in this way then Google-managed encryption keys are used in the GCS bucket by default.

This PR adds the alternative option of using a customer-managed KMS key to encrypt the state files in the bucket (in addition to Google's own encryption). Details of the KMS key are included in Storage API calls, and retrieving the key is internal to the GCS service.

Things to note

The new encryption method conflicts with the customer-supplied encryption feature that already exists in the backend BUT there isn't a risk of data being lost due to this. When switching between the two types of encryption, in either direction, the running terraform init -migrate-state results in errors returned from the API and state being OK and accessible in the 'old' encryption style, still.

Users will need to remove the old method of encryption from the state file before beginning to use the other type of encryption. There needs to be an explicit write to state to remove the old encryption method, and after that the new encryption method can start to be used.

I've updated documentation to give guidance about this, but I could use a review specifically about technical writing. This is the altered page in the deployment of this PR: https://terraform-8l6jf7ots-hashicorp.vercel.app/language/settings/backends/gcs

Due to the conflict between different encryption methods, this PR changes the schema to mark these fields as conflicting. As a result of this, I've needed to stop the encryption_key field having a default value of "". This isn't a breaking change because the (schema.ResourceData).Get function will supply the zero value for that field when it is not defined in the config (or ENV).

Open questions

  1. Can I improve my use of contexts in the new tests?

  2. Do we need to add any code to help handle the transition between different configurations of the GCS backend?

  • I've addressed this in documentation for now
  1. Technical writing review needed? See changes in deployment here

@SarahFrench
Copy link
Member Author

SarahFrench commented Sep 27, 2022

I had another go implementing the new config fields as a nested block instead of a string and investigated the problem I had before:

  • I can use the new backend with a nested block ok in manual tests
  • Automated tests fail to process the new schema containing a nested block and show this error :

Unsupported attribute: An attribute named "kms_encryption_key" is not expected here.


I think I've tracked down the problem to this comment:

// We just ignore blocks altogether, because this body type never has
// nested blocks.

And because of this I'm inclined to leave the new kms_encryption_key attribute as a string

Copy link
Contributor

@mgarrell777 mgarrell777 left a comment

Choose a reason for hiding this comment

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

@SarahFrench Thanks very much for your contribution with this PR. I have some suggestions for wording changes, but overall things look good to me.

website/docs/language/settings/backends/gcs.mdx Outdated Show resolved Hide resolved
website/docs/language/settings/backends/gcs.mdx Outdated Show resolved Hide resolved
website/docs/language/settings/backends/gcs.mdx Outdated Show resolved Hide resolved
website/docs/language/settings/backends/gcs.mdx Outdated Show resolved Hide resolved
website/docs/language/settings/backends/gcs.mdx Outdated Show resolved Hide resolved
website/docs/language/settings/backends/gcs.mdx Outdated Show resolved Hide resolved
website/docs/language/settings/backends/gcs.mdx Outdated Show resolved Hide resolved
website/docs/language/settings/backends/gcs.mdx Outdated Show resolved Hide resolved
@mgarrell777 mgarrell777 added the tw-reviewed Technical Writing has reviewed this PR. label Sep 29, 2022
@SarahFrench SarahFrench removed the request for review from a team September 30, 2022 09:55
@SarahFrench
Copy link
Member Author

Note: just force pushed to squash a few commits and clean up commit messages

@mgarrell777 - Thanks for all your help, I'll watch out for using passive voice in future!

Copy link
Contributor

@alisdair alisdair left a comment

Choose a reason for hiding this comment

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

These changes look good to me!

I like the choice to make the two encryption options conflict rather than attempt to support automatic migration between the two. That kind of change ought to be rare and I think it's better to ask our users to carefully handle it manually than take responsibility for doing it ourselves.

internal/backend/remote-state/gcs/backend.go Outdated Show resolved Hide resolved
internal/backend/remote-state/gcs/backend_test.go Outdated Show resolved Hide resolved
internal/backend/remote-state/gcs/backend_test.go Outdated Show resolved Hide resolved
@SarahFrench
Copy link
Member Author

SarahFrench commented Sep 30, 2022

Thanks for your help @alisdair ! I'm feeling a lot more confident about getting this merged. And also I'm glad that you agree that we should leave changing between encryption methods as a manual steps for users, instead of automating it 😅

What should the next steps for this PR be? Is it ok for me to merge, and should I update CHANGELOG.md afterwards? Or should I leave that to the core team? Here's a proposed enhancement entry:

* backend/gcs: Add `kms_encryption_key` argument, to allow encryption of state files using Cloud KMS keys. [GH-24967]

@alisdair
Copy link
Contributor

Yep, it's fine for you to merge, and please do update the CHANGELOG after doing so. That looks good to me—house style is to end the sentence with a period, and don't forget the GitHub issue at the end (i.e. [GH-31786]).

@SarahFrench SarahFrench merged commit d43ec0f into main Oct 4, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Oct 4, 2022

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 4, 2022

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backend/gcs documentation enhancement tw-reviewed Technical Writing has reviewed this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Encrypt Terraform state with KMS managed keys for GCS backend
4 participants