Improve support for gitea instances not running as root or uid 1000 #266

2021-12-20T17:20:58Z

nmasse-itix commented

2021-12-20 17:20:58 +00:00

(Migrated from gitea.com)

Context

PR #259 introduced support for running Gitea as a uid different than 1000 (git) or 0 (root).

Problem

In init_directory_structure.sh, there is a "chown 1000:1000" on /tmp/gitea.
This chown only works when running as root or when the target directory is already owned by uid 1000.

As a result, the init container "init-directories" fails on startup when running Gitea with a uid different from 0 or 1000.

Initially, I worked around it by implementing an "initPreScript". But it would make user's life easier if we can make it work out-of-the-box.

Resolution

I'm taking model on the chown a few lines above that depends on the value of image.rootless. Since the chown only works on default (root) image and is useless on rootless image, there is no need to run it on rootless image.

## Context PR #259 introduced support for running Gitea as a uid different than 1000 (git) or 0 (root). ## Problem In init_directory_structure.sh, there is a "chown 1000:1000" on /tmp/gitea. This chown only works when running as root or when the target directory is already owned by uid 1000. As a result, the init container "init-directories" fails on startup when running Gitea with a uid different from 0 or 1000. Initially, I worked around it by implementing an "initPreScript". But it would make user's life easier if we can make it work out-of-the-box. ## Resolution I'm taking model on the chown a few lines above that depends on the value of image.rootless. Since the chown only works on default (root) image and is useless on rootless image, there is no need to run it on rootless image.

justusbunsi commented

2021-12-21 13:23:01 +00:00

(Migrated from gitea.com)

It may be a good way to check the user currently running the init container. In case of root, there is probably no securityContexts set and a file/folder permission modification is necessary to match Gitea requirements. In case there is any securityContext defined, the runAsUser + runAsGroup setting should be used to actually chown any files for the later running live container. As fallback, we should use 1000:1000.

For some reasons I haven't noticed that in the previous PR #259.

It may be a good way to check the user currently running the init container. In case of root, there is probably no securityContexts set and a file/folder permission modification is necessary to match Gitea requirements. In case there is any securityContext defined, the runAsUser + runAsGroup setting should be used to actually `chown` any files for the later running live container. As fallback, we should use 1000:1000. For some reasons I haven't noticed that in the previous PR #259.

nmasse-itix commented

2021-12-21 15:00:01 +00:00

(Migrated from gitea.com)

For some reasons I haven't noticed that in the previous PR #259.

Yes, I should have included this in the previous PR since both topics are tied. I will be more careful for my next PR. :)

We need to handle two different cases:

when using the rootful image and running as uid == 0
when using the rootless image and running as uid > 0

When using the rootless image, all processes are running as the specified uid (1000 by default but customizable). And all created files and folders are necessarily owned by that uid.

Also, with uid > 0, we cannot use the chown command.

The Linux kernel prevents any user from:

giving ownership of their files to others
taking ownership of other's files

For instance :

$ id
uid=1000(nicolas) gid=1000(nicolas) groups=1000(nicolas)

$ chown nobody:nobody $HOME/Desktop 
chown: changing ownership of '/home/nicolas/Desktop': Operation not permitted

$ chown $UID:$GID /tmp
chown: changing ownership of '/tmp': Operation not permitted

The only special case where we can use chown is when the user already owns the file. It is a "noop": nothing is actually done.

For instance :

$ id
uid=1000(nicolas) gid=1000(nicolas) groups=1000(nicolas)

$ ls -ld $HOME/Desktop
drwx------. 1 nicolas nicolas 552 Dec 10 22:31 /home/nicolas/Desktop

$ chown nicolas:nicolas $HOME/Desktop
$ echo $?
0

$ ls -ld $HOME/Desktop
drwx------. 1 nicolas nicolas 552 Dec 10 22:31 /home/nicolas/Desktop

The previous behavior in the Helm chart was unconditionnally "chown 1000:1000 /tmp/gitea" which:

changes file ownership when running the rootful image (with uid == 0)
does nothing when running the rootless image with uid == 1000 because the file is already owned by uid 1000
returns an error when running the rootless image with uid > 0 and uid != 1000 because the file is owned by the current user (so ownership is correct) but we are asking to give ownership to uid 1000 (forbidden by Linux kernel)

The new proposed behavior :

changes file ownership when running the rootful image (with uid == 0)
does nothing when running the rootless image (with uid > 0) since by design the Linux kernel prevents us from using chown

To accomplish this, I was using the .Values.image.rootless flag since:

this behavior is used a few lines above in the same file (consistency)
the behavior seems to be the correct one (correctness)

Let me know if something is unclear or if I'm wrong on some point.

> For some reasons I haven't noticed that in the previous PR #259. Yes, I should have included this in the previous PR since both topics are tied. I will be more careful for my next PR. :) We need to handle two different cases: - when using the rootful image and running as uid == 0 - when using the rootless image and running as uid > 0 When using the rootless image, all processes are running as the specified uid (1000 by default but customizable). And all created files and folders are necessarily owned by that uid. Also, with uid > 0, we **cannot** use the chown command. The Linux kernel prevents any user from: - giving ownership of their files to others - taking ownership of other's files For instance : ``` $ id uid=1000(nicolas) gid=1000(nicolas) groups=1000(nicolas) $ chown nobody:nobody $HOME/Desktop chown: changing ownership of '/home/nicolas/Desktop': Operation not permitted $ chown $UID:$GID /tmp chown: changing ownership of '/tmp': Operation not permitted ``` The only special case where we can use chown is when the user already owns the file. It is a "noop": nothing is actually done. For instance : ``` $ id uid=1000(nicolas) gid=1000(nicolas) groups=1000(nicolas) $ ls -ld $HOME/Desktop drwx------. 1 nicolas nicolas 552 Dec 10 22:31 /home/nicolas/Desktop $ chown nicolas:nicolas $HOME/Desktop $ echo $? 0 $ ls -ld $HOME/Desktop drwx------. 1 nicolas nicolas 552 Dec 10 22:31 /home/nicolas/Desktop ``` The previous behavior in the Helm chart was *unconditionnally "chown 1000:1000 /tmp/gitea"* which: - changes file ownership when running the rootful image (with uid == 0) - does nothing when running the rootless image with uid == 1000 because the file is already owned by uid 1000 - returns an error when running the rootless image with uid > 0 and uid != 1000 because the file is owned by the current user (so ownership is correct) but we are asking to give ownership to uid 1000 (forbidden by Linux kernel) The new proposed behavior : - changes file ownership when running the rootful image (with uid == 0) - does nothing when running the rootless image (with uid > 0) since by design the Linux kernel prevents us from using chown To accomplish this, I was using the .Values.image.rootless flag since: - this behavior is used a few lines above in the same file (consistency) - the behavior seems to be the correct one (correctness) Let me know if something is unclear or if I'm wrong on some point.

nmasse-itix commented

2021-12-21 15:05:24 +00:00

(Migrated from gitea.com)

this behavior is used a few lines above in the same file (consistency)

I was refering to line 24-26 of templates/gitea/init.yaml:

    {{- if not .Values.image.rootless }}
    chown 1000:1000 /data
    {{- end }}

> this behavior is used a few lines above in the same file (consistency) I was refering to line 24-26 of templates/gitea/init.yaml: ``` {{- if not .Values.image.rootless }} chown 1000:1000 /data {{- end }} ```

justusbunsi commented

2021-12-21 15:29:34 +00:00

(Migrated from gitea.com)

Let me know if something is unclear or if I'm wrong on some point.

Your comment covers every possibility I can think of atm. ?
Are you going to modify your PR to take all this into account?

(BTW, really like the way you are structuring your PR and issue descriptions).

> Let me know if something is unclear or if I'm wrong on some point. Your comment covers every possibility I can think of atm. ? Are you going to modify your PR to take all this into account? (BTW, really like the way you are structuring your PR and issue descriptions).

luhahn commented

2021-12-22 16:26:50 +00:00

(Migrated from gitea.com)

@nmasse-itix we would like to include this PR in 5.0.0 would it be possible to make the changes mentioned above?

nmasse-itix commented

2021-12-22 16:38:06 +00:00

(Migrated from gitea.com)

@luhanh I may have missed something but the PR already implements the described behavior.

When using a rootless image, no chown is executed.
And when using rootful image, chown is executed.

Do you want me to add a comment in the code to clarify why chown operations need to be conditional ?

@luhanh I may have missed something but the PR already implements the described behavior. When using a rootless image, no chown is executed. And when using rootful image, chown is executed. Do you want me to add a comment in the code to clarify why chown operations need to be conditional ?

luhahn commented

2021-12-22 16:44:31 +00:00

(Migrated from gitea.com)

Sorry, no you've not missed something, I just missunderstood the previous conversation :D

justusbunsi commented

2021-12-22 16:54:19 +00:00

(Migrated from gitea.com)

@nmasse-itix I thought you'd make the chown itself using dynamic uid:gid values, depending on the configuration, as it now would fail under some conditions when actually changing the user with securityContexts. That's what I was reading from your larger comment. But I may have just interpreted this. ?

For 5.0.0 I can totally live with the proposed change as-is. We can provide another change with 5.0.1. Changing the uid was not possible before and I don't expect many users to switch their uid for running environments. ?

In any case, please update your branch to match the current base. Let me know how we want to proceed.

@nmasse-itix I thought you'd make the chown itself using dynamic uid:gid values, depending on the configuration, as it now would fail under some conditions when actually changing the user with securityContexts. That's what I was reading from your larger comment. But I may have just interpreted this. ? For 5.0.0 I can totally live with the proposed change as-is. We can provide another change with 5.0.1. Changing the uid was not possible before and I don't expect many users to switch their uid for running environments. ? In any case, please update your branch to match the current base. Let me know how we want to proceed.

nmasse-itix commented

2021-12-22 17:16:41 +00:00

(Migrated from gitea.com)

From all the tests I've done, this PR covers the following cases:

deploying the rootful image without securityContext.runAsUser/runAsGroup (uid == 0)
deploying the rootless image without securityContext.runAsUser/runAsGroup (uid == 1000)
deploying the rootless image with securityContext.runAsUser/runAsGroup (uid > 0 and != 1000)

Of course, there is always room for improvement. :)

edit: PR rebased on current master

From all the tests I've done, this PR covers the following cases: - deploying the rootful image without securityContext.runAsUser/runAsGroup (uid == 0) - deploying the rootless image without securityContext.runAsUser/runAsGroup (uid == 1000) - deploying the rootless image with securityContext.runAsUser/runAsGroup (uid > 0 and != 1000) Of course, there is always room for improvement. :) edit: PR rebased on current master

nmasse-itix commented

2021-12-22 17:45:07 +00:00

(Migrated from gitea.com)

I think we can move on with this PR. If there are new use cases or if we want to refine the init_directory_structure.sh script, we can do it in another milestone.

What do you think ?

I think we can move on with this PR. If there are new use cases or if we want to refine the init_directory_structure.sh script, we can do it in another milestone. What do you think ?

justusbunsi commented

2021-12-22 18:34:07 +00:00

(Migrated from gitea.com)

You are right. The init container will proceed with your fix for all three cases. It seems we have issues with the rootless image not running as 1000:1000 in general. I tried it with 2000:2000 and got this at Gitea startup.

mkdir: can't create directory '/var/lib/gitea/git': Permission denied
/var/lib/gitea/git is not writable
docker setup failed

Here is my securityContext related values.yaml:

podSecurityContext:
  runAsUser: 2000
  runAsGroup: 2000
  fsGroup: 2000

containerSecurityContext:
  allowPrivilegeEscalation: false
  capabilities:
    drop:
      - ALL
  privileged: false
  readOnlyRootFilesystem: true
  runAsGroup: 2000
  runAsNonRoot: true
  runAsUser: 2000

Even setting environment variables to overrwide UID/GID for the git user didn't work.

statefulset:
  env:
    - name: USER_UID
      value: "2000"
    - name: USER_GID
      value: "2000"

You are right. The init container will proceed with your fix for all three cases. It seems we have issues with the rootless image not running as 1000:1000 in general. I tried it with 2000:2000 and got this at Gitea startup. ```plain mkdir: can't create directory '/var/lib/gitea/git': Permission denied /var/lib/gitea/git is not writable docker setup failed ``` Here is my securityContext related `values.yaml`: ```yaml podSecurityContext: runAsUser: 2000 runAsGroup: 2000 fsGroup: 2000 containerSecurityContext: allowPrivilegeEscalation: false capabilities: drop: - ALL privileged: false readOnlyRootFilesystem: true runAsGroup: 2000 runAsNonRoot: true runAsUser: 2000 ``` Even setting environment variables to overrwide UID/GID for the git user didn't work. ```yaml statefulset: env: - name: USER_UID value: "2000" - name: USER_GID value: "2000" ```

nmasse-itix commented

2021-12-22 18:38:32 +00:00

(Migrated from gitea.com)

Yes, that directory in the rootless image is not writable.

You can choose another one in your values.yaml like this:

statefulset:
  env:
  # Override the home directory of the git user since the default one
  # (/var/lib/gitea/git) is not writable.
  - name: HOME
    value: /data/git

Yes, that directory in the rootless image is not writable. You can choose another one in your values.yaml like this: ```yaml statefulset: env: # Override the home directory of the git user since the default one # (/var/lib/gitea/git) is not writable. - name: HOME value: /data/git ```

luhahn (Migrated from gitea.com) approved these changes 2021-12-23 10:46:33 +00:00

luhahn (Migrated from gitea.com) left a comment

LGTM

justusbunsi (Migrated from gitea.com) approved these changes 2021-12-23 10:48:14 +00:00

justusbunsi (Migrated from gitea.com) left a comment

LGTM

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: lunny/helm-chart#266