[Freeipa-devel] [PATCHES] 0322-0327 New permissions system

Thu Dec 12 13:00:03 UTC 2013

On 12/06/2013 11:49 AM, Petr Viktorin wrote:
> On 12/02/2013 01:04 PM, Martin Kosek wrote:
>> On 12/01/2013 11:46 PM, Petr Viktorin wrote:
>>> This seems to work now. Please tell me what I missed.
>>>
>>> Design: http://www.freeipa.org/page/V3/Permissions_V2
>>> Ticket: https://fedorahosted.org/freeipa/ticket/4034
>>>
>>>
>>> 0322 Allow sets for initialization of frozenset-typed Param keywords
>>> because my OCD compels me to use sets instead of lists when the order does not
>>> matter.
>>>
>>>
>>> 0323 Allow Declarative test classes to specify the API version
>>> For the next patch, I want to test how the rewrite handles old clients. To make
>>> that easy I made the default API version a testclass attribute
>>>
>>>
>>> 0324 Add tests for permission plugin with older clients
>>> These tests will not pass yet, but comparing this file with the old
>>> test_permission_plugin.py will can serve as a nice summary of API changes. A
>>> summary of the summary:
>>> - Lots of new attributes will be added for output
>>> - The `type` and `subtree` options now interact in a different way: setting one
>>> affects the other. Same with `type`/`filter` and `memberof`/`targetfilter`.
>>> (Some change here was necessary for
>>> https://fedorahosted.org/freeipa/ticket/2355)
>>> - Validation will be stricter (and/or done in different order)
>>> - Some error messages will change (hopefully for the better)
>>> - `subtree` must now point to an existing entry
>>> - Permission names may now contain '.' (this is to allow names of DNS
>>> permissions that were previously internal)
>>>
>>> P.S. a handy command for listing the changes (once this patch is applied):
>>> git diff ipa-3-3:ipatests/test_xmlrpc/test_permission_plugin.py
>>> ipatests/test_xmlrpc/test_old_permission_plugin.py
>>>
>>>
>>> 0325 Add new permission schema
>>> Introducing the new OIDs
>>>
>>>
>>> 0326 Rewrite the Permission plugin
>>> See the design for what this does.
>>>
>>> The new permission plugin does not use aci plugin at all. The plan is to retire
>>> the aci plugin when the time comes to also refactor delegation & selfservice.
>>> It does use ipalib's ACI class, mainly for parsing (needed for
>>> upgrading/showing old ACIs).
>>>
>>> The permission-find command is a bit faster than the old one, but still
>>> painfully slow (5s instead of 7s on my box). The good news is that it now
>>> scales with the number of *old* permissions, so as you upgrade it'll get
>>> faster.
>>>
>>> Tests are updated, including privilege and DNS tests that worked with
>>> permissions.
>>>
>>>
>>> 0327 Verify ACIs are added correctly in tests
>>> Right after saying I want to get rid of it, I found a new use for the aci
>>> plugin: an tested code path for getting at ACIs (Declaratrive tests can only
>>> use the API, they don't play well with LDAP connections).
>>> Now we can be sure the ACIs are actually changed when we play with permissions.
>>>
>>
>> Great job!
>>
>> I read through the code and did few tests, this is what I've found so far:
>>
>> 1) When I tried to move ACI to non-existent DN, I got a general error + the
>> underlying ACI was gone:
>>
>> # ipa permission-mod "Write Group Description" --subtree=foo=bar
>> ipa: ERROR: no such entry
>>
>>
>> When I then tried to delete it, I got Internal Error:
>> # ipa permission-del "Write Group Description"
>> ipa: ERROR: an internal error has occurred
>>
>> ...
>> /python2.7/site-packages/ipalib/plugins/permission.py", line 681, in
>> pre_callback
>> [Mon Dec 02 10:49:11.972422 2013] [:error] [pid 14181]
>> self.obj.remove_aci(entry)
>> [Mon Dec 02 10:49:11.972426 2013] [:error] [pid 14181]   File
>> "/usr/lib/python2.7/site-packages/ipalib/plugins/permission.py", line 374, in
>> remove_aci
>> [Mon Dec 02 10:49:11.972430 2013] [:error] [pid 14181]
>> self._replace_aci(permission_entry)
>> [Mon Dec 02 10:49:11.972434 2013] [:error] [pid 14181]   File
>> "/usr/lib/python2.7/site-packages/ipalib/plugins/permission.py", line 392, in
>> _replace_aci
>> [Mon Dec 02 10:49:11.972438 2013] [:error] [pid 14181]     acidn = acientry.dn
>>   # pylint: disable=E1103
>> [Mon Dec 02 10:49:11.972441 2013] [:error] [pid 14181] AttributeError: 'dict'
>> object has no attribute 'dn'
>>
>> I think we should add a check for that + at least try to rollback if we fail to
>> move the ACI.
> 
> Fixed. I did in in 2 additional patches for clarity:
> - 0331 adds rollback
> - 0332 adds the check (you can test the rollback without this applied)
> 
>> 2) In filter check:
>>
>> +        # Rough filter validation by a search
>> +        if self.env.in_server and 'ipapermtargetfilter' in options:
>> +            ldap = self.Backend.ldap2
>> +            try:
>> +                ldap.find_entries(
>> +                    filter=options['ipapermtargetfilter'],
>> +                    base_dn=self.env.basedn,
>> +                    size_limit=0)
>>
>> This is great for starters, but I would make it less costly and only search
>> with BASE scope and sizelimit==1 to avoid costly, possibly unindexed searches
>> across whole database.
> 
> Agreed, fixed.
> 
>> 3) I see that I can add ALL bind type permission as a member to a privilege:
>>
>> # ipa permission-show "Write Group Description 2"
>>    Permission name: Write Group Description 2
>>    Permissions: write
>>    Attributes: description
>>    Bind rule type: all
>>    Subtree: cn=groups,cn=accounts,dc=example,dc=com
>>    ACI target DN: cn=*,cn=groups,cn=accounts,dc=example,dc=com
>>    Type: group
>>
>> # ipa privilege-add-permission foo --permissions "Write Group Description 2"
>>    Privilege name: foo
>>    Description: foo
>>    Permissions: write group description 2
>> -----------------------------
>> Number of permissions added 1
>> -----------------------------
>>
>> Is this a bug or do you already plan to fix it later?
> 
> Yes, I plan to fix that soon (#4032).
> I've modified the patch to allow "permission" only. I'll re-introduce the
> others when I add the necessary checks.
> 
>> 4) Typo in refactored permission plugin:
>>
>> +            errors.NotFound('ACI of to permission %s was not found' %
>> +                            keys[0])
> 
> Fixed, thanks for the catch!
> 

