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

[Petr Vobornik]

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.

Ideally we would have e.g.

 ipa user-rename oldCN --remove-aliases

which would drop everything including oldCN (I would expect that).

Unfortunately we have --rename in mod command
 ipa user-mod oldCN --rename newCn --remove-aliases

And there --remove-aliases might not be the best thing.

Do we want to support also:
  ipa user-mod CN --remove-aliases
?

> 
> Simo.
> 


-- 
Petr Vobornik