On Fri, Jun 03, 2016 at 02:56:08PM +0200, Jakub Hrozek wrote:
> On Fri, May 20, 2016 at 09:13:29PM +0200, Sumit Bose wrote:
> > Hi,
> >
> > this set of patches should resolve
> >
https://fedorahosted.org/sssd/ticket/2897 "Smart Cards: Certificate in
> > the ID View" and cover all other use cases from
> >
https://fedorahosted.org/sssd/wiki/DesignDocs/LookupUsersByCertificatePart2
> > as well. So basically certificates can be read from IPA and local
> > overrides and from AD with direct in indirect integration.
> >
> > The patches are all about lookups, so Smartcards and authentication is
> > not needed to test them. All is needed is a certificate which can be
> > added to an AD user object or an override object and then try to lookup
> > the user with
> >
> > dbus-send --system --print-reply --dest=org.freedesktop.sssd.infopipe \
> > /org/freedesktop/sssd/infopipe/Users \
> > org.freedesktop.sssd.infopipe.Users.FindByCertificate
> > string:"BASE64_CERTIFICATE_STRING"
> >
> > from a IPA client, IPA server or AD client with AD provier.
> >
> > If the certificate is store in the AD user object and the lookup is
> > started on an IPA client a patch for the IPA server is needed, because
> > the request has to run via the extdom plugin. I'll send a patch to
> > freeipa-devel which will use the sss_nss_getnamebycert() call added by
> > one of the patches to allow the extdom plugin to do lookups by
> > certificate. This means that SSSD on the IPA server must used the
> > attached patches as well.
> >
> > bye,
> > Sumit
>
> Hi,
>
> so far I read the patches, see comments inline. I haven't tested them
> yet, feel free to postpone sending new patches until the final ack/nack.
Thank you for reviewing and testing.
>
> > From b081fc781a4bb22f3fdc6200256086f6b2cb811f Mon Sep 17 00:00:00 2001
> > From: Sumit Bose <sbose(a)redhat.com>
> > Date: Wed, 6 Apr 2016 11:12:30 +0200
> > Subject: [PATCH 02/12] sysdb: add searches by certificate with overrides
> > + /* If there are views we have to check if override values must be added
to
> > + * the original object. */
> > + if (DOM_HAS_VIEWS(domain) && orig_obj->count == 1) {
> > + ret = sysdb_add_overrides_to_object(domain, orig_obj->msgs[0],
> > + override_obj == NULL ? NULL :
override_obj->msgs[0],
> > + NULL);
>
> As a style nitpick, I would prefer to use:
> if (ret == ENOENT) {
> } else if (ret != EOK) {
> }
fixed
>
> > + if (ret != EOK && ret != ENOENT) {
> > + DEBUG(SSSDBG_OP_FAILURE, "sysdb_add_overrides_to_object
failed.\n");
> > + goto done;
> > + }
>
>
> > From 2a2a021aa6ff9f3651c389201292c4066af4319d Mon Sep 17 00:00:00 2001
> > From: Sumit Bose <sbose(a)redhat.com>
> > Date: Fri, 8 Apr 2016 13:22:24 +0200
> > Subject: [PATCH 09/12] sss_override: add certificate support
> >
> > ---
> > src/db/sysdb.h | 1 +
> > src/tests/intg/ldap_local_override_test.py | 8 +++----
> > src/tools/sss_override.c | 38
++++++++++++++++++++++++++----
>
> It would be nice to amend the sss_override manpage, too.
fixed
>
>
> > From d564862024bb208614b2f3b547d99b72a9787a45 Mon Sep 17 00:00:00 2001
> > From: Sumit Bose <sbose(a)redhat.com>
> > Date: Mon, 25 Apr 2016 16:09:48 +0200
> > Subject: [PATCH 11/12] NSS: add SSS_NSS_GETNAMEBYCERT request
>
> > +static void ifp_users_find_by_cert_done(struct tevent_req *req);
>
> I guess using the ifp prefix here is a copy-and-paste error?
fixed
>
>
> > From f7647dbf154ce5cb082b391269f9b674ca47e712 Mon Sep 17 00:00:00 2001
> > From: Sumit Bose <sbose(a)redhat.com>
> > Date: Tue, 26 Apr 2016 13:13:43 +0200
> > Subject: [PATCH 12/12] nss-idmap: add sss_nss_getnamebycert()
> >
> > ---
> > Makefile.am | 2 +-
> > src/python/pysss_nss_idmap.c | 47
++++++++++++++++++++++++++++--
> > src/responder/nss/nsssrv_cmd.c | 1 +
> > src/sss_client/idmap/sss_nss_idmap.c | 26 ++++++++++++++++-
> > src/sss_client/idmap/sss_nss_idmap.exports | 6 ++++
> > src/sss_client/idmap/sss_nss_idmap.h | 15 ++++++++++
> > 6 files changed, 93 insertions(+), 4 deletions(-)
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index
d20a10fa61f8a2fc296f839318503960382a9879..58255b0dc766e3755d70c12ee312a9aa52d6a724 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -989,7 +989,7 @@ libsss_nss_idmap_la_LIBADD = \
> > $(CLIENT_LIBS)
> > libsss_nss_idmap_la_LDFLAGS = \
> > -Wl,--version-script,$(srcdir)/src/sss_client/idmap/sss_nss_idmap.exports
\
> > - -version-info 1:0:1
> > + -version-info 2:0:2
>
> The change itself is good, do we expect this is the final change to the
> library in this release?
I would says yes.
New version attached.