[SSSD] [patches] SDAP: "ldap_group_nesting_level = 0" does not appear to work

Pavel Reichl preichl at redhat.com
Sat May 17 20:52:22 UTC 2014


Hello Lukas, thanks for comments please see replies inline, new patches
are attached.

On Fri, 2014-05-16 at 15:34 +0200, Lukas Slebodnik wrote:
> On (13/05/14 00:07), Pavel Reichl wrote:
> >Hello,
> >
> >please see attached patches solving:
> >
> >https://fedorahosted.org/sssd/ticket/2294
> >
> >Thanks,
> >
> >Pavel Reichl
> 
> >From 17d8d7c4e53da2291daebfdbbf90d3448df9e765 Mon Sep 17 00:00:00 2001
> >From: Pavel Reichl <preichl at redhat.com>
> >Date: Mon, 12 May 2014 15:00:26 +0000
> >Subject: [PATCH 2/3] SDAP: Make nesting_level = 0 to ignore nested groups
> >
> >Make ldap_group_nesting_level = 0 to ignore group nesting entirely.
> >
> >Resolves:
> >https://fedorahosted.org/sssd/ticket/2294
> >---
> > src/providers/ldap/sdap_async_groups.c | 94 ++++++++++++++++++++++++++++++++++
> > 1 file changed, 94 insertions(+)
> >
> >diff --git a/src/providers/ldap/sdap_async_groups.c b/src/providers/ldap/sdap_async_groups.c
> >index 4240bb585d57a80199d040ac44891ee1b6429e05..b956e55bbf4f60e411a6bcf9f9ad79cf977f7144 100644
> >--- a/src/providers/ldap/sdap_async_groups.c
> >+++ b/src/providers/ldap/sdap_async_groups.c
> >@@ -28,6 +28,86 @@
> > #include "providers/ldap/sdap_idmap.h"
> > #include "providers/ad/ad_common.h"
> > 
> >+static errno_t sdap_get_entry_attrs_dn(TALLOC_CTX *mem_ctx,
> >+                                       struct sysdb_ctx *ctx,
> >+                                       const char *dn_str,
> >+                                       const char **attrs,
> >+                                       struct ldb_message ***_msgs,
> >+                                       size_t *_num_msgs)
> >+{
> >+    TALLOC_CTX *tmp_ctx;
> >+    struct ldb_dn *base_dn;
> >+    struct ldb_message **msgs;
> >+    size_t num_msgs;
> >+    int ret;
> >+
> >+    tmp_ctx = talloc_new(NULL);
> >+    if (tmp_ctx == NULL) {
> >+        return ENOMEM;
> >+    }
> >+
> >+    base_dn = sysdb_dn(ctx, tmp_ctx, dn_str);
> >+    if (base_dn == NULL) {
> >+        ret = ENOMEM;
> >+        goto done;
> >+    }
> >+
> >+    DEBUG(SSSDBG_TRACE_ALL, "Searching cache for [%s].\n", dn_str);
> >+    ret = sysdb_search_entry(tmp_ctx, ctx, base_dn, LDB_SCOPE_BASE, "", attrs,
> >+                             &num_msgs, &msgs);
> >+    if (ret != EOK) {
> >+        DEBUG(SSSDBG_OP_FAILURE, "sysdb_search_entry failed: [%d]: %s\n",
> >+              ret, strerror(ret));
> >+        goto done;
> >+    }
> >+
> >+    *_msgs = talloc_steal(mem_ctx, msgs);
> >+    *_num_msgs = num_msgs;
> >+
> >+done:
> >+    talloc_zfree(tmp_ctx);
> >+    return ret;
> >+}
> >+
> >+static errno_t sdap_is_member_type_group(struct sysdb_ctx *sctx,
> >+                                         const char* sysdb_member_dn,
> >+                                         bool *isGroup)
>                                            ^^^^^^^^^^^^^^
> http://www.freeipa.org/page/Coding_Style#Naming
> HIGHLY RECOMMENDED: Use low case multi word underscore separated notation for
> naming variables.
> 
> We have also another "SSSD rule" that output parameter start with underscore

