-
Notifications
You must be signed in to change notification settings - Fork 600
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
added the user and set the necessary permissions #2167
base: main
Are you sure you want to change the base?
added the user and set the necessary permissions #2167
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
6a188ac
to
c45df33
Compare
/gcbrun |
Hi @rock-007, have you tested this locally? The Docker image doesn't build.
|
Hi @bourgeoisor |
src/ledger/ledger-db/Dockerfile
Outdated
RUN if ! getent group postgres > /dev/null; then addgroup -S postgres; fi && \ | ||
if ! getent passwd postgres > /dev/null; then adduser -S postgres -G postgres; fi |
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.
Container images should have deterministic build steps. Either these groups / bindings exist, or they don't. We should need conditional checks.
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.
removed the conditional check.
/gcbrun |
This PR would need changes to the k8s YAML too, e.g. https://github.com/GoogleCloudPlatform/bank-of-anthos/pull/855/files |
/gcbrun |
Done. |
kubernetes-manifests/ledger-db.yaml
Outdated
@@ -103,6 +103,11 @@ spec: | |||
- mountPath: /var/lib/postgresql/data | |||
name: postgresdb | |||
subPath: postgres | |||
securityContext: |
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.
That's a start but it's also missing security contexts to prevent all access to root:
securityContext:
allowPrivilegeEscalation: false
capabilities:
drop:
- all
privileged: false
readOnlyRootFilesystem: true
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.
I have added these now to the ledger-db.yaml
file.
/gcbrun |
/gcbrun |
# Change ownership of the necessary directories | ||
RUN chown -R postgres:postgres /var/lib/postgresql /var/run/postgresql | ||
|
||
# Set thte correct permissions | ||
RUN chmod -R 0700 /var/lib/postgresql/data && chmod -R 0755 /var/run/postgresql |
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.
@mathieu-benoit do you think that's all that was needed? Seems too good to be true but maybe I'm overthinking it
Fixes #517
Background
The DB components fail to start when trying to start as non root user.
Change Summary
I have added group and user as postgres and set the permissions as raised by "mathieu-benoit" in the ticket ref:517
Testing Procedure
Related PRs or Issues
#517