Fix the review findings in connect-wireguard

The wireguard-go choice now survives sudo (it is passed through env), sudo is
used only when the job is not root, a configuration field with a line break is
refused, only the key values of a full configuration are masked, the key file
is removed when wg-quick fails, and the guide calls @main until the release.

Refs #184
Task: MIL-012#3

actions/connect-wireguard/connect-wireguard.sh

@@ -15,7 +15,17 @@
 WG_DIR="${WG_DIR:-/etc/wireguard}"
 WG_QUICK="${WG_QUICK:-wg-quick}"
 WG_PING="${WG_PING:-ping}"
-SUDO="${WG_SUDO-sudo}"
+# sudo is used only when the job is not root: a job container runs as root and
+# often has no sudo. WG_SUDO overrides it (empty for none).
+if [ -n "${WG_SUDO+x}" ]; then
+  SUDO="$WG_SUDO"
+elif [ "$(id -u)" = "0" ]; then
+  SUDO=""
+else
+  SUDO="sudo"
+fi
+# variables that must reach wg-quick through sudo, which resets the environment
+WG_QUICK_ENV=()
 
 fail() {
   echo "::error::[$1] $2"
@@ -23,14 +33,24 @@ fail() {
 }
 
 mask_secrets() {
-  local value line
+  local value line key
   for value in "${WG_PRIVATE_KEY:-}" "${WG_PRESHARED_KEY:-}"; do
     [ -n "$value" ] && echo "::add-mask::$value"
   done
-  # every line of a full configuration is masked, so a key cannot leak in pieces
+  # a full configuration: mask the value of every PrivateKey and PresharedKey
+  # line, and nothing else, so the rest of the log stays readable
   while IFS= read -r line; do
-    line="${line#"${line%%[![:space:]]*}"}"
+    line="${line//$'␍'/}"
-    [ -n "$line" ] && echo "::add-mask::$line"
+    key="${line%%=*}"
+    key="${key//[[:space:]]/}"
+    case "${key,,}" in
+      privatekey | presharedkey)
+        value="${line#*=}"
+        value="${value#"${value%%[![:space:]]*}"}"
+        value="${value%"${value##*[![:space:]]}"}"
+        [ -n "$value" ] && echo "::add-mask::$value"
+        ;;
+    esac
   done <<<"${WG_CONFIG:-}"
   return 0
 }
@@ -51,6 +71,11 @@ render_config() {
     [ -n "${!name:-}" ] ||
       fail configuration "set config, or all of private-key, address, peer-public-key, endpoint and allowed-ips ($name is empty)."
   done
+  for name in WG_PRIVATE_KEY WG_ADDRESS WG_PEER_PUBLIC_KEY WG_ENDPOINT WG_ALLOWED_IPS WG_PRESHARED_KEY WG_DNS; do
+    # a line break would add directives (PostUp runs as root) to the file
+    [[ "${!name:-}" != *[[:cntrl:]]* ]] ||
+      fail configuration "$name must be one line without control characters."
+  done
   [[ "$WG_KEEPALIVE" =~ ^[0-9]+$ ]] ||
     fail configuration "persistent-keepalive must be a number of seconds."
   printf '[Interface]\nPrivateKey = %s\nAddress = %s\n' "$WG_PRIVATE_KEY" "$WG_ADDRESS"
@@ -77,7 +102,7 @@ choose_implementation() {
     return 0
   fi
   if command -v wireguard-go >/dev/null 2>&1 || $SUDO apt-get install -y -qq wireguard-go 2>/dev/null; then
-    export WG_QUICK_USERSPACE_IMPLEMENTATION=wireguard-go
+    WG_QUICK_ENV=(WG_QUICK_USERSPACE_IMPLEMENTATION=wireguard-go)
     echo "No kernel WireGuard module; using wireguard-go."
   else
     fail environment "the runner has no WireGuard kernel module and wireguard-go could not be installed; the runner container needs NET_ADMIN and /dev/net/tun (see guides/vpn-wireguard.md)."
@@ -97,8 +122,12 @@ tunnel_up() {
   $SUDO install -d -m 700 "$WG_DIR"
   (umask 077 && printf '%s\n' "$rendered" | $SUDO tee "$conf" >/dev/null)
   $SUDO chmod 600 "$conf"
-  $SUDO "$WG_QUICK" up "$WG_INTERFACE" ||
+  # env keeps the userspace variable through sudo; the file holds the key, so
+  # it is removed when the interface does not come up
+  if ! $SUDO env ${WG_QUICK_ENV[@]+"${WG_QUICK_ENV[@]}"} "$WG_QUICK" up "$WG_INTERFACE"; then
+    $SUDO rm -f "$conf"
     fail connection "wg-quick could not bring $WG_INTERFACE up."
+  fi
   if [ -n "${WG_HEALTH_CHECK_HOST:-}" ]; then
     "$WG_PING" -c 3 -W 5 "$WG_HEALTH_CHECK_HOST" >/dev/null 2>&1 ||
       fail connection "health-check-host '$WG_HEALTH_CHECK_HOST' does not answer through the tunnel."

guides/vpn-wireguard.md

@@ -4,7 +4,8 @@ The composite action `actions/connect-wireguard` brings a WireGuard tunnel up on
 a Gitea Actions runner, so the next steps of the same job (a deployment over
 SSH, a database migration) can reach hosts that are not on the internet. It is
 the WireGuard counterpart of the Tailscale action. It is **experimental**: it
-lives on a branch until a trial shows it is wanted.
+is on `main` but not released, and it has not yet run on a runner with a real
+tunnel (MIL-012).
 
 A tunnel belongs to one job: every job that deploys brings its own tunnel up
 and takes it down. It cannot be a reusable workflow, because a called workflow
@@ -20,7 +21,7 @@ jobs:
       - uses: actions/checkout@v4.2.2
 
       - name: Connect the VPN
-        uses: TirSystem/github-action/actions/connect-wireguard@v2
+        uses: TirSystem/github-action/actions/connect-wireguard@main
         with:
           config: ${{ secrets.WG_CONFIG }}
           health-check-host: 10.8.0.1
@@ -30,17 +31,17 @@ jobs:
 
       - name: Disconnect the VPN
         if: always()
-        uses: TirSystem/github-action/actions/connect-wireguard@v2
+        uses: TirSystem/github-action/actions/connect-wireguard@main
         with:
           action: down
 ```
 
 If the short form does not resolve on your instance (Gitea looks for the action
 on the host set in `DEFAULT_ACTIONS_URL`), use the full address:
-`https://git.tirsystem.com/TirSystem/github-action/actions/connect-wireguard@v2`.
+`https://git.tirsystem.com/TirSystem/github-action/actions/connect-wireguard@main`.
 
-To try the branch before it is released, replace `@v2` with
+Until release 2.2.0 the actions exist only on `main`, so the examples call
-`@wireguard-vpn-action`.
+`@main`; `@v2` does not contain them yet. After the release, use `@v2`.
 
 ## Inputs
 
@@ -71,8 +72,9 @@ To try the branch before it is released, replace `@v2` with
 A tunnel needs a network interface, so the runner needs more than a plain job
 container:
 
-- the job runs as root or can use `sudo` (the action installs
+- the job runs as root, or `sudo` is available (the action uses `sudo` only
-  `wireguard-tools` with `apt` when it is missing, so a Debian or Ubuntu image);
+  when the job is not root, and installs `wireguard-tools` with `apt` when it is
+  missing, so a Debian or Ubuntu image);
 - the container has the capability `NET_ADMIN` and the device `/dev/net/tun`.
   In the `act_runner` configuration set under `container:`
   `options: "--cap-add=NET_ADMIN --device=/dev/net/tun"`;
@@ -85,9 +87,13 @@ privilege.
 
 ## What the action does
 
-- masks the keys in the log before anything else;
+- masks the keys in the log before anything else (the values of `PrivateKey`
+  and `PresharedKey` of a full configuration, and the key inputs);
+- refuses an input that holds a line break, because an extra line in the file
+  could add a `PostUp` command that runs as root;
 - writes `/etc/wireguard/<interface>.conf` with mode 600 and brings it up with
-  `wg-quick`;
+  `wg-quick` (through `env`, so the `wireguard-go` choice survives `sudo`); the
+  file is removed again when the interface does not come up;
 - prints errors as `::error::[category] message`, with the categories
   `configuration`, `environment` and `connection`;
 - on `down` takes the interface down and deletes the file.

tests/test_connect_wireguard.py

@@ -131,6 +131,40 @@ class TestUp:
         assert "::error::[connection]" in result.stdout
 
 
+def make_exec(path: Path, body: str) -> Path:
+    path.write_text(body, encoding="utf-8")
+    path.chmod(path.stat().st_mode | stat.S_IXUSR)
+    return path
+
+
+class TestEnvironment:
+    def test_the_userspace_variable_reaches_wg_quick_through_sudo(self, tmp_path):
+        # a sudo that resets the environment, as the real one does
+        sudo = make_exec(
+            tmp_path / "sudo-reset",
+            '#!/usr/bin/env bash\nexec env -i "PATH=$PATH" "STUB_LOG=$STUB_LOG" "$@"\n',
+        )
+        quick = make_exec(
+            tmp_path / "quick-env",
+            '#!/usr/bin/env bash\necho "$WG_QUICK_USERSPACE_IMPLEMENTATION $*" >> "$STUB_LOG"\n',
+        )
+        result = run(tmp_path, "up", **FIELDS, WG_SUDO=str(sudo), WG_QUICK=str(quick))
+        assert result.returncode == 0, result.stdout + result.stderr
+        assert "wireguard-go up wg0" in (tmp_path / "log").read_text(encoding="utf-8")
+
+    def test_a_field_with_a_line_break_is_refused(self, tmp_path):
+        result = run(tmp_path, "up", **{**FIELDS, "WG_ALLOWED_IPS": "10.8.0.0/24\nPostUp = id"})
+        assert result.returncode == 1
+        assert "WG_ALLOWED_IPS must be one line" in result.stdout
+        assert not (tmp_path / "etc" / "wg0.conf").exists()
+
+    def test_the_key_file_is_removed_when_wg_quick_fails(self, tmp_path):
+        failing = make_exec(tmp_path / "quick-fail", "#!/usr/bin/env bash\nexit 1\n")
+        result = run(tmp_path, "up", **FIELDS, WG_QUICK=str(failing))
+        assert result.returncode == 1
+        assert not (tmp_path / "etc" / "wg0.conf").exists()
+
+
 class TestDownAndMask:
     def test_down_removes_the_config(self, tmp_path):
         run(tmp_path, "up", **FIELDS)
@@ -141,7 +175,8 @@ class TestDownAndMask:
     def test_mask_prints_every_secret(self, tmp_path):
         result = run(tmp_path, "mask", WG_PRIVATE_KEY=KEY, WG_CONFIG="[Interface]\nPrivateKey = abc")
         assert f"::add-mask::{KEY}" in result.stdout
-        assert "::add-mask::PrivateKey = abc" in result.stdout
+        assert "::add-mask::abc" in result.stdout
+        assert "[Interface]" not in result.stdout  # only the key values are masked
 
     def test_unknown_command_fails(self, tmp_path):
         assert run(tmp_path, "sideways").returncode == 1