[Freeipa-devel] Ticket #2866 - referential integrity in IPA

[Nathan Kinder]

On 09/06/2012 09:28 AM, Martin Kosek wrote:
> On 09/04/2012 04:40 PM, Rich Megginson wrote:
>> On 09/03/2012 08:42 AM, Martin Kosek wrote:
>>> On 08/27/2012 06:29 PM, Rich Megginson wrote:
>>>> On 08/27/2012 10:24 AM, Martin Kosek wrote:
>>>>> On 08/17/2012 04:00 PM, Rich Megginson wrote:
>>>>>> On 08/17/2012 07:44 AM, Martin Kosek wrote:
>>>>>>> Hi guys,
>>>>>>>
>>>>>>> I am now investigating ticket #2866:
>>>>>>> https://fedorahosted.org/freeipa/ticket/2866
>>>>>>>
>>>>>>> And I am thinking about possible solutions for this problem. In a
>>>>>>> nutshell, we do not properly check referential integrity in some IPA
>>>>>>> objects where we keep one-way DN references to other objects, e.g. in
>>>>>>> - managedBy attribute for a host object
>>>>>>> - memberhost attribute for HBAC rule object
>>>>>>> - memberuser attribute for user object
>>>>>>> - memberallowcmd or memberdenycmd for SUDO command object (reported in
>>>>>>> #2866)
>>>>>>> ...
>>>>>>>
>>>>>>> Currently, I see 2 approaches to solve this:
>>>>>>> 1) Add relevant checks to our ipalib plugins where problematic
>>>>>>> operations with these operations are being executed (like we do for
>>>>>>> selinuxusermap's seealso attribute in HBAC plugin)
>>>>>>> This of course would not prevent direct LDAP deletes.
>>>>>>>
>>>>>>> 2) Implement a preop DS plugin that would hook to MODRDN and DELETE
>>>>>>> callbacks and check that this object's DN is not referenced in other
>>>>>>> objects. And if it does, it would reject such modification. Second
>>>>>>> option would be to delete the attribute value with now invalid
>>>>>>> reference. This would be probably  more suitable for example for
>>>>>>> references to user objects.
>>>>>>>
>>>>>>> Any comments to these possible approaches are welcome.
>>>>>>>
>>>>>>> Rich, do you think that as an alternative to these 2 approaches,
>>>>>>> memberOf plugin could be eventually modified to do this task?
>>>>>> This is very similar to the referential integrity plugin already in 389,
>>>>>> except
>>>>>> instead of cleaning up references to moved and deleted entries, you want
>>>>>> it to
>>>>>> prevent moving or deleting an entry if that entry is referenced by the
>>>>>> managedby/memberhost/memberuser/memberallowcmd/memberdenycmd of some other
>>>>>> entry.
>>>>> I think that using or enhancing current DS referential integrity plugin
>>>>> will be
>>>>> the best and the most consistent way to go.
>>>>>
>>>>> We already use that plugin for some user attributes like "manager" or
>>>>> "secretary". seeAlso is already covered by default, so for example seeAlso
>>>>> attribute in SELinux usermap object referencing an HBAC rule will get removed
>>>>> when relevant HBAC rule is removed (I just checked that).
>>>>>
>>>>>> Note that the managed entry plugin (mep) already handles this for the
>>>>>> managedby
>>>>>> attribute.
>>>>> I assume you are referencing "mepmanagedby" and "mepmanagedentry" attributes
>>>>> which then produce errors like this one:
>>>>>
>>>>> # ipa netgroup-del foo
>>>>> ipa: ERROR: Server is unwilling to perform: Deleting a managed entry is not
>>>>> allowed. It needs to be manually unlinked first.
>>>>>
>>>>> managedBy attribute used by hosts objects I had in mind seems to not be
>>>>> covered.
>>>>>
>>>>> But you are right, this is pretty much what I wanted. Though in case of MEP,
>>>>> there is a link in both referenced objects, but in our case, we have just the
>>>>> one-way link.
>>>>>
>>>>>> Are you already using the memberof plugin for
>>>>>> memberhost/memberuser/memberallowcmd/memberdenycmd?
>>>>>>
>>>>>> This doesn't seem like a job for memberof, this seems like more of a new
>>>>>> check
>>>>>> for the referential integrity plugin.
>>>>>>
>>>>> I am now considering if current move/delete clean up already present in
>>>>> Referential Integrity plugin would not be sufficient for us.
>>>>>
>>>>> Rich, please correct me if I am wrong, but in that case, we would just need to
>>>>> add relevant attribute names
>>>>> (memberhost/memberuser/memberallowcmd/memberdenycmd...) to Referential
>>>>> Integrity plugin configuration as nsslapd-pluginarg7, nsslapd-pluginarg8, ...
>>>>> I wonder if there would be some performance issues if we add attributes to the
>>>>> list this way.
>>>> No, not if they are indexed for presence and equality.
>>>>
>>> Hello Rich,
>>> I am back to investigate this ticket. In order to be able to deliver some
>>> working solution to IPA 3.0, I plan to take advantage of current Referential
>>> Integrity Plugin to clean up dangling references.
>>>
>>> This is the plan I plan to take:
>>> 1) Add pres,eq indexes for all un-indexed attributes that we want to check:
>>> sourcehost
>>> memberservice
>>> managedby
>>> memberallowcmd
>>> memberdenycmd
>>> ipasudorunas
>>> ipasudorunasgroup
>> ok
>>> 2) Add missing "pres" index for attributes we want to check, but only have eq
>>> index:
>>> manager
>>> secretary
>>> memberuser
>>> memberhost
>>>
>>> I assume this step is also needed in order to keep the server performance.
>> yes
>>> 3) Add all these attributes do Referential Integrity Plugin attribute list of
>>> not already
>> ok
>>> 4) Also add Index task (nsIndexAttribute) for all these new indexes so that
>>> they are created during IPA server upgrade.
>> ok
>>> Is this procedure OK DS-wise?
>> Yes
>>> I also have question regarding the following note in RHDS doc chapter 3.6.
>>> Maintaining Referential Integrity:
>>>
>>> "The Referential Integrity Plug-in should only be enabled on one supplier
>>> replica in a multi-master replication environment to avoid conflict resolution
>>> loops..."
>>>
>>> Currently, we enable this plugin on all IPA replicas. Is this something we need
>>> to be concerned about and fix ASAP (before we do all this RefInt effort)?
>> Mark/Nathan - I know you guys have looked at this issue.
> Hello guys,
>
> any update on that? Just checking before I implement this extended referential
> check in IPA.
The last I looked at this was a few years ago.  From what I remember, I 
wasn't ever convinced that the documentation was correct, but I couldn't 
prove that it was incorrect either.  I will dig some more to see if I 
can find any of my notes from that investigation.

-NGK
>
> Thanks,
> Martin