Makes docker images support non-root execution #1031

Spoffy · 2024-06-11T22:13:49Z

De-escalates to a normal user when the docker image is run as root.

Allows GRIST_DOCKER_USER and GRIST_DOCKER_GROUP to be passed to override the default de-escalation behaviour.

Backwards compatible with previous root installations.

This change adds a new docker_entrypoint.sh, which when run as root de-escalates to the provided user, defaulting to grist:grist. This is similar to the approach used by the official postgres docker image.

To achieve backwards compatibility, it changes ownership of any files in /persist to the user it's given at runtime. Since the docker container is typically run as root, this should always work.

If the container is run as a standard user from the very start:

It's the admin's responsibility to ensure /persist is writable by that user.
/grist remains owned by root and is read-only.

Tested locally in these scenarios:

Running current docker image as root with a /persist volume, then this image. ( root -> grist user )
Running this image as the base for a new container ( always grist user )
Running this image as the base for a new container, under user 1000:1000 ( run container as local machine user )

Addresses:

Defaults to running as grist:grist if executed as root. Allows GRIST_DOCKER_USER and GRIST_DOCKER_GROUP to be passed to override the default de-escalation behaviour.

paulfitz · 2024-06-12T14:59:38Z

Paging also @rouja, it'd be great to get your eyes on this too if you are available (this follows up on #789)

rouja · 2024-06-12T16:02:23Z

Hello,

Nice work !

I built the image locally and did some tests. It seems that the chown on /grist is very very slow. Do you have the same behaviour ?
This would be a problem in k8s because pod will be very long to start...

fflorent · 2024-06-12T16:06:19Z

sandbox/docker_entrypoint.sh

+
+  for dir in "${important_dirs[@]}"; do
+    # Make sure the target user owns everything that Grist needs read/write access to.
+    find "$dir" ! -user "$target_user" -exec chown "$target_user" "{}" +


Why not using chown -R "$target_user" "$dir" instead? For performance reasons?

Yeah, this line is derived from the postgres docker image:
https://github.com/docker-library/postgres/blame/29d4a29d8f750c7c55a22199311520de1e1f7f1e/docker-entrypoint.sh#L51

Which was changed from chown as a time save / performance boost in this PR:
docker-library/postgres#463

Spoffy · 2024-06-12T17:07:39Z

Hello,

Nice work !

I built the image locally and did some tests. It seems that the chown on /grist is very very slow. Do you have the same behaviour ? This would be a problem in k8s because pod will be very long to start...

Thanks @rouja!
We could probably get rid of the chown on /grist to be honest, if it's a performance problem. I think it only needs to be readable, so we don't need to change ownership. Would that address that problem?

rouja · 2024-06-13T12:09:37Z

@Spoffy I just check what I did on a testing deployment on openshift. Actually I do not touch to /grist permissions and I put a volume only on /persist. So I think if /grist is read-only It's Ok.

But maybe we can do the chown on /grist during the build to avoid that files to be owned by root user no matter the final running user ?

rouja · 2024-06-13T12:31:09Z

I think the issue comes from the fact that /grist is quite big :

root@168e17366122:/grist# du -sh /grist/
695M	/grist/

And when we do the chown we do it on the "docker image layer". For instance if we do something like that :

