Some improvements to safe.directory on Windows

Due to the semantics being substantially different from Unix, the safe.directory feature presents its own set of problems on Windows. One particular issue would have prevented it from working in GitHub Actions' build agents, which we definitely rely on in the Git project itself. This was addressed via the first two patches, which had made it already into Git for Windows v2.35.2, and they are ready to be applied to core Git, too.

The third patch came in later, and was released as part of Git for Windows v2.37.0, so I also have confidence that it is stable and ready to be integrated into core Git, too.

Johannes Schindelin (3):
Allow debugging unsafe directories' ownership
mingw: handle a file owned by the Administrators group correctly
mingw: be more informative when ownership check fails on FAT32

 Documentation/config/safe.txt |  6 ++++
 compat/mingw.c                | 53 +++++++++++++++++++++++++++++++++++
 setup.c                       | 14 +++++++--
 3 files changed, 71 insertions(+), 2 deletions(-)

base-commit: 3b0bf2704980b1ed6018622bdf5377ec22289688
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1286%2Fdscho%2Fsafe.directory-and-windows-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1286/dscho/safe.directory-and-windows-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1286

gitgitgadget

From: Johannes Schindelin <[email protected]>

When Git refuses to use an existing repository because it is owned by someone else than the current user, it can be a bit tricky on Windows to figure out what is going on.

Let's help with that by offering some more information via the environment variable GIT_TEST_DEBUG_UNSAFE_DIRECTORIES.

---
 Documentation/config/safe.txt |  6 ++++++
 compat/mingw.c                | 21 +++++++++++++++++++++
 setup.c                       | 14 ++++++++++++--
 3 files changed, 39 insertions(+), 2 deletions(-)

index 74627c5e7c6..18fac9cb7f3 100644
@@ -40,3 +40,9 @@ which id the original user has.
 If that is not what you would prefer and want git to only trust
 repositories that are owned by root instead, then you can remove
 the `SUDO_UID` variable from root's environment before invoking git.
++
+Due to the permission model on Windows where ACLs are used instead of
+Unix' simpler permission model, it can be a bit tricky to figure out why
+a directory is considered unsafe. To help with this, Git will provide
+more detailed information when the environment variable
+`GIT_TEST_DEBUG_UNSAFE_DIRECTORIES` is set to `true`.

index 38ac35913df..912444fb3ab 100644
@@ -1,6 +1,7 @@
 #include "../git-compat-util.h"
 #include "win32.h"
 #include <aclapi.h>
+#include <sddl.h>
 #include <conio.h>
 #include <wchar.h>
 #include "../strbuf.h"

@@ -2676,6 +2677,26 @@ int is_path_owned_by_current_sid(const char *path)
 		    IsValidSid(current_user_sid) &&
 		    EqualSid(sid, current_user_sid))
 			result = 1;
+		else if (git_env_bool("GIT_TEST_DEBUG_UNSAFE_DIRECTORIES", 0)) {
+			LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;
+
+			if (ConvertSidToStringSidA(sid, &str1))
+				to_free1 = str1;
+			else
+				str1 = "(inconvertible)";
+
+			if (!current_user_sid)
+				str2 = "(none)";
+			else if (!IsValidSid(current_user_sid))
+				str2 = "(invalid)";
+			else if (ConvertSidToStringSidA(current_user_sid, &str2))
+				to_free2 = str2;
+			else
+				str2 = "(inconvertible)";
+			warning("'%s' is owned by:\n\t'%s'\nbut the current user is:\n\t'%s'", path, str1, str2);
+			LocalFree(to_free1);
+			LocalFree(to_free2);
+		}
 	}
 
 	/*

index 9dcecda65b0..3ba42ffcb27 100644
@@ -1353,13 +1353,23 @@ const char *setup_git_directory_gently(int *nongit_ok)
 	case GIT_DIR_INVALID_OWNERSHIP:
 		if (!nongit_ok) {
 			struct strbuf quoted = STRBUF_INIT;
+			struct strbuf hint = STRBUF_INIT;
+
+#ifdef __MINGW32__
+			if (!git_env_bool("GIT_TEST_DEBUG_UNSAFE_DIRECTORIES", 0))
+				strbuf_addstr(&hint,
+					      _("\n\nSet the environment variable "
+						"GIT_TEST_DEBUG_UNSAFE_DIRECTORIES=true "
+						"and run\n"
+						"again for more information."));
+#endif
 
 			sq_quote_buf_pretty(&quoted, dir.buf);
 			die(_("detected dubious ownership in repository at '%s'\n"
 			      "To add an exception for this directory, call:\n"
 			      "\n"
-			      "\tgit config --global --add safe.directory %s"),
-			    dir.buf, quoted.buf);
+			      "\tgit config --global --add safe.directory %s%s"),
+			    dir.buf, quoted.buf, hint.buf);
 		}
 		*nongit_ok = 1;
 		break;

gitgitgadget

From: Johannes Schindelin <[email protected]>

When an Administrator creates a file or directory, the created file/directory is owned not by the Administrator SID, but by the _Administrators Group_ SID. The reason is that users with administrator privileges usually run in unprivileged ("non-elevated") mode, and their user SID does not change when running in elevated mode.

This is is relevant e.g. when running a GitHub workflow on a build agent, which runs in elevated mode: cloning a Git repository in a script step will cause the worktree to be owned by the Administrators Group SID, for example.

Let's handle this case as following: if the current user is an administrator, Git should consider a worktree owned by the Administrators Group as if it were owned by said user.

---
 compat/mingw.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

index 912444fb3ab..e0e020ee574 100644
@@ -2669,6 +2669,7 @@ int is_path_owned_by_current_sid(const char *path)
 	else if (sid && IsValidSid(sid)) {
 		/* Now, verify that the SID matches the current user's */
 		static PSID current_user_sid;
