[Freeipa-devel] [PATCH 0179] Preserve user principal aliases during rename operation

Wed Jul 13 11:53:23 UTC 2016

On 07/12/2016 04:19 PM, Simo Sorce wrote:
> On Tue, 2016-07-12 at 15:46 +0200, Martin Babinsky wrote:
>> On 07/12/2016 02:00 PM, Martin Babinsky wrote:
>>>
>>> On 07/12/2016 01:05 PM, Alexander Bokovoy wrote:
>>>>
>>>> On Mon, 11 Jul 2016, Martin Babinsky wrote:
>>>>>
>>>>> From 185bde00a76459430d95ff207bf1fb3fe31e811a Mon Sep 17
>>>>> 00:00:00 2001
>>>>> From: Martin Babinsky <mbabinsk at redhat.com>
>>>>> Date: Fri, 1 Jul 2016 18:09:04 +0200
>>>>> Subject: [PATCH] Preserve user principal aliases during rename
>>>>> operation
>>>>>
>>>>> When a MODRDN is performed on the user entry, the MODRDN plugin
>>>>> resets
>>>>> both
>>>>> krbPrincipalName and krbCanonicalName to the value constructed
>>>>> from
>>>>> uid. In
>>>>> doing so, hovewer, any principal aliases added to the
>>>>> krbPrincipalName
>>>>> are
>>>>> wiped clean. In this patch old aliases are fetched before the
>>>>> MODRDN
>>>>> operation
>>>>> takes place and inserted back after it is performed.
>>>>>
>>>>> This also preserves previous user logins which can be used
>>>>> further for
>>>>> authentication as aliases.
>>>>>
>>>>> https://fedorahosted.org/freeipa/ticket/6028
>>>>> ---
>>>>> ipaserver/plugins/baseuser.py | 46
>>>>> +++++++++++++++++++++++++++++++++++++++++++
>>>>> 1 file changed, 46 insertions(+)
>>>>>
>>>>> diff --git a/ipaserver/plugins/baseuser.py
>>>>> b/ipaserver/plugins/baseuser.py
>>>>> index
>>>>> 0052e718afe639bcc1c0a698ded39ea8407a0551..e4288a5a131157815ffb2
>>>>> 452692a7edb342f6ac3
>>>>>
>>>>> 100644
>>>>> --- a/ipaserver/plugins/baseuser.py
>>>>> +++ b/ipaserver/plugins/baseuser.py
>>>>> @@ -498,6 +498,50 @@ class baseuser_mod(LDAPUpdate):
>>>>>                             len =
>>>>> int(config.get('ipamaxusernamelength')[0])
>>>>>                         )
>>>>>                     )
>>>>> +
>>>>> +    def preserve_krbprincipalname_pre(self, ldap, entry_attrs,
>>>>> *keys,
>>>>> **options):
>>>>> +        """
>>>>> +        preserve user principal aliases during rename
>>>>> operation. This
>>>>> is the
>>>>> +        pre-callback part of this. Another method called
>>>>> during
>>>>> post-callback
>>>>> +        shall insert the principals back
>>>>> +        """
>>>>> +        if options.get('rename', None) is None:
>>>>> +            return
>>>>> +
>>>>> +        try:
>>>>> +            old_entry = ldap.get_entry(
>>>>> +                entry_attrs.dn, attrs_list=(
>>>>> +                    'krbprincipalname', 'krbcanonicalname'))
>>>>> +
>>>>> +            if 'krbcanonicalname' not in old_entry:
>>>>> +                return
>>>>> +        except errors.NotFound:
>>>>> +            self.obj.handle_not_found(*keys)
>>>>> +
>>>>> +        self.context.krbprincipalname = old_entry.get(
>>>>> +            'krbprincipalname', [])
>>>>> +
>>>>> +    def preserve_krbprincipalname_post(self, ldap,
>>>>> entry_attrs,
>>>>> **options):
>>>>> +        """
>>>>> +        Insert the preserved aliases back to the user entry
>>>>> during
>>>>> rename
>>>>> +        operation
>>>>> +        """
>>>>> +        if options.get('rename', None) is None or not hasattr(
>>>>> +                self.context, 'krbprincipalname'):
>>>>> +            return
>>>>> +
>>>>> +        obj_pkey =
>>>>> self.obj.get_primary_key_from_dn(entry_attrs.dn)
>>>>> +        canonical_name = entry_attrs['krbcanonicalname'][0]
>>>>> +
>>>>> +        principals_to_add = tuple(p for p in
>>>>> self.context.krbprincipalname if
>>>>> +                                  p != canonical_name)
>>>>> +
>>>>> +        if principals_to_add:
>>>>> +            result = self.api.Command.user_add_principal(
>>>>> +                obj_pkey, principals_to_add)['result']
>>>>> +
>>>>> +            entry_attrs['krbprincipalname'] =
>>>>> result.get('krbprincipalname', [])
>>>>> +
>>>>>     def check_mail(self, entry_attrs):
>>>>>         if 'mail' in entry_attrs:
>>>>>             entry_attrs['mail'] =
>>>>> self.obj.normalize_and_validate_email(entry_attrs['mail'])
>>>>> @@ -557,9 +601,11 @@ class baseuser_mod(LDAPUpdate):
>>>>>
>>>>>         self.check_objectclass(ldap, dn, entry_attrs)
>>>>>         self.obj.convert_usercertificate_pre(entry_attrs)
>>>>> +        self.preserve_krbprincipalname_pre(ldap, entry_attrs,
>>>>> *keys,
>>>>> **options)
>>>>>
>>>>>     def post_common_callback(self, ldap, dn, entry_attrs,
>>>>> *keys,
>>>>> **options):
>>>>>         assert isinstance(dn, DN)
>>>>> +        self.preserve_krbprincipalname_post(ldap, entry_attrs,
>>>>> **options)
>>>>>         if options.get('random', False):
>>>>>             try:
>>>>>                 entry_attrs['randompassword'] =
>>>>> unicode(getattr(context, 'randompassword'))
>>>>> --
>>>>> 2.5.5
>>>>>
>>>> The approach looks good.
>>>>
>>>> For the record, we also support aliases for hosts and service
>>>> kerberos
>>>> principals but we don't support rename options for them, so there
>>>> is no
>>>> need to add similar logic there.
>>>>
>>>>
>>> That's right, I have updated the corresponding section of the
>>> design
>>> page [1] for future reference.
>>>
>>> [1]
>>> http://www.freeipa.org/page/V4/Kerberos_principal_aliases#Managemen
>>> t_framework
>>>
>>>
>> Adding Simo to the loop since he is not convinced that this is the
>> right
>> behavior. As I see it, it seems to not be a security issue but more
>> of a
>> different expectations about the perceived correct behavior in this
>> particular situation.
>>
>> I can see the use case in keeping the old aliases, e.g. keeping the
>> old
>> credentials after legal name change, but I can also see the
>> available
>> space for user principal names piling up and cluttering quickly in
>> large
>> organizations.
>
> after some thinking I think it is ok to keep by default and then drop
> as it avoid races if you do really want to keep the aliases.
>
> However the CLI/UI should probably offer a button/switch to allow to
> drop all aliases on rename, what it would do would be to modify the
> entry after the rename and drop the aliases.
>
> I am a bit uncertain what to do by default with the "original name".
> I can see it going both ways on whether to keep it by default or not.
>
> Simo.
>

