Re: [PATCH] sslh: Update to version 1.20

[ummeegge <ummeegge@ipfire.org>]

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

Hi Michael,

On Sa, 2019-05-11 at 11:06 +0100, Michael Tremer wrote:
> Hi,
> 
> The parameter is “in-reply-to”. You got an extra “a” there.
autsch... Thanks for the hint. Have send it to the list now.

Best,

Erik

> 
> -Michael
> 
> > On 10 May 2019, at 12:54, ummeegge <ummeegge(a)ipfire.org> wrote:
> > 
> > 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
> > > 
> > > 
> 
>