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: Sat, 11 May 2019 11:06:23 +0100 Message-ID: In-Reply-To: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============5328673807160098025==" List-Id: --===============5328673807160098025== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi, The parameter is =E2=80=9Cin-reply-to=E2=80=9D. You got an extra =E2=80=9Ca= =E2=80=9D there. -Michael > On 10 May 2019, at 12:54, ummeegge wrote: >=20 > Hi Michael, > and thanks for your feedback. Made a V2 patch now but i get an=20 >=20 > $ git send-email -v2 -1 --in-replay-to 690B105B-733E-4283-9D05-9967D09BA86F= (a)ipfire.org > fatal: ambiguous argument '690B105B-733E-4283-9D05-9967D09BA86F(a)ipfire.or= g': unknown revision or path not in the working tree. > Use '--' to separate paths from revisions, like this: > 'git [...] -- [...]' > format-patch -o /tmp/qnlL0LbJPZ -v2 -1 --in-replay-to 690B105B-733E-4283-9D= 05-9967D09BA86F(a)ipfire.org: command returned error: 128 >=20 > am currently not sure how to fix this. Have nevertheless pushed the new ver= sion=20 > (hopefully all wanted is included) to Git --> > https://git.ipfire.org/?p=3Dpeople/ummeegge/ipfire-2.x.git;a=3Dcommit;h=3D3= 14975a6703f78d8e4c0cb9071819597ad72d48e >=20 > Since my time is currently a little less it might be great if you can > review it in that way. >=20 > Best, >=20 > Erik >=20 >=20 > On Mi, 2019-05-01 at 12:13 +0100, Michael Tremer wrote: >> Hi, >>=20 >> Apologies for the late reply again=E2=80=A6 I am trying to catch up on my >> rather large inbox. >>=20 >>> 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 >>>>>=20 >>>>> # >>>>> # IPFire.org - A linux based >>>>> firewall # >>>>> -# Copyright (C) 2007-2018 IPFire Team =20 >>>>>=20 >>>>> # >>>>> +# Copyright (C) 2007-2019 IPFire Team =20 >>>>>=20 >>>>> # >>>>> # =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. >>>=20 >>> 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. >>>=20 >>> 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 ? >>=20 >> 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 >>>>=20 >>>> What if the IP address on RED changes? How is sslh notified? >>>=20 >>> 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 ? >>=20 >> 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 interrupted. I think that is what we need here, isn=E2=80= =99t it? >>=20 >>>=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? >>>=20 >>> 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. >>=20 >> I would say that 443 probably masks traffic best and is most likely >> to be open even in more restricted environments. >>=20 >> 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 that should be optional. >>=20 >>> 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?! >>=20 >> The Web UI? No, that should not be open by default. People should use >> OpenVPN to access the firewall. >>=20 >>> 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?! >>=20 >> We do not know how critical the application of the user is. So >> assuming it is most critical is best. >>=20 >>>=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. >>>=20 >>> 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. >>>=20 >>> 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. >>=20 >> You won=E2=80=99t be able to stop the service then if the script is ended = in >> that block. >>=20 >> I think it should only be evaluated in the start block. The check >> isn=E2=80=99t needed for stopping the service. >>=20 >>>=20 >>>>=20 >>>>> + >>>>> case "$1" in >>>>> start) >>>>> boot_mesg "Starting SSLH Deamon..." >>>>> - >>>>> - LOCAL_IP_ADDRESS=3D"$(>>>> ipaddress)" >>>>> - 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). >>>=20 >>> Done. >>>=20 >>>>=20 >>>> You should also check if the group existed already. >>>=20 >>> I think the getent solution --> >>> if ! getent group sslh &>/dev/null; then >>> ... >>> will do this. >>=20 >> OK! >>=20 >>>>=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? >>>=20 >>> 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) ? >>=20 >> I think it is easiest to have it in the package so it will be deleted >> when the package is being removed. >>=20 >> This might become a script now - see above. >>=20 >>>>=20 >>>> Should we not check if the service is running and only then >>>> restart?=20 >>>=20 >>> Via pgrep check in start) ? >>=20 >> Not in start, but in the red.up script. >>=20 >> 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 u= p. >>=20 >>>> What happens when the service is disabled at boot time? >>>=20 >>> You mean disabled via WUI ? >>=20 >> Yes. >>=20 >>>=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 >>>> have >>>> 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. >>>=20 >>> Done. >>=20 >> Thanks! >>=20 >>>>=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. >>>=20 >>> 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. >>=20 >> If you are using it, it should of course be updated. >>=20 >> 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 make high-quality add-on out of this. >>=20 >> Maybe we should have awesome documentation on the Wiki so that more >> people are aware of it and therefore use it. >>=20 >> -Michael >>=20 >>>=20 >>>>=20 >>>> Best, >>>> -Michael >>>>=20 >>>=20 >>> Best, >>>=20 >>> Erik >>=20 >>=20 >=20 --===============5328673807160098025==--