fix(tls): use system trust store by default#2022
Closed
WilliamK112 wants to merge 1 commit into
Closed
Conversation
WilliamK112
requested review from
danielmeppiel and
sergio-sisternes-epam
as code owners
Collaborator
|
Thank you for this, @WilliamK112 -- it was a genuinely strong implementation of the #2004 fix. We ran a comparative review-panel pass across both open PRs for this issue (this one and #2005) with a CEO arbitration on which to drive to merge. The decision was to land #2005 as the base, primarily on three points: it ships a real private-CA end-to-end integration test that proves verification is not weakened (untrusted CA still rejected after injection), it keeps the CLI starting on a broken/incompatible That said, your PR was better on several axes, and we are folding those ideas into #2005 with attribution on the commits:
Closing in favor of #2005. Your contribution is credited via |
danielmeppiel
added a commit
to fangkangmi/apm
that referenced
this pull request
Jul 5, 2026
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>
danielmeppiel
added a commit
that referenced
this pull request
Jul 12, 2026
#2005) * feat(tls): verify against the OS trust store by default (#2004) 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. * test(tls): integration test for verification against a custom CA (#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. * fix(tls): don't let SSL_CERT_FILE suppress OS-trust injection (#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> * test(tls): drive the real frozen SSL runtime hook (#2004) 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> * docs(tls): tighten comments to repo style, fix stale SSL_CERT_FILE docstring 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> * docs(tls): address PR review feedback - 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 #2004) (#2005)", per repo convention. * fix(tls): fold OS trust follow-ups 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 #2005 and #2022. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Ching Wei Kang <WilliamK112@users.noreply.github.com> * docs(tls): complete trust environment guidance 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> * docs(tls): link enterprise proxy TLS guidance 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> * test(tls): isolate custom CA server SSL context 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> * fix(tls): use OS store in frozen binary, add trust-source diagnostics, 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> * fix(tls): propagate OS-store trust into runtime child processes 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> * test(tls): independent e2e verification of B1/B2/H2 OS-trust remediation Adversarial end-to-end tests authored from the PR #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> * fix(tls): deliver OS-trust to child venvs via install-time .pth bootstrap 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 #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> * test(tls): independent round-2 verification of foreign-venv OS-trust delivery Adds a from-scratch verifier suite (V1-V6) proving PR #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> * fix(tls): generate child .pth inline + package-data, harden child-trust delivery (#2005) Round-3 remediation of PR #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> * test(tls): independent round-3 verification of wheel-shipped OS-trust delivery Independently proves PR #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> * fix(tls): round-4 red-team LOW folds (certifi boundary, macOS py3.9 honesty, 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> * chore(notice): add truststore attribution + waive spec Mode-B for TLS 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> * test(tls): pin TLSv1.2 floor on loopback CA test servers 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> * fix(tls): replay trust source under verbose logging Cache the early TLS decision in its canonical owner and replay it after CLI logging is configured, without delaying security-sensitive injection. Addresses the CLI logging and architecture panel follow-up. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(tls): harden llm bootstrap delivery feedback Prove RuntimeManager delivers the child bootstrap, distinguish Python and proxy setup failures, and keep unexpected best-effort errors visible under debug logging. Addresses the test coverage, DevX, and CLI logging panel follow-ups. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs(tls): document enterprise transport trust Add the security-model boundary, lead troubleshooting with the OS-store fix, remove duplicate runtime caveats, and avoid advertising an unimplemented bundle variable. Addresses the security, growth, and documentation panel follow-ups. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(tls): complete canonical trust guardrails Fence truststore injection to its two owners, lock parent-child environment policy parity, normalize child diagnostics and subprocess env hygiene, and lead the changelog with user value. Addresses the reinforcement panel follow-ups. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(tls): contain bootstrap writes and exact cert paths Reject child site-packages symlink escapes, preserve user SSL_CERT_FILE paths unless they exactly match a recorded bundled cert, and correct verification and planned-scope docs. Addresses the terminal security, architecture, and documentation follow-ups. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * chore(ci): retrigger checks for PR #2005 The synchronize webhook for the preceding head produced no workflow runs; this empty commit retriggers the required checks without changing the tree. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs(changelog): preserve unreleased separator Keep the PR #2005 entry separated from the newly cut v0.25.0 release block after merging current main. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: danielmeppiel <danielmeppiel@users.noreply.github.com> Co-authored-by: Ching Wei Kang <WilliamK112@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #2004.
truststoreas a runtime dependency so APM uses the operating system trust store by default for Python HTTPS clientsREQUESTS_CA_BUNDLE,CURL_CA_BUNDLE,SSL_CERT_FILE,SSL_CERT_DIR)REQUESTS_CA_BUNDLEoverride pathValidation
uv run --extra dev pytest tests/unit/test_tls_truststore.py tests/unit/test_ssl_cert_hook.py tests/unit/install/test_validation_tls.py -q-> 27 passeduv run --extra dev pytest tests/unit/test_cli_consistency.py tests/unit/test_install_command.py tests/unit/test_registry_client.py tests/unit/test_registry_client_http_cache.py tests/unit/test_ssl_cert_hook.py tests/unit/test_tls_truststore.py tests/unit/install/test_install_cmd_auth_rendering.py tests/unit/commands/test_install_error_handling.py tests/unit/marketplace/test_marketplace_commands.py tests/unit/marketplace/test_marketplace_resolver.py tests/unit/marketplace/test_client_git.py tests/unit/marketplace/test_marketplace_client.py tests/unit/registry/test_client.py tests/unit/registry/test_resolver_e2e_http.py tests/integration/test_install_validation_rules.py tests/integration/test_marketplace_add_generic_git.py -q-> 496 passed, 10 subtests passeduv run --extra dev ruff check src tests-> passeduv run --extra dev ruff format --check src tests-> 1388 files already formatteduv lock --check-> passeduv run apm --help-> passednpm run build --prefix docs-> passed; internal links valid; only existing Astro/Vite warnings were reportedgit diff --check-> passedNot counted as passing:
uv run --extra dev pytest tests/unit -qexceeded a 5 minute local timeout without useful progress output, so I stopped the leftover pytest processes and used the targeted TLS/CLI/install/marketplace/registry validation above instead.