Skip to content
Merged
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
22 changes: 20 additions & 2 deletions cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,8 @@ type BearerTokenConfig struct {
type EmbeddedAuthServerConfig struct {
// Issuer is the issuer identifier for this authorization server.
// This will be included in the "iss" claim of issued tokens.
// Must be a valid HTTPS URL (or HTTP for localhost) without query, fragment, or trailing slash (per RFC 8414).
// Must be a valid HTTPS URL (or HTTP for localhost, or HTTP for trusted in-cluster hosts when
// insecureAllowHTTP is true) without query, fragment, or trailing slash (per RFC 8414).
// +kubebuilder:validation:Required
// +kubebuilder:validation:Pattern=`^https?://[^\s?#]+[^/\s?#]$`
Issuer string `json:"issuer"`
Expand All @@ -352,7 +353,8 @@ type EmbeddedAuthServerConfig struct {
// All other endpoints (token, registration, JWKS) remain derived from the issuer.
// This is useful when the browser-facing authorization endpoint needs to be on a
// different host than the issuer used for backend-to-backend calls.
// Must be a valid HTTPS URL (or HTTP for localhost) without query, fragment, or trailing slash.
// Must be a valid HTTPS URL (or HTTP for localhost, or HTTP for trusted in-cluster hosts
// when insecureAllowHTTP is true) without query, fragment, or trailing slash.
// +kubebuilder:validation:Pattern=`^https?://[^\s?#]+[^/\s?#]$`
// +optional
AuthorizationEndpointBaseURL string `json:"authorizationEndpointBaseUrl,omitempty"`
Expand Down Expand Up @@ -430,6 +432,22 @@ type EmbeddedAuthServerConfig struct {
// +optional
DisableUpstreamTokenInjection bool `json:"disableUpstreamTokenInjection,omitempty"`

// InsecureAllowHTTP permits an http:// issuer URL for non-localhost hosts.
// Only set this for in-cluster Kubernetes deployments where traffic between
// pods traverses a trusted network (e.g. the in-cluster service mesh).
// Production deployments reachable outside the cluster MUST use https://.
//
// On VirtualMCPServer: when false (the default), http:// issuers for non-localhost
// hosts are rejected at reconcile time with an AuthServerConfigValidated=False condition.
//
// On MCPServer and MCPRemoteProxy (via MCPExternalAuthConfig): this field is
// structurally present but enforcement is deferred to pod startup via Config.Validate();
// a misconfigured issuer will cause the pod to crash at startup rather than surface
// as an operator condition.
// +kubebuilder:default=false
// +optional
InsecureAllowHTTP bool `json:"insecureAllowHTTP,omitempty"`
Comment thread
tgrunnagle marked this conversation as resolved.

// BaselineClientScopes is a baseline set of OAuth 2.0 scopes guaranteed to be
// included in every client registration. The embedded auth server unions these
// scopes into the registered set returned by RFC 7591 Dynamic Client
Expand Down
38 changes: 33 additions & 5 deletions cmd/thv-operator/controllers/virtualmcpserver_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
stderrors "errors"
"fmt"
"maps"
"net/url"
"reflect"
"slices"
"strings"
Expand Down Expand Up @@ -42,6 +43,7 @@ import (
"github.com/stacklok/toolhive/cmd/thv-operator/pkg/virtualmcpserverstatus"
operatorvmcpconfig "github.com/stacklok/toolhive/cmd/thv-operator/pkg/vmcpconfig"
"github.com/stacklok/toolhive/pkg/authserver"
"github.com/stacklok/toolhive/pkg/networking"
vmcptypes "github.com/stacklok/toolhive/pkg/vmcp"
"github.com/stacklok/toolhive/pkg/vmcp/auth/converters"
authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types"
Expand Down Expand Up @@ -488,7 +490,33 @@ func (*VirtualMCPServerReconciler) validateAuthServerConfig(
metav1.ConditionFalse,
)
statusManager.SetObservedGeneration(vmcp.Generation)
return fmt.Errorf("%s", message)
return stderrors.New(message)
}

// Admission-time check: http:// issuers for non-localhost hosts require
// insecureAllowHTTP to be set explicitly. Without it the proxyrunner pod
// will crash at startup with a validateIssuerURL failure.
if strings.HasPrefix(cfg.Issuer, "http://") {
// url.Parse succeeds for any URL that passes the CRD regex; the
// parsed.Host != "" guard defends against the degenerate empty-host case.
parsed, err := url.Parse(cfg.Issuer)
if err == nil && parsed.Host != "" && !networking.IsLocalhost(parsed.Host) && !cfg.InsecureAllowHTTP {
message := fmt.Sprintf(
"spec.authServerConfig.issuer %q uses http:// with a non-localhost host; "+
"set spec.authServerConfig.insecureAllowHTTP: true to allow this for trusted "+
"in-cluster deployments, or use https:// for production deployments",
cfg.Issuer,
)
statusManager.SetPhase(mcpv1beta1.VirtualMCPServerPhaseFailed)
statusManager.SetMessage(message)
statusManager.SetAuthServerConfigValidatedCondition(
mcpv1beta1.ConditionReasonAuthServerConfigInvalid,
message,
metav1.ConditionFalse,
)
statusManager.SetObservedGeneration(vmcp.Generation)
return stderrors.New(message)
}
}

if len(cfg.UpstreamProviders) == 0 {
Expand All @@ -501,7 +529,7 @@ func (*VirtualMCPServerReconciler) validateAuthServerConfig(
metav1.ConditionFalse,
)
statusManager.SetObservedGeneration(vmcp.Generation)
return fmt.Errorf("%s", message)
return stderrors.New(message)
}

// Validate additionalAuthorizationParams on each upstream provider
Expand All @@ -518,7 +546,7 @@ func (*VirtualMCPServerReconciler) validateAuthServerConfig(
metav1.ConditionFalse,
)
statusManager.SetObservedGeneration(vmcp.Generation)
return fmt.Errorf("%s", message)
return stderrors.New(message)
}
}

Expand Down Expand Up @@ -1579,8 +1607,8 @@ func (*VirtualMCPServerReconciler) ensureServiceURL(
statusManager virtualmcpserverstatus.StatusManager,
) {
if vmcp.Status.URL == "" {
url := createVmcpServiceURL(vmcp.Name, vmcp.Namespace, vmcpDefaultPort)
statusManager.SetURL(url)
serviceURL := createVmcpServiceURL(vmcp.Name, vmcp.Namespace, vmcpDefaultPort)
statusManager.SetURL(serviceURL)
}
}

Expand Down
100 changes: 100 additions & 0 deletions cmd/thv-operator/controllers/virtualmcpserver_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3848,3 +3848,103 @@ func TestVirtualMCPServerReconciler_IdentitySynthesizedTransitionsOnValidationFa
assert.NotContains(t, cond.Message, "atlassian",
"stale message naming the now-removed upstream must not survive the broken edit")
}

// TestVirtualMCPServerValidateAuthServerConfig_InsecureAllowHTTP exercises the
// admission-time check that rejects http:// issuers for non-localhost hosts
// unless insecureAllowHTTP is explicitly set.
func TestVirtualMCPServerValidateAuthServerConfig_InsecureAllowHTTP(t *testing.T) {
t.Parallel()

validUpstreams := []mcpv1beta1.UpstreamProviderConfig{
{
Name: "dex",
Type: mcpv1beta1.UpstreamProviderTypeOIDC,
OIDCConfig: &mcpv1beta1.OIDCUpstreamConfig{
IssuerURL: "https://dex.example.com",
ClientID: "test-client",
},
},
}

tests := []struct {
name string
issuer string
insecureAllowHTTP bool
wantErr bool
wantCondition metav1.ConditionStatus
}{
{
name: "https issuer: always valid",
issuer: "https://authserver.example.com",
wantCondition: metav1.ConditionTrue,
},
{
name: "http localhost issuer: valid without flag",
issuer: "http://localhost:4483",
wantCondition: metav1.ConditionTrue,
},
{
name: "http in-cluster issuer without flag: rejected",
issuer: "http://vmcp-test.default.svc.cluster.local:4483",
wantErr: true,
wantCondition: metav1.ConditionFalse,
},
{
name: "http in-cluster issuer with flag: accepted",
issuer: "http://vmcp-test.default.svc.cluster.local:4483",
insecureAllowHTTP: true,
wantCondition: metav1.ConditionTrue,
},
{
name: "http non-localhost issuer without flag: rejected",
issuer: "http://authserver.example.com",
wantErr: true,
wantCondition: metav1.ConditionFalse,
},
{
name: "http non-localhost issuer with flag: accepted",
issuer: "http://authserver.example.com",
insecureAllowHTTP: true,
wantCondition: metav1.ConditionTrue,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()

vmcp := v1beta1test.NewVirtualMCPServer(testVmcpName, "default",
v1beta1test.WithVMCPGroupRef("test-group"),
v1beta1test.WithVMCPAuthServerConfig(&mcpv1beta1.EmbeddedAuthServerConfig{
Issuer: tt.issuer,
InsecureAllowHTTP: tt.insecureAllowHTTP,
UpstreamProviders: validUpstreams,
}),
v1beta1test.MutateVMCP(func(v *mcpv1beta1.VirtualMCPServer) {
v.Generation = 1
}),
)

r := &VirtualMCPServerReconciler{}
statusManager := virtualmcpserverstatus.NewStatusManager(vmcp)
err := r.validateAuthServerConfig(vmcp, statusManager)
statusManager.UpdateStatus(t.Context(), &vmcp.Status)

if tt.wantErr {
require.Error(t, err)
} else {
require.NoError(t, err)
}

cond := findCondition(vmcp.Status.Conditions, mcpv1beta1.ConditionTypeAuthServerConfigValidated)
require.NotNil(t, cond, "AuthServerConfigValidated condition must be set")
assert.Equal(t, tt.wantCondition, cond.Status)

if tt.wantErr {
assert.Equal(t, mcpv1beta1.ConditionReasonAuthServerConfigInvalid, cond.Reason)
assert.Contains(t, cond.Message, "insecureAllowHTTP",
"rejection message must guide the user to the fix")
}
})
}
}
4 changes: 4 additions & 0 deletions cmd/thv-operator/pkg/controllerutil/authserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -585,6 +585,10 @@ func BuildAuthServerRunConfig(
// Wire through upstream token injection flag
config.DisableUpstreamTokenInjection = authConfig.DisableUpstreamTokenInjection

// Wire through the insecure HTTP issuer flag from the CRD field.
// This replaces any auto-inference and moves control to the deployer.
config.InsecureAllowHTTP = authConfig.InsecureAllowHTTP

// Build CIMD configuration. CacheFallbackTTL is passed as-is (string);
// resolveCIMDConfig in the runner parses it to time.Duration at startup.
if authConfig.CIMD != nil && authConfig.CIMD.Enabled {
Expand Down
34 changes: 34 additions & 0 deletions cmd/thv-operator/pkg/controllerutil/authserver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1610,6 +1610,40 @@ func TestBuildAuthServerRunConfig(t *testing.T) {
"DisableUpstreamTokenInjection should default to false")
},
},
{
name: "insecureAllowHTTP true is propagated to RunConfig",
authConfig: &mcpv1beta1.EmbeddedAuthServerConfig{
Issuer: "http://vmcp-test.default.svc.cluster.local:4483",
InsecureAllowHTTP: true,
HMACSecretRefs: []mcpv1beta1.SecretKeyRef{
{Name: "hmac-secret", Key: "hmac"},
},
},
allowedAudiences: defaultAudiences,
scopesSupported: defaultScopes,
checkFunc: func(t *testing.T, config *authserver.RunConfig) {
t.Helper()
assert.True(t, config.InsecureAllowHTTP,
"InsecureAllowHTTP must propagate from CRD field to RunConfig")
},
},
{
name: "insecureAllowHTTP false is propagated to RunConfig",
authConfig: &mcpv1beta1.EmbeddedAuthServerConfig{
Issuer: "https://authserver.example.com",
InsecureAllowHTTP: false,
HMACSecretRefs: []mcpv1beta1.SecretKeyRef{
{Name: "hmac-secret", Key: "hmac"},
},
},
allowedAudiences: defaultAudiences,
scopesSupported: defaultScopes,
checkFunc: func(t *testing.T, config *authserver.RunConfig) {
t.Helper()
assert.False(t, config.InsecureAllowHTTP,
"InsecureAllowHTTP false must propagate from CRD field to RunConfig")
},
},
}

