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

sumit-bose · 2025-09-11T16:42:47Z

Recently add_ad_user_to_cached_groups() was modified to better handle
adding and removing group memberships of users from trusted domains in
groups of the local IPA domain. Before the change group members were only
added and with the change a removal was possible as well. This caused an
issues with an in-out parameter which contains the full list of IPA group
memberships at input and the list of group missing in the cache as output.
Since add_ad_user_to_cached_groups() is called twice, the second time after
the missing groups were read from the IPA server, this caused and
unexpected removal of group memberships since the second call to
add_ad_user_to_cached_groups() was done with the list of missing groups and
not with the full list.

With this patch a dedicated list is used for the missing groups to avoid
the described issues.

Additionally this PR contains a second patch which filters some objects SSSD
currently does not handle.

Resolves: #7921

gemini-code-assist

Code Review

This pull request effectively refactors the handling of external group memberships to resolve an issue with an in-out parameter, by introducing a dedicated list for missing groups. This correctly prevents unintended group membership removals. Additionally, it introduces filtering for objects that SSSD does not handle. However, I've found a critical use-after-free vulnerability in the new helper function get_rdn_name_from_dn_string which must be addressed.

src/providers/ipa/ipa_subdomains_ext_groups.c

sumit-bose · 2025-09-12T06:58:02Z

/gemini review

gemini-code-assist

Code Review

This pull request correctly refactors the handling of external group memberships to prevent incorrect group removals by using a dedicated list for missing groups. It also introduces filtering to avoid lookups for non-group objects. The logic seems sound, but I've identified a memory leak in the new filtering function that should be addressed.

src/providers/ipa/ipa_subdomains_ext_groups.c

pbrezina · 2025-09-12T11:24:07Z

Hi, would you mind adding test for the original issue? It should be fairly simple.

Once SSSD/sssd-test-framework#190 is merged, we can add also test for the sudo case.

sumit-bose · 2025-09-12T11:45:41Z

Hi, would you mind adding test for the original issue? It should be fairly simple.

Once SSSD/sssd-test-framework#190 is merged, we can add also test for the sudo case.

Hi,

you mean a test to check that a group membership is properly removed? For the "sudo case" Shridhar has already opened #8074 for the underlying issue.

bye,
Sumit

pbrezina · 2025-09-12T15:55:02Z

Hi, would you mind adding test for the original issue? It should be fairly simple.
Once SSSD/sssd-test-framework#190 is merged, we can add also test for the sudo case.

Hi,

you mean a test to check that a group membership is properly removed?

yes

sumit-bose · 2025-09-15T10:47:51Z

Hi, would you mind adding test for the original issue? It should be fairly simple.
Once SSSD/sssd-test-framework#190 is merged, we can add also test for the sudo case.

Hi,
you mean a test to check that a group membership is properly removed?

yes

added

pbrezina

Hi, there is a typo in the second commita message "removall". The third commit also contains some test bits that should be moved to the second commit.

Otherwise ack, thank you.

pbrezina · 2025-09-15T13:28:24Z

src/tests/system/tests/test_ipa_trusts.py

