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, 01 May 2019 12:13:22 +0100 Message-ID: <690B105B-733E-4283-9D05-9967D09BA86F@ipfire.org> In-Reply-To: <7f357513f7ebf639e6a0271b8a3e0dee6335323f.camel@ipfire.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2587403567307853963==" List-Id: --===============2587403567307853963== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi, Apologies for the late reply again=E2=80=A6 I am trying to catch up on my rat= her large inbox. > On 26 Apr 2019, at 05:55, ummeegge wrote: >=20 > Hi Michael and thanks for looking into this. >=20 > On Mi, 2019-04-24 at 12:04 +0100, Michael Tremer wrote: >> Hi, >>=20 >> Thanks for working on this. I have a couple of questions and remarks. >>=20 >>> 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 overview. >>> - 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 paks 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 @@ >>> ################################################################### >>> ############ >>> # =20 >>> # >>> # IPFire.org - A linux based >>> firewall # >>> -# Copyright (C) 2007-2018 IPFire Team =20 >>> # >>> +# Copyright (C) 2007-2019 IPFire Team =20 >>> # >>> # =20 >>> # >>> # 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 USELIBWRAP=3D >>> + cd $(DIR_APP) && install -v -m 755 sslh-fork /usr/sbin/sslh >>>=20 >>> #install initscripts >>> $(call INSTALL_INITSCRIPT,sslh) >>=20 >> It looks all fine up to this point. >>=20 >>> 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) $ >>> +# >>> +############################################################# >>> +# >>=20 >> You do not need to include authorship headers because Git will take >> care of this. > Done. >=20 >>=20 >>>=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 >>> +} >>=20 >> Although you are starting the service rather late in the boot >> process, it might indeed happen that this goes wrong. How do we >> recover from this? The service should be started eventually when RED >> comes up. > I tried to leave the most options pretty much the same as they where > before whereby the numbers of the symlinks are a part of this the > testings did not delivered problems with this but better to stay save > there. > The network initscript starts at 'S20' where i think RED will be > started too what you think is a good starting point for sslh ? If there is a cable plugged into RED and it actually comes up this is a good = time to start the service. But what if the DSL modem takes longer to sync or = so? It should still be started in red.up then. >=20 >>=20 >> What if the IP address on RED changes? How is sslh notified? > I think it won=C2=B4t currently. Have seen some configurations which uses > 0.0.0.0 for the --listen option but this can cause bigger troubles if > '443' is the --listen port here i think. As far as i can see only a > restart of sslh would fix this currently. > May you have an idea here ? Have a script in red.up which restarts the whole service - but only if the IP= address has actually changed so that standing connections won=E2=80=99t be i= nterrupted. I think that is what we need here, isn=E2=80=99t it? >=20 >=20 >>=20 >>> + >>> +# 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" >>=20 >> Can we use hard-coded port numbers here? > Here arises also some more questions for me. > We can grep for them in the configuration files but also may a check > for the correct usage can be made since e.g. OpenVPN needs to run with > TCP for example. I would say that 443 probably masks traffic best and is most likely to be ope= n even in more restricted environments. OpenVPN has to be running in TCP mode, but that is something the user has to = configure. It isn=E2=80=99t even certain if they are using OpenVPN here. So t= hat should be optional. > Should we really provide TCP 444 in here for external access ? I think > also SSH might be more secure via VPN but may a little more acceptable > if no VPN is in usage?! The Web UI? No, that should not be open by default. People should use OpenVPN= to access the firewall. > The main topic in the tests we made in the forum where mainly for LAN > instances on seperated machines (mostly webservers) where those > questions may be not that critical?! We do not know how critical the application of the user is. So assuming it is= most critical is best. >=20 >>=20 >>> + >>> +# 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 >>=20 >> No need for this check really. We don=E2=80=99t have it anywhere else. > Done. >=20 >>=20 >>> + >>> +# Check for external IP >>> +EXTERNAL_IP_FUNCT >>=20 >> This does not really need to be a function because the if statement >> above isn=E2=80=99t either. >>=20 >> And it should not be evaluated in the stop case. If the system is not >> online, this service can never be stopped. >>=20 >> I think it is best for this code to have it in the start section >> where is was before - unless I missed something here. > Have externalised the investigation of the RED address to provide it > also for the --listen option in the configuration block. To keep the > start) case also a little cleaner have decided to set the external IP > check completely outside of the start) case and set the check before > but this is fast changed. You won=E2=80=99t be able to stop the service then if the script is ended in = that block. I think it should only be evaluated in the start block. The check isn=E2=80= =99t needed for stopping the service. >=20 >>=20 >>> + >>> 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 >>=20 >> Can you change this to use getent? There are a couple of examples in >> some other scripts (e.g. tor). > Done. >=20 >>=20 >> You should also check if the group existed already. > I think the getent solution --> > if ! getent group sslh &>/dev/null; then > ... > will do this. OK! >>=20 >>> + >>> ln -s /etc/init.d/sslh /etc/rc.d/init.d/networking/red.up/50-sslh >>=20 >> Why is this link not in the package? > You mean in the LFS ? This is also a part of the old implementation, > might it be an idea to have it in the initscript (incl. deletion of the > symlink) ? I think it is easiest to have it in the package so it will be deleted when th= e package is being removed. This might become a script now - see above. >>=20 >> Should we not check if the service is running and only then restart?=20 > Via pgrep check in start) ? Not in start, but in the red.up script. That way we would only touch the service when it is active. If it isn=E2=80= =99t started, we need to figure out if it should be started - or check if the= boot process brought it up - even though RED wasn=E2=80=99t up. >> What happens when the service is disabled at boot time? > You mean disabled via WUI ? Yes. >=20 >>=20 >>> + >>> +# 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 >>=20 >> We don=E2=80=99t delete users. This will cause that log files or so will h= ave >> no owner any more. >>=20 >> This won=E2=80=99t be an issue because we check if the user exists in the >> install script and if it does we are not trying to create it any >> more. > Done. Thanks! >>=20 >>> + >>> 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 >>=20 >> Overall this looks good and makes this a better add-on. It looks like >> many things to change, but they are all small and should not take too >> long. > This Addon is really a old one and i was not sure if it should be > updated again or if it would be dropped since stunnel do a similar job > i think. But am happy if it can be a better one and the time to bring > it back to life was not wasted. If you are using it, it should of course be updated. There is a little bit of work to be done and some add-ons are really just the= package itself with little integration, but I think it is a good idea to mak= e high-quality add-on out of this. Maybe we should have awesome documentation on the Wiki so that more people ar= e aware of it and therefore use it. -Michael >=20 >>=20 >> Best, >> -Michael >>=20 >=20 > Best, >=20 > Erik --===============2587403567307853963==--