From ae32676181b122b4d682c8c7845b37829d9cbea4 Mon Sep 17 00:00:00 2001 From: amirejaz Date: Wed, 28 Jan 2026 16:37:21 +0000 Subject: [PATCH 1/4] Add bearer token authentication support for MCPRemoteProxy in Kubernetes --- .../mcpexternalauthconfig_controller.go | 159 ++++++++- .../mcpexternalauthconfig_controller_test.go | 205 +++++++++--- .../controllers/mcpremoteproxy_deployment.go | 15 + .../mcpremoteproxy_deployment_test.go | 61 ++++ .../mcpremoteproxy_runconfig_test.go | 233 ++++++++++++++ .../pkg/controllerutil/resources.go | 22 ++ .../pkg/controllerutil/resources_test.go | 304 ++++++++++++++++++ .../pkg/controllerutil/tokenexchange.go | 100 +++++- .../mcpremoteproxy_with_bearer_token.yaml | 60 ++++ pkg/config/config.go | 51 ++- pkg/config/config_test.go | 81 ++++- 11 files changed, 1227 insertions(+), 64 deletions(-) create mode 100644 cmd/thv-operator/pkg/controllerutil/resources_test.go create mode 100644 examples/operator/external-auth/mcpremoteproxy_with_bearer_token.yaml diff --git a/cmd/thv-operator/controllers/mcpexternalauthconfig_controller.go b/cmd/thv-operator/controllers/mcpexternalauthconfig_controller.go index 65fdf6cbde..3589d20916 100644 --- a/cmd/thv-operator/controllers/mcpexternalauthconfig_controller.go +++ b/cmd/thv-operator/controllers/mcpexternalauthconfig_controller.go @@ -5,9 +5,12 @@ package controllers import ( "context" + "crypto/sha256" + "encoding/hex" "fmt" "time" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" @@ -40,6 +43,7 @@ type MCPExternalAuthConfigReconciler struct { // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpexternalauthconfigs/status,verbs=get;update;patch // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpexternalauthconfigs/finalizers,verbs=update // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpservers,verbs=get;list;watch;update;patch +// +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch // Reconcile is part of the main kubernetes reconciliation loop which aims to // move the current state of the cluster closer to the desired state. @@ -77,8 +81,12 @@ func (r *MCPExternalAuthConfigReconciler) Reconcile(ctx context.Context, req ctr return ctrl.Result{RequeueAfter: externalAuthConfigRequeueDelay}, nil } - // Calculate the hash of the current configuration - configHash := r.calculateConfigHash(externalAuthConfig.Spec) + // Calculate the hash of the current configuration (including referenced Secret values) + configHash, err := r.calculateConfigHash(ctx, externalAuthConfig) + if err != nil { + logger.Error(err, "Failed to calculate config hash") + return ctrl.Result{}, err + } // Check if the hash has changed if externalAuthConfig.Status.ConfigHash != configHash { @@ -131,9 +139,71 @@ func (r *MCPExternalAuthConfigReconciler) Reconcile(ctx context.Context, req ctr return ctrl.Result{}, nil } -// calculateConfigHash calculates a hash of the MCPExternalAuthConfig spec using Kubernetes utilities -func (*MCPExternalAuthConfigReconciler) calculateConfigHash(spec mcpv1alpha1.MCPExternalAuthConfigSpec) string { - return ctrlutil.CalculateConfigHash(spec) +// calculateConfigHash calculates a hash of the MCPExternalAuthConfig spec including referenced Secret values +// This ensures that changes to Secret values trigger reconciliation +func (r *MCPExternalAuthConfigReconciler) calculateConfigHash( + ctx context.Context, + externalAuthConfig *mcpv1alpha1.MCPExternalAuthConfig, +) (string, error) { + // Start with the base spec hash + hashString := ctrlutil.CalculateConfigHash(externalAuthConfig.Spec) + + // Include referenced Secret values in the hash for bearer token configs + if externalAuthConfig.Spec.Type == mcpv1alpha1.ExternalAuthTypeBearerToken && + externalAuthConfig.Spec.BearerToken != nil && + externalAuthConfig.Spec.BearerToken.TokenSecretRef != nil { + var secret corev1.Secret + if err := r.Get(ctx, types.NamespacedName{ + Namespace: externalAuthConfig.Namespace, + Name: externalAuthConfig.Spec.BearerToken.TokenSecretRef.Name, + }, &secret); err != nil { + if errors.IsNotFound(err) { + // Secret doesn't exist yet, include that in hash + hashString += ":secret-not-found" + } else { + return "", fmt.Errorf("failed to get bearer token secret: %w", err) + } + } else { + // Include the secret value in the hash + if tokenValue, ok := secret.Data[externalAuthConfig.Spec.BearerToken.TokenSecretRef.Key]; ok { + hasher := sha256.New() + hasher.Write(tokenValue) + hashString += ":" + hex.EncodeToString(hasher.Sum(nil))[:16] + } else { + hashString += ":key-not-found" + } + } + } + + // Also include token exchange client secret if present + if externalAuthConfig.Spec.Type == mcpv1alpha1.ExternalAuthTypeTokenExchange && + externalAuthConfig.Spec.TokenExchange != nil && + externalAuthConfig.Spec.TokenExchange.ClientSecretRef != nil { + var secret corev1.Secret + if err := r.Get(ctx, types.NamespacedName{ + Namespace: externalAuthConfig.Namespace, + Name: externalAuthConfig.Spec.TokenExchange.ClientSecretRef.Name, + }, &secret); err != nil { + if errors.IsNotFound(err) { + hashString += ":client-secret-not-found" + } else { + return "", fmt.Errorf("failed to get client secret: %w", err) + } + } else { + if secretValue, ok := secret.Data[externalAuthConfig.Spec.TokenExchange.ClientSecretRef.Key]; ok { + hasher := sha256.New() + hasher.Write(secretValue) + hashString += ":" + hex.EncodeToString(hasher.Sum(nil))[:16] + } else { + hashString += ":client-secret-key-not-found" + } + } + } + + // Hash the final combined string + hasher := sha256.New() + hasher.Write([]byte(hashString)) + return hex.EncodeToString(hasher.Sum(nil))[:16], nil } // handleDeletion handles the deletion of a MCPExternalAuthConfig @@ -197,6 +267,54 @@ func (r *MCPExternalAuthConfigReconciler) findReferencingMCPServers( }) } +// findMCPExternalAuthConfigsReferencingSecret finds all MCPExternalAuthConfigs that reference the given Secret. +// This includes configs that reference the Secret for bearer tokens or token exchange client secrets. +func (r *MCPExternalAuthConfigReconciler) findMCPExternalAuthConfigsReferencingSecret( + ctx context.Context, + secret *corev1.Secret, +) ([]mcpv1alpha1.MCPExternalAuthConfig, error) { + // List all MCPExternalAuthConfigs in the same namespace as the Secret + externalAuthConfigs := &mcpv1alpha1.MCPExternalAuthConfigList{} + if err := r.List(ctx, externalAuthConfigs, client.InNamespace(secret.Namespace)); err != nil { + return nil, fmt.Errorf("failed to list MCPExternalAuthConfigs: %w", err) + } + + // Filter configs that reference this Secret + referencingConfigs := make([]mcpv1alpha1.MCPExternalAuthConfig, 0) + for _, config := range externalAuthConfigs.Items { + if configReferencesSecret(&config, secret.Name) { + referencingConfigs = append(referencingConfigs, config) + } + } + + return referencingConfigs, nil +} + +// configReferencesSecret checks if an MCPExternalAuthConfig references a Secret by name. +// This checks both bearer token and token exchange configurations. +func configReferencesSecret( + config *mcpv1alpha1.MCPExternalAuthConfig, + secretName string, +) bool { + // Check bearer token config + if config.Spec.Type == mcpv1alpha1.ExternalAuthTypeBearerToken && + config.Spec.BearerToken != nil && + config.Spec.BearerToken.TokenSecretRef != nil && + config.Spec.BearerToken.TokenSecretRef.Name == secretName { + return true + } + + // Check token exchange config + if config.Spec.Type == mcpv1alpha1.ExternalAuthTypeTokenExchange && + config.Spec.TokenExchange != nil && + config.Spec.TokenExchange.ClientSecretRef != nil && + config.Spec.TokenExchange.ClientSecretRef.Name == secretName { + return true + } + + return false +} + // SetupWithManager sets up the controller with the Manager. func (r *MCPExternalAuthConfigReconciler) SetupWithManager(mgr ctrl.Manager) error { // Create a handler that maps MCPExternalAuthConfig changes to MCPServer reconciliation requests @@ -229,10 +347,41 @@ func (r *MCPExternalAuthConfigReconciler) SetupWithManager(mgr ctrl.Manager) err }, ) + // Create a handler that maps Secret changes to MCPExternalAuthConfig reconciliation requests + secretHandler := handler.EnqueueRequestsFromMapFunc( + func(ctx context.Context, obj client.Object) []reconcile.Request { + secret, ok := obj.(*corev1.Secret) + if !ok { + return nil + } + + // Find all MCPExternalAuthConfigs that reference this Secret + configs, err := r.findMCPExternalAuthConfigsReferencingSecret(ctx, secret) + if err != nil { + log.FromContext(ctx).Error(err, "Failed to find MCPExternalAuthConfigs referencing Secret") + return nil + } + + requests := make([]reconcile.Request, 0, len(configs)) + for _, config := range configs { + requests = append(requests, reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: config.Name, + Namespace: config.Namespace, + }, + }) + } + + return requests + }, + ) + return ctrl.NewControllerManagedBy(mgr). For(&mcpv1alpha1.MCPExternalAuthConfig{}). // Watch for MCPServers and reconcile the MCPExternalAuthConfig when they change Watches(&mcpv1alpha1.MCPServer{}, externalAuthConfigHandler). + // Watch for Secrets and reconcile MCPExternalAuthConfigs that reference them + Watches(&corev1.Secret{}, secretHandler). Complete(r) } diff --git a/cmd/thv-operator/controllers/mcpexternalauthconfig_controller_test.go b/cmd/thv-operator/controllers/mcpexternalauthconfig_controller_test.go index e090cc5bde..acaead5aee 100644 --- a/cmd/thv-operator/controllers/mcpexternalauthconfig_controller_test.go +++ b/cmd/thv-operator/controllers/mcpexternalauthconfig_controller_test.go @@ -23,45 +23,84 @@ import ( func TestMCPExternalAuthConfigReconciler_calculateConfigHash(t *testing.T) { t.Parallel() + ctx := t.Context() + tests := []struct { - name string - spec mcpv1alpha1.MCPExternalAuthConfigSpec + name string + externalAuthConfig *mcpv1alpha1.MCPExternalAuthConfig + secrets []*corev1.Secret }{ { name: "empty spec", - spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ - Type: mcpv1alpha1.ExternalAuthTypeTokenExchange, + externalAuthConfig: &mcpv1alpha1.MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-config", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ + Type: mcpv1alpha1.ExternalAuthTypeTokenExchange, + }, }, }, { name: "with token exchange config", - spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ - Type: mcpv1alpha1.ExternalAuthTypeTokenExchange, - TokenExchange: &mcpv1alpha1.TokenExchangeConfig{ - TokenURL: "https://oauth.example.com/token", - ClientID: "test-client-id", - ClientSecretRef: &mcpv1alpha1.SecretKeyRef{ - Name: "test-secret", - Key: "client-secret", + externalAuthConfig: &mcpv1alpha1.MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-config", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ + Type: mcpv1alpha1.ExternalAuthTypeTokenExchange, + TokenExchange: &mcpv1alpha1.TokenExchangeConfig{ + TokenURL: "https://oauth.example.com/token", + ClientID: "test-client-id", + ClientSecretRef: &mcpv1alpha1.SecretKeyRef{ + Name: "test-secret", + Key: "client-secret", + }, + Audience: "backend-service", + Scopes: []string{"read", "write"}, + }, + }, + }, + secrets: []*corev1.Secret{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "test-secret", + Namespace: "default", + }, + Data: map[string][]byte{ + "client-secret": []byte("secret-value"), }, - Audience: "backend-service", - Scopes: []string{"read", "write"}, }, }, }, { - name: "with custom header", - spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ - Type: mcpv1alpha1.ExternalAuthTypeTokenExchange, - TokenExchange: &mcpv1alpha1.TokenExchangeConfig{ - TokenURL: "https://oauth.example.com/token", - ClientID: "test-client-id", - ClientSecretRef: &mcpv1alpha1.SecretKeyRef{ - Name: "test-secret", - Key: "client-secret", + name: "with bearer token config", + externalAuthConfig: &mcpv1alpha1.MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-config", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ + Type: mcpv1alpha1.ExternalAuthTypeBearerToken, + BearerToken: &mcpv1alpha1.BearerTokenConfig{ + TokenSecretRef: &mcpv1alpha1.SecretKeyRef{ + Name: "bearer-secret", + Key: "token", + }, + }, + }, + }, + secrets: []*corev1.Secret{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "bearer-secret", + Namespace: "default", + }, + Data: map[string][]byte{ + "token": []byte("bearer-token-value"), }, - Audience: "backend-service", - ExternalTokenHeaderName: "X-Upstream-Token", }, }, }, @@ -71,10 +110,30 @@ func TestMCPExternalAuthConfigReconciler_calculateConfigHash(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - r := &MCPExternalAuthConfigReconciler{} + scheme := runtime.NewScheme() + require.NoError(t, mcpv1alpha1.AddToScheme(scheme)) + require.NoError(t, corev1.AddToScheme(scheme)) + + objs := []client.Object{tt.externalAuthConfig} + for _, secret := range tt.secrets { + objs = append(objs, secret) + } - hash1 := r.calculateConfigHash(tt.spec) - hash2 := r.calculateConfigHash(tt.spec) + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(objs...). + Build() + + r := &MCPExternalAuthConfigReconciler{ + Client: fakeClient, + Scheme: scheme, + } + + hash1, err1 := r.calculateConfigHash(ctx, tt.externalAuthConfig) + require.NoError(t, err1) + + hash2, err2 := r.calculateConfigHash(ctx, tt.externalAuthConfig) + require.NoError(t, err2) // Same spec should produce same hash assert.Equal(t, hash1, hash2, "Hash should be consistent for same spec") @@ -85,34 +144,82 @@ func TestMCPExternalAuthConfigReconciler_calculateConfigHash(t *testing.T) { // Different specs should produce different hashes t.Run("different specs produce different hashes", func(t *testing.T) { t.Parallel() - r := &MCPExternalAuthConfigReconciler{} - spec1 := mcpv1alpha1.MCPExternalAuthConfigSpec{ - Type: mcpv1alpha1.ExternalAuthTypeTokenExchange, - TokenExchange: &mcpv1alpha1.TokenExchangeConfig{ - TokenURL: "https://oauth.example.com/token", - ClientID: "client1", - ClientSecretRef: &mcpv1alpha1.SecretKeyRef{ - Name: "secret1", - Key: "key1", + + scheme := runtime.NewScheme() + require.NoError(t, mcpv1alpha1.AddToScheme(scheme)) + require.NoError(t, corev1.AddToScheme(scheme)) + + config1 := &mcpv1alpha1.MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "config1", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ + Type: mcpv1alpha1.ExternalAuthTypeTokenExchange, + TokenExchange: &mcpv1alpha1.TokenExchangeConfig{ + TokenURL: "https://oauth.example.com/token", + ClientID: "client1", + ClientSecretRef: &mcpv1alpha1.SecretKeyRef{ + Name: "secret1", + Key: "key1", + }, + Audience: "audience1", }, - Audience: "audience1", }, } - spec2 := mcpv1alpha1.MCPExternalAuthConfigSpec{ - Type: mcpv1alpha1.ExternalAuthTypeTokenExchange, - TokenExchange: &mcpv1alpha1.TokenExchangeConfig{ - TokenURL: "https://oauth.example.com/token", - ClientID: "client2", - ClientSecretRef: &mcpv1alpha1.SecretKeyRef{ - Name: "secret2", - Key: "key2", + config2 := &mcpv1alpha1.MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "config2", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ + Type: mcpv1alpha1.ExternalAuthTypeTokenExchange, + TokenExchange: &mcpv1alpha1.TokenExchangeConfig{ + TokenURL: "https://oauth.example.com/token", + ClientID: "client2", + ClientSecretRef: &mcpv1alpha1.SecretKeyRef{ + Name: "secret2", + Key: "key2", + }, + Audience: "audience2", }, - Audience: "audience2", }, } - hash1 := r.calculateConfigHash(spec1) - hash2 := r.calculateConfigHash(spec2) + secret1 := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "secret1", + Namespace: "default", + }, + Data: map[string][]byte{ + "key1": []byte("value1"), + }, + } + secret2 := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "secret2", + Namespace: "default", + }, + Data: map[string][]byte{ + "key2": []byte("value2"), + }, + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(config1, config2, secret1, secret2). + Build() + + r := &MCPExternalAuthConfigReconciler{ + Client: fakeClient, + Scheme: scheme, + } + + hash1, err1 := r.calculateConfigHash(ctx, config1) + require.NoError(t, err1) + + hash2, err2 := r.calculateConfigHash(ctx, config2) + require.NoError(t, err2) assert.NotEqual(t, hash1, hash2, "Different specs should produce different hashes") }) diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_deployment.go b/cmd/thv-operator/controllers/mcpremoteproxy_deployment.go index 55ce2d6630..07cefb362e 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_deployment.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_deployment.go @@ -152,6 +152,21 @@ func (r *MCPRemoteProxyReconciler) buildEnvVarsForProxy( } else { env = append(env, tokenExchangeEnvVars...) } + + // Add bearer token environment variables + bearerTokenEnvVars, err := ctrlutil.GenerateBearerTokenEnvVar( + ctx, + r.Client, + proxy.Namespace, + proxy.Spec.ExternalAuthConfigRef, + ctrlutil.GetExternalAuthConfigByName, + ) + if err != nil { + ctxLogger := log.FromContext(ctx) + ctxLogger.Error(err, "Failed to generate bearer token environment variables") + } else { + env = append(env, bearerTokenEnvVars...) + } } // Add OIDC client secret environment variable if using inline config with secretRef diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_deployment_test.go b/cmd/thv-operator/controllers/mcpremoteproxy_deployment_test.go index 5d954db84a..bea461c136 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_deployment_test.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_deployment_test.go @@ -640,6 +640,67 @@ func TestBuildEnvVarsForProxy(t *testing.T) { assert.True(t, found, "Token exchange secret should be referenced") }, }, + { + name: "with bearer token", + proxy: &mcpv1alpha1.MCPRemoteProxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bearer-proxy", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPRemoteProxySpec{ + RemoteURL: "https://mcp.example.com", + OIDCConfig: mcpv1alpha1.OIDCConfigRef{ + Type: mcpv1alpha1.OIDCConfigTypeInline, + Inline: &mcpv1alpha1.InlineOIDCConfig{ + Issuer: "https://auth.example.com", + Audience: "mcp-proxy", + }, + }, + ExternalAuthConfigRef: &mcpv1alpha1.ExternalAuthConfigRef{ + Name: "bearer-config", + }, + }, + }, + externalAuth: &mcpv1alpha1.MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bearer-config", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ + Type: mcpv1alpha1.ExternalAuthTypeBearerToken, + BearerToken: &mcpv1alpha1.BearerTokenConfig{ + TokenSecretRef: &mcpv1alpha1.SecretKeyRef{ + Name: "bearer-secret", + Key: "token", + }, + }, + }, + }, + clientSecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bearer-secret", + Namespace: "default", + }, + Data: map[string][]byte{ + "token": []byte("my-bearer-token"), + }, + }, + validate: func(t *testing.T, envVars []corev1.EnvVar) { + t.Helper() + found := false + for _, env := range envVars { + if env.Name == "TOOLHIVE_SECRET_bearer-secret" { + require.NotNil(t, env.ValueFrom) + require.NotNil(t, env.ValueFrom.SecretKeyRef) + assert.Equal(t, "bearer-secret", env.ValueFrom.SecretKeyRef.Name) + assert.Equal(t, "token", env.ValueFrom.SecretKeyRef.Key) + found = true + break + } + } + assert.True(t, found, "Bearer token secret should be referenced as TOOLHIVE_SECRET_bearer-secret") + }, + }, } for _, tt := range tests { diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_runconfig_test.go b/cmd/thv-operator/controllers/mcpremoteproxy_runconfig_test.go index f45982c235..ae5bc18cc8 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_runconfig_test.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_runconfig_test.go @@ -384,6 +384,239 @@ func TestCreateRunConfigFromMCPRemoteProxy_WithTokenExchange(t *testing.T) { } } +// TestCreateRunConfigFromMCPRemoteProxy_WithBearerToken tests RunConfig generation with bearer token +func TestCreateRunConfigFromMCPRemoteProxy_WithBearerToken(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + proxy *mcpv1alpha1.MCPRemoteProxy + externalAuth *mcpv1alpha1.MCPExternalAuthConfig + bearerSecret *corev1.Secret + expectError bool + validate func(*testing.T, *runner.RunConfig) + }{ + { + name: "with bearer token", + proxy: &mcpv1alpha1.MCPRemoteProxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bearer-proxy", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPRemoteProxySpec{ + RemoteURL: "https://mcp.example.com/api", + Port: 8080, + OIDCConfig: mcpv1alpha1.OIDCConfigRef{ + Type: mcpv1alpha1.OIDCConfigTypeInline, + Inline: &mcpv1alpha1.InlineOIDCConfig{ + Issuer: "https://auth.example.com", + Audience: "mcp-proxy", + }, + }, + ExternalAuthConfigRef: &mcpv1alpha1.ExternalAuthConfigRef{ + Name: "api-bearer-auth", + }, + }, + }, + externalAuth: &mcpv1alpha1.MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "api-bearer-auth", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ + Type: mcpv1alpha1.ExternalAuthTypeBearerToken, + BearerToken: &mcpv1alpha1.BearerTokenConfig{ + TokenSecretRef: &mcpv1alpha1.SecretKeyRef{ + Name: "api-bearer-token", + Key: "token", + }, + }, + }, + }, + bearerSecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "api-bearer-token", + Namespace: "default", + }, + Data: map[string][]byte{ + "token": []byte("my-bearer-token-123"), + }, + }, + expectError: false, + validate: func(t *testing.T, config *runner.RunConfig) { + t.Helper() + assert.Equal(t, "bearer-proxy", config.Name) + assert.Equal(t, "https://mcp.example.com/api", config.RemoteURL) + + // Verify RemoteAuthConfig has bearer token in CLI format + require.NotNil(t, config.RemoteAuthConfig) + assert.Equal(t, "api-bearer-token,target=bearer_token", config.RemoteAuthConfig.BearerToken) + }, + }, + { + name: "missing TokenSecretRef", + proxy: &mcpv1alpha1.MCPRemoteProxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "broken-proxy", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPRemoteProxySpec{ + RemoteURL: "https://mcp.example.com", + Port: 8080, + OIDCConfig: mcpv1alpha1.OIDCConfigRef{ + Type: mcpv1alpha1.OIDCConfigTypeInline, + Inline: &mcpv1alpha1.InlineOIDCConfig{ + Issuer: "https://auth.example.com", + Audience: "mcp-proxy", + }, + }, + ExternalAuthConfigRef: &mcpv1alpha1.ExternalAuthConfigRef{ + Name: "broken-bearer", + }, + }, + }, + externalAuth: &mcpv1alpha1.MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "broken-bearer", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ + Type: mcpv1alpha1.ExternalAuthTypeBearerToken, + BearerToken: &mcpv1alpha1.BearerTokenConfig{ + TokenSecretRef: nil, // Missing TokenSecretRef + }, + }, + }, + expectError: true, + }, + { + name: "secret not found", + proxy: &mcpv1alpha1.MCPRemoteProxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "missing-secret-proxy", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPRemoteProxySpec{ + RemoteURL: "https://mcp.example.com", + Port: 8080, + OIDCConfig: mcpv1alpha1.OIDCConfigRef{ + Type: mcpv1alpha1.OIDCConfigTypeInline, + Inline: &mcpv1alpha1.InlineOIDCConfig{ + Issuer: "https://auth.example.com", + Audience: "mcp-proxy", + }, + }, + ExternalAuthConfigRef: &mcpv1alpha1.ExternalAuthConfigRef{ + Name: "missing-secret-bearer", + }, + }, + }, + externalAuth: &mcpv1alpha1.MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "missing-secret-bearer", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ + Type: mcpv1alpha1.ExternalAuthTypeBearerToken, + BearerToken: &mcpv1alpha1.BearerTokenConfig{ + TokenSecretRef: &mcpv1alpha1.SecretKeyRef{ + Name: "non-existent-secret", + Key: "token", + }, + }, + }, + }, + expectError: true, + }, + { + name: "secret missing key", + proxy: &mcpv1alpha1.MCPRemoteProxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "missing-key-proxy", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPRemoteProxySpec{ + RemoteURL: "https://mcp.example.com", + Port: 8080, + OIDCConfig: mcpv1alpha1.OIDCConfigRef{ + Type: mcpv1alpha1.OIDCConfigTypeInline, + Inline: &mcpv1alpha1.InlineOIDCConfig{ + Issuer: "https://auth.example.com", + Audience: "mcp-proxy", + }, + }, + ExternalAuthConfigRef: &mcpv1alpha1.ExternalAuthConfigRef{ + Name: "missing-key-bearer", + }, + }, + }, + externalAuth: &mcpv1alpha1.MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "missing-key-bearer", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ + Type: mcpv1alpha1.ExternalAuthTypeBearerToken, + BearerToken: &mcpv1alpha1.BearerTokenConfig{ + TokenSecretRef: &mcpv1alpha1.SecretKeyRef{ + Name: "incomplete-secret", + Key: "token", + }, + }, + }, + }, + bearerSecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "incomplete-secret", + Namespace: "default", + }, + Data: map[string][]byte{ + "other-key": []byte("value"), + // Missing "token" key + }, + }, + expectError: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + scheme := createRunConfigTestScheme() + objects := []runtime.Object{tt.proxy} + if tt.externalAuth != nil { + objects = append(objects, tt.externalAuth) + } + if tt.bearerSecret != nil { + objects = append(objects, tt.bearerSecret) + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithRuntimeObjects(objects...). + Build() + + reconciler := &MCPRemoteProxyReconciler{ + Client: fakeClient, + Scheme: scheme, + } + + runConfig, err := reconciler.createRunConfigFromMCPRemoteProxy(tt.proxy) + + if tt.expectError { + assert.Error(t, err) + } else { + require.NoError(t, err) + assert.NotNil(t, runConfig) + if tt.validate != nil { + tt.validate(t, runConfig) + } + } + }) + } +} + // TestValidateRunConfigForRemoteProxy tests the validation logic for remote proxy RunConfigs func TestValidateRunConfigForRemoteProxy(t *testing.T) { t.Parallel() diff --git a/cmd/thv-operator/pkg/controllerutil/resources.go b/cmd/thv-operator/pkg/controllerutil/resources.go index 68ec275d66..5fd165a00e 100644 --- a/cmd/thv-operator/pkg/controllerutil/resources.go +++ b/cmd/thv-operator/pkg/controllerutil/resources.go @@ -6,6 +6,7 @@ package controllerutil import ( "context" "fmt" + "strings" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" @@ -13,6 +14,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log" mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" + "github.com/stacklok/toolhive/pkg/secrets" ) // BuildResourceRequirements builds Kubernetes resource requirements from CRD spec @@ -70,6 +72,7 @@ func EnsureRequiredEnvVars(ctx context.Context, env []corev1.EnvVar) []corev1.En homeFound := false toolhiveRuntimeFound := false unstructuredLogsFound := false + hasSecrets := false for _, envVar := range env { switch envVar.Name { @@ -82,6 +85,10 @@ func EnsureRequiredEnvVars(ctx context.Context, env []corev1.EnvVar) []corev1.En case "UNSTRUCTURED_LOGS": unstructuredLogsFound = true } + // Check if this is a TOOLHIVE_SECRET_* env var (but not TOOLHIVE_SECRETS_PROVIDER itself) + if strings.HasPrefix(envVar.Name, secrets.EnvVarPrefix) && envVar.Name != secrets.ProviderEnvVar { + hasSecrets = true + } } if !xdgConfigHomeFound { @@ -117,6 +124,21 @@ func EnsureRequiredEnvVars(ctx context.Context, env []corev1.EnvVar) []corev1.En }) } + // Set secrets provider to environment if secrets are being used via TOOLHIVE_SECRET_* env vars + // This is needed to resolve CLI format secrets (e.g., "secret-name,target=bearer_token") + // The environment provider reads from TOOLHIVE_SECRET_* env vars to resolve CLI format secrets + // + // If TOOLHIVE_SECRETS_PROVIDER is already set to something other than "environment", + // we override it because TOOLHIVE_SECRET_* env vars REQUIRE the environment provider. + // Other providers (encrypted, 1password) cannot read from TOOLHIVE_SECRET_* env vars. + if hasSecrets { + ctxLogger.V(1).Info("TOOLHIVE_SECRET_* env vars found, setting TOOLHIVE_SECRETS_PROVIDER to environment") + env = append(env, corev1.EnvVar{ + Name: secrets.ProviderEnvVar, + Value: string(secrets.EnvironmentType), + }) + } + return env } diff --git a/cmd/thv-operator/pkg/controllerutil/resources_test.go b/cmd/thv-operator/pkg/controllerutil/resources_test.go new file mode 100644 index 0000000000..4f0da39419 --- /dev/null +++ b/cmd/thv-operator/pkg/controllerutil/resources_test.go @@ -0,0 +1,304 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package controllerutil + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" + + "github.com/stacklok/toolhive/pkg/secrets" +) + +func TestEnsureRequiredEnvVars(t *testing.T) { + t.Parallel() + + ctx := context.Background() + + t.Run("sets all default env vars when missing", func(t *testing.T) { + t.Parallel() + + env := []corev1.EnvVar{} + result := EnsureRequiredEnvVars(ctx, env) + + envMap := make(map[string]string) + for _, e := range result { + envMap[e.Name] = e.Value + } + + assert.Equal(t, "/tmp", envMap["XDG_CONFIG_HOME"]) + assert.Equal(t, "/tmp", envMap["HOME"]) + assert.Equal(t, "kubernetes", envMap["TOOLHIVE_RUNTIME"]) + assert.Equal(t, "false", envMap["UNSTRUCTURED_LOGS"]) + assert.Len(t, result, 4) + }) + + t.Run("does not override existing env vars", func(t *testing.T) { + t.Parallel() + + env := []corev1.EnvVar{ + {Name: "XDG_CONFIG_HOME", Value: "/custom/path"}, + {Name: "HOME", Value: "/home/user"}, + {Name: "TOOLHIVE_RUNTIME", Value: "docker"}, + {Name: "UNSTRUCTURED_LOGS", Value: "true"}, + } + result := EnsureRequiredEnvVars(ctx, env) + + envMap := make(map[string]string) + for _, e := range result { + envMap[e.Name] = e.Value + } + + assert.Equal(t, "/custom/path", envMap["XDG_CONFIG_HOME"]) + assert.Equal(t, "/home/user", envMap["HOME"]) + assert.Equal(t, "docker", envMap["TOOLHIVE_RUNTIME"]) + assert.Equal(t, "true", envMap["UNSTRUCTURED_LOGS"]) + assert.Len(t, result, 4) + }) + + t.Run("sets TOOLHIVE_SECRETS_PROVIDER when TOOLHIVE_SECRET_* vars are present", func(t *testing.T) { + t.Parallel() + + env := []corev1.EnvVar{ + {Name: "TOOLHIVE_SECRET_api-bearer-token", Value: "token-value"}, + } + result := EnsureRequiredEnvVars(ctx, env) + + envMap := make(map[string]string) + for _, e := range result { + envMap[e.Name] = e.Value + } + + assert.Equal(t, string(secrets.EnvironmentType), envMap[secrets.ProviderEnvVar]) + assert.Contains(t, result, corev1.EnvVar{ + Name: secrets.ProviderEnvVar, + Value: string(secrets.EnvironmentType), + }) + // Should also have all default env vars + assert.Equal(t, "/tmp", envMap["XDG_CONFIG_HOME"]) + assert.Equal(t, "/tmp", envMap["HOME"]) + assert.Equal(t, "kubernetes", envMap["TOOLHIVE_RUNTIME"]) + assert.Equal(t, "false", envMap["UNSTRUCTURED_LOGS"]) + }) + + t.Run("does not set TOOLHIVE_SECRETS_PROVIDER when no secrets are present", func(t *testing.T) { + t.Parallel() + + env := []corev1.EnvVar{ + {Name: "SOME_OTHER_VAR", Value: "value"}, + } + result := EnsureRequiredEnvVars(ctx, env) + + envMap := make(map[string]string) + for _, e := range result { + envMap[e.Name] = e.Value + } + + _, found := envMap[secrets.ProviderEnvVar] + assert.False(t, found, "TOOLHIVE_SECRETS_PROVIDER should not be set when no secrets are present") + }) + + t.Run("sets TOOLHIVE_SECRETS_PROVIDER with multiple secret env vars", func(t *testing.T) { + t.Parallel() + + env := []corev1.EnvVar{ + {Name: "TOOLHIVE_SECRET_token1", Value: "value1"}, + {Name: "TOOLHIVE_SECRET_token2", Value: "value2"}, + } + result := EnsureRequiredEnvVars(ctx, env) + + envMap := make(map[string]string) + for _, e := range result { + envMap[e.Name] = e.Value + } + + assert.Equal(t, string(secrets.EnvironmentType), envMap[secrets.ProviderEnvVar]) + }) + + t.Run("does not treat TOOLHIVE_SECRETS_PROVIDER itself as a secret", func(t *testing.T) { + t.Parallel() + + env := []corev1.EnvVar{ + {Name: secrets.ProviderEnvVar, Value: "encrypted"}, + } + result := EnsureRequiredEnvVars(ctx, env) + + envMap := make(map[string]string) + for _, e := range result { + envMap[e.Name] = e.Value + } + + // Should not set TOOLHIVE_SECRETS_PROVIDER because only the provider itself is present, no actual secrets + // The current implementation will append a new one if secrets are found, but since we only have the provider var, + // no secrets are detected, so it should not be set + _, found := envMap[secrets.ProviderEnvVar] + assert.True(t, found, "TOOLHIVE_SECRETS_PROVIDER should be preserved") + assert.Equal(t, "encrypted", envMap[secrets.ProviderEnvVar]) + }) + + t.Run("appends TOOLHIVE_SECRETS_PROVIDER when provider is set but secrets are also present", func(t *testing.T) { + t.Parallel() + + // When TOOLHIVE_SECRETS_PROVIDER is set to something other than "environment" but secrets are present, + // the current implementation will append a new one (creating a duplicate) + env := []corev1.EnvVar{ + {Name: secrets.ProviderEnvVar, Value: "encrypted"}, + {Name: "TOOLHIVE_SECRET_api-key", Value: "key-value"}, + } + result := EnsureRequiredEnvVars(ctx, env) + + envMap := make(map[string]string) + providerCount := 0 + for _, e := range result { + if e.Name == secrets.ProviderEnvVar { + providerCount++ + envMap[e.Name] = e.Value + } else { + envMap[e.Name] = e.Value + } + } + + // Current implementation appends, so we'll have both values + // The last one appended will be "environment" + assert.GreaterOrEqual(t, providerCount, 1, "Should have at least one provider env var") + // The appended one should be "environment" + providerVars := []corev1.EnvVar{} + for _, e := range result { + if e.Name == secrets.ProviderEnvVar { + providerVars = append(providerVars, e) + } + } + // Check that "environment" is in the list + hasEnvironment := false + for _, pv := range providerVars { + if pv.Value == string(secrets.EnvironmentType) { + hasEnvironment = true + break + } + } + assert.True(t, hasEnvironment, "Should have environment provider set") + }) + + t.Run("handles empty env list", func(t *testing.T) { + t.Parallel() + + env := []corev1.EnvVar{} + result := EnsureRequiredEnvVars(ctx, env) + + assert.Len(t, result, 4) // All defaults should be set + envMap := make(map[string]string) + for _, e := range result { + envMap[e.Name] = e.Value + } + + assert.Equal(t, "/tmp", envMap["XDG_CONFIG_HOME"]) + assert.Equal(t, "/tmp", envMap["HOME"]) + assert.Equal(t, "kubernetes", envMap["TOOLHIVE_RUNTIME"]) + assert.Equal(t, "false", envMap["UNSTRUCTURED_LOGS"]) + }) + + t.Run("preserves existing env vars when adding defaults", func(t *testing.T) { + t.Parallel() + + env := []corev1.EnvVar{ + {Name: "CUSTOM_VAR", Value: "custom-value"}, + } + result := EnsureRequiredEnvVars(ctx, env) + + envMap := make(map[string]string) + for _, e := range result { + envMap[e.Name] = e.Value + } + + assert.Equal(t, "custom-value", envMap["CUSTOM_VAR"]) + assert.Equal(t, "/tmp", envMap["XDG_CONFIG_HOME"]) + assert.Equal(t, "/tmp", envMap["HOME"]) + assert.Equal(t, "kubernetes", envMap["TOOLHIVE_RUNTIME"]) + assert.Equal(t, "false", envMap["UNSTRUCTURED_LOGS"]) + }) + + t.Run("sets TOOLHIVE_SECRETS_PROVIDER when secret env var is present regardless of other vars", func(t *testing.T) { + t.Parallel() + + // The current implementation checks for secrets outside the switch, so it works regardless + env := []corev1.EnvVar{ + {Name: "TOOLHIVE_SECRET_my-secret", Value: "secret-value"}, + } + result := EnsureRequiredEnvVars(ctx, env) + + envMap := make(map[string]string) + for _, e := range result { + envMap[e.Name] = e.Value + } + + assert.Equal(t, string(secrets.EnvironmentType), envMap[secrets.ProviderEnvVar]) + }) + + t.Run("sets all defaults and provider when secrets are present", func(t *testing.T) { + t.Parallel() + + env := []corev1.EnvVar{ + {Name: "TOOLHIVE_SECRET_api-key", Value: "key-value"}, + } + result := EnsureRequiredEnvVars(ctx, env) + + envMap := make(map[string]string) + for _, e := range result { + envMap[e.Name] = e.Value + } + + // Should have all defaults plus the provider + assert.Equal(t, "/tmp", envMap["XDG_CONFIG_HOME"]) + assert.Equal(t, "/tmp", envMap["HOME"]) + assert.Equal(t, "kubernetes", envMap["TOOLHIVE_RUNTIME"]) + assert.Equal(t, "false", envMap["UNSTRUCTURED_LOGS"]) + assert.Equal(t, string(secrets.EnvironmentType), envMap[secrets.ProviderEnvVar]) + assert.Equal(t, "key-value", envMap["TOOLHIVE_SECRET_api-key"]) + assert.Len(t, result, 6) // 1 original secret + 4 defaults + 1 provider + }) + + t.Run("handles secret env var with hyphens in name", func(t *testing.T) { + t.Parallel() + + env := []corev1.EnvVar{ + {Name: "TOOLHIVE_SECRET_api-bearer-token", Value: "bearer-token-value-123"}, + } + result := EnsureRequiredEnvVars(ctx, env) + + envMap := make(map[string]string) + for _, e := range result { + envMap[e.Name] = e.Value + } + + assert.Equal(t, string(secrets.EnvironmentType), envMap[secrets.ProviderEnvVar]) + assert.Equal(t, "bearer-token-value-123", envMap["TOOLHIVE_SECRET_api-bearer-token"]) + }) + + t.Run("detects secrets correctly when mixed with other env vars", func(t *testing.T) { + t.Parallel() + + env := []corev1.EnvVar{ + {Name: "CUSTOM_VAR", Value: "custom"}, + {Name: "ANOTHER_VAR", Value: "another"}, + {Name: "TOOLHIVE_SECRET_token", Value: "secret-token"}, + {Name: "REGULAR_VAR", Value: "regular"}, + } + result := EnsureRequiredEnvVars(ctx, env) + + envMap := make(map[string]string) + for _, e := range result { + envMap[e.Name] = e.Value + } + + // Should detect the secret and set provider + assert.Equal(t, string(secrets.EnvironmentType), envMap[secrets.ProviderEnvVar]) + // Should preserve all original vars + assert.Equal(t, "custom", envMap["CUSTOM_VAR"]) + assert.Equal(t, "another", envMap["ANOTHER_VAR"]) + assert.Equal(t, "secret-token", envMap["TOOLHIVE_SECRET_token"]) + assert.Equal(t, "regular", envMap["REGULAR_VAR"]) + }) +} diff --git a/cmd/thv-operator/pkg/controllerutil/tokenexchange.go b/cmd/thv-operator/pkg/controllerutil/tokenexchange.go index 8bdba44f8b..f3c11540ac 100644 --- a/cmd/thv-operator/pkg/controllerutil/tokenexchange.go +++ b/cmd/thv-operator/pkg/controllerutil/tokenexchange.go @@ -12,6 +12,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" + "github.com/stacklok/toolhive/pkg/auth/remote" "github.com/stacklok/toolhive/pkg/auth/tokenexchange" "github.com/stacklok/toolhive/pkg/runner" ) @@ -120,7 +121,7 @@ func AddExternalAuthConfigOptions( case mcpv1alpha1.ExternalAuthTypeHeaderInjection: return addHeaderInjectionConfig(ctx, c, namespace, externalAuthConfig, options) case mcpv1alpha1.ExternalAuthTypeBearerToken: - return fmt.Errorf("bearerToken authentication is not yet implemented") + return addBearerTokenConfig(ctx, c, namespace, externalAuthConfig, options) case mcpv1alpha1.ExternalAuthTypeUnauthenticated: // No config to add for unauthenticated return nil @@ -206,3 +207,100 @@ func addHeaderInjectionConfig( // but header injection doesn't make sense in that context return nil } + +// addBearerTokenConfig adds bearer token configuration to runner options +func addBearerTokenConfig( + ctx context.Context, + c client.Client, + namespace string, + externalAuthConfig *mcpv1alpha1.MCPExternalAuthConfig, + options *[]runner.RunConfigBuilderOption, +) error { + bearerTokenSpec := externalAuthConfig.Spec.BearerToken + if bearerTokenSpec == nil { + return fmt.Errorf("bearer token configuration is nil for type bearerToken") + } + + if bearerTokenSpec.TokenSecretRef == nil { + return fmt.Errorf("bearer token configuration is missing TokenSecretRef") + } + + // Validate secret exists + var secret corev1.Secret + if err := c.Get(ctx, types.NamespacedName{ + Namespace: namespace, + Name: bearerTokenSpec.TokenSecretRef.Name, + }, &secret); err != nil { + return fmt.Errorf("failed to get bearer token secret %s/%s: %w", + namespace, bearerTokenSpec.TokenSecretRef.Name, err) + } + + // Validate key exists + if _, ok := secret.Data[bearerTokenSpec.TokenSecretRef.Key]; !ok { + return fmt.Errorf("bearer token secret %s/%s is missing key %q", + namespace, bearerTokenSpec.TokenSecretRef.Name, bearerTokenSpec.TokenSecretRef.Key) + } + + // Convert to CLI format: "secret-name,target=bearer_token" + // Note: The secret name in CLI format must match the Kubernetes Secret name + // This will be resolved by EnvironmentProvider looking for TOOLHIVE_SECRET_{secret-name} + cliFormat := fmt.Sprintf("%s,target=bearer_token", bearerTokenSpec.TokenSecretRef.Name) + + // Create remote auth config + remoteConfig := &remote.Config{ + BearerToken: cliFormat, + } + + *options = append(*options, runner.WithRemoteAuth(remoteConfig)) + return nil +} + +// GenerateBearerTokenEnvVar generates environment variables for bearer token authentication +func GenerateBearerTokenEnvVar( + ctx context.Context, + c client.Client, + namespace string, + externalAuthConfigRef *mcpv1alpha1.ExternalAuthConfigRef, + getExternalAuthConfig func(context.Context, client.Client, string, string) (*mcpv1alpha1.MCPExternalAuthConfig, error), +) ([]corev1.EnvVar, error) { + var envVars []corev1.EnvVar + + if externalAuthConfigRef == nil { + return envVars, nil + } + + externalAuthConfig, err := getExternalAuthConfig(ctx, c, namespace, externalAuthConfigRef.Name) + if err != nil { + return nil, fmt.Errorf("failed to get MCPExternalAuthConfig: %w", err) + } + + if externalAuthConfig == nil { + return nil, fmt.Errorf("MCPExternalAuthConfig %s not found", externalAuthConfigRef.Name) + } + + if externalAuthConfig.Spec.Type != mcpv1alpha1.ExternalAuthTypeBearerToken { + return envVars, nil + } + + bearerTokenSpec := externalAuthConfig.Spec.BearerToken + if bearerTokenSpec == nil || bearerTokenSpec.TokenSecretRef == nil { + return envVars, nil + } + + // Environment variable name: TOOLHIVE_SECRET_{secret-name} + envVarName := fmt.Sprintf("TOOLHIVE_SECRET_%s", bearerTokenSpec.TokenSecretRef.Name) + + envVars = append(envVars, corev1.EnvVar{ + Name: envVarName, + ValueFrom: &corev1.EnvVarSource{ + SecretKeyRef: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: bearerTokenSpec.TokenSecretRef.Name, + }, + Key: bearerTokenSpec.TokenSecretRef.Key, + }, + }, + }) + + return envVars, nil +} diff --git a/examples/operator/external-auth/mcpremoteproxy_with_bearer_token.yaml b/examples/operator/external-auth/mcpremoteproxy_with_bearer_token.yaml new file mode 100644 index 0000000000..f30549039d --- /dev/null +++ b/examples/operator/external-auth/mcpremoteproxy_with_bearer_token.yaml @@ -0,0 +1,60 @@ +# Example: MCPRemoteProxy with Bearer Token Authentication +# This example demonstrates how to configure bearer token authentication +# for a remote MCP server + +--- +# Secret containing the bearer token for authenticating with the remote +# MCP server +apiVersion: v1 +kind: Secret +metadata: + name: api-bearer-token + namespace: default +type: Opaque +stringData: + token: your-bearer-token-here + +--- +# External authentication configuration that references the bearer token secret +apiVersion: toolhive.stacklok.dev/v1alpha1 +kind: MCPExternalAuthConfig +metadata: + name: api-bearer-auth + namespace: default +spec: + type: bearerToken + bearerToken: + tokenSecretRef: + name: api-bearer-token + key: token + +--- +# MCPRemoteProxy that uses bearer token authentication for outgoing requests +apiVersion: toolhive.stacklok.dev/v1alpha1 +kind: MCPRemoteProxy +metadata: + name: api-proxy + namespace: default +spec: + remoteURL: "https://mcp.example.com/api" + port: 8080 + transport: streamable-http + + # OIDC configuration for incoming authentication (validates tokens from clients) + oidcConfig: + type: inline + inline: + issuer: "https://auth.example.com" + audience: "mcp-api" + + # Reference to external auth configuration (bearer token) + externalAuthConfigRef: + name: api-bearer-auth + + resources: + limits: + cpu: "200m" + memory: "256Mi" + requests: + cpu: "100m" + memory: "128Mi" diff --git a/pkg/config/config.go b/pkg/config/config.go index 9445ea0ca1..d0c4907c05 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -79,15 +79,54 @@ func (s *Secrets) GetProviderType() (secrets.ProviderType, error) { // GetProviderTypeWithEnv returns the secrets provider type using the provided environment reader. // This method allows for dependency injection of environment variable access for testing. +// +// Precedence order: +// 1. Environment variable (TOOLHIVE_SECRETS_PROVIDER) - takes highest precedence +// 2. Config file (requires SetupCompleted to be true) +// +// Special handling when SetupCompleted is false: +// - Only the "environment" provider can be set via env var when SetupCompleted is false +// - Other providers (encrypted, 1password) require setup and will return ErrSecretsNotSetup +// - This prevents confusing errors later when trying to create providers that need setup +// +// Why environment provider bypasses SetupCompleted: +// - In Kubernetes environments, pods don't have config files set up +// - The operator sets TOOLHIVE_SECRETS_PROVIDER=environment via env vars +// - The environment provider doesn't require "setup" - it reads directly from env vars +// - This allows the operator to work without requiring users to run 'thv secret setup' +// +// For CLI users: +// - If they set TOOLHIVE_SECRETS_PROVIDER=environment, it works without setup +// - If they set TOOLHIVE_SECRETS_PROVIDER=encrypted/1password without setup, it returns an error +// - This prevents confusing errors when providers fail to initialize later func (s *Secrets) GetProviderTypeWithEnv(envReader env.Reader) (secrets.ProviderType, error) { - // Check if secrets setup has been completed - if !s.SetupCompleted { - return "", secrets.ErrSecretsNotSetup + // First check the environment variable (takes precedence) + envVar := envReader.Getenv(secrets.ProviderEnvVar) + if envVar != "" { + providerType, err := validateProviderType(envVar) + if err != nil { + return "", err + } + + // Special case: Only allow "environment" provider when SetupCompleted is false + // Other providers (encrypted, 1password) require setup and will fail later when + // trying to create them (keyring, password, 1Password CLI, etc.) + if !s.SetupCompleted && providerType != secrets.EnvironmentType { + return "", fmt.Errorf( + "provider %q requires setup to be completed. "+ + "Only 'environment' provider can be used without setup. "+ + "Please run 'thv secret setup' or use TOOLHIVE_SECRETS_PROVIDER=environment", + providerType, + ) + } + + return providerType, nil } - // First check the environment variable - if envVar := envReader.Getenv(secrets.ProviderEnvVar); envVar != "" { - return validateProviderType(envVar) + // Check if secrets setup has been completed (required for config file-based provider) + // Only checked when environment variable is not set + if !s.SetupCompleted { + return "", secrets.ErrSecretsNotSetup } // Fall back to config file diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index abe99dada0..617622c395 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -348,7 +348,7 @@ func TestSecrets_GetProviderType_EnvironmentVariable(t *testing.T) { assert.Contains(t, err.Error(), "invalid secrets provider type", "Error should mention invalid provider type") }) - t.Run("Setup not completed returns error", func(t *testing.T) { + t.Run("Setup not completed returns error when env var is unset", func(t *testing.T) { t.Parallel() ctrl := gomock.NewController(t) defer ctrl.Finish() @@ -359,9 +359,84 @@ func TestSecrets_GetProviderType_EnvironmentVariable(t *testing.T) { SetupCompleted: false, } - // No expectation needed since the function returns early when SetupCompleted is false + // Explicitly set expectation: env var is unset (empty string) + mockEnv.EXPECT().Getenv(secrets.ProviderEnvVar).Return("") _, err := s.GetProviderTypeWithEnv(mockEnv) - assert.Error(t, err, "Should return error when setup not completed") + assert.Error(t, err, "Should return error when setup not completed and env var is unset") assert.ErrorIs(t, err, secrets.ErrSecretsNotSetup, "Should return ErrSecretsNotSetup when setup not completed") }) + + t.Run("Environment variable bypasses SetupCompleted check", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockEnv := mocks.NewMockReader(ctrl) + s := &Secrets{ + ProviderType: string(secrets.OnePasswordType), + SetupCompleted: false, // Setup not completed + } + + // Environment variable is set - should bypass SetupCompleted check + // This is the Kubernetes use case: operator sets env var, no config file needed + mockEnv.EXPECT().Getenv(secrets.ProviderEnvVar).Return(string(secrets.EnvironmentType)) + got, err := s.GetProviderTypeWithEnv(mockEnv) + require.NoError(t, err, "Should succeed when env var is set, even if SetupCompleted is false") + assert.Equal(t, secrets.EnvironmentType, got, "Should return provider from environment variable") + }) + + t.Run("Non-environment providers require SetupCompleted when set via env var", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockEnv := mocks.NewMockReader(ctrl) + s := &Secrets{ + ProviderType: "", + SetupCompleted: false, // Setup not completed + } + + // Encrypted provider requires setup - should return error + mockEnv.EXPECT().Getenv(secrets.ProviderEnvVar).Return(string(secrets.EncryptedType)) + _, err := s.GetProviderTypeWithEnv(mockEnv) + assert.Error(t, err, "Should return error when non-environment provider is set without setup") + assert.Contains(t, err.Error(), "requires setup to be completed", "Error should mention setup requirement") + assert.Contains(t, err.Error(), "environment", "Error should suggest using environment provider") + }) + + t.Run("Non-environment providers work when SetupCompleted is true", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockEnv := mocks.NewMockReader(ctrl) + s := &Secrets{ + ProviderType: "", + SetupCompleted: true, // Setup completed + } + + // Encrypted provider should work when setup is completed + mockEnv.EXPECT().Getenv(secrets.ProviderEnvVar).Return(string(secrets.EncryptedType)) + got, err := s.GetProviderTypeWithEnv(mockEnv) + require.NoError(t, err, "Should succeed when SetupCompleted is true") + assert.Equal(t, secrets.EncryptedType, got, "Should return provider from environment variable") + }) + + t.Run("1password provider requires SetupCompleted when set via env var", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockEnv := mocks.NewMockReader(ctrl) + s := &Secrets{ + ProviderType: "", + SetupCompleted: false, // Setup not completed + } + + // 1password provider requires setup - should return error + mockEnv.EXPECT().Getenv(secrets.ProviderEnvVar).Return(string(secrets.OnePasswordType)) + _, err := s.GetProviderTypeWithEnv(mockEnv) + assert.Error(t, err, "Should return error when 1password provider is set without setup") + assert.Contains(t, err.Error(), "requires setup to be completed", "Error should mention setup requirement") + }) } From 810480e30d154ac19511c3a7e0ecb0e2ce624e33 Mon Sep 17 00:00:00 2001 From: amirejaz Date: Wed, 28 Jan 2026 23:37:16 +0000 Subject: [PATCH 2/4] refactor the PR to reduce size - secret change reconcilitaon will be added in the follow up PR --- .../mcpexternalauthconfig_controller.go | 169 +------------- .../mcpexternalauthconfig_controller_test.go | 216 ------------------ 2 files changed, 9 insertions(+), 376 deletions(-) diff --git a/cmd/thv-operator/controllers/mcpexternalauthconfig_controller.go b/cmd/thv-operator/controllers/mcpexternalauthconfig_controller.go index 3589d20916..76b6bd3642 100644 --- a/cmd/thv-operator/controllers/mcpexternalauthconfig_controller.go +++ b/cmd/thv-operator/controllers/mcpexternalauthconfig_controller.go @@ -5,12 +5,9 @@ package controllers import ( "context" - "crypto/sha256" - "encoding/hex" "fmt" "time" - corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" @@ -81,12 +78,8 @@ func (r *MCPExternalAuthConfigReconciler) Reconcile(ctx context.Context, req ctr return ctrl.Result{RequeueAfter: externalAuthConfigRequeueDelay}, nil } - // Calculate the hash of the current configuration (including referenced Secret values) - configHash, err := r.calculateConfigHash(ctx, externalAuthConfig) - if err != nil { - logger.Error(err, "Failed to calculate config hash") - return ctrl.Result{}, err - } + // Calculate the hash of the current configuration (spec only, without secret values) + configHash := r.calculateConfigHash(externalAuthConfig.Spec) // Check if the hash has changed if externalAuthConfig.Status.ConfigHash != configHash { @@ -123,14 +116,10 @@ func (r *MCPExternalAuthConfigReconciler) Reconcile(ctx context.Context, req ctr logger.Info("Triggering reconciliation of MCPServer due to MCPExternalAuthConfig change", "mcpserver", server.Name, "externalAuthConfig", externalAuthConfig.Name) - // Add an annotation to the MCPServer to trigger reconciliation - if server.Annotations == nil { - server.Annotations = make(map[string]string) - } - server.Annotations["toolhive.stacklok.dev/externalauthconfig-hash"] = configHash - + // Trigger reconciliation by updating the MCPServer + // This will cause the MCPServer controller to reconcile and pick up the new config if err := r.Update(ctx, &server); err != nil { - logger.Error(err, "Failed to update MCPServer annotation", "mcpserver", server.Name) + logger.Error(err, "Failed to trigger MCPServer reconciliation", "mcpserver", server.Name) // Continue with other servers even if one fails } } @@ -139,71 +128,10 @@ func (r *MCPExternalAuthConfigReconciler) Reconcile(ctx context.Context, req ctr return ctrl.Result{}, nil } -// calculateConfigHash calculates a hash of the MCPExternalAuthConfig spec including referenced Secret values -// This ensures that changes to Secret values trigger reconciliation -func (r *MCPExternalAuthConfigReconciler) calculateConfigHash( - ctx context.Context, - externalAuthConfig *mcpv1alpha1.MCPExternalAuthConfig, -) (string, error) { - // Start with the base spec hash - hashString := ctrlutil.CalculateConfigHash(externalAuthConfig.Spec) - - // Include referenced Secret values in the hash for bearer token configs - if externalAuthConfig.Spec.Type == mcpv1alpha1.ExternalAuthTypeBearerToken && - externalAuthConfig.Spec.BearerToken != nil && - externalAuthConfig.Spec.BearerToken.TokenSecretRef != nil { - var secret corev1.Secret - if err := r.Get(ctx, types.NamespacedName{ - Namespace: externalAuthConfig.Namespace, - Name: externalAuthConfig.Spec.BearerToken.TokenSecretRef.Name, - }, &secret); err != nil { - if errors.IsNotFound(err) { - // Secret doesn't exist yet, include that in hash - hashString += ":secret-not-found" - } else { - return "", fmt.Errorf("failed to get bearer token secret: %w", err) - } - } else { - // Include the secret value in the hash - if tokenValue, ok := secret.Data[externalAuthConfig.Spec.BearerToken.TokenSecretRef.Key]; ok { - hasher := sha256.New() - hasher.Write(tokenValue) - hashString += ":" + hex.EncodeToString(hasher.Sum(nil))[:16] - } else { - hashString += ":key-not-found" - } - } - } - - // Also include token exchange client secret if present - if externalAuthConfig.Spec.Type == mcpv1alpha1.ExternalAuthTypeTokenExchange && - externalAuthConfig.Spec.TokenExchange != nil && - externalAuthConfig.Spec.TokenExchange.ClientSecretRef != nil { - var secret corev1.Secret - if err := r.Get(ctx, types.NamespacedName{ - Namespace: externalAuthConfig.Namespace, - Name: externalAuthConfig.Spec.TokenExchange.ClientSecretRef.Name, - }, &secret); err != nil { - if errors.IsNotFound(err) { - hashString += ":client-secret-not-found" - } else { - return "", fmt.Errorf("failed to get client secret: %w", err) - } - } else { - if secretValue, ok := secret.Data[externalAuthConfig.Spec.TokenExchange.ClientSecretRef.Key]; ok { - hasher := sha256.New() - hasher.Write(secretValue) - hashString += ":" + hex.EncodeToString(hasher.Sum(nil))[:16] - } else { - hashString += ":client-secret-key-not-found" - } - } - } - - // Hash the final combined string - hasher := sha256.New() - hasher.Write([]byte(hashString)) - return hex.EncodeToString(hasher.Sum(nil))[:16], nil +// calculateConfigHash calculates a hash of the MCPExternalAuthConfig spec using Kubernetes utilities. +// Note: This only hashes the spec, not referenced Secret values. Secret value hashing will be added in a follow-up PR. +func (r *MCPExternalAuthConfigReconciler) calculateConfigHash(spec mcpv1alpha1.MCPExternalAuthConfigSpec) string { + return ctrlutil.CalculateConfigHash(spec) } // handleDeletion handles the deletion of a MCPExternalAuthConfig @@ -267,54 +195,6 @@ func (r *MCPExternalAuthConfigReconciler) findReferencingMCPServers( }) } -// findMCPExternalAuthConfigsReferencingSecret finds all MCPExternalAuthConfigs that reference the given Secret. -// This includes configs that reference the Secret for bearer tokens or token exchange client secrets. -func (r *MCPExternalAuthConfigReconciler) findMCPExternalAuthConfigsReferencingSecret( - ctx context.Context, - secret *corev1.Secret, -) ([]mcpv1alpha1.MCPExternalAuthConfig, error) { - // List all MCPExternalAuthConfigs in the same namespace as the Secret - externalAuthConfigs := &mcpv1alpha1.MCPExternalAuthConfigList{} - if err := r.List(ctx, externalAuthConfigs, client.InNamespace(secret.Namespace)); err != nil { - return nil, fmt.Errorf("failed to list MCPExternalAuthConfigs: %w", err) - } - - // Filter configs that reference this Secret - referencingConfigs := make([]mcpv1alpha1.MCPExternalAuthConfig, 0) - for _, config := range externalAuthConfigs.Items { - if configReferencesSecret(&config, secret.Name) { - referencingConfigs = append(referencingConfigs, config) - } - } - - return referencingConfigs, nil -} - -// configReferencesSecret checks if an MCPExternalAuthConfig references a Secret by name. -// This checks both bearer token and token exchange configurations. -func configReferencesSecret( - config *mcpv1alpha1.MCPExternalAuthConfig, - secretName string, -) bool { - // Check bearer token config - if config.Spec.Type == mcpv1alpha1.ExternalAuthTypeBearerToken && - config.Spec.BearerToken != nil && - config.Spec.BearerToken.TokenSecretRef != nil && - config.Spec.BearerToken.TokenSecretRef.Name == secretName { - return true - } - - // Check token exchange config - if config.Spec.Type == mcpv1alpha1.ExternalAuthTypeTokenExchange && - config.Spec.TokenExchange != nil && - config.Spec.TokenExchange.ClientSecretRef != nil && - config.Spec.TokenExchange.ClientSecretRef.Name == secretName { - return true - } - - return false -} - // SetupWithManager sets up the controller with the Manager. func (r *MCPExternalAuthConfigReconciler) SetupWithManager(mgr ctrl.Manager) error { // Create a handler that maps MCPExternalAuthConfig changes to MCPServer reconciliation requests @@ -347,41 +227,10 @@ func (r *MCPExternalAuthConfigReconciler) SetupWithManager(mgr ctrl.Manager) err }, ) - // Create a handler that maps Secret changes to MCPExternalAuthConfig reconciliation requests - secretHandler := handler.EnqueueRequestsFromMapFunc( - func(ctx context.Context, obj client.Object) []reconcile.Request { - secret, ok := obj.(*corev1.Secret) - if !ok { - return nil - } - - // Find all MCPExternalAuthConfigs that reference this Secret - configs, err := r.findMCPExternalAuthConfigsReferencingSecret(ctx, secret) - if err != nil { - log.FromContext(ctx).Error(err, "Failed to find MCPExternalAuthConfigs referencing Secret") - return nil - } - - requests := make([]reconcile.Request, 0, len(configs)) - for _, config := range configs { - requests = append(requests, reconcile.Request{ - NamespacedName: types.NamespacedName{ - Name: config.Name, - Namespace: config.Namespace, - }, - }) - } - - return requests - }, - ) - return ctrl.NewControllerManagedBy(mgr). For(&mcpv1alpha1.MCPExternalAuthConfig{}). // Watch for MCPServers and reconcile the MCPExternalAuthConfig when they change Watches(&mcpv1alpha1.MCPServer{}, externalAuthConfigHandler). - // Watch for Secrets and reconcile MCPExternalAuthConfigs that reference them - Watches(&corev1.Secret{}, secretHandler). Complete(r) } diff --git a/cmd/thv-operator/controllers/mcpexternalauthconfig_controller_test.go b/cmd/thv-operator/controllers/mcpexternalauthconfig_controller_test.go index acaead5aee..dc7bab8f6e 100644 --- a/cmd/thv-operator/controllers/mcpexternalauthconfig_controller_test.go +++ b/cmd/thv-operator/controllers/mcpexternalauthconfig_controller_test.go @@ -20,211 +20,6 @@ import ( mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" ) -func TestMCPExternalAuthConfigReconciler_calculateConfigHash(t *testing.T) { - t.Parallel() - - ctx := t.Context() - - tests := []struct { - name string - externalAuthConfig *mcpv1alpha1.MCPExternalAuthConfig - secrets []*corev1.Secret - }{ - { - name: "empty spec", - externalAuthConfig: &mcpv1alpha1.MCPExternalAuthConfig{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-config", - Namespace: "default", - }, - Spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ - Type: mcpv1alpha1.ExternalAuthTypeTokenExchange, - }, - }, - }, - { - name: "with token exchange config", - externalAuthConfig: &mcpv1alpha1.MCPExternalAuthConfig{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-config", - Namespace: "default", - }, - Spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ - Type: mcpv1alpha1.ExternalAuthTypeTokenExchange, - TokenExchange: &mcpv1alpha1.TokenExchangeConfig{ - TokenURL: "https://oauth.example.com/token", - ClientID: "test-client-id", - ClientSecretRef: &mcpv1alpha1.SecretKeyRef{ - Name: "test-secret", - Key: "client-secret", - }, - Audience: "backend-service", - Scopes: []string{"read", "write"}, - }, - }, - }, - secrets: []*corev1.Secret{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "test-secret", - Namespace: "default", - }, - Data: map[string][]byte{ - "client-secret": []byte("secret-value"), - }, - }, - }, - }, - { - name: "with bearer token config", - externalAuthConfig: &mcpv1alpha1.MCPExternalAuthConfig{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-config", - Namespace: "default", - }, - Spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ - Type: mcpv1alpha1.ExternalAuthTypeBearerToken, - BearerToken: &mcpv1alpha1.BearerTokenConfig{ - TokenSecretRef: &mcpv1alpha1.SecretKeyRef{ - Name: "bearer-secret", - Key: "token", - }, - }, - }, - }, - secrets: []*corev1.Secret{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "bearer-secret", - Namespace: "default", - }, - Data: map[string][]byte{ - "token": []byte("bearer-token-value"), - }, - }, - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - scheme := runtime.NewScheme() - require.NoError(t, mcpv1alpha1.AddToScheme(scheme)) - require.NoError(t, corev1.AddToScheme(scheme)) - - objs := []client.Object{tt.externalAuthConfig} - for _, secret := range tt.secrets { - objs = append(objs, secret) - } - - fakeClient := fake.NewClientBuilder(). - WithScheme(scheme). - WithObjects(objs...). - Build() - - r := &MCPExternalAuthConfigReconciler{ - Client: fakeClient, - Scheme: scheme, - } - - hash1, err1 := r.calculateConfigHash(ctx, tt.externalAuthConfig) - require.NoError(t, err1) - - hash2, err2 := r.calculateConfigHash(ctx, tt.externalAuthConfig) - require.NoError(t, err2) - - // Same spec should produce same hash - assert.Equal(t, hash1, hash2, "Hash should be consistent for same spec") - assert.NotEmpty(t, hash1, "Hash should not be empty") - }) - } - - // Different specs should produce different hashes - t.Run("different specs produce different hashes", func(t *testing.T) { - t.Parallel() - - scheme := runtime.NewScheme() - require.NoError(t, mcpv1alpha1.AddToScheme(scheme)) - require.NoError(t, corev1.AddToScheme(scheme)) - - config1 := &mcpv1alpha1.MCPExternalAuthConfig{ - ObjectMeta: metav1.ObjectMeta{ - Name: "config1", - Namespace: "default", - }, - Spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ - Type: mcpv1alpha1.ExternalAuthTypeTokenExchange, - TokenExchange: &mcpv1alpha1.TokenExchangeConfig{ - TokenURL: "https://oauth.example.com/token", - ClientID: "client1", - ClientSecretRef: &mcpv1alpha1.SecretKeyRef{ - Name: "secret1", - Key: "key1", - }, - Audience: "audience1", - }, - }, - } - config2 := &mcpv1alpha1.MCPExternalAuthConfig{ - ObjectMeta: metav1.ObjectMeta{ - Name: "config2", - Namespace: "default", - }, - Spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ - Type: mcpv1alpha1.ExternalAuthTypeTokenExchange, - TokenExchange: &mcpv1alpha1.TokenExchangeConfig{ - TokenURL: "https://oauth.example.com/token", - ClientID: "client2", - ClientSecretRef: &mcpv1alpha1.SecretKeyRef{ - Name: "secret2", - Key: "key2", - }, - Audience: "audience2", - }, - }, - } - - secret1 := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "secret1", - Namespace: "default", - }, - Data: map[string][]byte{ - "key1": []byte("value1"), - }, - } - secret2 := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "secret2", - Namespace: "default", - }, - Data: map[string][]byte{ - "key2": []byte("value2"), - }, - } - - fakeClient := fake.NewClientBuilder(). - WithScheme(scheme). - WithObjects(config1, config2, secret1, secret2). - Build() - - r := &MCPExternalAuthConfigReconciler{ - Client: fakeClient, - Scheme: scheme, - } - - hash1, err1 := r.calculateConfigHash(ctx, config1) - require.NoError(t, err1) - - hash2, err2 := r.calculateConfigHash(ctx, config2) - require.NoError(t, err2) - - assert.NotEqual(t, hash1, hash2, "Different specs should produce different hashes") - }) -} - func TestMCPExternalAuthConfigReconciler_Reconcile(t *testing.T) { t.Parallel() @@ -787,15 +582,4 @@ func TestMCPExternalAuthConfigReconciler_ConfigChangeTriggersReconciliation(t *t assert.NotEmpty(t, finalConfig.Status.ConfigHash, "Config hash should still be set") assert.NotEqual(t, firstHash, finalConfig.Status.ConfigHash, "Hash should change when spec changes") assert.Equal(t, int64(2), finalConfig.Status.ObservedGeneration, "ObservedGeneration should be updated") - - // Verify MCPServer has annotation with new hash - var updatedServer mcpv1alpha1.MCPServer - err = fakeClient.Get(ctx, types.NamespacedName{ - Name: mcpServer.Name, - Namespace: mcpServer.Namespace, - }, &updatedServer) - require.NoError(t, err) - assert.Equal(t, finalConfig.Status.ConfigHash, - updatedServer.Annotations["toolhive.stacklok.dev/externalauthconfig-hash"], - "MCPServer should have annotation with new config hash") } From b86013f2984b84ab4049134d267bb9f3c71530a9 Mon Sep 17 00:00:00 2001 From: amirejaz Date: Wed, 28 Jan 2026 23:58:08 +0000 Subject: [PATCH 3/4] refactored to reduce PR size --- .../mcpexternalauthconfig_controller.go | 3 +- .../mcpexternalauthconfig_controller_test.go | 109 ++++++++++++++++++ 2 files changed, 110 insertions(+), 2 deletions(-) diff --git a/cmd/thv-operator/controllers/mcpexternalauthconfig_controller.go b/cmd/thv-operator/controllers/mcpexternalauthconfig_controller.go index 825d7e32ed..38c47c7689 100644 --- a/cmd/thv-operator/controllers/mcpexternalauthconfig_controller.go +++ b/cmd/thv-operator/controllers/mcpexternalauthconfig_controller.go @@ -78,7 +78,7 @@ func (r *MCPExternalAuthConfigReconciler) Reconcile(ctx context.Context, req ctr return ctrl.Result{RequeueAfter: externalAuthConfigRequeueDelay}, nil } - // Calculate the hash of the current configuration (spec only, without secret values) + // Calculate the hash of the current configuration configHash := r.calculateConfigHash(externalAuthConfig.Spec) // Check if the hash has changed @@ -133,7 +133,6 @@ func (r *MCPExternalAuthConfigReconciler) Reconcile(ctx context.Context, req ctr } // calculateConfigHash calculates a hash of the MCPExternalAuthConfig spec using Kubernetes utilities. -// Note: This only hashes the spec, not referenced Secret values. Secret value hashing will be added in a follow-up PR. func (*MCPExternalAuthConfigReconciler) calculateConfigHash(spec mcpv1alpha1.MCPExternalAuthConfigSpec) string { return ctrlutil.CalculateConfigHash(spec) } diff --git a/cmd/thv-operator/controllers/mcpexternalauthconfig_controller_test.go b/cmd/thv-operator/controllers/mcpexternalauthconfig_controller_test.go index dc7bab8f6e..e090cc5bde 100644 --- a/cmd/thv-operator/controllers/mcpexternalauthconfig_controller_test.go +++ b/cmd/thv-operator/controllers/mcpexternalauthconfig_controller_test.go @@ -20,6 +20,104 @@ import ( mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" ) +func TestMCPExternalAuthConfigReconciler_calculateConfigHash(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + spec mcpv1alpha1.MCPExternalAuthConfigSpec + }{ + { + name: "empty spec", + spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ + Type: mcpv1alpha1.ExternalAuthTypeTokenExchange, + }, + }, + { + name: "with token exchange config", + spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ + Type: mcpv1alpha1.ExternalAuthTypeTokenExchange, + TokenExchange: &mcpv1alpha1.TokenExchangeConfig{ + TokenURL: "https://oauth.example.com/token", + ClientID: "test-client-id", + ClientSecretRef: &mcpv1alpha1.SecretKeyRef{ + Name: "test-secret", + Key: "client-secret", + }, + Audience: "backend-service", + Scopes: []string{"read", "write"}, + }, + }, + }, + { + name: "with custom header", + spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ + Type: mcpv1alpha1.ExternalAuthTypeTokenExchange, + TokenExchange: &mcpv1alpha1.TokenExchangeConfig{ + TokenURL: "https://oauth.example.com/token", + ClientID: "test-client-id", + ClientSecretRef: &mcpv1alpha1.SecretKeyRef{ + Name: "test-secret", + Key: "client-secret", + }, + Audience: "backend-service", + ExternalTokenHeaderName: "X-Upstream-Token", + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + r := &MCPExternalAuthConfigReconciler{} + + hash1 := r.calculateConfigHash(tt.spec) + hash2 := r.calculateConfigHash(tt.spec) + + // Same spec should produce same hash + assert.Equal(t, hash1, hash2, "Hash should be consistent for same spec") + assert.NotEmpty(t, hash1, "Hash should not be empty") + }) + } + + // Different specs should produce different hashes + t.Run("different specs produce different hashes", func(t *testing.T) { + t.Parallel() + r := &MCPExternalAuthConfigReconciler{} + spec1 := mcpv1alpha1.MCPExternalAuthConfigSpec{ + Type: mcpv1alpha1.ExternalAuthTypeTokenExchange, + TokenExchange: &mcpv1alpha1.TokenExchangeConfig{ + TokenURL: "https://oauth.example.com/token", + ClientID: "client1", + ClientSecretRef: &mcpv1alpha1.SecretKeyRef{ + Name: "secret1", + Key: "key1", + }, + Audience: "audience1", + }, + } + spec2 := mcpv1alpha1.MCPExternalAuthConfigSpec{ + Type: mcpv1alpha1.ExternalAuthTypeTokenExchange, + TokenExchange: &mcpv1alpha1.TokenExchangeConfig{ + TokenURL: "https://oauth.example.com/token", + ClientID: "client2", + ClientSecretRef: &mcpv1alpha1.SecretKeyRef{ + Name: "secret2", + Key: "key2", + }, + Audience: "audience2", + }, + } + + hash1 := r.calculateConfigHash(spec1) + hash2 := r.calculateConfigHash(spec2) + + assert.NotEqual(t, hash1, hash2, "Different specs should produce different hashes") + }) +} + func TestMCPExternalAuthConfigReconciler_Reconcile(t *testing.T) { t.Parallel() @@ -582,4 +680,15 @@ func TestMCPExternalAuthConfigReconciler_ConfigChangeTriggersReconciliation(t *t assert.NotEmpty(t, finalConfig.Status.ConfigHash, "Config hash should still be set") assert.NotEqual(t, firstHash, finalConfig.Status.ConfigHash, "Hash should change when spec changes") assert.Equal(t, int64(2), finalConfig.Status.ObservedGeneration, "ObservedGeneration should be updated") + + // Verify MCPServer has annotation with new hash + var updatedServer mcpv1alpha1.MCPServer + err = fakeClient.Get(ctx, types.NamespacedName{ + Name: mcpServer.Name, + Namespace: mcpServer.Namespace, + }, &updatedServer) + require.NoError(t, err) + assert.Equal(t, finalConfig.Status.ConfigHash, + updatedServer.Annotations["toolhive.stacklok.dev/externalauthconfig-hash"], + "MCPServer should have annotation with new config hash") } From 31408a89d7d1298237d34b79dcde7245b58c7d50 Mon Sep 17 00:00:00 2001 From: amirejaz Date: Thu, 29 Jan 2026 12:30:20 +0000 Subject: [PATCH 4/4] Remove secret RBAC annotation from MCPExternalAuthConfig controller --- .../controllers/mcpexternalauthconfig_controller.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cmd/thv-operator/controllers/mcpexternalauthconfig_controller.go b/cmd/thv-operator/controllers/mcpexternalauthconfig_controller.go index 38c47c7689..65fdf6cbde 100644 --- a/cmd/thv-operator/controllers/mcpexternalauthconfig_controller.go +++ b/cmd/thv-operator/controllers/mcpexternalauthconfig_controller.go @@ -40,7 +40,6 @@ type MCPExternalAuthConfigReconciler struct { // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpexternalauthconfigs/status,verbs=get;update;patch // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpexternalauthconfigs/finalizers,verbs=update // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpservers,verbs=get;list;watch;update;patch -// +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch // Reconcile is part of the main kubernetes reconciliation loop which aims to // move the current state of the cluster closer to the desired state. @@ -132,7 +131,7 @@ func (r *MCPExternalAuthConfigReconciler) Reconcile(ctx context.Context, req ctr return ctrl.Result{}, nil } -// calculateConfigHash calculates a hash of the MCPExternalAuthConfig spec using Kubernetes utilities. +// calculateConfigHash calculates a hash of the MCPExternalAuthConfig spec using Kubernetes utilities func (*MCPExternalAuthConfigReconciler) calculateConfigHash(spec mcpv1alpha1.MCPExternalAuthConfigSpec) string { return ctrlutil.CalculateConfigHash(spec) }