[Freeipa-devel] [PATCHES] 0540-0542 Add managed read permissions to user

[Martin Kosek]

On 05/26/2014 12:04 PM, Petr Viktorin wrote:
> On 05/25/2014 09:29 PM, Martin Kosek wrote:
>> On 05/23/2014 04:50 PM, Simo Sorce wrote:
>>> On Fri, 2014-05-23 at 10:59 +0200, Martin Kosek wrote:
>>>> On 05/22/2014 04:20 PM, Petr Viktorin wrote:
>>>>> On 05/21/2014 12:14 PM, Simo Sorce wrote:
>>>>>> On Wed, 2014-05-21 at 08:03 +0200, Martin Kosek wrote:
>>>>>>> On 05/16/2014 04:33 PM, Petr Viktorin wrote:
>>>>>>>> On 05/16/2014 01:54 PM, Martin Kosek wrote:
>>>>>>>>> On 04/29/2014 11:00 PM, Petr Viktorin wrote:
>>>>>>>>>> Patch 0540 adds a bunch of managed read ACIs for user, as
>>>>>>>>>> discussed
>>>>>>>>>> previously
>>>>>>>>>> [0].
>>>>>>>>>>
>>>>>>>>>> Patch 0541 is some minor refactoring for the next part.
>>>>>>>>>>
>>>>>>>>>> Patch 0542 sets the read acces to addressbook attributes to
>>>>>>>>>> anonymous when
>>>>>>>>>> upgrading from pre-4.0.
>>>>>>>>>> I first this by checking if the update is run from
>>>>>>>>>> ipa-server-install or
>>>>>>>>>> not,
>>>>>>>>>> but then I realized the logic I want is simple: if the global
>>>>>>>>>> anon read ACI
>>>>>>>>>> exists, we want to preserve its spirit by setting addressbook
>>>>>>>>>> attribute
>>>>>>>>>> ACI to
>>>>>>>>>> anonymous.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> [0]
>>>>>>>>>> http://www.redhat.com/archives/freeipa-devel/2014-April/msg00363.html
>>>>>>>>>> et
>>>>>>>>>> al.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 540:
>>>>>>>>>
>>>>>>>>> Looks good! The only attributes I am concerned about are special
>>>>>>>>> IPA
>>>>>>>>> attributes:
>>>>>>>>>
>>>>>>>>> - ipauniqueid
>>>>>>>>> - ipasshpubkey
>>>>>>>>> - ipauserauthtype
>>>>>>>>> - userclass
>>>>>>>>>
>>>>>>>>> I personally do not think they should be included in POSIX
>>>>>>>>> attributes
>>>>>>>>> permissions, they are far from POSIX definition...
>>>>>>>>>
>>>>>>>>> What about creating one more permission "System: Read User IPA
>>>>>>>>> Attributes" as
>>>>>>>>> these are specific to FreeIPA use and allowing that permission
>>>>>>>>> for all
>>>>>>>>> authenticated users?
>>>>>>>>
>>>>>>>> Sounds reasonable. I assume we want this one to be also set to
>>>>>>>> anonymous when
>>>>>>>> upgrading from old versions.
>>>>>>>> Attaching updated patches.
>>>>>>>
>>>>>>> Ok, looks good.
>>>>>>>
>>>>>>> I am now just pondering whether "System: Read User POSIX
>>>>>>> Attributes" is the
>>>>>>> right name for the permission as there are not just POSIX
>>>>>>> attributes, but also
>>>>>>> attributes from organizationalPerson or inetOrgPerson objectclasses.
>>>>>>>
>>>>>>> Maybe we should name it "System: Read User Core Attributes" or
>>>>>>> "System: Read
>>>>>>> User Basic Attributes"? Simo, any preference?
>>>>>>
>>>>>> We could use: "System: Read User Standard Attributes"
>>>>>
>>>>> I've used this one, then.
>>>>>
>>>>>>
>>>>>> but the 'posix' version is also ok to me.
>>>>>
>>>>> On Wed, 2014-05-21 at 08:03 +0200, Martin Kosek wrote:
>>>>>> Also, I just realized we forgot memberOf attribute - it needs to be
>>>>>> available
>>>>>> to authenticated users otherwise group membership will fall apart.
>>>>>
>>>>> Good catch. Added.
>>>>>
>>>>
>>>> We are very close to push this one - I have just one last concern about
>>>> userpkcs12 attribute. On upgrade, we previously hidden userpkcs12
>>>> from user,
>>>> now we added it to be read by default. This results in this warning
>>>> during upgrade:
>>>>
>>>> Excluded attributes for System: Read User Addressbook Attributes:
>>>> userpkcs12
>>>>
>>>> Simo (or others), is this OK or do we want to keep hiding userpkcs12
>>>> by default?
>>>
>>> Is there any client that needs access to that information that we are
>>> aware of ?
>>>
>>> Simo.
>>
>> I do not think so. Rob, do you know?
> 
> This was my mistake. We never allowed non-admins to see that attribute by
> default, so we shouldn't start now.

ack, we probably had a good reason and it is much safer to keep this decision.

> I'm glad the updater caught it, sorry that I didn't.

Actually, that means that you made the security checks in the updater right :-)

I diffed the change in the patch and it removed the last obstacle I saw with
this patch set. Thus, ACK for all 3.

Martin