for _, tt := range tests {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,8 @@ spec:
All other endpoints (token, registration, JWKS) remain derived from the issuer.
This is useful when the browser-facing authorization endpoint needs to be on a
different host than the issuer used for backend-to-backend calls.
Must be a valid HTTPS URL (or HTTP for localhost) without query, fragment, or trailing slash.
Must be a valid HTTPS URL (or HTTP for localhost, or HTTP for trusted in-cluster hosts
when insecureAllowHTTP is true) without query, fragment, or trailing slash.
pattern: ^https?://[^\s?#]+[^/\s?#]$
type: string
baselineClientScopes:
Expand Down Expand Up @@ -307,11 +308,28 @@ spec:
type: object
type: array
x-kubernetes-list-type: atomic
insecureAllowHTTP:
default: false
description: |-
InsecureAllowHTTP permits an http:// issuer URL for non-localhost hosts.
Only set this for in-cluster Kubernetes deployments where traffic between
pods traverses a trusted network (e.g. the in-cluster service mesh).
Production deployments reachable outside the cluster MUST use https://.

On VirtualMCPServer: when false (the default), http:// issuers for non-localhost
hosts are rejected at reconcile time with an AuthServerConfigValidated=False condition.

On MCPServer and MCPRemoteProxy (via MCPExternalAuthConfig): this field is
structurally present but enforcement is deferred to pod startup via Config.Validate();
a misconfigured issuer will cause the pod to crash at startup rather than surface
as an operator condition.
type: boolean
issuer:
description: |-
Issuer is the issuer identifier for this authorization server.
This will be included in the "iss" claim of issued tokens.
Must be a valid HTTPS URL (or HTTP for localhost) without query, fragment, or trailing slash (per RFC 8414).
Must be a valid HTTPS URL (or HTTP for localhost, or HTTP for trusted in-cluster hosts when
insecureAllowHTTP is true) without query, fragment, or trailing slash (per RFC 8414).
pattern: ^https?://[^\s?#]+[^/\s?#]$
type: string
primaryUpstreamProvider:
Expand Down Expand Up @@ -1693,7 +1711,8 @@ spec:
All other endpoints (token, registration, JWKS) remain derived from the issuer.
This is useful when the browser-facing authorization endpoint needs to be on a
different host than the issuer used for backend-to-backend calls.
Must be a valid HTTPS URL (or HTTP for localhost) without query, fragment, or trailing slash.
Must be a valid HTTPS URL (or HTTP for localhost, or HTTP for trusted in-cluster hosts
when insecureAllowHTTP is true) without query, fragment, or trailing slash.
pattern: ^https?://[^\s?#]+[^/\s?#]$
type: string
baselineClientScopes:
Expand Down Expand Up @@ -1787,11 +1806,28 @@ spec:
type: object
type: array
x-kubernetes-list-type: atomic
insecureAllowHTTP:
default: false
description: |-
InsecureAllowHTTP permits an http:// issuer URL for non-localhost hosts.
Only set this for in-cluster Kubernetes deployments where traffic between
pods traverses a trusted network (e.g. the in-cluster service mesh).
Production deployments reachable outside the cluster MUST use https://.

On VirtualMCPServer: when false (the default), http:// issuers for non-localhost
hosts are rejected at reconcile time with an AuthServerConfigValidated=False condition.

On MCPServer and MCPRemoteProxy (via MCPExternalAuthConfig): this field is
structurally present but enforcement is deferred to pod startup via Config.Validate();
a misconfigured issuer will cause the pod to crash at startup rather than surface
as an operator condition.
type: boolean
issuer:
description: |-
Issuer is the issuer identifier for this authorization server.
This will be included in the "iss" claim of issued tokens.
Must be a valid HTTPS URL (or HTTP for localhost) without query, fragment, or trailing slash (per RFC 8414).
Must be a valid HTTPS URL (or HTTP for localhost, or HTTP for trusted in-cluster hosts when
insecureAllowHTTP is true) without query, fragment, or trailing slash (per RFC 8414).
pattern: ^https?://[^\s?#]+[^/\s?#]$
type: string
primaryUpstreamProvider:
Expand Down
Loading
Loading