On Fri, Dec 05, 2014 at 04:48:47PM +0100, Lukas Slebodnik wrote:
> On (05/12/14 16:22), Jakub Hrozek wrote:
> >On Mon, Nov 24, 2014 at 02:14:27PM +0100, Lukas Slebodnik wrote:
> >> On (24/11/14 10:47), Jakub Hrozek wrote:
> >> >On Mon, Nov 24, 2014 at 09:51:58AM +0100, Lukas Slebodnik wrote:
> >> >> On (22/11/14 17:25), Jakub Hrozek wrote:
> >> >> >On Sat, Nov 22, 2014 at 05:12:09PM +0100, Jakub Hrozek wrote:
> >> >> >> On Tue, Nov 18, 2014 at 12:25:40PM +0100, Lukas Slebodnik
wrote:
> >> >> >> > On (18/11/14 12:22), Lukas Slebodnik wrote:
> >> >> >> > >On (13/11/14 10:08), Simo Sorce wrote:
> >> >> >> > >>On Thu, 13 Nov 2014 10:44:45 +0100
> >> >> >> > >>Jakub Hrozek <jhrozek(a)redhat.com>
wrote:
> >> >> >> > >>
> >> >> >> > >>> On Wed, Nov 12, 2014 at 08:04:46PM
-0500, Simo Sorce wrote:
> >> >> >> > >>> > On Wed, 12 Nov 2014 16:36:00
+0100
> >> >> >> > >>> > Lukas Slebodnik
<lslebodn(a)redhat.com> wrote:
> >> >> >> > >>> >
> >> >> >> > >>> > > On (12/11/14 10:00), Simo
Sorce wrote:
> >> >> >> > >>> > > >I would create a helper
function to be called on return that
> >> >> >> > >>> > > >transforms the error
accordingly. This will allow to write the
> >> >> >> > >>> > > >code _and_ the comment
once.
> >> >> >> > >>> > > >
> >> >> >> > >>> > > In this case, Stephan's
patch is better
> >> >> >> > >>> > >
https://bugzilla.redhat.com/attachment.cgi?id=788567
> >> >> >> > >>> >
> >> >> >> > >>> > Yes, this is a valid alternative.
> >> >> >> > >>> >
> >> >> >> > >>> > > >The comment should be
changed to something like this in either
> >> >> >> > >>> > > >case: /* When sssd is
stopped return a safe error code as if sss
> >> >> >> > >>> > > >was not configured at all
in nsswitch. This prevents bogus
> >> >> >> > >>> > > >errors from causing
issues in applications, before sssd starts
> >> >> >> > >>> > > >or if it fails to
respond. */
> >> >> >> > >>> > > >
> >> >> >> > >>> > > >No need to mention that
sss is by default in nsswitch, as it is
> >> >> >> > >>> > > >not in all distributions
and it really is inconsequential, the
> >> >> >> > >>> > > >same behaviour change
hleps when sss is not the default but is
> >> >> >> > >>> > > >has been manually added
and sssd is stopped or not started yet
> >> >> >> > >>> > > >(for example during
boot).
> >> >> >> > >>> > > nss-pam-ldapd has the same
behaviour in the same situation.
> >> >> >> > >>> > > Will we patch it as well?
It's very likely we won't.
> >> >> >> > >>> >
> >> >> >> > >>> > Sorry, I do not see how that
matters :)
> >> >> >> > >>> >
> >> >> >> > >>> > > The biggest problem is that
sss is by default in nsswitch on
> >> >> >> > >>> > > fedora/rhel>=7 due to
glibc caching and problem with GNOME,
> >> >> >> > >>> > > a) sssd-client is installed
by default on this platforms.
> >> >> >> > >>> > > b) sssd need't be
configured by default and in most cases won't be
> >> >> >> > >>> > > => sssd cannot run
> >> >> >> > >>> > > c) glibc developers don't
want to adjust the error return code in
> >> >> >> > >>> > > glibc
> >> >> >> > >>> > >
> >> >> >> > >>> > > As a result of this, we need
to patch sssd.
> >> >> >> > >>> > > I would say we should patch
sssd just in downstream and
> >> >> >> > >>> > > Stephan's patch works
well. I tested it.
> >> >> >> > >>> >
> >> >> >> > >>> > Ok, then let's go with
Stephen's patch.
> >> >> >> > >>> >
> >> >> >> > >>> > Simo.
> >> >> >> > >>> >
> >> >> >> > >>>
> >> >> >> > >>> I don't personally mind whether we
use Lukas' or Stephen's patch but I
> >> >> >> > >>> would /strongly/ prefer the patch to be
pushed upstream.
> >> >> >> > >>>
> >> >> >> > >>> Non-upstream patches should be
considered valid only as a temporary
> >> >> >> > >>> workarounds..
> >> >> >> > >>
> >> >> >> > >>I was not aware the patch was not proposed
for upstream. I strongly
> >> >> >> > >>disagree on this kind of patches being
platform specific too.
> >> >> >> > >>
> >> >> >> > >New version of patch is attached.
> >> >> >> > >
> >> >> >> > Ups, one more time.
> >> >> >> >
> >> >> >> > LS
> >> >> >>
> >> >> >> > From eb617a980d50c7081ecb6eda0d489fc0eb8963f7 Mon
Sep 17 00:00:00 2001
> >> >> >> > From: Lukas Slebodnik <lslebodn(a)redhat.com>
> >> >> >> > Date: Tue, 18 Nov 2014 12:02:10 +0100
> >> >> >> > Subject: [PATCH] sss_client: Work around glibc bug
> >> >> >> >
> >> >> >> > glibc is inconsistent with how it treats and returns
NSS_STATUS_UNAVAIL.
> >> >> >> >
> >> >> >> > The sss nss plugin is present in nsswitch by default
on some platforms
> >> >> >> > due to glibc caching and problem with long living
applications (e.g. GNOME).
> >> >> >> > The SSSD nss plugin should behave as if it was
functioning but had no data even
> >> >> >> > thought sssd is not running. The errors have to be
passed from nss plugin up to
> >> >> >> > the user with minimal moidiffication.
> >> >> >> >
> >> >> >> > Thanks to Stephen Gallagher for initial patch.
> >> >> >> >
> >> >> >> > Resolves:
> >> >> >> >
https://fedorahosted.org/sssd/ticket/2439
> >> >> >> > ---
> >> >> >> > src/conf_macros.m4 | 13 +++++++++++++
> >> >> >> > src/sss_client/common.c | 10 ++++++++++
> >> >> >> > 2 files changed, 23 insertions(+)
> >> >> >> >
> >> >> >> > diff --git a/src/conf_macros.m4
b/src/conf_macros.m4
> >> >> >> > index
fbee81f56e484b618379f7c987ecee50ae48917e..bb283ef248ee017fd608bd85cc68666db0300fd9 100644
> >> >> >> > --- a/src/conf_macros.m4
> >> >> >> > +++ b/src/conf_macros.m4
> >> >> >> > @@ -708,6 +708,19 @@ AC_ARG_ENABLE([dbus-tests],
> >> >> >> > [build_dbus_tests=yes])
> >> >> >> > AM_CONDITIONAL([BUILD_DBUS_TESTS], [test
x$build_dbus_tests = xyes])
> >> >> >> >
> >> >> >> > +AC_ARG_ENABLE([sss-default-nss-plugin],
> >> >> >> > +
[AS_HELP_STRING([--enable-sss-default-nss-plugin],
> >> >> >> > + [This option change
standard behaviour of sss nss
> >> >> >> > + plugin. If this
option is enabled the sss nss
> >> >> >> > + plugin will behave
as it was not in
> >> >> >> > + nsswitch.conf when
sssd is not running.
> >> >> >> > + [default=no]])],
> >> >> >> > + [],
> >> >> >>
> >> >> >> Didn't you mean to add:
> >> >> >> enable_sss-default-nss-plugin=$enablevar
> >> >> >>
> >> >> >> here?
> >> >> >>
> >> >> >> Additionally, it would be nice to explain in the commit
message why the
> >> >> >> feature is disabled -- explain the effect enabling by
default would have
> >> >> >> on searches that don't find anything.
> >> >> >
> >> >> >Actually, you also need to convert dashes to underscores in
the variable
> >> >> >name. As the autoconf docs say:
> >> >> > The option's argument is available to the shell
commands
> >> >> > action-if-given in the shell variable enableval, which is
actually
> >> >> > just the value of the shell variable named enable_feature,
with any
> >> >> > non-alphanumeric characters in feature changed into ‘_’.
> >> >> >
> >> >> >This change did the trick for me and I was able to test the
return value
> >> >> >in gdb.
> >> >> In gdb? really
> >> >> Could you ellaborate?
> >> >
> >> >Once I fixed the config option so the build-time switch actually
worked:
> >> >
> >> I thought you was able to see value of bash variable
> >> enable_sss-default-nss-plugin :-)
> >>
> >> New version is attached.
> >>
> >> LS
> >
> >Thank you, the code looks good. Were you able to reproduce the exact
> >same error as the user saw, though?
> >
> >I tested with "files sss ldap" in nsswitch.conf and SSSD was off.
Even
> >without the patch, getent group $ipagroup was able to fall back to
> >ldap..so I guess I must be missing something.
>
> Current version of sssd nsswitch plugin return error codeas as it is specified
> in glibc documentation. ldap nsswitch plugin does the same.
> So if sssd(nslcd) is stopped then sssd(ldap) plugin will return
> NSS_STATUS_UNAVAIL.
>
> The problem is with some distributions where sss is by default in
> /etc/nsswitch.conf and sssd is not configured thus cannot be running.
> The purpose of this patch is that sss plugin should behave as it wasn't in
> nsswitch.conf.
>
> So you should not test with ldap plugin, because ldap is not by default in
> nsswitch.conf
OK, I reread the private bugzilla and realized where I got the idea of
three modules from..the reporter was actually using that as a
workaround.
>
> So the test shoudl be:
> a)
> passwd: files sss
> && sssd service should be stopped
> b)
> passwd: files
>
> and behaviour should be the same.
> The C reprodurer is in this thread, public BZ [1] an private BZ[2]
That helped, thank you.
Since noone else commented on the configure-time switch, I'm ACKing this
patch.