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

[Simo Sorce]

On Mon, 2013-09-09 at 13:00 +0200, Petr Viktorin wrote:
> 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.

I guess it depends on how you define 'smart', but we agree it shouldn't
be smarter than it needs to be.

> [...]
> >> 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.

The template is just an example, we can negotiate on it :)

> 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.

We solved this problem in the past for LDAP updates, I agree it is a
problem we need to address, and I agree the way we have done in the past
with .update files may not be up to task for the ACI system, so let's
talk.

> Second, we already have a place where users can customize ACIs: the 
> permissions. I don't see the point of another layer.

Well, you want a 'blueprint' for the permissions, where you set the
system defaults, I called it template.

> 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,
^^^ this is equivalent to what I called 'template', only you make it
read-only for admins

>  attributes added by the admin, and attributes removed by the admin. 
^^^ and this sounds like a read-write, second order version of a
template.

I am not entirely sure I like the complexity of layered templates, but I
do understand the proposal, and will think a little bit more about it, I
am not opposed in principle to your scheme.

> 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.

How do you handle a case where we add 'read-only by admin' for an
attribute that was not in the default ACI list at all previously, but
the admin already added 'read by everyone' in the custom ACI ?
This is the kind of thing that makes me dislike the 2 separate sets, now
you need intra-set conflict resolution.

> 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.

I do not understand the nuances of this SYSTEM permission, can you
explain why we want it, and why it should't "locate or touch its ACI"
whatever it really means ?

> 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.)

Sounds a bit fragile, I think you never want to remove system defaults,
because you will always want to be able to go back to a known state if
you mess up. Maybe we can mark something as DISABLED and preserve
things.

> >>>>> # 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.

I do not have a strong opinion here (yet).

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York