# We want to avoid running Grist as root if possible.
# Try to setup permissions and de-elevate to a normal user.
if [[ $current_user_id == 0 ]]; then
	target_user=${GRIST_DOCKER_USER:-grist}
	target_group=${GRIST_DOCKER_GROUP:-grist}

	cd /
	mv /grist /grist-sav
	mkdir /grist
	cp -r /grist-sav/* /grist/
	chown -R $target_user:$target_user /grist
	cd /grist

	for dir in "${important_dirs[@]}"; do
		# Make sure the target user owns everything that Grist needs read/write access to.
		id_user=$(id -u $target_user)
		test "$(stat -c "%U" $dir)" = "$target_user" || chown -R $target_user $dir
	done

	# Restart as the target user, replacing the current process (replacement is needed for security).
	# Alternative tools are: setpriv, chroot, gosu.
	exec setpriv --reuid "$target_user" --regid "$target_group" --init-groups /usr/bin/env bash "$BASH_SOURCE" "$@"
fi

The start is much faster but I'm pretty sure that it's not the right way to fix our problem.

Spoffy · 2024-06-14T00:25:37Z

Could you give it a try now @rouja? I moved the permissions changes for /grist into the image itself, which adds 2.2s to the image build (on my machine) but means it doesn't need doing at startup.

It does a mean user can't mount /grist and expect it to just work as another user, but that feels like an edge case to me compared to /persist - I'm not sure it's recommended anywhere to actually do that.

We also gain a little bit of security, as a compromised services wouldn't be able to modify the grist code to serve up malicious content.

rouja · 2024-06-14T07:33:53Z

Hi,

Looks good to me. @paulfitz what is the process to add a note in the release note ? Even if changes should be backwards-compatible, it's better to be safe than sorry.

jordigh · 2024-06-21T19:49:21Z

Dockerfile

+# Add a user to allow de-escalating from root on startup
+RUN useradd -ms /bin/bash grist
+ENV GRIST_DOCKER_USER=grist\
+    GRIST_DOCKER_GROUP=grist


Is there a reason why this isn't just a separate ENV stanza?

Figured we may as well not create an extra docker layer for it :)

jordigh · 2024-06-21T19:52:44Z

sandbox/docker_entrypoint.sh

+  target_user=${GRIST_DOCKER_USER:-grist}
+  target_group=${GRIST_DOCKER_GROUP:-grist}
+
+  for dir in "${important_write_dirs[@]}"; do


This loops over a one-element, hardcoded array.

What other write dirs are you foreseeing we'll have one day?

In my opinion, until the day comes when we have those directories, we just get rid of this loop and convert import_write_dirs into a scalar.

Originally, it had /grist in there, which was removed when I realised it was better read-only for performance reasons (chown takes ages).

To be honest - I'm not sure what we'd gain from converting it into a scalar right now? It'd be additional effort now, and more to undo it later if we did end up deciding it was needed.

Future examples may include:

Splitting /persist into /data, /config or similar.

Fixing any issues if there ends up being a subdirectory of /grist that does need to be writable.

jordigh · 2024-06-21T20:08:25Z

sandbox/docker_entrypoint.sh

+    find "$dir" ! -user "$target_user" -exec chown "$target_user" "{}" +
+  done
+
+  # Restart as the target user, replacing the current process (replacement is needed for security).


What security does exec give us? If we had a parent process, a security hole in setpriv would keep the root process running, and thus potentially exploitable?

Exactly - exec throws away the parent process running as root.

Digging out the issue, it looks like it was a vuln mentioned in Gosu (an alternative to setpriv), concerning TTYs persisting across privilege boundaries:
tianon/gosu#37

The exec gets rid of the parent shell, which removes vulnerabilities where malicious code can interact with it.

jordigh · 2024-06-21T20:18:41Z

sandbox/docker_entrypoint.sh

+
+  # Restart as the target user, replacing the current process (replacement is needed for security).
+  # Alternative tools to setpriv are: chroot, gosu.
+  exec setpriv --reuid "$target_user" --regid "$target_group" --init-groups /usr/bin/env bash "$BASH_SOURCE" "$@"


Although a valid bashism, I find $BASH_SOURCE a little obscure here, as you're relying on bash's behaviour of $lol being a synonym for ${lol[0]} for an array lol.

Can we just call it $0 instead, as we do with other shell scripts in Grist?

Can't remember why I chose one over the other - happy to move to $0

paulfitz · 2024-06-24T16:07:14Z

Looks good to me. @paulfitz what is the process to add a note in the release note ? Even if changes should be backwards-compatible, it's better to be safe than sorry.

Current process is to ask @jordigh to bear it in mind for when he writes the notes :-)

paulfitz

Kicked the tires a bit and it seems to work as expected. Hard to imagine all the permutations of setups out there but this seems OK to me. Thanks @Spoffy !

Good to see our image finally passing its own checks :)

paulfitz · 2024-06-17T19:49:55Z

Dockerfile

+
+# Add a user to allow de-escalating from root on startup
+RUN useradd -ms /bin/bash grist
+ENV GRIST_DOCKER_USER=grist\


tiny lint: maybe a space before the \ just so separation is immediately clear without needing to mentally process next line

De-escalates to a normal user when the docker image is run as root. Allows GRIST_DOCKER_USER and GRIST_DOCKER_GROUP to be passed to override the default de-escalation behaviour. Backwards compatible with previous root installations. -------- This change adds a new docker_entrypoint.sh, which when run as root de-escalates to the provided user, defaulting to grist:grist. This is similar to the approach used by the official postgres docker image. To achieve backwards compatibility, it changes ownership of any files in `/persist` to the user it's given at runtime. Since the docker container is typically run as root, this should always work. If the container is run as a standard user from the very start: * It's the admin's responsibility to ensure `/persist` is writable by that user. * `/grist` remains owned by root and is read-only.

Spoffy added 2 commits June 11, 2024 22:55

Makes docker images support non-root execution

c88a9a2

Defaults to running as grist:grist if executed as root. Allows GRIST_DOCKER_USER and GRIST_DOCKER_GROUP to be passed to override the default de-escalation behaviour.

Splits directory checks into read and write

c87999c

paulfitz self-requested a review June 12, 2024 15:00

fflorent reviewed Jun 12, 2024

View reviewed changes

Moves /grist permissions setting into image build

ef58f0e

jordigh mentioned this pull request Jun 14, 2024

Add an endpoint to restart Grist with a new config #1016

Merged

paulfitz mentioned this pull request Jun 17, 2024

Accidentally ran rootless: (Fatal SSL error crash), can rootless be supported? #1050

Open

jordigh reviewed Jun 21, 2024

View reviewed changes

Spoffy and others added 3 commits June 25, 2024 17:48

Cleans up docker_entrypoint.sh

6aedf45

Removes chmod with explanation from Dockerfile

fcb7154

Merge branch 'main' into spoffy/run-docker-as-non-root

4830a70

paulfitz approved these changes Jun 25, 2024

View reviewed changes

Adds space to the Dockerfile for readability

a15ecaa

Spoffy merged commit a8431c6 into main Jun 27, 2024
13 checks passed

Spoffy deleted the spoffy/run-docker-as-non-root branch June 27, 2024 13:24

paulfitz mentioned this pull request Sep 26, 2024

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Makes docker images support non-root execution #1031

Makes docker images support non-root execution #1031

Spoffy commented Jun 11, 2024 •

edited

Loading

paulfitz commented Jun 12, 2024

rouja commented Jun 12, 2024

fflorent Jun 12, 2024

Spoffy Jun 12, 2024 •

edited

Loading

Spoffy commented Jun 12, 2024

rouja commented Jun 13, 2024

rouja commented Jun 13, 2024

Spoffy commented Jun 14, 2024 •

edited

Loading

rouja commented Jun 14, 2024

jordigh Jun 21, 2024

Spoffy Jun 24, 2024 •

edited

Loading

jordigh Jun 21, 2024

Spoffy Jun 24, 2024

jordigh Jun 21, 2024

Spoffy Jun 24, 2024

jordigh Jun 21, 2024

Spoffy Jun 24, 2024

paulfitz commented Jun 24, 2024

paulfitz left a comment

paulfitz Jun 17, 2024

Makes docker images support non-root execution #1031

Makes docker images support non-root execution #1031

Conversation

Spoffy commented Jun 11, 2024 • edited Loading

paulfitz commented Jun 12, 2024

rouja commented Jun 12, 2024

Choose a reason for hiding this comment

Spoffy Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

Spoffy commented Jun 12, 2024

rouja commented Jun 13, 2024

rouja commented Jun 13, 2024

Spoffy commented Jun 14, 2024 • edited Loading

rouja commented Jun 14, 2024

Choose a reason for hiding this comment

Spoffy Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulfitz commented Jun 24, 2024

paulfitz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Spoffy commented Jun 11, 2024 •

edited

Loading

Spoffy Jun 12, 2024 •

edited

Loading

Spoffy commented Jun 14, 2024 •

edited

Loading

Spoffy Jun 24, 2024 •

edited

Loading