From 28f2ea86e2d048e833f49ff59915bc480e79ecf7 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Tue, 25 Aug 2026 15:24:59 -0700 Subject: [PATCH] fix(desktop): make --setup-tcc-identity produce a VALID signing identity on modern macOS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes the two live E2E blockers @ctaylor86 found on PR #77189 (macOS 26.3.1, OpenSSL 3.6.3): - retry the PKCS#12 export with -legacy when security import rejects the OpenSSL 3 default format ('MAC verification failed during PKCS12 import') - trust the self-signed root for the codeSign policy (security add-trusted-cert -r trustRoot -p codeSign) — an imported-but-untrusted cert is invisible to find-identity -v and unusable by codesign - gate success on find-identity -v -p codesigning (postcondition), and use the same -v probe for idempotency so an untrusted leftover cert is repaired instead of reported as done Tests rewritten as stateful fakes (valid only after import+trust), plus new coverage for the -legacy retry, trust failure, postcondition gate, and the untrusted-cert repair path; sabotage-verified (reverting to the name-in-output probe fails 4 tests). Docs: manual fallback now includes the Trust step. --- hermes_cli/main.py | 116 ++++++++++++++---- tests/hermes_cli/test_gui_command.py | 172 +++++++++++++++++++++++++-- website/docs/user-guide/desktop.md | 6 +- 3 files changed, 257 insertions(+), 37 deletions(-) diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 854cf18185..80351fb31b 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -7793,6 +7793,27 @@ def _desktop_macos_relaunchable_fixup( return False +def _macos_codesigning_identity_valid(security: str, identity: str) -> bool: + """True when `identity` appears among VALID code-signing identities. + + ``security find-identity -p codesigning`` (without ``-v``) also lists + certificates macOS will refuse to sign with — e.g. a self-signed cert that + was imported but never trusted for the codeSign policy. Only the ``-v`` + listing proves codesign can actually use it, so this is both the + idempotency probe and the success postcondition for + ``--setup-tcc-identity``. Never raises. + """ + try: + result = subprocess.run( + [security, "find-identity", "-v", "-p", "codesigning"], + capture_output=True, text=True, check=False, + ) + except Exception: + return False + + return f'"{identity}"' in (result.stdout or "") + + def _desktop_macos_setup_tcc_identity(identity: str = "Hermes Local Signing") -> bool: """Create/import a self-signed code-signing cert and configure Hermes to use it. @@ -7829,11 +7850,11 @@ def _desktop_macos_setup_tcc_identity(identity: str = "Hermes Local Signing") -> return False keychain = str(Path.home() / "Library" / "Keychains" / "login.keychain-db") - existing = subprocess.run( - [security, "find-identity", "-p", "codesigning"], - capture_output=True, text=True, check=False, - ) - already_imported = identity in f"{existing.stdout}\n{existing.stderr}" + # A certificate that merely EXISTS in the keychain is not enough — macOS + # only treats it as a code-signing identity once it is trusted for the + # codeSign policy. Probe with `-v` (valid identities only) so a previously + # imported-but-untrusted cert is repaired rather than reported as done. + already_imported = _macos_codesigning_identity_valid(security, identity) if not already_imported: # Create a self-signed code-signing cert (valid 10 years) and import it @@ -7856,33 +7877,82 @@ def _desktop_macos_setup_tcc_identity(identity: str = "Hermes Local Signing") -> ], capture_output=True, check=True, ) - subprocess.run( - [ - openssl, "pkcs12", "-export", - "-inkey", str(key), "-in", str(crt), - "-out", str(p12), "-passout", "pass:hermeslocal", - ], - capture_output=True, check=True, - ) - imported = subprocess.run( - [ - security, "import", str(p12), "-k", keychain, - "-P", "hermeslocal", - "-T", codesign, "-T", "/usr/bin/codesign_allocate", - ], - capture_output=True, text=True, check=False, - ) + # OpenSSL 3 defaults to AES/SHA-2 PKCS#12 encryption that macOS + # `security import` rejects with "MAC verification failed during + # PKCS12 import (wrong password?)". The `-legacy` flag restores the + # RC2/SHA-1 format the importer accepts, but only exists on + # OpenSSL 3 — so try the plain export first and fall back to + # `-legacy` when the IMPORT fails with that signature. (Verified + # E2E on macOS 26.3.1 / OpenSSL 3.6.3 by @ctaylor86 on PR #77189.) + def _export_p12(extra_args: list) -> None: + subprocess.run( + [ + openssl, "pkcs12", "-export", *extra_args, + "-inkey", str(key), "-in", str(crt), + "-out", str(p12), "-passout", "pass:hermeslocal", + ], + capture_output=True, check=True, + ) + + def _import_p12(): + return subprocess.run( + [ + security, "import", str(p12), "-k", keychain, + "-P", "hermeslocal", + "-T", codesign, "-T", "/usr/bin/codesign_allocate", + ], + capture_output=True, text=True, check=False, + ) + + _export_p12([]) + imported = _import_p12() + if imported.returncode != 0 and "MAC verification failed" in (imported.stderr or ""): + try: + _export_p12(["-legacy"]) + imported = _import_p12() + except subprocess.CalledProcessError: + # Older OpenSSL without -legacy: keep the original failure. + pass if imported.returncode != 0: print(f" (could not import signing identity into keychain: {imported.stderr.strip()})") return False - print(f" → created and imported self-signed identity: {identity!r}") + + # Importing is still not enough: without explicit trust for the + # codeSign policy, `security find-identity -v -p codesigning` + # reports 0 valid identities and codesign refuses the cert. Trust + # the self-signed root for code signing. This writes to the user's + # trust settings, so macOS may prompt for the login password ONCE + # here — that is the one-time setup cost this command exists to + # front-load. + trusted = subprocess.run( + [security, "add-trusted-cert", "-r", "trustRoot", "-p", "codeSign", "-k", keychain, str(crt)], + capture_output=True, text=True, check=False, + ) + if trusted.returncode != 0: + print( + " (could not trust the certificate for code signing: " + f"{(trusted.stderr or trusted.stdout).strip()})" + ) + return False + print(f" → created, imported, and trusted self-signed identity: {identity!r}") except Exception as exc: print(f" (certificate creation failed: {exc})") return False finally: shutil.rmtree(tmp_dir, ignore_errors=True) else: - print(f" → identity {identity!r} already in keychain") + print(f" → identity {identity!r} already valid in keychain") + + # Postcondition gate: only report success once macOS actually agrees the + # identity is usable for code signing. Name-in-output checks pass for + # invalid identities; this is the check that failed silently before. + if not _macos_codesigning_identity_valid(security, identity): + print( + f" (identity {identity!r} was imported but is not a VALID code-signing identity; " + "run `security find-identity -v -p codesigning` to inspect, and see the manual " + "Keychain Access steps in the desktop docs)" + ) + return False # Point Hermes at the identity (config.yaml, not .env — it's not a secret). try: diff --git a/tests/hermes_cli/test_gui_command.py b/tests/hermes_cli/test_gui_command.py index 7154f9eb4a..e7744672aa 100644 --- a/tests/hermes_cli/test_gui_command.py +++ b/tests/hermes_cli/test_gui_command.py @@ -499,8 +499,8 @@ def _fake_proc(cmd, returncode=0, stdout="", stderr=""): return subprocess.CompletedProcess(cmd, returncode, stdout=stdout, stderr=stderr) -def test_setup_tcc_identity_creates_cert_imports_and_configures(tmp_path, monkeypatch, capsys): - """Fresh identity: openssl generates, security imports, config is written.""" +def test_setup_tcc_identity_creates_cert_imports_trusts_and_configures(tmp_path, monkeypatch, capsys): + """Fresh identity: openssl generates, security imports + trusts, config is written.""" monkeypatch.setattr(cli_main.sys, "platform", "darwin") monkeypatch.setattr( cli_main.shutil, @@ -511,12 +511,20 @@ def test_setup_tcc_identity_creates_cert_imports_and_configures(tmp_path, monkey identity = "Hermes Local Signing" calls = [] + state = {"trusted": False} def fake_run(cmd, **kwargs): calls.append(list(cmd)) - # `security find-identity` on first run reports no matching identity. - if cmd[:3] == ["/usr/bin/security", "find-identity", "-p"]: - return _fake_proc(cmd, stdout=" 0 identities found") + if cmd[:4] == ["/usr/bin/security", "find-identity", "-v", "-p"]: + # Valid only after import AND trust have both happened — mirrors + # real macOS, where an untrusted self-signed cert is invisible to + # the -v listing (the #77189 review finding). + if state["trusted"]: + return _fake_proc(cmd, stdout=f' 1) ABCD "{identity}"\n 1 valid identities found') + return _fake_proc(cmd, stdout=" 0 valid identities found") + if cmd[0] == "/usr/bin/security" and cmd[1] == "add-trusted-cert": + state["trusted"] = True + return _fake_proc(cmd) return _fake_proc(cmd) monkeypatch.setattr(cli_main.subprocess, "run", fake_run) @@ -528,18 +536,119 @@ def test_setup_tcc_identity_creates_cert_imports_and_configures(tmp_path, monkey assert cli_main._desktop_macos_setup_tcc_identity(identity) is True out = capsys.readouterr().out - assert "created and imported self-signed identity" in out + assert "created, imported, and trusted self-signed identity" in out assert "set desktop.macos_signing_identity" in out - # openssl cert generation + pkcs12 export + security import all ran. + # openssl cert generation + pkcs12 export + security import + trust all ran. assert any(c[0] == "/usr/bin/openssl" and "req" in c for c in calls) assert any(c[0] == "/usr/bin/openssl" and "pkcs12" in c for c in calls) assert any(c[0] == "/usr/bin/security" and c[1] == "import" for c in calls) + assert any(c[0] == "/usr/bin/security" and c[1] == "add-trusted-cert" for c in calls) + # The trust step targets the codeSign policy specifically. + trust_call = next(c for c in calls if c[1:2] == ["add-trusted-cert"]) + assert "codeSign" in trust_call and "trustRoot" in trust_call # Temp files cleaned up. assert not list(tmp_path.glob("hermes-tcc-*")) -def test_setup_tcc_identity_skips_generation_when_already_present(tmp_path, monkeypatch, capsys): - """Idempotent: an existing identity is reused, not regenerated.""" +def test_setup_tcc_identity_retries_pkcs12_with_legacy_on_mac_verification_failure(tmp_path, monkeypatch, capsys): + """OpenSSL 3: first import fails with the MAC-verification signature, the + -legacy re-export imports cleanly (the exact failure @ctaylor86 hit live).""" + monkeypatch.setattr(cli_main.sys, "platform", "darwin") + monkeypatch.setattr( + cli_main.shutil, + "which", + lambda name: {"openssl": "/usr/bin/openssl", "security": "/usr/bin/security", "codesign": "/usr/bin/codesign"}.get(name), + ) + monkeypatch.setattr(cli_main.Path, "home", classmethod(lambda cls: tmp_path)) + + identity = "Hermes Local Signing" + calls = [] + state = {"legacy_exported": False, "trusted": False} + + def fake_run(cmd, **kwargs): + calls.append(list(cmd)) + if cmd[:4] == ["/usr/bin/security", "find-identity", "-v", "-p"]: + if state["trusted"]: + return _fake_proc(cmd, stdout=f' 1) ABCD "{identity}"\n 1 valid identities found') + return _fake_proc(cmd, stdout=" 0 valid identities found") + if cmd[0] == "/usr/bin/openssl" and "pkcs12" in cmd: + state["legacy_exported"] = "-legacy" in cmd + return _fake_proc(cmd) + if cmd[0] == "/usr/bin/security" and cmd[1] == "import": + if not state["legacy_exported"]: + return _fake_proc( + cmd, returncode=1, + stderr="security: SecKeychainItemImport: MAC verification failed during PKCS12 import (wrong password?)", + ) + return _fake_proc(cmd) + if cmd[0] == "/usr/bin/security" and cmd[1] == "add-trusted-cert": + state["trusted"] = True + return _fake_proc(cmd) + return _fake_proc(cmd) + + monkeypatch.setattr(cli_main.subprocess, "run", fake_run) + monkeypatch.setattr(cli_main, "_desktop_packaged_executable", lambda d: None) + monkeypatch.setattr(cli_main, "_desktop_macos_relaunchable_fixup", lambda d: True) + monkeypatch.setattr("hermes_cli.config.set_config_value", lambda key, value: None) + + assert cli_main._desktop_macos_setup_tcc_identity(identity) is True + + # Two pkcs12 exports (plain then -legacy) and two import attempts. + pkcs12_calls = [c for c in calls if c[0] == "/usr/bin/openssl" and "pkcs12" in c] + assert len(pkcs12_calls) == 2 + assert "-legacy" not in pkcs12_calls[0] and "-legacy" in pkcs12_calls[1] + assert len([c for c in calls if c[0] == "/usr/bin/security" and c[1] == "import"]) == 2 + + +def test_setup_tcc_identity_fails_when_trust_step_fails(tmp_path, monkeypatch, capsys): + """A cert that imports but cannot be trusted for codeSign is a failure, + not a silent success.""" + monkeypatch.setattr(cli_main.sys, "platform", "darwin") + monkeypatch.setattr( + cli_main.shutil, + "which", + lambda name: {"openssl": "/usr/bin/openssl", "security": "/usr/bin/security", "codesign": "/usr/bin/codesign"}.get(name), + ) + monkeypatch.setattr(cli_main.Path, "home", classmethod(lambda cls: tmp_path)) + + def fake_run(cmd, **kwargs): + if cmd[:4] == ["/usr/bin/security", "find-identity", "-v", "-p"]: + return _fake_proc(cmd, stdout=" 0 valid identities found") + if cmd[0] == "/usr/bin/security" and cmd[1] == "add-trusted-cert": + return _fake_proc(cmd, returncode=1, stderr="SecTrustSettingsSetTrustSettings: authorization denied") + return _fake_proc(cmd) + + monkeypatch.setattr(cli_main.subprocess, "run", fake_run) + + assert cli_main._desktop_macos_setup_tcc_identity("Hermes Local Signing") is False + assert "could not trust the certificate" in capsys.readouterr().out + + +def test_setup_tcc_identity_fails_when_identity_never_becomes_valid(tmp_path, monkeypatch, capsys): + """Postcondition gate: import + trust both 'succeed' but find-identity -v + still lists nothing → report failure with guidance (the silent-success bug + from the original PR).""" + monkeypatch.setattr(cli_main.sys, "platform", "darwin") + monkeypatch.setattr( + cli_main.shutil, + "which", + lambda name: {"openssl": "/usr/bin/openssl", "security": "/usr/bin/security", "codesign": "/usr/bin/codesign"}.get(name), + ) + monkeypatch.setattr(cli_main.Path, "home", classmethod(lambda cls: tmp_path)) + + def fake_run(cmd, **kwargs): + if cmd[:4] == ["/usr/bin/security", "find-identity", "-v", "-p"]: + return _fake_proc(cmd, stdout=" 0 valid identities found") + return _fake_proc(cmd) + + monkeypatch.setattr(cli_main.subprocess, "run", fake_run) + + assert cli_main._desktop_macos_setup_tcc_identity("Hermes Local Signing") is False + assert "not a VALID code-signing identity" in capsys.readouterr().out + + +def test_setup_tcc_identity_skips_generation_when_already_valid(tmp_path, monkeypatch, capsys): + """Idempotent: an existing VALID identity is reused, not regenerated.""" monkeypatch.setattr(cli_main.sys, "platform", "darwin") monkeypatch.setattr( cli_main.shutil, @@ -552,8 +661,8 @@ def test_setup_tcc_identity_skips_generation_when_already_present(tmp_path, monk def fake_run(cmd, **kwargs): calls.append(list(cmd)) - if cmd[:3] == ["/usr/bin/security", "find-identity", "-p"]: - return _fake_proc(cmd, stdout=' "Hermes Local Signing" (CSSMERR_TP_NOT_TRUSTED)') + if cmd[:4] == ["/usr/bin/security", "find-identity", "-v", "-p"]: + return _fake_proc(cmd, stdout=' 1) ABCD "Hermes Local Signing"\n 1 valid identities found') return _fake_proc(cmd) monkeypatch.setattr(cli_main.subprocess, "run", fake_run) @@ -564,12 +673,49 @@ def test_setup_tcc_identity_skips_generation_when_already_present(tmp_path, monk assert cli_main._desktop_macos_setup_tcc_identity("Hermes Local Signing") is True out = capsys.readouterr().out - assert "already in keychain" in out + assert "already valid in keychain" in out # No openssl generation, no security import — only find-identity + config. assert not any(c[0] == "/usr/bin/openssl" for c in calls) assert not any(c[0] == "/usr/bin/security" and c[1] == "import" for c in calls) +def test_setup_tcc_identity_untrusted_existing_cert_is_repaired(tmp_path, monkeypatch, capsys): + """A cert that EXISTS but is not valid (CSSMERR_TP_NOT_TRUSTED) is repaired + — regenerated/trusted — instead of being reported as already done. The + original name-in-output probe treated this state as success.""" + monkeypatch.setattr(cli_main.sys, "platform", "darwin") + monkeypatch.setattr( + cli_main.shutil, + "which", + lambda name: {"openssl": "/usr/bin/openssl", "security": "/usr/bin/security", "codesign": "/usr/bin/codesign"}.get(name), + ) + monkeypatch.setattr(cli_main.Path, "home", classmethod(lambda cls: tmp_path)) + + calls = [] + state = {"trusted": False} + + def fake_run(cmd, **kwargs): + calls.append(list(cmd)) + if cmd[:4] == ["/usr/bin/security", "find-identity", "-v", "-p"]: + # -v never lists the untrusted cert; it only appears once the + # repair path has run add-trusted-cert. + if state["trusted"]: + return _fake_proc(cmd, stdout=' 1) ABCD "Hermes Local Signing"\n 1 valid identities found') + return _fake_proc(cmd, stdout=" 0 valid identities found") + if cmd[0] == "/usr/bin/security" and cmd[1] == "add-trusted-cert": + state["trusted"] = True + return _fake_proc(cmd) + return _fake_proc(cmd) + + monkeypatch.setattr(cli_main.subprocess, "run", fake_run) + monkeypatch.setattr(cli_main, "_desktop_packaged_executable", lambda d: None) + monkeypatch.setattr(cli_main, "_desktop_macos_relaunchable_fixup", lambda d: True) + monkeypatch.setattr("hermes_cli.config.set_config_value", lambda key, value: None) + + assert cli_main._desktop_macos_setup_tcc_identity("Hermes Local Signing") is True + assert any(c[0] == "/usr/bin/security" and c[1] == "add-trusted-cert" for c in calls) + + def test_setup_tcc_identity_non_macos_skips(tmp_path, monkeypatch, capsys): """On non-macOS the setup is a no-op failure (not a crash).""" monkeypatch.setattr(cli_main.sys, "platform", "linux") @@ -583,7 +729,7 @@ def test_cmd_gui_setup_tcc_identity_exits_before_build(tmp_path, monkeypatch): without building or launching the app.""" root = _make_desktop_tree(tmp_path) monkeypatch.setattr(cli_main, "PROJECT_ROOT", root) - _make_packaged_executable(root, monkeypatch, platform="darwin") + _make_packaged_executable(root, monkeypatch) with patch("hermes_cli.main._desktop_macos_setup_tcc_identity", return_value=True) as mock_setup, \ patch("hermes_cli.main._run_npm_install_deterministic") as mock_install, \ diff --git a/website/docs/user-guide/desktop.md b/website/docs/user-guide/desktop.md index 1a5e2b06bf..96c28eb979 100644 --- a/website/docs/user-guide/desktop.md +++ b/website/docs/user-guide/desktop.md @@ -561,7 +561,11 @@ Or do it manually: 1. Keychain Access → Certificate Assistant → **Create a Certificate…** 2. Name: `Hermes Local Signing`, Identity Type: *Self-Signed Root*, Certificate Type: **Code Signing**. -3. `hermes config set desktop.macos_signing_identity "Hermes Local Signing"` +3. In Keychain Access, double-click the new certificate → **Trust** → set + **Code Signing** to *Always Trust* (an imported self-signed certificate is + not a valid signing identity until it is trusted for code signing — + `security find-identity -v -p codesigning` should list it afterwards). +4. `hermes config set desktop.macos_signing_identity "Hermes Local Signing"` Use `--identity ` with the command to create/use a differently named certificate (default: `Hermes Local Signing`). The command is idempotent —