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

Set unprivileged user to container image #2838

Merged
merged 2 commits into from
Mar 31, 2021

Conversation

jpkrohling
Copy link
Member

Signed-off-by: Juraci Paixão Kröhling juraci@kroehling.de

Description: This change sets a custom user to the container image, under an ID that yields an unprivileged user.

@jpkrohling
Copy link
Member Author

Setting this to draft, as I wanted to test actually deploying this image. I remember that the process needed permissions to write to the local disk in the past, and need to make sure that the collector either doesn't do that anymore or that the permissions are correctly set to the path it writes.

@codecov
Copy link

codecov bot commented Mar 29, 2021

Codecov Report

Merging #2838 (6433bb2) into main (2e2b338) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2838   +/-   ##
=======================================
  Coverage   91.76%   91.76%           
=======================================
  Files         281      281           
  Lines       15145    15145           
=======================================
  Hits        13898    13898           
  Misses        853      853           
  Partials      394      394           

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 2e2b338...6433bb2. Read the comment docs.

@jpkrohling jpkrohling marked this pull request as ready for review March 29, 2021 19:19
@jpkrohling
Copy link
Member Author

I ran a smoke test, and it does seem to work fine. This is now ready for review.

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
@jpkrohling jpkrohling force-pushed the jpkrohling/docker-user branch from 315fdbd to f3e18d1 Compare March 29, 2021 19:32
@jpkrohling
Copy link
Member Author

I went ahead and changed a couple of other things:

  • use the latest alpine image, so that we get the latest security updates
  • expose the new OTLP port 4317
  • use the latest Go builder image, which addresses some CVEs

@@ -1,16 +1,20 @@
FROM alpine:3.12 as certs
FROM alpine:latest as certs
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like dependabot has support for Docker. Perhaps configure it instead of using latest to keep things reproducible?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just tried changing the gendependabot target here, but the newlines were printed as \n here. I changed this version then to 3.13, and a future PR can enable dependabot for this Dockerfile.

COPY otelcol /
# Note that this shouldn't be necessary, but in some cases the file seems to be
# copied with the execute bit lost (see #1317)
RUN chmod 755 /otelcol

FROM scratch

ARG USER_UID=10001
Copy link
Contributor

Choose a reason for hiding this comment

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

https://docs.docker.com/engine/reference/builder/#user says

When the user doesn’t have a primary group then the image (or the next instructions) will be run with the root group.

So could this end up running as root group unless 10001 is a user that happens to exist and has a primary group? I'm not sure how this works really.

Copy link
Member Author

@jpkrohling jpkrohling Mar 30, 2021

Choose a reason for hiding this comment

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

The USER instruction is affecting the user during the runtime for the process. The user 10001 is unlikely to be in use in typical container hosts, so the user itself will be unprivileged. When running the process, the GID is set to 0, which isn't a privileged group like the UID 0 ("root"). So, when it comes to runtime permissions, we are safe. I just verified if this was still true based on the changes from this PR:

$ ps auxwww | grep otelcol
jpkroeh+  898681  0.0  0.1 1430636 58220 pts/2   Sl+  21:26   0:00 docker run otelcol:latest
10001     898731  0.2  0.1 774488 56228 ?        Ssl  21:26   0:01 /otelcol --config /etc/otel/config.yaml

$ cat /proc/898731/status | head
Name:	otelcol
Umask:	0022
State:	S (sleeping)
Tgid:	898731
Ngid:	0
Pid:	898731
PPid:	898710
TracerPid:	0
Uid:	10001	10001	10001	10001
Gid:	0	0	0	0

I wasn't aware of the second part you mentioned, "or the next instructions". This would explain why files might be created with the root group when the USER is set in previous commands, but again, I don't see a practical problem (or security) because of that.

Copy link
Member

Choose a reason for hiding this comment

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

@jrcamp are you ok with this change?

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
@bogdandrutu
Copy link
Member

@jrcamp if you have extra comments please open an issue :)

@bogdandrutu bogdandrutu merged commit f8bdcf6 into open-telemetry:main Mar 31, 2021
pjanotti pushed a commit to pjanotti/opentelemetry-service that referenced this pull request Apr 6, 2021
* Set unprivileged user to container image

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

* Set alpine version to 3.13

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
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.

4 participants