[Freeipa-devel] [PATCHES] 0572-0575 Add ACI.txt + default bind rule type

[Petr Viktorin]

On 06/10/2014 04:28 PM, Martin Kosek wrote:
> On 06/10/2014 03:22 PM, Petr Viktorin wrote:
>> On 06/10/2014 01:30 PM, Martin Kosek wrote:
>>> On 06/10/2014 10:05 AM, Petr Viktorin wrote:
>>>> On 06/09/2014 08:08 PM, Petr Viktorin wrote:
>>>>> Having another verification tool should help reviewing the permission
>>>>> patches.
>>>>>
>>>>>
>>>>> To avoid conflicts, apply on top of my patches 0568-0570 (Write User
>>>>> permissions).
>>>>>
>>>>>
>>>>> 0572: I tried to make the ACIs generated by the permission plugin as
>>>>> predictable as possible, but I missed one place it's affected by
>>>>> dict/set iteration order (which is undefined). Here's a fix.
>>>>>
>>>>> 0573: Minor refactoring to make the next patch easier.
>>>>>
>>>>> 0574: Add ACI.txt & makeaci. Due to the predictable ACIs, all this needs
>>>>> to do is generate the file; comparing can be done bit-by-bit.
>>>>> I do run the validation results through difflib, but frankly it's easier
>>>>> just to use Git.
>>>>
>>>> On my way home yesterday I remembered I left out an important piece of
>>>> information - the DN where the ACI is. Attaching updated patch 0574.
>>>>
>>>>> 0575: Make 'permission' the default bind rule type for managed
>>>>> permissions. Rationale in the commit message.
>>>>> Run makeaci to verify this doesn't change the result.
>>>
>>> This will create some additional burden for you when converting ACIs, but the
>>> idea is good.
>>
>> Any ACI.txt conflicts are easily resolved by running makeaci, and I think the
>> additional verification is worth it.
>
> Yup, I am not complaining :-)
>
>>> The script worked for me, can we just create some more friendly error message
>>> than an assertion traceback?
>>>
>>> # ./makeaci --validate
>>> ...
>>> Traceback (most recent call last):
>>>     File "./makeaci", line 116, in <module>
>>>       main(options)
>>>     File "./makeaci", line 108, in main
>>>       raise AssertionError('validation failed')
>>> AssertionError: validation failed
>>
>> The tool is meant for developers and packagers; tracebacks are designed to be
>> friendly for this target group.
>>
>> But, OK, I've made it exit() instead, and added some instructions.
>
> Ok, thanks.
>
>>  From the thread on user permissions:
>> On 06/10/2014 10:13 AM, Martin Kosek wrote:
>>> 1) Hm, I think was not clear enough. memberOf should not be added to users, as
>>> no object should be "member of user". Please revert this change. I originally
>>> thought it is missing in "Read Group Membership" permissions, but it isn't.
>>>
>>> We are again hitting the problem of a search operation when we are not allowed
>>> to search on all attributes (the CVE fix). This is the search produced by "ipa
>>> user-show fbar":
>>>
>>> [10/Jun/2014:09:59:28 +0200] conn=155 op=5 SRCH base="dc=example,dc=com"
>>> scope=2
>>> filter="(|(member=uid=fbar,cn=users,cn=accounts,dc=example,dc=com)(memberUser=uid=fbar,cn=users,cn=accounts,dc=example,dc=com)(memberHost=uid=fbar,cn=users,cn=accounts,dc=example,dc=com))"
>>>
>>> attrs=""
>>> [10/Jun/2014:09:59:28 +0200] conn=155 op=5 RESULT err=0 tag=101 nentries=0
>>> etime=0
>>>
>>> It returns zero results, until I also allow memberUser and memberHost:
>>>
>>> # ipa permission-mod 'System: Read Group Membership'
>>> --attrs={member,memberof,memberuid,memberuser,memberhost}
>>>
>>> Now I get the results:
>>>
>>> [10/Jun/2014:10:04:25 +0200] conn=160 op=5 SRCH base="dc=example,dc=com"
>>> scope=2
>>> filter="(|(member=uid=fbar,cn=users,cn=accounts,dc=example,dc=com)(memberUser=uid=fbar,cn=users,cn=accounts,dc=example,dc=com)(memberHost=uid=fbar,cn=users,cn=accounts,dc=example,dc=com))"
>>>
>>> attrs=""
>>> [10/Jun/2014:10:04:25 +0200] conn=160 op=5 RESULT err=0 tag=101 nentries=1
>>> etime=0
>>>
>>> ipa user-show fbar
>>> ...
>>>    Member of groups: ipausers    <<<<<
>>>    Indirect Member of role: test
>>> ...
>>>
>>> User still cannot see if he is direct or indirect member of role, but this may
>>> not be a problem.
>>>
>>> The easiest approach solution may be to update all "Read * Membership"
>>> permissions/ACIs to always contain member&memberuser&memberhost unless we want
>>> to again produce multiple LDAP searches for checking direct/indirect membership.
>>
>> I've amended the read permissions for containerish objects, and added a check
>> to makeaci to enforce this in the future.
>>
>> I insist on printing a traceback on error here. This check is meant for
>> developers, tracebacks are perfect for them.
>>
>
> If you insist, I can live with that. As you said, it is for packagers and
> developers after all.
>
> ACK to the whole patch set.

Thanks! Pushed to master: b6258d08d6c5605b32151654c6259f7c77f1a32b



-- 
Petr³