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

[Petr Viktorin]

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.

> I am thinking if we need to add a special objectClass for this new attribute,
> it could be added as MAY to ipaPermission, along with ipaPermDefaultAttrs,
> ipaPermAllowedAttrs, ipaPermExcludedAttrs. When the DN is not there, it would
> simply default to SUFFIX DN.

Sounds right. I'll think about the schema more in the context of 
designing a backwards compatibility story.

-- 
Petr³