So let me summarize the proposed behavior just to see if I understand it 
correctly:

1.) when the user is renamed, all his kerberos aliases are added back to 
the attribute after rename

2.) a new Flag will be added to *user-mod (e.g. --remove-aliases) which 
will suppress this behavior when True so that all previous aliases will 
be erased as is done now. The administrator will use this flag to 
override the default behavior in 1.)

We now have the following concerns:

3.) What to do with old canonical names:

The proposed patch treats old canonical names as aliases which are 
re-added after rename (since krbPrincipalName before rename contains the 
alias equal to the original canonical name anyway, we get this for 
free). If we implement the alias-wiping flag from 2.), we have to decide 
whether the old canonical name should be wiped away as well, or 
implement an additional logic to preserve it.

4.) What to do with users coming from pre-4.4 FreeIPA versions. These 
have no krbCanonicalName attribute and have only one alias = canonical 
name, so the code does no special handling of them and behaves as 
before. If we want to keep his old principal after rename, we have to 
implement another path which constructs krbCanonicalName attribute for 
him during rename and adds old principal as an alias among his 
krbPrincipalNames.

I would say that decoupling the preservation of old canonical principals 
from the preservation of aliases is relatively easy to implement, but I 
am bit afraid that it will increase the complexity and potential 
fragility of the solution.

-- 
Martin^3 Babinsky