RFC: Cloudflare deployment artifact for ToolHive MCP servers

[samuv]

Summary

Proposes thv-cloudflare, a dedicated artifact for deploying and reconciling ToolHive-managed MCP servers on Cloudflare Workers and Containers.

Related to stacklok/toolhive#5854.

Phase 1 scope

• One declarative MCPServerDeployment
• One Cloudflare Worker and deterministic Container instance
• Streamable HTTP transport
• Validate, plan, apply, status, update, readiness, and delete lifecycle
• Authentication required by default, with an explicit insecure exception
• MCP OAuth flow with DCR and PKCE
• GitHub App authentication with per-user upstream token injection
• SQLite-backed Durable Object state with encrypted upstream tokens
• Cloudflare Secrets Store references
• Disabled or unrestricted container egress
• Explicit client, image, registry, and authentication compatibility matrix

Security and delivery

The RFC includes:

• A blocking 13-control Phase 1 security gate
• Two-user identity and session-isolation acceptance tests
• A dependency-ordered P1-00 through P1-14 shipping breakdown
• Concrete release, documentation, compatibility, and rollback criteria

Review focus

Feedback is especially useful on:

• The dedicated artifact and Cloudflare-native manifest boundary
• DCR and GitHub App authentication scope
• The per-request bearer-token compatibility requirement
• Durable Object token storage and key rotation
• Wrangler as the initial replaceable deployment backend
• Phase 1 security and shipping gates

[JAORMX]

RFC Review: THV-0082 — Cloudflare Deployment Artifact

Summary

So, this is a genuinely strong RFC. The security discipline is the best I've seen in this repo, the Cloudflare claims check out against the primary docs, and the CloudflareDeployer interface is the right call. I'm recommending revision rather than acceptance for one main reason: it reimplements ToolHive's entire security-critical OAuth core in TypeScript inside the Worker, when that core already exists and is already conformance-tested in Go. That, plus a provider-prefixed manifest API and a few deviations from the original issue that aren't owned by name, are worth fixing before this ships. The good news is the biggest one is fixable by topology, not more tests.

Strengths

• The SEC-01..13 blocking security gate and the dependency-ordered P1-00..14 breakdown are unusually rigorous. Secure-by-default with an explicit --allow-insecure-public-endpoint opt-in matches ToolHive's stance exactly.
• The Cloudflare facts are accurate. I verified the egress/HTTPS-interception mechanics, the Containers/Durable Object model, DO encryption-at-rest (app-layer AES-GCM on top is legitimately defense-in-depth, not redundant), the Secrets Store binding shape, and the GHCR exclusion against the primary docs. No contradictions found.
• The CloudflareDeployer interface (Inspect/Plan/Apply/Delete/WaitReady, argv-only Wrangler invocation, pinned version) correctly confines the Node dependency to thv-cloudflare and makes a future Go backend a drop-in replacement.
• The stateless reconcile (no local state, desired = manifest, actual = Cloudflare) is the right model for CI, and the ownership-marker + desired-digest approach faithfully transplants operator principles to a CLI.
• cmd/thv-cloudflare fits the existing thv-<role> multi-binary pattern cleanly.

Critical issues (must fix)

1. The security core should stay in Go, not be rewritten in TypeScript. This is the big one and it's covered in detail in the inline comment on the Worker-side TS modules. Short version: the RFC reimplements DCR, PKCE, the OAuth metadata, JWKS rotation, the token vault, and upstreamInject in TS, when all of that already exists in pkg/auth/* and pkg/vmcp/auth/*. Two implementations of security-critical code in two languages will drift. The fix is the topology the original issue #5854 actually proposed: keep the security core in the existing Go proxy running as the container entrypoint, and shrink the Worker to a dumb, non-security shim that routes paths and serves the static metadata docs. One implementation of the auth core, in Go. If the broker stays Worker-resident instead, then at minimum this needs to be flagged as a risk and backed by a shared conformance harness run against both implementations in CI.

2. The new provider-prefixed manifest API invents parallel vocabulary instead of a portable core. cloudflare.toolhive.stacklok.dev bakes the provider into the API group (Open Question 1 already flags this as unresolved), and the manifest invents runtime.instanceName, permissions.egress.mode: disabled|unrestricted, and the MultiUserPerRequest|SingleIdentity|Unknown status enum with no mapping to the existing PermissionProfile / auth-server vocabulary. The "later conversion layer" is currently one sentence. I'd like a Portable Core subsection with an explicit field mapping, a neutral API group with a provider: cloudflare discriminator, and the allowlist egress enum value reserved now (strict-decoded v1alpha1 makes adding it later a breaking change).

3. Several deviations from issue #5854 aren't owned by name. The issue asked for three run modes (the RFC ships one), pure Go with no Node (the RFC ships Wrangler), CRD reuse (the RFC goes greenfield), and OCI mirroring (deferred, but the success criteria depend on a manually mirrored image with no tooling to produce it). Most are defensible calls. They just need to be stated as deliberate tradeoffs in the Non-Goals so the RFC reads as a faithful response to the issue it came from. Detail in the inline comment on the Non-Goals list.

Suggestions (should consider)

• Secrets Store is in open beta and is a blocking Phase-1 dependency for every secret type. Add an explicit written risk-acceptance or a documented fallback (Workers encrypted vars).
• Give CloudflareDeployer typed errors (ErrUnownedCollision, ErrVersionDrift, ErrPreflightFailed) and a credential-passage contract, and add a default deadline to Apply.
• Add a Drifted: true condition to status (compare the live TOOLHIVE_DESIRED_DIGEST to the manifest's canonical digest) so the stateless model can detect divergence between CI runs, not just converge on apply.
• interceptOutboundHttp is named in the issue but never addressed, even to explain the deferral to Container-level egress.
• Goal 11 ("track compatibility explicitly") has no test or gate. The matrix is prose, not a verifiable property. Add a unit asserting a few "unsupported combination -> correct error" cases.
• The GitHub App callback URL is manual; apply doesn't verify it matches before the OAuth redirect, which is the worst time to discover a mismatch.

Security assessment

The security section is the strongest part of the RFC: real threat model, all required subsections present, secure defaults, layered encryption, reference-only secrets, and a blocking gate with named owners. My one concern is structural, not content: the Worker-resident broker is the component that carries most of these guarantees, and it's the component being reimplemented in a second language. Moving that core to the existing Go proxy (or locking the two implementations together with a shared conformance suite) is what makes the otherwise-excellent security story hold up over time. Also worth elevating the Secrets Store open-beta dependency into an explicit risk-acceptance.

Architectural alignment

Mostly aligned. The dedicated binary, the narrow deployer interface, the stateless ownership-marker reconcile, and the secure-by-default posture all fit ToolHive's conventions. The two places it diverges are the two critical issues above: the second-language security core (against the "don't duplicate security-critical logic" spirit) and the provider-coupled manifest vocabulary (against the "platform, not runner" and RunConfig-portability principles). Both are fixable without changing the RFC's Phase-1 scope.

Questions for the author

1. Is there a reason the security core has to live in the Worker rather than in the existing Go proxy as the container entrypoint? If it's DO-state access, the outboundByHost shim (option 2 in the inline comment) or external Redis (option 1) both keep it in Go.
2. Are you open to a neutral API group + provider: cloudflare discriminator, and a documented portable-core field mapping, instead of the provider-prefixed group?
3. Can the Non-Goals name the dropped run modes (daemon, self-hosted-on-Cloudflare) and the deferred pure-Go / image-mirroring work explicitly?
4. On Wrangler distribution (Open Question 2): do you have a lean? The ship checklist can't be fully validated while that's open.
5. For the Phase-2 native-Go trigger, is "Cloudflare exposes a public Containers REST API and cloudflare-go ships a containers package" the right condition to write down?

Recommendation

• Ready to accept
• Accept with minor changes
• Needs revision (address critical issues)
• Major rework needed

Happy to talk through the Go-proxy topology in more detail if useful. And thanks @samuv for putting this together, the depth here (especially the security gate and the shipping breakdown) is exactly what a deployment-mode RFC should have.

[JAORMX]

So, this line is the one I'd most like to see change before this ships.

Right now the whole security core (DCR, PKCE, RFC 9728/8414/8707 metadata, one-time codes, JWKS rotation, hashed refresh-token families, the AEAD token vault, tsid binding, upstreamInject) gets reimplemented in TypeScript inside the Worker. That's the exact surface that already exists and is already conformance-tested in Go: pkg/auth/upstreamswap/middleware.go, pkg/auth/upstreamtoken/service.go, pkg/auth/dcr/, and pkg/vmcp/auth/strategies/upstream_inject.go.

Two implementations of security-critical OAuth logic in two languages will drift. A bug fixed in the Go path won't get fixed in the TS path (or the other way around) until something breaks. This is the kind of divergence the repo's own boundaries exist to prevent, and right now the RFC doesn't flag it as a risk.

The good news is we don't have to accept it. The topology the original issue #5854 proposed (thin Worker router + Go proxy container) already solves it:

• Keep the security core in the Go proxy, running as the container entrypoint wrapping the MCP server image. Same data-plane pattern ToolHive uses everywhere else (proxyrunner in K8s, the transparent proxy locally). One implementation of the auth core, in Go. The conformance suite already exists for it.
• Shrink the Worker to a dumb, non-security shim: route declared paths, serve the two static metadata docs (RFC 9728 PRM and RFC 8414 AS metadata are per-deployment constants you can render at apply time), healthz. No tokens, no crypto, no DCR. It becomes freely replaceable, same philosophy as Wrangler-behind-the-interface.

The one real design decision is state. DO storage is only reachable from Workers code, so three options:

1. External Redis (reuses the existing auth-server Redis backend, operator parity) but adds an out-of-account dependency this RFC deliberately avoided.
2. A Worker-side dumb blob store over DO SQLite. The Go proxy holds all keys and crypto; the shim stores opaque AEAD ciphertext (your AD-bound encryption means shim tampering gets detected in Go). The container reaches it without a public hairpin via outboundByHost, which per the docs invokes Worker code from the container on a plain HTTP virtual host.
3. Latency is manageable either way: JWT validation is local (Go is the issuer), token reads are cacheable, refresh is rare (GitHub user tokens live ~8h).

Honest costs: OAuth endpoints now cold-start when the container sleeps (mitigated by keeping static metadata in the shim and tuning sleepAfter), and the derived-image build + manual mirror pipeline becomes Phase-1-relevant instead of Phase-2. But that also fixes a separate gap, there's currently no tooling to produce the image the E2E depends on.

If you'd rather keep the broker Worker-resident, then at minimum I'd want (a) this called out in Residual Risks, and (b) a shared, versioned OAuth/MCP conformance harness run against both the Go and TS implementations in CI. But moving the core into the existing Go proxy is the cleaner fix and, I think, less total code than the TS rewrite.

[JAORMX]

The justification for a new API instead of reusing the MCPServer CRD is honest (the CRD carries K8s scheduling/pod/namespace semantics that don't map). But I think this inverts the dependency in a way we'll regret.

Two things:

First, the API group is provider-coupled. cloudflare.toolhive.stacklok.dev bakes the provider into the group, and Open Question 1 already flags this as unresolved. The existing operator CRD group is a domain (mcp.toolhive.stacklok.dev), not a provider prefix. Baking the provider in makes the "later conversion layer" a rename instead of a projection. A neutral group with a provider: cloudflare discriminator would age better.

Second, the manifest invents parallel vocabulary for concepts ToolHive already has: runtime.instanceName/maxInstances, permissions.egress.mode: disabled|unrestricted, and the status enum MultiUserPerRequest|SingleIdentity|Unknown. None of these reference the existing PermissionProfile egress-allowlist model or the auth-server config shape. The "later conversion layer" (the Forward Compatibility bullet) is currently a single sentence. I'd rather see a Portable Core subsection with an explicit field mapping (server.image, transport.{type,port,path}, permissions.egress -> allowlist superset, auth -> existing auth-server config, secrets -> SecretKeyRef) so we can see the mapping is real and not aspirational.

One concrete ask on egress: since v1alpha1 is strict-decoded, reserve an allowlist enum value for permissions.egress.mode now even if it's unimplemented. Otherwise adding it later is a breaking schema change you could have avoided for free.

[JAORMX]

This Non-Goals list is thorough, but it quietly drops a few things the original issue #5854 explicitly asked for, and I'd like to see them owned by name here rather than discovered by a reviewer cross-referencing the issue:

• The issue wanted one reconciler core with three run modes (--once CI, a long-running daemon, and self-hosted-on-Cloudflare). This RFC ships only the one-shot command. The compatibility matrix mentions "Git watcher, daemon, self-hosted reconciler" as one Phase-2 bullet, but daemon and self-hosted-on-Cloudflare are two architecturally distinct modes and neither is named here.
• The issue explicitly wanted pure Go with no Node dependency. That's a central architectural tradeoff, not just an alternative. Worth stating here as an explicit Non-Goal ("a native Go implementation of every Cloudflare API" is close but doesn't say "we are deferring the issue's no-Node constraint").
• OCI image mirroring to registry.cloudflare.com in pure Go. Deferred here, but the Phase-1 success criteria depend on a manually mirrored image and there's no tooling to produce it, so "a new user can reproduce the E2E guide from released artifacts" isn't actually achievable without an out-of-band manual step. Either ship a minimal mirror helper or say plainly that Phase 1 can't be reproduced without manual image ops.

None of these are necessarily the wrong call. They just need to be visible as deliberate tradeoffs so the RFC reads as a faithful response to the issue it came from.

[JAORMX]

Worth flagging explicitly: Cloudflare Secrets Store is currently in open beta (per https://developers.cloudflare.com/secrets-store/), and Phase 1 makes it a blocking dependency for the GitHub App client secret, the JWT signing keys, and the token-encryption keys. A beta regression would block the entire deployment.

The RFC does say beta/stability status gets reported by validate and tracked in the compatibility matrix, which is good. But given it's a hard dependency for every secret type in Phase 1, I'd add an explicit written risk-acceptance here, or a documented fallback (Workers encrypted vars) if Secrets Store hits a breaking change mid-beta. Right now it's acknowledged but not really owned.

[JAORMX]

This interface is the strongest decision in the RFC, clean and replaceable. Two small hardening asks:

1. Every method returns bare error. For a stateless reconciler, plan/apply need to distinguish "refuse unowned collision" from "transient API failure" for retry and exit codes. Worth defining typed errors (ErrUnownedCollision, ErrVersionDrift, ErrPreflightFailed) so callers can use errors.Is/As. Right now "version drift fails before mutation with a clear installation command" is prose, not a contract.

2. The argv-array rule prevents command injection, but nothing says how the Cloudflare API credential reaches the Wrangler subprocess. Add an explicit contract: the credential goes to the pinned Wrangler process via a single scoped env var, never written to generated config, never inherited by the Container. SEC-07 covers log hygiene but not this passage path.

Also: Apply has no deadline. It takes ctx (good) but P1-03's exit criterion should require a default apply timeout so a hung control plane doesn't hang the command.

[JAORMX]

The stateless model (no local state file, desired = manifest, actual = Cloudflare) is the right call for CI and I'm glad it's here. One gap: reconciliation only happens on explicit plan/apply/status. Between invocations, Cloudflare-side drift (someone edits the Worker, removes a binding, DO eviction) is invisible until the next command.

status reports a point-in-time actual, but nothing flags "actual diverged from last-applied digest without a new apply." You already have TOOLHIVE_DESIRED_DIGEST on the live Worker, so this is cheap: have status compute and surface a Drifted: true condition when the live digest no longer equals the canonical digest of the manifest being inspected, with a clear remediation (apply to converge). That turns the stateless model from "converges when you run it" into "can also tell you when it needs to converge," which is what makes it trustworthy in CI.

[JAORMX]

I checked this claim against cloudflare/cloudflare-go v7 and it's correct, but the gap is narrower and more specific than "the complete Containers deployment surface," and it's worth being precise here because it defines the Phase-2 scope and the exit condition.

The Go SDK today does cover: Worker script upload (workers.ScriptService), Durable Object bindings + SQLite migrations (NewSqliteClasses etc.), Secrets Store bindings and full store/secret CRUD, and routes. What it does NOT have is (a) a containers package / container binding type in the Workers Script Upload metadata, and (b) any documented public flow for pushing an image to registry.cloudflare.com (Wrangler handles that auth automatically, undocumented).

So the actual Phase-2 blocker is two undocumented surfaces: the containers[] application config block (image ref, class binding, max_instances, instance_type, rollout) and the registry push auth. Everything else is already reachable in pure Go. Naming that precisely makes the Phase-2 trigger concrete: "when Cloudflare exposes a public Containers REST API and cloudflare-go ships a containers package."

And to be clear about the recommendation: keep Wrangler behind CloudflareDeployer for Phase 1. Reverse-engineering those two undocumented surfaces is fragile and breaks on every Wrangler release, which is exactly the kind of undocumented-internal-API dependency this RFC's security posture should reject. The interface is the load-bearing part; the backend is replaceable.