Adding support for non-root containers #1584

bsobbe · 2023-08-01T14:31:27Z

Steps:

Upgrading s6-overlay to v3.1.5.0 as suggested in this issue in base image.
Refactoring the scripts and making sure they have sufficient permission.
Changing ownerships and permissions of files and directories needed for the service to run.
Running a test call in a non-root k8s environment using services: web, jvb, jicofo, prosody.

Useful links during my process:
https://github.com/just-containers/s6-overlay/blob/master/MOVING-TO-V3.md
just-containers/s6-overlay#539
just-containers/s6-overlay#526

…to noroot

…ing to noroot

…gacy command s6-setuidgid

…ons to dockerfile

…rfile due to permission changes

…ating paths

…ripts to use noroot

Pull

1447 non root

saghul · 2023-08-01T16:08:51Z

Thanks for getting this started!

saghul

I made a first round of review, impressive work, thank you!

docker-compose.yml

jibri/Dockerfile

jibri/rootfs/etc/services.d/10-xorg/run

jigasi/Dockerfile

jigasi/rootfs/etc/services.d/jigasi/run

jvb/rootfs/etc/cont-init.d/10-config

prosody/Dockerfile

saghul · 2023-08-02T07:53:50Z

prosody/rootfs/defaults/prosody.cfg.lua

@@ -215,7 +215,7 @@ http_interfaces = { "*", "::" }
 http_interfaces = { "*" }
 {{ end }}

-data_path = "/config/data"
+data_path = "/config/noroot_data"


Don't do this please. It will break existing installations for no good reason.

this is the only way I could get over permission issues while generating TLS certificates by prosody to /config/data.
it appears to me that something sets the permission of /config/data to root no matter what we set it in the Dockerfile. After creating noroot_data and setting the right ownerships, the SSL issue was solved and I was finally able to make a call. otherwise, JVB and Jicofo both failed while trying to reach prosody internally because it doesn't have cert files.

If you have any ideas that can preserve the current setup and still solve the SSL issue with this user I am definitely open to hearing. but I'm curious to know why this will break the existing installations. since the /config/data is still there and the prosody PID is also still in /config/data. this is just changing the data directory to noroot_data to be able to generate SSLs. It appears to me that once the 10-config script is done it will even move the certificates to /config/certs and removes them from noroot_data at the end of the file.

What user does the 10-config script run as? A cursory look at https://github.com/bjc/prosody/blob/master/util/prosodyctl/cert.lua reveals some ownership changes if the user is root. Or maybe we should set prosody_user and prosody_group to the new user on the config file.

The user is noroot, with user and group ids = 1000. I bet that's why it can't create the certs in /config/data since apparently we can't change that one's ownership but it can create the certs in noroot_data and move them to the correct location since it's the owner. I could not find a better way to make this work, so I reverted it back to noroot_data so prosody doesn't break.

But we can configure the prosody user and group with the options I showed above, did you test that? Why can noroot create stuff in /config/data tough?

We need to fix this before this PR can land.

Since it uses ~= that line checks if the user is NOT root and is NOT the owner of the certs directory, doesn't it? Maybe try creating the certs directory inside data?

@saghul yes precisely. our uid = 1000 and the owner is root:daemon. therefore we hit that if statement and get the error: The directory /config/data is not owned by the current user, won't be able to write files to it.

I don't see why putting /config/certs directory inside/config/datawould help solve this problem. I feel like there is a misunderstanding about the nature of the issue. Ownership over /config/certs is not the issue here. Ownership over /config/data is the actual problem.
Prosody expects noroot to be the owner of that directory. But, regardless of changing that ownership to noroot in the dockerfile, something always overwrites its ownership to root:daemon. My guess is it has something to do with prosody.pid being there (not actually sure if it can be because of this).

The bottom line is, if we can't use noroot_data as a temp path for generating certificates, we need to find a way to change ownership of /config/data. Otherwise, it will keep trying to write the certs in that one and floods the container log with failed to load TLS certificates errors.

yes precisely. our uid = 1000 and the owner is root:daemon. therefore we hit that if statement and get the error: The directory /config/data is not owned by the current user, won't be able to write files to it.

Oh hold on. Why can't we change the permissions of that?

But, regardless of changing that ownership to noroot in the dockerfile, something always overwrites its ownership to root:daemon. My guess is it has something to do with prosody.pid being there (not actually sure if it can be because of this).

Aha. Maybe try with chattr -i ?

