Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial support for IDM IDM Trust #7679

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

justin-stephenson
Copy link
Contributor

@justin-stephenson justin-stephenson commented Nov 1, 2024

This PR adds support for IDM subdomains, enabling IDM IDM Trust functionality in SSSD. These are building blocks in SSSD to incorporate the IDM IDM Trust feature. Note however that on the freeIPA side development is still ongoing, this means the entire IDM IDM Trust feature (freeIPA + SSSD) is not yet available, and full integration cannot be tested yet. Current testing is being done using COPR packages.

src/providers/ipa/ipa_common.c Dismissed Show dismissed Hide dismissed
src/providers/ipa/ipa_subdomains_id.c Dismissed Show dismissed Hide dismissed
@justin-stephenson justin-stephenson force-pushed the idm_idm_trust_pr branch 8 times, most recently from 85a4fb8 to 1807596 Compare November 5, 2024 21:19
@justin-stephenson justin-stephenson changed the title Draft - Initial support for IDM IDM Trust Initial support for IDM IDM Trust Nov 6, 2024
@justin-stephenson justin-stephenson marked this pull request as ready for review November 6, 2024 16:09
Copy link
Contributor

@thalman thalman left a comment

Choose a reason for hiding this comment

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

Looks good. Nitpicking - You have broken indentation in many places. Maybe some of them are intentional. Please check it.

src/providers/ipa/ipa_subdomains.h Show resolved Hide resolved
@justin-stephenson
Copy link
Contributor Author

Looks good. Nitpicking - You have broken indentation in many places. Maybe some of them are intentional. Please check it.

Thank you Tomas. Would you mind commenting in-line on a couple indentation areas that need to be fixed? I didn't go through every line but I scanned the PR again and couldn't find indentation issues.

@thalman
Copy link
Contributor

thalman commented Dec 17, 2024

Looks good. Nitpicking - You have broken indentation in many places. Maybe some of them are intentional. Please check it.

Thank you Tomas. Would you mind commenting in-line on a couple indentation areas that need to be fixed? I didn't go through every line but I scanned the PR again and couldn't find indentation issues.

Sure, will do...

Copy link
Contributor

@thalman thalman left a comment

Choose a reason for hiding this comment

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

I believe that in some cases you broke the indentation on purpose for better readability. Feel free to resolve those conversations.

src/providers/ad/ad_common.c Outdated Show resolved Hide resolved
src/providers/ad/ad_common.h Outdated Show resolved Hide resolved
src/providers/ad/ad_common.h Outdated Show resolved Hide resolved
src/providers/ad/ad_subdomains.c Outdated Show resolved Hide resolved
src/providers/ad/ad_subdomains.c Outdated Show resolved Hide resolved
src/providers/ipa/ipa_subdomains_server.c Outdated Show resolved Hide resolved
src/providers/ipa/ipa_subdomains_server.c Outdated Show resolved Hide resolved
src/providers/ipa/ipa_subdomains_server.c Outdated Show resolved Hide resolved
src/providers/ipa/ipa_subdomains_server.c Outdated Show resolved Hide resolved
src/providers/ipa/ipa_subdomains_server.c Show resolved Hide resolved
@justin-stephenson
Copy link
Contributor Author

I believe that in some cases you broke the indentation on purpose for better readability. Feel free to resolve those conversations.

Thank you for pointing out the indentation issues, I had missed several. They should be fixed now, I resolved each conversation for simplicity but feel free to check back to see if I missed anything.

@justin-stephenson
Copy link
Contributor Author

Hi @thalman @danlavu @sumit-bose Gentle reminder ping about reviewing this PR.

Copy link
Contributor

@thalman thalman left a comment

Choose a reason for hiding this comment

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

LGTM, please fix that one indentation.

@abbra
Copy link
Contributor

abbra commented Feb 12, 2025

Thank you. The only reason to have different ranges is when both deployments have overlapping ID ranges themselves. I haven't fully defined this use case yet. Perhaps, we need to do an analysis of the exported ID ranges and force admins to make a chose if needed, but this will be part of the new trust-add logic.

One-way trust can be established already. Perhaps you were thinking about a shared secret to establish trust? This can be done regardless of the direction of the trust.

@alexey-tikhonov
Copy link
Member

alexey-tikhonov commented Feb 12, 2025

@justin-stephenson, wouldn't it make sense to update man pages?

And also please add a detailed release note.

@sumit-bose
Copy link
Contributor

One-way trust can be established already. Perhaps you were thinking about a shared secret to establish trust? This can be done regardless of the direction of the trust.

Hi,

without --two-way=True it was not possible to get a TGT with the trust account ([email protected]), so SSSD on the IPA server was not able to connect the the LDAP server of the remote domain. I guess some principals or aliases were not created in this case.

bye,
Sumit

@abbra
Copy link
Contributor

abbra commented Feb 12, 2025

Ah, I'll cross check this. It should not affect SSSD code, though.

@sumit-bose
Copy link
Contributor

Ah, I'll cross check this. It should not affect SSSD code, though.

Yes, that's why I ACKed the current version.

@justin-stephenson
Copy link
Contributor Author

@justin-stephenson, wouldn't it make sense to update man pages?

And also please add a detailed release note.

Done, added a new 'man' commit and release note to the ipa: Add ipa subdomain provider initialization commit

@alexey-tikhonov alexey-tikhonov added the coverity Trigger a coverity scan label Feb 13, 2025
Similar to AD server/service discovery initialization,
Allows callers to provide a service, and not just use "IPA"
IPA subdomain functions often include ad in the name, these functions
will now handle IPA and AD subdomains, not only AD.
:feature:SSSD IPA provider now supports IPA subdomains, not only
Active Directory. This IPA subdomain support will enable SSSD
support of IPA-IPA Trust feature, the full usable feature coming
in a later FreeIPA release. Trusted domain configuration options
are specified in the 'sssd-ipa' man page.
After b3d7a4f we no longer use
the 'upn' variable. During certain codepaths to ipa_s2n_save_objects()
SYSDB_UPN is expected to be missing, so no need to check for it.
This gets executed when a one-way or two-way trust ipa
is added. Rename this to avoid confusion.
SSSD goes offline in IPA trusted user look due to the IPA user private group:

    [ipa_get_ad_acct_ad_part_done] (0x0020): [RID#7] Cannot find a SID.

In IPA-IPA trust, user private groups do not contain a SID. Lookup the
equivalent user object of the same name in IPA and use this SID instead.
Don't fail when processing the IPA user private group retrieved
from the IPA server in a trusted user lookup. It is expected
this object will have no SID.
@justin-stephenson justin-stephenson added coverity Trigger a coverity scan and removed coverity Trigger a coverity scan labels Feb 14, 2025
@alexey-tikhonov alexey-tikhonov added coverity Trigger a coverity scan and removed coverity Trigger a coverity scan labels Feb 15, 2025
@alexey-tikhonov
Copy link
Member

Coverity says:

Newly Detected: 1

snapshot = 88491

But as far as I can tell, that's because this PR wasn't rebased on top of c36c320

@alexey-tikhonov alexey-tikhonov added the Ready to push Ready to push label Feb 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants