Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docker: rootless image #10154

Merged
merged 5 commits into from
Nov 1, 2020
Merged

Conversation

sapk
Copy link
Member

@sapk sapk commented Feb 5, 2020

Replace #7129

This solution propose a new docker image tag -rootless with a image using internal ssh running with an unprivileged user.

The main advantage is to provide a breaking solution that users can transition slowly and when adoption and returns are good should become the default.

This will allow us to break past mistake/compatibility for docker image.

How to build image docker build -f Dockerfile.rootless -t gitea/gitea:latest-rootless .
How to migrate/test
  • Backup your setup
  • Change volume mountpoint from /data to /var/lib/gitea
  • If you used a custom app.ini move it to a new volume mounted to /etc/gitea
  • Rename folder (inside volume) gitea to custom
  • chown -R uid:gid /data volume of --user flag if needed
  • Edit app.ini if needed
    • Set START_SSH_SERVER = true
  • Use image gitea/gitea:latest-rootless (when merged or use sapk/gitea:rootless for testing)

@sapk sapk added topic/deployment pr/wip This PR is not ready for review topic/distribution This PR changes something about the packaging of Gitea labels Feb 5, 2020
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 6, 2020
@codecov-io
Copy link

codecov-io commented Feb 6, 2020

Codecov Report

Merging #10154 into master will increase coverage by 0.03%.
The diff coverage is 55.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10154      +/-   ##
==========================================
+ Coverage   42.14%   42.18%   +0.03%     
==========================================
  Files         690      690              
  Lines       75871    75888      +17     
==========================================
+ Hits        31975    32010      +35     
+ Misses      38668    38645      -23     
- Partials     5228     5233       +5     
Impacted Files Coverage Δ
modules/context/api.go 75.22% <ø> (ø)
routers/api/v1/repo/release_tags.go 53.57% <46.66%> (-7.97%) ⬇️
routers/api/v1/api.go 79.50% <100.00%> (+0.05%) ⬆️
modules/charset/charset.go 68.53% <0.00%> (-6.75%) ⬇️
services/gitdiff/gitdiff.go 67.39% <0.00%> (-1.99%) ⬇️
modules/git/repo.go 46.70% <0.00%> (-1.02%) ⬇️
modules/notification/webhook/webhook.go 49.65% <0.00%> (+0.34%) ⬆️
services/pull/pull.go 41.27% <0.00%> (+0.49%) ⬆️
models/error.go 36.56% <0.00%> (+0.83%) ⬆️
models/notification.go 66.66% <0.00%> (+0.90%) ⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5e974c...f35a3ef. Read the comment docs.

@sapk sapk removed the pr/wip This PR is not ready for review label Feb 6, 2020
@lafriks lafriks added this to the 1.12.0 milestone Feb 6, 2020
Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine to me, I guess one thing I might suggest is to put a note in the app.ini stating that only internal SSH is supported on rootless installs

@techknowlogick
Copy link
Member

Speaking of “breaking” items, maybe we make the binary in this image FHS compliant?

@techknowlogick
Copy link
Member

This looks great! Thanks @sapk for your work on this.

In addition to @zeripath’s review could you add a small section to our documentation’s docker section re:rootless?

@sapk
Copy link
Member Author

sapk commented Feb 9, 2020

For FHS, I am hesitating for the binary path:

  • 👍 /usr/local/bin/gitea
  • 👎 /usr/lib/gitea/gitea
  • 😄 /usr/bin/gitea

Since we don't have any file I would be in favor to put the binary directly to /usr/local/bin/gitea

@zeripath
Copy link
Contributor

zeripath commented Feb 9, 2020

I think /usr/bin would even be appropriate as it's a pre-installed package

@sapk
Copy link
Member Author

sapk commented Feb 9, 2020

@zeripath I have looked at docker-library official image and they tend to kept under /usr/local/bin everything that are not package managed (apt, apk, ...).
I will try to follow that and also move entrypoint and setup scripts there.
Ex for redis: https://github.com/docker-library/redis/blob/master/5.0/alpine/Dockerfile#L49

