[PATCH 0/1 v2] Replace SID with domain/username on Windows

[PATCH 0/1 v2] Replace SID with domain/username on Windows

[Sören Krecker]

Improve error message on windows systems, if owner of reposotory and current user are not equal. 


Old Message:
'''
fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
'C:/Users/test/source/repos/git' is owned by:
	'S-1-5-21-571067702-4104414259-3379520149-500'
but the current user is:
	'S-1-5-21-571067702-4104414259-3379520149-1001'
To add an exception for this directory, call:

	git config --global --add safe.directory C:/Users/test/source/repos/git 
'''

New Massage:
'''
fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
'C:/Users/soren/source/repos/git' is owned by:
        'DESKTOP-L78JVA6/Administrator'
but the current user is:
        'DESKTOP-L78JVA6/test'
To add an exception for this directory, call:

        git config --global --add safe.directory C:/Users/test/source/repos/git
'''


I hope that I have succeeded in addressing all the points raised.

Sören Krecker (1):
  Replace SID with domain/username

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


base-commit: e79552d19784ee7f4bbce278fe25f93fbda196fa
-- 
2.39.2

[PATCH v2 1/1] Replace SID with domain/username

[Sören Krecker]

Replace SID with domain/username in erromessage, if owner of repository
and user are not equal on windows systems.

Signed-off-by: Sören Krecker <soekkle@freenet.de>
---
 compat/mingw.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 42053c1f65..05aeaaa9ad 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2684,6 +2684,26 @@ static PSID get_current_user_sid(void)
 	return result;
 }
 
+static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)
+{
+	SID_NAME_USE pe_use;
+	DWORD len_user = 0, len_domain = 0;
+	BOOL translate_sid_to_user;
+
+	/* returns only FALSE, because the string pointers are NULL*/
+	LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,
+			  &pe_use); 
+	/*Alloc needed space of the strings*/
+	ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); 
+	translate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user,
+				   *str, &len_domain, &pe_use);
+	*(*str + len_domain) = '/';
+	if (translate_sid_to_user == FALSE) {
+		FREE_AND_NULL(*str);
+	}
+	return translate_sid_to_user;
+}
+
 static int acls_supported(const char *path)
 {
 	size_t offset = offset_1st_component(path);
@@ -2767,7 +2787,7 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
 		} else if (report) {
 			LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;
 
-			if (ConvertSidToStringSidA(sid, &str1))
+			if (user_sid_to_user_name(sid, &str1))
 				to_free1 = str1;
 			else
 				str1 = "(inconvertible)";
@@ -2776,7 +2796,7 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
 				str2 = "(none)";
 			else if (!IsValidSid(current_user_sid))
 				str2 = "(invalid)";
-			else if (ConvertSidToStringSidA(current_user_sid, &str2))
+			else if (user_sid_to_user_name(current_user_sid, &str2))
 				to_free2 = str2;
 			else
 				str2 = "(inconvertible)";
@@ -2784,8 +2804,8 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *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);
+			free(to_free1);
+			free(to_free2);
 		}
 	}
 
-- 
2.39.2

Re: [PATCH 0/1 v2] Replace SID with domain/username on Windows

[Eric Sunshine]

On Fri, Dec 29, 2023 at 7:03 AM Sören Krecker <soekkle@freenet.de> wrote:
> Improve error message on windows systems, if owner of reposotory and current user are not equal.
>
> Old Message:
> '''
> fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
> 'C:/Users/test/source/repos/git' is owned by:
>         'S-1-5-21-571067702-4104414259-3379520149-500'
> but the current user is:
>         'S-1-5-21-571067702-4104414259-3379520149-1001'
> To add an exception for this directory, call:
>
>         git config --global --add safe.directory C:/Users/test/source/repos/git
> '''
>
> New Massage:
> '''
> fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
> 'C:/Users/soren/source/repos/git' is owned by:
>         'DESKTOP-L78JVA6/Administrator'
> but the current user is:
>         'DESKTOP-L78JVA6/test'
> To add an exception for this directory, call:
>
>         git config --global --add safe.directory C:/Users/test/source/repos/git
> '''
>
> I hope that I have succeeded in addressing all the points raised.

Thanks, this explanation does an excellent job of helping reviewers
understand why the patch is desirable.

It's also important that people digging through the project history in
the future also understand the reason for this change, so it's very
valuable for the explanation to be part of the commit message of the
patch itself, not just in the cover letter (which doesn't become part
of the permanent project history). Therefore, can you reroll once
again, placing this explanation in the commit message of the patch
itself? Doing so should help the patch get accepted into the project.

That's as much as I can add. Hopefully, one or more Windows folks will
chime in regarding the actual patch content (whether it's acceptable
as-is or needs some tweaks).

Thanks.

[PATCH V3 0/1] Replace SID with domain/username on Windows

[Sören Krecker]

I improve the commit message with example of old and new error output.

Thanks to Eric Sunshine.

Sören Krecker (1):
  Replace SID with domain/username

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


base-commit: e79552d19784ee7f4bbce278fe25f93fbda196fa
-- 
2.39.2

[PATCH v3 1/1] Replace SID with domain/username

[Sören Krecker]

Replace SID with domain/username in error message, if owner of repository
and user are not equal on windows systems.

Old message:
'''
fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
'C:/Users/test/source/repos/git' is owned by:
	'S-1-5-21-571067702-4104414259-3379520149-500'
but the current user is:
	'S-1-5-21-571067702-4104414259-3379520149-1001'
To add an exception for this directory, call:

	git config --global --add safe.directory C:/Users/test/source/repos/git
'''

New massage:
'''
fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
'C:/Users/test/source/repos/git' is owned by:
        'DESKTOP-L78JVA6/Administrator'
but the current user is:
        'DESKTOP-L78JVA6/test'
To add an exception for this directory, call:

        git config --global --add safe.directory C:/Users/test/source/repos/git
'''

Signed-off-by: Sören Krecker <soekkle@freenet.de>
---
 compat/mingw.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 42053c1f65..05aeaaa9ad 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2684,6 +2684,26 @@ static PSID get_current_user_sid(void)
 	return result;
 }
 
+static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)
+{
+	SID_NAME_USE pe_use;
+	DWORD len_user = 0, len_domain = 0;
+	BOOL translate_sid_to_user;
+
+	/* returns only FALSE, because the string pointers are NULL*/
+	LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,
+			  &pe_use); 
+	/*Alloc needed space of the strings*/
+	ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); 
+	translate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user,
+				   *str, &len_domain, &pe_use);
+	*(*str + len_domain) = '/';
+	if (translate_sid_to_user == FALSE) {
+		FREE_AND_NULL(*str);
+	}
+	return translate_sid_to_user;
+}
+
 static int acls_supported(const char *path)
 {
 	size_t offset = offset_1st_component(path);
@@ -2767,7 +2787,7 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
 		} else if (report) {
 			LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;
 
-			if (ConvertSidToStringSidA(sid, &str1))
+			if (user_sid_to_user_name(sid, &str1))
 				to_free1 = str1;
 			else
 				str1 = "(inconvertible)";
@@ -2776,7 +2796,7 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
 				str2 = "(none)";
 			else if (!IsValidSid(current_user_sid))
 				str2 = "(invalid)";
-			else if (ConvertSidToStringSidA(current_user_sid, &str2))
+			else if (user_sid_to_user_name(current_user_sid, &str2))
 				to_free2 = str2;
 			else
 				str2 = "(inconvertible)";
@@ -2784,8 +2804,8 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *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);
+			free(to_free1);
+			free(to_free2);
 		}
 	}
 
