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

Resource Custom_SLL dumps private keys to terminal/logs #643

Closed
joel-g opened this issue Apr 2, 2020 · 3 comments
Closed

Resource Custom_SLL dumps private keys to terminal/logs #643

joel-g opened this issue Apr 2, 2020 · 3 comments

Comments

@joel-g
Copy link
Contributor

joel-g commented Apr 2, 2020

If you run an apply or plan on a project with a cloudflare.cloudflare_custom_ssl object you will see the entire private key dumped to the terminal and will appear in logs.

The code already has "Sensitive: true" in the cloudflare_custom_ssl schema however it is not hiding the key:
https://github.com/terraform-providers/terraform-provider-cloudflare/blob/bef1fc99c565f946770c0d5c806f1ae5cd529536/cloudflare/resource_cloudflare_custom_ssl.go#L61

I have patched this on the fork we are using in production by adding "Sensitive: true" to the entire custom_ssl_options field but I haven't opened a PR for that because some users would need to see bundle_methods, geo_restrictions and type and this would hide them all.

@jacobbednarz
Copy link
Member

This is a tough one because Terraform doesn't actually do anything secure when it comes to the data; it just doesn't output it to the terminal. The onus is on the operator to store it securely and provide some means of access control (S3 IAM or something like Terraform Cloud).

The output will also should only happen on the first run, subsequent runs without updates shouldn't show it. I'm going to close this off as there isn't anything actionable however feel free to continue brainstorming or digging in if this is something you'd like to change.

@UrosSimovic
Copy link
Contributor

UrosSimovic commented Jul 15, 2020

it just doesn't output it to the terminal.

It does. We are aware of sensitive data in the state, but apply or plan shows the private key in the output and this is not good. Don't know if this is terraform or providers bug, but Sensitve: true is not wokring in this case.

Maybe @joel-g's way of making whole custom_ssl_options block sensitive is way to go. Or putting everything out of typeMap, don't know why those properties are in the map in the first place:

resource "cloudflare_custom_ssl" "project_wildcard" {
  zone_id = var.CLOUDFLARE_ZONE_ID

  private_key = "..."
  certificate = "..."
  bundle_method  = "ubiquitous"
  
  ...
}

@jacobbednarz
Copy link
Member

From the looks of things, this is a known issue since introducing the Terraform SDK plugin due to the way it's passing core the schema, see hashicorp/terraform-plugin-sdk#201 for a full explanation of the situation.

Maybe @joel-g's way of making whole custom_ssl_options block sensitive is way to go.

As @joel-g mentioned, this would essentially make that block unable for in-place updates and would require a new block per certificate. This would make the resource less useful (and more difficult to manage) for those that already using those fields. This would introduce a breaking change and at the earliest, that would land in the next major release which hasn't really started planning for yet.

Or putting everything out of typeMap, don't know why those properties are in the map in the first place:

It's a map for two reasons:

  • To reflect the data structure of the remote API which we attempt to follow to ensure as minimal translation between the two as possible and;
  • The attributes are properties of the certificate itself and this resource also handles options surrounding the deployment configuration too.

At this point, there isn't a whole heap we can do due to being between an internal issue with Terraform and breaking change for other users. Additionally, the security posture isn't changing significantly enough to warrant a drop everything and release a breaking change as the value has always been present, unencrypted in the state file and potentially in your configuration files. The only difference is that Terraform isn't masking this to the output streams.

If this is a blocker for you, I'd recommend maintaining your own fork with the whole block marked as sensitive until we can look at a way to roll this out to all users in an upcoming major release.

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

No branches or pull requests

3 participants