Univention Bugzilla – Bug 43628
UCS@school: replication cycle for nTSecurityDescriptor
Last modified: 2017-09-29 17:43:25 CEST
Created attachment 8455 [details] pre_con_modify_check.patch Bug #41571 introduced added a check to the sync_from_ucs part of the S4-Connector, which skips modifying the nTSecurityDescriptor on Samba/AD if the S4-Object has been modified since the last sync_to_ucs. Unfortunately this check only skips overwriting the nTSecurityDescriptor (via post_con_modify_function) but doesn't skip the modification of other attributes. This can lead to strange effects: A) Not seen in reality yet: some tool changes nTSecurityDescriptor and a another attribute ATTRIBUTE_X to VALUE_A in one operation in Samba/AD. Then it again changes nTSecurityDescriptor and a another attribute ATTRIBUTE_X to VALUE_B in one operation. During the next sync_from_ucs operation the ATTRIBUTE_X probably gets reset to VALUE_A. B) Seen in reality (Ticket #2017021721000386): In an UCS@school scenario one of the school slaves decided to skip modifying the nTSecurityDescriptor during sync_from_ucs (for yet unknown reasons). In the next sync_to_ucs the S4-Connector probably detected the updated uSNChanged on the object and then overwrote the msNTSecurityDescriptor in OpenLDAP (Master) with the old value that was preserved in the Samba/AD of the school slave. The attached patch proposes to move the nTSecurityDescriptor check of Bug #41571 into a pre_con_modify_function, which then decides of the whole modification is skipped. That's still dangerous, but may give a more consistent result.
Created attachment 9006 [details] post_read_control.patch The attached patch proposes a different approach: It uses the LDAP PostReadControl (rfc4527) for add and modify operations to obtain the entryCSN of the LDAP operation in an atomic fashion. By that information the S4-Connector is able to exactly identify the changesets it committed itself, so it can ignore those changes once they come back in from the Listener. The attached patch introduces this technology for the msGPO objects only and avoids changes in the "hot path" of both the uldap.py modules and the UDM handlers. Basically, I implemented two methods udm_object.create2 and .modify2 that can be used to update the oldattr dictionary with the "entryCSN" reported back by the OpenLDAP server. For msGPO objects the S4-Connector stores the entryCSN values. Later, when the msGPO object modification is reported by the Listener, the S4-Connector ignores that change, logs a PROCESS level message and forgets about the entryCSN value. The patch also reverts the previous workaround for Bug #41571.
The fundamental difference between the approaches of Comment 1 and Comment 0 is that the latter ignores a changeset on the source side (OpenLDAP) because that change is irrelevant. On the other hand the former (Comment 0) approach ignores a change set because the target changed, regardless of the attributes changed on the source object.
Created attachment 9008 [details] post_read_control.patch The attached patch also adds the required versioned package dependencies. Let's wait until the pending bunch of errata updates of univention-directory-manager-modules is released before committing any of this.
I've implemented the PostReadControl solution. I've designed the change in such a way that I just: 1. added support for the passing of LDAP serverctrls to the uldap layer(s) (currently only for add and modify) 2. pass the PostReadControl from S4-Connector for add and modify if it's a msGPO object and record the entryCSN in an sqlite table 3. if PostReadControl has been used, then evaluate the result in simpleLDAP to update oldattr accordingly [optional, might be useful in the future] 4. skip a change in sync_from_ucs if the entryCSN is found in the new sqlite table output a log message on loglevel INFO Worked in manual tests. Advisories: * univention-directory-manager-modules.yaml * staging/univention-python.yaml * staging/univention-s4-connector.yaml
The Jenkins rename tests fail with the following traceback: [2017-07-20 00:35:03.568317] Traceback (most recent call last): [2017-07-20 00:35:03.568358] File "/usr/share/univention-directory-manager-tools/univention-cli-server", line 218, in doit [2017-07-20 00:35:03.568374] output = univention.admincli.admin.doit(arglist) [2017-07-20 00:35:03.568388] File "/usr/lib/pymodules/python2.7/univention/admincli/admin.py", line 398, in doit [2017-07-20 00:35:03.568398] out = _doit(arglist) [2017-07-20 00:35:03.568408] File "/usr/lib/pymodules/python2.7/univention/admincli/admin.py", line 890, in _doit [2017-07-20 00:35:03.568416] dn = object.modify() [2017-07-20 00:35:03.568425] File "/usr/lib/pymodules/python2.7/univention/admin/handlers/__init__.py", line 711, in modify [2017-07-20 00:35:03.568434] res = base.modify(self, modify_childs=1, ignore_license=0, response=response) [2017-07-20 00:35:03.568444] File "/usr/lib/pymodules/python2.7/univention/admin/handlers/__init__.py", line 338, in modify [2017-07-20 00:35:03.568453] return self._modify(modify_childs, ignore_license=ignore_license, response=response) [2017-07-20 00:35:03.568466] File "/usr/lib/pymodules/python2.7/univention/admin/handlers/__init__.py", line 872, in _modify [2017-07-20 00:35:03.568476] self.lo.modify(self.dn, ml, ignore_license=ignore_license, serverctrls=self.serverctrls, response=response) [2017-07-20 00:35:03.568515] File "/usr/lib/pymodules/python2.7/univention/admin/uldap.py", line 494, in modify [2017-07-20 00:35:03.568527] return self.lo.modify(dn, changes, serverctrls=serverctrls, response=response) [2017-07-20 00:35:03.568535] File "/usr/lib/pymodules/python2.7/univention/uldap.py", line 472, in modify [2017-07-20 00:35:03.568544] self.rename_ext_s(dn, new_rdn, None, delold=1, serverctrls=serverctrls, response=response) For example: http://jenkins.knut.univention.de:8080/job/UCS-4.2/job/UCS-4.2-1/job/AutotestJoin/39/SambaVersion=s4,Systemrolle=member/testReport/01_base/96rename_domain_admins/test/
I fixed that typo by removing the arguments (they are already the default). univention-python (10.0.4-3): r81274 | Bug #43628: fix typo: delold=1 and newsuperior=None is already the default
I like the updating of self.oldattr after create() and modify() - this makes the first hunk in the patch at Bug #41711 (attachment 7779 [details]) unnecessary. I would vote for also do self.oldattr = {} at the end of remove().
Support for simplePolicy is missing, therefore this fails for every policy: >>> import univention.admin.uldap >>> lo, po=univention.admin.uldap.getMachineConnection() >>> univention.admin.modules.update() >>> umc = univention.admin.modules.get('policies/umc') >>> o = umc.object(None, lo, po) >>> o.create(response={}, serverctrls=[]) Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: create() got an unexpected keyword argument 'response' I don't see a reason to store the serverctrl in self.serverctrls. Why? To make them modifyable? Then better implement a function like we do for _ldap_modlist, _ldap_ready, _ldap_object_classes. Also I don't see a reason to implement create() in simpleLDAP instead of adapting base. This makes merging "base" and "simpleLDAP" harder. I will fix these issues.
univention-directory-manager-modules (12.0.18-3): r81305 | Bug #43628: fix create() in modules inheriting from simplePolicy
> This makes merging "base" and "simpleLDAP" harder. I didn't know of any plans to that effect and tried to adapt to the currently implemented style.
univention-python: * Code review: OK * Tests: OK (ucs-test) * YAML: OK univention-directory-manager: * Code review: OK * Tests: OK (ucs-test) * YAML: OK univention-s4-connector: * Code review: OK * Tests: OK (ucs-test) * Manual Tests: OK * YAML: OK
<http://errata.software-univention.de/ucs/4.2/114.html> <http://errata.software-univention.de/ucs/4.2/115.html> <http://errata.software-univention.de/ucs/4.2/116.html>