[WIP] Rootless docker #7129

sapk · 2019-06-05T05:45:26Z

Following #4749

Currently work with the default user git pid (1000)

The main blocking adjustment to use the --user docker flag to adjust running user is various check of the user by gitea but also by sub process like ssh-keygen in #7128

lunny · 2019-06-05T05:54:22Z

docker/root/usr/bin/setup

@@ -1,5 +1,6 @@
 #!/bin/bash

+#TODO maybe /data/git/.ssh not needed anymore


If docker user internal SSH server, it's not needed

While it is not needed for the single process setup, removing it would break existing setups.

@das7pad yes but it is for the setup part so if we don't need it anymore we don't need to create it if it doesn't exist.

For folks who backup the entire /data/git directory it is not a problem. But for those who just backup non-generated files, read only the repositories directory, the .ssh directory would be missing upon restore.

But the internal sshserver doesn't use the .ssh directory...

I was thinking about the third example in #7129 (review).
I know that the files in the .ssh directory are only needed to get the spawned sshd-childs talk with the correct gitea instance.

codecov-io · 2019-06-05T06:17:23Z

Codecov Report

Merging #7129 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #7129      +/-   ##
==========================================
- Coverage   41.57%   41.57%   -0.01%     
==========================================
  Files         446      446              
  Lines       60848    60841       -7     
==========================================
- Hits        25299    25295       -4     
+ Misses      32265    32264       -1     
+ Partials     3284     3282       -2

Impacted Files	Coverage Δ
models/ssh_key.go	`46.01% <100%> (+0.2%)`	⬆️
modules/setting/setting.go	`48.51% <100%> (+0.17%)`	⬆️
modules/log/log.go	`69.36% <0%> (-2.71%)`	⬇️

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 5d3177d...ba98261. Read the comment docs.

das7pad

This breaks almost every usage of the docker image.

Some broken usage examples:

all of the 'official' docker-compose configs
https://docs.gitea.io/en-us/install-with-docker/
docker run -d -p 80:3000 -p 2222:22 gitea/gitea
running sshd in the container to get the gitea binary
- run gitea with git user
- run sshd with root

das7pad · 2019-06-15T11:04:05Z

docker/root/usr/bin/setup

-if ! [[ $(ls -ld /data/git   | awk '{print $3}') = ${USER} ]]; then chown -R ${USER}:git /data/git;   fi
-chmod 0755 /data/gitea /app/gitea /data/git
+chmod 0755 /data/gitea /data/git
+#chmod 0755 /app/gitea


/app/gitea is owned by root.

Suggested change

#chmod 0755 /app/gitea

Yes and I don't think it is a problem. I will recheck but that a good point that the process inside the containre couldn't change content inside /app/gitea

das7pad · 2019-06-15T11:11:23Z

models/ssh_key.go

-		return "", errors.New("not enough output for calculating fingerprint: " + stdout)
-	}
-	return strings.Split(stdout, " ")[1], nil
+	return ssh.FingerprintSHA256(pk), nil


This seems to be an unrelated change. Can we move the change into a new PR?

It is a needed change because openssh does un-needed system check (that fail in this configuration with --user flag) for calculating the fingerprint of the key. I allready separate this in an other PR #7128

What system check is failing for you?

I setup a test with alpine and the --user flag, but can not see any error.

https://gist.github.com/das7pad/62b58bfb67da516542992d8bf5744446

If you pass an uid/gid (numerical) that doesn't exist inside the container it should failed.

das7pad · 2019-06-15T11:14:02Z

modules/setting/setting.go

@@ -436,7 +436,7 @@ func forcePathSeparator(path string) {
 // This check is ignored under Windows since SSH remote login is not the main
 // method to login on Windows.
 func IsRunUserMatchCurrentUser(runUser string) (string, bool) {
-	if IsWindows {
+	if IsWindows || SSH.StartBuiltinServer {


Seems to be a bug fix? Let's move it into a new PR.

Done in #7215

das7pad · 2019-06-15T11:17:41Z

docker/root/usr/bin/setup

@@ -1,5 +1,6 @@
 #!/bin/bash

+#TODO maybe /data/git/.ssh not needed anymore


While it is not needed for the single process setup, removing it would break existing setups.

das7pad · 2019-06-15T11:56:31Z

How about adding a new image variant for the proposed changes?

For example gitea/gitea:root-less, gitea/gitea:1-root-less, gitea/gitea:1.8-root-less

This would allow any existing deployment to stay as is. And folks which require additional hardening of the container can use the new variant.

With a reasonable delay we can eventually swap the default tag: gitea/gitea:root-less-> gitea/gitea:latest.

sapk · 2019-06-15T23:43:09Z

@das7pad the docs will need to be updated to follow some changed needed to the run command in some cases but it should be able to reuse the same data folder.
I like the idea to have a transition tag to not enforce this rootless image at start.

das7pad

Manual ownership changes to the ssh directory in the data volume are needed.

das7pad · 2019-06-16T12:37:32Z

docker/root/etc/s6/openssh/setup

-
-chown root:root /data/ssh/*
-chmod 0700 /data/ssh
-chmod 0600 /data/ssh/*


The ssh-hostkeys are owned by root and locked down to root only read access. The internal ssh-server will not be able to read them as git user.

stale · 2019-08-17T03:16:28Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

westurner · 2020-02-04T08:09:39Z

Rather than try and maintain/rebase this separate branch, would it be easier to:

leave the existing config in
check for an environment variable like GITEA_ROOTLESS in the setup and entrypoint scripts
create a templates/app_rootless.ini

sapk · 2020-02-04T08:55:48Z

@westurner I would suggest more to define a rootless tagged image that could become the main later but would ease the transition and allow us to break some pattern existing in the current image.

alexanderadam · 2020-02-04T09:23:38Z

@westurner I would suggest more to define a rootless tagged image that could become the main later but would ease the transition and allow us to break some pattern existing in the current image.

I like this idea.

When the rootless image is working (i.e. two months without new issues on the rootless image) the 'legacy root' image could show a warning in the frontend (i.e. Hey, we are now switching to a proper rootless image. You should migrate, too. Click here for more information on how to migrate).

sapk · 2020-02-04T09:32:43Z

Yes and this transition duration would help us write a migration guide (like changing owner, possible change of key signature of ssh server, ...)

sapk · 2020-02-05T16:59:56Z

I will send another PR that will replace this one for a image tagged rootless

sapk · 2020-02-05T20:49:30Z

The fixes related to help remove external deps (openssh) are already merged (in separe PR) and the rootless part of this PR is moved to a new PR (non breaking by using an other tag -rootless)

sapk added 2 commits June 5, 2019 05:38

First step to rootless docker

dd8601d

...

ba98261

lunny reviewed Jun 5, 2019

View reviewed changes

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 5, 2019

sapk mentioned this pull request Jun 5, 2019

Don't run docker image as root #1190

Closed

techknowlogick added topic/deployment topic/distribution This PR changes something about the packaging of Gitea labels Jun 9, 2019

das7pad suggested changes Jun 15, 2019

View reviewed changes

sapk mentioned this pull request Jun 15, 2019

Disable same user check for internal SSH #7215

Merged

das7pad suggested changes Jun 16, 2019

View reviewed changes

Merge branch 'master' into rootless-docker

aa8cb98

stale bot added the issue/stale label Aug 17, 2019

lunny added the issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented label Aug 17, 2019

stale bot removed the issue/stale label Aug 17, 2019

sapk mentioned this pull request Feb 5, 2020

docker: rootless image #10154

Merged

sapk closed this Feb 5, 2020

go-gitea locked and limited conversation to collaborators Nov 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Rootless docker #7129

[WIP] Rootless docker #7129

sapk commented Jun 5, 2019

lunny Jun 5, 2019

das7pad Jun 15, 2019

sapk Jun 15, 2019

das7pad Jun 16, 2019

zeripath Jun 16, 2019

das7pad Jun 16, 2019

codecov-io commented Jun 5, 2019

das7pad left a comment

das7pad Jun 15, 2019

sapk Jun 15, 2019 •

edited

Loading

das7pad Jun 15, 2019

sapk Jun 15, 2019

das7pad Jun 16, 2019

sapk Jun 16, 2019

das7pad Jun 15, 2019

sapk Jun 15, 2019

das7pad Jun 15, 2019

das7pad commented Jun 15, 2019

sapk commented Jun 15, 2019

das7pad left a comment

das7pad Jun 16, 2019

stale bot commented Aug 17, 2019

westurner commented Feb 4, 2020

sapk commented Feb 4, 2020

alexanderadam commented Feb 4, 2020

sapk commented Feb 4, 2020

sapk commented Feb 5, 2020

sapk commented Feb 5, 2020

		@@ -1,5 +1,6 @@
		#!/bin/bash

		#TODO maybe /data/git/.ssh not needed anymore

[WIP] Rootless docker #7129

[WIP] Rootless docker #7129

Conversation

sapk commented Jun 5, 2019

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

codecov-io commented Jun 5, 2019

Codecov Report

das7pad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sapk Jun 15, 2019 • 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

das7pad commented Jun 15, 2019

sapk commented Jun 15, 2019

das7pad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Aug 17, 2019

westurner commented Feb 4, 2020

sapk commented Feb 4, 2020

alexanderadam commented Feb 4, 2020

sapk commented Feb 4, 2020

sapk commented Feb 5, 2020

sapk commented Feb 5, 2020

sapk Jun 15, 2019 •

edited

Loading