1) 0352.2: I think that ipaPermTargetFilter and ipaPermTarget should be
SINGLE-VALUE'd as well

2) More about the schema, I think we should move the attributes that permission
plugin always depends on to MUST list, I think this should affect:
* ipapermright
* ipapermbindruletype

I was pondering about ipapermallowedattr, but ACI works without it well, it is
just NOOP. Other attributes are just different combinations of targetting the
entries to protect, so they may stay MAY.

3)326.2: shouldn't

+        StrEnum(
+            'ipapermright*',
+            cli_name='permissions',

rather read 'ipapermright+'? This is what I get if I omit the permissions:

# ipa permission-add foo --attrs=description --type group
ipa: ERROR: an internal error has occurred

Same with update and other attributes:
# ipa permission-mod foo3 --permissions ''
ipa: ERROR: an internal error has occurred

# ipa permission-mod foo3 --memberof=''
ipa: ERROR: an internal error has occurred

The later one is only reproducible if there is not memberof part set.

4) Internall error when entering blank subtree
# ipa permission-add foo3 --permissions write --subtree ''
ipa: ERROR: an internal error has occurred

5) Internal error when entering non-blank subtree and nothing else:

# ipa permission-add foo3 --permissions write --subtree 'cn=otp,dc=example,dc=com'
ipa: ERROR: an internal error has occurred

[Thu Dec 12 13:18:04.635874 2013] [:error] [pid 17259]     raise SyntaxError,
"target must be a non-empty dictionary"

It seems this case should translate into error "there should be at least one
target entry specifier".

6) permission-find gives 0 results when searching in the default DN and it was
not explicitly set:

# ipa permission-find  --subtree dc=example,dc=com
---------------------
0 permissions matched
---------------------
----------------------------
Number of entries returned 0
----------------------------

7) Web UI list of permissions returns weird results (just 2 of my new testing
permissions). This is what it calls:

[Thu Dec 12 13:41:01.473157 2013] [:error] [pid 17258] ipa: INFO:
[jsonserver_session] admin at IDM.LAB.ENG.BRQ.REDHAT.COM: permission_find(u'',
sizelimit=0, pkey_only=True): SUCCESS

But as I see, Web UI is broken in many other aspects, so it needs to be adapted
to the new output. As I do not want to stop development of the server framework
part (there is a lot to do) it can be done in other patches after yours are
merged, so that we have at least the server part in. We just need to fix it
before 3.4 release.

This is all I found in the second round of review, these are mostly corner
cases, the core of the patch set seems to be nice.

Martin