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 <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.


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.

--
Martin^3 Babinsky

--
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