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

Wed Jun 4 08:15:26 UTC 2014

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...

> 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.

Martin