+		BOOL is_member;
 
 		if (!current_user_sid)
 			current_user_sid = get_current_user_sid();

@@ -2677,6 +2678,15 @@ int is_path_owned_by_current_sid(const char *path)
 		    IsValidSid(current_user_sid) &&
 		    EqualSid(sid, current_user_sid))
 			result = 1;
+		else if (IsWellKnownSid(sid, WinBuiltinAdministratorsSid) &&
+			 CheckTokenMembership(NULL, sid, &is_member) &&
+			 is_member)
+			/*
+			 * If owned by the Administrators group, and the
+			 * current user is an administrator, we consider that
+			 * okay, too.
+			 */
+			result = 1;
 		else if (git_env_bool("GIT_TEST_DEBUG_UNSAFE_DIRECTORIES", 0)) {
 			LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;
 

gitgitgadget

From: Johannes Schindelin <[email protected]>

The FAT file system has no concept of ACLs. Therefore, it cannot store any ownership information anyway, and the GetNamedSecurityInfoW() call pretends that everything is owned "by the world".

Let's special-case that scenario and tell the user what's going on, at least when they set GIT_TEST_DEBUG_UNSAFE_DIRECTORIES.

This addresses https://github.com/git-for-windows/git/issues/3886

---
 compat/mingw.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

index e0e020ee574..4144d6247bd 100644
@@ -2630,6 +2630,21 @@ static PSID get_current_user_sid(void)
 	return result;
 }
 
+static int acls_supported(const char *path)
+{
+	size_t offset = offset_1st_component(path);
+	WCHAR wroot[MAX_PATH];
+	DWORD file_system_flags;
+
+	if (offset &&
+	    xutftowcsn(wroot, path, MAX_PATH, offset) > 0 &&
+	    GetVolumeInformationW(wroot, NULL, 0, NULL, NULL,
+				  &file_system_flags, NULL, 0))
+		return !!(file_system_flags & FILE_PERSISTENT_ACLS);
+
+	return 0;
+}
+
 int is_path_owned_by_current_sid(const char *path)
 {
 	WCHAR wpath[MAX_PATH];

@@ -2687,7 +2702,14 @@ int is_path_owned_by_current_sid(const char *path)
 			 * okay, too.
 			 */
 			result = 1;
-		else if (git_env_bool("GIT_TEST_DEBUG_UNSAFE_DIRECTORIES", 0)) {
+		else if (IsWellKnownSid(sid, WinWorldSid) &&
+			 git_env_bool("GIT_TEST_DEBUG_UNSAFE_DIRECTORIES", 0) &&
+			 !acls_supported(path)) {
+			/*
+			 * On FAT32 volumes, ownership is not actually recorded.
+			 */
+			warning("'%s' is on a file system that does not record ownership", path);
+		} else if (git_env_bool("GIT_TEST_DEBUG_UNSAFE_DIRECTORIES", 0)) {
 			LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;
 
 			if (ConvertSidToStringSidA(sid, &str1))

gitgitgadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

I can see how the change to compat/ part is useful, but ...

I am wondering if passing a struct to allow is_path_owned_by*() helper(s) to report the detail of the failures they discover a cleaner way to do this. To illustrate what I meant, along the lines of the following patch, with any additional code to actually stuff messages in the strbuf report, in the is_path_owned_by*() implementation.

I am perfectly OK if we make it a debug-only option by hiding this behind an environment variable, but if we were to do so, I do not want to see us advertize the environment variable in the die() message (a debug-only option can be documented, but not worth surfacing in and contaminating the usual UI output).

Comments?

 compat/mingw.c    |  2 +-
 git-compat-util.h |  3 ++-
 setup.c           | 12 +++++++++---
 3 files changed, 12 insertions(+), 5 deletions(-)

index 2607de93af..f12b7df16d 100644
@@ -2673,7 +2673,7 @@ static PSID get_current_user_sid(void)
 	return result;
 }
 
-int is_path_owned_by_current_sid(const char *path)
+int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
 {
 	WCHAR wpath[MAX_PATH];
 	PSID sid = NULL;

index 58d7708296..de34b0ea7e 100644
@@ -487,7 +487,8 @@ static inline void extract_id_from_env(const char *env, uid_t *id)
 	}
 }
 
-static inline int is_path_owned_by_current_uid(const char *path)
+struct strbuf;
+static inline int is_path_owned_by_current_uid(const char *path, struct strbuf *report)
 {
 	struct stat st;
 	uid_t euid;

index 09b6549ba9..ed823585f7 100644
@@ -1143,12 +1143,18 @@ static int ensure_valid_ownership(const char *gitfile,
 	struct safe_directory_data data = {
 		.path = worktree ? worktree : gitdir
 	};
+	struct strbuf report = STRBUF_INIT;
 
 	if (!git_env_bool("GIT_TEST_ASSUME_DIFFERENT_OWNER", 0) &&
-	   (!gitfile || is_path_owned_by_current_user(gitfile)) &&
-	   (!worktree || is_path_owned_by_current_user(worktree)) &&
-	   (!gitdir || is_path_owned_by_current_user(gitdir)))
+	    (!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))) {
+		if (report.len) {
+			fputs(report.buf, stderr);
+			strbuf_release(&report);
+		}
 		return 1;
+	}
 
 	/*
 	 * data.path is the "path" that identifies the repository and it is

Hi Junio,

I like it! Let me play with this, after this coming week (during which I plan to be mostly offline).

Thanks,
Dscho

Due to the semantics being substantially different from Unix, the safe.directory feature presents its own set of problems on Windows. One particular issue would have prevented it from working in GitHub Actions' build agents, which we definitely rely on in the Git project itself. This was addressed via the fifth patch, which had made it (in a slightly different form) already into Git for Windows v2.35.2, and they are ready to be applied to core Git, too.

The FAT32 patch came in later, and was released as part of Git for Windows v2.37.0, so I also have confidence that it is stable and ready to be integrated into core Git, too.

Changes since v1:

* Restructured the patch series.
* Instead of an environment variable to turn on debugging, we now always show the platform-dependent information together with the error message about the dubious ownership (iff it is shown, that is), based on an idea by Junio.
* Rebased onto gc/bare-repo-discovery to avoid a merge conflict.

Johannes Schindelin (5):
setup: fix some formatting
Prepare for more detailed "dubious ownership" messages
mingw: provide details about unsafe directories' ownership
mingw: be more informative when ownership check fails on FAT32
mingw: handle a file owned by the Administrators group correctly

 compat/mingw.c    | 59 ++++++++++++++++++++++++++++++++++++++++++++++-
 compat/mingw.h    |  2 +-
 git-compat-util.h |  5 +++-
 setup.c           | 30 ++++++++++++++----------
 4 files changed, 81 insertions(+), 15 deletions(-)

base-commit: 776f184893d2861a729aa4b91d69931036e03e4b
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1286%2Fdscho%2Fsafe.directory-and-windows-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1286/dscho/safe.directory-and-windows-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1286

Range-diff vs v1:

 -:  ----------- > 1:  301d94f18f5 setup: fix some formatting
 -:  ----------- > 2:  8cc45e4922a Prepare for more detailed "dubious ownership" messages
 1:  3480381b8b9 ! 3:  63494818105 Allow debugging unsafe directories' ownership
     @@ Metadata
      Author: Johannes Schindelin <[email protected]>
      
       ## Commit message ##
     -    Allow debugging unsafe directories' ownership
     +    mingw: provide details about unsafe directories' ownership
      
          When Git refuses to use an existing repository because it is owned by
          someone else than the current user, it can be a bit tricky on Windows to
          figure out what is going on.
      
     -    Let's help with that by offering some more information via the
     -    environment variable `GIT_TEST_DEBUG_UNSAFE_DIRECTORIES`.
     +    Let's help with that by providing more detailed information.
      
          Signed-off-by: Johannes Schindelin <[email protected]>
      
     - ## Documentation/config/safe.txt ##
     -@@ Documentation/config/safe.txt: which id the original user has.
     - If that is not what you would prefer and want git to only trust
     - repositories that are owned by root instead, then you can remove
     - the `SUDO_UID` variable from root's environment before invoking git.
     -++
     -+Due to the permission model on Windows where ACLs are used instead of
     -+Unix' simpler permission model, it can be a bit tricky to figure out why
     -+a directory is considered unsafe. To help with this, Git will provide
     -+more detailed information when the environment variable
     -+`GIT_TEST_DEBUG_UNSAFE_DIRECTORIES` is set to `true`.
     -
       ## compat/mingw.c ##
      @@
       #include "../git-compat-util.h"
     @@ compat/mingw.c
       #include <conio.h>
       #include <wchar.h>
       #include "../strbuf.h"
     -@@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path)
     +@@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
       		    IsValidSid(current_user_sid) &&
       		    EqualSid(sid, current_user_sid))
       			result = 1;
     -+		else if (git_env_bool("GIT_TEST_DEBUG_UNSAFE_DIRECTORIES", 0)) {
     ++		else if (report) {
      +			LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;
      +
      +			if (ConvertSidToStringSidA(sid, &str1))
     @@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path)
      +				to_free2 = str2;
      +			else
      +				str2 = "(inconvertible)";
     -+			warning("'%s' is owned by:\n\t'%s'\nbut the current user is:\n\t'%s'", path, str1, str2);
     ++			strbuf_addf(report,
     ++				    "'%s' is owned by:\n"
     ++				    "\t'%s'\nbut the current user is:\n"
     ++				    "\t'%s'\n", path, str1, str2);
      +			LocalFree(to_free1);
      +			LocalFree(to_free2);
      +		}
       	}
       
       	/*
     -
     - ## setup.c ##
     -@@ setup.c: const char *setup_git_directory_gently(int *nongit_ok)
     - 	case GIT_DIR_INVALID_OWNERSHIP:
     - 		if (!nongit_ok) {
     - 			struct strbuf quoted = STRBUF_INIT;
     -+			struct strbuf hint = STRBUF_INIT;
     -+
     -+#ifdef __MINGW32__
     -+			if (!git_env_bool("GIT_TEST_DEBUG_UNSAFE_DIRECTORIES", 0))
     -+				strbuf_addstr(&hint,
     -+					      _("\n\nSet the environment variable "
     -+						"GIT_TEST_DEBUG_UNSAFE_DIRECTORIES=true "
     -+						"and run\n"
     -+						"again for more information."));
     -+#endif
     - 
     - 			sq_quote_buf_pretty(&quoted, dir.buf);
     - 			die(_("detected dubious ownership in repository at '%s'\n"
     - 			      "To add an exception for this directory, call:\n"
     - 			      "\n"
     --			      "\tgit config --global --add safe.directory %s"),
     --			    dir.buf, quoted.buf);
     -+			      "\tgit config --global --add safe.directory %s%s"),
     -+			    dir.buf, quoted.buf, hint.buf);
     - 		}
     - 		*nongit_ok = 1;
     - 		break;
 3:  dae03f1b204 ! 4:  7aaa6248dfe mingw: be more informative when ownership check fails on FAT32
     @@ Commit message
          any ownership information anyway, and the `GetNamedSecurityInfoW()` call
          pretends that everything is owned "by the world".
      
     -    Let's special-case that scenario and tell the user what's going on, at
     -    least when they set `GIT_TEST_DEBUG_UNSAFE_DIRECTORIES`.
     +    Let's special-case that scenario and tell the user what's going on.
      
          This addresses https://github.com/git-for-windows/git/issues/3886
      
     @@ compat/mingw.c: static PSID get_current_user_sid(void)
      +	return 0;
      +}
      +
     - int is_path_owned_by_current_sid(const char *path)
     + int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
       {
       	WCHAR wpath[MAX_PATH];
     -@@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path)
     - 			 * okay, too.
     - 			 */
     +@@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
     + 		    IsValidSid(current_user_sid) &&
     + 		    EqualSid(sid, current_user_sid))
       			result = 1;
     --		else if (git_env_bool("GIT_TEST_DEBUG_UNSAFE_DIRECTORIES", 0)) {
     -+		else if (IsWellKnownSid(sid, WinWorldSid) &&
     -+			 git_env_bool("GIT_TEST_DEBUG_UNSAFE_DIRECTORIES", 0) &&
     +-		else if (report) {
     ++		else if (report &&
     ++			 IsWellKnownSid(sid, WinWorldSid) &&
      +			 !acls_supported(path)) {
      +			/*
      +			 * On FAT32 volumes, ownership is not actually recorded.
      +			 */
     -+			warning("'%s' is on a file system that does not record ownership", path);
     -+		} else if (git_env_bool("GIT_TEST_DEBUG_UNSAFE_DIRECTORIES", 0)) {
     ++			strbuf_addf(report, "'%s' is on a file system that does"
     ++				    "not record ownership\n", path);
     ++		} else if (report) {
       			LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;
       
       			if (ConvertSidToStringSidA(sid, &str1))

2: be06d711a13 ! 5: fbfaff2ec21 mingw: handle a file owned by the Administrators group correctly
@@ Commit message

## compat/mingw.c ##
-@@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path)
+@@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
else if (sid && IsValidSid(sid)) {
/* Now, verify that the SID matches the current user's */
static PSID current_user_sid;
@@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path)

if (!current_user_sid)
current_user_sid = get_current_user_sid();
-@@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path)
+@@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
IsValidSid(current_user_sid) &&
EqualSid(sid, current_user_sid))
result = 1;
@@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path)
+ * okay, too.
+ */
+ result = 1;
- else if (git_env_bool("GIT_TEST_DEBUG_UNSAFE_DIRECTORIES", 0)) {
- LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;
-
+ else if (report &&
+ IsWellKnownSid(sid, WinWorldSid) &&
+ !acls_supported(path)) {

gitgitgadget

From: Johannes Schindelin <[email protected]>

In preparation for touching code that was introduced in 3b0bf2704980 (setup: tighten ownership checks post CVE-2022-24765, 2022-05-10) and that was formatted differently than preferred in the Git project, fix the indentation before actually modifying the code.

---
 setup.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

index 8c683e92b62..2f7b3e598f8 100644
@@ -1142,7 +1142,7 @@ static int safe_directory_cb(const char *key, const char *value, void *d)
  * added, for bare ones their git directory.
  */
 static int ensure_valid_ownership(const char *gitfile,
-				const char *worktree, const char *gitdir)
+				  const char *worktree, const char *gitdir)
 {
 	struct safe_directory_data data = {
 		.path = worktree ? worktree : gitdir

@@ -1316,10 +1316,11 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 		strbuf_setlen(dir, offset);
 		if (gitdirenv) {
 			enum discovery_result ret;
+			const char *gitdir_candidate =
+				gitdir_path ? gitdir_path : gitdirenv;
 
-			if (ensure_valid_ownership(gitfile,
-						 dir->buf,
-				 (gitdir_path ? gitdir_path : gitdirenv))) {
+			if (ensure_valid_ownership(gitfile, dir->buf,
+						   gitdir_candidate)) {
 				strbuf_addstr(gitdir, gitdirenv);
 				ret = GIT_DIR_DISCOVERED;
 			} else

gitgitgadget

From: Johannes Schindelin <[email protected]>

When verifying the ownership of the Git directory, we sometimes would like to say a bit more about it, e.g. when using a platform-dependent code path (think: Windows and the permission model that is so different from Unix'), but only when it is a appropriate to actually say something.

To allow for that, collect that information and hand it back to the caller (whose responsibility it is to show it or not).

Note: We do not actually fill in any platform-dependent information yet, this commit just adds the infrastructure to be able to do so.

---
 compat/mingw.c    |  2 +-
 compat/mingw.h    |  2 +-
 git-compat-util.h |  5 ++++-
 setup.c           | 25 +++++++++++++++----------
 4 files changed, 21 insertions(+), 13 deletions(-)

index 2607de93af5..f12b7df16d9 100644
@@ -2673,7 +2673,7 @@ static PSID get_current_user_sid(void)
 	return result;
 }
 
-int is_path_owned_by_current_sid(const char *path)
+int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
 {
 	WCHAR wpath[MAX_PATH];
 	PSID sid = NULL;

index a74da68f313..209cf7cebad 100644
@@ -463,7 +463,7 @@ char *mingw_query_user_email(void);
  * Verifies that the specified path is owned by the user running the
  * current process.
  */
-int is_path_owned_by_current_sid(const char *path);
+int is_path_owned_by_current_sid(const char *path, struct strbuf *report);
 #define is_path_owned_by_current_user is_path_owned_by_current_sid
 
 /**

index 58d7708296b..36a25ae252f 100644
@@ -23,6 +23,9 @@
 #include <crtdbg.h>
 #endif
 
+struct strbuf;
+
+
 #define _FILE_OFFSET_BITS 64
 
 

@@ -487,7 +490,7 @@ static inline void extract_id_from_env(const char *env, uid_t *id)
 	}
 }
 
-static inline int is_path_owned_by_current_uid(const char *path)
+static inline int is_path_owned_by_current_uid(const char *path, struct strbuf *report)
 {
 	struct stat st;
 	uid_t euid;

index 2f7b3e598f8..cefd5f63c46 100644
@@ -1142,16 +1142,17 @@ static int safe_directory_cb(const char *key, const char *value, void *d)
  * added, for bare ones their git directory.
  */
 static int ensure_valid_ownership(const char *gitfile,
-				  const char *worktree, const char *gitdir)
+				  const char *worktree, const char *gitdir,
+				  struct strbuf *report)
 {
 	struct safe_directory_data data = {
 		.path = worktree ? worktree : gitdir
 	};
 
 	if (!git_env_bool("GIT_TEST_ASSUME_DIFFERENT_OWNER", 0) &&
-	   (!gitfile || is_path_owned_by_current_user(gitfile)) &&
-	   (!worktree || is_path_owned_by_current_user(worktree)) &&
-	   (!gitdir || is_path_owned_by_current_user(gitdir)))
+	    (!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;
 
 	/*

@@ -1232,6 +1233,7 @@ enum discovery_result {
  */
 static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 							  struct strbuf *gitdir,
+							  struct strbuf *report,
 							  int die_on_error)
 {
 	const char *env_ceiling_dirs = getenv(CEILING_DIRECTORIES_ENVIRONMENT);

@@ -1320,7 +1322,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 				gitdir_path ? gitdir_path : gitdirenv;
 
 			if (ensure_valid_ownership(gitfile, dir->buf,
-						   gitdir_candidate)) {
+						   gitdir_candidate, report)) {
 				strbuf_addstr(gitdir, gitdirenv);
 				ret = GIT_DIR_DISCOVERED;
 			} else

@@ -1345,7 +1347,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 		if (is_git_directory(dir->buf)) {
 			if (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT)
 				return GIT_DIR_DISALLOWED_BARE;
-			if (!ensure_valid_ownership(NULL, NULL, dir->buf))
+			if (!ensure_valid_ownership(NULL, NULL, dir->buf, report))
 				return GIT_DIR_INVALID_OWNERSHIP;
 			strbuf_addstr(gitdir, ".");
 			return GIT_DIR_BARE;

@@ -1378,7 +1380,7 @@ int discover_git_directory(struct strbuf *commondir,
 		return -1;
 
 	cwd_len = dir.len;
-	if (setup_git_directory_gently_1(&dir, gitdir, 0) <= 0) {
+	if (setup_git_directory_gently_1(&dir, gitdir, NULL, 0) <= 0) {
 		strbuf_release(&dir);
 		return -1;
 	}

@@ -1425,7 +1427,7 @@ int discover_git_directory(struct strbuf *commondir,
 const char *setup_git_directory_gently(int *nongit_ok)
 {
 	static struct strbuf cwd = STRBUF_INIT;
-	struct strbuf dir = STRBUF_INIT, gitdir = STRBUF_INIT;
+	struct strbuf dir = STRBUF_INIT, gitdir = STRBUF_INIT, report = STRBUF_INIT;
 	const char *prefix = NULL;
 	struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;
 

@@ -1450,7 +1452,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 		die_errno(_("Unable to read current working directory"));
 	strbuf_addbuf(&dir, &cwd);
 
-	switch (setup_git_directory_gently_1(&dir, &gitdir, 1)) {
+	switch (setup_git_directory_gently_1(&dir, &gitdir, &report, 1)) {
 	case GIT_DIR_EXPLICIT:
 		prefix = setup_explicit_git_dir(gitdir.buf, &cwd, &repo_fmt, nongit_ok);
 		break;

@@ -1482,12 +1484,14 @@ const char *setup_git_directory_gently(int *nongit_ok)
 		if (!nongit_ok) {
 			struct strbuf quoted = STRBUF_INIT;
 
+			strbuf_complete(&report, '\n');
 			sq_quote_buf_pretty(&quoted, dir.buf);
 			die(_("detected dubious ownership in repository at '%s'\n"
+			      "%s"
 			      "To add an exception for this directory, call:\n"
 			      "\n"
 			      "\tgit config --global --add safe.directory %s"),
-			    dir.buf, quoted.buf);
+			    dir.buf, report.buf, quoted.buf);
 		}
 		*nongit_ok = 1;
 		break;

@@ -1574,6 +1578,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 
 	strbuf_release(&dir);
 	strbuf_release(&gitdir);
+	strbuf_release(&report);
 	clear_repository_format(&repo_fmt);
 
 	return prefix;

gitgitgadget

From: Johannes Schindelin <[email protected]>

When Git refuses to use an existing repository because it is owned by someone else than the current user, it can be a bit tricky on Windows to figure out what is going on.

Let's help with that by providing more detailed information.

---
 compat/mingw.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

index f12b7df16d9..2c09c5bffee 100644
@@ -1,6 +1,7 @@
 #include "../git-compat-util.h"
 #include "win32.h"
 #include <aclapi.h>
+#include <sddl.h>
 #include <conio.h>
 #include <wchar.h>
 #include "../strbuf.h"

@@ -2720,6 +2721,29 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
 		    IsValidSid(current_user_sid) &&
 		    EqualSid(sid, current_user_sid))
 			result = 1;
+		else if (report) {
+			LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;
+
+			if (ConvertSidToStringSidA(sid, &str1))
+				to_free1 = str1;
+			else
+				str1 = "(inconvertible)";
+
+			if (!current_user_sid)
+				str2 = "(none)";
+			else if (!IsValidSid(current_user_sid))
+				str2 = "(invalid)";
+			else if (ConvertSidToStringSidA(current_user_sid, &str2))
+				to_free2 = str2;
+			else
+				str2 = "(inconvertible)";
+			strbuf_addf(report,
+				    "'%s' is owned by:\n"
+				    "\t'%s'\nbut the current user is:\n"
+				    "\t'%s'\n", path, str1, str2);
+			LocalFree(to_free1);
+			LocalFree(to_free2);
+		}
 	}
 
 	/*

gitgitgadget

From: Johannes Schindelin <[email protected]>

The FAT file system has no concept of ACLs. Therefore, it cannot store any ownership information anyway, and the GetNamedSecurityInfoW() call pretends that everything is owned "by the world".

Let's special-case that scenario and tell the user what's going on.

This addresses https://github.com/git-for-windows/git/issues/3886

---
 compat/mingw.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

index 2c09c5bffee..22f960c7e34 100644
@@ -2674,6 +2674,21 @@ static PSID get_current_user_sid(void)
 	return result;
 }
 
+static int acls_supported(const char *path)
+{
+	size_t offset = offset_1st_component(path);
+	WCHAR wroot[MAX_PATH];
+	DWORD file_system_flags;
+
+	if (offset &&
+	    xutftowcsn(wroot, path, MAX_PATH, offset) > 0 &&
+	    GetVolumeInformationW(wroot, NULL, 0, NULL, NULL,
+				  &file_system_flags, NULL, 0))
+		return !!(file_system_flags & FILE_PERSISTENT_ACLS);
+
+	return 0;
+}
+
 int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
 {
 	WCHAR wpath[MAX_PATH];

@@ -2721,7 +2736,15 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
 		    IsValidSid(current_user_sid) &&
 		    EqualSid(sid, current_user_sid))
 			result = 1;
-		else if (report) {
+		else if (report &&
+			 IsWellKnownSid(sid, WinWorldSid) &&
+			 !acls_supported(path)) {
+			/*
+			 * On FAT32 volumes, ownership is not actually recorded.
+			 */
+			strbuf_addf(report, "'%s' is on a file system that does"
+				    "not record ownership\n", path);
+		} else if (report) {
 			LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;
 
 			if (ConvertSidToStringSidA(sid, &str1))

gitgitgadget

From: Johannes Schindelin <[email protected]>

When an Administrator creates a file or directory, the created file/directory is owned not by the Administrator SID, but by the _Administrators Group_ SID. The reason is that users with administrator privileges usually run in unprivileged ("non-elevated") mode, and their user SID does not change when running in elevated mode.

This is is relevant e.g. when running a GitHub workflow on a build agent, which runs in elevated mode: cloning a Git repository in a script step will cause the worktree to be owned by the Administrators Group SID, for example.

Let's handle this case as following: if the current user is an administrator, Git should consider a worktree owned by the Administrators Group as if it were owned by said user.

---
 compat/mingw.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

index 22f960c7e34..7aa9318db72 100644
@@ -2728,6 +2728,7 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
 	else if (sid && IsValidSid(sid)) {
 		/* Now, verify that the SID matches the current user's */
 		static PSID current_user_sid;
+		BOOL is_member;
 
 		if (!current_user_sid)
 			current_user_sid = get_current_user_sid();

@@ -2736,6 +2737,15 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
 		    IsValidSid(current_user_sid) &&
 		    EqualSid(sid, current_user_sid))
 			result = 1;
+		else if (IsWellKnownSid(sid, WinBuiltinAdministratorsSid) &&
+			 CheckTokenMembership(NULL, sid, &is_member) &&
+			 is_member)
+			/*
+			 * If owned by the Administrators group, and the
+			 * current user is an administrator, we consider that
+			 * okay, too.
+			 */
+			result = 1;
 		else if (report &&
 			 IsWellKnownSid(sid, WinWorldSid) &&
 			 !acls_supported(path)) {

gitgitgadget

Hi Junio,

I had to play with it for a while until I could make it work, but eventually I managed to do it. The second iteration was just sent, and implements this route.

Thank you,
Dscho

"Johannes Schindelin via GitGitGadget" <[email protected]>

I actually had to rebase it back so that we could merge it to 'maint' for further 2.37.x releases. I'll refer to the original patches in this thread when I merge the result to 'seen', of course, to make sure the results do match. It would have been slightly less convenient if you did not do this rebase, but it would have allowed me to have much better confidence in the result that may eventually go to 'maint'. After all, mistakes in resolving merge conflicts on 'seen' can be corrected before the topic hits 'next'.

Thanks. I do not know about the API calls mingw.c part of these patches make, but the overall structure looks sensible to me.

Hi Junio,

I appreciate the effort you put into it, even if it is not your responsibility to take care of Git for Windows' releases.

The range-diff shows that you snuck in a commit message change that I would have either not made at all (I think the original was fine) or would have made differently (because Access Control Lists are not specific to Windows, even if Windows is the most obvious example for this permission model):

-- snip --
1: 301d94f18f5 ! 1: d51e1dff980 setup: fix some formatting
@@ Commit message
the indentation before actually modifying the code.

+ Signed-off-by: Junio C Hamano <[email protected]>

## setup.c ##
@@ setup.c: static int safe_directory_cb(const char *key, const char *value, void *d)
2: 8cc45e4922a ! 2: 17d3883fe9c Prepare for more detailed "dubious ownership" messages
@@ Metadata
Author: Johannes Schindelin <[email protected]>

## Commit message ##
- Prepare for more detailed "dubious ownership" messages
+ setup: prepare for more detailed "dubious ownership" messages

When verifying the ownership of the Git directory, we sometimes would like to say a bit more about it, e.g. when using a platform-dependent
- code path (think: Windows and the permission model that is so different
+ code path (think: Windows has the permission model that is so different
from Unix'), but only when it is a appropriate to actually say something.

@@ Commit message

+ Signed-off-by: Junio C Hamano <[email protected]>

## compat/mingw.c ##
@@ compat/mingw.c: static PSID get_current_user_sid(void)
@@ setup.c: static enum discovery_result setup_git_directory_gently_1(struct strbuf
ret = GIT_DIR_DISCOVERED;
} else
@@ setup.c: static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
+ }
+
if (is_git_directory(dir->buf)) {
- if (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT)
- return GIT_DIR_DISALLOWED_BARE;
- if (!ensure_valid_ownership(NULL, NULL, dir->buf))
+ if (!ensure_valid_ownership(NULL, NULL, dir->buf, report))
return GIT_DIR_INVALID_OWNERSHIP;
3: 63494818105 ! 3: e883e04b68b mingw: provide details about unsafe directories' ownership
@@ Commit message
Let's help with that by providing more detailed information.

+ Signed-off-by: Junio C Hamano <[email protected]>

## compat/mingw.c ##
@@
4: 7aaa6248dfe ! 4: 7c83470e64e mingw: be more informative when ownership check fails on FAT32
@@ Commit message
This addresses https://github.com/git-for-windows/git/issues/3886

+ Signed-off-by: Junio C Hamano <[email protected]>

## compat/mingw.c ##
@@ compat/mingw.c: static PSID get_current_user_sid(void)
5: fbfaff2ec21 ! 5: 3f7207e2ea9 mingw: handle a file owned by the Administrators group correctly
@@ Commit message
Administrators Group as if it were owned by said user.

+ Signed-off-by: Junio C Hamano <[email protected]>

## compat/mingw.c ##
@@ compat/mingw.c: int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
-- snap --

Retitling the commit message was okay, of course, using the setup: prefix.

Yes, mistakes can happen, and the more people work together the easier it is to avoid or fix them.

I do not think that anybody expected you to know about Win32 API calls ;-)

Ciao,
Dscho