On (06/06/13 20:11), Lukas Slebodnik wrote:
>On (05/06/13 14:50), Jakub Hrozek wrote:
>>On Thu, Oct 11, 2012 at 09:39:41AM -0400, Simo Sorce wrote:
>>> On Thu, 2012-10-11 at 13:14 +0200, Jakub Hrozek wrote:
>>> >
>>> > Hi,
>>> >
>>> > the attached patch splits the previously monolithic sssd package into
>>> > sssd-common that contains the deamon and the responders and
>>> > per-provider
>>> > packages such as sssd-ldap or sssd-ipa.
>>> >
>>> > This split would benefit two parties:
>>> > 1) security auditors who are often trying to find the smallest
>>> > package
>>> > set including dependencies needed for the package to function.
>>> > They
>>> > would be able to i.e. install sssd-ldap and not bother about
>>> > sssd-ipa or sssd-ad pulling in more dependencies.
>>> > 2) 3rd party programs such as realmd or authconfig that would only
>>> > be able to require or install on demand the needed packages.
>>> >
>>> > The patch addresses
https://fedorahosted.org/sssd/ticket/1510 and must
>>> > b
>>> > applied on the two specfile patches I sent earlier (the thread subject
>>> > included libsss_sudo).
>>>
>>> Questions inline.
>>>
>>
>>Not even nine months after the initial submission, here comes a revised
>>patch. I remember we had a discussion on IRC with Simo about this
>>problem, but I'll reply to the thread.
>>
>>With the Radius provider patches on the list and requiring Samba bits in
>>the last couple of releases, I think that splitting the providers is
>>something we really should do.
>>
>>> >
>>> >
>>> >
>>> >
>>> >
>>> >
>>> > plain text
>>> > document
>>> > attachment
>>> > (0001-Split-the-providers-into-separate-subpackages.patch)
>>> >
>>> > From f59cfde30777a2c46f0ba2d6bd57dff62561851f Mon Sep 17 00:00:00 2001
>>> > From: Jakub Hrozek <jhrozek(a)redhat.com>
>>> > Date: Fri, 28 Sep 2012 09:21:18 +0200
>>> > Subject: [PATCH] Split the providers into separate subpackages
>>> >
>>> > ---
>>> > contrib/sssd.spec.in | 145
>>> > ++++++++++++++++++++++++++++++++++++++++-----------
>>> > 1 file changed, 115 insertions(+), 30 deletions(-)
>>> >
>>> > diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in
>>> > index
>>> >
e194245d166c7dee2f1988019b414e5fb47df2de..9b5a9b475544d245fbad0cbdd056ab55a0df4437 100644
>>> > --- a/contrib/sssd.spec.in
>>> > +++ b/contrib/sssd.spec.in
>>> > @@ -45,17 +45,13 @@ BuildRoot: %(mktemp -ud
>>> > %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)
>>> > Patch0001: sssd-1.9-man-change-default-ccache.patch
>>> >
>>> > ### Dependencies ###
>>> > -
>>> > -Requires: libldb >= 0.9.3
>>> > -Requires: libtdb >= 1.1.3
>>> > +Conflicts: sssd < %{version}-%{release}
>>> > Requires: sssd-client%{?_isa} = %{version}-%{release}
>>> > -Requires: libipa_hbac = %{version}-%{release}
>>> > -Requires: libsss_idmap = %{version}-%{release}
>>> > -Requires: cyrus-sasl-gssapi
>>> > -Requires: keyutils-libs
>>> > -Requires(post): initscripts chkconfig
>>> > -Requires(preun): initscripts chkconfig
>>> > -Requires(postun): initscripts chkconfig
>>> > +Requires: sssd-common = %{version}-%{release}
>>> > +Requires: sssd-ldap = %{version}-%{release}
>>> > +Requires: sssd-krb5 = %{version}-%{release}
>>> > +Requires: sssd-ipa = %{version}-%{release}
>>> > +Requires: sssd-ad = %{version}-%{release}
>>>
>>>
>>> Doesn't this set of requires makes the split useless ?
>>> If I read it correctly it means sssd will require all subpackages anyway
>>> so you cannot pick and choose to install only one as you say the purpose
>>> is in the mail message.
>>>
>>
>>The intent of the sssd package requiring all the dependencies is to make sure
>>that any kickstart that specified "sssd" would get the whole set,
because
>>we can't currently know what functionality and which provider was used.
>>
>>To pick the minimal set for LDAP, you can run:
>># yum install sssd-ldap
>>for instance.
>>
>>> > %global servicename sssd
>>> > %global sssdstatedir %{_localstatedir}/lib/sss
>>> > @@ -126,6 +122,21 @@ the system and a pluggable backend system to
>>> > connect to multiple different
>>> > account sources. It is also the basis to provide client auditing and
>>> > policy
>>> > services for projects like FreeIPA.
>>> >
>>> > +%package common
>>> > +Summary: Common files for the SSSD
>>> > +Group: Applications/System
>>> > +License: GPLv3+
>>> > +Requires: libldb >= 0.9.3
>>> > +Requires: libtdb >= 1.1.3
>>> > +Requires: sssd-client%{?_isa} = %{version}-%{release}
>>> > +Requires(post): initscripts chkconfig
>>> > +Requires(preun): initscripts chkconfig
>>> > +Requires(postun): initscripts chkconfig
>>> > +Conflicts: sssd < %{version}-%{release}
>>> > +
>>> > +%description common
>>> > +Common files for the SSSD.
>>> > +
>>> > %package client
>>> > Summary: SSSD Client libraries for NSS and PAM
>>> > Group: Applications/System
>>> > @@ -141,7 +152,7 @@ service.
>>> > Summary: Userspace tools for use with the SSSD
>>> > Group: Applications/System
>>> > License: GPLv3+
>>> > -Requires: sssd = %{version}-%{release}
>>> > +Requires: sssd-common = %{version}-%{release}
>>> >
>>> > %description tools
>>> > Provides userspace tools for manipulating users, groups, and nested
>>> > groups in
>>> > @@ -153,6 +164,61 @@ Also provides several other administrative tools:
>>> > * sss_seed which pre-creates a user entry for use in kickstarts
>>> > * sss_obfuscate for generating an obfuscated LDAP password
>>> >
>>> > +%package ldap
>>> > +Summary: The LDAP back end of the SSSD
>>> > +Group: Applications/System
>>> > +License: GPLv3+
>>> > +Conflicts: sssd < %{version}-%{release}
>>> > +Requires: cyrus-sasl-gssapi
>>> > +Requires: sssd-common = %{version}-%{release}
>>> > +Requires: libsss_idmap = %{version}-%{release}
>>> > +
>>> > +%description ldap
>>> > +Provides the LDAP back end that the SSSD can utilize to fetch
>>> > identity data
>>> > +from and authenticate against an LDAP server.
>>> > +
>>> > +%package krb5
>>> > +Summary: The Kerberos authentication back end for the SSSD
>>> > +Group: Applications/System
>>> > +License: GPLv3+
>>> > +Conflicts: sssd < %{version}-%{release}
>>> > +Requires: cyrus-sasl-gssapi
>>> > +Requires: sssd-common = %{version}-%{release}
>>> > +
>>> > +%description krb5
>>> > +Provides the Kerberos back end that the SSSD can utilize authenticate
>>> > +against a Kerberos server.
>>> > +
>>> > +%package ipa
>>> > +Summary: The IPA back end of the SSSD
>>> > +Group: Applications/System
>>> > +License: GPLv3+
>>> > +Conflicts: sssd < %{version}-%{release}
>>> > +Requires: sssd-common = %{version}-%{release}
>>> > +Requires: sssd-ldap = %{version}-%{release}
>>> > +Requires: sssd-krb5 = %{version}-%{release}
>>> > +Requires: libipa_hbac = %{version}-%{release}
>>> > +Requires: libsss_idmap = %{version}-%{release}
>>> > +Requires: bind-utils
>>>
>>> Does the ipa provider really need the sssd-ldap and sssd-krb5
>>> subpackages ?
>>> IIRC we statically compile the ldap and krb5 packages bits we need in
>>> the ipa provider.
>>> If you change this you probably want a require on cyrus-sasl-gssapi
>>> here.
>>>
>>> (if it is just for the ldap and krb child processes shouldn't we simply
>>> keep those binaries in the sssd or sssd-common package ?)
>>
>>Yes, the intent was to make sure the ldap child and krb5 child processes
>>are pulled in. But now that we switched to internal shared libraries, I
>>think a better solution is to have the krb5_common internal shared
>>library along with the ldap and krb5 child in a subpackage of its own
>>and let the Kerberos-aware providers pull these in.
>>
>>>
>>>
>>> > +%description ipa
>>> > +Provides the IPA back end that the SSSD can utilize to fetch identity
>>> > data
>>> > +from and authenticate against an IPA server.
>>> > +
>>> > +%package ad
>>> > +Summary: The AD back end of the SSSD
>>> > +Group: Applications/System
>>> > +License: GPLv3+
>>> > +Conflicts: sssd < %{version}-%{release}
>>> > +Requires: sssd-common = %{version}-%{release}
>>> > +Requires: sssd-ldap = %{version}-%{release}
>>> > +Requires: sssd-krb5 = %{version}-%{release}
>>> > +Requires: libsss_idmap = %{version}-%{release}
>>>
>>> SAme questions as for the ipa subpackage
>>>
>>> > +%description ad
>>> > +Provides the Active Directory back end that the SSSD can utilize to
>>> > fetch
>>> > +identity data from and authenticate against an Active Directory
>>> > server.
>>> > +
>>> > %package -n libsss_idmap
>>> > Summary: FreeIPA Idmap library
>>> > Group: Development/Libraries
>>> > @@ -205,7 +271,7 @@ used by Python applications.
>>> > Summary: A library to allow communication between SUDO and SSSD
>>> > Group: Development/Libraries
>>> > License: LGPLv3+
>>> > -Requires: sssd = %{version}-%{release}
>>> > +Requires: sssd-ldap = %{version}-%{release}
>>> > Requires(post): /sbin/ldconfig
>>> > Requires(postun): /sbin/ldconfig
>>>
>>> why libsss_idmap would require the sssd-ldap subpakage ?
>>
>>I think this was a mass-replace bug, fixed.
>>
>>>
>>> Simo.
>>
>>There are also two patches preceding the one that splits the providers:
>>
>>[PATCH 1/3] rpm: Fold libsss_sudo and libsss_autofs back into the main SSSD
package
>>https://fedorahosted.org/sssd/ticket/1845
>>
>>libsss_sudo and libsss_autofs are separate packages that contain just a
>>single client library with no additional dependencies. This separation
>>comes from the F-17 timeframe where the feature was really just a tech
>>preview so we didn't want it to be packaged in sssd proper. On the other
>>hand users are getting regularly confused about "sudo not working"
when
>>all they really miss is the single library.
>>
>>This patch moves the files owned by the libsss_autofs and libsss_sudo
>>packages back to the main sssd package. We also no longer build the
>>libsss_sudo documentation by default and do not ship the header file as
>>it was just a private one.
>>
>
>I tested upgrade with installed freeipa-client and sssd
>and upgrade worked as expected.
>
>ACK
>
>>
>>
>>[PATCH 2/3] rpm: Use hardened flags for RPM build
>>https://fedorahosted.org/sssd/ticket/1797
>>
>>This patch adds relro and bind_now linker flags to produce hardened
>>binaries. The change amounts to adding "-Wl,-z,now".
>
>Rpm packages are built without problems and Stephen blessed this patch.
>ACK
Second patch NACK.
+export CFLAGS="%{optflags} -Wl,-z,now"
^^^^^^^^^^
This is linker argument and should not be among CFLAGS