@saghul did not work. tried this:
// to install chattr cause it did not exist by default.
RUN apt-get install -y e2fsprogs
// Create /config/data (cause it does not exist at this stage) - change ownership - change attribute and remove immutability.
RUN mkdir -p /config/data && chown -R noroot:noroot /config/data && chattr -R -i /config/data

all of that resulted in:
│ The directory /config/data is not owned by the current user, won't be able to write files to it │
│ The directory /config/data is not owned by the current user, won't be able to write files to it │
│ mv: cannot stat '/config/data/.crt': No such file or directory │
│ mv: cannot stat '/config/data/.key': No such file or directory

same old same old.

Hum, that's very odd, as if the user running prosodyctl is root or something. Can you please take care of the rest of the items and leave this one for last? I'll try to take a look too.

bsobbe · 2023-08-03T08:21:00Z

I made a first round of review, impressive work, thank you!

Thanks for the detailed review. I made a few changes, cleaned up Dockerfiles, and tried to adapt to the requests as much as possible. feel free to check it out.

saghul

Good progress! Some some comments. A couple of generic things:

Use a single RUN
Make the permission fixes geenric with a script if needed so we don't keep repeating that

saghul · 2023-08-03T09:31:01Z

jibri/Dockerfile

+    mkdir -p /config/recordings && \
+    mkdir -p /home/jibri && chown -R noroot:noroot /home/jibri
+
+RUN chown -R noroot:noroot /etc/cont-init.d && \


These 2 are already done in the base image. Is it necessary?

saghul · 2023-08-03T09:31:26Z

jibri/Dockerfile

+
+RUN chown -R noroot:noroot /etc/cont-init.d && \
+    chown -R noroot:noroot /etc/services.d && \
+    chown -R noroot:noroot /etc/jitsi && \


Why is this necessary? Even if files are owned by root we can read them, right?

saghul · 2023-08-03T09:32:04Z

jibri/Dockerfile

