On Wed, Jul 27, 2016 at 07:06:53PM +0200, Jakub Hrozek wrote: > On Tue, Jul 26, 2016 at 04:57:19PM +0200, Sumit Bose wrote: > > 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 <sb...@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 <sb...@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 <sb...@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 <sb...@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 <sb...@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 <sb...@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 <sb...@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 <sb...@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 <sb...@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 <sb...@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 <sb...@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 <sb...@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@IPA.REALM. To allow searches with the plain alias > > > > > 'name@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 <sb...@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@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.
CI: http://sssd-ci.duckdns.org/logs/job/50/40/summary.html _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
