From 409e328880c4c50998e9b606946a4e76dd9e5958 Mon Sep 17 00:00:00 2001 From: saphid <4596216+saphid@users.noreply.github.com> Date: Tue, 29 Sep 2026 07:52:28 +1000 Subject: [PATCH] Devices: each headset its own SSH connection, and each server keeps its own headset (review round 27) ssh's %C hashes only address, user and port, so two headsets reached at one address shared a ControlMaster and one's commands could run on the other: the ControlPath now names the headset. Another Frame Control server choosing a different headset no longer moves this one's commands mid-install. Co-Authored-By: Claude Opus 5.5 (1M context) --- tests/test_devices.py | 11 +++++++++++ tests/test_link.py | 17 +++++++++++++++-- ui/frame_devices.py | 6 ++++++ ui/frame_host.py | 7 +++++-- ui/frame_link.py | 15 +++++++++++++-- ui/server.py | 3 ++- 6 files changed, 52 insertions(+), 7 deletions(-) diff --git a/tests/test_devices.py b/tests/test_devices.py index 1cd1169..0eed78b 100644 --- a/tests/test_devices.py +++ b/tests/test_devices.py @@ -184,6 +184,17 @@ class SharedFile(Base): self.assertIn("100.101.1.2", hosts) self.assertIn("100.101.1.2", [a["host"] for a in other.get(d["id"])["addresses"]]) # and it sees it + def test_another_servers_choice_of_headset_doesnt_move_this_one(self): + self.reg.sync_from_config(seed=False) + other = fd.Registry(self.dir / "devices.json") + mine, theirs = self.reg.by_alias("frame")["id"], self.reg.by_alias("frame-2")["id"] + self.reg.set_active(mine) + other.set_active(theirs) + self.assertEqual(self.reg.active(), mine) # after a refresh + self.reg.add_address(mine, "192.0.2.9") # and after a change reloads the file + self.assertEqual(self.reg.active(), mine) + self.assertEqual(fd.Registry(self.dir / "devices.json").active(), mine) # last to save: next start + class ConfigRewrite(Base): def test_hostname_user_and_port_change_only_inside_the_block(self): cfg = self.ssh / "config" diff --git a/tests/test_link.py b/tests/test_link.py index 6566a00..01e2618 100644 --- a/tests/test_link.py +++ b/tests/test_link.py @@ -348,8 +348,21 @@ class Connecting(unittest.TestCase): self.assertEqual(s["phase"], "connected", s["error"]) self.assertTrue(s["device"]["transient"]) # Where ~/.ssh/config sends it, pinned down for the connection (ssh's own known_hosts). - self.assertEqual(self.routes[-1], ("frame-bare", ["-o", "HostName=localhost", "-o", f"Port={self.port}", - "-o", "User=tester"])) + alias, opts = self.routes[-1] + self.assertEqual((alias, opts[2:]), ("frame-bare", ["-o", "HostName=localhost", "-o", f"Port={self.port}", + "-o", "User=tester"])) + self.assertEqual(opts[:2], ["-o", "ControlPath=" + fl.frame_host.control_path(fl.Link.control_tag(s["device"]))]) + + def test_each_headset_has_its_own_shared_connection(self): + """ssh's %C hashes only address, user and port: two headsets at one address + (one moved) must still never share a ControlMaster.""" + a, b = (dict(fl.Link.bare("x"), id=i, transient=False, user="steamos", port=22) for i in ("aaaa1111", "bbbb2222")) + pa, pb = (next(o for o in self.link.host_opts(d, "192.0.2.5") if o.startswith("ControlPath=")) for d in (a, b)) + self.assertNotEqual(pa, pb) + self.assertIn("aaaa1111", pa) + long_alias = fl.Link.bare("x" * 64) + path = next(o for o in self.link.host_opts(long_alias, None) if o.startswith("ControlPath=")) + self.assertLess(len(path) - len("ControlPath=") - len("%C") + 40 + 17, 104) # fits a macOS socket path def test_a_bare_alias_keeps_its_pinned_route_for_reconnects_and_terminals(self): self.link.override = "frame-bare" diff --git a/ui/frame_devices.py b/ui/frame_devices.py index 063896f..146bcb9 100644 --- a/ui/frame_devices.py +++ b/ui/frame_devices.py @@ -461,6 +461,12 @@ class Registry: data.setdefault("networks", {}) data.setdefault("active", None) data["devices"] = [d for d in data["devices"] if self._sane(d)] + # The headset in use is this server's own choice: another server picking a + # different one mustn't move commands (an install, say) under it. The file's + # choice is only where a server starts. + mine = self.data.get("active") + if mine and any(d["id"] == mine for d in data["devices"]): + data["active"] = mine self.data = data @staticmethod diff --git a/ui/frame_host.py b/ui/frame_host.py index c2ecff4..568e07f 100644 --- a/ui/frame_host.py +++ b/ui/frame_host.py @@ -56,12 +56,15 @@ def cache_dir(*parts): return base.joinpath(*parts) -def control_path(): +def control_path(tag="x"): """ssh ControlPath for the shared connection, or None where it isn't supported. + `tag` names the headset: ssh's %C hashes only the address, user and port, so two + headsets reached at the same address (one of them moved) would otherwise share a + connection, and one's commands would run on the other. /tmp, not $TMPDIR: macOS's per-user temp path overflows the unix socket path limit. """ - return f"/tmp/frame-ui-{os.getuid()}-%C" if MUX else None + return f"/tmp/frame-ui-{os.getuid()}-{tag}-%C" if MUX else None def which(name, *extra): diff --git a/ui/frame_link.py b/ui/frame_link.py index d8e0813..794938e 100644 --- a/ui/frame_link.py +++ b/ui/frame_link.py @@ -26,6 +26,7 @@ when the page asks. Python stdlib only. Runs on this computer, never on the Frame. """ import copy +import hashlib import ipaddress import queue import re @@ -364,17 +365,27 @@ class Link: return {"id": f"alias-{alias}", "name": alias, "alias": alias, "user": None, "port": None, "addresses": [], "transient": True, "identity_files": []} + @staticmethod + def control_tag(device): + """A short, path-safe name for the headset's ControlPath: its id, or for a bare + alias a hash of it (an alias can be too long for a socket path).""" + if device.get("transient"): + return "a" + hashlib.sha1(device["alias"].encode()).hexdigest()[:8] + return device["id"] + def host_opts(self, device, host): """What every ssh command adds to reach DEVICE at HOST.""" if device.get("none"): return ["-o", "HostName=no-headset.invalid"] # fails at once, with ssh's own "can't resolve" + # Each headset its own shared connection (see frame_host.control_path). + mux = ["-o", f"ControlPath={frame_host.control_path(self.control_tag(device))}"] if self.control else [] if device.get("transient"): - return list(device.get("frozen") or []) # what ~/.ssh/config said when it was routed + return [*mux, *(device.get("frozen") or [])] # what ~/.ssh/config said when it was routed if not host: # a headset with no addresses: reach nothing, not whatever ~/.ssh/config says return ["-o", "HostName=no-address.invalid"] # StrictHostKeyChecking=yes: whatever ~/.ssh/config says for Host *, every command # checks the headset's pinned key (only the connector's first handshake may save one). - return ["-o", "StrictHostKeyChecking=yes", "-o", f"HostName={frame_devices.ssh_host(host)}", + return [*mux, "-o", "StrictHostKeyChecking=yes", "-o", f"HostName={frame_devices.ssh_host(host)}", "-o", f"HostKeyAlias={frame_devices.host_key_alias(device['id'])}", "-o", f"UserKnownHostsFile={frame_devices.known_hosts_opt(device['id'])}", "-o", "HashKnownHosts=no", "-o", f"User={device['user']}", "-o", f"Port={device['port']}"] diff --git a/ui/server.py b/ui/server.py index 49e0bf6..19846f6 100755 --- a/ui/server.py +++ b/ui/server.py @@ -66,7 +66,8 @@ if not re.fullmatch(r"[A-Za-z0-9][A-Za-z0-9._-]*", FRAME): # Reuse one SSH connection for the frequent status/screenshot calls, where ssh # supports it (not on Windows: there every command connects on its own). CONTROL = None if LOCAL else frame_host.control_path() -MUX = ["ssh", "-o", "BatchMode=yes", *(["-o", f"ControlPath={CONTROL}"] if CONTROL else [])] +# The ControlPath itself is per headset: the connector puts it in HOST_OPTS. +MUX = ["ssh", "-o", "BatchMode=yes"] MUX_BASE = list(MUX) # Commands use the master when it's up and connect directly when it isn't. SSH_TAIL = [*(["-o", "ControlMaster=no"] if CONTROL else []), "-o", "ConnectTimeout=5"]