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

CKV_GIT_4 GitHub "Ensure Secrets are encrypted" is based on a misinterpretation #2374

Closed
shrink opened this issue Feb 8, 2022 · 4 comments · Fixed by #2383
Closed

CKV_GIT_4 GitHub "Ensure Secrets are encrypted" is based on a misinterpretation #2374

shrink opened this issue Feb 8, 2022 · 4 comments · Fixed by #2383
Labels
checks Check additions or changes terraform

Comments

@shrink
Copy link

shrink commented Feb 8, 2022

note: I am not a user of Checkov but I contributed to the GitHub terraform provider so this comment by @ned1313 made me aware or Checkov behaviour which I believe to be incorrect.

Describe the issue

The GitHub Ensure Secrets are encrypted check appears to be based on a misinterpretation of the GitHub functionality and the GitHub terraform provider documentation, because of ambiguity in the argument's naming scheme and the documentation.

Secrets can be provided to GitHub in either a plain text form or an encrypted form. GitHub will encrypt any plain text secrets that arrive, and leave encrypted secrets as is. Regardless of the choice of how to provide the input (plain text or encrypted) it will be stored encrypted by GitHub, then decrypted by GitHub Actions at runtime using the private key.

The choice to use encrypted_value over plaintext_value is made when you have a secret that you are providing as an input to terraform that you do not want to end up in your terraform state in plain text. If a secret has been generated by a different terraform provider (e.g: a cloud provider access token) then it will already exist in the terraform state, so passing it as a plaintext_value to GitHub doesn't introduce any additional exposure.

class SecretsEncrypted(BaseResourceNegativeValueCheck):
    def __init__(self) -> None:
        #  -from github docs "It is also advised that you do not store plaintext values in your code but rather populate
        #  the encrypted_value using fields from a resource, data source or variable as,
        #  while encrypted in state, these will be easily accessible in your code"

The full quote here the GitHub documentation is...

For the purposes of security, the contents of the plaintext_value field have been marked as sensitive to Terraform, but it is important to note that this does not hide it from state files. You should treat state as sensitive always. It is also advised that you do not store plaintext values in your code but rather populate the encrypted_value using fields from a resource, data source or variable as, while encrypted in state, these will be easily accessible in your code.

Rather than serve to describe the behaviour of encrypted_value and plaintext_value the statement is actually just a generic warning about terraform best practices.

Here's an example of what would Checkov currently produces a warning for, despite it being a valid and secure use:

resource "azuread_service_principal" "gh_actions" {
  application_id = azuread_application.gh_actions.application_id
  owners         = [data.azuread_client_config.current.object_id]
}

resource "azuread_service_principal_password" "gh_actions" {
  service_principal_id = azuread_service_principal.gh_actions.object_id
}

resource "github_actions_secret" "example_secret" {
  repository       = "example_repository"
  secret_name      = "example_secret_name"
  plaintext_value  = azuread_service_principal_password.gh_actions.value
}

The check could either be removed completely (because plaintext_value is appropriate to use in most situations) or, if possible, modified to only warn when the value for the plaintext_value argument is a string value directly written into the terraform configuration.

I hope that's clear, please let me know if I can clarify anything.

@shrink shrink added the checks Check additions or changes label Feb 8, 2022
@gruebel
Copy link
Contributor

gruebel commented Feb 9, 2022

@shrink thanks for the detailed explanation. I like both of your suggestions, either to remove the check or limit it to hardcoded values. @schosterbarak any thoughts?

@schosterbarak
Copy link
Contributor

@gruebel let's limit to hard coded values

@lucaspierru-convelio
Copy link

lucaspierru-convelio commented Sep 3, 2024

Hi, it seems this false positive hasn't been fixed as it still triggers when using a plaintext_value that isn't hardcoded, here's an example I encountered on my codebase:

Check: CKV_GIT_4: "Ensure GitHub Actions secrets are encrypted"
        FAILED for resource: module.api_core.github_actions_secret.cloudsql_password
        File: /base/module/api-core/github.tf:13-17
        Calling File: /base/env/staging/main.tf:51-111
        Guide: https://docs.prismacloud.io/en/enterprise-edition/policy-reference/build-integrity-policies/github-policies/ensure-github-actions-secrets-are-encrypted

                13 | resource "github_actions_secret" "cloudsql_name" {
                14 |   repository      = var.github_repository
                15 |   secret_name     = "CLOUDSQL_NAME_${upper(var.environment)}"
                16 |   plaintext_value = google_sql_user.api_core.name
                17 | }

@maximveksler
Copy link

Issue persists

resource "github_actions_environment_secret" "self" {
  for_each = var.github_actions_environment_secrets

  repository    = data.github_repository.self.name
  environment   = github_repository_environment.self.environment
  secret_name   = each.key
  plaintext_value = each.value
}
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
checks Check additions or changes terraform
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants