Re: [PATCH v2] CRL updater: Update script for OpenVPNs CRL

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

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

Hi,

On Tue, 2018-02-06 at 21:09 +0100, Erik Kapfer wrote:
> Update script for OpenVPNs CRL cause OpenVPN refactors the CRL handling since
> v.2.4.0 .
>     Script checks the next update field from the CRL and executes an update
> before it expires.
>     Script is placed under fcron.daily for daily checks.
> 
> Signed-off-by: Erik Kapfer <erik.kapfer(a)ipfire.org>
> ---
>  config/ovpn/openvpn-crl-updater | 88
> +++++++++++++++++++++++++++++++++++++++++
>  config/rootfiles/common/openvpn |  1 +
>  lfs/openvpn                     |  6 +++
>  3 files changed, 95 insertions(+)
>  create mode 100644 config/ovpn/openvpn-crl-updater
> 
> diff --git a/config/ovpn/openvpn-crl-updater b/config/ovpn/openvpn-crl-updater
> new file mode 100644
> index 0000000..9063b04
> --- /dev/null
> +++ b/config/ovpn/openvpn-crl-updater
> @@ -0,0 +1,88 @@
> +#!/bin/bash
> +
> +#############################################################################
> ############

There is an extra empty line before the header and an extra hash in the first
line of the header.

> +#										
> 	#
> +# This file is part of the IPFire Firewall.					
> 	#
> +#										
> 	#
> +# IPFire is free software: you can redistribute it and/or modify		
> 	#
> +# it under the terms of the GNU General Public License as published by	
> 		#
> +# the Free Software Foundation, either version 3 of the License, or 		
> 	#
> +# (at your option) any later version.						
> 	#
> +#										
> 	#
> +# IPFire is distributed in the hope that it will be useful,			
> 	#
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of 		
> 	#
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the 		
> 	#
> +# GNU General Public License for more details. 				
> 		#
> +#										
> 	#
> +# You should have received a copy of the GNU General Public License 		
> 	#
> +# along with IPFire.  If not, see <http://www.gnu.org/licenses/>. 		
> 	#
> +#										
> 	#
> +# Copyright (C) 2007 IPFire-Team <info(a)ipfire.org>.				
> 	#
> +#										
> 	#
> +#############################################################################
> ############
> +#										
> 	#
> +# Script Name: openvpn-crl-updater 						
> 	#
> +# Description: This script checks the "Next Update:" field of the CRL		
> 	#
> +#   and renews it if needed, which prevents the expiration of OpenVPNs CRL.	
> 	#
> +#   With OpenVPN 2.4.x the CRL handling has been refactored, 			
> 	#
> +#   whereby the verification logic has been removed from
> ssl_verify_<backend>.c . 	#
> +#   For more infos:								
> 	#
> +#   https://github.com/OpenVPN/openvpn/commit/160504a2955c4478cd2c0323452929e
> 07016a336 	#
> +#										
> 	#
> +# Run Information: If OpenVPNs CRL is presant, 				

*present*

> 		#
> +#   this script provides a cronjob which checks daily if an update of the
> CRL 		#
> +#   is needed.	If the expiring date reaches the value			
> 		#
> +#   (defined in the 'UPDATE' variable in days) before the CRL expiration, an
> openssl	# 
> +#   command will be executed to renew the CRL.				
> 		#
> +#   Script execution will be logged into /var/log/messages.			
> 	#
> +# 										
> 	#
> +# Author: Erik Kapfer 							
> 		#
> +#										
> 	#
> +# Date: 06.02.2018								

Dates are not required. Git does this for us.

> 	#
> +#										
> 	#
> +#############################################################################
> ############
> +
> +# Check if OpenVPN is active or if the CRL is presant
> +if [ ! -e "/var/ipfire/ovpn/crls/cacrl.pem" ]; then
> +	exit 0;
> +fi

You got a hardcoded path here. Variables are set after this. It probably makes
sense to move the check after the initialisation block and then check things
and/or exit.

> +## Paths
> +OVPN="/var/ipfire/ovpn"
> +CRL="${OVPN}/crls/cacrl.pem"
> +CAKEY="${OVPN}/ca/cakey.pem"
> +CACERT="${OVPN}/ca/cacert.pem"
> +OPENSSLCONF="${OVPN}/openssl/ovpn.cnf"
> +
> +## Values
> +# CRL check for the 'Next Update:' in seconds
> +EXPIRINGDATEINSEC="$((
> +$(/bin/date -d "$(/usr/bin/openssl crl -in "${CRL}" -text | \
> +    /bin/grep -oP 'Next Update: *\K.*')" +%s) - \
> +    $(/bin/date +%s) \
> +))"

You never need to use "/bin" or so before a command. The shell will find it.
Just use date, grep, and (further down) openssl.

And I didn't mean just breaking the lines. I meant splitting this into smaller
chunks that are easy to understand and modify if we need to. Like:

NOW="$(date "+%s")"
EXPIRES_AT="$(openssl ... | grep ...)"

# Convert into seconds from epoch
EXPIRES_AT="$(date "${EXPIRES_AT}" "+%s")"

EXPIRINGDATEINSEC=$(( EXPIRES_AT - NOW ))

I find this way easier to read and audit and it will execute in the same amount
of time.

> +# Day in seconds to calculate
> +DAYINSEC="86400"
> +
> +# Convert seconds to days
> +NEXTUPDATE="$((EXPIRINGDATEINSEC / DAYINSEC))"

Here this is super easy to read and understand. Way better.

> +# Update of the CRL in days before CRL expiring date
> +UPDATE="14"
> +
> +
> +# Check if OpenVPNs CRL needs to be renewed
> +if [ ${NEXTUPDATE} -le ${UPDATE} ]; then
> +    if /usr/bin/openssl ca -gencrl -keyfile "${CAKEY}" -cert "${CACERT}" -out
> "${CRL}" -config "${OPENSSLCONF}"; then
> +	logger -t openvpn "CRL has been updated"
> +    else
> +	logger -t openvpn "error: Could not update CRL"
> +    fi
> +fi
> +
> +exit 0
> +
> +
> +# EOF
> +
> diff --git a/config/rootfiles/common/openvpn b/config/rootfiles/common/openvpn
> index 2b63424..131d798 100644
> --- a/config/rootfiles/common/openvpn
> +++ b/config/rootfiles/common/openvpn
> @@ -1,3 +1,4 @@
> +etc/fcron.daily/openvpn-crl-updater
>  #usr/include/openvpn-msg.h
>  #usr/include/openvpn-plugin.h
>  #usr/lib/openvpn
> diff --git a/lfs/openvpn b/lfs/openvpn
> index 3913f02..1ecc18c 100644
> --- a/lfs/openvpn
> +++ b/lfs/openvpn
> @@ -96,5 +96,11 @@ $(TARGET) : $(patsubst %,$(DIR_DL)/%,$(objects))
>  	mv -v /var/ipfire/ovpn/verify /usr/lib/openvpn/verify
>  	chown root:root /usr/lib/openvpn/verify
>  	chmod 755 /usr/lib/openvpn/verify
> +	# Add crl updater
> +	mv -v /var/ipfire/ovpn/openvpn-crl-updater /etc/fcron.daily
> +	chown root:root /etc/fcron.daily/openvpn-crl-updater
> +	chmod 750 /etc/fcron.daily/openvpn-crl-updater
> +
>  	@rm -rf $(DIR_APP)
>  	@$(POSTBUILD)
> +

There is an extra empty line at the end of the LFS file.

Best,
-Michael

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]