Re: [Freeipa-devel] [PATCH] 0538 aci-update: Trim the admin write blacklist

2014-04-25 Thread Martin Kosek
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 A

[Freeipa-devel] [PATCH] 0538 aci-update: Trim the admin write blacklist

2014-04-25 Thread Petr Viktorin

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.
Martin, just to make sure: 0536-0537 are good to push, right?

--
PetrĀ³
From 7a5deb8323348cca3475efd908dabe68466ffd19 Mon Sep 17 00:00:00 2001
From: Petr Viktorin 
Date: Fri, 25 Apr 2014 10:42:05 +0200
Subject: [PATCH] aci-update: Trim the admin write blacklist

These attributes are removed from the blacklist, which means
high-level admins can now modify them:

- krbPrincipalAliases
- krbPrincipalType
- krbPwdPolicyReference
- krbTicketPolicyReference
- krbUPEnabled
- serverHostName

The intention is to only blacklist password attributes and attributes
that are managed by DS plugins.
---
 install/updates/20-aci.update | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/install/updates/20-aci.update b/install/updates/20-aci.update
index c25fdcc92edd29f505c82959d8a70e4fd59e4c2f..b6012afea73d63a83a6791b63e43c7dd4abff9a8 100644
--- a/install/updates/20-aci.update
+++ b/install/updates/20-aci.update
@@ -38,7 +38,8 @@ dn: $SUFFIX
 remove:aci:'(targetattr != "userPasswo