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

Wed May 28 14:27:54 UTC 2014

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.

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

-- 
Petr³