Skip to content

fix(tls): honor macOS Keychain trust - #5903

Merged
bolichen97 merged 1 commit into
kirodotdev:mainfrom
Tiger-0512:fix/macos-keychain-trust-5602
Aug 27, 2026
Merged

fix(tls): honor macOS Keychain trust#5903
bolichen97 merged 1 commit into
kirodotdev:mainfrom
Tiger-0512:fix/macos-keychain-trust-5602

Conversation

@Tiger-0512

@Tiger-0512 Tiger-0512 commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

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.pem exists, 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's aiohttp clients, 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.pem with sudo, a non-durable workaround that OS updates can erase.

Blindly exporting security find-certificate -a -p is 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)

  • Add pinned, macOS-only truststore==0.10.4 and inject it before HTTPS libraries cache an SSLContext. truststore delegates each real chain to Security.framework, preserving user/admin/system trust, explicit deny, hostname policy, and validity checks.
  • Preserve an explicit SSL_CERT_FILE as 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.
  • Injection does not short-circuit the environment export. inject_into_ssl() is process-local, so after injecting, the prelude still runs the existing file/certifi discovery and exports SSL_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.
  • Make injection process-idempotent because both __main__.py and cli.py may run the prelude.
  • Restore the early gatewayd prelude. The earlier env-only version (fix(mcp-gateway): run _ensure_ssl_certs() prelude in gatewayd #5831) was correctly closed as a no-op because env values are inherited. This version is different: 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.
  • Centralize the CA-trust availability check used by embeddings, Papyrus, and PPTX downloads. A normal OpenSSL context exposes cert_store_stats(); a truststore context intentionally raises NotImplementedError because Security.framework evaluates anchors dynamically. The helper accepts the injected dynamic store without weakening the existing OpenSSL CA-count check. Because inject_into_ssl() replaces ssl.SSLContext process-wide, the full backend suite was run to confirm no other caller depends on an API the truststore context does not implement; these three cert_store_stats() sites were the only collisions.

Tests

Added/updated SSL regression coverage for:

  • macOS system-trust injection
  • explicit SSL_CERT_FILE precedence
  • idempotent injection
  • successful injection still exports the certifi env for child processes
  • warning + file-based fallback on injection failure
  • gatewayd process-local prelude
  • OpenSSL CA-count behavior
  • truststore's dynamic, non-enumerable CA store

Local results:

  • test/test_ssl_certs.py: 17 passed; focused SSL/download set (embeddings, Papyrus, PPTX): 285 passed
  • isort --check-only src/kiro_crew test: clean
  • flake8 src/kiro_crew test: clean
  • mypy src/kiro_crew/: 1102 source files, no issues
  • Wheel build: metadata contains Requires-Dist: truststore==0.10.4; sys_platform == "darwin"
  • Full backend: 68,264 passed; 20 host/environment failures. The exact same 20 failures reproduce against a clean origin/main worktree (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.
  • Frontend: TypeScript passed; Vitest 24,120 passed, with one Hindi style ratchet failure. The same 120-vs-119 failure reproduces on clean origin/main.

Manual verification

A public endpoint smoke test from this macOS host returned TLS_OK 404 both before and after injection, so this host does not reproduce the managed-Keychain/private-CA condition. Validation on the affected environment is still required:

  1. the Kiro Crew remote MCP probe succeeds with the Keychain-trusted corporate CA;
  2. a real MCP tool call succeeds.

The first is directly fixed here. truststore does not propagate into unrelated external runtimes such as Node or a separately packaged kiro-cli process; those continue to receive the inherited SSL_CERT_FILE env (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

  • At most two commits (one commit), with a Conventional Commits title
  • Existing affected tests pass and regression tests cover new behavior
  • Self-review completed; code follows project style guidelines
  • Documentation updated where behavior and security rationale changed
  • No secrets, credentials, certificate contents, or internal references in the diff

Contribution License Agreement

N/A — the repository template currently provides no CLA wording for contributors.

@Tiger-0512
Tiger-0512 requested a review from a team as a code owner August 25, 2026 15:48
@Tiger-0512
Tiger-0512 requested a review from smeyffret August 25, 2026 15:48
@github-actions github-actions Bot added the fork Pull request from a fork (external contributor) label Aug 25, 2026
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 25, 2026
@Tiger-0512
Tiger-0512 marked this pull request as draft August 25, 2026 15:53
@Tiger-0512
Tiger-0512 force-pushed the fix/macos-keychain-trust-5602 branch from 903e93d to 7a4314b Compare August 25, 2026 15:56
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 25, 2026
@Tiger-0512
Tiger-0512 force-pushed the fix/macos-keychain-trust-5602 branch from 7a4314b to 200dc8f Compare August 25, 2026 15:59
@dwu96

dwu96 commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

👋 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.

@Tiger-0512
Tiger-0512 marked this pull request as ready for review August 25, 2026 16:02
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 25, 2026
@Tiger-0512
Tiger-0512 force-pushed the fix/macos-keychain-trust-5602 branch from 200dc8f to 60ff5b4 Compare August 26, 2026 00:04
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 26, 2026
@github-actions

github-actions Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of 0d37fa76c4cb0d31db5f1d5048bc98766c418cc9 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The design is sound. No docs currently cover the SSL bootstrap, so no spec obligation is violated. The gatewayd prelude mirrors the established cli.py/__main__.py pattern, the alternative (flattening the Keychain via security find-certificate) is correctly rejected in the description, and the acknowledged limitation (child processes like kiro-cli still get the certifi file bundle, not Keychain trust) is honestly scoped rather than hidden.

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

  • inject_into_ssl() makes any future direct cert_store_stats()/get_ca_certs() call a macOS-only runtime failure; a small grep gate (like check_harness_parity.py) forcing new callers through _ssl_context_has_ca_trust would keep the audit you did by hand from decaying.

[DESIGN-REVIEWED] 0d37fa7

@github-actions

github-actions Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5, fork) — ✅ PASS

Premise-level review of 0d37fa76c4cb0d31db5f1d5048bc98766c418cc9 via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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 ships

Intent: make Kiro Crew's own HTTPS accept corporate roots that macOS Keychain trusts (refs #5602). This is a FIX.

  1. macOS HTTPS now honors Keychain-managed trust via Security.framework — justified, cause-level
  2. New darwin-only pinned dependency truststore==0.10.4 — justified (mechanism of the fix)
  3. Explicit SSL_CERT_FILE skips injection, stays the operator override — justified, declared
  4. Injection failure or missing package warns and keeps prior file behavior — justified, declared
  5. Gateway daemon installs the trust prelude itself — justified: spawned as python -m kiro_crew.mcp_gateway.gatewayd (manager.py:5), so neither existing prelude runs there and a monkey-patch is not inherited
  6. Embeddings/Papyrus/PPTX CA probes accept the dynamic store via one shared helper — justified: 3 cert_store_stats sites counted in base, all 3 fixed, zero unfixed siblings; without it NotImplementedError escapes their ssl.SSLError-only handlers
  7. Child processes still receive exported SSL_CERT_FILE/REQUESTS_CA_BUNDLE — declared, justified
  8. Injection is once-per-process — justified: three prelude entry points now exist

No duplicate mechanism: grep for truststore|Security.framework|SecTrust in base src/ finds 0 hits; certifi is a static bundle, not policy trust. The unfixed remainder (external kiro-cli/Node runtimes) is explicitly declared out of scope, not hidden.

Subtractions

  • Drop the unused bool return of _inject_macos_system_trust — its sole caller (_ensure_ssl_certs, _ssl_compat.py) ignores it and no test reads it (0 consumers; grepped the diff's call sites); make it return None and keep the _TRUSTSTORE_INJECTED flag as the signal.

[FIRST-PRINCIPLES-REVIEWED] 0d37fa7

@github-actions

github-actions Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed 0d37fa76c4cb0d31db5f1d5048bc98766c418cc9 via the fork AI-review pipeline; updated in place on each push.

Review details

Both candidates rest on unverifiable assumptions about truststore's internal behavior and macOS CPython specifics that I cannot re-derive here.

Candidate 1 requires that the parent process actually sets SSL_CERT_FILE on macOS (depends on ssl.get_default_verify_paths().cafile being absent on the target build — the candidate itself couldn't confirm, and I cannot). Even granting the trigger, the outcome is a degraded feature (gateway validates against a certifi bundle instead of honoring Keychain distrust), not a crash, data loss, or a reachable security hole — and SSL_CERT_FILE winning is the function's documented, intended precedence. (a) and (c) are both "might," below the 80 bar.

Candidate 2 claims the truststore context is discarded or crashes. Reading _make_ssl_context in full: ctx = ssl.create_default_context() is the truststore-backed context, and every exit path (return ctx) returns that same context — even when _ssl_context_has_ca_trust returns False the fallback loop finds no matching Linux path on macOS and return ctx at the tail hands back the truststore context. So the Security.framework context is never actually discarded. The crash branch requires load_default_certs()/cert_store_stats() to raise something other than ssl.SSLError/NotImplementedError — an unverifiable "if" about truststore 0.10.4 internals not present in this repo. The candidate's own confidence is "low." Below the bar.

Nothing else in the diff grounds a new finding at 80+.

No findings.

[OPUS-REVIEWED] 0d37fa7

@github-actions

github-actions Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed 0d37fa76c4cb0d31db5f1d5048bc98766c418cc9 via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 0d37fa7

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 26, 2026
@Tiger-0512
Tiger-0512 force-pushed the fix/macos-keychain-trust-5602 branch from 60ff5b4 to 382cbcd Compare August 26, 2026 02:30
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 26, 2026
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 26, 2026
@Tiger-0512
Tiger-0512 force-pushed the fix/macos-keychain-trust-5602 branch from 382cbcd to 3c0dacd Compare August 26, 2026 05:08
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 26, 2026
@Tiger-0512
Tiger-0512 force-pushed the fix/macos-keychain-trust-5602 branch from 3c0dacd to 07ace0e Compare August 26, 2026 05:24
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Aug 26, 2026
@Tiger-0512
Tiger-0512 force-pushed the fix/macos-keychain-trust-5602 branch from 07ace0e to 0d37fa7 Compare August 27, 2026 02:25
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 27, 2026
@bolichen97
bolichen97 enabled auto-merge (squash) August 27, 2026 05:37

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bolichen97
bolichen97 merged commit d2c586e into kirodotdev:main Aug 27, 2026
70 checks passed
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Aug 27, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator

Open PR relationship audit

This 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

  • PR #6230 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #6230: REBASE. The merged macOS work is the sibling half of the same design, not a substitute. Nothing on main implements any Windows equivalent. Files: src/kiro_crew/_ssl_compat.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants