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

Change Dockerfile to run app under non-root user #922

Merged
merged 1 commit into from
Dec 21, 2024

Conversation

dmeremyanin
Copy link
Contributor

@dmeremyanin dmeremyanin commented Dec 12, 2024

This PR updates the Dockerfile to improve security by running the application as a non-root user inside the container. This change ensures that the application no longer runs with root privileges, following best practices for containerized environments.

For more information, refer to the following resources:

@dmeremyanin dmeremyanin force-pushed the dockerfile-non-root-user branch from 08660a3 to ddb29de Compare December 14, 2024 09:49
@dmeremyanin
Copy link
Contributor Author

Hey @FZambia, just wanted to make sure you're aware of this PR. We'd love to get this change merged into the main branch to address the concerns raised by our security team.

@FZambia
Copy link
Member

FZambia commented Dec 19, 2024

Hello @dmeremyanin ,

Thanks!

Yep, I've seen. Generally the improvement is useful, but did not have time to test it out. In general, I'd like to have sth like this in Dockerfile eventually:

FROM alpine:3.18

ARG USER=centrifugo
ARG UID=1000
ARG GID=1000

RUN addgroup -S -g $GID $USER && \
    adduser -S -G $USER -u $UID $USER

RUN apk --no-cache upgrade && \
    apk --no-cache add ca-certificates && \
    update-ca-certificates

WORKDIR /centrifugo

COPY --chown=$USER:$USER centrifugo /usr/local/bin/centrifugo

USER $USER

CMD ["centrifugo"]

So I may eventually change it to this version. Please tell me whether you have any concerns about this version compared to the one you submitted?

@dmeremyanin
Copy link
Contributor Author

dmeremyanin commented Dec 19, 2024

Thanks for reviewing the PR. Regarding your suggestion, I have a couple of points I'd like to clarify:

  1. COPY --chown=$USER:$USER centrifugo /usr/local/bin/centrifugo

I understand the intent behind this line, but I think changing the file ownership in the Dockerfile could present a security risk. Specifically, it opens up the possibility of the files being altered by the process itself, which could lead to unintended consequences. As a best practice, I would generally avoid using --chown in the COPY command for this reason.

See #1.3 in Sysdis's recommendations:
https://sysdig.com/learn-cloud-native/dockerfile-best-practices/#1-3-make-executables-owned-by-root-and-not-writable

  1. WORKDIR /centrifugo and user access

As for the WORKDIR, in my initial approach, I had to make sure the USER had write access to the working directory. I ran into an issue where Centrifugo wouldn't start properly if the Centrifugo user didn't own the directory, but I can't fully recall the exact cause of the issue now 😬

In any case, I'd be happy to adjust my approach to align with your recommendations, if you don't mind.

@FZambia
Copy link
Member

FZambia commented Dec 19, 2024

Thx, let's drop chown, was my mistake to assume that user won't be able to execute binary without it. Regarding WORKDIR – I guess sth may go wrong since user can't read config which is volumed into it.

If you can update your PR taking best parts of both my suggestions and your concerns - please do, I'll try to test it out as soon as I can.

@dmeremyanin dmeremyanin force-pushed the dockerfile-non-root-user branch from ddb29de to a9f6cf4 Compare December 20, 2024 07:55
@dmeremyanin
Copy link
Contributor Author

@FZambia I've updated the PR to include your suggestions.

@FZambia FZambia self-requested a review December 21, 2024 09:19
Copy link
Member

@FZambia FZambia left a comment

Choose a reason for hiding this comment

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

👍 👍

@FZambia FZambia merged commit 74b714a into centrifugal:master Dec 21, 2024
2 checks passed
@FZambia
Copy link
Member

FZambia commented Dec 21, 2024

Many thanks @dmeremyanin

@dmeremyanin
Copy link
Contributor Author

Thank you for the review and for merging the PR. Looking forward to the new release!

@FZambia
Copy link
Member

FZambia commented Dec 23, 2024

https://github.com/centrifugal/centrifugo/releases/tag/v5.4.9

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.

2 participants