From 5874fe33f6cfbf6d0fad207ecab21d6ba24baf41 Mon Sep 17 00:00:00 2001 From: saphid <4596216+saphid@users.noreply.github.com> Date: Mon, 28 Sep 2026 23:31:22 +1000 Subject: [PATCH] Devices: fixes from review round 13 - Every command to a set-up headset checks its pinned key (StrictHostKeyChecking=yes, whatever ~/.ssh/config says); only the connector's first handshake may save one. - A reconnect during an install keeps the whole route it started with, also when a bare alias is set up meanwhile. - Removing the headset FRAME_ALIAS named doesn't bring it back as a bare alias. Co-Authored-By: Claude Opus 5.5 (1M context) --- tests/test_link.py | 9 +++++++++ ui/frame_link.py | 24 ++++++++++++++++-------- 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/tests/test_link.py b/tests/test_link.py index bcb34aa..7fff303 100644 --- a/tests/test_link.py +++ b/tests/test_link.py @@ -169,6 +169,9 @@ class Connecting(unittest.TestCase): self.assertIn(f"Port={self.port}", opts) master = [c for c in self.calls() if "ControlMaster=yes" in c][-1] self.assertIn("StrictHostKeyChecking=accept-new", master) # first connection: nothing pinned yet + # ssh takes an option's first value: accept-new must come before the commands' own "yes". + self.assertLess(master.index("StrictHostKeyChecking=accept-new"), master.index("StrictHostKeyChecking=yes")) + self.assertIn("StrictHostKeyChecking=yes", opts) # every other command checks the pinned key self.assertTrue(fd.known_hosts(d["id"]).parent.is_dir()) # where ssh saves the key it accepts # It learned: 127.0.0.1 works on this network. learned = {a["host"]: a for a in self.reg.get(d["id"])["addresses"]} @@ -263,6 +266,12 @@ class Connecting(unittest.TestCase): self.assertEqual(self.link.active_device()["alias"], "frame-bare") self.assertEqual(self.routes[-1], ("frame-bare", [])) + def test_removing_the_headset_frame_alias_named_doesnt_bring_it_back_bare(self): + d = self.device("localhost") + self.link.override = self.link.session_alias = "frame-t" + fl.devices_action(self.link, {"action": "remove", "id": d["id"], "config": True}, None) + self.assertNotIn("alias-frame-t", [x["id"] for x in fl.devices_view(self.link)["devices"]]) + def test_setup_changing_the_login_waits_for_installs(self): d = self.device("localhost") self.hosts({"localhost": "ok"}) diff --git a/ui/frame_link.py b/ui/frame_link.py index efb17af..d804fcf 100644 --- a/ui/frame_link.py +++ b/ui/frame_link.py @@ -182,6 +182,7 @@ class Link: self.gen = 0 # bumped when the headset or its login changes: older attempts are void self.route_lock = threading.Lock() self.routed = None # the device id every ssh command points at + self.routed_device = None # ---- publishing ---- def publish(self, **fields): @@ -352,7 +353,9 @@ class Link: return [] if not host: # a headset with no addresses: reach nothing, not whatever ~/.ssh/config says return ["-o", "HostName=no-address.invalid"] - return ["-o", f"HostName={frame_devices.ssh_host(host)}", + # 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)}", "-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']}"] @@ -439,11 +442,12 @@ class Link: with self.work_lock, self.route_lock: gen = self.attempt_gen = self.gen device = self.active_device() - if self.routed and self.routed[0] == device["id"] and self.route_key(device) != self.routed \ - and self.work(): - # Its login changed in ~/.ssh/config while an install runs: reconnect with the - # one it started with; the change applies once it's done (see watch_config). - device = dict(device, user=self.routed[1], port=self.routed[2]) + if self.routed and self.routed_device and device["alias"] == self.routed_device["alias"] \ + and self.route_key(device) != self.routed and self.work(): + # Set Up Connection changed this headset in ~/.ssh/config (its login, or a bare + # alias became a set-up headset) while an install runs: reconnect as it + # started; the change applies once it's done (see watch_config). + device = self.routed_device self.deferred = True if self.route_key(device) != self.routed: # Another headset, or a new user or port: nothing may go on using the old @@ -451,6 +455,7 @@ class Link: self.alias, self.opts = device["alias"], self.first_route(device) self.apply(self.alias, self.opts) self.routed = self.route_key(device) + self.routed_device = device with self.cond: self.state.update(phase="connecting", reason=why, device=self.public_device(device), via=None, error=None, retry_at=None, attempt=self.state["attempt"] + 1, started=now(), @@ -736,10 +741,11 @@ class Link: pins.mkdir(**({} if frame_host.WINDOWS else {"mode": 0o700}), parents=True, exist_ok=True) if self.control: # No ConnectTimeout: with it, OpenSSH's master takes ~5s to open its socket. - argv = [*self.mux_base, *opts, *extra, "-v", "-o", "ControlMaster=yes", "-o", "ServerAliveInterval=5", + # `extra` first: ssh takes the first value of an option, and it may say accept-new. + argv = [*self.mux_base, *extra, *opts, "-v", "-o", "ControlMaster=yes", "-o", "ServerAliveInterval=5", "-o", "ServerAliveCountMax=2", "-N", alias] else: - argv = [*self.mux_base, *opts, *extra, "-v", "-o", "ConnectTimeout=10", alias, "true"] + argv = [*self.mux_base, *extra, *opts, "-v", "-o", "ConnectTimeout=10", alias, "true"] try: proc = subprocess.Popen(argv, stdin=subprocess.DEVNULL, stdout=subprocess.DEVNULL, stderr=subprocess.PIPE, **frame_host.DETACHED) @@ -977,6 +983,8 @@ def devices_action(link, body, open_setup, busy=lambda: 0): raise frame_devices.DeviceError("This is your only headset. To remove it completely, also remove its " "entry from ~/.ssh/config (the box below)") d = reg.remove_device(did) + if d["alias"] == link.session_alias: + link.session_alias = None # removed on purpose: not back as a bare alias if is_active: link.override = None link.invalidate()