Subject: Bug#1458: unattended Debian installations (using preseed) fail when x2gobroker-ssh is involved

From: Stefan Baur <[email-masked]>
Date: Fri, 17 Apr 2020 16:06:27 +0200

package: x2gobroker-ssh
version 0.0.4.1
severity: important

The postinst file for x2gobroker-ssh contains a check to see if the nscd
binary is somewhere in the path.  If it is, then nscd -i group is
called, to purge the group cache (which makes sense, as a new group has
been added in a previous step, so the cached information is outdated).
If it is not, then it is assumed that nscd is not installed, thus, no
group information is cached, and thus, no purge is needed.

This fails to take into account that nscd might be (freshly) installed,
but not running.  Which is the case during a preseeded Debian
installation (and probably when running debootstrap as well).

The result is that "nscd -i group" is called even though nscd isn't
active at the moment.

Which triggers a return code of "1".

Which in turn causes the postinst script to report "1" as its return code.

This, of course, gets passed back up to the debian-installer, which
decides to abort the installation, as it seems that something went wrong
during installation of the package.

The patch is rather simple: Instead of checking for the presence of the
executable in the file system, check if it is currently running:

-                                        if which nscd 1>/dev/null; then
+                                        if ps -C nscd 1>/dev/null; then

As this is currently breaking all automated x2gobroker-ssh installs, I
would suggest releasing a new version containing only this patch ASAP,
if a full release with all changes since the last proper release of
x2gobroker-ssh would bring too many changes at once for a timely release.

Kind Regards,
Stefan Baur

-- 
BAUR-ITCS UG (haftungsbeschränkt)
Geschäftsführer: Stefan Baur
Eichenäckerweg 10, 89081 Ulm | Registergericht Ulm, HRB 724364
Fon/Fax 0731 40 34 66-36/-35 | USt-IdNr.: DE268653243

---

From: Ulrich Sibiller <[email-masked]>
Date: Fri, 17 Apr 2020 16:24:57 +0200

I think it is a totally wrong approach to fiddle with nscd. Creating a
group using system tools should take of that already. If not it's a
bug, I'd say.

Besides: what about sssd that can also cache groups?

On Fri, Apr 17, 2020 at 4:11 PM Stefan Baur <[email-masked]> wrote:
> _______________________________________________
> x2go-dev mailing list
> [email-masked]
> https://lists.x2go.org/listinfo/x2go-dev

---

From: Stefan Baur <[email-masked]>
Date: Fri, 17 Apr 2020 16:45:06 +0200

> group using system tools should take of that already. If not it's a

LOL.  nscd caching the wrong(TM) things at the wrong(TM) time is an
issue that's probably as old as Unix (or at least nscd) itself.  If you
take a look at the postinst script in question, you will see that it
does, in fact, use the system tools to add the group.  Still, it is
neccesary to flush the cache or things have a tendency to go wrong.

> Besides: what about sssd that can also cache groups?

That's a more interesting question, and we might have to add a check for
it as well.  But as of right now, sssd being installed in combination
with x2gobroker-ssh during a preseeded installation won't break anything.

-Stefan

---

From: Ulrich Sibiller <[email-masked]>
Date: Fri, 17 Apr 2020 20:02:31 +0200

> neccesary to flush the cache or things have a tendency to go wrong.

Well, if you go that route there are more things to take into account:
- is nscd properly configured to cache groups at all?
- is there a distro-tool available for configuring/flushing/handling nscd
- are the multiple versions of nscd around? Which one to take?
- probably more
- what happens if a newer version of nscd is around that needs to be
called otherwise
- waht happens if the nscd binary is something completely different
and just happens to have the same name?
- same for ssd
- same for ANY other caching mechanism you might not even know

All these things tend to break sooner or later. That's the reason why
you should not do this in an installation script but report a bug
instead. This must be fixed at distro level.

Are you aware of any installation postscripts other than x2go that
handle nscd problems?

> with x2gobroker-ssh during a preseeded installation won't break anything.

I have seen the weirdest problems with sssd (and nscd as well). An I
still have one bug open at redhat for more  than year which redhat has
not fixed yet...

While you can work around such problems in local (site) scripts or as
local administrator you should NOT include such workaround in release
packages.

Uli

---

From: Stefan Baur <[email-masked]>
Date: Fri, 17 Apr 2020 20:33:14 +0200

Uli,

let me make this clear, in case you missed the "severity: important" tag
I gave this bug:

Currently, all automated Debian installs for x2gobroker-ssh are failing
due to this faulty piece of code.

Even our own demo install scripts we have in the Wiki, for the newbies
that want to try out X2GoBroker without having to figure out how to
configure this hairy beast manually, are broken.

This is NOT GOOD.

At the same time, "when in doubt, dike it out" is not a feasible
approach, as this code has been put there for a reason - and we can
assume it was added after things broke while testing a manual install.

So if we remove it, we're likely to get another bug report tagged
"severity: important", just coming from the other direction, begging us
to add this code again.

That is why our only option right now is to deploy this patch into
stable ASAP, to make the code do what it is supposed to do - flush the
cache IF nscd is RUNNING, and ONLY THEN.

No one is claiming that this is a magic cure-all for every problem there
may be with nscd or sssd caching.

But it WILL fix an actual issue we have RIGHT NOW and which is blocking
users from deploying x2gobroker-ssh in an automated way.  AND it will
NOT make things WORSE for anyone else.

After the release, we can re-open this bug and downgrade its severity as
a reminder that this caching issue should be investigated further.
But we won't be under pressure to get something working again that
worked before like we are right now.

-Stefan

---

From: Ulrich Sibiller <[email-masked]>
Date: Fri, 17 Apr 2020 21:59:59 +0200

> let me make this clear, in case you missed the "severity: important" tag

Yes, I did.

> due to this faulty piece of code.

So, let's make this clear: Code, which should not be there at all ist
now breaking installation. That's exactly my point... It should not be
there.

I have just checked on my Debian 10. Here's a snipped from /usr/sbin/groupadd:
...
    printf (gtx("Adding group `%s' (GID %d)
...\n"),$new_name,$new_gid) if $verbose;
    &invalidate_nscd("group");
    my $groupadd = &which('groupadd');
    &systemcall($groupadd, '-g', $new_gid, $new_name);
    &invalidate_nscd("group");
    print (gtx("Done.\n")) if $verbose;
...

So this is where this kind of code belongs to. Debian has everything in place.

So I agree that this should be fixed asap in x2gobroker-ssh, but it
should be fixed by removing the nscd code altogether.

> "severity: important", just coming from the other direction, begging us

No, those people begging for it can be instructed to check their
distro. And to run the flush manually. Or even to simply reboot.

> may be with nscd or sssd caching.

I see it - as this bug report proves - as an unneccessary source for problems.

Uli

---

From: Stefan Baur <[email-masked]>
Date: Sun, 19 Apr 2020 15:30:45 +0200

> now breaking installation. That's exactly my point... It should not be

Code that has been put there to solve an actual issue, but which has a
flaw in its detection routine as to when it should trigger.

> So this is where this kind of code belongs to. Debian has everything in place.

And it seems to be broken, or this code in the postinst script wouldn't
be needed.  Or maybe it was only added to groupadd after the code in the
postinst file had already been written.  It would be interesting to know
when those "invalidate_nscd" calls were added.  Maybe an older Debian
version is still affected by this, while Buster is safe?

> distro. And to run the flush manually. Or even to simply reboot.

The proper way to handle this, IMO, is a two-step process.

1. Apply my patch ASAP and release a fixed package, to un-break
   current preseeded installations.
2. Turn the blind execution if the condition is triggered into an
   interactive, preseedable question, and check for sssd as well.
   This is what Debian does e.g. when updating libc - you get a popup
   asking you if it is okay to restart affected services, or if you
   would like to do it manually at a later date.
   So in addition to

x2gobroker-ssh x2gobroker-ssh/group-x2gobroker-users string x2gobroker-users
x2gobroker-ssh x2gobroker-ssh/group-does-not-exist note
x2gobroker-ssh x2gobroker-ssh/use-existing-group boolean true|false
x2gobroker-ssh x2gobroker-ssh/create-group boolean true|false
x2gobroker-ssh x2gobroker-ssh/manual-setup-required boolean true|false
x2gobroker-ssh x2gobroker-ssh/del-last-group-x2gobroker-users boolean
true|false
x2gobroker-ssh x2gobroker-ssh/last-group-x2gobroker-users string
x2gobroker-users

   we'd have new preseedable entries like

x2gobroker-ssh x2gobroker-ssh/flush-nscd-groups boolean true|false
x2gobroker-ssh x2gobroker-ssh/flush-sssd-groups boolean true|false

  Whoever tries to run an unattended installation using preseeds can
  then choose whichever option suits them best.
  And if you're worried that the attempt to flush the cache has a
  negative impact, even with the triggering condition fixed, we can make
  those options default to "false" and add a notice like "this should
  not be neccessary unless your groupadd implementation is faulty" in
  the popup message.

-Stefan

---

From: Stefan Baur <[email-masked]>
Date: Sun, 19 Apr 2020 20:20:52 +0200

For the record,

Uli has contacted me off-list and indicated this is a course of action
he can live with.

In the meantime, I decided to dig through X2Go's git repo.

This is the commit that introduced the faulty nscd cache flushing code
in x2goserver-ssh:

<https://code.x2go.org/gitweb?p=x2gobroker.git;a=commit;h=9e44861e4a29897228cb70a95d6853dbe85779b0>

author	Mike Gabriel <[email-masked]>	
	Wed, 1 Apr 2015 03:50:45 +0000 (05:50 +0200)
committer	Mike Gabriel <[email-masked]>	
	Wed, 1 Apr 2015 03:50:45 +0000 (05:50 +0200)
commit	9e44861e4a29897228cb70a95d6853dbe85779b0
tree	9bba4aeb6d2ffa2ac5bfe47a63e2f20fbb4c6a2a
parent	2cd0cdc8b73967b87d53c615a5952a83309ba63f

The previous commit that touched this file is:

author	Mike Gabriel <[email-masked]>	
	Thu, 11 Sep 2014 23:29:14 +0000 (01:29 +0200)
committer	Mike Gabriel <[email-masked]>	
	Thu, 11 Sep 2014 23:29:37 +0000 (01:29 +0200)
commit	67d9cfd0ba235c919af84aa9b0d647d0e8a47968
tree	88ed368b14cb2e440f7eebedec9a342257317ff3
parent	806224f2caab8db9c7accb7fa4c5e587d34cd5ca

So some time between September 11, 2014, and April 1, 2015, Mike#1
noticed that groupadd did not behave as expected, and added this code.
Which means Wheezy (7) and Jessie (8) are the most likely candidates
where the issue might have cropped up, as there's a good chance Mike#1
was using testing instead of stable, being so close to the official
release (which happened on April 25./26., 2015).  Squeeze (6) was
already out of support by September 11, 2014, which means it's unlikely
that whatever happened, happened on Squeeze.

Anyone curious enough to go through the groupadd source code for the
binaries that shipped in these two Debian releases in said time frame?

-Stefan

---

From: Mike Gabriel <[email-masked]>
Date: Mon, 20 Apr 2020 18:42:56 +0000

> binaries that shipped in these two Debian releases in said time frame?

Would it make sense filing a bug against the appropriate Debian package???

Mike
-- 

DAS-NETZWERKTEAM
c\o Technik- und Ökologiezentrum Eckernförde
Mike Gabriel, Marienthaler Str. 17, 24340 Eckernförde
mobile: +49 (1520) 1976 148
landline: +49 (4351) 850 8940

GnuPG Fingerprint: 9BFB AEE8 6C0A A5FF BF22  0782 9AF4 6B30 2577 1B31
mail: [email-masked], http://das-netzwerkteam.de

---

From: Stefan Baur <[email-masked]>
Date: Mon, 20 Apr 2020 22:20:40 +0200

> Would it make sense filing a bug against the appropriate Debian package???

Not if the code Uli has quoted has since been put there to fix the issue
you saw back when you added the workaround to our postinst script.

We should do "our" homework before filing a bug report and confusing the
maintainer(s) of groupadd, by reporting an issue that has already been
fixed since.

So $SOMEONE needs to look at the groupadd sources to confirm or rule out
that the lines Uli spotted have been put there after you wrote the
workaround, and we shouldn't place that burden on the nscd maintainer(s).

If it turns out that the code has already been there, yet you saw a need
to flush the nscd cache with your workaround, we need to create a list
of steps to reproduce the issue you ran into.

Because from looking at the code, as it is now, your workaround would
seem unnecessary - *if* the function in groupadd works as intended.

If not, we need to prove that it fails, in a reproducible way.  Then a
bug report against groupadd's package would be warranted.

Kind Regards,
Stefan Baur

---

From: Mihai Moldovan <[email-masked]>
Date: Tue, 21 Apr 2020 14:50:15 +0200 (CEST)

tag #1458 pending
fixed #1458 0.0.4.2
thanks

Hi!

X2Go issue #1458 (src:x2gobroker) reported by you has been
fixed in X2Go Git. You can see the changelog below, and you can
check the diff of the fix at:

    https://code.x2go.org/gitweb?p=x2gobroker.git;a=commitdiff;h=52a8aeef7311e384889d07537183e6b4c8b695a7

The issue will most likely be fixed in src:x2gobroker (0.0.4.2).

light+love
X2Go Git Admin (on behalf of the sender of this mail)

---
commit 52a8aeef7311e384889d07537183e6b4c8b695a7
Author: Mihai Moldovan <[email-masked]>
Date:   Tue Apr 21 14:44:23 2020 +0200

    debian/x2gobroker-ssh.postinst: only reload nscd configuration if daemon is installed and actually running. Fixes: #1458.
    
    Based on a patch by Stefan Baur.

diff --git a/debian/changelog b/debian/changelog
index 85985e9..1d07af6 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -13,6 +13,9 @@ x2gobroker (0.0.4.2-0x2go1) UNRELEASED; urgency=medium
   * New upstream version (0.0.4.2):
   * debian/control:
     + Add psproc dependency to x2gobroker-ssh. We'll need it later on.
+  * debian/x2gobroker-ssh.postinst:
+    + Only reload nscd configuration if daemon is installed and actually
+      running. Fixes: #1458. Based on a patch by Stefan Baur.
 
  -- X2Go Release Manager <[email-masked]>  Mon, 22 Apr 2019 12:31:49 +0200