Hello,
please see attached patches solving:
https://fedorahosted.org/sssd/ticket/2294
Thanks,
Pavel Reichl
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@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
+{
- 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_* 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
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@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 ofnesting that will be processed by the LDAPprovider. However, nested groups beyond this limit<emphasis>may be</emphasis> returned.If ldap_group_nesting_level is set to 0 then nonested groups are processed at all.</para><para> Default: 2 </para> </listitem>-- 1.8.4.2
LS
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@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@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 ofnesting that will be processed by the LDAPprovider. However, nested groups beyond this limit<emphasis>may be</emphasis> returned.If ldap_group_nesting_level is set to 0 then nonested groups are processed at all.</para><para> Default: 2 </para> </listitem>-- 1.8.4.2
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On (17/05/14 22:52), Pavel Reichl wrote:
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@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:
Thank you for pasting next two ldb DNs. They are vere helpful
dn: name=ub,cn=users,cn=default,cn=sysdb
^^^^^^^^
dn: name=ub,cn=groups,cn=default,cn=sysdb
^^^^^^^^^
From highlighted parts of DN, it is obvious which one is group.
This fact will simplify your patches.
LS
Side note: You wrote two functions with prefix "sdap_", but they are not related to LDAP (sdap is abbreviation of sss LDAP). They are "sysdb" functions with confusing name.
LS
On Mon, 2014-05-19 at 08:08 +0200, Lukas Slebodnik wrote:
On (17/05/14 22:52), Pavel Reichl wrote:
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@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:
Thank you for pasting next two ldb DNs. They are vere helpful
dn: name=ub,cn=users,cn=default,cn=sysdb
^^^^^^^^dn: name=ub,cn=groups,cn=default,cn=sysdb
^^^^^^^^^From highlighted parts of DN, it is obvious which one is group. This fact will simplify your patches.
Well its obvious to me:-), not sure hoe to code general solution without doing something overly complex.
LS
Side note: You wrote two functions with prefix "sdap_", but they are not related to LDAP (sdap is abbreviation of sss LDAP). They are "sysdb" functions with confusing name.
Please be sure to see patch attached in my 2nd mail. I have merged these two functions into one saving some boilerplate code.
This function utilizes sysdb_search_entry and I find it too specific to be moved to sysdb.h. This is the reason why it is a part of sdap module and hence I thought it is mandatory to use sdap prefix...?
In response to your first mail I have tried your approach and although it is working, I find my solution a bit more elegant.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On (19/05/14 10:16), Pavel Reichl wrote:
On Mon, 2014-05-19 at 08:08 +0200, Lukas Slebodnik wrote:
On (17/05/14 22:52), Pavel Reichl wrote:
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@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:
Thank you for pasting next two ldb DNs. They are vere helpful
dn: name=ub,cn=users,cn=default,cn=sysdb
^^^^^^^^dn: name=ub,cn=groups,cn=default,cn=sysdb
^^^^^^^^^From highlighted parts of DN, it is obvious which one is group. This fact will simplify your patches.
Well its obvious to me:-), not sure hoe to code general solution without doing something overly complex.
LS
Side note: You wrote two functions with prefix "sdap_", but they are not related to LDAP (sdap is abbreviation of sss LDAP). They are "sysdb" functions with confusing name.
Please be sure to see patch attached in my 2nd mail. I have merged these two functions into one saving some boilerplate code.
In 1st mail, I didn't noticed easier way.
This function utilizes sysdb_search_entry and I find it too specific to be moved to sysdb.h. This is the reason why it is a part of sdap module and hence I thought it is mandatory to use sdap prefix...?
In response to your first mail I have tried your approach and although it is working, I find my solution a bit more elegant.
LS
On Mon, 2014-05-19 at 13:45 +0200, Lukas Slebodnik wrote:
On (19/05/14 10:16), Pavel Reichl wrote:
On Mon, 2014-05-19 at 08:08 +0200, Lukas Slebodnik wrote:
On (17/05/14 22:52), Pavel Reichl wrote:
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@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:
Thank you for pasting next two ldb DNs. They are vere helpful
dn: name=ub,cn=users,cn=default,cn=sysdb
^^^^^^^^dn: name=ub,cn=groups,cn=default,cn=sysdb
^^^^^^^^^From highlighted parts of DN, it is obvious which one is group. This fact will simplify your patches.
Well its obvious to me:-), not sure hoe to code general solution without doing something overly complex.
LS
Side note: You wrote two functions with prefix "sdap_", but they are not related to LDAP (sdap is abbreviation of sss LDAP). They are "sysdb" functions with confusing name.
Please be sure to see patch attached in my 2nd mail. I have merged these two functions into one saving some boilerplate code.
In 1st mail, I didn't noticed easier way.
This function utilizes sysdb_search_entry and I find it too specific to be moved to sysdb.h. This is the reason why it is a part of sdap module and hence I thought it is mandatory to use sdap prefix...?
In response to your first mail I have tried your approach and although it is working, I find my solution a bit more elegant.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Hello,
I have updated patches and in 3rd patch I added option to disable Token-Groups for AD provider and thus make nesting_level = 0 work.
2nd patch may be simplified even more if kind reviewer (or Lukas:-) ) thinks that call of is_group_dn should be replaced by:
"strstr(strdn, "cn=groups") != NULL"
Thanks,
Pavel Reichl
On (21/05/14 11:54), Pavel Reichl wrote:
On Mon, 2014-05-19 at 13:45 +0200, Lukas Slebodnik wrote:
On (19/05/14 10:16), Pavel Reichl wrote:
On Mon, 2014-05-19 at 08:08 +0200, Lukas Slebodnik wrote:
On (17/05/14 22:52), Pavel Reichl wrote:
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@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:
Thank you for pasting next two ldb DNs. They are vere helpful
dn: name=ub,cn=users,cn=default,cn=sysdb
^^^^^^^^dn: name=ub,cn=groups,cn=default,cn=sysdb
^^^^^^^^^From highlighted parts of DN, it is obvious which one is group. This fact will simplify your patches.
Well its obvious to me:-), not sure hoe to code general solution without doing something overly complex.
LS
Side note: You wrote two functions with prefix "sdap_", but they are not related to LDAP (sdap is abbreviation of sss LDAP). They are "sysdb" functions with confusing name.
Please be sure to see patch attached in my 2nd mail. I have merged these two functions into one saving some boilerplate code.
In 1st mail, I didn't noticed easier way.
This function utilizes sysdb_search_entry and I find it too specific to be moved to sysdb.h. This is the reason why it is a part of sdap module and hence I thought it is mandatory to use sdap prefix...?
In response to your first mail I have tried your approach and although it is working, I find my solution a bit more elegant.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Hello,
I have updated patches and in 3rd patch I added option to disable Token-Groups for AD provider and thus make nesting_level = 0 work.
2nd patch may be simplified even more if kind reviewer (or Lukas:-) ) thinks that call of is_group_dn should be replaced by:
"strstr(strdn, "cn=groups") != NULL"
Yes, please.
For group with 100 users without any nesting group, you will save 700 allocations,
strstr approach is much more lightweight :-)
LS
On Mon, 2014-05-26 at 16:07 +0200, Lukas Slebodnik wrote:
On (21/05/14 11:54), Pavel Reichl wrote:
On Mon, 2014-05-19 at 13:45 +0200, Lukas Slebodnik wrote:
On (19/05/14 10:16), Pavel Reichl wrote:
On Mon, 2014-05-19 at 08:08 +0200, Lukas Slebodnik wrote:
On (17/05/14 22:52), Pavel Reichl wrote:
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@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:
Thank you for pasting next two ldb DNs. They are vere helpful
dn: name=ub,cn=users,cn=default,cn=sysdb
^^^^^^^^dn: name=ub,cn=groups,cn=default,cn=sysdb
^^^^^^^^^From highlighted parts of DN, it is obvious which one is group. This fact will simplify your patches.
Well its obvious to me:-), not sure hoe to code general solution without doing something overly complex.
LS
Side note: You wrote two functions with prefix "sdap_", but they are not related to LDAP (sdap is abbreviation of sss LDAP). They are "sysdb" functions with confusing name.
Please be sure to see patch attached in my 2nd mail. I have merged these two functions into one saving some boilerplate code.
In 1st mail, I didn't noticed easier way.
This function utilizes sysdb_search_entry and I find it too specific to be moved to sysdb.h. This is the reason why it is a part of sdap module and hence I thought it is mandatory to use sdap prefix...?
In response to your first mail I have tried your approach and although it is working, I find my solution a bit more elegant.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Hello,
I have updated patches and in 3rd patch I added option to disable Token-Groups for AD provider and thus make nesting_level = 0 work.
2nd patch may be simplified even more if kind reviewer (or Lukas:-) ) thinks that call of is_group_dn should be replaced by:
"strstr(strdn, "cn=groups") != NULL"
Yes, please.
For group with 100 users without any nesting group, you will save 700 allocations,
strstr approach is much more lightweight :-)
LS
OK, whatever.
Updated patches attached.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Mon, May 26, 2014 at 07:28:37PM +0200, Pavel Reichl wrote:
Hello,
I have updated patches and in 3rd patch I added option to disable Token-Groups for AD provider and thus make nesting_level = 0 work.
2nd patch may be simplified even more if kind reviewer (or Lukas:-) ) thinks that call of is_group_dn should be replaced by:
"strstr(strdn, "cn=groups") != NULL"
Yes, please.
For group with 100 users without any nesting group, you will save 700 allocations,
strstr approach is much more lightweight :-)
LS
OK, whatever.
Updated patches attached.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
From 328def1e84e5d5baa81c8beca3484a659c353a6a Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Mon, 12 May 2014 22:45:00 +0000 Subject: [PATCH 1/3] MAN: Detailed ldap_group_nesting_level option
Resolves: https://fedorahosted.org/sssd/ticket/2294
src/man/sssd-ldap.5.xml | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/src/man/sssd-ldap.5.xml b/src/man/sssd-ldap.5.xml index 6426fe4fca5dc9bb9bc84fcbf633404144052d01..d7bb3475d38ca93044632673bb9c7ac6d83c02f6 100644 --- a/src/man/sssd-ldap.5.xml +++ b/src/man/sssd-ldap.5.xml @@ -928,6 +928,18 @@ RFC2307 schema. </para> <para>
Note: This option specifies the minimal level ofnesting that will be processed by the LDAPprovider. However, nested groups beyond this limit<emphasis>may be</emphasis> returned.
I think this needs further clarification, something like "may be returned if previous lookups already resolved the deeper nesting levels". As with any man page change, please ask some native English speaker for language check.
</para><para>If ldap_group_nesting_level is set to 0 and LDAPprovider is used then no nested groups are processedat all. Same behavior may be achieved for ADprovider if ldap_use_tokengroups is set to false.
I think the last sentence is misleading, the way I read it, disabling tokengroups would always disable nesting, which is not the case, is it?
</para><para> Default: 2 </para> </listitem>-- 1.8.4.2
From 484bda4041269e0e52211a1660ff5e5172cb208f Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@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 | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/src/providers/ldap/sdap_async_groups.c b/src/providers/ldap/sdap_async_groups.c index 89a5afb91e8fe666f834720c6b97e74e266822a6..c3c4e72c0b60cd5e689b5d9b0c42612c1feefa8b 100644 --- a/src/providers/ldap/sdap_async_groups.c +++ b/src/providers/ldap/sdap_async_groups.c @@ -1211,6 +1211,9 @@ sdap_process_group_members_2307bis(struct tevent_req *req, char *strdn; int ret; int i;
int nesting_level;
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 +1223,15 @@ 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. */if (strstr(strdn, SYSDB_GROUPS_CONTAINER) != 0 ) {
While I agree with Lukas that saving allocations for these insensive operations with potentially many loops should be done, could we check that the dn contains the domain group container (cn=Groups,cn=$domname I think) and ideally also that the DN ends with this string? Computing the group container and its lenght could be done just once per group, so it wouldn't be too expensive.
continue;}}/* * User already cached in sysdb. Remember the sysdb DN for later * use by sdap_save_groups()-- 1.8.4.2
From 94722009db7fee6320af1cef5adfad9e0e3356e2 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Wed, 21 May 2014 09:30:13 +0100 Subject: [PATCH 3/3] SDAP: Add option to disable use of Token-Groups
Disabling use of Token-Groups is mandatory if expansion of nested groups is not desired (ldap_group_nesting_level = 0) for AD provider.
Resolves: https://fedorahosted.org/sssd/ticket/2294
src/config/SSSDConfig/__init__.py.in | 1 + src/config/etc/sssd.api.d/sssd-ad.conf | 1 + src/config/etc/sssd.api.d/sssd-ipa.conf | 1 + src/config/etc/sssd.api.d/sssd-ldap.conf | 1 + src/man/sssd-ldap.5.xml | 13 +++++++++++++ src/providers/ad/ad_opts.h | 1 + src/providers/ipa/ipa_opts.h | 1 + src/providers/ldap/ldap_opts.h | 1 + src/providers/ldap/sdap.h | 1 + src/providers/ldap/sdap_async_initgroups.c | 7 +++++-- 10 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/src/config/SSSDConfig/__init__.py.in b/src/config/SSSDConfig/__init__.py.in index e221eba278518b07ead0c39a6ecd7527f86d9b5e..d9b186f73220363f429b51260e6b83bd00a9cdb6 100644 --- a/src/config/SSSDConfig/__init__.py.in +++ b/src/config/SSSDConfig/__init__.py.in @@ -321,6 +321,7 @@ option_strings = {
'ldap_groups_use_matching_rule_in_chain' : _('Use LDAP_MATCHING_RULE_IN_CHAIN for group lookups'), 'ldap_initgroups_use_matching_rule_in_chain' : _('Use LDAP_MATCHING_RULE_IN_CHAIN for initgroup lookups'),
- 'ldap_use_tokengroups' : _('Whether to use Token-Groups'), 'ldap_min_id' : _('Set lower boundary for allowed IDs from the LDAP server'), 'ldap_max_id' : _('Set upper boundary for allowed IDs from the LDAP server'),
diff --git a/src/config/etc/sssd.api.d/sssd-ad.conf b/src/config/etc/sssd.api.d/sssd-ad.conf index b70e74c0ac17da95574756f54538fd4414f6f807..33d460e828607f3640b9d19572b1319d4c0cc001 100644 --- a/src/config/etc/sssd.api.d/sssd-ad.conf +++ b/src/config/etc/sssd.api.d/sssd-ad.conf @@ -111,6 +111,7 @@ ldap_idmap_default_domain = str, None, false ldap_idmap_default_domain_sid = str, None, false ldap_groups_use_matching_rule_in_chain = bool, None, false ldap_initgroups_use_matching_rule_in_chain = bool, None, false +ldap_use_tokengroups = bool, None, false ldap_rfc2307_fallback_to_local_users = bool, None, false
[provider/ad/auth] diff --git a/src/config/etc/sssd.api.d/sssd-ipa.conf b/src/config/etc/sssd.api.d/sssd-ipa.conf index 3a3f6a4cf9586b2a81844e38498ba86b504f1f52..11484e7d4bd9a9da15be38ef2a0fe3a9d931cb50 100644 --- a/src/config/etc/sssd.api.d/sssd-ipa.conf +++ b/src/config/etc/sssd.api.d/sssd-ipa.conf @@ -129,6 +129,7 @@ ldap_idmap_default_domain = str, None, false ldap_idmap_default_domain_sid = str, None, false ldap_groups_use_matching_rule_in_chain = bool, None, false ldap_initgroups_use_matching_rule_in_chain = bool, None, false +ldap_use_tokengroups = bool, None, false ldap_rfc2307_fallback_to_local_users = bool, None, false ipa_server_mode = bool, None, false
diff --git a/src/config/etc/sssd.api.d/sssd-ldap.conf b/src/config/etc/sssd.api.d/sssd-ldap.conf index 0057c080f84d4b5f75107903b03a3cc91f03cc83..fa9cdd69846f142cbeef9b0192360816c9b554fc 100644 --- a/src/config/etc/sssd.api.d/sssd-ldap.conf +++ b/src/config/etc/sssd.api.d/sssd-ldap.conf @@ -118,6 +118,7 @@ ldap_idmap_default_domain = str, None, false ldap_idmap_default_domain_sid = str, None, false ldap_groups_use_matching_rule_in_chain = bool, None, false ldap_initgroups_use_matching_rule_in_chain = bool, None, false +ldap_use_tokengroups = bool, None, false ldap_rfc2307_fallback_to_local_users = bool, None, false ldap_min_id = int, None, false ldap_max_id = int, None, false diff --git a/src/man/sssd-ldap.5.xml b/src/man/sssd-ldap.5.xml index d7bb3475d38ca93044632673bb9c7ac6d83c02f6..f33e138009f40ed56860d3c5f7ab5fe4772d5746 100644 --- a/src/man/sssd-ldap.5.xml +++ b/src/man/sssd-ldap.5.xml @@ -1005,6 +1005,19 @@ </varlistentry>
<varlistentry>
<term>ldap_use_tokengroups</term><listitem><para>This options enables of disables use of Token-Groupsattribute. It is used by AD provider only.
Are you sure? According to the code, the TG lookup is used also if you set ldap_schema to ad..
</para><para>Default: True</para></listitem></varlistentry><varlistentry> <term>ldap_netgroup_object_class (string)</term> <listitem> <para>diff --git a/src/providers/ad/ad_opts.h b/src/providers/ad/ad_opts.h
The rest of the patches looks good to me but I haven't ran any tests yet.
Thank you Jakub for comments. I proposed new version of text to man pages. I updated 2nd patch and generally speaking returned to solution which I proposed in 1st or 2nd iteration of this patches. But this time I traded off code beauty for performance, but I think it's OK.
Regards,
PR
On Tue, 2014-05-27 at 21:37 +0200, Jakub Hrozek wrote:
On Mon, May 26, 2014 at 07:28:37PM +0200, Pavel Reichl wrote:
Hello,
I have updated patches and in 3rd patch I added option to disable Token-Groups for AD provider and thus make nesting_level = 0 work.
2nd patch may be simplified even more if kind reviewer (or Lukas:-) ) thinks that call of is_group_dn should be replaced by:
"strstr(strdn, "cn=groups") != NULL"
Yes, please.
For group with 100 users without any nesting group, you will save 700 allocations,
strstr approach is much more lightweight :-)
LS
OK, whatever.
Updated patches attached.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
From 328def1e84e5d5baa81c8beca3484a659c353a6a Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Mon, 12 May 2014 22:45:00 +0000 Subject: [PATCH 1/3] MAN: Detailed ldap_group_nesting_level option
Resolves: https://fedorahosted.org/sssd/ticket/2294
src/man/sssd-ldap.5.xml | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/src/man/sssd-ldap.5.xml b/src/man/sssd-ldap.5.xml index 6426fe4fca5dc9bb9bc84fcbf633404144052d01..d7bb3475d38ca93044632673bb9c7ac6d83c02f6 100644 --- a/src/man/sssd-ldap.5.xml +++ b/src/man/sssd-ldap.5.xml @@ -928,6 +928,18 @@ RFC2307 schema. </para> <para>
Note: This option specifies the minimal level ofnesting that will be processed by the LDAPprovider. However, nested groups beyond this limit<emphasis>may be</emphasis> returned.I think this needs further clarification, something like "may be returned if previous lookups already resolved the deeper nesting levels". As with any man page change, please ask some native English speaker for language check.
</para><para>If ldap_group_nesting_level is set to 0 and LDAPprovider is used then no nested groups are processedat all. Same behavior may be achieved for ADprovider if ldap_use_tokengroups is set to false.I think the last sentence is misleading, the way I read it, disabling tokengroups would always disable nesting, which is not the case, is it?
</para><para> Default: 2 </para> </listitem>-- 1.8.4.2
From 484bda4041269e0e52211a1660ff5e5172cb208f Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@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 | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/src/providers/ldap/sdap_async_groups.c b/src/providers/ldap/sdap_async_groups.c index 89a5afb91e8fe666f834720c6b97e74e266822a6..c3c4e72c0b60cd5e689b5d9b0c42612c1feefa8b 100644 --- a/src/providers/ldap/sdap_async_groups.c +++ b/src/providers/ldap/sdap_async_groups.c @@ -1211,6 +1211,9 @@ sdap_process_group_members_2307bis(struct tevent_req *req, char *strdn; int ret; int i;
int nesting_level;
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 +1223,15 @@ 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. */if (strstr(strdn, SYSDB_GROUPS_CONTAINER) != 0 ) {While I agree with Lukas that saving allocations for these insensive operations with potentially many loops should be done, could we check that the dn contains the domain group container (cn=Groups,cn=$domname I think) and ideally also that the DN ends with this string? Computing the group container and its lenght could be done just once per group, so it wouldn't be too expensive.
continue;}}/* * User already cached in sysdb. Remember the sysdb DN for later * use by sdap_save_groups()-- 1.8.4.2
From 94722009db7fee6320af1cef5adfad9e0e3356e2 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Wed, 21 May 2014 09:30:13 +0100 Subject: [PATCH 3/3] SDAP: Add option to disable use of Token-Groups
Disabling use of Token-Groups is mandatory if expansion of nested groups is not desired (ldap_group_nesting_level = 0) for AD provider.
Resolves: https://fedorahosted.org/sssd/ticket/2294
src/config/SSSDConfig/__init__.py.in | 1 + src/config/etc/sssd.api.d/sssd-ad.conf | 1 + src/config/etc/sssd.api.d/sssd-ipa.conf | 1 + src/config/etc/sssd.api.d/sssd-ldap.conf | 1 + src/man/sssd-ldap.5.xml | 13 +++++++++++++ src/providers/ad/ad_opts.h | 1 + src/providers/ipa/ipa_opts.h | 1 + src/providers/ldap/ldap_opts.h | 1 + src/providers/ldap/sdap.h | 1 + src/providers/ldap/sdap_async_initgroups.c | 7 +++++-- 10 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/src/config/SSSDConfig/__init__.py.in b/src/config/SSSDConfig/__init__.py.in index e221eba278518b07ead0c39a6ecd7527f86d9b5e..d9b186f73220363f429b51260e6b83bd00a9cdb6 100644 --- a/src/config/SSSDConfig/__init__.py.in +++ b/src/config/SSSDConfig/__init__.py.in @@ -321,6 +321,7 @@ option_strings = {
'ldap_groups_use_matching_rule_in_chain' : _('Use LDAP_MATCHING_RULE_IN_CHAIN for group lookups'), 'ldap_initgroups_use_matching_rule_in_chain' : _('Use LDAP_MATCHING_RULE_IN_CHAIN for initgroup lookups'),
- 'ldap_use_tokengroups' : _('Whether to use Token-Groups'), 'ldap_min_id' : _('Set lower boundary for allowed IDs from the LDAP server'), 'ldap_max_id' : _('Set upper boundary for allowed IDs from the LDAP server'),
diff --git a/src/config/etc/sssd.api.d/sssd-ad.conf b/src/config/etc/sssd.api.d/sssd-ad.conf index b70e74c0ac17da95574756f54538fd4414f6f807..33d460e828607f3640b9d19572b1319d4c0cc001 100644 --- a/src/config/etc/sssd.api.d/sssd-ad.conf +++ b/src/config/etc/sssd.api.d/sssd-ad.conf @@ -111,6 +111,7 @@ ldap_idmap_default_domain = str, None, false ldap_idmap_default_domain_sid = str, None, false ldap_groups_use_matching_rule_in_chain = bool, None, false ldap_initgroups_use_matching_rule_in_chain = bool, None, false +ldap_use_tokengroups = bool, None, false ldap_rfc2307_fallback_to_local_users = bool, None, false
[provider/ad/auth] diff --git a/src/config/etc/sssd.api.d/sssd-ipa.conf b/src/config/etc/sssd.api.d/sssd-ipa.conf index 3a3f6a4cf9586b2a81844e38498ba86b504f1f52..11484e7d4bd9a9da15be38ef2a0fe3a9d931cb50 100644 --- a/src/config/etc/sssd.api.d/sssd-ipa.conf +++ b/src/config/etc/sssd.api.d/sssd-ipa.conf @@ -129,6 +129,7 @@ ldap_idmap_default_domain = str, None, false ldap_idmap_default_domain_sid = str, None, false ldap_groups_use_matching_rule_in_chain = bool, None, false ldap_initgroups_use_matching_rule_in_chain = bool, None, false +ldap_use_tokengroups = bool, None, false ldap_rfc2307_fallback_to_local_users = bool, None, false ipa_server_mode = bool, None, false
diff --git a/src/config/etc/sssd.api.d/sssd-ldap.conf b/src/config/etc/sssd.api.d/sssd-ldap.conf index 0057c080f84d4b5f75107903b03a3cc91f03cc83..fa9cdd69846f142cbeef9b0192360816c9b554fc 100644 --- a/src/config/etc/sssd.api.d/sssd-ldap.conf +++ b/src/config/etc/sssd.api.d/sssd-ldap.conf @@ -118,6 +118,7 @@ ldap_idmap_default_domain = str, None, false ldap_idmap_default_domain_sid = str, None, false ldap_groups_use_matching_rule_in_chain = bool, None, false ldap_initgroups_use_matching_rule_in_chain = bool, None, false +ldap_use_tokengroups = bool, None, false ldap_rfc2307_fallback_to_local_users = bool, None, false ldap_min_id = int, None, false ldap_max_id = int, None, false diff --git a/src/man/sssd-ldap.5.xml b/src/man/sssd-ldap.5.xml index d7bb3475d38ca93044632673bb9c7ac6d83c02f6..f33e138009f40ed56860d3c5f7ab5fe4772d5746 100644 --- a/src/man/sssd-ldap.5.xml +++ b/src/man/sssd-ldap.5.xml @@ -1005,6 +1005,19 @@ </varlistentry>
<varlistentry>
<term>ldap_use_tokengroups</term><listitem><para>This options enables of disables use of Token-Groupsattribute. It is used by AD provider only.Are you sure? According to the code, the TG lookup is used also if you set ldap_schema to ad..
</para><para>Default: True</para></listitem></varlistentry><varlistentry> <term>ldap_netgroup_object_class (string)</term> <listitem> <para>diff --git a/src/providers/ad/ad_opts.h b/src/providers/ad/ad_opts.h
The rest of the patches looks good to me but I haven't ran any tests yet. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 05/30/2014 08:52 AM, Pavel Reichl wrote:
Thank you Jakub for comments. I proposed new version of text to man pages. I updated 2nd patch and generally speaking returned to solution which I proposed in 1st or 2nd iteration of this patches. But this time I traded off code beauty for performance, but I think it's OK.
Regards,
PR
On Tue, 2014-05-27 at 21:37 +0200, Jakub Hrozek wrote:
On Mon, May 26, 2014 at 07:28:37PM +0200, Pavel Reichl wrote:
Hello,
I have updated patches and in 3rd patch I added option to disable Token-Groups for AD provider and thus make nesting_level = 0 work.
2nd patch may be simplified even more if kind reviewer (or Lukas:-) ) thinks that call of is_group_dn should be replaced by:
"strstr(strdn, "cn=groups") != NULL"
Yes, please.
For group with 100 users without any nesting group, you will save 700 allocations,
strstr approach is much more lightweight :-)
LS
OK, whatever.
Updated patches attached.
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
From 328def1e84e5d5baa81c8beca3484a659c353a6a Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Mon, 12 May 2014 22:45:00 +0000 Subject: [PATCH 1/3] MAN: Detailed ldap_group_nesting_level option
Resolves: https://fedorahosted.org/sssd/ticket/2294 --- src/man/sssd-ldap.5.xml | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/src/man/sssd-ldap.5.xml b/src/man/sssd-ldap.5.xml index 6426fe4fca5dc9bb9bc84fcbf633404144052d01..d7bb3475d38ca93044632673bb9c7ac6d83c02f6 100644 --- a/src/man/sssd-ldap.5.xml +++ b/src/man/sssd-ldap.5.xml @@ -928,6 +928,18 @@ 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.
I think this needs further clarification, something like "may be returned if previous lookups already resolved the deeper nesting levels". As with any man page change, please ask some native English speaker for language check.
</para> +<para> + If ldap_group_nesting_level is set to 0 and LDAP + provider is used then no nested groups are processed + at all. Same behavior may be achieved for AD + provider if ldap_use_tokengroups is set to false.
I think the last sentence is misleading, the way I read it, disabling tokengroups would always disable nesting, which is not the case, is it?
</para> +<para> Default: 2 </para> </listitem> -- 1.8.4.2
From 484bda4041269e0e52211a1660ff5e5172cb208f Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@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 | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/src/providers/ldap/sdap_async_groups.c b/src/providers/ldap/sdap_async_groups.c index 89a5afb91e8fe666f834720c6b97e74e266822a6..c3c4e72c0b60cd5e689b5d9b0c42612c1feefa8b 100644 --- a/src/providers/ldap/sdap_async_groups.c +++ b/src/providers/ldap/sdap_async_groups.c @@ -1211,6 +1211,9 @@ sdap_process_group_members_2307bis(struct tevent_req *req, char *strdn; int ret; int i; + int nesting_level; + + 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 +1223,15 @@ 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. */ + if (strstr(strdn, SYSDB_GROUPS_CONTAINER) != 0 ) {
While I agree with Lukas that saving allocations for these insensive operations with potentially many loops should be done, could we check that the dn contains the domain group container (cn=Groups,cn=$domname I think) and ideally also that the DN ends with this string? Computing the group container and its lenght could be done just once per group, so it wouldn't be too expensive.
continue; + } +} + /* * User already cached in sysdb. Remember the sysdb DN for later * use by sdap_save_groups() -- 1.8.4.2
From 94722009db7fee6320af1cef5adfad9e0e3356e2 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Wed, 21 May 2014 09:30:13 +0100 Subject: [PATCH 3/3] SDAP: Add option to disable use of Token-Groups
Disabling use of Token-Groups is mandatory if expansion of nested groups is not desired (ldap_group_nesting_level = 0) for AD provider.
Resolves: https://fedorahosted.org/sssd/ticket/2294 --- src/config/SSSDConfig/__init__.py.in | 1 + src/config/etc/sssd.api.d/sssd-ad.conf | 1 + src/config/etc/sssd.api.d/sssd-ipa.conf | 1 + src/config/etc/sssd.api.d/sssd-ldap.conf | 1 + src/man/sssd-ldap.5.xml | 13 +++++++++++++ src/providers/ad/ad_opts.h | 1 + src/providers/ipa/ipa_opts.h | 1 + src/providers/ldap/ldap_opts.h | 1 + src/providers/ldap/sdap.h | 1 + src/providers/ldap/sdap_async_initgroups.c | 7 +++++-- 10 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/src/config/SSSDConfig/__init__.py.in b/src/config/SSSDConfig/__init__.py.in index e221eba278518b07ead0c39a6ecd7527f86d9b5e..d9b186f73220363f429b51260e6b83bd00a9cdb6 100644 --- a/src/config/SSSDConfig/__init__.py.in +++ b/src/config/SSSDConfig/__init__.py.in @@ -321,6 +321,7 @@ option_strings = {
'ldap_groups_use_matching_rule_in_chain' : _('Use LDAP_MATCHING_RULE_IN_CHAIN for group lookups'), 'ldap_initgroups_use_matching_rule_in_chain' : _('Use LDAP_MATCHING_RULE_IN_CHAIN for initgroup lookups'), + 'ldap_use_tokengroups' : _('Whether to use Token-Groups'), 'ldap_min_id' : _('Set lower boundary for allowed IDs from the LDAP server'), 'ldap_max_id' : _('Set upper boundary for allowed IDs from the LDAP server'),
diff --git a/src/config/etc/sssd.api.d/sssd-ad.conf b/src/config/etc/sssd.api.d/sssd-ad.conf index b70e74c0ac17da95574756f54538fd4414f6f807..33d460e828607f3640b9d19572b1319d4c0cc001 100644 --- a/src/config/etc/sssd.api.d/sssd-ad.conf +++ b/src/config/etc/sssd.api.d/sssd-ad.conf @@ -111,6 +111,7 @@ ldap_idmap_default_domain = str, None, false ldap_idmap_default_domain_sid = str, None, false ldap_groups_use_matching_rule_in_chain = bool, None, false ldap_initgroups_use_matching_rule_in_chain = bool, None, false +ldap_use_tokengroups = bool, None, false ldap_rfc2307_fallback_to_local_users = bool, None, false
[provider/ad/auth] diff --git a/src/config/etc/sssd.api.d/sssd-ipa.conf b/src/config/etc/sssd.api.d/sssd-ipa.conf index 3a3f6a4cf9586b2a81844e38498ba86b504f1f52..11484e7d4bd9a9da15be38ef2a0fe3a9d931cb50 100644 --- a/src/config/etc/sssd.api.d/sssd-ipa.conf +++ b/src/config/etc/sssd.api.d/sssd-ipa.conf @@ -129,6 +129,7 @@ ldap_idmap_default_domain = str, None, false ldap_idmap_default_domain_sid = str, None, false ldap_groups_use_matching_rule_in_chain = bool, None, false ldap_initgroups_use_matching_rule_in_chain = bool, None, false +ldap_use_tokengroups = bool, None, false ldap_rfc2307_fallback_to_local_users = bool, None, false ipa_server_mode = bool, None, false
diff --git a/src/config/etc/sssd.api.d/sssd-ldap.conf b/src/config/etc/sssd.api.d/sssd-ldap.conf index 0057c080f84d4b5f75107903b03a3cc91f03cc83..fa9cdd69846f142cbeef9b0192360816c9b554fc 100644 --- a/src/config/etc/sssd.api.d/sssd-ldap.conf +++ b/src/config/etc/sssd.api.d/sssd-ldap.conf @@ -118,6 +118,7 @@ ldap_idmap_default_domain = str, None, false ldap_idmap_default_domain_sid = str, None, false ldap_groups_use_matching_rule_in_chain = bool, None, false ldap_initgroups_use_matching_rule_in_chain = bool, None, false +ldap_use_tokengroups = bool, None, false ldap_rfc2307_fallback_to_local_users = bool, None, false ldap_min_id = int, None, false ldap_max_id = int, None, false diff --git a/src/man/sssd-ldap.5.xml b/src/man/sssd-ldap.5.xml index d7bb3475d38ca93044632673bb9c7ac6d83c02f6..f33e138009f40ed56860d3c5f7ab5fe4772d5746 100644 --- a/src/man/sssd-ldap.5.xml +++ b/src/man/sssd-ldap.5.xml @@ -1005,6 +1005,19 @@
</varlistentry>
<varlistentry> + <term>ldap_use_tokengroups</term> + <listitem> + <para> + This options enables of disables use of Token-Groups + attribute. It is used by AD provider only.
Are you sure? According to the code, the TG lookup is used also if you set ldap_schema to ad..
</para> +<para> + Default: True + </para> + </listitem> + </varlistentry> + + <varlistentry> <term>ldap_netgroup_object_class (string)</term> <listitem> <para> diff --git a/src/providers/ad/ad_opts.h b/src/providers/ad/ad_opts.h
The rest of the patches looks good to me but I haven't ran any tests yet.
Patch 0001: Ack to the manpage contents.
On Fri, May 30, 2014 at 02:52:50PM +0200, Pavel Reichl wrote:
Thank you Jakub for comments. I proposed new version of text to man pages. I updated 2nd patch and generally speaking returned to solution which I proposed in 1st or 2nd iteration of this patches. But this time I traded off code beauty for performance, but I think it's OK.
Regards,
PR
ACK.
I tested the patch with LDAP provider connected to an IPA server, AD provider connected to an AD server and finally with LDAP provider connected to an AD server. The first two worked as I would have expected.
The third one, however did not, seems like we broke that combination in 1.11: https://fedorahosted.org/sssd/ticket/2345
But that issue is not related to your patches, so we can move forward and fix that bug separately.
On Mon, Jun 02, 2014 at 03:15:33PM +0200, Jakub Hrozek wrote:
On Fri, May 30, 2014 at 02:52:50PM +0200, Pavel Reichl wrote:
Thank you Jakub for comments. I proposed new version of text to man pages. I updated 2nd patch and generally speaking returned to solution which I proposed in 1st or 2nd iteration of this patches. But this time I traded off code beauty for performance, but I think it's OK.
Regards,
PR
ACK.
I tested the patch with LDAP provider connected to an IPA server, AD provider connected to an AD server and finally with LDAP provider connected to an AD server. The first two worked as I would have expected.
The third one, however did not, seems like we broke that combination in 1.11: https://fedorahosted.org/sssd/ticket/2345
But that issue is not related to your patches, so we can move forward and fix that bug separately.
Pushed to master: 69994add9cd4e57d40b3b7a0b1783ef2d0aa974c c30f1d05d6c75080fbe8ea9868f291e7a331ad44 3c1899348804713b49ba9c1f2bc782892c47c2fa and to sssd-1-11: 4221bd76e2b631684f2dc7e8c625fd7b27947cf8 98052f6f186f27a6fde4786274132a6bb4d69e79 e97a7f5bfbb6de33fd722cf613dbaea042126ca6
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Pavel Reichl -
Stephen Gallagher