[Freeipa-devel] [PATCH] 0537-0539 aci-update: Trim the admin write blacklist & Add ACI for read-only admin attributes

Fri Apr 25 11:29:32 UTC 2014

On 04/25/2014 01:08 PM, Martin Kosek wrote:
> On 04/25/2014 01:01 PM, Petr Viktorin wrote:
>> On 04/24/2014 05:15 PM, Simo Sorce wrote:
>>> On Thu, 2014-04-24 at 16:47 +0200, Martin Kosek wrote:
>>>> On 04/24/2014 03:42 PM, Simo Sorce wrote:
>>>>> On Thu, 2014-04-24 at 15:18 +0200, Martin Kosek wrote:
>>>>>> On 04/24/2014 02:28 PM, Simo Sorce wrote:
>>>>>>> On Thu, 2014-04-24 at 14:17 +0200, Martin Kosek wrote:
>>>>>>>> On 04/24/2014 09:41 AM, Petr Viktorin wrote:
>>>>>>>>> On 04/23/2014 08:56 PM, Simo Sorce wrote:
>>>>>>>>>> On Wed, 2014-04-23 at 20:37 +0200, Petr Viktorin wrote:
>>>>>>>>>>> Admin access to read-only attributes such as ipaUniqueId, memberOf,
>>>>>>>>>>> krbPrincipalName is provided by the anonymous read ACI, which will go
>>>>>>>>>>> away. This patch adds a blanket read ACI for these.
>>>>>>>>>>> I also moved some related ACIs to 20-aci.update.
>>>>>>>>>>>
>>>>>>>>>>> Previously krbPwdHistory was also readable by admins. I don't think we
>>>>>>>>>>> want to include that.
>>>>>>>>>>> Simo, should admins be allowed to read krbExtraData?
>>>>>>>>>>
>>>>>>>>>> Probably not necessary but there is nothing secret in it either.
>>>>>>>>>>
>>>>>>>>>> Simo.
>>>>>>>>>
>>>>>>>>> OK. I'm not a fan of hiding things from the admin, so no changes to the
>>>>>>>>> patch
>>>>>>>>> are necessary here.
>>>>>>>>>
>>>>>>>>
>>>>>>>> 536:
>>>>>>>> As we are touching these ACIs, may it is a time to see the blacklist of
>>>>>>>> attributes that admin cannot write and check if this is still wanted:
>>>>>>>>
>>>>>>>> ipaUniqueId - OK, generated by DS plugin
>>>>>>>> memberOf - OK, generated by DS plugin
>>>>>>>> serverHostName - I did not even find a place where we manipulate it, except
>>>>>>>> host.py -> remove from blacklist?
>>>>>>
>>>>>> What about this one?
>>>>>
>>>>> not sure, I did not work much on the hosts objects.
>>>>
>>>> Rob? Do you know?
>>>>
>>>>>>>> enrolledBy - OK, generated by DS plugin
>>>>>>>> krbExtraData - OK, generated by DS plugin
>>>>>>>> krbPrincipalName - why can't admin change it? It is filled by framework, I
>>>>>>>> would not personally blacklist it
>>>>>>>
>>>>>>> It is changed by the ipa rename plugin when the user uid change, that's
>>>>>>> why we prevent the admin from explicitly change it.
>>>>>>>
>>>>>>>> krbCanonicalName - same as krbPrincipalName
>>>>>>
>>>>>> Ok, in that case krbPrincipalName and krbCanonicalName should stay, right?
>>>>>
>>>>> They should be accessible by admin, yes.
>>>>
>>>> Note that we are now discussing write access. I.e. what attributes needs to
>>>> stay in the admin's global write ACI blacklist and which can be removed ->
>>>> admin can write in them.
>>>>
>>>> IIUC, these should be only readable by admin, not writeable.
>>>>
>>>>>
>>>>>>>> krbPrincipalAliases - same as krbPrincipalName - we need this removed if we
>>>>>>>> want to set aliases anyway
>>>>>>
>>>>>> Allow this one?
>>>>>
>>>>> This is one I wanted to eventually get rid of, Alexander seem to have
>>>>> introduced it, but we discussed in the past of a way to kill it.
>>>>> I forgot the details thouhg.
>>>>> Alexander did we open a ticket for this ?
>>>>
>>>> Ok, we may eventually get rid of it, but does it need to be blacklisted in
>>>> admins global write ACI?
>>>>
>>>>>
>>>>>>>> krbPasswordExpiration - OK, generated by DS plugin
>>>>>>>> krbLastPwdChange - OK, generated by DS plugin
>>>>>>>> krbUPEnabled - not used, can we remove it?
>>>>>>
>>>>>> What about this one?
>>>>>
>>>>> We never use it.
>>>>
>>>> I read this as "remove it from global admin write ACI blacklist".
>>>>
>>>>>
>>>>>>>> krbTicketPolicyReference - why cannot admin set it?
>>>>>>>
>>>>>>> Unclear why, probably should be able to.
>>>>>>
>>>>>> We may want to remove it from blacklist then.
>>>>>
>>>>> We never used it, but we should probably allow admin to modify
>>>>
>>>> Ok, let us remove it from global admin write ACI blacklist.
>>>>
>>>>>
>>>>>>>> krbPwdPolicyReference - why cannot admin set it?
>>>>>>>
>>>>>>> We assign password policy based on groups now, right ?
>>>>>>
>>>>>> Yup.
>>>>
>>>> I see no complains, i.e. I read it as "remove it from global admin write ACI
>>>> blacklist".
>>>>
>>>>>>
>>>>>>>
>>>>>>>> krbPrincipalType - why cannot admin set it?
>>>>>>>
>>>>>>> Unused.
>>>>>>
>>>>>> So... allow this one?
>>>>>
>>>>> we never use it so we do not need to allow admins by default.
>>>>
>>>> My point was to limit admin write ACI blacklist to the bare minimum. Only to
>>>> attributes that really needs to be blacklisted as they are operated by DS
>>>> plugins - like memberOf and others.
>>>>
>>>> My proposal is to remove it from global admin write ACI blacklist given it is
>>>> not used.
>>>
>>> Ack, I agree with your intent.
>>>
>>> Simo.
>>>
>>
>> If I understand correctly, we want to allow:
>> - krbPrincipalAliases
>> - krbPrincipalType
>> - krbPwdPolicyReference
>> - krbTicketPolicyReference
>> - krbUPEnabled
>> - serverHostName
>>
>> Here is the patch for that.
>
> The list looks good to me.
>
>> Martin, just to make sure: 0536-0537 are good to push, right?
>
> One of the reasons why I wanted to prune the blacklist a bit was to make the
> Admin read ACI (Admin read-only attributes) simpler (read - shorter). So please
> also update this aci in 536.

Ah, right.

> IMO, 536 and 538 can be squashed, but that's up to you.

I tried, but I couldn't get the commit message to not read like two 
unrelated changes, which to me is a clear sign they shouldn't be 
squashed. (I thought about also split moving the ACIs and the 
modifications, but maybe that'd be too purist.)

I renumbered 0536 to 0538 to keep the order they should be applied in. 
Entire patchset attached.

-- 
Petr³

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0537.2-test_ldap-Read-a-publicly-accessible-attribute-when-.patch
Type: text/x-patch
Size: 1410 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140425/b461a33f/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0538.2-aci-update-Trim-the-admin-write-blacklist.patch
Type: text/x-patch
Size: 13036 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140425/b461a33f/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0539.2-aci-update-Add-ACI-for-read-only-admin-attributes.patch
Type: text/x-patch
Size: 2058 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140425/b461a33f/attachment-0002.bin>