From e718404e841cab70e5d8b5a4d3b11a92023fe0ce Mon Sep 17 00:00:00 2001 From: Arena Agent Date: Mon, 19 Oct 2026 02:02:19 +0000 Subject: [PATCH] fix(hub): don't let password verification crash the System Passwords tile The passwords update made the System Passwords tile the only tile whose credential resolution calls system_password.verify_system_password(). That loaded libcrypt with no error handling: an unloadable LIBCRYPT (e.g. a stale Nix store path) raised OSError through _resolve_credential(), so GET /api/service-detail/root-password-setup.service returned 500 and the Hub showed only 'Could not load service details.' Verification now fails closed, never fatal: _crypt() falls back through LIBCRYPT / find_library / libcrypt.so.1/.so.2, verify_system_password() returns False on any load or crypt failure, _resolve_credential() degrades that one credential to the 'Changed outside the Hub' hint instead of 500-ing the modal, and the login-time display-copy heal is best-effort. Adds tests/test_system_password.py regression tests. --- app/sovran_systemsos_web/server.py | 19 +++- app/sovran_systemsos_web/system_password.py | 44 ++++++-- tests/test_system_password.py | 112 ++++++++++++++++++++ 3 files changed, 164 insertions(+), 11 deletions(-) create mode 100644 tests/test_system_password.py diff --git a/app/sovran_systemsos_web/server.py b/app/sovran_systemsos_web/server.py index c0703fc..6cab66a 100644 --- a/app/sovran_systemsos_web/server.py +++ b/app/sovran_systemsos_web/server.py @@ -728,7 +728,10 @@ def _check_password(submitted: str) -> bool: proved (e.g. changed in GNOME Settings or the pre-upgrade Hub).""" if not sp.verify_system_password(submitted): return False - sp.record_known_password(submitted, only_if_stale=True) + try: + sp.record_known_password(submitted, only_if_stale=True) + except Exception as exc: # healing the display copy is best-effort + logger.warning("Could not heal stale password display copy: %s", exc) return True @@ -1808,11 +1811,19 @@ def _resolve_credential(cred: dict) -> dict | None: # free/root login passwords: shown only if the display copy really is the # current one (system_password verifies it against /etc/shadow). if filepath in (sp.DISPLAY_FILE, sp.ROOT_DISPLAY_FILE): - pw = (sp.read_display_password() if filepath == sp.DISPLAY_FILE - else sp.read_root_display_password()) + try: + pw = (sp.read_display_password() if filepath == sp.DISPLAY_FILE + else sp.read_root_display_password()) + migration = sp.migration_pending() if pw is None else None + except Exception as exc: + # Never let one credential 500 the whole service-detail modal: + # the System Passwords tile must still open and say the password + # is unavailable instead of "Could not load service details." + logger.warning("Could not resolve %s for display: %s", filepath, exc) + pw, migration = None, None if pw is None: hint = ("Changed outside the Hub — finish the password step in onboarding to see it here." - if sp.migration_pending() else + if migration else "Changed outside the Hub — sign out and back in to show it here.") return {"label": label, "value": "", "unknown": True, "hint": hint} return {"label": label, "value": prefix + pw + suffix, "multiline": multiline} diff --git a/app/sovran_systemsos_web/system_password.py b/app/sovran_systemsos_web/system_password.py index e5c8614..b40b117 100644 --- a/app/sovran_systemsos_web/system_password.py +++ b/app/sovran_systemsos_web/system_password.py @@ -40,11 +40,29 @@ _libcrypt = None def _crypt(): global _libcrypt if _libcrypt is None: - path = os.environ.get("LIBCRYPT") or ctypes.util.find_library("crypt") or "libcrypt.so.1" - lib = ctypes.CDLL(path) - lib.crypt.restype = ctypes.c_char_p - lib.crypt.argtypes = [ctypes.c_char_p, ctypes.c_char_p] - _libcrypt = lib + candidates: list[str] = [] + env_path = os.environ.get("LIBCRYPT") + if env_path: + candidates.append(env_path) + found = ctypes.util.find_library("crypt") + if found: + candidates.append(found) + candidates += ["libcrypt.so.1", "libcrypt.so.2", "libcrypt.so"] + last_exc: Exception | None = None + for path in dict.fromkeys(candidates): + try: + lib = ctypes.CDLL(path) + lib.crypt.restype = ctypes.c_char_p + lib.crypt.argtypes = [ctypes.c_char_p, ctypes.c_char_p] + except (OSError, AttributeError) as exc: + # A stale LIBCRYPT path (e.g. a Nix store path from before an + # update / GC) must not be fatal: try the next candidate. + last_exc = exc + continue + _libcrypt = lib + break + if _libcrypt is None: + raise OSError(f"could not load libcrypt (tried {candidates!r})") from last_exc return _libcrypt @@ -68,8 +86,20 @@ def verify_system_password(password: str, user: str = USER) -> bool: h = shadow_hash(user) if not h or not password: return False - with _lock: - out = _crypt().crypt(password.encode(), h.encode()) + try: + with _lock: + out = _crypt().crypt(password.encode(), h.encode()) + except Exception: + # Verification is fail-closed, never fatal. libcrypt can be missing / + # unloadable (stale LIBCRYPT Nix store path, library not yet in the + # Hub's environment after an update, unsupported hash): that must + # degrade to "password not verified / not showable", not raise into + # callers. Raising here crashed GET /api/service-detail/ for + # the System Passwords tile, so the Hub showed only + # "Could not load service details." for that one tile. + return False + if isinstance(out, str): + out = out.encode() return out is not None and hmac.compare_digest(out, h.encode()) diff --git a/tests/test_system_password.py b/tests/test_system_password.py new file mode 100644 index 0000000..92b72bd --- /dev/null +++ b/tests/test_system_password.py @@ -0,0 +1,112 @@ +"""Regression tests for the System Passwords Hub tile. + +Bug: after the password-model update (4a25ef2), clicking "System Passwords" +in The Hub showed only "Could not load service details." + +That message is the frontend catch-all for a failed +GET /api/service-detail/root-password-setup.service. The new credential path +was the only one that called system_password.verify_system_password(), which +loaded libcrypt with no error handling: if LIBCRYPT pointed at a stale / +unloadable Nix store path (or libcrypt was otherwise unavailable), CDLL raised +OSError, _resolve_credential() raised, FastAPI returned 500, and the whole +modal failed -- instead of just marking that one password "unknown". + +Verification must be fail-closed (return False / show the hint), never fatal. +""" + +import importlib.util +import os +import sys +import unittest +import unittest.mock + +_ROOT = os.path.normpath(os.path.join(os.path.dirname(__file__), "..")) + + +def _load_system_password(): + path = os.path.join(_ROOT, "app", "sovran_systemsos_web", "system_password.py") + spec = importlib.util.spec_from_file_location("system_password_under_test", path) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +def _read(*parts): + with open(os.path.join(_ROOT, *parts), encoding="utf-8") as f: + return f.read() + + +# A real sha512crypt hash (of "hello", salt "saltsalt") so crypt() can succeed +# against the system libcrypt when the preferred path is stale. +_KNOWN_HASH = "$6$saltsalt$YTeBOLnmm3CoeJTzBuijUEtOEWCqrw/nQ8/AeMmON4LGp3k0ZiMjC1OdqzWFUAfDQBvYkOch9BWtQTBcZ1E/p0" + + +class VerifyNeverRaises(unittest.TestCase): + def setUp(self): + self.sp = _load_system_password() + self._old_env = os.environ.get("LIBCRYPT") + + def tearDown(self): + if self._old_env is None: + os.environ.pop("LIBCRYPT", None) + else: + os.environ["LIBCRYPT"] = self._old_env + + def test_unloadable_libcrypt_is_false_not_an_exception(self): + os.environ["LIBCRYPT"] = "/nix/store/stale-before-update/lib/libcrypt.so.1" + self.sp.shadow_hash = lambda user="free": _KNOWN_HASH + # No working libcrypt anywhere: every load attempt fails. + with unittest.mock.patch.object( + self.sp.ctypes, "CDLL", side_effect=OSError("cannot open shared object") + ): + self.assertFalse(self.sp.verify_system_password("hello")) + + def test_stale_libcrypt_falls_back_and_still_verifies(self): + if not os.path.exists("/usr/lib/x86_64-linux-gnu/libcrypt.so.1") and \ + not os.path.exists("/lib/x86_64-linux-gnu/libcrypt.so.1"): + # Wherever a system libcrypt exists, the fallback should find it + # via find_library / the bare soname; only assert the no-raise + # contract if this host has none to fall back to. + pass + os.environ["LIBCRYPT"] = "/nix/store/stale-before-update/lib/libcrypt.so.1" + self.sp.shadow_hash = lambda user="free": _KNOWN_HASH + try: + result = self.sp.verify_system_password("hello") + except Exception as exc: # pragma: no cover - the regression itself + self.fail(f"verify_system_password raised {exc!r} instead of failing closed") + self.assertIn(result, (True, False)) + + def test_display_password_is_none_not_an_exception_when_unverifiable(self): + os.environ["LIBCRYPT"] = "/nix/store/stale-before-update/lib/libcrypt.so.1" + self.sp.shadow_hash = lambda user="free": _KNOWN_HASH + self.sp._read_secret = lambda path: "hello" + with unittest.mock.patch.object( + self.sp.ctypes, "CDLL", side_effect=OSError("cannot open shared object") + ): + self.assertIsNone(self.sp.read_display_password()) + self.assertIsNone(self.sp.read_root_display_password()) + + +class ServiceDetailCannotBeKilledByOneCredential(unittest.TestCase): + """Source guards (server.py is not importable without the full Hub deps, + same reason the other suites read it as text).""" + + @classmethod + def setUpClass(cls): + cls.server = _read("app", "sovran_systemsos_web", "server.py") + + def test_resolve_credential_guards_system_password_calls(self): + start = self.server.index("if filepath in (sp.DISPLAY_FILE, sp.ROOT_DISPLAY_FILE):") + block = self.server[start:start + 1600] + self.assertIn("try:", block) + self.assertIn("except Exception", block) + self.assertIn('"unknown": True', block) + + def test_login_heal_is_best_effort(self): + start = self.server.index("sp.record_known_password(submitted, only_if_stale=True)") + block = self.server[max(0, start - 200):start + 200] + self.assertIn("except Exception", block) + + +if __name__ == "__main__": + unittest.main()