Dont store gid 0 for non-posix groups

[justin-stephenson]

No description provided.

[gemini-code-assist]

Code Review

This pull request aims to prevent storing a GID of 0 for non-POSIX groups. However, the implementation is too broad and affects all groups, including POSIX groups. This can lead to creating invalid POSIX group entries without a gidNumber when called from sysdb_add_incomplete_group, which is a critical issue. My review includes a comment explaining the problem and suggests reverting the change in favor of a safer approach in the calling functions.

[justin-stephenson]

This is now ready for review. Do we want to backport this behavior? I'm not sure.

[alexey-tikhonov]

Word "testing" in commit message looks like a leftover. A more detailed description there would be beneficial.

[alexey-tikhonov]

@justin-stephenson, could you please rebase?

[justin-stephenson]

@justin-stephenson, could you please rebase?

Done. I updated the commit message.

[alexey-tikhonov]

I updated the commit message.

Remove logic to store 'gidNumber: 0' in the cache for posix groups.

Did you mean 'for non-posix groups'?

this improves performance when a large amount of posix groups may be stored

I would rephrase this. Something like
"""
This avoids performance hit due to huge GID=0 index when a large number of non-posix groups are stored.
"""

[justin-stephenson]

I updated the commit message.

Remove logic to store 'gidNumber: 0' in the cache for posix groups.

Did you mean 'for non-posix groups'?

this improves performance when a large amount of posix groups may be stored

I would rephrase this. Something like """ This avoids performance hit due to huge GID=0 index when a large number of non-posix groups are stored. """

Updated, and rebased.

[alexey-tikhonov]

Note: Covscan is green.

[sumit-bose]

Hi,

I have no further comments to the code, so ACK.

@alexey-tikhonov, do you agree with the latest changes?

I think it might be worth to ask the FreeIPA if they can run their test-suite against a copr build with the patch to see if they detect some unexpected failures before mergin the patches?

bye,
Sumit

[alexey-tikhonov]

I think it might be worth to ask the FreeIPA if they can run their test-suite against a copr build with the patch to see if they detect some unexpected failures before mergin the patches?

@amore17, @flo-renaud, would this be possible?

[alexey-tikhonov]

Covscan is still green.

[flo-renaud]

I think it might be worth to ask the FreeIPA if they can run their test-suite against a copr build with the patch to see if they detect some unexpected failures before mergin the patches?

@amore17, @flo-renaud, would this be possible?

Hi @alexey-tikhonov

yes it's possible. You can open a PR against https://github.com/freeipa/freeipa/:

• edit ipatests/prci_definitions/nightly_latest_sssd.yaml and replace all occurrences of copr: '@sssd/nightly' with your own copr
• call ln -sf ipatests/prci_definitions/nightly_latest_sssd.yaml .freeipa-pr-ci.yaml
• git add ipatests/prci_definitions/nightly_latest_sssd.yaml .freeipa-pr-ci.yaml
• git commit -m "Temp commit"
• push to your fork and create the PR

[justin-stephenson]

I think it might be worth to ask the FreeIPA if they can run their test-suite against a copr build with the patch to see if they detect some unexpected failures before mergin the patches?

@amore17, @flo-renaud, would this be possible?

Hi @alexey-tikhonov

yes it's possible. You can open a PR against https://github.com/freeipa/freeipa/:

* edit ipatests/prci_definitions/nightly_latest_sssd.yaml and replace all occurrences of `copr: '@sssd/nightly'` with your own copr

* call `ln -sf ipatests/prci_definitions/nightly_latest_sssd.yaml .freeipa-pr-ci.yaml`

* `git add ipatests/prci_definitions/nightly_latest_sssd.yaml .freeipa-pr-ci.yaml`

* `git commit -m "Temp commit"`

* push to your fork and create the PR

Thank you, I created freeipa/freeipa#8012

[alexey-tikhonov]

Thank you, I created freeipa/freeipa#8012

sssd-fedora/test_replica_promotion_TestReplicaPromotionLevel1 and sssd-fedora/test_trust_autoprivate are red there but not sure how to figure if it's introduced in this PR...

[flo-renaud]

Thank you, I created freeipa/freeipa#8012

sssd-fedora/test_replica_promotion_TestReplicaPromotionLevel1 and sssd-fedora/test_trust_autoprivate are red there but not sure how to figure if it's introduced in this PR...

The PR needs to replace all occurrences of @sssd/nightly with packit/SSSD-sssd-8075. Right now, only the build is using the right copr repo, all the test jobs are using SSSD nightly build.

[alexey-tikhonov]

Is this to be able to return non-posix groups via IFP?

[justin-stephenson]

Before the PR non-posix groups were stored with gidNumber: 0 so this filter made sense. Now we need to remove the gidNumber part of the search filter constraint to find the relevant nonPosix groups.

Is this to be able to return non-posix groups via IFP?

Only no if these groups are stored with gidNumber: 0 based on previous behavior of the PR. What about systems updated to this SSSD version and still have groups stored in the old format? Do we need to handle this case?

[alexey-tikhonov]

Now we need to remove the gidNumber part of the search filter constraint to find the relevant nonPosix groups.

That's what I'm asking: who consumes non-posix groups in INITGR list?
Is it only IFP?

What about systems updated to this SSSD version and still have groups stored in the old format?

Your patch looks backward compatible, no?
If "no" then new cache-upgrade proc is needed :-/

[justin-stephenson]

That's what I'm asking: who consumes non-posix groups in INITGR list?
Is it only IFP?

As far as I know, yes. I hope infopipe system tests would provide coverage for this, but I could test it manually with ipa-tuura if needed.

[justin-stephenson]

Let's wait for green CI results from freeipa/freeipa#8012

[alexey-tikhonov]

/gemini review

[gemini-code-assist]

Code Review

This pull request aims to prevent storing the gidNumber attribute for non-POSIX groups. The core of the change is in sysdb_add_basic_group, which now conditionally adds gidNumber based on an is_posix flag. Most of the changes correctly update callers to use this new logic. However, I've identified a critical issue in the group update path where a gidNumber could still be stored for a non-POSIX group, which would defeat the purpose of this change. My review includes a comment and a suggested fix for this issue.

[alexey-tikhonov]

Let's wait for green CI results from freeipa/freeipa#8012

No regression found.

[sssd-bot]

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

---

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