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

Make Docker image immutable on runtime #248

Closed
wants to merge 2 commits into from

Conversation

shaman007
Copy link

@bitwarden-bot
Copy link

Logo
Checkmarx One – Scan Summary & Detailsc865b656-e4c6-481a-8d0c-73f93931462f

No New Or Fixed Issues Found

@djsmith85 djsmith85 requested a review from a team April 23, 2024 19:59
@yggdrasil-tynor
Copy link

Will this be approved / merged? Very much needed to be able to host on Azure App Service or Azure Container Apps

@djsmith85 djsmith85 requested a review from a team July 18, 2024 20:15
@georglauterbach
Copy link

The changes in this PR are very welcome, especially when running BW in Kubernetes and settings the UID/GID is a security best-practice. Why is this PR not moving forward?

@CLAassistant
Copy link

CLAassistant commented Sep 25, 2024

CLA assistant check
All committers have signed the CLA.

@tangowithfoxtrot
Copy link
Contributor

Hi, @shaman007

Thanks for your work on this! I'll be giving this a review. Right now, the build is failing for me when building locally, mostly due to references to gulp in the Dockerfile, which we removed in #281. Would you mind pulling in the updates from main and updating your branch?

Copy link
Contributor

@tangowithfoxtrot tangowithfoxtrot left a comment

Choose a reason for hiding this comment

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

Thanks again for your work on this! I made a few suggestions:

exec setpriv --reuid=1000--regid=1000 --init-groups /usr/bin/supervisord
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
exec setpriv --reuid=1000--regid=1000 --init-groups /usr/bin/supervisord
exec setpriv --reuid=1000 --regid=1000 --init-groups /usr/bin/supervisord

Comment on lines +277 to +278
#Set up user and group
RUN addgroup --gid 1000 bitwarden
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#Set up user and group
RUN addgroup --gid 1000 bitwarden
# Set up user and group
RUN addgroup --gid $PGID bitwarden

Copy link
Contributor

Choose a reason for hiding this comment

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

We'll also need to set the following in the final stage:

ENV PUID=1000
ENV PGID=1000

Copy link
Contributor

@tangowithfoxtrot tangowithfoxtrot Nov 5, 2024

Choose a reason for hiding this comment

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

We also need to consider the impact of hard-coding the user and group ID in the image. Since they're currently set at runtime (in the entrypoint script), it's possible to provide custom IDs. Setting them in the Dockerfile removes this capability, and could potentially impact other deployments.

Comment on lines +281 to +283
#Create symlincs for the identity files:
RUN ln -s /app/Identity/identity.pfx /etc/bitwarden/identity.pfx
RUN ln -s /app/Sso/identity.pfx /etc/bitwarden/identity.pfx
Copy link
Contributor

@tangowithfoxtrot tangowithfoxtrot Nov 5, 2024

Choose a reason for hiding this comment

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

Suggested change
#Create symlincs for the identity files:
RUN ln -s /app/Identity/identity.pfx /etc/bitwarden/identity.pfx
RUN ln -s /app/Sso/identity.pfx /etc/bitwarden/identity.pfx
# Create symlinks for the identity files:
RUN ln -sf /app/Identity/identity.pfx /etc/bitwarden/identity.pfx
RUN ln -sf /app/Sso/identity.pfx /etc/bitwarden/identity.pfx

I ran into some issues when building this locally, were the existing files where getting cached in the docker build, so I had to force-create the symlinks.

sed -i "s/autostart=true/autostart=${BW_ENABLE_SSO}/" /etc/supervisor.d/sso.ini

# Set desired ownership:
RUN chown -R 1000:1000 \
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
RUN chown -R 1000:1000 \
RUN chown -R $PUID:$PGID \

@tangowithfoxtrot
Copy link
Contributor

tangowithfoxtrot commented Nov 5, 2024

Thinking more about this, I don't think we can merge this. This will cause issues for people running on systems where the host UID and GID don't match what's hard-coded in the image.

While this specific PR will be closed, we are looking into similar functionality in an internal ticket.

@shaman007
Copy link
Author

@tangowithfoxtrot, I cannot understand why security related software with personal data and secrects and direct access from the internet needs to be explicitly run as a super user and only as a superuser.

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.

Run Bitwarden-selfhosted as non-root container on the read-only filesystem
7 participants