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

Fri Sep 6 12:46:53 UTC 2013

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

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

>>
>>>
>>> # 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 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""'
>         ),
>         ...
>      ]
>
> 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.
>
> Makes sense?

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 can't say I'm a fan of programmatic ACI generation.

rob