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

[Petr Viktorin]

On 05/26/2014 12:09 PM, Martin Kosek wrote:
> 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.

Thanks for the thorough review!
Pushed to master: 63becae88c6c270b98f0432dc474b661b82f3119


-- 
Petr³