Add the .git subdir as another safe.directory on Cygwin CI

[EliahKagan]

As shown in this run on my fork, the CI test job for Cygwin is failing now. This fixes that.

The first commit in this pull request just demonstrates that the failure is due to the upgrade of the Cygwin git package from 2.43.0-1 to 2.45.1-1. Although that commit makes test pass, I recommend against following that approach, mainly because the new version contains multiple security updates (coming in with the upstream 2.45.1 version, not with any downstream patches), but also because the older version will eventually be dropped from the Cygwin repositories.

The better workaround is in the second commit here, which adds the .git subdirectory of the cloned GitPython directory as a value of the multi-valued safe.directory Git configuration variable. Its parent directory is already added, which was previously sufficient, but not anymore.

I suspect that, rather than this being any bug in the downstream package, this is actually the correct behavior due to one of the several security fixes in the new version of Git, though I have not verified that. So maybe this is really not a workaround, but a permanent fix.

I believe the reason there is no need to modify any other workflow is that the other workflows don't need to add any safe.directory paths at all: they either clone the repository with the necessary ownership in the first place or, in the case of the Alpine Linux job, set the ownership with chown.

[Byron]

Thanks a lot!

Seeing the change to safe.directory made me wonder if this is indeed a change in how it works and if gitoxide is affected.

And indeed, 22 months ago I see no evidence of it.

This line tells us it will check the git-dir/repository-directory if there is no worktree, but it's certainly strange that a wt/.git is a clone that is bare, it's an unusual name.

Anyway, it's good this is fixed and I don't worry about it.

[EliahKagan]

I'm still not totally clear what's going on here. Some of the changes in 2.45.1 were judged overzealous and undone in 2.45.2. But don't know if that has anything to do with this. Anyway, git 2.45.2 is not yet in the Cygwin repositories, and I'm not sure it will be since it's not a security fix (so it might be skipped over, with some future version being packaged next).

[EliahKagan]

This line tells us it will check the git-dir/repository-directory if there is no worktree, but it's certainly strange that a wt/.git is a clone that is bare, it's an unusual name.

That specific fragment is:

struct safe_directory_data data = {
	.path = worktree ? worktree : gitdir
};

But that is only used for this:

/*
 * data.path is the "path" that identifies the repository and it is
 * constant regardless of what failed above. data.is_safe should be
 * initialized to false, and might be changed by the callback.
 */
git_protected_config(safe_directory_cb, &data);

return data.is_safe;

However, that code is just for figuring out which path to look for as a value of the safe.directory configuration variable. (It makes sense that you were looking at that: it's the part that is directly relevant to what I was saying about the safe.directory variable in the PR description above! But I think the change may be subtler than that, and may not directly involve a change to that.)

In between the two is the ownership check, which does not reference data:

if (!git_env_bool("GIT_TEST_ASSUME_DIFFERENT_OWNER", 0) &&
	(!gitfile || is_path_owned_by_current_user(gitfile, report)) &&
	(!worktree || is_path_owned_by_current_user(worktree, report)) &&
	(!gitdir || is_path_owned_by_current_user(gitdir, report)))
	return 1;

While none of this code has changed around the same time as the breakage that this PR fixed, nor recently, I do not know if the safe.directory checking logic implemented through git_protected_config changed, though I would guess it has not--I think it's just doing a straightforward comparisong to values of safe.directory where it is defined in protected (not local or worktree) scopes. I also don't know if the way the effects of call that are used later changed.

More likely relevant, I think, is that I am also unsure if any of the gitfile, worktree, and gitdir parameters are passed as non-NULL values where they had been NULL before.

So while it seems like maybe the need to add the .git directory explicitly is due to some oddity of Cygwin paths, I am not sure. Likewise, I cannot tell directly from this if any recent changes to safe.directory-related checks in Git would have any implications for gitoxide.

---

I am wondering if some GitPython tests run commands in the .git directory of a non-bare repository.

At least with current versions of Git, this requires that the .git directory specifically be owned by the current user or listed as a value of the safe.directory variable (in a protected scope), even if the working tree is a parent directory of that .git directory and git will operate on the working tree itself due to it being listed explicitly in safe.directory:

ek@Kip:~/src$ git version
git version 2.48.1
ek@Kip:~/src$ git init ownership-experiment
Initialized empty Git repository in /home/ek/src/ownership-experiment/.git/
ek@Kip:~/src$ sudo chown -R ek2:ek2 ownership-experiment
[sudo] password for ek:
ek@Kip:~/src$ ls -ld ownership-experiment ownership-experiment/.git
drwxrwxr-x 3 ek2 ek2 4096 Feb 20 15:45 ownership-experiment
drwxrwxr-x 6 ek2 ek2 4096 Feb 20 15:45 ownership-experiment/.git
ek@Kip:~/src$ git -C ownership-experiment status
fatal: detected dubious ownership in repository at '/home/ek/src/ownership-experiment'
To add an exception for this directory, call:

        git config --global --add safe.directory /home/ek/src/ownership-experiment
ek@Kip:~/src[128]$ git config --global --add safe.directory /home/ek/src/ownership-experiment
ek@Kip:~/src$ git -C ownership-experiment status
On branch main

No commits yet

nothing to commit (create/copy files and use "git add" to track)
ek@Kip:~/src$ git -C ownership-experiment/.git status
fatal: detected dubious ownership in repository at '/home/ek/src/ownership-experiment/.git'
To add an exception for this directory, call:

        git config --global --add safe.directory /home/ek/src/ownership-experiment/.git

That was on GNU/Linux, but it is not specific to it (nor to systems with the somewhat rare relaxed 0002 umask value I have set there). It is also not specific to passing the directory to operate on using -C (which makes sense, since -C is implemented by actually changing directory).

That same experiment in Cygwin, where I create the repository with Cygwin git 2.45.1, then outside the Cygwin environment navigate to the working tree in Windows Explorer and recursively change its ownership to another user (which includes changing that of its .git subdirectory, since I do it recursively) has the same effect.

Could this behavior have been what changed? Could the working tree have been sufficient before, even when operating directly in the .git subdirectory?

[Byron]

Thanks for reviving this conversation.

I am entirely unsure how all this relates to GitPython, but know that gitoxide has a completely different security model when it comes to that. It determines the ownership of configuration files so it can trust (or ignore) sections in the configuration (each section has 'trust' attached to it, based on file ownership).

[EliahKagan]

I am entirely unsure how all this relates to GitPython

As far as I know, the only connection to GitPython is through its Cygwin CI workflow, which had to be changed to accommodate this. That is, I am not proposing that a bug in GitPython itself causes this or that anything in the git/ directory should be changed related to it. Rather, I am curious if it is due to something specific to Cygwin. The Cygwin workflow breaks more often than some of the others, so if I can understand more of what is happening in it, then maybe that can be improved or at least handled more easily. To be clear, I do not think this has anything at all to do with the current breakage described in #2004.

but know that gitoxide has a completely different security model when it comes to that. It determines the ownership of configuration files so it can trust (or ignore) sections in the configuration (each section has 'trust' attached to it, based on file ownership).

Thanks--this way of putting it clarifies a comment that puzzled me during something I'm working on in gitoxide!

[Byron]

Thanks for clarifying - indeed I was trying to understand if there was something actionable for GitPython and had/have no memory on how this works here. In that regard I am happy that gitoxide won't have that problem at least as it's generally more open than Git is by default, ideally not any disadvantage.