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 _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel