gitea/helm-chart
45
55
Fork 126
Code Issues 32 Pull Requests 8 Actions Releases 87 Activity

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

Merged
luhahn merged 1 commits from nmasse-itix/helm-chart:no-chown into master 2021-12-23 10:50:57 +00:00
Conversation 12 Commits 1 Files Changed 1 +2
nmasse-itix commented 2021-12-20 17:20:58 +00:00
Contributor
Copy Link

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
Member
Copy Link

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
Author
Contributor
Copy Link

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
Author
Contributor
Copy Link

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
Member
Copy Link

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
Member
Copy Link

@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 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
Author
Contributor
Copy Link

@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
Member
Copy Link

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

Sorry, no you've not missed something, I just missunderstood the previous conversation :D
justusbunsi commented 2021-12-22 16:54:19 +00:00
Member
Copy Link

@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 force-pushed no-chown from fa49489242 to 9c8bcb4ea8 2021-12-22 17:12:15 +00:00 Compare
nmasse-itix commented 2021-12-22 17:16:41 +00:00
Author
Contributor
Copy Link

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
Author
Contributor
Copy Link

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
Member
Copy Link

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" ```
justusbunsi added this to the 5.0.0 milestone 2021-12-22 18:37:27 +00:00
nmasse-itix commented 2021-12-22 18:38:32 +00:00
Author
Contributor
Copy Link

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 approved these changes 2021-12-23 10:46:33 +00:00
luhahn left a comment
Member
Copy Link

LGTM

LGTM
justusbunsi approved these changes 2021-12-23 10:48:14 +00:00
justusbunsi left a comment
Member
Copy Link

LGTM

LGTM
luhahn merged commit d550b5a2c4 into master 2021-12-23 10:50:57 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
luhahn
justusbunsi
Labels
Clear labels   
has
backport

  
in progress

   
invalid
  
kind
breaking

  
kind
bug

  
kind
build

  
kind
dependency

  
kind
deployment

  
kind
docs

  
kind
enhancement

  
kind
feature

  
kind
lint

  
kind
proposal

  
kind
question

  
kind
refactor

  
kind
security

  
kind
testing

  
kind
translation

  
kind
ui

  
need
backport

  
priority
critical

  
priority
low

  
priority
maybe

  
priority
medium

  
reviewed
duplicate

  
reviewed
invalid

  
reviewed
wontfix

  
skip-changelog
  
status
blocked

  
status
needs-feedback

  
status
needs-reviews

  
status
wip

  
upstream
gitea

  
upstream
other

No Label
has
backport
in progress
invalid
kind
breaking
kind
bug
kind
build
kind
dependency
kind
deployment
kind
docs
kind
enhancement
kind
feature
kind
lint
kind
proposal
kind
question
kind
refactor
kind
security
kind
testing
kind
translation
kind
ui
need
backport
priority
critical
priority
low
priority
maybe
priority
medium
reviewed
duplicate
reviewed
invalid
reviewed
wontfix
skip-changelog
status
blocked
status
needs-feedback
status
needs-reviews
status
wip
upstream
gitea
upstream
other
Milestone
Clear milestone
No items
No Milestone
5.0.0
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: gitea/helm-chart#266
No description provided.
Delete Branch "nmasse-itix/helm-chart:no-chown"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?