[Freeipa-devel] [PATCHES] 0322-0327 New permissions system
Martin Kosek
mkosek at redhat.com
Fri Dec 13 09:49:32 UTC 2013
On 12/12/2013 05:17 PM, Petr Viktorin wrote:
> On 12/12/2013 02:00 PM, Martin Kosek wrote:
>> 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
>
> Thanks, changed.
>
>> 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
>
> OK. This means that SYSTEM permissions stay without the new objectclass, which
> means removing most options from permission_add_noaci.
>
>> 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.
>
> Optional ipapermallowedattr will be required for managed permissions. Also,
> add/delete permissions could be specified without an attr filter.
>
>> 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.
>
> Unfortunately it can't be required because it can be specified by a different
> name via the old API. But, it is now checked.
>
>> 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
>> ----------------------------
>
> 4-6: Thanks for the catches, fixed & added to tests.
>
>> 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.
>
> Right. Here's a ticket for it: https://fedorahosted.org/freeipa/ticket/4079
>
>> 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
>>
>
>
Looks better, this is hopefully the last issue:
1) There is some problem with DNS zone permissions:
# ipa dnszone-add-permission example.com
-----------------------------------------------------
Added system permission "Manage DNS zone example.com"
-----------------------------------------------------
# ipa permission-show 'Manage DNS zone example.com' --all --raw
ipa: ERROR: The ACI for permission Manage DNS zone example.com was not found in
dc=idm,dc=example,dc=com
# ipa privilege-add-permission foo --permissions foo
Privilege name: foo
Description: foo
Failed members:
permission: foo: missing attribute "ipaPermLocation" required by object
class "ipaPermissionV2"
-----------------------------
Number of permissions added 0
-----------------------------
Martin
More information about the Freeipa-devel
mailing list