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 <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.
CI:
http://sssd-ci.duckdns.org/logs/job/50/40/summary.html
* master:
* 0d5d490fb5ec685fd8ef7a75e612e6ec7ef6bde3
* 83a796ec8de4bde65b11cc8032675406950641fa
* 78677495a7762469002b0976809fa20ac2196f42
* ba9ebfc49ab3bacb96213c8620411128c09f39da
* 91767924bdf9b5a28e8902206a40348d6c83a139
* 04d4c4d45f3942a813b7f772737f801f877f4e64
* 9a310913d696d190db14c625080678db853a33fd
* 447b1da857368678990b54cd6b9cfed940357c44
* 3381d9736b698d6111d10e219a0b5b898a4c757c
* 62df78512145db94b51c5573d4df1737197e368a
* 9b8fcf685c5ca70a5067a621385bcdc8d9fd6469
* 15694ca762f61a414f0017c57ed97a8d57456b80
* 50a7a92f92e1584702bf25e61a50cb1c09c7e260