-
Notifications
You must be signed in to change notification settings - Fork 480
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(security): secure containers run as non-root #3003
feat(security): secure containers run as non-root #3003
Conversation
cmd/core-command/Dockerfile
Outdated
# Create simulated /etc/passwd file with a single user | ||
RUN echo "edgex_user:x:2002:2001:Linux User,,,:/home/edgex_user:/sbin/nologin" >> /tmp/passwd | ||
|
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.
This will change when I pull in the Alpine changes to Scratch images from @jim-wang-intel - however, for future reference, this is how you force a Scratch based image to run as a specific user_id.
echo "Executing custom command: $@" | ||
"$@" | ||
|
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.
This is the security issue I called out in the commit message. This change came in on a commit that @tingyuz and @bnevis-i worked on in Jan 2020. Given that this container is a "run and done" container, we left it with root privileges to appropriately create/distribute secrets. Having root perms to execute whatever command you wanted on the command line is just asking for an injection at some point.
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.
LGTM
@@ -36,7 +36,7 @@ RUN make cmd/security-file-token-provider/security-file-token-provider \ | |||
|
|||
FROM alpine:3.12 | |||
|
|||
RUN apk add --update --no-cache ca-certificates dumb-init curl | |||
RUN apk add --update --no-cache ca-certificates dumb-init curl su-exec |
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.
Do we still need su-exe
?
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.
Already removed it and checked it in...this is "outdated"...
@@ -55,4 +55,8 @@ COPY --from=builder /edgex-go/cmd/security-secretstore-setup/entrypoint.sh /usr/ | |||
RUN chmod +x /usr/local/bin/entrypoint.sh \ | |||
&& ln -s /usr/local/bin/entrypoint.sh / | |||
|
|||
# Create token directory and assign appropriate permissions | |||
RUN mkdir -p /vault/config/assets && \ | |||
chown -Rh 100:1000 /vault/ |
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.
Add to comment why 100:1000
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 didn't create that - that was added before I touched it - I just moved it around...
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.
it is 100 because dockerd volume mount is 100. it is 1000 because that is the first grounp in the container :-)
# write a sentinel file when we're done because consul is not | ||
# secure and we don't trust it it access to the EdgeX secret store | ||
if [ -n "${SECRETSTORE_SETUP_DONE_FLAG}" ]; then | ||
|
||
echo "Changing ownership of secrets to edgex_user:edgex_group" | ||
chown -R 2002:2001 /tmp/edgex/secrets |
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.
Please use environment variables EDGEX_USER and EDGEX_GROUP. In compose builder we'll set them in environment section of vault worker.
environment:
EDGEX_USER=${EDGEX_USER}
EDGEX_GROUP=${EDGEX_GROUP}
cmd/core-command/Dockerfile
Outdated
|
||
# Copy over /tmp/passwd file with injected "user" entry as required by Docker's "USER <user>" command | ||
COPY --from=builder /tmp/passwd /etc/passwd | ||
USER edgex_user |
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 would like to try doing this from compose file, rather than the Dockerfiles.
user: "${EDGEX_USER}:${EDGEX_GROUP}"
define EDGEX_USER and EDGEX_GROUP in .env file and use them through out compose and entry point scripts.
EDGEX_USER=2002
EDGEX_GROUP=2001
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.
IMO, for image itself, i think it makes sense to define it as ENV variable for docker run to use; doesn't have to be in compose-builder only but also any docker run command
Modified the entrypoint script of security-secretstore-setup to set the appropriate ownership of the /tmp/edgex/secrets directory that's mounted to retrieve secrets in various services used throughout. Removed a security concern from the entrypoint script of security-secretstore-setup that allowed root level CLI execution access to anyone that could add parameters via CMD in the dockerfile. Signed-off-by: Beau Frusetta <beau.frusetta@intel.com>
@lenny-intel must be some kind of a Docker Jedi - he sat back, watched me do all these updates, and then get it working - then he said, "wouldn't it be easier if we just put this in the compose?". Backed out all my Dockerfile changes. Recent changes are checked in. |
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.
LIKE IT! LGTM
@@ -47,5 +47,6 @@ EXPOSE $APP_PORT | |||
COPY --from=builder /edgex-go/cmd/support-scheduler/Attribution.txt / | |||
COPY --from=builder /edgex-go/cmd/support-scheduler/support-scheduler / | |||
COPY --from=builder /edgex-go/cmd/support-scheduler/res/configuration.toml /res/configuration.toml | |||
|
|||
ENTRYPOINT ["/support-scheduler"] | |||
CMD ["-cp=consul.http://edgex-core-consul:8500", "--registry", "--confdir=/res"] |
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.
Need newline at end of this file.
Kudos, SonarCloud Quality Gate passed! |
Codecov Report
@@ Coverage Diff @@
## master #3003 +/- ##
=======================================
Coverage 40.21% 40.21%
=======================================
Files 157 157
Lines 13673 13673
=======================================
Hits 5498 5498
Misses 7852 7852
Partials 323 323 Continue to review full report at Codecov.
|
PR Checklist
Please check if your PR fulfills the following requirements:
If your build fails due to your commit message not passing the build checks, please review the guidelines here: https://github.com/edgexfoundry/edgex-go/blob/master/.github/Contributing.md.
What is the current behavior?
Docker containers run as root user.
Issue Number:
#2983
What is the new behavior?
Modified the dockerfiles of various services to create/run as
"edgex_user:edgex_group".
Modified the entrypoint script of security-secretstore-setup to set
the appropriate ownership of the /tmp/edgex/secrets directory that's
mounted to retrieve secrets in various services used throughout.
Removed a security concern from the entrypoint script of
security-secretstore-setup that allowed root level CLI execution access
to anyone that could add parameters via CMD in the dockerfile.
Does this PR introduce a breaking change?
New Imports
Specific Instructions
Are there any specific instructions or things that should be known prior to reviewing?
Other information