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 <mbabi...@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..e4288a5a13115
> > > > > > > > 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_aliases#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.

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to