From 07f44f908272672879d0a8be3e1066ccdd506e93 Mon Sep 17 00:00:00 2001 From: saphid <4596216+saphid@users.noreply.github.com> Date: Mon, 5 Oct 2026 22:38:01 +1100 Subject: [PATCH] Windows: fix ssh config ACL, link-local IPv6, and setup under python -I - ~/.ssh/config writes swapped in a temp file that inherited the .ssh folder's ACL; Windows' OpenSSH refuses one granting another account (even a deleted one) more than read: "Bad owner or permissions". Writes now give the file an owner-only ACL (frame_host.make_private), and the server repairs a refused config once per run and retries. - frame_link.probe named a link-local IPv6 zone with if_indextoname, which on Windows is "ethernet_32769"; Windows' ssh can't resolve that, so a headset found at fe80:: showed as "can't find the Frame". Use the zone number there. - frame_connect.py imports frame_host (since #60), but the app runs it with python -I, which leaves its folder off sys.path: Set Up Connection exited with ModuleNotFoundError. Add the folder, as server.py does. Verified on a Windows 11 VM against OpenSSH_for_Windows 9.5p2: the old write reproduces the reported error with an orphan SID's Modify ACE; the new write, repair and server retry all leave a config ssh accepts; ssh to %ethernet_32769 fails to resolve while %5 connects. Co-Authored-By: Claude Opus 5.5 (1M context) --- tests/test_windows_fixes.py | 162 ++++++++++++++++++++++++++++++++++++ ui/frame_connect.py | 9 +- ui/frame_devices.py | 34 +++++++- ui/frame_host.py | 36 ++++++++ ui/frame_link.py | 3 +- ui/server.py | 26 ++++++ 6 files changed, 264 insertions(+), 6 deletions(-) create mode 100644 tests/test_windows_fixes.py diff --git a/tests/test_windows_fixes.py b/tests/test_windows_fixes.py new file mode 100644 index 0000000..1903630 --- /dev/null +++ b/tests/test_windows_fixes.py @@ -0,0 +1,162 @@ +"""Windows-only paths, faked on any OS: ~/.ssh/config's ACL and link-local IPv6 zones. + +Run: python3 -m unittest discover -s tests +""" +import sandbox # noqa: F401 (first: keeps tests off real data and services) +import os +import shutil +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path +from unittest import mock + +ROOT = Path(__file__).resolve().parent.parent +sys.path.insert(0, str(ROOT / "ui")) + +import frame_host # noqa: E402 +import frame_devices as fd # noqa: E402 + +REFUSED = ("Bad permissions. Try removing permissions for user: UNKNOWN\\UNKNOWN (S-1-5-21-1-2-3-1000) " + "on file C:/Users/bob/.ssh/config.\r\nBad owner or permissions on C:\\Users\\bob/.ssh/config\r\n") + + +def ran(*results): + """subprocess.run stand-in answering whoami, then icacls.""" + calls = [] + + def run(argv, **kw): + calls.append(argv) + return results[len(calls) - 1] + return run, calls + + +class MakePrivate(unittest.TestCase): + def test_windows_sets_owner_only_acl_by_sid(self): + run, calls = ran(subprocess.CompletedProcess([], 0, '"desktop\\björn","S-1-5-21-9-8-7-1001"\r\n'.encode("cp850")), + subprocess.CompletedProcess([], 0)) + with mock.patch.object(frame_host, "WINDOWS", True), mock.patch.object(frame_host.subprocess, "run", run): + self.assertTrue(frame_host.make_private(Path("C:/x/config"))) + self.assertEqual(calls[1][1:], [str(Path("C:/x/config")), "/inheritance:r", "/grant:r", + "*S-1-5-21-9-8-7-1001:F", "*S-1-5-18:F", "*S-1-5-32-544:F"]) + + def test_windows_falls_back_to_username_and_reports_failure(self): + run, calls = ran(subprocess.CompletedProcess([], 1, b""), subprocess.CompletedProcess([], 5)) + with mock.patch.object(frame_host, "WINDOWS", True), mock.patch.object(frame_host.subprocess, "run", run), \ + mock.patch.dict(os.environ, {"USERNAME": "bob"}): + self.assertFalse(frame_host.make_private(Path("config"))) + self.assertIn("bob:F", calls[1]) + + @unittest.skipIf(os.name == "nt", "POSIX modes") + def test_posix_chmods_600(self): + with tempfile.NamedTemporaryFile() as f: + os.chmod(f.name, 0o644) + self.assertTrue(frame_host.make_private(f.name)) + self.assertEqual(os.stat(f.name).st_mode & 0o777, 0o600) + + +class ConfigWrites(unittest.TestCase): + def setUp(self): + self.ssh = Path(tempfile.mkdtemp(prefix="frame-acl-")) + self.addCleanup(shutil.rmtree, self.ssh, ignore_errors=True) + self.config = self.ssh / "config" + + def test_devices_and_connect_writes_make_the_file_private(self): + import frame_connect as fc + self.config.write_text("Host other\n User me\n", encoding="utf-8") + with mock.patch.object(frame_host, "make_private", return_value=True) as private, \ + mock.patch.object(fc, "SSH_DIR", self.ssh), mock.patch.object(fc, "CONFIG", self.config): + fc.write_config("10.0.0.5") + self.assertTrue(fd.repair_permissions(self.config)) + fd.rewrite_block("frame", path=self.config, user="deck") + self.assertEqual(private.call_count, 3) + self.assertIn("User deck", self.config.read_text(encoding="utf-8")) + self.assertTrue(all(Path(c.args[0]).parent == self.ssh for c in private.call_args_list)) + self.assertIn("Host other", self.config.read_text(encoding="utf-8")) + + def test_setup_runs_isolated_as_the_app_starts_it(self): + r = subprocess.run([sys.executable, "-I", "-B", str(ROOT / "ui" / "frame_connect.py"), "--help"], + capture_output=True, text=True, stdin=subprocess.DEVNULL, timeout=30) + self.assertNotIn("ModuleNotFoundError", r.stderr) + self.assertIn("frame_connect.py", r.stdout + r.stderr) + + def test_repair_keeps_the_bytes_and_skips_a_missing_file(self): + self.assertFalse(fd.repair_permissions(self.config)) + data = "# caf\xe9 (ANSI, not UTF-8)\r\nHost a\r\n".encode("cp1252") + self.config.write_bytes(data) + with mock.patch.object(frame_host, "make_private", return_value=True): + self.assertTrue(fd.repair_permissions(self.config)) + self.assertEqual(self.config.read_bytes(), data) + + def test_repair_fails_without_the_acl_and_leaves_the_file(self): + self.config.write_bytes(b"Host a\n") + before = self.config.stat().st_ino + with mock.patch.object(frame_host, "make_private", return_value=False): + self.assertFalse(fd.repair_permissions(self.config)) + self.assertEqual((self.config.read_bytes(), self.config.stat().st_ino), (b"Host a\n", before)) + self.assertEqual(sorted(f.name for f in self.ssh.iterdir()), ["config", fd.LOCK_NAME]) + + +class ServerRepair(unittest.TestCase): + @classmethod + def setUpClass(cls): + import server + cls.server = server + + def setUp(self): + self.ssh = Path(tempfile.mkdtemp(prefix="frame-acl-")) + self.addCleanup(shutil.rmtree, self.ssh, ignore_errors=True) + (self.ssh / "config").write_text("Host a\n", encoding="utf-8") + patches = [mock.patch.dict(os.environ, {"FRAME_CONTROL_SSH_DIR": str(self.ssh)}), + mock.patch.object(frame_host, "WINDOWS", True), + mock.patch.object(self.server, "_config_repaired", False)] + for p in patches: + p.start() + self.addCleanup(p.stop) + + def test_repairs_the_refused_config_once(self): + with mock.patch.object(fd, "repair_permissions", return_value=True) as repair: + self.assertTrue(self.server.repair_ssh_config(REFUSED)) + self.assertFalse(self.server.repair_ssh_config(REFUSED)) + repair.assert_called_once() + + def test_leaves_other_files_and_errors_alone(self): + key = REFUSED.replace(".ssh/config", ".ssh/id_ed25519_frame") + with mock.patch.object(fd, "repair_permissions") as repair: + self.assertFalse(self.server.repair_ssh_config(key)) + self.assertFalse(self.server.repair_ssh_config("ssh: connect to host frame port 22: timed out")) + with mock.patch.object(frame_host, "WINDOWS", False): + self.assertFalse(self.server.repair_ssh_config(REFUSED)) + repair.assert_not_called() + + def test_ssh_retries_after_repairing(self): + results = iter([subprocess.CompletedProcess([], 255, "", REFUSED), subprocess.CompletedProcess([], 0, "ok", "")]) + with mock.patch.object(frame_host, "run_ssh", lambda *a, **k: next(results)), \ + mock.patch.object(fd, "repair_permissions", return_value=True), \ + mock.patch.object(self.server, "LINK", None): + self.assertEqual(self.server.ssh("true"), "ok") + + +class LinkLocalZone(unittest.TestCase): + """A .local name answering on fe80::: Windows' ssh needs fe80::1%12, not %wireless_32768.""" + + def probe(self, windows): + import frame_link as fl + info = [(fl.socket.AF_INET6, fl.socket.SOCK_STREAM, 6, "", ("fe80::1", 22, 0, 12))] + sock = mock.MagicMock() + with mock.patch.object(frame_host, "WINDOWS", windows), \ + mock.patch.object(fl.socket, "getaddrinfo", return_value=info), \ + mock.patch.object(fl.socket, "socket", return_value=sock), \ + mock.patch.object(fl.socket, "if_indextoname", return_value="wireless_32768", create=True): + return fl.probe("frame.local", 22)["ip"] + + def test_windows_uses_the_numeric_zone(self): + self.assertEqual(self.probe(True), "fe80::1%12") + + def test_elsewhere_uses_the_interface_name(self): + self.assertEqual(self.probe(False), "fe80::1%wireless_32768") + + +if __name__ == "__main__": + unittest.main() diff --git a/ui/frame_connect.py b/ui/frame_connect.py index c0cefa4..1b66a9b 100644 --- a/ui/frame_connect.py +++ b/ui/frame_connect.py @@ -24,7 +24,9 @@ import urllib.error import urllib.request from pathlib import Path -import frame_host +# The app runs this with python -I, which leaves the script's folder off sys.path. +sys.path.insert(0, str(Path(__file__).resolve().parent)) +import frame_host # noqa: E402 FRAME_USER = os.environ.get("FRAME_USER", "steamos") USER_FROM_ENV = "FRAME_USER" in os.environ @@ -332,8 +334,9 @@ def _write_config(host, port, user): block = config_block(host, port, user) tmp = CONFIG.with_name(f"config.frame-control.{os.getpid()}.tmp") tmp.write_text("\n".join(block + kept) + "\n", encoding="utf-8") - if os.name != "nt": - tmp.chmod(0o600) + if not frame_host.make_private(tmp): + say(" couldn't make ~/.ssh/config private; if ssh says \"Bad owner or permissions\", " + "Frame Control repairs it when it next connects") # On Windows a running ssh.exe (Frame Control's own, say) keeps the config open # and locked, so the swap can fail for a moment; keep trying for a while. for attempt in range(60): diff --git a/ui/frame_devices.py b/ui/frame_devices.py index fb2f29a..4be298d 100644 --- a/ui/frame_devices.py +++ b/ui/frame_devices.py @@ -241,8 +241,7 @@ def _write_config(path, text, expected): try: with os.fdopen(fd_, "w", encoding="utf-8") as fh: fh.write(text) - if not frame_host.WINDOWS: - tmp.chmod(0o600) + frame_host.make_private(tmp) # best effort: an edit still beats none (repair_permissions insists) for attempt in range(20): # Windows: a running ssh.exe can hold the file for a moment if read_config(path) != expected: return False @@ -271,6 +270,37 @@ def _edit_config(path, change): raise OSError(f"{path} kept changing while Frame Control tried to update it") +def repair_permissions(path=None): + """Give ~/.ssh/config make_private's ACL by swapping in a byte-for-byte copy: for a + file Windows' OpenSSH refuses ("Bad owner or permissions"). -> True only if the copy + got that ACL and replaced the file.""" + path = Path(path or ssh_config()) + with _config_lock, file_lock(path.with_name(LOCK_NAME)): + try: + data = path.read_bytes() + except OSError: + return False + fd_, tmp = tempfile.mkstemp(prefix="config.frame-control.", dir=str(path.parent)) + tmp = Path(tmp) + try: + with os.fdopen(fd_, "wb") as fh: + fh.write(data) + if not frame_host.make_private(tmp): + return False + for attempt in range(20): # a running ssh.exe can hold the file for a moment + if path.read_bytes() != data: + return False + try: + os.replace(tmp, path) + return True + except PermissionError: + time.sleep(0.25) + return False + finally: + if tmp.exists(): + tmp.unlink() + + def rewrite_block(alias, path=None, hostname=None, user=None, port=None, expect=None): """Change HostName, User or Port inside ALIAS's managed block, leaving the rest of the file alone. -> True if the file changed. Does nothing if there's no such block, or diff --git a/ui/frame_host.py b/ui/frame_host.py index 4fc1d10..fac720e 100644 --- a/ui/frame_host.py +++ b/ui/frame_host.py @@ -278,6 +278,42 @@ def clipboard_text(): raise HostError("Can't read the clipboard") +# What Windows' OpenSSH says when it refuses ~/.ssh/config (or a key) for its ACL. +BAD_PERMISSIONS = "Bad owner or permissions on " + + +def make_private(path): + """Leave only this user able to open PATH, as ssh insists for ~/.ssh/config. + Windows: an ACL of just this user, SYSTEM and Administrators, inherited nothing. + A file written into ~/.ssh otherwise takes the folder's ACL, and Windows' OpenSSH + refuses it if that grants anyone else, even an account deleted long ago + ("Bad owner or permissions"). Best effort: -> False if it couldn't.""" + if not WINDOWS: + try: + os.chmod(path, 0o600) + return True + except OSError: + return False + me = os.environ.get("USERNAME", "") + try: # "desktop\me","S-1-5-21-..." + # Bytes: the account name is in the console's code page, the SID is ASCII. + out = subprocess.run(["whoami", "/user", "/fo", "csv", "/nh"], capture_output=True, + stdin=subprocess.DEVNULL, timeout=10).stdout + sid = out.decode("ascii", "replace").strip().rsplit(",", 1)[-1].strip('"') + if sid.startswith("S-1-"): + me = "*" + sid + except (OSError, subprocess.TimeoutExpired): + pass + if not me: + return False + try: + return subprocess.run(["icacls", str(path), "/inheritance:r", "/grant:r", f"{me}:F", + "*S-1-5-18:F", "*S-1-5-32-544:F"], capture_output=True, + stdin=subprocess.DEVNULL, timeout=10).returncode == 0 + except (OSError, subprocess.TimeoutExpired): + return False + + def ssh_hostname(alias): """The real host name an ssh alias points at (`ssh -G`), for non-SSH clients like RDP.""" try: diff --git a/ui/frame_link.py b/ui/frame_link.py index d9fd551..ed97100 100644 --- a/ui/frame_link.py +++ b/ui/frame_link.py @@ -108,7 +108,8 @@ def probe(host, port, timeout=PROBE_TIMEOUT, update=None): ip = addr[0] if family == socket.AF_INET6 and len(addr) > 3 and addr[3] and "%" not in ip: try: # a link-local IPv6 address only works with its interface - ip = f"{ip}%{socket.if_indextoname(addr[3])}" + # Windows' ssh takes only the number: its names ("wireless_32768") don't resolve. + ip = f"{ip}%{addr[3] if frame_host.WINDOWS else socket.if_indextoname(addr[3])}" except (OSError, AttributeError): pass left = deadline - now() diff --git a/ui/server.py b/ui/server.py index b4e25e7..129835b 100755 --- a/ui/server.py +++ b/ui/server.py @@ -339,6 +339,8 @@ def ssh(remote, *, stdin=None, timeout=30, text=True): raise Failure(f"Timed out talking to {FRAME}") if r.returncode != 0: err = (r.stderr or r.stdout) if text else (r.stderr or r.stdout).decode(errors="replace") + if r.returncode == 255 and repair_ssh_config(err): + return ssh(remote, stdin=stdin, timeout=timeout, text=text) if r.returncode == 255 and LINK and unreachable(err): LINK.lost(err, route_gen) # ssh itself failed: the connector reconnects failure = Failure(strip_ansi(err).strip() or f"ssh exited {r.returncode}") @@ -347,6 +349,30 @@ def ssh(remote, *, stdin=None, timeout=30, text=True): return r.stdout +_config_repaired = False + + +def repair_ssh_config(err): + """Windows' OpenSSH refused ~/.ssh/config for its ACL: give the file a private one, + once per run. -> True if it did, so the command is worth retrying.""" + global _config_repaired + if _config_repaired or not frame_host.WINDOWS or frame_host.BAD_PERMISSIONS not in err: + return False + # ssh doubles the backslashes: "C:\\Users\\me/.ssh/config" + named = re.sub(r"[\\/]+", "/", err.split(frame_host.BAD_PERMISSIONS, 1)[1].splitlines()[0].strip()) + config = frame_devices.ssh_config() + if not named.lower().endswith("/" + config.name.lower()): + return False # a key or another file: not ours to rewrite + _config_repaired = True + try: + if frame_devices.repair_permissions(config): + print(f"Gave {config} a private ACL: ssh refused it ({named})", file=sys.stderr) + return True + except OSError as e: + print(f"Couldn't repair {config}'s permissions: {e}", file=sys.stderr) + return False + + def strip_ansi(s): return re.sub(r"\x1b\[[0-9;?]*[A-Za-z]|\r", "", s)