-- 
2.39.2

Re: [PATCH V3 0/1] Replace SID with domain/username on Windows

[Eric Sunshine]

On Sun, Dec 31, 2023 at 4:12 AM Sören Krecker <soekkle@freenet.de> wrote:
> I improve the commit message with example of old and new error output.
>
> Thanks to Eric Sunshine.

Thanks for rerolling.

Re: [PATCH v2 1/1] Replace SID with domain/username

[Junio C Hamano]

Sören Krecker <soekkle@freenet.de> writes:

> Replace SID with domain/username in erromessage, if owner of repository
> and user are not equal on windows systems.

"erromessage" -> "error messages" or something?

This may not be a question raised by anybody who know Windows, but
because I do not do Windows, it makes me wonder if this is losing
information.  Can two SID for the same user be active at the same
time, which would cause user_sid_to_user_name() potentially yield
the same string for two different SID?

In any case, I am reasonably sure that Dscho will say yes or no to
this patch (the above "makes me wonder" does not need to be
resolved) and I can wait until then.

Thanks.

> Signed-off-by: Sören Krecker <soekkle@freenet.de>
> ---
>  compat/mingw.c | 28 ++++++++++++++++++++++++----
>  1 file changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 42053c1f65..05aeaaa9ad 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -2684,6 +2684,26 @@ static PSID get_current_user_sid(void)
>  	return result;
>  }
>  
> +static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)
> +{
> +	SID_NAME_USE pe_use;
> +	DWORD len_user = 0, len_domain = 0;
> +	BOOL translate_sid_to_user;
> +
> +	/* returns only FALSE, because the string pointers are NULL*/
> +	LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,
> +			  &pe_use); 
> +	/*Alloc needed space of the strings*/
> +	ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); 
> +	translate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user,
> +				   *str, &len_domain, &pe_use);
> +	*(*str + len_domain) = '/';
> +	if (translate_sid_to_user == FALSE) {
> +		FREE_AND_NULL(*str);
> +	}
> +	return translate_sid_to_user;
> +}
> +
>  static int acls_supported(const char *path)
>  {
>  	size_t offset = offset_1st_component(path);
> @@ -2767,7 +2787,7 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
>  		} else if (report) {
>  			LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;
>  
> -			if (ConvertSidToStringSidA(sid, &str1))
> +			if (user_sid_to_user_name(sid, &str1))
>  				to_free1 = str1;
>  			else
>  				str1 = "(inconvertible)";
> @@ -2776,7 +2796,7 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
>  				str2 = "(none)";
>  			else if (!IsValidSid(current_user_sid))
>  				str2 = "(invalid)";
> -			else if (ConvertSidToStringSidA(current_user_sid, &str2))
> +			else if (user_sid_to_user_name(current_user_sid, &str2))
>  				to_free2 = str2;
>  			else
>  				str2 = "(inconvertible)";
> @@ -2784,8 +2804,8 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *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);
> +			free(to_free1);
> +			free(to_free2);
>  		}
>  	}

Re: [PATCH v2 1/1] Replace SID with domain/username

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Sören Krecker <soekkle@freenet.de> writes:
>
>> Replace SID with domain/username in erromessage, if owner of repository
>> and user are not equal on windows systems.
>
> "erromessage" -> "error messages" or something?
>
> This may not be a question raised by anybody who know Windows, but
> because I do not do Windows, it makes me wonder if this is losing
> information.  Can two SID for the same user be active at the same
> time, which would cause user_sid_to_user_name() potentially yield
> the same string for two different SID?
>
> In any case, I am reasonably sure that Dscho will say yes or no to
> this patch (the above "makes me wonder" does not need to be
> resolved) and I can wait until then.
>
> Thanks.

Another thing I forgot to mention (but did wonder).  The new helper
function does allow LookupAccountSidA() to fail.  Should it fall
back to ConvertSidToStringSidA() that the original has been using?

In any case, I do not think a failure to convert will result in an
attempt to format ("%s", NULL) thanks to the existing code that uses
the stringified SID, which is good.

Re: [PATCH v3 1/1] Replace SID with domain/username

[Junio C Hamano]

Sören Krecker <soekkle@freenet.de> writes:

> Replace SID with domain/username in error message, if owner of repository
> and user are not equal on windows systems.
>
> Old message:
> '''
> fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
> 'C:/Users/test/source/repos/git' is owned by:
> 	'S-1-5-21-571067702-4104414259-3379520149-500'
> but the current user is:
> 	'S-1-5-21-571067702-4104414259-3379520149-1001'
> To add an exception for this directory, call:
>
> 	git config --global --add safe.directory C:/Users/test/source/repos/git
> '''
>
> New massage:

"massage"???

> '''
> fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
> 'C:/Users/test/source/repos/git' is owned by:
>         'DESKTOP-L78JVA6/Administrator'
> but the current user is:
>         'DESKTOP-L78JVA6/test'
> To add an exception for this directory, call:
>
>         git config --global --add safe.directory C:/Users/test/source/repos/git
> '''

The same "does this lose information?" comment applies to this one
as well, as the fundamental approach is unchanged.

> +static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)
> +{
> +	SID_NAME_USE pe_use;
> +	DWORD len_user = 0, len_domain = 0;
> +	BOOL translate_sid_to_user;
> +
> +	/* returns only FALSE, because the string pointers are NULL*/
> +	LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,
> +			  &pe_use); 
> +	/*Alloc needed space of the strings*/
> +	ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); 
> +	translate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user,
> +				   *str, &len_domain, &pe_use);
> +	*(*str + len_domain) = '/';
> +	if (translate_sid_to_user == FALSE) {
> +		FREE_AND_NULL(*str);
> +	}

Is this "FREE_AND_NULL" about clearing after you see an error?  If
so, shouldn't "overwrite the byte after the domain part with a slash"
be done only when you have no error, i.e., perhaps

	translate_sid_to_user = LookupAccountSidA(...);
	if (!translate_sid_to_user)
		FREE_AND_NULL(*str);
	else
        	(*str)[len_domain] = '/';

or something along the line?

> +	return translate_sid_to_user;
> +}

[PATCH V4 0/1] Replace SID with domain/username on Windows

[Sören Krecker]

Hi everone,

I improve the commit message with information from Microsoft, fixes a memmory access error and the message in case of an error.

