[Freeipa-devel] [PATCHES] 0455-0459 Add support for managed permissions
Martin Kosek
mkosek at redhat.com
Wed Feb 12 15:57:51 UTC 2014
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
More information about the Freeipa-devel
mailing list