Skip to content

fix(ssl): stop shadowing the Windows trust store for child processes - #9370

Merged
bolichen97 merged 1 commit into
kirodotdev:mainfrom
ShortEmperor:fix/windows-ssl-trust-store
Sep 9, 2026
Merged

fix(ssl): stop shadowing the Windows trust store for child processes#9370
bolichen97 merged 1 commit into
kirodotdev:mainfrom
ShortEmperor:fix/windows-ssl-trust-store

Conversation

@ShortEmperor

Copy link
Copy Markdown
Contributor

_ensure_ssl_certs() has no Windows-aware branch, so on win32 the Unix CA candidates never exist, get_default_verify_paths().cafile is None, and the certifi fallback always exports SSL_CERT_FILE + REQUESTS_CA_BUNDLE.

rustls-native-certs checks SSL_CERT_FILE first on every platform and loads that file INSTEAD of the platform store, so the export silently removed the Windows ROOT store from kiro-cli's trust while leaving the gateway's own HTTPS intact -- CPython reads the Windows store directly and treats the variable as an addition. Behind a TLS-inspecting proxy whose root is installed in Windows ROOT, every turn failed with "dispatch failure" on the intercepted q..amazonaws.com endpoint while sign-in and the usage API, which are not intercepted, kept working.

Return before discovery on win32. Nothing there needs the export: CPython and requests are already covered, and Node children read NODE_EXTRA_CA_CERTS. An explicit SSL_CERT_FILE is still honoured above.

Problem / Motivation

On Windows, Kiro Crew chat fails on every turn with An unknown error occurred: dispatch failure, while sign-in and the usage/credit pill keep working. The natural
diagnostic — run kiro-cli in a terminal — passes, because a plain shell has no
SSL_CERT_FILE set.

The split is explained by which endpoints a TLS-inspecting proxy intercepts:

Endpoint Purpose Issuer as presented Intercepted
q.us-east-1.amazonaws.com model / streaming Zscaler Intermediate Root CA (zscalertwo.net) yes
oidc.us-east-1.amazonaws.com sign-in Amazon RSA 2048 M04 no
codewhisperer.us-east-1.amazonaws.com usage API Amazon RSA 2048 M04 no

Only the model endpoint is inspected, so only chat dies.

The Zscaler root is installed in both Cert:\LocalMachine\Root and
Cert:\CurrentUser\Root, so every ordinary Windows application is fine. The shipped
desktop backend's certifi bundle holds 121 public roots and contains no Zscaler entry.

_ensure_ssl_certs() runs at import of cli.py / __main__.py (and from
mcp_gateway/gatewayd.py), so it is in the gateway process before any kiro-cli spawn.
On Windows ssl.get_default_verify_paths().cafile is None and all three
_CA_CANDIDATES are Linux paths, so control always reaches the certifi fallback and
exports SSL_CERT_FILE + REQUESTS_CA_BUNDLE. kiro-cli inherits it.

Why it matters

Kiro Crew is unusable for chat on any Windows host behind TLS inspection -- most
corporate Windows fleets. The failure is silent and misattributed: it presents as a
network fault or a Kiro CLI bug, and the obvious check (kiro-cli in a terminal)
passes, so the gateway is the last place anyone looks.

What changed (motivation → approach → change)

The asymmetry is what makes this invisible. SSL_CERT_FILE means different things to
the two consumers:

  • CPythoncreate_default_context() on Windows loads the Windows ROOT store
    and honours SSL_CERT_FILE. A union. The gateway's own HTTPS keeps working,
    which hides the problem.
  • rustls-native-certs, which builds kiro-cli's trust store — checks
    SSL_CERT_FILE first, on every platform, and when set loads that file
    instead of the platform store. A replacement.

So exporting a public-roots-only bundle subtracts every private CA the machine
trusts from the child, while leaving the parent working.

The Windows branch was never intentional. The module docstring scopes the file-based
bootstrap to "Linux distributions whose Python default points at the wrong CA
location"; Windows falls into it only because the Unix candidate paths don't exist.

Fix: return before discovery on win32. Nothing there needs the export —

  • CPython reads the Windows ROOT store directly, so this process and any Python child
    are already covered.
  • requests never reads the Windows store, but its default is certifi, so pointing
    REQUESTS_CA_BUNDLE at certifi changes nothing.
  • Node children read NODE_EXTRA_CA_CERTS, never this variable.

Placement is deliberate: below the explicit-override check, so an operator's own
SSL_CERT_FILE still wins (the current workaround for affected users, and the
supported way to add a private CA); above the discovery block, so no future edit
can reintroduce an export on that path.

Linux behaviour is unchanged. That branch is the module's stated purpose, the paths it
finds are real system bundles rather than public-roots-only files, and
rustls-native-certs reads /etc/ssl/... there anyway.

Tests