root@2a327f2bc55e:/data# ls /usr/local/bin/
docker-entrypoint.sh  gosu  redis-benchmark  redis-check-aof  redis-check-rdb  redis-cli  redis-sentinel  redis-server

@sapk
Copy link
Member Author

sapk commented Feb 9, 2020

The only part that still isn't FHS compliant is that the config file is currently under /var/lib/gitea/custom/conf/app.ini but I hesitate to create a second volume only for the config file.

@lunny
Copy link
Member

lunny commented Feb 13, 2020

@sapk, create a link?

@lunny
Copy link
Member

lunny commented Feb 13, 2020

And I think we could also create some necessary envs to skip the installation guide which will override app.ini. Maybe another PR.

Dockerfile.rootless Outdated Show resolved Hide resolved
@sapk
Copy link
Member Author

sapk commented Feb 13, 2020

Rethinking of the config file for FHS, It maybe a good idea to separate it from the data folder since in most cases it is not needed to backup it and can be generated at each startup of container. The config file could still be backup but it would need to be explicitly defined (with an other volume) if needed. An other advantage is that someone that want to configure via variable would just need to restart the container and not rewrite the config file.

@sapk
Copy link
Member Author

sapk commented Feb 17, 2020

I will re-add the wip label since I will make change that need to be retested.

@sapk sapk added the pr/wip This PR is not ready for review label Feb 17, 2020
Dockerfile.rootless Outdated Show resolved Hide resolved
docker/rootless/etc/templates/app.ini Outdated Show resolved Hide resolved
@sapk sapk added pr/wip This PR is not ready for review and removed pr/wip This PR is not ready for review labels Mar 20, 2020
@lafriks lafriks removed this from the 1.12.0 milestone May 16, 2020
@zeripath
Copy link
Contributor

So can we get this merged at some point?

@sapk
Copy link
Member Author

sapk commented Oct 31, 2020

I will intentionally not backport this commit ff50274#diff-dd2c0eb6ea5cfc6c4bd4eac30934e2d5746747af48fef6da689e85b752f39557
As I think, socat is not needed for 99% of gitea users and could easily be added by the at most 1% left. socat can be leveraged to open two-way connection to keep open a remote shell. Ex: https://gtfobins.github.io/gtfobins/socat/
I don't think it is a big security risk but we shouldn't make it more easy by default.

@sapk sapk removed the pr/wip This PR is not ready for review label Oct 31, 2020
@sapk
Copy link
Member Author

sapk commented Oct 31, 2020

This PR is ready for review.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 31, 2020
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 31, 2020
@lafriks
Copy link
Member

lafriks commented Oct 31, 2020

Please update with base branch

@techknowlogick techknowlogick merged commit fe458ce into go-gitea:master Nov 1, 2020
@sapk sapk deleted the rootless-docker-image branch November 1, 2020 01:12
@Grepsd
Copy link

Grepsd commented Dec 14, 2020

I've run into an error when trying to install the helm chart on my local minikube, runAsNonRoot issues.

After using the rootless images, the error is now : Error: container has runAsNonRoot and image has non-numeric user (git), cannot verify user is non-root

This happens because I use pod security policies.

Is there any solution beside replacing USER git:git (if it does even work) in the rootless docker image ?

@techknowlogick
Copy link
Member

@Grepsd please don't comment on closed issues/PRs as your comment will be lost. Please reach out on discord or discourse for support.

@go-gitea go-gitea locked and limited conversation to collaborators Dec 14, 2020
@sapk
Copy link
Member Author

sapk commented Jan 4, 2021

@Grepsd I am not against using numeric id. The fact that your tool cannot verify the user is not root is strange since it is how it is done in most docker official image :

If you want to discuss more, create a issue on the subject or come on discord

@wxiaoguang
Copy link
Contributor

Please help to take a look at this one: Fix the mode of custom dir to 0700 in docker-rootless

@delvh delvh removed the theme/docker label Oct 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/deployment topic/distribution This PR changes something about the packaging of Gitea
Projects
None yet
Development

Successfully merging this pull request may close these issues.