[Freeipa-devel] [PATCHES] 0322-0327 New permissions system
Petr Viktorin
pviktori at redhat.com
Fri Dec 13 12:35:11 UTC 2013
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.
--
Petr³
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0435.3-Make-sure-SYSTEM-permissions-can-be-retreived-with-a.patch
Type: text/x-patch
Size: 3121 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131213/b9da3802/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0436.3-Test-adding-noaci-system-permissions-to-privileges.patch
Type: text/x-patch
Size: 2774 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131213/b9da3802/attachment-0001.bin>
More information about the Freeipa-devel
mailing list