Fixed.
> >+{
> >+    struct ldb_message **msgs;
> >+    size_t num_msgs;
> >+    const char *attrs[] = {SYSDB_OBJECTCLASS, NULL};
> >+    TALLOC_CTX *tmp_ctx;
> >+    errno_t ret;
> >+    const char *object_class;
> >+
> >+    tmp_ctx = talloc_new(NULL);
> >+    if (tmp_ctx == NULL) {
> >+        return ENOMEM;
> >+    }
> >+    ret = sdap_get_entry_attrs_dn(tmp_ctx, sctx, sysdb_member_dn, attrs, &msgs,
> >+                                  &num_msgs);
> >+    if (ret == EOK) {
> >+        if (num_msgs != 1) {
> >+            DEBUG(SSSDBG_OP_FAILURE,
> >+                  "Expected 1 value for %s, got %zu\n", attrs[0], num_msgs);
> >+            ret = EINVAL;
> >+            goto done;
> >+        }
> >+
> >+        object_class = ldb_msg_find_attr_as_string(msgs[0],
> >+                                                   SYSDB_OBJECTCLASS, NULL);
> >+        if (strcmp(object_class, SYSDB_GROUP_CLASS) == 0) {
> >+            *isGroup = true;
> >+        } else {
> >+            *isGroup = false;
> >+        }
> >+    }
> >+
> >+done:
> >+    talloc_free(tmp_ctx);
> >+    return ret;
> >+}
> >+
> > /* ==Group-Parsing Routines=============================================== */
> > 
> > static int sdap_find_entry_by_origDN(TALLOC_CTX *memctx,
> >@@ -1211,6 +1291,10 @@ sdap_process_group_members_2307bis(struct tevent_req *req,
> >     char *strdn;
> >     int ret;
> >     int i;
> >+    int nesting_level;
> >+    bool isMemberGroup = false;
> >+
> >+    nesting_level = dp_opt_get_int(state->opts->basic, SDAP_NESTING_LEVEL);
> > 
> >     for (i=0; i < memberel->num_values; i++) {
> >         member_dn = (char *)memberel->values[i].data;
> >@@ -1220,7 +1304,17 @@ sdap_process_group_members_2307bis(struct tevent_req *req,
> >                                         state->dom,
> >                                         member_dn,
> >                                         &strdn);
> >+
> >         if (ret == EOK) {
> >+            if (nesting_level == 0) {
> >+                /* Ignore group members which are groups themselves. */
> >+                ret = sdap_is_member_type_group(state->sysdb, strdn,
> >+                                                &isMemberGroup);
> I think you reimplemented already existing sysdb functions.
> I think that aim is to find out if "strdn" is group.
> 
> strdn is already "sysdb DN" which can be used for function sysdb_search_*
well actually strdn is pointer to char, which can't be directly used by
sysdb_search_*, because they expect ldb_dn. 

> You can obtain groupname from dn with function sysdb_group_dn_name
>     hint: test_sysdb_group_dn_name
> and then you can find group with the function sysdb_search_group_by_name
You are right, but AFIK sysdb_group_dn_nane just returns RDN from DN,
which will cause problems:

dn: name=ub,cn=users,cn=default,cn=sysdb
dn: name=ub,cn=groups,cn=default,cn=sysdb

will both return 'ub'. Implying necessity to test if sysdb_search_group_by_name result's DN matches strdn. 

This leads to a code that is IMHO more complex than my proposed solution.

My solution straightforwardly checks if object type of entry (specified by strdb) is group.    

Please see updated patches (only 2nd patch differs).

Thanks!

> 
> It will simplify your code.
> 
> 
> >+                if (ret == EOK && isMemberGroup) {
> >+                    continue;
> >+                }
> >+            }
> >+
> >             /*
> >              * User already cached in sysdb. Remember the sysdb DN for later
> >              * use by sdap_save_groups()
> >-- 
> >1.8.4.2
> >
> 
> >From a37ab579f7cde089f46d20136bb8987be9039a13 Mon Sep 17 00:00:00 2001
> >From: Pavel Reichl <preichl at redhat.com>
> >Date: Mon, 12 May 2014 22:45:00 +0000
> >Subject: [PATCH 3/3] MAN: Detailed ldap_group_nesting_level option
> >
> >Resolves:
> >https://fedorahosted.org/sssd/ticket/2294
> >---
> > src/man/sssd-ldap.5.xml | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> >diff --git a/src/man/sssd-ldap.5.xml b/src/man/sssd-ldap.5.xml
> >index f93b418c45d9bcd32499860a858c3f829bb245ca..16eb290b3786627e2d44884006f684605e74a7e5 100644
> >--- a/src/man/sssd-ldap.5.xml
> >+++ b/src/man/sssd-ldap.5.xml
> >@@ -880,6 +880,14 @@
> >                             RFC2307 schema.
> >                         </para>
> >                         <para>
> >+                            Note: This option specifies the minimal level of
> >+                            nesting that will be processed by the LDAP
> >+                            provider. However, nested groups beyond this limit
> >+                            <emphasis>may be</emphasis> returned.
> >+                            If ldap_group_nesting_level is set to 0 then no
> >+                            nested groups are processed at all.
> >+                        </para>
> >+                        <para>
> >                             Default: 2
> >                         </para>
> >                     </listitem>
> >-- 
> >1.8.4.2
> >
> 
> LS
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-SYSDB-add-utility-function-sysdb_dn.patch
Type: text/x-patch
Size: 1688 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140517/569414ac/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-SDAP-Make-nesting_level-0-to-ignore-nested-groups.patch
Type: text/x-patch
Size: 3583 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140517/569414ac/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-MAN-Detailed-ldap_group_nesting_level-option.patch
Type: text/x-patch
Size: 1319 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140517/569414ac/attachment-0002.bin>


More information about the sssd-devel mailing list