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 <preichl(a)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 <preichl(a)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