Skip to content

Dockerfile: upgrade to git >= 2.36 and set safe.directory = * #19707

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

Closed
wants to merge 3 commits into from

Conversation

singuliere
Copy link
Contributor

@singuliere singuliere commented May 14, 2022

@@ -206,6 +206,12 @@ func Init(ctx context.Context) error {
SupportProcReceive = false
}

if CheckGitVersionAtLeast("2.36") == nil {
if err := checkAndSetConfig("safe.directory", "*", true); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should have a comment here, explaining why this is chosen and the rationale of it being safe.

Otherwise I would already see the comments/issues/questions pouring in. of why we're doing this and why we're deliberately making Gitea insecure.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like a windows only issue - so can we only set this on windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is described for windows but is not windows specific:

/tmp/a/b$ strace -e stat git log
stat("/usr/share/locale", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
stat("/tmp/a/b", {st_mode=S_IFDIR|0775, st_size=4096, ...}) = 0
stat("/tmp/a/b/.git", 0x7ffe70098e60)   = -1 ENOENT (No such file or directory)
stat("/tmp/a", {st_mode=S_IFDIR|0775, st_size=4096, ...}) = 0
stat("/tmp/a/.git", 0x7ffe70098e60)     = -1 ENOENT (No such file or directory)
stat("/tmp", {st_mode=S_IFDIR|S_ISVTX|0777, st_size=139264, ...}) = 0
stat("/tmp/.git", 0x7ffe70098e60)       = -1 ENOENT (No such file or directory)
stat("/", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
stat("/.git", 0x7ffe70098e60)           = -1 ENOENT (No such file or directory)
fatal: not a git repository (or any of the parent directories): .git
+++ exited with 128 +++

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 14, 2022
@zeripath
Copy link
Contributor

I'm really not convinced about this. This problem is a configuration issue with people's Dockerfiles. Gitea needs to be the owner of its repositories.

@singuliere
Copy link
Contributor Author

Please see the rationale here.

@singuliere
Copy link
Contributor Author

I'm really not convinced about this. This problem is a configuration issue with people's Dockerfiles. Gitea needs to be the owner of its repositories.

I agree. There appear to be two different but related problems. One has a simple fix. The other is not yet fully understood and has been going on for a long time, apparently.

@singuliere
Copy link
Contributor Author

@Gusted added the suggested comment.

@techknowlogick
Copy link
Member

Copying my comment from elsewhere: I’m concerned about it targeting edge, could the appropriate version of git be backported to alpine 3.15?

@wxiaoguang
Copy link
Contributor

wxiaoguang commented May 15, 2022

This PR is incorrect, do not pollute user's git config file, otherwise the globally polluted option changes git's security protection and leaves users in risk.

I think I have said very clearly in the original issue.

image

image

image

@wxiaoguang
Copy link
Contributor

wxiaoguang commented May 15, 2022

I'm really not convinced about this. This problem is a configuration issue with people's Dockerfiles. Gitea needs to be the owner of its repositories.

@zeripath these comments may answer your question.
#19455 (comment)
#19455 (comment)

Copied some content here:

In fact, if everything is correct, the files in the mounted volume should be owned by 1000:1000 inside docker and can not be changed on Windows (host volume). I don't know what happens to the users who meet the problem at the moment. And just like what I have said many times, not only docker users, but also samba (and maybe more) users could be affected by this problem, so make Gitea use customized config file is the proper way to handle this problem at the moment if users are not able to chown the files to a correct owner.

(ps: a doctor command can be added to fix the chown/chmod for Gitea's data directory and help to check if the fix really works.)

And more information (FYI), docker desktop for macOS uses xattr (com.docker.grpcfuse.ownership: {"UID":1000,"GID":1000}) to store uid and gid. It may answer lunny 's question: if your xattr of the directory/file is broken/incorrect, then the uid/gid inside docker could be incorrect.

And

Even if the author's docker problem can be fixed by some methods luckily (maybe chown works again), there are others environments mentioned in this issue thread (affected by safe.directory). I admit that some problems are caused by mistakes from users side, but users would only complain Gitea if there is no clear solution.

@singuliere
Copy link
Contributor Author

@wxiaoguang A gentle reminder that this PR originates from Loïc Dachary.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented May 15, 2022

Oh fine, let him ignore my comments. My comments are written for maintainers, not for him. I won't do anything to him, let him relax.


Update: to explain more, since there are a lot of noises in #19455, so I just copied my comments which I think should be useful here, to answer zeripath's question, and for others to refer.

@singuliere
Copy link
Contributor Author

@wxiaoguang please do not change the state of my comments without my consent. You may think it is off-topic and maybe I agree with you but please ask me first instead of doing that unilaterally.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented May 15, 2022

Fine. May I mark these 4 comments + 1 more your reply as off-topic?

@lunny
Copy link
Member

lunny commented May 15, 2022

This PR ignored to figure out the real reason why there are files owned by root but not git and just allow mixed owned directories and files. I think it's only a temporary fix for the problem. If it's not related security, I think maybe we can hold on it to spend more time to find out the root cause.

I like @wxiaoguang 's idea which is a final resolution to change the git configuration. But that idea looks like it's not conflicted with this one.

Off topic, I think there is no reason to refuse a maintainer to review the PR if he has a real and reasonable review.
Hiding a comment as off topic is also a permission of maintainers, if you think that's not off topic you could have a following comment and post your reason.

If every action of maintainers need poster or commenter's allow, it will make maintainers work difficult.

@singuliere
Copy link
Contributor Author

This fix looks good to me, but I'm only the intermediary. Since there is a disagreement I don't feel I can keep relaying this pull requests.

@singuliere singuliere closed this May 16, 2022
@wxiaoguang wxiaoguang removed this from the 1.17.0 milestone Jun 12, 2022
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New git security policy safe.directory (eg: 1.16.6 docker image) makes pushing fails
8 participants