Re: [Freeipa-devel] [PATCH] 798 Fix indirect member calculation
Endi Sukma Dewata wrote:
On 6/14/2011 8:46 AM, Rob Crittenden wrote:
Endi Sukma Dewata wrote:
On 6/13/2011 10:28 PM, Rob Crittenden wrote:
Endi Sukma Dewata wrote:
NACK. If there's a circular membership the code will run into an
infinite loop. Here's a test scenario:
Group 1 has 2 members: group 2 and group 3.
Group 2 is a member of group 3.
Group 3 is a member of group 2.
Run ipa group-show on group 1, the command doesn't return until it's
killed.
I think the solution will be to deny creating circular groups.
It might be possible to avoid infinite loop this way:
for member in checkmembers:
(result, truncated) = self.find_entries(...)
for m in result[0][1].get('member', []):
# make sure the member is only added once
if m in checkmembers:
continue
checkmembers.append(m)
I came to the same conclusion but I did:
if m not in checkmembers:
checkmembers.append(m)
Updated patch attached
ACK and pushed to master.
pushed to ipa-2-0 as well
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 798 Fix indirect member calculation
On 6/14/2011 8:46 AM, Rob Crittenden wrote:
Endi Sukma Dewata wrote:
On 6/13/2011 10:28 PM, Rob Crittenden wrote:
Endi Sukma Dewata wrote:
NACK. If there's a circular membership the code will run into an
infinite loop. Here's a test scenario:
Group 1 has 2 members: group 2 and group 3.
Group 2 is a member of group 3.
Group 3 is a member of group 2.
Run ipa group-show on group 1, the command doesn't return until it's
killed.
I think the solution will be to deny creating circular groups.
It might be possible to avoid infinite loop this way:
for member in checkmembers:
(result, truncated) = self.find_entries(...)
for m in result[0][1].get('member', []):
# make sure the member is only added once
if m in checkmembers:
continue
checkmembers.append(m)
I came to the same conclusion but I did:
if m not in checkmembers:
checkmembers.append(m)
Updated patch attached
ACK and pushed to master.
--
Endi S. Dewata
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 798 Fix indirect member calculation
Endi Sukma Dewata wrote:
On 6/13/2011 10:28 PM, Rob Crittenden wrote:
Endi Sukma Dewata wrote:
NACK. If there's a circular membership the code will run into an
infinite loop. Here's a test scenario:
Group 1 has 2 members: group 2 and group 3.
Group 2 is a member of group 3.
Group 3 is a member of group 2.
Run ipa group-show on group 1, the command doesn't return until it's
killed.
I think the solution will be to deny creating circular groups.
It might be possible to avoid infinite loop this way:
for member in checkmembers:
(result, truncated) = self.find_entries(...)
for m in result[0][1].get('member', []):
# make sure the member is only added once
if m in checkmembers:
continue
checkmembers.append(m)
I came to the same conclusion but I did:
if m not in checkmembers:
checkmembers.append(m)
Updated patch attached
rob
>From d4a76cb2893215c3efbf4d31e21c420581451a4e Mon Sep 17 00:00:00 2001
From: Rob Crittenden
Date: Mon, 13 Jun 2011 14:54:42 -0400
Subject: [PATCH] Fix indirect member calculation
Indirect membership is calculated by looking at each member and pulling
all the memberof out of it. What was missing was doing nested searches
on any members in that member group.
So if group2 was a member of group1 and group3 was a member of group2
we would miss group3 as being an indirect member of group1.
I updated the nesting test to do deeper nested testing. I confirmed
that this test failed with the old code and works with the new.
This also prevents duplicate indirect users and looping on circular
membership.
ticket https://fedorahosted.org/freeipa/ticket/1273
---
ipaserver/plugins/ldap2.py| 25 ++-
tests/test_xmlrpc/test_nesting.py | 293 -
2 files changed, 271 insertions(+), 47 deletions(-)
diff --git a/ipaserver/plugins/ldap2.py b/ipaserver/plugins/ldap2.py
index b0a5c2c..e4cc72d 100644
--- a/ipaserver/plugins/ldap2.py
+++ b/ipaserver/plugins/ldap2.py
@@ -943,14 +943,21 @@ class ldap2(CrudBackend, Encoder):
# Verify group membership
results = []
-for member in members:
-try:
-(result, truncated) = self.find_entries(searchfilter, attr_list,
-member, time_limit=time_limit,
-size_limit=size_limit, normalize=normalize)
-results.append(list(result[0]))
-except errors.NotFound:
-pass
+if membertype == MEMBERS_ALL or membertype == MEMBERS_INDIRECT:
+checkmembers = copy.deepcopy(members)
+for member in checkmembers:
+try:
+(result, truncated) = self.find_entries(searchfilter,
+attr_list, member, time_limit=time_limit,
+size_limit=size_limit, normalize=normalize)
+results.append(list(result[0]))
+for m in result[0][1].get('member', []):
+# This member may contain other members, add it to our
+# candidate list
+if m not in checkmembers:
+checkmembers.append(m)
+except errors.NotFound:
+pass
if membertype == MEMBERS_ALL:
entries = []
@@ -969,7 +976,7 @@ class ldap2(CrudBackend, Encoder):
entries = []
for e in results:
-if unicode(e[0]) not in real_members:
+if unicode(e[0]) not in real_members and unicode(e[0]) not in entries:
if membertype == MEMBERS_INDIRECT:
entries.append(e[0])
else:
diff --git a/tests/test_xmlrpc/test_nesting.py b/tests/test_xmlrpc/test_nesting.py
index a7e6cb8..5418628 100644
--- a/tests/test_xmlrpc/test_nesting.py
+++ b/tests/test_xmlrpc/test_nesting.py
@@ -27,8 +27,11 @@ from xmlrpc_test import Declarative, fuzzy_digits, fuzzy_uuid
group1 = u'testgroup1'
group2 = u'testgroup2'
group3 = u'testgroup3'
+group4 = u'testgroup4'
user1 = u'tuser1'
user2 = u'tuser2'
+user3 = u'tuser3'
+user4 = u'tuser4'
hostgroup1 = u'testhostgroup1'
hgdn1 = u'cn=%s,cn=hostgroups,cn=accounts,%s' % (hostgroup1, api.env.basedn)
@@ -44,8 +47,11 @@ class test_nesting(Declarative):
('group_del', [group1], {}),
('group_del', [group2], {}),
('group_del', [group3], {}),
+('group_del', [group4], {}),
('user_del', [user1], {}),
('user_del', [user2], {}),
+('user_del', [user3], {}),
+('user_del', [user4], {}),
('host_del', [fqdn1], {}),
('hostgroup_del', [hostgroup1], {}),
('hostgroup_del', [hostgroup2], {}),
@@ -119,6 +125,26 @@ class test_nesting(Declarative):
dict(
+desc='Create %r' % group4,
+command=(
+'group_add', [group4], dict(description=u'Test desc 4')
+),
+expected=dict(
+value=group4,
+sum
Re: [Freeipa-devel] [PATCH] 798 Fix indirect member calculation
On 6/13/2011 10:28 PM, Rob Crittenden wrote:
Endi Sukma Dewata wrote:
NACK. If there's a circular membership the code will run into an
infinite loop. Here's a test scenario:
Group 1 has 2 members: group 2 and group 3.
Group 2 is a member of group 3.
Group 3 is a member of group 2.
Run ipa group-show on group 1, the command doesn't return until it's
killed.
I think the solution will be to deny creating circular groups.
It might be possible to avoid infinite loop this way:
for member in checkmembers:
(result, truncated) = self.find_entries(...)
for m in result[0][1].get('member', []):
# make sure the member is only added once
if m in checkmembers:
continue
checkmembers.append(m)
--
Endi S. Dewata
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 798 Fix indirect member calculation
On Tue, 2011-06-14 at 08:46 +0200, Martin Kosek wrote: > IIRC the algorithms for circular groups processing are already > implemented in SSSD, so we don't have to reinvent the wheel and let us > get some inspiration there :-) They are not very efficient and we have some ideas on how to improve the situation already, but yeah nothing impossible. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 798 Fix indirect member calculation
On Mon, 2011-06-13 at 23:37 -0400, Simo Sorce wrote: > On Mon, 2011-06-13 at 23:28 -0400, Rob Crittenden wrote: > > Endi Sukma Dewata wrote: > > > On 6/13/2011 6:00 PM, Rob Crittenden wrote: > > >> Endi Sukma Dewata wrote: > > >>> On 6/13/2011 2:45 PM, Rob Crittenden wrote: > > Indirect membership is calculated by looking at each member and pulling > > all the memberof out of it. What was missing was doing nested searches > > on any members in that member group. > > > > So if group2 was a member of group1 and group3 was a member of > > group2 we > > would miss group3 as being an indirect member of group1. > > > > I updated the nesting test to do deeper nested testing. I confirmed > > that > > this test failed with the old code and works with the new. > > > > ticket https://fedorahosted.org/freeipa/ticket/1273 > > >>> > > >>> NACK. If a user is an indirect member of a group via 2 different paths, > > >>> the user will be listed twice. Here is a test scenario: > > >>> > > >>> Group 1 has 2 members: group 2 and group 3. > > >>> User X is a member of both group 2 and group 3. > > >>> Group 1's indirect members should only list the user X once. Currently > > >>> it is listed twice. > > >> > > >> Patch and test case updated. > > > > > > NACK. If there's a circular membership the code will run into an > > > infinite loop. Here's a test scenario: > > > > > > Group 1 has 2 members: group 2 and group 3. > > > Group 2 is a member of group 3. > > > Group 3 is a member of group 2. > > > Run ipa group-show on group 1, the command doesn't return until it's > > > killed. > > > > > > > I think the solution will be to deny creating circular groups. > > Although it would be nice to avoid creating circular groups as they are > pointless we really can't assume we can prevent that. In a multi-master > scenario it is possible that 2 admins operating on 2 different masters > will end up creating a circular group dependency. Even though on each > master they will not be, until replication takes place. > > So we MUST (capital as in RFCs) deal with circular groups in the UI and > framework. Entering infinite loops is not an option, use a max-recursion > limit if detecting circular deps is too hard. > If you set the max-recursion limit high enough you will still operate > properly in most scenarios with complex memberships w/o side effects. > > Simo. > IIRC the algorithms for circular groups processing are already implemented in SSSD, so we don't have to reinvent the wheel and let us get some inspiration there :-) Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 798 Fix indirect member calculation
On Mon, 2011-06-13 at 23:28 -0400, Rob Crittenden wrote: > Endi Sukma Dewata wrote: > > On 6/13/2011 6:00 PM, Rob Crittenden wrote: > >> Endi Sukma Dewata wrote: > >>> On 6/13/2011 2:45 PM, Rob Crittenden wrote: > Indirect membership is calculated by looking at each member and pulling > all the memberof out of it. What was missing was doing nested searches > on any members in that member group. > > So if group2 was a member of group1 and group3 was a member of > group2 we > would miss group3 as being an indirect member of group1. > > I updated the nesting test to do deeper nested testing. I confirmed > that > this test failed with the old code and works with the new. > > ticket https://fedorahosted.org/freeipa/ticket/1273 > >>> > >>> NACK. If a user is an indirect member of a group via 2 different paths, > >>> the user will be listed twice. Here is a test scenario: > >>> > >>> Group 1 has 2 members: group 2 and group 3. > >>> User X is a member of both group 2 and group 3. > >>> Group 1's indirect members should only list the user X once. Currently > >>> it is listed twice. > >> > >> Patch and test case updated. > > > > NACK. If there's a circular membership the code will run into an > > infinite loop. Here's a test scenario: > > > > Group 1 has 2 members: group 2 and group 3. > > Group 2 is a member of group 3. > > Group 3 is a member of group 2. > > Run ipa group-show on group 1, the command doesn't return until it's > > killed. > > > > I think the solution will be to deny creating circular groups. Although it would be nice to avoid creating circular groups as they are pointless we really can't assume we can prevent that. In a multi-master scenario it is possible that 2 admins operating on 2 different masters will end up creating a circular group dependency. Even though on each master they will not be, until replication takes place. So we MUST (capital as in RFCs) deal with circular groups in the UI and framework. Entering infinite loops is not an option, use a max-recursion limit if detecting circular deps is too hard. If you set the max-recursion limit high enough you will still operate properly in most scenarios with complex memberships w/o side effects. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 798 Fix indirect member calculation
Endi Sukma Dewata wrote: On 6/13/2011 6:00 PM, Rob Crittenden wrote: Endi Sukma Dewata wrote: On 6/13/2011 2:45 PM, Rob Crittenden wrote: Indirect membership is calculated by looking at each member and pulling all the memberof out of it. What was missing was doing nested searches on any members in that member group. So if group2 was a member of group1 and group3 was a member of group2 we would miss group3 as being an indirect member of group1. I updated the nesting test to do deeper nested testing. I confirmed that this test failed with the old code and works with the new. ticket https://fedorahosted.org/freeipa/ticket/1273 NACK. If a user is an indirect member of a group via 2 different paths, the user will be listed twice. Here is a test scenario: Group 1 has 2 members: group 2 and group 3. User X is a member of both group 2 and group 3. Group 1's indirect members should only list the user X once. Currently it is listed twice. Patch and test case updated. NACK. If there's a circular membership the code will run into an infinite loop. Here's a test scenario: Group 1 has 2 members: group 2 and group 3. Group 2 is a member of group 3. Group 3 is a member of group 2. Run ipa group-show on group 1, the command doesn't return until it's killed. I think the solution will be to deny creating circular groups. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 798 Fix indirect member calculation
On 6/13/2011 6:00 PM, Rob Crittenden wrote: Endi Sukma Dewata wrote: On 6/13/2011 2:45 PM, Rob Crittenden wrote: Indirect membership is calculated by looking at each member and pulling all the memberof out of it. What was missing was doing nested searches on any members in that member group. So if group2 was a member of group1 and group3 was a member of group2 we would miss group3 as being an indirect member of group1. I updated the nesting test to do deeper nested testing. I confirmed that this test failed with the old code and works with the new. ticket https://fedorahosted.org/freeipa/ticket/1273 NACK. If a user is an indirect member of a group via 2 different paths, the user will be listed twice. Here is a test scenario: Group 1 has 2 members: group 2 and group 3. User X is a member of both group 2 and group 3. Group 1's indirect members should only list the user X once. Currently it is listed twice. Patch and test case updated. NACK. If there's a circular membership the code will run into an infinite loop. Here's a test scenario: Group 1 has 2 members: group 2 and group 3. Group 2 is a member of group 3. Group 3 is a member of group 2. Run ipa group-show on group 1, the command doesn't return until it's killed. -- Endi S. Dewata ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 798 Fix indirect member calculation
Endi Sukma Dewata wrote:
On 6/13/2011 2:45 PM, Rob Crittenden wrote:
Indirect membership is calculated by looking at each member and pulling
all the memberof out of it. What was missing was doing nested searches
on any members in that member group.
So if group2 was a member of group1 and group3 was a member of group2 we
would miss group3 as being an indirect member of group1.
I updated the nesting test to do deeper nested testing. I confirmed that
this test failed with the old code and works with the new.
ticket https://fedorahosted.org/freeipa/ticket/1273
NACK. If a user is an indirect member of a group via 2 different paths,
the user will be listed twice. Here is a test scenario:
Group 1 has 2 members: group 2 and group 3.
User X is a member of both group 2 and group 3.
Group 1's indirect members should only list the user X once. Currently
it is listed twice.
Patch and test case updated.
rob
>From 8f9b36c40ffa5bf75fd8baba5f2185f940182607 Mon Sep 17 00:00:00 2001
From: Rob Crittenden
Date: Mon, 13 Jun 2011 14:54:42 -0400
Subject: [PATCH] Fix indirect member calculation
Indirect membership is calculated by looking at each member and pulling
all the memberof out of it. What was missing was doing nested searches
on any members in that member group.
So if group2 was a member of group1 and group3 was a member of group2
we would miss group3 as being an indirect member of group1.
I updated the nesting test to do deeper nested testing. I confirmed
that this test failed with the old code and works with the new.
This also prevents duplicate indirect users.
ticket https://fedorahosted.org/freeipa/ticket/1273
---
ipaserver/plugins/ldap2.py| 24 ++-
tests/test_xmlrpc/test_nesting.py | 293 -
2 files changed, 270 insertions(+), 47 deletions(-)
diff --git a/ipaserver/plugins/ldap2.py b/ipaserver/plugins/ldap2.py
index b0a5c2c..a0b03c1 100644
--- a/ipaserver/plugins/ldap2.py
+++ b/ipaserver/plugins/ldap2.py
@@ -943,14 +943,20 @@ class ldap2(CrudBackend, Encoder):
# Verify group membership
results = []
-for member in members:
-try:
-(result, truncated) = self.find_entries(searchfilter, attr_list,
-member, time_limit=time_limit,
-size_limit=size_limit, normalize=normalize)
-results.append(list(result[0]))
-except errors.NotFound:
-pass
+if membertype == MEMBERS_ALL or membertype == MEMBERS_INDIRECT:
+checkmembers = copy.deepcopy(members)
+for member in checkmembers:
+try:
+(result, truncated) = self.find_entries(searchfilter,
+attr_list, member, time_limit=time_limit,
+size_limit=size_limit, normalize=normalize)
+results.append(list(result[0]))
+for m in result[0][1].get('member', []):
+# This member may contain other members, add it to our
+# candidate list
+checkmembers.append(m)
+except errors.NotFound:
+pass
if membertype == MEMBERS_ALL:
entries = []
@@ -969,7 +975,7 @@ class ldap2(CrudBackend, Encoder):
entries = []
for e in results:
-if unicode(e[0]) not in real_members:
+if unicode(e[0]) not in real_members and unicode(e[0]) not in entries:
if membertype == MEMBERS_INDIRECT:
entries.append(e[0])
else:
diff --git a/tests/test_xmlrpc/test_nesting.py b/tests/test_xmlrpc/test_nesting.py
index a7e6cb8..5418628 100644
--- a/tests/test_xmlrpc/test_nesting.py
+++ b/tests/test_xmlrpc/test_nesting.py
@@ -27,8 +27,11 @@ from xmlrpc_test import Declarative, fuzzy_digits, fuzzy_uuid
group1 = u'testgroup1'
group2 = u'testgroup2'
group3 = u'testgroup3'
+group4 = u'testgroup4'
user1 = u'tuser1'
user2 = u'tuser2'
+user3 = u'tuser3'
+user4 = u'tuser4'
hostgroup1 = u'testhostgroup1'
hgdn1 = u'cn=%s,cn=hostgroups,cn=accounts,%s' % (hostgroup1, api.env.basedn)
@@ -44,8 +47,11 @@ class test_nesting(Declarative):
('group_del', [group1], {}),
('group_del', [group2], {}),
('group_del', [group3], {}),
+('group_del', [group4], {}),
('user_del', [user1], {}),
('user_del', [user2], {}),
+('user_del', [user3], {}),
+('user_del', [user4], {}),
('host_del', [fqdn1], {}),
('hostgroup_del', [hostgroup1], {}),
('hostgroup_del', [hostgroup2], {}),
@@ -119,6 +125,26 @@ class test_nesting(Declarative):
dict(
+desc='Create %r' % group4,
+command=(
+'group_add', [group4], dict(description=u'Test desc 4')
+),
+expected=dict(
+value=group4,
+summary=u'Added group "tes
Re: [Freeipa-devel] [PATCH] 798 Fix indirect member calculation
On 6/13/2011 2:45 PM, Rob Crittenden wrote: Indirect membership is calculated by looking at each member and pulling all the memberof out of it. What was missing was doing nested searches on any members in that member group. So if group2 was a member of group1 and group3 was a member of group2 we would miss group3 as being an indirect member of group1. I updated the nesting test to do deeper nested testing. I confirmed that this test failed with the old code and works with the new. ticket https://fedorahosted.org/freeipa/ticket/1273 NACK. If a user is an indirect member of a group via 2 different paths, the user will be listed twice. Here is a test scenario: Group 1 has 2 members: group 2 and group 3. User X is a member of both group 2 and group 3. Group 1's indirect members should only list the user X once. Currently it is listed twice. -- Endi S. Dewata ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
