[Merge] ~lucaskanashiro/ubuntu/+source/openvpn:groovy-merge into ubuntu/+source/openvpn:debian/sid
Bryce Harrington
bryce at bryceharrington.org
Tue May 5 01:53:35 UTC 2020
Review: Approve
* Changelog:
- [√] old content and logical tag match as expected
- [√] changelog entry correct version and targeted codename
- [√] changelog entries correct
- [√] update-maintainer has been run
* Actual changes:
- [√] no upstream changes to consider
- [√] no further upstream version to consider
- [√] debian changes look safe
* Old Delta:
- [-] dropped changes are ok to be dropped
- [-] nothing else to drop
- [√] changes forwarded upstream/debian (if appropriate)
* New Delta:
- [-] no new patches added
- [√] patches match what was proposed upstream
- [√] patches correctly included in debian/patches/series
- [√] patches have correct DEP3 metadata
I don't know much about openvpn beyond just being an occasional user, so can't give meaningful advice on the test design, just some Bash syntax discussion inlined. Nothing that has to be fixed, just food for thought if you want. Otherwise, +1 LGTM, go ahead and upload.
Diff comments:
> diff --git a/debian/tests/server-setup-with-ca b/debian/tests/server-setup-with-ca
> new file mode 100755
> index 0000000..bc64e02
> --- /dev/null
> +++ b/debian/tests/server-setup-with-ca
> @@ -0,0 +1,79 @@
> +#!/bin/bash
> +
> +# ----------------------------------------------
> +# Test an OpenVPN server setup with CA
> +# ----------------------------------------------
> +
> +exec 2>&1
> +set -exu
I tend to prefer doing error handling manually rather than relying on set -ex, however I admit this is a pretty common idiom in autopkgtests.
> +
> +CONFIG_DIR=/etc/openvpn
> +CA_DIR=easy-rsa
> +CA_VARS_FILE=vars
> +DEVICE=tun1
> +IP_NETWORK=10.9.8.0
> +NETWORK_MASK=255.255.255.0
> +LOG_FILE=$AUTOPKGTEST_TMP/openvpn.log
> +
> +# Create the CA directory inside the config directory
> +cd $CONFIG_DIR && make-cadir $CA_DIR && cd $CA_DIR
With set -ex, there's no reason to chain the commands with &&, just put each on its own line. If any one fails the script will bail.
> +
> +# Add some variables to the $CA_VARS_FILE to build the CA and keys in a
> +# non interactive mode
> +cat << EOF >> $CA_VARS_FILE
> +set_var EASYRSA_REQ_COUNTRY "US"
> +set_var EASYRSA_REQ_PROVINCE "California"
> +set_var EASYRSA_REQ_CITY "San Francisco"
> +set_var EASYRSA_REQ_ORG "Copyleft Certificate Co"
> +set_var EASYRSA_REQ_EMAIL "me at example.net"
> +set_var EASYRSA_REQ_OU "My Organizational Unit"
> +
> +set_var EASYRSA_BATCH "1"
> +EOF
> +
> +# Setup the CA and the server keys
> +./easyrsa init-pki
> +./easyrsa build-ca nopass
> +./easyrsa build-server-full server nopass
> +./easyrsa gen-dh
> +
> +# Create the OpenVPN server config file
> +cat << EOF > /etc/openvpn/server.conf
> +dev $DEVICE
> +server $IP_NETWORK $NETWORK_MASK
> +
> +ca $CONFIG_DIR/$CA_DIR/pki/ca.crt
> +cert $CONFIG_DIR/$CA_DIR/pki/issued/server.crt
> +key $CONFIG_DIR/$CA_DIR/pki/private/server.key
> +dh $CONFIG_DIR/$CA_DIR/pki/dh.pem
> +EOF
> +
> +# Start an OpenVPN process in background and redirect its output to a file
> +openvpn --config $CONFIG_DIR/server.conf --verb 6 > $LOG_FILE &
> +
> +# Give some time to start the process
> +sleep 5
Did you look into if this sleep could be avoided or minimized, e.g. by directly testing the process readiness in a while loop?
> +
> +# Check if the $DEVICE was correctly created
> +ip address | grep $DEVICE
> +
> +# Check if OpenVPN is listening on port 1194 (default port)
> +ss -lnptu | grep openvpn | grep 1194
> +
> +# Check if Diffie-Hellman was initialized
> +cat $LOG_FILE | grep Diffie-Hellman | grep initialized
> +
> +# Check if the $DEVICE is linked
> +cat $LOG_FILE | grep link | grep $DEVICE
> +
> +# Check if the network route was correctly configured
> +cat $LOG_FILE | grep route | grep $IP_NETWORK/24
> +
> +# Check if the Initialization Sequence completed
> +cat $LOG_FILE | grep 'Initialization Sequence Completed'
> +
> +# Clean up
> +cleanup() {
> + pkill openvpn
> +}
> +trap cleanup INT TERM EXIT
> diff --git a/debian/tests/server-setup-with-static-key b/debian/tests/server-setup-with-static-key
> new file mode 100755
> index 0000000..62f5976
> --- /dev/null
> +++ b/debian/tests/server-setup-with-static-key
> @@ -0,0 +1,53 @@
> +#!/bin/bash
> +
> +# ----------------------------------------------
> +# Test an OpenVPN server setup with a static key
> +# ----------------------------------------------
> +
> +exec 2>&1
> +set -exu
> +
> +CONFIG_DIR=/etc/openvpn
> +STATIC_KEY=static.key
> +DEVICE=tun0
> +IP_SERVER=10.9.8.1
> +IP_CLIENT=10.9.8.2
> +LOG_FILE=$AUTOPKGTEST_TMP/openvpn.log
> +
> +# Generate the static key inside the config directory
> +cd $CONFIG_DIR && openvpn --genkey --secret $STATIC_KEY
> +
> +# Create the config file
> +cat << EOF > $CONFIG_DIR/$DEVICE.conf
> +dev $DEVICE
> +ifconfig $IP_SERVER $IP_CLIENT
> +secret $CONFIG_DIR/$STATIC_KEY
> +EOF
> +
> +# Start an OpenVPN process in background and redirect its output to a file
> +openvpn --config $CONFIG_DIR/$DEVICE.conf --verb 6 > $LOG_FILE &
> +
> +# Give some time to start the process
> +sleep 5
> +
> +# Check if the $DEVICE was correctly created
> +ip address | grep $DEVICE
> +
> +# Check if OpenVPN is listening on port 1194 (default port)
> +ss -lnptu | grep openvpn | grep 1194
> +
> +# Check if the $STATIC_KEY is used by OpenVPN
> +cat $LOG_FILE | grep shared_secret_file | grep $CONFIG_DIR/$STATIC_KEY
> +
> +# Check if the $DEVICE is linked
> +cat $LOG_FILE | grep link | grep $DEVICE
> +
> +# Check if the specified IP addresses were configured
> +cat $LOG_FILE | grep addr | grep local | grep $IP_SERVER
> +cat $LOG_FILE | grep addr | grep peer | grep $IP_CLIENT
> +
> +# Clean up
> +cleanup() {
> + pkill openvpn
> +}
> +trap cleanup INT TERM EXIT
Should the CA directory also get removed in cleanup?
--
https://code.launchpad.net/~lucaskanashiro/ubuntu/+source/openvpn/+git/openvpn/+merge/383178
Your team Ubuntu Server Developers is requested to review the proposed merge of ~lucaskanashiro/ubuntu/+source/openvpn:groovy-merge into ubuntu/+source/openvpn:debian/sid.
More information about the Ubuntu-reviews
mailing list