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

[Simo Sorce]

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:
> Martin Kosek wrote:
> > On 09/05/2013 07:48 PM, Rob Crittenden wrote:
> >> Petr Viktorin wrote:
> >>> Hello,
> >>> I have some notes and questions on
> >>> https://fedorahosted.org/freeipa/ticket/3566 (Control access of user
> >>> roles to server functions).
> >>>
> >>>
> >>> An IPA terminology refresher for reference:
> >>> - ACI: The DS-level permission.
> >>> - Permission: IPA object that encapsulates one ACI. Example: "add user".
> >>> Permissions aren't as flexible as raw ACIs.
> >>> - Privilege: IPA object that groups several permissions, e.g. with a
> >>> "manage users" privilege you can "add user", "modify user", ...
> >>> - Role: IPA object that groups privileges, e.g. an "User Administrator"
> >>> can manage users and groups. Roles are assigned to users/groups/hosts.
> >>>
> >>>
> >>> # Permission structure
> >>>
> >>> I think it would be best to have two permissions for each object, one
> >>> for the entries and one for the container. This keeps the ACIs
> >>> manageable with existing permission API:
> >>>
> >>> aci: (target = "ldap:///cn=*,cn=groups,cn=accounts,$SUFFIX")(version
> >>> 3.0;acl "permission:Read Groups";allow (read,search,compare) groupdn =
> >>> "ldap:///cn=Read Groups,cn=permissions,cn=pbac,$SUFFIX";)
> >>>
> >>> aci: (target = "ldap:///cn=groups,cn=accounts,$SUFFIX")(version 3.0;acl
> >>> "permission:Read Group Container";allow (read,search,compare) groupdn =
> >>> "ldap:///cn=Read Group Container,cn=permissions,cn=pbac,$SUFFIX";)
> >>>
> >>> These would be combined in a "Group Readers" privilege.
> >>>
> >>> All the privileges would be granted to a role called "Users", which
> >>> would contain ipausers and admins.
> >>
> >> 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.

> > aci: (targetfilter =
> > "(&(!(objectClass=ipaToken))(!(objectClass=ipatokenTOTP))(!(objectClass=ipatokenRadiusConfiguration)))")(target
> > != "ldap:///idnsname=*,cn=dns,$SUFFIX")(targetattr != "userPassword ||
> > krbPrincipalKey || sambaLMPassword || sambaNTPassword || passwordHistory ||
> > krbMKey || userPKCS12 || ipaNTHash || ipaNTTrustAuthOutgoing ||
> > ipaNTTrustAuthIncoming")(version 3.0; acl "Enable Anonymous access"; allow
> > (read, search, compare) userdn = "ldap:///anyone";)
> >
> > ... as this ACI does not scale and adds burden for developers or plugin author
> > wishing to add an attribute that should not be visible by default. Number of
> > ACIs should still be kept low, that's true.
> 
> Right, I just want to know why it was proposed to add 2 ACIs for every 
> container.

I am puzzled as well.

> >>
> >>> # External users & system accounts
> >>>
> >>> I'm not sure how to handle external users here, since they're not added
> >>> to any group. Either we'll need a special ACI for them, or somehow make
> >>> it possible to add non-group sets of users to Roles.
> >>>
> >>> The same goes for system accounts, except those aren't even implemented
> >>> in IPA yet (https://fedorahosted.org/freeipa/ticket/2801).
> >>
> >> I think they would have to be part of a group. Otherwise 389-ds has nothing to
> >> evaluate against (and even with groups I'm not 100% sure it'll work).
> >>
> >>>
> >>> # Protected attributes
> >>>
> >>> How to handle passwords and other non-public attributes? I'm thinking
> >>> about keeping a global list of such attributes, and applying it to each
> >>> read permission ACI on normal operations and upgrades; either generating
> >>> a (targetfilter != ...) clause or filtering the (targetfilter = ...) one.
> >>> Possibly that list would be configurable and stored in LDAP.
> >>>
> >>> For reference, we currently exclude these in the anonymous read rule:
> >>> "userPassword || krbPrincipalKey || sambaLMPassword || sambaNTPassword
> >>> || passwordHistory || krbMKey || userPKCS12 || ipaNTHash ||
> >>> ipaNTTrustAuthOutgoing || ipaNTTrustAuthIncoming"

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.

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

> >>
> >>>
> >>> # Compat tree
> >>>
> >>> Do we want to reuse the read privileges for the compat tree, or create
> >>> extra ones?
> >>
> >> I don't think so.

Why not ?

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


> >>> # P.S.
> >>>
> >>> I believe that we should strive to put our info about default
> >>> permissions, containers, settings, and the schema for each plugin in the
> >>> actual plugin module, rather than it all being split across several
> >>> ldif/update files. This would make this data more manageable,
> >>> introspectable and consistent, expose dependencies between plugins, and
> >>> make it possible to actually write self-contained plugins.
> >>> This should be done when the time comes for a new version of the
> >>> ldap-updater.


I agree with the above.


> >> I don't think we really have any problems having a more or less monolithic set
> >> up now. I think this would just add complexity.


It also adds enormous flexibility.


> > I actually think this could be a killer feature for the refactored ACI system.
> > Let's take a host object as an example:
> >
> > class host(LDAPObject):
> >      """
> >      Host object.
> >      """
> >      ...
> >      takes_params = (
> >          Str('fqdn', _hostname_validator,
> >          Str('description?',
> >          Str('l?',
> >          Str('nshostlocation?',
> >          Str('nshardwareplatform?',
> >          Str('nsosversion?',
> >          Str('userpassword?',
> >          Flag('random?',
> >          Str('randompassword?',
> >          Bytes('usercertificate?', validate_certificate,
> >          Str('krbprincipalname?',
> >          Str('macaddress*',
> >          Str('ipasshpubkey*', validate_sshpubkey_no_options,
> >          Str('userclass*',
> >      ) + ticket_flags_params
> >     ...
> >
> > It has the following standard default ACIs:
> >
> > aci: (target = "ldap:///fqdn=*,cn=computers,cn=accounts,$SUFFIX")(version
> > 3.0;acl "permission:Add Hosts";allow (add) groupdn = "ldap:///cn=Add
> > Hosts,cn=permissions,cn=pbac,$SUFFIX";)
> >
> > aci: (target = "ldap:///fqdn=*,cn=computers,cn=accounts,$SUFFIX")(version
> > 3.0;acl "permission:Remove Hosts";allow (delete) groupdn = "ldap:///cn=Remove
> > Hosts,cn=permissions,cn=pbac,$SUFFIX";)
> >
> > aci: (targetattr = "description || l || nshostlocation || nshardwareplatform ||
> > nsosversion")(target =
> > "ldap:///fqdn=*,cn=computers,cn=accounts,$SUFFIX")(version 3.0;acl
> > "permission:Modify Hosts";allow (write) groupdn = "ldap:///cn=Modify
> > Hosts,cn=permissions,cn=pbac,$SUFFIX";)
> >
> > Out of those:
> > - Add hosts is standard and can be generated automatically from host container
> > and host primary key.
> > - Remove Hosts is also standard and can be generated
> > - Modify Hosts is a bit tricky as we have a list of host attributes that can be
> > updated by default. Right now, this list is not maintained as it lies somewhere
> > in delegation.ldif or some of our *.update file.
> > - New Read Hosts could be also generated, we would just need to filter out
> > attributes we do not want to be read (this should be recorded in the Param
> > object itself, see even more below).


If we put this information in simple attributes lists in LDAP (see
above) and then generate out the defaults I think we'll get in a much
better situation.


> > This leads to outdated permissions not allowing privileged users to modify some
> > parts of the host entry. Just to demonstrate, I found these 2 missing rights
> > when testing a regular user with "Host Administrators" privilege:
> >
> > # kinit fbar
> > # ipa host-add foo.example.com --force
> > # ipa host-mod foo.example.com --macaddress=00:11:22:33:44:55
> > ipa: ERROR: Insufficient access: Insufficient 'write' privilege to the
> > 'macAddress' attribute of entry
> > 'fqdn=foo.example.com,cn=computers,cn=accounts,dc=idm,dc=lab,dc=bos,dc=redhat,dc=com'.
> > # ipa host-mod foo.example.com --class foo
> > ipa: ERROR: Insufficient access: Insufficient 'write' privilege to the
> > 'userClass' attribute of entry
> > 'fqdn=foo.example.com,cn=computers,cn=accounts,dc=idm,dc=lab,dc=bos,dc=redhat,dc=com'.
> >
> > If the basic set of host ACI would be part of the host LDAPObject, we would
> > have everything in one place and avoid issues like that.


And we may fail to set the right ACIs on different replicas of slightly
different versions. I agree with the idea, but the attribute lists
should be in the LDAP tree not in the framework.

As another example, if they are in LDAP you can remove an older plugin
but the data related to it will not have its ACIs disappear, because
even if we remove the related objects from the framework, we still have
the default lists in LDAP (thin of upgrade situations too).

>  Default write
> > privilege could be generated out of the takes_params. We would just need to
> > filter out parameters that we do not want to be writable in default ACI, for
> > example ipasshpubkey where we have a separate permission.
> >
> > We could mark this in the param:
> >
> >          ...
> >          Str('ipasshpubkey*', validate_sshpubkey_no_options,
> >              ...
> >              default_aci=False
> >          ),
> >          ...
> >
> > and define a custom ACIs handling it and other special ACIs. Something like:
> >
> > class host(LDAPObject):
> >      """
> >      Host object.
> >      """
> >      ...
> >      extra_aci = [
> >         ACI(
> >              dn=api.env.container_computers,
> >              name="Hosts can modify their own SSH public keys",
> >              attrs=['ipasshpubkey'],
> >              rights=['write'],
> >              bind='userdn = "ldap:///self"'
> >         ),
> >         ACI(
> >              dn=api.env.container_computers,
> >              name="Hosts can modify their own certs and keytabs",
> >              attrs=['usercertificate', 'krblastpwdchange', 'description', 'l',
> > 'nshostlocation', 'nshardwareplatform', 'nsosversion'],
> >              rights=['write'],
> >              bind='userdn = "ldap:///self"'
> >         ),
> >         ACI(
> >              dn=api.env.container_computers,
> >              name="Hosts can manage other host Certificates and kerberos keys",
> >              attrs=['userCertificate', 'krbPrincipalKey'],
> >              rights=['write'],
> >              bind='userdn = "userattr = "parent[0,1].managedby#USERDN""'
> >         ),
> >         ...
> >      ]
> >
> > IMHO, this would have several benefits:
> > 1) It would place Host entry, params and ACIs to one single location -> easier
> > maintenance

very brittle across versions.

> > 2) It would make delegation.ldif and default-aci.ldif and *.update files
> > shorter given that all these default ACIs would be removed

harder to inspect, you have to go chase objects all around the code
base, nack

> > 3) It would make lives easier for plugin authors which would not have to deploy
> > LDIFs or *.update files to deploy a new plugin or modify functionality of an
> > existing one.

Again even harder to inspect and if the admin 'disables' a plugin, now
suddenly there is data in the LDAP server (that was added by the plugin)
but no way for the system to know what are the default ACIs to apply.

> > Makes sense?


As a general plan yes, but I am deadset against having the defaults in
framework code. They need to stay where the data is, and the actual ACIs
are, in LDAP.


> Well, my alarm bells are going off. This would require some significant 
> review each and every time any object is updated. Consider how many 
> times API.txt is overlooked, and it has no security implications.

I agree this is an issue, we can get much more confidence (than we have
even now) we a testing framework. Automated tests beats any dreadful
review, for some things. I am not saying review shouldn't be very
careful of course.

> I can't say I'm a fan of programmatic ACI generation.

Here I strongly disagree, we certainly need to be very confident of the
generation code, but staring at an ACI that spans 6+ lines on the screen
is extremely hard. It is *much* simpler to look at a list of attributes
in an ldif/update file and see which ones are marked for read or write
or add or delete or search, etc...

If we are confident those list translate to ACI correctly it is a huge
review win.


Simo.

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