feat(security): secure containers run as non-root #3003

beaufrusetta · 2021-01-08T21:45:33Z

PR Checklist

Please check if your PR fulfills the following requirements:

Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

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?

Yes
No

New Imports

Yes
No

Specific Instructions

Are there any specific instructions or things that should be known prior to reviewing?

Other information

beaufrusetta · 2021-01-08T21:48:38Z

cmd/core-command/Dockerfile

+# 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
+


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.

beaufrusetta · 2021-01-08T21:51:18Z

cmd/security-secretstore-setup/entrypoint.sh

-echo "Executing custom command: $@"
-"$@"
-


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.

jim-wang-intel

LGTM

cmd/security-secretstore-setup/Dockerfile

cmd/security-secretstore-setup/entrypoint.sh

lenny-goodell · 2021-01-08T22:01:49Z

cmd/security-secretstore-setup/Dockerfile

@@ -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


Do we still need su-exe?

Already removed it and checked it in...this is "outdated"...

lenny-goodell · 2021-01-08T22:02:36Z

cmd/security-secretstore-setup/Dockerfile

@@ -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/


Add to comment why 100:1000

I didn't create that - that was added before I touched it - I just moved it around...

it is 100 because dockerd volume mount is 100. it is 1000 because that is the first grounp in the container :-)

lenny-goodell · 2021-01-08T22:20:12Z

cmd/security-secretstore-setup/entrypoint.sh

 # 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


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}

lenny-goodell · 2021-01-08T22:23:02Z

cmd/core-command/Dockerfile

+
+# 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


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

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>

beaufrusetta · 2021-01-08T23:44:01Z

@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.

@jim-wang-intel @bnevis-i

lenny-goodell

LIKE IT! LGTM

bnevis-i · 2021-01-09T01:01:33Z

cmd/support-scheduler/Dockerfile

@@ -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"]


Need newline at end of this file.

sonarcloud · 2021-01-09T01:07:17Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

codecov-io · 2021-01-09T01:31:15Z

Codecov Report

Merging #3003 (d164d0f) into master (6aacc02) will not change coverage.
The diff coverage is n/a.

@@           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.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6aacc02...d164d0f. Read the comment docs.

beaufrusetta commented Jan 8, 2021

View reviewed changes

lenny-goodell self-requested a review January 8, 2021 21:59

beaufrusetta mentioned this pull request Jan 8, 2021

Make docker container images run as non-root users edgexfoundry/developer-scripts#371

Closed

jim-wang-intel reviewed Jan 8, 2021

View reviewed changes

cmd/security-secretstore-setup/Dockerfile Outdated Show resolved Hide resolved

cmd/security-secretstore-setup/entrypoint.sh Outdated Show resolved Hide resolved

jim-wang-intel assigned beaufrusetta Jan 8, 2021

jim-wang-intel added enhancement New feature or request ireland security-services labels Jan 8, 2021

lenny-goodell requested changes Jan 8, 2021

View reviewed changes

jim-wang-intel added this to the Ireland milestone Jan 8, 2021

jim-wang-intel linked an issue Jan 8, 2021 that may be closed by this pull request

Allow containers to run as non-root user - write Vault token files as non-root user #2983

Closed

lenny-goodell approved these changes Jan 8, 2021

View reviewed changes

bnevis-i approved these changes Jan 9, 2021

View reviewed changes

Merge branch 'master' into issue-2983-non-root-containers

d164d0f

lenny-goodell merged commit 310fcf0 into edgexfoundry:master Jan 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(security): secure containers run as non-root #3003

feat(security): secure containers run as non-root #3003

beaufrusetta commented Jan 8, 2021

beaufrusetta Jan 8, 2021

beaufrusetta Jan 8, 2021

jim-wang-intel left a comment

lenny-goodell Jan 8, 2021

beaufrusetta Jan 8, 2021

lenny-goodell Jan 8, 2021

beaufrusetta Jan 8, 2021

jim-wang-intel Jan 8, 2021

lenny-goodell Jan 8, 2021

lenny-goodell Jan 8, 2021

jim-wang-intel Jan 8, 2021

beaufrusetta commented Jan 8, 2021

lenny-goodell left a comment

bnevis-i Jan 9, 2021

sonarcloud bot commented Jan 9, 2021

codecov-io commented Jan 9, 2021

		# 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

feat(security): secure containers run as non-root #3003

feat(security): secure containers run as non-root #3003

Conversation

beaufrusetta commented Jan 8, 2021

PR Checklist

What is the current behavior?

Issue Number:

What is the new behavior?

Does this PR introduce a breaking change?

New Imports

Specific Instructions

Other information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jim-wang-intel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beaufrusetta commented Jan 8, 2021

lenny-goodell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Jan 9, 2021

codecov-io commented Jan 9, 2021

Codecov Report