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

refactor(server): load secrets from file instead of env var #972

Closed
wants to merge 6 commits into from

Conversation

iainsproat
Copy link
Contributor

Description & motivation

Security recommendations for Kubernetes are to mount secrets from file instead of env vars. Env
vars are available to all users of a container, often get leaked in log output etc.. If file does
not exist, secrets are loaded from env var for backward compatibility.

fix #897

Changes:

  • direct call to environment variable for secret is replaced with a helper method, this attempts to retrieve the secret from a file and falls back to environment variable.

To-do before merge:

Screenshots:

Validation of changes:

Checklist:

  • My pull request follows the guidelines in the Contributing guide?
  • My pull request does not duplicate any other open Pull Requests for the same update/change?
  • My commits are related to the pull request and do not amend unrelated code or documentation.
  • My code follows a similar style to existing code.
  • I have added appropriate tests.
  • I have updated or added relevant documentation.

References

Security recommendations for Kubernetes are to mount secrets from file instead of env vars.  Env
vars are available to all users of a container, often get leaked in log output etc..  If file does
not exist, secrets are loaded from env var for backward compatibility.

fix #897
}

function getSecret(secretName: string): string {
const secretPath = join('/etc/secrets', secretName)
Copy link
Contributor

@fabis94 fabis94 Aug 29, 2022

Choose a reason for hiding this comment

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

(How?) Will this work on local development environments? Also I'm wondering if this shouldn't be an infra only solution (e.g. some cloud feature that stores some env vars separately, but when they reach the app theyre just env vars), not something that changes how our app reads environment config values? Just considering that this seems like a security feature only relevant for the prod env

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for challenging this, it led to a better solution.
To summarise what we discussed for future reference; the file won't exist on a dev environment, so this will fallback to using environment variables. It is backwards-compatible with our current dev & test setups.
On production the files would exist, so we wouldn't need the secrets in environment variables (secrets in env vars are relatively insecure).

@iainsproat
Copy link
Contributor Author

Obsolete. Still worth doing at some point, but doesn't have priority

@iainsproat iainsproat closed this Jun 18, 2024
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.

Helm: mount secrets as files instead of env vars
2 participants