Skip to content

Set unprivileged user to container image #2925

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

Merged
merged 1 commit into from
Mar 30, 2021

Conversation

jpkrohling
Copy link
Member

Signed-off-by: Juraci Paixão Kröhling [email protected]

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

Related PR: open-telemetry/opentelemetry-collector#2838

@jpkrohling jpkrohling requested a review from a team March 29, 2021 16:09
@codecov
Copy link

codecov bot commented Mar 29, 2021

Codecov Report

Merging #2925 (c78c3fd) into main (791c7f9) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2925   +/-   ##
=======================================
  Coverage   91.53%   91.54%           
=======================================
  Files         463      463           
  Lines       22779    22779           
=======================================
+ Hits        20851    20853    +2     
+ Misses       1436     1435    -1     
+ Partials      492      491    -1     
Flag Coverage Δ
integration 69.09% <ø> (ø)
unit 90.51% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
receiver/k8sclusterreceiver/watcher.go 97.64% <0.00%> (+2.35%) ⬆️

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 791c7f9...c78c3fd. Read the comment docs.

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

We probably need the same change on the core, and in all dockers (including examples)

@jpkrohling
Copy link
Member Author

I opened a PR for the core already: open-telemetry/opentelemetry-collector#2838

I did find other images for other purposes, like examples but wasn't sure those are published somewhere.

@bogdandrutu
Copy link
Member

They are not published, but we should still not build them as root correct?

@jpkrohling jpkrohling force-pushed the jpkrohling/docker-user branch from 170d054 to 655674c Compare March 29, 2021 19:31
@jpkrohling
Copy link
Member Author

They are not published, but we should still not build them as root correct?

The problem isn't at build time, but at runtime. We should indeed provide the best image we can, even for examples, but I'd argue that those are not that critical and may potentially confuse users.

In any case, I went through the one in this repo and did a couple of extra changes:

  • expose port 4317 in addition to 55679/55680, as a first step in deprecating the old OTLP port
  • add the high UID to all images, except the fpm one, which I have no idea how it works
  • bumped the Go builder images to 1.16, which should cover a couple of CVEs in Go itself

Let me know if you think those changes aren't desirable.

@jpkrohling jpkrohling requested a review from bogdandrutu March 29, 2021 19:43
@bogdandrutu
Copy link
Member

@jpkrohling looks like examples failed to build:

ERROR: Unable to lock database: Permission denied
ERROR: Failed to open apk database: Permission denied
ERROR: Service 'otel-collector' failed to build : The command '/bin/sh -c apk --update add ca-certificates' returned a non-zero code: 99
make: *** [Makefile:237: build-examples] Error 1

@jpkrohling jpkrohling force-pushed the jpkrohling/docker-user branch from 655674c to ae38122 Compare March 30, 2021 07:58
@jpkrohling
Copy link
Member Author

Dockerfile for the example fixed.

Signed-off-by: Juraci Paixão Kröhling <[email protected]>
@jpkrohling jpkrohling force-pushed the jpkrohling/docker-user branch from ae38122 to c78c3fd Compare March 30, 2021 07:59
@bogdandrutu bogdandrutu merged commit acea466 into open-telemetry:main Mar 30, 2021
pmatyjasek-sumo pushed a commit to pmatyjasek-sumo/opentelemetry-collector-contrib that referenced this pull request Apr 28, 2021
alexperez52 referenced this pull request in open-o11y/opentelemetry-collector-contrib Aug 18, 2021
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