[SSSD] [PATCH] LDAP: Add a new error code for malformed access control filter

Jakub Hrozek jhrozek at redhat.com
Thu Jan 9 09:50:36 UTC 2014


On Thu, Jan 09, 2014 at 10:39:25AM +0100, Sumit Bose wrote:
> On Wed, Jan 08, 2014 at 05:55:35PM +0100, Jakub Hrozek wrote:
> > Hi,
> > 
> > the attached patch solves https://fedorahosted.org/sssd/ticket/2164 by
> > special casing an openldap return code.
> > 
> > One drawback of the patch is that if the new return code bubbles all the
> > way up, calls to strerror (as opposed to sss_strerror) wouldn't be able
> > to print a nice error message. This patch at least converts direct
> > consumers of the generic_search request.
> 
> I think we should just start using sss_strerror() in new code and
> replace strerror() in old code we modify.

I was actually thinking about mass-replacing strerror for sss_strerror
in master..since sss_strerror is backwards-compatible, there should be no
issues even if strerror was used to print error message from a system
call like write(2) where only errno-ranged error code was present.

Given that we already have one large automated patch (the DEBUG
levels) coming up and the strerror() calls are largely in the same area
the churn wouldn't be too high..

> 
> ...
> 
> >  
> > diff --git a/src/util/util_errors.c b/src/util/util_errors.c
> > index 114c8b04fd354b166d14e526a3bab6a6c0c05951..6860151e1e2fdefd9ebda4005276c6211c447607 100644
> > --- a/src/util/util_errors.c
> > +++ b/src/util/util_errors.c
> > @@ -51,6 +51,7 @@ struct err_string error_to_str[] = {
> >      { "Entry not found" }, /* ERR_NOT_FOUND */
> >      { "Domain not found" }, /* ERR_DOMAIN_NOT_FOUND */
> >      { "Missing configuration file" }, /* ERR_MISSING_CONF */
> > +    { "Malformed search filter" }, /* ERR_LDAP_INVALID_FILTER */
> 
> minor nitpick                                ^^^^^

Right, in the first version I included LDAP in the name of the error but
then I realized a slightly more generic error might be better (perhaps
for sysdb searches as well).

> 
> otherwise ACK
> 
> bye,
> Sumit

Thanks for the review, a new patch is attached.
-------------- next part --------------
>From c373c2695da6e7525c46c83a96337879ba8bbc06 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Wed, 8 Jan 2014 17:12:17 +0100
Subject: [PATCH] LDAP: Add a new error code for malformed access control
 filter

https://fedorahosted.org/sssd/ticket/2164

The patch adds a new error code and special cases the new code so that
access is denied and a nicer log message is shown.
---
 src/providers/ldap/sdap_access.c              |  8 +++++++-
 src/providers/ldap/sdap_async.c               | 12 ++++++------
 src/providers/ldap/sdap_async_groups_ad.c     |  2 +-
 src/providers/ldap/sdap_async_initgroups_ad.c |  4 ++--
 src/util/util_errors.c                        |  1 +
 src/util/util_errors.h                        |  1 +
 6 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/src/providers/ldap/sdap_access.c b/src/providers/ldap/sdap_access.c
index e93400db1c33b04a5c7bf5cd01eca5c9325a4593..8f1f5261bcbadec0ff57d0f2d3622aff5b72fd92 100644
--- a/src/providers/ldap/sdap_access.c
+++ b/src/providers/ldap/sdap_access.c
@@ -854,9 +854,15 @@ static void sdap_access_filter_get_access_done(struct tevent_req *subreq)
             }
         } else if (dp_error == DP_ERR_OFFLINE) {
             ret = sdap_access_filter_decide_offline(req);
+        } else if (ret == ERR_INVALID_FILTER) {
+            sss_log(SSS_LOG_ERR,
+                    "Malformed access control filter [%s]\n", state->filter);
+            DEBUG(SSSDBG_CRIT_FAILURE,
+                ("Malformed access control filter [%s]\n", state->filter));
+            ret = ERR_ACCESS_DENIED;
         } else {
             DEBUG(1, ("sdap_get_generic_send() returned error [%d][%s]\n",
-                      ret, strerror(ret)));
+                      ret, sss_strerror(ret)));
         }
 
         goto done;
diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
index e905d2dd6d539baadcd29aa0869ca04e845947e2..367007bde0011ed4de283b2a50b22538830a5275 100644
--- a/src/providers/ldap/sdap_async.c
+++ b/src/providers/ldap/sdap_async.c
@@ -1306,9 +1306,9 @@ static errno_t sdap_get_generic_ext_step(struct tevent_req *req)
                 sss_log(SSS_LOG_ERR, "LDAP connection error, %s",
                                      sss_ldap_err2string(lret));
             }
