feat(tls): add additive corporate CA support - #2741
feat(tls): add additive corporate CA support#2741Josh Bazar (TameTheGame) wants to merge 7 commits into
Conversation
# Conflicts: # CHANGELOG.md
|
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
Pull request overview
Adds opt-in additive enterprise CA support via APM_EXTRA_CA_BUNDLE, ensuring APM can trust corporate/private roots in addition to its normal trust (OS truststore when available, otherwise certifi), and propagates stable, validated CA snapshots to child processes (Python Requests children and Node children) without TOCTOU on operator-controlled files.
Changes:
- Introduces
APM_EXTRA_CA_BUNDLEwith bounded, certificate-only PEM validation; transactional publication of OS-plus-extra TLS context and robust fallback behavior. - Implements per-process CA snapshotting under
~/.apm/tls/and derives child env mappings (REQUESTS_CA_BUNDLEmerged snapshot;NODE_EXTRA_CA_CERTSextra-only snapshot) with ownership markers for nested runs. - Updates CLI early-failure handling, runtime/script spawn seams, tests, and docs/changelog to reflect the new precedence and scope boundaries.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/apm_cli/core/tls_trust.py |
Core owner for additive CA validation, transactional truststore publication, stable snapshots, child env derivation, and managed-venv bootstrap refresh. |
src/apm_cli/core/_child_tls/_apm_tls_bootstrap.py |
Managed Python bootstrap updated to support OS-plus-extra injection silently and transactionally in child interpreters. |
src/apm_cli/cli.py |
Converts early TLS configuration errors into a single Click failure before command callbacks execute. |
src/apm_cli/core/script_runner.py |
Ensures apm run spawn paths apply build_child_tls_env for both shell and runtime-exec boundaries. |
src/apm_cli/core/script_executors.py |
Ensures hardened Sessions (trust_env=False) still honor explicit CA replacement variables via the canonical helper. |
src/apm_cli/runtime/llm_runtime.py |
Ensures managed llm runtime invocations use the canonical child TLS environment (and bootstrap refresh). |
src/apm_cli/install/validation.py |
Updates TLS failure guidance to recommend additive trust (APM_EXTRA_CA_BUNDLE) before replacement (REQUESTS_CA_BUNDLE). |
CHANGELOG.md |
Adds an Unreleased entry documenting the new additive enterprise CA behavior and Node non-overwrite semantics. |
docs/src/content/docs/troubleshooting/ssl-issues.md |
Documents precedence, runtime coverage, failure behavior, and configuration recipes for additive trust. |
docs/src/content/docs/reference/environment-variables.md |
Documents APM_EXTRA_CA_BUNDLE, NODE_EXTRA_CA_CERTS, and explicit resolution order/scope. |
docs/src/content/docs/enterprise/security.md |
Updates enterprise security model with additive trust mechanics, snapshots, and precedence boundaries. |
docs/src/content/docs/enterprise/registry-proxy.md |
Updates proxy troubleshooting to prefer additive trust while retaining public roots. |
docs/src/content/docs/troubleshooting/common-errors.md |
Aligns common TLS error recovery guidance with additive trust. |
docs/src/content/docs/troubleshooting/install-failures.md |
Aligns install TLS troubleshooting with additive trust and precedence guidance. |
packages/apm-guide/.apm/skills/apm-usage/troubleshooting.md |
Keeps packaged troubleshooting guidance in sync with additive trust behavior. |
tests/unit/core/test_tls_trust.py |
Adds unit coverage for additive validation, rollback, snapshotting, ownership markers, and managed bootstrap refresh. |
tests/unit/core/test_script_runner_execution.py |
Adds unit coverage to lock TLS child env application at both runtime and shell spawn seams. |
tests/unit/test_llm_runtime.py |
Updates expectations to ensure managed runtime spawns request the llm-scoped child TLS environment. |
tests/unit/test_lifecycle_executor_paths.py |
Adds tests proving hardened Sessions honor explicit CA bundle settings even with trust_env=False. |
tests/unit/test_tls_docs_scope.py |
Updates doc drift guards to enforce runtime scope wording and additive-variable documentation. |
tests/integration/test_tls_custom_ca.py |
Adds integration coverage for additive trust behavior (parent Requests, preloaded contexts, CLI fail-fast, apm run propagation). |
tests/integration/test_tls_child_runtime.py |
Adds integration coverage for managed/foreign Python bootstraps, descendants, Node child propagation, and source mutation stability. |
tests/integration/test_tls_frozen_hook.py |
Extends frozen-hook env coverage to include additive variables and derived ownership markers. |
tests/integration/test_tls_r2_verify.py |
Extends verification coverage to include additive variables and derived ownership markers. |
tests/integration/test_tls_r3_verify.py |
Updates docs-scope integration assertions to match the new runtime-coverage wording and additive guidance. |
tests/integration/test_wave6_validation_uninstall_coverage.py |
Updates validation guidance assertions to include additive trust recommendations. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Merged upstream The merge also exposed a new architecture check: two TLS integration tests selected the CLI directly. They now consume the canonical Validation on Windows:
These are local results; upstream workflow approval and CI results remain separate. Conflict resolution and the test adaptation were performed with OpenAI Codex assistance. |
|
Resolved the new conflicts after upstream advanced again: retained the corrected 0.29.1 release date and the new MCP staging-path troubleshooting entry alongside this PR's additive TLS guidance. Merged current upstream main; no changes to this PR's TLS implementation or tests were needed. Validation for this documentation conflict resolution: all 9 TLS documentation-scope tests passed; Ruff lint/format, duplication, architecture and auth boundaries, source guards, and diff whitespace checks passed. Documentation build: 124 pages, 1,006 relative links, no broken links. The earlier 223-test runtime result remains evidence for the previous head; that broader selection was not rerun for this documentation-only resolution. Performed with OpenAI Codex assistance. Upstream workflow approval and CI results remain separate from these local checks. |
Keep rollback at the parent and child bootstrap boundaries, consolidate the parent fallback, and remove the duplicate HTTPS test server. Link the TLS reference to the existing runtime coverage explanation. Exercise fallback after actual additive-context publication. Correct the Windows CA override expectation and run the Bash probe through stdin.
|
Pushed the simplification follow-up in ec035c10. The change removes redundant inner rollback while retaining recovery around the complete parent and child operations, consolidates the parent fallback, and deletes the duplicate HTTPS test server. It also replaces repeated documentation with a link, removes temporary-file handling from the Bash probe, and corrects the Windows CA-override test expectation. The additive trust and child-runtime scope remain intact. Net change: 93 lines removed across eight files. Validation for this commit on Windows/Python 3.12.13:
The fallback regression now forces failure after actual additive-context publication and verifies that real Requests HTTPS still succeeds. Other coverage includes private-CA HTTPS, independent existing-root retention, and Python/Node child propagation. The PR description has the full validation scope. Linux/macOS execution, a fresh packaged executable, a complete private-registry Review, changes, and validation were performed with OpenAI Codex assistance. |
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 1 | TLS trust is centralized in tls_trust with scoped child bootstrap and rollback; no blocking architecture issues found. |
| CLI Logging Expert | 0 | 1 | 1 | Two UX gaps remain: managed llm bootstrap refresh can fail silently, and the startup TLS hard-fail lacks a recovery hint. |
| DevX UX Expert | 0 | 0 | 0 | No DevX concerns: additive CA, precedence, child coverage, shell semantics, and unset behavior are documented and exercised. |
| Supply Chain Security | 0 | 0 | 0 | No supply-chain security regressions found in the additive CA implementation. |
| OSS Growth Hacker | 0 | 0 | 0 | Enterprise onboarding story is clear and scoped: one additive CA knob, runnable docs, and honest runtime boundaries. |
| Auth Expert | 1 | 0 | 0 | One shell-spawn path lets inline TLS opt-outs inherit APM-derived CA variables; token/auth policy remains isolated. |
| Doc Writer | 0 | 1 | 0 | Runtime and fallback disclosures match the amended issue; extend the inline-shell caveat to opt-out and replacement controls. |
| Test Coverage | 0 | 1 | 0 | TLS run/child/default-root tests pass; full private-CA apm install remains unguarded. |
| Performance Expert | 0 | 0 | 0 | No material performance defect found. Opt-in additive TLS adds bounded local I/O only; warm build_child_tls_env averaged 6.1 ms in a narrow local repro, with no new network RTTs or algorithmic growth. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 3 follow-ups
- [Test Coverage] Add the full private-CA apm install plus default-root regression fixture. -- Issue Add additive corporate-CA support: APM_EXTRA_CA_BUNDLE (npm NODE_EXTRA_CA_CERTS parity) #2034 acceptance explicitly names apm install as well as apm run; current targeted tests pass, but the install path is still missing automated proof that a private-CA package source works without replacing normal roots.
- [Doc Writer + Auth Expert] Broaden the inline-shell TLS boundary documentation and add a focused nested-env regression trap. -- APM resolves child TLS env before shell execution, so inline opt-out or curl replacement assignments cannot remove already-derived Requests/Node settings. Users should be told to set those controls in the environment launching APM rather than inside an apm.yml shell command; do not add fragile cross-platform shell parsing.
- [CLI Logging Expert] Show a default-level notice when managed llm TLS bootstrap refresh fails at child launch. -- The fallback still carries certifi plus the extra CA and does not disable verification, but a failed managed bootstrap refresh is security-relevant degradation that should not be debug-only.
Architecture
classDiagram
direction LR
class CLI {
<<Entrypoint>>
+configure_process_tls_trust()
+cli(ctx, verbose)
}
class TLS_TRUST {
<<Facade>>
+configure_tls_trust(env) bool
+build_child_tls_env(base_env, runtime_name) dict
+explicit_ca_bundle_path(env) str
+ensure_child_tls_bootstrap(venv_path) bool
}
class TLSConfigurationError {
<<Exception>>
}
class ChildCASnapshotStore {
<<Factory>>
+_ensure_child_ca_snapshots(bundle_pem) tuple
}
class TLSPublicationState {
<<Memento>>
+_capture_tls_publication_state()
+_restore_tls_publication_state(state)
}
class ChildBootstrap {
<<BootstrapAdapter>>
+_bootstrap()
}
class ScriptRunner {
<<Spawner>>
+_execute_script_command(command, params) bool
+_execute_runtime_command(command, content, env) CompletedProcess
}
class RuntimeBase {
<<Streamer>>
+_stream_subprocess_output(cmd, timeout, env) tuple
}
class LLMRuntime {
<<RuntimeAdapter>>
+execute_prompt(prompt_content) str
+is_available() bool
}
class ScriptExecutors {
<<HardenedSessionFactory>>
+_build_guarded_session()
+_build_capturing_session()
}
class RequestsSession {
<<ExternalAdapter>>
}
class NodeRuntime {
<<ExternalRuntime>>
}
class PythonRequestsChild {
<<ExternalRuntime>>
}
CLI ..> TLS_TRUST : imports before command modules
TLS_TRUST ..> TLSConfigurationError : raises invalid additive config
TLS_TRUST *-- ChildCASnapshotStore : freezes extra and certifi-plus-extra
TLS_TRUST *-- TLSPublicationState : transactional publish/rollback
TLS_TRUST ..> ChildBootstrap : ships and refreshes
ScriptRunner ..> TLS_TRUST : build_child_tls_env()
RuntimeBase ..> TLS_TRUST : default child env
LLMRuntime ..> TLS_TRUST : runtime_name="llm"
ScriptExecutors ..> TLS_TRUST : explicit_ca_bundle_path()
ChildBootstrap ..> TLSPublicationState : local rollback copy
ChildBootstrap ..> PythonRequestsChild : preserves derived REQUESTS_CA_BUNDLE
TLS_TRUST ..> NodeRuntime : NODE_EXTRA_CA_CERTS snapshot
ScriptExecutors ..> RequestsSession : trust_env=False plus explicit verify
note for TLS_TRUST "Single authority:\nprecedence, validation,\nsnapshots, parent fallback,\nchild env mapping"
note for TLSPublicationState "Memento-style rollback:\nssl, urllib3, Requests preloaded context"
class CLI:::touched
class TLS_TRUST:::touched
class TLSConfigurationError:::touched
class ChildCASnapshotStore:::touched
class TLSPublicationState:::touched
class ChildBootstrap:::touched
class ScriptRunner:::touched
class LLMRuntime:::touched
class ScriptExecutors:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A["[I/O] src/apm_cli/cli.py import calls configure_process_tls_trust()"] --> B["src/apm_cli/core/tls_trust.py::configure_tls_trust(env)"]
B --> C{"has_explicit_ca_override(env)?"}
C -->|yes| D["Return False; REQUESTS_CA_BUNDLE or CURL_CA_BUNDLE remains replacement authority"]
C -->|no| E{"APM_DISABLE_TRUSTSTORE truthy?"}
E -->|yes| F["Return False; OS/additive propagation suppressed"]
E -->|no| G{"APM_EXTRA_CA_BUNDLE set?"}
G -->|yes| H["[I/O] _read_extra_ca_bundle(): resolve, fstat, size/ascii/private-key/parser validation"]
G -->|no| I["[I/O] import truststore; truststore.inject_into_ssl()"]
H --> I
I --> J{"truststore injection and _install_additive_ca_context() succeed?"}
J -->|yes| K["[I/O] publish ssl.SSLContext, urllib3.util.ssl_.SSLContext, requests.adapters._preloaded_ssl_context"]
J -->|no| L["_restore_tls_publication_state(): rollback loaded ssl/urllib3/Requests globals"]
L --> M{"extra CA was selected?"}
M -->|yes| N["[FS] _ensure_child_ca_snapshots(): write certifi-plus-extra fallback under ~/.apm/tls/apm_tls_*/"]
N --> O["[I/O] set REQUESTS_CA_BUNDLE and APM_REQUESTS_CA_BUNDLE_IS_DERIVED_ADDITIVE in os.environ"]
M -->|no| P["Return False; bundled certifi fallback only"]
K --> Q["apm command callbacks can perform HTTPS with OS-plus-extra or OS trust"]
O --> Q
Q --> R["src/apm_cli/core/script_runner.py::_execute_script_command() or _execute_runtime_command()"]
R --> S["src/apm_cli/core/tls_trust.py::build_child_tls_env(env, runtime_name)"]
S --> T["Clear APM-derived REQUESTS_CA_BUNDLE and NODE_EXTRA_CA_CERTS markers before recomputing"]
T --> U{"disable or genuine Requests/curl replacement present?"}
U -->|yes| V["[EXEC] subprocess.run(..., env=child) preserves operator-owned replacement and suppresses derived Node mapping"]
U -->|no| W["[FS] validate APM_EXTRA_CA_BUNDLE again; write/reuse extra-only and certifi-plus-extra snapshots"]
W --> X["[EXEC] Python child receives REQUESTS_CA_BUNDLE=certifi-plus-extra snapshot"]
W --> Y["[EXEC] Node child receives NODE_EXTRA_CA_CERTS=extra-only snapshot unless native value is non-empty"]
W --> Z{"runtime_name == 'llm'?"}
Z -->|yes| AA["[FS] _refresh_managed_llm_tls_bootstrap(): ensure_child_tls_bootstrap(~/.apm/runtimes/llm-venv)"]
Z -->|no| AB["No managed bootstrap refresh"]
sequenceDiagram
participant User
participant CLI as src/apm_cli/cli.py
participant TLS as src/apm_cli/core/tls_trust.py
participant Runner as src/apm_cli/core/script_runner.py
participant Child as Python/Node child process
User->>CLI: APM_EXTRA_CA_BUNDLE=/corp.pem apm run tls-probe
CLI->>TLS: configure_process_tls_trust()
TLS->>TLS: _read_extra_ca_bundle() and configure_tls_trust()
alt truststore publication succeeds
TLS-->>CLI: OS trust plus additive CA published
else publication fails after validation
TLS->>TLS: _restore_tls_publication_state()
TLS->>TLS: _ensure_child_ca_snapshots()
TLS-->>CLI: Requests fallback env uses certifi-plus-extra snapshot
end
CLI->>Runner: run command callback
Runner->>TLS: build_child_tls_env(env, runtime_name)
TLS->>TLS: clear derived markers, revalidate source, create stable snapshots
TLS-->>Runner: child env with REQUESTS_CA_BUNDLE and/or NODE_EXTRA_CA_CERTS
Runner->>Child: subprocess.run(..., env=child_env)
Child-->>Runner: HTTPS uses frozen additive trust bytes
Recommendation
Ship the additive CA feature with the three follow-ups above tracked. Treat the code and docs as aligned with the amended issue plan, but do not close the loop on #2034 as fully demonstrated until the private-CA apm install fixture is added or explicitly deferred by the maintainer; describe current workflow status as action_required, not green.
Full per-persona findings
Python Architect
- [nit] Architecture pattern note: current TLS owner shape is sufficient. at
src/apm_cli/core/tls_trust.py:452
Design patterns; Used in this PR: Facade / single-authority module -- src/apm_cli/core/tls_trust.py owns trust precedence, additive validation, child snapshots, parent fallback, and child env mapping through configure_tls_trust() and build_child_tls_env().; Used in this PR: Memento-style transactional rollback -- _capture_tls_publication_state() and _restore_tls_publication_state() keep process-wide ssl, urllib3, and Requests publication reversible when additive context installation fails.; Used in this PR: Factory -- _ensure_child_ca_snapshots() creates content-addressed extra-only and certifi-plus-extra artifacts for children rather than letting each spawn path write its own TLS files.; Pragmatic suggestion: none -- splitting this into a registry or strategy hierarchy would add indirection without a third independent TLS policy consumer; keep extending tls_trust as the canonical owner.
Suggested: Keep future TLS precedence, snapshot, and rollback changes routed through tls_trust; the existing architecture boundary guard already confines truststore.inject_into_ssl() to tls_trust and the child bootstrap.
CLI Logging Expert
- [recommended] Warn when a managed llm launch cannot refresh its TLS bootstrap. at
src/apm_cli/core/tls_trust.py:679
src/apm_cli/core/tls_trust.py degrades launch-time refresh failure to a debug-only line even though the PR and docs promise that APM refreshes the managed llm bootstrap before managed launches. In a reproduced failure where ensure_child_tls_bootstrap returned False, build_child_tls_env({}, runtime_name='llm') emitted no default-level warning and proceeded. That turns a trust-store regression into a later child TLS failure with no immediate guidance, which is the wrong default for a security-relevant runtime degradation.
Suggested: Mirror runtime_manager._install_llm_tls_bootstrap's yellow warning here, or route both paths through one shared formatter, so a failed launch-time refresh tells the user to re-runapm runtime setup llm, use Python 3.10+, or setPIP_CERTbefore the child command continues.
Proof (manual only):(no test ref)-- proves: A managed llm launch can lose the advertised bootstrap refresh with no default-level operator guidance. [secure-by-default,devx] - [nit] Add a one-line recovery hint to the early Click TLS configuration error. at
src/apm_cli/cli.py:163
src/apm_cli/cli.py raises ClickException(str(_TLS_BOOTSTRAP_ERROR)) before any command callback runs. For a mis-set APM_EXTRA_CA_BUNDLE, the surfaced text is only the raw diagnosis, for example 'APM_EXTRA_CA_BUNDLE path does not exist: ...'. That explains what is wrong but not what to do next, which is a poor first-run experience for a startup-blocking error.
Suggested: Append one short fix hint such as 'Unset APM_EXTRA_CA_BUNDLE or point it at a readable certificate-only PEM; see SSL / TLS issues.'
DevX UX Expert
No findings.
Supply Chain Security
No findings.
OSS Growth Hacker
No findings.
Auth Expert
- [blocking] Inline shell TLS opt-outs do not clear APM-derived child trust variables. at
src/apm_cli/core/script_runner.py:196
When APM_EXTRA_CA_BUNDLE is set in the environment that launches APM, the shell=True apm run path builds REQUESTS_CA_BUNDLE and NODE_EXTRA_CA_CERTS before the shell applies inline assignments such as APM_DISABLE_TRUSTSTORE=1 or REQUESTS_CA_BUNDLE=/replacement.pem. The direct child can therefore still inherit APM-derived CA trust even though the command explicitly opted out or selected replacement trust, violating the documented replacement/disable precedence and the exact-path ownership invariant for derived values.
Suggested: Add a regression test with parent APM_EXTRA_CA_BUNDLE plus an inline shell APM_DISABLE_TRUSTSTORE/REQUESTS_CA_BUNDLE assignment, then either clear derived CA variables for that shell command before exec or document and enforce that shell-inline trust policy is unsupported by failing closed instead of silently keeping the derived bundle.
Proof (manual only):(no test ref)-- proves: A direct apm run shell child can receive additive CA trust after the command-level disable is applied. [secure-by-default,governed-by-policy,multi-harness-support]
Doc Writer
- [recommended] Explain that inline opt-out and curl replacement cannot undo precomputed child trust at
docs/src/content/docs/troubleshooting/ssl-issues.md:106
The caveat explains only that an inline additive assignment is not translated for Node. With APM_EXTRA_CA_BUNDLE exported before apm run, script_runner.py:192-196 derives the environment before the shell evaluates assignments; tls_trust.py:729-739 has already populated REQUESTS_CA_BUNDLE and NODE_EXTRA_CA_CERTS. Consequently, an apm.yml shell command such as 'APM_DISABLE_TRUSTSTORE=1 node probe.js' still inherits the derived Node CA, and 'CURL_CA_BUNDLE=/replacement.pem python probe.py' leaves an ordinary Requests child using the higher-priority derived REQUESTS_CA_BUNDLE. The documented precedence can therefore mislead users attempting per-script opt-out or replacement. A search of the docs and packaged guidance found no explanation of these cases. This is missing operational guidance for the deliberately retained shell boundary, not a request to parse shell commands or a demonstrated verification bypass.
Suggested: Replace the existing inline-assignment sentence with a concise explanation that APM resolves precedence before shell execution. Tell users to set opt-out/replacement controls in the environment launching APM, for example 'APM_DISABLE_TRUSTSTORE=1 apm run probe' on POSIX, rather than inside the apm.yml shell command. Explicitly note that inline assignments do not remove already-derived Requests/Node settings.
Test Coverage
- [recommended] Add a full-install private-CA regression trap. at
src/apm_cli/cli.py:24
Issue Add additive corporate-CA support: APM_EXTRA_CA_BUNDLE (npm NODE_EXTRA_CA_CERTS parity) #2034 acceptance names both apm install and apm run, but the Scenario Evidence table maps only parent Requests plus apm run/child-runtime probes. I searched tests/integration and tests for install plus APM_EXTRA_CA_BUNDLE/private_ca_https_server/private CA overlap; the only install hits are docs/guidance or unrelated local install scenarios, and there is no apm_binary_path/CliRunner install test that drives APM_EXTRA_CA_BUNDLE through a private-CA HTTPS install fixture while also proving the default roots remain usable. The affected suite I ran passed once APM_BINARY_PATH was pinned to the snapshot-importing venv script, so this is a missing evidence gap, not a demonstrated branch regression.
Suggested: Add tests/integration/test_tls_install_custom_ca.py::test_apm_install_trusts_private_ca_and_retains_default_root using synthetic loopback roots, APM_BINARY_PATH=/.venv/bin/apm, PYTHONPATH=src, and no live Internet.
Proof (test MISSING at):tests/integration/test_tls_install_custom_ca.py::test_apm_install_trusts_private_ca_and_retains_default_root-- proves: apm install can consume a private-CA HTTPS package source via APM_EXTRA_CA_BUNDLE without replacing the normal/default trust roots. [secure-by-default,devx]
assert install.returncode == 0 and private_package_installed and default_root_probe.returncode == 0
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.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Daniel Meppiel (danielmeppiel)
left a comment
Daniel Meppiel (danielmeppiel)
left a comment
There was a problem hiding this comment.
Tahnk you for this! Please fix CodeQL vulnerabilities and the panel identified blockers + recommendations/followups
…bundle # Conflicts: # CHANGELOG.md
|
Thanks, Daniel Meppiel (@danielmeppiel). Pushed e9e0301a to address the CodeQL findings and the panel's follow-ups:
Validation on Windows/Python 3.12.13: 373 passed, 2 warnings in 83.30 seconds, with no skips or deselections. All three symlink tests passed with elevation; the two warnings are from the unchanged lifecycle timeout test's reader threads. Required Ruff/format, duplication, architecture/auth, CI source guards, and whitespace checks passed. Documentation built 124 pages and checked 1,031 relative links without errors. The branch includes upstream Would you mind approving the new workflows, reapply |
Daniel Meppiel (danielmeppiel)
added
the
panel-review
Daniel Meppiel (danielmeppiel)
added
panel-review
There was a problem hiding this comment.
🟡 Changes recommended
The new CHANGELOG entry does not end with a required PR reference (#PR_NUMBER) per repo changelog rules.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 29/29 changed files
- Comments generated: 1
- Review effort level: Lite
add(tls): support additive enterprise CA bundles
TL;DR
This adds
APM_EXTRA_CA_BUNDLE, an opt-in certificate-only PEM bundle that augments APM's active trust roots instead of replacing them. It covers parent Requests traffic, generic and managed Python children, and Node children launched byapm run, while preserving explicit replacement and runtime-native settings. Invalid bundles fail before command execution, process-wide TLS publication is transactional, and child processes receive immutable per-process snapshots rather than reopening operator-controlled files.Note
No behavior changes when
APM_EXTRA_CA_BUNDLEis unset. Closes #2034.Problem (WHY)
SSLContext; triage recommends "writing a merged PEM bundle to a temp file and setting REQUESTS_CA_BUNDLE to it".truststorepublication can otherwise leavessl, urllib3, and Requests in different trust modes.Approach (WHAT)
certifi-plus-extra snapshots beneath~/.apm/tls/for child processes.Implementation (HOW)
src/apm_cli/core/tls_trust.pysrc/apm_cli/core/_child_tls/_apm_tls_bootstrap.pysrc/apm_cli/cli.pysrc/apm_cli/core/script_runner.pysrc/apm_cli/core/script_executors.pysrc/apm_cli/runtime/llm_runtime.pysrc/apm_cli/install/validation.pyCHANGELOG.mddocs/src/content/docs/enterprise/security.mddocs/src/content/docs/reference/environment-variables.mddocs/src/content/docs/troubleshooting/ssl-issues.mddocs/src/content/docs/enterprise/registry-proxy.mddocs/src/content/docs/troubleshooting/common-errors.mddocs/src/content/docs/troubleshooting/install-failures.mdpackages/apm-guide/.apm/skills/apm-usage/troubleshooting.mdtests/unit/core/test_tls_trust.pytests/unit/core/test_script_runner_execution.pytests/unit/test_lifecycle_executor_paths.pytests/unit/test_llm_runtime.pytests/unit/test_tls_docs_scope.pytests/integration/test_tls_custom_ca.pyapm run.tests/integration/test_tls_install_custom_ca.pytests/integration/test_tls_shell_boundary.pytests/integration/test_tls_child_runtime.pytests/integration/test_tls_frozen_hook.pytests/integration/test_tls_r2_verify.pyPYTHONPATHchild boundary.tests/integration/test_tls_r3_verify.pytests/integration/test_wave6_validation_uninstall_coverage.pyDiagrams
Legend: dashed nodes are the new trust stages; the two child outputs intentionally carry different PEM compositions.
flowchart LR subgraph Parent[APM parent] E[APM_EXTRA_CA_BUNDLE] V[Validate certificate-only PEM] P{Precedence permits additive trust} I[Inject OS trust] A[Publish additive SSLContext] R[Requests and urllib3] end subgraph Snapshot[Per-process snapshot directory] S[Freeze validated bytes] M[certifi plus extra] X[extra only] end subgraph Children[apm run children] PY[Python Requests] LL[Managed Python bootstrap] N[Node runtime] end E --> V --> P P --> I --> A --> R P --> S S --> M --> PY M --> LL S --> X --> N classDef new stroke-dasharray: 5 5; class V,A,S,M,X new;Trade-offs
truststoreinjection fails, Requests keepscertifiplus the extra CA; direct parent urllib callers retain their own default rather than risking recursiveSSLContextreplacement.Benefits
Validation
The maintainer revision at e9e0301a includes upstream
mainat1cab81dc. It sets an explicit TLS 1.2 minimum in the two flagged test contexts, adds the complete private-CA install fixture and shell-boundary regressions, makes managed-bootstrap refresh failure visible, and adds startup recovery guidance.Affected TLS, script-runner, lifecycle, managed-runtime, validation, documentation-scope, and child-runtime regression suite on Windows/Python 3.12.13:
All three symlink tests ran successfully with Windows elevation; there were no skips or deselections. The two warnings are subprocess-reader thread exceptions from the unchanged lifecycle timeout test.
The new install fixture executes the real installed Python CLI using the repository's
apm_engine_commandfixture, which retains startup instrumentation even when CI also supplies a frozen binary. It tests five fresh projects: default-root success, private-root rejection without the extra CA, private-root success with it, default-root retention with it, and rejection when replacement-only trust excludes the default root. Successful installs verify metadata and archive requests, installed package bytes, the lockfile, and deployed instructions. The default root is synthetic and seeded into test-process certifi; no machine trust store is modified and no resolver or HTTP success is mocked.Repository-required lint scope includes
src/,tests/,scripts/lint_architecture_boundaries.py, andscripts/architecture_linter/:Documentation build and link validation:
The local results above are for
e9e0301a. All six upstream workflows on that implementation commit passed, including CI and the separate CodeQL findings check, which reported no new alerts and zero annotations. The install acceptance fixture exercises the source CLI; the separate upstream binary smoke job also passed.The latest commit, 4b923664, only formats the changelog entry and adds the required
(#2741)reference. Its diff contains no source or test changes, andgit diff --checkpassed. All six newly triggered workflows currently require maintainer authorization; results for the implementation commit are not presented as results for this new head. Human review remains pending.Scenario Evidence
tests/integration/test_tls_custom_ca.py::test_additive_context_retains_independent_existing_rootapm rungives an ordinary Python child the selected extra CA.tests/integration/test_tls_custom_ca.py::test_apm_run_propagates_extra_ca_to_real_childtests/integration/test_tls_child_runtime.py::test_node_child_consumes_derived_extra_ca_on_real_loopbacktests/integration/test_tls_child_runtime.py::test_bootstrap_preserves_derived_marker_for_python_descendantstests/integration/test_tls_child_runtime.py::test_generic_python_child_uses_stable_snapshot_after_source_mutationtests/integration/test_tls_custom_ca.py::test_invalid_extra_ca_fails_before_real_cli_commandtests/unit/core/test_tls_trust.py::test_private_key_bundle_is_rejected_before_snapshot_creationtests/unit/core/test_tls_trust.py::test_build_child_tls_env_preserves_node_value_replacing_derived_valuetests/unit/core/test_script_runner_execution.py::TestExecuteScriptCommand::test_shell_assignments_with_shell_semantics_remain_untouchedtests/integration/test_tls_install_custom_ca.py::test_apm_install_trusts_private_ca_and_retains_default_roottests/integration/test_tls_shell_boundary.py::test_apm_run_shell_controls_apply_at_apm_boundarytests/unit/core/test_tls_trust.py::test_managed_bootstrap_refresh_notice_preserves_additive_fallbackHow to test
APM_EXTRA_CA_BUNDLEto a certificate-only corporate PEM and run an APM command through the private endpoint; it should verify normally.apm runwith a Python or Node HTTPS probe; the private endpoint should succeed without replacing public trust.REQUESTS_CA_BUNDLEorCURL_CA_BUNDLE; confirm the explicit replacement remains authoritative.APM_DISABLE_TRUSTSTORE=1; confirm APM does not derive Python or Node additive settings.APM_EXTRA_CA_BUNDLEat a missing, malformed, oversized, or private-key-bearing file; the CLI should fail before the command starts.Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com