[SSSD] [PATCH] ldap_opts: Get rid on 389ds specific values in rfc2307bis schema

Jakub Hrozek jhrozek at redhat.com
Fri Jul 25 12:55:51 UTC 2014


On Fri, Jul 25, 2014 at 02:33:24PM +0200, Lukas Slebodnik wrote:
> On (25/07/14 10:40), Jakub Hrozek wrote:
> >On Tue, Jul 22, 2014 at 01:28:17PM +0200, Lukas Slebodnik wrote:
> >> On (21/07/14 11:10), Stephen Gallagher wrote:
> >> >On 07/20/2014 03:20 PM, Jakub Hrozek wrote:
> >> >> On Fri, Jul 18, 2014 at 05:32:09PM +0200, Lukas Slebodnik wrote:
> >> >>> On (18/07/14 16:34), Jakub Hrozek wrote:
> >> >>>> On Thu, Jul 17, 2014 at 04:35:31PM +0200, Lukas Slebodnik
> >> >>>> wrote:
> >> >>>>> ehlo,
> >> >>>>> 
> >> >>>>> There is problem with OpenLDAP server and dereferencing of
> >> >>>>> attributes that is not in the schema  of the server?
> >> >>>>> 
> >> >>>>> sh-4.2$ ldapsearch -x -LLL -h openldap.server.test -b
> >> >>>>> 'dc=example,dc=com' \ -E 'deref=member:uid,dummy_attr'
> >> >>>>> cn=ref_grp Protocol error (2) Additional information:
> >> >>>>> Dereference control: attribute decoding error sh-4.2$ echo
> >> >>>>> $? 2
> >> >>>>> 
> >> >>>>> The attribute nsUniqueID is a 389-only, non-standard
> >> >>>>> attribute. It is an operational attribute that is not in the
> >> >>>>> rfc2307bis nor inetOrgPerson nor posixAccount schema.
> >> >>>>> OpenLDAP supports the standard entryUUID attribute, which is
> >> >>>>> basically the same (uniquely identifies an entry throughout a
> >> >>>>> replication topology), but uses the standard UUID format
> >> >>>>> rather than the non-standard format used by 389.
> >> >>>>> 
> >> >>>>> 4x FIXME removed :-)
> >> >>>>> 
> >> >>>>> 
> >> >>>>> Any comments are welcomed.
> >> >>>>> 
> >> >>>>> LS
> >> >>>> 
> >> >>>> Thanks for the detective work on finding the root cause of the
> >> >>>> problem!
> >> >>>> 
> >> >>>> I wonder if we could remove the attribute completely, though.
> >> >>>> It appears to be completely unused now:
> >> >>>> 
> >> >>>> $ git grep -l SYSDB_UUID src/db/sysdb.h:#define SYSDB_UUID
> >> >>>> "uniqueID" src/providers/ad/ad_opts.h:    { "ldap_user_uuid",
> >> >>>> "objectGUID", SYSDB_UUID, NULL }, src/providers/ad/ad_opts.h:
> >> >>>> { "ldap_group_uuid", "objectGUID", SYSDB_UUID, NULL }, 
> >> >>>> src/providers/ad/ad_opts.h:    { "ldap_netgroup_uuid",
> >> >>>> "nsUniqueId", SYSDB_UUID, NULL }, src/providers/ipa/ipa_opts.h:
> >> >>>> { "ldap_user_uuid", "nsUniqueId", SYSDB_UUID, NULL }, 
> >> >>>> src/providers/ipa/ipa_opts.h:    { "ldap_group_uuid",
> >> >>>> "nsUniqueId", SYSDB_UUID, NULL }, src/providers/ipa/ipa_opts.h:
> >> >>>> { "ipa_netgroup_uuid", "ipaUniqueID", SYSDB_UUID, NULL }, 
> >> >>>> src/providers/ipa/ipa_opts.h:    { "ipa_host_uuid",
> >> >>>> "ipaUniqueID", SYSDB_UUID, NULL}, src/providers/ipa/ipa_opts.h:
> >> >>>> { "ipa_hostgroup_uuid", "ipaUniqueID", SYSDB_UUID, NULL}, 
> >> >>>> src/providers/ipa/ipa_opts.h:    { "ipa_selinux_usermap_uuid",
> >> >>>> "ipaUniqueID", SYSDB_UUID, NULL}, 
> >> >>>> src/providers/ldap/ldap_opts.h:    { "ldap_user_uuid", NULL,
> >> >>>> SYSDB_UUID, NULL }, src/providers/ldap/ldap_opts.h:    {
> >> >>>> "ldap_group_uuid", NULL, SYSDB_UUID, NULL }, 
> >> >>>> src/providers/ldap/ldap_opts.h:    { "ldap_user_uuid",
> >> >>>> "nsUniqueId", SYSDB_UUID, NULL }, 
> >> >>>> src/providers/ldap/ldap_opts.h:    { "ldap_group_uuid",
> >> >>>> "nsUniqueId", SYSDB_UUID, NULL }, 
> >> >>>> src/providers/ldap/ldap_opts.h:    { "ldap_user_uuid",
> >> >>>> "objectGUID", SYSDB_UUID, NULL }, 
> >> >>>> src/providers/ldap/ldap_opts.h:    { "ldap_group_uuid",
> >> >>>> "objectGUID", SYSDB_UUID, NULL }, 
> >> >>>> src/providers/ldap/ldap_opts.h:    { "ldap_netgroup_uuid",
> >> >>>> "nsUniqueId", SYSDB_UUID, NULL },
> >> >>>> 
> >> >>>> And according to "git log -S" it was actually never used.
> >> >>>> 
> >> >>>> So my proposal is to just remove the attribute along with
> >> >>>> SYSDB_UUID.
> >> >>> I was thinking about this change but I thought that we need to
> >> >>> deprecate options before removing. I don't have a problem with
> >> >>> removing them. and it should not be a problem in this case. I
> >> >>> would like to know opinion of older developers (Sumit, Stephen,
> >> >>> ...) Why were these options introduced? Do they help with ldap
> >> >>> search?
> >> >>> 
> >> >>> LS
> >> >> 
> >> >> Sure, being defensive when removing anything is a good idea. But
> >> >> since the option doesn't do anything and never did, I think we're
> >> >> fine. You're right that removing an option requires two acks,
> >> >> though. So far you have mine :-)
> >> >
> >> >I'll ack it as well. I suspect that this was planned for a future use
> >> >that we never ended up with. I don't remember any reason for it.
> >> >
> >> I prepared separate patches for different *_uuid options.
> >> I thought it would be simple change. There is small complication with
> >> IPA_UNIQUE_ID. I am not sure why it was used for storing HBAC rules.
> >> (0005-Remove-macro-IPA_UNIQUE_ID.patch). HBAC works with 5th patch.
> >> I hope I tested all possible ways. If we agree with this version it can be
> >> squashed to 4th patch (0004-LDAP-Remove-unused-option-ipa_hostgroup_uuid)
> >
> >I can't see a reason either. The only benefit of storing the uniqueID,
> >as far as I can see, is to be able to match objects with their server
> >side counterparts. I also tested some HBAC combinations, even offline
> >auth with cache using the old RDN, but with patched code -- everything
> >worked fine, because the code doesn't care about either the DN format or
> >the presence of UUID. We don't need to upgrade sysdb either.
> >
> >I wonder if, with some very old IPA versions, the rules could have no
> >name? The following code in hbac_attrs_to_rule() seems to suggest it:
> >
> >274     ret = sysdb_attrs_get_el(hbac_ctx->rules[idx],
> >275                              IPA_CN, &el);
> >276     if (ret != EOK || el->num_values == 0) {
> >277         DEBUG(SSSDBG_CONF_SETTINGS, "rule has no name, assuming '(none)'.\n");
> >278         new_rule->name = talloc_strdup(new_rule, "(none)");
> >279     } else {
> >280         new_rule->name = talloc_strndup(new_rule,
> >281                                         (const char*) el->values[0].data,
> >282                                         el->values[0].length);
> >283     }
> >284
> >
> >Your patches make 'cn' mandatory because it's used as an RDN value,
> >so additional change to patch 5/7 should also change the code above to
> >require CN.  BTW I also tested that IPA wouldn't let you define two rules
> >with the same CN..
> >
> >So I think we should push these patches to master, if we can't justify
> >the presence of some code, it should be removed..but at the same time,
> >sssd-1-11 should only contain minimal and safe changes at this point.
> >
> >Can you also prepare patches #1, #2 so that they apply on sssd-1-11 and
> >send a version of #3 that only removes the LDAP part?
> 
> Attached patches remove only options ldap_*_uuid. They can be applied on master
> or sssd-1-11.
> 
> Now, you should decide which patch set you want to push to which branch :-)
> 
> LS
> 
> 

ACK to these three.

We can also push the IPA patches on top of these but it's OK to do it
after you're back from vacation, those patches do just cleanup, these
three fix a bug.



More information about the sssd-devel mailing list