From 8d904b1b8f382061cb9a68b7955443a5c12840ab Mon Sep 17 00:00:00 2001 From: Dmitrii Iurco Date: Thu, 11 Jun 2026 01:52:26 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20clean-install=20bugs=20=E2=80=94=20Tor?= =?UTF-8?q?=20false-installed,=20WG=20port-check=20honesty,=20encrypted=20?= =?UTF-8?q?backup=20upload?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three independent bugs surfaced during pic1 clean-install testing: 1. Tor _exit_status hardcoded configured=True regardless of whether Tor was actually installed. Status now flows through the same store-installed / container-running bridge used by every other optional service, so Tor only reports installed when the container is present and running. 2. check_port_open compared the port from wg0.conf against the kernel-reported listening port, causing false "port closed" results whenever the conf and the running container were momentarily out of sync. The function is now an honest liveness check: any wg0 interface that is up and has a "listening port:" line in `wg show` is considered open. The check-port API endpoint now also returns the actual kernel listening_port and a port_mismatch flag so the UI can inform the user when a container recreate is needed. (The recreate machinery already exists via the port-change pending-restart path; this fix makes the mismatch visible rather than silently lying about reachability.) 3. upload_backup only handled .zip archives; encrypted .age blobs were rejected with a generic error. The endpoint now calls backup_crypto.is_encrypted() to detect Age-encrypted blobs and stores them verbatim as .tar.gz.age with mode 0600 so they can be uploaded and then restored with a passphrase. The plaintext zip path is unchanged. Tests added/updated: test_connectivity_manager.py (Tor status bridge), test_wireguard_manager.py + test_wireguard_endpoints.py (port-check liveness and mismatch flag), test_config_backup_restore_http.py (encrypted upload round-trip). Co-Authored-By: Claude Fable 5 --- api/connectivity_manager.py | 5 +- api/routes/config.py | 35 +++++++++++--- api/routes/wireguard.py | 11 ++++- api/wireguard_manager.py | 37 ++++++++++++--- tests/test_config_backup_restore_http.py | 59 ++++++++++++++++++++++++ tests/test_connectivity_manager.py | 26 ++++++++++- tests/test_wireguard_endpoints.py | 21 +++++++++ tests/test_wireguard_manager.py | 31 ++++++++++--- webui/src/pages/Settings.jsx | 8 ++-- 9 files changed, 207 insertions(+), 26 deletions(-) diff --git a/api/connectivity_manager.py b/api/connectivity_manager.py index fc1a824..df03103 100644 --- a/api/connectivity_manager.py +++ b/api/connectivity_manager.py @@ -2159,7 +2159,10 @@ class ConnectivityManager(BaseServiceManager): except OSError: info['configured'] = False elif exit_type == 'tor': - info['configured'] = True # Tor uses defaults; no per-cell config + # Tor has no per-cell config file; it counts as configured only via + # the store-installed / container-running bridge below, like every + # other exit type. Do not hardcode True here. + pass elif exit_type == 'sshuttle': info['configured'] = os.path.isfile( os.path.join(self.sshuttle_dir, 'sshuttle.conf')) diff --git a/api/routes/config.py b/api/routes/config.py index 663f5c0..f1a5040 100644 --- a/api/routes/config.py +++ b/api/routes/config.py @@ -961,27 +961,48 @@ def download_backup(backup_id): def upload_backup(): try: from app import config_manager + import backup_crypto if 'file' not in request.files: return jsonify({'error': 'No file provided'}), 400 f = request.files['file'] filename = f.filename or '' - if filename.endswith('.zip'): - backup_id = filename[:-4] - else: + raw = f.read() + + # Derive a clean backup id from the filename, stripping known suffixes. + stem = filename + for suffix in ('.tar.gz.age', '.age', '.zip'): + if stem.endswith(suffix): + stem = stem[:-len(suffix)] + break + backup_id = ''.join(c for c in stem if c.isalnum() or c == '_') + if not backup_id: backup_id = f"backup_{datetime.utcnow().strftime('%Y%m%d_%H%M%S')}" - backup_id = ''.join(c for c in backup_id if c.isalnum() or c == '_') + + # Encrypted backups are opaque blobs: store them verbatim as + # .tar.gz.age so restore_config()/_resolve_backup_dir() can decrypt + # them with the passphrase supplied at restore time. + if backup_crypto.is_encrypted(raw): + config_manager.backup_dir.mkdir(parents=True, exist_ok=True) + archive_path = config_manager.backup_dir / f'{backup_id}.tar.gz.age' + archive_path.write_bytes(raw) + try: + os.chmod(archive_path, 0o600) + except OSError as e: + logger.warning(f"Could not chmod uploaded backup: {e}") + return jsonify({'backup_id': backup_id, 'encrypted': True}) + backup_path = config_manager.backup_dir / backup_id backup_path.mkdir(parents=True, exist_ok=True) try: - with zipfile.ZipFile(io.BytesIO(f.read())) as zf: + with zipfile.ZipFile(io.BytesIO(raw)) as zf: zf.extractall(backup_path) except zipfile.BadZipFile: shutil.rmtree(backup_path, ignore_errors=True) - return jsonify({'error': 'Invalid zip file'}), 400 + return jsonify({'error': 'Invalid backup file'}), 400 if not (backup_path / 'manifest.json').exists(): shutil.rmtree(backup_path, ignore_errors=True) return jsonify({'error': 'Invalid backup: missing manifest.json'}), 400 - return jsonify({'backup_id': backup_id}) + return jsonify({'backup_id': backup_id, 'encrypted': False}) except Exception as e: logger.error(f"Error uploading backup: {e}") return jsonify({'error': str(e)}), 500 diff --git a/api/routes/wireguard.py b/api/routes/wireguard.py index 55904d9..d0a9026 100644 --- a/api/routes/wireguard.py +++ b/api/routes/wireguard.py @@ -288,6 +288,15 @@ def check_wireguard_port(): try: from app import wireguard_manager port_open = wireguard_manager.check_port_open() - return jsonify({'port_open': port_open, 'port': wireguard_manager._get_configured_port()}) + configured_port = wireguard_manager._get_configured_port() + listening_port = wireguard_manager._kernel_listening_port() + return jsonify({ + 'port_open': port_open, + 'port': configured_port, + 'listening_port': listening_port, + 'port_mismatch': ( + listening_port is not None and listening_port != configured_port + ), + }) except Exception as e: return jsonify({"error": str(e)}), 500 diff --git a/api/wireguard_manager.py b/api/wireguard_manager.py index 7e59f71..142214f 100644 --- a/api/wireguard_manager.py +++ b/api/wireguard_manager.py @@ -984,19 +984,44 @@ class WireGuardManager(BaseServiceManager): pass return ip - def check_port_open(self, port: int = None) -> bool: - """Check if WireGuard is running and listening on the configured UDP port.""" - configured_port = port if port is not None else self._get_configured_port() - # Primary: verify wg0 is up AND listening on the configured port + def _kernel_listening_port(self) -> Optional[int]: + """Return the UDP port wg0 is actually bound to per `wg show`, or None. + + This reads the live kernel state, which is the source of truth for what + port traffic must reach — it may differ from wg0.conf's ListenPort if the + container has not been recreated since the port was changed. + """ try: result = subprocess.run( ['docker', 'exec', 'cell-wireguard', 'wg', 'show', 'wg0'], capture_output=True, text=True, timeout=5, ) - if result.returncode == 0 and f'listening port: {configured_port}' in result.stdout.lower(): - return True + if result.returncode != 0: + return None + for line in result.stdout.lower().splitlines(): + line = line.strip() + if line.startswith('listening port:'): + try: + return int(line.split(':', 1)[1].strip()) + except (ValueError, IndexError): + return None except Exception: pass + return None + + def check_port_open(self, port: int = None) -> bool: + """True when WireGuard is up and bound to a UDP port (reachable). + + This is a liveness check, not a strict equality check against the + configured port: an interface that is up with a `listening port:` line + is serving traffic on that bound port. The bound port may differ from + wg0.conf's ListenPort if the container has not yet been recreated — that + is surfaced separately via the endpoint's actual-port field, not by + reporting the port closed. + """ + # Primary: wg0 is up and has a listening port → reachable on that port. + if self._kernel_listening_port() is not None: + return True # Fallback: recent peer handshake confirms external reachability try: statuses = self.get_all_peer_statuses() diff --git a/tests/test_config_backup_restore_http.py b/tests/test_config_backup_restore_http.py index 17ac5e6..393150e 100644 --- a/tests/test_config_backup_restore_http.py +++ b/tests/test_config_backup_restore_http.py @@ -30,6 +30,8 @@ api_dir = Path(__file__).parent.parent / 'api' sys.path.insert(0, str(api_dir)) from app import app +import backup_crypto +import tarfile class TestCreateConfigBackup(unittest.TestCase): @@ -345,6 +347,63 @@ class TestUploadBackup(unittest.TestCase): ) self.assertEqual(r.status_code, 400) + @patch('app.config_manager') + def test_upload_stores_encrypted_blob_verbatim(self, mock_cm): + backup_dir = Path(self.tmp) + mock_cm.backup_dir = backup_dir + blob = backup_crypto.encrypt_bytes(b'payload-bytes', 'secret') + self.assertTrue(blob.startswith(backup_crypto.MAGIC)) + r = self.client.post( + '/api/config/backup/upload', + data={'file': (io.BytesIO(blob), 'backup_20260101_010101.tar.gz.age')}, + content_type='multipart/form-data', + ) + self.assertEqual(r.status_code, 200) + data = json.loads(r.data) + self.assertTrue(data['encrypted']) + self.assertEqual(data['backup_id'], 'backup_20260101_010101') + archive = backup_dir / 'backup_20260101_010101.tar.gz.age' + self.assertTrue(archive.exists()) + self.assertEqual(archive.read_bytes(), blob) + + @patch('app.config_manager') + def test_upload_encrypted_then_restore_round_trip(self, mock_cm): + # Build a real encrypted backup archive (tar.gz of a manifest, then + # encrypted), upload it, then restore it through the real ConfigManager + # decrypt/resolve path with the correct and an incorrect passphrase. + from config_manager import ConfigManager + + backup_dir = Path(self.tmp) / 'backups' + backup_dir.mkdir(parents=True, exist_ok=True) + mock_cm.backup_dir = backup_dir + + tar_buf = io.BytesIO() + with tarfile.open(fileobj=tar_buf, mode='w:gz') as tar: + inner = json.dumps({'backup_id': 'rt', 'services': []}).encode() + info = tarfile.TarInfo('manifest.json') + info.size = len(inner) + tar.addfile(info, io.BytesIO(inner)) + blob = backup_crypto.encrypt_bytes(tar_buf.getvalue(), 'pw123') + + r = self.client.post( + '/api/config/backup/upload', + data={'file': (io.BytesIO(blob), 'rt.tar.gz.age')}, + content_type='multipart/form-data', + ) + self.assertEqual(r.status_code, 200) + backup_id = json.loads(r.data)['backup_id'] + + # Resolve+decrypt with the correct passphrase succeeds. + real_cm = ConfigManager.__new__(ConfigManager) + real_cm.backup_dir = backup_dir + path, cleanup = real_cm._resolve_backup_dir(f'{backup_id}.tar.gz.age', 'pw123') + self.assertTrue((path / 'manifest.json').exists()) + shutil.rmtree(cleanup, ignore_errors=True) + + # Wrong passphrase raises PermissionError → route returns 400. + with self.assertRaises(PermissionError): + real_cm._resolve_backup_dir(f'{backup_id}.tar.gz.age', 'wrong') + if __name__ == '__main__': unittest.main() diff --git a/tests/test_connectivity_manager.py b/tests/test_connectivity_manager.py index de763a5..829559e 100644 --- a/tests/test_connectivity_manager.py +++ b/tests/test_connectivity_manager.py @@ -1037,11 +1037,35 @@ class TestExitStatus(unittest.TestCase): self.assertIn('status', item) self.assertIn(item['status'], ('active', 'configured', 'not_configured')) - def test_tor_defaults_to_configured(self): + def test_tor_not_configured_when_not_installed_or_running(self): + # Tor must not report configured just because it has no per-cell config; + # it flows through the store-installed / container-running bridge. mgr = self._mgr() with patch.object(cm_module, 'subprocess') as mock_sp: mock_sp.run.return_value = MagicMock(returncode=1, stdout='', stderr='') info = mgr._exit_status('tor') + self.assertFalse(info['configured']) + self.assertEqual(info['status'], 'not_configured') + + def test_tor_configured_when_store_installed(self): + mgr = self._mgr(installed={'tor': {'manifest': {'id': 'tor'}}}) + with patch.object(cm_module, 'subprocess') as mock_sp: + mock_sp.run.return_value = MagicMock(returncode=1, stdout='', stderr='') + info = mgr._exit_status('tor') + self.assertTrue(info['configured']) + self.assertEqual(info['status'], 'configured') + + def test_tor_configured_when_container_running(self): + mgr = self._mgr() + + def fake_run(cmd, **kwargs): + if 'inspect' in cmd: + return MagicMock(returncode=0, stdout='true\n', stderr='') + return MagicMock(returncode=1, stdout='', stderr='') + + with patch.object(cm_module, 'subprocess') as mock_sp: + mock_sp.run.side_effect = fake_run + info = mgr._exit_status('tor') self.assertTrue(info['configured']) self.assertEqual(info['status'], 'configured') diff --git a/tests/test_wireguard_endpoints.py b/tests/test_wireguard_endpoints.py index 817e144..c9f6b5a 100644 --- a/tests/test_wireguard_endpoints.py +++ b/tests/test_wireguard_endpoints.py @@ -36,6 +36,7 @@ class TestWireGuardEndpoints(unittest.TestCase): def test_check_port_returns_port_open_true(self, mock_wg): mock_wg.check_port_open.return_value = True mock_wg._get_configured_port.return_value = 51820 + mock_wg._kernel_listening_port.return_value = 51820 r = self.client.post('/api/wireguard/check-port') self.assertEqual(r.status_code, 200) data = json.loads(r.data) @@ -43,15 +44,35 @@ class TestWireGuardEndpoints(unittest.TestCase): self.assertIn('port', data) self.assertTrue(data['port_open']) self.assertEqual(data['port'], 51820) + self.assertEqual(data['listening_port'], 51820) + self.assertFalse(data['port_mismatch']) + + @patch('app.wireguard_manager') + def test_check_port_reports_actual_listening_port_on_mismatch(self, mock_wg): + # Configured 51821 but kernel bound to 51820 — endpoint surfaces the real + # bound port and flags the mismatch without reporting the port closed. + mock_wg.check_port_open.return_value = True + mock_wg._get_configured_port.return_value = 51821 + mock_wg._kernel_listening_port.return_value = 51820 + r = self.client.post('/api/wireguard/check-port') + self.assertEqual(r.status_code, 200) + data = json.loads(r.data) + self.assertTrue(data['port_open']) + self.assertEqual(data['port'], 51821) + self.assertEqual(data['listening_port'], 51820) + self.assertTrue(data['port_mismatch']) @patch('app.wireguard_manager') def test_check_port_returns_port_open_false(self, mock_wg): mock_wg.check_port_open.return_value = False mock_wg._get_configured_port.return_value = 51820 + mock_wg._kernel_listening_port.return_value = None r = self.client.post('/api/wireguard/check-port') self.assertEqual(r.status_code, 200) data = json.loads(r.data) self.assertFalse(data['port_open']) + self.assertIsNone(data['listening_port']) + self.assertFalse(data['port_mismatch']) @patch('app.wireguard_manager') def test_check_port_returns_500_on_exception(self, mock_wg): diff --git a/tests/test_wireguard_manager.py b/tests/test_wireguard_manager.py index c3d852e..edfcd2a 100644 --- a/tests/test_wireguard_manager.py +++ b/tests/test_wireguard_manager.py @@ -615,27 +615,44 @@ class TestWireGuardSysctlAndPortCheck(unittest.TestCase): self.assertTrue(result) @patch('subprocess.run') - def test_check_port_open_wrong_port_returns_false(self, mock_run): - # wg0 is up but listening on 51820 while wg0.conf says 51821 — must return False + def test_check_port_open_true_despite_port_mismatch(self, mock_run): + # wg0 is up and listening on 51820 while wg0.conf says 51821. The kernel + # port is what actually serves traffic, so this is a reachability success; + # check_port_open must NOT return False merely because of the mismatch. mock_run.return_value.returncode = 0 mock_run.return_value.stdout = 'interface: wg0\n listening port: 51820\n' - # Write wg0.conf with a different port so _get_configured_port() returns 51821 cfg_path = os.path.join(self.wg.wireguard_dir, 'wg0.conf') with open(cfg_path, 'w') as f: f.write('[Interface]\nListenPort = 51821\nPrivateKey = abc\n') - self.assertFalse(self.wg.check_port_open()) + self.assertTrue(self.wg.check_port_open()) @patch('subprocess.run') - def test_check_port_open_explicit_port_matches(self, mock_run): + def test_kernel_listening_port_parses_actual_bound_port(self, mock_run): + mock_run.return_value.returncode = 0 + mock_run.return_value.stdout = 'interface: wg0\n listening port: 51820\n' + self.assertEqual(self.wg._kernel_listening_port(), 51820) + + @patch('subprocess.run') + def test_kernel_listening_port_none_when_down(self, mock_run): + mock_run.return_value.returncode = 1 + mock_run.return_value.stdout = '' + self.assertIsNone(self.wg._kernel_listening_port()) + + @patch('subprocess.run') + def test_check_port_open_true_when_interface_bound(self, mock_run): + # check_port_open is a liveness check: an up interface with a bound port + # is reachable, regardless of which port number it is. mock_run.return_value.returncode = 0 mock_run.return_value.stdout = 'interface: wg0\n listening port: 12345\n' self.assertTrue(self.wg.check_port_open(port=12345)) @patch('subprocess.run') - def test_check_port_open_explicit_port_mismatch(self, mock_run): + def test_check_port_open_true_even_when_bound_port_differs(self, mock_run): + # Bound port (51820) differs from the configured/expected port (51821), + # but the interface is up and serving — this is reachable, not closed. mock_run.return_value.returncode = 0 mock_run.return_value.stdout = 'interface: wg0\n listening port: 51820\n' - self.assertFalse(self.wg.check_port_open(port=51821)) + self.assertTrue(self.wg.check_port_open(port=51821)) # ── get_peer_status ─────────────────────────────────────────────────────── diff --git a/webui/src/pages/Settings.jsx b/webui/src/pages/Settings.jsx index 8a7c35f..acff0d1 100644 --- a/webui/src/pages/Settings.jsx +++ b/webui/src/pages/Settings.jsx @@ -720,7 +720,9 @@ function Settings() { const url = URL.createObjectURL(res.data); const a = document.createElement('a'); a.href = url; - a.download = `${id}.zip`; + // Encrypted archives already carry their .tar.gz.age extension; only + // plaintext backups (served as a zip) need a .zip name appended. + a.download = id.endsWith('.age') ? id : `${id}.zip`; a.click(); URL.revokeObjectURL(url); } catch { @@ -739,7 +741,7 @@ function Settings() { const res = await cellAPI.listBackups(); setBackups(res.data || []); } catch { - toast('Upload failed — ensure it is a valid backup zip', 'error'); + toast('Upload failed — ensure it is a valid backup (.zip or encrypted .age) file', 'error'); } finally { setBackupUploading(false); } @@ -1088,7 +1090,7 @@ function Settings() {