[Freeipa-devel] [PATCHES] 0289-0302 Managed Read permissions

Wed Oct 2 11:26:00 UTC 2013

On 10/02/2013 01:07 PM, Simo Sorce wrote:
>
>
> ----- Original Message -----
>> On 10/01/2013 10:56 AM, Petr Viktorin wrote:
>>> Hello,
>>>
>>> These patches implement the framework for
>>> https://fedorahosted.org/freeipa/ticket/3566
>>>
>>> Design is at http://www.freeipa.org/page/V3/Managed_Read_permissions.
>>> As you can see from the TODOs it's not yet complete; I'll need a few more
>>> discussions about some details and future work.
>>>
>>> The patches only add read permissions for User objects, and the global
>>> anonymous read ACI is not removed. I'll add the other objects after the
>>> overall
>>> structure is ACKed.
>>> To test, remove the ACI (cheatsheet: http://fpaste.org/43296/13806142/) and
>>> verify that anonymous read is disabled and normal users can't read anything
>>> but
>>> user info.
>>>
>>>
>>> These depend on some of my earlier patches:
>>> - 0258-0265, 0275 - LDIF-based schema updater
>>> - 0276-0277 - Split large doc strings for translation
>>> - 0288 - user template in tests

These are the dependencies ^

>>> I needed to test both server and client plugins. Since we only have one API
>>> object (#3090) and can't unload plugins, I needed to fix some issues when
>>> they
>>> are loaded at the same time.
>>>
>>> Terminology note: currently IPA calls the "read"/"search"/"write"/"delete"
>>> part
>>> of an ACI a "permission", which is confusing since our ACI wrapper objects
>>> are
>>> also "permissions".
>>> Wherever I can, I use the term "rights" for these.
>>> "Rights" is also used in ACI docs:
>>> https://access.redhat.com/site/documentation/en-US/Red_Hat_Directory_Server/8.2/html/Administration_Guide/Managing_Access_Control-Creating_ACIs_Manually.html#id3349243
>>>
>>>
>>>
>>> /Enter patches.
>>>
>>> Act I.
>>>
>>> 0289: Might as well update to new API since I'll be making extensive
>>> changes here
>>>
>>> 0290: My linting tools were complaining heavily about the tabs, so I fixed
>>> the
>>> indentation here.
>>>
>>> 0291: Fix a crash when ldap2 and a client RPC backend are connected at the
>>> same
>>> time. (This happens in tests that I'll add later)
>>>
>>>
>>> Act II.
>>>
>>> 0292: See the "Permission flags" section of the design.
>>>
>>> 0293: Add schema. (The OIDs aren't registered yet.)
>>>
>>> 0294: This makes the test in the next patch possible.
>>>
>>> 0295: See the "MANAGED Permissions" section of the design.
>>>
>>> 0296: See the "Read rights" section of the design.
>>>
>>>
>>> Act III.
>>>
>>> 0297: See "Marking Attributes in Plugins" and "Adding permissions for
>>> default
>>> read permissions"  in the design.
>>>
>>> 0298: Make the help plugin not fail when server plugins are loaded. This
>>> will
>>> happen in later tests.
>>>
>>> 0299: Tests for 0297
>>>
>>> 0300! Fix a TODO from 0295
>>>
>>> 0301: See "Adding privileges and role for default read permissions" in the
>>> design
>>>
>>> 0302: Tests for 0301
>>>
>>
>> Thanks for all the patches and carrying on with this major effort! I swiftly
>> went through the patches and have few comments:
>>
>> 1) Patch 293 requires your schema updater patches, right? That means patches
>> 258-265, 275
>>
>> There may be more dependencies though:
>>
>> ...
>> Applying: Add Reader role and user read privilege
>> Applying: Add tests for the new Reader role
>> error: patch failed: ipatests/test_xmlrpc/test_user_plugin.py:106
>> error: ipatests/test_xmlrpc/test_user_plugin.py: patch does not apply
>> Patch failed at 0014 Add tests for the new Reader role
>>

See the list of deps earlier in the mail :)

>>
>> 2) 297: in update_managed_permissions, handling of
>> ipapermissionallowedattribute and ipapermissionexcludedattribute should be
>> handled by
>>
>> +        try:
>> +            api.Command.permission_mod(name)
>> +        except errors.EmptyModlist:
>> +            self.log.debug('Attributes unchanged')
>>
>> right?

Yes. ipapermdefaultattrs is added in ldap.update_entry(entry), and then 
from that and allowed+excluded, attributes is recomputed in permission_mod.
This way the algorithm is kept in one place.

>> 3) 301: I did not like the Reader role & User Readers privilege very much. I
>> see several issues related to that:
>>
>> a) performance reasons - read operation on LDAP must be as fast as possible.
>> DS
>> checking membership in ipausers where may be 100000 of users does not look
>> right
>>
>> b) ipausers group may be changed to other group by setting
>> ipaDefaultPrimaryGroup. I also think we are guaranteed that it really
>> contains
>> all users. There have been also thoughts about removing it.

Having ipausers in Readers is just a default, the admin can change it to 
match their setting. But your point is still valid.

>> To sum it up,  I would rather not build our permission system on this group.
>>
>> I think we need top base our ACIs on LDAP bind targets ldap:///all and
>> ldap:///anyone to avoid performance issues and issues with ipausers not being
>> complete:
>>
>> https://access.redhat.com/site/documentation/en-US/Red_Hat_Directory_Server/8.2/html/Administration_Guide/Managing_Access_Control-Bind_Rules.html
>>
>> I rather think we want to base the permissions on binddn. In the beginning,
>> there would be 3 types of permissions based on binddn:
>>
>> * groupdn based: standard permissions that can be assigned privileges
>> * ldap:///all permissions for all authenticated users. Cannot be assigned to
>> privileges (would not make sense)
>> * ldap:///anyone permissions for all, including anonymous users. Cannot be
>> assigned to privileges (would not make sense)
>>
>> Just few examples:
>> Read users - ldap:///anyone
>> Read groups - ldap:///anyone
>> Read sudo - ldap:///all
>> Read hbac - ldap:///all
>> ...
>>
>> Basing the permissions on these would allow us to get rid of all the awful
>> deny permissions.
>>
>> To make the permission Bind part more user friendly, there should be an
>> option
>> in permission-find and a switch in Web UI to e.g. list permissions by bind
>> type, i.e.:
>> - anonymous permissions
>> - authenticated users permissions
>> - group based permissions
>>
>> If use would want to e.g restrict sudo only to specified group, I would see
>> this workflow:
>> 1) Change bind type from "authenticated users" to "group based"
>> 2) Bind permission to chosen privilege
>> 3) ...
>>
>> And the opposite, if he wants to make reading more open:
>> 1) Unbind permission from privilege
>> 2) Change bind type to "authenticated users" or "anonymous"
>>
>> Makes sense?

Yes.

> I agree with Martin's comments too.
>
> Simo.

We use privileges to group related permissions. For example an 
"Automount Reader" privilege would contain "read automount keys" and 
"read automount maps" permissions.
Wouldn't it make more sense (from the user's point of view at least) to 
have this setting at the privilege level?

The selfservice plugin does pretty much the same thing. Should we (aim 
to) move selfservice functionality to this system?

And, since these permissions are longer be compatible with old versions, 
I could move them out of $SUFFIX and onto the relevant containers. That 
should also help performance.

-- 
Petr³