Repository navigation
Conversation
Remote MCP servers can now carry a tls config with a CA file, inline CA PEM, or a pinned SHA256 certificate fingerprint, scoping trust to that server only. System verification is unchanged for everything else.
Remote MCP servers at private IP addresses can now be trusted on first use instead of requiring a tls config: the first connect fails with the presented certificate's fingerprint and the command to trust it; opencode mcp trust <url> [--force] pins the certificate for the project; later connects verify the pinned certificate, and rotations fail until re-pinned with --force. Hostnames and public IPs never auto-trust; an explicit tls config takes precedence. Adds the mcp_trusted_host table (per project, machine-local), the mcp.trust.inspect and mcp.trust.pin endpoints, and the CLI command. Builds on the per-server tls config from the previous pull request.
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 72 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
There was a problem hiding this comment.
Trust-on-first-use works for the PR's own test certificate, but not for the certificates it is meant for: trusting a typical appliance certificate still leaves the server unable to connect. I ran the core flow against real TLS servers, including one with a self-signed CN=OPNsense certificate without IP subject alternative names (SANs). I also ran the new core, server and CLI tests (all pass) and tsc in core, server and cli (clean). I reviewed only the trust-on-first-use commit and left the tls config part to #60039.
Public API (all additions, backwards compatible):
- CLI command
opencode mcp trust <url> [--force]. - HTTP endpoints
POST /api/experimental/mcp/trust/inspectandPOST /api/experimental/mcp/trust, in the OpenAPI spec. - Client methods
mcp.trust.inspectandmcp.trust.pin. - Schemas
Mcp.TrustedHost,Mcp.TrustInspectandMcp.TrustResult. - New database table
mcp_trusted_hostwith a migration.
Concerns, see the inline comments:
- A pinned certificate without the IP in its subject alternative names still fails to connect (
ERR_TLS_CERT_ALTNAME_INVALID), and most appliance certificates are like that. - Unrelated connection failures on private IPs are reported as "certificate not trusted", which hides the real error.
--forcealso skips the confirmation, including when re-pinning a changed certificate.
A test with a certificate that has no IP SAN would cover the main case. The existing tests only use a certificate with IP:127.0.0.1.
| `TLS certificate for ${host}:${McpTls.port(url)} changed since it was last trusted: expected ${stored.fingerprint}, got ${cert.fingerprint}. If the appliance rotated its certificate, re-pin it with: opencode mcp trust ${config.url} --force`, | ||
| ), | ||
| ) | ||
| return { ...config, tls: { ca_pem: cert.pem } } |
There was a problem hiding this comment.
Using the pinned certificate as a ca_pem trust anchor only works if it lists the server's IP in its subject alternative names. Bun's fetch still checks that the names match. I tried a self-signed CN=OPNsense certificate without SANs, the kind most appliances ship: mcp.trust succeeds, but the reconnect fails with ERR_TLS_CERT_ALTNAME_INVALID. explainFailure then returns the raw error, since the stored fingerprint matches. The fixture certificates include IP:127.0.0.1, so the tests don't catch this. You have already checked the exact fingerprint, so the name check could be replaced by an exact-fingerprint comparison on this connection, for example a checkServerIdentity that compares fingerprint256. That would also remove the gap between the probe and the real connection. #60039's fingerprint mode has the same issue.
This rewrites every failed connect to a private-IP https server without a stored pin into "TLS certificate … is not trusted", whatever the actual error was. With the CA trusted through NODE_EXTRA_CA_CERTS and a server that returns HTTP 500, the status says the certificate isn't trusted and suggests mcp trust. The real error is lost. It also opens an extra TLS probe on every failure. Could you only rewrite the message when the error is a certificate verification failure (e.g. DEPTH_ZERO_SELF_SIGNED_CERT, SELF_SIGNED_CERT_IN_CHAIN, UNABLE_TO_VERIFY_LEAF_SIGNATURE, ERR_TLS_CERT_ALTNAME_INVALID), and keep the original message otherwise?
| } | ||
| log.warn(`The certificate changed: the previously trusted fingerprint was ${stored.fingerprint}`) | ||
| } | ||
| if (!force) yield* requireInteractive("Pass --force when trusting a certificate without an interactive terminal") |
There was a problem hiding this comment.
--force does two things here: it allows re-pinning a changed certificate, and it skips the confirmation, even in an interactive terminal. A changed certificate is exactly when a tampered connection is most likely, yet that is the case that never asks. The description and docs say the command asks for confirmation. Consider still prompting when a terminal is available and only skipping the prompt when there isn't one, or keeping 're-pin' and 'skip the prompt' as separate flags.
# Conflicts: # packages/client/src/effect/api/api.ts # packages/client/src/effect/generated/client.ts # packages/client/src/promise/generated/client.ts # packages/client/src/promise/generated/types.ts # packages/core/schema.json # packages/core/src/database/migration.gen.ts
There was a problem hiding this comment.
The new push only merges dev, so neither earlier thread is addressed yet: a pinned appliance certificate without the IP in its SANs still fails to connect, unrelated errors are still reported as untrusted certificates, and --force still skips the confirmation. The trust tests in core, server and protocol pass, and tsc is clean in core, server, cli and client.
What the merge changes: after #59072, the regenerated client lists these endpoints under client.experimental.mcp.trust.inspect / .pin, and client.mcp.trust.* is generated only as a deprecated alias, "Removed in the next minor". packages/cli/src/commands/handlers/mcp/trust.ts still calls client.mcp.trust.inspect and client.mcp.trust.pin, so opencode mcp trust will break when the aliases are removed. Please switch it to client.experimental.mcp.trust.*.
Public API (additions, backwards compatible): the opencode mcp trust <url> [--force] CLI command; POST /api/experimental/mcp/trust/inspect and POST /api/experimental/mcp/trust, which the SDK now exposes as client.experimental.mcp.trust.inspect / .pin; the Mcp.TrustedHost, Mcp.TrustInspect and Mcp.TrustResult schemas; and the mcp_trusted_host table.
Issue for this PR
Part of #40111 — trust-on-first-use for private-network MCP servers.
Stacked on #60039 (per-server
tlsconfig); this diff includes that PR until it lands, then shrinks to the trust-on-first-use portion.What does this PR do?
For a remote MCP server whose URL is a private IP address (RFC 1918, loopback, link-local) with no
tlsconfig, the first connect fails with the presented certificate's fingerprint, subject, and the exact command to trust it.opencode mcp trust <url> [--force]shows the certificate, asks for confirmation, and pins it for the project (machine-local store, per project). Later connects verify the server presents exactly that certificate; a rotated certificate fails until re-pinned with--force. Hostnames and public IPs never auto-trust; an explicittlsconfig takes precedence.Adds the
mcp_trusted_hosttable and migration, themcp.trust.inspect/mcp.trust.pinendpoints with regenerated client, and the CLI command.How did you verify your code works?
--force, hostname/public-IP rejection, TOCTOU guard), 2 end-to-end server tests over real HTTP, CLI command testsChecklist
Added by the opencode bot (AI-generated).
Related: #40111, #57012
Public API: adds the
opencode mcp trust <url> [--force]CLI command, the experimental/api/experimental/mcp/trustendpoints (client.experimental.mcp.trust.inspect/.pin),Mcp.TrustedHost/TrustInspect/TrustResultschemas and themcp_trusted_hosttable (backwards compatible)