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 support to manage secrets in repositories #58

Merged
merged 3 commits into from
Feb 9, 2021

Conversation

mrodm
Copy link
Contributor

@mrodm mrodm commented Feb 3, 2021

In this PR, it's added the support to create and manage secrets in the repositories.

For that it has been used the resource github_actions_secret (link direct to the markdown file in the github repo since I didn't find the documentation anymore to that specific version, sorry) and added the needed code to the module.

Following the example with webhooks, a new test has been added for this, and the README has been updated.

secrets variable is using the same interafece (name and plaintext_value fields) for setting each secret.

It needs to be taken into account that plaintext_value fields are the secrets without being encrypted at all. As the resource exposed by the github provider requires.

As an example to keep the secrets safe, for our organization we're using the provider vault to get the correspoinding secrets and through this new variable set the secrets needed in the Github repository.

provider "vault" {
  address = var.vault_url
  auth_login {
    path = "auth/approle/login"

    parameters = {
      role_id   = var.vault_approle_id
      secret_id = var.vault_secret_id
    }
  }
  version = "~> 2.17.0"
}
data "vault_generic_secret" "github_vault_secrets" {
  path = "secret/key/path/secret"
}
module "new-repository" {
  source  = "xx"
  version = "x.y.z"

  name    = "new-repository"
  ...
  secrets = concat(local.default_secrets, [
    {
      name            = "MY_SECRET"
      plaintext_value = data.vault_generic_secret.github_vault_secrets.data["MY_SECRET_KEY"]
    }
  ])
}

Updated README and added tests
@mrodm mrodm marked this pull request as ready for review February 3, 2021 17:14
@soerenmartius
Copy link
Member

Hi @mrodm,

Thanks a lot for providing this PR! We appreciate that a lot! We'll review your contribution asap., but most likely closer to the weekend.

Again, thank you!
Soren

@mrodm
Copy link
Contributor Author

mrodm commented Feb 5, 2021

Just as a note, I guess it is expected because of the provider, I realised that the secrets are written in plain text also in the terraform state file:

    {
      "module": "module.test-repo",
      "mode": "managed",
      "type": "github_actions_secret",
      "name": "repository_secret",
      "provider": "provider[\"registry.terraform.io/hashicorp/github\"]",
      "instances": [
        {
          "index_key": 0,
          "schema_version": 0,
          "attributes": {
            "created_at": "default",
            "id": "test-repo:MY_SECRET",
            "plaintext_value": "42",
            "repository": "test-repo",
            "secret_name": "MY_SECRET",
            "updated_at": "default"
          },
          "sensitive_attributes": [],
          "private": "xx",
          "dependencies": [
            "module.test-repo.github_repository.repository"
          ]
        }
      ]
    },

So, terraform state should be considered as a sensitive file.

Copy link
Member

@mariux mariux left a comment

Choose a reason for hiding this comment

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

Thanks for the great contribution.

And you are right about sensitive values being stored in state files in terraform and state files should always be considered sensitive information (not checked into to git and stored in an encrypted storage (e.g. S3))

We could also think about making this clear in the README by also pointing to
https://www.terraform.io/docs/language/state/sensitive-data.html

I requested some changes to not also leak secrets in CI by marking the input sensitive. This is not needed for the output though as this will just be a list of the names.

Also added one suggestion to refactor the resource to use for_each as described in the code comments.

variables.tf Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
main.tf Outdated
@@ -429,3 +429,11 @@ resource "github_repository_webhook" "repository_webhook" {
secret = try(var.webhooks[count.index].secret, null)
}
}

resource "github_actions_secret" "repository_secret" {
count = length(var.secrets)
Copy link
Member

Choose a reason for hiding this comment

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

This is fine and works well but has the downside of recreating secrets when elements are added to the list of secrets..

To fix this behavior, I would suggest to use for_each here and key it by secret_name.

another option would be to not have a list as input type but a map:

benefits:

  • no duplicate keys (secret_name) by design
  • less code to write as you can just map secret_name => plaintext_value

downside:

  • not having plaintext in such a clear way.

The downside could be tackled by renaming the input variable to plaintext_secrets to make this clear to the user that this input is sensitive and you might not want to check it into your git repository but just grab it from some data source

wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't know about this issue when using count to create resources.I'll check the options you mentioned
Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

as long as the names are known in the plan phase it makes sense to use them as keys for . it gets problematic once you want to use for_each on computed values (depend on resources you create in the same run).. but I guess secret names are static and not the result of creating other resources..

when using count it is safe to add new values to the end of a list and removing them from the end. once you remove them in the middle or beginning or add them there, you will see a cascading set of changes being planned and applied..

In this case this would be of little impact, I assume, as it would just recreate all secrets.. the chance of hitting a situation where a secret is recreated via terraform while a CI job tries to read it is very very low.. ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just updated this resource using for_each and a map type instead of a list of objects. Readme and test have been updated accordingly too.

I didn't find any other better name for this variable, so I took your sugestion and rename it to be plaintext_secrets

Copy link
Member

@mariux mariux left a comment

Choose a reason for hiding this comment

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

lgtm.. thanks for contributing this awesome feature ;)

@soerenmartius pls check & merge ;)

@mariux mariux changed the base branch from master to integration February 9, 2021 13:00
@mariux mariux merged commit 86c351d into mineiros-io:integration Feb 9, 2021
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.

None yet

3 participants