fix(desktop): never delete safeStorage keychain item in the updater
Addresses round-2 review feedback on #90961. The previous commits scoped the keychain deletion to the legacy ad-hoc fallback, but the reviewer correctly held the blocker: the fallback ran codesign with check=False, ignored the result, and unconditionally deleted 'Hermes Safe Storage' — permanently orphaning gateway and native OAuth credentials even when signing failed or a configured identity had failed and routed into the fallback. This commit removes the deletion entirely: - _desktop_macos_reset_keychain_safe_storage is gone; no code path touches the keychain item anymore. - The legacy fallback now checks the codesign result and runs codesign --verify --deep --strict; any failure leaves the item untouched and prints a warning. - The keychain prompt after an ad-hoc re-sign is recoverable (Always Allow updates the ACL partition list and preserves the key); deletion is not. The durable proof-carrying migration belongs in Electron (safeStorage can read the old key) and is tracked as a follow-up. Tests: 4 witnesses (stable path, default no-config success, fallback failure, fallback success) all mutation-verified against both the deletion regression and the ignored-codesign-result regression.
This commit is contained in:
David Metcalfe
+31
-55
@@ -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 ``<productName> 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 "<appName> 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
|
||||
|
||||
@@ -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) -------------------------------
|
||||
|
||||
Reference in New Issue
Block a user