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