diff --git a/cmd/thv-operator/controllers/mcpexternalauthconfig_controller.go b/cmd/thv-operator/controllers/mcpexternalauthconfig_controller.go index 65fdf6cbde..38c47c7689 100644 --- a/cmd/thv-operator/controllers/mcpexternalauthconfig_controller.go +++ b/cmd/thv-operator/controllers/mcpexternalauthconfig_controller.go @@ -40,6 +40,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. @@ -131,7 +132,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) } diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_deployment.go b/cmd/thv-operator/controllers/mcpremoteproxy_deployment.go index b5649c7690..6c18da270e 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 9a99bd85c9..c5fc4c55ff 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_deployment_test.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_deployment_test.go @@ -843,6 +843,67 @@ func TestBuildEnvVarsForProxy(t *testing.T) { assert.True(t, authFound, "Authorization header 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 0d53295cc9..dbe4e58f37 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_runconfig_test.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_runconfig_test.go @@ -512,6 +512,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 cc45263730..8408e0d342 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -80,14 +80,53 @@ 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) { - // First check the environment variable - this allows Kubernetes deployments + // First check the environment variable (takes precedence) - this allows Kubernetes deployments // to override the secrets provider without requiring local setup - if envVar := envReader.Getenv(secrets.ProviderEnvVar); envVar != "" { - return validateProviderType(envVar) + 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 } - // Check if secrets setup has been completed (only for config file fallback) + // 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 } diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index cd8d2fc00c..806f271834 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -383,4 +383,78 @@ func TestSecrets_GetProviderType_EnvironmentVariable(t *testing.T) { require.NoError(t, err, "Should not return error when env var is set, even if setup not completed") assert.Equal(t, secrets.EnvironmentType, got, "Should return provider type from env var") }) + + 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") + }) }