Adds google_kms_secret data source #741

mrparkers · 2017-11-15T04:57:40Z

Discussion in #495

cc @rochdev and @tragiclifestories who asked for this.

Example:

resource "google_kms_key_ring" "key_ring" {
  name     = "kms-secret-keyring"
  location = "us-central1"
}

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

# After running `gcloud kms encrypt`
data "google_kms_secret" "secret" {
  crypto_key = "${google_kms_crypto_key.crypto_key.id}"
  ciphertext = "CiQAWm5esXYu3WnKrDRi5zY63pw4cZl13InzSGcx4Z7t/6NHneYSOAD3jiTonMNNnUb+iRsD79ir2L91Kfe2VTgD988ixe/VawV/nLHXKD1UOPyXoeJzQWCb96DEluMs"
}

resource "google_sql_database_instance" "master" {
  name = "master-instance"

  settings {
    tier = "D0"
  }
}

resource "google_sql_user" "users" {
  name     = "me"
  instance = "${google_sql_database_instance.master.name}"
  host     = "me.com"
  password = "${data.google_kms_secret.secret.plaintext}"
}

Acceptance test:

TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccGoogleKmsSecret_basic -timeout 120m
?   	github.com/terraform-providers/terraform-provider-google	[no test files]
=== RUN   TestAccGoogleKmsSecret_basic
--- PASS: TestAccGoogleKmsSecret_basic (55.97s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	55.995s
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-google/scripts	0.026s [no tests to run]

tragiclifestories

Mostly looks good - a few comments/questions.

tragiclifestories · 2017-11-15T10:38:55Z

website/docs/d/google_kms_secret.html.markdown

+```hcl
+data "google_kms_secret" "sql_user_password" {
+  crypto_key = "${google_kms_crypto_key.my_crypto_key.id}"
+  ciphertext = "CiQAqD+xX4SXOSziF4a8JYvq4spfAuWhhYSNul33H85HnVtNQW4SOgDu2UZ46dQCRFl5MF6ekabviN8xq+F+2035ZJ85B+xTYXqNf4mZs0RJitnWWuXlYQh6axnnJYu3kDU="


Nitpick: maybe this base64 text should be truncated? It's going to spill out of the <code></code> container on the doc site ...

ciphertext = "C1QAqD+x ... " would probably do.

I agree that you'd probably have to scroll right a bunch to see the whole thing, but I think that a complete example without truncated strings is worth that cost.

The docs for aws_kms_secret also displays the entire encrypted string.

I could, however, encrypt a shorter string. That might make it easier to read.

tragiclifestories · 2017-11-15T10:41:28Z

google/data_source_google_kms_secret_test.go

+	})
+}
+
+func testAccEncryptSecretDataWithCryptoKey(s *terraform.State, resourceName, plaintext string) (string, *kmsCryptoKeyId, error) {


Can we rename resourceName to refer to the fact that it's a CryptoKey? Given that something else is actually under test I think it would clarify things.

tragiclifestories · 2017-11-15T10:43:50Z

google/data_source_google_kms_secret.go

+		return fmt.Errorf("Error decrypting ciphertext: %s", err)
+	}
+
+	plaintext, err := base64.StdEncoding.DecodeString(decryptResponse.Plaintext)


Shouldn't these two calls be the other way around - ie, base64 decode the ciphertext first, and then send the ciphertext to KMS?

Nope. decrypt expects the encrypted data that was originally returned from encrypt, which is already in base64, and it returns some plaintext that is base64 encoded.

Docs

Looks on closer inspection like I got confused by a difference between how the gcloud CLI handles this (ie, it spits raw bytes out to the terminal) and how the REST API does (ie, it base64 encodes everything). Sorry about that.

No worries!

rochdev · 2017-11-15T11:50:13Z

LGTM functionality wise

rosbo · 2017-12-08T19:34:13Z

Hi Michael,

Coding wise, your solution is ready to be merged.

However, we are reluctant to accept this change because we think it encourages a bad practice security wise. As you wrote in the documentation for this data source, the plaintext password is saved in the Terraform state (on operators and potentially CI server machines). This defeats the whole purpose of using KMS in the first place. It gives the user a false sense of security...

I can see that this is better than putting the plaintext password in the configuration file itself. At least, the code repository doesn't store the plaintext password.

Have you considered using Vault to dynamically provision your db users/passwords?
https://www.vaultproject.io/docs/secrets/databases/mysql-maria.html

I am happy to discuss this further :)

mrparkers · 2017-12-08T20:50:35Z

Hi @rosbo, thank you for the response.

One of the reasons I decided to add this was because the AWS provider has a similar data source. I didn't have an immediate need for this data source when I started working on it, but after working with terraform a bit more, I've started to see how a data source like this could be useful.

Perhaps I have a misunderstanding of Terraform state, but I've always treated it as sensitive and encrypted it using Google Cloud KMS before storing it in version control. This is because all of my projects have GKE clusters, and the Terraform state file includes the master's username and password. There are probably other examples of this, but as soon as I saw this, I started encrypting my state files.

The problem this data source solves for me is that it removes the need for me to encrypt my terraform variables file. I have a specific variables file reserved for secrets that could store things like database passwords or pre-shared keys for Cloud VPN, and those are also encrypted with KMS before storing them in version control.

I understand that moving towards a solution like Vault is probably the best way to go. Unfortunately, I just haven't had the time at my company to set this up, especially when our applications are already using KMS and it's extremely easy to use it for this purpose as well.

I definitely understand your concerns, and I'm not opposed to having this PR closed because of them. The workflow I'm using now (encrypting terraform variable files) works until I move to something more permanent like Vault.

wyardley · 2017-12-15T18:10:07Z

I personally also find this very useful (though didn't realize the limitation about it being in the state in cleartext). If there's the issue with this, why does the AWS provider have a kms data source?

I would love to see some other suggestions for lighter weight solutions (short of using something like Vault) to avoid having cleartext secrets stored along with the code. As far as the potential security issue, maybe there should be a docs fix, and / or the kms data source for AWS should be dropped?

(unrelated, also looks like there's a merge conflict now).

rochdev · 2017-12-15T18:14:09Z

I agree that the tfstate already contains a lot of sensitive information and should be handled accordingly (until Hashicorp release a better solution for tfstate secrets). I don't see any other security problems with this approach.

wyardley · 2017-12-15T18:16:09Z

FWIW, the equivalent AWS one has this at the top of the docs now:

NOTE: Using this data provider will allow you to conceal secret data within your resource definitions but does not take care of protecting that data in the logging output, plan output or state output.

maybe a warning like that? It would be nice if resources could be marked sensitive so they're not printed and / or encrypted somehow within the state.

rochdev · 2017-12-15T18:17:57Z

I think they can be marked sensitive (there are quite a few resources that don't display secrets in plans). But there is no way currently to encrypt secrets in the tfstate.

mrparkers · 2017-12-15T18:27:21Z

@wyardley I included a similar warning in the documentation: https://github.com/terraform-providers/terraform-provider-google/pull/741/files#diff-c558c1ef972405ca1d0adcc58f6487b9R17

I think I copy/pasted it from the aws resource.

wyardley · 2017-12-15T18:46:18Z

@rosbo it would seem like using Vault as a backend currently may have some of the same limitations anyway?
https://www.terraform.io/docs/providers/vault/index.html
There are some options like https://github.com/orange-cloudfoundry/terraform-secure-backend that encrypt the backend.

tragiclifestories · 2017-12-16T10:08:04Z

Additionally Google cloud storage encrypts at rest as standard, so using the GCS backend would offer some protection. Then you're left with keeping it out of logs etc.

I no longer have a use case for this, but it seems to me possible to mitigate the security risks on the model of the equivalent aws resource.

rosbo · 2017-12-18T23:07:59Z

@danawillow I am on the fence about this one. What do you think?

danawillow · 2017-12-18T23:40:22Z

I think that given there's precedent for a similar resource in the AWS provider, a way to encrypt state files using remote state, and a prominent warning message on the documentation page, I'm a cautious yes.

mrparkers · 2017-12-19T19:25:43Z

I can fix the merge conflicts here if there's a desire to get this merged. I've been holding off since I wasn't sure.

rosbo

We will accept it. Please fix the merge conflict and I also suggested one small change below. Thank you

rosbo · 2017-12-19T20:37:48Z

google/data_source_google_kms_secret_test.go

+		}...,
+	)
+
+	projectOrg := os.Getenv("GOOGLE_ORG")


Please use these instead and delete the skipIfEnvNotSet block above:

projectOrg := getTestOrgFromEnv(t) projectBillingAccount := getTestBillingAccountFromEnv(t)

mrparkers · 2017-12-19T20:57:26Z

@rosbo I rebased to fix the conflicts and included your suggestion.

rosbo · 2017-12-19T21:00:03Z

Failing build:
google/data_source_google_kms_secret_test.go:13:2: imported and not used: "os"

mrparkers · 2017-12-19T21:00:42Z

Yup, my bad. I fixed it.

rosbo · 2017-12-19T21:24:30Z

--- PASS: TestAccGoogleKmsSecret_basic (60.84s)

gaba-xyz · 2017-12-27T09:08:10Z

Would you accept a PR on a data source for encrypting the plaintext using Google KMS as well? It's quite a logical flow that you would want to create the KMS keyring and crypto key and then encrypt some secret with that. Like how they are doing in the GCP Terraform Vault template now.

rosbo · 2018-01-02T17:12:21Z

Storing the plaintext secret in your code repo is a bad idea but if the secret is the output of some other resource like in the example for the GCP Terraform Vault template you linked above, I see some value and would accept it.

Again, we would need to add a big warning sings in the docs about the terraform state not being encrypted by default.

* Create google_kms_secret datasource * Create google_kms_secret datasource documentation * Remove duplicated code * Create acceptance test * Fix indentation * Add documentation to sidebar * Update Cloud SDK link in docs * Oxford comma * Rename variable to make it clear which resource is under test * Update test to use utils from provider_test

) Signed-off-by: Modular Magician <[email protected]>

shree1988 · 2020-03-20T12:43:55Z

How to fetch encrypted certificates file from Google cloud storage to deploy vault instead of using self sign certificate with terraform code?

ghost · 2020-03-20T13:50:19Z

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!

tragiclifestories reviewed Nov 15, 2017

View reviewed changes

rosbo self-assigned this Dec 7, 2017

rosbo added the waiting-response label Dec 8, 2017

rosbo mentioned this pull request Dec 15, 2017

Feature Request: kms secret in data #867

Closed

tragiclifestories mentioned this pull request Dec 18, 2017

Cloud KMS secret data source #495

Closed

rosbo removed the waiting-response label Dec 18, 2017

rosbo approved these changes Dec 19, 2017

View reviewed changes

mrparkers added 9 commits December 19, 2017 14:55

Create google_kms_secret datasource

18ab638

Create google_kms_secret datasource documentation

8f6e73f

Remove duplicated code

8cb537f

Create acceptance test

bd92b7f

Fix indentation

d1895bd

Add documentation to sidebar

01ea781

Update Cloud SDK link in docs

11af26c

Oxford comma

e4981dc

Rename variable to make it clear which resource is under test

b3e509f

mrparkers force-pushed the data-kms-secret branch from 9a15cad to b3e509f Compare December 19, 2017 20:55

Update test to use utils from provider_test

2c601f3

mrparkers force-pushed the data-kms-secret branch from 858719d to 2c601f3 Compare December 19, 2017 21:00

rosbo merged commit 18a6255 into hashicorp:master Dec 19, 2017

mrparkers deleted the data-kms-secret branch December 19, 2017 21:34

modular-magician added a commit to modular-magician/terraform-provider-google that referenced this pull request Sep 27, 2019

Make cloud composer environment image version updateable. (hashicorp#741

fce58e9

) Signed-off-by: Modular Magician <[email protected]>

ghost locked and limited conversation to collaborators Mar 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds google_kms_secret data source #741

Adds google_kms_secret data source #741

mrparkers commented Nov 15, 2017 •

edited

Loading

tragiclifestories left a comment

tragiclifestories Nov 15, 2017

mrparkers Nov 15, 2017

mrparkers Nov 15, 2017

tragiclifestories Nov 15, 2017

tragiclifestories Nov 15, 2017

mrparkers Nov 15, 2017

tragiclifestories Nov 15, 2017

mrparkers Nov 15, 2017

rochdev commented Nov 15, 2017

rosbo commented Dec 8, 2017

mrparkers commented Dec 8, 2017

wyardley commented Dec 15, 2017 •

edited

Loading

rochdev commented Dec 15, 2017

wyardley commented Dec 15, 2017

rochdev commented Dec 15, 2017

mrparkers commented Dec 15, 2017 •

edited

Loading

wyardley commented Dec 15, 2017

tragiclifestories commented Dec 16, 2017

rosbo commented Dec 18, 2017

danawillow commented Dec 18, 2017

mrparkers commented Dec 19, 2017

rosbo left a comment

rosbo Dec 19, 2017

mrparkers commented Dec 19, 2017

rosbo commented Dec 19, 2017

mrparkers commented Dec 19, 2017

rosbo commented Dec 19, 2017

gaba-xyz commented Dec 27, 2017

rosbo commented Jan 2, 2018 •

edited

Loading

shree1988 commented Mar 20, 2020

ghost commented Mar 20, 2020

Adds google_kms_secret data source #741

Adds google_kms_secret data source #741

Conversation

mrparkers commented Nov 15, 2017 • edited Loading

tragiclifestories left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rochdev commented Nov 15, 2017

rosbo commented Dec 8, 2017

mrparkers commented Dec 8, 2017

wyardley commented Dec 15, 2017 • edited Loading

rochdev commented Dec 15, 2017

wyardley commented Dec 15, 2017

rochdev commented Dec 15, 2017

mrparkers commented Dec 15, 2017 • edited Loading

wyardley commented Dec 15, 2017

tragiclifestories commented Dec 16, 2017

rosbo commented Dec 18, 2017

danawillow commented Dec 18, 2017

mrparkers commented Dec 19, 2017

rosbo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrparkers commented Dec 19, 2017

rosbo commented Dec 19, 2017

mrparkers commented Dec 19, 2017

rosbo commented Dec 19, 2017

gaba-xyz commented Dec 27, 2017

rosbo commented Jan 2, 2018 • edited Loading

shree1988 commented Mar 20, 2020

ghost commented Mar 20, 2020

mrparkers commented Nov 15, 2017 •

edited

Loading

wyardley commented Dec 15, 2017 •

edited

Loading

mrparkers commented Dec 15, 2017 •

edited

Loading

rosbo commented Jan 2, 2018 •

edited

Loading