Vielen dank für die Hinweise von Junio C Hamano.

Sören Krecker (1):
  Replace SID with domain/username

 compat/mingw.c | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)


base-commit: e79552d19784ee7f4bbce278fe25f93fbda196fa
-- 
2.39.2

[PATCH V4 1/1] Replace SID with domain/username

[Sören Krecker]

Replace SID with domain/username in error message, if owner of repository
and user are not equal on windows systems. Each user should have a unique
SID (https://learn.microsoft.com/en-us/windows-server/identity/ad-ds/manage/understand-security-identifiers#what-are-security-identifiers).
This means that domain/username is not a loss of information. If the translation
fails the message contains the SID as string.

Old Prompted error message:
'''
fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
'C:/Users/test/source/repos/git' is owned by:
	'S-1-5-21-571067702-4104414259-3379520149-500'
but the current user is:
	'S-1-5-21-571067702-4104414259-3379520149-1001'
To add an exception for this directory, call:

	git config --global --add safe.directory C:/Users/test/source/repos/git
'''

New prompted error massage:
'''
fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
'C:/Users/test/source/repos/git' is owned by:
	'DESKTOP-L78JVA6/Administrator'
but the current user is:
	'DESKTOP-L78JVA6/test'
To add an exception for this directory, call:

	git config --global --add safe.directory C:/Users/test/source/repos/git
'''

Signed-off-by: Sören Krecker <soekkle@freenet.de>
---
 compat/mingw.c | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 42053c1f65..bfd9573a29 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2684,6 +2684,27 @@ static PSID get_current_user_sid(void)
 	return result;
 }
 
+static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)
+{
+	SID_NAME_USE pe_use;
+	DWORD len_user = 0, len_domain = 0;
+	BOOL translate_sid_to_user;
+
+	/* returns only FALSE, because the string pointers are NULL*/
+	LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,
+			  &pe_use); 
+	/*Alloc needed space of the strings*/
+	ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); 
+	translate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user,
+				   *str, &len_domain, &pe_use);
+	if (translate_sid_to_user == FALSE) {
+		FREE_AND_NULL(*str);
+	}
+	else
+		(*str)[len_domain] = '/';
+	return translate_sid_to_user;
+}
+
 static int acls_supported(const char *path)
 {
 	size_t offset = offset_1st_component(path);
@@ -2767,7 +2788,9 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
 		} else if (report) {
 			LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;
 
-			if (ConvertSidToStringSidA(sid, &str1))
+			if (user_sid_to_user_name(sid, &str1))
+				to_free1 = str1;
+			else if (ConvertSidToStringSidA(sid, &str1))
 				to_free1 = str1;
 			else
 				str1 = "(inconvertible)";
@@ -2776,7 +2799,10 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
 				str2 = "(none)";
 			else if (!IsValidSid(current_user_sid))
 				str2 = "(invalid)";
-			else if (ConvertSidToStringSidA(current_user_sid, &str2))
+			else if (user_sid_to_user_name(current_user_sid, &str2))
+				to_free2 = str2;
+			else if (ConvertSidToStringSidA(current_user_sid,
+							&str2))
 				to_free2 = str2;
 			else
 				str2 = "(inconvertible)";
@@ -2784,8 +2810,8 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *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);
+			free(to_free1);
+			free(to_free2);
 		}
 	}
 
-- 
2.39.2

Re: [PATCH V4 1/1] Replace SID with domain/username

[Junio C Hamano]

Sören Krecker <soekkle@freenet.de> writes:

> Replace SID with domain/username in error message, if owner of repository
> and user are not equal on windows systems. Each user should have a unique
> SID (https://learn.microsoft.com/en-us/windows-server/identity/ad-ds/manage/understand-security-identifiers#what-are-security-identifiers).

That paragraph your URL refers to does say that a SID that is used
for an account will never be reused to identify a different account.
But I am not sure if it means a user will never be assigned more
than one SID (in other words, the reverse is not necessarily true).

The paragraph also mentions that a SID can identify a non-user
entity like a computer account (as opposed to "a user account")---I
do not know what its implications are in the context of this patch,
though.

> This means that domain/username is not a loss of information.

This statement does not (grammatically) make sense, but more
importantly, loss of information may not be a bad thing in this
case.  If more than one SIDs are given to a user account and
processes working for that account, these different SIDs may be
translated, by using LookupAccountSidA(), to the same string for a
single user@domain, and it would be an operation that loses
information in that sense.

But if what we *care* about is user@domain between the current
process and the owner of the directory in question being the same
(or not), then such a loss of information is a *good* thing.  

So I dunno.  Arguing what we care about (is that exact SID equality
between the "owner of the directory" and the "user, which the
current process is working on behalf of", or do we care about the
equality of the "accounts"?) may be a better way to justify this
change, if you ask me.

> +static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)
> +{
> +	SID_NAME_USE pe_use;
> +	DWORD len_user = 0, len_domain = 0;
> +	BOOL translate_sid_to_user;
> +
> +	/* returns only FALSE, because the string pointers are NULL*/
> +	LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,
> +			  &pe_use); 
> +	/*Alloc needed space of the strings*/
> +	ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); 
> +	translate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user,
> +				   *str, &len_domain, &pe_use);
> +	if (translate_sid_to_user == FALSE) {
> +		FREE_AND_NULL(*str);
> +	}

Style: do not enclose a single-statement block inside {}.

> +	else
> +		(*str)[len_domain] = '/';
> +	return translate_sid_to_user;
> +}

> @@ -2767,7 +2788,9 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
>  		} else if (report) {
>  			LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;
>  
> -			if (ConvertSidToStringSidA(sid, &str1))
> +			if (user_sid_to_user_name(sid, &str1))
> +				to_free1 = str1;
> +			else if (ConvertSidToStringSidA(sid, &str1))
>  				to_free1 = str1;

Do these two helper functions return pointers pointing into the same
kind of memory that you can free with the same function?  That is ...

> ...
>  				    "'%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);
> +			free(to_free1);
> +			free(to_free2);

... the original code seems to say that the piece of memory we
obtain from ConvertSidToStringSidA() must not be freed by calling
free() but use something special called LocalFree().  I am assuing
that your user_sid_to_user_name() returns a regular piece of memory
that can be freed by calling regular free()?  Do we need to keep
track of where we got the memory from and use different function to
free each variable, or something (again I do not do Windows so I'll
defer all of these to Dscho, who is CC'ed this time).

Thanks and a happy new year.

>  		}
>  	}

Re: [PATCH V4 1/1] Replace SID with domain/username

[Matthias Aßhauer]

On Tue, 2 Jan 2024, Junio C Hamano wrote:

> Sören Krecker <soekkle@freenet.de> writes:
>
>> Replace SID with domain/username in error message, if owner of repository
>> and user are not equal on windows systems. Each user should have a unique
>> SID (https://learn.microsoft.com/en-us/windows-server/identity/ad-ds/manage/understand-security-identifiers#what-are-security-identifiers).
>
> That paragraph your URL refers to does say that a SID that is used
> for an account will never be reused to identify a different account.
> But I am not sure if it means a user will never be assigned more
> than one SID (in other words, the reverse is not necessarily true).

To my knowledge a user account will never have multiple active SIDs, but
the documentation of LookupAccountSidA [1] explicitly mentions that it 
does look up historic SIDs.

> In addition to looking up SIDs for local accounts, local domain 
> accounts, and explicitly trusted domain accounts, LookupAccountSid can 
> look up SIDs for any account in any domain in the forest, including SIDs 
> that appear only in the SIDhistory field of an account in the forest. 
> The SIDhistory field stores former SIDs of an account that has been
> moved from another domain.

[1] https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-lookupaccountsida#remarks

>
> The paragraph also mentions that a SID can identify a non-user
> entity like a computer account (as opposed to "a user account")---I
> do not know what its implications are in the context of this patch,
> though.
>
>> This means that domain/username is not a loss of information.
>
> This statement does not (grammatically) make sense, but more
> importantly, loss of information may not be a bad thing in this
> case.  If more than one SIDs are given to a user account and
> processes working for that account, these different SIDs may be
> translated, by using LookupAccountSidA(), to the same string for a
> single user@domain, and it would be an operation that loses
> information in that sense.
>
> But if what we *care* about is user@domain between the current
> process and the owner of the directory in question being the same
> (or not), then such a loss of information is a *good* thing.

This patch only changes the output of our error message, though.
It does not change what ownership information we actually compare.
So if we had a hypothetical user Bob that was part of the  domain 
example.com (SID S-1-5-21-100000001-1000000001-10000001-1001) and
had been moved over from the example.org domain (old SID S-1-5-21-
2000000002-2000000002-20000002-2002) and we would detect a repository 
owned by bobs old SID, we would now lookup the old SID, find it 
attached to a user named example.com\Bob, look up Bobs  current SID, find 
it belongs to a user named example.com\Bob and print a confusing error 
message.

> So I dunno.  Arguing what we care about (is that exact SID equality
> between the "owner of the directory" and the "user, which the
> current process is working on behalf of", or do we care about the
> equality of the "accounts"?) may be a better way to justify this
> change, if you ask me.
>
>> +static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)
>> +{
>> +	SID_NAME_USE pe_use;
>> +	DWORD len_user = 0, len_domain = 0;
>> +	BOOL translate_sid_to_user;
>> +
>> +	/* returns only FALSE, because the string pointers are NULL*/
>> +	LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,
>> +			  &pe_use);
>> +	/*Alloc needed space of the strings*/
>> +	ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user);
>> +	translate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user,
>> +				   *str, &len_domain, &pe_use);
>> +	if (translate_sid_to_user == FALSE) {
>> +		FREE_AND_NULL(*str);
>> +	}
>
> Style: do not enclose a single-statement block inside {}.
>
>> +	else
>> +		(*str)[len_domain] = '/';
>> +	return translate_sid_to_user;
>> +}
>
>> @@ -2767,7 +2788,9 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
>>  		} else if (report) {
>>  			LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;
>>
>> -			if (ConvertSidToStringSidA(sid, &str1))
>> +			if (user_sid_to_user_name(sid, &str1))
>> +				to_free1 = str1;
>> +			else if (ConvertSidToStringSidA(sid, &str1))
>>  				to_free1 = str1;
>
> Do these two helper functions return pointers pointing into the same
> kind of memory that you can free with the same function?  That is ...
>
>> ...
>>  				    "'%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);
>> +			free(to_free1);
>> +			free(to_free2);
>
> ... the original code seems to say that the piece of memory we
> obtain from ConvertSidToStringSidA() must not be freed by calling
> free() but use something special called LocalFree().  I am assuing
> that your user_sid_to_user_name() returns a regular piece of memory
> that can be freed by calling regular free()?  Do we need to keep
> track of where we got the memory from and use different function to
> free each variable, or something (again I do not do Windows so I'll
> defer all of these to Dscho, who is CC'ed this time).
>
> Thanks and a happy new year.
>
>>  		}
>>  	}
>
>

Re: [PATCH V4 1/1] Replace SID with domain/username

[Junio C Hamano]

Matthias Aßhauer <mha1993@live.de> writes:

> This patch only changes the output of our error message, though.
> It does not change what ownership information we actually compare.
> So if we had a hypothetical user Bob that was part of the  domain
> example.com (SID S-1-5-21-100000001-1000000001-10000001-1001) and
> had been moved over from the example.org domain (old SID S-1-5-21-
> 2000000002-2000000002-20000002-2002) and we would detect a repository
> owned by bobs old SID, we would now lookup the old SID, find it
> attached to a user named example.com\Bob, look up Bobs  current SID,
> find it belongs to a user named example.com\Bob and print a confusing
> error message.

Yup, that is exactly the kind of breakage I was worried about.

Perhaps we should do something along the lines of ...

 - The erroring out should be done purely by SID comparison, as that
   is what we have been doing to protect the users.

 - When creating a message, use LookupAccountSidA() to come up with
   a pair of domain\user strings for the directory and the process
   to be used in the error message:

   - If they are different (which is expected to be the normal
     case), we just use the pair of strings.

   - If they are the same, show old and new SID in stringified form
     (hopefully different SIDs would strigify to different
     strings?), and optionally we give the domain\user string next
     to it.

... then?  Then we would emit an error message (in the best case)

    'directory' is owned by:
    'bob@example.org'
    but the current user is:
    'charlie@example.com'

and in a bad case we would instead see something like:

    'directory' is owned by:
    SID S-1-5-21-100000001-1000000001-10000001-1001 ('bob@example.org')
    but the current user is:
    SID S-1-5-21-200000002-2000000002-20000002-2002 ('bob@example.org')

which may still be serviceable.  I dunno.

[PATCH v5 0/1] Replace SID with domain/username on Windows

[Sören Krecker]

Hi everyone,

I change the error message. I Hope that it is now better for every one.

Thanks

Sören Krecker (1):
  Adds domain/username to error message

 compat/mingw.c | 64 ++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 51 insertions(+), 13 deletions(-)


base-commit: e79552d19784ee7f4bbce278fe25f93fbda196fa
-- 
2.39.2

[PATCH v5 1/1] Adds domain/username to error message

[Sören Krecker]

Adds domain/username in error message, if owner sid of repository and
user sid are not equal on windows systems.

Old Prompted error message:
'''
fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
'C:/Users/test/source/repos/git' is owned by:
	'S-1-5-21-571067702-4104414259-3379520149-500'
but the current user is:
	'S-1-5-21-571067702-4104414259-3379520149-1001'
To add an exception for this directory, call:

	git config --global --add safe.directory C:/Users/test/source/repos/git
'''

