now with patches ...
On Tue, Jul 26, 2016 at 04:55:37PM +0200, Sumit Bose wrote:
> On Tue, Jul 26, 2016 at 11:12:34AM +0200, Jakub Hrozek wrote:
> > On Fri, Jul 22, 2016 at 09:44:33PM +0200, Sumit Bose wrote:
> > > Hi,
> > >
> > > this patch set should fix
https://fedorahosted.org/sssd/ticket/2958
> > > "Support multiple principals for IPA users" so the IPA users can
log in
> > > with their Kerberos alias as well.
> >
> > First, thank you very much for splitting the patchset into small
> > self-contained changes. The review was much easier this way.
> >
> > >
> > > The overall code-path was already added for the UPN feature but had to
> > > be extended at various places. The main difference is that the realm
> > > part of the AD UPNs with an alternative domain suffix do not related to
> > > a known domain name. The realm part from the Kerberos aliases can come
> > > from any domain. The same is true for email addresses which are supported
> > > by this patch set as well.
> >
> > I wonder if we will immediatelly get a request to provide a list of
> > e-mail domain suffixes by the users that could be used to map the e-mail
> > address to a domain. Do you think it would make sense (maybe in a future
> > patch) to let the admin configure the suffixes for direct integration in
> > sssd.conf and maybe on the server for IPA-AD trusts?
>
> It might help in some cases, but I think in general it will not help
> much. Email domain suffixes as e.g. AD's alternative domain suffixes are
> typically used for a whole organization/forest so I would expect that
> the cases where you have multiple domains and a single email domain
> suffix relates only to a specific domain and is not equal to the domain
> name is rare.
>
> What might help more (and would help lookup by id as well) is to reorder
> the lookups in the responders to check the cache for all domains first
> before iterating over the backends. I think we already have a ticket for
> this.
>
> >
> > >
> > > I know that Jakub had some concerns adding email addresses now and the
> > > another attribute in the next version and so on. I would like to make
> > > this scheme more generic so that the attributes which should be used for
> > > login names can be configured. Unfortunately I didn't had the time do
> > > already do it in this patch-set.
> >
> > np, I think the patches are well structured and make the extension
> > possible.
> >
> > >
> > > Adding a larger number of sources for the login name increases the
> > > chance for collisions. But since each of the name types, Kerberos
> > > principals and email addresses, are expected to be unique in their
> > > domain, I hope the chances are still low enough.
> >
> > Yes.
> >
> > >
> > > bye,
> > > Sumit
> >
> > > From b2012e5b0cc8af65f18a6e48774b80246c68c04a Mon Sep 17 00:00:00 2001
> > > From: Sumit Bose <sbose(a)redhat.com>
> > > Date: Fri, 22 Jul 2016 17:35:43 +0200
> > > Subject: [PATCH 01/14] IPA: fix lookup by UPN for subdomains
> > >
> > > Currently the user name used in the extdom exop request is
> > > unconditionally set to the short name. While this is correct for the
> > > general name based lookups it breaks UPN/email based lookups where the
> > > name part after the @-sign might not match to domain name. I guess this
> > > was introduce during the sysdb refactoring.
> >
> > Yes, it was. Sorry about that. I tested UPN lookups on an IPA-AD trust
> > client and they work fine with this patch.
> >
> > ACK
> >
> > > From 762cd84cc5851666607c871d53f4e309b8760f6c Mon Sep 17 00:00:00 2001
> > > From: Sumit Bose <sbose(a)redhat.com>
> > > Date: Fri, 22 Jul 2016 12:19:26 +0200
> > > Subject: [PATCH 02/14] LDAP: allow multiple user principals
> > >
> > > In general a user can have multiple principals and recent IPA version
> > > added support to defined multiple principals. With this patch SSSD does
> > > not only store the first but all principals read by LDAP from a server.
> > >
> > > Resolves
https://fedorahosted.org/sssd/ticket/2958
> >
> > ACK
> >
> >
> > > From 5c1a50ef35d816438880e4e9951d0355b283980f Mon Sep 17 00:00:00 2001
> > > From: Sumit Bose <sbose(a)redhat.com>
> > > Date: Sat, 18 Jun 2016 18:24:50 +0200
> > > Subject: [PATCH 03/14] LDAP: new attribute option ldap_user_email
> >
> > ACK
> >
> > > From 3f09cfc6598d34ff42be1c85f448b5c0635410e0 Mon Sep 17 00:00:00 2001
> > > From: Sumit Bose <sbose(a)redhat.com>
> > > Date: Mon, 20 Jun 2016 12:57:43 +0200
> > > Subject: [PATCH 04/14] sysdb: include email in UPN searches
> >
> > ACK just one question..
> >
> > >
> > > Email addresses and Kerberos user principals names (UPNs) do not only
> > > look similar they also can be used to identify a user uniquely.
> > >
> > > In future this approach should be replace by a more generic one where
> > > the attributes which can uniquely identifies a user can be configured to
> > > support even a wider range of login names.
> >
> > ..I think we want to track this with a ticket (even if it was deferred
> > now). If you agree I can file one.
>
> yes, please.
>
> >
> > > From 6ac7dacf122da2933ce6fe9b2ceff356c1b65721 Mon Sep 17 00:00:00 2001
> > > From: Sumit Bose <sbose(a)redhat.com>
> > > Date: Mon, 20 Jun 2016 12:58:16 +0200
> > > Subject: [PATCH 05/14] LDAP: include email in UPN searches
> >
> > ACK
> >
> > > From b3c9c37baa49ade82f8cf873a2e0d2e0e7e16fad Mon Sep 17 00:00:00 2001
> > > From: Sumit Bose <sbose(a)redhat.com>
> > > Date: Mon, 20 Jun 2016 13:37:56 +0200
> > > Subject: [PATCH 06/14] NSS: add user email to fill_orig()
> > >
> > > The IPA server must send the email address of a user to the clients to
> > > allow login by email.
> >
> > ACK
> >
> > > From 88118645466c50c52a81e5ff38d66ecb2dfda2a7 Mon Sep 17 00:00:00 2001
> > > From: Sumit Bose <sbose(a)redhat.com>
> > > Date: Mon, 20 Jun 2016 16:11:11 +0200
> > > Subject: [PATCH 07/14] utils: add is_email_from_domain()
> >
> > ACK
> >
> > > From 72fb323490acc4155227b56be6470965830b7ade Mon Sep 17 00:00:00 2001
> > > From: Sumit Bose <sbose(a)redhat.com>
> > > Date: Mon, 20 Jun 2016 16:30:03 +0200
> > > Subject: [PATCH 08/14] LDAP/IPA: add local email address to aliases
> > >
> > > Adding email-addresses from the local domain to the alias names is
> > > strictly not needed by might help to speed up lookups in the NSS
> > > responder.
> >
> > ACK, this makes sense, the aliases are indexed.
> >
> > > From 0f2863a0fb1620bf86ffb828e0c51ead4f807b2a Mon Sep 17 00:00:00 2001
> > > From: Sumit Bose <sbose(a)redhat.com>
> > > Date: Tue, 21 Jun 2016 11:06:19 +0200
> > > Subject: [PATCH 09/14] NSS: continue with UPN/email search if name was
not
> > > found
> > >
> > > Currently we only search for UPNs if the domain part of the name was not
> > > know, with Kerberos aliases and email addresses we have to do this even
> > > if the domain name is a know domain.
> >
> > ACK
> >
> > > From 84813abfb71dc23ad054c42af0e3d2eba1e74e7e Mon Sep 17 00:00:00 2001
> > > From: Sumit Bose <sbose(a)redhat.com>
> > > Date: Wed, 22 Jun 2016 18:21:11 +0200
> > > Subject: [PATCH 10/14] PAM: continue with UPN/email search if name was
not
> > > found
> > >
> > > Currently we only search for UPNs if the domain part of the name was not
> > > know, with Kerberos aliases and email addresses we have to do this even
> > > if the domain name is a know domain.
> >
> > ACK
> >
> > > From bccc06776316459ec10c8103465664df2433f260 Mon Sep 17 00:00:00 2001
> > > From: Sumit Bose <sbose(a)redhat.com>
> > > Date: Fri, 22 Jul 2016 17:34:20 +0200
> > > Subject: [PATCH 12/14] PAM: Fix domain for UPN based lookups
> > >
> > > Since sysdb_search_user_by_upn() searches the whole cache we have to set
> > > the domain so that it matches the result.
> >
> > ACK, we already have a matching commit in NSS
> >
> > > From c132fa210341b30ec4586c0bb951ffb1dc6feb94 Mon Sep 17 00:00:00 2001
> > > From: Sumit Bose <sbose(a)redhat.com>
> > > Date: Fri, 22 Jul 2016 12:20:33 +0200
> > > Subject: [PATCH 13/14] SDAP: add special handling for IPA Kerberos
enterprise
> > > principal strings
> > >
> > > Unfortunately principal aliases with an alternative realm are stored in
> > > IPA as the string representation of an enterprise principal, i.e.
> > > name\@alt.realm(a)IPA.REALM. To allow searches with the plain alias
> > > 'name(a)alt.realm' the returned value is converted before it is
saved to
> > > the cache.
> >
> > ACK
> >
> > > From 3b4deb7deb64136094aa5e70705b467a836dd05b Mon Sep 17 00:00:00 2001
> > > From: Sumit Bose <sbose(a)redhat.com>
> > > Date: Fri, 22 Jul 2016 20:10:42 +0200
> > > Subject: [PATCH 14/14] SDAP: add enterprise principal strings for user
> > > searches
> > >
> > > Unfortunately principal aliases with an alternative realm are stored in
> > > IPA as the string representation of an enterprise principal, i.e.
> > > name\@alt.realm(a)IPA.REALM. To be able to lookup the alternative
> > > principal in LDAP properly the UPN search filter is extended to search
> > > for this type of name as well.
> >
> > There is a warning:
> > CC src/tests/cmocka/nestedgroups_tests-common_mock_sdap.o
> > /home/remote/jhrozek/devel/sssd/src/providers/ldap/sdap_utils.c: In function
‘get_enterprise_principal_string_filter’:
> > /home/remote/jhrozek/devel/sssd/src/providers/ldap/sdap_utils.c:254:37:
warning: field precision specifier ‘.*’ expects argument of type ‘int’, but argument 4 has
type ‘long int’ [-Wformat=]
> > return talloc_asprintf(mem_ctx, "(%s=%.*s\\\\@%s@%s)",
attr_name,
> > ^
> >
> > But the intent of the code looks good to me.
>
> I fixed this with a cast, I hope this is sufficient to silence the
> warning.
>
> New version which fixes the Coverity warning as well is attached.
>
> bye,
> Sumit
Thank you, I saw one login failure with these patches where a wrong UPN
was passed to the krb5_child, causing a System Error. But neither me nor
Sumit were able to reproduce the bug afterwards. Because the patches are
working for me now just fine, I will push them and watch closely for any
failures in my IPA-AD trust setup..
ACK, waiting for CI to finish before pushing.