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
> > > >
> > > > > _______________________________________________
> > > > > sssd-devel mailing list
> > > > > sssd-devel@lists.fedorahosted.org
> > > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > > _______________________________________________
> > > > sssd-devel mailing list
> > > > sssd-devel@lists.fedorahosted.org
> > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > >
> > >
> > _______________________________________________
> > sssd-devel mailing list
> > sssd-devel@lists.fedorahosted.org
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> _______________________________________________
> sssd-devel mailing list
> sssd-devel@lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>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
---
src/providers/ldap/sdap_async_groups.c | 5 +-
src/providers/ldap/sdap_async_initgroups_ad.c | 82 +++++++++++++++++++++++----
src/util/domain_info_utils.c | 14 +++++
src/util/util.h | 5 ++
4 files changed, 93 insertions(+), 13 deletions(-)
diff --git a/src/providers/ldap/sdap_async_groups.c b/src/providers/ldap/sdap_async_groups.c
index 5ca0487a851eb14a150c1e0c79193b89fbb096c0..2a46d5c1a3a4666413aae6d504fd28c6cc90b737 100644
--- a/src/providers/ldap/sdap_async_groups.c
+++ b/src/providers/ldap/sdap_async_groups.c
@@ -513,7 +513,8 @@ static int sdap_save_group(TALLOC_CTX *memctx,
/* If this object has a SID available, we will determine the correct
* domain by its SID. */
if (sid_str != NULL) {
- subdomain = find_subdomain_by_sid(get_domains_head(dom), sid_str);
+ subdomain = sss_get_domain_by_sid_ldap_fallback(get_domains_head(dom),
+ sid_str);
if (subdomain) {
dom = subdomain;
} else {
@@ -537,7 +538,7 @@ static int sdap_save_group(TALLOC_CTX *memctx,
goto done;
}
- DEBUG(SSSDBG_TRACE_ALL, "AD group [%s] has type flags %#x.",
+ DEBUG(SSSDBG_TRACE_ALL, "AD group [%s] has type flags %#x.\n",
group_name, ad_group_type);
/* Only security groups from AD are considered for POSIX groups.
* Additionally only global and universal group are taken to account
diff --git a/src/providers/ldap/sdap_async_initgroups_ad.c b/src/providers/ldap/sdap_async_initgroups_ad.c
index 80e4f29ad8556944c42669350339f62cfb28b36e..1550f6ceab7fe7ad6c4aed3af674407b92802752 100644
--- a/src/providers/ldap/sdap_async_initgroups_ad.c
+++ b/src/providers/ldap/sdap_async_initgroups_ad.c
@@ -606,7 +606,9 @@ static errno_t sdap_ad_resolve_sids_step(struct tevent_req *req)
}
state->index++;
- domain = find_subdomain_by_sid(state->domain, state->current_sid);
+ domain = sss_get_domain_by_sid_ldap_fallback(state->domain,
+ state->current_sid);
+
if (domain == NULL) {
DEBUG(SSSDBG_MINOR_FAILURE, "SID %s does not belong to any known "
"domain\n", state->current_sid);
@@ -691,6 +693,15 @@ struct sdap_ad_tokengroups_initgr_mapping_state {
static void
sdap_ad_tokengroups_initgr_mapping_connect_done(struct tevent_req *subreq);
static void sdap_ad_tokengroups_initgr_mapping_done(struct tevent_req *subreq);
+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);
static struct tevent_req *
sdap_ad_tokengroups_initgr_mapping_send(TALLOC_CTX *mem_ctx,
@@ -733,11 +744,18 @@ sdap_ad_tokengroups_initgr_mapping_send(TALLOC_CTX *mem_ctx,
sdom = sdap_domain_get(opts, domain);
if (sdom == NULL || sdom->pvt == NULL) {
- DEBUG(SSSDBG_CRIT_FAILURE, "No ID ctx available for [%s].\n",
- domain->name);
- ret = EINVAL;
- goto immediately;
+ ret = handle_missing_pvt(mem_ctx, ev, opts, orig_dn, timeout,
+ state->username, sh, req,
+ sdap_ad_tokengroups_initgr_mapping_done);
+ if (ret == EOK) {
+ return req;
+ } else {
+ DEBUG(SSSDBG_CRIT_FAILURE, "No ID ctx available for [%s].\n",
+ domain->name);
+ goto immediately;
+ }
}
+
subdom_id_ctx = talloc_get_type(sdom->pvt, struct ad_id_ctx);
state->op = sdap_id_op_create(state, subdom_id_ctx->ldap_ctx->conn_cache);
if (!state->op) {
@@ -872,7 +890,7 @@ static void sdap_ad_tokengroups_initgr_mapping_done(struct tevent_req *subreq)
continue;
}
- domain = find_subdomain_by_sid(get_domains_head(state->domain), sid);
+ domain = sss_get_domain_by_sid_ldap_fallback(state->domain, sid);
if (domain == NULL) {
DEBUG(SSSDBG_MINOR_FAILURE, "Domain not found for SID %s\n", sid);
continue;
@@ -1028,10 +1046,16 @@ sdap_ad_tokengroups_initgr_posix_send(TALLOC_CTX *mem_ctx,
sdom = sdap_domain_get(opts, domain);
if (sdom == NULL || sdom->pvt == NULL) {
- DEBUG(SSSDBG_CRIT_FAILURE, "No ID ctx available for [%s].\n",
- domain->name);
- ret = EINVAL;
- goto immediately;
+ ret = handle_missing_pvt(mem_ctx, ev, opts, orig_dn, timeout,
+ state->username, sh, req,
+ sdap_ad_tokengroups_initgr_posix_tg_done);
+ if (ret == EOK) {
+ return req;
+ } else {
+ DEBUG(SSSDBG_CRIT_FAILURE, "No ID ctx available for [%s].\n",
+ domain->name);
+ goto immediately;
+ }
}
subdom_id_ctx = talloc_get_type(sdom->pvt, struct ad_id_ctx);
state->op = sdap_id_op_create(state, subdom_id_ctx->ldap_ctx->conn_cache);
@@ -1161,7 +1185,7 @@ sdap_ad_tokengroups_initgr_posix_tg_done(struct tevent_req *subreq)
sid = sids[i];
DEBUG(SSSDBG_TRACE_LIBS, "Processing membership SID [%s]\n", sid);
- domain = find_subdomain_by_sid(get_domains_head(state->domain), sid);
+ domain = sss_get_domain_by_sid_ldap_fallback(state->domain, sid);
if (domain == NULL) {
DEBUG(SSSDBG_MINOR_FAILURE, "Domain not found for SID %s\n", sid);
continue;
@@ -1378,3 +1402,39 @@ errno_t sdap_ad_tokengroups_initgroups_recv(struct tevent_req *req)
return EOK;
}
+
+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);
+ goto done;
+ }
+
+ tevent_req_set_callback(subreq, callback, req);
+ ret = EOK;
+ goto done;
+
+ } else {
+ ret = EINVAL;
+ goto done;
+ }
+
+done:
+ return ret;
+}
diff --git a/src/util/domain_info_utils.c b/src/util/domain_info_utils.c
index a0fb7b2abe720e798817636b3810d36b7bbc0d93..498672e912a99c01c8b391fbb289c3076108e762 100644
--- a/src/util/domain_info_utils.c
+++ b/src/util/domain_info_utils.c
@@ -153,6 +153,20 @@ struct sss_domain_info *find_subdomain_by_sid(struct sss_domain_info *domain,
return NULL;
}
+struct sss_domain_info*
+sss_get_domain_by_sid_ldap_fallback(struct sss_domain_info *domain,
+ const char* sid)
+{
+ /* LDAP provider doesn't know about sub-domains and hence can only
+ * have one configured domain
+ */
+ if (domain == NULL || strcmp(domain->provider, "ldap") == 0) {
+ return domain;
+ } else {
+ return find_subdomain_by_sid(get_domains_head(domain), sid);
+ }
+}
+
struct sss_domain_info *
find_subdomain_by_object_name(struct sss_domain_info *domain,
const char *object_name)
diff --git a/src/util/util.h b/src/util/util.h
index af2a578180826fa39240ec18c8e11ac8c2d2ae64..3412f668a87f83da3d430bcefadd1ba4c0cff60b 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -504,6 +504,11 @@ struct sss_domain_info *find_subdomain_by_name(struct sss_domain_info *domain,
bool match_any);
struct sss_domain_info *find_subdomain_by_sid(struct sss_domain_info *domain,
const char *sid);
+
+struct sss_domain_info*
+sss_get_domain_by_sid_ldap_fallback(struct sss_domain_info *domain,
+ const char* sid);
+
struct sss_domain_info *
find_subdomain_by_object_name(struct sss_domain_info *domain,
const char *object_name);
--
1.9.3
>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) {
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));
--
1.9.3
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
