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

Allow for images to run as non-root #334

Closed
wiardvanrij opened this issue Nov 11, 2020 · 10 comments · Fixed by #2265
Closed

Allow for images to run as non-root #334

wiardvanrij opened this issue Nov 11, 2020 · 10 comments · Fixed by #2265
Assignees
Labels
keepalive Label to exempt Issues / PRs from stale workflow operations

Comments

@wiardvanrij
Copy link

Is your feature request related to a problem? Please describe.
In the baseline: security. It could pose as a serious security concern.
Do we need to run as root? No.

Especially in enterprise environment, we are simply not even allowed to run images as root. Therefore I would like to see a fix.

Describe the solution you'd like
Add users to the docker images, perhaps change a few paths (i.e. placement of the executables).

Describe alternatives you've considered

The compactor can be used with the following workaround:

        volumeMounts:
        - mountPath: /conf
          name: tempo-conf
        - mountPath: /var/tempo
          name: tempo  
      volumes:
      - name: tempo
        emptyDir: {}

By adding an emptydir we override the /var/tempo folder as non-root.

For the querier we can do the same "trick" for the /var/tempo folder BUT it also wants to write and use files in the /tmp folder. This is an issue because we cannot mount an empty dir on the /tmp folder. This is because the original /tmp folder contains the tempo-query executable.

The real dodgy workaround is to build an image with tempo-query in a different place. I.e.:

FROM  tempo-query:latest
COPY tempo-query /tempo-query 

Then use an initcontainer:

      initContainers:
      - name: please-no
        image: busybox
        command: ['sh', '-c', 'ln -s /tempo-query /tmp/tempo-query']
        volumeMounts:
          - name: tmp
            mountPath: /tmp

while having the following emptydir mounts:

        volumeMounts:
        - mountPath: /tmp
          name: tmp
        - mountPath: /conf
          name: tempo-query-conf         
      volumes:
      - name: tempo
        emptyDir: {}
      - name: tmp
        emptyDir: {}

Additional context

The workaround is fine for a proof of concept, but not production usage :)

@joe-elliott
Copy link
Member

Agree that non root users would be preferred.

The compactor doesn't actually need to write anything to disk. Maybe it just attempts to create a folder or something, but this is not necessary. Is the problem that writing to /var requires root in the container?

The querier only needs disk if you use the disk cache option which I don't think is configured in any of our examples. It sounds like there are similar problems here with /tmp? Is /tmp root only also?

Sadly the GRPC Go plugin system always throws an error if /tmp doesn't exist. Adding the binary there was a hack to just allow me to move forward.

@wiardvanrij
Copy link
Author

I did not have time to actually look into all the details and present an actual solution. Personally I would recommend adding strict limits and then test a deployment. With these limits I'm pointing towards forcing a pod to run as non-root. With that in place you can see the actual behaviour, errors, and logs which indicate the specific changes required to achieve running as non-root.

@savishy
Copy link

savishy commented Jan 12, 2021

I am in a similar situation where the organization policy disallows images with non-root or even a user/group set to nobody.
I added the following code snippet prior to the entrypoint and rolled out a custom image that worked in our restricted setup.

If this helps, I would love to file a PR.

RUN addgroup -S -g 10001 tempo \
    && adduser -S -D -u 10000 -s /sbin/nologin -h /app -G tempo tempo \
    && chown -R 10000:10001 /tempo
USER 10000:10001

@joe-elliott
Copy link
Member

@savishy

That roughly looks correct to me. A PR would be much appreciated!

@joe-elliott
Copy link
Member

Related to #456. Also note that we will be dropping the tempo-query dependency soon: #382.

We still want to do this and will leave this issue open.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 7, 2022

This issue has been automatically marked as stale because it has not had any activity in the past 60 days.
The next time this stale check runs, the stale label will be removed if there is new activity. The issue will be closed after 15 days if there is no new activity.
Please apply keepalive label to exempt this Issue.

@github-actions github-actions bot added the stale Used for stale issues / PRs label Dec 7, 2022
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Dec 23, 2022
@zalegrala zalegrala removed the stale Used for stale issues / PRs label Feb 9, 2023
@zalegrala
Copy link
Contributor

@savishy To clarify, your organization requires non-root containers, correct? Are you interested in submitting a PR for this change?

@bonzo71
Copy link

bonzo71 commented Mar 6, 2023

+1 for having this implemented. Running containers as non-root should be a standard.

@zalegrala zalegrala reopened this Mar 23, 2023
@zalegrala zalegrala self-assigned this Mar 23, 2023
@zalegrala
Copy link
Contributor

I'll put up a PR when I get a chance for this.

@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had any activity in the past 60 days.
The next time this stale check runs, the stale label will be removed if there is new activity. The issue will be closed after 15 days if there is no new activity.
Please apply keepalive label to exempt this Issue.

@github-actions github-actions bot added the stale Used for stale issues / PRs label May 23, 2023
@zalegrala zalegrala added the keepalive Label to exempt Issues / PRs from stale workflow label May 24, 2023
@github-actions github-actions bot removed the stale Used for stale issues / PRs label May 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keepalive Label to exempt Issues / PRs from stale workflow operations
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants