Re: [PATCH] sslh: Update to version 1.20

[ummeegge <ummeegge@ipfire.org>]

[-- Attachment #1: Type: text/plain, Size: 17332 bytes --]

Hi Michael,
and thanks for your feedback. Made a V2 patch now but i get an 

$ 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.org': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
format-patch -o /tmp/qnlL0LbJPZ -v2 -1 --in-replay-to 690B105B-733E-4283-9D05-9967D09BA86F(a)ipfire.org: command returned error: 128

am currently not sure how to fix this. Have nevertheless pushed the new version 
(hopefully all wanted is included) to Git -->
https://git.ipfire.org/?p=people/ummeegge/ipfire-2.x.git;a=commit;h=314975a6703f78d8e4c0cb9071819597ad72d48e

Since my time is currently a little less it might be great if you can
review it in that way.

Best,

Erik


On Mi, 2019-05-01 at 12:13 +0100, Michael Tremer wrote:
> Hi,
> 
> Apologies for the late reply again… I am trying to catch up on my
> rather large inbox.
> 
> > On 26 Apr 2019, at 05:55, ummeegge <ummeegge(a)ipfire.org> wrote:
> > 
> > Hi Michael and thanks for looking into this.
> > 
> > On Mi, 2019-04-24 at 12:04 +0100, Michael Tremer wrote:
> > > Hi,
> > > 
> > > Thanks for working on this. I have a couple of questions and
> > > remarks.
> > > 
> > > > On 23 Apr 2019, at 08:06, Erik Kapfer <ummeegge(a)ipfire.org>
> > > > wrote:
> > > > 
> > > > - Added USELIBCAP=1 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.
> > > > 
> > > > Signed-off-by: Erik Kapfer <ummeegge(a)ipfire.org>
> > > > ---
> > > > 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(-)
> > > > 
> > > > 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  <info(a)ipfire.org>     
> > > >     
> > > >            #
> > > > +# Copyright (C) 2007-2019  IPFire Team  <info(a)ipfire.org>     
> > > >     
> > > >            #
> > > > #                                                              
> > > >     
> > > >           #
> > > > # 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 @@
> > > > 
> > > > include Config
> > > > 
> > > > -VER        = 1.7a
> > > > +VER        = 1.20
> > > > 
> > > > THISAPP    = sslh-$(VER)
> > > > DL_FILE    = $(THISAPP).tar.gz
> > > > @@ -32,7 +32,7 @@ DL_FROM    = $(URL_IPFIRE)
> > > > DIR_APP    = $(DIR_SRC)/$(THISAPP)
> > > > TARGET     = $(DIR_INFO)/$(THISAPP)
> > > > PROG       = sslh
> > > > -PAK_VER    = 5
> > > > +PAK_VER    = 6
> > > > 
> > > > DEPS       = ""
> > > > 
> > > > @@ -44,7 +44,7 @@ objects = $(DL_FILE)
> > > > 
> > > > $(DL_FILE) = $(DL_FROM)/$(DL_FILE)
> > > > 
> > > > -$(DL_FILE)_MD5 = ee124654412198a5e11fe28acf10634d
> > > > +$(DL_FILE)_MD5 = 0db26ed2825b1ef6c83959a988279912
> > > > 
> > > > install : $(TARGET)
> > > > 
> > > > @@ -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="$(CFLAGS)" $(MAKETUNING)
> > > > USELIBWRAP=
> > > > -	cd $(DIR_APP) && install -v -m 755 sslh /usr/sbin
> > > > +	cd $(DIR_APP) && make CFLAGS="$(CFLAGS)" $(MAKETUNING)
> > > > USELIBCAP=1 USELIBWRAP=
> > > > +	cd $(DIR_APP) && install -v -m 755 sslh-fork
> > > > /usr/sbin/sslh
> > > > 
> > > > 	#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 @@
> > > > 
> > > > # 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 this.
> > 
> > Done.
> > 
> > > 
> > > > 
> > > > . /etc/sysconfig/rc
> > > > . $rc_functions
> > > > 
> > > > +DAEMON="/usr/sbin/sslh"
> > > > +
> > > > +# Check for external IP address and provide it to listening
> > > > option
> > > > +EXTERNAL_IP_ADDRESS="$(</var/ipfire/red/local-ipaddress)"
> > > > +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 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.
> 
> > 
> > > 
> > > What if the IP address on RED changes? How is sslh notified?
> > 
> > I think it won´t 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’t be interrupted. I think that is what we need here, isn’t it?
> 
> > 
> > 
> > > 
> > > > +
> > > > +# Loopback interface
> > > > +LO="127.0.0.1"
> > > > +# Used TCP ports
> > > > +LISTENPORT="443"
> > > > +SSHPORT="222"
> > > > +TLSPORT="444"
> > > > +OPENVPNPORT=“1194"
> > > 
> > > 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 open even in more restricted environments.
> 
> OpenVPN has to be running in TCP mode, but that is something the user
> has to configure. It isn’t even certain if they are using OpenVPN
> here. So that 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.
> 
> > 
> > > 
> > > > +
> > > > +# Configuration options
> > > > +DAEMON_OPTS="
> > > > +--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’t have it anywhere else.
> > 
> > Done.
> > 
> > > 
> > > > +
> > > > +# Check for external IP
> > > > +EXTERNAL_IP_FUNCT
> > > 
> > > This does not really need to be a function because the if
> > > statement
> > > above isn’t 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.
> > 
> > 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’t 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’t needed for stopping the service.
> 
> > 
> > > 
> > > > +
> > > > case "$1" in
> > > > 	start)
> > > > 		boot_mesg "Starting SSLH Deamon..."
> > > > -
> > > > -		LOCAL_IP_ADDRESS="$(</var/ipfire/red/local-
> > > > 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
> > > > 		;;
> > > > 
> > > > 	stop)
> > > > 		boot_mesg "Stopping SSLH Deamon..."
> > > > -		killproc /usr/sbin/sslh
> > > > +		killproc ${DAEMON}
> > > > 		evaluate_retval
> > > > 		;;
> > > > 
> > > > @@ -38,7 +75,7 @@ case "$1" in
> > > > 		;;
> > > > 
> > > > 	status)
> > > > -		statusproc /usr/sbin/sslh
> > > > +		statusproc ${DAEMON}
> > > > 		;;
> > > > 
> > > > 	*)
> > > > 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 other scripts (e.g. tor).
> > 
> > Done.
> > 
> > > 
> > > 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!
> 
> > > 
> > > > +
> > > > ln -s /etc/init.d/sslh /etc/rc.d/init.d/networking/red.up/50-
> > > > sslh
> > > 
> > > 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 the package is being removed.
> 
> This might become a script now - see above.
> 
> > > 
> > > Should we not check if the service is running and only then
> > > restart? 
> > 
> > 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’t started, we need to figure out if it should be started - or
> check if the boot process brought it up - even though RED wasn’t up.
> 
> > > What happens when the service is disabled at boot time?
> > 
> > You mean disabled via WUI ?
> 
> Yes.
> 
> > 
> > > 
> > > > +
> > > > +# 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’t delete users. This will cause that log files or so will
> > > have
> > > no owner any more.
> > > 
> > > This won’t 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!
> 
> > > 
> > > > +
> > > > 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
> > > > -- 
> > > > 2.12.2
> > > > 
> > > 
> > > 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 make high-quality add-on out of this.
> 
> Maybe we should have awesome documentation on the Wiki so that more
> people are aware of it and therefore use it.
> 
> -Michael
> 
> > 
> > > 
> > > Best,
> > > -Michael
> > > 
> > 
> > Best,
> > 
> > Erik
> 
>