Skip to content

fix: trust Windows system store CAs for kiro-cli on corporate proxies - #6230

Open
shaibarlev wants to merge 1 commit into
kirodotdev:mainfrom
shaibarlev:fix/windows-ssl-corporate-proxy
Open

fix: trust Windows system store CAs for kiro-cli on corporate proxies#6230
shaibarlev wants to merge 1 commit into
kirodotdev:mainfrom
shaibarlev:fix/windows-ssl-corporate-proxy

Conversation

@shaibarlev

@shaibarlev shaibarlev commented Aug 27, 2026

Copy link
Copy Markdown

Problem / Motivation

On Windows behind a corporate TLS-inspecting proxy, kiro-cli child processes can reject the proxy certificate with InvalidCertificate(UnknownIssuer): the enterprise root is trusted by Windows but absent from Mozilla's certifi roots.

Why it matters

Kiro Crew starts kiro-cli and MCP children that cannot inherit Python's process-local Windows trust evaluation. Without a verified child-process bundle, otherwise valid enterprise installations cannot connect. Disabling verification is not acceptable, and copying certifi unchanged would also retain a root that Windows explicitly placed in Disallowed.

What changed

  • Preserve an operator-provided SSL_CERT_FILE at highest precedence.
  • Parse certifi through SSLContext.get_ca_certs(binary_form=True) rather than copying its PEM bytes.
  • Read Windows Disallowed first and use SHA-256 over DER as one identity rule across certifi and Windows ROOT. A disallowed certificate is excluded regardless of source.
  • Enumerate only all-purpose or TLS-server-capable certificates from Windows ROOT; never promote intermediates from CA.
  • Publish the complete filtered certifi + Windows ROOT set after every successful store read, including certifi-only results. A private provenance marker refreshes the managed stable path across re-exec while operator bundle overrides remain authoritative.
  • Export the generated bundle for child TLS stacks while preserving an operator-provided REQUESTS_CA_BUNDLE.
  • Keep trust/ca-bundle.pem inside the existing read/write-blocked trust/ boundary. Treat explicit command references to SSL_CERT_FILE, REQUESTS_CA_BUNDLE, and the managed provenance variable as aliases of that protected path, preventing PowerShell, cmd, POSIX-shell, and embedded-interpreter indirect writes without removing ambient TLS inheritance.
  • If config resolution, store enumeration, or protected publication fails, publish no partial mirror and retain verified file-based fallback. The logged/documented degradation may not reproduce Windows explicit distrust, but never disables certificate verification.
  • Keep guarded optional imports at module scope and preserve the pre-HTTPS bootstrap boundary.

Tests

  • Deterministic red-before regression: all 7 indirect alias forms were accepted before the gate change; all are now denied through both is_sensitive_bash_command and the real HookManager path.
  • Focused post-rebase matrix: 42 SSL/bootstrap tests and 72 sensitive-shell-command tests passed on Windows.
  • RuntimeWarning and PytestUnraisableExceptionWarning were treated as errors; no retry, sleep, timeout increase, tolerance, or warning suppression was used.
  • Baseline-aware Black, isort, Flake8, mypy on the three production files, docs/spec updates, and git diff --check passed.

Manual verification

Verified against Windows' real Disallowed and ROOT stores. Eligible roots load into OpenSSL, explicit deny identities are not published in the generated mirror, and CLI import remains before HTTPS client initialization.

Screenshots / video

Why no screenshot: This changes TLS bootstrap, security gates, tests, and documentation only; it has no rendered UI effect.

@shaibarlev
shaibarlev requested a review from a team as a code owner August 27, 2026 05:57
@shaibarlev
shaibarlev requested review from hoang-phan98 and a lite review from Copilot August 27, 2026 05:57
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: checking Automated validation is still running labels Aug 27, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR updates Kiro Crew’s SSL bootstrap logic to better support Windows environments behind corporate TLS-inspecting proxies by generating and using a combined CA bundle so spawned kiro-cli processes (Rust TLS stack) can trust Windows enterprise CAs via SSL_CERT_FILE.

Changes:

  • Add Windows-specific logic to export additional certificates from the Windows system Root/CA stores (excluding those already present in certifi’s Mozilla bundle).
  • Generate a combined CA bundle under config_dir()/ca-bundle.pem and point SSL_CERT_FILE / REQUESTS_CA_BUNDLE at it when applicable.
  • Expand module and function documentation to describe the Windows corporate proxy failure mode and the mitigation approach.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/kiro_crew/_ssl_compat.py Outdated
Comment thread src/kiro_crew/_ssl_compat.py Outdated
@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Aug 27, 2026
@bolichen97
bolichen97 force-pushed the fix/windows-ssl-corporate-proxy branch from 38183cc to 9cf4db1 Compare August 29, 2026 19:42
@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 29, 2026
@iamwhatever

Copy link
Copy Markdown
Collaborator

👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated.

Missing sections:

  • ## Problem / Motivation
  • ## Why it matters
  • ## What changed
  • ## Tests

Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle.

@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 merge conflict Branch has merge conflicts with its base — author must resolve before merge labels Aug 29, 2026
@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

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

No duplicate mechanism exists — _ssl_compat is the single owner of the child-process trust bootstrap, and this change extends it in place using the existing atomic_write(restrict_to_owner=) helper. My verification is complete; here is the review.

First-Principles-Verdict: CONCERNS

The Windows trust fix is derived and singular, but the diff ships a no-op reformat and claims a protection it never adds — trust/ was already gated.

What this change ships

Intent: let Windows installs behind TLS-inspecting corporate proxies connect, by giving kiro-cli/MCP children a verified CA bundle reflecting Windows trust. FIX.

  1. Windows startup publishes trust/ca-bundle.pem (certifi + ROOT, minus Disallowed) and points children at it — justified (named defect, no existing mechanism: 0 other enum_certificates in src)
  2. Operator SSL_CERT_FILE / REQUESTS_CA_BUNDLE still win — justified (existing escape hatch preserved)
  3. New env marker KIROCREW_MANAGED_CA_BUNDLE so a re-exec refreshes instead of freezing trust — justified (3 real re-exec sites: slack/gateway.py:8746,9654, updates.py:1446)
  4. Store-read failure publishes nothing, falls back to certifi, logged as degraded — justified (keeps --help importable)
  5. Bundle protection = placement inside pre-gated trust/ — justified, but description misframes it as added (see Watch)
  6. Deny message "write-protected config path" → "write-protected path" — rides along (defensible: tier already held non-config rotation.yaml, security.py:5273)
  7. security.py tuple reformat, zero behavior change — rides along, undeclared
  8. certifi import hoisted to guarded module scope — justified (needed by the builder)
  9. Specs updated in same commit — mandated (AGENTS.md)

Watch

  • Description says "Add ca-bundle.pem to both agent file-edit and home-anchored shell write protection," but the diff adds no protection code — trust has been a whole-directory read+write gate since before this PR (security.py:4959: "future trust-root material is covered without a new entry"). The new test merely pins it; the spec text honestly says "existing read+write-blocked." A human should read the security.py/hooks.py hunks knowing they are cosmetic.
  • The bundle never reaches the write-only branch the relabel touches — it is denied earlier as a sensitive path (the PR's own test asserts reason "sensitive path"), so item 6 is an independent micro-fix, not part of this defect.

Subtractions

  • Drop the security.py:5255 hunk — it only re-wraps ("config.json", "config.local.json", "playwright-cli-config.json") across lines, adds nothing, and pads a security-file diff.

[FIRST-PRINCIPLES-REVIEWED] b252d7e

@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — 🔴 changes requested (blocking)

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

BLOCKING -- src/kiro_crew/_ssl_compat.py:247 -- Managed CA path bypasses the sensitive-path gate
os.environ["SSL_CERT_FILE"] = str(bundle_path)
Windows startup -> ACP child inherits this alias -> Add-Content -Path $env:SSL_CERT_FILE passes the command gate -> later children trust an agent-added CA.
Anchor: backend-security-controls
Fix: Revert this export until indirect environment-variable writes are denied.
[BLOCK-MERGE] b252d7e
[GPT-REVIEWED] b252d7e

@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — ✅ PASS

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

Import chain is clean (config.paths is a stdlib-only leaf; atomic_writeplatform_compat pulls no HTTPS client), the trust/ directory was already read+write-blocked so the bundle inherits the keystone gate by placement rather than new matcher surface, and the re-exec marker fails safe (a forged marker triggers a fresh store read, never adoption of an inherited file). Failure paths all degrade to the pre-existing verified certifi route and the degradation is documented as such.

Design-Verdict: PASS

Child processes need a file, not a monkey-patch — the export-and-filter design is the only viable shape, and every failure path degrades to the previously-shipped verified route.

Suggestions

  • Every Windows process start (CLI, gatewayd, each spawned subagent via the reentry marker) enumerates both stores and atomically rewrites the bundle; comparing the built bytes against the existing file and skipping the write when identical would cut per-invocation churn and avoid the Windows rename-retry window (_REPLACE_MAX_ATTEMPTS) that AV scanners trip on short-lived commands.

[DESIGN-REVIEWED] b252d7e

@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] b252d7e

