memberOf plugin :: mbof_add_operation() optimizations

[alexey-tikhonov]

Using the same test case as described in #8454 and testing on top of that PR:

• users tu1 and tu2 are members of the same 5k LDAP groups (RFC2307 case, no nested groups)
• SSSD stared with an empty cache
• time id tu1@ldap.test | tr ',' '\n' | wc -l and then time id tu2@ldap.test | tr ',' '\n' | wc -l are executed

This patch set reduces time spent by id tu1 from 27 to 8 seconds and by consequent id tu2 from 23 to 5 seconds on my laptop.

[alexey-tikhonov]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces several optimizations to the mbof_add_operation() function in the memberof plugin. The changes include a more efficient way to remove duplicate entries from an array and the removal of a redundant check, both of which are good improvements. However, one of the optimizations replaces ldb_dn_compare() with strcmp() for checking DN equality. This is a critical issue as strcmp() performs a case-sensitive comparison, which is incorrect for DNs and can lead to failing to detect duplicates. This could cause data inconsistencies by allowing duplicate memberof values. I have added a review comment with a suggestion to fix this.

[alexey-tikhonov]

(rebased)

[alexey-tikhonov]

Replaced strcmp() with strcasecmp().

If I read rfc4519 correctly:

( 2.5.4.3 NAME 'cn'
 SUP name )

( 2.5.4.41 NAME 'name'
 EQUALITY caseIgnoreMatch
 SUBSTR caseIgnoreSubstringsMatch
 SYNTAX 1.3.6.1.4.1.1466.115.121.1.15 )

-- EQUALITY caseIgnoreMatch is mandated for any compliant LDAP server.

[sumit-bose]

Hi,

about EQUALITY caseIgnoreMatch I agree that base on that it should not be possible to add two objects with the cn RDN to the same sub-tree where the attribute value only differ in case for compliant LDAP servers. And although the majority of our data sources will be LDAP servers which are at least compliant in this respect there are other data sources e.g. via the proxy provider. I would even agree that if the sources allows to e.g. create different users where the name can only differ in the case, it is still a bad idea to do so. But currently the cache allows to store such objects:

[root@client /]# ldbsearch -H /var/lib/sss/db/cache_ipa.test.ldb namealias=admin@ipa.test dn name
asq: Unable to register control with rootdse!
# record 1
dn: name=admin@ipa.test,cn=users,cn=ipa.test,cn=sysdb
name: admin@ipa.test

# record 2
dn: name=AdMiN@IpA.tEsT,cn=users,cn=ipa.test,cn=sysdb
name: admin@ipa.test

# returned 2 records
# 2 entries
# 0 referrals

and this is only true for the name attribute. If you try to add something where a cn attribute differs in spelling you get

[root@client /]# ldbadd -H /var/lib/sss/db/cache_ipa.test.ldb /tmp/bla.ldif 
asq: Unable to register control with rootdse!
ERR: Entry already exists : "Entry name=AdMiN@IpA.tEsT,cn=USERS,cn=ipa.test,cn=sysdb already exists" on DN name=AdMiN@IpA.tEsT,cn=USERS,cn=ipa.test,cn=sysdb at block before line 90
Add failed after processing 0 records

which is because of https://github.com/SSSD/sssd/blob/master/src/db/sysdb_private.h#L58. For name there is no such line.

Long story short, since the cache handles the name attributes case-sensitive in DNs I think this should be respected in the comparison here as well. But I think we also can safely assume that name is only used in the most specific RDN and it might help to speed things up to do this additional check only if strcasecmp() returns 0 which should be a rare case if I understand it correctly.

HTH

bye,
Sumit

[alexey-tikhonov]

(rebased)

[alexey-tikhonov]

Thank you.

I think we also can safely assume that name is only used in the most specific RDN and it might help to speed things up to do this additional check only if strcasecmp() returns 0

I have implemented this.

[ikerexxe]

LGTM!

Codewise looks solid. I’ve focused on the implementation details and am relying on the test cases Alexey highlighted to confirm the improvement

[sumit-bose]

Hi,

thank you for the improvements, ACK.

bye,
Sumit

[sssd-bot]

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

---

🟢 CodeQL (success)
🟢 osh-diff-scan:fedora-rawhide-x86_64:upstream (success)
🟢 rpm-build:centos-stream-10-x86_64:upstream (success)
🟢 rpm-build:fedora-42-x86_64:upstream (success)
🟢 rpm-build:fedora-43-x86_64:upstream (success)
🟢 rpm-build:fedora-44-x86_64:upstream (success)
🟢 rpm-build:fedora-rawhide-x86_64:upstream (success)
🟢 Analyze (target) / cppcheck (success)
🟢 Build / freebsd (success)
🟢 Build / make-distcheck (success)
🟢 ci / intgcheck (centos-10) (success)
🟢 ci / intgcheck (fedora-42) (success)
🟢 ci / intgcheck (fedora-43) (success)
🟢 ci / intgcheck (fedora-44) (success)
🟢 ci / intgcheck (fedora-45) (success)
🟢 ci / prepare (success)
🟢 ci / system (centos-10) (success)
🟢 ci / system (fedora-42) (success)
🟢 ci / system (fedora-43) (success)
🟢 ci / system (fedora-44) (success)
🟢 ci / system (fedora-45) (success)
➖ Coverity scan / coverity (skipped)
🟢 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.