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

[ummeegge <ummeegge@ipfire.org>]

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

Hi Michael,
thanks for your feedback.

Am 02.02.2018 um 11:51 schrieb Michael Tremer:

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

OK, I think i would use then GPL 3 like IPFire.

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

Done.

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

Done.

> 
>> +# Day in seconds to calculate
>> +DAYINSEC="86400";
> 
> No ; needed here and everywhere else...
> 
> It's shell, not C.

OK :-) done 

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

So i would need then an frcontab entry and another location for the script since the fcron directories provides only daily, weekly and monthly.
Another possibility might  be a weekly check so we can use the fcron directories ?

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

Done

> 
> Should we catch any errors of the openssl command?

OK i would then use may a '2>&1 | logger -i -t openvpn' instead so we get an OpenSSL command output in messages if the CRL has been renewed.

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

Done.

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

Done.

>> 
>> 	@rm -rf $(DIR_APP)
>> 	@$(POSTBUILD)
> 
> Apart from that this looks good. Just minor stuff.

Great that you looked over it.

> 
> Best,
> -Michael

Greetings,

Erik