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.
This commit is contained in:
1 parent
07566c6a28
commit
e718404e84
3 files changed
+164
-11
No files matched your search
@@ -728,7 +728,10 @@ def _check_password(submitted: str) -> bool:
|
|||||||
proved (e.g. changed in GNOME Settings or the pre-upgrade Hub)."""
|
proved (e.g. changed in GNOME Settings or the pre-upgrade Hub)."""
|
||||||
if not sp.verify_system_password(submitted):
|
if not sp.verify_system_password(submitted):
|
||||||
return False
|
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
|
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
|
# free/root login passwords: shown only if the display copy really is the
|
||||||
# current one (system_password verifies it against /etc/shadow).
|
# current one (system_password verifies it against /etc/shadow).
|
||||||
if filepath in (sp.DISPLAY_FILE, sp.ROOT_DISPLAY_FILE):
|
if filepath in (sp.DISPLAY_FILE, sp.ROOT_DISPLAY_FILE):
|
||||||
pw = (sp.read_display_password() if filepath == sp.DISPLAY_FILE
|
try:
|
||||||
else sp.read_root_display_password())
|
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:
|
if pw is None:
|
||||||
hint = ("Changed outside the Hub — finish the password step in onboarding to see it here."
|
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.")
|
"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": "", "unknown": True, "hint": hint}
|
||||||
return {"label": label, "value": prefix + pw + suffix, "multiline": multiline}
|
return {"label": label, "value": prefix + pw + suffix, "multiline": multiline}
|
||||||
|
|||||||
@@ -40,11 +40,29 @@ _libcrypt = None
|
|||||||
def _crypt():
|
def _crypt():
|
||||||
global _libcrypt
|
global _libcrypt
|
||||||
if _libcrypt is None:
|
if _libcrypt is None:
|
||||||
path = os.environ.get("LIBCRYPT") or ctypes.util.find_library("crypt") or "libcrypt.so.1"
|
candidates: list[str] = []
|
||||||
lib = ctypes.CDLL(path)
|
env_path = os.environ.get("LIBCRYPT")
|
||||||
lib.crypt.restype = ctypes.c_char_p
|
if env_path:
|
||||||
lib.crypt.argtypes = [ctypes.c_char_p, ctypes.c_char_p]
|
candidates.append(env_path)
|
||||||
_libcrypt = lib
|
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
|
return _libcrypt
|
||||||
|
|
||||||
|
|
||||||
@@ -68,8 +86,20 @@ def verify_system_password(password: str, user: str = USER) -> bool:
|
|||||||
h = shadow_hash(user)
|
h = shadow_hash(user)
|
||||||
if not h or not password:
|
if not h or not password:
|
||||||
return False
|
return False
|
||||||
with _lock:
|
try:
|
||||||
out = _crypt().crypt(password.encode(), h.encode())
|
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/<unit> 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())
|
return out is not None and hmac.compare_digest(out, h.encode())
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -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()
|
||||||
Reference in new issue
Block a user