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

feat: make ssm parameters automatic creation optional to avoid having sensitive data in the state #4335

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

Conversation

AppliNH
Copy link

@AppliNH AppliNH commented Jan 6, 2025

Hi! 👋

Throughout my recent experiences, we had strong requirements to not have secret values in the state. To handle that, I suggest to make optional the creation of SSM parameters as part of the regular flow so it doesn't leak secrets in Terraform state.

We could alternatively use the latest ephemeral feature, but I'm not sure everyone is using Terraform 1.10+ atm since it's quite recent. I also think relying on ephemerals should be part of a new breaking release as it won't be compatible with Terraform < 1.10.

Let me know what you think of this!

@npalm npalm self-requested a review January 9, 2025 14:00
Copy link
Collaborator

@npalm npalm left a comment

Choose a reason for hiding this comment

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

@AppliNH thx for the contribution.

Supporting external maanaged ssm parameters is fine. A view things to consider are

  • Update docs see docs dir to explain the usages
  • Explain or express via an example how to ajust the policies so the lambda can access the secrets. Lambda roles are exported.
  • I have added a comment related to the variables you introduces. I think it can done in a single app variable. Which avoid the introdduction of new variables. It also makes it easier to validat the variable.

key_base64 = string
id = string
webhook_secret = string
key_base64 = optional(string)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer if we can combine the new variable in a single one. But not having the time to dig deep now.

variable "github_app" {
  type = type = object({
    key_base64     = optional(string)
    key_base64_ssm = optional(object({
     arn  = string
     name = string
    )}
    ... others

    # validation rules
    # key_base64 != && key_base64_ssm == null || vice versa
    # same for others.

with a local var we can calculate which mechanism should be used so the create_ssm_parameters_github_app var is not needed.

@@ -0,0 +1,77 @@
# Amazon Linux X64 (default)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you intend to keep this example. Having the example is good. But in that case it needs a bit more cleaning.

  • Readme needs to be adjusted, with short explanation
  • Would call it external managed SSM secrets
  • To keep the example convenient, you can create outside the module with terraform the ssm parameters. So the example is simple to use.

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.

2 participants