On (10/07/14 16:38), Pavel Reichl wrote:
>Hello,
>
>please see attached patches.
>
>I found out that if we go with approach introduced in previous version
>(in case of LDAP provider assume SID comes from default domain) this can
>lead to resolutions of SIDs like S-1-5-32-545 and also SIDs of non-posix
>groups which in case of disabled id mapping leads to failure which will
>end request prematurely. 2nd patch should make SID resolution more
>resilient to handle this.
>
>Regards,
>Pavel Reichl
>
>On Tue, 2014-07-08 at 09:57 +0200, Sumit Bose wrote:
>> On Mon, Jul 07, 2014 at 10:25:23PM +0200, Jakub Hrozek wrote:
>> > On Mon, Jul 07, 2014 at 07:03:01PM +0200, Pavel Reichl wrote:
>> > > On Mon, 2014-06-23 at 11:41 +0200, Sumit Bose wrote:
>> > > > On Mon, Jun 23, 2014 at 11:20:52AM +0200, Jakub Hrozek wrote:
>> > > > > On Tue, Jun 17, 2014 at 06:30:31PM +0200, Pavel Reichl wrote:
>> > > > > > Hello,
>> > > > > >
>> > > > > > please see attached patch.
>> > > > > >
>> > > > > > Regards,
>> > > > > >
>> > > > > > PR
>> > > > >
>> > > > > The patch solves the problem, but I think one part should be
>> > > > > improved:
>> > > > >
>> > > > > > @@ -875,7 +893,13 @@ static void
>> > > > > > sdap_ad_tokengroups_initgr_mapping_done(struct tevent_req *subreq)
>> > > > > > domain =
>> > > > > > find_subdomain_by_sid(get_domains_head(state->domain), sid);
>> > > > > > if (domain == NULL) {
>> > > > > > DEBUG(SSSDBG_MINOR_FAILURE, "Domain not found for SID
>> > > > > > %s\n", sid);
>> > > > > > - continue;
>> > > > > > + if (state->domain->parent == NULL &&
>> > > > > > + state->domain->subdomains == NULL) {
>> > > > > > + domain = state->domain;
>> > > > > > + DEBUG(SSSDBG_TRACE_FUNC, "Using domain %s\n",
>> > > > > > domain->name);
>> > > > > > + } else {
>> > > > > > + continue;
>> > > > > > + }
>> > > > > > }
>> > > > >
>> > > > > I think this is a bit dangerous. I wonder if we should have some
>> > > > > modification of find_subdomain_by_sid that would return the first
>> > > > > configured domain if no subdomain provider was configured or if no
>> > > > > domains had a SID. This could be a separate function.
>> > > >
>> > > > This sounds even more dangerous to me.
>> > > >
>> > > > >
>> > > > > Anyhow, find_subdomain_by_sid is misnamed, we routinely use the
>> > > > > function
>> > > > > to find the primary domain.
>> > > >
>> > > > I think find_subdomain_by_sid() does what the name says and of course
>> > > > it
>> > > > can return the primary domain as long as the SID of the domain is know
>> > ^^^^^^
>> > fwiw, this was my concern, the function is named "find_subdomain" yet it
>> > can find both main domain and subdomain. But I won't bikeshed any further.
>>
>> ah, sorry, now I see your point. I agree that the name misleading but I
>> think this can be fixed after the release.
>
>Would 's/find_subdomain_by_sid/find_domain_by_sid/' be sufficient
>solution?
>
>> bye,
>> Sumit
>>
>> >
>> > > > which is the case for the IPA and AD provider.
>> > > >
>> > >
>> > > > What about adding an explicit check if the running id provider is the
>> > > > plain LDAP provider?
>> > > Would that be acceptable with you Jakub?
>> >
>> > It's a bit hackish but I don't see any other way.
>> >
>> > >
>> > > > As an alternative the LDAP provider can add a
>> > > > special value in the id member of the sss_dom_info struct and then
>> > > > find_subdomain_by_sid can handle this case specially?
>> > > >
>> > > > bye,
>> > > > Sumit
>> > > >
You will change patch to address Jakub comments. So, you can also change
few nitpicks ;-) I would ignore them if patches were already ACK-ed.
>From 77f41aea31afd642d26057018181394c65d81000 Mon Sep 17 00:00:00 2001
>From: Pavel Reichl <prei...@redhat.com>
>Date: Tue, 17 Jun 2014 17:16:14 +0100
>Subject: [PATCH 1/2] LDAP: tokengroups do not work with id_provider=ldap
>
>With plain LDAP provider we already have a sdap_handle, so it should be
>possible
>that in the case where sdom->pvt == NULL sdap_id_op_connect_send() can be
>skipped and sdap_get_ad_tokengroups_send() can be already send with the
>sdap_handle passed to sdap_ad_tokengroups_initgr_mapping_send(). So we should
>only fail if sdom->pvt == NULL and sh == NULL.
>
>if find_subdomain_by_sid() failed we can check if there is only one domain in
>the domain list (state->domain) and in this case continue with this domain
>since
>the LDAP provider does not know about sub-domains and hence can only have one
>configured domain.
>
>Resolves:
>https://fedorahosted.org/sssd/ticket/2345
>---
//snip
>+
>+static errno_t handle_missing_pvt(TALLOC_CTX *mem_ctx,
>+ struct tevent_context *ev,
>+ struct sdap_options *opts,
>+ const char *orig_dn,
>+ int timeout,
>+ const char *username,
>+ struct sdap_handle *sh,
>+ struct tevent_req *req,
>+ tevent_req_fn callback)
>+{
>+ struct tevent_req *subreq = NULL;
>+ errno_t ret;
>+
>+ if (sh != NULL) {
>+ /* plain LDAP provider already has a sdap_handle */
>+ subreq = sdap_get_ad_tokengroups_send(mem_ctx, ev, opts, sh, username,
>+ orig_dn, timeout);
>+ if (subreq == NULL) {
>+ ret = ENOMEM;
>+ tevent_req_error(req, ENOMEM);
^^^^^^
We call in this way: tevent_req_error(req, ret);
There are some exceptions, but it this case you can use it.
>+ goto done;
>+ }
>+
>+ tevent_req_set_callback(subreq, callback, req);
>+ ret = EOK;
>+ goto done;
>+
>+ } else {
>+ ret = EINVAL;
>+ goto done;
>+ }
>+
>+done:
>+ return ret;
>+}
>From 2056ad183feaf41423610f7f1705dad827bb8470 Mon Sep 17 00:00:00 2001
>From: Pavel Reichl <prei...@redhat.com>
>Date: Thu, 10 Jul 2014 10:48:42 +0100
>Subject: [PATCH 2/2] SDAP: Continue resolving SID even if some fail
>
>Resolving groups obtained via Token-Groups in case of disabled ID mapping may
>lead to failure as non-posix groups are not resolved. This patch amends
>sdap_ad_resolve_sids_done() not to abruptly finish request if ENOENT is
>returned.
>
>Resolves:
>https://fedorahosted.org/sssd/ticket/2345
>---
> src/providers/ldap/sdap_async_initgroups_ad.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
>diff --git a/src/providers/ldap/sdap_async_initgroups_ad.c
>b/src/providers/ldap/sdap_async_initgroups_ad.c
>index
>1550f6ceab7fe7ad6c4aed3af674407b92802752..f6a45fb70173355010289dfae30b0a898c61f166
> 100644
>--- a/src/providers/ldap/sdap_async_initgroups_ad.c
>+++ b/src/providers/ldap/sdap_async_initgroups_ad.c
>@@ -646,7 +646,12 @@ static void sdap_ad_resolve_sids_done(struct tevent_req
>*subreq)
>
> ret = groups_get_recv(subreq, &dp_error, &sdap_error);
> talloc_zfree(subreq);
>- if (ret != EOK || sdap_error != EOK || dp_error != DP_ERR_OK) {
>+
>+ if (ret == EOK && sdap_error == ENOENT && dp_error == DP_ERR_OK) {
>+ DEBUG(SSSDBG_CRIT_FAILURE,
>+ "Unable to resolve SID %s - will try next sid.\n",
>+ state->current_sid);
>+ } else if (ret != EOK || (sdap_error != EOK) || dp_error != DP_ERR_OK) {
^^^^^^^^^^^^^^^^^^^
Could you explain why you added parentheses just here?
They weren't in previous code.
> DEBUG(SSSDBG_CRIT_FAILURE, "Unable to resolve SID %s [dp_error: %d, "
> "sdap_error: %d, ret: %d]: %s\n", state->current_sid, dp_error,
> sdap_error, ret, strerror(ret));
LS
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
