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

[GCP] Extract tls_private_key resource from hashicorp/tls provider into an insecure module #222

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

roger2hk
Copy link
Contributor

@roger2hk roger2hk commented Mar 26, 2025

Towards #110, #219.

The staging log arche2025h1 would be affected by the auto deployment. A manual deployment is required. It's probably a good chance to run the Manual Deployment playbook.

@roger2hk roger2hk requested a review from phbnf March 26, 2025 19:00
@phbnf
Copy link
Collaborator

phbnf commented Apr 2, 2025

I like that the bit that we know for sure is insecure is carved out. However, the private key is still passed as an argument, and I don't know for sure how safe this is, and I wouldn't be surprised if there were ways to extract the key from debug logs, or by printing it. Until we have a battle and tested production I think we'd still want to have warnings that mention that the secretmanager module is not safe? What do you think?

@roger2hk
Copy link
Contributor Author

roger2hk commented Apr 2, 2025

I like that the bit that we know for sure is insecure is carved out. However, the private key is still passed as an argument, and I don't know for sure how safe this is, and I wouldn't be surprised if there were ways to extract the key from debug logs, or by printing it. Until we have a battle and tested production I think we'd still want to have warnings that mention that the secretmanager module is not safe? What do you think?

Gotcha, I understand your concern. Enforcing the write-only attributes (i.e. secret_data to secret_data_wo) for the private key field can mitigate the risk. All arguments marked as write-only values will not be stored in the Terraform state. There will also be no way to retrieve the private key field secret data value.

https://registry.terraform.io/providers/hashicorp/google/latest/docs/guides/using_write_only_attributes

Would that address your concern?

@phbnf
Copy link
Collaborator

phbnf commented Apr 2, 2025

I did not know about this! It looks promising indeed, and it seems to be supported by AWS as well. I had an alternative idea in mind, which would not depend on Terraform. It's not a full design, and would certainly requite a bit more work.

Throughout the lifecycle of a log, we need to perform these operations:

  1. Generate the private and public key: today, this happens with terraform
  2. Store the private key (or a secret that allows to generate/access it) in Secret(s) Manager: today, this happens via terraform as well
  3. Load the private key in TesseraCT's memory: this happens using GCP Go APIs directly

We could write a simple go binary that achieves 1, 2 using GCP Go APIs. A user would be set up in advance, with the right permissions to launch this binary, and run it once only. That would make 1. and 2. more transparent I think (a few lines of Go), and means that all key access happens through Go APIs (like 3.). I like that the key provisioning system would have a low number of dependencies, that it would be easier to read the code that generates it, and easy to track where the key lives (only in memory and Secrets Manager).

This means that Terraform would never touch keys, and that the users allowed to deploy Terraform wouldn't have access to it either. Depending on the access that the user has, we could even make sure that key generation can only happen on compute ressources that run on GCP, not on someone's laptop for instance. The only thing we'd need in terraform would be a way to identify the key, through a name or an id for instance, which could be hardcoded for every log. It comes with the downside that one piece of infrastructure (keys) would be created without Terraform, but I think that's an ok compromise.

Note that the binary that generates this key could still be launch via terraform to make the deployment process smooth!
I'm not 100% sure yet how role delegation would work though, I'd need to look into it a bit more to see how do that safely for production logs, but I'd be surprised if there was no way to do this safely. For test logs, it shouldn't be an issue.

@phbnf
Copy link
Collaborator

phbnf commented Apr 2, 2025

In the meantime, if the write_only feature makes the setup a bit better, sure thing! Would it be compatible with this PR, or would we have to go back to using a single module, specifically because of the write_only bit? We could scope this to the private key only, right?

@roger2hk roger2hk force-pushed the gcp-insecure-tls-key branch from 6b54908 to 806cfeb Compare April 3, 2025 09:58
@phbnf
Copy link
Collaborator

phbnf commented Apr 3, 2025

Does this forbid the use of opentofu?

@roger2hk
Copy link
Contributor Author

roger2hk commented Apr 3, 2025

Does this forbid the use of opentofu?

It should be ready now. Please try again to see if it works in OpenTufu.

@roger2hk
Copy link
Contributor Author

roger2hk commented Apr 3, 2025

There is a new warning message after bumping the GCP provider to the latest version. We can also move the public key to the write-only attribute, and then grab the secret value from gcloud secrets versions access VERSION_ID --secret=SECRET_ID. Let's not do it in this pull request.

17:14:28.879 STDOUT terraform: │ Warning: Available Write-only Attribute Alternative
17:14:28.879 STDOUT terraform: │ 
17:14:28.880 STDOUT terraform: │   with module.secretmanager.google_secret_manager_secret_version.sctfe_ecdsa_p256_public_key,
17:14:28.880 STDOUT terraform: │   on ../../secretmanager/main.tf line 34, in resource "google_secret_manager_secret_version" "sctfe_ecdsa_p256_public_key":
17:14:28.880 STDOUT terraform: │   34:   secret_data = var.tls_private_key_ecdsa_p256_public_key_pem
17:14:28.880 STDOUT terraform: │ 
17:14:28.880 STDOUT terraform: │ The attribute secret_data has a write-only alternative secret_data_wo
17:14:28.880 STDOUT terraform: │ available. Use the write-only alternative of the attribute when possible.
17:14:28.880 STDOUT terraform: │ 
17:14:28.880 STDOUT terraform: │ (and one more similar warning elsewhere)

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.

3 participants