On 09/05/2012 09:22 AM, Martin Kosek wrote: > On 09/05/2012 03:47 AM, Rob Crittenden wrote: >> Rob Crittenden wrote: >>> Martin Kosek wrote: >>>> On 08/30/2012 02:53 PM, Rob Crittenden wrote: >>>>> Martin Kosek wrote: >>>>>> Current objectclass updates in a form of "replace" update instruction >>>>>> dependent on exact match of the old object class specification in the >>>>>> update instruction and the real value in LDAP. However, this >>>>>> approach is >>>>>> very error prone as object class definition can easily differ as for >>>>>> example because of unexpected X-ORIGIN value. Such objectclass update >>>>>> failures may lead to serious malfunctions later. >>>>>> >>>>>> Add new update instruction type "replaceoc" with the following format: >>>>>> replaceoc:OID:new >>>>>> This update instruction will always replace an objectclass with >>>>>> specified OID with the new definition. >>>>>> >>>>>> https://fedorahosted.org/freeipa/ticket/2440 >>>>> >>>>> This works ok. Martin and I had a conversation in IRC about it. >>>>> >>>>> This moves from replacing a specific bit of schema with a new one, in >>>>> all >>>>> cases. I wonder if we should be more conservative and know what we're >>>>> replacing >>>>> in advance. >>>>> >>>>> rob >>>>> >>>> >>>> You are right, I was too harsh when replacing the objectclasses. This >>>> would >>>> cause issues when LDAP update would be run on a replica with lower >>>> version and >>>> older objectclass definitions. >>>> >>>> I came up with an alternative solution and instead of always replacing >>>> the >>>> objectclass I rather reverted to old-OC:new-OC style which should be >>>> safer. >>>> Now, the LDAP updater always normalizes an objectclass before >>>> comparing it >>>> using python-ldap objectclass model. With this approach, objectclasses >>>> differing only in X-ORIGIN or white spaces should match and be updated. >>>> >>>> Martin >>>> >>> >>> NACK >>> >>> I think this: >>> >>> + for value in replaced_values: >>> + entry_values.remove(old) >>> >>> Should be: >>> >>> + entry_values.remove(value) > > Right. Thanks for the fix. It only worked in my testing because I had no > objectclass update which would differ in X-ORIGIN or whitespaces or case. I > tried to mangle an update file and with this fix it did the update even if > X-ORIGIN and whitespaces differed. > >>> >>> I'm still doing other testing but this is what I've found so far. >>> >>> rob >> >> I did some more testing and it looks like this will do the trick. >> >> I also found a place where the schema was left as unicode and causing it to >> blow up inside python-ldap. Here is the diff on my working instance: >> >> diff -u ipaserver/install/ldapupdate.py >> /usr/lib/python2.7/site-packages/ipaserver/install/ldapupdate.py >> --- ipaserver/install/ldapupdate.py 2012-09-04 16:59:33.210688723 -0400 >> +++ /usr/lib/python2.7/site-packages/ipaserver/install/ldapupdate.py >> 2012-09-04 >> 21:47:01.583574375 -0400 >> @@ -643,7 +643,7 @@ >> self.debug('replace: no match for replaced >> ObjectClass "%s"', old) >> continue >> for value in replaced_values: >> - entry_values.remove(old) >> + entry_values.remove(value) >> else: >> entry_values.remove(old) >> entry_values.append(new) >> @@ -772,7 +772,11 @@ >> updated = False >> changes = self.conn.generateModList(entry.origDataDict(), >> entry.toDict()) >> if (entry.dn == DN(('cn', 'schema'))): >> - updated = self.is_schema_updated(entry.toDict()) >> + d = dict() >> + e = entry.toDict() >> + for k,v in e.items(): >> + d[k] = [str(x) for x in v] >> + updated = self.is_schema_updated(d) >> else: >> if len(changes) >= 1: >> updated = True >> >> rob >> > > I did not hit this error during testing, but at least it won't harm. Sending > an > updated patch. >
Sending a slightly updated patch which now makes sure that we always pass objectclass of str (and not unicode) type to ObjectClass model. This should make it more bullet-proof. Martin
>From 22c4d3343039644eca5a2d5838b3149a1ff6807b Mon Sep 17 00:00:00 2001 From: Martin Kosek <mko...@redhat.com> Date: Tue, 4 Sep 2012 13:18:54 +0200 Subject: [PATCH] Add safe updates for objectClasses Current objectclass updates in a form of "replace" update instruction dependent on exact match of the old object class specification in the update instruction and the real value in LDAP. However, this approach is very error prone as object class definition can easily differ as for example because of unexpected X-ORIGIN value. Such objectclass update failures may lead to serious malfunctions later. When comparing the objectclasses, make sure we normalize them both before we compare them to mitigate these kinds of errors. python-ldap's objectclass model can be utilized to do the normalization part. One objectclass update instruction was changed to do a replace of an objectclass separately from add update instruction so that we really only replace what's stored in LDAP. https://fedorahosted.org/freeipa/ticket/2440 --- install/updates/10-bind-schema.update | 2 ++ ipaserver/install/ldapupdate.py | 52 +++++++++++++++++++++++++++++++++-- 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/install/updates/10-bind-schema.update b/install/updates/10-bind-schema.update index 0edbad2046929210e8b88d8232d24b5298912eb5..3c43c8ec79fe6cb9830a27fb2880b6ed0cf0d8e4 100644 --- a/install/updates/10-bind-schema.update +++ b/install/updates/10-bind-schema.update @@ -75,4 +75,6 @@ add:objectClasses: MUST idnsName MAY managedBy X-ORIGIN 'IPA v3' ) + +dn: cn=schema replace:objectClasses:( 2.16.840.1.113730.3.8.6.1 NAME 'idnsZone' DESC 'Zone class' SUP idnsRecord STRUCTURAL MUST ( idnsZoneActive $$ idnsSOAmName $$ idnsSOArName $$ idnsSOAserial $$ idnsSOArefresh $$ idnsSOAretry $$ idnsSOAexpire $$ idnsSOAminimum ) MAY idnsUpdatePolicy )::( 2.16.840.1.113730.3.8.6.1 NAME 'idnsZone' DESC 'Zone class' SUP idnsRecord STRUCTURAL MUST ( idnsName $$ idnsZoneActive $$ idnsSOAmName $$ idnsSOArName $$ idnsSOAserial $$ idnsSOArefresh $$ idnsSOAretry $$ idnsSOAexpire $$ idnsSOAminimum ) MAY ( idnsUpdatePolicy $$ idnsAllowQuery $$ idnsAllowTransfer $$ idnsAllowSyncPTR $$ idnsForwardPolicy $$ idnsForwarders ) ) diff --git a/ipaserver/install/ldapupdate.py b/ipaserver/install/ldapupdate.py index d673ad57dd4abef68fefb256d933dd6341e6c847..111769ffee1d04f2036d3abe49190c715e13f99a 100644 --- a/ipaserver/install/ldapupdate.py +++ b/ipaserver/install/ldapupdate.py @@ -35,6 +35,7 @@ from ipalib import errors from ipalib import api from ipapython.dn import DN import ldap +from ldap.schema.models import ObjectClass from ipapython.ipa_log_manager import * import krbV import platform @@ -546,6 +547,28 @@ class LDAPUpdate: entry_values = [] else: entry_values = [entry_values] + + # Replacing objectClassess needs a special handling and + # normalization of OC definitions to avoid update failures for + # example when X-ORIGIN is the only difference + objectclass_replacement = False + if action == "replace" and entry.dn == DN(('cn', 'schema')) and \ + attr.lower() == "objectclasses": + objectclass_replacement = True + oid_index = {} + # build the OID index for replacing + for objectclass in entry_values: + try: + objectclass_object = ObjectClass(str(objectclass)) + except Exception, e: + self.error('replace: cannot parse ObjectClass "%s": %s', + objectclass, e) + continue + # In a corner case, there may be more representations of + # the same objectclass due to the previous updates + # We want to replace them all + oid_index.setdefault(objectclass_object.oid, []).append(objectclass) + for update_value in update_values: if action == 'remove': self.debug("remove: '%s' from %s, current value %s", update_value, attr, entry_values) @@ -601,7 +624,28 @@ class LDAPUpdate: except ValueError: raise BadSyntax, "bad syntax in replace, needs to be in the format old::new in %s" % update_value try: - entry_values.remove(old) + if objectclass_replacement: + try: + objectclass_old = ObjectClass(str(old)) + except Exception, e: + self.error('replace: cannot parse replaced ObjectClass "%s": %s', + old, e) + continue + replaced_values = [] + for objectclass in oid_index.get(objectclass_old.oid, []): + objectclass_object = ObjectClass(str(objectclass)) + if str(objectclass_old).lower() == str(objectclass_object).lower(): + # compare normalized values + replaced_values.append(objectclass) + self.debug('replace: replace ObjectClass "%s" with "%s"', + old, new) + if not replaced_values: + self.debug('replace: no match for replaced ObjectClass "%s"', old) + continue + for value in replaced_values: + entry_values.remove(value) + else: + entry_values.remove(old) entry_values.append(new) self.debug('replace: updated value %s', entry_values) entry.setValues(attr, entry_values) @@ -728,7 +772,11 @@ class LDAPUpdate: updated = False changes = self.conn.generateModList(entry.origDataDict(), entry.toDict()) if (entry.dn == DN(('cn', 'schema'))): - updated = self.is_schema_updated(entry.toDict()) + d = dict() + e = entry.toDict() + for k,v in e.items(): + d[k] = [str(x) for x in v] + updated = self.is_schema_updated(d) else: if len(changes) >= 1: updated = True -- 1.7.11.4
_______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
