Windows well-known-sids

[amalachow]

During a recent upgrade we noted a change in behavior when running Git in a Window's container. We were able to narrow down to a the change starting with 2.46.0.windows.1 persisting through the latest releases (related #5137).

Digging into the msys-runtime diffs between the last working (2.45.2.windows.1), the change stems what I believe is a combination of a24bc22 and 8f9fcab. Specifically the SID for the ContainerAdministrator is S-1-5-93-2-1 which is matching the condition if (acc_type == SidTypeUser && (sid_sub_auth_count (sid) <= 3 || sid_id_auth (sid) == 11)). This correctly allows for NSS_SCHEME_ENV in get_home; however, the local home isn't being propagated into the in memory passwd any longer when a builtin account is forced into the SidTypeWellKnownGroup type but is not well_known_system_sid. It looks like its intentional to stop altering the home resolution for non-user's but I'm trying to understand the subauthority logic that's employed.

Reading through Microsoft's SID documentation there isn't any clear indication that the number of subauthorities would be indicative of a builtin account per the comment in uinfo.cc, Builtin account? SYSTEM, for instance, is returned as SidTypeUser, if a process is running as LocalSystem service. Is this an optimization to avoid the use of IsWellKnownSid with WinBuiltinDomainSid or are is there supporting documentation that the subauthority count less than or equal to 3 is viable to disambiguate that of a domain identifier? Trying to improve my comprehension of the logic but finding Microsoft's documentation on the APIs and details hard to discover.

Naively iterating the WELL_KNOWN_SID_TYPE enum calling CreateWellKnownSid would indicate that the subauthority count for the local machine will be strictly less than 3 in all instances except WinUserModeDriversSid, which is contradictory to the logic above though I'm not sure it'd sufficiently cover the breadth of Windows versions etc.

[dscho]

I cannot claim to understand the full details, either; The commits you refer to are part of this PR: git-for-windows/msys2-runtime#63. This PR corresponds to this thread on the cygwin-patches mailing list where I tried to upstream the patches carried in Git for Windows. The PR simply backported the final version of those patches.

Digging into the msys-runtime diffs between the last working (2.45.2.windows.1), the change stems what I believe is a combination of a24bc22 and 8f9fcab.

Did you really mean git-for-windows/msys2-runtime@8f9fcab, and not git-for-windows/msys2-runtime@94c27b7 instead?

In any case, I am puzzled by the change of behavior, given that both before and after that change, the sub authorities <= 3 are handled.
If I understand correctly, the change of behavior stems from this hunk? That change was made in response to this reasonable-sounding feedback on the cygwin-patches mailing list.

[amalachow]

Sorry for the confusion git-for-windows/msys2-runtime@8f9fcab looked to have been merged upstream so the cleanup in git-for-windows/msys2-runtime@94c27b7 is immaterial as the logic still exists. I linked to that change as it handles NSS_SCHEME_ENV for the account.

The change in behavior actually manifests because the local home which is used by the above change is no longer propagated in this hunk when the acc_type is forced into SidTypeWellKnownGroup (https://github.com/cygwin/cygwin/blob/cygwin-3.6.5/winsup/cygwin/uinfo.cc#L2317) but it is not the well_known_system_sid. From what I can tell the feedback in this thread caused the code to be replaced with IIS specific handling. Reverting that hunk e.g. does respect the configuration as expected; however, it is not clear that the criteria for non-user accounts upstream is correct (hence the original question).

diff --git a/winsup/cygwin/uinfo.cc b/winsup/cygwin/uinfo.cc
index e68aeb9e03..1ed37425ea 100644
--- a/winsup/cygwin/uinfo.cc
+++ b/winsup/cygwin/uinfo.cc
@@ -2818,10 +2818,11 @@ pwdgrp::fetch_account_from_windows (fetch_user_arg_t &arg, cyg_ldap *pldap)
      logon.  Unless it's the SYSTEM account.  This conveniently allows to
      logon interactively as SYSTEM for debugging purposes. */
   else if (acc_type != SidTypeUser && sid != well_known_system_sid)
-    __small_sprintf (linebuf, "%W:*:%u:%u:U-%W\\%W,%s:/:/sbin/nologin",
+    __small_sprintf (linebuf, "%W:*:%u:%u:U-%W\\%W,%s:%s:/sbin/nologin",
                     posix_name, uid, gid,
                     dom, name,
-                    sid.string ((char *) sidstr));
+                    sid.string ((char *) sidstr),
+                    home);
   else
     __small_sprintf (linebuf, "%W:*:%u:%u:%s%sU-%W\\%W,%s:%s%W:%s",
                     posix_name, uid, gid,

[dscho]

That looks quite plausible to me. Maybe you could transmogrify that finding into a commit message and then send the patch to cygwin-patches@cygwin.com ? The reviewers on that mailing list would know a lot better than me whether this patch can be applied as-is, or whether it needs further adjustments.