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

[Martin Kosek]

On 04/25/2014 01:29 PM, Petr Viktorin wrote:
> 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.

I think this is fine, I also tested upgrade and it went well.

ACK for all 3, pushed to master: 99691d117168e9ed95413f96f839b38320ac17f9

Martin