Full_Name: Norbert Pueschel Version: HEAD OS: Solaris URL: http://www.pueschel.net/openldap/norbert-pueschel-autogroup-25102010.tar Submission from: (NULL) (80.152.154.98) When starting to use the autogroup-overlay from the contrib directory, I stumbled over some issues: 1) Using non-DN-valued URIs for autogroup does not work correctly, even with the latest version from HEAD. Especially changing group member is not tracked. 2) Using the memberOf-overlay for constructing autogroups does not work 3) autogroup crashes slaptools (e.g. slaptest) when used together with the ppolicy-overlay. 4) The Makefile install the overlay into the wrong directory (lib, should be libexec) I have invested some time and found fixes for all these problems; see the URL for the patch I uploaded.
norbert@pueschel.net wrote: > Full_Name: Norbert Pueschel > Version: HEAD > OS: Solaris > URL: http://www.pueschel.net/openldap/norbert-pueschel-autogroup-25102010.tar > Submission from: (NULL) (80.152.154.98) > > > When starting to use the autogroup-overlay from the contrib directory, I > stumbled over some issues: > > 1) Using non-DN-valued URIs for autogroup does not work correctly, even > with the latest version from HEAD. Especially changing group member is > not tracked. > > 2) Using the memberOf-overlay for constructing autogroups does not work > > 3) autogroup crashes slaptools (e.g. slaptest) when used together with > the ppolicy-overlay. > > 4) The Makefile install the overlay into the wrong directory (lib, > should be libexec) > > I have invested some time and found fixes for all these problems; see > the URL for the patch I uploaded. > > Please read the contributing Guidelines http://www.openldap.org/devel/contributing.html and provide a suitable IPR notice. -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
Updated TAR-file with (hopefully) sufficient copyright notice... http://www.pueschel.net/openldap/norbert-pueschel-autogroup-27102010.tar
I'm not sure if this is still relevant or useful, but included below is a set of steps that will reliably reproduce the issue and a backtrace using autogroup from HEAD and ppolicy, which as noted causes segmentation faults in the slaptools utilities (e.g., slapcat). Note that this backtrace is from a build of 2.4.21, but the issue is present and occurs with 2.4.23 as well, as Norbert has stated. rgs@ldapmaster3:~# ldapmodify -x -h localhost -ZZ -D 'cn=admin,cn=config' -y /etc/ldap.secret dn: cn=module{0},cn=config changetype: modify add: olcModuleLoad olcModuleLoad: ppolicy.la modifying entry "cn=module{0},cn=config" rgs@ldapmaster3:~# ldapadd -x -h localhost -ZZ -D 'cn=admin,cn=config' -y /etc/ldap.secret dn: olcOverlay={3}ppolicy,olcDatabase={1}hdb,cn=config objectClass: olcOverlayConfig objectClass: olcPPolicyConfig olcOverlay: {3}ppolicy olcPPolicyHashCleartext: FALSE olcPPolicyForwardUpdates: FALSE olcPPolicyUseLockout: TRUE adding new entry "olcOverlay={3}ppolicy,olcDatabase={1}hdb,cn=config" rgs@ldapmaster3:~# cat /etc/ldap/slapd.d/cn=config/cn=module{0}.ldif dn: cn=module{0} objectClass: olcModuleList cn: module{0} olcModulePath: /usr/lib/ldap olcModuleLoad: {0}back_hdb.la olcModuleLoad: {1}autogroup.la olcModuleLoad: {2}syncprov.la olcModuleLoad: {3}smbk5pwd.la olcModuleLoad: {4}ppolicy.la structuralObjectClass: olcModuleList creatorsName: cn=config entryUUID: f83e3413-5ba7-4c85-9bf7-ed8728d9fbce createTimestamp: 20091018205311Z entryCSN: 20101129205850.805900Z#000000#001#000000 modifiersName: cn=admin,cn=config modifyTimestamp: 20101129205850Z rgs@ldapmaster3:~/ldap/ldifs/ppolicy# gdb --args slapcat -n1 (gdb) run Starting program: /usr/sbin/slapcat -n1 [Thread debugging using libthread_db enabled] Program received signal SIGSEGV, Segmentation fault. 0x00007ffff2228840 in ppolicy_restrict (op=0x7fffffffd700, rs=0x7fffffffd660) at /usr/src/openldap/openldap-2.4.21/servers/slapd/overlays/ppolicy.c:1271 1271 /usr/src/openldap/openldap-2.4.21/servers/slapd/overlays/ppolicy.c: No such file or directory. in /usr/src/openldap/openldap-2.4.21/servers/slapd/overlays/ppolicy.c (gdb) backtrace #0 0x00007ffff2228840 in ppolicy_restrict (op=0x7fffffffd700, rs=0x7fffffffd660) at /usr/src/openldap/openldap-2.4.21/servers/slapd/overlays/ppolicy.c:1271 #1 0x00007ffff7f6ed3a in overlay_op_walk (op=0x7fffffffd700, rs=0x7fffffffd660, which=<value optimized out>, oi=0x7ffff83220c0, on=0x7ffff8341e50) at /usr/src/openldap/openldap-2.4.21/servers/slapd/backover.c:659 #2 0x00007ffff7f6f8ab in over_op_func (op=0x7fffffffd700, rs=0xfffffffffffffff0, which=4294957760) at /usr/src/openldap/openldap-2.4.21/servers/slapd/backover.c:721 #3 0x00007ffff2846141 in autogroup_db_open (be=<value optimized out>, cr=<value optimized out>) at autogroup.c:1754 #4 0x00007ffff7f6e994 in over_db_open (be=<value optimized out>, cr=0x7fffffffdfb0) at /usr/src/openldap/openldap-2.4.21/servers/slapd/backover.c:155 #5 0x00007ffff7f1388c in backend_startup_one (be=0x7ffff8315f80, cr=0x7fffffffdfb0) at /usr/src/openldap/openldap-2.4.21/servers/slapd/backend.c:224 #6 0x00007ffff7f13a83 in backend_startup (be=0x7ffff8315f80) at /usr/src/openldap/openldap-2.4.21/servers/slapd/backend.c:278 #7 0x00007ffff7f742cc in slap_tool_init (progname=<value optimized out>, tool=2, argc=2, argv=0x7fffffffe5a8) at /usr/src/openldap/openldap-2.4.21/servers/slapd/slapcommon.c:780 #8 0x00007ffff7f7335a in slapcat (argc=-9536, argv=<value optimized out>) at /usr/src/openldap/openldap-2.4.21/servers/slapd/slapcat.c:51 #9 0x00007ffff7ee84f5 in main (argc=2, argv=0x7fffffffe5a8) at /usr/src/openldap/openldap-2.4.21/servers/slapd/main.c:657 (gdb) x 0x7fffffffd700 0x7fffffffd700: 0xffffd870 (gdb) x 0x7fffffffd660 0x7fffffffd660: 0x00000000 For reference, line 1271, located within the ppolicy_restrict function, looks like so: if ( op->o_conn && !BER_BVISEMPTY( &pwcons[op->o_conn->c_conn_idx].dn )) { -Ryan
Just wanted to say that I've tested the patch Norbert submitted with 2.4.21 and 2.4.23, and it fixes the problems for me on both installations. Great job on the patch, Norbert. -Ryan
changed notes changed state Open to Partial moved from Incoming to Contrib
norbert@pueschel.net wrote: > Updated TAR-file with (hopefully) sufficient copyright notice... > > http://www.pueschel.net/openldap/norbert-pueschel-autogroup-27102010.tar Your code does a string compare againset "memberOf" to detect those filter references. 1) it should simply be comparing the AttributeDescription pointers 2) since the "memberof" attribute is actually configurable in the memberof overlay, there's no guarantee that this is the correct attribute to be looking for. It should also be configurable in your patch. You're using strcasecmp, but your inputs are already normalized values. You should just use ber_bvcmp. Replying to the original: > 1) Using non-DN-valued URIs for autogroup does not work correctly, even > with the latest version from HEAD. Especially changing group member is > not tracked. I don't see why this should ever work or be supported. LDAP groups list DNs. > 2) Using the memberOf-overlay for constructing autogroups does not work I don't see any reason why this should work. The memberof overlay is not used to construct groups, it is only used to report on group memberships that have already been defined. -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
Am 30.12.10 18:28, schrieb Howard Chu: > norbert@pueschel.net wrote: >> Updated TAR-file with (hopefully) sufficient copyright notice... >> >> http://www.pueschel.net/openldap/norbert-pueschel-autogroup-27102010.tar > > Your code does a string compare againset "memberOf" to detect those > filter references. > 1) it should simply be comparing the AttributeDescription pointers > 2) since the "memberof" attribute is actually configurable in the > memberof overlay, there's no guarantee that this is the correct > attribute to be looking for. It should also be configurable in your > patch. > You are right, of course. The problem is, I do not understand enough of internal structures to find the correct pointer... I would need to detect the memberof-overlay and find the correct string or pointer to compare to. I will gladly do so if you can give me some hints where to look. > You're using strcasecmp, but your inputs are already normalized > values. You should just use ber_bvcmp. > Right, see above. > Replying to the original: > >> 1) Using non-DN-valued URIs for autogroup does not work correctly, even >> with the latest version from HEAD. Especially changing group member is >> not tracked. > > I don't see why this should ever work or be supported. LDAP groups > list DNs. > Wrong. If you really think so, why did you accept Raphael Ouazana's patch, which is all about making this case work? Also see below. >> 2) Using the memberOf-overlay for constructing autogroups does not work > > I don't see any reason why this should work. The memberof overlay is > not used to construct groups, it is only used to report on group > memberships that have already been defined. Well, consider the following construction (which I am using in our ldap directory and which is the reason I started work on the autogroup overlay): dn: cn=admins,ou=access,dc=networker-gmbh,dc=de objectClass: groupOfNames cn: admins member: uid=xxxxx,ou=people,dc=networker-gmbh,dc=de member: uid=yyyyyy,ou=people,dc=networker-gmbh,dc=de dn: cn=admins@networker-gmbh.de,ou=aliases,dc=networker-gmbh,dc=de objectClass: nisMailAlias objectClass: labeledURIObject cn: admins@networker-gmbh.de labeledURI: ldap:///ou=people,dc=networker-gmbh,dc=de?mail?one?(&(objectClass=inetOrgPerson)(memberOf=cn=admins,ou=access,dc=networker-gmbh,dc=de)) dn: cn=admins,ou=groups,dc=networker-gmbh,dc=de objectClass: posixGroup objectClass: labeledURIObject cn: admins gidNumber: 10100 labeledURI: ldap:///ou=people,dc=networker-gmbh,dc=de?uid?one?(&(objectClass=posixAccount)(memberOf=cn=admins,ou=access,dc=networker-gmbh,dc=de)) Additionally, consider this relevant excerpt from slapd.conf: overlay dynlist dynlist-attrset groupOfNames labeledURI member dynlist-attrset nisMailAlias labeledURI rfc822MailMember:mail overlay autogroup autogroup-attrset posixGroup labeledURI memberUid As you can see, I use memberOf the construct mail aliases and posix group memberships from a groupOfNames master-list. For the mail aliases, dyngroup is sufficient, but the posix groups also need reverse lookups, which is why I'm using the autogroup overlay for this. Also, I cannot use a dn-valued list for the posix groups, as the Solaris NSS-libraries require the uid attribute to not contain a full dn.
changed notes
Norbert Püschel wrote: > Am 30.12.10 18:28, schrieb Howard Chu: >> norbert@pueschel.net wrote: >>> Updated TAR-file with (hopefully) sufficient copyright notice... >>> >>> http://www.pueschel.net/openldap/norbert-pueschel-autogroup-27102010.tar >> >> Your code does a string compare againset "memberOf" to detect those >> filter references. >> 1) it should simply be comparing the AttributeDescription pointers >> 2) since the "memberof" attribute is actually configurable in the >> memberof overlay, there's no guarantee that this is the correct >> attribute to be looking for. It should also be configurable in your >> patch. >> > You are right, of course. The problem is, I do not understand enough of > internal structures to find the correct pointer... I would need to > detect the memberof-overlay and find the correct string or pointer to > compare to. I will gladly do so if you can give me some hints where to look. AttributeDescriptions are a fundamental data structure used throughout the code. It would be impossible to have read any of the existing overlay code without encountering countless examples of their use. There is no need to detect the memberof overlay. All you need to do is provide a configuration option to specify the attribute you are going to treat as a memberof attribute. >> You're using strcasecmp, but your inputs are already normalized >> values. You should just use ber_bvcmp. >> > Right, see above. > >> Replying to the original: >> >>> 1) Using non-DN-valued URIs for autogroup does not work correctly, even >>> with the latest version from HEAD. Especially changing group member is >>> not tracked. >> >> I don't see why this should ever work or be supported. LDAP groups >> list DNs. >> > > Wrong. If you really think so, why did you accept Raphael Ouazana's > patch, which is all about making this case work? > Also see below. >>> 2) Using the memberOf-overlay for constructing autogroups does not work >> >> I don't see any reason why this should work. The memberof overlay is >> not used to construct groups, it is only used to report on group >> memberships that have already been defined. > Well, consider the following construction (which I am using in our ldap > directory and which is the reason I started work on the autogroup overlay): Ugh. > dn: cn=admins,ou=access,dc=networker-gmbh,dc=de > objectClass: groupOfNames > cn: admins > member: uid=xxxxx,ou=people,dc=networker-gmbh,dc=de > member: uid=yyyyyy,ou=people,dc=networker-gmbh,dc=de > > dn: cn=admins@networker-gmbh.de,ou=aliases,dc=networker-gmbh,dc=de > objectClass: nisMailAlias > objectClass: labeledURIObject > cn: admins@networker-gmbh.de > labeledURI: > ldap:///ou=people,dc=networker-gmbh,dc=de?mail?one?(&(objectClass=inetOrgPerson)(memberOf=cn=admins,ou=access,dc=networker-gmbh,dc=de)) > > dn: cn=admins,ou=groups,dc=networker-gmbh,dc=de > objectClass: posixGroup > objectClass: labeledURIObject > cn: admins > gidNumber: 10100 > labeledURI: > ldap:///ou=people,dc=networker-gmbh,dc=de?uid?one?(&(objectClass=posixAccount)(memberOf=cn=admins,ou=access,dc=networker-gmbh,dc=de)) > > Additionally, consider this relevant excerpt from slapd.conf: > > overlay dynlist > dynlist-attrset groupOfNames labeledURI member > dynlist-attrset nisMailAlias labeledURI rfc822MailMember:mail > > overlay autogroup > autogroup-attrset posixGroup labeledURI memberUid > > As you can see, I use memberOf the construct mail aliases and posix > group memberships from a groupOfNames master-list. For the mail aliases, > dyngroup is sufficient, but the posix groups also need reverse lookups, > which is why I'm using the autogroup overlay for this. Also, I cannot > use a dn-valued list for the posix groups, as the Solaris NSS-libraries > require the uid attribute to not contain a full dn. Short answer - use an NSS client that actually supports RFC2307bis, like every other vendor in the world already does. In the meantime, if you submit a cleaned up patch that makes better use of the slapd APIs, we can review it again. -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
> 1) it should simply be comparing the AttributeDescription pointers > 2) since the "memberof" attribute is actually configurable in the memberof > overlay, there's no guarantee that this is the correct attribute to be looking > for. It should also be configurable in your patch. > > You're using strcasecmp, but your inputs are already normalized values. You > should just use ber_bvcmp. > Since I am also interested in this, I took some time to make a new patch. I took Norberts original patch, applied it to a current checkout from HEAD and tried to fix the issues mentioned by Howard. My initial tests are looking good. My C skills are rather mediocre, though, so I hope I didn't slaughter the thing :-) http://www.informatik.uni-bremen.de/~moenoel/ldap/christian-manal-autogroup-27012011.tgz Regards, Christian Manal
Am 27.01.2011 16:38, schrieb moenoel@informatik.uni-bremen.de: >> 1) it should simply be comparing the AttributeDescription pointers >> 2) since the "memberof" attribute is actually configurable in the memberof >> overlay, there's no guarantee that this is the correct attribute to be looking >> for. It should also be configurable in your patch. >> >> You're using strcasecmp, but your inputs are already normalized values. You >> should just use ber_bvcmp. >> > > Since I am also interested in this, I took some time to make a new > patch. I took Norberts original patch, applied it to a current checkout > from HEAD and tried to fix the issues mentioned by Howard. My initial > tests are looking good. > > My C skills are rather mediocre, though, so I hope I didn't slaughter > the thing :-) > > http://www.informatik.uni-bremen.de/~moenoel/ldap/christian-manal-autogroup-27012011.tgz > > > Regards, > Christian Manal > > I just noticed that I messed up the copyright notice. I fixed that and replaced the archive, so the link is still valid.
changed notes changed state Partial to Test
moenoel@informatik.uni-bremen.de wrote: >> 1) it should simply be comparing the AttributeDescription pointers >> 2) since the "memberof" attribute is actually configurable in the memberof >> overlay, there's no guarantee that this is the correct attribute to be looking >> for. It should also be configurable in your patch. >> >> You're using strcasecmp, but your inputs are already normalized values. You >> should just use ber_bvcmp. >> > > Since I am also interested in this, I took some time to make a new > patch. I took Norberts original patch, applied it to a current checkout > from HEAD and tried to fix the issues mentioned by Howard. My initial > tests are looking good. > > My C skills are rather mediocre, though, so I hope I didn't slaughter > the thing :-) > > http://www.informatik.uni-bremen.de/~moenoel/ldap/christian-manal-autogroup-27012011.tgz This patch looks pretty good. There are only one or two minor issues with it, which I will clean up. (E.g., config actions require no mutexes; slapd is always single-threaded when processing config changes.) -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
Am 29.01.2011 21:10, schrieb hyc@symas.com: > moenoel@informatik.uni-bremen.de wrote: >>> 1) it should simply be comparing the AttributeDescription pointers >>> 2) since the "memberof" attribute is actually configurable in the memberof >>> overlay, there's no guarantee that this is the correct attribute to be looking >>> for. It should also be configurable in your patch. >>> >>> You're using strcasecmp, but your inputs are already normalized values. You >>> should just use ber_bvcmp. >>> >> >> Since I am also interested in this, I took some time to make a new >> patch. I took Norberts original patch, applied it to a current checkout >> from HEAD and tried to fix the issues mentioned by Howard. My initial >> tests are looking good. >> >> My C skills are rather mediocre, though, so I hope I didn't slaughter >> the thing :-) >> >> http://www.informatik.uni-bremen.de/~moenoel/ldap/christian-manal-autogroup-27012011.tgz > > This patch looks pretty good. There are only one or two minor issues with it, > which I will clean up. (E.g., config actions require no mutexes; slapd is > always single-threaded when processing config changes.) > Great, thanks!
changed notes changed state Test to Release
changed notes changed state Release to Closed
points 3 and 4 patched in HEAD points 3 and 4 patched in RE24 1 and 2 patched in HEAD 1 and 2 patched in RE24