[Freeipa-devel] [PATCHES] 0289-0302 Managed Read permissions

[Martin Kosek]

On 11/04/2013 04:48 PM, Petr Viktorin wrote:
> On 10/21/2013 03:57 PM, Martin Kosek wrote:
>> On 10/18/2013 04:28 PM, Petr Viktorin wrote:
> [...]
>>>
>>> Alright, I'm crafting an updated design page with the above in mind. Here are
>>> the main differences.
>>>
>>>
>>> New permissions won't (necessarily) be in $SUFFIX, so old IPA servers will not
>>> be able to modify them.
>>> Extra attribute types needed in addition to ipaPerm*Attributes would be:
>>>    - ipaPermBindType (anonymous/any authenticated user/normal permission)
>>>    - ipaPermDN (container DN where the ACI is stored)
>>>
>>> And objectclasses to group them:
>>>
>>> 'ipaPermissionV2' SUP ipaPermission AUXILIARY MUST ( ipaPermBindType $
>>> ipaPermDN )
>>> 'ipaManagedPermission' SUP ipaPermissionV2 AUXILIARY MAY ( ipaPermDefaultAttrs
>>> $ ipaPermAllowedAttrs $ ipaPermExcludedAttrs )
>>>
>>> As for 'ipaPermissionV2', all non-SYSTEM permissions should be updated to it.
>>> Maybe a better name is needed.
>>>
>>>
>>> Another idea I had is to store all variable parts of the ACI in the permission
>>> entry. This would mean we'd not need to parse ACIs to read, search, or update
>>> them, which should make these operations faster and the code could be
>>> simplified.
>>> Doing this would require these new attribute types:
>>>    - ipaPermRight (add, update, read, delete, etc.)
>>>    - ipaPermObjectType
>>>    - ipaPermMemberof
>>>    - ipaPermFilter
>>>    - ipaPermSubtree
>>>    - ipaPermTargetgroup
>>>
>>> Would that make sense?
> 
> The more I think about this, the more I want to go this way after all.
> 
>> It partially makes sense - it would speed up permission-find commands. However,
>> it would also duplicate information and sets it in 2 places. Which smells like
>> a bucket of potential bugs to me.
> 
> True. However, this has to be weighed against the status quo.
> The current code is complicated. Converting ACIs to dicts and back, calling IPA
> commands from within IPA commands, incorrect error handling, entry_attrs vs.
> options in callbacks -- all these details come together to make the code very
> hard to change, or even verify it works as it should. I fear that a bucket of
> real bugs is already hiding in the code, and that incremental changes are bound
> to create more.
> 
>> What if somebody changes ipaPermObjectType, but ACI update fails or is
>> interrupted for some reason? It would create inconsistency between permission
>> entry and the ACI itself. Which should prevail?
> 
> Obviously the DS would only take the ACI into consideration.
> Conceptually the permission would prevail, the ACI would be rewritten the next
> time the permission is updated.
> This is an error state, comparable to e.g. an UPG not being created for a user,
> or the memberOf plugin failing to update membership info.
> (In an ideal world the ACI updates would be done in a DS plugin that can
> leverage transactions.)
> 
> The existing implementation has this problem with renames - if a permission is
> renamed by the ACI is for some reason not updated, an old ACI will stay behind,
> and it will be pretty hard to find.
> (We should have an audit tool that checks out-of-sync ACIs -- it would be
> helpful even if the status quo stays.)
> 
>> Unless permission-find performance is not a problem (yet?), I would not add
>> these new attributes and only add ipaPermDN as this information is required.
> 
> Performance is only part of the problem. Code simplicity is another -- simple
> code usually has less bugs, and is easier to work on/review, etc.

Makes sense to me, please continue investigaing this way then. The audit tool
may indeed be very useful, if admin want's to check if ACIs did not get out of
sync.

Martin