Two cases added to test/test_ssl_certs.py::TestEnsureSslCerts:

  • test_win32_exports_nothing_even_when_a_bundle_is_discoverable — the mirror of the
    existing test_falls_back_to_certifi_when_no_system_path_exists: identical setup,
    only the platform differs, opposite outcome. Both discovery paths are made to
    succeed (an existing candidate path and a resolvable certifi bundle), so it pins
    that the guard sits above discovery rather than merely that certifi goes
    unreached. Confirmed to fail without the guard, exporting the bundle.
  • test_win32_still_honours_an_explicit_ssl_cert_file — pins that an operator's
    explicit bundle survives the guard. This passes without the fix too (the override
    check already existed); it guards the placement rather than catching the defect.

Both override the file's autouse _reset_ssl_bootstrap fixture, which forces
sys.platform = "linux".

Manual verification

Measured by the reporter on the affected Windows host:

TLS probe against the intercepted endpoint, trusting one bundle at a time:

certifi bundle only (what KiroCrew exports today):  FAIL -> SSLCertVerificationError:
    certificate verify failed: unable to get local issuer certificate
certifi + Zscaler root:                             OK  (tls=TLSv1.3)

End-to-end against the real kiro-cli binary, three runs in one shell:

$env:SSL_CERT_FILE = $null                            # -> "Hi!"  (works)
$env:SSL_CERT_FILE = '<...>\certifi\cacert.pem'       # -> dispatch failure
$env:SSL_CERT_FILE = '<...>\bundle-with-zscaler.pem'  # -> "Hi!"  (works)

The failing run also lost the model name from its status bar (kiro_default · ◔ 4%
versus kiro_default · claude-opus-5), so model listing was failing on the same
handshake.

Prelude behaviour on that host, post-patch:

platform: win32
SSL_CERT_FILE after prelude: None
REQUESTS_CA_BUNDLE after prelude: None
this process default ctx CAs: {'x509': 69, 'crl': 0, 'x509_ca': 66}

