Wire backend dial-control policy into vMCP serve.go #6566
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,7 @@ import ( | |
| "net" | ||
| "os" | ||
| "path/filepath" | ||
| "syscall" | ||
| "time" | ||
|
|
||
| "go.opentelemetry.io/otel/trace" | ||
|
|
@@ -31,6 +32,7 @@ import ( | |
| "github.com/stacklok/toolhive/pkg/container/runtime" | ||
| "github.com/stacklok/toolhive/pkg/groups" | ||
| "github.com/stacklok/toolhive/pkg/migration" | ||
| "github.com/stacklok/toolhive/pkg/networking" | ||
| "github.com/stacklok/toolhive/pkg/telemetry" | ||
| "github.com/stacklok/toolhive/pkg/versions" | ||
| "github.com/stacklok/toolhive/pkg/vmcp" | ||
|
|
@@ -144,6 +146,15 @@ func Serve(ctx context.Context, cfg ServeConfig) error { | |
| slog.Info("audit logging enabled with default configuration") | ||
| } | ||
|
|
||
| // Warn when the backend SSRF / DNS-rebinding guard is disabled. Both backend | ||
| // dial paths (per-call client and session factory) then dial private ranges | ||
| // unchecked; this is intended only for in-cluster / development use. | ||
| if vmcpCfg.BackendAllowPrivateIP { | ||
|
Collaborator
Author
There was a problem hiding this comment. [MEDIUM] Proxy env vars silently defeat the SSRF guard (Consensus: 7/10)
Consider disabling proxying on the guarded transport ( Raised by: security |
||
| slog.Warn("backendAllowPrivateIp is enabled; backend dials into private, loopback, and " + | ||
| "link-local ranges are NOT blocked (SSRF / DNS-rebinding guard disabled). " + | ||
| "Intended for in-cluster / development use only.") | ||
| } | ||
|
|
||
| // Load auth server config from sibling file if present. | ||
| // Skip in quick mode (no config file) — there is no sibling directory to search. | ||
| var authServerRC *authserverconfig.RunConfig | ||
|
|
@@ -358,6 +369,19 @@ func Serve(ctx context.Context, cfg ServeConfig) error { | |
| sessionFactoryOpts, | ||
| vmcpsession.WithRequestTimeoutResolver(backendRequestTimeoutResolver(vmcpCfg)), | ||
| ) | ||
| // Guard session-init dials against SSRF / DNS-rebinding into private ranges, | ||
| // unless the operator opted out for in-cluster / development use. The same | ||
| // policy guards the per-call backend client built in discoverBackends. | ||
| // | ||
| // The policy is currently global (config.BackendAllowPrivateIP), so the | ||
| // per-workload resolver returns the same hook for every backend; its shape | ||
| // leaves room for a future per-backend policy without re-wiring here. | ||
| if dialControl := backendDialControl(vmcpCfg); dialControl != nil { | ||
| sessionFactoryOpts = append(sessionFactoryOpts, | ||
|
Collaborator
Author
There was a problem hiding this comment. [MEDIUM] No test exercises the actual wiring at either production call site (Consensus: 9/10)
Consider extracting a Raised by: security, architecture, test-coverage |
||
| vmcpsession.WithDialControlResolver(func(string) func(network, address string, c syscall.RawConn) error { | ||
| return dialControl | ||
| })) | ||
| } | ||
| sessionFactory := vmcpsession.NewSessionFactory(outgoingRegistry, sessionFactoryOpts...) | ||
|
|
||
| // When the optimizer is enabled, its meta-tools are pass-through tools. | ||
|
|
@@ -545,6 +569,22 @@ func backendRequestTimeoutResolver(cfg *config.Config) func(workloadID string) t | |
| } | ||
| } | ||
|
|
||
| // backendDialControl returns the net.Dialer.Control hook that guards backend | ||
| // dials against SSRF / DNS-rebinding into private, loopback, or link-local | ||
| // ranges. It is the single policy source shared by both production dial paths — | ||
| // the per-call backend client (discoverBackends) and the session factory | ||
| // (Serve) — so neither can drift from the other. | ||
| // | ||
| // It returns nil (no guard) when cfg.BackendAllowPrivateIP is true, which is the | ||
| // opt-out for in-cluster / development deployments where backends legitimately | ||
| // resolve to private addresses. The default (false) returns the guarding hook. | ||
| func backendDialControl(cfg *config.Config) func(network, address string, c syscall.RawConn) error { | ||
| if cfg != nil && cfg.BackendAllowPrivateIP { | ||
|
Collaborator
Author
There was a problem hiding this comment. [HIGH] Default guard breaks backend connectivity in both deployment modes (Consensus: 9/10)
Consider scoping the guard to the actual SSRF risk (redirects / re-resolution after the initial connect) rather than the operator's own initially-configured target — see the existing Raised by: security |
||
| return nil | ||
| } | ||
| return networking.ProtectedDialerControl | ||
| } | ||
|
|
||
| // loadAndValidateConfig loads and validates the vMCP configuration file. | ||
| func loadAndValidateConfig(configPath string) (*config.Config, error) { | ||
| slog.Info(fmt.Sprintf("Loading configuration from: %s", configPath)) | ||
|
|
@@ -649,10 +689,15 @@ func discoverBackends( | |
| return nil, nil, nil, fmt.Errorf("failed to create outgoing authentication registry: %w", err) | ||
| } | ||
|
|
||
| clientOpts := []vmcpclient.Option{ | ||
| vmcpclient.WithRequestTimeoutResolver(backendRequestTimeoutResolver(cfg)), | ||
| } | ||
| // Guard per-call backend dials against SSRF / DNS-rebinding into private | ||
| // ranges, unless the operator opted out. Mirrors the session factory in Serve. | ||
| if dialControl := backendDialControl(cfg); dialControl != nil { | ||
| clientOpts = append(clientOpts, vmcpclient.WithDialControl(dialControl)) | ||
| } | ||
| backendClient, err := vmcpclient.NewHTTPBackendClient(outgoingRegistry, clientOpts...) | ||
| if err != nil { | ||
| return nil, nil, nil, fmt.Errorf("failed to create backend client: %w", err) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -70,6 +70,31 @@ func TestBackendRequestTimeoutResolver(t *testing.T) { | |
| } | ||
| } | ||
|
|
||
| // TestBackendDialControl covers the guarded default and the opt-out path for the | ||
| // backend dial-control policy wired into both production dial paths in serve.go. | ||
| func TestBackendDialControl(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| t.Run("guarded by default", func(t *testing.T) { | ||
| t.Parallel() | ||
| // Nil config and BackendAllowPrivateIP=false both mean "guard". | ||
| for _, cfg := range []*config.Config{nil, {}, {BackendAllowPrivateIP: false}} { | ||
| control := backendDialControl(cfg) | ||
| require.NotNil(t, control, "guarded default must install a dial control") | ||
| // The installed control must refuse a private-range dial. | ||
| require.Error(t, control("tcp", "127.0.0.1:8080", nil)) | ||
| // ...and permit a public one. | ||
| require.NoError(t, control("tcp", "93.184.216.34:443", nil)) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("opt-out disables the guard", func(t *testing.T) { | ||
| t.Parallel() | ||
| control := backendDialControl(&config.Config{BackendAllowPrivateIP: true}) | ||
| require.Nil(t, control, "opt-out must return no dial control so private dials are allowed") | ||
| }) | ||
| } | ||
|
|
||
| // TestLoadAndValidateConfig covers all config-loading paths. | ||
| func TestLoadAndValidateConfig(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -146,6 +146,15 @@ type Config struct { | |
| // Operational configures operational settings. | ||
| Operational *OperationalConfig `json:"operational,omitempty" yaml:"operational,omitempty"` | ||
|
|
||
| // BackendAllowPrivateIP allows the virtual MCP server to dial backend | ||
| // endpoints that resolve to private, loopback, or link-local addresses. | ||
| // When false (the default), backend dials into those ranges are refused | ||
| // after DNS resolution to blunt SSRF / DNS-rebinding, which is the safe | ||
| // production behavior. Enable only for in-cluster or development | ||
| // deployments where backends legitimately resolve to private addresses. | ||
| // +optional | ||
| BackendAllowPrivateIP bool `json:"backendAllowPrivateIp,omitempty" yaml:"backendAllowPrivateIp,omitempty"` | ||
|
|
||
| // Metadata stores additional configuration metadata. | ||
| Metadata map[string]string `json:"metadata,omitempty" yaml:"metadata,omitempty"` | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[HIGH] Unrelated
miniredisimport commit bundled into this PR (Consensus: 8/10)This PR's own description covers only
pkg/networking,pkg/vmcp/config,pkg/vmcp/cli/serve.go, and generated CRD artifacts — nothing about the auth server. This import fixes a pre-existing compile error for a no-auth-Redis test, unrelated to SSRF/dial-control wiring. Per this repo's PR-scope rule, each PR should contain only related changes.Consider rebasing this commit out onto its own PR against
main(or dropping it here if it's already landed elsewhere).Raised by: general-quality, test-coverage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intentional and kept here as a build fix. #6551 added a
miniredis.RunT(t)call to this test but omitted thegithub.laiyagushi.com/alicebob/miniredis/v2import (the dependency is already ingo.mod), sopkg/authserver/runner's test package does not compile on currentmain—go vet/go test/golangci-lintover./...all fail withundefined: miniredis(go build ./...passes because it skips test files, which is why it slipped through). The one-line import add restores the build, which this PR needs in order to runtask test/task lint-fixat all. Agree it's out of scope for the dial-control feature; happy to split it into a standalone hotfix PR againstmainif you'd prefer it not ride along here.