fix(tls): honor macOS Keychain trust - #5903
Conversation
903e93d to
7a4314b
Compare
7a4314b to
200dc8f
Compare
|
👋 Hi! This PR is currently in draft status. Workflow runs won't be auto-approved until it's marked as ready for review. When you're ready, click "Ready for review" and the workflows will be approved on the next cycle automatically. |
200dc8f to
60ff5b4
Compare
Design Review (Fable 5, fork) — ✅ PASSDesign-level review of The design is sound. No docs currently cover the SSL bootstrap, so no spec obligation is violated. The gatewayd prelude mirrors the established Design-Verdict: PASS Truststore is the root-cause fix for Keychain trust, correctly scoped to this process, with fallback, operator override, and child-env export all preserved. Suggestions
[DESIGN-REVIEWED] 0d37fa7 |
First Principles Review (Fable 5, fork) — ✅ PASSPremise-level review of All evidence checks out. Final assessment: First-Principles-Verdict: PASS A reported macOS defect fixed at its cause — static PEM can't carry Keychain policy — with every rider derived, counted, and declared. What this change shipsIntent: make Kiro Crew's own HTTPS accept corporate roots that macOS Keychain trusts (refs #5602). This is a FIX.
No duplicate mechanism: grep for Subtractions
[FIRST-PRINCIPLES-REVIEWED] 0d37fa7 |
Opus 4.8 Review (fork) — ✅ no blocking findingsReviewed Review detailsBoth candidates rest on unverifiable assumptions about Candidate 1 requires that the parent process actually sets Candidate 2 claims the truststore context is discarded or crashes. Reading Nothing else in the diff grounds a new finding at 80+. No findings. [OPUS-REVIEWED] 0d37fa7 |
GPT 5.6 Review (fork) — ✅ no blocking findingsReviewed Review detailsNo findings. |
60ff5b4 to
382cbcd
Compare
382cbcd to
3c0dacd
Compare
3c0dacd to
07ace0e
Compare
07ace0e to
0d37fa7
Compare
Open PR relationship auditThis is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion. Relationship findings
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
Problem / Motivation
Kiro Crew's bundled macOS Python validates TLS through OpenSSL's file store.
_ensure_ssl_certs()returns as soon as/etc/ssl/cert.pemexists, so it never consults the macOS Keychain. A corporate root trusted by Security.framework (for example, a TLS-inspection root) is therefore invisible to Kiro Crew'saiohttpclients, even though native macOS clients accept it.Why it matters
Enterprise environments commonly install private roots through managed Keychain trust. The mismatch blocks Kiro Crew-owned HTTPS — including the remote Streamable HTTP MCP probe — and currently pushes users toward modifying
/etc/ssl/cert.pemwithsudo, a non-durable workaround that OS updates can erase.Blindly exporting
security find-certificate -a -pis not a safe substitute: it enumerates stored certificates, not effective TLS trust, and can flatten explicit distrust, policy constraints, expired certificates, and leaves/intermediates into unconditional OpenSSL anchors.What changed (motivation → approach → change)
truststore==0.10.4and inject it before HTTPS libraries cache anSSLContext.truststoredelegates each real chain to Security.framework, preserving user/admin/system trust, explicit deny, hostname policy, and validity checks.SSL_CERT_FILEas the highest-precedence operator override. Keep the existing file-based Linux/certifi fallback unchanged; if truststore initialization fails, warn and fall back rather than blocking startup.inject_into_ssl()is process-local, so after injecting, the prelude still runs the existing file/certifi discovery and exportsSSL_CERT_FILE/REQUESTS_CA_BUNDLE— child processes (kiro-cli, Node MCP servers) inherit the env exactly as before and cannot lose the CA bundle they previously received.__main__.pyandcli.pymay run the prelude.truststore.inject_into_ssl()is a process-local monkey-patch, so the separate gateway Python runtime must install it for itself before TLS-touching imports.cert_store_stats(); a truststore context intentionally raisesNotImplementedErrorbecause Security.framework evaluates anchors dynamically. The helper accepts the injected dynamic store without weakening the existing OpenSSL CA-count check. Becauseinject_into_ssl()replacesssl.SSLContextprocess-wide, the full backend suite was run to confirm no other caller depends on an API the truststore context does not implement; these threecert_store_stats()sites were the only collisions.Tests
Added/updated SSL regression coverage for:
SSL_CERT_FILEprecedenceLocal results:
test/test_ssl_certs.py: 17 passed; focused SSL/download set (embeddings, Papyrus, PPTX): 285 passedisort --check-only src/kiro_crew test: cleanflake8 src/kiro_crew test: cleanmypy src/kiro_crew/: 1102 source files, no issuesRequires-Dist: truststore==0.10.4; sys_platform == "darwin"origin/mainworktree (missing local AWS CLI, xdist host-budget assumptions, PTY timeouts, platform capability assertions, and existing test isolation issues), so none is introduced by this change.origin/main.Manual verification
A public endpoint smoke test from this macOS host returned
TLS_OK 404both before and after injection, so this host does not reproduce the managed-Keychain/private-CA condition. Validation on the affected environment is still required:The first is directly fixed here.
truststoredoes not propagate into unrelated external runtimes such as Node or a separately packaged kiro-cli process; those continue to receive the inheritedSSL_CERT_FILEenv (unchanged from before this PR). If the probe succeeds but the runtime call still fails, that external-runtime trust path remains a follow-up (upstream system-trust support or a supported CA-bundle bridge) rather than being hidden by this PR.Related Issues
Refs #5602. Do not auto-close until the affected environment confirms the probe/tool-call scope above.
Checklist
Contribution License Agreement
N/A — the repository template currently provides no CLA wording for contributors.