Re: [PATCH] CRL updater: Update script for OpenVPN CRL

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

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

Hi,

thanks for working on this.

On Fri, 2018-02-02 at 07:34 +0100, Erik Kapfer wrote:
> Update script for OpenVPNs CRL has been integrated cause OpenVPN refactors the CRL handling since v.2.4.0 .
>     Script checks the next update field from the CRL and executes an update two days before it expires.
>     Script is placed under fcron.daily for daily checks.
>     OpenVPN changes can be found in here https://github.com/OpenVPN/openvpn/commit/160504a2955c4478cd2c0323452929e07016a336 .
> 
> Signed-off-by: Erik Kapfer <erik.kapfer(a)ipfire.org>
> ---
>  config/ovpn/ovpn_crl_updater.sh | 53 +++++++++++++++++++++++++++++++++++++++++
>  lfs/openvpn                     |  4 ++++
>  2 files changed, 57 insertions(+)
>  create mode 100644 config/ovpn/ovpn_crl_updater.sh
> 
> diff --git a/config/ovpn/ovpn_crl_updater.sh b/config/ovpn/ovpn_crl_updater.sh
> new file mode 100644
> index 0000000..309edc2
> --- /dev/null
> +++ b/config/ovpn/ovpn_crl_updater.sh
> @@ -0,0 +1,53 @@
> +#!/bin/bash

The file needs a GPL header here or what ever license you choose this
will be.

> +
> +#
> +# Script Name: ovpn_crl_updater.sh
> +# 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 .
> +#     See for more infos:
> +#     https://github.com/OpenVPN/openvpn/commit/160504a2955c4478cd2c0323452929e07016a336
> +#
> +# Run Information: If OpenVPNs CRL is presant, 
> +#     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.
> +#     The renewing of the CRL will be logged into /var/log/messages.
> +# 
> +# Author: Erik Kapfer
> +#
> +# Date: 17.01.2018
> +#
> +###############################################################################################
> +
> +# Check if OpenVPN is active or if the CRL is presant
> +if [ ! -e "/var/ipfire/ovpn/crls/cacrl.pem" ]; then
> +	exit 0;
> +fi
> +
> +## 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";

You may use some empty lines here to make the coder easier to read.

> +## Values
> +# CRL check for the the 'Next Update:' in seconds
> +EXPIRINGDATEINSEC="$(( $(date -d "$(openssl crl -in "${CRL}" -text | grep -oP 'Next Update: *\K.*')" +%s) - $(date +%s) ))";

Complicated command. Can we break this down a little bit? Code doesn't
necessarily run faster when everything is just one line, but it will be
way easier to understand.

> +# Day in seconds to calculate
> +DAYINSEC="86400";

No ; needed here and everywhere else...

It's shell, not C.

> +# Convert seconds to days
> +NEXTUPDATE="$((EXPIRINGDATEINSEC / DAYINSEC))";
> +# Update of the CRL in days before CRL expiring date
> +UPDATE="2";

I think we should update every 14 days if the usual expiry time is 30.
Therefore we will never get too close by accident.

> +# Check if OpenVPNs CRL needs to be renewed
> +if [ "${NEXTUPDATE}" -le "${UPDATE}" ]; then
> +	openssl ca -gencrl -keyfile "${CAKEY}" -cert "${CACERT}" -out "${CRL}" -config "${OPENSSLCONF}";
> +	logger -t openssl "OpenVPN CRL has been renewed";
> +fi

You don't need the quotes around the integer comparison.

Should we catch any errors of the openssl command?

I think the logging tag should rather be openvpn instead of openssl.

> +
> +exit 0
> +
> +# EOF
> diff --git a/lfs/openvpn b/lfs/openvpn
> index a925f78..1e1ddc2 100644
> --- a/lfs/openvpn
> +++ b/lfs/openvpn
> @@ -96,6 +96,10 @@ $(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/ovpn_crl_updater.sh /etc/fcron.daily
> +	chown root:root /etc/fcron.daily/ovpn_crl_updater.sh
> +	chmod 750 /etc/fcron.daily/ovpn_crl_updater.sh

Can we rename the script to openvpn-crl-updater?
>  
>  	@rm -rf $(DIR_APP)
>  	@$(POSTBUILD)

Apart from that this looks good. Just minor stuff.

Best,
-Michael

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