+    chown -R noroot:noroot /usr/bin
+
+RUN chmod +x /etc/cont-init.d/* && \
+    chmod +x /etc/services.d/10-xorg/* && \


Any chance we can use wildcards to use a single command here?

saghul · 2023-08-03T09:33:32Z

jicofo/Dockerfile

-COPY rootfs/ /
+COPY --chown=noroot:noroot rootfs/ /
+
+RUN chown -R noroot:noroot /etc/cont-init.d && \


Same, is this necessary? If it is, which I'd like you to double-check, let's a script in the base image which all mages call, instead of duplicating the code on each of them.

saghul · 2023-08-03T09:33:50Z

jicofo/Dockerfile

+    chown -R noroot:noroot /usr/share/jicofo && \
+    chown -R noroot:noroot /etc/jitsi && \
+    chown -R noroot:noroot /etc/jitsi/jicofo && \
+    mkdir -p /config && chown -R noroot:noroot /config


This should go on the base image.

saghul · 2023-08-03T09:35:26Z

prosody/Dockerfile

+COPY --chown=noroot:noroot --from=builder /usr/local/lib/lua/5.4 /usr/local/lib/lua/5.4
+COPY --chown=noroot:noroot --from=builder /usr/local/share/lua/5.4 /usr/local/share/lua/5.4
+
+RUN mkdir -pm777 /var/run/saslauthd && \


Please use a single RUN block, here and elsewhere.

saghul · 2023-08-03T09:36:59Z

prosody/rootfs/defaults/prosody.cfg.lua

@@ -215,7 +215,7 @@ http_interfaces = { "*", "::" }
 http_interfaces = { "*" }
 {{ end }}

-data_path = "/config/data"
+data_path = "/config/noroot_data"


But we can configure the prosody user and group with the options I showed above, did you test that? Why can noroot create stuff in /config/data tough?

We need to fix this before this PR can land.

saghul · 2023-09-12T08:34:24Z

I've just had an idea about the certificate generation @bsobbe . We could stop using prosodyctl to generate them and use the openssl command line instead.

saghul · 2023-11-14T12:15:38Z

@bsobbe do you intend to pick this back up?

bsobbe added 30 commits July 17, 2023 12:27

Upgraded s6-overlay to version v3.1.5.0 - Fixed broken build

3cf538c

Change to s6-overlay new command path

c2f8fc2

Refactoring scripts to be s6-overlay v3 friendly

af959ba

Refactoring jicofo scripts to be s6-overlay v3 friendly

deb8892

Refactoring jigasi scripts to be s6-overlay v3 friendly

1f283d7

Refactoring prosody scripts to be s6-overlay v3 friendly

a984ff8

Refactoring web scripts to be s6-overlay v3 friendly

5c97820

Upgrade s6-overlay to v3.1.5.0 - making cont-init.d scripts executable

e452fec

Make cont-init and services.d scripts executable

8bb646e

Make cont-init and services.d scripts executable

b76dd96

Make cont-init and services.d scripts executable

4524b24

Make cont-init and services.d scripts executable

f0c374e

Refactoring jvb scripts to be s6-overlay v3 friendly

1d854cc

Make cont-init and services.d scripts executable

4163e2c

Make cont-init and services.d scripts executable

13e8663

Adding noroot user with id 1000 - setting scripts ownership for noroot

2053f9d

Setting script ownerships to noroot

9f2d13d

Fixing ownership and permission issues for noroot - setting the user …

9831e43

…to noroot

Fixing ownership and permission issues for noroot

b0c08b7

Refactoring paths for s6-overlay v3

493490e

Moving this block to dockerfile due to permission issues after switch…

93b4cae

…ing to noroot

Fixing permission issues - moving /config prepare block - removing le…

339c6f6

…gacy command s6-setuidgid

Creating user and group noroot with ids 1000

e042186

Changing /var ownership to noroot:noroot

5b3c8af

Removing legacy command

a88e09a

Fixing permission issues due to noroot - moving root specific operati…

6ac6ca6

…ons to dockerfile

Moving back some commands to init config

5020778

Moving back some commands to init config

d224159

Fixing issues with permissions and moving some init commands to docke…

2491511

…rfile due to permission changes

Refactoring for s6 overlay v3

ca8f66a

bsobbe and others added 9 commits July 24, 2023 15:40

Cleaning up commands that moved to dockerfile

d3e63ef

Fixing permission issue causing prosody tls not being loaded

63e15d7

Fixing permission issues for noroot

0a337ab

Changing directory to get over tls certificate permission issue - upd…

7a92697

…ating paths

Change user to noroot - change ownerships and permissions - update sc…

62633f7

…ripts to use noroot

Change user to noroot - change ownerships and permissions - update sc…

a9c7328

…ripts to use noroot

Comment out build blocks with noroot as UID and GID

9264bbf

Merge pull request #2 from jitsi/master

e23b95c

Pull

Merge pull request #1 from bsobbe/1447-non-root

714e525

1447 non root

bsobbe mentioned this pull request Aug 1, 2023

Is there any plan to run container as non root user? #1447

Open

saghul requested changes Aug 2, 2023

View reviewed changes

bsobbe added 6 commits August 3, 2023 10:14

Cleaning up Dockerfile - changing bad saslauthd path and ownership

a5c98ea

Cleaning up Dockerfile - fix indent

304401f

Removing comments

0ae86e7

Cleaning up Dockerfile - changing how the scripts are being started

0efcf93

Cleaning up

d359557

Cleaning up Dockerfile - changing transcripts path

43c9507

bsobbe requested a review from saghul August 3, 2023 08:25

saghul requested changes Aug 3, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for non-root containers #1584

Adding support for non-root containers #1584

bsobbe commented Aug 1, 2023

saghul commented Aug 1, 2023

saghul left a comment

saghul Aug 2, 2023

bsobbe Aug 2, 2023 •

edited

Loading

saghul Aug 2, 2023

bsobbe Aug 3, 2023

saghul Aug 3, 2023

saghul Aug 10, 2023

bsobbe Aug 10, 2023

saghul Aug 10, 2023

bsobbe Aug 11, 2023

saghul Aug 15, 2023

bsobbe commented Aug 3, 2023

saghul left a comment

saghul Aug 3, 2023

saghul Aug 3, 2023

saghul Aug 3, 2023

saghul Aug 3, 2023

saghul Aug 3, 2023

saghul Aug 3, 2023

saghul Aug 3, 2023

saghul commented Sep 12, 2023

saghul commented Nov 14, 2023

Adding support for non-root containers #1584

Are you sure you want to change the base?

Adding support for non-root containers #1584

Conversation

bsobbe commented Aug 1, 2023

saghul commented Aug 1, 2023

saghul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsobbe Aug 2, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsobbe commented Aug 3, 2023

saghul left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saghul commented Sep 12, 2023

saghul commented Nov 14, 2023

bsobbe Aug 2, 2023 •

edited

Loading