Developers of Sympa

[Marc Chantreux <address@concealed>]

hello people,

as i setup a devel instance of sympa, i realized that i wasn't able to
start sympa daemons as my sympa user is member of multiple groups.

as attachment, a patch that works for me and not really tested.

i don't know how to go further:

* a simple grep shown me that we should probably remove some copy-pasta

grep -RF setuid

src/sbin/bulk.pl.in:POSIX::setuid((getpwnam(Sympa::Constants::USER))[2]);
src/sbin/sympa.pl.in:POSIX::setuid((getpwnam(Sympa::Constants::USER))[2]);
src/sbin/task_manager.pl.in:POSIX::setuid((getpwnam(Sympa::Constants::USER))[2]);
src/sbin/sympa_msg.pl.in:POSIX::setuid((getpwnam(Sympa::Constants::USER))[2]);
src/sbin/archived.pl.in:POSIX::setuid((getpwnam(Sympa::Constants::USER))[2]);
src/sbin/sympa_automatic.pl.in:POSIX::setuid((getpwnam(Sympa::Constants::USER))[2]);
src/sbin/bounced.pl.in:POSIX::setuid((getpwnam(Sympa::Constants::USER))[2]);
src/bin/upgrade_shared_repository.pl.in:POSIX::setuid((getpwnam(Sympa::Constants::USER))[2]);
src/bin/upgrade_send_spool.pl.in:POSIX::setuid((getpwnam(Sympa::Constants::USER))[2]);
src/bin/upgrade_bulk_spool.pl.in:POSIX::setuid((getpwnam(Sympa::Constants::USER))[2]);

* i'm scared to introduce bugs in other weird plateforms that is sure
  still used by some members of the sympa community without our ability
  to test anything on it. this is one of the reasons of the point 2:

* i really think that in a future major version of sympa,
  this code should be just removed instead of being optimistically
  maintained: there are lot of external tools to make those kind of
  things and administrators should be happy pick the one he wants.

  As exemples:
  * old debian has stop-start-daemon so you can use --user and --group
  * ubuntu used to use upstart which has setuid and setgid instructions
  * systemd [service] section has a User= and a Group= variable
  * ...

  so it seems to me that removing this code is not only good for us but
  also good for the sympa system administrators.

* however: if we use this patch, maybe set_daemon_identity should be in
  a module and `grep -RF setuid` should release 1 single line.

so ... your opinion about it ?

regards,
marc


From a7bef83eba513d7e289aac8c3093bddaf8c70887 Mon Sep 17 00:00:00 2001
From: Marc Chantreux <address@concealed>
Date: Tue, 9 Jan 2018 16:16:43 +0000
Subject: [PATCH] when the sympa user is member of more than one group, returns
 a string that is actually the list of GIDs that must be split. to make things
 work.

this is a "work for me" patch
---
 src/sbin/task_manager.pl.in | 45 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 32 insertions(+), 13 deletions(-)

diff --git a/src/sbin/task_manager.pl.in b/src/sbin/task_manager.pl.in
index 3d29b639a..959d2c142 100644
--- a/src/sbin/task_manager.pl.in
+++ b/src/sbin/task_manager.pl.in
@@ -123,21 +123,40 @@ unless ($main::options{foreground}) {
     $process->direct_stderr_to_file;
 }
 
-## Set the UserID & GroupID for the process
-$GID = $EGID = (getgrnam(Sympa::Constants::GROUP))[2];
-$UID = $EUID = (getpwnam(Sympa::Constants::USER))[2];
-
-## Required on FreeBSD to change ALL IDs(effective UID + real UID + saved UID)
-POSIX::setuid((getpwnam(Sympa::Constants::USER))[2]);
-POSIX::setgid((getgrnam(Sympa::Constants::GROUP))[2]);
-
-# Check if the UID has correctly been set (useful on OS X)
-unless (($GID == (getgrnam(Sympa::Constants::GROUP))[2])
-    && ($UID == (getpwnam(Sympa::Constants::USER))[2])) {
-    die
-        "Failed to change process user ID and group ID. Note that on some OS Perl scripts can't change their real UID. In such circumstances Sympa should be run via sudo.\n";
+sub xid_must_be_one_of {
+    my ($name, $got, $expected) = @_;
+    grep {$expected==$_} ($got =~ /\d+/g)
+	or die
+	"$name is $got when sympa is configured to run expect $expected\n"
+	, "Note that on some OS Perl scripts can't change their real UID."
+	, "In such circumstances Sympa should be run via sudo.\n";
 }
 
+
+sub set_daemon_identity {
+
+	my
+	( $UID
+	, $EUID
+	, $GID
+	, $EGID );
+
+	## Set the UserID & GroupID for the process
+	$GID = $EGID = (getgrnam Sympa::Constants::GROUP)[2];
+	$UID = $EUID = (getpwnam Sympa::Constants::USER )[2];
+
+	## Required on FreeBSD to change ALL IDs(effective UID + real UID + saved UID)
+	POSIX::setuid((getpwnam Sympa::Constants::USER )[2]);
+	POSIX::setgid((getgrnam Sympa::Constants::GROUP)[2]);
+
+	# Check if the UID has correctly been set (useful on OS X)
+	xid_must_be_one_of GID => $GID, (getgrnam Sympa::Constants::GROUP )[2];
+	xid_must_be_one_of UID => $UID, (getpwnam Sympa::Constants::USER  )[2];
+
+}
+
+set_daemon_identity;
+
 ## Sets the UMASK
 umask(oct($Conf::Conf{'umask'}));
 
-- 
2.15.1