Skip to content

[autobackport: sssd-2-9] Dont store gid 0 for non-posix groups#8184

Merged
alexey-tikhonov merged 3 commits intoSSSD:sssd-2-9from
sssd-bot:SSSD-sssd-backport-pr8075-to-sssd-2-9
Nov 20, 2025
Merged

[autobackport: sssd-2-9] Dont store gid 0 for non-posix groups#8184
alexey-tikhonov merged 3 commits intoSSSD:sssd-2-9from
sssd-bot:SSSD-sssd-backport-pr8075-to-sssd-2-9

Conversation

@sssd-bot
Copy link
Contributor

@sssd-bot sssd-bot commented Nov 12, 2025

This is an automatic backport of PR#8075 Dont store gid 0 for non-posix groups to branch sssd-2-9, created by @justin-stephenson.

Please make sure this backport is correct.

Note

The commits were cherry-picked without conflicts.

You can push changes to this pull request

git remote add sssd-bot git@github.com:sssd-bot/sssd.git
git fetch sssd-bot refs/heads/SSSD-sssd-backport-pr8075-to-sssd-2-9
git checkout SSSD-sssd-backport-pr8075-to-sssd-2-9
git push sssd-bot SSSD-sssd-backport-pr8075-to-sssd-2-9

Original commits

  • 936dbfb - SYSDB: Add sysdb_add_bool()
  • dc9188b - SYSDB: Dont store gid 0 for non-posix groups
  • d953045 - SDAP: Remove sdap_store_group_with_gid()

Backported commits

  • dc2e805 - SYSDB: Add sysdb_add_bool()
  • 03e7751 - SYSDB: Dont store gid 0 for non-posix groups
  • 25de302 - SDAP: Remove sdap_store_group_with_gid()

Original Pull Request Body

gemini-code-assist[bot]
gemini-code-assist bot reviewed Nov 12, 2025
View reviewed changes
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request aims to prevent storing a GID of 0 for non-POSIX groups. The changes involve modifying sysdb_add_basic_group to conditionally store the gidNumber and updating its callers. While the changes for groups seem correct, I've found a couple of issues related to user handling that seem to have been introduced as part of this refactoring. One issue may lead to users being created without a posix attribute, and another relates to an inconsistent handling of gidNumber for non-POSIX users compared to non-POSIX groups.

@alexey-tikhonov alexey-tikhonov added the no-backport This should go to target branch only. label Nov 12, 2025
@alexey-tikhonov
Copy link
Member

alexey-tikhonov commented Nov 12, 2025

@sumit-bose, do you want to take a look?

@alexey-tikhonov
Copy link
Member

alexey-tikhonov commented Nov 14, 2025 •
edited
Loading

@justin-stephenson, this backport failed test_access_control_simple__permits_user_login_based_on_group-samba several times in a row.
All system tests were green in the original PR, but it didn't run on C9S.
Test fails on SSS_PAM_ACCT_MGMT:

[dp_pam_handler_send] (0x0100): Got request with the following data
[pam_print_data] (0x0100): command: SSS_PAM_ACCT_MGMT
[pam_print_data] (0x0100): domain: test
[pam_print_data] (0x0100): user: user1@test
[pam_print_data] (0x0100): service: sshd
[pam_print_data] (0x0100): tty: ssh
[pam_print_data] (0x0100): ruser: 
[pam_print_data] (0x0100): rhost: ::1
[pam_print_data] (0x0100): authtok type: 0 (No authentication token available)
[pam_print_data] (0x0100): newauthtok type: 0 (No authentication token available)
[pam_print_data] (0x0100): priv: 1
[pam_print_data] (0x0100): cli_pid: 1008481
[pam_print_data] (0x0100): child_pid: 0
[pam_print_data] (0x0100): logon name: not set
[pam_print_data] (0x0100): flags: 0
[dp_attach_req] (0x0400): [RID#12] DP Request [PAM Account #12]: REQ_TRACE: New request. [sssd.pam CID #2] Flags [0000].
[dp_attach_req] (0x0400): [RID#12] Number of active DP request: 1
[sss_domain_get_state] (0x2000): [RID#12] Domain test is Active
[sss_domain_get_state] (0x2000): [RID#12] Domain test is Active
[simple_access_obtain_filter_lists] (0x0200): [RID#12] Allow users list is empty.
[simple_access_obtain_filter_lists] (0x0200): [RID#12] Deny users list is empty.
[sss_parse_name] (0x0200): [RID#12] Domain not provided!
[sss_parse_name] (0x0200): [RID#12] Domain not provided!
[simple_access_check_send] (0x0200): [RID#12] Simple access check for user1@test
[simple_check_get_groups_send] (0x1000): [RID#12] Looking up groups for user user1@test
[sss_domain_get_state] (0x2000): [RID#12] Domain test is Active
[simple_check_get_groups_send] (0x0400): [RID#12] User user1@test is a member of 2 supplemental groups
[simple_check_process_group] (0x2000): [RID#12] Adding group S-1-5-21-1203729467-969503546-995321174-513@test
[simple_check_process_group] (0x2000): [RID#12] Adding group S-1-5-21-1203729467-969503546-995321174-1108@test
[simple_check_process_group] (0x2000): [RID#12] Adding group S-1-5-21-1203729467-969503546-995321174-513@test
[simple_check_get_groups_send] (0x0400): [RID#12] All groups had name attribute
[simple_check_groups] (0x4000): [RID#12] Checking against allow list group name [group1@test].
[sss_domain_get_state] (0x2000): [RID#12] Domain test is Active
[simple_check_groups] (0x4000): [RID#12] Checking against deny list group name [group2@test].
[sss_domain_get_state] (0x2000): [RID#12] Domain test is Active
[simple_access_check_done] (0x2000): [RID#12] Group check done
[simple_access_check_recv] (0x1000): [RID#12] Access not granted

Could you please take a look?
Perhaps something else needs backporting to sssd-2-9...

@justin-stephenson justin-stephenson force-pushed the SSSD-sssd-backport-pr8075-to-sssd-2-9 branch from a5e0785 to 25de302 Compare November 18, 2025 16:10
@justin-stephenson
Copy link
Contributor

justin-stephenson commented Nov 19, 2025

Could you please take a look? Perhaps something else needs backporting to sssd-2-9...

This test seems to be passing now? I tried re-running also and it was still green.

tests/test_access_control_simple.py::test_access_control_simple__permits_user_login_based_on_group (samba) PASSED [ 70%]

@alexey-tikhonov
Copy link
Member

alexey-tikhonov commented Nov 19, 2025

This test seems to be passing now?

Even without topology markers backported?

@justin-stephenson
Copy link
Contributor

justin-stephenson commented Nov 19, 2025

This test seems to be passing now?

Even without topology markers backported?

Correct, no idea what changed. Let me try to re-run a couple more times.

@justin-stephenson @sssd-bot
SYSDB: Add sysdb_add_bool()
7e1bf88 
Reviewed-by: Alexey Tikhonov <atikhono@redhat.com>
Reviewed-by: Sumit Bose <sbose@redhat.com>
(cherry picked from commit 936dbfb)
@justin-stephenson @sssd-bot
SYSDB: Dont store gid 0 for non-posix groups
bc6181c 
Remove logic to store 'gidNumber: 0' in the cache for
non-posix groups. Instead do not add a gidNumber at all,
this avoids performance hit due to huge GID=0 index
when a large number of non-posix groups are stored.

Reviewed-by: Alexey Tikhonov <atikhono@redhat.com>
Reviewed-by: Sumit Bose <sbose@redhat.com>
(cherry picked from commit dc9188b)
@justin-stephenson @sssd-bot
SDAP: Remove sdap_store_group_with_gid()
c72cf9e 
It is no longer needed as we no longer want to store
'gid: 0' for non-POSIX groups.

Reviewed-by: Alexey Tikhonov <atikhono@redhat.com>
Reviewed-by: Sumit Bose <sbose@redhat.com>
(cherry picked from commit d953045)
@sssd-bot
Copy link
Contributor Author

sssd-bot commented Nov 20, 2025

The pull request was accepted by @alexey-tikhonov with the following PR CI status:

🟢 CodeQL (success)
🟢 rpm-build:centos-stream-9-x86_64:upstream (success)
🟢 Analyze (target) / All tests are successful (success)
🟢 Analyze (target) / cppcheck (success)
🟢 Build / make-distcheck (success)
🔴 ci / All tests are successful (failure)
🟢 ci / prepare (success)
🔴 ci / system (centos-9) (failure)
🟢 Static code analysis / All tests are successful (success)
🟢 Static code analysis / codeql (success)
🟢 Static code analysis / pre-commit (success)
🟢 Static code analysis / python-system-tests (success)

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

@alexey-tikhonov
Copy link
Member

alexey-tikhonov commented Nov 20, 2025

This test seems to be passing now?

Even without topology markers backported?

Correct, no idea what changed. Let me try to re-run a couple more times.

Looks to be fine now.

@sssd-bot sssd-bot force-pushed the SSSD-sssd-backport-pr8075-to-sssd-2-9 branch from 25de302 to c72cf9e Compare November 20, 2025 07:50
@alexey-tikhonov alexey-tikhonov merged commit 456782d into SSSD:sssd-2-9 Nov 20, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexey-tikhonov alexey-tikhonov alexey-tikhonov approved these changes

@sumit-bose sumit-bose Awaiting requested review from sumit-bose

+1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@sumit-bose sumit-bose

@alexey-tikhonov alexey-tikhonov

Labels

Accepted no-backport This should go to target branch only.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sssd-bot @alexey-tikhonov @justin-stephenson @sumit-bose