[Freeipa-devel] [PATCH] 0156 extdom: add certificate request

[Lukas Slebodnik]

On (22/06/16 11:57), Martin Basti wrote:
>
>
>On 09.06.2016 21:02, Martin Basti wrote:
>> 
>> 
>> On 09.06.2016 14:45, Martin Basti wrote:
>> > 
>> > 
>> > On 09.06.2016 14:42, Martin Basti wrote:
>> > > 
>> > > 
>> > > On 09.06.2016 14:38, Lukas Slebodnik wrote:
>> > > > On (09/06/16 14:29), Martin Basti wrote:
>> > > > > On 09.06.2016 14:22, Alexander Bokovoy wrote:
>> > > > > > On Thu, 09 Jun 2016, Jakub Hrozek wrote:
>> > > > > > > On Fri, May 20, 2016 at 09:23:46PM +0200, Sumit Bose wrote:
>> > > > > > > > Hi,
>> > > > > > > > 
>> > > > > > > > this patch allows the extom plugin to lookup
>> > > > > > > > users by certificate which
>> > > > > > > > is needed in the case where a IPA client
>> > > > > > > > wants to lookup an AD user who
>> > > > > > > > has the certificate stored in AD. To make
>> > > > > > > > this work the related patches
>> > > > > > > > I just send to sssd-devel are needed as well.
>> > > > > > > > 
>> > > > > > > > Currently the patches miss the change in the
>> > > > > > > > required version of SSSD.
>> > > > > > > > since the SSSD patches are not committed. But
>> > > > > > > > the patches are needed to
>> > > > > > > > fully test the SSSD patches. I will send a
>> > > > > > > > new version with the needed
>> > > > > > > > changes to the minimal SSSD version when the SSSD patches are
>> > > > > > > > committed.
>> > > > > > > > 
>> > > > > > > > bye,
>> > > > > > > > Sumit
>> > > > > > > The patch works fine (tested together with the corresponding SSSD
>> > > > > > > patches), so ACK from me. The code also looks
>> > > > > > > good to me, but I'm not
>> > > > > > > sure if reviewing an IPA patch requires something
>> > > > > > > more (CI? Coverity?)
>> > > > > > ACK from me as well, I forgot to send email about it,
>> > > > > > though I reviewed
>> > > > > > this patch a week ago.
>> > > > > > 
>> > > > > Pushed to master: aa734da49440c5d12c0f8d4566505adaeef254e8
>> > > > > 
>> > > > It's very likey that this commit will break build of
>> > > > freeipa-master. I didn't try.
>> > > > 
>> > > > Because it uses new function sss_nss_getnamebycert
>> > > > from the library libsss_nss_idmap which is not in fedora.
>> > > > It was pushed to sssd master just today.
>> > > > 
>> > > > LS
>> > > 
>> > > If this is true, can you/somebody provide the SRPM of SSSD with
>> > > the required functionality please? We may need to add it to
>> > > @freeipa/freeipa-master copr and bump required version of SSSD.
>> > > 
>> > > Martin^2
>> > > 
>> > 
>> > Yes, you were right, master build is broken.
>> > Martin^2
>> > 
>> 
>> SSSD master build has been added to @freeipa/freeipa-master copr as a
>> workaround (to unblock automatic testing an developers)
>> 
>> Please bump version in specfile accordingly (I don't know in which
>> version of SSSD will be required function)
>> 
>> Martin^2
>> 
>Bumping SSSD version in requires and buildrequires
>Patch attached

>From f2b394085157954768bc93a73b854778c65bfdcd Mon Sep 17 00:00:00 2001
>From: Martin Basti <mbasti at redhat.com>
>Date: Wed, 22 Jun 2016 10:49:39 +0200
>Subject: [PATCH] Bump SSSD requires
>
>https://fedorahosted.org/freeipa/ticket/4955
>---
> freeipa.spec.in | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/freeipa.spec.in b/freeipa.spec.in
>index 0d5c745d5306cd7141c573454bd1c1e6a78c7e7f..befc7af9ee2ceefa41b1b999df4bdb1c6607bea8 100644
>--- a/freeipa.spec.in
>+++ b/freeipa.spec.in
>@@ -85,7 +85,7 @@ BuildRequires:  python-pyasn1 >= 0.0.9a
> BuildRequires:  python-qrcode-core >= 5.0.0
> BuildRequires:  python-dns >= 1.11.1
> BuildRequires:  libsss_idmap-devel
>-BuildRequires:  libsss_nss_idmap-devel >= 1.12.2
>+BuildRequires:  libsss_nss_idmap-devel >= 1.14.0
> BuildRequires:  java-headless
> BuildRequires:  rhino
> BuildRequires:  libverto-devel
>@@ -327,7 +327,7 @@ Requires: pam_krb5
> Requires: curl
> Requires: libcurl >= 7.21.7-2
> Requires: xmlrpc-c >= 1.27.4
>-Requires: sssd >= 1.13.3-5
>+Requires: sssd >= 1.14.0
NACK

A) It's not explained in commit message why you need to bump Requires for sssd.
   IIRC, you need just new libsss_nss_idmap-devel.
B) You forgot add detection for newer version of libsss_nss_idmap at configure
   time
Hint: * daemons/configure.ac
      * https://autotools.io/pkgconfig/pkg_check_modules.html#pkgconfig.pkg_check_modules.specification

LS