[Freeipa-devel] Notes and questions for fine-grained read permissions

[Petr Viktorin]

On 09/07/2013 04:45 PM, Simo Sorce wrote:
> Sorry to come late to this thread.
>
> I think I like some of Petr plan, but not all of it.
>
> On Fri, 2013-09-06 at 08:46 -0400, Rob Crittenden wrote:
[...]
>>>> I'm not sure I follow, what are you trying to achieve here? The more ACIs the
>>>> slower the processing.
>>>
>>> One of the main reason for this feature is to get rid of the global allowing ACI:
>
> And this is good, one-size-fit-all, doesn't work anymore, however we
> need to try to keep the number of ACIs low anyway, this can be done my
> smartly and programmatically generate ACIs, however we need a very
> complete test suite and in order to have a complete one, we need enough
> information to know what is the intended behavior. Plus I think for
> robustness the test suite should be generated from different code, so
> that bugs in the ACI generation code does not also cause blind spots in
> the ACI testing code.

I don't want to build a "smart" system; it should rather be simple and 
straightforward. Other than that I agree, good tests are always necessary.


[...]
>> Right, I just want to know why it was proposed to add 2 ACIs for every
>> container.
>
> I am puzzled as well.

Consider that point dropped; I trusted the bug description without checking.


>>>>> How to handle passwords and other non-public attributes? I'm thinking
>>>>> about keeping a global list [...]
>
> No I will veto anything that overhauls ACIs and keeps negative filtering
> lists.
> Any big change to the ACIs system *MUST* get rid of ( targetattr !=
> list ).
>
> We already risked many times of exposing data and a few years ago had to
> issue a CVE because of this. We do not want to keep doing it. Especially
> if you are building this system to allow plugins to change permissions
> you do not want to rely on the authors to remember to add negative
> entries.

+1, a negative list doesn't make sense any more.


>>>> It could get ugly real fast, and potentially cause a lot of extra processing. I
>>>> think the object(s) for each attribute should be considered so these wouldn't
>>>> have to be applied to every ACI but only those that are affected. We don't need
>>>> to worry about userPassword in groups, for example.
>>>
>>> I think that a decision that a param should not be included in default read
>>> rule should be included in the param object itself, see below.
>
> I really do not want to see the ACI templates/definition/call it how you
> want int the framework, because the framework will need to change with
> versions of IPA but the data may remain in LDAP for long. therefore any
> ACI template should be stored in LDAP, probably under cn=etc,$suffix.
>
> It may make sense to have cn=aci-templates,cn=etc,$suffix, and then
> within that container (accessible only by security admins) we have one
> template per object/container, that is used to generate ACIs.
>
> Something like:
>
> dn: cn=users,cn=aci-templates,cn=etc,$suffix
> objectclass: ipaAciTemplate
> anonReadAttributes: <multivalue list>
> authReadAttributes: <multivalue list>
> selfReadAttributes: <multivalue list>
> selfWriteAttributes: <multivalue list>
>
> and so on.
> These are the defaults, only attributes that must be disclosed are
> listed, *no* negative lists, the default is changed globaly to deny all,
> and if there is no allow ACI an attribute is inaccessible by default.
>
> This allows easy customization, if someone decides 'description' should
> be kept confidential, all he needs to do is to remove it from the right
> list (and perhaps add it to a privileged list), and just run a tool that
> regenerates and changes the default ACIs for the container.
> * No need to modify framework code anywhere. *

+1 to storing the attribute lists in LDAP. I don't like the Template 
idea though.

First, on IPA upgrades, we need to be able to add attributes to the 
ACIs. Automatically.
We cannot make the IPA-generated lists user-modifiable. If some past 
upgrade added a readable 'description' attribute, but the admin decides 
that it should not be readable, the next upgrade should not re-add it.
So we need to store the IPA defaults and user changes separately.

Second, we already have a place where users can customize ACIs: the 
permissions. I don't see the point of another layer.
The only thing we're adding here over regular permissions is the 
aforementioned ability to auto-add new attributes on upgrades.
We just need a special "managed" permission for each object's "anon 
read", "auth read", "self read", "self write", etc.


I propose a new Permission type with three lists: attributes added by 
IPA, attributes added by the admin, and attributes removed by the admin. 
On IPA updates, new items can be added to the first list, and the ACI 
gets regenerated from all three. This ensures that an admin's changes 
get preserved across updates.

If we mark a Permission as SYSTEM, old IPA versions won't try to locate 
or touch its ACI, but they'll still be able to add it to privileges and 
roles using the existing API/CLI/UI.

So we'd have something like:

cn=Read Users,cn=permissions,cn=pbac,$SUFFIX
objectclass: ipaPermission
objectclass: ipaManagedPermission
ipaDefaultAttributes: <multivalue list>
ipaAdminAddedAttributes: <multivalue list>
ipaAdminExcludedAttributes: <multivalue list>
ipaPermissionType: SYSTEM

Admins that don't want IPA updates messing with their ACIs can just 
remove the permission from privileges and add custom plain permissions 
instead.
(Or possibly even convert the default permission to non-managed, or 
delete it -- if we write UI to do+undo these actions.)



>>>>> # Combining read rights
>>>>>
>>>>> I think (read, search, compare) should be exposed in permission objects
>>>>> as a single right. Or is there a reason to keep it split?
>>>>
>>>> Yes, they are separate for a reason. Using only search and compare isn't
>>>> common, but it isn't unheard of either. For example, to be able to detect the
>>>> presence of an attribute you can provide just the search permission.
>>>
>>> Wouldn't most users use the (read, search, compare) triple? It would lower our
>>> number of ACIs significantly if we do not have 3 permissions per each object.
>>
>> There is nothing to prevent an ACI from containing multiple permissions.
>> I wasn't proposing that. But rolling these three logically into the same
>> thing in IPA I think is short-sighted.
>
> I think it would make sense for an optimizer ACI generator to come later
> at a second stage, where we go through the generated list and combine
> ACIs together.
>
> Ie the first version could generate three distinct ACIs for read, search
> and compare, and later on we add an optimizer that consolidates them
> into a single ACI.

For the default permissions I'd rather generate ACIs with all three. 
Admins (or IPA if needed) can add an additional permission if some 
attribute only needs search.


-- 
Petr³