[Freeipa-devel] [PATCH 0179] Preserve user principal aliases during rename operation
Simo Sorce
simo at redhat.com
Wed Jul 13 15:00:56 UTC 2016
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.
More information about the Freeipa-devel
mailing list