From 71abfccd150edbf9cb13fa2c3fa9449ffb47bd7c Mon Sep 17 00:00:00 2001 From: Derek Date: Sun, 1 Nov 2026 14:44:22 +1100 Subject: [PATCH 01/24] fix: stop compose and .env.example overriding profiles docker-compose.yaml defaulted every CULVERT_* setting, and env beats the profile, so CULVERT_PROFILE=corporate ran with OIDC, the TCP listener and full tunnel off, and edge-fleet ran without routing control on 10.8.0.0. A copied .env.example also cleared the corporate group gate. Settings now pass through only when the operator sets them. The TLS paths onto the compose-mounted volume keep their defaults. --- .env.example | 68 +++++++++++++---------- docker-compose.yaml | 111 +++++++++++++++++++------------------- tests/unit/test_config.py | 79 +++++++++++++++++++++++++++ 3 files changed, 175 insertions(+), 83 deletions(-) diff --git a/.env.example b/.env.example index 03d0876..938ab3e 100644 --- a/.env.example +++ b/.env.example @@ -4,15 +4,25 @@ # All configuration uses the CULVERT_* environment variable prefix. # See README.md for full documentation. # +# Every CULVERT_* value set here overrides CULVERT_PROFILE, so the settings below +# stay commented at their defaults. Uncomment only what you mean to change. +# # License: Apache-2.0 # Copyright: (c) 2026 HYPERI PTY LIMITED +#=============================================================================== +# Site Profile +#=============================================================================== +# Loads profiles/.yaml (home, corporate, travel, mobile, edge-fleet, +# default) as the base for everything below, or a path to your own YAML. +#CULVERT_PROFILE="corporate" + #=============================================================================== # Protocol Selection #=============================================================================== # VPN protocol to run. Options: openvpn, wireguard, both # Default: openvpn -CULVERT_PROTOCOL="openvpn" +#CULVERT_PROTOCOL="openvpn" #=============================================================================== # WireGuard Configuration (only when CULVERT_PROTOCOL includes wireguard) @@ -68,16 +78,16 @@ CULVERT_SERVER_CN="vpn.example.com" # - Optional: per-listener OAuth2 enable/disable # UDP Listener (primary - best performance; the default) -CULVERT_UDP_ENABLED="true" -CULVERT_UDP_PORT="1194" -CULVERT_UDP_NETWORK="10.8.0.0" -CULVERT_UDP_NETMASK="255.255.255.0" +#CULVERT_UDP_ENABLED="true" +#CULVERT_UDP_PORT="1194" +#CULVERT_UDP_NETWORK="10.8.0.0" +#CULVERT_UDP_NETMASK="255.255.255.0" # HTTPS Listener: OpenVPN wrapped in TLS on port 443 -- opt-in -CULVERT_HTTPS_ENABLED="false" -CULVERT_HTTPS_PORT="443" -CULVERT_HTTPS_NETWORK="10.8.2.0" -CULVERT_HTTPS_NETMASK="255.255.255.0" +#CULVERT_HTTPS_ENABLED="false" +#CULVERT_HTTPS_PORT="443" +#CULVERT_HTTPS_NETWORK="10.8.2.0" +#CULVERT_HTTPS_NETMASK="255.255.255.0" # Internal port stunnel forwards decrypted traffic to (localhost only) #CULVERT_HTTPS_INTERNAL_PORT=1195 # TLS certificate + key for the HTTPS-tunnelled listeners. REQUIRED when @@ -89,26 +99,26 @@ CULVERT_HTTPS_NETMASK="255.255.255.0" #CULVERT_STUNNEL_KEY="/etc/vpn/tls/privkey.pem" # TCP Listener (proxy fallback) -- opt-in -CULVERT_TCP_ENABLED="false" -CULVERT_TCP_PORT="1194" -CULVERT_TCP_NETWORK="10.8.1.0" -CULVERT_TCP_NETMASK="255.255.255.0" +#CULVERT_TCP_ENABLED="false" +#CULVERT_TCP_PORT="1194" +#CULVERT_TCP_NETWORK="10.8.1.0" +#CULVERT_TCP_NETMASK="255.255.255.0" #=============================================================================== # DNS and Routing #=============================================================================== # DNS servers pushed to clients -CULVERT_DNS1="1.1.1.1" -CULVERT_DNS2="1.0.0.1" +#CULVERT_DNS1="1.1.1.1" +#CULVERT_DNS2="1.0.0.1" # Routing mode # true = all traffic through VPN (full tunnel) # false = only VPN network (split tunnel) -CULVERT_FULL_TUNNEL="false" +#CULVERT_FULL_TUNNEL="false" # Additional routes to push (comma-separated CIDR) # Example: 10.1.0.0/16,192.168.1.0/24 -CULVERT_PUSH_ROUTES="" +#CULVERT_PUSH_ROUTES="" #=============================================================================== # Routing Control (opt-in FORWARD filtering) @@ -148,7 +158,7 @@ CULVERT_PUSH_ROUTES="" #=============================================================================== # Performance Tuning #=============================================================================== -# Tuning is DATA, carried by a profile YAML -- see CULVERT_PROFILE above and +# Tuning is DATA, carried by a profile YAML -- see Site Profile above and # profiles/*.yaml. The shipped tuning profiles are: # default - robust general-purpose baseline (mssfix 1420, OS-auto buffers) # mobile - 5G / tether / hotel wifi (WireGuard MTU 1280, conservative @@ -176,7 +186,7 @@ CULVERT_PUSH_ROUTES="" # Server limits # max-clients: "auto" calculates from CPU/memory (formula: min(cores*64, GB*650, 1000)) # Set explicit number to override auto-calculation -CULVERT_MAX_CLIENTS="auto" +#CULVERT_MAX_CLIENTS="auto" #CULVERT_RENEG_SEC=3600 #=============================================================================== @@ -209,7 +219,7 @@ CULVERT_MAX_CLIENTS="auto" # Logging configuration # CULVERT_LOG_MODE: "file" (default), "stdout" (K8s/containers), "both" # Kubernetes/EKS/Rancher: Use "stdout" for container logging pipelines -CULVERT_LOG_MODE="file" +#CULVERT_LOG_MODE="file" #CULVERT_VERB=3 #CULVERT_MUTE=20 @@ -226,7 +236,7 @@ CULVERT_LOG_MODE="file" # Access: https://:/ to list files (with the token). # Enable/disable client download server -CULVERT_CLIENT_DOWNLOAD_ENABLED="false" +#CULVERT_CLIENT_DOWNLOAD_ENABLED="false" # Bearer token - REQUIRED when enabled (generate: openssl rand -hex 32) #CULVERT_CLIENT_DOWNLOAD_TOKEN= @@ -268,7 +278,7 @@ CULVERT_LOGS_PATH="./logs" # external secrets backend via CULVERT_SECRETS_PROVIDER. # Best for: production Kubernetes with enterprise PKI. -CULVERT_PKI_MODE="local" +#CULVERT_PKI_MODE="local" #------------------------------------------------------------------------------- # LOCAL PKI Settings (CULVERT_PKI_MODE=local) @@ -282,11 +292,11 @@ CULVERT_PKI_MODE="local" #CULVERT_CA_CN="Culvert CA" # Key type: ec (ECDSA - recommended) or rsa -CULVERT_KEY_TYPE="ec" +#CULVERT_KEY_TYPE="ec" # For EC: secp384r1 (CNSA 2.0), secp256r1 # For RSA: 4096 (CNSA 2.0), 2048 -CULVERT_KEY_SIZE="secp384r1" +#CULVERT_KEY_SIZE="secp384r1" # OpenVPN data-channel cipher order (NCP). AES-256-GCM first is the CNSA 2.0 / # FIPS-compliant default. Reorder to prefer ChaCha20-Poly1305 for AES-NI-less @@ -512,7 +522,7 @@ CULVERT_KEY_SIZE="secp384r1" # For example: HTTPS requires OAuth2, UDP is cert-only for automation. # Global OAuth2 toggle (default for all listeners if per-listener not set) -CULVERT_OAUTH2_ENABLED="false" +#CULVERT_OAUTH2_ENABLED="false" # Per-listener OAuth2 enable/disable (defaults to CULVERT_OAUTH2_ENABLED if not set) # Set to false to use certificate-only auth on specific listeners @@ -549,8 +559,10 @@ CULVERT_OAUTH2_TLS_KEY="/etc/vpn/oauth2-tls/privkey.pem" # Group-based access control # Only members of these groups can connect (comma-separated) -# Use your IdP's group identifiers (GUIDs for Entra ID, names for others) -CULVERT_OAUTH2_VALIDATE_GROUPS="" +# Use your IdP's group identifiers (GUIDs for Entra ID, names for others). +# Left unset, any authenticated user connects (the corporate profile gates on +# "vpn-users"). An empty value set here clears a profile's gate. +#CULVERT_OAUTH2_VALIDATE_GROUPS="vpn-users" # Session secret (generate with: openssl rand -hex 16) CULVERT_OAUTH2_HTTP_SECRET="GENERATE_ME_openssl_rand_hex_16" @@ -558,7 +570,7 @@ CULVERT_OAUTH2_HTTP_SECRET="GENERATE_ME_openssl_rand_hex_16" # OIDC scopes. Default matches the code (openid,profile,email). Add # offline_access for IdPs that issue refresh tokens through it (e.g. # Entra ID); some IdPs reject unknown scopes, so it is not the default. -CULVERT_OAUTH2_SCOPES="openid,profile,email" +#CULVERT_OAUTH2_SCOPES="openid,profile,email" #=============================================================================== # Container Image (REQUIRED by docker-compose.yaml) diff --git a/docker-compose.yaml b/docker-compose.yaml index fe5a27e..3724911 100644 --- a/docker-compose.yaml +++ b/docker-compose.yaml @@ -111,6 +111,10 @@ services: # Mount wildcard or server-specific certificate for HTTPS callback endpoint - ${OAUTH2_TLS_CERT_PATH:-/opt/openvpn/certs}:/etc/vpn/oauth2-tls:ro + # Every CULVERT_* setting below passes through ONLY when you set it (shell or + # .env). Anything left unset comes from CULVERT_PROFILE, then the code + # defaults. A default written here would override the profile: a corporate + # profile would lose OIDC and an edge-fleet one its routing control. environment: # Server identity - MUST match DNS/certificate - CULVERT_SERVER_CN=${CULVERT_SERVER_CN:-vpn.example.com} @@ -118,59 +122,59 @@ services: # Listener configuration. Default is the simplest working server: # OpenVPN UDP only. The TCP and HTTPS listeners are deliberate opt-ins. # UDP - Primary (best performance) - - CULVERT_UDP_ENABLED=${CULVERT_UDP_ENABLED:-true} - - CULVERT_UDP_PORT=${CULVERT_UDP_PORT:-1194} - - CULVERT_UDP_NETWORK=${CULVERT_UDP_NETWORK:-10.8.0.0} - - CULVERT_UDP_NETMASK=${CULVERT_UDP_NETMASK:-255.255.255.0} + - CULVERT_UDP_ENABLED + - CULVERT_UDP_PORT + - CULVERT_UDP_NETWORK + - CULVERT_UDP_NETMASK # HTTPS - OpenVPN wrapped in TLS on port 443 (opt-in) - - CULVERT_HTTPS_ENABLED=${CULVERT_HTTPS_ENABLED:-false} - - CULVERT_HTTPS_PORT=${CULVERT_HTTPS_PORT:-443} - - CULVERT_HTTPS_NETWORK=${CULVERT_HTTPS_NETWORK:-10.8.2.0} - - CULVERT_HTTPS_NETMASK=${CULVERT_HTTPS_NETMASK:-255.255.255.0} + - CULVERT_HTTPS_ENABLED + - CULVERT_HTTPS_PORT + - CULVERT_HTTPS_NETWORK + - CULVERT_HTTPS_NETMASK # TCP - Proxy fallback (opt-in) - - CULVERT_TCP_ENABLED=${CULVERT_TCP_ENABLED:-false} - - CULVERT_TCP_PORT=${CULVERT_TCP_PORT:-1194} - - CULVERT_TCP_NETWORK=${CULVERT_TCP_NETWORK:-10.8.1.0} - - CULVERT_TCP_NETMASK=${CULVERT_TCP_NETMASK:-255.255.255.0} + - CULVERT_TCP_ENABLED + - CULVERT_TCP_PORT + - CULVERT_TCP_NETWORK + - CULVERT_TCP_NETMASK # DNS servers pushed to clients - - CULVERT_DNS1=${CULVERT_DNS1:-1.1.1.1} - - CULVERT_DNS2=${CULVERT_DNS2:-1.0.0.1} + - CULVERT_DNS1 + - CULVERT_DNS2 # Routing - - CULVERT_FULL_TUNNEL=${CULVERT_FULL_TUNNEL:-false} + - CULVERT_FULL_TUNNEL - CULVERT_PUSH_ROUTES - CULVERT_DNS_DOMAIN # Routing control: who clients may reach, and who may reach back down the # tunnels. Off by default (clients route wherever the host can). Turning it # on denies client-to-client and unsolicited inbound; see .env.example. - - CULVERT_ROUTING_CONTROL_ENABLED=${CULVERT_ROUTING_CONTROL_ENABLED:-false} - - CULVERT_CLIENT_ISOLATION=${CULVERT_CLIENT_ISOLATION:-true} + - CULVERT_ROUTING_CONTROL_ENABLED + - CULVERT_CLIENT_ISOLATION - CULVERT_ALLOWED_DESTINATIONS - CULVERT_DOWNSTREAM_ADMIN_CIDRS # Clients cannot reach 169.254.0.0/16, which on a cloud instance is the # metadata service and this host's credentials. On by default, and applies # whether or not the routing control above is enabled. - - CULVERT_BLOCK_LINK_LOCAL=${CULVERT_BLOCK_LINK_LOCAL:-true} + - CULVERT_BLOCK_LINK_LOCAL # Site profile: loads profiles/.yaml as a base for everything above - CULVERT_PROFILE # WireGuard (opt-in: set CULVERT_PROTOCOL=wireguard, or both to run it # alongside OpenVPN, then uncomment the knobs you need) - # - CULVERT_PROTOCOL=${CULVERT_PROTOCOL:-openvpn} - # - CULVERT_WG_NETWORK=${CULVERT_WG_NETWORK:-10.8.3.0/24} - # - CULVERT_WG_PORT=${CULVERT_WG_PORT:-51820} - # - CULVERT_WG_MTU=${CULVERT_WG_MTU:-1420} - # - CULVERT_WG_PERSISTENT_KEEPALIVE=${CULVERT_WG_PERSISTENT_KEEPALIVE:-25} + # - CULVERT_PROTOCOL + # - CULVERT_WG_NETWORK + # - CULVERT_WG_PORT + # - CULVERT_WG_MTU + # - CULVERT_WG_PERSISTENT_KEEPALIVE # WireGuard over HTTPS via wstunnel (a further opt-in on top of WireGuard) - # - CULVERT_WG_HTTPS_TUNNEL_ENABLED=${CULVERT_WG_HTTPS_TUNNEL_ENABLED:-false} - # - CULVERT_WG_HTTPS_TUNNEL_PORT=${CULVERT_WG_HTTPS_TUNNEL_PORT:-4443} + # - CULVERT_WG_HTTPS_TUNNEL_ENABLED + # - CULVERT_WG_HTTPS_TUNNEL_PORT # Network profile: "default" (fast internet) or "wireless" (4G/mobile optimized) - - CULVERT_NETWORK_PROFILE=${CULVERT_NETWORK_PROFILE:-default} + - CULVERT_NETWORK_PROFILE # Performance tuning (optional - profile sets sensible defaults) - CULVERT_SNDBUF @@ -179,51 +183,50 @@ services: - CULVERT_MSSFIX - CULVERT_KEEPALIVE_PING - CULVERT_KEEPALIVE_TIMEOUT - - CULVERT_MAX_CLIENTS=${CULVERT_MAX_CLIENTS:-auto} + - CULVERT_MAX_CLIENTS - CULVERT_RENEG_SEC # Concurrent connections per client identity (laptop + phone). Set # ALLOW_SHARED_CLIENTS=false to make each credential exclusive. - - CULVERT_ALLOW_SHARED_CLIENTS=${CULVERT_ALLOW_SHARED_CLIENTS:-true} - - CULVERT_SHARED_CLIENT_SLOTS=${CULVERT_SHARED_CLIENT_SLOTS:-2} - - CULVERT_LOG_MODE=${CULVERT_LOG_MODE:-file} + - CULVERT_ALLOW_SHARED_CLIENTS + - CULVERT_SHARED_CLIENT_SLOTS + - CULVERT_LOG_MODE - CULVERT_VERB - CULVERT_MUTE # Observability listener: always serves health probes; /metrics is # served when CULVERT_METRICS_ENABLED=true - - CULVERT_METRICS_ENABLED=${CULVERT_METRICS_ENABLED:-false} - - CULVERT_METRICS_ADDR=${CULVERT_METRICS_ADDR:-0.0.0.0:9090} + - CULVERT_METRICS_ENABLED + - CULVERT_METRICS_ADDR # Client config download server (REQUIRES a bearer token when enabled: # the served configs embed client private keys, so it fails closed) - - CULVERT_CLIENT_DOWNLOAD_ENABLED=${CULVERT_CLIENT_DOWNLOAD_ENABLED:-false} - - CULVERT_CLIENT_DOWNLOAD_PORT=${CULVERT_CLIENT_DOWNLOAD_PORT:-8443} + - CULVERT_CLIENT_DOWNLOAD_ENABLED + - CULVERT_CLIENT_DOWNLOAD_PORT - CULVERT_CLIENT_DOWNLOAD_TOKEN # PKI settings (CNSA 2.0 compliant defaults) # ORG_NAME names the CA: "Acme" -> "Acme VPN CA". CA_CN overrides that - # derivation outright, so it is passed through empty rather than - # defaulted - set it only if you want to name the CA yourself. + # derivation outright - set it only if you want to name the CA yourself. - CULVERT_ORG_NAME - CULVERT_CA_CN - - CULVERT_KEY_TYPE=${CULVERT_KEY_TYPE:-ec} - - CULVERT_KEY_SIZE=${CULVERT_KEY_SIZE:-secp384r1} + - CULVERT_KEY_TYPE + - CULVERT_KEY_SIZE # OpenVPN data-channel cipher order. AES-256-GCM first is the CNSA/FIPS # default; a profile (or this var) may prefer ChaCha20 -- which leaves CNSA. - - CULVERT_DATA_CIPHERS=${CULVERT_DATA_CIPHERS:-AES-256-GCM:CHACHA20-POLY1305} + - CULVERT_DATA_CIPHERS # Local-PKI certificate lifetimes (days). Defaults: 10-year CA, 2-year # leaf. Raise for a set-and-forget deployment with no rotation operator, # e.g. 7300 (20y), so the server cert cannot silently expire. - - CULVERT_CA_EXPIRE_DAYS=${CULVERT_CA_EXPIRE_DAYS:-3650} - - CULVERT_CERT_EXPIRE_DAYS=${CULVERT_CERT_EXPIRE_DAYS:-730} - - CULVERT_CRL_DAYS=${CULVERT_CRL_DAYS:-180} + - CULVERT_CA_EXPIRE_DAYS + - CULVERT_CERT_EXPIRE_DAYS + - CULVERT_CRL_DAYS # "local" mints a CA on first start. "external" takes the CA and server # keypair from a secrets backend instead, which needs every # CULVERT_SECRETS_* path below - see the External PKI section of # .env.example. TC_KEY_PATH is what lets more than one server share # clients; a single server can leave it unset and mint its own. - - CULVERT_PKI_MODE=${CULVERT_PKI_MODE:-local} + - CULVERT_PKI_MODE - CULVERT_SECRETS_PROVIDER - CULVERT_SECRETS_CA_CERT_PATH - CULVERT_SECRETS_SERVER_CERT_PATH @@ -232,7 +235,7 @@ services: - CULVERT_SECRETS_TC_KEY_PATH # OIDC SSO, opt-in (works with Entra ID, Okta, Keycloak, Google, Auth0, etc.) - - CULVERT_OAUTH2_ENABLED=${CULVERT_OAUTH2_ENABLED:-false} + - CULVERT_OAUTH2_ENABLED - CULVERT_OAUTH2_ISSUER - CULVERT_OAUTH2_CLIENT_ID - CULVERT_OAUTH2_CLIENT_SECRET @@ -242,29 +245,27 @@ services: - CULVERT_OAUTH2_HTTPS_ENABLED - CULVERT_OAUTH2_TCP_ENABLED # Per-listener OAuth2 callback ports (defaults: 9000=UDP, 9001=HTTPS, 9002=TCP) - - CULVERT_OAUTH2_UDP_PORT=${CULVERT_OAUTH2_UDP_PORT:-9000} - - CULVERT_OAUTH2_HTTPS_PORT=${CULVERT_OAUTH2_HTTPS_PORT:-9001} - - CULVERT_OAUTH2_TCP_PORT=${CULVERT_OAUTH2_TCP_PORT:-9002} + - CULVERT_OAUTH2_UDP_PORT + - CULVERT_OAUTH2_HTTPS_PORT + - CULVERT_OAUTH2_TCP_PORT - CULVERT_OAUTH2_HTTP_SECRET # Default scopes match the code default. Add offline_access for IdPs # that issue refresh tokens through it (e.g. Entra ID); some IdPs # reject unknown scopes, so it is not the default. - - CULVERT_OAUTH2_SCOPES=${CULVERT_OAUTH2_SCOPES:-openid,profile,email} + - CULVERT_OAUTH2_SCOPES # OAuth2 TLS (REQUIRED when CULVERT_OAUTH2_ENABLED=true - most IdPs - # require HTTPS redirect URIs). Paths inside the container; mount certs - # via the OAUTH2_TLS_CERT_PATH volume above. + # require HTTPS redirect URIs). These four default to the + # OAUTH2_TLS_CERT_PATH volume above, which only this file mounts. - CULVERT_OAUTH2_TLS_CERT=${CULVERT_OAUTH2_TLS_CERT:-/etc/vpn/oauth2-tls/fullchain.pem} - CULVERT_OAUTH2_TLS_KEY=${CULVERT_OAUTH2_TLS_KEY:-/etc/vpn/oauth2-tls/privkey.pem} # stunnel TLS (for the opt-in HTTPS tunnel - same certs as OAuth2 by default) # Note: Use fullchain (leaf + intermediate CA) for proper certificate verification - CULVERT_STUNNEL_CERT=${CULVERT_STUNNEL_CERT:-/etc/vpn/oauth2-tls/fullchain.pem} - CULVERT_STUNNEL_KEY=${CULVERT_STUNNEL_KEY:-/etc/vpn/oauth2-tls/privkey.pem} - # OAuth2 Branding (optional) - # Custom HTML template and assets for OAuth login page. The assets - # fallback repeats the code default because an empty-but-set env var - # would override it with "" and drop the baked-in branding. + # OAuth2 Branding (optional). Set ASSETS_PATH to a non-empty path or + # leave it unset: an empty value replaces the baked-in branding with "". - CULVERT_OAUTH2_TEMPLATE - - CULVERT_OAUTH2_ASSETS_PATH=${CULVERT_OAUTH2_ASSETS_PATH:-/etc/openvpn-auth-oauth2/assets} + - CULVERT_OAUTH2_ASSETS_PATH # Timezone - TZ=${TZ:-Australia/Sydney} diff --git a/tests/unit/test_config.py b/tests/unit/test_config.py index df355f6..4bfca3c 100644 --- a/tests/unit/test_config.py +++ b/tests/unit/test_config.py @@ -8,7 +8,11 @@ """Unit tests for Config.from_settings() using CULVERT_* env prefix.""" +import re +from pathlib import Path + import pytest +import yaml from lib.config import Config, endpoint_inside_zone @@ -485,6 +489,81 @@ def test_edge_fleet(self, clean_env, monkeypatch): assert cfg.log_mode == "stdout" +_PROJECT_ROOT = Path(__file__).resolve().parents[2] +_COMPOSE_DEFAULTED = re.compile(r"^\$\{(?P\w+):-(?P.*)\}$") +_ENV_FILE_LINE = re.compile(r'^(?P[A-Z0-9_]+)="(?P.*)"$') +# Compose owns these: every deployment names its own server, and the TLS pair +# lives on the volume only docker-compose.yaml mounts. +_COMPOSE_OWNED = {"server_cn", "stunnel_cert", "stunnel_key"} + + +def _env_example() -> dict[str, str]: + """The settings `cp .env.example .env` hands docker compose.""" + text = (_PROJECT_ROOT / ".env.example").read_text(encoding="utf-8") + matches = (_ENV_FILE_LINE.match(line) for line in text.splitlines()) + return {m["name"]: m["value"] for m in matches if m} + + +def _compose_environment(env_file: dict[str, str]) -> dict[str, str]: + """The CULVERT_* environment docker-compose.yaml gives the server container. + + Resolved as compose does: a bare entry passes through only when set, and + `${VAR:-default}` falls back to the default when VAR is unset or empty. + """ + compose = yaml.safe_load( + (_PROJECT_ROOT / "docker-compose.yaml").read_text(encoding="utf-8") + ) + resolved: dict[str, str] = {} + for entry in compose["services"]["openvpn"]["environment"]: + name, sep, value = entry.partition("=") + if not name.startswith("CULVERT_") or name == "CULVERT_PROFILE": + continue + if not sep: + if name in env_file: + resolved[name] = env_file[name] + continue + if defaulted := _COMPOSE_DEFAULTED.match(value): + value = env_file.get(defaulted["name"]) or defaulted["default"] + resolved[name] = value + return resolved + + +class TestComposeDefersToProfiles: + """docker-compose.yaml and .env.example must not override a profile. + + A setting the operator never chose that reaches the container as an env + var beats the profile, so `CULVERT_PROFILE=corporate` would run with OIDC + off and `edge-fleet` without routing control. + """ + + @pytest.mark.parametrize("env_file", ["none", ".env.example"]) + @pytest.mark.parametrize( + "profile", sorted(p.stem for p in (_PROJECT_ROOT / "profiles").glob("*.yaml")) + ) + def test_every_profile_setting_survives_compose( + self, clean_env, monkeypatch, profile, env_file + ): + path = _PROJECT_ROOT / "profiles" / f"{profile}.yaml" + keys = set(yaml.safe_load(path.read_text(encoding="utf-8"))) - _COMPOSE_OWNED + monkeypatch.setenv("CULVERT_PROFILE", str(path)) + expected = Config.from_settings() + + dotenv = _env_example() if env_file == ".env.example" else {} + for name, value in _compose_environment(dotenv).items(): + monkeypatch.setenv(name, value) + actual = Config.from_settings() + + overridden = { + key: (getattr(expected, key), getattr(actual, key)) + for key in sorted(keys) + if getattr(expected, key) != getattr(actual, key) + } + assert not overridden, ( + f"compose overrides profiles/{profile}.yaml (profile, container): " + f"{overridden}" + ) + + class TestStunnelValidation: """HTTPS listener requires stunnel cert/key.""" From c37351f89c88f3b39ca6f5fe3e50d14a07dcbba4 Mon Sep 17 00:00:00 2001 From: Derek Date: Sun, 1 Nov 2026 14:46:34 +1100 Subject: [PATCH 02/24] fix: hold one lock across WireGuard issue and revoke The allocation lock covered only allocations.json, so a reissue that read a peer before a concurrent revoke could write the revoked key back into wg0.conf and sync it to the running interface after the revoke reported success. generate-client, revoke-client and the server's own wg0.conf rebuild now hold a lifecycle lock from the first read of peers to the live sync. --- scripts/generate-client.py | 217 ++++++++++++++-------------- scripts/lib/wireguard.py | 61 +++++--- scripts/revoke-client.py | 117 +++++++-------- tests/unit/test_client_lifecycle.py | 82 +++++++++++ 4 files changed, 288 insertions(+), 189 deletions(-) diff --git a/scripts/generate-client.py b/scripts/generate-client.py index d2893ca..3e0b095 100644 --- a/scripts/generate-client.py +++ b/scripts/generate-client.py @@ -471,128 +471,129 @@ def generate_wireguard_configs( peers_dir = wg_dir / "peers" peers_dir.mkdir(parents=True, exist_ok=True) - # Generate or load server keys - server_private, server_public = wireguard.generate_server_keys(cfg.pki_dir) - - slots = len(pubkeys) if pubkeys else cfg.shared_client_slots - if slots > 1: - logger.info( - "Issuing concurrent WireGuard device slots", - client=client_name, - slots=slots, - ) - - # Lowering the slot count does not retire the peers already issued above it: - # they keep their key and their allocation, so the server still accepts them. - # Reissuing must not delete a credential, so say so instead of doing it. - surplus = wireguard.existing_peer_ids(cfg.pki_dir, client_name)[slots:] - if surplus: - logger.warning( - "Client still has WireGuard peers above the configured slot count;" - " they remain able to connect until revoked", - client=client_name, - slots=slots, - surplus=", ".join(surplus), - revoke=f"revoke-client --protocol wireguard {client_name}", - ) - - split_allowed_ips = wireguard.split_allowed_ips(cfg.push_routes, cfg.wg_network) - full_allowed_ips = "0.0.0.0/0, ::/0" - - # Generate client configs - configs_to_write: list[tuple[str, str]] = [] - - for slot in range(1, slots + 1): - peer = wireguard.peer_id(client_name, slot) - infix = wireguard.slot_infix(slot) + with wireguard.peer_lifecycle_lock(cfg.pki_dir): + # Generate or load server keys + server_private, server_public = wireguard.generate_server_keys(cfg.pki_dir) - # Each slot needs its own tunnel IP: two devices sharing one address - # would be indistinguishable to the server's routing table. - client_ip = wireguard.allocate_peer_ip(cfg.pki_dir, cfg.wg_network, peer) - logger.info("Allocated WireGuard IP", peer=peer, ip=client_ip) - - # Reuse the slot's keypair by default; mint only when it does not exist - # yet or rotate is requested. Regenerating it changes the server's - # accepted peer and silently breaks configs already issued to that slot. - if pubkeys: - client_private = None - client_public = pubkeys[slot - 1] - (peers_dir / f"{peer}.pub").write_text(client_public + "\n") + slots = len(pubkeys) if pubkeys else cfg.shared_client_slots + if slots > 1: logger.info( - "Using provided client public key (client-side key generation)", - peer=peer, + "Issuing concurrent WireGuard device slots", + client=client_name, + slots=slots, ) - else: - client_private, _ = wireguard.load_or_generate_client_keys( - cfg.pki_dir, peer, rotate=rotate + + # Lowering the slot count does not retire the peers already issued above it: + # they keep their key and their allocation, so the server still accepts them. + # Reissuing must not delete a credential, so say so instead of doing it. + surplus = wireguard.existing_peer_ids(cfg.pki_dir, client_name)[slots:] + if surplus: + logger.warning( + "Client still has WireGuard peers above the configured slot count;" + " they remain able to connect until revoked", + client=client_name, + slots=slots, + surplus=", ".join(surplus), + revoke=f"revoke-client --protocol wireguard {client_name}", ) - # Everything the four variants share. Held in one place so a tunnel mode - # cannot silently drift from the others -- they differ only in the - # routes they claim, and the HTTPS pair in the port wstunnel listens on. - peer_config = { - "client_private_key": client_private, - "client_ip": client_ip, - "server_public_key": server_public, - "server_endpoint": cfg.client_endpoint, - "server_port": cfg.wg_port, - "dns_servers": cfg.dns_servers, - "dns_domain": cfg.dns_domain, - "mtu": cfg.wg_mtu, - "persistent_keepalive": cfg.wg_persistent_keepalive, - } + split_allowed_ips = wireguard.split_allowed_ips(cfg.push_routes, cfg.wg_network) + full_allowed_ips = "0.0.0.0/0, ::/0" + + # Generate client configs + configs_to_write: list[tuple[str, str]] = [] - for mode, allowed_ips in ( - ("split", split_allowed_ips), - ("full", full_allowed_ips), - ): - configs_to_write.append( - ( - f"{client_name}-wg{infix}-{mode}.conf", - wireguard.generate_client_config( - **peer_config, allowed_ips=allowed_ips - ), + for slot in range(1, slots + 1): + peer = wireguard.peer_id(client_name, slot) + infix = wireguard.slot_infix(slot) + + # Each slot needs its own tunnel IP: two devices sharing one address + # would be indistinguishable to the server's routing table. + client_ip = wireguard.allocate_peer_ip(cfg.pki_dir, cfg.wg_network, peer) + logger.info("Allocated WireGuard IP", peer=peer, ip=client_ip) + + # Reuse the slot's keypair by default; mint only when it does not exist + # yet or rotate is requested. Regenerating it changes the server's + # accepted peer and silently breaks configs already issued to that slot. + if pubkeys: + client_private = None + client_public = pubkeys[slot - 1] + (peers_dir / f"{peer}.pub").write_text(client_public + "\n") + logger.info( + "Using provided client public key (client-side key generation)", + peer=peer, + ) + else: + client_private, _ = wireguard.load_or_generate_client_keys( + cfg.pki_dir, peer, rotate=rotate ) - ) - # HTTPS-tunnelled variant (WireGuard inside WebSocket/TLS) - if cfg.wg_https_tunnel_enabled: + # Everything the four variants share. Held in one place so a tunnel mode + # cannot silently drift from the others -- they differ only in the + # routes they claim, and the HTTPS pair in the port wstunnel listens on. + peer_config = { + "client_private_key": client_private, + "client_ip": client_ip, + "server_public_key": server_public, + "server_endpoint": cfg.client_endpoint, + "server_port": cfg.wg_port, + "dns_servers": cfg.dns_servers, + "dns_domain": cfg.dns_domain, + "mtu": cfg.wg_mtu, + "persistent_keepalive": cfg.wg_persistent_keepalive, + } + + for mode, allowed_ips in ( + ("split", split_allowed_ips), + ("full", full_allowed_ips), + ): configs_to_write.append( ( - f"{client_name}-wg{infix}-https-{mode}.conf", - wireguard.generate_https_tunnel_client_config( - **peer_config, - allowed_ips=allowed_ips, - wstunnel_port=cfg.wg_https_tunnel_port, - tls_server_name=cfg.server_cn, + f"{client_name}-wg{infix}-{mode}.conf", + wireguard.generate_client_config( + **peer_config, allowed_ips=allowed_ips ), ) ) - # Write all config files - for filename, content in configs_to_write: - out_path = cfg.output_dir / filename - write_secret(out_path, content) - logger.info(f"Created WireGuard config: {out_path}") - - # Regenerate the server config with the new peer, at the path the server - # itself uses, and push the peer list into the running interface. Written - # anywhere else it is a file nothing reads, and left unsynced the client - # cannot connect until the container restarts. - alloc_file = wg_dir / "allocations.json" - server_conf = wireguard.generate_server_config( - private_key=server_private, - network=cfg.wg_network, - listen_port=cfg.wg_port, - mtu=cfg.wg_mtu, - peers_dir=peers_dir, - alloc_file=alloc_file, - post_up=cfg.wg_post_up, - post_down=cfg.wg_post_down, - ) - write_secret(cfg.wg_conf, server_conf) - logger.info("Regenerated WireGuard server config", path=str(cfg.wg_conf)) - wireguard.sync_running_interface(cfg.wg_conf) + # HTTPS-tunnelled variant (WireGuard inside WebSocket/TLS) + if cfg.wg_https_tunnel_enabled: + configs_to_write.append( + ( + f"{client_name}-wg{infix}-https-{mode}.conf", + wireguard.generate_https_tunnel_client_config( + **peer_config, + allowed_ips=allowed_ips, + wstunnel_port=cfg.wg_https_tunnel_port, + tls_server_name=cfg.server_cn, + ), + ) + ) + + # Write all config files + for filename, content in configs_to_write: + out_path = cfg.output_dir / filename + write_secret(out_path, content) + logger.info(f"Created WireGuard config: {out_path}") + + # Regenerate the server config with the new peer, at the path the server + # itself uses, and push the peer list into the running interface. Written + # anywhere else it is a file nothing reads, and left unsynced the client + # cannot connect until the container restarts. + alloc_file = wg_dir / "allocations.json" + server_conf = wireguard.generate_server_config( + private_key=server_private, + network=cfg.wg_network, + listen_port=cfg.wg_port, + mtu=cfg.wg_mtu, + peers_dir=peers_dir, + alloc_file=alloc_file, + post_up=cfg.wg_post_up, + post_down=cfg.wg_post_down, + ) + write_secret(cfg.wg_conf, server_conf) + logger.info("Regenerated WireGuard server config", path=str(cfg.wg_conf)) + wireguard.sync_running_interface(cfg.wg_conf) def _report_client_states(cfg: Config) -> bool: diff --git a/scripts/lib/wireguard.py b/scripts/lib/wireguard.py index 1e12fa3..c433ba6 100644 --- a/scripts/lib/wireguard.py +++ b/scripts/lib/wireguard.py @@ -28,20 +28,34 @@ @contextmanager +def _exclusive(lock_path: Path): + """Hold an exclusive flock on lock_path, across processes.""" + lock_path.parent.mkdir(parents=True, exist_ok=True) + with open(lock_path, "w", encoding="utf-8") as lf: + fcntl.flock(lf, fcntl.LOCK_EX) + try: + yield + finally: + fcntl.flock(lf, fcntl.LOCK_UN) + + def _alloc_lock(wg_dir: Path): """Serialise read-modify-write of allocations.json across processes. Concurrent generate-client / revoke-client runs would otherwise race and double-allocate an IP or clobber the peer list. """ - wg_dir.mkdir(parents=True, exist_ok=True) - lock_path = wg_dir / "allocations.lock" - with open(lock_path, "w") as lf: - fcntl.flock(lf, fcntl.LOCK_EX) - try: - yield - finally: - fcntl.flock(lf, fcntl.LOCK_UN) + return _exclusive(wg_dir / "allocations.lock") + + +def peer_lifecycle_lock(pki_dir: Path): + """Serialise a whole issue, revoke or server-config rebuild across processes. + + Hold it from the first read of peers and allocations to the live sync. An + issuer that read a peer just before a revoke would otherwise write the + revoked key back into wg0.conf and the running interface. + """ + return _exclusive(pki_dir / "wireguard" / "lifecycle.lock") # --------------------------------------------------------------------------- @@ -491,25 +505,26 @@ def setup_wireguard(cfg) -> None: ) sys.exit(1) - server_priv, server_pub = generate_server_keys(cfg.pki_dir) - logger.info(f"WireGuard server public key: {server_pub}") - peers_dir = cfg.pki_dir / "wireguard" / "peers" alloc_file = cfg.pki_dir / "wireguard" / "allocations.json" - config_content = generate_server_config( - private_key=server_priv, - network=cfg.wg_network, - listen_port=cfg.wg_port, - mtu=cfg.wg_mtu, - peers_dir=peers_dir, - alloc_file=alloc_file, - post_up=cfg.wg_post_up, - post_down=cfg.wg_post_down, - ) + with peer_lifecycle_lock(cfg.pki_dir): + server_priv, server_pub = generate_server_keys(cfg.pki_dir) + logger.info(f"WireGuard server public key: {server_pub}") + + config_content = generate_server_config( + private_key=server_priv, + network=cfg.wg_network, + listen_port=cfg.wg_port, + mtu=cfg.wg_mtu, + peers_dir=peers_dir, + alloc_file=alloc_file, + post_up=cfg.wg_post_up, + post_down=cfg.wg_post_down, + ) - # wg0.conf embeds the server private key - 0600 from creation. - write_secret(cfg.wg_conf, config_content) + # wg0.conf embeds the server private key - 0600 from creation. + write_secret(cfg.wg_conf, config_content) logger.info(f"WireGuard config written to {cfg.wg_conf}") diff --git a/scripts/revoke-client.py b/scripts/revoke-client.py index f56f938..c32e8b6 100644 --- a/scripts/revoke-client.py +++ b/scripts/revoke-client.py @@ -272,67 +272,68 @@ def revoke_wireguard_client(client_name: str) -> bool: wg_dir = PKI_DIR / "wireguard" peers_dir = wg_dir / "peers" - peers = wireguard.existing_peer_ids(PKI_DIR, client_name) - if not peers: - logger.warning( - f"WireGuard peer key not found: {client_name}", - path=str(peers_dir / f"{client_name}.pub"), - ) - return False - - if len(peers) > 1: - logger.info( - "Revoking every device slot for client", - client=client_name, - slots=len(peers), - ) + with wireguard.peer_lifecycle_lock(PKI_DIR): + peers = wireguard.existing_peer_ids(PKI_DIR, client_name) + if not peers: + logger.warning( + f"WireGuard peer key not found: {client_name}", + path=str(peers_dir / f"{client_name}.pub"), + ) + return False - for peer in peers: - _revoke_wireguard_peer(peer, peers_dir) + if len(peers) > 1: + logger.info( + "Revoking every device slot for client", + client=client_name, + slots=len(peers), + ) - # Exact filenames only: "-wg*" also matches a client named "-wg-x". - removed = 0 - for wg_conf in wireguard_files(OUTPUT_DIR, client_name): - wg_conf.unlink() - removed += 1 - logger.info(f" Removed: {wg_conf}") - _rebundle_zip(client_name) + for peer in peers: + _revoke_wireguard_peer(peer, peers_dir) - # Regenerate the server config without the removed peer, at the path the - # server actually reads. Writing it under the PKI directory instead left the - # revoked peer in the file the server loads, so a container restart brought - # the peer back - a revocation that silently expired. The PostUp/PostDown - # hooks go back in too, or the next wg-quick up drops the operator's rules. - priv_path = wg_dir / "server_private.key" - if priv_path.exists(): - from lib.config import Config as VpnConfig - - vcfg = VpnConfig.from_settings() - alloc_file = wg_dir / "allocations.json" - - server_private = priv_path.read_text().strip() - server_conf = wireguard.generate_server_config( - private_key=server_private, - network=vcfg.wg_network, - listen_port=vcfg.wg_port, - mtu=vcfg.wg_mtu, - peers_dir=peers_dir, - alloc_file=alloc_file, - post_up=vcfg.wg_post_up, - post_down=vcfg.wg_post_down, - ) - write_secret(vcfg.wg_conf, server_conf) - logger.info( - "Regenerated WireGuard server config without revoked peer", - path=str(vcfg.wg_conf), - ) - else: - logger.warning( - "No WireGuard server private key found, so the server config was not" - " regenerated - the peer is removed from the live interface but" - " would return if the server config were rebuilt from these files", - path=str(priv_path), - ) + # Exact filenames only: "-wg*" also matches a client named "-wg-x". + removed = 0 + for wg_conf in wireguard_files(OUTPUT_DIR, client_name): + wg_conf.unlink() + removed += 1 + logger.info(f" Removed: {wg_conf}") + _rebundle_zip(client_name) + + # Regenerate the server config without the removed peer, at the path the + # server actually reads. Writing it under the PKI directory instead left the + # revoked peer in the file the server loads, so a container restart brought + # the peer back - a revocation that silently expired. The PostUp/PostDown + # hooks go back in too, or the next wg-quick up drops the operator's rules. + priv_path = wg_dir / "server_private.key" + if priv_path.exists(): + from lib.config import Config as VpnConfig + + vcfg = VpnConfig.from_settings() + alloc_file = wg_dir / "allocations.json" + + server_private = priv_path.read_text().strip() + server_conf = wireguard.generate_server_config( + private_key=server_private, + network=vcfg.wg_network, + listen_port=vcfg.wg_port, + mtu=vcfg.wg_mtu, + peers_dir=peers_dir, + alloc_file=alloc_file, + post_up=vcfg.wg_post_up, + post_down=vcfg.wg_post_down, + ) + write_secret(vcfg.wg_conf, server_conf) + logger.info( + "Regenerated WireGuard server config without revoked peer", + path=str(vcfg.wg_conf), + ) + else: + logger.warning( + "No WireGuard server private key found, so the server config was not" + " regenerated - the peer is removed from the live interface but" + " would return if the server config were rebuilt from these files", + path=str(priv_path), + ) logger.info( f"WireGuard client {client_name} has been revoked", diff --git a/tests/unit/test_client_lifecycle.py b/tests/unit/test_client_lifecycle.py index 04dbad7..0c816ae 100644 --- a/tests/unit/test_client_lifecycle.py +++ b/tests/unit/test_client_lifecycle.py @@ -91,10 +91,15 @@ def predate_proxy_refusal() -> None: """Let generate-client issue a "-proxy" name, as it did before refusing it.""" monkeypatch.setattr(generate, "collides_with_proxy_stunnel", lambda name: False) + monkeypatch.setattr(revoke, "PKI_DIR", pki) + monkeypatch.setattr(revoke, "OUTPUT_DIR", clients) + return SimpleNamespace( pki=pki, clients=clients, wg_conf=wg_conf, + generate_module=generate, + revoke_module=revoke, issue=issue, predate_proxy_refusal=predate_proxy_refusal, generate=lambda *args: run(generate, "generate-client", *args), @@ -180,3 +185,80 @@ def test_revoking_alice_proxy_keeps_it_while_alice_uses_a_proxy(self, stack): assert "alice-proxy-stunnel.conf" in _volume(stack) assert "alice-proxy.zip" not in _volume(stack) + + +class TestRevokeDuringReissue: + """A WireGuard revoke that lands while the same client is reissued stays revoked. + + The reissue snapshots the peer into the server config it is about to write. + If the revoke can run to completion inside that window, the reissue then + writes the revoked key back into wg0.conf and the next start re-admits it. + """ + + def test_the_revoked_key_does_not_return_to_wg0(self, stack, monkeypatch): + import fcntl + import threading + + from lib import wireguard + + stack.generate( + "--protocol", "wireguard", "--name", "alice", "--pubkey", ALICE_KEY + ) + assert ALICE_KEY in stack.wg_conf.read_text(encoding="utf-8") + + # Records when the revoke has to wait for a lock rather than racing on. + revoke_waiting = threading.Event() + real_flock = fcntl.flock + + def flock(fd, operation): + if operation == fcntl.LOCK_EX: + try: + real_flock(fd, operation | fcntl.LOCK_NB) + return + except BlockingIOError: + revoke_waiting.set() + real_flock(fd, operation) + + monkeypatch.setattr(fcntl, "flock", flock) + + revoked = threading.Event() + errors: list[BaseException] = [] + + def revoke_alice() -> None: + try: + stack.revoke_module.revoke_wireguard_client("alice") + except BaseException as exc: # surfaced on the main thread below + errors.append(exc) + finally: + revoked.set() + + revoker = threading.Thread(target=revoke_alice) + real_server_config = wireguard.generate_server_config + + def server_config_then_revoke(**kwargs): + conf = real_server_config(**kwargs) + if threading.current_thread() is threading.main_thread(): + revoker.start() + # Polls to a 10 s backstop for whichever comes first. + for _ in range(1000): + if revoked.wait(0.01) or revoke_waiting.is_set(): + break + assert revoked.is_set() or revoke_waiting.is_set(), ( + "the revoke neither finished nor waited on a lock" + ) + return conf + + monkeypatch.setattr( + wireguard, "generate_server_config", server_config_then_revoke + ) + + cfg = stack.generate_module.Config() + stack.generate_module.generate_wireguard_configs( + "alice", cfg, pubkeys=[ALICE_KEY] + ) + revoker.join(timeout=30) + + assert not revoker.is_alive(), "the revoke never completed" + assert not errors, errors + assert ALICE_KEY not in stack.wg_conf.read_text(encoding="utf-8") + assert not (stack.pki / "wireguard" / "peers" / "alice.pub").exists() From d21f7c52f86b5334226f600cd03a124cf8023b41 Mon Sep 17 00:00:00 2001 From: Derek Date: Sun, 1 Nov 2026 14:57:29 +1100 Subject: [PATCH 03/24] fix: stop OpenVPN full tunnel leaking IPv6 Full-tunnel client configs and the server's full-tunnel push redirected IPv4 only, so a dual-stack client sent IPv6 straight out its own uplink. Both now add route-ipv6 2000::/3 and block-ipv6, so IPv6 enters the tunnel and is refused there. redirect-gateway ipv6 is not used: it installs no route without ifconfig-ipv6, and ifconfig-ipv6 makes OpenVPN exit on a client with IPv6 disabled. The e2e full-tunnel test now runs a dual-stack client and checks IPv6 routes into tun0. --- docs/vpn-client-setup.md | 4 +--- scripts/generate-client.py | 5 +++++ scripts/lib/openvpn.py | 5 +++++ tests/e2e/docker-compose.yml | 3 +++ tests/e2e/test_openvpn_connect.py | 8 ++++++++ tests/unit/test_generate_client.py | 9 ++++++++- tests/unit/test_openvpn.py | 9 +++++++++ 7 files changed, 39 insertions(+), 4 deletions(-) diff --git a/docs/vpn-client-setup.md b/docs/vpn-client-setup.md index 833771f..7f5948b 100644 --- a/docs/vpn-client-setup.md +++ b/docs/vpn-client-setup.md @@ -573,9 +573,7 @@ config works unchanged on a normal host. ### Split vs Full Tunnel - **Split tunnel:** Only routes to your site's internal networks go through the VPN -- **Full tunnel:** All traffic goes through VPN (including internet) - -Use split tunnel for normal work; use full tunnel for maximum security. +- **Full tunnel:** All traffic, internet included. OpenVPN refuses IPv6 while it is up, as the tunnel carries IPv4 only. --- diff --git a/scripts/generate-client.py b/scripts/generate-client.py index 3e0b095..09225c0 100644 --- a/scripts/generate-client.py +++ b/scripts/generate-client.py @@ -339,6 +339,11 @@ def generate_ovpn_config( #=============================================================================== redirect-gateway def1 bypass-dhcp +# The tunnel carries IPv4 only. IPv6 is routed into it and refused there, so a +# dual-stack network cannot carry it around the VPN. +route-ipv6 2000::/3 +block-ipv6 + # WINDOWS ONLY: stops applications querying a DNS server outside the tunnel. # Uncomment on Windows. Leave it commented everywhere else - OpenVPN 2.7 on # Linux and macOS does not know the option and refuses to start. diff --git a/scripts/lib/openvpn.py b/scripts/lib/openvpn.py index 6714452..8e5a90d 100644 --- a/scripts/lib/openvpn.py +++ b/scripts/lib/openvpn.py @@ -131,7 +131,12 @@ def _apply_common_options(content: str, cfg, listener: str = "") -> str: logger.info(f"{prefix}Pushing route: {route}") if cfg.full_tunnel: + # The tunnels carry IPv4 only, so route IPv6 in and refuse it there. + # redirect-gateway ipv6 adds no route without ifconfig-ipv6, which kills + # a client with IPv6 disabled. content += 'push "redirect-gateway def1 bypass-dhcp"\n' + content += 'push "route-ipv6 2000::/3"\n' + content += 'push "block-ipv6"\n' if listener: logger.info(f"{prefix}Full tunnel mode enabled") diff --git a/tests/e2e/docker-compose.yml b/tests/e2e/docker-compose.yml index a5a169b..9eac066 100644 --- a/tests/e2e/docker-compose.yml +++ b/tests/e2e/docker-compose.yml @@ -67,6 +67,9 @@ services: # chart uses a privileged init container for it. sysctls: net.ipv4.conf.all.src_valid_mark: "1" + # A dual-stack client, so the full-tunnel test can see where IPv6 goes. + net.ipv6.conf.all.disable_ipv6: "0" + net.ipv6.conf.default.disable_ipv6: "0" volumes: - clients:/etc/vpn/clients:ro depends_on: diff --git a/tests/e2e/test_openvpn_connect.py b/tests/e2e/test_openvpn_connect.py index 3d33460..4dbd451 100644 --- a/tests/e2e/test_openvpn_connect.py +++ b/tests/e2e/test_openvpn_connect.py @@ -67,6 +67,14 @@ def test_full_tunnel_connect_and_reach_target(self, openvpn_udp_full_connection) assert ip.startswith("10.8.0."), f"Expected IP in 10.8.0.0/24, got {ip}" assert_tunnel_mode("tun0", "full") + # The tunnel carries IPv4 only, so IPv6 must be routed into it (where + # block-ipv6 refuses it) rather than leave by the client's own uplink. + v6 = docker_exec(CLIENT_CONTAINER, "ip -6 route get 2001:db8::1", check=False) + assert "dev tun0" in v6.stdout, ( + "full tunnel leaves IPv6 outside the tunnel:\n" + f"{v6.stdout}{v6.stderr}\nOpenVPN log:\n{get_openvpn_log()}" + ) + body = curl_target() assert body == TARGET_RESPONSE, ( f"Expected '{TARGET_RESPONSE}', got '{body}'. " diff --git a/tests/unit/test_generate_client.py b/tests/unit/test_generate_client.py index 550cdbb..7eb63f5 100644 --- a/tests/unit/test_generate_client.py +++ b/tests/unit/test_generate_client.py @@ -180,7 +180,14 @@ def test_split_carries_extra_routes_and_full_redirects(self, run): assert "route 10.1.0.0/16\n" in split assert "route 192.168.5.0/24\n" in split assert "redirect-gateway" not in split - assert "redirect-gateway def1 bypass-dhcp" in full + assert "block-ipv6" not in split + # The tunnel carries IPv4 only, so a full tunnel routes IPv6 in and drops + # it there rather than letting a dual-stack client send it outside. + assert "redirect-gateway def1 bypass-dhcp\n" in full + assert "\nroute-ipv6 2000::/3\n" in full + assert "\nblock-ipv6\n" in full + # ifconfig-ipv6 would make a client with IPv6 disabled exit at connect. + assert "ifconfig-ipv6" not in full assert "route 10.1.0.0/16" not in full def test_https_listener_gets_a_tls_1_3_stunnel_config(self, run): diff --git a/tests/unit/test_openvpn.py b/tests/unit/test_openvpn.py index 36359b4..a396dc9 100644 --- a/tests/unit/test_openvpn.py +++ b/tests/unit/test_openvpn.py @@ -173,6 +173,14 @@ def test_full_tunnel_adds_redirect(self): cfg = self.FakeCfg(full_tunnel=True) content = _apply_common_options("", cfg) assert 'push "redirect-gateway def1 bypass-dhcp"' in content + assert 'push "route-ipv6 2000::/3"' in content + assert 'push "block-ipv6"' in content + assert "ifconfig-ipv6" not in content + + def test_split_tunnel_leaves_ipv6_alone(self): + content = _apply_common_options("", self.FakeCfg(full_tunnel=False)) + assert "redirect-gateway" not in content + assert "block-ipv6" not in content def test_all_options_combined(self): """All options work together.""" @@ -372,6 +380,7 @@ def test_common_options_reach_every_listener(self, server_dir, listener): assert 'push "dhcp-option DOMAIN corp.example.test"' in rendered assert 'push "route 10.20.0.0 255.255.0.0"' in rendered assert 'push "redirect-gateway def1 bypass-dhcp"' in rendered + assert 'push "block-ipv6"' in rendered @pytest.mark.parametrize("listener", sorted(LISTENERS)) def test_disabled_listener_writes_nothing(self, server_dir, listener): From f524b39891d09cb7baea556c7f73232d8020071c Mon Sep 17 00:00:00 2001 From: Derek Date: Sun, 1 Nov 2026 18:57:43 +1100 Subject: [PATCH 04/24] fix: write OpenVPN status files in the format metrics reads The metrics collector parses tab-separated status-version 3, but no server template set it, so OpenVPN wrote version 1 and every listener reported zero connected clients. All three templates now set status-version 3, and the status file paths live in one map the collector and the template test share. The e2e session count reads status.log with the collector's own parser. --- config/server-https.conf.template | 1 + config/server-tcp.conf.template | 1 + config/server.conf.template | 1 + scripts/lib/metrics.py | 14 +++++++++----- tests/e2e/test_concurrent_clients.py | 19 ++++++++++--------- tests/unit/test_openvpn.py | 18 ++++++++++++++++++ 6 files changed, 40 insertions(+), 14 deletions(-) diff --git a/config/server-https.conf.template b/config/server-https.conf.template index ecab816..f94f6a1 100644 --- a/config/server-https.conf.template +++ b/config/server-https.conf.template @@ -149,6 +149,7 @@ reneg-sec ${OPENVPN_RENEG_SEC} # Logging #=============================================================================== status /var/log/vpn/status-https.log 10 +status-version 3 log-append /var/log/vpn/openvpn-https.log verb ${OPENVPN_VERB} mute ${OPENVPN_MUTE} diff --git a/config/server-tcp.conf.template b/config/server-tcp.conf.template index b012050..d5f92f1 100644 --- a/config/server-tcp.conf.template +++ b/config/server-tcp.conf.template @@ -140,6 +140,7 @@ reneg-sec ${OPENVPN_RENEG_SEC} # Logging #=============================================================================== status /var/log/vpn/status-tcp.log 10 +status-version 3 log-append /var/log/vpn/openvpn-tcp.log verb ${OPENVPN_VERB} mute ${OPENVPN_MUTE} diff --git a/config/server.conf.template b/config/server.conf.template index b210ae0..99f57d2 100644 --- a/config/server.conf.template +++ b/config/server.conf.template @@ -143,6 +143,7 @@ reneg-sec ${OPENVPN_RENEG_SEC} # Logging #=============================================================================== status /var/log/vpn/status.log 10 +status-version 3 log-append /var/log/vpn/openvpn.log verb ${OPENVPN_VERB} mute ${OPENVPN_MUTE} diff --git a/scripts/lib/metrics.py b/scripts/lib/metrics.py index 785b55f..118beec 100644 --- a/scripts/lib/metrics.py +++ b/scripts/lib/metrics.py @@ -41,6 +41,14 @@ from scalo.metrics import create_metrics from scalo.metrics.groups import AppMetrics +# Each listener's status file, as its server template names it, with +# `status-version 3` beside it. +OPENVPN_STATUS_FILES = { + "/var/log/vpn/status.log": "udp", + "/var/log/vpn/status-https.log": "https", + "/var/log/vpn/status-tcp.log": "tcp", +} + # --------------------------------------------------------------------------- # Data types (used by parsers and tests) # --------------------------------------------------------------------------- @@ -640,11 +648,7 @@ def _update_metrics_locked() -> None: if remaining is not None: _gauge_set(_g_crl_expiry, remaining) - for status_file, listener_name in [ - ("/var/log/vpn/status.log", "udp"), - ("/var/log/vpn/status-https.log", "https"), - ("/var/log/vpn/status-tcp.log", "tcp"), - ]: + for status_file, listener_name in OPENVPN_STATUS_FILES.items(): status = collect_openvpn_status(status_file, listener_name) if status is not None: total_clients += status.client_count diff --git a/tests/e2e/test_concurrent_clients.py b/tests/e2e/test_concurrent_clients.py index 2df09f4..30115f2 100644 --- a/tests/e2e/test_concurrent_clients.py +++ b/tests/e2e/test_concurrent_clients.py @@ -28,6 +28,7 @@ has_wireguard_module, wait_for_tunnel, ) +from lib.metrics import parse_openvpn_status_v3 # The connectivity stack is shared across this module's tests. pytestmark = pytest.mark.usefixtures("compose_stack") @@ -45,28 +46,28 @@ def wireguard_available() -> bool: def _wait_for_status_sessions( common_name: str, wanted: int, timeout: int = 45 -) -> tuple[list[str], str]: +) -> tuple[list[dict], str]: """Poll the server's status log until it lists ``wanted`` sessions. The server rewrites status.log on a 10s timer, so a read taken straight - after a client connects still shows the previous state. Returns the - matching lines and the last status file read, so a failure can show both. + after a client connects still shows the previous state. The file is read + with the metrics collector's own parser, so this also proves the server + writes the format the collector reads. Returns the matching sessions and the + last status file read, so a failure can show both. """ deadline = time.monotonic() + timeout status = "" - sessions: list[str] = [] + sessions: list[dict] = [] while time.monotonic() < deadline: status = docker_exec( SERVER_CONTAINER, "cat /var/log/vpn/status.log", check=False, ).stdout - # Only the CLIENT LIST rows name a session; the routing table repeats - # the common name once per virtual address and would double the count. sessions = [ - line - for line in status.split("ROUTING TABLE")[0].splitlines() - if line.startswith(f"{common_name},") + client + for client in parse_openvpn_status_v3(status).clients + if client["common_name"] == common_name ] if len(sessions) >= wanted: return sessions, status diff --git a/tests/unit/test_openvpn.py b/tests/unit/test_openvpn.py index a396dc9..6a77ca4 100644 --- a/tests/unit/test_openvpn.py +++ b/tests/unit/test_openvpn.py @@ -65,6 +65,24 @@ def test_external_scripts_are_not_permitted(self, name): ] assert not active, f"{name} enables external scripts: {active}" + @pytest.mark.parametrize("name", SERVER_TEMPLATES) + def test_status_file_is_the_format_metrics_reads(self, name): + """The metrics collector reads tab-separated status-version 3 only. + + OpenVPN writes version 1 unless told otherwise, and the collector then + counts no client on any listener. + """ + from lib.metrics import OPENVPN_STATUS_FILES + + lines = (REPO_ROOT / "config" / name).read_text(encoding="utf-8").splitlines() + directives = { + line.split()[0]: line.split()[1:] + for line in (raw.strip() for raw in lines) + if line and not line.startswith("#") + } + assert directives.get("status-version") == ["3"], name + assert directives["status"][0] in OPENVPN_STATUS_FILES, name + def test_https_listener_binds_loopback_only(self): """The stunnel-fronted listener must not be reachable directly. From 27a34a930d367ed6b9cb0cf00f2d633b7220e0df Mon Sep 17 00:00:00 2001 From: Derek Date: Sun, 1 Nov 2026 18:59:12 +1100 Subject: [PATCH 05/24] fix: replace WireGuard allocations.json atomically allocate_peer_ip and deallocate_peer_ip rewrote allocations.json in place, so a crash or full disk mid-write left a truncated file that every later issue, revoke and server start failed to parse. Both now write a temp sibling, fsync it, rename it over the old file and fsync the directory. write_file_atomic gained the two fsyncs, so the external PKI fetch gets the same guarantee. --- scripts/lib/process.py | 11 ++++++++- scripts/lib/wireguard.py | 12 +++++++--- tests/unit/test_process.py | 16 +++++++++++++ tests/unit/test_wireguard_keys.py | 38 +++++++++++++++++++++++++++++++ 4 files changed, 73 insertions(+), 4 deletions(-) diff --git a/scripts/lib/process.py b/scripts/lib/process.py index 9928c3b..cb13818 100644 --- a/scripts/lib/process.py +++ b/scripts/lib/process.py @@ -63,7 +63,9 @@ def write_file_atomic(path: Path, data: bytes, mode: int) -> None: The content goes to a private temp file beside path and is renamed over it, so a reader never sees a partial file, a private key is never wider than 0600 while it is written, and two writers sharing the directory never share a temp - file. The final mode is applied before the rename. + file. The final mode is applied before the rename. The file is flushed before + the rename and the directory after it, so a crash leaves the old file or the + new one, never an empty one. """ fd, name = tempfile.mkstemp(dir=path.parent, prefix=f".{path.name}.", suffix=".tmp") tmp = Path(name) @@ -71,10 +73,17 @@ def write_file_atomic(path: Path, data: bytes, mode: int) -> None: with os.fdopen(fd, "wb") as f: f.write(data) os.fchmod(f.fileno(), mode) + f.flush() + os.fsync(f.fileno()) os.replace(tmp, path) except BaseException: tmp.unlink(missing_ok=True) raise + dir_fd = os.open(path.parent, os.O_RDONLY | os.O_DIRECTORY) + try: + os.fsync(dir_fd) + finally: + os.close(dir_fd) def setup_directories(cfg) -> None: diff --git a/scripts/lib/wireguard.py b/scripts/lib/wireguard.py index c433ba6..de4a58d 100644 --- a/scripts/lib/wireguard.py +++ b/scripts/lib/wireguard.py @@ -24,7 +24,7 @@ from scalo.logger import logger -from lib.process import write_secret +from lib.process import write_file_atomic, write_secret @contextmanager @@ -201,6 +201,12 @@ def load_or_generate_client_keys( # --------------------------------------------------------------------------- +def _write_allocations(alloc_file: Path, allocations: dict[str, str]) -> None: + """Replace allocations.json whole, so a failed write leaves the old one.""" + data = (json.dumps(allocations, indent=2) + "\n").encode() + write_file_atomic(alloc_file, data, 0o644) + + def allocate_peer_ip(pki_dir: Path, network: str, client_name: str) -> str: """Allocate next available IP for a WireGuard peer.""" alloc_file = pki_dir / "wireguard" / "allocations.json" @@ -222,7 +228,7 @@ def allocate_peer_ip(pki_dir: Path, network: str, client_name: str) -> str: ip_str = str(host) if ip_str not in used_ips: allocations[client_name] = ip_str - alloc_file.write_text(json.dumps(allocations, indent=2) + "\n") + _write_allocations(alloc_file, allocations) return ip_str raise RuntimeError(f"No available IPs in {network}") @@ -238,7 +244,7 @@ def deallocate_peer_ip(pki_dir: Path, client_name: str) -> str | None: allocations: dict[str, str] = json.loads(alloc_file.read_text()) ip = allocations.pop(client_name, None) if ip: - alloc_file.write_text(json.dumps(allocations, indent=2) + "\n") + _write_allocations(alloc_file, allocations) return ip diff --git a/tests/unit/test_process.py b/tests/unit/test_process.py index e9023fd..67dac6d 100644 --- a/tests/unit/test_process.py +++ b/tests/unit/test_process.py @@ -40,6 +40,22 @@ def test_does_not_use_another_writers_temp_file(self, tmp_path): assert other.read_bytes() == b"half written by another container" + def test_flushes_the_file_and_then_the_directory(self, tmp_path, monkeypatch): + """Without both, a power cut can leave the name pointing at an empty file.""" + import os + + synced = [] + real_fsync = os.fsync + + def recording_fsync(fd): + synced.append(stat.S_ISDIR(os.fstat(fd).st_mode)) + real_fsync(fd) + + monkeypatch.setattr(os, "fsync", recording_fsync) + write_file_atomic(tmp_path / "allocations.json", b"{}", 0o644) + + assert synced == [False, True] + class TestWriteSecret: """write_secret's defining property: 0600 with no wider-perms window.""" diff --git a/tests/unit/test_wireguard_keys.py b/tests/unit/test_wireguard_keys.py index c5c3466..9d7be5f 100644 --- a/tests/unit/test_wireguard_keys.py +++ b/tests/unit/test_wireguard_keys.py @@ -223,6 +223,44 @@ def test_persists_allocations_to_json(self, pki_dir: Path) -> None: data = json.loads(alloc_file.read_text()) assert data["alice"] == "10.8.0.2" + @pytest.mark.parametrize( + "change", + [ + pytest.param( + lambda pki: allocate_peer_ip(pki, "10.8.0.0/24", "bob"), id="allocate" + ), + pytest.param(lambda pki: deallocate_peer_ip(pki, "alice"), id="deallocate"), + ], + ) + def test_a_failed_write_keeps_the_previous_allocations( + self, pki_dir: Path, monkeypatch, change + ) -> None: + """A write that dies part way must not leave a truncated allocations.json. + + Every later issue, revoke and server start parses it, so a half-written + file stops WireGuard for every client. + """ + import os + + wg_dir = pki_dir / "wireguard" + wg_dir.mkdir(parents=True) + allocate_peer_ip(pki_dir, "10.8.0.0/24", "alice") + alloc_file = wg_dir / "allocations.json" + before = alloc_file.read_text() + + def disk_full(fd): + raise OSError(28, "No space left on device") + + monkeypatch.setattr(os, "fsync", disk_full) + with pytest.raises(OSError, match="No space left"): + change(pki_dir) + + assert alloc_file.read_text() == before + assert sorted(p.name for p in wg_dir.iterdir()) == [ + "allocations.json", + "allocations.lock", + ] + def test_concurrent_allocations_do_not_collide(self, pki_dir: Path) -> None: """Parallel allocations get unique IPs and leave valid JSON (flock).""" import threading From 217aff199edad12507d9a0c7d7e6f9201c963dcf Mon Sep 17 00:00:00 2001 From: Derek Date: Sun, 1 Nov 2026 19:00:57 +1100 Subject: [PATCH 06/24] fix: keep client tls-crypt-v2 keys apart from client private keys Client alice's tls-crypt-v2 key was private/alice-tc.key, which is also the private key of a client named alice-tc. Issuing alice-tc overwrote alice's key, and revoking either deleted the other's. Per-client tls-crypt-v2 keys now live in tls-crypt-v2/.key, a path no other client name can produce. generate-client also refuses to issue a name whose private key already exists without a certificate, since Easy-RSA in batch mode overwrites it. --- scripts/generate-client.py | 30 +++++++++++---- scripts/lib/pki.py | 9 +++++ scripts/revoke-client.py | 3 +- tests/integration/test_client_generation.bats | 2 +- tests/unit/test_client_lifecycle.py | 37 ++++++++++++++++++- tests/unit/test_generate_client.py | 6 ++- 6 files changed, 75 insertions(+), 12 deletions(-) diff --git a/scripts/generate-client.py b/scripts/generate-client.py index 09225c0..dcb9454 100644 --- a/scripts/generate-client.py +++ b/scripts/generate-client.py @@ -52,7 +52,7 @@ client_files, collides_with_proxy_stunnel, ) -from lib.pki import validate_client_name # noqa: E402 +from lib.pki import client_tc_key_path, validate_client_name # noqa: E402 from lib.process import write_secret # noqa: E402 from scalo.logger import logger # noqa: E402 @@ -182,8 +182,15 @@ def generate_certificate( capture_output=True, ) - # Generate tls-crypt-v2 client key + generate_tc_client_key(client_name, pki_dir) + logger.info("Certificate generated successfully", client=client_name) + + +def generate_tc_client_key(client_name: str, pki_dir: Path) -> None: + """Mint the client's tls-crypt-v2 key, wrapped by the server's tc.key.""" logger.info("Generating tls-crypt-v2 client key...") + key_path = client_tc_key_path(pki_dir, client_name) + key_path.parent.mkdir(mode=0o700, exist_ok=True) subprocess.run( [ "openvpn", @@ -191,13 +198,12 @@ def generate_certificate( str(pki_dir / "tc.key"), "--genkey", "tls-crypt-v2-client", - str(pki_dir / "private" / f"{client_name}-tc.key"), + str(key_path), ], check=True, capture_output=True, ) - - logger.info("Certificate generated successfully", client=client_name) + key_path.chmod(0o600) # =============================================================================== @@ -318,7 +324,7 @@ def generate_ovpn_config( # Read certificate files ca_cert = (cfg.pki_dir / "ca.crt").read_text() client_key = (cfg.pki_dir / "private" / f"{client_name}.key").read_text() - tc_key = (cfg.pki_dir / "private" / f"{client_name}-tc.key").read_text() + tc_key = client_tc_key_path(cfg.pki_dir, client_name).read_text() # Extract just the certificate (not the full chain info) result = subprocess.run( @@ -862,6 +868,14 @@ def main() -> None: "or use --config-only to regenerate configs" ) sys.exit(1) + # Easy-RSA in batch mode overwrites an existing key without asking. + stray_key = cfg.pki_dir / "private" / f"{client_name}.key" + if stray_key.exists(): + logger.error( + f"{stray_key} exists with no certificate beside it. Refusing" + " to overwrite key material: remove it, or choose another --name." + ) + sys.exit(1) generate_certificate( client_name, cert_days, @@ -950,7 +964,9 @@ def main() -> None: logger.info("OpenVPN certificate files:") logger.info(f" Certificate: {cfg.pki_dir}/issued/{client_name}.crt") logger.info(f" Private key: {cfg.pki_dir}/private/{client_name}.key") - logger.info(f" TLS key: {cfg.pki_dir}/private/{client_name}-tc.key") + logger.info( + f" TLS key: {client_tc_key_path(cfg.pki_dir, client_name)}" + ) logger.info("") logger.info("OpenVPN configuration files (6 total):") diff --git a/scripts/lib/pki.py b/scripts/lib/pki.py index 0bc8e4e..b7eb930 100644 --- a/scripts/lib/pki.py +++ b/scripts/lib/pki.py @@ -48,6 +48,15 @@ def validate_client_name(name: str) -> bool: return bool(re.match(r"^[a-zA-Z0-9_-]+$", name)) +def client_tc_key_path(pki_dir: Path, client_name: str) -> Path: + """Return where a client's tls-crypt-v2 key lives. + + Outside Easy-RSA's private/ directory, so it can never be another client's + private key: "-tc.key" there was also the key of a client "alice-tc". + """ + return Path(pki_dir) / "tls-crypt-v2" / f"{client_name}.key" + + # --------------------------------------------------------------------------- # PEM validation # --------------------------------------------------------------------------- diff --git a/scripts/revoke-client.py b/scripts/revoke-client.py index c32e8b6..632e3a7 100644 --- a/scripts/revoke-client.py +++ b/scripts/revoke-client.py @@ -52,6 +52,7 @@ ) from lib.pki import ( # noqa: E402 EASYRSA_DIR, + client_tc_key_path, regenerate_local_crl, validate_client_name, ) @@ -177,7 +178,7 @@ def revoke_client(client_name: str, crl_days: int, missing_ok: bool = False) -> logger.info("Removing client files...") files_to_remove = [ PKI_DIR / "private" / f"{client_name}.key", - PKI_DIR / "private" / f"{client_name}-tc.key", + client_tc_key_path(PKI_DIR, client_name), PKI_DIR / "reqs" / f"{client_name}.req", ] diff --git a/tests/integration/test_client_generation.bats b/tests/integration/test_client_generation.bats index 7bf0df2..a479689 100644 --- a/tests/integration/test_client_generation.bats +++ b/tests/integration/test_client_generation.bats @@ -92,7 +92,7 @@ teardown_file() { @test "client tls-crypt-v2 key is created" { generate_test_client "testuser6" - run container_exec test -f /etc/vpn/pki/private/testuser6-tc.key + run container_exec test -f /etc/vpn/pki/tls-crypt-v2/testuser6.key assert_success } diff --git a/tests/unit/test_client_lifecycle.py b/tests/unit/test_client_lifecycle.py index 0c816ae..c76e780 100644 --- a/tests/unit/test_client_lifecycle.py +++ b/tests/unit/test_client_lifecycle.py @@ -20,6 +20,7 @@ from types import SimpleNamespace import pytest +from lib.pki import client_tc_key_path SCRIPTS = Path(__file__).resolve().parents[2] / "scripts" SERVER_CN = "vpn.example.test" @@ -81,7 +82,9 @@ def issue(name: str) -> None: client = issue_cert(name, issuer=ca) (pki / "issued" / f"{name}.crt").write_bytes(client.cert_pem) (pki / "private" / f"{name}.key").write_bytes(client.key_pem) - (pki / "private" / f"{name}-tc.key").write_text(TC_KEY, encoding="utf-8") + tc_key = client_tc_key_path(pki, name) + tc_key.parent.mkdir(exist_ok=True) + tc_key.write_text(TC_KEY, encoding="utf-8") def run(module, prog: str, *args: str) -> None: monkeypatch.setattr(sys, "argv", [prog, *args]) @@ -187,6 +190,38 @@ def test_revoking_alice_proxy_keeps_it_while_alice_uses_a_proxy(self, stack): assert "alice-proxy.zip" not in _volume(stack) +class TestTlsCryptKeyNames: + """A client's tls-crypt-v2 key never shares a path with another client's key. + + The tls-crypt-v2 key of "alice" and the private key of "alice-tc" used to be + one file, so revoking one client deleted the other's key material. + """ + + def test_revoking_alice_tc_leaves_alice_able_to_reissue(self, stack): + stack.issue("alice") + stack.issue("alice-tc") + stack.generate("--protocol", "openvpn", "--config-only", "--name", "alice") + before = (stack.clients / "alice-udp-split.ovpn").read_text(encoding="utf-8") + + stack.revoke("--protocol", "openvpn", "alice-tc") + stack.generate("--protocol", "openvpn", "--config-only", "--name", "alice") + + after = (stack.clients / "alice-udp-split.ovpn").read_text(encoding="utf-8") + assert after.split("")[1] == before.split("")[1] + assert after.split("")[1] == before.split("")[1] + + def test_a_stray_private_key_is_never_overwritten(self, stack): + """A key with no certificate is someone's material, not a free name.""" + stray = stack.pki / "private" / "bob.key" + stray.write_text("not bob's to lose\n", encoding="utf-8") + + with pytest.raises(SystemExit) as exc: + stack.generate("--protocol", "openvpn", "--name", "bob") + + assert exc.value.code == 1 + assert stray.read_text(encoding="utf-8") == "not bob's to lose\n" + + class TestRevokeDuringReissue: """A WireGuard revoke that lands while the same client is reissued stays revoked. diff --git a/tests/unit/test_generate_client.py b/tests/unit/test_generate_client.py index 7eb63f5..d30ab4f 100644 --- a/tests/unit/test_generate_client.py +++ b/tests/unit/test_generate_client.py @@ -21,6 +21,7 @@ from types import SimpleNamespace import pytest +from lib.pki import client_tc_key_path SCRIPT = Path(__file__).resolve().parents[2] / "scripts" / "generate-client.py" @@ -72,7 +73,8 @@ def pki(tmp_path, issue_cert): CERT_TEXT_DUMP + client.cert_pem.decode(), encoding="utf-8" ) (pki_dir / "private" / f"{CLIENT}.key").write_bytes(client.key_pem) - (pki_dir / "private" / f"{CLIENT}-tc.key").write_text(TC_KEY, encoding="utf-8") + (pki_dir / "tls-crypt-v2").mkdir() + (pki_dir / "tls-crypt-v2" / f"{CLIENT}.key").write_text(TC_KEY, encoding="utf-8") wg_dir = pki_dir / "wireguard" (wg_dir / "server_private.key").write_text("ServerPrivate=\n", encoding="utf-8") @@ -281,7 +283,7 @@ def issue(name: str): client = issue_cert(name, issuer=pki.ca) (pki.dir / "issued" / f"{name}.crt").write_bytes(client.cert_pem) (pki.dir / "private" / f"{name}.key").write_bytes(client.key_pem) - (pki.dir / "private" / f"{name}-tc.key").write_text(TC_KEY, encoding="utf-8") + client_tc_key_path(pki.dir, name).write_text(TC_KEY, encoding="utf-8") return client return issue From 759998cabfe52d8f7692cf8a95ea7e93e67b8979 Mon Sep 17 00:00:00 2001 From: Derek Date: Sun, 1 Nov 2026 19:03:38 +1100 Subject: [PATCH 07/24] fix: fail and roll back a WireGuard issue the live interface refuses generate-client discarded sync_running_interface's result, so after --rotate a refused wg syncconf left the old key live in the kernel while the files and the operator both believed it retired. A refused sync now raises, and generate-client restores every file the issue touched (peer keys, allocations.json, wg0.conf, the client's configs) and exits 1 saying the server still accepts the keys it had. Any other failure part way through restores the same files. --- scripts/generate-client.py | 232 ++++++++++++++++------------- scripts/lib/process.py | 22 +++ scripts/lib/wireguard.py | 36 ++++- tests/unit/test_generate_client.py | 78 ++++++++++ 4 files changed, 262 insertions(+), 106 deletions(-) diff --git a/scripts/generate-client.py b/scripts/generate-client.py index dcb9454..796f05a 100644 --- a/scripts/generate-client.py +++ b/scripts/generate-client.py @@ -53,7 +53,7 @@ collides_with_proxy_stunnel, ) from lib.pki import client_tc_key_path, validate_client_name # noqa: E402 -from lib.process import write_secret # noqa: E402 +from lib.process import restore_files, snapshot_files, write_secret # noqa: E402 from scalo.logger import logger # noqa: E402 # =============================================================================== @@ -508,103 +508,127 @@ def generate_wireguard_configs( revoke=f"revoke-client --protocol wireguard {client_name}", ) - split_allowed_ips = wireguard.split_allowed_ips(cfg.push_routes, cfg.wg_network) - full_allowed_ips = "0.0.0.0/0, ::/0" - - # Generate client configs - configs_to_write: list[tuple[str, str]] = [] - + # A failure part way, or a live interface that refuses the new peers, + # puts every file back: on disk the client is then exactly what the + # kernel still accepts. + touched = [cfg.wg_conf, wg_dir / "allocations.json"] for slot in range(1, slots + 1): peer = wireguard.peer_id(client_name, slot) - infix = wireguard.slot_infix(slot) - - # Each slot needs its own tunnel IP: two devices sharing one address - # would be indistinguishable to the server's routing table. - client_ip = wireguard.allocate_peer_ip(cfg.pki_dir, cfg.wg_network, peer) - logger.info("Allocated WireGuard IP", peer=peer, ip=client_ip) - - # Reuse the slot's keypair by default; mint only when it does not exist - # yet or rotate is requested. Regenerating it changes the server's - # accepted peer and silently breaks configs already issued to that slot. - if pubkeys: - client_private = None - client_public = pubkeys[slot - 1] - (peers_dir / f"{peer}.pub").write_text(client_public + "\n") - logger.info( - "Using provided client public key (client-side key generation)", - peer=peer, - ) - else: - client_private, _ = wireguard.load_or_generate_client_keys( - cfg.pki_dir, peer, rotate=rotate + touched += [peers_dir / f"{peer}.key", peers_dir / f"{peer}.pub"] + touched += [ + cfg.output_dir / name + for name in wireguard.client_config_names( + client_name, slot, cfg.wg_https_tunnel_enabled ) + ] + saved = snapshot_files(touched) - # Everything the four variants share. Held in one place so a tunnel mode - # cannot silently drift from the others -- they differ only in the - # routes they claim, and the HTTPS pair in the port wstunnel listens on. - peer_config = { - "client_private_key": client_private, - "client_ip": client_ip, - "server_public_key": server_public, - "server_endpoint": cfg.client_endpoint, - "server_port": cfg.wg_port, - "dns_servers": cfg.dns_servers, - "dns_domain": cfg.dns_domain, - "mtu": cfg.wg_mtu, - "persistent_keepalive": cfg.wg_persistent_keepalive, - } - - for mode, allowed_ips in ( - ("split", split_allowed_ips), - ("full", full_allowed_ips), - ): - configs_to_write.append( - ( - f"{client_name}-wg{infix}-{mode}.conf", - wireguard.generate_client_config( - **peer_config, allowed_ips=allowed_ips - ), - ) + try: + split_allowed_ips = wireguard.split_allowed_ips( + cfg.push_routes, cfg.wg_network + ) + full_allowed_ips = "0.0.0.0/0, ::/0" + + # Generate client configs + configs_to_write: list[tuple[str, str]] = [] + + for slot in range(1, slots + 1): + peer = wireguard.peer_id(client_name, slot) + + # Each slot needs its own tunnel IP: two devices sharing one address + # would be indistinguishable to the server's routing table. + client_ip = wireguard.allocate_peer_ip( + cfg.pki_dir, cfg.wg_network, peer ) + logger.info("Allocated WireGuard IP", peer=peer, ip=client_ip) + + # Reuse the slot's keypair by default; mint only when it does not exist + # yet or rotate is requested. Regenerating it changes the server's + # accepted peer and silently breaks configs already issued to that slot. + if pubkeys: + client_private = None + client_public = pubkeys[slot - 1] + (peers_dir / f"{peer}.pub").write_text(client_public + "\n") + logger.info( + "Using provided client public key (client-side key generation)", + peer=peer, + ) + else: + client_private, _ = wireguard.load_or_generate_client_keys( + cfg.pki_dir, peer, rotate=rotate + ) - # HTTPS-tunnelled variant (WireGuard inside WebSocket/TLS) - if cfg.wg_https_tunnel_enabled: + # Everything the four variants share. Held in one place so a tunnel mode + # cannot silently drift from the others -- they differ only in the + # routes they claim, and the HTTPS pair in the port wstunnel listens on. + peer_config = { + "client_private_key": client_private, + "client_ip": client_ip, + "server_public_key": server_public, + "server_endpoint": cfg.client_endpoint, + "server_port": cfg.wg_port, + "dns_servers": cfg.dns_servers, + "dns_domain": cfg.dns_domain, + "mtu": cfg.wg_mtu, + "persistent_keepalive": cfg.wg_persistent_keepalive, + } + + for mode, allowed_ips in ( + ("split", split_allowed_ips), + ("full", full_allowed_ips), + ): configs_to_write.append( ( - f"{client_name}-wg{infix}-https-{mode}.conf", - wireguard.generate_https_tunnel_client_config( - **peer_config, - allowed_ips=allowed_ips, - wstunnel_port=cfg.wg_https_tunnel_port, - tls_server_name=cfg.server_cn, + wireguard.client_config_name(client_name, slot, mode), + wireguard.generate_client_config( + **peer_config, allowed_ips=allowed_ips ), ) ) - # Write all config files - for filename, content in configs_to_write: - out_path = cfg.output_dir / filename - write_secret(out_path, content) - logger.info(f"Created WireGuard config: {out_path}") - - # Regenerate the server config with the new peer, at the path the server - # itself uses, and push the peer list into the running interface. Written - # anywhere else it is a file nothing reads, and left unsynced the client - # cannot connect until the container restarts. - alloc_file = wg_dir / "allocations.json" - server_conf = wireguard.generate_server_config( - private_key=server_private, - network=cfg.wg_network, - listen_port=cfg.wg_port, - mtu=cfg.wg_mtu, - peers_dir=peers_dir, - alloc_file=alloc_file, - post_up=cfg.wg_post_up, - post_down=cfg.wg_post_down, - ) - write_secret(cfg.wg_conf, server_conf) - logger.info("Regenerated WireGuard server config", path=str(cfg.wg_conf)) - wireguard.sync_running_interface(cfg.wg_conf) + # HTTPS-tunnelled variant (WireGuard inside WebSocket/TLS) + if cfg.wg_https_tunnel_enabled: + configs_to_write.append( + ( + wireguard.client_config_name( + client_name, slot, mode, https=True + ), + wireguard.generate_https_tunnel_client_config( + **peer_config, + allowed_ips=allowed_ips, + wstunnel_port=cfg.wg_https_tunnel_port, + tls_server_name=cfg.server_cn, + ), + ) + ) + + # Write all config files + for filename, content in configs_to_write: + out_path = cfg.output_dir / filename + write_secret(out_path, content) + logger.info(f"Created WireGuard config: {out_path}") + + # Regenerate the server config with the new peer, at the path the server + # itself uses, and push the peer list into the running interface. Written + # anywhere else it is a file nothing reads, and left unsynced the client + # cannot connect until the container restarts. + alloc_file = wg_dir / "allocations.json" + server_conf = wireguard.generate_server_config( + private_key=server_private, + network=cfg.wg_network, + listen_port=cfg.wg_port, + mtu=cfg.wg_mtu, + peers_dir=peers_dir, + alloc_file=alloc_file, + post_up=cfg.wg_post_up, + post_down=cfg.wg_post_down, + ) + write_secret(cfg.wg_conf, server_conf) + logger.info("Regenerated WireGuard server config", path=str(cfg.wg_conf)) + wireguard.sync_running_interface(cfg.wg_conf) + except BaseException: + restore_files(saved) + raise def _report_client_states(cfg: Config) -> bool: @@ -928,12 +952,22 @@ def main() -> None: # ---- WireGuard ---- if generate_wg: - generate_wireguard_configs( - client_name=client_name, - cfg=cfg, - pubkeys=args.pubkey, - rotate=args.rotate, - ) + from lib.wireguard import WireGuardSyncError + + try: + generate_wireguard_configs( + client_name=client_name, + cfg=cfg, + pubkeys=args.pubkey, + rotate=args.rotate, + ) + except WireGuardSyncError as exc: + logger.error( + f"WireGuard issue for {client_name} FAILED and was rolled back: {exc}." + " The running server still accepts the keys it had before, and the" + " files on disk match them." + ) + sys.exit(1) _record_generated_protocol(cfg, client_name, "wireguard") # Copy vpn-client-setup.md to output directory @@ -998,16 +1032,10 @@ def main() -> None: logger.info("") logger.info(f"WireGuard configuration files ({slots} device slot(s)):") for slot in range(1, slots + 1): - infix = wireguard.slot_infix(slot) - logger.info(f" {cfg.output_dir}/{client_name}-wg{infix}-split.conf") - logger.info(f" {cfg.output_dir}/{client_name}-wg{infix}-full.conf") - if cfg.wg_https_tunnel_enabled: - logger.info( - f" {cfg.output_dir}/{client_name}-wg{infix}-https-split.conf" - ) - logger.info( - f" {cfg.output_dir}/{client_name}-wg{infix}-https-full.conf" - ) + for name in wireguard.client_config_names( + client_name, slot, cfg.wg_https_tunnel_enabled + ): + logger.info(f" {cfg.output_dir}/{name}") logger.info("") logger.info("WireGuard peer keys:") for peer in wireguard.peer_ids(client_name, slots): diff --git a/scripts/lib/process.py b/scripts/lib/process.py index cb13818..6b84164 100644 --- a/scripts/lib/process.py +++ b/scripts/lib/process.py @@ -15,9 +15,11 @@ import os import signal +import stat import subprocess import sys import tempfile +from collections.abc import Iterable from pathlib import Path from typing import BinaryIO @@ -86,6 +88,26 @@ def write_file_atomic(path: Path, data: bytes, mode: int) -> None: os.close(dir_fd) +def snapshot_files(paths: Iterable[Path]) -> dict[Path, tuple[bytes, int] | None]: + """Record each file's content and mode, or None where it does not exist.""" + saved: dict[Path, tuple[bytes, int] | None] = {} + for path in paths: + try: + saved[path] = (path.read_bytes(), stat.S_IMODE(path.stat().st_mode)) + except FileNotFoundError: + saved[path] = None + return saved + + +def restore_files(saved: dict[Path, tuple[bytes, int] | None]) -> None: + """Put back every file snapshot_files recorded, and remove those it did not find.""" + for path, before in saved.items(): + if before is None: + path.unlink(missing_ok=True) + else: + write_file_atomic(path, *before) + + def setup_directories(cfg) -> None: """Create required directories.""" logger.info("Setting up directories...") diff --git a/scripts/lib/wireguard.py b/scripts/lib/wireguard.py index de4a58d..094148c 100644 --- a/scripts/lib/wireguard.py +++ b/scripts/lib/wireguard.py @@ -89,6 +89,24 @@ def slot_infix(slot: int) -> str: return "" if slot == 1 else str(slot) +def client_config_name( + client_name: str, slot: int, mode: str, https: bool = False +) -> str: + """Filename of one slot's split or full config, plain or over HTTPS.""" + tunnel = "-https" if https else "" + return f"{client_name}-wg{slot_infix(slot)}{tunnel}-{mode}.conf" + + +def client_config_names(client_name: str, slot: int, https: bool) -> list[str]: + """Filenames of every config one slot is issued.""" + variants = (False, True) if https else (False,) + return [ + client_config_name(client_name, slot, mode, https=tunnelled) + for tunnelled in variants + for mode in ("split", "full") + ] + + def existing_peer_ids(pki_dir: Path, client_name: str) -> list[str]: """Every peer identity currently on disk for a client, slot order. @@ -534,6 +552,10 @@ def setup_wireguard(cfg) -> None: logger.info(f"WireGuard config written to {cfg.wg_conf}") +class WireGuardSyncError(RuntimeError): + """The running wg0 interface refused the new peer list and kept the old one.""" + + def sync_running_interface(conf_path: Path) -> bool: """Apply a changed wg0.conf to the live interface. @@ -544,6 +566,10 @@ def sync_running_interface(conf_path: Path) -> bool: Returns False when there is no interface to sync, which is the normal case for issuing a config before the server has started. A missing `wg` binary is the same case: there is no WireGuard here to sync. + + Raises: + WireGuardSyncError: The interface is up and did not take the new peers, + so it still accepts whatever keys it held before. """ try: shown = subprocess.run(["wg", "show", "wg0"], capture_output=True) @@ -562,8 +588,9 @@ def sync_running_interface(conf_path: Path) -> bool: errors="replace", ) if stripped.returncode != 0: - logger.warning(f"Could not read {conf_path}: {stripped.stderr.strip()}") - return False + raise WireGuardSyncError( + f"could not read {conf_path}: {stripped.stderr.strip()}" + ) # syncconf takes a path, and the stripped form still carries the server # private key, so it goes to a 0600 file beside the config and is removed. @@ -581,8 +608,9 @@ def sync_running_interface(conf_path: Path) -> bool: tmp_path.unlink(missing_ok=True) if result.returncode != 0: - logger.warning(f"Could not apply peers to wg0: {result.stderr.strip()}") - return False + raise WireGuardSyncError( + f"wg0 refused the new peer list: {result.stderr.strip()}" + ) logger.info("Applied peer list to the running wg0 interface") return True diff --git a/tests/unit/test_generate_client.py b/tests/unit/test_generate_client.py index d30ab4f..af935aa 100644 --- a/tests/unit/test_generate_client.py +++ b/tests/unit/test_generate_client.py @@ -14,6 +14,7 @@ """ import importlib.util +import os import stat import sys import zipfile @@ -514,3 +515,80 @@ def test_https_tunnel_variants_when_enabled(self, run, monkeypatch): tunnelled = (out / f"{CLIENT}-wg-https-split.conf").read_text(encoding="utf-8") assert "Endpoint = 127.0.0.1:51820" in tunnelled assert f"wss://{SERVER_CN}:4443" in tunnelled + + +# Stand-ins for wg and wg-quick against a server whose interface is up. Keys are +# real-shaped base64, and FAKE_WG_SYNC_FAIL makes the live sync refuse. +FAKE_WG = """ +import base64 +import hashlib +import os +import sys + +command = sys.argv[1] +if command == "genkey": + print(base64.b64encode(os.urandom(32)).decode()) +elif command == "pubkey": + print(base64.b64encode(hashlib.sha256(sys.stdin.read().encode()).digest()).decode()) +elif command == "syncconf" and os.environ.get("FAKE_WG_SYNC_FAIL"): + sys.exit("Unable to modify interface: Operation not permitted") +""" +FAKE_WG_QUICK = """ +import sys +from pathlib import Path + +print(Path(sys.argv[2]).read_text(), end="") +""" + + +@pytest.fixture +def wg_tools(tmp_path, monkeypatch): + """Put wg and wg-quick stand-ins first on PATH, for a server that is running.""" + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + for name, body in (("wg", FAKE_WG), ("wg-quick", FAKE_WG_QUICK)): + script = bin_dir / name + script.write_text(f"#!{sys.executable}\n{body}", encoding="utf-8") + script.chmod(0o755) + monkeypatch.setenv("PATH", f"{bin_dir}:{os.environ['PATH']}") + monkeypatch.delenv("FAKE_WG_SYNC_FAIL", raising=False) + + +def _wireguard_state(pki, out: Path, wg_conf: Path) -> dict[str, bytes]: + """Every file an issue can change, by path.""" + paths = [*(pki.dir / "wireguard").rglob("*"), *out.glob("*"), wg_conf] + return {str(p): p.read_bytes() for p in paths if p.is_file()} + + +class TestWireGuardLiveSyncFailure: + """A peer list the running interface refused is not an issued client. + + Reporting success would leave the kernel on the old key after a --rotate, + so the retired key keeps working while the operator believes it is gone. + """ + + def test_a_refused_rotate_fails_and_keeps_the_live_key( + self, run, pki, wg_tools, monkeypatch + ): + run("--protocol", "wireguard", "--name", CLIENT) + before = _wireguard_state(pki, run.out, run.wg_conf) + + monkeypatch.setenv("FAKE_WG_SYNC_FAIL", "1") + with pytest.raises(SystemExit) as exc: + run("--protocol", "wireguard", "--name", CLIENT, "--rotate") + + assert exc.value.code == 1 + assert _wireguard_state(pki, run.out, run.wg_conf) == before + + def test_a_refused_new_client_leaves_nothing_issued( + self, run, pki, wg_tools, monkeypatch + ): + run("--protocol", "wireguard", "--name", CLIENT) + before = _wireguard_state(pki, run.out, run.wg_conf) + + monkeypatch.setenv("FAKE_WG_SYNC_FAIL", "1") + with pytest.raises(SystemExit) as exc: + run("--protocol", "wireguard", "--name", SIBLING) + + assert exc.value.code == 1 + assert _wireguard_state(pki, run.out, run.wg_conf) == before From 5b6e7a21077622edfd54fa4e4d49d15e46ee05a8 Mon Sep 17 00:00:00 2001 From: Derek Date: Sun, 1 Nov 2026 19:08:40 +1100 Subject: [PATCH 08/24] fix: validate fetched external PKI as one set before writing any of it The external fetch wrote each file the moment it arrived, after a check that only looked for "-----BEGIN ", so a failure on the server key left a new CA beside the old certificate, and a key from another server or a CRL from another CA was accepted. All files are now fetched first, then checked together with openssl: the certificate verifies against the CA and is in date, the key matches it, the CRL is signed by the CA and the tls-crypt-v2 key is a server key. Nothing is written unless the whole set passes. The CRL refresh checks a re-fetched CRL against the CA on disk the same way. --- docs/use-case-kubernetes.md | 12 +- scripts/lib/pki.py | 237 +++++++++++++++----------- tests/conftest.py | 25 ++- tests/integration/test_openbao_pki.py | 57 +++---- tests/unit/test_pki.py | 196 +++++++++++++++------ 5 files changed, 327 insertions(+), 200 deletions(-) diff --git a/docs/use-case-kubernetes.md b/docs/use-case-kubernetes.md index 379efbb..0740859 100644 --- a/docs/use-case-kubernetes.md +++ b/docs/use-case-kubernetes.md @@ -195,11 +195,11 @@ extraEnv: Production clusters generally do not want a self-generated CA. Set `CULVERT_PKI_MODE=external` and point at one of: -- **File** - certs mounted from a Kubernetes Secret (or cert-manager) volume. +- File: certs mounted from a Kubernetes Secret (or cert-manager) volume. `CULVERT_SECRETS_*_PATH` are file paths. The values below do this. -- **OpenBao** - Kubernetes auth exchanges the pod ServiceAccount token for a +- OpenBao: Kubernetes auth exchanges the pod ServiceAccount token for a short-lived token, with no static secret in env. -- **AWS Secrets Manager** - use IRSA on EKS, with no static AWS keys. +- AWS Secrets Manager: use IRSA on EKS, with no static AWS keys. For the file provider, create the Secret, then add this to `values.yaml`: @@ -242,6 +242,12 @@ optional, so drop their paths if the Secret does not carry them. With no `tc.key` the server mints its own tls-crypt-v2 key, and a client config only works against the server that issued it. +The fetched files are checked as one set before any is written: the server +certificate must verify against `ca.crt` and be in date, `server.key` must be its +key, `crl.pem` must be signed by `ca.crt`, and `tc.key` must be a tls-crypt-v2 +server key. If any check fails, nothing is written and the server starts on the +files already in its PKI volume. + Detail is in [External PKI in the README](../README.md#external-pki) and the External PKI section of [.env.example](../.env.example). diff --git a/scripts/lib/pki.py b/scripts/lib/pki.py index b7eb930..9385267 100644 --- a/scripts/lib/pki.py +++ b/scripts/lib/pki.py @@ -19,7 +19,9 @@ import random import re import shlex +import subprocess import sys +import tempfile import time as _time from datetime import UTC, datetime from pathlib import Path @@ -58,26 +60,71 @@ def client_tc_key_path(pki_dir: Path, client_name: str) -> Path: # --------------------------------------------------------------------------- -# PEM validation +# PKI material validation # --------------------------------------------------------------------------- +TC_SERVER_KEY_BEGIN = b"-----BEGIN OpenVPN tls-crypt-v2 server key-----" +TC_SERVER_KEY_END = b"-----END OpenVPN tls-crypt-v2 server key-----" -def _validate_pem(data: bytes, name: str) -> bool: - """Check that data contains a PEM block. - Anywhere in the file, not only at the start: Easy-RSA and `openssl x509` - write a human-readable dump of the certificate above the BEGIN line, and - OpenVPN reads such a file quite happily. Requiring the header first rejected - the output of the CA culvert itself ships with. +def _openssl(args: list[str], data: bytes) -> subprocess.CompletedProcess: + """Run openssl on data from stdin, so key material never touches a file.""" + return subprocess.run( + ["openssl", *args], input=data, capture_output=True, check=False + ) + + +def crl_problem(crl: bytes, ca_file: Path) -> str | None: + """Return why crl cannot be used with the CA in ca_file, or None.""" + if _openssl(["crl", "-noout", "-CAfile", str(ca_file)], crl).returncode != 0: + return "the CRL is not a CRL signed by the CA" + return None + - Non-fragile: logs a warning on failure but never crashes. +def pki_set_problems( + ca: bytes, + cert: bytes, + key: bytes, + crl: bytes | None = None, + tc_key: bytes | None = None, +) -> list[str]: + """Return what stops this material serving as one server's PKI, empty if none. + + Each file parsing is not enough: a key from another server, or a certificate + from another CA, passes that and fails every client handshake. The server + certificate is also checked for expiry, by the chain verification. """ - if b"-----BEGIN " in data: - return True - logger.error( - f"Fetched content for '{name}' is not valid PEM (starts with: {data[:40]!r})" - ) - return False + problems = [] + if _openssl(["x509", "-noout"], ca).returncode != 0: + return ["the CA certificate is not an X.509 certificate"] + + cert_public = _openssl(["x509", "-noout", "-pubkey"], cert) + key_public = _openssl(["pkey", "-pubout"], key) + if cert_public.returncode != 0: + problems.append("the server certificate is not an X.509 certificate") + if key_public.returncode != 0: + problems.append("the server private key is not a private key") + elif cert_public.returncode == 0 and cert_public.stdout != key_public.stdout: + problems.append("the server private key does not match the server certificate") + + with tempfile.TemporaryDirectory() as tmp: + ca_file = Path(tmp) / "ca.crt" + ca_file.write_bytes(ca) + if cert_public.returncode == 0: + verify = _openssl(["verify", "-CAfile", str(ca_file)], cert) + if verify.returncode != 0: + detail = verify.stderr.decode("utf-8", "replace").strip() + problems.append(f"the server certificate does not verify: {detail}") + if crl is not None and (problem := crl_problem(crl, ca_file)): + problems.append(problem) + + if tc_key is not None and not ( + TC_SERVER_KEY_BEGIN in tc_key and TC_SERVER_KEY_END in tc_key + ): + problems.append( + "the tls-crypt-v2 key is not an OpenVPN tls-crypt-v2 server key" + ) + return problems # --------------------------------------------------------------------------- @@ -126,32 +173,13 @@ def create_manager(cfg): # --------------------------------------------------------------------------- -def _fetch_one( - manager, - provider_name: str, - secret_ref: str, - dest: Path, - perms: int, - name: str, -) -> bool: - """Fetch a single secret and write to disk if valid PEM. - - Returns True if the file was successfully written. - Does not overwrite existing local file if fetched content is invalid. - """ +def _fetch_one(manager, provider_name: str, secret_ref: str, name: str) -> bytes | None: + """Fetch one secret's bytes, or None when the provider cannot supply it.""" try: - result = manager.get_sync(secret_ref, provider=provider_name) - data = result.data - if not _validate_pem(data, name): - logger.warning(f"Skipping {name} - invalid PEM from '{secret_ref}'") - return False - dest.parent.mkdir(parents=True, exist_ok=True) - write_file_atomic(dest, data, perms) - logger.info(f" {name}: written to {dest}") - return True + return manager.get_sync(secret_ref, provider=provider_name).data except Exception as e: logger.warning(f"Failed to fetch {name} from '{secret_ref}': {e}") - return False + return None def _health_check_with_retry(manager, provider_name: str, retries: int = 3) -> bool: @@ -186,76 +214,80 @@ def fetch_external_pki(cfg) -> bool: logger.warning(f"Provider '{provider_name}' is unreachable") return False - # Fetch required certs - required_ok = all( - [ - _fetch_one( - manager, - provider_name, - cfg.secrets_ca_cert_path, - cfg.pki_dir / "ca.crt", - 0o644, - "CA certificate", - ), - _fetch_one( - manager, - provider_name, - cfg.secrets_server_cert_path, - cfg.pki_dir / "issued" / "server.crt", - 0o644, - "server certificate", - ), - _fetch_one( - manager, - provider_name, - cfg.secrets_server_key_path, - cfg.pki_dir / "private" / "server.key", - 0o600, - "server private key", - ), - ] - ) - - # CRL is optional - if cfg.secrets_crl_path: - _fetch_one( - manager, - provider_name, - cfg.secrets_crl_path, - cfg.pki_dir / "crl.pem", + wanted = { + "CA certificate": (cfg.secrets_ca_cert_path, "ca.crt", 0o644), + "server certificate": ( + cfg.secrets_server_cert_path, + "issued/server.crt", 0o644, - "CRL", - ) + ), + "server private key": ( + cfg.secrets_server_key_path, + "private/server.key", + 0o600, + ), + } + if cfg.secrets_crl_path: + wanted["CRL"] = (cfg.secrets_crl_path, "crl.pem", 0o644) else: logger.info(" CRL: not configured (optional)") - # The tls-crypt-v2 server key. Optional, and minted locally when the # provider does not carry one - but then every server has a different # key, and a client config only works against the server that issued it. # Supply it here to run more than one server behind one address. if cfg.secrets_tc_key_path: - # Required once configured. Asking for a shared key and silently - # getting a locally minted one is worse than not asking: the install - # comes up healthy and rejects clients at random, which is the exact - # failure the setting exists to prevent. - if not _fetch_one( - manager, - provider_name, + wanted["tls-crypt-v2 server key"] = ( cfg.secrets_tc_key_path, - cfg.pki_dir / "tc.key", + "tc.key", 0o600, - "tls-crypt-v2 server key", - ): - logger.error( - "CULVERT_SECRETS_TC_KEY_PATH is set but the key could not be" - f" fetched from '{cfg.secrets_tc_key_path}'. Refusing to mint" - " a local one - siblings would reject each other's clients." - ) - required_ok = False + ) else: logger.info(" tls-crypt-v2 key: not configured (minted locally)") - return required_ok + fetched = { + name: _fetch_one(manager, provider_name, ref, name) + for name, (ref, _, _) in wanted.items() + } + # Required once configured, the shared tls-crypt-v2 key included. Asking + # for a shared key and silently getting a locally minted one is worse + # than not asking: the install comes up healthy and rejects clients at + # random, which is the exact failure the setting exists to prevent. + missing = [ + name for name, data in fetched.items() if data is None and name != "CRL" + ] + if fetched.get("CRL", b"") is None: + logger.warning("CRL could not be fetched, keeping the one on disk") + del fetched["CRL"] + if missing: + logger.error( + f"Not writing any fetched PKI material: missing {', '.join(missing)}" + ) + return False + + problems = pki_set_problems( + fetched["CA certificate"], + fetched["server certificate"], + fetched["server private key"], + crl=fetched.get("CRL"), + tc_key=fetched.get("tls-crypt-v2 server key"), + ) + if problems: + for problem in problems: + logger.error(f"Fetched PKI rejected: {problem}") + logger.error( + "Not writing any fetched PKI material, keeping what is on disk" + ) + return False + + # Only a set that passed together is written, so the files on disk are + # never one server's certificate beside another's key. + for name, data in fetched.items(): + _, relative, perms = wanted[name] + dest = cfg.pki_dir / relative + dest.parent.mkdir(parents=True, exist_ok=True) + write_file_atomic(dest, data, perms) + logger.info(f" {name}: written to {dest}") + return True finally: try: # close() is async - run the coroutine or aiohttp sessions leak @@ -521,14 +553,15 @@ def refetch_external_crl(cfg) -> bool: " CRL already on disk" ) return False - return _fetch_one( - manager, - cfg.secrets_provider, - cfg.secrets_crl_path, - cfg.pki_dir / "crl.pem", - 0o644, - "CRL", - ) + crl = _fetch_one(manager, cfg.secrets_provider, cfg.secrets_crl_path, "CRL") + if crl is None: + return False + # A CRL the running CA did not sign makes OpenVPN refuse every client. + if problem := crl_problem(crl, cfg.pki_dir / "ca.crt"): + logger.error(f"Fetched CRL rejected, keeping the one on disk: {problem}") + return False + write_file_atomic(cfg.pki_dir / "crl.pem", crl, 0o644) + return True finally: try: import asyncio diff --git a/tests/conftest.py b/tests/conftest.py index 5e80ac5..bb4ee7c 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -75,7 +75,8 @@ def write_crl(): to `openssl crl`, so anything less would not exercise the parse. """ - def _write(path, next_update): + def _write(path, next_update, *, issuer=None, revoked=()): + """Sign with issuer (an issue_cert CA) or a throwaway key, revoking serials.""" from datetime import timedelta from cryptography import x509 @@ -83,17 +84,29 @@ def _write(path, next_update): from cryptography.hazmat.primitives.asymmetric import ec from cryptography.x509.oid import NameOID - key = ec.generate_private_key(ec.SECP256R1()) - issuer = x509.Name([x509.NameAttribute(NameOID.COMMON_NAME, "culvert-test-ca")]) + if issuer is None: + key = ec.generate_private_key(ec.SECP256R1()) + name = x509.Name( + [x509.NameAttribute(NameOID.COMMON_NAME, "culvert-test-ca")] + ) + else: + key, name = issuer.key, issuer.cert.subject # Issued before it expires, whether next_update is past or future. last_update = min(datetime.now(UTC), next_update) - timedelta(days=1) - crl = ( + builder = ( x509.CertificateRevocationListBuilder() - .issuer_name(issuer) + .issuer_name(name) .last_update(last_update) .next_update(next_update) - .sign(private_key=key, algorithm=hashes.SHA256()) ) + for serial in revoked: + builder = builder.add_revoked_certificate( + x509.RevokedCertificateBuilder() + .serial_number(serial) + .revocation_date(last_update) + .build() + ) + crl = builder.sign(private_key=key, algorithm=hashes.SHA256()) path.write_bytes(crl.public_bytes(serialization.Encoding.PEM)) return _write diff --git a/tests/integration/test_openbao_pki.py b/tests/integration/test_openbao_pki.py index 9a55ac6..eef1f6b 100644 --- a/tests/integration/test_openbao_pki.py +++ b/tests/integration/test_openbao_pki.py @@ -28,28 +28,8 @@ reason="OPENBAO_ADDR not set - no OpenBao access", ) -FAKE_CA = ( - "-----BEGIN CERTIFICATE-----\n" - "TEST_CA_CERT_FOR_CULVERT_INTEGRATION\n" - "-----END CERTIFICATE-----\n" -) -FAKE_CERT = ( - "-----BEGIN CERTIFICATE-----\n" - "TEST_SERVER_CERT_FOR_CULVERT_INTEGRATION\n" - "-----END CERTIFICATE-----\n" -) -FAKE_KEY = ( - "-----BEGIN PRIVATE KEY-----\n" - "TEST_SERVER_KEY_FOR_CULVERT_INTEGRATION\n" - "-----END PRIVATE KEY-----\n" -) - SECRET_BASE = "secret/data/culvert-test" -TEST_SECRETS = { - "ca-cert": FAKE_CA, - "server-cert": FAKE_CERT, - "server-key": FAKE_KEY, -} +SECRET_NAMES = ("ca-cert", "server-cert", "server-key") def _bao_request(method: str, path: str, data: dict | None = None): @@ -74,21 +54,33 @@ def _bao_request(method: str, path: str, data: dict | None = None): return urllib.request.urlopen(req, timeout=10, context=ctx) -@pytest.fixture(scope="module") -def openbao_test_secrets(): - """Store test secrets in OpenBao, yield, then clean up.""" - # Write test secrets - for name, content in TEST_SECRETS.items(): +@pytest.fixture +def openbao_test_secrets(issue_cert): + """Store a real CA, server certificate and key in OpenBao, then clean up. + + Real material, because the fetch accepts only a server certificate that + verifies against the CA and matches the key. + """ + ca = issue_cert("culvert-integration-ca", is_ca=True) + server = issue_cert("culvert-integration-server", issuer=ca) + secrets = dict( + zip( + SECRET_NAMES, + (pem.decode() for pem in (ca.cert_pem, server.cert_pem, server.key_pem)), + strict=True, + ) + ) + for name, content in secrets.items(): _bao_request( "POST", f"{SECRET_BASE}/{name}", {"data": {"value": content}}, ) - yield + yield secrets # Cleanup - for name in TEST_SECRETS: + for name in SECRET_NAMES: try: _bao_request("DELETE", f"{SECRET_BASE}/{name}") except Exception: @@ -144,11 +136,8 @@ def test_fetch_certs_from_openbao(openbao_test_secrets, tmp_path): result = fetch_external_pki(cfg) assert result is True - ca = (pki / "ca.crt").read_text() - assert "TEST_CA_CERT" in ca - + assert (pki / "ca.crt").read_text() == openbao_test_secrets["ca-cert"] cert = (pki / "issued" / "server.crt").read_text() - assert "TEST_SERVER_CERT" in cert - + assert cert == openbao_test_secrets["server-cert"] key = (pki / "private" / "server.key").read_text() - assert "TEST_SERVER_KEY" in key + assert key == openbao_test_secrets["server-key"] diff --git a/tests/unit/test_pki.py b/tests/unit/test_pki.py index 650abcf..b06a5e2 100644 --- a/tests/unit/test_pki.py +++ b/tests/unit/test_pki.py @@ -16,51 +16,94 @@ import pytest # --------------------------------------------------------------------------- -# PEM validation +# PKI material validation # --------------------------------------------------------------------------- +TC_SERVER_KEY = ( + b"-----BEGIN OpenVPN tls-crypt-v2 server key-----\n" + b"dGxzLWNyeXB0LXYyIHNlcnZlciBrZXk=\n" + b"-----END OpenVPN tls-crypt-v2 server key-----\n" +) -class TestValidatePem: - """Tests for PEM content validation.""" - def test_valid_pem_cert(self): - from lib.pki import _validate_pem +@pytest.fixture +def material(issue_cert, write_crl, tmp_path): + """A CA, a server certificate it signed, and a CRL it signed, as real PEM.""" + ca = issue_cert("culvert-test-ca", is_ca=True) + server = issue_cert("vpn.example.test", issuer=ca) + crl = tmp_path / "material-crl.pem" + write_crl(crl, datetime.now(UTC) + timedelta(days=30), issuer=ca) + return SimpleNamespace(ca=ca, server=server, crl=crl.read_bytes()) - data = b"-----BEGIN CERTIFICATE-----\nMIIB...\n-----END CERTIFICATE-----\n" - assert _validate_pem(data, "test") is True - def test_valid_pem_key(self): - from lib.pki import _validate_pem +class TestPkiSetProblems: + """Fetched material is accepted only as one coherent server PKI.""" - data = b"-----BEGIN PRIVATE KEY-----\nMIIE...\n-----END PRIVATE KEY-----\n" - assert _validate_pem(data, "test") is True - - def test_invalid_pem(self): - from lib.pki import _validate_pem - - assert _validate_pem(b"not a certificate", "test") is False - - def test_empty_data(self): - from lib.pki import _validate_pem - - assert _validate_pem(b"", "test") is False - - def test_pem_with_leading_whitespace(self): - from lib.pki import _validate_pem - - data = b" \n-----BEGIN CERTIFICATE-----\ndata\n" - assert _validate_pem(data, "test") is True - - def test_pem_after_a_certificate_text_dump(self): - """Easy-RSA's own issued/server.crt looks exactly like this.""" - from lib.pki import _validate_pem + @staticmethod + def _problems(material, issue_cert, write_crl, tmp_path, case): + from lib.pki import pki_set_problems + + files = { + "ca": material.ca.cert_pem, + "cert": material.server.cert_pem, + "key": material.server.key_pem, + "crl": material.crl, + "tc_key": TC_SERVER_KEY, + } + if case == "key_of_another_server": + files["key"] = issue_cert("other", issuer=material.ca).key_pem + elif case == "cert_from_another_ca": + other_ca = issue_cert("other-ca", is_ca=True) + other = issue_cert("vpn.example.test", issuer=other_ca) + files["cert"], files["key"] = other.cert_pem, other.key_pem + elif case == "expired_cert": + expired = issue_cert("vpn.example.test", issuer=material.ca, valid_days=-1) + files["cert"], files["key"] = expired.cert_pem, expired.key_pem + elif case == "pem_shaped_garbage": + files["cert"] = ( + b"-----BEGIN CERTIFICATE-----\nFAKE\n-----END CERTIFICATE-----\n" + ) + elif case == "crl_from_another_ca": + write_crl(tmp_path / "other.pem", datetime.now(UTC) + timedelta(days=30)) + files["crl"] = (tmp_path / "other.pem").read_bytes() + elif case == "client_tc_key": + files["tc_key"] = TC_SERVER_KEY.replace(b"server", b"client") + return pki_set_problems( + files["ca"], files["cert"], files["key"], files["crl"], files["tc_key"] + ) - data = ( - b"Certificate:\n Data:\n Version: 3 (0x2)\n" - b" Serial Number: 1 (0x1)\n" - b"-----BEGIN CERTIFICATE-----\nMIIB...\n-----END CERTIFICATE-----\n" + @pytest.mark.parametrize( + ("case", "want"), + [ + pytest.param("valid", None, id="valid"), + pytest.param("key_of_another_server", "does not match", id="foreign_key"), + pytest.param("cert_from_another_ca", "does not verify", id="foreign_ca"), + pytest.param("expired_cert", "does not verify", id="expired"), + pytest.param("pem_shaped_garbage", "not an X.509", id="garbage"), + pytest.param("crl_from_another_ca", "CRL", id="foreign_crl"), + pytest.param("client_tc_key", "tls-crypt-v2", id="client_tc_key"), + ], + ) + def test_material(self, material, issue_cert, write_crl, tmp_path, case, want): + problems = self._problems(material, issue_cert, write_crl, tmp_path, case) + if want is None: + assert problems == [] + else: + assert any(want in problem for problem in problems), problems + + def test_a_certificate_text_dump_is_accepted(self, material): + """Easy-RSA writes a readable dump above the PEM block of what it issues.""" + from lib.pki import pki_set_problems + + dump = b"Certificate:\n Data:\n Version: 3 (0x2)\n" + assert ( + pki_set_problems( + dump + material.ca.cert_pem, + dump + material.server.cert_pem, + material.server.key_pem, + ) + == [] ) - assert _validate_pem(data, "server certificate") is True # --------------------------------------------------------------------------- @@ -171,17 +214,17 @@ class TestFetchExternalPki: """Tests for external PKI fetch with resilience.""" @pytest.fixture - def pki_env(self, tmp_path, clean_env, monkeypatch): + def pki_env(self, tmp_path, clean_env, monkeypatch, material): """Set up temp PKI dir and source cert files for FileProvider.""" from lib.config import Config # Source certs (what FileProvider reads) src = tmp_path / "source" src.mkdir() - (src / "ca.crt").write_text(FAKE_CA) - (src / "server.crt").write_text(FAKE_CERT) - (src / "server.key").write_text(FAKE_KEY) - (src / "crl.pem").write_text(FAKE_CRL) + (src / "ca.crt").write_bytes(material.ca.cert_pem) + (src / "server.crt").write_bytes(material.server.cert_pem) + (src / "server.key").write_bytes(material.server.key_pem) + (src / "crl.pem").write_bytes(material.crl) # PKI dest dir pki = tmp_path / "pki" @@ -323,6 +366,32 @@ def test_invalid_pem_not_written(self, pki_env): # Good local file preserved assert "FAKE_CA" in (pki / "ca.crt").read_text() + @pytest.mark.parametrize("source_key", ["another_server", "missing"]) + def test_a_bad_set_writes_none_of_it(self, pki_env, issue_cert, source_key): + """The CA and certificate are not written ahead of a key that fails.""" + from lib.pki import fetch_external_pki + + cfg, pki, src = pki_env + if source_key == "missing": + cfg.secrets_server_key_path = str(src / "absent.key") + else: + other = issue_cert("another-server") + (src / "server.key").write_bytes(other.key_pem) + + assert fetch_external_pki(cfg) is False + assert not (pki / "ca.crt").exists() + assert not (pki / "issued" / "server.crt").exists() + assert not (pki / "private" / "server.key").exists() + + def test_a_crl_the_ca_did_not_sign_is_refused(self, pki_env, write_crl): + from lib.pki import fetch_external_pki + + cfg, pki, src = pki_env + write_crl(src / "crl.pem", datetime.now(UTC) + timedelta(days=30)) + + assert fetch_external_pki(cfg) is False + assert not (pki / "crl.pem").exists() + class TestCrlRefresh: """The CRL has to keep moving, in BOTH PKI modes. @@ -334,21 +403,23 @@ class TestCrlRefresh: """ @pytest.fixture - def crl_env(self, tmp_path, clean_env, monkeypatch): + def crl_env(self, tmp_path, clean_env, monkeypatch, material): """External-PKI config backed by real files, as the fetch tests use.""" from lib.config import Config src = tmp_path / "source" src.mkdir() - (src / "ca.crt").write_text(FAKE_CA) - (src / "server.crt").write_text(FAKE_CERT) - (src / "server.key").write_text(FAKE_KEY) - (src / "crl.pem").write_text(FAKE_CRL) + (src / "ca.crt").write_bytes(material.ca.cert_pem) + (src / "server.crt").write_bytes(material.server.cert_pem) + (src / "server.key").write_bytes(material.server.key_pem) + (src / "crl.pem").write_bytes(material.crl) pki = tmp_path / "pki" pki.mkdir() (pki / "issued").mkdir() (pki / "private").mkdir() + # The CA the server runs on, which a fetched CRL must be signed by. + (pki / "ca.crt").write_bytes(material.ca.cert_pem) monkeypatch.setenv("CULVERT_PKI_MODE", "external") monkeypatch.setenv("CULVERT_SECRETS_PROVIDER", "file") @@ -388,18 +459,30 @@ def test_external_without_crl_path_has_no_refresher(self, crl_env): cfg.secrets_crl_path = "" assert crl_refresher(cfg) is None - def test_refetch_picks_up_an_upstream_change(self, crl_env): + def test_refetch_picks_up_an_upstream_change(self, crl_env, material, write_crl): """A CRL updated at the provider reaches disk without a restart.""" from lib.pki import refetch_external_crl cfg, pki, src = crl_env - (pki / "crl.pem").write_text(FAKE_CRL) - - updated = FAKE_CRL.replace("FAKE_CRL", "FAKE_CRL_REVOKED_ALICE") - (src / "crl.pem").write_text(updated) + (pki / "crl.pem").write_bytes(material.crl) + next_update = datetime.now(UTC) + timedelta(days=30) + write_crl(src / "crl.pem", next_update, issuer=material.ca, revoked=[4242]) assert refetch_external_crl(cfg) is True - assert "FAKE_CRL_REVOKED_ALICE" in (pki / "crl.pem").read_text() + assert (pki / "crl.pem").read_bytes() == (src / "crl.pem").read_bytes() + + def test_refetch_refuses_a_crl_another_ca_signed( + self, crl_env, material, write_crl + ): + """OpenVPN refuses every client against a CRL its CA did not sign.""" + from lib.pki import refetch_external_crl + + cfg, pki, src = crl_env + (pki / "crl.pem").write_bytes(material.crl) + write_crl(src / "crl.pem", datetime.now(UTC) + timedelta(days=30)) + + assert refetch_external_crl(cfg) is False + assert (pki / "crl.pem").read_bytes() == material.crl def test_refetch_keeps_the_existing_crl_when_unreachable(self, crl_env): """A provider that cannot be read must not blank the CRL on disk. @@ -450,12 +533,15 @@ def test_an_expired_crl_is_reported_at_startup( start_crl_refresh(cfg, SimpleNamespace(shutdown_requested=True)) assert any("CRL has expired" in e for e in errors), errors - def test_a_refreshed_crl_makes_openvpn_reread_it(self, crl_env): + def test_a_refreshed_crl_makes_openvpn_reread_it( + self, crl_env, material, write_crl + ): """OpenVPN reads the CRL at startup, so a new file needs a reload.""" from lib.pki import start_crl_refresh cfg, pki, src = crl_env - (src / "crl.pem").write_text(FAKE_CRL.replace("FAKE_CRL", "REVOKED_BOB")) + next_update = datetime.now(UTC) + timedelta(days=30) + write_crl(src / "crl.pem", next_update, issuer=material.ca, revoked=[4242]) class OneCycle: """Stands in for ProcessManager: stops the loop after its first reload.""" @@ -474,7 +560,7 @@ def _reload_handler(self, signum, frame): start_crl_refresh(cfg, manager, interval_hours=0.0003) assert manager.reloaded.wait(timeout=30), "the refreshed CRL was not reloaded" - assert "REVOKED_BOB" in (pki / "crl.pem").read_text() + assert (pki / "crl.pem").read_bytes() == (src / "crl.pem").read_bytes() class TestRegenerateLocalCrl: From 6c0eb499b773136b9d20937265cae358dfce8ef2 Mon Sep 17 00:00:00 2001 From: Derek Date: Sun, 1 Nov 2026 19:09:49 +1100 Subject: [PATCH 09/24] fix: never mint a local tls-crypt-v2 key when a shared one is configured With CULVERT_SECRETS_TC_KEY_PATH set and the provider unreachable at startup, init_pki fell back to the cached certificates and then minted a local tc.key, so the server came up healthy on a key no sibling shares and refused their clients. It now exits naming the setting unless a previously fetched tc.key is cached. --- scripts/lib/pki.py | 14 +++++++++++--- tests/unit/test_pki.py | 25 +++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/scripts/lib/pki.py b/scripts/lib/pki.py index 9385267..b6cf801 100644 --- a/scripts/lib/pki.py +++ b/scripts/lib/pki.py @@ -340,11 +340,19 @@ def init_pki(cfg) -> None: ) sys.exit(1) - # Mint a tls-crypt-v2 key only if the provider did not supply one. A - # locally minted key is fine for a single server and wrong for several: - # a client's key derives from it, so siblings reject each other's clients. + # Mint a tls-crypt-v2 key only when none is configured. A locally minted + # key is fine for a single server and wrong for several: a client's key + # derives from it, so siblings reject each other's clients. tc_key = cfg.pki_dir / "tc.key" if not tc_key.exists(): + if cfg.secrets_tc_key_path: + logger.error( + "CULVERT_SECRETS_TC_KEY_PATH is set, but the shared" + f" tls-crypt-v2 key could not be fetched from" + f" '{cfg.secrets_tc_key_path}' and none is cached. Refusing to" + " mint a local one - siblings would reject each other's clients." + ) + sys.exit(1) generate_tc_key(cfg.pki_dir) return diff --git a/tests/unit/test_pki.py b/tests/unit/test_pki.py index b06a5e2..60d58c3 100644 --- a/tests/unit/test_pki.py +++ b/tests/unit/test_pki.py @@ -334,6 +334,31 @@ def test_fallback_to_local_certs(self, pki_env): init_pki(cfg) assert "FAKE_CA" in (pki / "ca.crt").read_text() + @pytest.mark.parametrize("cached_tc_key", [True, False], ids=["cached", "none"]) + def test_a_configured_shared_tc_key_is_never_minted_locally( + self, pki_env, cached_tc_key + ): + """A server holding its own tc.key rejects every client its siblings issue.""" + from lib.pki import init_pki + + cfg, pki, src = pki_env + cfg.secrets_tc_key_path = str(src / "tc.key") + (pki / "ca.crt").write_text(FAKE_CA) + (pki / "issued" / "server.crt").write_text(FAKE_CERT) + (pki / "private" / "server.key").write_text(FAKE_KEY) + if cached_tc_key: + (pki / "tc.key").write_bytes(TC_SERVER_KEY) + # The provider is down, so nothing is fetched. + cfg.secrets_ca_cert_path = "/nonexistent/ca.crt" + + if cached_tc_key: + init_pki(cfg) + assert (pki / "tc.key").read_bytes() == TC_SERVER_KEY + else: + with pytest.raises(SystemExit): + init_pki(cfg) + assert not (pki / "tc.key").exists() + def test_no_local_no_provider_exits(self, pki_env): """Provider fails AND no local certs - init_pki exits.""" from lib.pki import init_pki From aaff42502f63628c9f0c0387c4314dd1b64db7bc Mon Sep 17 00:00:00 2001 From: Derek Date: Sun, 1 Nov 2026 19:11:12 +1100 Subject: [PATCH 10/24] test: wait for the management stand-in before reading its commands revoke-client returns once it has sent quit, before the stand-in's thread records it, so the assertion read ['kill alice'] about one run in six. The test now waits on the stand-in's handled event, as test_management already does. --- tests/unit/test_revoke.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/unit/test_revoke.py b/tests/unit/test_revoke.py index e6e9089..4583c53 100644 --- a/tests/unit/test_revoke.py +++ b/tests/unit/test_revoke.py @@ -413,6 +413,8 @@ def test_live_sessions_are_ended_once_the_crl_carries_the_revocation( alice("--protocol", "openvpn", "alice") + # revoke-client returns before the stand-in has recorded "quit". + assert server.handled.wait(5) assert server.commands == ["kill alice", "quit"] assert crl_present == [True] From 26e8d295858f24c2e18c0f6e0e2c4d33fddfb60d Mon Sep 17 00:00:00 2001 From: Derek Date: Sun, 1 Nov 2026 19:13:13 +1100 Subject: [PATCH 11/24] fix: reload OpenVPN after a CRL refresh only when it revokes something new Every CRL refresh sent SIGHUP, which restarts every OpenVPN listener and drops every connected client, though OpenVPN re-reads the CRL on each handshake. The refresh loop now compares the revoked serials before and after, and reloads only when the new CRL revokes a certificate the old one did not, so that session ends before it renegotiates. A refresh that only moves nextUpdate leaves the server alone. --- docs/architecture.md | 14 ++++++++------ scripts/lib/pki.py | 25 ++++++++++++++++++++++--- tests/unit/test_pki.py | 42 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 72 insertions(+), 9 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index 92cfcf3..11c4324 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -81,7 +81,7 @@ unreachable, on cached certificates, with a warning. Three things here are not guessable from the code's shape: -- **`easyrsa init-pki` is never called.** All it creates is three directories, +- `easyrsa init-pki` is never called. All it creates is three directories, and it insists on removing the PKI directory first, unconditionally on 3.2.x with no opt-out. The directory is a volume mount, so the removal fails outright, and where it could succeed it would take the CA and every @@ -90,16 +90,18 @@ Three things here are not guessable from the code's shape: Culvert creates the directories itself, and `build-ca` still refuses to overwrite an existing CA, so a half-built PKI fails loudly instead of being replaced. -- **The `tls-crypt-v2` server key is minted locally only when no provider - supplies one.** A client's key derives from it, so two servers with +- The `tls-crypt-v2` server key is minted locally only when no provider + supplies one. A client's key derives from it, so two servers with different keys reject each other's clients. Once `CULVERT_SECRETS_TC_KEY_PATH` is set, a failed fetch is fatal rather than a fall back to minting, because an install that comes up healthy and rejects clients at random is worse than one that refuses to start. -- **CRL refresh differs by mode and cannot be shared.** Local PKI regenerates +- CRL refresh differs by mode and cannot be shared. Local PKI regenerates from its own CA. External PKI has no CA key present, so the only way to pick - up a revocation is to re-fetch. OpenVPN reads the CRL at startup, so a new - file on disk changes nothing until the server is told to re-read it. + up a revocation is to re-fetch. OpenVPN re-reads the CRL on every handshake, + so a new file needs no reload. The server reloads only when the new CRL + revokes a certificate the old one did not, to end that session before it + renegotiates, and that reload drops every connected client. A CRL that reaches its `nextUpdate` makes OpenVPN refuse every client, including valid ones. The refresher therefore reports remaining life on every diff --git a/scripts/lib/pki.py b/scripts/lib/pki.py index b6cf801..a314e83 100644 --- a/scripts/lib/pki.py +++ b/scripts/lib/pki.py @@ -493,6 +493,21 @@ def crl_seconds_until_expiry(pki_dir) -> float | None: return None +def revoked_serials(pki_dir) -> frozenset[str]: + """Serial numbers the CRL revokes, empty when there is no readable CRL.""" + crl_path = Path(pki_dir) / "crl.pem" + if not crl_path.exists(): + return frozenset() + result = run( + ["openssl", "crl", "-in", str(crl_path), "-noout", "-text"], + check=False, + capture=True, + ) + if result.returncode != 0: + return frozenset() + return frozenset(re.findall(r"Serial Number: ([0-9A-Fa-f]+)", result.stdout)) + + def log_crl_expiry(cfg) -> float | None: """Log how much CRL life is left, at a severity that matches the margin.""" remaining = crl_seconds_until_expiry(cfg.pki_dir) @@ -618,11 +633,15 @@ def refresh_loop(): break logger.info("Refreshing CRL...") try: + before = revoked_serials(cfg.pki_dir) if refresh(cfg): logger.info("CRL refreshed successfully") - # OpenVPN reads the CRL at startup, so a new file changes - # nothing until the server re-reads it. - proc_manager._reload_handler(None, None) + # OpenVPN re-reads the CRL per handshake, so a reload only + # ends a newly revoked session early, and drops all others. + newly = revoked_serials(cfg.pki_dir) - before + if newly: + logger.info("CRL revokes new certificates", count=len(newly)) + proc_manager._reload_handler(None, None) else: logger.warning( "CRL was not updated this cycle - the CRL on disk is" diff --git a/tests/unit/test_pki.py b/tests/unit/test_pki.py index 60d58c3..d073178 100644 --- a/tests/unit/test_pki.py +++ b/tests/unit/test_pki.py @@ -587,6 +587,48 @@ def _reload_handler(self, signum, frame): assert manager.reloaded.wait(timeout=30), "the refreshed CRL was not reloaded" assert (pki / "crl.pem").read_bytes() == (src / "crl.pem").read_bytes() + def test_a_refresh_that_revokes_nothing_new_does_not_reload( + self, crl_env, material, write_crl + ): + """A reload restarts every listener and drops every connected client. + + OpenVPN re-reads the CRL on each handshake, so a CRL whose only change is + a later nextUpdate needs nothing from the running server. + """ + from lib.pki import start_crl_refresh + + cfg, pki, src = crl_env + next_update = datetime.now(UTC) + timedelta(days=30) + write_crl(pki / "crl.pem", next_update, issuer=material.ca, revoked=[4242]) + later = next_update + timedelta(days=30) + write_crl(src / "crl.pem", later, issuer=material.ca, revoked=[4242]) + refreshed = (src / "crl.pem").read_bytes() + + class TwoChecks: + """Stands in for ProcessManager: stops the loop at its second check.""" + + def __init__(self): + self.checks = 0 + self.second_check = threading.Event() + self.reloads = 0 + + @property + def shutdown_requested(self): + self.checks += 1 + if self.checks >= 2: + self.second_check.set() + return self.checks >= 2 + + def _reload_handler(self, signum, frame): + self.reloads += 1 + + manager = TwoChecks() + start_crl_refresh(cfg, manager, interval_hours=0.0003) + + assert manager.second_check.wait(timeout=30), "the refresh loop never cycled" + assert (pki / "crl.pem").read_bytes() == refreshed + assert manager.reloads == 0 + class TestRegenerateLocalCrl: """Every regeneration carries the configured lifetime and stays readable. From 695a0c92f06c68747d4d65b09ad9c0208eb537a8 Mon Sep 17 00:00:00 2001 From: Derek Date: Sun, 1 Nov 2026 19:15:05 +1100 Subject: [PATCH 12/24] fix: refuse OpenVPN client signing and revocation under external PKI With CULVERT_PKI_MODE=external the CA key lives upstream, yet generate-client still ran Easy-RSA's sign-req and revoke-client its revoke, which fail with an Easy-RSA error at best. Both now refuse with the way through: issue at the CA, place the pair under issued/ and private/, and run generate-client --config-only, which now mints the client's tls-crypt-v2 key when it has none. Revocation is the CA's, picked up by the CRL refresh. The README's External PKI section says so. --- README.md | 6 +++ scripts/generate-client.py | 24 +++++++++--- scripts/revoke-client.py | 17 +++++++++ tests/unit/test_client_lifecycle.py | 58 +++++++++++++++++++++++++++++ 4 files changed, 100 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 8ead9a4..2fa4549 100644 --- a/README.md +++ b/README.md @@ -212,6 +212,12 @@ culvert supports three external PKI backends via Local PKI mode (self-generated Easy-RSA CA) is the default for quick starts and dev environments. +External PKI keeps the CA key at your CA, so culvert neither signs nor revokes +OpenVPN client certificates. Issue one there, place it at `pki/issued/.crt` +and `pki/private/.key`, and run `generate-client --config-only --name +`. Revoke at the CA too: the refresh from `CULVERT_SECRETS_CRL_PATH` ends +the session. WireGuard peers work as in local mode. + ## Cryptography and CNSA 2.0 The OpenVPN path ships the CNSA 2.0 *classical* suite by default, aligned diff --git a/scripts/generate-client.py b/scripts/generate-client.py index 796f05a..c0f2314 100644 --- a/scripts/generate-client.py +++ b/scripts/generate-client.py @@ -75,6 +75,7 @@ def __init__(self): vpn = VpnConfig.from_settings() self._vpn = vpn self.pki_dir = vpn.pki_dir + self.pki_mode = vpn.pki_mode self.output_dir = Path(os.environ.get("OUTPUT_DIR", "/etc/vpn/clients")) self.server_cn = vpn.server_cn self.client_endpoint = vpn.client_endpoint @@ -877,13 +878,27 @@ def main() -> None: # Handle certificate cert_path = cfg.pki_dir / "issued" / f"{client_name}.crt" + key_path = cfg.pki_dir / "private" / f"{client_name}.key" if args.config_only: - if not cert_path.exists(): + if not cert_path.exists() or not key_path.exists(): logger.error( - f"Config-only mode but certificate doesn't exist: {client_name}" + f"Config-only mode needs {cert_path} and {key_path}:" + f" {client_name} has no certificate and key to build configs from" ) sys.exit(1) logger.info("Config-only mode: using existing certificate") + # An externally issued pair arrives without one. + if not client_tc_key_path(cfg.pki_dir, client_name).exists(): + generate_tc_client_key(client_name, cfg.pki_dir) + elif cfg.pki_mode == "external": + logger.error( + "CULVERT_PKI_MODE=external: the CA key is not on this server, so" + f" generate-client cannot sign a certificate for {client_name}." + f" Issue it at your CA, place the certificate at {cert_path} and its" + f" key at {key_path}, then run generate-client --config-only" + f" --name {client_name}." + ) + sys.exit(1) else: if cert_path.exists(): logger.warning(f"Client certificate already exists: {client_name}") @@ -893,10 +908,9 @@ def main() -> None: ) sys.exit(1) # Easy-RSA in batch mode overwrites an existing key without asking. - stray_key = cfg.pki_dir / "private" / f"{client_name}.key" - if stray_key.exists(): + if key_path.exists(): logger.error( - f"{stray_key} exists with no certificate beside it. Refusing" + f"{key_path} exists with no certificate beside it. Refusing" " to overwrite key material: remove it, or choose another --name." ) sys.exit(1) diff --git a/scripts/revoke-client.py b/scripts/revoke-client.py index 632e3a7..e3df1bd 100644 --- a/scripts/revoke-client.py +++ b/scripts/revoke-client.py @@ -418,6 +418,23 @@ def main() -> None: ) sys.exit(1) + cert_path = PKI_DIR / "issued" / f"{args.client_name}.crt" + if ( + vpn_cfg.pki_mode == "external" + and args.protocol in ("openvpn", "all") + and cert_path.exists() + ): + # Refused before anything is touched: ending the session first would only + # let the client straight back in on a CRL that does not revoke it yet. + logger.error( + "CULVERT_PKI_MODE=external: the CA key is not on this server, so" + f" revoke-client cannot revoke {args.client_name}'s certificate. Revoke it" + " at your CA: the CRL refresh from CULVERT_SECRETS_CRL_PATH picks it up" + " and ends the session. Use --protocol wireguard to remove only the" + " WireGuard peer." + ) + sys.exit(1) + try: if args.protocol == "all": ovpn_revoked = revoke_client( diff --git a/tests/unit/test_client_lifecycle.py b/tests/unit/test_client_lifecycle.py index c76e780..ea17d6e 100644 --- a/tests/unit/test_client_lifecycle.py +++ b/tests/unit/test_client_lifecycle.py @@ -14,6 +14,8 @@ """ import importlib.util +import shutil +import subprocess import sys import zipfile from pathlib import Path @@ -297,3 +299,59 @@ def server_config_then_revoke(**kwargs): assert not errors, errors assert ALICE_KEY not in stack.wg_conf.read_text(encoding="utf-8") assert not (stack.pki / "wireguard" / "peers" / "alice.pub").exists() + + +@pytest.fixture +def external(stack, monkeypatch): + """The stack on external PKI: the CA key is held upstream, not here.""" + monkeypatch.setenv("CULVERT_PKI_MODE", "external") + monkeypatch.setenv("CULVERT_SECRETS_PROVIDER", "file") + for name in ("CA_CERT", "SERVER_CERT", "SERVER_KEY"): + monkeypatch.setenv(f"CULVERT_SECRETS_{name}_PATH", f"/mnt/pki/{name.lower()}") + return stack + + +class TestExternalPki: + """Easy-RSA cannot sign or revoke without the CA key, which external PKI lacks.""" + + def test_issuing_a_certificate_is_refused_with_the_way_through(self, external): + with pytest.raises(SystemExit) as exc: + external.generate("--protocol", "openvpn", "--name", "alice") + + assert exc.value.code == 1 + assert not (external.pki / "issued" / "alice.crt").exists() + assert not (external.pki / "private" / "alice.key").exists() + + @pytest.mark.skipif(shutil.which("openvpn") is None, reason="needs openvpn") + def test_an_externally_issued_certificate_gets_configs(self, external): + """The operator places the CA-signed pair, and generate-client does the rest.""" + subprocess.run( + [ + "openvpn", + "--genkey", + "tls-crypt-v2-server", + str(external.pki / "tc.key"), + ], + check=True, + capture_output=True, + ) + external.issue("alice") + client_tc_key_path(external.pki, "alice").unlink() + + external.generate("--protocol", "openvpn", "--config-only", "--name", "alice") + + config = (external.clients / "alice-udp-split.ovpn").read_text(encoding="utf-8") + assert "BEGIN OpenVPN tls-crypt-v2 client key" in config + + def test_revoking_a_certificate_is_refused_and_changes_nothing(self, external): + external.issue("alice") + external.generate("--protocol", "openvpn", "--config-only", "--name", "alice") + before = _volume(external) + + with pytest.raises(SystemExit) as exc: + external.revoke("alice") + + assert exc.value.code == 1 + assert (external.pki / "issued" / "alice.crt").exists() + assert (external.pki / "private" / "alice.key").exists() + assert _volume(external) == before From aef910895555de910778742ad091a7188baba7b7 Mon Sep 17 00:00:00 2001 From: Derek Date: Sun, 1 Nov 2026 19:17:20 +1100 Subject: [PATCH 13/24] fix: run logrotate on a schedule from the server process The entrypoint wrote /etc/logrotate.d/vpn and nothing ever ran it, because PID 1 is Python and the image has no cron, so the OpenVPN, stunnel, wstunnel and oauth2 logs grew without bound. The server now runs logrotate hourly from a daemon thread, with copytruncate because every writer holds an O_APPEND descriptor it never reopens, daily or past 100M, 7 kept. The status files are left out: OpenVPN rewrites them in place. --- .env.example | 1 + scripts/entrypoint.py | 3 +- scripts/lib/process.py | 67 +++++++++++++++++++++++++++++++------- tests/unit/test_process.py | 41 +++++++++++++++++++++++ 4 files changed, 100 insertions(+), 12 deletions(-) diff --git a/.env.example b/.env.example index 938ab3e..7e4ea36 100644 --- a/.env.example +++ b/.env.example @@ -219,6 +219,7 @@ CULVERT_SERVER_CN="vpn.example.com" # Logging configuration # CULVERT_LOG_MODE: "file" (default), "stdout" (K8s/containers), "both" # Kubernetes/EKS/Rancher: Use "stdout" for container logging pipelines +# Files under /var/log/vpn rotate daily or past 100M, and 7 are kept. #CULVERT_LOG_MODE="file" #CULVERT_VERB=3 #CULVERT_MUTE=20 diff --git a/scripts/entrypoint.py b/scripts/entrypoint.py index 7a52ccb..d8817d6 100644 --- a/scripts/entrypoint.py +++ b/scripts/entrypoint.py @@ -44,6 +44,7 @@ setup_directories, setup_log_rotation, setup_scripts, + start_log_rotation, ) from lib.stunnel import configure_stunnel from lib.wireguard import setup_wireguard, start_wg_connection_monitor, start_wireguard @@ -59,7 +60,7 @@ def run_server(cfg: Config) -> None: # Infrastructure setup setup_directories(cfg) - setup_log_rotation() + start_log_rotation(*setup_log_rotation(cfg)) setup_network(cfg) setup_routing_control(cfg) # LAST of the three: it inserts at FORWARD position 1, and it has to sit diff --git a/scripts/lib/process.py b/scripts/lib/process.py index 6b84164..c0eea27 100644 --- a/scripts/lib/process.py +++ b/scripts/lib/process.py @@ -131,26 +131,71 @@ def setup_directories(cfg) -> None: run_dir.chmod(0o700) -def setup_log_rotation() -> None: - """Configure logrotate for OpenVPN logs.""" - logger.info("Configuring log rotation...") +# logrotate rotates daily or past maxsize, so an hourly check holds the size cap. +LOG_ROTATION_INTERVAL_SECONDS = 3600 - logrotate_conf = Path("/etc/logrotate.d/vpn") - logrotate_conf.write_text( - """ -/var/log/vpn/*.log { + +def setup_log_rotation( + cfg, conf_path: Path = Path("/etc/logrotate.d/vpn") +) -> tuple[Path, Path]: + """Write the logrotate config for the server's logs, returning it and its state. + + copytruncate, because OpenVPN, stunnel and the daemon logs keep an O_APPEND + descriptor open and never reopen it. The status files are not logs: OpenVPN + rewrites them in place every few seconds, so they are not listed. + """ + log_dir = Path(cfg.log_dir) + logs = " ".join( + str(log_dir / name) + for name in ("openvpn*.log", "stunnel.log", "wstunnel.log", "oauth2-*.log") + ) + conf_path.write_text( + f"""{logs} {{ daily + maxsize 100M rotate 7 compress delaycompress + copytruncate notifempty missingok - create 0640 nobody nogroup -} -""".strip() +}} +""" ) + logger.info("Log rotation configured (daily or past 100M, 7 kept)") + return conf_path, log_dir / "logrotate.status" - logger.info("Log rotation configured (daily, 7 days retention)") + +def rotate_logs(conf_path: Path, state_path: Path, force: bool = False) -> bool: + """Run logrotate once over conf_path. True when it exited cleanly.""" + cmd = ["logrotate", "--state", str(state_path), str(conf_path)] + if force: + cmd.insert(1, "--force") + try: + result = run(cmd, check=False, capture=True) + except FileNotFoundError: + logger.error("logrotate is not installed, so the server logs grow unbounded") + return False + if result.returncode != 0: + logger.warning(f"logrotate failed: {result.stderr.strip()}") + return result.returncode == 0 + + +def start_log_rotation( + conf_path: Path, + state_path: Path, + interval: float = LOG_ROTATION_INTERVAL_SECONDS, +) -> None: + """Run logrotate on a schedule in a daemon thread, since no cron runs here.""" + import threading + import time + + def rotate_loop() -> None: + while True: + rotate_logs(conf_path, state_path) + time.sleep(interval) + + threading.Thread(target=rotate_loop, daemon=True, name="logrotate").start() def setup_scripts(cfg) -> None: diff --git a/tests/unit/test_process.py b/tests/unit/test_process.py index 67dac6d..58a2ef8 100644 --- a/tests/unit/test_process.py +++ b/tests/unit/test_process.py @@ -6,9 +6,11 @@ # License: Apache-2.0 # Copyright: (c) 2026 HYPERI PTY LIMITED +import shutil import signal import stat import subprocess +import time from types import SimpleNamespace import pytest @@ -145,6 +147,45 @@ class FakeCfg: setup_directories(FakeCfg()) # Second call should not raise +@pytest.mark.skipif(shutil.which("logrotate") is None, reason="needs logrotate") +class TestLogRotation: + """PID 1 is Python with no cron, so culvert runs logrotate itself.""" + + def test_rotates_while_the_writer_keeps_its_file_open(self, tmp_path): + """OpenVPN and the daemons hold an O_APPEND fd that is never reopened.""" + from lib.process import rotate_logs, setup_log_rotation + + cfg = SimpleNamespace(log_dir=tmp_path) + conf, state = setup_log_rotation(cfg, conf_path=tmp_path / "logrotate.conf") + log = tmp_path / "openvpn.log" + status = tmp_path / "status.log" + status.write_text("CLIENT_LIST\talice\n") + with log.open("ab") as writer: + writer.write(b"before rotation\n") + writer.flush() + assert rotate_logs(conf, state, force=True) + writer.write(b"after rotation\n") + + assert (tmp_path / "openvpn.log.1").read_bytes() == b"before rotation\n" + assert log.read_bytes() == b"after rotation\n" + assert status.read_text() == "CLIENT_LIST\talice\n" + assert not (tmp_path / "status.log.1").exists() + + def test_the_schedule_runs_logrotate(self, tmp_path): + from lib.process import setup_log_rotation, start_log_rotation + + cfg = SimpleNamespace(log_dir=tmp_path) + conf, state = setup_log_rotation(cfg, conf_path=tmp_path / "logrotate.conf") + (tmp_path / "openvpn.log").write_text("line\n") + + start_log_rotation(conf, state, interval=0.05) + + deadline = time.monotonic() + 10 + while not state.exists() and time.monotonic() < deadline: + time.sleep(0.05) + assert state.exists(), "logrotate never ran, so no state file was written" + + class TestSetupScripts: """Tests for script permission setup.""" From 7735b784aaf55d9147affbfa2dc1065b3651a918 Mon Sep 17 00:00:00 2001 From: Derek Date: Sun, 1 Nov 2026 19:23:30 +1100 Subject: [PATCH 14/24] fix: name WireGuard configs so wg-quick and Android accept them wg-quick and the Android app name the interface after the config file, and Linux allows 15 characters, so -wg-https-full.conf failed for every client name, the 15-character default vpn-example-com included. WireGuard configs now go in a directory named for the client, as wg-split.conf, wg-full.conf, wg-https-split.conf and wg-https-full.conf, with the slot number after "wg" from slot 2. generate-client refuses a slot count whose names would not fit (over HTTPS from slot 10). revoke-client, the zip bundle and the download server follow the directory, and the download server serves only a client name as a directory. --- README.md | 4 +- docs/use-case-travel.md | 6 +-- docs/vpn-client-setup.md | 35 +++++++------ scripts/generate-client.py | 30 ++++++++--- scripts/lib/clients.py | 21 ++++++-- scripts/lib/download.py | 30 +++++++---- scripts/lib/wireguard.py | 24 +++++++-- scripts/revoke-client.py | 4 +- tests/e2e/conftest.py | 32 ++++++++---- tests/e2e/test_https_tunnel.py | 2 +- tests/e2e/test_wireguard_connect.py | 2 +- tests/integration/test_wireguard_client.bats | 30 +++++------ tests/unit/test_client_download.py | 19 +++++-- tests/unit/test_client_lifecycle.py | 8 +-- tests/unit/test_clients.py | 54 +++++++++++--------- tests/unit/test_generate_client.py | 46 +++++++++++++---- tests/unit/test_revoke.py | 20 +++++--- 17 files changed, 239 insertions(+), 128 deletions(-) diff --git a/README.md b/README.md index 2fa4549..1dfaaaa 100644 --- a/README.md +++ b/README.md @@ -101,8 +101,8 @@ That mints a real credential, so treat the output accordingly: One identity covers Zaphod's devices concurrently, laptop and phone both connected. OpenVPN shares the certificate across sessions; WireGuard cannot -share a key, so alice gets one peer per device slot (`zaphod-wg-split.conf`, -`zaphod-wg2-split.conf`) - `CULVERT_SHARED_CLIENT_SLOTS` sets how many. Set +share a key, so alice gets one peer per device slot (`zaphod/wg-split.conf`, +`zaphod/wg2-split.conf`) - `CULVERT_SHARED_CLIENT_SLOTS` sets how many. Set `CULVERT_ALLOW_SHARED_CLIENTS=false` where a credential must identify a single device. diff --git a/docs/use-case-travel.md b/docs/use-case-travel.md index 591ebae..f9a2235 100644 --- a/docs/use-case-travel.md +++ b/docs/use-case-travel.md @@ -56,7 +56,7 @@ docker exec -it generate-client --name laptop For OpenVPN this emits, among the set, `laptop-https-split.ovpn`, `laptop-https-full.ovpn`, and a matching `laptop-stunnel.conf`. For WireGuard (the profile runs both protocols) it also emits -`laptop-wg-https-split.conf` and `laptop-wg-https-full.conf`. +`laptop/wg-https-split.conf` and `laptop/wg-https-full.conf`. ## 4. Connect through the tunnel @@ -73,12 +73,12 @@ sudo openvpn --config laptop-https-full.ovpn stunnel listens on `127.0.0.1:1195` and forwards to your server on `443`; the `.ovpn` connects to that local port. -**WireGuard** - start the wstunnel client, then activate the `-wg-https-` +**WireGuard** - start the wstunnel client, then activate a `wg-https-` config: ```bash wstunnel client wss://vpn.example.com:4443 -L udp://51820:127.0.0.1:51820 -# then activate laptop-wg-https-full.conf in your WireGuard client +# then activate laptop/wg-https-full.conf in your WireGuard client ``` Full platform detail (install steps, one-liners, wrapper scripts, the diff --git a/docs/vpn-client-setup.md b/docs/vpn-client-setup.md index 7f5948b..da8da87 100644 --- a/docs/vpn-client-setup.md +++ b/docs/vpn-client-setup.md @@ -44,18 +44,21 @@ These clients support the web-based OAuth2 authentication flow required for SSO. ### WireGuard +WireGuard configs come in a folder named for you, because the file name is the +name of the tunnel and Linux and Android allow it 15 characters. + | File | Protocol | Use Case | |------|----------|----------| -| `*-wg-split.conf` | UDP 51820 | **Recommended** - VPN routes only | -| `*-wg-full.conf` | UDP 51820 | All traffic through VPN | -| `*-wg-https-split.conf` | WSS 4443 (wstunnel) | over HTTPS, VPN routes only | -| `*-wg-https-full.conf` | WSS 4443 (wstunnel) | over HTTPS, all traffic | +| `wg-split.conf` | UDP 51820 | **Recommended** - VPN routes only | +| `wg-full.conf` | UDP 51820 | All traffic through VPN | +| `wg-https-split.conf` | WSS 4443 (wstunnel) | over HTTPS, VPN routes only | +| `wg-https-full.conf` | WSS 4443 (wstunnel) | over HTTPS, all traffic | ### Using more than one device The same `.ovpn` works on a laptop and a phone at once. WireGuard does not: a key connects from one place at a time, so you get a set of files per device - -`*-wg-split.conf` for the first, `*-wg2-split.conf` for the second. Load one set +`wg-split.conf` for the first, `wg2-split.conf` for the second. Load one set per device, or they will keep knocking each other offline. **Which to use:** @@ -85,33 +88,33 @@ These files are a snapshot of the server's settings when they were issued. Ask y ### Importing Configuration 1. Request your configuration files from the VPN administrator -2. You will receive `{name}-wg-split.conf` and `{name}-wg-full.conf` +2. You will receive `wg-split.conf` and `wg-full.conf`, in a folder named for you 3. Import into your WireGuard client (drag and drop, or "Import tunnel from file") 4. Activate the tunnel ### Split vs Full Tunnel -- **Split tunnel** (`*-wg-split.conf`): Only VPN traffic goes through the tunnel. Internet traffic uses your normal connection. -- **Full tunnel** (`*-wg-full.conf`): All traffic goes through the tunnel. Use when you need full network isolation. +- **Split tunnel** (`wg-split.conf`): Only VPN traffic goes through the tunnel. Internet traffic uses your normal connection. +- **Full tunnel** (`wg-full.conf`): All traffic goes through the tunnel. Use when you need full network isolation. ### Linux: Command Line ```bash # Import and bring up the tunnel -sudo wg-quick up ./alice-wg-split.conf +sudo wg-quick up ./wg-split.conf # Check status sudo wg show # Bring down the tunnel -sudo wg-quick down ./alice-wg-split.conf +sudo wg-quick down ./wg-split.conf ``` ### Linux: Always-On WireGuard (systemd) ```bash # Copy config to system directory -sudo cp alice-wg-split.conf /etc/wireguard/culvert.conf +sudo cp wg-split.conf /etc/wireguard/culvert.conf # Enable and start sudo systemctl enable wg-quick@culvert @@ -134,7 +137,7 @@ sudo systemctl status wg-quick@culvert ```bash brew install wireguard-tools -sudo wg-quick up ./alice-wg-split.conf +sudo wg-quick up ./wg-split.conf ``` ### Windows @@ -167,7 +170,7 @@ The `*-https-*.ovpn` configs use stunnel to wrap OpenVPN in real TLS on port 443 ### WireGuard over HTTPS (wstunnel) -For networks that block WireGuard, use the HTTPS-tunnel configs (`*-wg-https-*.conf`). These require wstunnel on your machine. +For networks that block WireGuard, use the HTTPS-tunnel configs (`wg-https-*.conf`). These require wstunnel on your machine. #### Installing wstunnel @@ -185,7 +188,7 @@ For networks that block WireGuard, use the HTTPS-tunnel configs (`*-wg-https-*.c (Replace `vpn.example.com` with your VPN server hostname) -2. Then import and activate the `*-wg-https-*.conf` config in your WireGuard client +2. Then import and activate the `wg-https-*.conf` config in your WireGuard client 3. The VPN traffic is tunnelled over WebSocket/TLS on port 4443, which looks like normal HTTPS to firewalls @@ -201,10 +204,10 @@ wstunnel client "wss://${VPN_SERVER}:${WS_PORT}" -L udp://51820:127.0.0.1:51820 WSTUNNEL_PID=$! sleep 2 -sudo wg-quick up ./your-config-wg-https-split.conf +sudo wg-quick up ./wg-https-split.conf # Cleanup on exit -trap 'sudo wg-quick down ./your-config-wg-https-split.conf; kill $WSTUNNEL_PID' EXIT +trap 'sudo wg-quick down ./wg-https-split.conf; kill $WSTUNNEL_PID' EXIT wait ``` diff --git a/scripts/generate-client.py b/scripts/generate-client.py index c0f2314..4602f2f 100644 --- a/scripts/generate-client.py +++ b/scripts/generate-client.py @@ -23,8 +23,8 @@ Outputs vary by --protocol flag: openvpn: 6 .ovpn files (3 listeners x 2 tunnel modes) - wireguard: 2-4 .conf files PER DEVICE SLOT (split/full, plus the - HTTPS-tunnelled pair when WireGuard-over-HTTPS is enabled) + wireguard: 2-4 .conf files PER DEVICE SLOT in / (split/full, plus + the HTTPS-tunnelled pair when WireGuard-over-HTTPS is enabled) all: both sets of files A client identity carries CULVERT_SHARED_CLIENT_SLOTS concurrent connections by @@ -606,6 +606,7 @@ def generate_wireguard_configs( # Write all config files for filename, content in configs_to_write: out_path = cfg.output_dir / filename + out_path.parent.mkdir(mode=0o700, exist_ok=True) write_secret(out_path, content) logger.info(f"Created WireGuard config: {out_path}") @@ -699,13 +700,14 @@ def main() -> None: {{name}}-https-split.ovpn - TCP {cfg.https_port} over TLS, split tunnel {{name}}-https-full.ovpn - TCP {cfg.https_port} over TLS, full tunnel -WireGuard output files, PER DEVICE SLOT (2-4 per slot): - {{name}}-wg-split.conf - Split tunnel - {{name}}-wg-full.conf - Full tunnel - {{name}}-wg-https-split.conf - Over HTTPS, split tunnel (if enabled) - {{name}}-wg-https-full.conf - Over HTTPS, full tunnel (if enabled) +WireGuard output files, PER DEVICE SLOT (2-4 per slot), in {{name}}/ because the +file name is the tunnel's interface name (15 characters at most): + {{name}}/wg-split.conf - Split tunnel + {{name}}/wg-full.conf - Full tunnel + {{name}}/wg-https-split.conf - Over HTTPS, split tunnel (if enabled) + {{name}}/wg-https-full.conf - Over HTTPS, full tunnel (if enabled) -Slots 2+ carry the slot number ({{name}}-wg2-split.conf, ...). Each slot is an +Slots 2+ carry the slot number ({{name}}/wg2-split.conf, ...). Each slot is an independent peer with its own key and tunnel IP, which is what lets one client identity hold {cfg.shared_client_slots} concurrent WireGuard connections. Set CULVERT_ALLOW_SHARED_CLIENTS=false for one exclusive connection per client. @@ -850,6 +852,18 @@ def main() -> None: ) sys.exit(1) + if generate_wg: + from lib import wireguard + + last_slot = len(args.pubkey) if args.pubkey else cfg.shared_client_slots + try: + wireguard.client_config_names( + client_name, last_slot, cfg.wg_https_tunnel_enabled + ) + except ValueError as exc: + logger.error(f"{exc}. Issue fewer device slots.") + sys.exit(1) + # An unset --days falls back to the configured cert lifetime, so a # deployment with a long CULVERT_CERT_EXPIRE_DAYS issues clients to match. cert_days = args.days if args.days is not None else cfg.cert_expire_days diff --git a/scripts/lib/clients.py b/scripts/lib/clients.py index 40ffb46..ec44881 100644 --- a/scripts/lib/clients.py +++ b/scripts/lib/clients.py @@ -8,12 +8,16 @@ """The files generate-client writes for one client in the clients directory. +OpenVPN and stunnel configs sit in the clients directory as "-". A client name may contain "-", so one name can prefix another: "alice" and "alice-laptop". A "-*" glob for alice also returns alice-laptop's configs, private keys included. Each file is matched instead on the exact suffix generate-client writes after "-", so one client's files are never bundled with, or deleted alongside, another client's. +WireGuard configs sit in a directory named for the client, because their file +name is the interface name wg-quick creates, which cannot also carry the client. + One filename is ambiguous by construction: "-proxy-stunnel.conf" is also the direct stunnel config of a client named "-proxy". generate-client refuses to issue such a name (collides_with_proxy_stunnel). A client issued @@ -39,7 +43,7 @@ ) # Slot 1 carries no number and later slots start at 2, as wireguard.slot_infix. -_WIREGUARD_SUFFIX = re.compile( +_WIREGUARD_CONFIG = re.compile( r"wg(?:[2-9]|[1-9][0-9]+)?(?:-https)?-(?:split|full)\.conf" ) @@ -72,18 +76,25 @@ def openvpn_files(output_dir: Path, client_name: str) -> list[Path]: def wireguard_files(output_dir: Path, client_name: str) -> list[Path]: - """Return the client's WireGuard configs in output_dir, for every slot. + """Return the client's WireGuard configs, for every slot. Args: output_dir: The directory generate-client writes client configs to. client_name: The client, already checked by validate_client_name. Returns: - The matching files, sorted. Empty when there are none or the - directory does not exist. + The matching files in the client's directory, sorted. Empty when there + are none or the directory does not exist. """ - return _matching(output_dir, client_name, _WIREGUARD_SUFFIX) + client_dir = output_dir / client_name + if not client_dir.is_dir(): + return [] + return sorted( + path + for path in client_dir.iterdir() + if _WIREGUARD_CONFIG.fullmatch(path.name) and path.is_file() + ) def client_files(output_dir: Path, client_name: str) -> list[Path]: diff --git a/scripts/lib/download.py b/scripts/lib/download.py index d2bea72..8151fcf 100644 --- a/scripts/lib/download.py +++ b/scripts/lib/download.py @@ -24,6 +24,7 @@ from scalo.logger import logger from lib.health import BaseHandler +from lib.pki import validate_client_name # What generate-client writes for a client: OpenVPN (.ovpn), WireGuard and # stunnel (.conf) and the per-client bundle (.zip). @@ -95,11 +96,20 @@ def _list_clients(self) -> None: ) return - config_files = sorted( + config_files = [ f for f in self.clients_dir.iterdir() if f.is_file() and f.suffix in SERVED_SUFFIXES - ) + ] + # WireGuard configs sit in a directory per client. + for client_dir in self.clients_dir.iterdir(): + if client_dir.is_dir() and validate_client_name(client_dir.name): + config_files.extend( + f + for f in client_dir.iterdir() + if f.is_file() and f.suffix == ".conf" + ) + config_files.sort() if not config_files: self.send_html(self._render_error("No client configs available")) @@ -108,17 +118,19 @@ def _list_clients(self) -> None: self.send_html(self._render_file_list(config_files)) def _download_client(self) -> None: - """Download a specific client config.""" - filename = self.path.split("/")[-1] + """Download a client config, or a WireGuard config in a client's directory.""" + filename = self.path.removeprefix("/clients/") + parts = filename.split("/") - # Security: prevent path traversal - if ".." in filename or "/" in filename: + # Security: prevent path traversal. Only a client name may be a directory. + nested_ok = len(parts) == 2 and validate_client_name(parts[0]) + if ".." in filename or not all(parts) or (len(parts) > 1 and not nested_ok): self.send_error(400, "Invalid filename") return - file_path = self.clients_dir / filename + file_path = self.clients_dir.joinpath(*parts) - if not file_path.exists(): + if not file_path.is_file(): self.send_error(404, "Client config not found") return @@ -137,7 +149,7 @@ def _render_file_list(self, files: list[Path]) -> str: stat = f.stat() size_kb = stat.st_size / 1024 mtime = datetime.fromtimestamp(stat.st_mtime).strftime("%Y-%m-%d %H:%M") - name = html.escape(f.name) + name = html.escape(f.relative_to(self.clients_dir).as_posix()) file_rows += f""" {name} diff --git a/scripts/lib/wireguard.py b/scripts/lib/wireguard.py index 094148c..e23e36f 100644 --- a/scripts/lib/wireguard.py +++ b/scripts/lib/wireguard.py @@ -16,6 +16,7 @@ import fcntl import ipaddress import json +import re import subprocess import sys from collections import deque @@ -89,16 +90,33 @@ def slot_infix(slot: int) -> str: return "" if slot == 1 else str(slot) +# wg-quick and the Android app name the interface after the config file, and +# Linux caps an interface name at 15 characters, so the client name is the +# directory rather than part of the file name. +WG_QUICK_NAME = re.compile(r"[a-zA-Z0-9_=+.-]{1,15}") + + def client_config_name( client_name: str, slot: int, mode: str, https: bool = False ) -> str: - """Filename of one slot's split or full config, plain or over HTTPS.""" + """Path, under the clients directory, of one slot's split or full config. + + Raises: + ValueError: The file name would not be a valid interface name, which + happens over HTTPS from slot 10. + """ tunnel = "-https" if https else "" - return f"{client_name}-wg{slot_infix(slot)}{tunnel}-{mode}.conf" + stem = f"wg{slot_infix(slot)}{tunnel}-{mode}" + if not WG_QUICK_NAME.fullmatch(stem): + raise ValueError( + f"WireGuard config {stem}.conf is too long for an interface name," + " which wg-quick and the Android app derive from it (15 characters)" + ) + return f"{client_name}/{stem}.conf" def client_config_names(client_name: str, slot: int, https: bool) -> list[str]: - """Filenames of every config one slot is issued.""" + """Paths of every config one slot is issued.""" variants = (False, True) if https else (False,) return [ client_config_name(client_name, slot, mode, https=tunnelled) diff --git a/scripts/revoke-client.py b/scripts/revoke-client.py index e3df1bd..d66ce7d 100644 --- a/scripts/revoke-client.py +++ b/scripts/revoke-client.py @@ -292,12 +292,14 @@ def revoke_wireguard_client(client_name: str) -> bool: for peer in peers: _revoke_wireguard_peer(peer, peers_dir) - # Exact filenames only: "-wg*" also matches a client named "-wg-x". removed = 0 for wg_conf in wireguard_files(OUTPUT_DIR, client_name): wg_conf.unlink() removed += 1 logger.info(f" Removed: {wg_conf}") + client_dir = OUTPUT_DIR / client_name + if client_dir.is_dir() and not any(client_dir.iterdir()): + client_dir.rmdir() _rebundle_zip(client_name) # Regenerate the server config without the removed peer, at the path the diff --git a/tests/e2e/conftest.py b/tests/e2e/conftest.py index 9d24cc0..6af1b16 100644 --- a/tests/e2e/conftest.py +++ b/tests/e2e/conftest.py @@ -214,16 +214,26 @@ def compose_stack(): f"{CLIENT_NAME}-tcp-split.ovpn", f"{CLIENT_NAME}-https-split.ovpn", f"{CLIENT_NAME}-stunnel.conf", - f"{CLIENT_NAME}-wg-split.conf", - f"{CLIENT_NAME}-wg-full.conf", - f"{CLIENT_NAME}-wg-https-split.conf", + f"{CLIENT_NAME}/wg-split.conf", + f"{CLIENT_NAME}/wg-full.conf", + f"{CLIENT_NAME}/wg-https-split.conf", # The second WireGuard device slot, asserted here so a generator that # stopped issuing slots fails rather than skipping the concurrency tests. - f"{CLIENT_NAME}-wg2-split.conf", - f"{CLIENT_NAME}-wg2-full.conf", + f"{CLIENT_NAME}/wg2-split.conf", + f"{CLIENT_NAME}/wg2-full.conf", ] result = subprocess.run( - ["docker", "exec", CLIENT_CONTAINER, "ls", "/etc/vpn/clients/"], + [ + "docker", + "exec", + CLIENT_CONTAINER, + "find", + "/etc/vpn/clients", + "-type", + "f", + "-printf", + "%P\n", + ], capture_output=True, text=True, encoding="utf-8", @@ -298,7 +308,7 @@ def wireguard_connection(): """Connect WireGuard, yield, then disconnect.""" from helpers import connect_wireguard, disconnect_wireguard - config_name = f"{CLIENT_NAME}-wg-split.conf" + config_name = f"{CLIENT_NAME}/wg-split.conf" connect_wireguard(config_name) try: yield @@ -311,7 +321,7 @@ def wireguard_full_connection(): """Connect WireGuard in FULL-tunnel mode, yield, then disconnect.""" from helpers import connect_wireguard, disconnect_wireguard - config_name = f"{CLIENT_NAME}-wg-full.conf" + config_name = f"{CLIENT_NAME}/wg-full.conf" connect_wireguard(config_name) try: yield @@ -347,8 +357,8 @@ def wireguard_slot_pair_connection(): """ from helpers import CLIENT_B_CONTAINER, connect_wireguard, disconnect_wireguard - slot_one = f"{CLIENT_NAME}-wg-split.conf" - slot_two = f"{CLIENT_NAME}-wg2-split.conf" + slot_one = f"{CLIENT_NAME}/wg-split.conf" + slot_two = f"{CLIENT_NAME}/wg2-split.conf" connect_wireguard(slot_one, container=CLIENT_CONTAINER) connect_wireguard(slot_two, container=CLIENT_B_CONTAINER) try: @@ -366,7 +376,7 @@ def wireguard_https_tunnel_connection(): disconnect_wireguard_https_tunnel, ) - config_name = f"{CLIENT_NAME}-wg-https-split.conf" + config_name = f"{CLIENT_NAME}/wg-https-split.conf" connect_wireguard_https_tunnel(config_name) try: yield diff --git a/tests/e2e/test_https_tunnel.py b/tests/e2e/test_https_tunnel.py index aa75081..f515056 100644 --- a/tests/e2e/test_https_tunnel.py +++ b/tests/e2e/test_https_tunnel.py @@ -158,7 +158,7 @@ def _require_wireguard(self): def test_connects_and_reaches_target_when_plain_blocked(self, plain_ports_blocked): """WireGuard over WebSocket/TLS with UDP 51820 blocked.""" - config_name = f"{CLIENT_NAME}-wg-https-split.conf" + config_name = f"{CLIENT_NAME}/wg-https-split.conf" connect_wireguard_https_tunnel(config_name) try: body = curl_target() diff --git a/tests/e2e/test_wireguard_connect.py b/tests/e2e/test_wireguard_connect.py index 75512e0..7566dca 100644 --- a/tests/e2e/test_wireguard_connect.py +++ b/tests/e2e/test_wireguard_connect.py @@ -91,7 +91,7 @@ def _require_https_tunnel_config(self): """Skip if HTTPS-tunnel client config was not generated.""" result = docker_exec( CLIENT_CONTAINER, - f"test -f /etc/vpn/clients/{CLIENT_NAME}-wg-https-split.conf", + f"test -f /etc/vpn/clients/{CLIENT_NAME}/wg-https-split.conf", check=False, ) if result.returncode != 0: diff --git a/tests/integration/test_wireguard_client.bats b/tests/integration/test_wireguard_client.bats index 16d463a..704d225 100644 --- a/tests/integration/test_wireguard_client.bats +++ b/tests/integration/test_wireguard_client.bats @@ -25,10 +25,10 @@ teardown() { assert_success # Check WireGuard configs were created - run docker exec "${TEST_CONTAINER_NAME}" ls /etc/vpn/clients/ + run docker exec "${TEST_CONTAINER_NAME}" ls /etc/vpn/clients/wgonly/ assert_success - [[ "${output}" == *"-wg-split.conf"* ]] - [[ "${output}" == *"-wg-full.conf"* ]] + [[ "${output}" == *"wg-split.conf"* ]] + [[ "${output}" == *"wg-full.conf"* ]] } @test "generate-client creates both OpenVPN and WireGuard configs in both mode" { @@ -41,7 +41,8 @@ teardown() { assert_success # Should have both .ovpn and .conf files [[ "${output}" == *".ovpn"* ]] - [[ "${output}" == *"-wg-split.conf"* ]] + run docker exec "${TEST_CONTAINER_NAME}" test -f /etc/vpn/clients/wgboth/wg-split.conf + assert_success } @test "WireGuard client config contains correct endpoint" { @@ -50,10 +51,7 @@ teardown() { run docker exec "${TEST_CONTAINER_NAME}" generate-client --name wgendpoint --protocol wireguard assert_success - # Find the generated config and check endpoint - # shellcheck disable=SC2012 - config_file=$(docker exec "${TEST_CONTAINER_NAME}" ls /etc/vpn/clients/ | grep 'wg-split.conf' | head -1) - run docker exec "${TEST_CONTAINER_NAME}" grep "Endpoint" "/etc/vpn/clients/${config_file}" + run docker exec "${TEST_CONTAINER_NAME}" grep "Endpoint" /etc/vpn/clients/wgendpoint/wg-split.conf assert_success [[ "${output}" == *"vpn.test.io:51820"* ]] } @@ -64,17 +62,17 @@ teardown() { run docker exec "${TEST_CONTAINER_NAME}" generate-client --name wgslots --protocol wireguard assert_success - run docker exec "${TEST_CONTAINER_NAME}" ls /etc/vpn/clients/ + run docker exec "${TEST_CONTAINER_NAME}" ls /etc/vpn/clients/wgslots/ assert_success - [[ "${output}" == *"wgslots-wg-split.conf"* ]] - [[ "${output}" == *"wgslots-wg2-split.conf"* ]] + [[ "${output}" == *"wg-split.conf"* ]] + [[ "${output}" == *"wg2-split.conf"* ]] # Distinct keys and addresses are what make the slots concurrently usable. - run docker exec "${TEST_CONTAINER_NAME}" grep -h "^PrivateKey" /etc/vpn/clients/wgslots-wg-split.conf /etc/vpn/clients/wgslots-wg2-split.conf + run docker exec "${TEST_CONTAINER_NAME}" grep -h "^PrivateKey" /etc/vpn/clients/wgslots/wg-split.conf /etc/vpn/clients/wgslots/wg2-split.conf assert_success [[ "$(echo "${output}" | sort -u | wc -l)" -eq 2 ]] - run docker exec "${TEST_CONTAINER_NAME}" grep -h "^Address" /etc/vpn/clients/wgslots-wg-split.conf /etc/vpn/clients/wgslots-wg2-split.conf + run docker exec "${TEST_CONTAINER_NAME}" grep -h "^Address" /etc/vpn/clients/wgslots/wg-split.conf /etc/vpn/clients/wgslots/wg2-split.conf assert_success [[ "$(echo "${output}" | sort -u | wc -l)" -eq 2 ]] } @@ -86,10 +84,10 @@ teardown() { run docker exec "${TEST_CONTAINER_NAME}" generate-client --name wgexclusive --protocol wireguard assert_success - run docker exec "${TEST_CONTAINER_NAME}" ls /etc/vpn/clients/ + run docker exec "${TEST_CONTAINER_NAME}" ls /etc/vpn/clients/wgexclusive/ assert_success - [[ "${output}" == *"wgexclusive-wg-split.conf"* ]] - [[ "${output}" != *"wgexclusive-wg2-split.conf"* ]] + [[ "${output}" == *"wg-split.conf"* ]] + [[ "${output}" != *"wg2-split.conf"* ]] run docker exec "${TEST_CONTAINER_NAME}" grep -c '^duplicate-cn' /etc/vpn/server/server.conf assert_failure diff --git a/tests/unit/test_client_download.py b/tests/unit/test_client_download.py index a3d0197..745b52c 100644 --- a/tests/unit/test_client_download.py +++ b/tests/unit/test_client_download.py @@ -134,9 +134,13 @@ def clients_dir(self, temp_dir): (clients / "user2-udp.ovpn").write_text( "client\nremote vpn.example.com 1194 udp\n" ) - (clients / "user1-wg-split.conf").write_text( + (clients / "user1").mkdir() + (clients / "user1" / "wg-split.conf").write_text( "[Interface]\nAddress = 10.9.0.2\n" ) + # Not a client name, so nothing under it is served. + (clients / "not.a.client").mkdir() + (clients / "not.a.client" / "wg-split.conf").write_text("[Interface]\n") (clients / "user1.zip").write_bytes(b"PK\x03\x04zip") (clients / "vpn-client-setup.md").write_text("# not a client config\n") (clients / 'x">.conf').write_text("[Interface]\n") @@ -208,9 +212,10 @@ def test_listing_includes_wireguard_configs_and_the_zip_but_not_other_files( body = conn.getresponse().read().decode() conn.close() - assert "user1-wg-split.conf" in body + assert "user1/wg-split.conf" in body assert "user1.zip" in body assert "vpn-client-setup.md" not in body + assert "not.a.client" not in body def test_listing_escapes_file_names(self, test_server): conn = http.client.HTTPConnection("127.0.0.1", test_server) @@ -224,10 +229,13 @@ def test_listing_escapes_file_names(self, test_server): @pytest.mark.parametrize( ("name", "status", "content"), [ - ("user1-wg-split.conf", 200, b"[Interface]\nAddress = 10.9.0.2\n"), + ("user1/wg-split.conf", 200, b"[Interface]\nAddress = 10.9.0.2\n"), ("user1.zip", 200, b"PK\x03\x04zip"), ("vpn-client-setup.md", 404, None), - ("missing-wg-full.conf", 404, None), + ("missing/wg-full.conf", 404, None), + ("not.a.client/wg-split.conf", 400, None), + ("user1/extra/wg-split.conf", 400, None), + ("user1//wg-split.conf", 400, None), ], ) def test_download_wireguard_conf_and_zip(self, test_server, name, status, content): @@ -240,7 +248,8 @@ def test_download_wireguard_conf_and_zip(self, test_server, name, status, conten assert response.status == status if content is not None: assert body == content - assert name in (response.getheader("Content-Disposition") or "") + disposition = response.getheader("Content-Disposition") or "" + assert name.rsplit("/", 1)[-1] in disposition def test_download_nonexistent_file(self, test_server): """GET /clients/nonexistent.ovpn returns 404.""" diff --git a/tests/unit/test_client_lifecycle.py b/tests/unit/test_client_lifecycle.py index ea17d6e..2fc8fb8 100644 --- a/tests/unit/test_client_lifecycle.py +++ b/tests/unit/test_client_lifecycle.py @@ -141,11 +141,11 @@ def test_wireguard_revocation_rebuilds_the_zip_without_its_configs(self, stack): stack.revoke("--protocol", "wireguard", "alice") - assert not any(name.startswith("alice-wg") for name in _volume(stack)) + assert "alice" not in _volume(stack) with zipfile.ZipFile(stack.clients / "alice.zip") as zf: names = zf.namelist() assert "alice-udp-split.ovpn" in names - assert not any(name.startswith("alice-wg") for name in names) + assert not any(name.startswith("wg") for name in names) def test_openvpn_revocation_rebuilds_the_zip_without_its_configs(self, stack): _issue_both_protocols(stack, "alice", ALICE_KEY) @@ -156,8 +156,8 @@ def test_openvpn_revocation_rebuilds_the_zip_without_its_configs(self, stack): assert not any(name.endswith((".ovpn", "stunnel.conf")) for name in remaining) with zipfile.ZipFile(stack.clients / "alice.zip") as zf: assert sorted(zf.namelist()) == [ - "alice-wg-full.conf", - "alice-wg-split.conf", + "wg-full.conf", + "wg-split.conf", ] diff --git a/tests/unit/test_clients.py b/tests/unit/test_clients.py index e507fe2..4da7ad7 100644 --- a/tests/unit/test_clients.py +++ b/tests/unit/test_clients.py @@ -9,8 +9,9 @@ """Which files in the clients directory belong to which client. A client name may contain "-", so "alice" prefixes "alice-laptop", "alice-udp" -and "alice-wg2". Each of those clients must keep its own files, because the -OpenVPN and WireGuard configs embed that client's private keys. +and "alice-https". Each of those clients must keep its own files, because the +OpenVPN and WireGuard configs embed that client's private keys. WireGuard configs +sit in a directory named for the client. """ import stat @@ -33,7 +34,7 @@ for mode in ("split", "full") ] + ["stunnel.conf", "proxy-stunnel.conf"] -WIREGUARD_SUFFIXES = [ +WIREGUARD_NAMES = [ f"wg{slot}{https}-{mode}.conf" for slot in ("", "2", "10") for https in ("", "-https") @@ -42,15 +43,17 @@ def _issue(directory: Path, client: str) -> list[str]: - """Write every file generate-client can produce for client; return the names.""" - names = [f"{client}-{suffix}" for suffix in OPENVPN_SUFFIXES + WIREGUARD_SUFFIXES] + """Write every file generate-client can produce for client; return the paths.""" + names = [f"{client}-{suffix}" for suffix in OPENVPN_SUFFIXES] + names += [f"{client}/{name}" for name in WIREGUARD_NAMES] + (directory / client).mkdir(exist_ok=True) for name in names: (directory / name).write_text(f"{client} key", encoding="utf-8") return sorted(names) -def _names(paths: list[Path]) -> list[str]: - return [path.name for path in paths] +def _names(paths: list[Path], root: Path) -> list[str]: + return sorted(path.relative_to(root).as_posix() for path in paths) class TestOwnFiles: @@ -58,17 +61,17 @@ class TestOwnFiles: def test_every_generated_file_is_found(self, tmp_path): expected = _issue(tmp_path, "alice") - assert _names(client_files(tmp_path, "alice")) == expected + assert _names(client_files(tmp_path, "alice"), tmp_path) == expected def test_protocols_split_by_file(self, tmp_path): _issue(tmp_path, "alice") assert all( name.endswith((".ovpn", "stunnel.conf")) - for name in _names(openvpn_files(tmp_path, "alice")) + for name in _names(openvpn_files(tmp_path, "alice"), tmp_path) ) assert all( - name.startswith("alice-wg") - for name in _names(wireguard_files(tmp_path, "alice")) + name.startswith("alice/wg") + for name in _names(wireguard_files(tmp_path, "alice"), tmp_path) ) @pytest.mark.parametrize( @@ -77,12 +80,15 @@ def test_protocols_split_by_file(self, tmp_path): "alice.zip", "vpn-client-setup.md", "alice-notes.txt", - "alice-wg1-split.conf", - "alice-wg0-split.conf", + "alice-wg-split.conf", + "alice/wg1-split.conf", + "alice/wg0-split.conf", + "alice/notes.txt", "alice-udp-split.ovpn.bak", ], ) def test_files_generate_client_does_not_write_are_ignored(self, tmp_path, name): + (tmp_path / "alice").mkdir() (tmp_path / name).write_text("x", encoding="utf-8") assert client_files(tmp_path, "alice") == [] @@ -101,16 +107,14 @@ class TestPrefixedNames: "alice-https", "alice-stunnel", "alice-wg", - "alice-wg2", - "alice-wg-phone", ], ) def test_files_stay_with_their_own_client(self, tmp_path, other): alice = _issue(tmp_path, "alice") theirs = _issue(tmp_path, other) - assert _names(client_files(tmp_path, "alice")) == alice - assert _names(client_files(tmp_path, other)) == theirs + assert _names(client_files(tmp_path, "alice"), tmp_path) == alice + assert _names(client_files(tmp_path, other), tmp_path) == theirs def test_proxy_stunnel_name_is_the_only_shared_one(self, tmp_path): """Both clients write alice-proxy-stunnel.conf. @@ -122,8 +126,8 @@ def test_proxy_stunnel_name_is_the_only_shared_one(self, tmp_path): alice = set(_issue(tmp_path, "alice")) theirs = set(_issue(tmp_path, "alice-proxy")) - assert set(_names(client_files(tmp_path, "alice"))) == alice - assert set(_names(client_files(tmp_path, "alice-proxy"))) == theirs + assert set(_names(client_files(tmp_path, "alice"), tmp_path)) == alice + assert set(_names(client_files(tmp_path, "alice-proxy"), tmp_path)) == theirs assert alice & theirs == {"alice-proxy-stunnel.conf"} @@ -139,7 +143,8 @@ def test_unshared_files_all_belong_to_the_client(self, tmp_path): def test_proxy_stunnel_stays_while_alice_proxy_has_configs(self, tmp_path): _issue(tmp_path, "alice") _issue(tmp_path, "alice-proxy") - owned = _names(owned_files(tmp_path, "alice", client_files(tmp_path, "alice"))) + files = client_files(tmp_path, "alice") + owned = _names(owned_files(tmp_path, "alice", files), tmp_path) assert "alice-proxy-stunnel.conf" not in owned assert "alice-proxy-udp-split.ovpn" not in owned @@ -147,7 +152,7 @@ def test_direct_stunnel_stays_while_alice_has_configs(self, tmp_path): _issue(tmp_path, "alice") _issue(tmp_path, "alice-proxy") files = client_files(tmp_path, "alice-proxy") - owned = _names(owned_files(tmp_path, "alice-proxy", files)) + owned = _names(owned_files(tmp_path, "alice-proxy", files), tmp_path) assert "alice-proxy-stunnel.conf" not in owned assert "alice-proxy-udp-split.ovpn" in owned @@ -155,7 +160,7 @@ def test_shared_name_is_owned_once_the_other_client_is_gone(self, tmp_path): _issue(tmp_path, "alice") files = client_files(tmp_path, "alice") assert "alice-proxy-stunnel.conf" in _names( - owned_files(tmp_path, "alice", files) + owned_files(tmp_path, "alice", files), tmp_path ) @@ -164,7 +169,8 @@ class TestBundleZip: def test_bundles_only_this_clients_files(self, tmp_path): (tmp_path / "alice-udp-split.ovpn").write_text("a1", encoding="utf-8") - (tmp_path / "alice-wg-split.conf").write_text("a2", encoding="utf-8") + (tmp_path / "alice").mkdir() + (tmp_path / "alice" / "wg-split.conf").write_text("a2", encoding="utf-8") (tmp_path / "alice-laptop-udp-split.ovpn").write_text("b1", encoding="utf-8") zip_path = bundle_zip(tmp_path, "alice", client_files(tmp_path, "alice")) @@ -172,7 +178,7 @@ def test_bundles_only_this_clients_files(self, tmp_path): assert zip_path is not None assert zip_path == tmp_path / "alice.zip" with zipfile.ZipFile(zip_path) as zf: - assert set(zf.namelist()) == {"alice-udp-split.ovpn", "alice-wg-split.conf"} + assert set(zf.namelist()) == {"alice-udp-split.ovpn", "wg-split.conf"} assert stat.S_IMODE(zip_path.stat().st_mode) == 0o600 def test_an_existing_world_readable_zip_is_made_private(self, tmp_path): diff --git a/tests/unit/test_generate_client.py b/tests/unit/test_generate_client.py index af935aa..1dfcaea 100644 --- a/tests/unit/test_generate_client.py +++ b/tests/unit/test_generate_client.py @@ -15,6 +15,7 @@ import importlib.util import os +import re import stat import sys import zipfile @@ -475,17 +476,17 @@ class TestWireGuardClientSideKeys: def test_issues_split_and_full_per_slot(self, run): out = run(*TWO_SLOTS) - assert sorted(p.name for p in out.glob("*.conf")) == [ - f"{CLIENT}-wg-full.conf", - f"{CLIENT}-wg-split.conf", - f"{CLIENT}-wg2-full.conf", - f"{CLIENT}-wg2-split.conf", + assert sorted(p.name for p in (out / CLIENT).glob("*.conf")) == [ + "wg-full.conf", + "wg-split.conf", + "wg2-full.conf", + "wg2-split.conf", ] def test_configs_carry_no_private_key_and_distinct_addresses(self, run): out = run(*TWO_SLOTS) - first = (out / f"{CLIENT}-wg-split.conf").read_text(encoding="utf-8") - second = (out / f"{CLIENT}-wg2-split.conf").read_text(encoding="utf-8") + first = (out / CLIENT / "wg-split.conf").read_text(encoding="utf-8") + second = (out / CLIENT / "wg2-split.conf").read_text(encoding="utf-8") assert "PrivateKey = YOUR_PRIVATE_KEY_HERE" in first assert f"PublicKey = {SERVER_PUBLIC}" in first @@ -495,8 +496,8 @@ def test_configs_carry_no_private_key_and_distinct_addresses(self, run): def test_split_routes_only_the_tunnel_network(self, run): out = run("--protocol", "wireguard", "--name", CLIENT, "--pubkey", PUBKEY_1) - split = (out / f"{CLIENT}-wg-split.conf").read_text(encoding="utf-8") - full = (out / f"{CLIENT}-wg-full.conf").read_text(encoding="utf-8") + split = (out / CLIENT / "wg-split.conf").read_text(encoding="utf-8") + full = (out / CLIENT / "wg-full.conf").read_text(encoding="utf-8") assert "AllowedIPs = 10.8.3.0/24" in split assert "AllowedIPs = 0.0.0.0/0, ::/0" in full @@ -509,10 +510,33 @@ def test_server_config_gains_a_peer_per_slot(self, run): assert f"PublicKey = {PUBKEY_2}" in server assert stat.S_IMODE(run.wg_conf.stat().st_mode) == 0o600 + def test_every_config_is_named_as_wg_quick_needs(self, run, monkeypatch): + """wg-quick and the Android app take the interface name from the file name. + + Linux allows 15 characters, so "-wg-https-split" fits no client + name at all, the 15-character default vpn-example-com included. + """ + monkeypatch.setenv("CULVERT_WG_HTTPS_TUNNEL_ENABLED", "true") + name = "vpn-example-com" + out = run( + "--protocol", + "wireguard", + "--name", + name, + "--pubkey", + PUBKEY_1, + "--pubkey", + PUBKEY_2, + ) + configs = sorted((out / name).glob("*.conf")) + assert len(configs) == 8 + for path in configs: + assert re.fullmatch(r"[a-zA-Z0-9_=+.-]{1,15}", path.stem), path.name + def test_https_tunnel_variants_when_enabled(self, run, monkeypatch): monkeypatch.setenv("CULVERT_WG_HTTPS_TUNNEL_ENABLED", "true") out = run("--protocol", "wireguard", "--name", CLIENT, "--pubkey", PUBKEY_1) - tunnelled = (out / f"{CLIENT}-wg-https-split.conf").read_text(encoding="utf-8") + tunnelled = (out / CLIENT / "wg-https-split.conf").read_text(encoding="utf-8") assert "Endpoint = 127.0.0.1:51820" in tunnelled assert f"wss://{SERVER_CN}:4443" in tunnelled @@ -556,7 +580,7 @@ def wg_tools(tmp_path, monkeypatch): def _wireguard_state(pki, out: Path, wg_conf: Path) -> dict[str, bytes]: """Every file an issue can change, by path.""" - paths = [*(pki.dir / "wireguard").rglob("*"), *out.glob("*"), wg_conf] + paths = [*(pki.dir / "wireguard").rglob("*"), *out.rglob("*"), wg_conf] return {str(p): p.read_bytes() for p in paths if p.is_file()} diff --git a/tests/unit/test_revoke.py b/tests/unit/test_revoke.py index 4583c53..34f37af 100644 --- a/tests/unit/test_revoke.py +++ b/tests/unit/test_revoke.py @@ -319,14 +319,17 @@ def test_every_slot_and_its_files_go_and_other_clients_stay(self, cli, revoke): json.dumps({"alice": "10.8.3.2", "alice.2": "10.8.3.3", "bob": "10.8.3.4"}) ) (cli.pki / "wireguard" / "server_private.key").write_text("ServerPriv=\n") - for name in ("alice-wg-split.conf", "alice-wg2-full.conf", "bob-wg-split.conf"): + for name in ("alice/wg-split.conf", "alice/wg2-full.conf", "bob/wg-split.conf"): + (cli.clients / name).parent.mkdir(exist_ok=True) (cli.clients / name).write_text("config", encoding="utf-8") cli("--protocol", "wireguard", "alice") assert sorted(p.name for p in peers.iterdir()) == ["bob.key", "bob.pub"] assert json.loads(allocations.read_text()) == {"bob": "10.8.3.4"} - assert [p.name for p in cli.clients.iterdir()] == ["bob-wg-split.conf"] + assert sorted( + p.relative_to(cli.clients).as_posix() for p in cli.clients.rglob("*") + ) == ["bob", "bob/wg-split.conf"] server = cli.server_conf.read_text(encoding="utf-8") assert "PublicKey = BobKey=" in server assert "AliceKey" not in server @@ -346,17 +349,18 @@ def test_post_up_and_post_down_survive_revocation(self, cli, revoke, monkeypatch assert "PostUp = iptables -A FORWARD -i wg0 -j ACCEPT\n" in server assert "PostDown = iptables -D FORWARD -i wg0 -j ACCEPT\n" in server - def test_a_client_whose_name_starts_with_wg_keeps_its_configs(self, cli, revoke): - """A "-wg*" glob also matches the client "alice-wg-phone".""" + def test_a_client_whose_name_extends_alice_keeps_its_configs(self, cli, revoke): + """Each client's WireGuard configs are its own directory, whatever the name.""" _add_peer(revoke) - for name in ("alice-wg-split.conf", "alice-wg-phone-wg-split.conf"): + for name in ("alice/wg-split.conf", "alice-wg-phone/wg-split.conf"): + (cli.clients / name).parent.mkdir() (cli.clients / name).write_text("config", encoding="utf-8") cli("--protocol", "wireguard", "alice") - assert [p.name for p in cli.clients.iterdir()] == [ - "alice-wg-phone-wg-split.conf" - ] + assert sorted( + p.relative_to(cli.clients).as_posix() for p in cli.clients.rglob("*") + ) == ["alice-wg-phone", "alice-wg-phone/wg-split.conf"] def test_revocation_without_a_server_key_still_removes_the_peer(self, cli, revoke): """No server key means no config to rebuild; the peer still goes.""" From 227a26ff97295f82e27318f4be58d335ecd36e2f Mon Sep 17 00:00:00 2001 From: Derek Date: Sun, 1 Nov 2026 19:25:50 +1100 Subject: [PATCH 15/24] fix: keep wstunnel's connection out of a WireGuard full tunnel over HTTPS A full tunnel puts the default route in wg-quick's own table, so the wstunnel connection to the server that carries wg0 was itself routed into wg0 and the tunnel never came up. The full-tunnel HTTPS config's wstunnel command now carries --socket-so-mark 51820, wg-quick's fwmark, which its policy rule sends out the normal route, and the config says what to do on platforms without SO_MARK. The e2e helper now runs the command the config documents, and a new e2e case brings up the full tunnel over HTTPS with the plain ports blocked. --- docs/use-case-travel.md | 3 ++- docs/vpn-client-setup.md | 8 +++++++- scripts/lib/wireguard.py | 20 +++++++++++++++++++- tests/e2e/conftest.py | 17 +++++++++++++++++ tests/e2e/helpers.py | 18 ++++++++++-------- tests/e2e/test_https_tunnel.py | 14 ++++++++++++++ tests/unit/test_wg_config_gen.py | 30 ++++++++++++++++++++++++++++++ 7 files changed, 99 insertions(+), 11 deletions(-) diff --git a/docs/use-case-travel.md b/docs/use-case-travel.md index f9a2235..e62a364 100644 --- a/docs/use-case-travel.md +++ b/docs/use-case-travel.md @@ -77,7 +77,8 @@ stunnel listens on `127.0.0.1:1195` and forwards to your server on config: ```bash -wstunnel client wss://vpn.example.com:4443 -L udp://51820:127.0.0.1:51820 +# --socket-so-mark keeps wstunnel's own connection out of the full tunnel (Linux, root) +sudo wstunnel client wss://vpn.example.com:4443 -L udp://51820:127.0.0.1:51820 --socket-so-mark 51820 # then activate laptop/wg-https-full.conf in your WireGuard client ``` diff --git a/docs/vpn-client-setup.md b/docs/vpn-client-setup.md index da8da87..e1357ae 100644 --- a/docs/vpn-client-setup.md +++ b/docs/vpn-client-setup.md @@ -186,7 +186,13 @@ For networks that block WireGuard, use the HTTPS-tunnel configs (`wg-https-*.con wstunnel client wss://vpn.example.com:4443 -L udp://51820:127.0.0.1:51820 ``` - (Replace `vpn.example.com` with your VPN server hostname) + (Replace `vpn.example.com` with your VPN server hostname. The first lines of + each `wg-https-*.conf` carry the exact command for that config.) + + A full tunnel would route wstunnel's own connection into the tunnel it + carries. On Linux, start wstunnel with `sudo` and add `--socket-so-mark 51820`, + which wg-quick sends outside the tunnel. Elsewhere, route the server's address + through your normal gateway before you activate `wg-https-full.conf`. 2. Then import and activate the `wg-https-*.conf` config in your WireGuard client diff --git a/scripts/lib/wireguard.py b/scripts/lib/wireguard.py index e23e36f..e12c684 100644 --- a/scripts/lib/wireguard.py +++ b/scripts/lib/wireguard.py @@ -466,6 +466,11 @@ def generate_client_config( return "\n".join(lines) +# wg-quick's fwmark for a full tunnel, which it routes outside wg0. It takes the +# next free table number when 51820 is in use, which a client host rarely has. +WG_QUICK_FWMARK = 51820 + + def generate_https_tunnel_client_config( client_private_key: str | None, client_ip: str, @@ -496,6 +501,12 @@ def generate_https_tunnel_client_config( if tls_server_name and tls_server_name != server_endpoint: tls_flags += f" --tls-sni-override {tls_server_name}" + # A full tunnel would carry wstunnel's own connection to the server inside + # the tunnel it carries, so the full variant marks it to bypass wg0. + full_tunnel = "0.0.0.0/0" in allowed_ips + if full_tunnel: + tls_flags += f" --socket-so-mark {WG_QUICK_FWMARK}" + header = [ "# WireGuard over HTTPS (wstunnel required)", "#", @@ -508,8 +519,15 @@ def generate_https_tunnel_client_config( "# The WireGuard Endpoint below connects to the local wstunnel listener.", "# wstunnel carries the UDP tunnel inside WebSocket/TLS, so it travels", "# as ordinary HTTPS on a web port.", - "", ] + if full_tunnel: + header += [ + "#", + "# --socket-so-mark keeps wstunnel's connection out of this tunnel. It", + "# is Linux only and needs root. Elsewhere, drop it and route", + f"# {server_endpoint} through your normal gateway instead.", + ] + header.append("") body = [ "[Interface]", diff --git a/tests/e2e/conftest.py b/tests/e2e/conftest.py index 6af1b16..3530f70 100644 --- a/tests/e2e/conftest.py +++ b/tests/e2e/conftest.py @@ -217,6 +217,7 @@ def compose_stack(): f"{CLIENT_NAME}/wg-split.conf", f"{CLIENT_NAME}/wg-full.conf", f"{CLIENT_NAME}/wg-https-split.conf", + f"{CLIENT_NAME}/wg-https-full.conf", # The second WireGuard device slot, asserted here so a generator that # stopped issuing slots fails rather than skipping the concurrency tests. f"{CLIENT_NAME}/wg2-split.conf", @@ -382,3 +383,19 @@ def wireguard_https_tunnel_connection(): yield finally: disconnect_wireguard_https_tunnel(config_name) + + +@pytest.fixture +def wireguard_https_full_connection(): + """Connect WireGuard over HTTPS in FULL-tunnel mode, yield, then disconnect.""" + from helpers import ( + connect_wireguard_https_tunnel, + disconnect_wireguard_https_tunnel, + ) + + config_name = f"{CLIENT_NAME}/wg-https-full.conf" + connect_wireguard_https_tunnel(config_name) + try: + yield + finally: + disconnect_wireguard_https_tunnel(config_name) diff --git a/tests/e2e/helpers.py b/tests/e2e/helpers.py index 54ac716..87200a5 100644 --- a/tests/e2e/helpers.py +++ b/tests/e2e/helpers.py @@ -239,16 +239,18 @@ def connect_wireguard_https_tunnel(config_name: str) -> None: f"mkdir -p /etc/wireguard && sed '/^DNS/d' {src} > /etc/wireguard/wg0.conf", ) - # Start wstunnel client (tunnels local UDP 51820 to server wstunnel on 4443) - # --tls-verify-certificate=false because server uses self-signed cert - # Use setsid to properly daemonise inside docker exec - # wstunnel 10.x: --tls-verify-certificate is opt-in; omit it to skip verification + # Run the wstunnel command the config itself documents, so a full tunnel + # proves that command keeps wstunnel's connection out of wg0. Certificate + # verification is dropped because the e2e server's certificate is + # self-signed. setsid daemonises it inside docker exec. + documented = docker_exec( + CLIENT_CONTAINER, f"sed -n 's/^# \\(wstunnel client .*\\)$/\\1/p' {src}" + ).stdout.strip() + assert documented, f"{config_name} documents no wstunnel command" + command = documented.replace(" --tls-verify-certificate", "") docker_exec( CLIENT_CONTAINER, - "setsid wstunnel client" - " -L udp://127.0.0.1:51820:127.0.0.1:51820" - " wss://172.30.1.10:4443" - " /dev/null 2>&1 &", + f"setsid {command} /tmp/wstunnel.log 2>&1 &", ) time.sleep(2) diff --git a/tests/e2e/test_https_tunnel.py b/tests/e2e/test_https_tunnel.py index f515056..2076903 100644 --- a/tests/e2e/test_https_tunnel.py +++ b/tests/e2e/test_https_tunnel.py @@ -24,6 +24,7 @@ from helpers import ( CLIENT_CONTAINER, TARGET_RESPONSE, + assert_tunnel_mode, block_plain_vpn_ports, connect_openvpn, connect_openvpn_https, @@ -168,3 +169,16 @@ def test_connects_and_reaches_target_when_plain_blocked(self, plain_ports_blocke ) finally: disconnect_wireguard_https_tunnel(config_name) + + def test_full_tunnel_reaches_target_without_routing_into_itself( + self, plain_ports_blocked, wireguard_https_full_connection + ): + """wstunnel's own connection must stay off the wg0 default route it carries.""" + wait_for_tunnel("wg0", timeout=15) + assert_tunnel_mode("wg0", "full") + + body = curl_target() + assert body == TARGET_RESPONSE, ( + f"Expected '{TARGET_RESPONSE}', got '{body}': the full tunnel over" + " HTTPS did not carry traffic" + ) diff --git a/tests/unit/test_wg_config_gen.py b/tests/unit/test_wg_config_gen.py index 0a8f963..0302e00 100644 --- a/tests/unit/test_wg_config_gen.py +++ b/tests/unit/test_wg_config_gen.py @@ -233,6 +233,36 @@ def test_no_sni_override_when_the_endpoint_is_the_cert_name(self) -> None: assert "--tls-sni-override" not in config +class TestWstunnelFullTunnel: + """A full tunnel routes everything into wg0, wstunnel's own connection too. + + That connection is what carries wg0, so unmarked it loops into itself. + wg-quick sends traffic carrying its fwmark out the normal route. + """ + + @pytest.mark.parametrize( + ("allowed_ips", "marked"), + [ + pytest.param("0.0.0.0/0, ::/0", True, id="full"), + pytest.param("10.8.3.0/24", False, id="split"), + ], + ) + def test_only_the_full_tunnel_marks_wstunnels_socket(self, allowed_ips, marked): + config = generate_https_tunnel_client_config( + client_private_key=FAKE_CLIENT_PRIV, + client_ip="10.8.0.2", + server_public_key=FAKE_SERVER_PUB, + server_endpoint="vpn.example.com", + server_port=51820, + dns_servers=["1.1.1.1"], + allowed_ips=allowed_ips, + ) + command = next( + line for line in config.splitlines() if line.startswith("# wstunnel") + ) + assert ("--socket-so-mark 51820" in command) is marked + + class TestGenerateClientConfig: """Tests for generate_client_config.""" From 6cc72456cad4593fee6ac8677effd6d9bef86ce0 Mon Sep 17 00:00:00 2001 From: Derek Date: Sun, 1 Nov 2026 19:38:35 +1100 Subject: [PATCH 16/24] fix: drain on SIGTERM before stopping the VPN processes SIGTERM dropped readiness and in the same instant took WireGuard down and terminated every OpenVPN listener, although the README and the Kubernetes guide promise a drain. Readiness now drops first and connected clients keep their tunnels for CULVERT_DRAIN_SECONDS (10 by default) while load balancers stop sending new ones, ending early once none is connected. A second signal skips the rest. The children then share one 15 s stop deadline instead of 10 s each, so the whole stop fits the 30 s grace period. docker-compose.yaml sets stop_grace_period to match. --- .env.example | 6 +++ docker-compose.yaml | 3 ++ docs/use-case-kubernetes.md | 7 +++- scripts/lib/config.py | 10 +++++ scripts/lib/process.py | 83 +++++++++++++++++++++++++++++++++---- scripts/lib/wireguard.py | 9 ++-- tests/unit/test_config.py | 24 +++++++++++ tests/unit/test_process.py | 80 +++++++++++++++++++++++++++++++++++ 8 files changed, 208 insertions(+), 14 deletions(-) diff --git a/.env.example b/.env.example index 7e4ea36..482900a 100644 --- a/.env.example +++ b/.env.example @@ -216,6 +216,12 @@ CULVERT_SERVER_CN="vpn.example.com" # Forced to 1 when CULVERT_ALLOW_SHARED_CLIENTS=false. #CULVERT_SHARED_CLIENT_SLOTS=2 +# Seconds connected clients keep their tunnels after SIGTERM drops readiness, +# so load balancers stop sending new ones first. Ends early with none connected. +# The children then get 15 s, so keep the total inside the stop timeout +# (docker run --stop-timeout, compose stop_grace_period, K8s grace period). +#CULVERT_DRAIN_SECONDS=10 + # Logging configuration # CULVERT_LOG_MODE: "file" (default), "stdout" (K8s/containers), "both" # Kubernetes/EKS/Rancher: Use "stdout" for container logging pipelines diff --git a/docker-compose.yaml b/docker-compose.yaml index 3724911..a45e7e0 100644 --- a/docker-compose.yaml +++ b/docker-compose.yaml @@ -41,6 +41,9 @@ services: # dockerfile: Dockerfile container_name: ${CONTAINER_NAME:-culvert} restart: unless-stopped + # The SIGTERM drain and the children's stop take up to 25 s. Docker's + # default of 10 s would kill the server part way through. + stop_grace_period: 30s labels: - "com.centurylinklabs.watchtower.enable=true" diff --git a/docs/use-case-kubernetes.md b/docs/use-case-kubernetes.md index 0740859..0a4dfc8 100644 --- a/docs/use-case-kubernetes.md +++ b/docs/use-case-kubernetes.md @@ -259,8 +259,11 @@ External PKI section of [.env.example](../.env.example). - **Metrics:** the pod carries `prometheus.io/scrape`, `prometheus.io/port` and `prometheus.io/path` annotations. `/metrics` serves when `CULVERT_METRICS_ENABLED=true`. -- **Drain:** the container handles SIGTERM to drain connections on a rollout, - inside a 30 second termination grace period. +- **Drain:** on SIGTERM readiness drops at once, and connected clients keep + their tunnels for `CULVERT_DRAIN_SECONDS` (10 by default) while load balancers + stop sending new ones. The drain ends early once no client is connected. + OpenVPN is then stopped, which tells UDP clients to reconnect, and every child + gets 15 seconds to exit, all inside a 30 second termination grace period. ## See also diff --git a/scripts/lib/config.py b/scripts/lib/config.py index 7c3cbc8..1b3375c 100644 --- a/scripts/lib/config.py +++ b/scripts/lib/config.py @@ -362,6 +362,10 @@ class Config: crl_refresh_hours: int = 24 + # Seconds between SIGTERM dropping readiness and the children being stopped, + # for load balancers to stop sending new clients. Ends early with none left. + drain_seconds: int = 10 + # Certificate lifetimes for local PKI, in days. Defaults keep the historical # 10-year CA / 2-year leaf. Raise them for a set-and-forget deployment that # has no rotation operator -- e.g. 7300 (20 years) so the server cert cannot @@ -652,6 +656,7 @@ def from_settings(cls) -> Config: otel_protocol=s.get("otel_protocol", "grpc"), otel_insecure=_settings_bool(s, "otel_insecure", False), crl_refresh_hours=_settings_int(s, "crl_refresh_hours", 24), + drain_seconds=_settings_int(s, "drain_seconds", 10), # Certificate lifetimes (local PKI) ca_expire_days=_settings_int(s, "ca_expire_days", 3650), cert_expire_days=_settings_int(s, "cert_expire_days", 730), @@ -909,6 +914,11 @@ def validate(self) -> None: # raised count shrinks how many clients the pool can hold. Only a count # above the shipped default warns: the default's cost is documented, and # a warning every stock deployment emits is one nobody reads. + if self.drain_seconds < 0: + errors.append( + f"CULVERT_DRAIN_SECONDS={self.drain_seconds} must be 0 or more" + ) + if self.shared_client_slots < 1: errors.append( f"CULVERT_SHARED_CLIENT_SLOTS={self.shared_client_slots}" diff --git a/scripts/lib/process.py b/scripts/lib/process.py index c0eea27..3e6b310 100644 --- a/scripts/lib/process.py +++ b/scripts/lib/process.py @@ -19,12 +19,18 @@ import subprocess import sys import tempfile -from collections.abc import Iterable +import time +from collections.abc import Callable, Iterable from pathlib import Path from typing import BinaryIO from scalo.logger import logger +# Inside the deployment contract's 30 s termination grace period: the drain +# (CULVERT_DRAIN_SECONDS, 10 by default) and then this long for children to exit. +CHILD_STOP_SECONDS = 15 +DRAIN_POLL_SECONDS = 1.0 + # Children with a SIGHUP reload. Any other child (wstunnel) dies of the signal. RELOADS_ON_SIGHUP_PREFIX = "openvpn-" RELOADS_ON_SIGHUP = frozenset({"stunnel"}) @@ -216,6 +222,36 @@ def setup_scripts(cfg) -> None: logger.info(f"Made hook script executable: {script}") +def connected_clients() -> int: + """Count OpenVPN sessions in the status files and recently active WireGuard peers.""" + from lib.metrics import ( + OPENVPN_STATUS_FILES, + collect_openvpn_status, + parse_wg_handshakes, + ) + from lib.wireguard import PEER_ACTIVE_SECONDS + + total = 0 + for path, listener in OPENVPN_STATUS_FILES.items(): + status = collect_openvpn_status(path, listener) + if status is not None: + total += status.client_count + try: + shown = run( + ["wg", "show", "wg0", "latest-handshakes"], check=False, capture=True + ) + except FileNotFoundError: + return total + if shown.returncode == 0: + now = time.time() + total += sum( + 1 + for handshake in parse_wg_handshakes(shown.stdout).values() + if handshake.timestamp and now - handshake.timestamp <= PEER_ACTIVE_SECONDS + ) + return total + + class ProcessManager: """Manages VPN child processes with proper signal handling.""" @@ -225,6 +261,7 @@ def __init__(self): self.shutdown_requested = False self.failed_starts: dict[str, str] = {} self.config = None + self.stop_timeout = CHILD_STOP_SECONDS self._daemon_logs: dict[str, BinaryIO] = {} self._setup_signal_handlers() @@ -234,16 +271,42 @@ def _setup_signal_handlers(self) -> None: signal.signal(signal.SIGINT, self._signal_handler) signal.signal(signal.SIGHUP, self._reload_handler) - def _signal_handler(self, signum: int, frame) -> None: # noqa: V107 - signal.signal passes the frame - """Handle shutdown signals.""" + def _signal_handler( # noqa: V107 - signal.signal passes the frame + self, + signum: int, + frame, + count_connected: Callable[[], int] | None = None, + ) -> None: + """Drain, then shut down. A second signal skips what is left of the drain.""" sig_name = signal.Signals(signum).name - logger.info( - f"Received {sig_name}, initiating graceful shutdown...", - signal=sig_name, - ) + if self.shutdown_requested: + logger.info(f"Received {sig_name} again, stopping now", signal=sig_name) + self.shutdown() + logger.info(f"Received {sig_name}, draining before shutdown", signal=sig_name) self.shutdown_requested = True + self.drain(count_connected or connected_clients) self.shutdown() + def drain(self, count_connected: Callable[[], int]) -> None: + """Drop readiness, then serve connected clients for drain_seconds. + + The wait is for load balancers to see readiness drop and send new + clients elsewhere. It ends early once no client is connected. + """ + from lib.health import health + + health.set_ready(False) + seconds = getattr(self.config, "drain_seconds", 0) + deadline = time.monotonic() + seconds + while (remaining := deadline - time.monotonic()) > 0: + connected = count_connected() + if connected == 0: + logger.info("No clients connected, drain finished") + return + logger.info("Draining", clients=connected, seconds_left=round(remaining)) + time.sleep(min(DRAIN_POLL_SECONDS, remaining)) + logger.info("Drain period over") + def _reload_handler(self, signum: int, frame) -> None: # noqa: V107 - signal.signal passes the frame """Handle SIGHUP for config reload.""" logger.info("Received SIGHUP, signaling OpenVPN processes to reload...") @@ -328,10 +391,12 @@ def shutdown(self, exit_code: int = 0) -> None: logger.info(f"Sending SIGTERM to {name}", pid=proc.pid) proc.terminate() - # Wait for processes to exit (with timeout) + # One deadline for all of them: per-child timeouts add up past the + # orchestrator's termination grace period. + deadline = time.monotonic() + self.stop_timeout for name, proc in self.processes.items(): try: - proc.wait(timeout=10) + proc.wait(timeout=max(0.0, deadline - time.monotonic())) logger.info( f"{name} stopped", pid=proc.pid, diff --git a/scripts/lib/wireguard.py b/scripts/lib/wireguard.py index e12c684..70210b2 100644 --- a/scripts/lib/wireguard.py +++ b/scripts/lib/wireguard.py @@ -734,6 +734,11 @@ def forget_missing(self, peers: set[str]) -> None: del state[peer] +# A peer with no handshake for this long is disconnected. WireGuard rekeys every +# 2 minutes while traffic flows. +PEER_ACTIVE_SECONDS = 180 + + def start_wg_connection_monitor(cfg, interval: int = 15) -> None: """Start background thread that polls WireGuard for connection events. @@ -750,8 +755,6 @@ def start_wg_connection_monitor(cfg, interval: int = 15) -> None: record_endpoint_flap, ) - stale_threshold = 180 # 3 minutes without handshake = disconnected - def _poll_loop(): pubkey_names = _build_pubkey_to_name(cfg.pki_dir) connected_peers: set[str] = set() @@ -827,7 +830,7 @@ def _poll_loop(): age = now - hs.timestamp name = pubkey_names.get(pubkey, pubkey[:8]) - if age <= stale_threshold: + if age <= PEER_ACTIVE_SECONDS: current_connected.add(pubkey) if pubkey not in connected_peers: diff --git a/tests/unit/test_config.py b/tests/unit/test_config.py index 4bfca3c..869ae14 100644 --- a/tests/unit/test_config.py +++ b/tests/unit/test_config.py @@ -9,6 +9,7 @@ """Unit tests for Config.from_settings() using CULVERT_* env prefix.""" import re +from contextlib import nullcontext from pathlib import Path import pytest @@ -194,6 +195,29 @@ def test_tc_key_path_defaults_empty(self, clean_env): assert cfg.secrets_tc_key_path == "" +class TestDrainSeconds: + """The SIGTERM drain fits the 30 s grace period with the children's stop.""" + + @pytest.mark.parametrize( + ("value", "want", "valid"), + [ + pytest.param(None, 10, True, id="default"), + pytest.param("0", 0, True, id="no_drain"), + pytest.param("25", 25, True, id="longer"), + pytest.param("-1", -1, False, id="negative"), + ], + ) + def test_drain_seconds(self, clean_env, monkeypatch, value, want, valid): + monkeypatch.setenv("CULVERT_SERVER_CN", "vpn.example.com") + monkeypatch.setenv("CULVERT_HTTPS_ENABLED", "false") + if value is not None: + monkeypatch.setenv("CULVERT_DRAIN_SECONDS", value) + cfg = Config.from_settings() + assert cfg.drain_seconds == want + with nullcontext() if valid else pytest.raises(SystemExit): + cfg.validate() + + class TestConfigNetworkProfiles: """network_profile is a descriptive label; tuning is data in the YAML.""" diff --git a/tests/unit/test_process.py b/tests/unit/test_process.py index 58a2ef8..3b7aab3 100644 --- a/tests/unit/test_process.py +++ b/tests/unit/test_process.py @@ -335,3 +335,83 @@ def test_sighup_reaches_only_children_that_reload(self, manager, name, receives) else: with pytest.raises(subprocess.TimeoutExpired): proc.wait(timeout=1) + + +class TestDrain: + """SIGTERM drops readiness, gives load balancers time, then stops the children. + + Stopping at once lets a load balancer that has not yet seen readiness drop + send a client to a server that is already gone. + """ + + @staticmethod + def _draining(manager, drain_seconds, connected): + """Record, at each count, whether the child is up and the pod is ready.""" + from lib.health import health + + manager.config = SimpleNamespace( + drain_seconds=drain_seconds, protocol="openvpn" + ) + child = manager.start("openvpn-udp", ["sleep", "30"]) + seen = [] + + def count(): + seen.append((child.poll() is None, health.is_ready())) + return connected + + health.set_ready(True) + return child, seen, count + + def test_children_serve_through_the_drain_while_clients_are_connected( + self, manager + ): + child, seen, count = self._draining(manager, drain_seconds=2, connected=3) + + started = time.monotonic() + with pytest.raises(SystemExit): + manager._signal_handler(signal.SIGTERM, None, count_connected=count) + + assert time.monotonic() - started >= 2 + assert seen and all(alive and not ready for alive, ready in seen) + assert child.poll() is not None + + def test_the_drain_ends_once_no_client_is_connected(self, manager): + _, seen, count = self._draining(manager, drain_seconds=30, connected=0) + + started = time.monotonic() + with pytest.raises(SystemExit): + manager._signal_handler(signal.SIGTERM, None, count_connected=count) + + assert time.monotonic() - started < 10 + assert len(seen) == 1 + + def test_a_second_signal_stops_without_finishing_the_drain(self, manager): + child, _, count = self._draining(manager, drain_seconds=30, connected=3) + + def second_signal(): + count() + manager._signal_handler(signal.SIGTERM, None) + return 3 + + started = time.monotonic() + with pytest.raises(SystemExit): + manager._signal_handler(signal.SIGTERM, None, count_connected=second_signal) + + assert time.monotonic() - started < 10 + assert child.poll() is not None + + def test_children_share_one_stop_deadline(self, manager): + """Sequential per-child timeouts outlast a 30 second grace period.""" + manager.stop_timeout = 1 + for name in ("openvpn-udp", "openvpn-tcp", "stunnel"): + manager.start(name, ["sh", "-c", "trap '' TERM; sleep 30"]) + time.sleep(0.2) + + started = time.monotonic() + with pytest.raises(SystemExit): + manager.shutdown() + + assert time.monotonic() - started < 3 + assert all( + proc.poll() == -signal.SIGKILL for proc in manager.processes.values() + ) From db98cb9a3cddfe9d9d6126c3109a14625c160bbe Mon Sep 17 00:00:00 2001 From: Derek Date: Sun, 1 Nov 2026 19:41:40 +1100 Subject: [PATCH 17/24] fix: annotate the registry lock as a Generator ty reports the Iterator return annotation on a @contextmanager function as deprecated. The lock yields once and returns, which is what Generator[None] says. --- scripts/lib/client_state.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/lib/client_state.py b/scripts/lib/client_state.py index 03fc722..7183930 100644 --- a/scripts/lib/client_state.py +++ b/scripts/lib/client_state.py @@ -17,7 +17,7 @@ import hashlib import json import os -from collections.abc import Iterator +from collections.abc import Generator from contextlib import contextmanager from dataclasses import dataclass from pathlib import Path @@ -107,7 +107,7 @@ def current_protocol_state(cfg) -> dict[str, dict[str, object]]: @contextmanager -def _registry_lock(pki_dir: Path) -> Iterator[None]: +def _registry_lock(pki_dir: Path) -> Generator[None]: """Serialise registry read-modify-write operations.""" pki_dir.mkdir(parents=True, exist_ok=True) with open(pki_dir / _LOCK, "w", encoding="utf-8") as lock_file: From fb477fd565ad2a6b179d4ad8a9f3002c11de9a4b Mon Sep 17 00:00:00 2001 From: Derek Date: Sun, 1 Nov 2026 19:41:41 +1100 Subject: [PATCH 18/24] fix: type the fetched PKI set and clear the docstring and vulture findings ty flagged the fetched secrets as bytes | None where pki_set_problems takes bytes, so the set passed on is now only what was fetched. The signal handler's frame parameter carries its vulture mark on its own line, and two Raises sections and one summary line follow the docstring rules. --- scripts/lib/pki.py | 20 ++++++++++---------- scripts/lib/process.py | 5 ++--- scripts/lib/wireguard.py | 2 ++ 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/scripts/lib/pki.py b/scripts/lib/pki.py index a314e83..4c51304 100644 --- a/scripts/lib/pki.py +++ b/scripts/lib/pki.py @@ -255,21 +255,21 @@ def fetch_external_pki(cfg) -> bool: missing = [ name for name, data in fetched.items() if data is None and name != "CRL" ] - if fetched.get("CRL", b"") is None: - logger.warning("CRL could not be fetched, keeping the one on disk") - del fetched["CRL"] if missing: logger.error( f"Not writing any fetched PKI material: missing {', '.join(missing)}" ) return False + if "CRL" in fetched and fetched["CRL"] is None: + logger.warning("CRL could not be fetched, keeping the one on disk") + material = {name: data for name, data in fetched.items() if data is not None} problems = pki_set_problems( - fetched["CA certificate"], - fetched["server certificate"], - fetched["server private key"], - crl=fetched.get("CRL"), - tc_key=fetched.get("tls-crypt-v2 server key"), + material["CA certificate"], + material["server certificate"], + material["server private key"], + crl=material.get("CRL"), + tc_key=material.get("tls-crypt-v2 server key"), ) if problems: for problem in problems: @@ -281,7 +281,7 @@ def fetch_external_pki(cfg) -> bool: # Only a set that passed together is written, so the files on disk are # never one server's certificate beside another's key. - for name, data in fetched.items(): + for name, data in material.items(): _, relative, perms = wanted[name] dest = cfg.pki_dir / relative dest.parent.mkdir(parents=True, exist_ok=True) @@ -494,7 +494,7 @@ def crl_seconds_until_expiry(pki_dir) -> float | None: def revoked_serials(pki_dir) -> frozenset[str]: - """Serial numbers the CRL revokes, empty when there is no readable CRL.""" + """Return the serial numbers the CRL revokes, empty with no readable CRL.""" crl_path = Path(pki_dir) / "crl.pem" if not crl_path.exists(): return frozenset() diff --git a/scripts/lib/process.py b/scripts/lib/process.py index 3e6b310..f51d2d8 100644 --- a/scripts/lib/process.py +++ b/scripts/lib/process.py @@ -194,7 +194,6 @@ def start_log_rotation( ) -> None: """Run logrotate on a schedule in a daemon thread, since no cron runs here.""" import threading - import time def rotate_loop() -> None: while True: @@ -271,10 +270,10 @@ def _setup_signal_handlers(self) -> None: signal.signal(signal.SIGINT, self._signal_handler) signal.signal(signal.SIGHUP, self._reload_handler) - def _signal_handler( # noqa: V107 - signal.signal passes the frame + def _signal_handler( self, signum: int, - frame, + frame, # noqa: V107 - signal.signal passes the frame count_connected: Callable[[], int] | None = None, ) -> None: """Drain, then shut down. A second signal skips what is left of the drain.""" diff --git a/scripts/lib/wireguard.py b/scripts/lib/wireguard.py index 70210b2..eb928c6 100644 --- a/scripts/lib/wireguard.py +++ b/scripts/lib/wireguard.py @@ -104,6 +104,7 @@ def client_config_name( Raises: ValueError: The file name would not be a valid interface name, which happens over HTTPS from slot 10. + """ tunnel = "-https" if https else "" stem = f"wg{slot_infix(slot)}{tunnel}-{mode}" @@ -606,6 +607,7 @@ def sync_running_interface(conf_path: Path) -> bool: Raises: WireGuardSyncError: The interface is up and did not take the new peers, so it still accepts whatever keys it held before. + """ try: shown = subprocess.run(["wg", "show", "wg0"], capture_output=True) From 23410944fbee8e46ff11ebac3fb541c65f6ebbea Mon Sep 17 00:00:00 2001 From: Derek Date: Sun, 1 Nov 2026 19:59:24 +1100 Subject: [PATCH 19/24] fix: route the wstunnel server outside a WireGuard full tunnel The socket mark the previous commit added keeps wstunnel's packets out of wg0, but wg-quick restores its mark on UDP replies only, so on a host with strict rp_filter the server's TCP replies fail the reverse-path check and the tunnel still never comes up. The full-tunnel HTTPS config now tells the user to add a host route for the server through their current gateway, with the Linux, macOS and Windows commands, which is wstunnel's own documented fix and survives strict rp_filter. The e2e case now reaches the server through the default route, as a real client does, and passes only with that route in place. --- docs/use-case-travel.md | 5 ++-- docs/vpn-client-setup.md | 7 +++-- scripts/lib/wireguard.py | 28 +++++++++--------- tests/e2e/conftest.py | 25 ++++++++++++++-- tests/e2e/helpers.py | 15 +++++++--- tests/e2e/test_https_tunnel.py | 13 ++++++++- tests/unit/test_wg_config_gen.py | 50 ++++++++++++++++++++++---------- 7 files changed, 100 insertions(+), 43 deletions(-) diff --git a/docs/use-case-travel.md b/docs/use-case-travel.md index e62a364..d0a89b6 100644 --- a/docs/use-case-travel.md +++ b/docs/use-case-travel.md @@ -77,8 +77,9 @@ stunnel listens on `127.0.0.1:1195` and forwards to your server on config: ```bash -# --socket-so-mark keeps wstunnel's own connection out of the full tunnel (Linux, root) -sudo wstunnel client wss://vpn.example.com:4443 -L udp://51820:127.0.0.1:51820 --socket-so-mark 51820 +wstunnel client wss://vpn.example.com:4443 -L udp://51820:127.0.0.1:51820 +# a full tunnel needs the server routed outside it first (Linux shown) +sudo ip route add via # then activate laptop/wg-https-full.conf in your WireGuard client ``` diff --git a/docs/vpn-client-setup.md b/docs/vpn-client-setup.md index e1357ae..bd471a0 100644 --- a/docs/vpn-client-setup.md +++ b/docs/vpn-client-setup.md @@ -190,9 +190,10 @@ For networks that block WireGuard, use the HTTPS-tunnel configs (`wg-https-*.con each `wg-https-*.conf` carry the exact command for that config.) A full tunnel would route wstunnel's own connection into the tunnel it - carries. On Linux, start wstunnel with `sudo` and add `--socket-so-mark 51820`, - which wg-quick sends outside the tunnel. Elsewhere, route the server's address - through your normal gateway before you activate `wg-https-full.conf`. + carries. Before you activate `wg-https-full.conf`, route the server's address + through the gateway you use now, for example + `sudo ip route add 203.0.113.10 via 192.168.1.1` on Linux. The config's first + lines give the macOS and Windows forms. 2. Then import and activate the `wg-https-*.conf` config in your WireGuard client diff --git a/scripts/lib/wireguard.py b/scripts/lib/wireguard.py index eb928c6..4d4afba 100644 --- a/scripts/lib/wireguard.py +++ b/scripts/lib/wireguard.py @@ -467,11 +467,6 @@ def generate_client_config( return "\n".join(lines) -# wg-quick's fwmark for a full tunnel, which it routes outside wg0. It takes the -# next free table number when 51820 is in use, which a client host rarely has. -WG_QUICK_FWMARK = 51820 - - def generate_https_tunnel_client_config( client_private_key: str | None, client_ip: str, @@ -502,12 +497,6 @@ def generate_https_tunnel_client_config( if tls_server_name and tls_server_name != server_endpoint: tls_flags += f" --tls-sni-override {tls_server_name}" - # A full tunnel would carry wstunnel's own connection to the server inside - # the tunnel it carries, so the full variant marks it to bypass wg0. - full_tunnel = "0.0.0.0/0" in allowed_ips - if full_tunnel: - tls_flags += f" --socket-so-mark {WG_QUICK_FWMARK}" - header = [ "# WireGuard over HTTPS (wstunnel required)", "#", @@ -521,12 +510,21 @@ def generate_https_tunnel_client_config( "# wstunnel carries the UDP tunnel inside WebSocket/TLS, so it travels", "# as ordinary HTTPS on a web port.", ] - if full_tunnel: + # A full tunnel would carry wstunnel's own connection to the server inside + # the tunnel it carries. wstunnel's documented fix is a host route. + if "0.0.0.0/0" in allowed_ips: + try: + server = str(ipaddress.ip_address(server_endpoint)) + except ValueError: + server = f"
" header += [ "#", - "# --socket-so-mark keeps wstunnel's connection out of this tunnel. It", - "# is Linux only and needs root. Elsewhere, drop it and route", - f"# {server_endpoint} through your normal gateway instead.", + "# Full tunnel: before activating, route the server outside the tunnel,", + "# through the gateway your machine uses now, or wstunnel's own", + "# connection to the server loops back into the tunnel it carries:", + f"# Linux: sudo ip route add {server} via ", + f"# macOS: sudo route -n add -host {server} ", + f"# Windows: route add {server} mask 255.255.255.255 ", ] header.append("") diff --git a/tests/e2e/conftest.py b/tests/e2e/conftest.py index 3530f70..cdb0355 100644 --- a/tests/e2e/conftest.py +++ b/tests/e2e/conftest.py @@ -387,15 +387,36 @@ def wireguard_https_tunnel_connection(): @pytest.fixture def wireguard_https_full_connection(): - """Connect WireGuard over HTTPS in FULL-tunnel mode, yield, then disconnect.""" + """Connect WireGuard over HTTPS in FULL-tunnel mode, yield, then disconnect. + + The server shares the client's subnet, and wg-quick's full tunnel leaves a + subnet route alone, so wstunnel would reach it either way. The client goes + through the default route instead, as it does to a server on the internet, + and adds the host route the config tells a full-tunnel user to add. + """ from helpers import ( + SERVER_INTERNAL_IP, + SERVER_IP, connect_wireguard_https_tunnel, disconnect_wireguard_https_tunnel, + docker_exec, + tls_handshake, ) + gateway = docker_exec(CLIENT_CONTAINER, "ip route show default").stdout.split()[2] + docker_exec(CLIENT_CONTAINER, f"ip route replace default via {SERVER_IP}") config_name = f"{CLIENT_NAME}/wg-https-full.conf" - connect_wireguard_https_tunnel(config_name) try: + reached = tls_handshake(4443, host=SERVER_INTERNAL_IP) + assert "Protocol version" in reached, ( + f"the server is not reachable through the default route:\n{reached}" + ) + docker_exec( + CLIENT_CONTAINER, f"ip route add {SERVER_INTERNAL_IP} via {SERVER_IP}" + ) + connect_wireguard_https_tunnel(config_name, server=SERVER_INTERNAL_IP) yield finally: disconnect_wireguard_https_tunnel(config_name) + docker_exec(CLIENT_CONTAINER, f"ip route del {SERVER_INTERNAL_IP}", check=False) + docker_exec(CLIENT_CONTAINER, f"ip route replace default via {gateway}") diff --git a/tests/e2e/helpers.py b/tests/e2e/helpers.py index 87200a5..9044749 100644 --- a/tests/e2e/helpers.py +++ b/tests/e2e/helpers.py @@ -8,6 +8,7 @@ """Helper functions for E2E VPN connection tests.""" +import re import subprocess import time from pathlib import Path @@ -32,6 +33,10 @@ # The server as the client sees it, on the shared external network. SERVER_IP = "172.30.1.10" +# The server's address on the internal network, which the client reaches only +# through a gateway. +SERVER_INTERNAL_IP = "172.30.0.10" + # The plain VPN listeners, as (protocol, port). Blocking exactly these leaves # only the web ports (443 stunnel, 4443 wstunnel) reachable, which is the # network condition the HTTPS-tunnelled listeners exist for. @@ -226,11 +231,12 @@ def disconnect_wireguard( ) -def connect_wireguard_https_tunnel(config_name: str) -> None: +def connect_wireguard_https_tunnel(config_name: str, server: str = "") -> None: """Start wstunnel client + WireGuard for WireGuard-over-HTTPS mode. The HTTPS-tunnel client config points Endpoint at 127.0.0.1:51820 (local wstunnel). - wstunnel client tunnels that UDP over WebSocket/TLS to the server. + wstunnel client tunnels that UDP over WebSocket/TLS to the server. A given + server address replaces the endpoint the config dials. """ src = f"/etc/vpn/clients/{config_name}" # Copy config, strip DNS line @@ -239,8 +245,7 @@ def connect_wireguard_https_tunnel(config_name: str) -> None: f"mkdir -p /etc/wireguard && sed '/^DNS/d' {src} > /etc/wireguard/wg0.conf", ) - # Run the wstunnel command the config itself documents, so a full tunnel - # proves that command keeps wstunnel's connection out of wg0. Certificate + # Run the wstunnel command the config itself documents. Certificate # verification is dropped because the e2e server's certificate is # self-signed. setsid daemonises it inside docker exec. documented = docker_exec( @@ -248,6 +253,8 @@ def connect_wireguard_https_tunnel(config_name: str) -> None: ).stdout.strip() assert documented, f"{config_name} documents no wstunnel command" command = documented.replace(" --tls-verify-certificate", "") + if server: + command = re.sub(r"wss://[^:]+:", f"wss://{server}:", command) docker_exec( CLIENT_CONTAINER, f"setsid {command} /tmp/wstunnel.log 2>&1 &", diff --git a/tests/e2e/test_https_tunnel.py b/tests/e2e/test_https_tunnel.py index 2076903..7c467d8 100644 --- a/tests/e2e/test_https_tunnel.py +++ b/tests/e2e/test_https_tunnel.py @@ -23,6 +23,7 @@ from conftest import CLIENT_NAME from helpers import ( CLIENT_CONTAINER, + SERVER_CONTAINER, TARGET_RESPONSE, assert_tunnel_mode, block_plain_vpn_ports, @@ -178,7 +179,17 @@ def test_full_tunnel_reaches_target_without_routing_into_itself( assert_tunnel_mode("wg0", "full") body = curl_target() + state = docker_exec( + CLIENT_CONTAINER, + "cat /tmp/wstunnel.log; wg show; ip rule; ip route; ss -tnp;" + " sysctl net.ipv4.conf.all.rp_filter net.ipv4.conf.eth0.rp_filter", + check=False, + ) + server_log = docker_exec( + SERVER_CONTAINER, "tail -20 /var/log/vpn/wstunnel.log", check=False + ) assert body == TARGET_RESPONSE, ( f"Expected '{TARGET_RESPONSE}', got '{body}': the full tunnel over" - " HTTPS did not carry traffic" + f" HTTPS did not carry traffic\n{state.stdout}{state.stderr}" + f"\nserver wstunnel:\n{server_log.stdout}" ) diff --git a/tests/unit/test_wg_config_gen.py b/tests/unit/test_wg_config_gen.py index 0302e00..e1784f4 100644 --- a/tests/unit/test_wg_config_gen.py +++ b/tests/unit/test_wg_config_gen.py @@ -236,31 +236,49 @@ def test_no_sni_override_when_the_endpoint_is_the_cert_name(self) -> None: class TestWstunnelFullTunnel: """A full tunnel routes everything into wg0, wstunnel's own connection too. - That connection is what carries wg0, so unmarked it loops into itself. - wg-quick sends traffic carrying its fwmark out the normal route. + That connection is what carries wg0, so the server needs a host route + outside the tunnel, which is wstunnel's own documented fix. """ - @pytest.mark.parametrize( - ("allowed_ips", "marked"), - [ - pytest.param("0.0.0.0/0, ::/0", True, id="full"), - pytest.param("10.8.3.0/24", False, id="split"), - ], - ) - def test_only_the_full_tunnel_marks_wstunnels_socket(self, allowed_ips, marked): - config = generate_https_tunnel_client_config( + @staticmethod + def _config(endpoint: str, allowed_ips: str) -> str: + return generate_https_tunnel_client_config( client_private_key=FAKE_CLIENT_PRIV, client_ip="10.8.0.2", server_public_key=FAKE_SERVER_PUB, - server_endpoint="vpn.example.com", + server_endpoint=endpoint, server_port=51820, dns_servers=["1.1.1.1"], allowed_ips=allowed_ips, ) - command = next( - line for line in config.splitlines() if line.startswith("# wstunnel") - ) - assert ("--socket-so-mark 51820" in command) is marked + + @pytest.mark.parametrize( + ("endpoint", "allowed_ips", "route"), + [ + pytest.param( + "203.0.113.10", + "0.0.0.0/0, ::/0", + "sudo ip route add 203.0.113.10 via ", + id="full_ip", + ), + pytest.param( + "vpn.example.com", + "0.0.0.0/0, ::/0", + "sudo ip route add
via ", + id="full_hostname", + ), + pytest.param("203.0.113.10", "10.8.3.0/24", None, id="split"), + ], + ) + def test_only_the_full_tunnel_routes_the_server_outside( + self, endpoint, allowed_ips, route + ): + config = self._config(endpoint, allowed_ips) + if route is None: + assert "ip route add" not in config + else: + assert f"# Linux: {route}\n" in config + assert "route add" in config.split("Windows:")[1] class TestGenerateClientConfig: From c1ba325596c26479efcc8e445d9d8d216f5d0036 Mon Sep 17 00:00:00 2001 From: Derek Date: Sun, 1 Nov 2026 20:16:01 +1100 Subject: [PATCH 20/24] fix: require openssl's verify OK before accepting a CRL OpenSSL before 3.2, the 3.0.13 in the image and on ubuntu-latest included, exits 0 from openssl crl -CAfile when the signature does not verify and reports the failure only on stderr. A CRL signed by a different key under the CA's name passed the check there. The check now also requires verify OK on stderr. --- scripts/lib/pki.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/scripts/lib/pki.py b/scripts/lib/pki.py index 4c51304..8b106dc 100644 --- a/scripts/lib/pki.py +++ b/scripts/lib/pki.py @@ -76,7 +76,9 @@ def _openssl(args: list[str], data: bytes) -> subprocess.CompletedProcess: def crl_problem(crl: bytes, ca_file: Path) -> str | None: """Return why crl cannot be used with the CA in ca_file, or None.""" - if _openssl(["crl", "-noout", "-CAfile", str(ca_file)], crl).returncode != 0: + # OpenSSL before 3.2 exits 0 on a bad signature and says so only on stderr. + checked = _openssl(["crl", "-noout", "-CAfile", str(ca_file)], crl) + if checked.returncode != 0 or b"verify OK" not in checked.stderr: return "the CRL is not a CRL signed by the CA" return None From c0f5b334069d09a6160e206a9f3c0c3c9e7028f0 Mon Sep 17 00:00:00 2001 From: Derek Date: Sun, 1 Nov 2026 20:21:22 +1100 Subject: [PATCH 21/24] fix: let the log rotation schedule be stopped The schedule thread looped forever, so the unit test's 50 ms schedule kept running logrotate against a deleted tmp directory for the rest of the session and logged a warning on every pass. start_log_rotation now returns an event that stops it, and the test sets it. --- scripts/lib/process.py | 16 +++++++++++----- tests/unit/test_process.py | 12 +++++++----- 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/scripts/lib/process.py b/scripts/lib/process.py index f51d2d8..a1653be 100644 --- a/scripts/lib/process.py +++ b/scripts/lib/process.py @@ -19,6 +19,7 @@ import subprocess import sys import tempfile +import threading import time from collections.abc import Callable, Iterable from pathlib import Path @@ -191,16 +192,21 @@ def start_log_rotation( conf_path: Path, state_path: Path, interval: float = LOG_ROTATION_INTERVAL_SECONDS, -) -> None: - """Run logrotate on a schedule in a daemon thread, since no cron runs here.""" - import threading +) -> threading.Event: + """Run logrotate on a schedule in a daemon thread, since no cron runs here. + + Returns: + An event that stops the schedule once set. + """ + stop = threading.Event() def rotate_loop() -> None: - while True: + rotate_logs(conf_path, state_path) + while not stop.wait(interval): rotate_logs(conf_path, state_path) - time.sleep(interval) threading.Thread(target=rotate_loop, daemon=True, name="logrotate").start() + return stop def setup_scripts(cfg) -> None: diff --git a/tests/unit/test_process.py b/tests/unit/test_process.py index 3b7aab3..3a86e5c 100644 --- a/tests/unit/test_process.py +++ b/tests/unit/test_process.py @@ -178,11 +178,13 @@ def test_the_schedule_runs_logrotate(self, tmp_path): conf, state = setup_log_rotation(cfg, conf_path=tmp_path / "logrotate.conf") (tmp_path / "openvpn.log").write_text("line\n") - start_log_rotation(conf, state, interval=0.05) - - deadline = time.monotonic() + 10 - while not state.exists() and time.monotonic() < deadline: - time.sleep(0.05) + stop = start_log_rotation(conf, state, interval=0.05) + try: + deadline = time.monotonic() + 10 + while not state.exists() and time.monotonic() < deadline: + time.sleep(0.05) + finally: + stop.set() assert state.exists(), "logrotate never ran, so no state file was written" From 71ef9417834d499da1affff6260eec10f93bfbfd Mon Sep 17 00:00:00 2001 From: Derek Date: Sun, 1 Nov 2026 20:22:21 +1100 Subject: [PATCH 22/24] fix: add the blank line the Returns section needs ruff D413 wants a blank line after the last docstring section of start_log_rotation. --- scripts/lib/process.py | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts/lib/process.py b/scripts/lib/process.py index a1653be..5e1990e 100644 --- a/scripts/lib/process.py +++ b/scripts/lib/process.py @@ -197,6 +197,7 @@ def start_log_rotation( Returns: An event that stops the schedule once set. + """ stop = threading.Event() From 9515c838546a1bf0c28d1c3c5c9b45e983f3acba Mon Sep 17 00:00:00 2001 From: Derek Date: Sun, 1 Nov 2026 20:31:30 +1100 Subject: [PATCH 23/24] fix: require a release tag in the generated compose fragment scalo's generator produced deploy/compose/culvert.yaml with `${CULVERT_VERSION:-latest}`. An unset version pulled whatever image latest referenced that day. The generator rewrites it to `${CULVERT_VERSION:?...}` and rejects fragments without that tag, so compose stops if the release is unset. A unit test checks the committed fragment requires the tag. hyperi-ci's compose-pins gate misses deploy/compose/culvert.yaml because it only scans files named docker-compose or compose. --- deploy/compose/culvert.yaml | 2 +- scripts/generate-deploy-artefacts.py | 10 ++++++++++ tests/unit/test_deploy_artefacts.py | 8 ++++++++ 3 files changed, 19 insertions(+), 1 deletion(-) diff --git a/deploy/compose/culvert.yaml b/deploy/compose/culvert.yaml index e3bef5d..5d55589 100644 --- a/deploy/compose/culvert.yaml +++ b/deploy/compose/culvert.yaml @@ -10,7 +10,7 @@ # Generated by scalo deployment module services: culvert: - image: ghcr.io/hyperi-io/culvert:${CULVERT_VERSION:-latest} + image: ghcr.io/hyperi-io/culvert:${CULVERT_VERSION:?set CULVERT_VERSION to the culvert release to run} expose: - "9090" ports: diff --git a/scripts/generate-deploy-artefacts.py b/scripts/generate-deploy-artefacts.py index e43ab6a..b26b095 100644 --- a/scripts/generate-deploy-artefacts.py +++ b/scripts/generate-deploy-artefacts.py @@ -94,6 +94,16 @@ def _write_compose(contract, path: Path) -> None: raise SystemExit(f"compose fragment has no {udp_port.strip()} line to fix up") fragment = fragment.replace(udp_port, udp_port.rstrip("\n")[:-1] + '/udp"\n') + # The generator falls back to `latest`, which runs whatever the registry + # holds on the day. Compose refuses to start until the release is named. + floating = "${CULVERT_VERSION:-latest}" + if floating not in fragment: + raise SystemExit(f"compose fragment has no {floating} tag to pin") + fragment = fragment.replace( + floating, + "${CULVERT_VERSION:?set CULVERT_VERSION to the culvert release to run}", + ) + path.parent.mkdir(parents=True, exist_ok=True) path.write_text( header + fragment, diff --git a/tests/unit/test_deploy_artefacts.py b/tests/unit/test_deploy_artefacts.py index 315a36c..7824020 100644 --- a/tests/unit/test_deploy_artefacts.py +++ b/tests/unit/test_deploy_artefacts.py @@ -355,6 +355,14 @@ def test_vpn_port_is_published_as_udp(self): ) assert "1194:1194/udp" in text + def test_the_image_tag_is_required_not_latest(self): + """A `:-latest` fallback runs whatever latest is on the day of the pull.""" + text = (REPO_ROOT / "deploy" / "compose" / "culvert.yaml").read_text( + encoding="utf-8" + ) + assert "latest" not in text + assert "${CULVERT_VERSION:?" in text + def test_gated_wireguard_port_is_not_published(self): """A default server opens no WireGuard port, so the fragment has none.""" text = (REPO_ROOT / "deploy" / "compose" / "culvert.yaml").read_text( From da70b7aff23aefe72f9971eacd03a713750ecd6d Mon Sep 17 00:00:00 2001 From: Derek Date: Sun, 1 Nov 2026 20:31:30 +1100 Subject: [PATCH 24/24] test: check the client zip carries the full-tunnel host route The server can't know a client's gateway. The full-tunnel HTTPS config includes the host-route command. A unit test opens the client's zip and checks wg-https-full.conf contains the Linux host-route command. The README slot example names Zaphod to match its paragraph. --- README.md | 2 +- tests/unit/test_generate_client.py | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 1dfaaaa..9cc6823 100644 --- a/README.md +++ b/README.md @@ -101,7 +101,7 @@ That mints a real credential, so treat the output accordingly: One identity covers Zaphod's devices concurrently, laptop and phone both connected. OpenVPN shares the certificate across sessions; WireGuard cannot -share a key, so alice gets one peer per device slot (`zaphod/wg-split.conf`, +share a key, so Zaphod gets one peer per device slot (`zaphod/wg-split.conf`, `zaphod/wg2-split.conf`) - `CULVERT_SHARED_CLIENT_SLOTS` sets how many. Set `CULVERT_ALLOW_SHARED_CLIENTS=false` where a credential must identify a single device. diff --git a/tests/unit/test_generate_client.py b/tests/unit/test_generate_client.py index 1dfcaea..dc149f9 100644 --- a/tests/unit/test_generate_client.py +++ b/tests/unit/test_generate_client.py @@ -540,6 +540,15 @@ def test_https_tunnel_variants_when_enabled(self, run, monkeypatch): assert "Endpoint = 127.0.0.1:51820" in tunnelled assert f"wss://{SERVER_CN}:4443" in tunnelled + def test_the_bundle_carries_the_full_tunnel_host_route(self, run, monkeypatch): + """The client's gateway is unknowable here, so the config names the command.""" + monkeypatch.setenv("CULVERT_WG_HTTPS_TUNNEL_ENABLED", "true") + out = run("--protocol", "wireguard", "--name", CLIENT, "--pubkey", PUBKEY_1) + with zipfile.ZipFile(out / f"{CLIENT}.zip") as zf: + full = zf.read("wg-https-full.conf").decode() + route = f"sudo ip route add
via " + assert f"# Linux: {route}\n" in full + # Stand-ins for wg and wg-quick against a server whose interface is up. Keys are # real-shaped base64, and FAKE_WG_SYNC_FAIL makes the live sync refuse.