feat(tls): verify against the OS trust store by default (closes #2004)#2005
feat(tls): verify against the OS trust store by default (closes #2004)#2005fangkangmi wants to merge 24 commits into
Conversation
|
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
Pull request overview
Adds best-effort OS trust-store TLS verification to align APM’s requests HTTPS behavior with git/curl in enterprise (corporate CA / TLS-inspecting proxy) environments, while preserving explicit CA-bundle overrides and providing an opt-out.
Changes:
- Introduces
apm_cli.core.tls_trust.configure_tls_trust()(truststore injection with safe fallback to certifi) and runs it at CLI startup. - Adds
truststoreas a runtime dependency and ensures it is included in the PyInstaller build. - Adds unit + integration coverage (custom CA loopback server + frozen runtime hook regression) and updates SSL troubleshooting docs + changelog.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Locks truststore into the resolved dependency set. |
| pyproject.toml | Declares truststore>=0.9.1 as a runtime dependency. |
| build/apm.spec | Adds truststore to PyInstaller hidden imports for frozen builds. |
| src/apm_cli/core/tls_trust.py | Implements best-effort OS trust-store injection with override/opt-out behavior. |
| src/apm_cli/cli.py | Calls configure_tls_trust() early at process startup before HTTPS usage. |
| tests/unit/core/test_tls_trust.py | Unit coverage for all decision branches of configure_tls_trust(). |
| tests/integration/test_tls_custom_ca.py | End-to-end TLS verification behavior tests with a freshly minted private CA. |
| tests/integration/test_tls_frozen_hook.py | Regression coverage ensuring the frozen runtime hook’s SSL_CERT_FILE does not suppress injection. |
| docs/src/content/docs/troubleshooting/ssl-issues.md | Documents new default behavior + override/opt-out details. |
| CHANGELOG.md | Records the behavioral change under Unreleased. |
- test_tls_trust docstring: drop the stale SSL_CERT_FILE-suppresses claim and note it explicitly does NOT suppress injection (matches the code + its test). - ssl-issues.md / CHANGELOG: normalise em dashes to ASCII "--" to match the surrounding docs. - CHANGELOG: reference the PR, "(closes microsoft#2004) (microsoft#2005)", per repo convention.
APM verified HTTPS against the bundled certifi CA set, which omits internal/corporate root CAs and TLS-proxy certs. Since APM also shells out to git (which reads the OS trust store), `git clone` of an internal host succeeded while APM's requests-based Contents API calls failed against the same chain -- a confusing enterprise first-run failure. Route requests' TLS verification through the OS trust store via truststore, injected once at CLI startup. Best-effort and non-regressive: - An explicit REQUESTS_CA_BUNDLE / CURL_CA_BUNDLE / SSL_CERT_FILE still wins (we skip injection so a pinned bundle is honoured verbatim). - APM_DISABLE_TRUSTSTORE=1 restores the legacy certifi-only behaviour. - Falls back to certifi if truststore is missing or injection raises; configure_tls_trust() never raises. Adds truststore to dependencies and the PyInstaller hiddenimports so the frozen binary ships it, documents the new default in the SSL troubleshooting guide, and covers every branch with unit tests.
…rosoft#2004) Completes the triage panel's checklist item: exercise the real requests -> urllib3 -> ssl stack against a loopback HTTPS server whose leaf is signed by a freshly minted private CA (in no trust store). Covers: untrusted CA is rejected (verification is genuinely on); an explicit REQUESTS_CA_BUNDLE is honoured end-to-end while configure_tls_trust() declines to override it; and truststore injection keeps verification on (a CA absent from the OS store is still rejected, proving injection redirects trust rather than disabling it). Skips where the openssl CLI is unavailable; isolates the global ssl / truststore mutation per test.
…oft#2004) Review follow-up. The frozen binary's runtime hook (build/hooks/runtime_hook_ssl_certs.py) sets SSL_CERT_FILE to the bundled certifi before app code runs. configure_tls_trust() treated any SSL_CERT_FILE as an explicit override and skipped truststore -- so OS-trust injection was a no-op in exactly the shipped artifact this feature targets. requests only consults REQUESTS_CA_BUNDLE / CURL_CA_BUNDLE for its verify bundle (not SSL_CERT_FILE), so: - Drop SSL_CERT_FILE from the override set; injection now runs in the frozen binary, and a lone SSL_CERT_FILE no longer misleadingly "wins". - Broaden the truststore import guard from ImportError to Exception so a broken/incompatible install degrades to certifi instead of crashing startup. - Correct the docs and changelog (REQUESTS_CA_BUNDLE / CURL_CA_BUNDLE are the HTTP-layer overrides; SSL_CERT_FILE is not read by requests). - Add a unit regression guard (SSL_CERT_FILE set still injects) and cover CURL_CA_BUNDLE in the integration precedence test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Regression guard for the no-op found in review: execute the actual build/hooks/runtime_hook_ssl_certs.py under a simulated frozen process and assert configure_tls_trust() still injects truststore when the hook has pinned SSL_CERT_FILE to the bundled certifi -- and that a user-set REQUESTS_CA_BUNDLE still wins (no injection) in the same frozen state. Uses a manual env/ssl/sys.frozen snapshot fixture because the hook mutates os.environ directly, which monkeypatch would not track. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…cstring Trim the module docstring and inline comments to match the density of comparable core modules; drop two comments that restated the code. Also correct the docstring, which still listed SSL_CERT_FILE among the overrides that "win" after it was removed from the skip set. Keep the frozen-hook and never-raises rationale (load-bearing, matches validation.py's TLS comments). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- test_tls_trust docstring: drop the stale SSL_CERT_FILE-suppresses claim and note it explicitly does NOT suppress injection (matches the code + its test). - ssl-issues.md / CHANGELOG: normalise em dashes to ASCII "--" to match the surrounding docs. - CHANGELOG: reference the PR, "(closes microsoft#2004) (microsoft#2005)", per repo convention.
Updates the TLS failure guidance, truststore floor, override detection seam, docs, and import-timing regression coverage for the OS trust-store feature. Addresses the shepherd-driver fold set from the comparative review of microsoft#2005 and microsoft#2022. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Ching Wei Kang <WilliamK112@users.noreply.github.com>
Adds the TLS trust environment variables to the canonical reference, aligns the install-failures page with the OS trust-store default, and locks the CLI TLS bootstrap idempotency guard with a unit regression test. Addresses apm-review-panel doc, DevX, growth, and coverage follow-ups. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Adds the OS trust-store TLS troubleshooting entry to the enterprise registry-proxy page so corporate proxy users reach the same CA guidance from the page they are likely to read first. Addresses the final growth-panel nit. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Ensures the private-CA loopback server creates its server-side SSL context from the stdlib context even when broader integration collection has already imported the CLI and installed truststore globally. This keeps the end-to-end TLS test runnable alongside install and marketplace integration coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
danielmeppiel
force-pushed
the
feat/truststore-os-trust
branch
from
98a9755 to
503e698
Compare
|
@fangkangmi Final apm-review-panel convergence pass for #2005. CEO recommendationship_now: the OS trust-store TLS feature is ready for maintainer review. The panel converged with zero remaining foldable items after the shepherd folds below. The performance lazy-injection idea was not adopted because the maintainer explicitly required startup-level injection before Folded in this run
Copilot signals reviewed
Regression-trap evidence (mutation-break gate)
Lint contract
CI and local validationGitHub reported no check suite for the pushed fork head (
Mergeability statusCaptured from
Convergence2 outer iterations; 2 Copilot fetch rounds. Final panel stance: |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| context.load_cert_chain(certfile=str(srv_pem), keyfile=str(srv_key)) | ||
|
|
||
| httpd = http.server.HTTPServer(("127.0.0.1", 0), _OkHandler) | ||
| httpd.socket = context.wrap_socket(httpd.socket, server_side=True) |
…, add child-env shim B2: the frozen runtime hook set SSL_CERT_FILE=certifi.where() before app code, and truststore's Linux backend honors SSL_CERT_FILE via set_default_verify_paths, so the injected context loaded certifi instead of the OS store. The hook now also sets APM_SSL_CERT_FILE_IS_BUNDLED_DEFAULT=1 to mark that WE set the default. configure_tls_trust pops that bundled SSL_CERT_FILE before inject_into_ssl so the system default is read, restores it on injection failure (never zero trust), and clears the marker so it does not leak into children. A genuine user SSL_CERT_FILE (no marker) is left untouched. H2: each branch now emits one ASCII-only, greppable trust-source line via the module logger (debug): OS trust store, certifi fallback, explicit CA bundle, or disabled. B1 (core): add build_child_tls_env() and a silent, never-raise sitecustomize shim under apm_cli/core/_child_tls/. build_child_tls_env prepends the shim dir to a child PYTHONPATH so each Python child re-runs configure_tls_trust at its own interpreter startup (single source of truth; no logic duplicated). The shim ships in the frozen binary via apm.spec datas. Docs + CHANGELOG updated. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Wire build_child_tls_env into every runtime child-spawn surface so each Python child runtime re-runs the OS-trust bootstrap: - base._stream_subprocess_output gains an env param; defaults to build_child_tls_env(os.environ) and passes env= to Popen (covers copilot and llm prompt-execution paths). - codex_runtime prompt-execution Popen passes the child TLS env. - llm_runtime models-list subprocess passes the child TLS env (the --version availability probes make no HTTPS call and are left as-is to avoid regressing their pinned unit-test kwargs). - manager wraps the runtime-setup child env as build_child_tls_env(setup_runtime_environment(env)). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Adversarial end-to-end tests authored from the PR microsoft#2005 acceptance specs, proving the fixes empirically rather than asserting a function was called. B1 (tests/integration/test_tls_child_runtime.py): OS-store trust crosses the process boundary into a real child via build_child_tls_env -- the shim runs at child startup (ssl.SSLContext becomes truststore-backed vs stdlib "ssl"), a real requests.get against a private-CA server succeeds only through the env-delivered trust (control fails SSLERROR), and APM_DISABLE_TRUSTSTORE suppresses the shim. No shim stdout/stderr contamination. B2 (tests/integration/test_tls_frozen_hook.py): the bundled-default SSL_CERT_FILE is popped before injection (marker-gated), a genuine user override is preserved and honored end-to-end, and an inject failure restores the certifi path. H2 (tests/unit/core/test_tls_trust.py): each trust-source branch emits its exact ASCII diagnostic line at DEBUG. Shared private-CA server harness (tests/integration/_tls_ca_server.py) reuses the proven cert factory and guards against global truststore injection bleed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
# Conflicts: # CHANGELOG.md
Remediation cycle: red-team blockers folded + empirically verifiedA CEO-arbitrated red-team of four adversarial reviewers (enterprise-proxy, SSL/CA/truststore internals, npm/uv/pip precedent, Python ssl/httpx integration) analyzed this PR and surfaced two release blockers plus supporting concerns. This cycle folds the must-fix set and proves each fix empirically. Commits: Fixed and verified
Tests authored by an independent verifier (not the implementer): Deferred as fast-follow (filed)
Node-based runtimes are not covered by the child-shim propagation (Python |
…trap The round-1 child-trust design prepended a sitecustomize shim dir to each child's PYTHONPATH so it re-ran apm_cli.core.tls_trust.configure_tls_trust. That was a silent no-op for the flagship `llm` runtime, which runs in its own venv (~/.apm/runtimes/llm-venv) that has neither apm_cli nor truststore: the shim's `from apm_cli...` import failed, was swallowed, and the child fell back to certifi -- so `apm run` still failed behind a corporate proxy. Prepending the shim dir also shadowed any user/corporate sitecustomize.py. Switch to CEO-approved Mechanism 2a: deliver trust at venv-setup time. - New self-contained bootstrap (_apm_tls_bootstrap.py) with ZERO apm_cli dependency (only truststore) plus a one-line .pth (_apm_tls.pth) that Python executes at interpreter startup. Both ship as package data / apm.spec datas. - setup-llm.sh / setup-llm.ps1 now `pip install truststore` into the llm venv; RuntimeManager copies the two bootstrap files into that venv's site-packages via a new Python helper ensure_child_tls_bootstrap() (Python-driven so it resolves identically for source and frozen apm; no fragile shell globbing). - build_child_tls_env() no longer mutates PYTHONPATH (kills the sitecustomize hijack); it is now an env-hygiene pass that only strips the internal bundled-cert marker. - configure_tls_trust() clears the bundled-default marker unconditionally, up-front, so it can no longer leak on the opt-out / explicit-override / truststore-import-fail early returns. - Delete the old sitecustomize.py shim; update apm.spec datas. - Honest scope-down in CHANGELOG + ssl-issues.md: OS-trust covers `apm install` and the Python `llm` runtime; Node (Copilot) / Rust (Codex) runtimes are not yet covered (tracked in microsoft#2034), with a Known limitations note. - Tests: foreign-venv regression gate (C1, offline via copied truststore), additive-to-user-sitecustomize (T2), marker no-leak across all 5 configure_tls_trust branches (T4), build_child_tls_env hygiene, delivery helper (T7), and a docs-scope drift guard (T3). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…delivery Adds a from-scratch verifier suite (V1-V6) proving PR microsoft#2005's install-time .pth bootstrap delivers OS-trust into a genuine FOREIGN venv that cannot import apm_cli -- the exact field scenario the round-1 PYTHONPATH/sitecustomize shim silently no-op'd on. Independent assertions (own probes/sentinels), not a reuse of the implementer's tests: - V1: foreign venv (no apm_cli) + shipped bootstrap -> truststore-backed ssl; remove the .pth -> reverts to stdlib ssl (asymmetry is the proof). Bootstrap carries zero apm_cli imports. - V2: a pre-existing user sitecustomize.py still runs AND truststore injects (no hijack). - V3: ensure_child_tls_bootstrap lands both files in a real venv and that interpreter injects while apm_cli stays unimportable. - V4: marker cleared on all 5 configure_tls_trust branches; bundled SSL_CERT_FILE popped before a successful inject, restored when inject raises. - V6: build_child_tls_env strips the marker with no PYTHONPATH mutation. Offline-by-design (truststore copied from the dev env, not pip-installed). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Round-2 red-team remediation folded (child-trust delivery rearchitected)A second adversarial red-team pass (4 enterprise-TLS/PKI + package-manager reviewers, CEO-arbitrated) found that the round-1 child-propagation mechanism was a silent no-op in the shipped layout: the What changed
Verification (independent)A separate verifier wrote foreign-venv e2e tests from scratch proving: trust injects in a venv that cannot import Parent-side |
…st delivery (microsoft#2005) Round-3 remediation of PR microsoft#2005: - H1: generate _apm_tls.pth inline in ensure_child_tls_bootstrap so child trust no longer depends on the .pth being packaged (setuptools packages.find dropped it from the wheel). Add [tool.setuptools.package-data] belt-and- suspenders + a wheel-content regression test. - M1: best-effort, pinned (truststore>=0.10.0) install in setup-llm.sh/.ps1 so a failed install no longer aborts llm setup under set -e. - M2: build_child_tls_env drops a BUNDLED certifi SSL_CERT_FILE so the child truststore reaches the OS store on Linux; a genuine user value is preserved. - M3: write the bootstrap module + .pth atomically (temp + os.replace, module first) so a failed write leaves no truncated module under a live .pth. - M5/L1/M4-docs: surface the Node/Codex caveat early, scope CHANGELOG to Python-based, note REQUESTS_CA_BUNDLE replaces the OS store + stale-bundle hint, document the PIP_CERT setup caveat. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
… delivery Independently proves PR microsoft#2005 round-3 fixes on the surface the HIGH bug lived on: the built wheel now ships _apm_tls.pth, a pip-installed wheel delivers the bootstrap into a foreign venv, and that interpreter injects truststore (with .pth removal as the negative control). Also covers the M2 certifi env-hygiene, M3 atomic no-partial write + module-before-pth ordering, M1 best-effort truststore install, and M5/L1/M4 docs scope. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Round-3 red-team remediation folded (packaging-channel fix + delivery hardening)A third adversarial red-team pass (4 enterprise-TLS / packaging / proxy reviewers, CEO-arbitrated) found the round-2 rearchitecture was correct in design but undelivered on the PyPI channel, plus a set of correctness/robustness gaps. All folded and independently verified against the built wheel. HIGH -- the child-trust
|
…onesty, sdist guard) Round-4 confirmation red team returned SHIP_NOW on all four lenses (no new HIGH/CRITICAL). Folds the surfaced LOW residuals that sharpen the round-3 surfaces: - F1 (ssl): _is_bundled_certifi matched on a raw string suffix, so a genuine user bundle at e.g. /opt/mycertifi/cacert.pem was wrongly dropped from the child env. Match on path COMPONENTS (last two = certifi/cacert.pem) instead. Regression test covers lookalike dirs + the genuine boundary. - FINDING 1 (proxy): truststore>=0.10.0 needs Python 3.10+, but setup-llm builds the llm venv from whatever python3 resolves to; stock macOS python3 is 3.9 -> silent certifi fallback behind a proxy. Name the Python-version cause in the setup-llm.sh/.ps1 warning and add a Known-limitations caveat to ssl-issues.md. - LOW-1 (pkgmgr): add an sdist-content regression test paralleling the wheel guard, so a future setuptools bump that drops the .pth from the sdist is caught (the bootstrap module must always ship; the .pth rides on package-data). - LOW-2 (pkgmgr): correct the stale "copies the .pth" docstring in _install_llm_tls_bootstrap (the .pth is generated inline; only the module is copied). Lint 10.00/10; 69 TLS tests green (incl. 2 new). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Red-team round 4 (confirmation pass) -- SHIP_NOW, loop closedRan the same four-expert adversarial panel a fourth time against the round-3 head, each with a dual mandate: confirm the round-3 fixes hold against the CEO ship bar, and hunt for any new high/critical in the folded surfaces. All four lenses returned
LOW residuals folded (e8993be)None were blocking; folded because they cheaply sharpen the round-3 surfaces:
Deferred (tracked, non-blocking)
Four remediation cycles complete (rounds 1-3 fixed 2 CRITICAL + 1 HIGH + several MEDIUMs; round 4 is clean). Lint 10.00/10 across all four gates. |
| context.load_cert_chain(certfile=str(srv_pem), keyfile=str(srv_key)) | ||
|
|
||
| httpd = http.server.HTTPServer(("127.0.0.1", 0), _OkHandler) | ||
| httpd.socket = context.wrap_socket(httpd.socket, server_side=True) |
# Conflicts: # CHANGELOG.md
… trust Adding `truststore` as a runtime dependency requires a curated NOTICE metadata block (Microsoft CELA manual-NOTICE process) -- the NOTICE Drift Check gate fails without one. Adds the truststore component (MIT, (c) 2022 Seth Michael Larson, github.com/sethmlarson/truststore) to scripts/notice-metadata.yaml and regenerates NOTICE. The PR touches OpenAPM runtime/install critical paths, tripping the spec Mode-B silent-extension detector. TLS transport trust is orthogonal to the package-format normative surface, so a waiver is the correct path. apm-spec-waiver: TLS transport trust is orthogonal to the OpenAPM package-format normative surface; no new manifest/lockfile/resolver/policy/registry/runtime-contract requirement is introduced by this HTTPS trust change. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
CodeQL py/insecure-protocol flagged two new HIGH alerts: the loopback HTTPS test servers in _tls_ca_server.py and test_tls_custom_ca.py build an ssl.SSLContext(PROTOCOL_TLS_SERVER) whose default still permits TLSv1/TLSv1.1. Set context.minimum_version = TLSv1_2 on both so the scanner stays clean; modern clients already negotiate 1.2/1.3 so the handshake tests are unaffected. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Summary
By default,
apmnow verifies HTTPS against the operating-system trust store (viatruststore) — the same sourcegitandcurluse — instead of the bundledcertifiset. This makesapmwork out-of-the-box behind a corporate CA or a TLS-inspecting proxy (closes #2004).Coverage is the Python-based paths:
apm install(in-process) and the Pythonllmchild runtime spawned byapm run(a self-contained.pthbootstrap +truststoreare installed into its venv at setup time). The frozen binary honours the system store as well, withcertifias a genuine fallback. An explicitREQUESTS_CA_BUNDLE/CURL_CA_BUNDLEstill wins, andAPM_DISABLE_TRUSTSTORE=1restores the previous certifi-only behaviour. Node (Copilot) and Rust (Codex) child runtimes are not yet covered — tracked in #2034.Type of change
Testing
Added unit tests (
tests/unit/core/test_tls_trust.py, every branch ofconfigure_tls_trust()), an integration test that verifies against a freshly-minted private CA over a loopback HTTPS server (tests/integration/test_tls_custom_ca.py), and a regression test that drives the real frozen SSL runtime hook (tests/integration/test_tls_frozen_hook.py). The child-delivery chain is covered end-to-end: wheel/sdist content guards (tests/integration/test_tls_wheel_content.py), foreign-venv.pthinjection (tests/integration/test_tls_child_runtime.py), and the round-2/round-3 verification suites. 69 TLS tests green.Also validated the end-to-end effect by mimicking the enterprise condition (corporate CA present in the OS trust store but absent from certifi):
Not yet exercised on a real PyInstaller build; a
make buildsmoke against a custom CA on Linux is recommended before release (the code degrades safely to certifi regardless).Hardening
Four adversarial red-team + remediation cycles (enterprise-TLS/CA, packaging/distribution, proxy/runtime, python-integration lenses) fixed 2 CRITICAL (child propagation; frozen
SSL_CERT_FILEhandling), 1 HIGH (the.pthwas dropped from the wheel), and several MEDIUM/LOW items (best-effort pinnedtruststoreinstall, bundled-certifi child drop, atomic bootstrap write, certifi component-boundary match, honest macOS-py3.9 docs caveat). The round-4 confirmation pass returnedSHIP_NOWon all four lenses.NOTICE
truststore(MIT, © 2022 Seth Michael Larson) is added toscripts/notice-metadata.yamlandNOTICEis regenerated per the Microsoft CELA manual-NOTICE process.Spec conformance (OpenAPM v0.1)
apm-spec-waiver: TLS transport trust is orthogonal to the OpenAPM package-format normative surface; no new manifest/lockfile/resolver/policy/registry/runtime-contract requirement is introduced by this HTTPS trust change.