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

[Martin Kosek]

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