[Freeipa-devel] [PATCHES] 0455-0459 Add support for managed permissions
Petr Viktorin
pviktori at redhat.com
Wed Feb 12 16:13:08 UTC 2014
On 02/12/2014 04:57 PM, Martin Kosek wrote:
> On 02/10/2014 04:53 PM, Petr Viktorin wrote:
>> On 01/31/2014 01:43 PM, Martin Kosek wrote:
>>> On 01/24/2014 04:48 PM, Petr Viktorin wrote:
>>>> On 01/23/2014 02:42 PM, Simo Sorce wrote:
>>>>> On Thu, 2014-01-23 at 13:23 +0100, Petr Viktorin wrote:
>>>>>> On 01/23/2014 12:24 PM, Martin Kosek wrote:
>>>>>>> On 01/22/2014 10:27 AM, Petr Viktorin wrote:
>>>>>>>> On 01/08/2014 04:49 PM, Petr Viktorin wrote:
>>>>>>>>> Hello,
>>>>>>>>> This adds "managed" permissions, the framework that will make our
>>>>>>>>> default permissions merge IPA updates and user changes sanely.
>>>>>>>>>
>>>>>>>>> There is no updater yet, nor does this add any actual managed
>>>>>>>>> permissions, so there's no user-visible change (beyond help text and a
>>>>>>>>> disabled option). To test the patch you might need to touch LDAP directly.
>>>>>>>>>
>>>>>>>>> Ticket: https://fedorahosted.org/freeipa/ticket/4033
>>>>>>>>> Design (no updater & plugin changes yet):
>>>>>>>>> http://www.freeipa.org/page/V3/Managed_Read_permissions
>>>>>>>>>
>>>>>>>>> 0447 - Minor fixes.
>>>>>>>>> 0448 - Since you can't create managed permissions through the API, I
>>>>>>>>> needed to get creative with the declarative tests. The tests will need a
>>>>>>>>> custom function that adds a managed perm.
>>>>>>>>> 0449 - The change itself.
>>>>>>>>
>>>>>>>> ping; any thoughts on this one?
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> 1) 449, the comment:
>>>>>>>
>>>>>>> +Deleting or renaming a managed permission, as well as changing its target,
>>>>>>> +is not supported.
>>>>>>> +""") + _("""
>>>>>>>
>>>>>>> I am not sure that the phrase "not supported" is the right one. It sounds
>>>>>>> to me
>>>>>>> like this is something we want to allow, just not implemented yet. IMO
>>>>>>> "is not
>>>>>>> allowed" would be better.
>>>>>>
>>>>>> Makes sense.
>>>>>>
>>>>>>> 2) Can you add allow_mod_for_managed flag description to parameters.py?
>>>>>>>
>>>>>>> + flags={'no_create', 'allow_mod_for_managed'},
>>>>>>>
>>>>>>> So far we try to add all flag descriptions there.
>>>>>>
>>>>>> OK
>>>>>>
>>>>>>> 3) When I updated the test to not delete the testperm, I tried to show the
>>>>>>> managed permission and it is not entirely clear, see:
>>>>>>>
>>>>>>> # ipa permission-show testperm
>>>>>>> Permission name: testperm
>>>>>>> Permissions: write
>>>>>>> * Attributes: cn, o, sn
>>>>>>> * Excluded attributes: cn, sn
>>>>>>> Bind rule type: all
>>>>>>> Subtree: cn=users,cn=accounts,dc=example,dc=com
>>>>>>> ACI target DN: uid=*,cn=users,cn=accounts,dc=example,dc=com
>>>>>>> Type: user
>>>>>>> * Default attributes: l, o, cn
>>>>>>> * Effective attributes: l, o
>>>>>>
>>>>>> Well, this is a tradeoff between presenting what's stored in LDAP and
>>>>>> what's in the ACI.
>>>>>>
>>>>>>> The "Attributes" mean actually attributes explicitly allowed by user, but
>>>>>>> it is
>>>>>>> not obvious from the output.
>>>>>>>
>>>>>>> Maybe it would be better to return only "Effective attributes" by default
>>>>>>> and
>>>>>>> return the 3 source lists only when --all is passed. But this would
>>>>>>> require us
>>>>>>> to let Command override LDAPObject's default_attributes, which framework
>>>>>>> cannot do.
>>>>>>
>>>>>> Modifying default_attributes would not work because the 3 lists need to
>>>>>> be loaded from LDAP to determine the effective attributes.
>>>>>> It's possible to remove the extra attributes in the post_callback,
>>>>>> postprocess_result already does similar output manipulation.
>>>>>>
>>>>>>> Alternatively, we may choose to use the attributes differently with managed
>>>>>>> permissions:
>>>>>>> - we add the new attributeType "ipaPermIncludedAttr". It would be used
>>>>>>> for the
>>>>>>> user-specified whitelist of attributes instead of ipaPermAllowedAttr
>>>>>>> - we do not use the ipaPermAllowedAttr with managed attributes at all or
>>>>>>> use it
>>>>>>> for the "Effective attributes" list
>>>>>>>
>>>>>>> My point is that the semantics of ipaPermAllowedAttr is different for
>>>>>>> managed
>>>>>>> and non-managed permission, so it may confuse people.
>>>>>>
>>>>>> Well, the semantics are always the same (effective = (default | allowed)
>>>>>> - excluded). I agree that it can be confusing; perhaps I'm in too deep
>>>>>> to judge how it looks from the outside.
>>>>>>
>>>>>>> For example, you may want
>>>>>>> to search for all permissions that allow attribute "sn":
>>>>>>>
>>>>>>> # ipa permission-find --attrs sn
>>>>>>> ---------------------
>>>>>>> 4 permissions matched
>>>>>>> ---------------------
>>>>>>> Permission name: anon
>>>>>>> Permissions: read
>>>>>>> Attributes: sn
>>>>>>> Bind rule type: anonymous
>>>>>>> Subtree: cn=users,cn=accounts,dc=example,dc=com
>>>>>>> ACI target DN: uid=*,cn=users,cn=accounts,dc=example,dc=com
>>>>>>> Type: user
>>>>>>> ...
>>>>>>> Permission name: testperm
>>>>>>> Permissions: write
>>>>>>> Attributes: cn, o, sn
>>>>>>> Excluded attributes: cn, sn
>>>>>>> Bind rule type: anonymous
>>>>>>> Subtree: cn=users,cn=accounts,dc=example,dc=com
>>>>>>> ACI target DN: uid=*,cn=users,cn=accounts,dc=example,dc=com
>>>>>>> Type: user
>>>>>>> Default attributes: l, o, cn
>>>>>>> Effective attributes: l, o
>>>>>>> ...
>>>>>>>
>>>>>>> As you see, it matched both testperm and anon even though testperm does not
>>>>>>> really allow sn as it excluded.
>>>>>>>
>>>>>>> Thoughts?
>>>>>>
>>>>>> Well, we could have default, included, excluded attributes stored in
>>>>>> LDAP as now (using the name "included" instead of "allowed"), and make
>>>>>> effective attributes (--attrs) into an updatable virtual attribute: when
>>>>>> setting it, IPA would consult the "default" attributes and update
>>>>>> "included"/"excluded" accordingly. (With non-managed permissions
>>>>>> "default" is empty, so only "included" would be updated.) And searching
>>>>>> on --attrs would construct an appropriate filter.
>>>>>>
>>>>>> I thought about this approach earlier but thought that it obscured
>>>>>> what's actually stored in LDAP. Given recent discussions I'm now
>>>>>> thinking I shouldn't have rejected it.
>>>>>
>>>>>
>>>>> I would take a consistent approach indeed.
>>>>> I do not much care for the virtual attribute part in LDAP, as long as
>>>>> our tool show prominently the effective list.
>>>>> And also as long as all permissions behave the same way in the general
>>>>> mechanism in LDAP.
>>>>>
>>>>> Simo.
>>>>>
>>>>
>>>> All right. Here are patches; I'll start updating the design page.
>>>>
>>>> **NOTE**
>>>> This renames the 'ipaPermAllowedAttr' attribute to 'ipaPermIncludedAttr'.
>>>> Upgrades from master will fail, so please install a new server. Of course no
>>>> released versions of FreeIPA are affected.
>>>> I assume there's no clean way to rename an attribute without changing the OID?
>>>> Is it OK to break master this way?
>>>>
>>>> As before three source lists are stored in LDAP:
>>>> - ipaPermDefaultAttr
>>>> - ipaPermIncludedAttr (--includedattrs)
>>>> - ipaPermExcludedAttr (--excludedattrs)
>>>>
>>>> Setting --attrs ("Effective Attributes") will set the included/excluded
>>>> attributes based on the default set.
>>>> For normal permissions, default & excluded will be empty, and in this case only
>>>> effective attributes will be displayed on output (unless --all or --raw is
>>>> given).
>>>>
>>>>
>>>>
>>>> I've added some preparatory patches for #4074 to this batch, mostly to prevent
>>>> rebase conflicts with that work. Re-numbering for a sane order.
>>>>
>>>> Apply on top of my patch 0452
>>>>
>>>> 0455 - minor fixes, unchanged from 0447
>>>>
>>>> 0456 - converting options on the server it will allow us to consult the entry's
>>>> existing state. Arguably it's also cleaner to use execute than
>>>> args_options_2_params for this.
>>>>
>>>> 0457 - generate ACIs in the plugin. This is the next step in obsoleting the ACI
>>>> class, which in the end should only be necessary for updating old-style ACIs.
>>>> The one-way function should be easier to maintain and extend.
>>>>
>>>> 0458 - allow callables in declarative tests, unchanged from 0448
>>>>
>>>> 0459 - managed permissions
>>>>
>>>>
>>>> Or: git pull https://github.com/encukou/freeipa 3566-managed-perms
>>>> commit 51bb7f27516202a062ffa25fedae18d0e9f302b6
>>>>
>>>
>>> 1) (cosmetic) Wouldn't it better to move ipapermdefaultattr to takes_params
>>> with ['no_create', 'no_update'] flags to:
>>>
>>> a) Have better ordering of output params. Now it looks like:
>>>
>>> # ipa permission-show testperm
>>> Permission name: testperm
>>> Permissions: write
>>> Effective attributes: cn, l, o, sn
>>> Included attributes: sn
>>> Bind rule type: all
>>> Subtree: cn=users,cn=accounts,dc=idm,dc=lab,dc=eng,dc=brq,dc=redhat,dc=com
>>> ACI target DN:
>>> uid=*,cn=users,cn=accounts,dc=idm,dc=lab,dc=eng,dc=brq,dc=redhat,dc=com
>>> Type: user
>>> Default attributes: l, o, cn
>>>
>>>
>>> I think it would be better if all the "* attributes" parameters are together.:
>>>
>>> Effective attributes: bla, bla
>>> Included attributes: bla, bla
>>> Excluded attributes: bla, bla
>>> Default attributes: bla, bla
>>>
>>> so that it is easier to read the output.
>>>
>>> b) If ipapermdefaultattr is in takes_params, one would be able to do
>>> permission-search for default attributes. Even if we don't allow user to change
>>> them, we should allow him to search them.
>>>
>>> 2) (also cosmetic) Given we have ipapermincludedattr described as
>>> doc=_('User-specified attributes to which the permission applies'),
>>> shouldn't we also describe ipapermexcludedattr as
>>> doc=_('User-specified attributes to which the permission explicitly '
>>> 'does not apply'),
>>> ? I think it would be more consistent.
>>>
>>>
>>> Besides these 2 cosmetic issues, I think the new patchset works pretty nice -
>>> good job!
>>
>> Thanks for the review, fixed.
>> I've also fixed the search filter for --attrs, and added more tests for that.
>>
>
> Looks good to me:
>
> # ipa permission-show testperm
> Permission name: testperm
> Permissions: write
> Effective attributes: cn, o, sn
> Included attributes: sn
> Excluded attributes: l
> Default attributes: l, o, cn
> Bind rule type: all
> Subtree: cn=users,cn=accounts,dc=example,dc=com
> ACI target DN: uid=*,cn=users,cn=accounts,dc=example,dc=com
> Type: user
>
> ACK to all 5 patches (the last patch had 2-fuzz chunk).
>
> Martin
>
Thank you! Pushed to master: 3db08227e8c760c688b8886e0b3b072e9b6dd94d
--
Petr³
More information about the Freeipa-devel
mailing list