[Freeipa-devel] [PATCHES] 0322-0327 New permissions system
Martin Kosek
mkosek at redhat.com
Fri Dec 13 14:13:28 UTC 2013
On 12/13/2013 01:35 PM, Petr Viktorin wrote:
> On 12/13/2013 10:49 AM, Martin Kosek wrote:
>> 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
>
> Thanks for the catch. Fixed in an additional patch.
>
>> # 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
>> -----------------------------
>
> Could not reproduce. This one used a permission created by the previous set of
> patches, where ipaPermLocation was not set when it was $SUFFIX. This is
> incompatible with the new schema. From the last update ipaPermLocation is in
> MUST, and is always added.
> I did write some additional tests before I asked Martin to explain this, so I'm
> also including these.
>
> Apply these on top of the patches sent earlier.
>
Works nice. That wraps up this part of your permission system refactoring, ACK
for that!
Pushed all 10 patches to master.
Thanks for great work! Let's continue with next ACI adventures :)
Martin
More information about the Freeipa-devel
mailing list