Re: [PATCH] sslh: Update to version 1.20

[Michael Tremer <michael.tremer@ipfire.org>]

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

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.

> 
> . /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.

What if the IP address on RED changes? How is sslh notified?

> +
> +# 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?

> +
> +# 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.

> +
> +# 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.

> +
> 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).

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 happens 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’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.

> +
> 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.

Best,
-Michael