diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 07a1cd8769..c100098ef5 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -7628,53 +7628,6 @@ def _desktop_macos_has_valid_real_signature(app: Path) -> bool: return False -def _desktop_macos_reset_keychain_safe_storage(app: Path) -> None: - """Delete the 'Hermes Safe Storage' keychain item after an ad-hoc re-sign. - - NOTE: this does NOT update the item's ACL — it deletes the item, which - permanently orphans every credential encrypted under it. It is only safe - on the LEGACY AD-HOC fallback path, where every rebuild produces a new - cdhash so the keychain ACL can never match and the alternative is a - recurring prompt. It MUST NOT be called on the stable signing path: there - the designated requirement is certificate-anchored and stable, so after - the first launch under the new identity the ACL already matches and - deleting the item only destroys credentials that were working fine. - - Electron's ``safeStorage`` stores its encryption key in a Keychain item - named `` Safe Storage`` (here: "Hermes Safe Storage"). macOS - ties each keychain item's Access Control List to the code signature of - the app that created it. When the self-updater rebuilds and re-signs the - bundle ad-hoc, the new cdhash no longer matches the stored ACL, so macOS - re-prompts on every launch. - - Deleting the item makes Electron recreate it with the correct ACL for the - newly-signed app on next launch. The trade-off: previously encrypted - tokens become unreadable (the user re-enters the gateway token once), but - the keychain prompt stops appearing on every launch. The durable fix is - a stable signing identity (``desktop.macos_signing_identity``), which - makes this path unreachable. - - If the item doesn't exist yet (first launch), this is a no-op. - - Best-effort: never raises. - """ - security = shutil.which("security") - if not security: - return - service_name = "Hermes Safe Storage" - # Chromium/Electron safeStorage uses " Key" as the account name. - account_name = "Hermes Key" - try: - result = subprocess.run( - [security, "delete-generic-password", "-s", service_name, "-a", account_name], - check=False, capture_output=True, text=True, - ) - if result.returncode == 0: - print(" → Reset Hermes Safe Storage keychain entry for new signing identity") - except Exception: - pass # Best-effort; the prompt is annoying but not fatal. - - def _desktop_macos_local_codesign( app: Path, *, desktop_dir: Path, identity: str = "-" ) -> bool: @@ -7834,14 +7787,37 @@ def _desktop_macos_relaunchable_fixup( ) print(f" (warning: stable macOS signing failed ({exc}); using legacy ad-hoc sign)") try: - subprocess.run([codesign, "--force", "--deep", "--sign", "-", str(app)], check=False) - # Legacy ad-hoc fallback only: every rebuild produces a new cdhash, so - # the keychain ACL can never match and the alternative is a recurring - # prompt. This deletes the item (credentials are re-entered once per - # update); it is intentionally NOT called on the stable identity path - # above, where the cert-anchored DR is stable and the deletion would - # destroy working credentials on every update. - _desktop_macos_reset_keychain_safe_storage(app) + # Legacy ad-hoc fallback: re-sign, but NEVER delete the safeStorage + # keychain item. Deleting it would permanently orphan every + # credential encrypted under it (gateway token, native OAuth access/ + # refresh tokens) — and this path is reached exactly when the + # entitlement-preserving signer failed, so there is no verified + # successor identity to hand the key to. The keychain prompt macOS + # shows instead is recoverable ("Always Allow" updates the item's ACL + # partition list and preserves the key); deletion is not. The real + # fix (proof-carrying rotation/migration) belongs in Electron, where + # safeStorage can read the old key. Tracked as follow-up. + result = subprocess.run( + [codesign, "--force", "--deep", "--sign", "-", str(app)], + check=False, capture_output=True, text=True, + ) + if result.returncode != 0: + print( + f" (warning: legacy ad-hoc re-sign failed (exit {result.returncode}); " + "leaving safeStorage keychain item untouched)" + ) + return False + verify = subprocess.run( + [codesign, "--verify", "--deep", "--strict", str(app)], + check=False, capture_output=True, text=True, + ) + if verify.returncode != 0: + print( + f" (warning: legacy ad-hoc re-sign did not pass strict verification; " + "leaving safeStorage keychain item untouched)" + ) + return False + print(" → macOS desktop re-signed (legacy ad-hoc); safeStorage keychain item left untouched") except Exception as exc: print(f" (warning: macOS relaunch fixup skipped: {exc})") return False diff --git a/tests/hermes_cli/test_gui_command.py b/tests/hermes_cli/test_gui_command.py index 14f57b3535..ab3dfd4dce 100644 --- a/tests/hermes_cli/test_gui_command.py +++ b/tests/hermes_cli/test_gui_command.py @@ -744,50 +744,133 @@ def test_cmd_gui_setup_tcc_identity_exits_before_build(tmp_path, monkeypatch): @pytest.mark.macos_only -def test_relaunchable_fixup_stable_identity_skips_keychain_reset(tmp_path, monkeypatch): +def test_relaunchable_fixup_stable_identity_never_touches_keychain(tmp_path, monkeypatch): """A successful stable-identity re-sign must NOT delete the safeStorage item. - Regression for review feedback on #90961: the keychain reset deletes the - item, permanently orphaning every safeStorage-backed credential (gateway - token, native OAuth access/refresh tokens — see electron/main.ts). On the - stable path the cert-anchored designated requirement is stable across - rebuilds, so after the first launch the keychain ACL already matches and - deleting the item would destroy working credentials on every update. - - ``macos_only``: the fixup no-ops on non-macOS (sys.platform guard), and - the subject is codesign against a real ``.app`` bundle layout. - """ - root = _make_desktop_tree(tmp_path) - desktop_dir = root / "apps" / "desktop" - monkeypatch.setattr(cli_main, "PROJECT_ROOT", root) - monkeypatch.delenv("CSC_LINK", raising=False) - monkeypatch.delenv("APPLE_SIGNING_IDENTITY", raising=False) - exe = _make_packaged_executable(root, monkeypatch) - app = exe.parents[2] - - resets: list[Path] = [] - monkeypatch.setattr(cli_main, "_desktop_macos_has_valid_real_signature", lambda a: False) - monkeypatch.setattr( - cli_main, "_desktop_macos_local_signing_identity", lambda: "Developer ID Application: Example" - ) - monkeypatch.setattr(cli_main, "_desktop_macos_local_codesign", lambda app, **kw: True) - monkeypatch.setattr( - cli_main, "_desktop_macos_reset_keychain_safe_storage", lambda app: resets.append(app) - ) - - assert cli_main._desktop_macos_relaunchable_fixup(desktop_dir) is True - assert resets == [] - - -@pytest.mark.macos_only -def test_relaunchable_fixup_legacy_adhoc_still_resets_keychain_item(tmp_path, monkeypatch): - """The legacy ad-hoc fallback keeps the keychain reset (documented trade-off). - - On the ad-hoc path every rebuild produces a new cdhash, so the keychain - ACL can never match and the alternative is a recurring prompt. The reset - is the documented trade-off there (re-enter credentials once per update); - the durable fix is a stable signing identity, which makes this path - unreachable. + Regression for review feedback on #90961: deleting the keychain item + permanently orphans every safeStorage-backed credential (gateway token, + native OAuth access/refresh tokens — see electron/main.ts). On the stable + path the cert-anchored designated requirement is stable across rebuilds, + so after the first launch the keychain ACL already matches and deleting + the item would destroy working credentials on every update. + + ``macos_only``: the fixup no-ops on non-macOS (sys.platform guard), and + the subject is codesign against a real ``.app`` bundle layout. + """ + root = _make_desktop_tree(tmp_path) + desktop_dir = root / "apps" / "desktop" + monkeypatch.setattr(cli_main, "PROJECT_ROOT", root) + monkeypatch.delenv("CSC_LINK", raising=False) + monkeypatch.delenv("APPLE_SIGNING_IDENTITY", raising=False) + exe = _make_packaged_executable(root, monkeypatch) + app = exe.parents[2] + + calls: list[list[str]] = [] + monkeypatch.setattr(cli_main, "_desktop_macos_has_valid_real_signature", lambda a: False) + monkeypatch.setattr( + cli_main, "_desktop_macos_local_signing_identity", lambda: "Developer ID Application: Example" + ) + monkeypatch.setattr(cli_main, "_desktop_macos_local_codesign", lambda app, **kw: True) + monkeypatch.setattr( + cli_main.subprocess, "run", + lambda cmd, **kw: calls.append(list(cmd)) or subprocess.CompletedProcess(cmd, 0), + ) + + assert cli_main._desktop_macos_relaunchable_fixup(desktop_dir) is True + assert not any("delete-generic-password" in c for c in calls) + + +@pytest.mark.macos_only +def test_relaunchable_fixup_default_noconfig_success_never_touches_keychain(tmp_path, monkeypatch): + """Default no-config path (identity == '-') must not delete the keychain item. + + Witness for the default ad-hoc success path: with no + ``desktop.macos_signing_identity`` configured, the fixup signs ad-hoc with + identifier-pinned requirements and must leave the safeStorage item alone. + + ``macos_only``: the fixup no-ops on non-macOS (sys.platform guard), and + the subject is codesign against a real ``.app`` bundle layout. + """ + root = _make_desktop_tree(tmp_path) + desktop_dir = root / "apps" / "desktop" + monkeypatch.setattr(cli_main, "PROJECT_ROOT", root) + monkeypatch.delenv("CSC_LINK", raising=False) + monkeypatch.delenv("APPLE_SIGNING_IDENTITY", raising=False) + exe = _make_packaged_executable(root, monkeypatch) + app = exe.parents[2] + + calls: list[list[str]] = [] + monkeypatch.setattr(cli_main, "_desktop_macos_has_valid_real_signature", lambda a: False) + monkeypatch.setattr(cli_main, "_desktop_macos_local_signing_identity", lambda: None) + monkeypatch.setattr(cli_main, "_desktop_macos_local_codesign", lambda app, **kw: True) + monkeypatch.setattr( + cli_main.subprocess, "run", + lambda cmd, **kw: calls.append(list(cmd)) or subprocess.CompletedProcess(cmd, 0), + ) + + assert cli_main._desktop_macos_relaunchable_fixup(desktop_dir) is True + assert not any("delete-generic-password" in c for c in calls) + + +@pytest.mark.macos_only +def test_relaunchable_fixup_legacy_adhoc_failure_never_touches_keychain(tmp_path, monkeypatch): + """A failed fallback re-sign must preserve the keychain item (no deletion). + + Regression for review feedback on #90961: the fallback previously deleted + the safeStorage item unconditionally, even when ``codesign`` failed + (``check=False`` result was ignored). A failed recovery can permanently + orphan gateway and native OAuth credentials without producing a verified + successor app/key identity. The fixup must check the codesign result, + run strict verification, and leave the keychain untouched on failure. + + ``macos_only``: the fixup no-ops on non-macOS (sys.platform guard), and + the subject is codesign against a real ``.app`` bundle layout. + """ + root = _make_desktop_tree(tmp_path) + desktop_dir = root / "apps" / "desktop" + monkeypatch.setattr(cli_main, "PROJECT_ROOT", root) + monkeypatch.delenv("CSC_LINK", raising=False) + monkeypatch.delenv("APPLE_SIGNING_IDENTITY", raising=False) + exe = _make_packaged_executable(root, monkeypatch) + app = exe.parents[2] + + calls: list[list[str]] = [] + + def fake_run(cmd, **kwargs): + calls.append(list(cmd)) + # First subprocess call is the xattr clear (exit 0); the deep sign + # fails with a non-zero exit. + if cmd[:2] == ["/usr/bin/codesign", "--force"]: + return subprocess.CompletedProcess(cmd, 1) + return subprocess.CompletedProcess(cmd, 0) + + monkeypatch.setattr( + cli_main.shutil, "which", lambda name: "/usr/bin/codesign" if name == "codesign" else None + ) + monkeypatch.setattr(cli_main.subprocess, "run", fake_run) + monkeypatch.setattr(cli_main, "_desktop_macos_has_valid_real_signature", lambda a: False) + monkeypatch.setattr(cli_main, "_desktop_macos_local_signing_identity", lambda: None) + + def boom(*a, **kw): + raise subprocess.CalledProcessError(1, ["codesign"]) + + monkeypatch.setattr(cli_main, "_desktop_macos_local_codesign", boom) + + assert cli_main._desktop_macos_relaunchable_fixup(desktop_dir) is False + assert ["/usr/bin/codesign", "--force", "--deep", "--sign", "-", str(app)] in calls + assert not any("--verify" in c for c in calls) + assert not any("delete-generic-password" in c for c in calls) + + +@pytest.mark.macos_only +def test_relaunchable_fixup_legacy_adhoc_success_still_verifies_and_never_deletes(tmp_path, monkeypatch): + """A successful fallback re-sign runs strict verification, no deletion. + + The legacy ad-hoc fallback signs, verifies with + ``codesign --verify --deep --strict``, and leaves the safeStorage keychain + item untouched. The keychain prompt macOS shows instead is recoverable + ("Always Allow" updates the ACL partition list and preserves the key); + deletion is not. ``macos_only``: the fixup no-ops on non-macOS (sys.platform guard), and the subject is codesign against a real ``.app`` bundle layout. @@ -801,7 +884,6 @@ def test_relaunchable_fixup_legacy_adhoc_still_resets_keychain_item(tmp_path, mo app = exe.parents[2] calls: list[list[str]] = [] - resets: list[Path] = [] def fake_run(cmd, **kwargs): calls.append(list(cmd)) @@ -818,13 +900,11 @@ def test_relaunchable_fixup_legacy_adhoc_still_resets_keychain_item(tmp_path, mo raise subprocess.CalledProcessError(1, ["codesign"]) monkeypatch.setattr(cli_main, "_desktop_macos_local_codesign", boom) - monkeypatch.setattr( - cli_main, "_desktop_macos_reset_keychain_safe_storage", lambda app: resets.append(app) - ) assert cli_main._desktop_macos_relaunchable_fixup(desktop_dir) is False assert ["/usr/bin/codesign", "--force", "--deep", "--sign", "-", str(app)] in calls - assert resets == [app] + assert ["/usr/bin/codesign", "--verify", "--deep", "--strict", str(app)] in calls + assert not any("delete-generic-password" in c for c in calls) # --- desktop.* launch options (config.yaml) -------------------------------