lunny/helm-chart
1
0
Fork 1
Code Issues 36 Pull Requests 14 Actions Packages Projects Releases 93 Wiki Activity

Fix rootless image usage with enhanced security-context #160

Merged
justusbunsi merged 1 commits from refs/pull/160/head into master 2021-06-07 13:27:26 +00:00
Conversation 9 Commits 1 Files Changed 2 +5 -2
justusbunsi commented 2021-05-15 17:59:18 +00:00 (Migrated from gitea.com)
Copy Link

I've noticed that the commented securityContext is not really useable with the rootless image due to different directory structure compared to the default image.

Important for the readOnlyRootFilesystem is to declare the TMPDIR environment variable, so that the tmp directory (which is readonly in this case) won't be used. Instead, another writeable directory can be used.

Another thing is the explicit hint that all these security options cannot be used with the default (root-based) image, because of its design.

Although this PR would fix the referenced issue, I am not totally happy with the current implementation. It would be more straight forward to use the same mount points for both image variants. Unfortunately, this is not possible right now due to hard coded paths in the default (root) image startup scripts.

Anyone have suggestions on how this could be more simple?

Sum-up:
As mentioned in Discord, this PR tried to make too many changes. The necessary changes made in 1f331a7e6577fc798196a84a957330aca0d663cd will fix an error that occurs due to restricted access to the /tmp directory in a rootless image with all the securityContext options enabled.

I also updated the default image to 1.14.2.

Fixes: #158

I've noticed that the commented `securityContext` is not really useable with the rootless image due to different directory structure compared to the default image. Important for the `readOnlyRootFilesystem` is to declare the `TMPDIR` environment variable, so that the tmp directory (which is readonly in this case) won't be used. Instead, another writeable directory can be used. Another thing is the explicit hint that all these security options cannot be used with the default (root-based) image, because of its design. ~~Although this PR would fix the referenced issue, I am not totally happy with the current implementation. It would be more straight forward to use the same mount points for both image variants. Unfortunately, this is not possible right now due to hard coded paths in the default (root) image startup scripts.~~ ~~Anyone have suggestions on how this could be more simple?~~ ------- **Sum-up:** As mentioned in Discord, this PR tried to make too many changes. The necessary changes made in 1f331a7e6577fc798196a84a957330aca0d663cd will fix an error that occurs due to restricted access to the `/tmp` directory in a rootless image with all the `securityContext` options enabled. I also updated the default image to 1.14.2. Fixes: #158
luhahn commented 2021-05-25 06:41:35 +00:00 (Migrated from gitea.com)
Copy Link

Thanks for the PR.

Will look at your changes this week.

Thanks for the PR. Will look at your changes this week.
justusbunsi commented 2021-05-30 16:52:26 +00:00 (Migrated from gitea.com)
Copy Link

Maybe I found a solution for my mentioned worries regarding the current fix. But it includes a patch for the rootful docker image in the Gitea project. Currently, this root based image has hard coded /data references in its setup scripts and templates. Making this part of the image configurable during runtime by setting the GITEA_WORK_DIR environment variable inside the image to /data allows us to handle both image variants the same way in this helm chart.

I am currently working on these modifications in Gitea and provide a PR; it's useful anyway to allow such configuration ?. After it's merged, my suggested changes for this helm chart will be less complex since both images can use the same paths configured from inside the chart.

What do you say @luhahn? Shall we wait for the necessary modification in Gitea before making any further actions in this PR?

EDIT: Without having it tested yet, this is how this PR could be simplified.

Maybe I found a solution for my mentioned worries regarding the current fix. But it includes a patch for the rootful docker image in the Gitea project. Currently, this root based image has hard coded `/data` references in its setup scripts and templates. Making this part of the image configurable during runtime by setting the `GITEA_WORK_DIR` environment variable inside the image to `/data` allows us to handle both image variants the same way in this helm chart. I am currently working on these modifications in Gitea and provide a PR; it's useful anyway to allow such configuration ?. After it's merged, my suggested changes for this helm chart will be less complex since both images can use the same paths configured from inside the chart. What do you say @luhahn? Shall we wait for the necessary modification in Gitea before making any further actions in this PR? EDIT: Without having it tested yet, [this is how this PR could be simplified](https://gitea.com/justusbunsi/helm-chart/commit/2c5493d0fcc176a3814f7805e7c9a5ad4715dd45).
luhahn commented 2021-06-02 09:45:57 +00:00 (Migrated from gitea.com)
Copy Link

sorry again, our cluster had some major issues, which caused me to have no time at all. I will try to get back to you this week!

sorry again, our cluster had some major issues, which caused me to have no time at all. I will try to get back to you this week!
luhahn (Migrated from gitea.com) reviewed 2021-06-07 07:36:39 +00:00
luhahn (Migrated from gitea.com) left a comment
Copy Link

Hi, I finally got some time to spend on gitea :)

I've got some questions regarding your PR.

Hi, I finally got some time to spend on gitea :) I've got some questions regarding your PR.
luhahn commented 2021-06-07 07:51:39 +00:00 (Migrated from gitea.com)
Copy Link

sorry, should've read your explanation before :D

sorry, should've read your explanation before :D
luhahn (Migrated from gitea.com) approved these changes 2021-06-07 07:57:34 +00:00
luhahn (Migrated from gitea.com) left a comment
Copy Link

Looks good to me, tested on my cluster

Looks good to me, tested on my cluster
6543 (Migrated from gitea.com) approved these changes 2021-06-07 11:52:23 +00:00
6543 commented 2021-06-07 11:53:09 +00:00 (Migrated from gitea.com)
Copy Link

@justusbunsi please "update branch" se we can merge it

@justusbunsi please "update branch" se we can merge it
justusbunsi commented 2021-06-07 12:34:04 +00:00 (Migrated from gitea.com)
Copy Link

@6543 Done.

@6543 Done.
Sign in to join this conversation.
Reviewers
No Reviewers
luhahn
6543
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
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
lunny
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: lunny/helm-chart#160
No description provided.
Delete Branch "refs/pull/160/head"

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?