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

Wed Jul 13 14:19:16 UTC 2016

On 07/13/2016 03:08 PM, Simo Sorce wrote:
> On Wed, 2016-07-13 at 14:37 +0200, Petr Vobornik wrote:
>> 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..e4288a5a131157815
>>>>>>> ffb2
>>>>>>> 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#Manag
>>>>> emen
>>>>> 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).
>
> I lean toward this conclusion too given that a later "--remove-alias"
> would drop it, and we want to behave consistently.
>
Makes sense to me, too.

>> 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.
>
> Why not ?
>
>> Do we want to support also:
>>   ipa user-mod CN --remove-aliases
>
> Yes we probably want to give the option to drop aliases if an admin
> realizes he should have done it at rename time but didn't.
>
> Simo.
>

In that case he can still use `user-remove-principal` to manually remove 
them.

-- 
Martin^3 Babinsky