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

[Simo Sorce]

On Mon, 2013-09-09 at 16:40 +0200, Petr Viktorin wrote:
> On 09/09/2013 03:46 PM, Simo Sorce wrote:
> > 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.
> >>>
> [...]
> >>>>>> 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.
> 
> How have we solved it? AFAIK, currently if the LDAP updater sees a state 
> that used to be default in an old IPA version but changed, it will 
> overwrite it with the new state.
> 
> So if the current IPA version adds an attribute that you don't want, 
> you're out of luck: removing it reverts to the old state, which will get 
> updated next time you upgrade.
> 
> Also, if you *add* an attribute, the resulting ACI no longer matches the 
> default so the LDAP updater won't ever touch it again.

As I said, it may not be sufficient :)

> >> 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.
> 
> There would be two permissions: "read by admin" and "read by everyone".
> Both would now allow reading the attribute, so it would be readable by 
> anyone -- as the admin wanted.
> 
> I don't see any need for intra-set conflict resolution. The algorithm is 
> simple -- take IPA defaults, add admin-added attributes, filter out 
> admin-excluded attributes, allow the resulting set. Recompute whenever 
> any of the lists changes.

Actually this make sense, I was mostly thinking in terms of adding deny
ACIs, but the most positive outcome here will happen if we finally
remove the dreadful deny ACI, so I think we are mostly ok.

I can still think of a scenario that may not be optimal.

Admin adds attribute for read by admin only and IPA update adds a
read-by-all permission.

I think this is unlikely to happen, but probably we need to add a rule
that any such change on IPA part must be reviewed by at least 2
experienced members before it is approved, so that more than one head
reviews and vets such a change.

> >> 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 ?
> 
> Permissions are IPA objects that encapsulate the underlying ACIs and 
> make them easier to manage. Permissions are less flexible than raw ACIs.
> Sometimes they need more power than what the Permission UI exposes, so 
> we mark them as SYSTEM. The CLI/UI then doesn't read or update the 
> underlying ACI.
> If we add permissions that work differently than the existing ones, we 
> can mark them SYSTEM so *old IPA* doesn't try to manipulate the 
> underlying ACIs.
> (And we need to add some kind of permision versioning, in case the 
> functionality changes again.)

Ok, now I get it, SYSTEM was your way to name 'low level ACIs we set
directly'.

> >> 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.
> 
> Removing the permission from the privilege is like disabling, you can 
> always add it back.
> We just need to make sure IPA doesn't overwrite the admin's choice on 
> updates.

Right.

And it will be really fun for you to find out how to upgrade from the
current scheme to the final state here (which is critical, no 'start
from scratch' strategies here ok ?) but I think it is quite doable.

Simo.

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