SUBID: add LDAP provider support#8097
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds support for subid to the generic LDAP provider, refactoring the existing IPA-specific implementation. The changes are well-structured and cover configuration, documentation, and the core logic. However, I've found a critical security vulnerability in the new implementation where a Distinguished Name (DN) is used in an LDAP filter without proper escaping, which could lead to LDAP injection. Please address this issue.
|
Steps to test this manually against 389-ds using sssd-ci-containers: (I) on LDAP container: (1) extend schema - add a snippet under (2) add a container to store subid ranges: (3) assign a range to a user: (II) on the CLIENT container: (1) install (2) enable (3) set Then: |
src/providers/ldap/ldap_id_subid.c
Outdated
| if (expire > time(NULL)) { | ||
| dn = ldb_msg_find_attr_as_string(res->msgs[0], SYSDB_ORIG_DN, NULL); | ||
| if (dn != NULL) { | ||
| sss_filter_sanitize(mem_ctx, dn, &result); |
There was a problem hiding this comment.
Covscan complains here:
Calling sss_filter_sanitize without checking return value (as is done elsewhere 66 out of 67 times).
But when functions fails its output arg is untouched, and thus NULL. Not much to do on top of this.
|
Will there be some tests for this functionality? |
I didn't find a QE owner (yet?) so plan to add a basic test myself. |
e62d9a4 to
8982dcd
Compare
|
(rebased) |
|
To have it recorded somewhere - current implementation (I guess both with and without patches from this PR) works in a weird way for AD: -- nss responder refuses a request immediately. I guess it strictly requires FQDN for AD but not sure why. That's ok since SSSD currently doesn't support subid ranges lookup for AD, but not... neat. |
|
As @justin-stephenson found out, there are at least two other flaws related to subid ranges support both in current "stock" SSSD and in this PR: (1) lookup by overridden name doesn't work for IPA: (2) lookup of a range in trusted IPA domain doesn't work: (note IPA-IPA trust support in IPA isn't yet released, testing was done against copr build of IPA) |
src/providers/ldap/ldap_id.c
Outdated
| #ifdef BUILD_SUBID | ||
| if (!ar->extra_value) { | ||
| if ((strcasecmp(sdom->dom->provider, "ldap") != 0) && | ||
| (strcasecmp(sdom->dom->provider, "ipa") != 0)) { |
There was a problem hiding this comment.
Check will not work as expected for a case "IPA-AD trusted subdomain" since "provider" will be "ipa" in this case.
There was a problem hiding this comment.
Should be fixed now.
b70b621 to
f9031c3
Compare
| #ifdef BUILD_SUBID | ||
| if (!ar->extra_value) { | ||
| if (((strcasecmp(sdom->dom->provider, "ldap") != 0) && | ||
| (strcasecmp(sdom->dom->provider, "ipa") != 0)) |
There was a problem hiding this comment.
Does this ever match? You are comparing the same provider instance to be two different values at the same time.
There was a problem hiding this comment.
@abbra, not "to be" but "to be not"
(if this is not LDAP and not IPA) or (this is trusted by IPA AD subdomain) => reject
There was a problem hiding this comment.
Ah, yes. I would have used !string_in_list() instead to be more clear here.
sumit-bose
left a comment
There was a problem hiding this comment.
Hi,
thank you for the patches (and your patience). I only have two comments (see inline), originally I had more but since I was going through the patches one by one it turned out that you already addressed most of my comments in later patches.
I ran couple of tests with a plain LDAP server and all went well.
bye,
Sumit
9b6ab51 to
3d0edac
Compare
|
(rebased) |
3d0edac to
6856aa4
Compare
sumit-bose
left a comment
There was a problem hiding this comment.
Hi,
all my comments are addressed and tests are still working as expected, ACK.
bye,
Sumit
82c2403 to
6856aa4
Compare
Reviewed-by: Justin Stephenson <jstephen@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>
Reviewed-by: Justin Stephenson <jstephen@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>
Proper STRUCTURAL objectClass is 'ipaSubordinateIdEntry'. Reviewed-by: Justin Stephenson <jstephen@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>
in favor of `ldap_subid_ranges_search_base`. :config:Option `ipa_subid_ranges_search_base` was deprecated in favor of `ldap_subid_ranges_search_base`. Reviewed-by: Justin Stephenson <jstephen@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>
Resolves: SSSD#8030 Reviewed-by: Justin Stephenson <jstephen@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>
It can be read from rootDSE upon first connection. Reviewed-by: Justin Stephenson <jstephen@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>
Reviewed-by: Justin Stephenson <jstephen@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>
related to attributes presence/manipulation. Reviewed-by: Justin Stephenson <jstephen@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>
Reviewed-by: Justin Stephenson <jstephen@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>
Reviewed-by: Justin Stephenson <jstephen@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>
Reviewed-by: Justin Stephenson <jstephen@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>
Resolves: #8030
Most patches could be squashed, but I kept them separate to make review easier.