Bug 43628 - UCS@school: replication cycle for nTSecurityDescriptor
UCS@school: replication cycle for nTSecurityDescriptor
Product: UCS
Classification: Unclassified
Component: S4 Connector
UCS 4.1
Other Linux
: P5 normal (vote)
: UCS 4.2-1-errata
Assigned To: Arvid Requate
Stefan Gohmann
Depends on: 41571
Blocks: 35336 45087 45088
  Show dependency treegraph
Reported: 2017-02-23 17:50 CET by Arvid Requate
Modified: 2017-09-05 20:07 CEST (History)
5 users (show)

See Also:
What kind of report is it?: Bug Report
What type of bug is this?: 5: Major Usability: Impairs usability in key scenarios
Who will be affected by this bug?: 3: Will affect average number of installed domains
How will those affected feel about the bug?: 3: A User would likely not purchase the product
User Pain: 0.257
Enterprise Customer affected?:
School Customer affected?: Yes
ISV affected?:
Ticket number: 2017021721000386
Bug group (optional):
Max CVSS v3 score:
best: Patch_Available+

pre_con_modify_check.patch (5.57 KB, patch)
2017-02-23 17:50 CET, Arvid Requate
Details | Diff
post_read_control.patch (20.46 KB, patch)
2017-07-10 17:06 CEST, Arvid Requate
Details | Diff
post_read_control.patch (24.04 KB, patch)
2017-07-10 19:39 CEST, Arvid Requate
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Arvid Requate univentionstaff 2017-02-23 17:50:04 CET
Created attachment 8455 [details]

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.
Comment 1 Arvid Requate univentionstaff 2017-07-10 17:06:55 CEST
Created attachment 9006 [details]

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.
Comment 2 Arvid Requate univentionstaff 2017-07-10 17:40:26 CEST
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.
Comment 3 Arvid Requate univentionstaff 2017-07-10 19:39:48 CEST
Created attachment 9008 [details]

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.
Comment 4 Arvid Requate univentionstaff 2017-07-19 19:32:45 CEST
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.

* univention-directory-manager-modules.yaml                          
* staging/univention-python.yaml                                             
* staging/univention-s4-connector.yaml
Comment 5 Stefan Gohmann univentionstaff 2017-07-20 07:41:59 CEST
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:

Comment 6 Florian Best univentionstaff 2017-07-20 09:31:17 CEST
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
Comment 7 Florian Best univentionstaff 2017-07-20 10:09:12 CEST
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().
Comment 8 Florian Best univentionstaff 2017-07-21 13:52:08 CEST
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.
Comment 9 Florian Best univentionstaff 2017-07-21 14:03:06 CEST
univention-directory-manager-modules (12.0.18-3):
r81305 | Bug #43628: fix create() in modules inheriting from simplePolicy
Comment 10 Arvid Requate univentionstaff 2017-07-24 12:03:11 CEST
> 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.
Comment 11 Stefan Gohmann univentionstaff 2017-07-25 16:17:58 CEST
* Code review: OK
* Tests: OK (ucs-test)

* Code review: OK
* Tests: OK (ucs-test)

* Code review: OK
* Tests: OK (ucs-test)
* Manual Tests: OK