From: Matthias Aßhauer Date: Wed, 03 Jan 2024 08:21:16 GMT Subject: Re: [PATCH V4 1/1] Replace SID with domain/username Message-ID: In-Reply-To: On Tue, 2 Jan 2024, Junio C Hamano wrote: > Sören Krecker 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. > >> } >> } > >