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

Closed

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.

@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.

@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.

@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.

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 Ready to push Ready to push and removed Ready to push Ready to push branch: sssd-2-9 labels Feb 15, 2025
@alexey-tikhonov
Copy link
Member

Pushed PR: #7679

  • master
    • 3c87b81 - man: IPA subdomain changes to sssd-ipa
    • 129b549 - AD: Remove unused AD_AT_TRUST_TYPE attribute
    • de4cea5 - ipa s2n: Ignore trusted IPA user private group
    • b63321c - Handle missing SID for user private group
    • 4eb75cc - ipa: Rename ipa_create_ad_1way_trust_ctx()
    • f085fe0 - ipa s2n: Remove check for SYSDB_UPN
    • 4378ea6 - ipa: Support ipa subdomain account info requests
    • dc7e280 - ipa: Add ipa subdomain provider initialization
    • 0862fcb - ipa: Make ipa server ad* functions generic
    • 1b0c620 - ad: Combine 1+2way trust options creation functions
    • 70daa00 - ipa: Make ipa_service_init() like ad_failover_init()
    • 8879cf8 - Rename struct ipa_ad_server_ctx, and add id_ctx union member
    • e87cc2c - SYSDB: Store IPA trust type

@alexey-tikhonov
Copy link
Member

@justin-stephenson, there was a conflict in sssd-2-9. Please open explicit backport PR.

@justin-stephenson
Copy link
Contributor Author

@justin-stephenson, there was a conflict in sssd-2-9. Please open explicit backport PR.

#7842

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
coverity Trigger a coverity scan Pushed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants