Fix wireguard.status()/set_tunnel_enabled() crash on unreadable conf path
Found by actually running furtka serve and clicking through the new Settings UI: /api/gateway/status crashed the whole request with a raw PermissionError traceback because conf_path.is_file() doesn't swallow every OSError (confirmed directly, not assumed) when the parent directory isn't accessible. A read-only status check must degrade to "not configured" instead of 500ing; set_tunnel_enabled() now raises a clean WireguardError instead of leaking an unhandled exception. Also gives wireguard.py an env-var-overridable default path (FURTKA_WIREGUARD_CONF / default_conf_path()), matching every other runtime path in the codebase, instead of a single hardcoded constant.
This commit is contained in:
parent
e88b57f74d
commit
91f2d6304c
2 changed files with 70 additions and 13 deletions
|
|
@ -16,13 +16,18 @@ that's unavoidable with stock wg-easy) and hands it to
|
||||||
set_tunnel_enabled() here.
|
set_tunnel_enabled() here.
|
||||||
"""
|
"""
|
||||||
|
|
||||||
|
import os
|
||||||
import subprocess
|
import subprocess
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
|
|
||||||
CONF_PATH = Path("/etc/wireguard/furtka-gw.conf")
|
DEFAULT_CONF_PATH = Path("/etc/wireguard/furtka-gw.conf")
|
||||||
INTERFACE = "furtka-gw"
|
INTERFACE = "furtka-gw"
|
||||||
|
|
||||||
|
|
||||||
|
def default_conf_path() -> Path:
|
||||||
|
return Path(os.environ.get("FURTKA_WIREGUARD_CONF", DEFAULT_CONF_PATH))
|
||||||
|
|
||||||
|
|
||||||
class WireguardError(Exception):
|
class WireguardError(Exception):
|
||||||
"""Recoverable failure from set_tunnel_enabled — the caller should 5xx."""
|
"""Recoverable failure from set_tunnel_enabled — the caller should 5xx."""
|
||||||
|
|
||||||
|
|
@ -60,7 +65,7 @@ def _default_reload(action: str) -> None:
|
||||||
def set_tunnel_enabled(
|
def set_tunnel_enabled(
|
||||||
enabled: bool,
|
enabled: bool,
|
||||||
wg: dict | None = None,
|
wg: dict | None = None,
|
||||||
conf_path: Path = CONF_PATH,
|
conf_path: Path | None = None,
|
||||||
reload=_default_reload,
|
reload=_default_reload,
|
||||||
) -> bool:
|
) -> bool:
|
||||||
"""Bring the tunnel up (writing conf_path from `wg`) or down.
|
"""Bring the tunnel up (writing conf_path from `wg`) or down.
|
||||||
|
|
@ -72,16 +77,21 @@ def set_tunnel_enabled(
|
||||||
"""
|
"""
|
||||||
if enabled and wg is None:
|
if enabled and wg is None:
|
||||||
raise ValueError("wg config is required to enable the tunnel")
|
raise ValueError("wg config is required to enable the tunnel")
|
||||||
|
if conf_path is None:
|
||||||
|
conf_path = default_conf_path()
|
||||||
|
|
||||||
conf_path.parent.mkdir(mode=0o700, parents=True, exist_ok=True)
|
try:
|
||||||
had_conf = conf_path.is_file()
|
conf_path.parent.mkdir(mode=0o700, parents=True, exist_ok=True)
|
||||||
previous = conf_path.read_text() if had_conf else None
|
had_conf = conf_path.is_file()
|
||||||
|
previous = conf_path.read_text() if had_conf else None
|
||||||
|
|
||||||
if enabled:
|
if enabled:
|
||||||
conf_path.write_text(_conf_content(wg))
|
conf_path.write_text(_conf_content(wg))
|
||||||
conf_path.chmod(0o600)
|
conf_path.chmod(0o600)
|
||||||
elif had_conf:
|
elif had_conf:
|
||||||
conf_path.unlink()
|
conf_path.unlink()
|
||||||
|
except OSError as e:
|
||||||
|
raise WireguardError(f"could not access {conf_path}: {e}") from e
|
||||||
|
|
||||||
try:
|
try:
|
||||||
reload("enable" if enabled else "disable")
|
reload("enable" if enabled else "disable")
|
||||||
|
|
@ -115,11 +125,22 @@ def _default_wg_show() -> str:
|
||||||
return result.stdout.strip()
|
return result.stdout.strip()
|
||||||
|
|
||||||
|
|
||||||
def status(conf_path: Path = CONF_PATH, wg_show=_default_wg_show) -> dict:
|
def status(conf_path: Path | None = None, wg_show=_default_wg_show) -> dict:
|
||||||
"""Whether the tunnel is configured, and its latest handshake if `wg`
|
"""Whether the tunnel is configured, and its latest handshake if `wg`
|
||||||
tooling is available (None if not — e.g. wg-tools missing, or the
|
tooling is available (None if not — e.g. wg-tools missing, or the
|
||||||
interface isn't actually up yet right after enabling)."""
|
interface isn't actually up yet right after enabling).
|
||||||
configured = conf_path.is_file()
|
|
||||||
|
Read-only and best-effort: any OSError stat-ing conf_path (missing
|
||||||
|
parent dir, an unexpected permission situation) is treated the same as
|
||||||
|
"not configured" rather than raised — a status check must never 500
|
||||||
|
the whole request over something this innocuous.
|
||||||
|
"""
|
||||||
|
if conf_path is None:
|
||||||
|
conf_path = default_conf_path()
|
||||||
|
try:
|
||||||
|
configured = conf_path.is_file()
|
||||||
|
except OSError:
|
||||||
|
configured = False
|
||||||
latest_handshake = None
|
latest_handshake = None
|
||||||
if configured:
|
if configured:
|
||||||
try:
|
try:
|
||||||
|
|
|
||||||
|
|
@ -37,6 +37,26 @@ def test_enable_requires_wg_config(tmp_path):
|
||||||
wireguard.set_tunnel_enabled(True, conf_path=tmp_path / "furtka-gw.conf")
|
wireguard.set_tunnel_enabled(True, conf_path=tmp_path / "furtka-gw.conf")
|
||||||
|
|
||||||
|
|
||||||
|
def test_enable_surfaces_clean_error_on_unreadable_conf_dir(tmp_path):
|
||||||
|
restricted = tmp_path / "restricted"
|
||||||
|
restricted.mkdir(mode=0o000)
|
||||||
|
try:
|
||||||
|
with pytest.raises(wireguard.WireguardError, match="could not access"):
|
||||||
|
wireguard.set_tunnel_enabled(True, wg=WG, conf_path=restricted / "furtka-gw.conf")
|
||||||
|
finally:
|
||||||
|
restricted.chmod(0o700)
|
||||||
|
|
||||||
|
|
||||||
|
def test_default_conf_path_uses_env_override(monkeypatch, tmp_path):
|
||||||
|
monkeypatch.setenv("FURTKA_WIREGUARD_CONF", str(tmp_path / "custom.conf"))
|
||||||
|
assert wireguard.default_conf_path() == tmp_path / "custom.conf"
|
||||||
|
|
||||||
|
|
||||||
|
def test_default_conf_path_default_when_unset(monkeypatch):
|
||||||
|
monkeypatch.delenv("FURTKA_WIREGUARD_CONF", raising=False)
|
||||||
|
assert wireguard.default_conf_path() == wireguard.DEFAULT_CONF_PATH
|
||||||
|
|
||||||
|
|
||||||
def test_enable_writes_conf_and_calls_reload(tmp_path):
|
def test_enable_writes_conf_and_calls_reload(tmp_path):
|
||||||
conf_path = tmp_path / "furtka-gw.conf"
|
conf_path = tmp_path / "furtka-gw.conf"
|
||||||
calls = []
|
calls = []
|
||||||
|
|
@ -115,6 +135,22 @@ def test_status_not_configured(tmp_path):
|
||||||
assert s == {"configured": False, "latest_handshake": None}
|
assert s == {"configured": False, "latest_handshake": None}
|
||||||
|
|
||||||
|
|
||||||
|
def test_status_survives_unreadable_conf_path(tmp_path):
|
||||||
|
"""Regression: an inaccessible parent directory raises PermissionError
|
||||||
|
even for a path that doesn't exist yet — confirmed by actually hitting
|
||||||
|
this against a real box's /etc/wireguard/ as a non-root user, which
|
||||||
|
crashed the whole /api/gateway/status request with a raw traceback.
|
||||||
|
A read-only status check must degrade to "not configured", never 500.
|
||||||
|
"""
|
||||||
|
restricted = tmp_path / "restricted"
|
||||||
|
restricted.mkdir(mode=0o000)
|
||||||
|
try:
|
||||||
|
s = wireguard.status(conf_path=restricted / "furtka-gw.conf")
|
||||||
|
assert s == {"configured": False, "latest_handshake": None}
|
||||||
|
finally:
|
||||||
|
restricted.chmod(0o700) # so tmp_path cleanup can remove it
|
||||||
|
|
||||||
|
|
||||||
def test_status_configured_reports_handshake(tmp_path):
|
def test_status_configured_reports_handshake(tmp_path):
|
||||||
conf_path = tmp_path / "furtka-gw.conf"
|
conf_path = tmp_path / "furtka-gw.conf"
|
||||||
conf_path.write_text(wireguard._conf_content(WG))
|
conf_path.write_text(wireguard._conf_content(WG))
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue