[Freeipa-devel] [PATCH 0179] Preserve user principal aliases during rename operation
Martin Babinsky
mbabinsk at redhat.com
Wed Jul 13 15:04:46 UTC 2016
On 07/13/2016 05:00 PM, Simo Sorce wrote:
> On Wed, 2016-07-13 at 16:35 +0200, Martin Babinsky wrote:
>> On 07/13/2016 04:28 PM, Simo Sorce wrote:
>>>
>>> On Wed, 2016-07-13 at 16:19 +0200, Martin Babinsky wrote:
>>>>
>>>> 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..e4288a5a1
>>>>>>>>>>> 3115
>>>>>>>>>>> 7815
>>>>>>>>>>> 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_
>>>>>>>>>>> attr
>>>>>>>>>>> s)
>>>>>>>>>>> + 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_alias
>>>>>>>>> es#M
>>>>>>>>> anag
>>>>>>>>> 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.
>>> True.
>>> Should we then not have --remove-aliases at all, and always force
>>> the
>>> admin to manually cleanup ?
>>>
>>>
>>> Simo.
>>>
>> I guess that would be the path of least resistance for us. We will
>> document the change of behavior and leave current API intact for
>> now.
>> The option can easily be added later if community requests it.
>
> Ok, then, let's proceed.
>
> Simo.
>
In that case, if nobody objects then the second revision of the patch
may be pushed since Alexander already acked it, right Alexander?
--
Martin^3 Babinsky
More information about the Freeipa-devel
mailing list