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

Fri Sep 6 11:48:43 UTC 2013

On 09/06/2013 09:26 AM, 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).
[...]
>>>
>>>
>>> # 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.

Yes, but with a feature request for fine-grained read permissions, I 
don't see how we can get away without increasing the number of ACIs.
If we want the ACIs to be manageable, we need two ACIs per object type - 
one for the object itself, one for the container.
Alternatively we could combine the two with some clever filter tricks 
and build a smart UI on top to manage them. I don't want to go this route.
We might be able to get away with one global system ACI to cover all 
containers, so each object type would only have one read ACI, but that's 
also quite ugly.

A potential way to get performance back is to move ACIs from $SUFFIX to 
the actual container they apply to.
What is the reason we put our ACIs on $SUFFIX? Is it so that we can 
easily find/list them later?

> One of the main reason for this feature is to get rid of the global allowing ACI:
>
> 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.
>
>>
>>> # 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"
>>
>> 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.

There are two considerations in play:
- We want users to be able to add or remove attributes to the ACIs.
- We need to be able to update the ACIs on upgrade (and on 
user-initiated config changes, e.g. ipauserobjectclasses), so that 
attributes from new added object classes are added

The hard part: merging the user changes back in after an upgrade.
The only solutions to this that I found require us tracking more data 
than we are tracking now: either attribute lists from past IPA versions, 
or records of the user's actions.
I plan to go with the latter.

> 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.
>
>>> # Compat tree
>>>
>>> Do we want to reuse the read privileges for the compat tree, or create
>>> extra ones?
>>
>> I don't think so.

Can you disambiguate? :)

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

True. But, FreeIPA is in the business of simplifying administration; in 
a way we're shielding the user from the complexities of LDAP.
Our permission objects are, by design, dumbed down so that they're 
easier to manage.
To rephrase: Would an admin want to allow, *to a user*, detecting the 
presence of an attribute but not reading? If it's not for users, it 
would IMO be better handled by plugins, raw ACIs, and System permissions.

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

You can have all the rights in a single ACI; this is merely about 
simplifying administration.

>>> # 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 don't think we really have any problems having a more or less monolithic set
>> up now.  I think this would just add complexity.

I beg to differ.
Our ldif and update files are growing out of sync.
Some data (and even schema!) is now only in updates. (And some is in 
plugins, of course.)

You can see our permission names are wildly inconsistent in things like 
capitalization and use of plurals. It's trivial but it doesn't look very 
professional. And as Martin points out below, some functional mistakes 
have crept in too.

If we stick to ldif+update files, with this feature I will add lots of 
data generated automatically by a script. In two slightly altered 
copies. To files that will be hand-edited later, with largely 
incomprehensible 1000+ character lines routinely added for regular 
changes. Whenever we (or anyone!) adds a new object to IPA, that data 
will be copied by hand, with the proper names and DNs substituted.

I don't see how we are not having any problems here.

I know it must look different to you -- and I don't blame you! But from 
the outside point of view, the ldif+update system is really not ideal.

> 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).
>
> 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. 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""'
>         ),
>         ...
>      ]

Originally I thought we should generally aim for this in the long term, 
but discussion with the Brno team has led me to the conclusion that I 
want this done, for the ACIs, for 3.4.

> IMHO, this would have several benefits:
> 1) It would place Host entry, params and ACIs to one single location -> easier
> maintenance
> 2) It would make delegation.ldif and default-aci.ldif and *.update files
> shorter given that all these default ACIs would be removed
> 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.

For 3) you'd also need schema, containers, configuration, etc. in the 
plugins. But ACIs are a good first step.

-- 
Petr³