Repository navigation
Support per-server custom CA trust for HTTP MCP servers - #71229
Conversation
Expose ca-cert-file in custom HTTPS MCP configuration and securely resolve and mount each PEM read-only for the gateway. Preserve system trust and reject unsupported configurations without global TLS changes. Fixes #68033 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/review |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
/matt |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
![github-actions[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: Comment
No actionable blocking issues found in the changed lines. The review was static; repository tests were not executed. The advisory first-pass reviewer was unavailable, so this assessment is based on the changed-line analysis.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · ~2 AIC · ⌖ ~1 AIC · ⊞ 1.6K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design to this per-server CA trust feature (fixes #68033). The implementation is careful and well-layered: Go-side path validation (no traversal, no shell metacharacters, no expressions beyond the two literal prefixes) happens at compile time; the JS launcher independently re-validates (realpath, regular-file check, readability, comma/newline rejection) before mounting, and the mount target is a deterministic per-server hash path so servers cannot see each other's CA. Good defense in depth across three layers (schema → Go parser → JS launcher).
📋 Key Themes & Highlights
Key Themes
- One actionable gap: args.splice(1, 0, ...caMountArgs) in start_mcp_gateway.cjs relies on an un-tested assumption about insertion order relative to injectCustomGatewayEnvArgs/injectProxyEnvArgs and the rest of the parsed dockerCommand. See inline comment.
Positive Highlights
- ✅ ValidateMCPCACertFile rejects .., shell substitutions, and non-literal expressions with a precise regex + traversal check — exactly the right place to stop injection before it reaches a heredoc.
- ✅ prepareMCPServerCAs resolves symlinks via realpathSync, requires a regular readable file, and never leaks filesystem errors or certificate contents into thrown messages — matches the PR's stated non-goal of not disclosing file contents, and is backed by a real integration test against a native MCPG binary (mcp_server_ca.integration.test.cjs).
- ✅ Version gating (versionAtLeast(..., "v0.4.35")) prevents silently-ignored config on older gateways, with a dedicated regression test (TestMCPCACertFileOldGateway).
- ✅ gh aw mcp inspect explicitly rejects ca-cert-file instead of silently connecting without CA trust, avoiding a confusing local/CI behavior mismatch.
No blocking issues found; approving.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · ~157 AIC · ⌖ ~16 AIC · ⊞ 10.2K
Comment /matt to run again
There was a problem hiding this comment.
🔵 Needs a closer look
The security-sensitive Docker mount boundary lacks end-to-end validation and needs final human review.
2 open issues
What changed in this PR
Adds per-server HTTPS CA configuration to gh-aw’s MCP pipeline, addressing #68033 without changing the gateway version pin or global trust settings.
Changes:
- Adds CA configuration to schemas, shared types, parsing, validation, and gateway rendering.
- Resolves CA files at startup and creates server-specific, read-only Docker mounts.
- Documents provisioning, rejects unsupported local inspection, and adds compiler, launcher, and native TLS coverage.
| File | Description |
|---|---|
pkg/workflow/tools_types.go |
Serializes the typed CA field. |
pkg/workflow/schemas/mcp-gateway-config.schema.json |
Adds the gateway CA property. |
pkg/workflow/mcp_setup_gateway.go |
Enforces gateway version compatibility. |
pkg/workflow/mcp_property_validation.go |
Validates CA configuration requirements. |
pkg/workflow/mcp_config_validation.go |
Recognizes the author-facing CA key. |
pkg/workflow/mcp_config_custom.go |
Parses and renders CA paths. |
pkg/workflow/mcp_ca_test.go |
Covers compilation, validation, and serialization. |
pkg/types/README.md |
Documents the shared CA field. |
pkg/types/mcp.go |
Adds the shared typed CA field. |
pkg/parser/schemas/mcp_config_schema.json |
Adds CA transport constraints. |
pkg/parser/schemas/main_workflow_schema.json |
Exposes the workflow CA option. |
pkg/parser/mcp.go |
Integrates CA parsing. |
pkg/parser/mcp_ca.go |
Validates paths and HTTPS requirements. |
pkg/parser/mcp_ca_test.go |
Covers accepted and rejected configurations. |
pkg/cli/mcp_inspect_mcp.go |
Rejects gateway-only CA configuration locally. |
pkg/cli/mcp_inspect_ca_test.go |
Tests the inspection rejection. |
docs/src/content/docs/reference/tools.md |
Documents CA paths and provisioning. |
docs/src/content/docs/reference/mcp-gateway.md |
Describes the gateway CA field. |
docs/public/schemas/mcp-gateway-config.schema.json |
Publishes the gateway schema addition. |
actions/setup/js/start_mcp_gateway.cjs |
Integrates CA preparation and mount arguments. |
actions/setup/js/mcp_server_ca.test.cjs |
Tests path resolution and mount generation. |
actions/setup/js/mcp_server_ca.integration.test.cjs |
Adds native TLS isolation tests. |
actions/setup/js/mcp_server_ca.cjs |
Resolves files and prepares read-only mounts. |
Review effort: Max
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /grill-with-docs per pr-triage classification (new_feature). Approving — this is a thorough, security-conscious feature with one low-severity note left inline.
📋 Key Themes & Highlights
Key Themes
- Defense in depth is real, not decorative: author-facing regex/path validation (
pkg/parser/mcp_ca.go), JSON Schema conditionals, and runtime JS validation (mcp_server_ca.cjs) each independently reject unsafe input — no single layer is load-bearing. - Heredoc escaping verified correct:
renderMCPScalarProperty's\${GITHUB_WORKSPACE}/\${RUNNER_TEMP}substitution looked suspicious at first (a raw\$is not valid JSON), but I confirmed empirically that the unquoted bash heredoc inmcp_renderer.gostrips the leading backslash before the gateway'sJSON.parseruns, matching the existingReplaceTemplateExpressionsWithEnvVarspattern (secret_extraction.go) and theTestMCPCACertFileCompilationassertion. This is correct and consistent, not a bug. - Version gating is sound:
generateMCPGatewaySetupcorrectly fails fast with an actionable error whenca-cert-fileis set butsandbox.mcp.versionpredates v0.4.35. - One speculative-generality nit (left inline,
/codebase-design): the newca_cert_fileTOML property-order branch indetermineMCPPropertyOrderhas no current production caller usingFormat: "toml"for HTTP tools — worth a comment or a test confirming the intended future caller.
Positive Highlights
- ✅ Excellent test coverage across all three validation layers, including adversarial cases (
$(touch injected), path traversal, comma/newline injection, non-HTTPS URLs) — strong/tddevidence. - ✅ Docs (
tools.md) are clear, show both the direct-path and secret-provisioning patterns, and explicitly state what this feature does not do (no global trust store changes, no private keys) — strong/grill-with-docsevidence. - ✅ Docker
--mountargs are spliced into argv at a safe fixed index (args.splice(1, 0, ...)), never built via string concatenation — avoids shell-injection risk. - ✅ Honest PR description about the integration test's Docker-mount-enforcement limitation (environment constraint, already disclosed).
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · ~226 AIC · ⌖ ~16 AIC · ⊞ 10.2K
Comment /matt to run again
Retain exact-version and SHA-256 requirements while allowing hash-locked packages to list multiple wheel artifacts. Cover malformed and unhashed entries with regression cases. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Preserve typed CA paths through raw configuration parsing and updates, exercise final Docker spawn arguments with proxy settings, and clarify workspace-relative paths and forward-looking TOML rendering. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
In response to #71229 (comment): This is a review completion notice and needs no separate code change. The associated launcher-argument coverage feedback has been addressed. |
|
In response to #71229 (comment): The /matt command was already served. The resulting feedback has been addressed, including clarification that HTTP TOML rendering is forward-looking; no additional run was needed. |
|
In response to #71229 (comment): This status notice has since been updated to confirm review completion and requests no separate code change. Its associated TOML-rendering feedback has been addressed. |
|
In response to #71229 (comment): The /review command was already served. Its actionable feedback has been addressed in this batch; no additional review run or workflow dispatch was needed. |
|
🎉 This pull request is included in a new release. Release: |
Summary
Fixes #68033.
mcp-servers.<name>.ca-cert-fileto the frontmatter/MCP schemas, shared typed configuration, parser, validation, and gateway rendering (caCertFile). Reject non-HTTPS/non-HTTP servers, malformed paths, unsupported expressions, traversal, and gateway versions older than v0.4.35.${{ github.workspace }}/, and${{ runner.temp }}/paths after reviewed setup steps. Resolve symlinks, require readable regular files, bind-mount each PEM read-only at a server-specific gateway path, and pass mount arguments directly to Docker without shell interpolation. Works independently of customsandbox.mcp.mounts.gh aw mcp inspectexplicitly rejects this gateway-only configuration rather than silently ignoring it.Gateway compatibility
No version, digest, or workflow-lock upgrade is included. Upstream CA implementation commit
5f5248620cbe7fb1ee155bc3a7b3646d474721a6is already in both v0.4.35 and the current default v0.4.36. This PR does not depend on #71219 and does not compete with its v0.4.37 upgrade. #69436 was used only as historical proposal context, not treated as shipped functionality.Coverage and validation
pkg/cli,pkg/parser, andpkg/workflow: valid/invalid CA configuration, typed round-trip, schema compatibility, unsupported local inspection, old gateway rejection, and compiled gateway output for Copilot, Claude, Codex, and Gemini.make build,make fmt,make lint-cjs, JavaScript typecheck, schema freshness,make check-workflow-drift, and serializedgolangci-lint run --allow-serial-runners --timeout 10m ./pkg/cli ./pkg/parser ./pkg/types ./pkg/workflowpassed (0 Go lint issues).make agent-report-progressonce: impacted Go unit tests and impacted JavaScript tests passed. The complete gate remains blocked by pre-existing custom-linter diagnostics on unchanged lines in touched files (including existing builder writes/deferred closes). New diagnostics were fixed; a diff-line comparison of the rerun reports 0 diagnostics on introduced/changed lines, with 45 existing findings on unchanged lines in touched files. Shared Go lint contention was resolved using the linter's serialized-runner option; no shared locks were removed.Reproduce native TLS coverage after building v0.4.36:
GH_AW_MCPG_BINARY=/path/to/awmg npm run test:js -- mcp_server_ca.integration.test.cjsfromactions/setup/js. On macOS, use a canonical non-symlinkTMPDIRfor the existing gateway log-directory safety tests.Remaining validation limitation
A real Docker bind-mount end-to-end run could not start because the shared Docker daemon reports
no space left on device. No shared images/containers were pruned. Native gateway TLS behavior and generated mount/path handling were verified separately. No GitHub Actions workflow was dispatched.