On Linux (this PR's development host): test_ssl_certs.py passes 20/20; black,
isort, flake8 and mypy report nothing on the changed files; the repo black and
subprocess-encoding gates pass.

Related Issues

Fixes #NNNN (filed alongside this PR — no existing issue covered the Windows case).

Related: #5602 — the macOS sibling of this class, same TLS-inspection context but
about this process's own trust rather than the child's. Still open after #5903
landed the Keychain work; see Open question 1.

Open questions for maintainers

  1. macOS may carry the same defect, and Bundled Python ignores macOS Keychain trust store — SSL handshakes to MCP endpoints fail behind Zscaler/TLS-inspection proxies #5602 is where it would land. The
    darwin branch calls _inject_macos_system_trust() (added by fix(tls): honor macOS Keychain trust #5903) and then
    falls through to discovery — there is no return. inject_into_ssl() is
    process-local, so the export exists deliberately to give children something they
    cannot inherit as a monkey-patch. But if discovery lands on certifi, a rustls child
    skips the Keychain for exactly the Windows reason — which would explain why Bundled Python ignores macOS Keychain trust store — SSL handshakes to MCP endpoints fail behind Zscaler/TLS-inspection proxies #5602
    is still open after fix(tls): honor macOS Keychain trust #5903. Whether it fires depends on whether
    get_default_verify_paths().cafile exists on a given macOS install, which was not
    testable from the affected host. Flagging rather than fixing: widening this diff
    onto an unverifiable platform is how a three-line fix stalls. Happy to take it as a
    follow-up against Bundled Python ignores macOS Keychain trust store — SSL handshakes to MCP endpoints fail behind Zscaler/TLS-inspection proxies #5602 if you agree with the reading.

  2. Want a windows-install.md note? No spec covers the SSL bootstrap (grepped
    docs/** and src/kiro_crew/docs/** for _ssl_compat, _ensure_ssl_certs,
    SSL_CERT_FILE, REQUESTS_CA_BUNDLE — no hits), so nothing goes stale. A short
    "corporate TLS inspection" section explaining that SSL_CERT_FILE is the supported
    way to add a private CA might help affected users find the workaround, but that is
    scope you did not ask for. Say the word and I'll add it here or as a follow-up.

  3. Interaction with fix(acp): classify prose-spelled dispatch failure as transient #9330. That PR classified the prose spelling of dispatch failure as transient, i.e. retry-eligible. A TLS trust failure is permanent, so on
    an affected host each turn now burns the retry ladder before surfacing. It does not
    change this fix, but the two share a symptom and it is worth knowing they interact.

Pattern harvest

Rule candidate: review-prompt
Pattern: a process-wide env export intended to ADD capability to child
processes, where the variable is defined by at least one consumer as a
REPLACEMENT — so the export silently subtracts capability from the child
while the exporting process, which reads the variable additively, keeps
working and hides it.

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable) — N/A, no doc covers this path (see Open questions)
  • No secrets, credentials, or internal references in the diff

Contribution License Agreement

@ShortEmperor
ShortEmperor requested a review from a team as a code owner September 8, 2026 03:55
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) 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 Sep 8, 2026
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — ✅ PASS

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

The patch is a three-line platform guard plus two pinning tests; the base file and PR description fully support each other. I've verified the design gate: real harm (Windows corporate fleets behind TLS inspection lose chat entirely), root-cause fix at the layer that owns the export, the operator escape hatch preserved, trivially reversible, extensive manual verification on the affected host, and no test pins the opposite behavior. No design-level findings survived the kill-filter.

Design-Verdict: PASS

Removes the export at the layer that owns it, preserves the operator override, and pins placement with mirrored tests — minimal, verified, reversible.

[DESIGN-REVIEWED] 50ef491

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

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

Premise-level review of 50ef491223585110237fd4d81f18b90b69fa772f 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.

The review contract, PR intent, patch, and base files are all read; the change is a three-line win32 guard in _ensure_ssl_certs() plus two tests. I verified against the base tree: on win32 the base code does reach the certifi export (all _CA_CANDIDATES are Linux paths, _ssl_compat.py:98-119), so the added test fails on base as claimed; the other SSL_CERT_FILE references in src/ (hooks.py, github_runner.py, issue_radar transports, etc.) are passthrough allowlists, not exporters; the declared macOS sibling is the author's own open question with a linked issue, so restating it would be noise. Final review follows.

First-Principles-Verdict: PASS

Verify the load-bearing vendor claim on the shipped kiro-cli: rustls-native-certs treats SSL_CERT_FILE as a replacement for, not an addition to, the Windows store.

The description carries its own reproduction (three-run SSL_CERT_FILE experiment against the real binary, TLS probe per bundle), the fix is a deletion of behavior at cause level for win32, and the mirror test fails on base — I confirmed by reading _ssl_compat.py:98-119 that win32 always reaches the export today. Grepped SSL_CERT_FILE|REQUESTS_CA_BUNDLE across src/: 8 non-defining hits, all env-passthrough allowlists that merely forward the variable if set — none needs the prelude to have exported it. The macOS fall-through sibling is declared in the description with a linked issue and is pinned as deliberate by test_macos_injection_still_exports_child_env, so it is accepted-and-deferred, not a finding.

What this change ships

Intent: make chat work on Windows hosts behind TLS-inspecting proxies by not overriding kiro-cli's trust store — a FIX.

Inventory (3 items)
  1. On Windows, startup no longer exports SSL_CERT_FILE/REQUESTS_CA_BUNDLE, so kiro-cli keeps the Windows ROOT store — justified
  2. An operator's explicit SSL_CERT_FILE still wins on Windows, now pinned by test — justified
  3. A test pins the guard sits above bundle discovery, so no future edit re-exports on win32 — justified

[FIRST-PRINCIPLES-REVIEWED] 50ef491

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] 50ef491

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — ✅ no blocking findings

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

Review details

No findings.
[GPT-REVIEWED] 50ef491

@ShortEmperor

Copy link
Copy Markdown
Contributor Author

Inherited failure, not from this change. test_security_conductor_skill_contract.py::TestSkillIsInstallable::test_no_bundled_scripts_are_shipped_here fails on main as of 5134dce. #9271 (19:18:53) added an assertion that builtin_skills/security-conductor/scripts/ must not exist; #9270 (19:20:02) created it with ledger.py. The same test file already lists ledger.py in BUNDLED_SCRIPTS and requires the prose to cite it, so the two assertions contradict each other. Reproduced on a pristine checkout with this branch's changes reverted. This PR touches only _ssl_compat.py and test_ssl_certs.py.

@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 Sep 8, 2026
_ensure_ssl_certs() has no Windows-aware branch, so on win32 the Unix CA
candidates never exist, get_default_verify_paths().cafile is None, and
the certifi fallback always exports SSL_CERT_FILE + REQUESTS_CA_BUNDLE.

rustls-native-certs checks SSL_CERT_FILE first on every platform and
loads that file INSTEAD of the platform store, so the export silently
removed the Windows ROOT store from kiro-cli's trust while leaving the
gateway's own HTTPS intact -- CPython reads the Windows store directly
and treats the variable as an addition. Behind a TLS-inspecting proxy
whose root is installed in Windows ROOT, every turn failed with
"dispatch failure" on the intercepted q.<region>.amazonaws.com endpoint
while sign-in and the usage API, which are not intercepted, kept
working.

Return before discovery on win32. Nothing there needs the export:
CPython and requests are already covered, and Node children read
NODE_EXTRA_CA_CERTS. An explicit SSL_CERT_FILE is still honoured above.
@ShortEmperor
ShortEmperor force-pushed the fix/windows-ssl-trust-store branch from c0fb167 to 50ef491 Compare September 8, 2026 05:07
@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 Sep 8, 2026

@cixuuz cixuuz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed the complete current-head diff. The Windows guard preserves explicit SSL_CERT_FILE overrides, avoids shadowing the native trust store, and is covered by focused regression tests.

@ShortEmperor

Copy link
Copy Markdown
Contributor Author

Hey @cixuuz @buluoray any news on the approval? No hurry but I was just wondering.

@bolichen97
bolichen97 merged commit c210e30 into kirodotdev:main Sep 9, 2026
69 checks passed
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 9, 2026
@ShortEmperor
ShortEmperor deleted the fix/windows-ssl-trust-store branch September 9, 2026 07:24
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