From d9180b92499314a77920837689814f713ef6305a Mon Sep 17 00:00:00 2001 From: ahcrm-core Date: Sun, 20 Sep 2026 19:43:19 -0500 Subject: [PATCH] fix(client): isolate prefixed client config files Signed-off-by: Arafat K. El haroun --- scripts/generate-client.py | 14 ++++++++++++-- scripts/revoke-client.py | 20 +++++++++++++++++--- tests/unit/test_revoke.py | 25 +++++++++++++++++++++++++ tests/unit/test_wg_config_gen.py | 24 ++++++++++++++++++++++++ 4 files changed, 78 insertions(+), 5 deletions(-) diff --git a/scripts/generate-client.py b/scripts/generate-client.py index 7a2b37e..2ea0e9b 100644 --- a/scripts/generate-client.py +++ b/scripts/generate-client.py @@ -448,6 +448,13 @@ def generate_ovpn_config( # =============================================================================== +_CLIENT_CONFIG_SUFFIX = re.compile( + r"(?:udp|tcp|https|proxy)-(?:split|full)\.ovpn" + r"|(?:proxy-)?stunnel\.conf" + r"|wg\d*(?:-https)?-(?:split|full)\.conf" +) + + def _bundle_client_zip(client_name: str, output_dir: Path) -> Path | None: """Zip a single client's generated files into .zip (0600). @@ -458,10 +465,13 @@ def _bundle_client_zip(client_name: str, output_dir: Path) -> Path | None: """ import zipfile + prefix = f"{client_name}-" members = sorted( p - for p in output_dir.glob(f"{client_name}-*") - if p.is_file() and p.suffix != ".zip" + for p in output_dir.iterdir() + if p.is_file() + and p.name.startswith(prefix) + and _CLIENT_CONFIG_SUFFIX.fullmatch(p.name[len(prefix) :]) ) if not members: return None diff --git a/scripts/revoke-client.py b/scripts/revoke-client.py index 0a56ccc..192f9ef 100644 --- a/scripts/revoke-client.py +++ b/scripts/revoke-client.py @@ -48,6 +48,17 @@ PKI_DIR = Path("/etc/vpn/pki") OUTPUT_DIR = Path("/etc/vpn/clients") +OPENVPN_CONFIG_SUFFIXES = ( + "udp-split.ovpn", + "udp-full.ovpn", + "tcp-split.ovpn", + "tcp-full.ovpn", + "https-split.ovpn", + "https-full.ovpn", + "proxy-split.ovpn", + "proxy-full.ovpn", +) + class RevocationError(RuntimeError): """Revocation could not be completed, so the client still has access.""" @@ -160,9 +171,12 @@ def revoke_client(client_name: str, missing_ok: bool = False) -> bool: PKI_DIR / "reqs" / f"{client_name}.req", ] - # Remove .ovpn files - for ovpn_file in OUTPUT_DIR.glob(f"{client_name}-*.ovpn"): - files_to_remove.append(ovpn_file) + # Remove only filenames generate-client can create for this exact client. + # A prefix glob also matches another client such as foo-bar when revoking + # foo, silently deleting that client's private configuration. + files_to_remove.extend( + OUTPUT_DIR / f"{client_name}-{suffix}" for suffix in OPENVPN_CONFIG_SUFFIXES + ) removed = 0 for f in files_to_remove: diff --git a/tests/unit/test_revoke.py b/tests/unit/test_revoke.py index 393a02c..795e73b 100644 --- a/tests/unit/test_revoke.py +++ b/tests/unit/test_revoke.py @@ -151,6 +151,31 @@ def fake_from_settings(*args, **kwargs): ) +class TestOpenVpnConfigRemoval: + """Revoking one client must not delete another client's credentials.""" + + def test_client_name_prefix_preserves_longer_clients_configs( + self, revoke, monkeypatch + ): + issued = revoke.PKI_DIR / "issued" + issued.mkdir() + (issued / "foo.crt").write_text("certificate", encoding="utf-8") + own_config = revoke.OUTPUT_DIR / "foo-udp-split.ovpn" + other_config = revoke.OUTPUT_DIR / "foo-bar-udp-split.ovpn" + own_config.write_text("foo key", encoding="utf-8") + other_config.write_text("foo-bar key", encoding="utf-8") + monkeypatch.setattr( + revoke.subprocess, + "run", + lambda *a, **kw: subprocess.CompletedProcess(a[0], 0, "", ""), + ) + + assert revoke.revoke_client("foo") is True + + assert not own_config.exists() + assert other_config.exists() + + class TestInterfaceDetection: """ "wg is not installed" and "wg0 refused the change" are different faults.""" diff --git a/tests/unit/test_wg_config_gen.py b/tests/unit/test_wg_config_gen.py index f4bda1d..03d1ba5 100644 --- a/tests/unit/test_wg_config_gen.py +++ b/tests/unit/test_wg_config_gen.py @@ -517,6 +517,30 @@ def test_bundles_only_this_clients_files(self, tmp_path: Path) -> None: assert names == {"alice-udp-split.ovpn", "alice-wg-split.conf"} assert stat.S_IMODE(zip_path.stat().st_mode) == 0o600 + def test_client_name_prefix_does_not_bundle_another_clients_keys( + self, tmp_path: Path + ) -> None: + """A client named foo must not receive foo-bar's private configs.""" + import zipfile + + (tmp_path / "foo-udp-split.ovpn").write_text("foo key", encoding="utf-8") + (tmp_path / "foo-wg-split.conf").write_text("foo wg key", encoding="utf-8") + (tmp_path / "foo-bar-udp-split.ovpn").write_text( + "foo-bar key", encoding="utf-8" + ) + (tmp_path / "foo-bar-wg-split.conf").write_text( + "foo-bar wg key", encoding="utf-8" + ) + + zip_path = self._module()._bundle_client_zip("foo", tmp_path) + + assert zip_path is not None + with zipfile.ZipFile(zip_path) as zf: + assert set(zf.namelist()) == { + "foo-udp-split.ovpn", + "foo-wg-split.conf", + } + def test_excludes_existing_zip_and_returns_none_when_empty( self, tmp_path: Path ) -> None: