feat(tls): verify against the OS trust store by default (closes #2004)#2005
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>
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: |
…, 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. |
# 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>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 1 | 1 | The TLS owner and fallback chain are clean; preserve early setup while centralizing observable outcome. |
| CLI Logging Expert | 0 | 3 | 1 | Trust-source diagnostics are lost before verbose logging, and bootstrap failure guidance needs a next step. |
| DevX UX Expert | 0 | 1 | 2 | The OS-store mental model is strong; setup diagnostics must distinguish proxy and Python-version failures. |
| Supply Chain Security Expert | 0 | 2 | 2 | Verification remains enabled, delivery is atomic, and internal markers do not leak. |
| OSS Growth Hacker | 0 | 0 | 1 | The enterprise docs funnel is honest and useful. |
| Auth Expert | 0 | 0 | 0 | Tokens, AuthResolver, credential precedence, and authorization headers are untouched. |
| Doc Writer | 0 | 2 | 1 | Docs match implementation; remove duplication and avoid naming an unimplemented variable as available. |
| Test Coverage Expert | 0 | 1 | 1 | Critical surfaces have strong integration evidence; one RuntimeManager wiring gap remains. |
| Performance Expert | 0 | 1 | 3 | Per-invocation overhead is negligible; keep optional install failure isolated. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Test Coverage Expert] Add an integration test proving
RuntimeManager.setup_runtime("llm")delivers both TLS bootstrap files into the child venv -- this is the missing guard at the secure-by-default integration seam. - [CLI Logging Expert] Cache the selected trust source and re-emit it after verbose logging is configured -- setup must stay early, but
--verbosecurrently loses the diagnostic. - [Supply Chain Security Expert] Add a concise transport-trust section to the enterprise security page -- enterprise reviewers need the OS-store default, child bootstrap, overrides, and opt-out documented together.
- [DevX UX Expert] Differentiate Python-version and pip/proxy failures in the llm truststore setup warning, including
PIP_CERTrecovery guidance -- the target proxy user currently receives a misleading cause. - [Doc Writer] Remove the unimplemented
APM_EXTRA_CA_BUNDLEname or present it as an explicit planned feature -- current prose can be mistaken for a supported variable.
Architecture
classDiagram
class tls_trust
class RuntimeManager
class LLMRuntime
class cli_main
RuntimeManager ..> tls_trust : bootstrap
LLMRuntime ..> tls_trust : child env
cli_main ..> tls_trust : configure
flowchart TD
A["CLI import"] --> B{"override or opt-out?"}
B -->|yes| C["certifi or explicit bundle"]
B -->|no| D["truststore injection"]
E["Runtime setup"] --> F["child venv bootstrap"]
Recommendation
Fold the five focused follow-ups into this PR, then re-run the panel on the converged head. The core trust shift is sound, keeps verification enabled, and addresses a high-impact enterprise onboarding failure.
Full per-persona findings
Python Architect
- [recommended] Module-level TLS setup in
src/apm_cli/cli.pycouples behavior to import order. Preserve early injection, centralize the cached outcome in the TLS owner, and expose it for verbose replay. - [nit] The runtime subprocess helper now defaults every child to TLS env hygiene; the cost is negligible but the coupling should remain intentional.
CLI Logging Expert
- [recommended]
--verbosenever sees the selected trust source because setup runs before DEBUG logging and the later guarded call is a no-op. - [recommended] The llm bootstrap warning gives no recovery step or troubleshooting link.
- [recommended] A defensive bare
exceptsilently swallows any break in the helper's never-raise contract. - [nit]
[i]inside DEBUG messages duplicates the log level.
DevX UX Expert
- [recommended] The setup warning attributes all truststore install failures to Python version, masking the target user's pip/proxy chicken-and-egg failure.
- [nit] The hardcoded stock macOS Python 3.9 statement will age.
- [nit] A future
apm doctorTLS posture check could improve discoverability, but it is a separate command surface.
Supply Chain Security Expert
- [recommended] Child-venv truststore installation lacks hash constraints, matching the existing unhashed llm install surface; address the whole child-runtime dependency channel together.
- [recommended]
docs/src/content/docs/enterprise/security.mddoes not yet describe the new transport-trust model. - [nit] Bundled-certifi detection uses string path splitting; current boundary tests cover the relevant cases.
- [nit] The child truststore requirement has no upper bound; fallback limits the impact of future incompatibility.
OSS Growth Hacker
- [nit] Lead the SSL troubleshooting section with the one-line OS-store fix before explaining mechanism details.
Auth Expert
No findings.
Doc Writer
- [recommended] The Node/Codex caveat is repeated almost verbatim within one section; keep the early statement and cross-reference it later.
- [recommended]
APM_EXTRA_CA_BUNDLEis named as future configuration without the required planned-feature treatment. - [nit] The default-behavior paragraph combines too many facts for quick scanning.
Test Coverage Expert
- [recommended] No integration-with-fixtures test proves the
RuntimeManager.setup_runtime("llm")to child-bootstrap delivery chain. Existing leaf tests do not cover that wiring.
Proof (missing at):tests/integration/test_tls_runtime_manager_wiring.py::test_setup_runtime_llm_installs_tls_bootstrap-- proves that llm runtime setup installs the OS-trust bootstrap. [secure-by-default,devx] - [nit] The changed TLS warning test asserts bundle guidance but not the new system-trust-store wording.
Performance Expert
- [recommended] A separate truststore pip invocation adds a resolver pass, but combining it must not destroy best-effort semantics.
- [nit] Child env copies and bundled-certifi checks are negligible compared with subprocess launch.
- [nit] Early trust setup adds less than 5 ms cold startup and is justified by import-order safety.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
Resolve runtime streaming and lockfile conflicts while preserving TLS child environment wiring. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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>
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>
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>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 1 | 1 | Add the static canonical-owner fence and an env-policy parity guard. |
| CLI Logging Expert | 0 | 0 | 1 | Parent logging is sound; remove redundant child DEBUG prefixes. |
| DevX UX Expert | 0 | 0 | 0 | Happy path is invisible and failures are actionable. |
| Supply Chain Security Expert | 0 | 0 | 1 | Trust flow is safe; make offline llm probes use the same child env hygiene. |
| OSS Growth Hacker | 0 | 0 | 1 | Lead the changelog with the corporate-proxy benefit. |
| Auth Expert | 0 | 0 | 0 | Token and credential behavior is unchanged. |
| Doc Writer | 0 | 0 | 1 | Add the missing changelog section separator. |
| Test Coverage Expert | 0 | 0 | 0 | Critical promises have regression traps at their required tiers. |
| Performance Expert | 0 | 0 | 0 | Schema placeholder; no finding admitted. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Python Architect] Add an architecture lint that rejects direct
truststore.inject_into_ssl()calls outsidecore/tls_trust.pyandcore/_child_tls/_apm_tls_bootstrap.py-- this completes the repository's dual-guardrail contract. - [Python Architect] Add a static parity test for parent and child TLS environment-variable policy -- the child is intentionally standalone, so literals must not drift silently.
- [Supply Chain Security Expert] Pass
build_child_tls_env()to the two offlinellm --versionprobes -- this keeps subprocess env handling uniform if those probes evolve. - [CLI Logging Expert] Remove redundant
[i]prefixes from child-bootstrap DEBUG messages -- the log level already carries severity. - [OSS Growth Hacker] Lead the changelog entry with the user benefit, then retain the mechanism and scope detail -- this makes the upgrade value scannable.
Architecture
classDiagram
class tls_trust {
+configure_process_tls_trust() bool
+configure_tls_trust(env) bool
+build_child_tls_env(env) dict
+ensure_child_tls_bootstrap(venv) bool
}
class RuntimeManager
class LLMRuntime
class ChildBootstrap
RuntimeManager ..> tls_trust : deliver bootstrap
LLMRuntime ..> tls_trust : scrub child env
tls_trust ..> ChildBootstrap : install into venv
flowchart TD
A["CLI entry"] --> B["Configure OS trust once"]
B --> C["Replay trust source after logging"]
D["Runtime setup"] --> E["Install child bootstrap"]
E --> F["Child interpreter loads OS trust"]
Recommendation
Fold the five bounded items, re-run the lint and CI evidence, then perform the terminal advisory pass. No design discussion is needed.
Full per-persona findings
Python Architect
- [recommended]
scripts/lint-architecture-boundaries.shdoes not prevent directtruststore.inject_into_ssl()calls outside the canonical parent and standalone-child owners. - [nit] Parent and child TLS environment-variable policy needs a static parity assertion because the standalone child cannot import parent constants.
CLI Logging Expert
- [nit]
_apm_tls_bootstrap.pystill prefixes DEBUG messages with[i], unlike the parent logger.
DevX UX Expert
No findings.
Supply Chain Security Expert
- [nit] Two
llm --versionprobes omitbuild_child_tls_env()even though network-bearing sibling subprocesses use it. The probes are offline today.
OSS Growth Hacker
- [nit] The changelog mechanism detail precedes the user benefit, reducing scanability.
Auth Expert
No findings.
Doc Writer
- [nit] The PR feat(tls): verify against the OS trust store by default (closes #2004) #2005 changelog entry needs a blank line before the next heading.
Test Coverage Expert
No findings.
Performance Expert
Schema failure after two correction attempts; no finding admitted.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
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>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 1 | Dual guardrails are complete; remove one redundant Codex env argument. |
| CLI Logging Expert | 0 | 0 | 0 | TLS logging and verbose replay are clean. |
| DevX UX Expert | 0 | 0 | 0 | Happy path is silent and recovery is actionable. |
| Supply Chain Security Expert | 0 | 0 | 0 | Schema placeholder after two malformed returns. |
| OSS Growth Hacker | 0 | 0 | 0 | Benefit-first changelog copy is effective. |
| Auth Expert | 0 | 0 | 0 | Schema placeholder after two malformed returns. |
| Doc Writer | 0 | 3 | 0 | Correct verification guidance, remove an ungrounded caveat, and mark planned scope. |
| Test Coverage Expert | 0 | 0 | 0 | Critical TLS surfaces have regression traps at required tiers. |
| Performance Expert | 0 | 0 | 0 | No meaningful hot-path overhead. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Doc Writer] Replace the bare
python -c "import requests"verification step with an actual APM debug/install path -- the bare process never runs APM's trust injection and can report a false negative. - [Supply Chain Security Expert] Make bundled-certifi detection exact before stripping
SSL_CERT_FILEfrom child env -- a genuine user path ending/certifi/cacert.pemmust survive. - [Supply Chain Security Expert] Reject a child
site-packagespath that resolves outside its managed venv before writing the bootstrap -- local symlink redirection should not escape the venv. - [Doc Writer] Remove the ungrounded Windows Schannel caveat -- it names no supported symptom or recovery.
- [Doc Writer] Present future Node/Rust coverage (Add additive corporate-CA support: APM_EXTRA_CA_BUNDLE (npm NODE_EXTRA_CA_CERTS parity) #2034) in the documented Planned callout -- shipped scope and roadmap should be visually distinct.
Architecture
flowchart TD
A["APM Python process"] --> B["Canonical TLS owner"]
B --> C["OS trust or certifi fallback"]
D["llm runtime setup"] --> E["Contained venv bootstrap"]
E --> F["Standalone child TLS owner"]
Recommendation
Fold the five items plus the small Codex call-site cleanup, re-run full lint and CI, then perform the final advisory pass.
Full per-persona findings
Python Architect
- [nit]
codex_runtime.pypasses the same child env that_stream_subprocess_outputalready supplies by default.
CLI Logging Expert
No findings.
DevX UX Expert
No findings.
Supply Chain Security Expert
Schema failure after two correction attempts. Original review context surfaced the contained-venv and exact-certifi-path edge cases listed above.
OSS Growth Hacker
No findings.
Auth Expert
Schema failure after two correction attempts. Original review context overlapped the exact-certifi-path edge case listed above.
Doc Writer
- [recommended] Bare Requests verification bypasses APM's injection and can false-negative.
- [recommended] The Windows Schannel caveat is ungrounded and non-actionable.
- [recommended] Node/Rust future scope should use the Planned callout convention.
Test Coverage Expert
No findings.
Performance Expert
No findings.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
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>
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>
Keep the PR microsoft#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>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 0 | Canonical ownership and dual guardrails are complete. |
| CLI Logging Expert | 0 | 0 | 0 | Quiet and verbose TLS diagnostics are consistent and actionable. |
| DevX UX Expert | 0 | 0 | 0 | Happy path is zero-config; recovery paths are explicit. |
| Supply Chain Security Expert | 0 | 0 | 0 | Dependency, trust, path-containment, and fallback boundaries are sound. |
| OSS Growth Hacker | 0 | 0 | 0 | Benefit-first changelog and enterprise docs are complete. |
| Auth Expert | 0 | 0 | 0 | AuthResolver and token handling are untouched. |
| Doc Writer | 0 | 0 | 0 | Verification, planned scope, security guidance, and links match code. |
| Test Coverage Expert | 0 | 0 | 0 | Critical TLS promises have regression traps at required tiers. |
| Performance Expert | 0 | 0 | 0 | No meaningful hot-path or algorithmic regression. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Recommendation
Merge as-is after required human review. No current panel follow-up remains.
Folded in this run
- (panel) Replay the early trust-source choice after verbose logging is configured -- resolved in
83eb7e568. - (panel) Prove
RuntimeManager.setup_runtime("llm")delivers the child bootstrap -- resolved in8b9d27ecf. - (panel) Differentiate Python-version and proxy/PIP_CERT setup failures -- resolved in
8b9d27ecf. - (panel) Make bootstrap failure output actionable and unexpected errors debug-visible -- resolved in
8b9d27ecf. - (panel) Document the enterprise transport-trust boundary -- resolved in
f4d51b0ab. - (panel) Remove duplicated caveats and the unimplemented extra-bundle variable name -- resolved in
f4d51b0ab. - (panel) Add a static architecture fence for direct truststore injection -- resolved in
6483e2284. - (panel) Lock parent-child TLS environment policy parity -- resolved in
6483e2284. - (panel) Apply child TLS env hygiene to llm availability probes -- resolved in
6483e2284. - (panel) Normalize child DEBUG diagnostics and lead the changelog with user value -- resolved in
6483e2284. - (panel) Reject child site-packages symlink escapes -- resolved in
a1ab076f9. - (panel) Preserve user
SSL_CERT_FILEunless it exactly matches the recorded bundled cert -- resolved ina1ab076f9. - (panel) Verify trust through an actual APM path and mark planned Node/Rust scope -- resolved in
a1ab076f9. - (panel) Remove the ungrounded Schannel caveat and redundant Codex env argument -- resolved in
a1ab076f9. - (panel) Reconcile the changelog with the v0.25.0 release cut after merging main -- resolved in
e08aa2145. - (copilot) Correct stale SSL_CERT_FILE test documentation -- resolved in
1756d1a6a. - (copilot) Replace non-ASCII dashes in TLS docs and changelog -- resolved in
1756d1a6a. - (copilot) Reference PR feat(tls): verify against the OS trust store by default (closes #2004) #2005 as well as issue [FEATURE] Verify TLS using the system trust store by default (breaks in enterprise/proxy environments) #2004 in the changelog -- resolved in
1756d1a6a.
Copilot signals reviewed
tests/unit/core/test_tls_trust.py:8-- LEGIT: the module docstring contradicted the implemented SSL_CERT_FILE precedence (resolved in1756d1a6a).docs/src/content/docs/troubleshooting/ssl-issues.md:57-- LEGIT: a non-ASCII em dash violated repository encoding conventions (resolved in1756d1a6a).docs/src/content/docs/troubleshooting/ssl-issues.md:61-- LEGIT: a second non-ASCII em dash violated repository encoding conventions (resolved in1756d1a6a).CHANGELOG.md:18-- LEGIT: the entry referenced only issue [FEATURE] Verify TLS using the system trust store by default (breaks in enterprise/proxy environments) #2004 instead of the PR that carries the change (resolved in1756d1a6a).CHANGELOG.md:14-- LEGIT: a non-ASCII em dash violated repository encoding conventions (resolved in1756d1a6a).
Deferred (out-of-scope follow-ups)
- (panel) Extend OS trust propagation to Node (Copilot) and Rust (Codex) child runtimes -- scope boundary: this PR explicitly covers Python paths; runtime-specific work is tracked in Add additive corporate-CA support: APM_EXTRA_CA_BUNDLE (npm NODE_EXTRA_CA_CERTS parity) #2034; suggested follow-up: Add additive corporate-CA support: APM_EXTRA_CA_BUNDLE (npm NODE_EXTRA_CA_CERTS parity) #2034.
- (panel) Add hash constraints for the entire child-runtime pip installation channel -- scope boundary: the existing unhashed
llminstallation is a broader child-runtime dependency policy, not the TLS transport-trust change. - (panel) Add a TLS posture section to
apm doctor-- scope boundary: this introduces a new diagnostic command surface beyond the trust behavior and troubleshooting documentation in this PR.
Regression-trap evidence (mutation-break gate)
test_cached_trust_source_can_be_replayed_after_logging_configuration-- deletedcached trust-source replay; test FAILED as expected; guard restored.test_runtime_manager_setup_llm_installs_tls_bootstrap-- deletedRuntimeManager bootstrap handoff; test FAILED as expected; guard restored.test_runtime_manager_bootstrap_warning_is_actionable-- deletedPIP_CERT/Python/docs recovery guidance; test FAILED as expected; guard restored.test_runtime_manager_bootstrap_exception_is_visible_in_debug_log-- deletedunexpected-exception DEBUG record; test FAILED as expected; guard restored.test_v4_setup_llm_truststore_is_best_effort_and_pinned-- deletedproxy recovery guidance in the setup script; test FAILED as expected; guard restored.test_ssl_docs_keep_planned_configuration_generic-- restoredAPM_EXTRA_CA_BUNDLEas an advertised name; test FAILED as expected; guard restored.test_enterprise_security_docs_transport_trust_model-- deletedHTTPS transport trust heading; test FAILED as expected; guard restored.test_tls_injection_has_one_canonical_authority-- deletedarchitecture injection boundary; test FAILED as expected; guard restored.test_child_bootstrap_tls_policy_matches_parent_constants-- changedREQUESTS_CA_BUNDLEin the child policy; test FAILED as expected; guard restored.test_child_bootstrap_debug_messages_do_not_embed_console_symbols-- restored an inline[i]prefix; test FAILED as expected; guard restored.test_is_available_uses_child_tls_env-- deletedllm availability env hygiene; test FAILED as expected; guard restored.test_ensure_child_tls_bootstrap_rejects_site_packages_symlink_escape-- deletedmanaged-venv containment; test FAILED as expected; guard restored.test_build_child_tls_env_preserves_user_certifi_component_path-- restored suffix-only bundled-cert detection; test FAILED as expected; guard restored.test_ssl_docs_verify_apm_path_and_mark_planned_scope-- removed thePlannedcallout marker; test FAILED as expected; guard restored.
Lint contract
Full current-main CI mirror exited 0: ruff lint, ruff format, YAML I/O, 2100-line, relative_to, pylint R0801, auth boundary, and architecture boundary checks all passed.
CI
All checks green on final head in CI run 29184285439, including both test shards, binary smoke, CodeQL, coverage, NOTICE, spec conformance, docs build, and merge gate (after 1 CI recovery iteration for a conflicting-main synchronize event).
Mergeability status
| PR | head SHA | CEO stance | iters | folds | defers | Copilot rounds | CI | mergeable | mergeStateStatus | notes |
|---|---|---|---|---|---|---|---|---|---|---|
| #2005 | e08aa21 |
ship_now | 4 | 18 | 3 | 2 | green | MERGEABLE | BLOCKED | awaiting required review |
Convergence
4 outer iterations; 2 Copilot rounds. Final panel stance: ship_now.
Ready for maintainer review.
Full per-persona findings
Python Architect
No findings.
CLI Logging Expert
No findings.
DevX UX Expert
No findings.
Supply Chain Security Expert
No findings.
OSS Growth Hacker
No findings.
Auth Expert
No findings.
Doc Writer
No findings.
Test Coverage Expert
No findings.
Performance Expert
No findings.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
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.