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 _______________________________________________ Freeipa-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/freeipa-devel
