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: add Why Storing Secrets in Environment Variables is a Bad Idea blog #13054

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

dirien
Copy link
Contributor

@dirien dirien commented Oct 7, 2024

Proposed changes

Unreleased product version (optional)

Related issues (optional)

@dirien dirien changed the title feat: add Why Storing Secrets in Environment Variables is a Bad Idea … feat: add Why Storing Secrets in Environment Variables is a Bad Idea blog Oct 7, 2024
@pulumi-bot
Copy link
Collaborator

@dirien dirien requested a review from interurban October 7, 2024 11:29
@pulumi-bot
Copy link
Collaborator

@aaronkao
Copy link
Contributor

aaronkao commented Oct 7, 2024

Please find a place in your blog to link to this page - https://www.pulumi.com/what-is/what-is-secrets-management/

We are trying to boost our domain authority for secrets management.

@dirien
Copy link
Contributor Author

dirien commented Oct 7, 2024

Please find a place in your blog to link to this page - https://www.pulumi.com/what-is/what-is-secrets-management/

We are trying to boost our domain authority for secrets management.

@aaronkao done!

@pulumi-bot
Copy link
Collaborator

@pulumi-bot
Copy link
Collaborator

@pulumi-bot
Copy link
Collaborator

@adamgordonbell adamgordonbell self-requested a review October 8, 2024 10:38
@pulumi-bot
Copy link
Collaborator

@pulumi-bot
Copy link
Collaborator

Copy link
Contributor

@adamgordonbell adamgordonbell left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@thoward thoward left a comment

Choose a reason for hiding this comment

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

This is a great article on the topic, however the provided example falls short in a few ways that I think need to block publishing unless resolved.

First, it is mono-lingual (we should show this in all languages we support)

Second, the solution at the end is actually very problematic. On one hand, it removes the vulnerability of passing the database password via environment variable, yes, however, it trades that db password environment variable for one with greater potential for risk. Instead of passing the database password which only lets you access the database, we pass the PULUMI_ACCESS_TOKEN which provides access to the entirety of ESC's secrets store (and any third-parties it interacts with). This could potentially leak MANY secrets instead of just the one database password, or even allow update access to replace secrets (which can take down the entire infrastructure through access denial while also compromising it with an attacker-provided password)... and all this is accomplished using the exact same cross-process leaks we just described. Not good.

Instead we should show some best practices that actually reduce risk:

  • read and then immediately clear the environment variable (minimize time of potential exposure)
  • use OIDC to generate dynamic short-lived credentials (minimize time/scope of exposure)
  • use a redacting logger like: https://blog.arcjet.com/redacting-sensitive-data-from-logs-with-go-log-slog/ (minimize leaks in logs)
  • security harden your git commit workflow by apply a pre-commit lint rule to prevent secret leaks via code (see infisical scan) also applying something like gitleaks will help bolster that. might need to create some custom pre-commit linter rules to check for uses of [env] access and logging calls, but that's going to be hard to make generic and would need to be customized to the application. (minimize leaks via source control)
  • run your application in an ephemeral single-use container environment rather than a process on a shared machine (minimize potential for cross-process attacks)
  • create and destroy unix users on the fly to minimize the potential for peer-level access (e.g. a service worker that always runs as the same userid can access any other service worker process as a peer because they are running as the same user). a modern unix system can have up to 4294967294 concurrent distinct users, so there's no reason not to create a new userid for every process because the system is unlikely to ever have that many concurrent processes. (reduce access to leaks via ps)
  • when spawning subprocesses (which typically inherit the parent process's entire environment), show code for how to provide a tailored environment that only passes the config/secrets that process needs access to (reduce surface area of risk)

In summary, we really should not present a solution that makes things worse, not better, and should provide best practice advice that improves the specific security vulnerabilities we're addressing (even if they aren't ESC product features.. yet).

@dirien
Copy link
Contributor Author

dirien commented Oct 9, 2024

Opened issue here pulumi/esc#402

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.

5 participants