-
Notifications
You must be signed in to change notification settings - Fork 4
Support setting secret via file instead of environment variable #38
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
base: dev
Are you sure you want to change the base?
Conversation
In general, environment variables are NOT a safe way to pass sensitive values. Environment variables are often visible to all processes on the container host, and they may be written to output or logs in some cases. For security, files (either bind-mounted or copied from the host) should always be used instead of environment variables to pass sensitive values.
|
Note that the 'openproject' Docker image also improperly uses environment variables for several sensitive values by default. However, for that image (unlike this image without this PR) it is possible to override the default behavior and pass in sensitive values using files without modifying the existing Docker image:
|
brunopagno
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, thanks a lot for the contribution. The solution looks good to me. Only two small details: I suggested a fix on the code. And we need at least to mention the env variable on the README.
Let me know if you'd like to handle that. Otherwise I can take it over.
🙇
| const SECRET = ( | ||
| process.env.SECRET_FILE | ||
| ? fs.readFileSync(process.env.SECRET_FILE, { encoding: 'utf8', flag: 'r' }) | ||
| : process.env.SECRET | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small fix, but since we're importin readFileSync we don't need to prefix the function call with the fs. on line 8
| const SECRET = ( | |
| process.env.SECRET_FILE | |
| ? fs.readFileSync(process.env.SECRET_FILE, { encoding: 'utf8', flag: 'r' }) | |
| : process.env.SECRET | |
| ); | |
| const SECRET = ( | |
| process.env.SECRET_FILE | |
| ? readFileSync(process.env.SECRET_FILE, { encoding: 'utf8', flag: 'r' }) | |
| : process.env.SECRET | |
| ); |
|
Once that's merged, could you please add this option to the documentation at https://github.com/opf/helm-charts/blob/main/charts/openproject/README.md, so that we inform users about their choices? |
In general, environment variables are NOT a safe way to pass sensitive values. Environment variables are often visible to all processes on the container host, and they may be written to output or logs in some cases. For security, files (either bind-mounted or copied from the host) should always be used instead of environment variables to pass sensitive values.