Fix for test_access_control_simple__permits_user_login_based_on_group samba failure

[justin-stephenson]

No description provided.

[justin-stephenson]

@sumit-bose

The changes from Dont store GID for non-posix groups commit 85b632d have resulted in the test test_access_control_simple__permits_user_login_based_on_group to fail, specifically when run against samba (ipa and ldap succeed). You can see this failure in the sssd-2-9 branch CI run as as you have pointed out here #8260 (comment)

The reason it isn't failing for PRs against master is because only ldap topology is being executed for this test due to:

@pytest.mark.preferred_topology(KnownTopology.LDAP)

Here you can see this test failing against master: https://github.com/SSSD/sssd/actions/runs/19898518551/job/57035518953

Regarding the actual failure, in simple_check_get_groups_send() we see this comment:

        /* Some providers (like the AD provider) might perform initgroups
         * without resolving the group names. In order for the simple access
         * provider to work correctly, we need to resolve the groups before
         * performing the access check. In AD provider, the situation is
         * even more tricky b/c the groups HAVE name, but their name
         * attribute is set to SID and they are set as non-POSIX
         */

However, what happens now after the Dont store GID for non-posix groups changes is the groups returned from initgroups have isPosix: True but functions like simple_check_process_group and simple_resolve_group_check expect these groups to be non-POSIX therefore we don't reach the expected [simple_check_process_group] (0x2000): [RID#10] Adding GID 1934600513

(2025-12-02 16:58:01): [be[test]] [simple_access_check_send] (0x0200): [RID#12] Simple access check for user1@test
(2025-12-02 16:58:01): [be[test]] [simple_check_get_groups_send] (0x1000): [RID#12] Looking up groups for user user1@test
(2025-12-02 16:58:01): [be[test]] [sss_domain_get_state] (0x2000): [RID#12] Domain test is Active
(2025-12-02 16:58:01): [be[test]] [simple_check_get_groups_send] (0x0400): [RID#12] User user1@test is a member of 2 supplemental groups
(2025-12-02 16:58:01): [be[test]] [simple_check_get_groups_send] (0x0400): [RID#12] JS-inside group_count loop
(2025-12-02 16:58:01): [be[test]] [simple_check_process_group] (0x0400): [RID#12] JS-Checking group: [S-1-5-21-3626845161-2982636267-1767198038-1108@test], [26001108], [True]
(2025-12-02 16:58:01): [be[test]] [simple_check_process_group] (0x2000): [RID#12] Adding group S-1-5-21-3626845161-2982636267-1767198038-1108@test
(2025-12-02 16:58:01): [be[test]] [simple_check_get_groups_send] (0x0400): [RID#12] JS-inside group_count loop
(2025-12-02 16:58:01): [be[test]] [simple_check_process_group] (0x0400): [RID#12] JS-Checking group: [S-1-5-21-3626845161-2982636267-1767198038-513@test], [26000513], [True]
(2025-12-02 16:58:01): [be[test]] [simple_check_process_group] (0x2000): [RID#12] Adding group S-1-5-21-3626845161-2982636267-1767198038-513@test
(2025-12-02 16:58:01): [be[test]] [simple_check_get_groups_primary] (0x0400): [RID#12] JS-simple_check_get_groups_primary
(2025-12-02 16:58:01): [be[test]] [simple_check_process_group] (0x0400): [RID#12] JS-Checking group: [S-1-5-21-3626845161-2982636267-1767198038-513@test], [26000513], [True]
(2025-12-02 16:58:01): [be[test]] [simple_check_process_group] (0x2000): [RID#12] Adding group S-1-5-21-3626845161-2982636267-1767198038-513@test
(2025-12-02 16:58:01): [be[test]] [simple_check_get_groups_send] (0x0400): [RID#12] All groups had name attribute
(2025-12-02 16:58:01): [be[test]] [simple_check_groups] (0x4000): [RID#12] Checking against allow list group name [group1@test].
(2025-12-02 16:58:01): [be[test]] [sss_domain_get_state] (0x2000): [RID#12] Domain test is Active
(2025-12-02 16:58:01): [be[test]] [simple_check_groups] (0x4000): [RID#12] Checking against deny list group name [group2@test].
(2025-12-02 16:58:01): [be[test]] [sss_domain_get_state] (0x2000): [RID#12] Domain test is Active
(2025-12-02 16:58:01): [be[test]] [simple_access_check_done] (0x2000): [RID#12] Group check done
(2025-12-02 16:58:01): [be[test]] [simple_access_check_recv] (0x1000): [RID#12] Access not granted

This PR adds a check if the group name begins with a SID prefix, and resolves these groups if it does have a sid prefix instead of adding the group. it makes the test pass now but to me it feels like the wrong way to fix this issue. Can you provide some guidance on what would be the best way to update the simple access check code behavior for AD groups?

[sumit-bose]

Hi,

your solution would work. As an alternative I would like to suggest to consider to revert two of the changes from the original patch:

diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
index cfc3b9e54..e41ee98e1 100644
--- a/src/db/sysdb_ops.c
+++ b/src/db/sysdb_ops.c
@@ -2058,10 +2058,8 @@ int sysdb_add_basic_group(struct sss_domain_info *domain,
     ret = sysdb_add_string(msg, SYSDB_NAME, name);
     if (ret) goto done;
 
-    if (is_posix) {
-        ret = sysdb_add_ulong(msg, SYSDB_GIDNUM, (unsigned long)gid);
-        if (ret) goto done;
-    }
+    ret = sysdb_add_ulong(msg, SYSDB_GIDNUM, (unsigned long)gid);
+    if (ret) goto done;
 
     /* creation time */
     ret = sysdb_add_ulong(msg, SYSDB_CREATE_TIME, (unsigned long)time(NULL));
diff --git a/src/providers/ldap/sdap_async_initgroups_ad.c b/src/providers/ldap/sdap_async_initgroups_ad.c
index c8f82d7ed..fb80c9242 100644
--- a/src/providers/ldap/sdap_async_initgroups_ad.c
+++ b/src/providers/ldap/sdap_async_initgroups_ad.c
@@ -640,7 +640,7 @@ errno_t sdap_ad_save_group_membership_with_idmapping(const char *username,
             }
 
      ret = sysdb_add_incomplete_group(domain, name, gid,
-                                             NULL, sid, NULL, gid != 0, now);
+                                             NULL, sid, NULL, false, now);
             if (ret == ERR_GID_DUPLICATED) {
                 /* In case o group id-collision, do:
                  * - Delete the group from sysdb

This would allow to have groups in the cache which have a non-zero GID but are (temporary) marked as non-POSIX. Then the test would pass without changes to the simple access provider code.

Another alternative might be a new flag which indicates that the name is not resolved yet, but this would require more code to make sure it is removed. So I think using the is_posix=False is easier and less error prone.

Please let me know what you think. As said, I agree with your solution as well and would ACK it.

bye,
Sumit

[justin-stephenson]

Hi,

your solution would work. As an alternative I would like to suggest to consider to revert two of the changes from the original patch:

diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
index cfc3b9e54..e41ee98e1 100644
--- a/src/db/sysdb_ops.c
+++ b/src/db/sysdb_ops.c
@@ -2058,10 +2058,8 @@ int sysdb_add_basic_group(struct sss_domain_info *domain,
     ret = sysdb_add_string(msg, SYSDB_NAME, name);
     if (ret) goto done;
 
-    if (is_posix) {
-        ret = sysdb_add_ulong(msg, SYSDB_GIDNUM, (unsigned long)gid);
-        if (ret) goto done;
-    }
+    ret = sysdb_add_ulong(msg, SYSDB_GIDNUM, (unsigned long)gid);
+    if (ret) goto done;
 
     /* creation time */
     ret = sysdb_add_ulong(msg, SYSDB_CREATE_TIME, (unsigned long)time(NULL));
diff --git a/src/providers/ldap/sdap_async_initgroups_ad.c b/src/providers/ldap/sdap_async_initgroups_ad.c
index c8f82d7ed..fb80c9242 100644
--- a/src/providers/ldap/sdap_async_initgroups_ad.c
+++ b/src/providers/ldap/sdap_async_initgroups_ad.c
@@ -640,7 +640,7 @@ errno_t sdap_ad_save_group_membership_with_idmapping(const char *username,
             }
 
      ret = sysdb_add_incomplete_group(domain, name, gid,
-                                             NULL, sid, NULL, gid != 0, now);
+                                             NULL, sid, NULL, false, now);
             if (ret == ERR_GID_DUPLICATED) {
                 /* In case o group id-collision, do:
                  * - Delete the group from sysdb

This would allow to have groups in the cache which have a non-zero GID but are (temporary) marked as non-POSIX. Then the test would pass without changes to the simple access provider code.

Another alternative might be a new flag which indicates that the name is not resolved yet, but this would require more code to make sure it is removed. So I think using the is_posix=False is easier and less error prone.

Please let me know what you think. As said, I agree with your solution as well and would ACK it.

bye, Sumit

Thank you for your comments.

Of those options I prefer to go with the current code in this PR, I will clean it up a bit first however.

I find having groups in the cache with a non-zero GID temporarily marked as non-POSIX to be confusing behavior, and flipping non-POSIX to POSIX during SSSD operations seems undesirable IMO.

I will ping you when this PR is ready for review.

[justin-stephenson]

@sumit-bose PR is updated now, just waiting for CI. I confirmed the test passes now with https://github.com/SSSD/sssd/actions/runs/19967886836/job/57264520857?pr=8263 CI run

[justin-stephenson]

Regarding string helper string_begins_with I don't insist on adding it, I can remove it if anyone feels it is overkill then just using a single strncmp call for the same result.

If we keep it, I can plan to create a separate PR later to use this helper in other parts of SSSD code.

[justin-stephenson]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a fix for an access control issue where group-based login fails in certain Samba configurations. The core of the fix is to ensure that group names that are still in SID format are properly resolved before being used in access checks. This is achieved by adding a new utility function string_begins_with to detect SID-formatted names and updating the simple access provider logic to trigger resolution for such groups.

My review identified a critical null pointer dereference vulnerability in the new string_begins_with function, which could lead to a crash. I also found an incorrect test case for this function that should be corrected. I've provided suggestions for both issues.