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

[Simo Sorce]

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.