[Freeipa-devel] [PATCHES] 0552-0554 Upgrading write permissions

Wed Jun 4 15:36:52 UTC 2014

On 06/04/2014 05:23 PM, Martin Kosek wrote:
> On 06/04/2014 10:43 AM, Petr Viktorin wrote:
>> On 06/04/2014 10:15 AM, Martin Kosek wrote:
>>> On 06/03/2014 02:41 PM, Petr Viktorin wrote:
>>>> On 05/28/2014 04:36 PM, Petr Viktorin wrote:
>>>>> On 05/28/2014 04:27 PM, Petr Viktorin wrote:
>>>>>> On 05/27/2014 04:20 PM, Martin Kosek wrote:
>>>>>>> On 05/26/2014 04:44 PM, Petr Viktorin wrote:
>>>>>>>> On 05/22/2014 03:07 PM, Petr Viktorin wrote:
>>>>>>>>> Hello,
>>>>>>>>> Here I start upgrading  the existing default permissions to the new
>>>>>>>>> Managed style.
>>>>>>>>>
>>>>>>>>> https://fedorahosted.org/freeipa/ticket/4346
>>>>>>>>>
>>>>>>>>> The patches rely on my patch 0551
>>>>>>>>> (https://fedorahosted.org/freeipa/ticket/4349)
>>>>>>>>> You may run into what seems to be a 389 bug. If you get a "Midair
>>>>>>>>> Collision" (NO_SUCH_ATTRIBUTE) error, restart the DS and try running
>>>>>>>>> ipa-ldap-updater again. I'm working with Ludwig on this one.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> The operation is now described at
>>>>>>>>> http://www.freeipa.org/page/V4/Managed_Read_permissions#Replacing_legacy_default_permissions
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> If there user has modified an old default permission, a warning is
>>>>>>>>> logged the replacement permission is not added/updated. The user needs
>>>>>>>>> to evaluate the situation: either update the old permission to match
>>>>>>>>> the
>>>>>>>>> original default, or remove the old permission, and then run
>>>>>>>>> ipa-ldap-updater will create the new one.
>>>>>>>>> Is bailing out the right thing to do if the old entry was modified?
>>>>>>>
>>>>>>> Forcing user to remove old permission and create new one seems as a
>>>>>>> too much
>>>>>>> work to me. After the upgrade, we need to be sure that the managed
>>>>>>> permissions
>>>>>>> is there.
>>>>>>
>>>>>> Note that this only happens if the user changed the permissions, so we
>>>>>> need to be extra careful to respect their wishes.
>>>>>>
>>>>>>> What is the problem of having both 2 permissions in the DS? The old
>>>>>>> modified
>>>>>>> permission and the new system managed one? As we are dealing with allow
>>>>>>> permissions, having 2 of them should be harmless.
>>>>>>
>>>>>> The new one could be granting too much access, the admin might have
>>>>>> wanted to restrict the defaults.
>>>>>>
>>>>>>
>>>>>>>>> It could be possible to parse the permission, figure out the changes
>>>>>>>>> the
>>>>>>>>> user made, and apply them to the new one, but that seems like too much
>>>>>>>>> guesswork to me.
>>>>>>>
>>>>>>> Maybe we could do the same we do with managed permissions upgrades?
>>>>>>> Only allow
>>>>>>> differences in the list of attributes? I am thinking that people could
>>>>>>> hotfix
>>>>>>> missing attributes at permissions themselves (like adding description to
>>>>>>> sudorule permission), this would lead to duplicate permissions later.
>>>>>>>
>>>>>>> What we could do when old ACI differs only in allowed attributes is to
>>>>>>> compare
>>>>>>> it to defaults and set whitelist and blacklist attributes of the new
>>>>>>> managed
>>>>>>> permission. Then we can safely delete the old ACI (with warning).
>>>>>>>
>>>>>>> If you think this is too much work, we can keep the old behavior and
>>>>>>> just add
>>>>>>> duplicate ACI.
>>>>>>
>>>>>> Having duplicate permissions would be possible, after all they have a
>>>>>> different name. However I'd expect that most people would still want to
>>>>>> delete the old ones, rather than letting them hide among user-defined
>>>>>> permissions.
>>>>>>
>>>>>>>>> On the other hand, my approach has a downside as well: if the
>>>>>>>>> 'memberallowcmd' attribute was removed from 'Modify Sudo rule',
>>>>>>>>> there's
>>>>>>>>> now no way to upgrade while allowing access but keeping that attribute
>>>>>>>>> off-limits, short of writing deny a ACI by hand. How big a problem is
>>>>>>>>> this? It might be worth it to create a special tool that upgrades a
>>>>>>>>> single permission and allows setting the excluded/included attributes
>>>>>>>>> explicitly.
>>>>>>>
>>>>>>> This problem would be removed with my approach proposed above.
>>>>>>>
>>>>>>>>> There are some interesting scenarios to think about with respect to
>>>>>>>>> upgrades and user changes:
>>>>>>>>>
>>>>>>>>> * Upgrade to old version, e.g.
>>>>>>>>>       - have IPA 3.2 master, IPA 3.2 replica
>>>>>>>>>       - upgrade master to 4.0 (old permissions are updated)
>>>>>>>>>       - then upgrade replica to 3.3 (old permissions are added again!)
>>>>>>>>>
>>>>>>>>> This is AFAIK not supported but it does happen.
>>>>>>>>> We can't change old IPA versions, so any upgrade to a pre-4.0 IPA will
>>>>>>>>> always add the old permissions, but with this patch, a subsequent
>>>>>>>>> upgrade to 4.0+, or running a 4.0+ ipa-ldap-update, will remove the
>>>>>>>>> old
>>>>>>>>> permissions again.
>>>>>>>
>>>>>>> Hm, I think this is the best option we have. We should warn about this
>>>>>>> behavior
>>>>>>> in our release notes though.
>>>>>>>
>>>>>>>>> Tied to that is another scenario:
>>>>>>>>>
>>>>>>>>> * Re-create permissions with old names
>>>>>>>>>       - have IPA 4.0 master
>>>>>>>>>       - Create a permission named 'Modify Sudo rule'
>>>>>>>>>       - Upgrade to IPA 4.1
>>>>>>>>>
>>>>>>>>> Here we need to make sure the new permission is *not* removed,
>>>>>>>>> because a
>>>>>>>>> new 'Modify Sudo rule' permission is no longer special in any way. To
>>>>>>>>> ensure this the updater only removes old-style permissions.
>>>>>>>
>>>>>>> Right, we can decide based on objectclasses - whether permissionsv2 OC
>>>>>>> is there
>>>>>>> or not.
>>>>>>>
>>>>>>>>>
>>>>>>>>> One thing that can happen when 4.0 masters are still mixed with 3.x is
>>>>>>>>> that an old permission named 'Modify Sudo rule' is added on the old
>>>>>>>>> server. Any update to 4.0+ will remove that.
>>>>>>>>> Old-style default permissions were sorta-kinda managed by IPA itself
>>>>>>>>> anyway, so users should expect this. We should still point it out in
>>>>>>>>> the
>>>>>>>>> docs though, since I expect some users to start messing with the
>>>>>>>>> permissions before upgrading all of the infrastructure to 4.0.
>>>>>>>
>>>>>>> +1, I would just point out that behavior in the release notes.
>>>>>>>
>>>>>>>>> The second patch upgrades sudorule permissions, this server as an
>>>>>>>>> example of how the  will work.
>>>>>>>>> The third patch fixes https://fedorahosted.org/freeipa/ticket/4344
>>>>>>>>
>>>>>>>> The user read permissions patches had a conflict with these;
>>>>>>>> attaching rebased
>>>>>>>> version.
>>>>>>>
>>>>>>> Now the actual review
>>>>>>> 552.2: worked fine for me. Some updates will probably be needed
>>>>>>> though, based
>>>>>>> on the discussion above.
>>>>>>>
>>>>>>> 553.2:
>>>>>>>
>>>>>>> 1) Why should we bother specifying ipapermdefaultattr for "add" ACIs?
>>>>>>> Looks
>>>>>>> like a noop to me, it was also never part of our add ACIs.
>>>>>>
>>>>>> Simo, I hazily remember discussing that we should only allow specific
>>>>>> attributes on add, otherwise users can add entries with any extra
>>>>>> objectclasses and attributes. Did we come to a conclusion?
>>>>>> I might have confused targetattr with targetattrfilter in my notes;
>>>>>> since I see targetarr is ineffective.
>>>>>
>>>>> OK, this was just me confused. As Ludwig told me,
>>>>>> for adding an entry you need add rights for the entry and write rights
>>>>>> for each attribute, so in the add aci the targetattrs are irrelevant.
>>>>> so I'll remove them from the add ACI.
>>>>>
>>>>>>> I tried to strip that down to just "description" and I was still able
>>>>>>> to add a
>>>>>>> whole new SUDO rule. Ludwig, is that correct - does DS ignore (should
>>>>>>> it?)
>>>>>>> targetattr part of add ACI?
>>>>>>>
>>>>>>> 2) You stated 'System: Modify Sudo rule' as "add" ACI, making it
>>>>>>> ineffective.
>>>>>>> Privileged user still cannot update all SUDO rule attributes.
>>>>>>
>>>>>> Duh. I've been staring at this too long.
>>>>>>
>>>>>>> Besides that, the ACIs were working fine.
>>>>
>>>> The attached version looks at the old permission in LDAP and if it differs from
>>>> the old default only in the targetattrs, it transplants the difference to the
>>>> new managed permission.
>>>>
>>>> There is a lot of logging here so if something didn't work the way you
>>>> expected, at least you'll know what happened.
>>>>
>>>> When there were multiple defaults, i.e. IPA added/removed some attributes in
>>>> some version: the new managed permission's attributes will be applied, so
>>>> upgrades from both very old and not-so-old versions should "do the right
>>>> thing".
>>>>
>>>> If the old permission differs in something else than targetattrs, an error is
>>>> logged (this will show up in yum update output), and as before the new managed
>>>> permission is not created. The user now has 3 options to fix this:
>>>> - Delete the old permission, then run ipa-ldap-updater to create the new
>>>> default.
>>>
>>> This will probably be the safest route to go for users.
>>>
>>>> - Modify the old permission on an *old* system to match the old default,
>>>> possibly with targetattr changes, then run a *new* ipa-ldap-updater to convert
>>>> that to the new default
>>>> - Modify the old permission on a *new* system, which changes it to a V2
>>>> permission, then run ipa-ldap-updater to create the new default, ending up with
>>>> both permissions.
>>>>
>>>> The distinction betwen the last two is subtle and error-prone, but
>>>> 1) I don't see a better way, considering that future upgrades need to work well
>>>> (in IPA 4.0+ a user-created permission named "Add Sudo Rule" has no special
>>>> status)
>>>> 2) I'm hoping that people didn't modify the old default permissions that much;
>>>> if they did they should have felt some pain already -- I don't think the update
>>>> system in 3.x would handle such changes wery well
>>>
>>> I would personally still be more forceful, especially given the arguments you
>>> gave in 2) and just log the old too-modified ACI and convert it to the new
>>> style, but there are not many followers to this opinion though...
>>
>> On the other hand, if someone did this kind of thing they probably (thought
>> they) knew what they were doing, so they deserve a chance for manual intervention.
>
> Ok, fair enough. I found permission-del & "ipa-ldap-update --upgrade" as the
> easiest way to restore such permission. We should add similar hint later to
> release notes.
>
>>
>>>> Apply my patch 0565 before trying these out.
>>>>
>>>>
>>>> Some testing tips:
>>>> - Create 3.x master and replica
>>>> - Upgrade master RPMs with these patches
>>>> - to add old permissions, run ipa-ldap-updater on the replica
>>>> - to simulate an upgrade, run ipa-ldap-updater on the master
>>>> - to delete a managed permission:
>>>> $ curl -v -H Content-Type:application/json -H Accept:applicaton/json
>>>> --negotiate -u : --delegation always --cacert /etc/ipa/ca.crt -X POST -H
>>>> referer:https://`hostname`/ipa/json -d '{"method": "permission_del",
>>>> "params":[["$PERMISSION_NAME"],{"force":true}], "id":0 }'
>>>> https://`hostname`/ipa/json
>>>> - be careful where you run permission-mod; on 4.0 it will convert the
>>>> permission to V2
>>>
>>> In my tests, the upgrade from 3.x worked as expected so that went well. What I
>>> still miss is a removal of ipapermdefaultattr in Add ACI in 553.3. As we
>>> mentioned earlier, it is a NOOP and rather confuses people instead of improving
>>> anything. Besides that, I did not spot any pending error.
>>
>> Ah, sorry, I rebased on the wrong patch.
>> Here is the fixed version.
>
> This one works fine, so ACK from me.
>
> Martin
>

Thanks for reviewing!
Pushed to master: f802845a7abfca0b414ad6801968d33e6788916b

-- 
Petr³