+    :customerscenario: True
+    """
+    ipa.sssd.subdom('ipa.test', trusted.domain)['ldap_use_tokengroups'] = use_token_groups
+    ipa.sssd.config_apply(check_config=False)


Why disable check_config?

Hi,

currently the config check does not like ldap_use_tokengroups in the sub-domain section. I didn't want to add it to the allowed options en passant because it might need some more careful testing when using the AD provider. Since token groups are not used in the IPA provider it might be possible to set ldap_use_tokengroups in the IPA domain section and inherit it, if you prefer.

bye,
Sumit

Thank you for the explanation, maybe add a comment then?

Hi,

I switched to the inherit variant and removed check_config=False.

bye,
Sumit

pbrezina · 2025-09-15T13:29:41Z

src/tests/system/tests/test_ipa_trusts.py

+    ipa.group("posix-group").add().add_member(external)
+
+    ipa.sssd.clear(db=True, memcache=True, logs=True)
+    ipa.host.conn.exec(["systemctl", "restart", "sssd.service"])


Why don't you use ipa.sssd.restart? And isn't it sufficient to restart it only once, since you are clearing the cache anyway?

Hi,

this is the work-around for SSSD/sssd-test-framework#203. If ipa.sssd.restart() is used there will be no valid Kerberos ticket anymore for the second ipa .... command.

bye,
Sumit

No need for workaround, I wrote a fix: SSSD/sssd-test-framework#205

Hi,

the test is now using ipa.sssd.restart()

bye,
Sumit

alexey-tikhonov · 2025-09-15T16:24:07Z

Note: Covscan is green.

src/providers/ipa/ipa_subdomains_ext_groups.c

alexey-tikhonov · 2025-09-19T17:10:17Z

src/providers/ipa/ipa_subdomains_ext_groups.c

+        return NULL;
+    }
+
+    tmp_str = ldb_dn_get_rdn_name(dn);


Since ldb_dn_get_rdn_name() returns name of 1st component only anyway:

dn->components[0].name

would it work to simply strchr('=')? Can '=' be escaped in a DN?

Hi,

using strchr() was my first implementation, but what I found that ldb functions were used for similar task I wanted to be consistent and switch to the current implementation.

Since LDAP attribute names (or short names or descriptors as they are called in the RFCs) cannot contain a =, only "ALPHA / DIGIT / HYPHEN" are allowed (https://www.rfc-editor.org/rfc/rfc4512#section-1.4) the strchr() variant would work as well. So, if you prefer I can change the implementation.

bye,
Sumit

So, if you prefer I can change the implementation.

It depends if this is a hot path.

Cache performance is already a huge pain point, especially for AD/IPA-AD cases.
If this is a hot path (and imo it can be) then I'd vote to not introduce additional overhead here (this includes my other perf related comments).

sumit-bose · 2025-09-23T08:16:07Z

Hi,

I removed the third patch with the filtering from this PR to be able to move more swiftly with the actual fix. I will add a now PR with the filtering including Alexey's suggestion once this here is finished.

bye,
Sumit

In a IPA-AD trust environment, AD-user has membership of a external-ipa group. This external-ipa-group is member of IPA-posix group. When a HBAC rule is added and is the ipa-posix member is added as member for that rule, AD-user misses that IPA-group-membership

src/tests/system/tests/test_ipa_trusts.py

alexey-tikhonov · 2025-09-23T11:36:21Z

ACK, thank you.

justin-stephenson

Ack.

alexey-tikhonov · 2025-09-23T13:38:00Z

Note: Covscan is still green.

alexey-tikhonov · 2025-09-23T14:00:29Z

Pushed PR: #8094

master
- 80ccb59 - Test: HBAC affecting AD-users ipa-group membership
- 2a388e7 - tests: test removal of external group membership
- 9f72fcd - ipa: improve handling of external group memberships
sssd-2-11
- 96b4251 - Test: HBAC affecting AD-users ipa-group membership
- 146b251 - tests: test removal of external group membership
- 5afeec3 - ipa: improve handling of external group memberships
sssd-2-9
- 0a26516 - Test: HBAC affecting AD-users ipa-group membership
- 46f7101 - tests: test removal of external group membership
- de347b2 - ipa: improve handling of external group memberships

gemini-code-assist bot reviewed Sep 11, 2025

View reviewed changes

src/providers/ipa/ipa_subdomains_ext_groups.c Outdated Show resolved Hide resolved

sumit-bose force-pushed the ipa_add_group_member_fix branch from 0eda138 to 77ae7dc Compare September 11, 2025 17:09

gemini-code-assist bot reviewed Sep 12, 2025

View reviewed changes

src/providers/ipa/ipa_subdomains_ext_groups.c Outdated Show resolved Hide resolved

sumit-bose force-pushed the ipa_add_group_member_fix branch from 77ae7dc to 4b9dce6 Compare September 12, 2025 07:12

sumit-bose force-pushed the ipa_add_group_member_fix branch from 4b9dce6 to dc0301b Compare September 15, 2025 10:47

sumit-bose force-pushed the ipa_add_group_member_fix branch 2 times, most recently from 96f3a1d to 54f2dc1 Compare September 15, 2025 10:53

alexey-tikhonov added the coverity Trigger a coverity scan label Sep 15, 2025

pbrezina requested changes Sep 15, 2025

View reviewed changes

sumit-bose mentioned this pull request Sep 16, 2025

ipa: restart SSSD and clean up before each test SSSD/sssd-test-framework#205

Merged

sumit-bose force-pushed the ipa_add_group_member_fix branch 2 times, most recently from 07b6c6b to afb2f50 Compare September 17, 2025 13:40

alexey-tikhonov assigned pbrezina Sep 18, 2025

alexey-tikhonov requested a review from justin-stephenson September 18, 2025 12:43

alexey-tikhonov assigned justin-stephenson and alexey-tikhonov Sep 18, 2025

alexey-tikhonov requested a review from pbrezina September 19, 2025 12:58