Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 25 additions & 3 deletions cmd/thv/app/proxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"github.com/stacklok/toolhive/pkg/networking"
"github.com/stacklok/toolhive/pkg/transport"
"github.com/stacklok/toolhive/pkg/transport/middleware"
"github.com/stacklok/toolhive/pkg/transport/middleware/origin"
"github.com/stacklok/toolhive/pkg/transport/proxy/transparent"
"github.com/stacklok/toolhive/pkg/transport/types"
)
Expand Down Expand Up @@ -110,9 +111,10 @@ Dynamic client registration (automatic OAuth client setup):
}

var (
proxyHost string
proxyPort int
proxyTargetURI string
proxyHost string
proxyPort int
proxyTargetURI string
proxyAllowedOrigins []string

resourceURL string // Explicit resource URL for OAuth discovery endpoint (RFC 9728)

Expand All @@ -133,6 +135,10 @@ const (
func init() {
proxyCmd.Flags().StringVar(&proxyHost, "host", transport.LocalhostIPv4, "Host for the HTTP proxy to listen on (IP or hostname)")
proxyCmd.Flags().IntVar(&proxyPort, "port", 0, "Port for the HTTP proxy to listen on (host port)")
proxyCmd.Flags().StringArrayVar(&proxyAllowedOrigins, "allowed-origins", nil,
"Exact-match allowlist for the HTTP Origin header (repeatable). Recommended when binding publicly; "+
"loopback binds derive a default allowlist automatically, non-loopback binds log a warning when "+
"no value is supplied. Example: https://my-mcp.example.com")
proxyCmd.Flags().StringVar(
&proxyTargetURI,
"target-uri",
Expand Down Expand Up @@ -226,6 +232,22 @@ func proxyCmdFunc(cmd *cobra.Command, args []string) error {
// Create middlewares slice for incoming request authentication
var middlewares []types.NamedMiddleware

// Origin-header validation (DNS-rebinding protection per MCP 2025-11-25
// §"Security Warning"). Added first so disallowed Origins are rejected
// before authentication or any outbound token acquisition runs.
if allowed := origin.ResolveAllowedOrigins(proxyHost, port, proxyAllowedOrigins); len(allowed) > 0 {
middlewares = append(middlewares, types.NamedMiddleware{
Name: origin.MiddlewareType,
Function: origin.CreateOriginMiddleware(allowed),
})
} else {
slog.Warn("Origin validation disabled — no allowlist configured for non-loopback bind",
"host", proxyHost,
"port", port,
"hint", "pass --allowed-origins=https://your-client.example to enable DNS-rebind protection",
)
}

// Get OIDC configuration if enabled (for protecting the proxy endpoint)
oidcConfig := getProxyOIDCConfig(cmd)

Expand Down
11 changes: 11 additions & 0 deletions cmd/thv/app/run_flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,12 @@ type RunFlags struct {
RemoteForwardHeaders []string
RemoteForwardHeadersSecret []string

// AllowedOrigins is the HTTP Origin-header allowlist for DNS-rebinding protection
// (MCP 2025-11-25 §"Security Warning"). Empty with a loopback host auto-derives
// loopback-only defaults; empty with a non-loopback host disables the check
// (operator must supply explicit origins for public bind).
AllowedOrigins []string

// Runtime configuration
RuntimeImage string
RuntimeAddPackages []string
Expand All @@ -156,6 +162,10 @@ func AddRunFlags(cmd *cobra.Command, config *RunFlags) {
cmd.Flags().StringVar(&config.Name, "name", "", "Name of the MCP server (default to auto-generated from image)")
cmd.Flags().StringVar(&config.Group, "group", "default", "Name of the group this workload should belong to")
cmd.Flags().StringVar(&config.Host, "host", transport.LocalhostIPv4, "Host for the HTTP proxy to listen on (IP or hostname)")
cmd.Flags().StringArrayVar(&config.AllowedOrigins, "allowed-origins", nil,
"Exact-match allowlist for the HTTP Origin header (repeatable). Recommended when binding publicly; "+
"loopback binds derive a default allowlist automatically, non-loopback binds log a warning when "+
"no value is supplied. Example: https://my-mcp.example.com")
cmd.Flags().IntVar(&config.ProxyPort, "proxy-port", 0, "Port for the HTTP proxy to listen on (host port)")
cmd.Flags().IntVar(&config.TargetPort, "target-port", 0,
"Port for the container to expose (only applicable to SSE or Streamable HTTP transport)")
Expand Down Expand Up @@ -678,6 +688,7 @@ func buildRunnerConfig(
PrintOverlays: runFlags.PrintOverlays,
}),
runner.WithPublish(runFlags.Publish),
runner.WithAllowedOrigins(runFlags.AllowedOrigins),
}
opts = append(opts, extraOpts...)

Expand Down
1 change: 1 addition & 0 deletions docs/cli/thv_proxy.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions docs/cli/thv_run.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions docs/server/docs.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions docs/server/swagger.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 12 additions & 0 deletions docs/server/swagger.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions pkg/runner/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,14 @@ type RunConfig struct {
// TargetHost is the host to forward traffic to (only applicable to SSE transport)
TargetHost string `json:"target_host,omitempty" yaml:"target_host,omitempty"`

// AllowedOrigins is the allowlist of values accepted on the HTTP Origin header,
// used for DNS-rebinding protection per MCP 2025-11-25 §"Security Warning".
// When empty and Host is loopback (127.0.0.1 / localhost / [::1]), a default
// loopback-only allowlist is derived at middleware-wiring time.
// When empty and Host is non-loopback, the middleware is disabled — operators
// exposing the proxy publicly must configure an explicit allowlist.
AllowedOrigins []string `json:"allowed_origins,omitempty" yaml:"allowed_origins,omitempty"`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The operator path uses PopulateMiddlewareConfigs so the factory side is wired, but MCPServerSpec / MCPRemoteProxySpec / VirtualMCPServerSpec have no AllowedOrigins field. Combined with operator-deployed pods binding to non-loopback addresses, ResolveAllowedOrigins returns nil and addOriginMiddleware skips registration with a WARN — so K8s deployments ship with Origin validation disabled.

Is this expected, planned for a follow-up PR, or considered out of scope for the CRDs? If a follow-up, would it be worth a // TODO or a note in the PR description so it's tracked?


// Publish lists ports to publish to the host in format "hostPort:containerPort"
Publish []string `json:"publish,omitempty" yaml:"publish,omitempty"`

Expand Down
12 changes: 12 additions & 0 deletions pkg/runner/config_builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,18 @@ func WithAllowDockerGateway(allow bool) RunConfigBuilderOption {
}
}

// WithAllowedOrigins sets the HTTP Origin-header allowlist used for
// DNS-rebinding protection (MCP 2025-11-25 §"Security Warning").
// An empty slice defers the choice to middleware wiring, which derives a
// loopback-only default when the bind host is loopback and otherwise leaves
// the middleware disabled.
func WithAllowedOrigins(origins []string) RunConfigBuilderOption {
return func(b *runConfigBuilder) error {
b.config.AllowedOrigins = origins
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new addOriginMiddleware is added to PopulateMiddlewareConfigs, but WithMiddlewareFromFlags / addCoreMiddlewares (the path used by thv run) doesn't include it. Since runner.Run skips PopulateMiddlewareConfigs when MiddlewareConfigs is pre-populated (runner.go:232), thv run --allowed-origins=... plumbs the flag into RunConfig.AllowedOrigins — which is exactly what this builder option does — but the middleware never registers at runtime.

Is this intentional, or an omission? If intentional, what's the rationale for excluding thv run from the protection?

return nil
}
}

// WithTrustProxyHeaders sets whether to trust X-Forwarded-* headers from reverse proxies
func WithTrustProxyHeaders(trust bool) RunConfigBuilderOption {
return func(b *runConfigBuilder) error {
Expand Down
46 changes: 43 additions & 3 deletions pkg/runner/middleware.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package runner

import (
"fmt"
"log/slog"

"github.com/stacklok/toolhive/pkg/audit"
"github.com/stacklok/toolhive/pkg/auth"
Expand All @@ -20,6 +21,7 @@ import (
"github.com/stacklok/toolhive/pkg/recovery"
"github.com/stacklok/toolhive/pkg/telemetry"
headerfwd "github.com/stacklok/toolhive/pkg/transport/middleware"
"github.com/stacklok/toolhive/pkg/transport/middleware/origin"
"github.com/stacklok/toolhive/pkg/transport/types"
"github.com/stacklok/toolhive/pkg/usagemetrics"
"github.com/stacklok/toolhive/pkg/webhook/mutating"
Expand All @@ -43,6 +45,7 @@ func GetSupportedMiddlewareFactories() map[string]types.MiddlewareFactory {
audit.MiddlewareType: audit.CreateMiddleware,
recovery.MiddlewareType: recovery.CreateMiddleware,
headerfwd.HeaderForwardMiddlewareName: headerfwd.CreateMiddleware,
origin.MiddlewareType: origin.CreateMiddleware,
validating.MiddlewareType: validating.CreateMiddleware,
mutating.MiddlewareType: mutating.CreateMiddleware,
}
Expand All @@ -56,13 +59,21 @@ func PopulateMiddlewareConfigs(config *RunConfig) error {
var middlewareConfigs []types.MiddlewareConfig
// TODO: Consider extracting other middleware setup into helper functions like addUsageMetricsMiddleware

// Origin-validation middleware (DNS-rebinding protection per MCP 2025-11-25).
// Positioned first in the slice so it runs earliest in the chain — disallowed
// Origin values are rejected before any authentication or business logic.
middlewareConfigs, err := addOriginMiddleware(middlewareConfigs, config)
if err != nil {
return err
}

// Authentication middleware (always present)
authParams := auth.MiddlewareParams{
OIDCConfig: config.OIDCConfig,
}
authConfig, err := types.NewMiddlewareConfig(auth.MiddlewareType, authParams)
if err != nil {
return fmt.Errorf("failed to create auth middleware config: %w", err)
authConfig, authErr := types.NewMiddlewareConfig(auth.MiddlewareType, authParams)
if authErr != nil {
return fmt.Errorf("failed to create auth middleware config: %w", authErr)
}
middlewareConfigs = append(middlewareConfigs, *authConfig)

Expand Down Expand Up @@ -419,6 +430,35 @@ func addAWSStsMiddleware(middlewares []types.MiddlewareConfig, config *RunConfig
return append(middlewares, *awsStsMwConfig), nil
}

// addOriginMiddleware adds Origin-header validation middleware for DNS-rebind
// protection per MCP 2025-11-25 §"Security Warning". Default-derivation logic
// lives in origin.ResolveAllowedOrigins so the standalone `thv proxy` command
// and the runner path agree on behavior.
//
// When the effective allowlist is empty — which happens when the operator
// binds to a non-loopback host without supplying --allowed-origins — the
// middleware is skipped entirely and a WARN is logged so the security-disabled
// state is visible in operator logs. A follow-up PR hardens the non-loopback
// path by requiring an explicit opt-in flag (see audit row 22).
func addOriginMiddleware(middlewares []types.MiddlewareConfig, config *RunConfig) ([]types.MiddlewareConfig, error) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commit message says the middleware is wired into "thv run / thv-proxyrunner / vMCP", but vMCP composes its own chain via factory.NewIncomingAuthMiddleware and pkg/vmcp/server/server.go Handler — neither references the new origin package. Grep across pkg/vmcp/ and cmd/vmcp/ finds no usage.

Should vMCP be wired here too, or should the commit message be corrected to drop the vMCP claim and tracked as a follow-up?

allowed := origin.ResolveAllowedOrigins(config.Host, config.Port, config.AllowedOrigins)
if len(allowed) == 0 {
slog.Warn("Origin validation disabled — no allowlist configured for non-loopback bind",
"host", config.Host,
"port", config.Port,
"hint", "pass --allowed-origins=https://your-client.example to enable DNS-rebind protection",
)
return middlewares, nil
}

params := origin.MiddlewareParams{AllowedOrigins: allowed}
mwCfg, err := types.NewMiddlewareConfig(origin.MiddlewareType, params)
if err != nil {
return nil, fmt.Errorf("failed to create origin middleware config: %w", err)
}
return append(middlewares, *mwCfg), nil
}

// addRateLimitMiddleware adds rate limit middleware if configured.
func addRateLimitMiddleware(middlewares []types.MiddlewareConfig, config *RunConfig) ([]types.MiddlewareConfig, error) {
if config.RateLimitConfig == nil {
Expand Down
Loading
Loading