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.
Martin
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
