Fix for test_access_control_simple__permits_user_login_based_on_group samba failure by justin-stephenson · Pull Request #8263 · SSSD/sssd

justin-stephenson · 2025-12-03T15:05:27Z

No description provided.

justin-stephenson · 2025-12-03T21:34:05Z

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 · 2025-12-05T10:36:20Z

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 · 2025-12-05T14:51:16Z

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 · 2025-12-05T16:00:16Z

@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 · 2025-12-05T16:03:18Z

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 · 2025-12-05T16:10:32Z

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

src/util/string_utils.c

src/tests/cmocka/test_string_utils.c

src/util/string_utils.c

src/providers/simple/simple_access_check.c

justin-stephenson · 2025-12-09T18:06:48Z

With these changes maybe simple_check_process_group() could be simplified down to the following?

if (!is_sid) {
        state->group_names[state->num_names] = talloc_strdup(state->group_names,
                                                             name);
        DEBUG(SSSDBG_TRACE_INTERNAL, "Adding group %s\n", name);
        state->num_names++;
else {
	
	..
	/* Group with a GID needs resolving */
	
	...code to add group to state->lookup_groups
}

sumit-bose

Hi,

thank you for the fix, I have no further comments to the code, ACK.

Since there is clear difference to the plain ldap version of the test wouldn't it make sense to enable the samab topology at least for this test in PR CI?

bye,
Sumit

Reviewed-by: Alexey Tikhonov <atikhono@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>

After changes from 'Dont store GID for non-posix groups', the simple access provider was not identifying group with names in SID format as group that needs to be resolved because they are no longer stored temporarily as non-POSIX. Add code to check for, and resolve any group names which are SIDs returned from initgroups (AD provider). Reviewed-by: Alexey Tikhonov <atikhono@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>

sssd-bot · 2025-12-10T10:39:01Z

The pull request was accepted by @sumit-bose with the following PR CI status:

There are unsuccessful or unfinished checks. Make sure that the failures are not related to this pull request before merging.

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). Drop the preferred LDAP topology, to ensure this test runs against all providers to catch potential issues like this.

justin-stephenson · 2025-12-10T14:44:14Z

Hi,

thank you for the fix, I have no further comments to the code, ACK.

Since there is clear difference to the plain ldap version of the test wouldn't it make sense to enable the samab topology at least for this test in PR CI?

bye, Sumit

Thank you, I added a commit to drop the preferred topology line for this test.

alexey-tikhonov · 2025-12-10T15:10:03Z

I added a commit to drop the preferred topology line for this test.

This patch doesn't have 'reviwed-by' trailer now. But probably doesn't matter...

justin-stephenson force-pushed the master_test_simple_provider_test_samba branch 3 times, most recently from 819a490 to 0da7d09 Compare December 3, 2025 19:13

justin-stephenson changed the title ~~Master test simple provider test samba~~ Fix for test_access_control_simple__permits_user_login_based_on_group samba failure Dec 3, 2025

alexey-tikhonov added the backport-to-sssd-2-9 label Dec 4, 2025

alexey-tikhonov requested a review from sumit-bose December 4, 2025 13:57

alexey-tikhonov assigned sumit-bose Dec 4, 2025

justin-stephenson force-pushed the master_test_simple_provider_test_samba branch 2 times, most recently from ffb69e0 to 028d641 Compare December 5, 2025 15:59

justin-stephenson marked this pull request as ready for review December 5, 2025 15:59

justin-stephenson added the Waiting for review label Dec 5, 2025

gemini-code-assist bot reviewed Dec 5, 2025

View reviewed changes

src/util/string_utils.c Outdated Show resolved Hide resolved

src/tests/cmocka/test_string_utils.c Outdated Show resolved Hide resolved

justin-stephenson force-pushed the master_test_simple_provider_test_samba branch 2 times, most recently from aa38d6b to 03579c4 Compare December 5, 2025 16:20

alexey-tikhonov self-assigned this Dec 8, 2025

alexey-tikhonov self-requested a review December 8, 2025 09:55

alexey-tikhonov reviewed Dec 8, 2025

View reviewed changes

src/util/string_utils.c Outdated Show resolved Hide resolved

alexey-tikhonov reviewed Dec 8, 2025

View reviewed changes

src/providers/simple/simple_access_check.c Outdated Show resolved Hide resolved

alexey-tikhonov reviewed Dec 8, 2025

View reviewed changes

src/providers/simple/simple_access_check.c Show resolved Hide resolved

alexey-tikhonov reviewed Dec 8, 2025

View reviewed changes

src/providers/simple/simple_access_check.c Outdated Show resolved Hide resolved

justin-stephenson force-pushed the master_test_simple_provider_test_samba branch from 03579c4 to 1da3c1e Compare December 9, 2025 13:59

alexey-tikhonov approved these changes Dec 10, 2025

View reviewed changes

sumit-bose approved these changes Dec 10, 2025

View reviewed changes

sumit-bose added Accepted and removed Waiting for review labels Dec 10, 2025

justin-stephenson added 2 commits December 10, 2025 10:38

util: Add string_begins_with() helper

0e67823

Reviewed-by: Alexey Tikhonov <atikhono@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>

sssd-bot force-pushed the master_test_simple_provider_test_samba branch from 1da3c1e to 5ad6e20 Compare December 10, 2025 10:39

justin-stephenson merged commit 4482fac into SSSD:master Dec 10, 2025
17 of 18 checks passed

justin-stephenson deleted the master_test_simple_provider_test_samba branch December 10, 2025 15:25

sssd-bot mentioned this pull request Dec 10, 2025

[autobackport: sssd-2-9] Fix for test_access_control_simple__permits_user_login_based_on_group samba failure #8293

Merged

Conversation

justin-stephenson commented Dec 3, 2025

Uh oh!

justin-stephenson commented Dec 3, 2025

Uh oh!

sumit-bose commented Dec 5, 2025

Uh oh!

justin-stephenson commented Dec 5, 2025

Uh oh!

justin-stephenson commented Dec 5, 2025

Uh oh!

justin-stephenson commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justin-stephenson commented Dec 5, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

justin-stephenson commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sumit-bose left a comment

Choose a reason for hiding this comment

Uh oh!

sssd-bot commented Dec 10, 2025

Uh oh!

justin-stephenson commented Dec 10, 2025

Uh oh!

alexey-tikhonov commented Dec 10, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

justin-stephenson commented Dec 5, 2025 •

edited

Loading

justin-stephenson commented Dec 9, 2025 •

edited

Loading