@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 29, 2026
@bolichen97
bolichen97 force-pushed the fix/windows-ssl-corporate-proxy branch from 9cf4db1 to 2b34625 Compare August 29, 2026 21:43
@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 29, 2026
@bolichen97
bolichen97 force-pushed the fix/windows-ssl-corporate-proxy branch from 2b34625 to 3f020d0 Compare August 29, 2026 22:41
@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 29, 2026
@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 merge conflict Branch has merge conflicts with its base — author must resolve before merge labels Aug 30, 2026
@bolichen97
bolichen97 force-pushed the fix/windows-ssl-corporate-proxy branch from 07adc8b to b252d7e Compare August 30, 2026 07:22
@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 30, 2026
Export HTTPS-capable trusted roots into an owner-only CA bundle so kiro-cli can connect through Windows TLS-inspecting proxies. Preserve explicit operator bundles and fall back without disabling verification.
@bolichen97
bolichen97 force-pushed the fix/windows-ssl-corporate-proxy branch from b252d7e to e78e721 Compare August 30, 2026 08:26
@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 30, 2026
@bolichen97
bolichen97 disabled auto-merge September 3, 2026 21:26
@bolichen97
bolichen97 enabled auto-merge (squash) September 3, 2026 21:26
@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

  • This PR is OVERLAPPING with PR #5903. 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.
  • This PR is OVERLAPPING with PR #7140. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #6230: REBASE. Both should land. Whichever merges second should carry WSLENV forwarding (or DrvFs translation) for the managed bundle variables so the Windows corporate-proxy fix survives under agent.sandbox: 'wsl2'. Files: src/kiro_crew/sandbox.py, src/kiro_crew/hooks.py.
  • This PR is OVERLAPPING with PR #7906. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #6230: REBASE. The rename is textually conflict-free (git merge-tree against current origin/main auto-merges all four code files with no conflict) but semantically stale against code merged after the PR's base. On rebase, either keep the literal 'write-protected config path' phrasing or add both the generalized phrase and the TLS-alias reason to deny_guidance._CLASS_ANCHORS under DENY_CLASS_TRUST_ROOT. Files: src/kiro_crew/hooks.py.

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

@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 5, 2026
@dwu96 dwu96 added the needs-pr-triage PR scanner: awaiting automated triage label Sep 8, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator

Rebase onto main 9af9543b0 attempted by a maintainer as part of the 2026-09-08 open-PR audit. Stopped — this needs an author decision, so your branch is untouched.

Two blockers:

_ssl_compat.py, the docs, and test_config_loader.py rebase cleanly. Reply if this reading looks wrong.

@dwu96 dwu96 added needs-author-decision PR blocked on author input and removed needs-pr-triage PR scanner: awaiting automated triage labels Sep 8, 2026
@dwu96

dwu96 commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

🤖 Kiro Crew [operator: dwu96#8c08bee0]: This PR has been inactive for 7+ days. I reviewed the blockers but they require your input:

  • The security gate this PR's blocking-finding fix attaches to no longer exists. The GPT 5.6 lane's [BLOCK-MERGE] on _ssl_compat.py:247 ("managed CA path bypasses the sensitive-path gate") was answered by the _TLS_TRUST_PATH_ENV_RE early return in is_sensitive_bash_command. On current main that mechanism is gone: refactor(security): drop the regex traversal simulation from the bash gate #9089 deliberately stopped matching paths in command text (_get_sensitive_re() and _RELATIVE_SENSITIVE_RE removed) and refactor(security): split security.py into a package and drop path regex #9183 split src/kiro_crew/security.py into src/kiro_crew/security/. Fencing the three TLS aliases needs a new enforcement point, and choosing where that gate lives — and how wide it denies — is a security-policy decision that is yours (and the maintainers'), not something automation should pick.
  • hooks.py deny-message rename conflicts with a landed literal anchor. Renaming the deny to write-protected path breaks the literal anchor at deny_guidance.py:89 added by fix: key deny remediation by rule identity in the regex tier #7906. Mechanical on its own, but the correct wording depends on the enforcement-point decision above.
  • Windows-only surface I cannot verify here. The substantive change enumerates the Windows ROOT/Disallowed stores; this pipeline runs on Linux, so I cannot exercise the real store paths or reproduce your manual verification. That part needs a Windows host.

Two notes so you are not chasing the wrong things:

  • The reviews on this PR (GPT 🔴 blocking, First Principles 🟡 CONCERNS, Design ✅, Opus ✅) all ran against b252d7ed, which predates your current head e78e721f0 — they are stale, not a verdict on the code as it stands.
  • The only failing CI shard is Backend Tests (Windows) (3), and its single failure is test/test_mcp_gatewayd_coverage.py::TestZombieDiagnostic::test_dead_accept_loop_is_dumped_and_stops_the_daemon (FileNotFoundError on diag.jsonl). This PR touches neither that test nor mcp_gateway, so it is not your regression. Everything else is green; PR Readiness is blocked by the merge conflict, not by a test.

A maintainer already attempted the rebase onto 9af9543b0 and stopped at the same first blocker, leaving your branch untouched. I am not duplicating that attempt — this note only records the triage outcome and hands the decision back to you.

When you've addressed these, the pipeline will re-assess on its next cycle. Add pr-no-autofix if you'd prefer no automated involvement at all.

@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 Sep 8, 2026
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) merge conflict Branch has merge conflicts with its base — author must resolve before merge needs-author-decision PR blocked on author input readiness: checking Automated validation is still running

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants