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

From: Matthias Aßhauer <mha1993@live.de>
Date: Jan 3, 2024, 08:21 UTC
Message-ID: <DB9P250MB0692C8B4D93ED92FEE680AA9A560A@DB9P250MB0692.EURP250.PROD.OUTLOOK.COM>
In-Reply-To: <xmqqa5pnckm4.fsf@gitster.g>

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

Show 10 quoted lines

> 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.

Show 6 quoted lines

> 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

Show 19 quoted lines

>
> 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.

Show 67 quoted lines

> 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.
>
>>  		}
>>  	}
>
>