Re: [Freeipa-devel] [PATCH] 0537-0539 aci-update: Trim the admin write blacklist Add ACI for read-only admin attributes
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Ā³ From 92a510d599760d2d3d3e97905eb41050bc6f276f Mon Sep 17 00:00:00 2001 From: Petr Viktorin pvikt...@redhat.com Date: Wed, 23 Apr 2014 19:09:31 +0200 Subject: [PATCH] test_ldap: Read a publicly
Re: [Freeipa-devel] [PATCH] 0537-0539 aci-update: Trim the admin write blacklist Add ACI for read-only admin attributes
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: