Skip to content

[fix] Decouple OpenVPN config filename from VPN name #572 #609

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nemesifier wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[fix] Decouple OpenVPN config filename from VPN name #572 #609

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 38 additions & 4 deletions images/common/utils.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,13 +236,47 @@ function openvpn_config_checksum {
}

function openvpn_config_download {
# Extract in isolation so stale files in / cannot be mistaken for
# files from the newly downloaded archive.
TMPDIR=$(mktemp -d) || return 1
curl --silent --retry 10 --retry-delay 5 --retry-max-time 300\
--insecure --output vpn.tar.gz \
${API_INTERNAL}/controller/vpn/download-config/$UUID/?key=$KEY
"${API_INTERNAL}/controller/vpn/download-config/${UUID}/?key=${KEY}" || {
rm -rf -- "$TMPDIR"
return 1
}
curl --silent --insecure --output checksum \
${API_INTERNAL}/controller/vpn/checksum/$UUID/?key=$KEY
tar xzf vpn.tar.gz
chmod 600 *.pem
"${API_INTERNAL}/controller/vpn/checksum/${UUID}/?key=${KEY}" || {
rm -rf -- "$TMPDIR"
return 1
}
tar xzf vpn.tar.gz -C "$TMPDIR" || {
rm -rf -- "$TMPDIR"
return 1
}
chmod 600 -- "$TMPDIR"/*.pem 2>/dev/null || true
# Supervisord always starts OpenVPN with openvpn.conf; normalize whatever
# config filename the archive contains, including names with whitespace.
CONF_FILE="$TMPDIR/openvpn.conf"
if [ ! -f "$CONF_FILE" ]; then
CONF_FILE=$(find "$TMPDIR" -maxdepth 1 -type f -name '*.conf' -print -quit)
fi
Comment on lines +261 to +263

@coderabbitai coderabbitai Bot May 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Fail fast when multiple extracted .conf files are present.

find ... -print -quit picks an arbitrary first match when there are multiple .conf files (and no pre-existing openvpn.conf), which can select the wrong profile non-deterministically.

Suggested fix 
  CONF_FILE="$TMPDIR/openvpn.conf"
  if [ ! -f "$CONF_FILE" ]; then
-    CONF_FILE=$(find "$TMPDIR" -maxdepth 1 -type f -name '*.conf' -print -quit)
+    CONF_COUNT=$(find "$TMPDIR" -maxdepth 1 -type f -name '*.conf' | wc -l)
+    if [ "$CONF_COUNT" -ne 1 ]; then
+      echo "ERROR: expected exactly one OpenVPN config file, found $CONF_COUNT" >&2
+      rm -rf -- "$TMPDIR"
+      return 1
+    fi
+    CONF_FILE=$(find "$TMPDIR" -maxdepth 1 -type f -name '*.conf' -print -quit)
  fi
  if [ -z "$CONF_FILE" ]; then

Also applies to: 264-268

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@images/common/utils.sh` around lines 261 - 263, The current logic in
images/common/utils.sh that sets CONF_FILE by using find ... -print -quit (when
CONF_FILE doesn't exist) can pick an arbitrary .conf when multiple are present;
change it to explicitly collect all matches from TMPDIR (e.g., into an array or
list), then: if exactly one match set CONF_FILE to that path, if zero leave
CONF_FILE unset, and if more than one log an error and exit (fail fast). Apply
the same change to the other similar block referenced (lines 264-268) so both
places check the number of .conf files and error out deterministically instead
of using find -print -quit.

if [ -z "$CONF_FILE" ]; then
echo "ERROR: no OpenVPN config file found after extraction" >&2
rm -rf -- "$TMPDIR"
return 1
fi
mv -f -- "$CONF_FILE" openvpn.conf || {
rm -rf -- "$TMPDIR"
return 1
}
# Move the remaining extracted files, but leave any extra .conf files behind
# so only the normalized openvpn.conf is used at runtime.
find "$TMPDIR" -mindepth 1 -maxdepth 1 ! -name '*.conf' -exec mv -f -- {} . \; || {
rm -rf -- "$TMPDIR"
return 1
}
rm -rf -- "$TMPDIR"
}

function crl_download {
Expand Down
3 changes: 1 addition & 2 deletions images/openwisp_openvpn/openvpn.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,5 @@ openvpn_config
openvpn_config_checksum

if [ "${OFILE}" != "${NFILE}" ]; then
openvpn_config_download
supervisorctl restart openvpn
openvpn_config_download && supervisorctl restart openvpn
fi
2 changes: 1 addition & 1 deletion images/openwisp_openvpn/supervisord.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ pidfile=/supervisord.pid
[program:openvpn]
user=root
directory=/
command=/usr/sbin/openvpn --config %(ENV_VPN_NAME)s.conf
command=/usr/sbin/openvpn --config openvpn.conf
autostart=true
autorestart=true
stopsignal=INT
Expand Down
56 changes: 55 additions & 1 deletion tests/runtests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
from selenium.webdriver.common.by import By
from selenium.webdriver.support import expected_conditions as EC
from selenium.webdriver.support.ui import WebDriverWait
from utils import TestUtilities
from utils import TestConfig, TestUtilities


class Pretest(TestUtilities, unittest.TestCase):
Expand Down Expand Up @@ -58,6 +58,60 @@ def test_wait_for_services(self):
self.fail(f"All celery workers are not online: {online_workers}")


class OpenVPNContainerTest(TestConfig, unittest.TestCase):
def test_openvpn_config_download_with_whitespace_name(self):
"""Ensure the OpenVPN container normalizes whitespace config names."""
script = r"""
set -e
TMPDIR=$(mktemp -d)
mkdir "$TMPDIR/archive" "$TMPDIR/work"
printf 'new config' > "$TMPDIR/archive/my vpn.conf"
printf 'pem' > "$TMPDIR/archive/client.pem"
tar -czf "$TMPDIR/vpn.tar.gz" -C "$TMPDIR/archive" \
'my vpn.conf' 'client.pem'
printf 'stale config' > "$TMPDIR/work/openvpn.conf"
cd "$TMPDIR/work"
source /utils.sh
curl() {
output=''
while [ "$#" -gt 0 ]; do
if [ "$1" = '--output' ]; then
shift
output="$1"
fi
shift || true
done
if [ "$output" = 'vpn.tar.gz' ]; then
cp "$TMPDIR/vpn.tar.gz" "$output"
elif [ "$output" = 'checksum' ]; then
printf '%s\n' 'checksum' > "$output"
else
return 1
fi
}
API_INTERNAL='https://api.internal'
UUID='vpn-uuid'
KEY='vpn-key'
openvpn_config_download
test "$(cat openvpn.conf)" = 'new config'
test ! -f 'my vpn.conf'
test "$(cat checksum)" = 'checksum'
test "$(stat -c '%a' client.pem)" = '600'
rm -rf "$TMPDIR"
"""
res = subprocess.run(
["docker", "compose", "exec", "-T", "openvpn", "sh", "-c", script],
cwd=self.root_location,
capture_output=True,
text=True,
)
self.assertEqual(
res.returncode,
0,
f"stdout:\n{res.stdout}\nstderr:\n{res.stderr}",
)


class TestServices(TestUtilities, unittest.TestCase):
custom_static_token = None

Expand Down
Loading