From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Tremer To: development@lists.ipfire.org Subject: Re: [PATCH] sslh: Update to version 1.20 Date: Wed, 24 Apr 2019 12:04:49 +0100 Message-ID: In-Reply-To: <20190423070606.5642-1-ummeegge@ipfire.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0374176785569156682==" List-Id: --===============0374176785569156682== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi, Thanks for working on this. I have a couple of questions and remarks. > On 23 Apr 2019, at 08:06, Erik Kapfer wrote: >=20 > - Added USELIBCAP=3D1 to enable the possibility for transparent option. > - Wrote configuration directives in initscript into variable for better ove= rview. > - Introduce chroot directive in start parameter. > - Added new user and group sslh (will be deleted if uninstall). > - Changed EXTERNAL_IP_FUNCT to serve data also for configuration block but = use it also as check as before. > - Added symlinks in sslh paks since the initscripts LFS do not serves it in= old installation (a reboot does not started sslh again). > - Deleted sslh symlinks in initscripts LFS since they are served via sslh p= aks and are not needed anymore. >=20 > Signed-off-by: Erik Kapfer > --- > lfs/initscripts | 3 -- > lfs/sslh | 12 ++++---- > src/initscripts/packages/sslh | 65 +++++++++++++++++++++++++++++++++-------= --- > src/paks/sslh/install.sh | 13 +++++++++ > src/paks/sslh/uninstall.sh | 9 ++++++ > 5 files changed, 79 insertions(+), 23 deletions(-) >=20 > diff --git a/lfs/initscripts b/lfs/initscripts > index 055e106d0..3173a04e4 100644 > --- a/lfs/initscripts > +++ b/lfs/initscripts > @@ -136,9 +136,6 @@ $(TARGET) : > ln -sf ../init.d/client175 /etc/rc.d/rc0.d/K34client175 > ln -sf ../init.d/client175 /etc/rc.d/rc3.d/S66client175 > ln -sf ../init.d/client175 /etc/rc.d/rc6.d/K34client175 > - ln -sf ../init.d/sslh /etc/rc.d/rc3.d/S98sslh > - ln -sf ../init.d/sslh /etc/rc.d/rc0.d/K02sslh > - ln -sf ../init.d/sslh /etc/rc.d/rc6.d/K02sslh > ln -sf ../init.d/vdradmin /etc/rc.d/rc3.d/S99vdradmin > ln -sf ../init.d/vdradmin /etc/rc.d/rc0.d/K01vdradmin > ln -sf ../init.d/vdradmin /etc/rc.d/rc6.d/K01vdradmin > diff --git a/lfs/sslh b/lfs/sslh > index 100cec065..dedd10272 100644 > --- a/lfs/sslh > +++ b/lfs/sslh > @@ -1,7 +1,7 @@ > ###########################################################################= #### > # = # > # IPFire.org - A linux based firewall = # > -# Copyright (C) 2007-2018 IPFire Team = # > +# Copyright (C) 2007-2019 IPFire Team = # > # = # > # This program is free software: you can redistribute it and/or modify = # > # it under the terms of the GNU General Public License as published by = # > @@ -24,7 +24,7 @@ >=20 > include Config >=20 > -VER =3D 1.7a > +VER =3D 1.20 >=20 > THISAPP =3D sslh-$(VER) > DL_FILE =3D $(THISAPP).tar.gz > @@ -32,7 +32,7 @@ DL_FROM =3D $(URL_IPFIRE) > DIR_APP =3D $(DIR_SRC)/$(THISAPP) > TARGET =3D $(DIR_INFO)/$(THISAPP) > PROG =3D sslh > -PAK_VER =3D 5 > +PAK_VER =3D 6 >=20 > DEPS =3D "" >=20 > @@ -44,7 +44,7 @@ objects =3D $(DL_FILE) >=20 > $(DL_FILE) =3D $(DL_FROM)/$(DL_FILE) >=20 > -$(DL_FILE)_MD5 =3D ee124654412198a5e11fe28acf10634d > +$(DL_FILE)_MD5 =3D 0db26ed2825b1ef6c83959a988279912 >=20 > install : $(TARGET) >=20 > @@ -77,8 +77,8 @@ $(subst %,%_MD5,$(objects)) : > $(TARGET) : $(patsubst %,$(DIR_DL)/%,$(objects)) > @$(PREBUILD) > @rm -rf $(DIR_APP) && cd $(DIR_SRC) && tar zxf $(DIR_DL)/$(DL_FILE) > - cd $(DIR_APP) && make CFLAGS=3D"$(CFLAGS)" $(MAKETUNING) USELIBWRAP=3D > - cd $(DIR_APP) && install -v -m 755 sslh /usr/sbin > + cd $(DIR_APP) && make CFLAGS=3D"$(CFLAGS)" $(MAKETUNING) USELIBCAP=3D1 US= ELIBWRAP=3D > + cd $(DIR_APP) && install -v -m 755 sslh-fork /usr/sbin/sslh >=20 > #install initscripts > $(call INSTALL_INITSCRIPT,sslh) It looks all fine up to this point. > diff --git a/src/initscripts/packages/sslh b/src/initscripts/packages/sslh > index 43e58f392..0935b1114 100644 > --- a/src/initscripts/packages/sslh > +++ b/src/initscripts/packages/sslh > @@ -3,31 +3,68 @@ >=20 > # Based on sysklogd script from LFS-3.1 and earlier. > # Rewritten by Gerard Beekmans - gerard(a)linuxfromscratch.org > +# > +# $LastChangedBy: ummeegge - ummeegge(a)ipfire.org $ > +# $Date: 2019-04-04 04:35:09 -0500 (Thu, 04 Apr 2019) $ > +# > +############################################################# > +# You do not need to include authorship headers because Git will take care of t= his. >=20 > . /etc/sysconfig/rc > . $rc_functions >=20 > +DAEMON=3D"/usr/sbin/sslh" > + > +# Check for external IP address and provide it to listening option > +EXTERNAL_IP_ADDRESS=3D"$( +EXTERNAL_IP_FUNCT() { > + if [ -z "${EXTERNAL_IP_ADDRESS}" ]; then > + echo_failure > + boot_mesg -n "FAILURE:\n\nCould not determine" ${FAILURE} > + boot_mesg -n " your external IP address." > + boot_mesg "" ${NORMAL} > + exit 1 > + fi > +} Although you are starting the service rather late in the boot process, it mig= ht indeed happen that this goes wrong. How do we recover from this? The servi= ce should be started eventually when RED comes up. What if the IP address on RED changes? How is sslh notified? > + > +# Loopback interface > +LO=3D"127.0.0.1" > +# Used TCP ports > +LISTENPORT=3D"443" > +SSHPORT=3D"222" > +TLSPORT=3D"444" > +OPENVPNPORT=3D=E2=80=9C1194" Can we use hard-coded port numbers here? > + > +# Configuration options > +DAEMON_OPTS=3D" > +--user sslh > +--listen ${EXTERNAL_IP_ADDRESS}:${LISTENPORT} > +--ssh ${LO}:${SSHPORT} > +--tls ${LO}:${TLSPORT} > +--openvpn ${LO}:${OPENVPNPORT} > +--pidfile /var/run/sslh.pid > +-C /var/empty > +" > + > +# Check for binary > +if ! [ -x "$(command -v ${DAEMON})" ]; then > + echo "Error: could not find ${DAEMON}" >&2 > + exit 1 > +fi No need for this check really. We don=E2=80=99t have it anywhere else. > + > +# Check for external IP > +EXTERNAL_IP_FUNCT This does not really need to be a function because the if statement above isn= =E2=80=99t either. And it should not be evaluated in the stop case. If the system is not online,= this service can never be stopped. I think it is best for this code to have it in the start section where is was= before - unless I missed something here. > + > case "$1" in > start) > boot_mesg "Starting SSLH Deamon..." > - > - LOCAL_IP_ADDRESS=3D"$( - if [ -z "${LOCAL_IP_ADDRESS}" ]; then > - echo_failure > - boot_mesg -n "FAILURE:\n\nCould not determine" ${FAILURE} > - boot_mesg -n " your external IP address." > - boot_mesg "" ${NORMAL} > - exit 1 > - fi > - > - loadproc /usr/sbin/sslh -u nobody \ > - -p "${LOCAL_IP_ADDRESS}:443" -s localhost:222 -l localhost:444 > + loadproc ${DAEMON} ${DAEMON_OPTS} > evaluate_retval > ;; >=20 > stop) > boot_mesg "Stopping SSLH Deamon..." > - killproc /usr/sbin/sslh > + killproc ${DAEMON} > evaluate_retval > ;; >=20 > @@ -38,7 +75,7 @@ case "$1" in > ;; >=20 > status) > - statusproc /usr/sbin/sslh > + statusproc ${DAEMON} > ;; >=20 > *) > diff --git a/src/paks/sslh/install.sh b/src/paks/sslh/install.sh > index 626884bdd..df7cafc78 100644 > --- a/src/paks/sslh/install.sh > +++ b/src/paks/sslh/install.sh > @@ -23,5 +23,18 @@ > # > . /opt/pakfire/lib/functions.sh > extract_files > + > +# Add user and group for sslh if not already done > +if ! grep -q sslh /etc/passwd; then > + groupadd sslh; > + useradd -g sslh -M -s /sbin/nologin sslh > +fi Can you change this to use getent? There are a couple of examples in some oth= er scripts (e.g. tor). You should also check if the group existed already. > + > ln -s /etc/init.d/sslh /etc/rc.d/init.d/networking/red.up/50-sslh Why is this link not in the package? Should we not check if the service is running and only then restart? What hap= pens when the service is disabled at boot time? > + > +# Set symlink for runlevels > +ln -svf ../init.d/sslh /etc/rc.d/rc0.d/K02sslh > +ln -svf ../init.d/sslh /etc/rc.d/rc3.d/S98sslh > +ln -svf ../init.d/sslh /etc/rc.d/rc6.d/K02sslh > + > start_service --background ${NAME} > diff --git a/src/paks/sslh/uninstall.sh b/src/paks/sslh/uninstall.sh > index dca34ccbd..05bb27945 100644 > --- a/src/paks/sslh/uninstall.sh > +++ b/src/paks/sslh/uninstall.sh > @@ -23,5 +23,14 @@ > # > . /opt/pakfire/lib/functions.sh > stop_service ${NAME} > + > +# Delete user and group sslh > +if grep -q sslh /etc/passwd; then > + userdel sslh > + groupdel sslh > +fi We don=E2=80=99t delete users. This will cause that log files or so will have= no owner any more. This won=E2=80=99t be an issue because we check if the user exists in the ins= tall script and if it does we are not trying to create it any more. > + > remove_files > rm -f /etc/rc.d/init.d/networking/red.up/50-sslh > +# Delete initscript symlinks > +rm -f /etc/rc.d/rc?.d/???sslh > --=20 > 2.12.2 >=20 Overall this looks good and makes this a better add-on. It looks like many th= ings to change, but they are all small and should not take too long. Best, -Michael --===============0374176785569156682==--