-        }
-
-        else {
+        } else if (lret == LDAP_FILTER_ERROR) {
+            ret = ERR_INVALID_FILTER;
+        } else {
             ret = EIO;
         }
         goto done;
@@ -1570,7 +1570,7 @@ static void sdap_get_generic_done(struct tevent_req *subreq)
     talloc_zfree(subreq);
     if (ret) {
         DEBUG(4, ("sdap_get_generic_ext_recv failed [%d]: %s\n",
-                  ret, strerror(ret)));
+                  ret, sss_strerror(ret)));
         tevent_req_error(req, ret);
         return;
     }
@@ -1790,7 +1790,7 @@ static void sdap_x_deref_search_done(struct tevent_req *subreq)
     talloc_zfree(subreq);
     if (ret) {
         DEBUG(4, ("sdap_get_generic_ext_recv failed [%d]: %s\n",
-                  ret, strerror(ret)));
+                  ret, sss_strerror(ret)));
         tevent_req_error(req, ret);
         return;
     }
@@ -2049,7 +2049,7 @@ static void sdap_asq_search_done(struct tevent_req *subreq)
     talloc_zfree(subreq);
     if (ret) {
         DEBUG(4, ("sdap_get_generic_ext_recv failed [%d]: %s\n",
-                  ret, strerror(ret)));
+                  ret, sss_strerror(ret)));
         tevent_req_error(req, ret);
         return;
     }
diff --git a/src/providers/ldap/sdap_async_groups_ad.c b/src/providers/ldap/sdap_async_groups_ad.c
index 9b61c697d5789c3ec3467ec52a7171f6a640ce9e..6a8a4fd139657040ff83cad10ba35a0dde4a0122 100644
--- a/src/providers/ldap/sdap_async_groups_ad.c
+++ b/src/providers/ldap/sdap_async_groups_ad.c
@@ -183,7 +183,7 @@ sdap_get_ad_match_rule_members_step(struct tevent_req *subreq)
     talloc_zfree(subreq);
     if (ret != EOK) {
         DEBUG(SSSDBG_MINOR_FAILURE,
-              ("LDAP search failed: [%s]\n", strerror(ret)));
+              ("LDAP search failed: [%s]\n", sss_strerror(ret)));
         tevent_req_error(req, ret);
         return;
     }
diff --git a/src/providers/ldap/sdap_async_initgroups_ad.c b/src/providers/ldap/sdap_async_initgroups_ad.c
index a3d3ff462622a84bc17293754db3e2ca7a444005..047ddfe65031502aa92e32024cb5f73bebb9fb3d 100644
--- a/src/providers/ldap/sdap_async_initgroups_ad.c
+++ b/src/providers/ldap/sdap_async_initgroups_ad.c
@@ -208,7 +208,7 @@ sdap_get_ad_match_rule_initgroups_step(struct tevent_req *subreq)
     talloc_zfree(subreq);
     if (ret != EOK) {
         DEBUG(SSSDBG_MINOR_FAILURE,
-              ("LDAP search failed: [%s]\n", strerror(ret)));
+              ("LDAP search failed: [%s]\n", sss_strerror(ret)));
         goto error;
     }
 
@@ -383,7 +383,7 @@ static void sdap_get_ad_tokengroups_done(struct tevent_req *subreq)
     talloc_zfree(subreq);
     if (ret != EOK) {
         DEBUG(SSSDBG_MINOR_FAILURE,
-              ("LDAP search failed: [%s]\n", strerror(ret)));
+              ("LDAP search failed: [%s]\n", sss_strerror(ret)));
         goto done;
     }
 
diff --git a/src/util/util_errors.c b/src/util/util_errors.c
index 114c8b04fd354b166d14e526a3bab6a6c0c05951..633257e8da0ef039e555a07ad8b51125114ca01c 100644
--- a/src/util/util_errors.c
+++ b/src/util/util_errors.c
@@ -51,6 +51,7 @@ struct err_string error_to_str[] = {
     { "Entry not found" }, /* ERR_NOT_FOUND */
     { "Domain not found" }, /* ERR_DOMAIN_NOT_FOUND */
     { "Missing configuration file" }, /* ERR_MISSING_CONF */
+    { "Malformed search filter" }, /* ERR_INVALID_FILTER, */
 };
 
 
diff --git a/src/util/util_errors.h b/src/util/util_errors.h
index bca45f392b0357c3f1c848768358cb1d47514715..1332085031dbe6935cbdc94543fa14b09fe81028 100644
--- a/src/util/util_errors.h
+++ b/src/util/util_errors.h
@@ -73,6 +73,7 @@ enum sssd_errors {
     ERR_NOT_FOUND,
     ERR_DOMAIN_NOT_FOUND,
     ERR_MISSING_CONF,
+    ERR_INVALID_FILTER,
     ERR_LAST            /* ALWAYS LAST */
 };
 
-- 
1.8.4.2



More information about the sssd-devel mailing list