New prompted error massage:
'''
fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
'C:/Users/test/source/repos/git' is owned by:
        'DESKTOP-L78JVA6/Administrator' (S-1-5-21-571067702-4104414259-3379520149-500)
but the current user is:
        'DESKTOP-L78JVA6/test' (S-1-5-21-571067702-4104414259-3379520149-1001)
To add an exception for this directory, call:

        git config --global --add safe.directory C:/Users/test/source/repos/git
'''

Signed-off-by: Sören Krecker <soekkle@freenet.de>
---
 compat/mingw.c | 64 ++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 51 insertions(+), 13 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 42053c1f65..6240387205 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2684,6 +2684,26 @@ static PSID get_current_user_sid(void)
 	return result;
 }
 
+static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)
+{
+	SID_NAME_USE pe_use;
+	DWORD len_user = 0, len_domain = 0;
+	BOOL translate_sid_to_user;
+
+	/* returns only FALSE, because the string pointers are NULL*/
+	LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,
+			  &pe_use); 
+	/*Alloc needed space of the strings*/
+	ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); 
+	translate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user,
+				   *str, &len_domain, &pe_use);
+	if (translate_sid_to_user == FALSE)
+		FREE_AND_NULL(*str);
+	else
+		(*str)[len_domain] = '/';
+	return translate_sid_to_user;
+}
+
 static int acls_supported(const char *path)
 {
 	size_t offset = offset_1st_component(path);
@@ -2765,27 +2785,45 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
 			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;
+			LPSTR str1, str2, str3, str4, to_free1 = NULL, to_free3 = NULL, to_local_free2=NULL, to_local_free4=NULL;
 
-			if (ConvertSidToStringSidA(sid, &str1))
+			if (user_sid_to_user_name(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;
+			if (ConvertSidToStringSidA(sid, &str2))
+				to_local_free2 = str2;
 			else
 				str2 = "(inconvertible)";
+
+			if (!current_user_sid) {
+				str3 = "(none)";
+				str4 = "(none)";
+			}
+			else if (!IsValidSid(current_user_sid)) {
+				str3 = "(invalid)";
+				str4 = "(invalid)";
+			} else {
+				if (user_sid_to_user_name(current_user_sid,
+							  &str3))
+					to_free3 = str3;
+				else
+					str3 = "(inconvertible)";
+				if (ConvertSidToStringSidA(current_user_sid,
+							   &str4))
+					to_local_free4 = str4;
+				else
+					str4 = "(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);
+				    "\t'%s' (%s)\nbut the current user is:\n"
+				    "\t'%s' (%s)\n",
+				    path, str1, str2, str3, str4);
+			free(to_free1);
+			LocalFree(to_local_free2);
+			free(to_free3);
+			LocalFree(to_local_free4);
 		}
 	}
 
-- 
2.39.2

Re: [PATCH v5 1/1] Adds domain/username to error message

[Junio C Hamano]

Sören Krecker <soekkle@freenet.de> writes:

> Subject: Re: [PATCH v5 1/1] Adds domain/username to error message

Looking at past commits that worked on the area this patch touches,
namely, 7c83470e (mingw: be more informative when ownership check
fails on FAT32, 2022-08-08) and e883e04b (mingw: provide details
about unsafe directories' ownership, 2022-08-08), I would retitle
the commit perhaps like so:

    Subject: [PATCH v5] mingw: give more details about unsafe directory's ownership

if I were doing this patch.

> Adds domain/username in error message, if owner sid of repository and

"Adds" -> "Add".

> user sid are not equal on windows systems.
>
> Old Prompted error message:
> '''
> fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
> 'C:/Users/test/source/repos/git' is owned by:
> 	'S-1-5-21-571067702-4104414259-3379520149-500'
> but the current user is:
> 	'S-1-5-21-571067702-4104414259-3379520149-1001'
> To add an exception for this directory, call:
>
> 	git config --global --add safe.directory C:/Users/test/source/repos/git
> '''
>
> New prompted error massage:

"massage" -> "message".

I probably would drop two "prompted" from the above, too, if I were
doing this patch.

Thanks for working on making this error message more readable.  I'll
queue it when I see an Ack from Dscho.





> '''
> fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
> 'C:/Users/test/source/repos/git' is owned by:
>         'DESKTOP-L78JVA6/Administrator' (S-1-5-21-571067702-4104414259-3379520149-500)
> but the current user is:
>         'DESKTOP-L78JVA6/test' (S-1-5-21-571067702-4104414259-3379520149-1001)
> To add an exception for this directory, call:
>
>         git config --global --add safe.directory C:/Users/test/source/repos/git
> '''
>
> Signed-off-by: Sören Krecker <soekkle@freenet.de>
> ---
>  compat/mingw.c | 64 ++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 51 insertions(+), 13 deletions(-)
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 42053c1f65..6240387205 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -2684,6 +2684,26 @@ static PSID get_current_user_sid(void)
>  	return result;
>  }
>  
> +static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)
> +{
> +	SID_NAME_USE pe_use;
> +	DWORD len_user = 0, len_domain = 0;
> +	BOOL translate_sid_to_user;
> +
> +	/* returns only FALSE, because the string pointers are NULL*/
> +	LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,
> +			  &pe_use); 
> +	/*Alloc needed space of the strings*/
> +	ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); 
> +	translate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user,
> +				   *str, &len_domain, &pe_use);
> +	if (translate_sid_to_user == FALSE)
> +		FREE_AND_NULL(*str);
> +	else
> +		(*str)[len_domain] = '/';
> +	return translate_sid_to_user;
> +}
> +
>  static int acls_supported(const char *path)
>  {
>  	size_t offset = offset_1st_component(path);
> @@ -2765,27 +2785,45 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
>  			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;
> +			LPSTR str1, str2, str3, str4, to_free1 = NULL, to_free3 = NULL, to_local_free2=NULL, to_local_free4=NULL;
>  
> -			if (ConvertSidToStringSidA(sid, &str1))
> +			if (user_sid_to_user_name(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;
> +			if (ConvertSidToStringSidA(sid, &str2))
> +				to_local_free2 = str2;
>  			else
>  				str2 = "(inconvertible)";
> +
> +			if (!current_user_sid) {
> +				str3 = "(none)";
> +				str4 = "(none)";
> +			}
> +			else if (!IsValidSid(current_user_sid)) {
> +				str3 = "(invalid)";
> +				str4 = "(invalid)";
> +			} else {
> +				if (user_sid_to_user_name(current_user_sid,
> +							  &str3))
> +					to_free3 = str3;
> +				else
> +					str3 = "(inconvertible)";
> +				if (ConvertSidToStringSidA(current_user_sid,
> +							   &str4))
> +					to_local_free4 = str4;
> +				else
> +					str4 = "(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);
> +				    "\t'%s' (%s)\nbut the current user is:\n"
> +				    "\t'%s' (%s)\n",
> +				    path, str1, str2, str3, str4);
> +			free(to_free1);
> +			LocalFree(to_local_free2);
> +			free(to_free3);
> +			LocalFree(to_local_free4);
>  		}
>  	}

[PATCH v6 0/1] mingw: give more details about unsafe directory's ownership

[Sören Krecker]

So I change the commit message and the title of this commit.

Thanks for your feedback.

Sören Krecker (1):
  mingw: give more details about unsafe directory's ownership

 compat/mingw.c | 64 ++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 51 insertions(+), 13 deletions(-)


base-commit: e79552d19784ee7f4bbce278fe25f93fbda196fa
-- 
2.39.2

[PATCH v6 1/1] mingw: give more details about unsafe directory's ownership

[Sören Krecker]

Add domain/username in error message, if owner sid of repository and
user sid are not equal on windows systems.

Old error message:
'''
fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
'C:/Users/test/source/repos/git' is owned by:
	'S-1-5-21-571067702-4104414259-3379520149-500'
but the current user is:
	'S-1-5-21-571067702-4104414259-3379520149-1001'
To add an exception for this directory, call:

	git config --global --add safe.directory C:/Users/test/source/repos/git
'''

New error message:
'''
fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
'C:/Users/test/source/repos/git' is owned by:
        'DESKTOP-L78JVA6/Administrator' (S-1-5-21-571067702-4104414259-3379520149-500)
but the current user is:
        'DESKTOP-L78JVA6/test' (S-1-5-21-571067702-4104414259-3379520149-1001)
To add an exception for this directory, call:

        git config --global --add safe.directory C:/Users/test/source/repos/git
'''

Signed-off-by: Sören Krecker <soekkle@freenet.de>
---
 compat/mingw.c | 64 ++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 51 insertions(+), 13 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 42053c1f65..6240387205 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2684,6 +2684,26 @@ static PSID get_current_user_sid(void)
 	return result;
 }
 
+static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)
+{
+	SID_NAME_USE pe_use;
+	DWORD len_user = 0, len_domain = 0;
+	BOOL translate_sid_to_user;
+
+	/* returns only FALSE, because the string pointers are NULL*/
+	LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,
+			  &pe_use); 
+	/*Alloc needed space of the strings*/
+	ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); 
+	translate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user,
+				   *str, &len_domain, &pe_use);
+	if (translate_sid_to_user == FALSE)
+		FREE_AND_NULL(*str);
+	else
+		(*str)[len_domain] = '/';
+	return translate_sid_to_user;
+}
+
 static int acls_supported(const char *path)
 {
 	size_t offset = offset_1st_component(path);
@@ -2765,27 +2785,45 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
 			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;
+			LPSTR str1, str2, str3, str4, to_free1 = NULL, to_free3 = NULL, to_local_free2=NULL, to_local_free4=NULL;
 
-			if (ConvertSidToStringSidA(sid, &str1))
+			if (user_sid_to_user_name(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;
+			if (ConvertSidToStringSidA(sid, &str2))
+				to_local_free2 = str2;
 			else
 				str2 = "(inconvertible)";
+
+			if (!current_user_sid) {
+				str3 = "(none)";
+				str4 = "(none)";
+			}
+			else if (!IsValidSid(current_user_sid)) {
+				str3 = "(invalid)";
+				str4 = "(invalid)";
+			} else {
+				if (user_sid_to_user_name(current_user_sid,
+							  &str3))
+					to_free3 = str3;
+				else
+					str3 = "(inconvertible)";
+				if (ConvertSidToStringSidA(current_user_sid,
+							   &str4))
+					to_local_free4 = str4;
+				else
+					str4 = "(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);
+				    "\t'%s' (%s)\nbut the current user is:\n"
+				    "\t'%s' (%s)\n",
+				    path, str1, str2, str3, str4);
+			free(to_free1);
+			LocalFree(to_local_free2);
+			free(to_free3);
+			LocalFree(to_local_free4);
 		}
 	}
 
-- 
2.39.2

Re: [PATCH v6 1/1] mingw: give more details about unsafe directory's ownership

[Johannes Sixt]

Am 06.01.24 um 12:29 schrieb Sören Krecker:
> Add domain/username in error message, if owner sid of repository and
> user sid are not equal on windows systems.
> 
> Old error message:
> '''
> fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
> 'C:/Users/test/source/repos/git' is owned by:
> 	'S-1-5-21-571067702-4104414259-3379520149-500'
> but the current user is:
> 	'S-1-5-21-571067702-4104414259-3379520149-1001'
> To add an exception for this directory, call:
> 
> 	git config --global --add safe.directory C:/Users/test/source/repos/git
> '''
> 
> New error message:
> '''
> fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
> 'C:/Users/test/source/repos/git' is owned by:
>         'DESKTOP-L78JVA6/Administrator' (S-1-5-21-571067702-4104414259-3379520149-500)
> but the current user is:
>         'DESKTOP-L78JVA6/test' (S-1-5-21-571067702-4104414259-3379520149-1001)
> To add an exception for this directory, call:
> 
>         git config --global --add safe.directory C:/Users/test/source/repos/git
> '''

I am not a fan of putting everything and the kitchen sink inside quotes.
In particular, the single-quotes around the user names are unnecessary,
IMO. Would you mind dropping them? (I do not mean to remove the quotes
around the path names because you do not touch this part of the code.)

> 
> Signed-off-by: Sören Krecker <soekkle@freenet.de>
> ---
>  compat/mingw.c | 64 ++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 51 insertions(+), 13 deletions(-)
> 
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 42053c1f65..6240387205 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -2684,6 +2684,26 @@ static PSID get_current_user_sid(void)
>  	return result;
>  }
>  
> +static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)
> +{
> +	SID_NAME_USE pe_use;
> +	DWORD len_user = 0, len_domain = 0;
> +	BOOL translate_sid_to_user;
> +
> +	/* returns only FALSE, because the string pointers are NULL*/
> +	LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,
> +			  &pe_use); 
> +	/*Alloc needed space of the strings*/

This comment line doesn't follow our style. Please either fix that (add
blanks after /* and before */ or (my preference) remove it altogether;
the code is clear without it.

> +	ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); 
> +	translate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user,
> +				   *str, &len_domain, &pe_use);
> +	if (translate_sid_to_user == FALSE)

We prefer to write this condition as

	if (!translate_sid_to_user)

> +		FREE_AND_NULL(*str);
> +	else
> +		(*str)[len_domain] = '/';
> +	return translate_sid_to_user;
> +}
> +
>  static int acls_supported(const char *path)
>  {
>  	size_t offset = offset_1st_component(path);
> @@ -2765,27 +2785,45 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
>  			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;
> +			LPSTR str1, str2, str3, str4, to_free1 = NULL, to_free3 = NULL, to_local_free2=NULL, to_local_free4=NULL;

This line grew a bit long now. Maybe break it to have lines not exceed
80 characters? While you do so, please insert blanks around = signs.

>  
> -			if (ConvertSidToStringSidA(sid, &str1))
> +			if (user_sid_to_user_name(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;
> +			if (ConvertSidToStringSidA(sid, &str2))
> +				to_local_free2 = str2;
>  			else
>  				str2 = "(inconvertible)";
> +
> +			if (!current_user_sid) {
> +				str3 = "(none)";
> +				str4 = "(none)";
> +			}
> +			else if (!IsValidSid(current_user_sid)) {
> +				str3 = "(invalid)";
> +				str4 = "(invalid)";
> +			} else {
> +				if (user_sid_to_user_name(current_user_sid,
> +							  &str3))
> +					to_free3 = str3;
> +				else
> +					str3 = "(inconvertible)";
> +				if (ConvertSidToStringSidA(current_user_sid,
> +							   &str4))
> +					to_local_free4 = str4;
> +				else
> +					str4 = "(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);
> +				    "\t'%s' (%s)\nbut the current user is:\n"
> +				    "\t'%s' (%s)\n",
> +				    path, str1, str2, str3, str4);
> +			free(to_free1);
> +			LocalFree(to_local_free2);
> +			free(to_free3);
> +			LocalFree(to_local_free4);
>  		}
>  	}
>  

Aside from this, the patch works well for me. It is a real usability
improvement. Thank you for working on it.

-- Hannes

[PATCH v6 0/1] mingw: give more details about unsafe directory's

[Sören Krecker]

I have processed the points raised.


Sören Krecker (1):
  mingw: give more details about unsafe directory's ownership

 compat/mingw.c | 70 ++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 57 insertions(+), 13 deletions(-)


base-commit: e79552d19784ee7f4bbce278fe25f93fbda196fa
-- 
2.39.2

[PATCH v7 1/1] mingw: give more details about unsafe directory's ownership

[Sören Krecker]

Add domain/username in error message, if owner sid of repository and
user sid are not equal on windows systems.

Old error message:
'''
fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
'C:/Users/test/source/repos/git' is owned by:
	'S-1-5-21-571067702-4104414259-3379520149-500'
but the current user is:
	'S-1-5-21-571067702-4104414259-3379520149-1001'
To add an exception for this directory, call:

	git config --global --add safe.directory C:/Users/test/source/repos/git
'''

New error message:
'''
fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
'C:/Users/test/source/repos/git' is owned by:
        DESKTOP-L78JVA6/Administrator (S-1-5-21-571067702-4104414259-3379520149-500)
but the current user is:
        DESKTOP-L78JVA6/test (S-1-5-21-571067702-4104414259-3379520149-1001)
To add an exception for this directory, call:

        git config --global --add safe.directory C:/Users/test/source/repos/git
'''

Signed-off-by: Sören Krecker <soekkle@freenet.de>
---
 compat/mingw.c | 70 ++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 57 insertions(+), 13 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 42053c1f65..d85fae3747 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2684,6 +2684,30 @@ static PSID get_current_user_sid(void)
 	return result;
 }
 
+static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)
+{
+	SID_NAME_USE pe_use;
+	DWORD len_user = 0, len_domain = 0;
+	BOOL translate_sid_to_user;
+
+	/*
+	 * returns only FALSE, because the string pointers are NULL
+	 */
+	LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,
+			  &pe_use); 
+	/*
+	 * Alloc needed space of the strings
+	 */
+	ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); 
+	translate_sid_to_user = LookupAccountSidA(NULL, sid,
+	    (*str) + len_domain, &len_user, *str, &len_domain, &pe_use);
+	if (!translate_sid_to_user)
+		FREE_AND_NULL(*str);
+	else
+		(*str)[len_domain] = '/';
+	return translate_sid_to_user;
+}
+
 static int acls_supported(const char *path)
 {
 	size_t offset = offset_1st_component(path);
@@ -2765,27 +2789,47 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
 			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;
+			LPSTR str1, str2, str3, str4, to_free1 = NULL, 
+			    to_free3 = NULL, to_local_free2 = NULL,
+			    to_local_free4 = NULL;
 
-			if (ConvertSidToStringSidA(sid, &str1))
+			if (user_sid_to_user_name(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;
+			if (ConvertSidToStringSidA(sid, &str2))
+				to_local_free2 = str2;
 			else
 				str2 = "(inconvertible)";
+
+			if (!current_user_sid) {
+				str3 = "(none)";
+				str4 = "(none)";
+			}
+			else if (!IsValidSid(current_user_sid)) {
+				str3 = "(invalid)";
+				str4 = "(invalid)";
+			} else {
+				if (user_sid_to_user_name(current_user_sid,
+							  &str3))
+					to_free3 = str3;
+				else
+					str3 = "(inconvertible)";
+				if (ConvertSidToStringSidA(current_user_sid,
+							   &str4))
+					to_local_free4 = str4;
+				else
+					str4 = "(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);
+				    "\t%s (%s)\nbut the current user is:\n"
+				    "\t%s (%s)\n",
+				    path, str1, str2, str3, str4);
+			free(to_free1);
+			LocalFree(to_local_free2);
+			free(to_free3);
+			LocalFree(to_local_free4);
 		}
 	}
 
-- 
2.39.2

Re: [PATCH v6 0/1] mingw: give more details about unsafe directory's

[Dragan Simic]

On 2024-01-08 18:38, Sören Krecker wrote:
> I have processed the points raised.

If I may suggest, there's no need for a cover letter for a single patch. 
  If you want to include some notes in the patch submission, which aren't 
supposed to be part of the commit summary, you can do that in the patch 
itself.

> Sören Krecker (1):
>   mingw: give more details about unsafe directory's ownership
> 
>  compat/mingw.c | 70 ++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 57 insertions(+), 13 deletions(-)
> 
> 
> base-commit: e79552d19784ee7f4bbce278fe25f93fbda196fa

Re: [PATCH v7 1/1] mingw: give more details about unsafe directory's ownership

[Junio C Hamano]

Sören Krecker <soekkle@freenet.de> writes:

> Add domain/username in error message, if owner sid of repository and
> user sid are not equal on windows systems.

Will queue.  "git am --whitespace=fix" noticed numerous whitespace
breakages in the patch, which has been corrected on the receiving
end.  Please make sure your future patches will not have such
problems.

Thanks.


$ git am -s3c ./+sk-v7-mingw-ownership-report
Applying: mingw: give more details about unsafe directory's ownership
.git/rebase-apply/patch:23: trailing whitespace.
			  &pe_use); 
.git/rebase-apply/patch:27: trailing whitespace.
	ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); 
.git/rebase-apply/patch:45: trailing whitespace.
			LPSTR str1, str2, str3, str4, to_free1 = NULL, 
warning: 3 lines add whitespace errors.
.git/rebase-apply/patch:23: trailing whitespace.
			  &pe_use); 
.git/rebase-apply/patch:27: trailing whitespace.
	ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); 
.git/rebase-apply/patch:45: trailing whitespace.
			LPSTR str1, str2, str3, str4, to_free1 = NULL, 
warning: 3 lines applied after fixing whitespace errors.
Using index info to reconstruct a base tree...
M	compat/mingw.c
Falling back to patching base and 3-way merge...
Auto-merging compat/mingw.c

Re: [PATCH v7 1/1] mingw: give more details about unsafe directory's ownership

[Johannes Sixt]

Am 08.01.24 um 18:38 schrieb Sören Krecker:
> +static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)
> +{
> +	SID_NAME_USE pe_use;
> +	DWORD len_user = 0, len_domain = 0;
> +	BOOL translate_sid_to_user;
> +
> +	/*
> +	 * returns only FALSE, because the string pointers are NULL
> +	 */
> +	LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,
> +			  &pe_use); 

At this point, the function fails, so len_user and len_domain contain
the required buffer size (including the trailing NUL).

> +	/*
> +	 * Alloc needed space of the strings
> +	 */
> +	ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); 
> +	translate_sid_to_user = LookupAccountSidA(NULL, sid,
> +	    (*str) + len_domain, &len_user, *str, &len_domain, &pe_use);

At this point, if the function is successful, len_user and len_domain
contain the lengths of the names (without the trailing NUL).

> +	if (!translate_sid_to_user)
> +		FREE_AND_NULL(*str);
> +	else
> +		(*str)[len_domain] = '/';

Therefore, this overwrites the NUL after the domain name and so
concatenates the two names. Good.

I found this by dumping the values of the variables, because the
documentation of LookupAccountSid is not clear about the values that the
variables receive in the success case.

> +	return translate_sid_to_user;
> +}
> +

This patch looks good and works for me.

Acked-by: Johannes Sixt <j6t@kdbg.org>

Thank you!

-- Hannes

Re: [PATCH v7 1/1] mingw: give more details about unsafe directory's ownership

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

>> +	LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,
>> +			  &pe_use); 
>
> At this point, the function fails, so len_user and len_domain contain
> the required buffer size (including the trailing NUL).

So (*str)[len_domain] would be the trailing NUL after the domain
part in the next call?  Or would that be (*str)[len_domain-1]?  I am
puzzled by off-by-one with your "including the trailing NUL" remark.

>> +	/*
>> +	 * Alloc needed space of the strings
>> +	 */
>> +	ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); 

This obviously assumes for domain 'd' and user 'u', we want "d/u" and
len_domain must be 1+1 (including NUL) and len_user must be 1+1
(including NUL).  But then ...

>> +	translate_sid_to_user = LookupAccountSidA(NULL, sid,
>> +	    (*str) + len_domain, &len_user, *str, &len_domain, &pe_use);

... ((*str)+len_domain) is presumably the beginning of the user
part, and (*str)+0) is where the domain part is to be stored.

Because len_domain includes the terminating NUL for the domain part,
(*str)[len_domain-1] is that NUL, no?  And that is what you want to
overwrite to make the two strings <d> <NUL> <u> <NUL> into a single
one <d> <slash> <u> <NUL>.  So...

> At this point, if the function is successful, len_user and len_domain
> contain the lengths of the names (without the trailing NUL).
>
>> +	if (!translate_sid_to_user)
>> +		FREE_AND_NULL(*str);
>> +	else
>> +		(*str)[len_domain] = '/';

... this offset looks fishy to me.  Am I off-by-one?

> Therefore, this overwrites the NUL after the domain name and so
> concatenates the two names. Good.
>
> I found this by dumping the values of the variables, because the
> documentation of LookupAccountSid is not clear about the values that the
> variables receive in the success case.
>
>> +	return translate_sid_to_user;
>> +}
>> +
>
> This patch looks good and works for me.
>
> Acked-by: Johannes Sixt <j6t@kdbg.org>
>
> Thank you!
>
> -- Hannes

Re: [PATCH v7 1/1] mingw: give more details about unsafe directory's ownership

[Johannes Sixt]

Am 09.01.24 um 21:06 schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
> 
>>> +	LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,
>>> +			  &pe_use); 
>>
>> At this point, the function fails, so len_user and len_domain contain
>> the required buffer size (including the trailing NUL).
> 
> So (*str)[len_domain] would be the trailing NUL after the domain
> part in the next call?  Or would that be (*str)[len_domain-1]?  I am
> puzzled by off-by-one with your "including the trailing NUL" remark.

"Required buffer size" must count the trailing NUL. So, the NUL would be
at (*str)[len_domain-1].

> 
>>> +	/*
>>> +	 * Alloc needed space of the strings
>>> +	 */
>>> +	ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); 
> 
> This obviously assumes for domain 'd' and user 'u', we want "d/u" and
> len_domain must be 1+1 (including NUL) and len_user must be 1+1
> (including NUL).  But then ...

So, this allocates the exact amount that is required to contain the
names with the trailing NULs: 1+1 plus 1+1 in this example.

> 
>>> +	translate_sid_to_user = LookupAccountSidA(NULL, sid,
>>> +	    (*str) + len_domain, &len_user, *str, &len_domain, &pe_use);
> 
> ... ((*str)+len_domain) is presumably the beginning of the user
> part, and (*str)+0) is where the domain part is to be stored.

Correct.

> 
> Because len_domain includes the terminating NUL for the domain part,
> (*str)[len_domain-1] is that NUL, no?  And that is what you want to
> overwrite to make the two strings <d> <NUL> <u> <NUL> into a single
> one <d> <slash> <u> <NUL>.  So...

But after a successful call, len_domain and len_user have been modified
to contain the lengths of the names (not counting the NULs), so, here
the NUL is at (*str)[len_domain]...

> 
>> At this point, if the function is successful, len_user and len_domain
>> contain the lengths of the names (without the trailing NUL).
>>
>>> +	if (!translate_sid_to_user)
>>> +		FREE_AND_NULL(*str);
>>> +	else
>>> +		(*str)[len_domain] = '/';
> 
> ... this offset looks fishy to me.  Am I off-by-one?

... and this offset is correct.

I followed the same train of thought and suspected an off-by-one error,
too, and was perplexed that I see a correct output. The documentation of
LookupAccountSid is unclear that the variables change values across the
(successful) call, but my tests verified that the change does happen.

>> Therefore, this overwrites the NUL after the domain name and so
>> concatenates the two names. Good.
>>
>> I found this by dumping the values of the variables, because the
>> documentation of LookupAccountSid is not clear about the values that the
>> variables receive in the success case.
>>
>>> +	return translate_sid_to_user;
>>> +}
>>> +
>>
>> This patch looks good and works for me.
>>
>> Acked-by: Johannes Sixt <j6t@kdbg.org>
>>
>> Thank you!
>>
>> -- Hannes

-- Hannes

Re: [PATCH v7 1/1] mingw: give more details about unsafe directory's ownership

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

>> Because len_domain includes the terminating NUL for the domain part,
>> (*str)[len_domain-1] is that NUL, no?  And that is what you want to
>> overwrite to make the two strings <d> <NUL> <u> <NUL> into a single
>> one <d> <slash> <u> <NUL>.  So...
>
> But after a successful call, len_domain and len_user have been modified
> to contain the lengths of the names (not counting the NULs), ...

Ah, that is what I missed.  Thanks.