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Ā³

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to