ipa: improve handling of external group memberships by sumit-bose · Pull Request #7938 · SSSD/sssd

sumit-bose · 2025-05-06T16:17:55Z

Currently add_ad_user_to_cached_groups() expects that all IPA
group-memberships of users from a trusted domain are removed when the
group-memberships from the trusted domain are updated. This is currently
only true for the code path where the tokenGroups request is used. The code
path without tokenGroups does not remove the IPA group-memberships.

Removing the IPA group-memberships is also not very efficient especially if
there are no changes to those at all. With this patch in
add_ad_user_to_cached_groups() it is checked which group-memberships have
to be added or removed. In this function the SYSDB_ORIG_MEMBEROF attribute
of the user is handled as well for the IPA group-memberships. Since this
attribute is removed in all code paths all IPA group-memberships are added
here again. But instead of doing it one by one as in the previous version,
the attribute is added for all groups in a single operation which should
help to improved the performance as well.

alexey-tikhonov · 2025-05-07T09:32:58Z

@sumit-bose, covscan reports an issue in function add_ad_user_to_cached_groups(): taking "user_dn has no RDN.\n" branch (and likely some others), return ret; returns uninitialized var.

alexey-tikhonov · 2025-05-07T13:43:13Z

@sumit-bose, shouldn't this have "Resolves: #7921" line in the commit message?

sumit-bose · 2025-05-07T15:24:26Z

@sumit-bose, covscan reports an issue in function add_ad_user_to_cached_groups(): taking "user_dn has no RDN.\n" branch (and likely some others), return ret; returns uninitialized var.

fixed

sumit-bose · 2025-05-07T15:24:42Z

@sumit-bose, shouldn't this have "Resolves: #7921" line in the commit message?

added

justin-stephenson · 2025-05-07T18:15:30Z


+    if (attr_name == NULL) {
+        attr_name = SYSDB_NAME;
+    }


what about passing SYSDB_NAME as an argument in sysdb_get_direct_parents() instead, and fail if attr_name is not provided in sysdb_get_direct_parents_ex() ? I don't insist however.

Hi,

thanks for the suggestion, I was thinking about this as well but decided against because using SYSDB_NAME is imo a sensible default and by using this default if no attribute name is giving we can avoid failing with an error.

bye,
Sumit

aplopez

I really didn't understand how this is working. So you might find my questions a bit strange.

justin-stephenson

Maybe a unit test for add_ad_user_to_cached_groups could be worthwhile here, Ack from me.

Patches LGTM in my basic tests (I did not test with thousands of group memberships).


[root@master /]# id [email protected]
uid=1157400500([email protected]) gid=1157400500([email protected]) groups=1157400500([email protected]),1157400520(group policy creator [email protected]),1157400519(enterprise [email protected]),1157400512(domain [email protected]),1157400518(schema [email protected]),1714600006(ad_admins),1714600007(ad_admins2),1157400513(domain [email protected])

[root@master /]# grep -irn 'add_ad_user_to_cached_groups' /var/log/sssd/
[be[ipa.test]] [add_ad_user_to_cached_groups] (0x4000): [RID#4] Cached IPA groups [0].
[be[ipa.test]] [add_ad_user_to_cached_groups] (0x4000): [RID#4] New IPA groups [2].
[be[ipa.test]] [add_ad_user_to_cached_groups] (0x4000): [RID#4] Groups to add [2].
[be[ipa.test]] [add_ad_user_to_cached_groups] (0x4000): [RID#4] Groups to delete [0].
[be[ipa.test]] [add_ad_user_to_cached_groups] (0x4000): [RID#4] Group [cn=ad_admins,cn=groups,cn=accounts,dc=ipa,dc=test] not in the cache.
[be[ipa.test]] [add_ad_user_to_cached_groups] (0x4000): [RID#4] Group [cn=ad_admins2,cn=groups,cn=accounts,dc=ipa,dc=test] not in the cache.
[be[ipa.test]] [add_ad_user_to_cached_groups] (0x4000): [RID#6] Cached IPA groups [0].
[be[ipa.test]] [add_ad_user_to_cached_groups] (0x4000): [RID#6] New IPA groups [2].
[be[ipa.test]] [add_ad_user_to_cached_groups] (0x4000): [RID#6] Groups to add [2].
[be[ipa.test]] [add_ad_user_to_cached_groups] (0x4000): [RID#6] Groups to delete [0].

justin-stephenson · 2025-05-13T13:23:25Z

Did a quick test with 100 IDM POSIX groups for External users.


/var/log/sssd/sssd_ipa.test.log:11646:(2025-05-13 13:20:07): [be[ipa.test]] [add_ad_user_to_cached_groups] (0x4000): [RID#6] Cached IPA groups [0].
/var/log/sssd/sssd_ipa.test.log:11647:(2025-05-13 13:20:07): [be[ipa.test]] [add_ad_user_to_cached_groups] (0x4000): [RID#6] New IPA groups [101].
/var/log/sssd/sssd_ipa.test.log:11648:(2025-05-13 13:20:07): [be[ipa.test]] [add_ad_user_to_cached_groups] (0x4000): [RID#6] Groups to add [101].
/var/log/sssd/sssd_ipa.test.log:11649:(2025-05-13 13:20:07): [be[ipa.test]] [add_ad_user_to_cached_groups] (0x4000): [RID#6] Groups to delete [0].
/var/log/sssd/sssd_ipa.test.log:12613:(2025-05-13 13:20:08): [be[ipa.test]] [add_ad_user_to_cached_groups] (0x4000): [RID#9] Cached IPA groups [0].
/var/log/sssd/sssd_ipa.test.log:12614:(2025-05-13 13:20:08): [be[ipa.test]] [add_ad_user_to_cached_groups] (0x4000): [RID#9] New IPA groups [101].
/var/log/sssd/sssd_ipa.test.log:12615:(2025-05-13 13:20:08): [be[ipa.test]] [add_ad_user_to_cached_groups] (0x4000): [RID#9] Groups to add [101].
/var/log/sssd/sssd_ipa.test.log:12616:(2025-05-13 13:20:08): [be[ipa.test]] [add_ad_user_to_cached_groups] (0x4000): [RID#9] Groups to delete [0].

Before the PR:

-- Uncached id user


# time id [email protected]
uid=1157401000([email protected]) gid=1157401000([email protected]) groups=1157401000([email protected]),392200133(ad_admins25),392200159(ad_admins51),392200163(ad_admins55),392200168(ad_admins60),392200205(ad_admins97),392200136(ad_admins28),392200141(ad_admins33),392200149(ad_admins41),392200200(ad_admins92),392200114(ad_admins6),392200117(ad_admins9),392200120(ad_admins12),392200184(ad_admins76),392200187(ad_admins79),392200189(ad_admins81),392200125(ad_admins17),392200165(ad_admins57),392200178(ad_admins70),392200127(ad_admins19),392200143(ad_admins35),392200196(ad_admins88),392200134(ad_admins26),392200144(ad_admins36),392200177(ad_admins69),392200208(ad_admins100),392200152(ad_admins44),392200207(ad_admins99),392200172(ad_admins64),392200174(ad_admins66),392200182(ad_admins74),392200193(ad_admins85),392200203(ad_admins95),392200132(ad_admins24),392200138(ad_admins30),392200113(ad_admins5),392200115(ad_admins7),392200118(ad_admins10),392200124(ad_admins16),392200145(ad_admins37),392200150(ad_admins42),392200191(ad_admins83),392200202(ad_admins94),392200122(ad_admins14),392200128(ad_admins20),392200137(ad_admins29),392200164(ad_admins56),392200190(ad_admins82),392200108(ad_admins),392200147(ad_admins39),392200185(ad_admins77),392200194(ad_admins86),392200199(ad_admins91),392200169(ad_admins61),392200170(ad_admins62),392200110(ad_admins1),392200129(ad_admins21),392200157(ad_admins49),392200158(ad_admins50),392200192(ad_admins84),392200130(ad_admins22),392200179(ad_admins71),392200195(ad_admins87),392200109(ad_admins2),392200119(ad_admins11),392200121(ad_admins13),392200123(ad_admins15),392200162(ad_admins54),392200167(ad_admins59),392200186(ad_admins78),392200111(ad_admins3),392200148(ad_admins40),392200160(ad_admins52),392200171(ad_admins63),392200173(ad_admins65),392200176(ad_admins68),392200180(ad_admins72),392200188(ad_admins80),392200197(ad_admins89),392200175(ad_admins67),392200201(ad_admins93),392200206(ad_admins98),392200151(ad_admins43),392200155(ad_admins47),392200135(ad_admins27),392200154(ad_admins46),392200161(ad_admins53),392200140(ad_admins32),392200204(ad_admins96),392200116(ad_admins8),392200126(ad_admins18),392200183(ad_admins75),392200131(ad_admins23),392200139(ad_admins31),392200156(ad_admins48),392200166(ad_admins58),392200146(ad_admins38),392200198(ad_admins90),392200112(ad_admins4),392200142(ad_admins34),392200153(ad_admins45),392200181(ad_admins73),1157400513(domain [email protected])

real    0m2.143s
user    0m0.025s
sys     0m0.034s

-- Login time

# time ssh [email protected]@localhost ls
([email protected]@localhost) Password: 

real    0m5.779s
user    0m0.032s
sys     0m0.034s

After the PR:

-- Uncached id command

# time id [email protected]
real    0m1.221s
user    0m0.023s
sys     0m0.035s

-- Login time

[root@master /]# time ssh [email protected]@localhost ls
([email protected]@localhost) Password: 

real    0m4.131s
user    0m0.037s
sys     0m0.029s

sumit-bose · 2025-05-13T15:06:16Z

Hi @justin-stephenson ,

thanks for testing. The main difference should be see after small changes and when using ldap_use_tokengroups = False (inherited to the subdomains as well). If you remove the membership to a single IPA group, call sss_cache -u [email protected] and call id [email protected] again it should be reasonable fast and the logs should say that only a single group is removed. Without the patch it should take much longer.

Currently with ldap_use_tokengroups = True the IPA group memberships are all removed so that the fix currently does not improve this case much but it might be worth to change the token groups path so that IPA groups are not removed at this step but handled here as well.

bye,
Sumit

alexey-tikhonov · 2025-05-14T06:35:48Z

Thank you, ACK.

alexey-tikhonov · 2025-05-14T06:55:29Z

JFTR: this had a fully green CI run. Re-starting to see if IPA DNS issue had really gone or this was just a lucky run.

AD user in external group is not cleared when expiring the cache In IPA-AD trust, a ADuser is member of large number of IPA groups. When ADuser is removed from one external-group, and cache of ADuser is expired, subsequent look up should update the cache correctly. verifies SSSD#7938

AD user in external group is not cleared when expiring the cache In IPA-AD trust, a ADuser is member of large number of IPA groups. When ADuser is removed from one external-group, and cache of ADuser is expired, subsequent look up should update the cache correctly. verifies SSSD#7938 Reviewed-by: Jakub Vávra <[email protected]> Reviewed-by: Sumit Bose <[email protected]>

AD user in external group is not cleared when expiring the cache In IPA-AD trust, a ADuser is member of large number of IPA groups. When ADuser is removed from one external-group, and cache of ADuser is expired, subsequent look up should update the cache correctly. verifies #7938 Reviewed-by: Jakub Vávra <[email protected]> Reviewed-by: Sumit Bose <[email protected]>

AD user in external group is not cleared when expiring the cache In IPA-AD trust, a ADuser is member of large number of IPA groups. When ADuser is removed from one external-group, and cache of ADuser is expired, subsequent look up should update the cache correctly. verifies SSSD#7938 Reviewed-by: Jakub Vávra <[email protected]> Reviewed-by: Sumit Bose <[email protected]> (cherry picked from commit ec81ea2)

AD user in external group is not cleared when expiring the cache In IPA-AD trust, a ADuser is member of large number of IPA groups. When ADuser is removed from one external-group, and cache of ADuser is expired, subsequent look up should update the cache correctly. verifies #7938 Reviewed-by: Jakub Vávra <[email protected]> Reviewed-by: Sumit Bose <[email protected]> (cherry picked from commit ec81ea2)

alexey-tikhonov added backport-to-sssd-2-9 backport-to-sssd-2-9-4 Corresponds to C8S labels May 6, 2025

alexey-tikhonov self-assigned this May 7, 2025

alexey-tikhonov requested review from alexey-tikhonov, aplopez and justin-stephenson May 7, 2025 08:42

alexey-tikhonov added Bugzilla coverity Trigger a coverity scan labels May 7, 2025

alexey-tikhonov removed the coverity Trigger a coverity scan label May 7, 2025

alexey-tikhonov reviewed May 7, 2025

View reviewed changes

Comment thread src/db/sysdb.h Outdated

alexey-tikhonov reviewed May 7, 2025

View reviewed changes

Comment thread src/db/sysdb_search.c Outdated

justin-stephenson self-assigned this May 7, 2025

sumit-bose force-pushed the save_rfc2307bis_user_memberships_fix branch from 677ab5e to 1c00679 Compare May 7, 2025 15:24

justin-stephenson reviewed May 7, 2025

View reviewed changes

alexey-tikhonov assigned aplopez May 9, 2025

aplopez reviewed May 9, 2025

View reviewed changes

justin-stephenson approved these changes May 12, 2025

View reviewed changes

alexey-tikhonov added the coverity Trigger a coverity scan label May 13, 2025

sumit-bose force-pushed the save_rfc2307bis_user_memberships_fix branch from 1c00679 to b19d91c Compare May 13, 2025 17:01

alexey-tikhonov reviewed May 13, 2025

View reviewed changes

Comment thread src/providers/ipa/ipa_subdomains_ext_groups.c Outdated

Comment thread src/providers/ipa/ipa_subdomains_ext_groups.c Outdated

sumit-bose force-pushed the save_rfc2307bis_user_memberships_fix branch from b19d91c to 5b18558 Compare May 13, 2025 17:30

alexey-tikhonov approved these changes May 14, 2025

View reviewed changes

alexey-tikhonov closed this Jul 4, 2025

alexey-tikhonov mentioned this pull request Jul 4, 2025

AD user in external group is not cleared when expiring the cache #7921

Closed

shridhargadekar mentioned this pull request Jul 22, 2025

Tests: ADuser external group cache update #8046

Merged

sssd-bot mentioned this pull request Nov 27, 2025

[autobackport: sssd-2-9] Tests: ADuser external group cache update #8247

Merged

Uh oh!

Conversation

sumit-bose commented May 6, 2025

Uh oh!

alexey-tikhonov commented May 7, 2025

Uh oh!

alexey-tikhonov commented May 7, 2025

Uh oh!

Uh oh!

Uh oh!

sumit-bose commented May 7, 2025

Uh oh!

sumit-bose commented May 7, 2025

Uh oh!

justin-stephenson May 7, 2025

Choose a reason for hiding this comment

Uh oh!

sumit-bose May 8, 2025

Choose a reason for hiding this comment

Uh oh!

aplopez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

justin-stephenson left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justin-stephenson commented May 13, 2025

Uh oh!

sumit-bose commented May 13, 2025

Uh oh!

Uh oh!

Uh oh!

alexey-tikhonov commented May 14, 2025

Uh oh!

alexey-tikhonov commented May 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

justin-stephenson left a comment •

edited

Loading