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

Add a grist user in Dockerfile to avoid root user #789

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rouja
Copy link

@rouja rouja commented Dec 12, 2023

Hello,

In order to follow the principle of least privilege, the docker image should not run as root.

@fflorent
Copy link
Collaborator

A bit of context: @rouja works for the DINUM (Interministerial Directorate for Digital Affairs, official website in French), a French public agency with which the ANCT collaborate for deploying Grist.

@rouja
Copy link
Author

rouja commented Dec 12, 2023

This change will probably break things on existing deployment. Because the old image run as root, some files/folders are created with root permissions under /persist/* and /grist/* folder. When the new image will boot with the grist user, it will not be able to edit theses files.
Maybe we need to prepare a migration procedure.

@paulfitz
Copy link
Member

Thanks @rouja! The overall change makes sense, I'm just trying to think about how to handle the impact on existing users, specifically if files in /persist become unwritable. I think we'd at least want an early check to fail with a clear error message and advice on how to fix?

Any particular significance to the 10022 number? With other tools I use under docker, I find myself matching the uid/gid with the host for maximum convenience, in case I want to manipulate the persisted files from the host.

@rouja
Copy link
Author

rouja commented Dec 15, 2023

Hi,

I choose 10022 arbitrarily. So if you prefer something else we can change it. Maybe we can add a check in run.sh with an explicit message ?

Moreover, on openshift cluster, the cluster assign a random uid to every pod. So if we want to support this case too, we need to configure the gid to 0 because on these cluster, the gid is always 0 and set the permissions consequently.

@paulfitz
Copy link
Member

paulfitz commented Jan 9, 2024

I'm sorry review of this PR has stalled. I'm not sure what to do with it. It is an improvement, but accepting and merging it as is would presumably lead to a wave of confused and upset users. A check in run.sh to give a clear error message would help mitigate that, but there will still be pain for users. Picking a concrete group and user id to use also seems hard. This issue seems very generic, and not at all special to Grist. Is there an example of an image that does a good job out there?

@rouja
Copy link
Author

rouja commented Jan 10, 2024

Hi,

https://github.com/docker-library/postgres does a good job for permissions but it adds complexity. It use gosu and nss_wrapper. And even if it's one of the best image I know about permissions this image doesn't handle the switch from root user to an unprivileged user...

Maybe the right way to manage this issue is to keep the image with root user and add a note somewhere in the documentation that says something like :

For best compatibility, our docker images run as root but best practices recommend to not run containers as root on production environments. We recommend to override the userId with -u on docker or with a specific security context on k8s

What do you think ?

@paulfitz
Copy link
Member

Yes, I guess documentation may be the best we can do, in practice. One thing we will be working on is a "bootstrap page" for Grist administrators, to inspect the status of their Grist installation and make sure everything is ok. For example, if sandboxing for documents is not enabled, the page will flag that prominently as a problem and give advice about how to resolve that. Perhaps this best practice could also be checked on such a page?

@rouja
Copy link
Author

rouja commented Jan 22, 2024

Hello,

I'm really sorry I forgot to answer. But yes, I think this "bootstrap page" would be a goode place.

@paulfitz
Copy link
Member

Hi @rouja, I've started work on a bootstrap page (now just called boot page) that includes a check of the user running Grist (see #850)

Screenshot from 2024-02-15 17-49-52

Should get a bit prettier before it lands I hope :)

@rouja
Copy link
Author

rouja commented Feb 16, 2024

Hi,

I just run a test locally and it seems ok to me.

2024-02-16_12-33

I think it's normal that Greast reachable is KO because I use grist.127.0.0.1.nip.io and this cannot works from pod.

What does the check Built-in Health check to know if I can fix it or not on my local testing deployment.

Nice work !

@paulfitz
Copy link
Member

I think it's normal that Grist reachable is KO because I use grist.127.0.0.1.nip.io and this cannot works from pod.

Thanks for testing @rouja !

@fflorent
Copy link
Collaborator

fflorent commented Mar 6, 2024

@paulfitz @rouja Looks like the need of this PR is now covered by #850

I feel like you are both OK to close this one.
I close it, if I am wrong don't hesitate to poke me so I can reopen it.

@fflorent fflorent closed this Mar 6, 2024
@paulfitz
Copy link
Member

paulfitz commented Mar 6, 2024

@paulfitz @rouja Looks like the need of this PR is now covered by #850

I think it would also be useful to give self-hosters guidance on how to run as a non-root user, and if there are steps we can take to make this easier without breaking existing installations, it would be good to take these steps. As a self-hoster, I could imagine being annoyed at Grist when I follow the instructions to install it, and then look at the boot page and see a big red X :-)

@fflorent fflorent reopened this Mar 6, 2024
@fflorent
Copy link
Collaborator

fflorent commented Mar 6, 2024

I think it would also be useful to give self-hosters guidance on how to run as a non-root user, and if there are steps we can take to make this easier without breaking existing installations, it would be good to take these steps. As a self-hoster, I could imagine being annoyed at Grist when I follow the instructions to install it, and then look at the boot page and see a big red X :-)

Alright, good point!

I tend to think that we would just have to update some documentation:

And probably just ensure that the image does not run using the grist user by default.

(and if we think it is worth, alerting with a message that running the docker image is discouraged now and invite to change that)

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.

None yet

3 participants