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?

> 
> 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 <[email protected]>
> 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 <[email protected]>
> 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 <[email protected]>
> 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 <[email protected]>
> 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.

> From 6ac7dacf122da2933ce6fe9b2ceff356c1b65721 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <[email protected]>
> 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 <[email protected]>
> 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 <[email protected]>
> 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 <[email protected]>
> 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 <[email protected]>
> 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 <[email protected]>
> 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 <[email protected]>
> 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 <[email protected]>
> 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\@[email protected]. To allow searches with the plain alias
> '[email protected]' 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 <[email protected]>
> 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\@[email protected]. 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.
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/admin/lists/[email protected]

Reply via email to