Merge remote-tracking branch 'origin/main' into fix/windows-ssh-config-acl

Remote desktop from Windows: sign in as steamos, and say why when the Frame doesn't answer

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()

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):

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

ui/frame_host.py

@@ -284,6 +284,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:

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()

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)