From 30ac0218aabbfe69c567aeb2c03a063516fa7100 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Fri, 28 Nov 2025 15:40:48 +0000 Subject: [PATCH 1/8] refactor(vmcp): add auth/types package for typed auth config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a new leaf package pkg/vmcp/auth/types containing: - BackendAuthStrategy struct with typed fields (HeaderInjection, TokenExchange) - HeaderInjectionConfig for header injection auth strategy - TokenExchangeConfig for OAuth 2.0 token exchange auth strategy - Strategy type constants (StrategyTypeUnauthenticated, etc.) This package has no dependencies on other vmcp packages, breaking the import cycle between config, strategies, and vmcp packages. The typed fields replace the previous map[string]any Metadata field, providing compile-time type safety and clean YAML serialization. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- pkg/vmcp/auth/types/types.go | 85 ++++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) create mode 100644 pkg/vmcp/auth/types/types.go diff --git a/pkg/vmcp/auth/types/types.go b/pkg/vmcp/auth/types/types.go new file mode 100644 index 000000000..cadfe0962 --- /dev/null +++ b/pkg/vmcp/auth/types/types.go @@ -0,0 +1,85 @@ +// Package types provides shared auth-related types for Virtual MCP Server. +// +// This package is designed as a leaf package with no dependencies on other +// pkg/vmcp/* packages, breaking potential import cycles between config, +// strategies, and other auth-related packages. +// +// Types defined here include: +// - Strategy type constants (StrategyTypeUnauthenticated, etc.) +// - Backend auth configuration structs (BackendAuthStrategy, etc.) +package types + +// Strategy type identifiers used to identify authentication strategies. +const ( + // StrategyTypeUnauthenticated identifies the unauthenticated strategy. + // This strategy performs no authentication and is used when a backend + // requires no authentication. + StrategyTypeUnauthenticated = "unauthenticated" + + // StrategyTypeHeaderInjection identifies the header injection strategy. + // This strategy injects a static header value into request headers. + StrategyTypeHeaderInjection = "header_injection" + + // StrategyTypeTokenExchange identifies the token exchange strategy. + // This strategy exchanges an incoming token for a new token to use + // when authenticating to the backend service. + StrategyTypeTokenExchange = "token_exchange" +) + +// BackendAuthStrategy defines how to authenticate to a specific backend. +// Uses typed fields for type safety and clean YAML serialization. +type BackendAuthStrategy struct { + // Type is the auth strategy: "unauthenticated", "header_injection", "token_exchange" + Type string `json:"type" yaml:"type"` + + // HeaderInjection contains configuration for header injection auth strategy. + // Used when Type = "header_injection". + HeaderInjection *HeaderInjectionConfig `json:"header_injection,omitempty" yaml:"header_injection,omitempty"` + + // TokenExchange contains configuration for token exchange auth strategy. + // Used when Type = "token_exchange". + TokenExchange *TokenExchangeConfig `json:"token_exchange,omitempty" yaml:"token_exchange,omitempty"` +} + +// HeaderInjectionConfig configures the header injection auth strategy. +// This strategy injects a static or environment-sourced header value into requests. +type HeaderInjectionConfig struct { + // HeaderName is the name of the header to inject (e.g., "Authorization"). + HeaderName string `json:"header_name" yaml:"header_name"` + + // HeaderValue is the static header value to inject. + // Either HeaderValue or HeaderValueEnv should be set, not both. + HeaderValue string `json:"header_value,omitempty" yaml:"header_value,omitempty"` + + // HeaderValueEnv is the environment variable name containing the header value. + // The value will be resolved at runtime from this environment variable. + // Either HeaderValue or HeaderValueEnv should be set, not both. + HeaderValueEnv string `json:"header_value_env,omitempty" yaml:"header_value_env,omitempty"` +} + +// TokenExchangeConfig configures the OAuth 2.0 token exchange auth strategy. +// This strategy exchanges incoming tokens for backend-specific tokens using RFC 8693. +type TokenExchangeConfig struct { + // TokenURL is the OAuth token endpoint URL for token exchange. + TokenURL string `json:"token_url" yaml:"token_url"` + + // ClientID is the OAuth client ID for the token exchange request. + ClientID string `json:"client_id,omitempty" yaml:"client_id,omitempty"` + + // ClientSecret is the OAuth client secret (use ClientSecretEnv for security). + ClientSecret string `json:"client_secret,omitempty" yaml:"client_secret,omitempty"` + + // ClientSecretEnv is the environment variable name containing the client secret. + // The value will be resolved at runtime from this environment variable. + ClientSecretEnv string `json:"client_secret_env,omitempty" yaml:"client_secret_env,omitempty"` + + // Audience is the target audience for the exchanged token. + Audience string `json:"audience,omitempty" yaml:"audience,omitempty"` + + // Scopes are the requested scopes for the exchanged token. + Scopes []string `json:"scopes,omitempty" yaml:"scopes,omitempty"` + + // SubjectTokenType is the token type of the incoming subject token. + // Defaults to "urn:ietf:params:oauth:token-type:access_token" if not specified. + SubjectTokenType string `json:"subject_token_type,omitempty" yaml:"subject_token_type,omitempty"` +} From cf69a91d2521db5a43cf1b90e7ac3e1200df4a4e Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Fri, 28 Nov 2025 15:42:08 +0000 Subject: [PATCH 2/8] refactor(vmcp): update core types to use auth/types package MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Update core domain types to use the new typed BackendAuthStrategy: - pkg/vmcp/types.go: Replace AuthStrategy string and AuthMetadata map[string]any with AuthConfig *authtypes.BackendAuthStrategy in Backend and BackendTarget structs - pkg/vmcp/registry.go: Update BackendToTarget to copy AuthConfig - pkg/vmcp/config/config.go: Update OutgoingAuthConfig to use *authtypes.BackendAuthStrategy, update ResolveForBackend to return the typed strategy directly 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- pkg/vmcp/config/config.go | 29 ++++++++++------------------- pkg/vmcp/registry.go | 3 +-- pkg/vmcp/types.go | 27 +++++++++++++-------------- 3 files changed, 24 insertions(+), 35 deletions(-) diff --git a/pkg/vmcp/config/config.go b/pkg/vmcp/config/config.go index 28f761a79..76b36e8a4 100644 --- a/pkg/vmcp/config/config.go +++ b/pkg/vmcp/config/config.go @@ -11,6 +11,7 @@ import ( "time" "github.com/stacklok/toolhive/pkg/vmcp" + authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" ) // Token cache provider types @@ -163,42 +164,32 @@ type OutgoingAuthConfig struct { Source string `json:"source" yaml:"source"` // Default is the default auth strategy for backends without explicit config. - Default *BackendAuthStrategy `json:"default,omitempty" yaml:"default,omitempty"` + Default *authtypes.BackendAuthStrategy `json:"default,omitempty" yaml:"default,omitempty"` // Backends contains per-backend auth configuration. - Backends map[string]*BackendAuthStrategy `json:"backends,omitempty" yaml:"backends,omitempty"` + Backends map[string]*authtypes.BackendAuthStrategy `json:"backends,omitempty" yaml:"backends,omitempty"` } -// BackendAuthStrategy defines how to authenticate to a specific backend. -type BackendAuthStrategy struct { - // Type is the auth strategy: "unauthenticated", "header_injection", "token_exchange" - Type string `json:"type" yaml:"type"` - - // Metadata contains strategy-specific configuration. - // This is opaque and interpreted by the auth strategy implementation. - Metadata map[string]any `json:"metadata,omitempty" yaml:"metadata,omitempty"` -} - -// ResolveForBackend returns the auth strategy and metadata for a given backend ID. +// ResolveForBackend returns the auth strategy for a given backend ID. // It checks for backend-specific config first, then falls back to default. -// Returns empty string and nil if no authentication is configured. -func (c *OutgoingAuthConfig) ResolveForBackend(backendID string) (string, map[string]any) { +// Returns nil if no authentication is configured. +func (c *OutgoingAuthConfig) ResolveForBackend(backendID string) *authtypes.BackendAuthStrategy { if c == nil { - return "", nil + return nil } // Check for backend-specific configuration if strategy, exists := c.Backends[backendID]; exists && strategy != nil { - return strategy.Type, strategy.Metadata + return strategy } // Fall back to default configuration if c.Default != nil { - return c.Default.Type, c.Default.Metadata + return c.Default } // No authentication configured - return "", nil + return nil } // AggregationConfig configures capability aggregation. diff --git a/pkg/vmcp/registry.go b/pkg/vmcp/registry.go index d83c94e94..ac8cb9b18 100644 --- a/pkg/vmcp/registry.go +++ b/pkg/vmcp/registry.go @@ -125,8 +125,7 @@ func BackendToTarget(backend *Backend) *BackendTarget { WorkloadName: backend.Name, BaseURL: backend.BaseURL, TransportType: backend.TransportType, - AuthStrategy: backend.AuthStrategy, - AuthMetadata: backend.AuthMetadata, + AuthConfig: backend.AuthConfig, SessionAffinity: false, // TODO: Add session affinity support in future phases HealthStatus: backend.HealthStatus, Metadata: backend.Metadata, diff --git a/pkg/vmcp/types.go b/pkg/vmcp/types.go index 41ffcc4c3..5afa9269b 100644 --- a/pkg/vmcp/types.go +++ b/pkg/vmcp/types.go @@ -1,6 +1,10 @@ package vmcp -import "context" +import ( + "context" + + authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" +) // This file contains shared domain types used across multiple vmcp subpackages. // Following DDD principles, these are core domain concepts that cross bounded contexts. @@ -45,14 +49,10 @@ type BackendTarget struct { // client.CallTool(ctx, target, target.GetBackendCapabilityName(toolName), args) OriginalCapabilityName string - // AuthStrategy identifies the authentication strategy for this backend. - // The actual authentication is handled by OutgoingAuthRegistry interface. - // Examples: "unauthenticated", "header_injection", "token_exchange" - AuthStrategy string - - // AuthMetadata contains strategy-specific authentication metadata. - // This is opaque to the router and interpreted by the authenticator. - AuthMetadata map[string]any + // AuthConfig contains the typed authentication configuration for this backend. + // This includes the strategy type and strategy-specific configuration. + // If nil, the backend uses no authentication. + AuthConfig *authtypes.BackendAuthStrategy // SessionAffinity indicates if requests from the same session // must be routed to this specific backend instance. @@ -128,11 +128,10 @@ type Backend struct { // HealthStatus is the current health state. HealthStatus BackendHealthStatus - // AuthStrategy identifies how to authenticate to this backend. - AuthStrategy string - - // AuthMetadata contains strategy-specific auth configuration. - AuthMetadata map[string]any + // AuthConfig contains the typed authentication configuration for this backend. + // This includes the strategy type and strategy-specific configuration. + // If nil, the backend uses no authentication. + AuthConfig *authtypes.BackendAuthStrategy // Metadata stores additional backend information. Metadata map[string]string From 32221038880e9591a6451b25ff3c5ba76ddd2812 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Fri, 28 Nov 2025 15:42:41 +0000 Subject: [PATCH 3/8] refactor(vmcp): update auth strategies to use typed config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Update Strategy interface and implementations to use typed *authtypes.BackendAuthStrategy instead of map[string]any: - pkg/vmcp/auth/auth.go: Update Strategy interface methods Authenticate() and Validate() to accept typed strategy - pkg/vmcp/auth/strategies/header_injection.go: Access typed HeaderInjection config fields directly instead of map lookups - pkg/vmcp/auth/strategies/tokenexchange.go: Access typed TokenExchange config fields directly instead of map lookups - pkg/vmcp/auth/strategies/unauthenticated.go: Update to accept typed strategy parameter - pkg/vmcp/auth/strategies/constants.go: Re-export strategy type constants from auth/types for backward compatibility This eliminates runtime type assertions and provides compile-time type safety for auth strategy configuration. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- pkg/vmcp/auth/auth.go | 8 +- pkg/vmcp/auth/strategies/constants.go | 36 ++-- pkg/vmcp/auth/strategies/header_injection.go | 75 ++++---- pkg/vmcp/auth/strategies/tokenexchange.go | 173 ++++++++----------- pkg/vmcp/auth/strategies/unauthenticated.go | 6 +- 5 files changed, 137 insertions(+), 161 deletions(-) diff --git a/pkg/vmcp/auth/auth.go b/pkg/vmcp/auth/auth.go index 455b6e71c..e076c1d79 100644 --- a/pkg/vmcp/auth/auth.go +++ b/pkg/vmcp/auth/auth.go @@ -15,6 +15,7 @@ import ( "net/http" "github.com/stacklok/toolhive/pkg/auth" + authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" ) // OutgoingAuthRegistry manages authentication strategies for outgoing requests to backend MCP servers. @@ -60,11 +61,12 @@ type Strategy interface { Name() string // Authenticate performs authentication and modifies the request. - // The metadata contains strategy-specific configuration. - Authenticate(ctx context.Context, req *http.Request, metadata map[string]any) error + // The strategy receives the typed BackendAuthStrategy which contains + // strategy-specific configuration (HeaderInjection, TokenExchange, etc.). + Authenticate(ctx context.Context, req *http.Request, strategy *authtypes.BackendAuthStrategy) error // Validate checks if the strategy configuration is valid. - Validate(metadata map[string]any) error + Validate(strategy *authtypes.BackendAuthStrategy) error } // Authorizer handles authorization decisions. diff --git a/pkg/vmcp/auth/strategies/constants.go b/pkg/vmcp/auth/strategies/constants.go index b74eed8a7..f81224a9c 100644 --- a/pkg/vmcp/auth/strategies/constants.go +++ b/pkg/vmcp/auth/strategies/constants.go @@ -1,36 +1,20 @@ // Package strategies provides authentication strategy implementations for Virtual MCP Server. package strategies -// Strategy type identifiers used to identify authentication strategies. +import authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" + +// Re-export strategy type constants from authtypes for backward compatibility. +// New code should import these directly from authtypes. const ( // StrategyTypeUnauthenticated identifies the unauthenticated strategy. - // This strategy performs no authentication and is used when a backend - // requires no authentication. - StrategyTypeUnauthenticated = "unauthenticated" + // Deprecated: Use authtypes.StrategyTypeUnauthenticated instead. + StrategyTypeUnauthenticated = authtypes.StrategyTypeUnauthenticated // StrategyTypeHeaderInjection identifies the header injection strategy. - // This strategy injects a static header value into request headers. - StrategyTypeHeaderInjection = "header_injection" + // Deprecated: Use authtypes.StrategyTypeHeaderInjection instead. + StrategyTypeHeaderInjection = authtypes.StrategyTypeHeaderInjection // StrategyTypeTokenExchange identifies the token exchange strategy. - // This strategy exchanges an incoming token for a new token to use - // when authenticating to the backend service. - StrategyTypeTokenExchange = "token_exchange" -) - -// Metadata key names used in strategy configurations. -const ( - // MetadataHeaderName is the key for the HTTP header name in metadata. - // Used by HeaderInjectionStrategy to identify which header to inject. - MetadataHeaderName = "header_name" - - // MetadataHeaderValue is the key for the HTTP header value in metadata. - // Used by HeaderInjectionStrategy to identify the value to inject. - MetadataHeaderValue = "header_value" - - // MetadataHeaderValueEnv is the key for the environment variable name - // that contains the header value. Used by converters during the conversion - // phase to indicate that secret resolution is needed. This is replaced with - // MetadataHeaderValue (containing the actual secret) before reaching the strategy. - MetadataHeaderValueEnv = "header_value_env" + // Deprecated: Use authtypes.StrategyTypeTokenExchange instead. + StrategyTypeTokenExchange = authtypes.StrategyTypeTokenExchange ) diff --git a/pkg/vmcp/auth/strategies/header_injection.go b/pkg/vmcp/auth/strategies/header_injection.go index e6b85fad2..7193c3ad6 100644 --- a/pkg/vmcp/auth/strategies/header_injection.go +++ b/pkg/vmcp/auth/strategies/header_injection.go @@ -7,20 +7,21 @@ import ( "net/http" "github.com/stacklok/toolhive/pkg/validation" + authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" ) // HeaderInjectionStrategy injects a static header value into request headers. // This is a general-purpose strategy that can inject any header with any value, // commonly used for API keys, bearer tokens, or custom authentication headers. // -// The strategy extracts the header name and value from the metadata -// configuration and injects them into the backend request headers. +// The strategy uses the typed HeaderInjectionConfig from BackendAuthStrategy +// and injects the configured header into the backend request. // -// Required metadata fields: -// - header_name: The HTTP header name to use (e.g., "X-API-Key", "Authorization") -// - header_value: The header value to inject (can be an API key, token, or any value) +// Required configuration (in BackendAuthStrategy.HeaderInjection): +// - HeaderName: The HTTP header name to use (e.g., "X-API-Key", "Authorization") +// - HeaderValue: The header value to inject (can be an API key, token, or any value) // Note: In YAML configuration, use either header_value (literal) or header_value_env (from environment). -// The value is resolved at config load time and passed here as header_value. +// The value is resolved at config load time and stored in HeaderValue. // // This strategy is appropriate when: // - The backend requires a static header value for authentication @@ -42,63 +43,71 @@ func (*HeaderInjectionStrategy) Name() string { return StrategyTypeHeaderInjection } -// Authenticate injects the header value from metadata into the request header. +// Authenticate injects the header value from the typed config into the request header. // // This method: -// 1. Validates that header_name and header_value are present in metadata +// 1. Validates that HeaderInjection config is present with HeaderName and HeaderValue // 2. Sets the specified header with the provided value // // Parameters: // - ctx: Request context (currently unused, reserved for future secret resolution) // - req: The HTTP request to authenticate -// - metadata: Strategy-specific configuration containing header_name and header_value +// - strategy: The typed BackendAuthStrategy containing HeaderInjection configuration // // Returns an error if: -// - header_name is missing or empty -// - header_value is missing or empty -func (*HeaderInjectionStrategy) Authenticate(_ context.Context, req *http.Request, metadata map[string]any) error { - headerName, ok := metadata[MetadataHeaderName].(string) - if !ok || headerName == "" { - return fmt.Errorf("header_name required in metadata") +// - strategy or HeaderInjection config is nil +// - HeaderName is missing or empty +// - HeaderValue is missing or empty +func (*HeaderInjectionStrategy) Authenticate(_ context.Context, req *http.Request, strategy *authtypes.BackendAuthStrategy) error { + if strategy == nil || strategy.HeaderInjection == nil { + return fmt.Errorf("header_injection configuration required") } - headerValue, ok := metadata[MetadataHeaderValue].(string) - if !ok || headerValue == "" { - return fmt.Errorf("header_value required in metadata") + cfg := strategy.HeaderInjection + if cfg.HeaderName == "" { + return fmt.Errorf("header_name required in header_injection configuration") } - req.Header.Set(headerName, headerValue) + if cfg.HeaderValue == "" { + return fmt.Errorf("header_value required in header_injection configuration") + } + + req.Header.Set(cfg.HeaderName, cfg.HeaderValue) return nil } -// Validate checks if the required metadata fields are present and valid. +// Validate checks if the required configuration fields are present and valid. // // This method verifies that: -// - header_name is present and non-empty -// - header_value is present and non-empty -// - header_name is a valid HTTP header name (prevents CRLF injection) -// - header_value is a valid HTTP header value (prevents CRLF injection) +// - strategy and HeaderInjection config are not nil +// - HeaderName is present and non-empty +// - HeaderValue is present and non-empty +// - HeaderName is a valid HTTP header name (prevents CRLF injection) +// - HeaderValue is a valid HTTP header value (prevents CRLF injection) // // This validation is typically called during configuration parsing to fail fast // if the strategy is misconfigured. -func (*HeaderInjectionStrategy) Validate(metadata map[string]any) error { - headerName, ok := metadata[MetadataHeaderName].(string) - if !ok || headerName == "" { - return fmt.Errorf("header_name required in metadata") +func (*HeaderInjectionStrategy) Validate(strategy *authtypes.BackendAuthStrategy) error { + if strategy == nil || strategy.HeaderInjection == nil { + return fmt.Errorf("header_injection configuration required") + } + + cfg := strategy.HeaderInjection + if cfg.HeaderName == "" { + return fmt.Errorf("header_name required in header_injection configuration") } - headerValue, ok := metadata[MetadataHeaderValue].(string) - if !ok || headerValue == "" { - return fmt.Errorf("header_value required in metadata") + if cfg.HeaderValue == "" { + return fmt.Errorf("header_value required in header_injection configuration") } // Validate header name to prevent injection attacks - if err := validation.ValidateHTTPHeaderName(headerName); err != nil { + if err := validation.ValidateHTTPHeaderName(cfg.HeaderName); err != nil { return fmt.Errorf("invalid header_name: %w", err) } // Validate header value to prevent injection attacks - if err := validation.ValidateHTTPHeaderValue(headerValue); err != nil { + if err := validation.ValidateHTTPHeaderValue(cfg.HeaderValue); err != nil { return fmt.Errorf("invalid header_value: %w", err) } diff --git a/pkg/vmcp/auth/strategies/tokenexchange.go b/pkg/vmcp/auth/strategies/tokenexchange.go index c9e1dd0b7..bb435810c 100644 --- a/pkg/vmcp/auth/strategies/tokenexchange.go +++ b/pkg/vmcp/auth/strategies/tokenexchange.go @@ -11,6 +11,7 @@ import ( "github.com/stacklok/toolhive/pkg/auth" "github.com/stacklok/toolhive/pkg/auth/tokenexchange" "github.com/stacklok/toolhive/pkg/env" + authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" ) const ( @@ -28,16 +29,13 @@ const ( // recreating configuration objects. Per-user token caching is handled by the upper // vMCP TokenCache layer. // -// Required metadata fields: -// - token_url: The OAuth 2.0 token endpoint URL for token exchange -// -// Optional metadata fields: -// - client_id: OAuth 2.0 client identifier (required for some token endpoints) -// - client_secret: OAuth 2.0 client secret (directly provided, mutually exclusive with client_secret_env) -// - client_secret_env: Name of environment variable containing the client secret (mutually exclusive with client_secret) -// - audience: Target audience for the exchanged token -// - scopes: Array of scope strings to request -// - subject_token_type: Type of the subject token (default: "access_token") +// The strategy uses the typed TokenExchangeConfig from BackendAuthStrategy with: +// - TokenURL: The OAuth 2.0 token endpoint URL for token exchange (required) +// - ClientID: OAuth 2.0 client identifier (optional, required for some token endpoints) +// - ClientSecret: OAuth 2.0 client secret (optional, resolved from config or env) +// - Audience: Target audience for the exchanged token (optional) +// - Scopes: Array of scope strings to request (optional) +// - SubjectTokenType: Type of the subject token (optional, default: "access_token") // // This strategy is appropriate when: // - The backend uses a different identity provider than the vMCP server @@ -69,7 +67,7 @@ func (*TokenExchangeStrategy) Name() string { // // This method: // 1. Retrieves the client's identity and token from the context -// 2. Parses and validates the token exchange configuration from metadata +// 2. Validates the token exchange configuration from the typed BackendAuthStrategy // 3. Gets or creates a cached ExchangeConfig for this backend configuration // 4. Creates a TokenSource with the current identity token // 5. Obtains an access token by performing the exchange @@ -81,14 +79,14 @@ func (*TokenExchangeStrategy) Name() string { // Parameters: // - ctx: Request context containing the authenticated identity // - req: The HTTP request to authenticate -// - metadata: Strategy-specific configuration for token exchange +// - strategy: The typed BackendAuthStrategy containing TokenExchange configuration // // Returns an error if: // - No identity is found in the context // - The identity has no token -// - Metadata is invalid or incomplete +// - strategy or TokenExchange config is nil or invalid // - Token exchange fails -func (s *TokenExchangeStrategy) Authenticate(ctx context.Context, req *http.Request, metadata map[string]any) error { +func (s *TokenExchangeStrategy) Authenticate(ctx context.Context, req *http.Request, strategy *authtypes.BackendAuthStrategy) error { identity, ok := auth.IdentityFromContext(ctx) if !ok { return fmt.Errorf("no identity found in context") @@ -98,14 +96,14 @@ func (s *TokenExchangeStrategy) Authenticate(ctx context.Context, req *http.Requ return fmt.Errorf("identity has no token") } - config, err := parseTokenExchangeMetadata(metadata, s.envReader) + cfg, err := s.extractAndValidateConfig(strategy) if err != nil { - return fmt.Errorf("invalid metadata: %w", err) + return err } // Get user-specific exchange config. This creates a fresh config instance // with the current user's token. The underlying server config is cached. - exchangeConfig := s.createUserConfig(config, identity.Token) + exchangeConfig := s.createUserConfig(cfg, identity.Token) tokenSource := exchangeConfig.TokenSource(ctx) token, err := tokenSource.Token() @@ -118,21 +116,23 @@ func (s *TokenExchangeStrategy) Authenticate(ctx context.Context, req *http.Requ return nil } -// Validate checks if the required metadata fields are present and valid. +// Validate checks if the required configuration fields are present and valid. // // This method verifies that: -// - token_url is present and valid -// - Optional fields (if present) have correct types and values -// - client_secret is only provided when client_id is present +// - strategy and TokenExchange config are not nil +// - TokenURL is present and valid +// - Optional fields (if present) have valid values +// - ClientSecret/ClientSecretEnv is only provided when ClientID is present // // This validation is typically called during configuration parsing to fail fast // if the strategy is misconfigured. -func (s *TokenExchangeStrategy) Validate(metadata map[string]any) error { - _, err := parseTokenExchangeMetadata(metadata, s.envReader) +func (s *TokenExchangeStrategy) Validate(strategy *authtypes.BackendAuthStrategy) error { + _, err := s.extractAndValidateConfig(strategy) return err } -// tokenExchangeConfig holds the parsed token exchange configuration. +// tokenExchangeConfig holds the resolved token exchange configuration. +// This is an internal representation used for caching and token exchange operations. type tokenExchangeConfig struct { TokenURL string ClientID string @@ -142,97 +142,76 @@ type tokenExchangeConfig struct { SubjectTokenType string } -// parseClientSecret parses and validates client_secret or client_secret_env from metadata. -// Returns the resolved client secret, or an error if validation fails. -func parseClientSecret(metadata map[string]any, clientID string, envReader env.Reader) (string, error) { - // Check for client_secret first (takes precedence) - if clientSecret, ok := metadata["client_secret"].(string); ok { - if clientID == "" { - return "", fmt.Errorf("client_secret cannot be provided without client_id") - } - return clientSecret, nil - } - - // Check for client_secret_env - if clientSecretEnv, ok := metadata["client_secret_env"].(string); ok && clientSecretEnv != "" { - if clientID == "" { - return "", fmt.Errorf("client_secret_env cannot be provided without client_id") - } - // Resolve the environment variable - secret := envReader.Getenv(clientSecretEnv) - if secret == "" { - return "", fmt.Errorf("environment variable %s not set or empty", clientSecretEnv) - } - return secret, nil +// extractAndValidateConfig extracts and validates configuration from a typed BackendAuthStrategy. +// Returns the resolved tokenExchangeConfig, or an error if validation fails. +func (s *TokenExchangeStrategy) extractAndValidateConfig(strategy *authtypes.BackendAuthStrategy) (*tokenExchangeConfig, error) { + if strategy == nil || strategy.TokenExchange == nil { + return nil, fmt.Errorf("token_exchange configuration required") } - // No client secret provided (which is valid) - return "", nil -} - -// parseTokenExchangeMetadata parses and validates token exchange metadata. -func parseTokenExchangeMetadata(metadata map[string]any, envReader env.Reader) (*tokenExchangeConfig, error) { - config := &tokenExchangeConfig{} + cfg := strategy.TokenExchange + result := &tokenExchangeConfig{} - // Required: token_url - tokenURL, ok := metadata["token_url"].(string) - if !ok || tokenURL == "" { - return nil, fmt.Errorf("token_url required in metadata") + // Required: TokenURL + if cfg.TokenURL == "" { + return nil, fmt.Errorf("token_url required in token_exchange configuration") } - config.TokenURL = tokenURL + result.TokenURL = cfg.TokenURL - // Optional: client_id - if clientID, ok := metadata["client_id"].(string); ok { - config.ClientID = clientID - } + // Optional: ClientID + result.ClientID = cfg.ClientID - // Optional: client_secret or client_secret_env - clientSecret, err := parseClientSecret(metadata, config.ClientID, envReader) + // Optional: ClientSecret or ClientSecretEnv + clientSecret, err := s.resolveClientSecret(cfg) if err != nil { return nil, err } - config.ClientSecret = clientSecret + result.ClientSecret = clientSecret + + // Optional: Audience + result.Audience = cfg.Audience - // Optional: audience - if audience, ok := metadata["audience"].(string); ok { - config.Audience = audience + // Optional: Scopes (already typed as []string) + result.Scopes = cfg.Scopes + + // Optional: SubjectTokenType + if cfg.SubjectTokenType != "" { + normalized, err := tokenexchange.NormalizeTokenType(cfg.SubjectTokenType) + if err != nil { + return nil, fmt.Errorf("invalid subject_token_type: %w", err) + } + result.SubjectTokenType = normalized } - // Optional: scopes (can be string array or single string) - if scopesRaw, ok := metadata["scopes"]; ok { - switch v := scopesRaw.(type) { - case []string: - config.Scopes = v - case []any: - // Handle case where JSON unmarshaling gives []any - scopes := make([]string, 0, len(v)) - for i, s := range v { - str, ok := s.(string) - if !ok { - return nil, fmt.Errorf("scopes[%d] must be a string", i) - } - scopes = append(scopes, str) - } - config.Scopes = scopes - case string: - // Single scope as string - config.Scopes = []string{v} - default: - return nil, fmt.Errorf("scopes must be a string or array of strings") + return result, nil +} + +// resolveClientSecret resolves the client secret from the typed TokenExchangeConfig. +// Returns the resolved client secret, or an error if validation fails. +func (s *TokenExchangeStrategy) resolveClientSecret(cfg *authtypes.TokenExchangeConfig) (string, error) { + // ClientSecret takes precedence over ClientSecretEnv + if cfg.ClientSecret != "" { + if cfg.ClientID == "" { + return "", fmt.Errorf("client_secret cannot be provided without client_id") } + return cfg.ClientSecret, nil } - // Optional: subject_token_type - if subjectTokenType, ok := metadata["subject_token_type"].(string); ok { - // Validate if provided - normalized, err := tokenexchange.NormalizeTokenType(subjectTokenType) - if err != nil { - return nil, fmt.Errorf("invalid subject_token_type: %w", err) + // Check ClientSecretEnv + if cfg.ClientSecretEnv != "" { + if cfg.ClientID == "" { + return "", fmt.Errorf("client_secret_env cannot be provided without client_id") + } + // Resolve the environment variable + secret := s.envReader.Getenv(cfg.ClientSecretEnv) + if secret == "" { + return "", fmt.Errorf("environment variable %s not set or empty", cfg.ClientSecretEnv) } - config.SubjectTokenType = normalized + return secret, nil } - return config, nil + // No client secret provided (which is valid) + return "", nil } // getOrCreateServerConfig retrieves or creates a cached server-level ExchangeConfig. diff --git a/pkg/vmcp/auth/strategies/unauthenticated.go b/pkg/vmcp/auth/strategies/unauthenticated.go index 9ee8a6178..6432caa5e 100644 --- a/pkg/vmcp/auth/strategies/unauthenticated.go +++ b/pkg/vmcp/auth/strategies/unauthenticated.go @@ -3,6 +3,8 @@ package strategies import ( "context" "net/http" + + authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" ) // UnauthenticatedStrategy is a no-op authentication strategy that performs no authentication. @@ -53,7 +55,7 @@ func (*UnauthenticatedStrategy) Name() string { // - metadata: Strategy-specific configuration (ignored) // // Returns nil (always succeeds). -func (*UnauthenticatedStrategy) Authenticate(_ context.Context, _ *http.Request, _ map[string]any) error { +func (*UnauthenticatedStrategy) Authenticate(_ context.Context, _ *http.Request, _ *authtypes.BackendAuthStrategy) error { // No-op: intentionally does nothing return nil } @@ -66,7 +68,7 @@ func (*UnauthenticatedStrategy) Authenticate(_ context.Context, _ *http.Request, // This permissive validation allows the strategy to be used without // configuration or with arbitrary configuration that may be present // for documentation purposes. -func (*UnauthenticatedStrategy) Validate(_ map[string]any) error { +func (*UnauthenticatedStrategy) Validate(_ *authtypes.BackendAuthStrategy) error { // No-op: accepts any metadata return nil } From e6083272971345bbf4538a4b1fe0243bdff4f3ce Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Fri, 28 Nov 2025 15:43:15 +0000 Subject: [PATCH 4/8] refactor(vmcp): update auth converters to return typed config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Update auth converters to return *authtypes.BackendAuthStrategy instead of map[string]any: - Rename ConvertToMetadata to Convert returning typed strategy - Update ResolveSecrets to accept and return typed strategy - Rename ConvertToStrategyMetadata to ConvertToBackendAuthStrategy - Update DiscoverAndResolveAuth to return typed strategy directly Converters now build typed HeaderInjectionConfig or TokenExchangeConfig structs directly, eliminating the need for runtime type assertions when consuming the converted auth configuration. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .../auth/converters/external_auth_config.go | 7 +- pkg/vmcp/auth/converters/header_injection.go | 34 +++++----- pkg/vmcp/auth/converters/interface.go | 67 +++++++++---------- pkg/vmcp/auth/converters/token_exchange.go | 57 ++++++++-------- 4 files changed, 84 insertions(+), 81 deletions(-) diff --git a/pkg/vmcp/auth/converters/external_auth_config.go b/pkg/vmcp/auth/converters/external_auth_config.go index 454b22b31..2db1f33ab 100644 --- a/pkg/vmcp/auth/converters/external_auth_config.go +++ b/pkg/vmcp/auth/converters/external_auth_config.go @@ -1,3 +1,8 @@ // Package converters provides functions to convert external authentication configurations -// to vMCP auth strategy metadata. +// to typed vMCP BackendAuthStrategy configurations. +// +// The package provides a registry-based approach where each auth type (e.g., token exchange, +// header injection) has a dedicated converter that implements the StrategyConverter interface. +// Converters produce typed *config.BackendAuthStrategy values instead of untyped map[string]any, +// providing type safety and clean serialization. package converters diff --git a/pkg/vmcp/auth/converters/header_injection.go b/pkg/vmcp/auth/converters/header_injection.go index 42a345895..154fcdf56 100644 --- a/pkg/vmcp/auth/converters/header_injection.go +++ b/pkg/vmcp/auth/converters/header_injection.go @@ -10,34 +10,36 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" - "github.com/stacklok/toolhive/pkg/vmcp/auth/strategies" + authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" ) -// HeaderInjectionConverter converts MCPExternalAuthConfig HeaderInjection to vMCP header_injection strategy metadata. +// HeaderInjectionConverter converts MCPExternalAuthConfig HeaderInjection to vMCP header_injection strategy. type HeaderInjectionConverter struct{} // StrategyType returns the vMCP strategy type for header injection. func (*HeaderInjectionConverter) StrategyType() string { - return "header_injection" + return authtypes.StrategyTypeHeaderInjection } -// ConvertToMetadata converts HeaderInjectionConfig to header_injection strategy metadata (without secrets resolved). +// Convert converts HeaderInjectionConfig to a typed BackendAuthStrategy (without secrets resolved). // The secret value will be added by ResolveSecrets. -func (*HeaderInjectionConverter) ConvertToMetadata( +func (*HeaderInjectionConverter) Convert( externalAuth *mcpv1alpha1.MCPExternalAuthConfig, -) (map[string]any, error) { +) (*authtypes.BackendAuthStrategy, error) { headerInjection := externalAuth.Spec.HeaderInjection if headerInjection == nil { return nil, fmt.Errorf("header injection config is nil") } - metadata := make(map[string]any) - metadata[strategies.MetadataHeaderName] = headerInjection.HeaderName - - return metadata, nil + return &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: headerInjection.HeaderName, + }, + }, nil } -// ResolveSecrets fetches the header value secret from Kubernetes and adds it to the metadata. +// ResolveSecrets fetches the header value secret from Kubernetes and adds it to the strategy. // Unlike token exchange which can use environment variables in non-discovered mode, header // injection always requires dynamic secret resolution because backends can be added or modified // at runtime, even in non-discovered mode. The vMCP pod cannot know all backend auth configs @@ -47,8 +49,8 @@ func (*HeaderInjectionConverter) ResolveSecrets( externalAuth *mcpv1alpha1.MCPExternalAuthConfig, k8sClient client.Client, namespace string, - metadata map[string]any, -) (map[string]any, error) { + strategy *authtypes.BackendAuthStrategy, +) (*authtypes.BackendAuthStrategy, error) { headerInjection := externalAuth.Spec.HeaderInjection if headerInjection == nil { return nil, fmt.Errorf("header injection config is nil") @@ -76,8 +78,8 @@ func (*HeaderInjectionConverter) ResolveSecrets( namespace, headerInjection.ValueSecretRef.Name, headerInjection.ValueSecretRef.Key) } - // Add the resolved secret value to metadata - metadata[strategies.MetadataHeaderValue] = string(secretValue) + // Add the resolved secret value to the strategy + strategy.HeaderInjection.HeaderValue = string(secretValue) - return metadata, nil + return strategy, nil } diff --git a/pkg/vmcp/auth/converters/interface.go b/pkg/vmcp/auth/converters/interface.go index 30edfbae7..2596888cf 100644 --- a/pkg/vmcp/auth/converters/interface.go +++ b/pkg/vmcp/auth/converters/interface.go @@ -1,5 +1,5 @@ // Package converters provides a registry for converting external authentication configurations -// to vMCP auth strategy metadata. +// to vMCP auth strategy configurations. package converters import ( @@ -10,21 +10,22 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" + authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" ) -// StrategyConverter defines the interface for converting external auth configs to strategy metadata. +// StrategyConverter defines the interface for converting external auth configs to typed BackendAuthStrategy. // Each auth type (e.g., token exchange, header injection) implements this interface. type StrategyConverter interface { // StrategyType returns the vMCP strategy type identifier (e.g., "token_exchange", "header_injection") StrategyType() string - // ConvertToMetadata converts an MCPExternalAuthConfig to strategy metadata. + // Convert converts an MCPExternalAuthConfig to a typed BackendAuthStrategy. // Secret references should be represented as environment variable names (e.g., "TOOLHIVE_*") // that will be resolved later by ResolveSecrets or at runtime. - ConvertToMetadata(externalAuth *mcpv1alpha1.MCPExternalAuthConfig) (map[string]any, error) + Convert(externalAuth *mcpv1alpha1.MCPExternalAuthConfig) (*authtypes.BackendAuthStrategy, error) // ResolveSecrets fetches secrets from Kubernetes and replaces environment variable references - // with actual secret values in the metadata. This is used in discovered auth mode where + // with actual secret values in the strategy config. This is used in discovered auth mode where // secrets cannot be mounted as environment variables because the vMCP pod doesn't know // about backend auth configs at pod creation time. // @@ -34,8 +35,8 @@ type StrategyConverter interface { externalAuth *mcpv1alpha1.MCPExternalAuthConfig, k8sClient client.Client, namespace string, - metadata map[string]any, - ) (map[string]any, error) + strategy *authtypes.BackendAuthStrategy, + ) (*authtypes.BackendAuthStrategy, error) } // Registry holds registered strategy converters @@ -91,28 +92,23 @@ func (r *Registry) GetConverter(authType mcpv1alpha1.ExternalAuthType) (Strategy return converter, nil } -// ConvertToStrategyMetadata is a convenience function that uses the default registry to convert -// an external auth config to strategy metadata. This is the main entry point for converting +// ConvertToBackendAuthStrategy is a convenience function that uses the default registry to convert +// an external auth config to a typed BackendAuthStrategy. This is the main entry point for converting // auth configs at runtime. -func ConvertToStrategyMetadata( +func ConvertToBackendAuthStrategy( externalAuth *mcpv1alpha1.MCPExternalAuthConfig, -) (strategyType string, metadata map[string]any, err error) { +) (*authtypes.BackendAuthStrategy, error) { if externalAuth == nil { - return "", nil, fmt.Errorf("external auth config is nil") + return nil, fmt.Errorf("external auth config is nil") } registry := DefaultRegistry() converter, err := registry.GetConverter(externalAuth.Spec.Type) if err != nil { - return "", nil, err - } - - metadata, err = converter.ConvertToMetadata(externalAuth) - if err != nil { - return "", nil, err + return nil, err } - return converter.StrategyType(), metadata, nil + return converter.Convert(externalAuth) } // ResolveSecretsForStrategy is a convenience function that uses the default registry to resolve @@ -122,8 +118,8 @@ func ResolveSecretsForStrategy( externalAuth *mcpv1alpha1.MCPExternalAuthConfig, k8sClient client.Client, namespace string, - metadata map[string]any, -) (map[string]any, error) { + strategy *authtypes.BackendAuthStrategy, +) (*authtypes.BackendAuthStrategy, error) { if externalAuth == nil { return nil, fmt.Errorf("external auth config is nil") } @@ -134,29 +130,28 @@ func ResolveSecretsForStrategy( return nil, err } - return converter.ResolveSecrets(ctx, externalAuth, k8sClient, namespace, metadata) + return converter.ResolveSecrets(ctx, externalAuth, k8sClient, namespace, strategy) } // DiscoverAndResolveAuth discovers authentication configuration from an MCPServer's -// ExternalAuthConfigRef and resolves it to a strategy type and metadata. +// ExternalAuthConfigRef and resolves it to a typed BackendAuthStrategy. // This is the main entry point for auth discovery from Kubernetes. // // Returns: -// - strategyType: The auth strategy type (e.g., "token_exchange", "header_injection") -// - metadata: The resolved auth metadata with secrets fetched from Kubernetes +// - strategy: The resolved BackendAuthStrategy with secrets fetched from Kubernetes // - error: Any error that occurred during discovery or resolution // -// Returns empty string and nil if externalAuthConfigRef is nil (no auth configured). +// Returns nil if externalAuthConfigRef is nil (no auth configured). func DiscoverAndResolveAuth( ctx context.Context, externalAuthConfigRef *mcpv1alpha1.ExternalAuthConfigRef, namespace string, k8sClient client.Client, -) (strategyType string, metadata map[string]any, err error) { +) (*authtypes.BackendAuthStrategy, error) { // Check if there's an ExternalAuthConfigRef if externalAuthConfigRef == nil { // No auth config to discover - return "", nil, nil + return nil, nil } // Fetch the MCPExternalAuthConfig @@ -167,7 +162,7 @@ func DiscoverAndResolveAuth( } if err := k8sClient.Get(ctx, key, externalAuth); err != nil { - return "", nil, fmt.Errorf("failed to get MCPExternalAuthConfig %s: %w", externalAuthConfigRef.Name, err) + return nil, fmt.Errorf("failed to get MCPExternalAuthConfig %s: %w", externalAuthConfigRef.Name, err) } // Get the converter registry @@ -176,20 +171,20 @@ func DiscoverAndResolveAuth( // Get the converter for this auth type converter, err := registry.GetConverter(externalAuth.Spec.Type) if err != nil { - return "", nil, fmt.Errorf("failed to get converter for auth type %s: %w", externalAuth.Spec.Type, err) + return nil, fmt.Errorf("failed to get converter for auth type %s: %w", externalAuth.Spec.Type, err) } - // Convert to metadata (without secrets resolved) - metadata, err = converter.ConvertToMetadata(externalAuth) + // Convert to typed BackendAuthStrategy (without secrets resolved) + strategy, err := converter.Convert(externalAuth) if err != nil { - return "", nil, fmt.Errorf("failed to convert to metadata: %w", err) + return nil, fmt.Errorf("failed to convert to strategy: %w", err) } // Resolve secrets from Kubernetes - metadata, err = converter.ResolveSecrets(ctx, externalAuth, k8sClient, namespace, metadata) + strategy, err = converter.ResolveSecrets(ctx, externalAuth, k8sClient, namespace, strategy) if err != nil { - return "", nil, fmt.Errorf("failed to resolve secrets: %w", err) + return nil, fmt.Errorf("failed to resolve secrets: %w", err) } - return converter.StrategyType(), metadata, nil + return strategy, nil } diff --git a/pkg/vmcp/auth/converters/token_exchange.go b/pkg/vmcp/auth/converters/token_exchange.go index 59fef9fd7..c70c930b9 100644 --- a/pkg/vmcp/auth/converters/token_exchange.go +++ b/pkg/vmcp/auth/converters/token_exchange.go @@ -10,45 +10,47 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" + authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" ) -// TokenExchangeConverter converts MCPExternalAuthConfig TokenExchange to vMCP token_exchange strategy metadata. +// TokenExchangeConverter converts MCPExternalAuthConfig TokenExchange to vMCP token_exchange strategy. type TokenExchangeConverter struct{} // StrategyType returns the vMCP strategy type for token exchange. func (*TokenExchangeConverter) StrategyType() string { - return "token_exchange" + return authtypes.StrategyTypeTokenExchange } -// ConvertToMetadata converts TokenExchangeConfig to token_exchange strategy metadata (without secrets resolved). +// Convert converts TokenExchangeConfig to a typed BackendAuthStrategy (without secrets resolved). // Secret references are represented as environment variable names that will be resolved at runtime. -func (*TokenExchangeConverter) ConvertToMetadata( +func (*TokenExchangeConverter) Convert( externalAuth *mcpv1alpha1.MCPExternalAuthConfig, -) (map[string]any, error) { +) (*authtypes.BackendAuthStrategy, error) { tokenExchange := externalAuth.Spec.TokenExchange if tokenExchange == nil { return nil, fmt.Errorf("token exchange config is nil") } - metadata := make(map[string]any) - metadata["token_url"] = tokenExchange.TokenURL + tokenConfig := &authtypes.TokenExchangeConfig{ + TokenURL: tokenExchange.TokenURL, + } // Add optional fields if present if tokenExchange.ClientID != "" { - metadata["client_id"] = tokenExchange.ClientID + tokenConfig.ClientID = tokenExchange.ClientID } if tokenExchange.ClientSecretRef != nil { // Reference to environment variable that will be mounted from secret - metadata["client_secret_env"] = "TOOLHIVE_TOKEN_EXCHANGE_CLIENT_SECRET" + tokenConfig.ClientSecretEnv = "TOOLHIVE_TOKEN_EXCHANGE_CLIENT_SECRET" } if tokenExchange.Audience != "" { - metadata["audience"] = tokenExchange.Audience + tokenConfig.Audience = tokenExchange.Audience } if len(tokenExchange.Scopes) > 0 { - metadata["scopes"] = tokenExchange.Scopes + tokenConfig.Scopes = tokenExchange.Scopes } if tokenExchange.SubjectTokenType != "" { @@ -62,14 +64,13 @@ func (*TokenExchangeConverter) ConvertToMetadata( case "jwt": subjectTokenType = "urn:ietf:params:oauth:token-type:jwt" // #nosec G101 - not a credential } - metadata["subject_token_type"] = subjectTokenType - } - - if tokenExchange.ExternalTokenHeaderName != "" { - metadata["external_token_header_name"] = tokenExchange.ExternalTokenHeaderName + tokenConfig.SubjectTokenType = subjectTokenType } - return metadata, nil + return &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: tokenConfig, + }, nil } // ResolveSecrets fetches the client secret from Kubernetes and replaces the env var reference @@ -78,26 +79,26 @@ func (*TokenExchangeConverter) ConvertToMetadata( // because the vMCP pod doesn't know about backend auth configs at pod creation time. // // This method: -// 1. Checks if client_secret_env is present in metadata +// 1. Checks if ClientSecretEnv is present in the strategy // 2. Fetches the referenced Kubernetes secret -// 3. Replaces client_secret_env with client_secret containing the actual value +// 3. Replaces ClientSecretEnv with ClientSecret containing the actual value // -// If client_secret_env is not present (or client_secret is already set), metadata is returned unchanged. +// If ClientSecretEnv is not present (or ClientSecret is already set), strategy is returned unchanged. func (*TokenExchangeConverter) ResolveSecrets( ctx context.Context, externalAuth *mcpv1alpha1.MCPExternalAuthConfig, k8sClient client.Client, namespace string, - metadata map[string]any, -) (map[string]any, error) { + strategy *authtypes.BackendAuthStrategy, +) (*authtypes.BackendAuthStrategy, error) { tokenExchange := externalAuth.Spec.TokenExchange if tokenExchange == nil { return nil, fmt.Errorf("token exchange config is nil") } - // If no client_secret_env is present, nothing to resolve - if _, hasEnvRef := metadata["client_secret_env"]; !hasEnvRef { - return metadata, nil + // If no ClientSecretEnv is present, nothing to resolve + if strategy.TokenExchange == nil || strategy.TokenExchange.ClientSecretEnv == "" { + return strategy, nil } // If ClientSecretRef is not configured, we cannot resolve @@ -124,8 +125,8 @@ func (*TokenExchangeConverter) ResolveSecrets( } // Replace the env var reference with actual secret value - delete(metadata, "client_secret_env") - metadata["client_secret"] = string(secretValue) + strategy.TokenExchange.ClientSecretEnv = "" + strategy.TokenExchange.ClientSecret = string(secretValue) - return metadata, nil + return strategy, nil } From e6e0732b973488777db2f867929e8821285fb1e5 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Fri, 28 Nov 2025 15:43:37 +0000 Subject: [PATCH 5/8] refactor(vmcp): update config loading to use typed auth strategy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Update YAML loader and validator to use typed auth configuration: - pkg/vmcp/config/yaml_loader.go: Update transformBackendAuthStrategy() to populate typed HeaderInjection or TokenExchange fields instead of the Metadata map. Environment variable resolution (header_value_env) now stores resolved value directly in HeaderValue field. - pkg/vmcp/config/validator.go: Use authtypes.StrategyType* constants for validation instead of importing from strategies package. This simplifies the config loading pipeline by working with typed structs throughout, eliminating intermediate map representations. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- pkg/vmcp/config/validator.go | 33 +++++++++++++------------ pkg/vmcp/config/yaml_loader.go | 44 ++++++++++++++++------------------ 2 files changed, 39 insertions(+), 38 deletions(-) diff --git a/pkg/vmcp/config/validator.go b/pkg/vmcp/config/validator.go index 3c8750bc6..198e14d8b 100644 --- a/pkg/vmcp/config/validator.go +++ b/pkg/vmcp/config/validator.go @@ -5,7 +5,7 @@ import ( "strings" "github.com/stacklok/toolhive/pkg/vmcp" - "github.com/stacklok/toolhive/pkg/vmcp/auth/strategies" + authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" ) // DefaultValidator implements comprehensive configuration validation. @@ -164,15 +164,15 @@ func (v *DefaultValidator) validateOutgoingAuth(auth *OutgoingAuthConfig) error return nil } -func (*DefaultValidator) validateBackendAuthStrategy(_ string, strategy *BackendAuthStrategy) error { +func (*DefaultValidator) validateBackendAuthStrategy(_ string, strategy *authtypes.BackendAuthStrategy) error { if strategy == nil { return fmt.Errorf("strategy is nil") } validTypes := []string{ - strategies.StrategyTypeUnauthenticated, - strategies.StrategyTypeHeaderInjection, - strategies.StrategyTypeTokenExchange, + authtypes.StrategyTypeUnauthenticated, + authtypes.StrategyTypeHeaderInjection, + authtypes.StrategyTypeTokenExchange, // TODO: Add more as strategies are implemented: // "pass_through", "client_credentials", "oauth_proxy", } @@ -182,19 +182,22 @@ func (*DefaultValidator) validateBackendAuthStrategy(_ string, strategy *Backend // Validate type-specific requirements switch strategy.Type { - case strategies.StrategyTypeTokenExchange: - // Token exchange requires token_url (other fields are optional) - if _, ok := strategy.Metadata["token_url"]; !ok { - return fmt.Errorf("token_exchange requires metadata field: token_url") + case authtypes.StrategyTypeTokenExchange: + // Token exchange requires TokenExchange config with TokenURL + if strategy.TokenExchange == nil || strategy.TokenExchange.TokenURL == "" { + return fmt.Errorf("token_exchange requires token_url") } - case strategies.StrategyTypeHeaderInjection: - // Header injection requires header name and value - if _, ok := strategy.Metadata[strategies.MetadataHeaderName]; !ok { - return fmt.Errorf("header_injection requires metadata field: %s", strategies.MetadataHeaderName) + case authtypes.StrategyTypeHeaderInjection: + // Header injection requires HeaderInjection config with header name and value + if strategy.HeaderInjection == nil { + return fmt.Errorf("header_injection requires header_injection configuration") } - if _, ok := strategy.Metadata[strategies.MetadataHeaderValue]; !ok { - return fmt.Errorf("header_injection requires metadata field: %s", strategies.MetadataHeaderValue) + if strategy.HeaderInjection.HeaderName == "" { + return fmt.Errorf("header_injection requires header_name") + } + if strategy.HeaderInjection.HeaderValue == "" && strategy.HeaderInjection.HeaderValueEnv == "" { + return fmt.Errorf("header_injection requires header_value or header_value_env") } } diff --git a/pkg/vmcp/config/yaml_loader.go b/pkg/vmcp/config/yaml_loader.go index f1d3564b7..c4cf62cea 100644 --- a/pkg/vmcp/config/yaml_loader.go +++ b/pkg/vmcp/config/yaml_loader.go @@ -9,7 +9,7 @@ import ( "github.com/stacklok/toolhive/pkg/env" "github.com/stacklok/toolhive/pkg/vmcp" - "github.com/stacklok/toolhive/pkg/vmcp/auth/strategies" + authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" ) // YAMLLoader loads configuration from a YAML file. @@ -294,7 +294,7 @@ func (*YAMLLoader) transformIncomingAuth(raw *rawIncomingAuth) (*IncomingAuthCon func (l *YAMLLoader) transformOutgoingAuth(raw *rawOutgoingAuth) (*OutgoingAuthConfig, error) { cfg := &OutgoingAuthConfig{ Source: raw.Source, - Backends: make(map[string]*BackendAuthStrategy), + Backends: make(map[string]*authtypes.BackendAuthStrategy), } if raw.Default != nil { @@ -316,22 +316,20 @@ func (l *YAMLLoader) transformOutgoingAuth(raw *rawOutgoingAuth) (*OutgoingAuthC return cfg, nil } -//nolint:gocyclo // We should split this into multiple functions per strategy type. -func (l *YAMLLoader) transformBackendAuthStrategy(raw *rawBackendAuthStrategy) (*BackendAuthStrategy, error) { - strategy := &BackendAuthStrategy{ - Type: raw.Type, - Metadata: make(map[string]any), +func (l *YAMLLoader) transformBackendAuthStrategy(raw *rawBackendAuthStrategy) (*authtypes.BackendAuthStrategy, error) { + strategy := &authtypes.BackendAuthStrategy{ + Type: raw.Type, } switch raw.Type { - case strategies.StrategyTypeHeaderInjection: + case authtypes.StrategyTypeHeaderInjection: if raw.HeaderInjection == nil { return nil, fmt.Errorf("header_injection configuration is required") } // Validate that exactly one of header_value or header_value_env is set - // to make the life of the strategy easier, we read the value here in set preference - // order and pass it in metadata in a single value regardless of how it was set. + // to make the life of the strategy easier, we read the value here and resolve + // environment variables, storing the resolved value in HeaderValue. hasValue := raw.HeaderInjection.HeaderValue != "" hasValueEnv := raw.HeaderInjection.HeaderValueEnv != "" @@ -351,15 +349,15 @@ func (l *YAMLLoader) transformBackendAuthStrategy(raw *rawBackendAuthStrategy) ( } } - strategy.Metadata = map[string]any{ - strategies.MetadataHeaderName: raw.HeaderInjection.HeaderName, - strategies.MetadataHeaderValue: headerValue, + strategy.HeaderInjection = &authtypes.HeaderInjectionConfig{ + HeaderName: raw.HeaderInjection.HeaderName, + HeaderValue: headerValue, } - case strategies.StrategyTypeUnauthenticated: - // No metadata required for unauthenticated strategy + case authtypes.StrategyTypeUnauthenticated: + // No configuration required for unauthenticated strategy - case "token_exchange": + case authtypes.StrategyTypeTokenExchange: if raw.TokenExchange == nil { return nil, fmt.Errorf("token_exchange configuration is required") } @@ -371,13 +369,13 @@ func (l *YAMLLoader) transformBackendAuthStrategy(raw *rawBackendAuthStrategy) ( } } - strategy.Metadata = map[string]any{ - "token_url": raw.TokenExchange.TokenURL, - "client_id": raw.TokenExchange.ClientID, - "client_secret_env": raw.TokenExchange.ClientSecretEnv, - "audience": raw.TokenExchange.Audience, - "scopes": raw.TokenExchange.Scopes, - "subject_token_type": raw.TokenExchange.SubjectTokenType, + strategy.TokenExchange = &authtypes.TokenExchangeConfig{ + TokenURL: raw.TokenExchange.TokenURL, + ClientID: raw.TokenExchange.ClientID, + ClientSecretEnv: raw.TokenExchange.ClientSecretEnv, + Audience: raw.TokenExchange.Audience, + Scopes: raw.TokenExchange.Scopes, + SubjectTokenType: raw.TokenExchange.SubjectTokenType, } } From a14819b21760f2f3e92112791f4e19c2698be502 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Fri, 28 Nov 2025 15:44:04 +0000 Subject: [PATCH 6/8] refactor(vmcp): update remaining consumers to use typed auth config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Update all remaining code that consumes auth configuration: - pkg/vmcp/workloads/k8s.go: Use typed BackendAuthStrategy from converters, simplified since converters return typed values - pkg/vmcp/client/client.go: Access AuthConfig.Type directly - pkg/vmcp/aggregator/discoverer.go: Use typed auth config - pkg/vmcp/auth/factory/outgoing.go: Pass typed strategy to strategy.Authenticate() and Validate() - cmd/thv-operator/pkg/vmcpconfig/converter.go: Build typed BackendAuthStrategy directly - pkg/vmcp/auth/mocks/mock_strategy.go: Regenerated mock with updated Strategy interface signatures 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- cmd/thv-operator/pkg/vmcpconfig/converter.go | 26 +++++++++++--------- pkg/vmcp/aggregator/discoverer.go | 17 +++++++------ pkg/vmcp/auth/factory/outgoing.go | 7 +++--- pkg/vmcp/auth/mocks/mock_strategy.go | 17 +++++++------ pkg/vmcp/client/client.go | 20 +++++++++------ pkg/vmcp/workloads/k8s.go | 11 ++++----- 6 files changed, 53 insertions(+), 45 deletions(-) diff --git a/cmd/thv-operator/pkg/vmcpconfig/converter.go b/cmd/thv-operator/pkg/vmcpconfig/converter.go index e4f3ef52b..45c088894 100644 --- a/cmd/thv-operator/pkg/vmcpconfig/converter.go +++ b/cmd/thv-operator/pkg/vmcpconfig/converter.go @@ -9,6 +9,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log" mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" + authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" vmcpconfig "github.com/stacklok/toolhive/pkg/vmcp/config" ) @@ -157,7 +158,7 @@ func (c *Converter) convertOutgoingAuth( ) *vmcpconfig.OutgoingAuthConfig { outgoing := &vmcpconfig.OutgoingAuthConfig{ Source: vmcp.Spec.OutgoingAuth.Source, - Backends: make(map[string]*vmcpconfig.BackendAuthStrategy), + Backends: make(map[string]*authtypes.BackendAuthStrategy), } // Convert Default @@ -174,20 +175,21 @@ func (c *Converter) convertOutgoingAuth( } // convertBackendAuthConfig converts BackendAuthConfig from CRD to vmcp config +// +// Note: The CRD's BackendAuthConfig only contains type and reference information. +// For type="external_auth_config_ref", the actual auth config (HeaderInjection or +// TokenExchange) is stored in a separate MCPExternalAuthConfig resource and resolved +// at runtime by the discovery mechanism, not in this static converter. func (*Converter) convertBackendAuthConfig( crdConfig *mcpv1alpha1.BackendAuthConfig, -) *vmcpconfig.BackendAuthStrategy { - strategy := &vmcpconfig.BackendAuthStrategy{ - Type: crdConfig.Type, - Metadata: make(map[string]any), - } - - // Convert type-specific configuration to metadata - if crdConfig.ExternalAuthConfigRef != nil { - strategy.Metadata["externalAuthConfigRef"] = crdConfig.ExternalAuthConfigRef.Name +) *authtypes.BackendAuthStrategy { + return &authtypes.BackendAuthStrategy{ + Type: crdConfig.Type, + // HeaderInjection and TokenExchange are nil here because the CRD's + // BackendAuthConfig references external configuration via ExternalAuthConfigRef. + // The actual auth strategy details are resolved at runtime from the + // referenced MCPExternalAuthConfig resource. } - - return strategy } // convertAggregation converts AggregationConfig from CRD to vmcp config diff --git a/pkg/vmcp/aggregator/discoverer.go b/pkg/vmcp/aggregator/discoverer.go index e5a4c571b..8a43eb489 100644 --- a/pkg/vmcp/aggregator/discoverer.go +++ b/pkg/vmcp/aggregator/discoverer.go @@ -179,12 +179,12 @@ func (d *backendDiscoverer) applyAuthConfigToBackend(backend *vmcp.Backend, back // In discovered mode, use auth discovered from MCPServer (if any exists) // If no auth is discovered, fall back to config-based auth via ResolveForBackend // which will use backend-specific config, then Default, then no auth - useDiscoveredAuth = backend.AuthStrategy != "" + useDiscoveredAuth = backend.AuthConfig != nil && backend.AuthConfig.Type != "" case "mixed": // In mixed mode, use discovered auth as default, but allow config overrides // If there's no explicit config for this backend, use discovered auth _, hasExplicitConfig := d.authConfig.Backends[backendName] - useDiscoveredAuth = !hasExplicitConfig && backend.AuthStrategy != "" + useDiscoveredAuth = !hasExplicitConfig && backend.AuthConfig != nil && backend.AuthConfig.Type != "" case "inline", "": // For inline mode or empty source, always use config-based auth // Ignore any discovered auth from backends @@ -197,14 +197,15 @@ func (d *backendDiscoverer) applyAuthConfigToBackend(backend *vmcp.Backend, back if useDiscoveredAuth { // Keep the auth discovered from MCPServer (already populated in backend) - logger.Debugf("Backend %s using discovered auth strategy: %s", backendName, backend.AuthStrategy) + if backend.AuthConfig != nil { + logger.Debugf("Backend %s using discovered auth strategy: %s", backendName, backend.AuthConfig.Type) + } } else { // Use auth from config (inline mode or explicit override in mixed mode) - authStrategy, authMetadata := d.authConfig.ResolveForBackend(backendName) - if authStrategy != "" { - backend.AuthStrategy = authStrategy - backend.AuthMetadata = authMetadata - logger.Debugf("Backend %s configured with auth strategy from config: %s", backendName, authStrategy) + authConfig := d.authConfig.ResolveForBackend(backendName) + if authConfig != nil && authConfig.Type != "" { + backend.AuthConfig = authConfig + logger.Debugf("Backend %s configured with auth strategy from config: %s", backendName, authConfig.Type) } } } diff --git a/pkg/vmcp/auth/factory/outgoing.go b/pkg/vmcp/auth/factory/outgoing.go index 356d1ef7f..81e45ee71 100644 --- a/pkg/vmcp/auth/factory/outgoing.go +++ b/pkg/vmcp/auth/factory/outgoing.go @@ -21,6 +21,7 @@ import ( "github.com/stacklok/toolhive/pkg/env" "github.com/stacklok/toolhive/pkg/vmcp/auth" "github.com/stacklok/toolhive/pkg/vmcp/auth/strategies" + authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" ) // NewOutgoingAuthRegistry creates an OutgoingAuthRegistry with all available strategies. @@ -49,19 +50,19 @@ func NewOutgoingAuthRegistry( // Always register all strategies - they're cheap and stateless if err := registry.RegisterStrategy( - strategies.StrategyTypeUnauthenticated, + authtypes.StrategyTypeUnauthenticated, strategies.NewUnauthenticatedStrategy(), ); err != nil { return nil, err } if err := registry.RegisterStrategy( - strategies.StrategyTypeHeaderInjection, + authtypes.StrategyTypeHeaderInjection, strategies.NewHeaderInjectionStrategy(), ); err != nil { return nil, err } if err := registry.RegisterStrategy( - strategies.StrategyTypeTokenExchange, + authtypes.StrategyTypeTokenExchange, strategies.NewTokenExchangeStrategy(envReader), ); err != nil { return nil, err diff --git a/pkg/vmcp/auth/mocks/mock_strategy.go b/pkg/vmcp/auth/mocks/mock_strategy.go index fec90af0e..258108a18 100644 --- a/pkg/vmcp/auth/mocks/mock_strategy.go +++ b/pkg/vmcp/auth/mocks/mock_strategy.go @@ -14,6 +14,7 @@ import ( http "net/http" reflect "reflect" + types "github.com/stacklok/toolhive/pkg/vmcp/auth/types" gomock "go.uber.org/mock/gomock" ) @@ -42,17 +43,17 @@ func (m *MockStrategy) EXPECT() *MockStrategyMockRecorder { } // Authenticate mocks base method. -func (m *MockStrategy) Authenticate(ctx context.Context, req *http.Request, metadata map[string]any) error { +func (m *MockStrategy) Authenticate(ctx context.Context, req *http.Request, strategy *types.BackendAuthStrategy) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Authenticate", ctx, req, metadata) + ret := m.ctrl.Call(m, "Authenticate", ctx, req, strategy) ret0, _ := ret[0].(error) return ret0 } // Authenticate indicates an expected call of Authenticate. -func (mr *MockStrategyMockRecorder) Authenticate(ctx, req, metadata any) *gomock.Call { +func (mr *MockStrategyMockRecorder) Authenticate(ctx, req, strategy any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Authenticate", reflect.TypeOf((*MockStrategy)(nil).Authenticate), ctx, req, metadata) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Authenticate", reflect.TypeOf((*MockStrategy)(nil).Authenticate), ctx, req, strategy) } // Name mocks base method. @@ -70,15 +71,15 @@ func (mr *MockStrategyMockRecorder) Name() *gomock.Call { } // Validate mocks base method. -func (m *MockStrategy) Validate(metadata map[string]any) error { +func (m *MockStrategy) Validate(strategy *types.BackendAuthStrategy) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Validate", metadata) + ret := m.ctrl.Call(m, "Validate", strategy) ret0, _ := ret[0].(error) return ret0 } // Validate indicates an expected call of Validate. -func (mr *MockStrategyMockRecorder) Validate(metadata any) *gomock.Call { +func (mr *MockStrategyMockRecorder) Validate(strategy any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Validate", reflect.TypeOf((*MockStrategy)(nil).Validate), metadata) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Validate", reflect.TypeOf((*MockStrategy)(nil).Validate), strategy) } diff --git a/pkg/vmcp/client/client.go b/pkg/vmcp/client/client.go index 351b26838..aa19e359c 100644 --- a/pkg/vmcp/client/client.go +++ b/pkg/vmcp/client/client.go @@ -18,6 +18,7 @@ import ( "github.com/stacklok/toolhive/pkg/logger" "github.com/stacklok/toolhive/pkg/vmcp" "github.com/stacklok/toolhive/pkg/vmcp/auth" + authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" ) const ( @@ -80,12 +81,12 @@ func (f roundTripperFunc) RoundTrip(req *http.Request) (*http.Response, error) { } // authRoundTripper is an http.RoundTripper that adds authentication to backend requests. -// The authentication strategy and metadata are pre-resolved and validated at client creation time, +// The authentication strategy and config are pre-resolved and validated at client creation time, // eliminating per-request lookups and validation overhead. type authRoundTripper struct { base http.RoundTripper authStrategy auth.Strategy - authMetadata map[string]any + authConfig *authtypes.BackendAuthStrategy target *vmcp.BackendTarget } @@ -97,7 +98,7 @@ func (a *authRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) reqClone := req.Clone(req.Context()) // Apply pre-resolved authentication strategy - if err := a.authStrategy.Authenticate(reqClone.Context(), reqClone, a.authMetadata); err != nil { + if err := a.authStrategy.Authenticate(reqClone.Context(), reqClone, a.authConfig); err != nil { return nil, fmt.Errorf("authentication failed for backend %s: %w", a.target.WorkloadID, err) } @@ -111,11 +112,14 @@ func (a *authRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) // This method should be called once at client creation time to enable fail-fast // behavior for invalid authentication configurations. func (h *httpBackendClient) resolveAuthStrategy(target *vmcp.BackendTarget) (auth.Strategy, error) { - strategyName := target.AuthStrategy + var strategyName string + if target.AuthConfig != nil { + strategyName = target.AuthConfig.Type + } // Default to unauthenticated if not specified if strategyName == "" { - strategyName = "unauthenticated" + strategyName = authtypes.StrategyTypeUnauthenticated } // Resolve strategy from registry @@ -139,8 +143,8 @@ func (h *httpBackendClient) defaultClientFactory(ctx context.Context, target *vm target.WorkloadID, err) } - // Validate metadata ONCE at client creation time - if err := authStrategy.Validate(target.AuthMetadata); err != nil { + // Validate auth config ONCE at client creation time + if err := authStrategy.Validate(target.AuthConfig); err != nil { return nil, fmt.Errorf("invalid authentication configuration for backend %s: %w", target.WorkloadID, err) } @@ -149,7 +153,7 @@ func (h *httpBackendClient) defaultClientFactory(ctx context.Context, target *vm baseTransport = &authRoundTripper{ base: baseTransport, authStrategy: authStrategy, - authMetadata: target.AuthMetadata, + authConfig: target.AuthConfig, target: target, } diff --git a/pkg/vmcp/workloads/k8s.go b/pkg/vmcp/workloads/k8s.go index dd1dd5157..196172b49 100644 --- a/pkg/vmcp/workloads/k8s.go +++ b/pkg/vmcp/workloads/k8s.go @@ -210,7 +210,7 @@ func (d *k8sDiscoverer) mcpServerToBackend(ctx context.Context, mcpServer *mcpv1 // - Returns error if auth config exists but discovery/resolution fails (e.g., missing secret, invalid config) func (d *k8sDiscoverer) discoverAuthConfig(ctx context.Context, mcpServer *mcpv1alpha1.MCPServer, backend *vmcp.Backend) error { // Discover and resolve auth using the converters package - strategyType, metadata, err := converters.DiscoverAndResolveAuth( + authConfig, err := converters.DiscoverAndResolveAuth( ctx, mcpServer.Spec.ExternalAuthConfigRef, mcpServer.Namespace, @@ -221,16 +221,15 @@ func (d *k8sDiscoverer) discoverAuthConfig(ctx context.Context, mcpServer *mcpv1 } // If no auth was discovered, nothing to populate - if strategyType == "" { + if authConfig == nil { logger.Debugf("MCPServer %s has no ExternalAuthConfigRef, no auth config to discover", mcpServer.Name) return nil } - // Populate backend auth fields - backend.AuthStrategy = strategyType - backend.AuthMetadata = metadata + // Populate backend auth config + backend.AuthConfig = authConfig - logger.Debugf("Discovered auth config for MCPServer %s: strategy=%s", mcpServer.Name, backend.AuthStrategy) + logger.Debugf("Discovered auth config for MCPServer %s: strategy=%s", mcpServer.Name, authConfig.Type) return nil } From 607d3947c291cfc91d699058c1679277b77be320 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Fri, 28 Nov 2025 15:44:38 +0000 Subject: [PATCH 7/8] refactor(vmcp): update tests for typed auth config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Update all test files to use typed BackendAuthStrategy: - Strategy tests: Use typed config in Authenticate/Validate calls - Converter tests: Verify typed output instead of map[string]any - Config tests: Build typed strategy structs in test cases - Integration tests: Use typed WithAuth helper - Operator tests: Remove Metadata field assertions All tests updated to construct typed HeaderInjectionConfig or TokenExchangeConfig structs instead of map[string]any metadata. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .../virtualmcpserver_vmcpconfig_test.go | 12 +- pkg/vmcp/aggregator/discoverer_test.go | 392 +++++------------- .../auth/converters/header_injection_test.go | 79 ++-- pkg/vmcp/auth/converters/registry_test.go | 91 ++-- .../auth/converters/token_exchange_test.go | 194 +++++---- pkg/vmcp/auth/factory/integration_test.go | 62 +-- pkg/vmcp/auth/factory/outgoing_test.go | 74 ++-- .../auth/strategies/header_injection_test.go | 307 +++++++------- .../auth/strategies/tokenexchange_test.go | 334 +++++++++++---- .../auth/strategies/unauthenticated_test.go | 50 +-- pkg/vmcp/client/client_test.go | 114 +++-- pkg/vmcp/config/config_test.go | 150 +++---- pkg/vmcp/config/validator_test.go | 17 +- pkg/vmcp/config/yaml_loader_test.go | 14 +- pkg/vmcp/config/yaml_loader_transform_test.go | 47 ++- pkg/vmcp/registry_test.go | 85 ++-- pkg/vmcp/workloads/k8s_test.go | 46 +- test/integration/vmcp/helpers/vmcp_server.go | 9 +- .../integration/vmcp/vmcp_integration_test.go | 19 +- 19 files changed, 1100 insertions(+), 996 deletions(-) diff --git a/cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig_test.go b/cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig_test.go index 5abdfb873..45be1234b 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig_test.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig_test.go @@ -162,7 +162,6 @@ func TestConvertBackendAuthConfig(t *testing.T) { name string authConfig *mcpv1alpha1.BackendAuthConfig expectedType string - hasMetadata bool }{ { name: "discovered", @@ -170,7 +169,6 @@ func TestConvertBackendAuthConfig(t *testing.T) { Type: mcpv1alpha1.BackendAuthTypeDiscovered, }, expectedType: mcpv1alpha1.BackendAuthTypeDiscovered, - hasMetadata: false, }, { name: "external auth config ref", @@ -181,7 +179,6 @@ func TestConvertBackendAuthConfig(t *testing.T) { }, }, expectedType: mcpv1alpha1.BackendAuthTypeExternalAuthConfigRef, - hasMetadata: true, }, } @@ -212,9 +209,12 @@ func TestConvertBackendAuthConfig(t *testing.T) { require.NotNil(t, strategy) assert.Equal(t, tt.expectedType, strategy.Type) - if tt.hasMetadata { - assert.NotEmpty(t, strategy.Metadata) - } + // Note: HeaderInjection and TokenExchange are nil because the CRD's + // BackendAuthConfig only stores type and reference information. + // For external_auth_config_ref, the actual auth config is resolved + // at runtime from the referenced MCPExternalAuthConfig resource. + assert.Nil(t, strategy.HeaderInjection) + assert.Nil(t, strategy.TokenExchange) }) } } diff --git a/pkg/vmcp/aggregator/discoverer_test.go b/pkg/vmcp/aggregator/discoverer_test.go index 00f9609cf..690d8ad4b 100644 --- a/pkg/vmcp/aggregator/discoverer_test.go +++ b/pkg/vmcp/aggregator/discoverer_test.go @@ -11,6 +11,7 @@ import ( "github.com/stacklok/toolhive/pkg/groups/mocks" "github.com/stacklok/toolhive/pkg/vmcp" + authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" "github.com/stacklok/toolhive/pkg/vmcp/config" discoverermocks "github.com/stacklok/toolhive/pkg/vmcp/workloads/mocks" ) @@ -292,11 +293,12 @@ func TestBackendDiscoverer_Discover(t *testing.T) { } authConfig := &config.OutgoingAuthConfig{ - Backends: map[string]*config.BackendAuthStrategy{ + Backends: map[string]*authtypes.BackendAuthStrategy{ "workload1": { - Type: "bearer", - Metadata: map[string]any{ - "token": "test-token", + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-API-Key", + HeaderValue: "test-token", }, }, }, @@ -312,8 +314,10 @@ func TestBackendDiscoverer_Discover(t *testing.T) { require.NoError(t, err) require.Len(t, backends, 1) - assert.Equal(t, "bearer", backends[0].AuthStrategy) - assert.Equal(t, "test-token", backends[0].AuthMetadata["token"]) + require.NotNil(t, backends[0].AuthConfig) + assert.Equal(t, authtypes.StrategyTypeHeaderInjection, backends[0].AuthConfig.Type) + require.NotNil(t, backends[0].AuthConfig.HeaderInjection) + assert.Equal(t, "test-token", backends[0].AuthConfig.HeaderInjection.HeaderValue) }) } @@ -422,11 +426,12 @@ func TestBackendDiscoverer_applyAuthConfigToBackend(t *testing.T) { authConfig := &config.OutgoingAuthConfig{ Source: "discovered", - Backends: map[string]*config.BackendAuthStrategy{ + Backends: map[string]*authtypes.BackendAuthStrategy{ "backend1": { - Type: "bearer", - Metadata: map[string]any{ - "token": "config-token", + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-API-Key", + HeaderValue: "config-token", }, }, }, @@ -439,19 +444,23 @@ func TestBackendDiscoverer_applyAuthConfigToBackend(t *testing.T) { } backend := &vmcp.Backend{ - ID: "backend1", - Name: "backend1", - AuthStrategy: "token_exchange", - AuthMetadata: map[string]any{ - "token_endpoint": "https://auth.example.com/token", + ID: "backend1", + Name: "backend1", + AuthConfig: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "https://auth.example.com/token", + }, }, } discoverer.applyAuthConfigToBackend(backend, "backend1") // In discovered mode, discovered auth should be preserved - assert.Equal(t, "token_exchange", backend.AuthStrategy) - assert.Equal(t, "https://auth.example.com/token", backend.AuthMetadata["token_endpoint"]) + require.NotNil(t, backend.AuthConfig) + assert.Equal(t, authtypes.StrategyTypeTokenExchange, backend.AuthConfig.Type) + require.NotNil(t, backend.AuthConfig.TokenExchange) + assert.Equal(t, "https://auth.example.com/token", backend.AuthConfig.TokenExchange.TokenURL) }) t.Run("discovered mode without discovered auth falls back to config", func(t *testing.T) { @@ -464,11 +473,12 @@ func TestBackendDiscoverer_applyAuthConfigToBackend(t *testing.T) { authConfig := &config.OutgoingAuthConfig{ Source: "discovered", - Backends: map[string]*config.BackendAuthStrategy{ + Backends: map[string]*authtypes.BackendAuthStrategy{ "backend1": { - Type: "bearer", - Metadata: map[string]any{ - "token": "config-token", + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-API-Key", + HeaderValue: "config-token", }, }, }, @@ -483,14 +493,16 @@ func TestBackendDiscoverer_applyAuthConfigToBackend(t *testing.T) { backend := &vmcp.Backend{ ID: "backend1", Name: "backend1", - // No AuthStrategy set - no discovered auth + // No AuthConfig set - no discovered auth } discoverer.applyAuthConfigToBackend(backend, "backend1") // Should fall back to config-based auth - assert.Equal(t, "bearer", backend.AuthStrategy) - assert.Equal(t, "config-token", backend.AuthMetadata["token"]) + require.NotNil(t, backend.AuthConfig) + assert.Equal(t, authtypes.StrategyTypeHeaderInjection, backend.AuthConfig.Type) + require.NotNil(t, backend.AuthConfig.HeaderInjection) + assert.Equal(t, "config-token", backend.AuthConfig.HeaderInjection.HeaderValue) }) t.Run("mixed mode with explicit config override", func(t *testing.T) { @@ -503,11 +515,12 @@ func TestBackendDiscoverer_applyAuthConfigToBackend(t *testing.T) { authConfig := &config.OutgoingAuthConfig{ Source: "mixed", - Backends: map[string]*config.BackendAuthStrategy{ + Backends: map[string]*authtypes.BackendAuthStrategy{ "backend1": { - Type: "bearer", - Metadata: map[string]any{ - "token": "override-token", + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-API-Key", + HeaderValue: "override-token", }, }, }, @@ -520,19 +533,23 @@ func TestBackendDiscoverer_applyAuthConfigToBackend(t *testing.T) { } backend := &vmcp.Backend{ - ID: "backend1", - Name: "backend1", - AuthStrategy: "token_exchange", - AuthMetadata: map[string]any{ - "token_endpoint": "https://auth.example.com/token", + ID: "backend1", + Name: "backend1", + AuthConfig: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "https://auth.example.com/token", + }, }, } discoverer.applyAuthConfigToBackend(backend, "backend1") // In mixed mode with explicit config, config should override discovered auth - assert.Equal(t, "bearer", backend.AuthStrategy) - assert.Equal(t, "override-token", backend.AuthMetadata["token"]) + require.NotNil(t, backend.AuthConfig) + assert.Equal(t, authtypes.StrategyTypeHeaderInjection, backend.AuthConfig.Type) + require.NotNil(t, backend.AuthConfig.HeaderInjection) + assert.Equal(t, "override-token", backend.AuthConfig.HeaderInjection.HeaderValue) }) t.Run("mixed mode without explicit config uses discovered auth", func(t *testing.T) { @@ -545,11 +562,12 @@ func TestBackendDiscoverer_applyAuthConfigToBackend(t *testing.T) { authConfig := &config.OutgoingAuthConfig{ Source: "mixed", - Backends: map[string]*config.BackendAuthStrategy{ + Backends: map[string]*authtypes.BackendAuthStrategy{ "other-backend": { - Type: "bearer", - Metadata: map[string]any{ - "token": "other-token", + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-API-Key", + HeaderValue: "other-token", }, }, }, @@ -562,19 +580,23 @@ func TestBackendDiscoverer_applyAuthConfigToBackend(t *testing.T) { } backend := &vmcp.Backend{ - ID: "backend1", - Name: "backend1", - AuthStrategy: "token_exchange", - AuthMetadata: map[string]any{ - "token_endpoint": "https://auth.example.com/token", + ID: "backend1", + Name: "backend1", + AuthConfig: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "https://auth.example.com/token", + }, }, } discoverer.applyAuthConfigToBackend(backend, "backend1") // In mixed mode without explicit config, discovered auth should be preserved - assert.Equal(t, "token_exchange", backend.AuthStrategy) - assert.Equal(t, "https://auth.example.com/token", backend.AuthMetadata["token_endpoint"]) + require.NotNil(t, backend.AuthConfig) + assert.Equal(t, authtypes.StrategyTypeTokenExchange, backend.AuthConfig.Type) + require.NotNil(t, backend.AuthConfig.TokenExchange) + assert.Equal(t, "https://auth.example.com/token", backend.AuthConfig.TokenExchange.TokenURL) }) t.Run("inline mode ignores discovered auth", func(t *testing.T) { @@ -587,53 +609,12 @@ func TestBackendDiscoverer_applyAuthConfigToBackend(t *testing.T) { authConfig := &config.OutgoingAuthConfig{ Source: "inline", - Backends: map[string]*config.BackendAuthStrategy{ - "backend1": { - Type: "bearer", - Metadata: map[string]any{ - "token": "inline-token", - }, - }, - }, - } - - discoverer := &backendDiscoverer{ - workloadsManager: mockWorkloadDiscoverer, - groupsManager: mockGroups, - authConfig: authConfig, - } - - backend := &vmcp.Backend{ - ID: "backend1", - Name: "backend1", - AuthStrategy: "token_exchange", - AuthMetadata: map[string]any{ - "token_endpoint": "https://auth.example.com/token", - }, - } - - discoverer.applyAuthConfigToBackend(backend, "backend1") - - // In inline mode, config-based auth should replace discovered auth - assert.Equal(t, "bearer", backend.AuthStrategy) - assert.Equal(t, "inline-token", backend.AuthMetadata["token"]) - }) - - t.Run("empty source mode ignores discovered auth", func(t *testing.T) { - t.Parallel() - ctrl := gomock.NewController(t) - t.Cleanup(ctrl.Finish) - - mockWorkloadDiscoverer := discoverermocks.NewMockDiscoverer(ctrl) - mockGroups := mocks.NewMockManager(ctrl) - - authConfig := &config.OutgoingAuthConfig{ - Source: "", // Empty source - Backends: map[string]*config.BackendAuthStrategy{ + Backends: map[string]*authtypes.BackendAuthStrategy{ "backend1": { - Type: "bearer", - Metadata: map[string]any{ - "token": "config-token", + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-API-Key", + HeaderValue: "inline-token", }, }, }, @@ -646,61 +627,23 @@ func TestBackendDiscoverer_applyAuthConfigToBackend(t *testing.T) { } backend := &vmcp.Backend{ - ID: "backend1", - Name: "backend1", - AuthStrategy: "token_exchange", - AuthMetadata: map[string]any{ - "token_endpoint": "https://auth.example.com/token", - }, - } - - discoverer.applyAuthConfigToBackend(backend, "backend1") - - // Empty source should behave like inline mode - assert.Equal(t, "bearer", backend.AuthStrategy) - assert.Equal(t, "config-token", backend.AuthMetadata["token"]) - }) - - t.Run("unknown source mode defaults to config-based auth", func(t *testing.T) { - t.Parallel() - ctrl := gomock.NewController(t) - t.Cleanup(ctrl.Finish) - - mockWorkloadDiscoverer := discoverermocks.NewMockDiscoverer(ctrl) - mockGroups := mocks.NewMockManager(ctrl) - - authConfig := &config.OutgoingAuthConfig{ - Source: "unknown-mode", - Backends: map[string]*config.BackendAuthStrategy{ - "backend1": { - Type: "bearer", - Metadata: map[string]any{ - "token": "fallback-token", - }, + ID: "backend1", + Name: "backend1", + AuthConfig: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "https://auth.example.com/token", }, }, } - discoverer := &backendDiscoverer{ - workloadsManager: mockWorkloadDiscoverer, - groupsManager: mockGroups, - authConfig: authConfig, - } - - backend := &vmcp.Backend{ - ID: "backend1", - Name: "backend1", - AuthStrategy: "token_exchange", - AuthMetadata: map[string]any{ - "token_endpoint": "https://auth.example.com/token", - }, - } - discoverer.applyAuthConfigToBackend(backend, "backend1") - // Unknown source should fall back to config-based auth for safety - assert.Equal(t, "bearer", backend.AuthStrategy) - assert.Equal(t, "fallback-token", backend.AuthMetadata["token"]) + // In inline mode, config-based auth should replace discovered auth + require.NotNil(t, backend.AuthConfig) + assert.Equal(t, authtypes.StrategyTypeHeaderInjection, backend.AuthConfig.Type) + require.NotNil(t, backend.AuthConfig.HeaderInjection) + assert.Equal(t, "inline-token", backend.AuthConfig.HeaderInjection.HeaderValue) }) t.Run("nil auth config does nothing", func(t *testing.T) { @@ -717,149 +660,27 @@ func TestBackendDiscoverer_applyAuthConfigToBackend(t *testing.T) { authConfig: nil, // No auth config } - backend := &vmcp.Backend{ - ID: "backend1", - Name: "backend1", - AuthStrategy: "token_exchange", - AuthMetadata: map[string]any{ - "token_endpoint": "https://auth.example.com/token", - }, - } - - discoverer.applyAuthConfigToBackend(backend, "backend1") - - // With nil auth config, backend should remain unchanged - assert.Equal(t, "token_exchange", backend.AuthStrategy) - assert.Equal(t, "https://auth.example.com/token", backend.AuthMetadata["token_endpoint"]) - }) - - t.Run("no config for backend in inline mode leaves backend unchanged", func(t *testing.T) { - t.Parallel() - ctrl := gomock.NewController(t) - t.Cleanup(ctrl.Finish) - - mockWorkloadDiscoverer := discoverermocks.NewMockDiscoverer(ctrl) - mockGroups := mocks.NewMockManager(ctrl) - - authConfig := &config.OutgoingAuthConfig{ - Source: "inline", - Backends: map[string]*config.BackendAuthStrategy{ - "other-backend": { - Type: "bearer", - Metadata: map[string]any{ - "token": "other-token", - }, - }, - }, - } - - discoverer := &backendDiscoverer{ - workloadsManager: mockWorkloadDiscoverer, - groupsManager: mockGroups, - authConfig: authConfig, - } - - backend := &vmcp.Backend{ - ID: "backend1", - Name: "backend1", - AuthStrategy: "token_exchange", - AuthMetadata: map[string]any{ - "token_endpoint": "https://auth.example.com/token", - }, - } - - discoverer.applyAuthConfigToBackend(backend, "backend1") - - // In inline mode with no config for this backend, discovered auth is cleared - // but no new auth is applied (ResolveForBackend returns empty) - assert.Equal(t, "token_exchange", backend.AuthStrategy) - assert.Equal(t, "https://auth.example.com/token", backend.AuthMetadata["token_endpoint"]) - }) - - t.Run("discovered mode with header injection auth", func(t *testing.T) { - t.Parallel() - ctrl := gomock.NewController(t) - t.Cleanup(ctrl.Finish) - - mockWorkloadDiscoverer := discoverermocks.NewMockDiscoverer(ctrl) - mockGroups := mocks.NewMockManager(ctrl) - - authConfig := &config.OutgoingAuthConfig{ - Source: "discovered", - Backends: map[string]*config.BackendAuthStrategy{}, - } - - discoverer := &backendDiscoverer{ - workloadsManager: mockWorkloadDiscoverer, - groupsManager: mockGroups, - authConfig: authConfig, - } - - backend := &vmcp.Backend{ - ID: "backend1", - Name: "backend1", - AuthStrategy: "header_injection", - AuthMetadata: map[string]any{ - "header_name": "X-API-Key", - "api_key": "secret-key-123", - }, - } - - discoverer.applyAuthConfigToBackend(backend, "backend1") - - // In discovered mode, header injection auth should be preserved - assert.Equal(t, "header_injection", backend.AuthStrategy) - assert.Equal(t, "X-API-Key", backend.AuthMetadata["header_name"]) - assert.Equal(t, "secret-key-123", backend.AuthMetadata["api_key"]) - }) - - t.Run("mixed mode falls back to config when no discovered auth", func(t *testing.T) { - t.Parallel() - ctrl := gomock.NewController(t) - t.Cleanup(ctrl.Finish) - - mockWorkloadDiscoverer := discoverermocks.NewMockDiscoverer(ctrl) - mockGroups := mocks.NewMockManager(ctrl) - - authConfig := &config.OutgoingAuthConfig{ - Source: "mixed", - Backends: map[string]*config.BackendAuthStrategy{ - "other-backend": { - Type: "bearer", - Metadata: map[string]any{ - "token": "other-token", - }, - }, - }, - Default: &config.BackendAuthStrategy{ - Type: "bearer", - Metadata: map[string]any{ - "token": "default-token", - }, - }, - } - - discoverer := &backendDiscoverer{ - workloadsManager: mockWorkloadDiscoverer, - groupsManager: mockGroups, - authConfig: authConfig, - } - backend := &vmcp.Backend{ ID: "backend1", Name: "backend1", - // No discovered auth + AuthConfig: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "https://auth.example.com/token", + }, + }, } discoverer.applyAuthConfigToBackend(backend, "backend1") - // In mixed mode with no explicit config and no discovered auth, - // should use default config - assert.Equal(t, "bearer", backend.AuthStrategy) - assert.Equal(t, "default-token", backend.AuthMetadata["token"]) + // With nil auth config, backend should remain unchanged + require.NotNil(t, backend.AuthConfig) + assert.Equal(t, authtypes.StrategyTypeTokenExchange, backend.AuthConfig.Type) + require.NotNil(t, backend.AuthConfig.TokenExchange) + assert.Equal(t, "https://auth.example.com/token", backend.AuthConfig.TokenExchange.TokenURL) }) - t.Run("discovered mode falls back to default config when no auth discovered", func(t *testing.T) { + t.Run("default auth config is applied when backend not in Backends map", func(t *testing.T) { t.Parallel() ctrl := gomock.NewController(t) t.Cleanup(ctrl.Finish) @@ -868,11 +689,13 @@ func TestBackendDiscoverer_applyAuthConfigToBackend(t *testing.T) { mockGroups := mocks.NewMockManager(ctrl) authConfig := &config.OutgoingAuthConfig{ - Source: "discovered", - Default: &config.BackendAuthStrategy{ - Type: "bearer", - Metadata: map[string]any{ - "token": "default-fallback-token", + Source: "inline", + Backends: map[string]*authtypes.BackendAuthStrategy{}, + Default: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-Default-Key", + HeaderValue: "default-token", }, }, } @@ -886,13 +709,14 @@ func TestBackendDiscoverer_applyAuthConfigToBackend(t *testing.T) { backend := &vmcp.Backend{ ID: "backend1", Name: "backend1", - // No discovered auth (AuthStrategy is empty) } discoverer.applyAuthConfigToBackend(backend, "backend1") - // In discovered mode with no discovered auth, should fall back to default config - assert.Equal(t, "bearer", backend.AuthStrategy) - assert.Equal(t, "default-fallback-token", backend.AuthMetadata["token"]) + // Default auth should be applied + require.NotNil(t, backend.AuthConfig) + assert.Equal(t, authtypes.StrategyTypeHeaderInjection, backend.AuthConfig.Type) + require.NotNil(t, backend.AuthConfig.HeaderInjection) + assert.Equal(t, "default-token", backend.AuthConfig.HeaderInjection.HeaderValue) }) } diff --git a/pkg/vmcp/auth/converters/header_injection_test.go b/pkg/vmcp/auth/converters/header_injection_test.go index 7cf39ea0e..99ca2b947 100644 --- a/pkg/vmcp/auth/converters/header_injection_test.go +++ b/pkg/vmcp/auth/converters/header_injection_test.go @@ -12,22 +12,23 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/fake" mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" + authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" ) func TestHeaderInjectionConverter_StrategyType(t *testing.T) { t.Parallel() converter := &HeaderInjectionConverter{} - assert.Equal(t, "header_injection", converter.StrategyType()) + assert.Equal(t, authtypes.StrategyTypeHeaderInjection, converter.StrategyType()) } -func TestHeaderInjectionConverter_ConvertToMetadata(t *testing.T) { +func TestHeaderInjectionConverter_Convert(t *testing.T) { t.Parallel() tests := []struct { name string externalAuth *mcpv1alpha1.MCPExternalAuthConfig - wantMetadata map[string]any + wantStrategy *authtypes.BackendAuthStrategy wantErr bool errContains string }{ @@ -49,8 +50,11 @@ func TestHeaderInjectionConverter_ConvertToMetadata(t *testing.T) { }, }, }, - wantMetadata: map[string]any{ - "header_name": "X-API-Key", + wantStrategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-API-Key", + }, }, wantErr: false, }, @@ -76,7 +80,7 @@ func TestHeaderInjectionConverter_ConvertToMetadata(t *testing.T) { t.Parallel() converter := &HeaderInjectionConverter{} - metadata, err := converter.ConvertToMetadata(tt.externalAuth) + strategy, err := converter.Convert(tt.externalAuth) if tt.wantErr { require.Error(t, err) @@ -87,7 +91,7 @@ func TestHeaderInjectionConverter_ConvertToMetadata(t *testing.T) { } require.NoError(t, err) - assert.Equal(t, tt.wantMetadata, metadata) + assert.Equal(t, tt.wantStrategy, strategy) }) } } @@ -96,13 +100,13 @@ func TestHeaderInjectionConverter_ResolveSecrets(t *testing.T) { t.Parallel() tests := []struct { - name string - externalAuth *mcpv1alpha1.MCPExternalAuthConfig - secret *corev1.Secret - inputMeta map[string]any - wantMetadata map[string]any - wantErr bool - errContains string + name string + externalAuth *mcpv1alpha1.MCPExternalAuthConfig + secret *corev1.Secret + inputStrategy *authtypes.BackendAuthStrategy + wantStrategy *authtypes.BackendAuthStrategy + wantErr bool + errContains string }{ { name: "successful secret resolution", @@ -131,12 +135,18 @@ func TestHeaderInjectionConverter_ResolveSecrets(t *testing.T) { "key": []byte("secret-value-123"), }, }, - inputMeta: map[string]any{ - "header_name": "X-API-Key", + inputStrategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-API-Key", + }, }, - wantMetadata: map[string]any{ - "header_name": "X-API-Key", - "header_value": "secret-value-123", + wantStrategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-API-Key", + HeaderValue: "secret-value-123", + }, }, wantErr: false, }, @@ -158,8 +168,11 @@ func TestHeaderInjectionConverter_ResolveSecrets(t *testing.T) { }, }, }, - inputMeta: map[string]any{ - "header_name": "X-API-Key", + inputStrategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-API-Key", + }, }, wantErr: true, errContains: "failed to get secret", @@ -191,8 +204,11 @@ func TestHeaderInjectionConverter_ResolveSecrets(t *testing.T) { "key": []byte("secret-value"), }, }, - inputMeta: map[string]any{ - "header_name": "X-API-Key", + inputStrategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-API-Key", + }, }, wantErr: true, errContains: "does not contain key", @@ -209,7 +225,9 @@ func TestHeaderInjectionConverter_ResolveSecrets(t *testing.T) { HeaderInjection: nil, }, }, - inputMeta: map[string]any{}, + inputStrategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + }, wantErr: true, errContains: "header injection config is nil", }, @@ -228,7 +246,12 @@ func TestHeaderInjectionConverter_ResolveSecrets(t *testing.T) { }, }, }, - inputMeta: map[string]any{}, + inputStrategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-API-Key", + }, + }, wantErr: true, errContains: "valueSecretRef is nil", }, @@ -255,12 +278,12 @@ func TestHeaderInjectionConverter_ResolveSecrets(t *testing.T) { Build() converter := &HeaderInjectionConverter{} - metadata, err := converter.ResolveSecrets( + strategy, err := converter.ResolveSecrets( context.Background(), tt.externalAuth, fakeClient, tt.externalAuth.Namespace, - tt.inputMeta, + tt.inputStrategy, ) if tt.wantErr { @@ -272,7 +295,7 @@ func TestHeaderInjectionConverter_ResolveSecrets(t *testing.T) { } require.NoError(t, err) - assert.Equal(t, tt.wantMetadata, metadata) + assert.Equal(t, tt.wantStrategy, strategy) }) } } diff --git a/pkg/vmcp/auth/converters/registry_test.go b/pkg/vmcp/auth/converters/registry_test.go index 61bf62adb..552cd253a 100644 --- a/pkg/vmcp/auth/converters/registry_test.go +++ b/pkg/vmcp/auth/converters/registry_test.go @@ -13,6 +13,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/fake" mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" + authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" ) func TestDefaultRegistry(t *testing.T) { @@ -71,13 +72,13 @@ func TestDefaultRegistry(t *testing.T) { tokenExchangeConverter, err := registry.GetConverter(mcpv1alpha1.ExternalAuthTypeTokenExchange) require.NoError(t, err) require.NotNil(t, tokenExchangeConverter) - assert.Equal(t, "token_exchange", tokenExchangeConverter.StrategyType()) + assert.Equal(t, authtypes.StrategyTypeTokenExchange, tokenExchangeConverter.StrategyType()) // Test header injection converter headerInjectionConverter, err := registry.GetConverter(mcpv1alpha1.ExternalAuthTypeHeaderInjection) require.NoError(t, err) require.NotNil(t, headerInjectionConverter) - assert.Equal(t, "header_injection", headerInjectionConverter.StrategyType()) + assert.Equal(t, authtypes.StrategyTypeHeaderInjection, headerInjectionConverter.StrategyType()) }) } @@ -121,8 +122,8 @@ func TestNewRegistry(t *testing.T) { authType mcpv1alpha1.ExternalAuthType expectedType string }{ - {mcpv1alpha1.ExternalAuthTypeTokenExchange, "token_exchange"}, - {mcpv1alpha1.ExternalAuthTypeHeaderInjection, "header_injection"}, + {mcpv1alpha1.ExternalAuthTypeTokenExchange, authtypes.StrategyTypeTokenExchange}, + {mcpv1alpha1.ExternalAuthTypeHeaderInjection, authtypes.StrategyTypeHeaderInjection}, } for _, tc := range testCases { @@ -166,7 +167,7 @@ func TestRegistry_Register(t *testing.T) { // Verify first converter is registered retrieved, err := registry.GetConverter(mcpv1alpha1.ExternalAuthTypeHeaderInjection) require.NoError(t, err) - assert.Equal(t, "header_injection", retrieved.StrategyType()) + assert.Equal(t, authtypes.StrategyTypeHeaderInjection, retrieved.StrategyType()) // Register a TokenExchangeConverter with same auth type (should overwrite) converter2 := &TokenExchangeConverter{} @@ -175,7 +176,7 @@ func TestRegistry_Register(t *testing.T) { // Verify second converter overwrote the first retrieved, err = registry.GetConverter(mcpv1alpha1.ExternalAuthTypeHeaderInjection) require.NoError(t, err) - assert.Equal(t, "token_exchange", retrieved.StrategyType(), "should return second converter with different strategy type") + assert.Equal(t, authtypes.StrategyTypeTokenExchange, retrieved.StrategyType(), "should return second converter with different strategy type") }) t.Run("is thread-safe", func(t *testing.T) { @@ -252,7 +253,7 @@ func TestRegistry_GetConverter(t *testing.T) { errs <- err return } - if converter.StrategyType() != "header_injection" { + if converter.StrategyType() != authtypes.StrategyTypeHeaderInjection { errs <- assert.AnError } }() @@ -268,7 +269,7 @@ func TestRegistry_GetConverter(t *testing.T) { }) } -func TestConvertToStrategyMetadata(t *testing.T) { +func TestConvertToBackendAuthStrategy(t *testing.T) { t.Parallel() t.Run("converts header injection config", func(t *testing.T) { @@ -291,11 +292,12 @@ func TestConvertToStrategyMetadata(t *testing.T) { }, } - strategyType, metadata, err := ConvertToStrategyMetadata(authConfig) + strategy, err := ConvertToBackendAuthStrategy(authConfig) require.NoError(t, err) - assert.Equal(t, "header_injection", strategyType) - assert.NotNil(t, metadata) - assert.Equal(t, "X-API-Key", metadata["header_name"]) + assert.NotNil(t, strategy) + assert.Equal(t, authtypes.StrategyTypeHeaderInjection, strategy.Type) + require.NotNil(t, strategy.HeaderInjection) + assert.Equal(t, "X-API-Key", strategy.HeaderInjection.HeaderName) }) t.Run("converts token exchange config", func(t *testing.T) { @@ -321,21 +323,20 @@ func TestConvertToStrategyMetadata(t *testing.T) { }, } - strategyType, metadata, err := ConvertToStrategyMetadata(authConfig) + strategy, err := ConvertToBackendAuthStrategy(authConfig) require.NoError(t, err) - assert.Equal(t, "token_exchange", strategyType) - assert.NotNil(t, metadata) - // Token exchange metadata contains URLs and config - assert.NotEmpty(t, metadata) + assert.NotNil(t, strategy) + assert.Equal(t, authtypes.StrategyTypeTokenExchange, strategy.Type) + require.NotNil(t, strategy.TokenExchange) + assert.Equal(t, "https://auth.example.com/token", strategy.TokenExchange.TokenURL) }) t.Run("returns error for nil config", func(t *testing.T) { t.Parallel() - strategyType, metadata, err := ConvertToStrategyMetadata(nil) + strategy, err := ConvertToBackendAuthStrategy(nil) assert.Error(t, err) - assert.Empty(t, strategyType) - assert.Nil(t, metadata) + assert.Nil(t, strategy) assert.Contains(t, err.Error(), "external auth config is nil") }) @@ -352,10 +353,9 @@ func TestConvertToStrategyMetadata(t *testing.T) { }, } - strategyType, metadata, err := ConvertToStrategyMetadata(authConfig) + strategy, err := ConvertToBackendAuthStrategy(authConfig) assert.Error(t, err) - assert.Empty(t, strategyType) - assert.Nil(t, metadata) + assert.Nil(t, strategy) assert.Contains(t, err.Error(), "unsupported auth type") }) @@ -373,10 +373,9 @@ func TestConvertToStrategyMetadata(t *testing.T) { }, } - strategyType, metadata, err := ConvertToStrategyMetadata(authConfig) + strategy, err := ConvertToBackendAuthStrategy(authConfig) assert.Error(t, err) - assert.Empty(t, strategyType) - assert.Nil(t, metadata) + assert.Nil(t, strategy) assert.Contains(t, err.Error(), "nil") }) } @@ -426,16 +425,18 @@ func TestResolveSecretsForStrategy(t *testing.T) { }, } - initialMetadata := map[string]any{ - "header_name": "X-API-Key", - "header_value_env": "TOOLHIVE_HEADER_INJECTION_VALUE", + initialStrategy := &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-API-Key", + }, } - resolvedMetadata, err := ResolveSecretsForStrategy(ctx, authConfig, k8sClient, "default", initialMetadata) + resolvedStrategy, err := ResolveSecretsForStrategy(ctx, authConfig, k8sClient, "default", initialStrategy) require.NoError(t, err) - assert.NotNil(t, resolvedMetadata) - assert.Equal(t, "X-API-Key", resolvedMetadata["header_name"]) - assert.Equal(t, "secret-value-123", resolvedMetadata["header_value"]) + assert.NotNil(t, resolvedStrategy) + assert.Equal(t, "X-API-Key", resolvedStrategy.HeaderInjection.HeaderName) + assert.Equal(t, "secret-value-123", resolvedStrategy.HeaderInjection.HeaderValue) }) t.Run("returns error for nil config", func(t *testing.T) { @@ -448,10 +449,10 @@ func TestResolveSecretsForStrategy(t *testing.T) { WithScheme(scheme). Build() - inputMetadata := map[string]any{} - metadata, err := ResolveSecretsForStrategy(ctx, nil, k8sClient, "default", inputMetadata) + inputStrategy := &authtypes.BackendAuthStrategy{} + strategy, err := ResolveSecretsForStrategy(ctx, nil, k8sClient, "default", inputStrategy) assert.Error(t, err) - assert.Nil(t, metadata, "should return nil on error") + assert.Nil(t, strategy, "should return nil on error") assert.Contains(t, err.Error(), "external auth config is nil") }) @@ -475,10 +476,10 @@ func TestResolveSecretsForStrategy(t *testing.T) { }, } - inputMetadata := map[string]any{} - metadata, err := ResolveSecretsForStrategy(ctx, authConfig, k8sClient, "default", inputMetadata) + inputStrategy := &authtypes.BackendAuthStrategy{} + strategy, err := ResolveSecretsForStrategy(ctx, authConfig, k8sClient, "default", inputStrategy) assert.Error(t, err) - assert.Nil(t, metadata, "should return nil on error") + assert.Nil(t, strategy, "should return nil on error") assert.Contains(t, err.Error(), "unsupported auth type") }) @@ -513,14 +514,16 @@ func TestResolveSecretsForStrategy(t *testing.T) { }, } - initialMetadata := map[string]any{ - "header_name": "X-API-Key", - "header_value_env": "TOOLHIVE_HEADER_INJECTION_VALUE", + initialStrategy := &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-API-Key", + }, } - metadata, err := ResolveSecretsForStrategy(ctx, authConfig, k8sClient, "default", initialMetadata) + strategy, err := ResolveSecretsForStrategy(ctx, authConfig, k8sClient, "default", initialStrategy) assert.Error(t, err) - assert.Nil(t, metadata, "should return nil on error") + assert.Nil(t, strategy, "should return nil on error") assert.Contains(t, err.Error(), "failed to get secret") }) } diff --git a/pkg/vmcp/auth/converters/token_exchange_test.go b/pkg/vmcp/auth/converters/token_exchange_test.go index 7220fb456..4da473a1a 100644 --- a/pkg/vmcp/auth/converters/token_exchange_test.go +++ b/pkg/vmcp/auth/converters/token_exchange_test.go @@ -13,22 +13,23 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/fake" mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" + authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" ) func TestTokenExchangeConverter_StrategyType(t *testing.T) { t.Parallel() converter := &TokenExchangeConverter{} - assert.Equal(t, "token_exchange", converter.StrategyType()) + assert.Equal(t, authtypes.StrategyTypeTokenExchange, converter.StrategyType()) } -func TestTokenExchangeConverter_ConvertToMetadata(t *testing.T) { +func TestTokenExchangeConverter_Convert(t *testing.T) { t.Parallel() tests := []struct { name string externalAuth *mcpv1alpha1.MCPExternalAuthConfig - wantMetadata map[string]any + wantStrategy *authtypes.BackendAuthStrategy wantErr bool errContains string }{ @@ -48,21 +49,22 @@ func TestTokenExchangeConverter_ConvertToMetadata(t *testing.T) { Name: "client-secret", Key: "secret", }, - Audience: "https://api.example.com", - Scopes: []string{"read", "write"}, - SubjectTokenType: "access_token", - ExternalTokenHeaderName: "X-Upstream-Token", + Audience: "https://api.example.com", + Scopes: []string{"read", "write"}, + SubjectTokenType: "access_token", }, }, }, - wantMetadata: map[string]any{ - "token_url": "https://auth.example.com/token", - "client_id": "test-client", - "client_secret_env": "TOOLHIVE_TOKEN_EXCHANGE_CLIENT_SECRET", - "audience": "https://api.example.com", - "scopes": []string{"read", "write"}, - "subject_token_type": "urn:ietf:params:oauth:token-type:access_token", - "external_token_header_name": "X-Upstream-Token", + wantStrategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "https://auth.example.com/token", + ClientID: "test-client", + ClientSecretEnv: "TOOLHIVE_TOKEN_EXCHANGE_CLIENT_SECRET", + Audience: "https://api.example.com", + Scopes: []string{"read", "write"}, + SubjectTokenType: "urn:ietf:params:oauth:token-type:access_token", + }, }, wantErr: false, }, @@ -80,8 +82,11 @@ func TestTokenExchangeConverter_ConvertToMetadata(t *testing.T) { }, }, }, - wantMetadata: map[string]any{ - "token_url": "https://auth.example.com/token", + wantStrategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "https://auth.example.com/token", + }, }, wantErr: false, }, @@ -101,10 +106,13 @@ func TestTokenExchangeConverter_ConvertToMetadata(t *testing.T) { }, }, }, - wantMetadata: map[string]any{ - "token_url": "https://auth.example.com/token", - "client_id": "test-client", - "audience": "https://api.example.com", + wantStrategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "https://auth.example.com/token", + ClientID: "test-client", + Audience: "https://api.example.com", + }, }, wantErr: false, }, @@ -123,9 +131,12 @@ func TestTokenExchangeConverter_ConvertToMetadata(t *testing.T) { }, }, }, - wantMetadata: map[string]any{ - "token_url": "https://auth.example.com/token", - "subject_token_type": "urn:ietf:params:oauth:token-type:id_token", + wantStrategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "https://auth.example.com/token", + SubjectTokenType: "urn:ietf:params:oauth:token-type:id_token", + }, }, wantErr: false, }, @@ -144,9 +155,12 @@ func TestTokenExchangeConverter_ConvertToMetadata(t *testing.T) { }, }, }, - wantMetadata: map[string]any{ - "token_url": "https://auth.example.com/token", - "subject_token_type": "urn:ietf:params:oauth:token-type:jwt", + wantStrategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "https://auth.example.com/token", + SubjectTokenType: "urn:ietf:params:oauth:token-type:jwt", + }, }, wantErr: false, }, @@ -165,9 +179,12 @@ func TestTokenExchangeConverter_ConvertToMetadata(t *testing.T) { }, }, }, - wantMetadata: map[string]any{ - "token_url": "https://auth.example.com/token", - "subject_token_type": "urn:ietf:params:oauth:token-type:access_token", + wantStrategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "https://auth.example.com/token", + SubjectTokenType: "urn:ietf:params:oauth:token-type:access_token", + }, }, wantErr: false, }, @@ -186,9 +203,12 @@ func TestTokenExchangeConverter_ConvertToMetadata(t *testing.T) { }, }, }, - wantMetadata: map[string]any{ - "token_url": "https://auth.example.com/token", - "scopes": []string{"openid", "profile", "email"}, + wantStrategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "https://auth.example.com/token", + Scopes: []string{"openid", "profile", "email"}, + }, }, wantErr: false, }, @@ -214,7 +234,7 @@ func TestTokenExchangeConverter_ConvertToMetadata(t *testing.T) { t.Parallel() converter := &TokenExchangeConverter{} - metadata, err := converter.ConvertToMetadata(tt.externalAuth) + strategy, err := converter.Convert(tt.externalAuth) if tt.wantErr { require.Error(t, err) @@ -225,7 +245,7 @@ func TestTokenExchangeConverter_ConvertToMetadata(t *testing.T) { } require.NoError(t, err) - assert.Equal(t, tt.wantMetadata, metadata) + assert.Equal(t, tt.wantStrategy, strategy) }) } } @@ -234,13 +254,13 @@ func TestTokenExchangeConverter_ResolveSecrets(t *testing.T) { t.Parallel() tests := []struct { - name string - externalAuth *mcpv1alpha1.MCPExternalAuthConfig - setupSecrets func(client.Client) error - inputMeta map[string]any - wantMetadata map[string]any - wantErr bool - errContains string + name string + externalAuth *mcpv1alpha1.MCPExternalAuthConfig + setupSecrets func(client.Client) error + inputStrategy *authtypes.BackendAuthStrategy + wantStrategy *authtypes.BackendAuthStrategy + wantErr bool + errContains string }{ { name: "successful secret resolution", @@ -273,20 +293,26 @@ func TestTokenExchangeConverter_ResolveSecrets(t *testing.T) { } return k8sClient.Create(context.Background(), secret) }, - inputMeta: map[string]any{ - "token_url": "https://auth.example.com/token", - "client_id": "test-client", - "client_secret_env": "TOOLHIVE_TOKEN_EXCHANGE_CLIENT_SECRET", + inputStrategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "https://auth.example.com/token", + ClientID: "test-client", + ClientSecretEnv: "TOOLHIVE_TOKEN_EXCHANGE_CLIENT_SECRET", + }, }, - wantMetadata: map[string]any{ - "token_url": "https://auth.example.com/token", - "client_id": "test-client", - "client_secret": "my-secret-value", + wantStrategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "https://auth.example.com/token", + ClientID: "test-client", + ClientSecret: "my-secret-value", + }, }, wantErr: false, }, { - name: "no-op when client_secret_env not present", + name: "no-op when ClientSecretEnv not present", externalAuth: &mcpv1alpha1.MCPExternalAuthConfig{ ObjectMeta: metav1.ObjectMeta{ Name: "test-auth", @@ -304,13 +330,19 @@ func TestTokenExchangeConverter_ResolveSecrets(t *testing.T) { }, }, }, - inputMeta: map[string]any{ - "token_url": "https://auth.example.com/token", - "client_id": "test-client", + inputStrategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "https://auth.example.com/token", + ClientID: "test-client", + }, }, - wantMetadata: map[string]any{ - "token_url": "https://auth.example.com/token", - "client_id": "test-client", + wantStrategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "https://auth.example.com/token", + ClientID: "test-client", + }, }, wantErr: false, }, @@ -328,11 +360,17 @@ func TestTokenExchangeConverter_ResolveSecrets(t *testing.T) { }, }, }, - inputMeta: map[string]any{ - "token_url": "https://auth.example.com/token", + inputStrategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "https://auth.example.com/token", + }, }, - wantMetadata: map[string]any{ - "token_url": "https://auth.example.com/token", + wantStrategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "https://auth.example.com/token", + }, }, wantErr: false, }, @@ -348,8 +386,11 @@ func TestTokenExchangeConverter_ResolveSecrets(t *testing.T) { TokenExchange: nil, }, }, - inputMeta: map[string]any{ - "client_secret_env": "TOOLHIVE_TOKEN_EXCHANGE_CLIENT_SECRET", + inputStrategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + ClientSecretEnv: "TOOLHIVE_TOKEN_EXCHANGE_CLIENT_SECRET", + }, }, wantErr: true, errContains: "token exchange config is nil", @@ -370,8 +411,11 @@ func TestTokenExchangeConverter_ResolveSecrets(t *testing.T) { }, }, }, - inputMeta: map[string]any{ - "client_secret_env": "TOOLHIVE_TOKEN_EXCHANGE_CLIENT_SECRET", + inputStrategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + ClientSecretEnv: "TOOLHIVE_TOKEN_EXCHANGE_CLIENT_SECRET", + }, }, wantErr: true, errContains: "clientSecretRef is nil", @@ -395,8 +439,11 @@ func TestTokenExchangeConverter_ResolveSecrets(t *testing.T) { }, }, }, - inputMeta: map[string]any{ - "client_secret_env": "TOOLHIVE_TOKEN_EXCHANGE_CLIENT_SECRET", + inputStrategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + ClientSecretEnv: "TOOLHIVE_TOKEN_EXCHANGE_CLIENT_SECRET", + }, }, wantErr: true, errContains: "failed to get secret", @@ -432,8 +479,11 @@ func TestTokenExchangeConverter_ResolveSecrets(t *testing.T) { } return k8sClient.Create(context.Background(), secret) }, - inputMeta: map[string]any{ - "client_secret_env": "TOOLHIVE_TOKEN_EXCHANGE_CLIENT_SECRET", + inputStrategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + ClientSecretEnv: "TOOLHIVE_TOKEN_EXCHANGE_CLIENT_SECRET", + }, }, wantErr: true, errContains: "does not contain key", @@ -457,12 +507,12 @@ func TestTokenExchangeConverter_ResolveSecrets(t *testing.T) { } converter := &TokenExchangeConverter{} - metadata, err := converter.ResolveSecrets( + strategy, err := converter.ResolveSecrets( context.Background(), tt.externalAuth, fakeClient, tt.externalAuth.Namespace, - tt.inputMeta, + tt.inputStrategy, ) if tt.wantErr { @@ -474,7 +524,7 @@ func TestTokenExchangeConverter_ResolveSecrets(t *testing.T) { } require.NoError(t, err) - assert.Equal(t, tt.wantMetadata, metadata) + assert.Equal(t, tt.wantStrategy, strategy) }) } } diff --git a/pkg/vmcp/auth/factory/integration_test.go b/pkg/vmcp/auth/factory/integration_test.go index ceb295ce5..2039f1620 100644 --- a/pkg/vmcp/auth/factory/integration_test.go +++ b/pkg/vmcp/auth/factory/integration_test.go @@ -16,11 +16,12 @@ import ( mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" "github.com/stacklok/toolhive/pkg/env" "github.com/stacklok/toolhive/pkg/vmcp/auth/converters" + authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" ) // TestHeaderInjectionIntegration validates the complete flow of: // 1. Creating a registry with header injection strategy -// 2. Converting MCPExternalAuthConfig to metadata +// 2. Converting MCPExternalAuthConfig to a typed BackendAuthStrategy // 3. Resolving secrets // 4. Using the strategy to authenticate a request func TestHeaderInjectionIntegration(t *testing.T) { @@ -75,35 +76,35 @@ func TestHeaderInjectionIntegration(t *testing.T) { }, } - // Step 4: Convert to metadata using the converter + // Step 4: Convert to typed BackendAuthStrategy using the converter converter := &converters.HeaderInjectionConverter{} - metadata, err := converter.ConvertToMetadata(authConfig) + strategy, err := converter.Convert(authConfig) require.NoError(t, err) - require.NotNil(t, metadata) + require.NotNil(t, strategy) - assert.Equal(t, "X-API-Key", metadata["header_name"]) + assert.Equal(t, "X-API-Key", strategy.HeaderInjection.HeaderName) // Step 5: Resolve secrets - resolvedMetadata, err := converter.ResolveSecrets(ctx, authConfig, fakeClient, "default", metadata) + resolvedStrategy, err := converter.ResolveSecrets(ctx, authConfig, fakeClient, "default", strategy) require.NoError(t, err) - require.NotNil(t, resolvedMetadata) + require.NotNil(t, resolvedStrategy) // Verify secret was resolved - assert.Equal(t, "X-API-Key", resolvedMetadata["header_name"]) - assert.Equal(t, "secret-api-key-12345", resolvedMetadata["header_value"]) + assert.Equal(t, "X-API-Key", resolvedStrategy.HeaderInjection.HeaderName) + assert.Equal(t, "secret-api-key-12345", resolvedStrategy.HeaderInjection.HeaderValue) // Step 6: Get the header injection strategy from registry - strategy, err := registry.GetStrategy("header_injection") + authStrategy, err := registry.GetStrategy(authtypes.StrategyTypeHeaderInjection) require.NoError(t, err) - require.NotNil(t, strategy) + require.NotNil(t, authStrategy) - // Step 7: Validate the metadata - err = strategy.Validate(resolvedMetadata) + // Step 7: Validate the typed BackendAuthStrategy + err = authStrategy.Validate(resolvedStrategy) require.NoError(t, err) // Step 8: Use the strategy to authenticate a request req := httptest.NewRequest(http.MethodGet, "/test", nil) - err = strategy.Authenticate(ctx, req, resolvedMetadata) + err = authStrategy.Authenticate(ctx, req, resolvedStrategy) require.NoError(t, err) // Step 9: Verify the header was injected @@ -160,18 +161,18 @@ func TestHeaderInjectionIntegration(t *testing.T) { // Convert and resolve converter := &converters.HeaderInjectionConverter{} - metadata, err := converter.ConvertToMetadata(authConfig) + strategy, err := converter.Convert(authConfig) require.NoError(t, err) - resolvedMetadata, err := converter.ResolveSecrets(ctx, authConfig, fakeClient, "default", metadata) + resolvedStrategy, err := converter.ResolveSecrets(ctx, authConfig, fakeClient, "default", strategy) require.NoError(t, err) - // Get strategy and authenticate - strategy, err := registry.GetStrategy("header_injection") + // Get auth strategy and authenticate + authStrategy, err := registry.GetStrategy(authtypes.StrategyTypeHeaderInjection) require.NoError(t, err) req := httptest.NewRequest(http.MethodGet, "/test", nil) - err = strategy.Authenticate(ctx, req, resolvedMetadata) + err = authStrategy.Authenticate(ctx, req, resolvedStrategy) require.NoError(t, err) // Verify custom header was injected @@ -213,16 +214,16 @@ func TestHeaderInjectionIntegration(t *testing.T) { // Convert should succeed (doesn't fetch secret yet) converter := &converters.HeaderInjectionConverter{} - metadata, err := converter.ConvertToMetadata(authConfig) + strategy, err := converter.Convert(authConfig) require.NoError(t, err) // Resolve secrets should fail - _, err = converter.ResolveSecrets(ctx, authConfig, fakeClient, "default", metadata) + _, err = converter.ResolveSecrets(ctx, authConfig, fakeClient, "default", strategy) require.Error(t, err) assert.Contains(t, err.Error(), "failed to get secret") }) - t.Run("header injection validates metadata before authentication", func(t *testing.T) { + t.Run("header injection validates typed strategy before authentication", func(t *testing.T) { t.Parallel() ctx := context.Background() @@ -233,23 +234,26 @@ func TestHeaderInjectionIntegration(t *testing.T) { require.NoError(t, err) // Get strategy - strategy, err := registry.GetStrategy("header_injection") + authStrategy, err := registry.GetStrategy(authtypes.StrategyTypeHeaderInjection) require.NoError(t, err) - // Test with invalid metadata (missing header_value) - invalidMetadata := map[string]any{ - "header_name": "X-API-Key", - // missing header_value + // Test with invalid typed strategy (missing header_value) + invalidStrategy := &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-API-Key", + // missing HeaderValue + }, } // Validate should fail - err = strategy.Validate(invalidMetadata) + err = authStrategy.Validate(invalidStrategy) require.Error(t, err) assert.Contains(t, err.Error(), "header_value") // Authenticate should also fail req := httptest.NewRequest(http.MethodGet, "/test", nil) - err = strategy.Authenticate(ctx, req, invalidMetadata) + err = authStrategy.Authenticate(ctx, req, invalidStrategy) require.Error(t, err) assert.Contains(t, err.Error(), "header_value") }) diff --git a/pkg/vmcp/auth/factory/outgoing_test.go b/pkg/vmcp/auth/factory/outgoing_test.go index 1e199d09c..9bde266ca 100644 --- a/pkg/vmcp/auth/factory/outgoing_test.go +++ b/pkg/vmcp/auth/factory/outgoing_test.go @@ -8,7 +8,7 @@ import ( "github.com/stretchr/testify/require" "github.com/stacklok/toolhive/pkg/env" - "github.com/stacklok/toolhive/pkg/vmcp/auth/strategies" + authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" ) func TestNewOutgoingAuthRegistry(t *testing.T) { @@ -25,13 +25,13 @@ func TestNewOutgoingAuthRegistry(t *testing.T) { require.NotNil(t, registry) // Verify all three strategies are registered - strategies := []string{ - strategies.StrategyTypeUnauthenticated, - strategies.StrategyTypeHeaderInjection, - strategies.StrategyTypeTokenExchange, + strategyTypes := []string{ + authtypes.StrategyTypeUnauthenticated, + authtypes.StrategyTypeHeaderInjection, + authtypes.StrategyTypeTokenExchange, } - for _, strategyType := range strategies { + for _, strategyType := range strategyTypes { strategy, err := registry.GetStrategy(strategyType) require.NoError(t, err, "strategy %s should be registered", strategyType) assert.NotNil(t, strategy, "strategy %s should not be nil", strategyType) @@ -65,28 +65,34 @@ func TestNewOutgoingAuthRegistry(t *testing.T) { require.NotNil(t, registry) // Get header injection strategy - strategy, err := registry.GetStrategy(strategies.StrategyTypeHeaderInjection) + strategy, err := registry.GetStrategy(authtypes.StrategyTypeHeaderInjection) require.NoError(t, err) require.NotNil(t, strategy) // Verify it's the correct type - assert.Equal(t, strategies.StrategyTypeHeaderInjection, strategy.Name()) - - // Verify it can validate metadata - validMetadata := map[string]any{ - "header_name": "X-API-Key", - "header_value": "test-key", + assert.Equal(t, authtypes.StrategyTypeHeaderInjection, strategy.Name()) + + // Verify it can validate typed strategy + validStrategy := &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-API-Key", + HeaderValue: "test-key", + }, } - err = strategy.Validate(validMetadata) - assert.NoError(t, err, "valid metadata should pass validation") - - // Verify it rejects invalid metadata - invalidMetadata := map[string]any{ - "header_name": "X-API-Key", - // missing header_value + err = strategy.Validate(validStrategy) + assert.NoError(t, err, "valid strategy should pass validation") + + // Verify it rejects invalid strategy (missing header_value) + invalidStrategy := &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-API-Key", + // missing HeaderValue + }, } - err = strategy.Validate(invalidMetadata) - assert.Error(t, err, "invalid metadata should fail validation") + err = strategy.Validate(invalidStrategy) + assert.Error(t, err, "invalid strategy should fail validation") assert.Contains(t, err.Error(), "header_value") }) @@ -101,12 +107,12 @@ func TestNewOutgoingAuthRegistry(t *testing.T) { require.NotNil(t, registry) // Get token exchange strategy - strategy, err := registry.GetStrategy(strategies.StrategyTypeTokenExchange) + strategy, err := registry.GetStrategy(authtypes.StrategyTypeTokenExchange) require.NoError(t, err) require.NotNil(t, strategy) // Verify it's the correct type - assert.Equal(t, strategies.StrategyTypeTokenExchange, strategy.Name()) + assert.Equal(t, authtypes.StrategyTypeTokenExchange, strategy.Name()) }) t.Run("unauthenticated strategy can be retrieved and used", func(t *testing.T) { @@ -120,19 +126,21 @@ func TestNewOutgoingAuthRegistry(t *testing.T) { require.NotNil(t, registry) // Get unauthenticated strategy - strategy, err := registry.GetStrategy(strategies.StrategyTypeUnauthenticated) + strategy, err := registry.GetStrategy(authtypes.StrategyTypeUnauthenticated) require.NoError(t, err) require.NotNil(t, strategy) // Verify it's the correct type - assert.Equal(t, strategies.StrategyTypeUnauthenticated, strategy.Name()) + assert.Equal(t, authtypes.StrategyTypeUnauthenticated, strategy.Name()) - // Verify it validates any metadata (no-op validation) + // Verify it validates any strategy config (no-op validation) err = strategy.Validate(nil) - assert.NoError(t, err, "unauthenticated strategy should accept nil metadata") + assert.NoError(t, err, "unauthenticated strategy should accept nil strategy") - err = strategy.Validate(map[string]any{}) - assert.NoError(t, err, "unauthenticated strategy should accept empty metadata") + err = strategy.Validate(&authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeUnauthenticated, + }) + assert.NoError(t, err, "unauthenticated strategy should accept empty strategy") }) t.Run("all strategies have correct names", func(t *testing.T) { @@ -150,9 +158,9 @@ func TestNewOutgoingAuthRegistry(t *testing.T) { strategyType string expectedName string }{ - {strategies.StrategyTypeUnauthenticated, "unauthenticated"}, - {strategies.StrategyTypeHeaderInjection, "header_injection"}, - {strategies.StrategyTypeTokenExchange, "token_exchange"}, + {authtypes.StrategyTypeUnauthenticated, "unauthenticated"}, + {authtypes.StrategyTypeHeaderInjection, "header_injection"}, + {authtypes.StrategyTypeTokenExchange, "token_exchange"}, } for _, tc := range testCases { diff --git a/pkg/vmcp/auth/strategies/header_injection_test.go b/pkg/vmcp/auth/strategies/header_injection_test.go index 86e70ef27..d7fb608c1 100644 --- a/pkg/vmcp/auth/strategies/header_injection_test.go +++ b/pkg/vmcp/auth/strategies/header_injection_test.go @@ -8,6 +8,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" ) func TestHeaderInjectionStrategy_Name(t *testing.T) { @@ -22,16 +24,19 @@ func TestHeaderInjectionStrategy_Authenticate(t *testing.T) { tests := []struct { name string - metadata map[string]any + strategy *authtypes.BackendAuthStrategy expectError bool errorContains string checkHeader func(t *testing.T, req *http.Request) }{ { name: "sets X-API-Key header correctly", - metadata: map[string]any{ - "header_name": "X-API-Key", - "header_value": "secret-key-123", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-API-Key", + HeaderValue: "secret-key-123", + }, }, expectError: false, checkHeader: func(t *testing.T, req *http.Request) { @@ -41,9 +46,12 @@ func TestHeaderInjectionStrategy_Authenticate(t *testing.T) { }, { name: "sets Authorization header with API key", - metadata: map[string]any{ - "header_name": "Authorization", - "header_value": "ApiKey my-secret-key", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "Authorization", + HeaderValue: "ApiKey my-secret-key", + }, }, expectError: false, checkHeader: func(t *testing.T, req *http.Request) { @@ -53,9 +61,12 @@ func TestHeaderInjectionStrategy_Authenticate(t *testing.T) { }, { name: "sets custom header name", - metadata: map[string]any{ - "header_name": "X-Custom-Auth-Token", - "header_value": "custom-token-value", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-Custom-Auth-Token", + HeaderValue: "custom-token-value", + }, }, expectError: false, checkHeader: func(t *testing.T, req *http.Request) { @@ -65,9 +76,12 @@ func TestHeaderInjectionStrategy_Authenticate(t *testing.T) { }, { name: "handles complex header values", - metadata: map[string]any{ - "header_name": "X-API-Key", - "header_value": "Bearer eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIn0.test", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-API-Key", + HeaderValue: "Bearer eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIn0.test", + }, }, expectError: false, checkHeader: func(t *testing.T, req *http.Request) { @@ -78,9 +92,12 @@ func TestHeaderInjectionStrategy_Authenticate(t *testing.T) { }, { name: "handles header value with special characters", - metadata: map[string]any{ - "header_name": "X-API-Key", - "header_value": "key-with-!@#$%^&*()-_=+[]{}|;:,.<>?", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-API-Key", + HeaderValue: "key-with-!@#$%^&*()-_=+[]{}|;:,.<>?", + }, }, expectError: false, checkHeader: func(t *testing.T, req *http.Request) { @@ -88,97 +105,75 @@ func TestHeaderInjectionStrategy_Authenticate(t *testing.T) { assert.Equal(t, "key-with-!@#$%^&*()-_=+[]{}|;:,.<>?", req.Header.Get("X-API-Key")) }, }, - { - name: "ignores additional metadata fields", - metadata: map[string]any{ - "header_name": "X-API-Key", - "header_value": "my-key", - "extra_field": "ignored", - "another": 123, - }, - expectError: false, - checkHeader: func(t *testing.T, req *http.Request) { - t.Helper() - assert.Equal(t, "my-key", req.Header.Get("X-API-Key")) - }, - }, { name: "returns error when header_name is missing", - metadata: map[string]any{ - "header_value": "my-key", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderValue: "my-key", + }, }, expectError: true, errorContains: "header_name required", }, { name: "returns error when header_name is empty string", - metadata: map[string]any{ - "header_name": "", - "header_value": "my-key", - }, - expectError: true, - errorContains: "header_name required", - }, - { - name: "returns error when header_name is not a string", - metadata: map[string]any{ - "header_name": 123, - "header_value": "my-key", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "", + HeaderValue: "my-key", + }, }, expectError: true, errorContains: "header_name required", }, { name: "returns error when header_value is missing", - metadata: map[string]any{ - "header_name": "X-API-Key", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-API-Key", + }, }, expectError: true, errorContains: "header_value required", }, { - name: "returns error when api_key is empty string", - metadata: map[string]any{ - "header_name": "X-API-Key", - "header_value": "", + name: "returns error when header_value is empty string", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-API-Key", + HeaderValue: "", + }, }, expectError: true, errorContains: "header_value required", }, { - name: "returns error when header_value is not a string", - metadata: map[string]any{ - "header_name": "X-API-Key", - "header_value": 123, - }, + name: "returns error when strategy is nil", + strategy: nil, expectError: true, - errorContains: "header_value required", + errorContains: "header_injection configuration required", }, { - name: "returns error when metadata is nil", - metadata: nil, - expectError: true, - errorContains: "header_name required", - }, - { - name: "returns error when metadata is empty", - metadata: map[string]any{}, - expectError: true, - errorContains: "header_name required", - }, - { - name: "returns error when both fields are missing", - metadata: map[string]any{ - "unrelated": "field", + name: "returns error when HeaderInjection config is nil", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: nil, }, expectError: true, - errorContains: "header_name required", + errorContains: "header_injection configuration required", }, { name: "overwrites existing header value", - metadata: map[string]any{ - "header_name": "X-API-Key", - "header_value": "new-key", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-API-Key", + HeaderValue: "new-key", + }, }, expectError: false, checkHeader: func(t *testing.T, req *http.Request) { @@ -189,9 +184,12 @@ func TestHeaderInjectionStrategy_Authenticate(t *testing.T) { }, { name: "handles very long header values", - metadata: map[string]any{ - "header_name": "X-API-Key", - "header_value": string(make([]byte, 10000)) + "very-long-key", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-API-Key", + HeaderValue: string(make([]byte, 10000)) + "very-long-key", + }, }, expectError: false, checkHeader: func(t *testing.T, req *http.Request) { @@ -202,9 +200,12 @@ func TestHeaderInjectionStrategy_Authenticate(t *testing.T) { }, { name: "handles case-sensitive header names", - metadata: map[string]any{ - "header_name": "x-api-key", // lowercase - "header_value": "my-key", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "x-api-key", // lowercase + HeaderValue: "my-key", + }, }, expectError: false, checkHeader: func(t *testing.T, req *http.Request) { @@ -229,7 +230,7 @@ func TestHeaderInjectionStrategy_Authenticate(t *testing.T) { req.Header.Set("X-API-Key", "old-key") } - err := strategy.Authenticate(ctx, req, tt.metadata) + err := strategy.Authenticate(ctx, req, tt.strategy) if tt.expectError { require.Error(t, err) @@ -250,141 +251,113 @@ func TestHeaderInjectionStrategy_Validate(t *testing.T) { tests := []struct { name string - metadata map[string]any + strategy *authtypes.BackendAuthStrategy expectError bool errorContains string }{ { - name: "valid metadata with all required fields", - metadata: map[string]any{ - "header_name": "X-API-Key", - "header_value": "secret-key", - }, - expectError: false, - }, - { - name: "valid with extra metadata fields", - metadata: map[string]any{ - "header_name": "X-API-Key", - "header_value": "secret-key", - "extra": "ignored", - "count": 123, + name: "valid strategy with all required fields", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-API-Key", + HeaderValue: "secret-key", + }, }, expectError: false, }, { name: "valid with different header name", - metadata: map[string]any{ - "header_name": "Authorization", - "header_value": "Bearer token", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "Authorization", + HeaderValue: "Bearer token", + }, }, expectError: false, }, { name: "returns error when header_name is missing", - metadata: map[string]any{ - "header_value": "secret-key", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderValue: "secret-key", + }, }, expectError: true, errorContains: "header_name required", }, { name: "returns error when header_name is empty", - metadata: map[string]any{ - "header_name": "", - "header_value": "secret-key", - }, - expectError: true, - errorContains: "header_name required", - }, - { - name: "returns error when header_name is not a string", - metadata: map[string]any{ - "header_name": 123, - "header_value": "secret-key", - }, - expectError: true, - errorContains: "header_name required", - }, - { - name: "returns error when header_name is a boolean", - metadata: map[string]any{ - "header_name": true, - "header_value": "secret-key", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "", + HeaderValue: "secret-key", + }, }, expectError: true, errorContains: "header_name required", }, { name: "returns error when header_value is missing", - metadata: map[string]any{ - "header_name": "X-API-Key", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-API-Key", + }, }, expectError: true, errorContains: "header_value required", }, { name: "returns error when header_value is empty", - metadata: map[string]any{ - "header_name": "X-API-Key", - "header_value": "", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-API-Key", + HeaderValue: "", + }, }, expectError: true, errorContains: "header_value required", }, { - name: "returns error when header_value is not a string", - metadata: map[string]any{ - "header_name": "X-API-Key", - "header_value": 123, - }, + name: "returns error when strategy is nil", + strategy: nil, expectError: true, - errorContains: "header_value required", + errorContains: "header_injection configuration required", }, { - name: "returns error when header_value is a map", - metadata: map[string]any{ - "header_name": "X-API-Key", - "header_value": map[string]any{"nested": "value"}, + name: "returns error when HeaderInjection config is nil", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: nil, }, expectError: true, - errorContains: "header_value required", - }, - { - name: "returns error when metadata is nil", - metadata: nil, - expectError: true, - errorContains: "header_name required", - }, - { - name: "returns error when metadata is empty", - metadata: map[string]any{}, - expectError: true, - errorContains: "header_name required", - }, - { - name: "returns error when both fields are wrong type", - metadata: map[string]any{ - "header_name": 123, - "header_value": false, - }, - expectError: true, - errorContains: "header_name required", + errorContains: "header_injection configuration required", }, { name: "returns error for whitespace in header_name", - metadata: map[string]any{ - "header_name": "X-Custom Header", - "header_value": "key", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-Custom Header", + HeaderValue: "key", + }, }, expectError: true, errorContains: "invalid header_name", }, { name: "accepts unicode in header_value", - metadata: map[string]any{ - "header_name": "X-API-Key", - "header_value": "key-with-unicode-日本語", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-API-Key", + HeaderValue: "key-with-unicode-日本語", + }, }, expectError: false, }, @@ -395,7 +368,7 @@ func TestHeaderInjectionStrategy_Validate(t *testing.T) { t.Parallel() strategy := NewHeaderInjectionStrategy() - err := strategy.Validate(tt.metadata) + err := strategy.Validate(tt.strategy) if tt.expectError { require.Error(t, err) diff --git a/pkg/vmcp/auth/strategies/tokenexchange_test.go b/pkg/vmcp/auth/strategies/tokenexchange_test.go index a4a5bfb85..8153f8ad3 100644 --- a/pkg/vmcp/auth/strategies/tokenexchange_test.go +++ b/pkg/vmcp/auth/strategies/tokenexchange_test.go @@ -13,6 +13,7 @@ import ( "github.com/stacklok/toolhive/pkg/auth" "github.com/stacklok/toolhive/pkg/env/mocks" + authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" ) // Test helpers for reducing boilerplate @@ -68,7 +69,8 @@ func TestTokenExchangeStrategy_Authenticate(t *testing.T) { tests := []struct { name string setupCtx func() context.Context - metadata map[string]any + strategy *authtypes.BackendAuthStrategy + tokenURLOverride string // If set, replace the strategy's TokenURL with this server URL setupServer func() *httptest.Server expectError bool errorContains string @@ -84,6 +86,13 @@ func TestTokenExchangeStrategy_Authenticate(t *testing.T) { assert.Equal(t, "client-token", r.Form.Get("subject_token")) }) }, + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "PLACEHOLDER", // Will be replaced with server URL + }, + }, + tokenURLOverride: "PLACEHOLDER", expectError: false, checkAuthHeader: func(t *testing.T, req *http.Request) { t.Helper() @@ -99,9 +108,14 @@ func TestTokenExchangeStrategy_Authenticate(t *testing.T) { assert.Equal(t, "https://backend.example.com", r.Form.Get("audience")) }) }, - metadata: map[string]any{ - "audience": "https://backend.example.com", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "PLACEHOLDER", + Audience: "https://backend.example.com", + }, }, + tokenURLOverride: "PLACEHOLDER", expectError: false, }, { @@ -113,9 +127,14 @@ func TestTokenExchangeStrategy_Authenticate(t *testing.T) { assert.Equal(t, "read write", r.Form.Get("scope")) }) }, - metadata: map[string]any{ - "scopes": []string{"read", "write"}, + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "PLACEHOLDER", + Scopes: []string{"read", "write"}, + }, }, + tokenURLOverride: "PLACEHOLDER", expectError: false, }, { @@ -137,10 +156,15 @@ func TestTokenExchangeStrategy_Authenticate(t *testing.T) { }) })) }, - metadata: map[string]any{ - "client_id": "client-id", - "client_secret": "client-secret", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "PLACEHOLDER", + ClientID: "client-id", + ClientSecret: "client-secret", + }, }, + tokenURLOverride: "PLACEHOLDER", expectError: false, }, { @@ -151,6 +175,13 @@ func TestTokenExchangeStrategy_Authenticate(t *testing.T) { t.Error("Server should not be called") })) }, + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "PLACEHOLDER", + }, + }, + tokenURLOverride: "PLACEHOLDER", expectError: true, errorContains: "no identity", }, @@ -162,16 +193,23 @@ func TestTokenExchangeStrategy_Authenticate(t *testing.T) { t.Error("Server should not be called") })) }, + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "PLACEHOLDER", + }, + }, + tokenURLOverride: "PLACEHOLDER", expectError: true, errorContains: "no token", }, { - name: "returns error when metadata is invalid", + name: "returns error when strategy is nil", setupCtx: func() context.Context { return createContextWithIdentity("metadata-test-user", "metadata-token") }, - setupServer: nil, // No server needed for validation error - metadata: map[string]any{}, // Missing token_url + setupServer: nil, // No server needed for validation error + strategy: nil, expectError: true, - errorContains: "invalid metadata", + errorContains: "token_exchange configuration required", }, { name: "returns error when token exchange fails", @@ -185,6 +223,13 @@ func TestTokenExchangeStrategy_Authenticate(t *testing.T) { }) })) }, + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "PLACEHOLDER", + }, + }, + tokenURLOverride: "PLACEHOLDER", expectError: true, errorContains: "token exchange failed", }, @@ -200,6 +245,13 @@ func TestTokenExchangeStrategy_Authenticate(t *testing.T) { }) })) }, + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "PLACEHOLDER", + }, + }, + tokenURLOverride: "PLACEHOLDER", expectError: true, errorContains: "empty access_token", }, @@ -219,16 +271,27 @@ func TestTokenExchangeStrategy_Authenticate(t *testing.T) { strategy := NewTokenExchangeStrategy(mockEnv) ctx := tt.setupCtx() - metadata := tt.metadata - if metadata == nil { - metadata = map[string]any{} - } - if server != nil { - metadata["token_url"] = server.URL + // Create a copy of the strategy config and set the server URL + var authStrategy *authtypes.BackendAuthStrategy + if tt.strategy != nil && server != nil && tt.tokenURLOverride != "" { + authStrategy = &authtypes.BackendAuthStrategy{ + Type: tt.strategy.Type, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: server.URL, + ClientID: tt.strategy.TokenExchange.ClientID, + ClientSecret: tt.strategy.TokenExchange.ClientSecret, + ClientSecretEnv: tt.strategy.TokenExchange.ClientSecretEnv, + Audience: tt.strategy.TokenExchange.Audience, + Scopes: tt.strategy.TokenExchange.Scopes, + SubjectTokenType: tt.strategy.TokenExchange.SubjectTokenType, + }, + } + } else { + authStrategy = tt.strategy } req := httptest.NewRequest(http.MethodGet, "/test", nil) - err := strategy.Authenticate(ctx, req, metadata) + err := strategy.Authenticate(ctx, req, authStrategy) if tt.expectError { require.Error(t, err) @@ -247,30 +310,87 @@ func TestTokenExchangeStrategy_Authenticate(t *testing.T) { func TestTokenExchangeStrategy_Validate(t *testing.T) { t.Parallel() - validBaseMetadata := map[string]any{"token_url": "https://auth.example.com/token"} - tests := []struct { name string - metadata map[string]any + strategy *authtypes.BackendAuthStrategy expectError string // empty means no error expected }{ - {name: "valid with only token_url", metadata: validBaseMetadata}, - {name: "valid with all fields", metadata: map[string]any{ - "token_url": "https://auth.example.com/token", "client_id": "my-client", - "client_secret": "my-secret", "audience": "https://backend.example.com", - "scopes": []string{"read", "write"}, "subject_token_type": "access_token", - }}, - {name: "valid with string scopes", metadata: map[string]any{"token_url": "https://auth.example.com/token", "scopes": "read"}}, - {name: "valid with id_token type", metadata: map[string]any{"token_url": "https://auth.example.com/token", "subject_token_type": "id_token"}}, - {name: "valid with client_id only", metadata: map[string]any{"token_url": "https://auth.example.com/token", "client_id": "my-client"}}, - {name: "valid with extra fields", metadata: map[string]any{"token_url": "https://auth.example.com/token", "extra": "ignored"}}, - {name: "error on missing token_url", metadata: map[string]any{}, expectError: "token_url required"}, - {name: "error on nil metadata", metadata: nil, expectError: "token_url required"}, - {name: "error on client_secret without client_id", metadata: map[string]any{"token_url": "https://auth.example.com/token", "client_secret": "secret"}, expectError: "client_secret cannot be provided without client_id"}, - {name: "error on client_secret_env without client_id", metadata: map[string]any{"token_url": "https://auth.example.com/token", "client_secret_env": "TEST_SECRET"}, expectError: "client_secret_env cannot be provided without client_id"}, - {name: "error on invalid scopes type", metadata: map[string]any{"token_url": "https://auth.example.com/token", "scopes": 123}, expectError: "scopes must be a string or array"}, - {name: "error on non-string in scopes", metadata: map[string]any{"token_url": "https://auth.example.com/token", "scopes": []any{"read", 123}}, expectError: "scopes[1] must be a string"}, - {name: "error on invalid token type", metadata: map[string]any{"token_url": "https://auth.example.com/token", "subject_token_type": "invalid"}, expectError: "invalid subject_token_type"}, + { + name: "valid with only token_url", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "https://auth.example.com/token", + }, + }, + }, + { + name: "valid with all fields", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "https://auth.example.com/token", + ClientID: "my-client", + ClientSecret: "my-secret", + Audience: "https://backend.example.com", + Scopes: []string{"read", "write"}, + SubjectTokenType: "urn:ietf:params:oauth:token-type:access_token", + }, + }, + }, + { + name: "valid with client_id only", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "https://auth.example.com/token", + ClientID: "my-client", + }, + }, + }, + { + name: "error on nil strategy", + strategy: nil, + expectError: "token_exchange configuration required", + }, + { + name: "error on nil TokenExchange config", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: nil, + }, + expectError: "token_exchange configuration required", + }, + { + name: "error on missing token_url", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{}, + }, + expectError: "token_url required", + }, + { + name: "error on client_secret without client_id", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "https://auth.example.com/token", + ClientSecret: "secret", + }, + }, + expectError: "client_secret cannot be provided without client_id", + }, + { + name: "error on client_secret_env without client_id", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "https://auth.example.com/token", + ClientSecretEnv: "TEST_SECRET", + }, + }, + expectError: "client_secret_env cannot be provided without client_id", + }, } for _, tt := range tests { @@ -278,7 +398,7 @@ func TestTokenExchangeStrategy_Validate(t *testing.T) { t.Parallel() mockEnv := createMockEnvReader(t) strategy := NewTokenExchangeStrategy(mockEnv) - err := strategy.Validate(tt.metadata) + err := strategy.Validate(tt.strategy) if tt.expectError != "" { require.Error(t, err) @@ -305,22 +425,28 @@ func TestTokenExchangeStrategy_CacheSeparation(t *testing.T) { ctx := createContextWithIdentity("cache-test-user", "test-token") // First request with "read" scope - metadata1 := map[string]any{ - "token_url": server1.URL, - "scopes": []string{"read"}, + authStrategy1 := &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: server1.URL, + Scopes: []string{"read"}, + }, } req1 := httptest.NewRequest(http.MethodGet, "/test", nil) - err := strategy.Authenticate(ctx, req1, metadata1) + err := strategy.Authenticate(ctx, req1, authStrategy1) require.NoError(t, err) assert.Equal(t, "Bearer token-scope-read", req1.Header.Get("Authorization")) // Second request with "write" scope - should use different cache entry - metadata2 := map[string]any{ - "token_url": server2.URL, - "scopes": []string{"write"}, + authStrategy2 := &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: server2.URL, + Scopes: []string{"write"}, + }, } req2 := httptest.NewRequest(http.MethodGet, "/test", nil) - err = strategy.Authenticate(ctx, req2, metadata2) + err = strategy.Authenticate(ctx, req2, authStrategy2) require.NoError(t, err) assert.Equal(t, "Bearer token-scope-write", req2.Header.Get("Authorization")) @@ -351,21 +477,27 @@ func TestTokenExchangeStrategy_CacheHitWithDifferentScopeOrder(t *testing.T) { ctx := createContextWithIdentity("scope-order-user", "test-token") // First request with scopes in one order - metadata1 := map[string]any{ - "token_url": server.URL, - "scopes": []string{"write", "read", "admin"}, + authStrategy1 := &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: server.URL, + Scopes: []string{"write", "read", "admin"}, + }, } req1 := httptest.NewRequest(http.MethodGet, "/test", nil) - err := strategy.Authenticate(ctx, req1, metadata1) + err := strategy.Authenticate(ctx, req1, authStrategy1) require.NoError(t, err) // Second request with same scopes in different order - metadata2 := map[string]any{ - "token_url": server.URL, - "scopes": []string{"admin", "read", "write"}, + authStrategy2 := &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: server.URL, + Scopes: []string{"admin", "read", "write"}, + }, } req2 := httptest.NewRequest(http.MethodGet, "/test", nil) - err = strategy.Authenticate(ctx, req2, metadata2) + err = strategy.Authenticate(ctx, req2, authStrategy2) require.NoError(t, err) // Note: callCount will be 2 since we don't cache per-user tokens at this layer @@ -388,21 +520,24 @@ func TestTokenExchangeStrategy_SharedConfigAcrossUsers(t *testing.T) { mockEnv := createMockEnvReader(t) strategy := NewTokenExchangeStrategy(mockEnv) - metadata := map[string]any{ - "token_url": server.URL, - "scopes": []string{"read", "write"}, + authStrategy := &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: server.URL, + Scopes: []string{"read", "write"}, + }, } // First user makes a request ctx1 := createContextWithIdentity("user1", "user1-token") req1 := httptest.NewRequest(http.MethodGet, "/test", nil) - err := strategy.Authenticate(ctx1, req1, metadata) + err := strategy.Authenticate(ctx1, req1, authStrategy) require.NoError(t, err) // Second user makes a request with same config ctx2 := createContextWithIdentity("user2", "user2-token") req2 := httptest.NewRequest(http.MethodGet, "/test", nil) - err = strategy.Authenticate(ctx2, req2, metadata) + err = strategy.Authenticate(ctx2, req2, authStrategy) require.NoError(t, err) // Verify only one ExchangeConfig cache entry exists (shared across users) @@ -434,21 +569,24 @@ func TestTokenExchangeStrategy_CurrentTokenUsed(t *testing.T) { mockEnv := createMockEnvReader(t) strategy := NewTokenExchangeStrategy(mockEnv) - metadata := map[string]any{ - "token_url": server.URL, + authStrategy := &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: server.URL, + }, } // First request with initial token ctx1 := createContextWithIdentity("user1", "initial-token") req1 := httptest.NewRequest(http.MethodGet, "/test", nil) - err := strategy.Authenticate(ctx1, req1, metadata) + err := strategy.Authenticate(ctx1, req1, authStrategy) require.NoError(t, err) assert.Equal(t, "initial-token", capturedToken, "Should use initial token") // Second request with refreshed token (simulating token refresh) ctx2 := createContextWithIdentity("user1", "refreshed-token") req2 := httptest.NewRequest(http.MethodGet, "/test", nil) - err = strategy.Authenticate(ctx2, req2, metadata) + err = strategy.Authenticate(ctx2, req2, authStrategy) require.NoError(t, err) assert.Equal(t, "refreshed-token", capturedToken, "Should use current refreshed token, not cached stale token") } @@ -459,7 +597,7 @@ func TestTokenExchangeStrategy_ClientSecretEnv(t *testing.T) { tests := []struct { name string setupMock func(t *testing.T, mockEnv *mocks.MockReader) - metadata map[string]any + strategy *authtypes.BackendAuthStrategy expectError bool errorContains string validateAuth func(t *testing.T, r *http.Request) @@ -470,9 +608,13 @@ func TestTokenExchangeStrategy_ClientSecretEnv(t *testing.T) { t.Helper() mockEnv.EXPECT().Getenv("TEST_CLIENT_SECRET").Return("secret-from-env").AnyTimes() }, - metadata: map[string]any{ - "client_id": "test-client", - "client_secret_env": "TEST_CLIENT_SECRET", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "PLACEHOLDER", + ClientID: "test-client", + ClientSecretEnv: "TEST_CLIENT_SECRET", + }, }, expectError: false, validateAuth: func(t *testing.T, r *http.Request) { @@ -489,9 +631,13 @@ func TestTokenExchangeStrategy_ClientSecretEnv(t *testing.T) { t.Helper() mockEnv.EXPECT().Getenv("MISSING_ENV_VAR").Return("").AnyTimes() }, - metadata: map[string]any{ - "client_id": "test-client", - "client_secret_env": "MISSING_ENV_VAR", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "https://auth.example.com/token", + ClientID: "test-client", + ClientSecretEnv: "MISSING_ENV_VAR", + }, }, expectError: true, errorContains: "environment variable MISSING_ENV_VAR not set", @@ -502,9 +648,13 @@ func TestTokenExchangeStrategy_ClientSecretEnv(t *testing.T) { t.Helper() mockEnv.EXPECT().Getenv("EMPTY_SECRET").Return("").AnyTimes() }, - metadata: map[string]any{ - "client_id": "test-client", - "client_secret_env": "EMPTY_SECRET", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "https://auth.example.com/token", + ClientID: "test-client", + ClientSecretEnv: "EMPTY_SECRET", + }, }, expectError: true, errorContains: "environment variable EMPTY_SECRET not set or empty", @@ -515,10 +665,14 @@ func TestTokenExchangeStrategy_ClientSecretEnv(t *testing.T) { t.Helper() // Mock should not be called since client_secret takes precedence }, - metadata: map[string]any{ - "client_id": "test-client", - "client_secret": "direct-secret", - "client_secret_env": "TEST_SECRET_ENV", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "PLACEHOLDER", + ClientID: "test-client", + ClientSecret: "direct-secret", + ClientSecretEnv: "TEST_SECRET_ENV", + }, }, expectError: false, validateAuth: func(t *testing.T, r *http.Request) { @@ -543,14 +697,7 @@ func TestTokenExchangeStrategy_ClientSecretEnv(t *testing.T) { strategy := NewTokenExchangeStrategy(mockEnv) - // Validation test - metadata := tt.metadata - if metadata == nil { - metadata = map[string]any{} - } - metadata["token_url"] = "https://auth.example.com/token" - - err := strategy.Validate(metadata) + err := strategy.Validate(tt.strategy) if tt.expectError { require.Error(t, err) @@ -573,11 +720,20 @@ func TestTokenExchangeStrategy_ClientSecretEnv(t *testing.T) { })) defer server.Close() - metadata["token_url"] = server.URL + // Create a new strategy with the server URL + authStrategy := &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: server.URL, + ClientID: tt.strategy.TokenExchange.ClientID, + ClientSecret: tt.strategy.TokenExchange.ClientSecret, + ClientSecretEnv: tt.strategy.TokenExchange.ClientSecretEnv, + }, + } ctx := createContextWithIdentity("test-user", "user-token") req := httptest.NewRequest(http.MethodGet, "/test", nil) - err := strategy.Authenticate(ctx, req, metadata) + err := strategy.Authenticate(ctx, req, authStrategy) require.NoError(t, err) } }) diff --git a/pkg/vmcp/auth/strategies/unauthenticated_test.go b/pkg/vmcp/auth/strategies/unauthenticated_test.go index 43ee62bb2..bf63f9416 100644 --- a/pkg/vmcp/auth/strategies/unauthenticated_test.go +++ b/pkg/vmcp/auth/strategies/unauthenticated_test.go @@ -8,6 +8,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" ) func TestUnauthenticatedStrategy_Name(t *testing.T) { @@ -22,13 +24,13 @@ func TestUnauthenticatedStrategy_Authenticate(t *testing.T) { tests := []struct { name string - metadata map[string]any + strategy *authtypes.BackendAuthStrategy setupRequest func() *http.Request checkRequest func(t *testing.T, req *http.Request) }{ { - name: "does not modify request with no metadata", - metadata: nil, + name: "does not modify request with nil strategy", + strategy: nil, setupRequest: func() *http.Request { req := httptest.NewRequest(http.MethodGet, "http://backend.example.com/test", nil) req.Header.Set("X-Custom-Header", "original-value") @@ -43,10 +45,9 @@ func TestUnauthenticatedStrategy_Authenticate(t *testing.T) { }, }, { - name: "does not modify request with metadata present", - metadata: map[string]any{ - "some_key": "some_value", - "count": 42, + name: "does not modify request with unauthenticated strategy", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeUnauthenticated, }, setupRequest: func() *http.Request { req := httptest.NewRequest(http.MethodGet, "http://backend.example.com/test", nil) @@ -63,7 +64,7 @@ func TestUnauthenticatedStrategy_Authenticate(t *testing.T) { }, { name: "preserves existing Authorization header", - metadata: nil, + strategy: nil, setupRequest: func() *http.Request { req := httptest.NewRequest(http.MethodGet, "http://backend.example.com/test", nil) req.Header.Set("Authorization", "Bearer existing-token") @@ -77,7 +78,7 @@ func TestUnauthenticatedStrategy_Authenticate(t *testing.T) { }, { name: "works with empty request", - metadata: nil, + strategy: nil, setupRequest: func() *http.Request { return httptest.NewRequest(http.MethodGet, "http://backend.example.com/test", nil) }, @@ -99,7 +100,7 @@ func TestUnauthenticatedStrategy_Authenticate(t *testing.T) { req := tt.setupRequest() ctx := context.Background() - err := strategy.Authenticate(ctx, req, tt.metadata) + err := strategy.Authenticate(ctx, req, tt.strategy) require.NoError(t, err) tt.checkRequest(t, req) @@ -112,31 +113,16 @@ func TestUnauthenticatedStrategy_Validate(t *testing.T) { tests := []struct { name string - metadata map[string]any + strategy *authtypes.BackendAuthStrategy }{ { - name: "accepts nil metadata", - metadata: nil, - }, - { - name: "accepts empty metadata", - metadata: map[string]any{}, - }, - { - name: "accepts arbitrary metadata", - metadata: map[string]any{ - "key1": "value1", - "key2": 42, - "key3": []string{"a", "b", "c"}, - "nested": map[string]any{"inner": "value"}, - }, + name: "accepts nil strategy", + strategy: nil, }, { - name: "accepts metadata with typical auth fields", - metadata: map[string]any{ - "token_url": "https://example.com/token", - "client_id": "client-123", - "header_name": "X-API-Key", + name: "accepts unauthenticated strategy", + strategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeUnauthenticated, }, }, } @@ -146,7 +132,7 @@ func TestUnauthenticatedStrategy_Validate(t *testing.T) { t.Parallel() strategy := NewUnauthenticatedStrategy() - err := strategy.Validate(tt.metadata) + err := strategy.Validate(tt.strategy) require.NoError(t, err) }) diff --git a/pkg/vmcp/client/client_test.go b/pkg/vmcp/client/client_test.go index 1f524866e..0f3f953a9 100644 --- a/pkg/vmcp/client/client_test.go +++ b/pkg/vmcp/client/client_test.go @@ -18,6 +18,7 @@ import ( "github.com/stacklok/toolhive/pkg/vmcp/auth" authmocks "github.com/stacklok/toolhive/pkg/vmcp/auth/mocks" "github.com/stacklok/toolhive/pkg/vmcp/auth/strategies" + authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" ) func TestHTTPBackendClient_ListCapabilities_WithMockFactory(t *testing.T) { @@ -270,9 +271,14 @@ func TestAuthRoundTripper_RoundTrip(t *testing.T) { { name: "successful authentication adds headers and forwards request", target: &vmcp.BackendTarget{ - WorkloadID: "backend-1", - AuthStrategy: "header_injection", - AuthMetadata: map[string]any{"key": "value"}, + WorkloadID: "backend-1", + AuthConfig: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "Authorization", + HeaderValue: "Bearer test-token", + }, + }, }, setupStrategy: func(ctrl *gomock.Controller) auth.Strategy { mockStrategy := authmocks.NewMockStrategy(ctrl) @@ -284,9 +290,9 @@ func TestAuthRoundTripper_RoundTrip(t *testing.T) { Authenticate( gomock.Any(), gomock.Any(), - map[string]any{"key": "value"}, + gomock.Any(), ). - DoAndReturn(func(_ context.Context, req *http.Request, _ map[string]any) error { + DoAndReturn(func(_ context.Context, req *http.Request, _ *authtypes.BackendAuthStrategy) error { // Simulate adding auth header req.Header.Set("Authorization", "Bearer test-token") return nil @@ -311,9 +317,10 @@ func TestAuthRoundTripper_RoundTrip(t *testing.T) { { name: "unauthenticated strategy skips authentication", target: &vmcp.BackendTarget{ - WorkloadID: "backend-1", - AuthStrategy: "unauthenticated", - AuthMetadata: nil, + WorkloadID: "backend-1", + AuthConfig: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeUnauthenticated, + }, }, setupStrategy: func(ctrl *gomock.Controller) auth.Strategy { mockStrategy := authmocks.NewMockStrategy(ctrl) @@ -325,9 +332,9 @@ func TestAuthRoundTripper_RoundTrip(t *testing.T) { Authenticate( gomock.Any(), gomock.Any(), - gomock.Nil(), + gomock.Any(), ). - DoAndReturn(func(_ context.Context, _ *http.Request, _ map[string]any) error { + DoAndReturn(func(_ context.Context, _ *http.Request, _ *authtypes.BackendAuthStrategy) error { // UnauthenticatedStrategy does nothing return nil }) @@ -350,21 +357,26 @@ func TestAuthRoundTripper_RoundTrip(t *testing.T) { { name: "authentication failure returns error without calling base transport", target: &vmcp.BackendTarget{ - WorkloadID: "backend-1", - AuthStrategy: "header_injection", - AuthMetadata: map[string]any{"key": "value"}, + WorkloadID: "backend-1", + AuthConfig: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-API-Key", + HeaderValue: "test-key", + }, + }, }, setupStrategy: func(ctrl *gomock.Controller) auth.Strategy { mockStrategy := authmocks.NewMockStrategy(ctrl) mockStrategy.EXPECT(). Name(). - Return("header_injection"). + Return(authtypes.StrategyTypeHeaderInjection). AnyTimes() mockStrategy.EXPECT(). Authenticate( gomock.Any(), gomock.Any(), - map[string]any{"key": "value"}, + gomock.Any(), ). Return(errors.New("auth failed")) return mockStrategy @@ -381,21 +393,26 @@ func TestAuthRoundTripper_RoundTrip(t *testing.T) { { name: "base transport error propagates after successful auth", target: &vmcp.BackendTarget{ - WorkloadID: "backend-1", - AuthStrategy: "header_injection", - AuthMetadata: map[string]any{"key": "value"}, + WorkloadID: "backend-1", + AuthConfig: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-API-Key", + HeaderValue: "test-key", + }, + }, }, setupStrategy: func(ctrl *gomock.Controller) auth.Strategy { mockStrategy := authmocks.NewMockStrategy(ctrl) mockStrategy.EXPECT(). Name(). - Return("header_injection"). + Return(authtypes.StrategyTypeHeaderInjection). AnyTimes() mockStrategy.EXPECT(). Authenticate( gomock.Any(), gomock.Any(), - map[string]any{"key": "value"}, + gomock.Any(), ). Return(nil) return mockStrategy @@ -412,23 +429,28 @@ func TestAuthRoundTripper_RoundTrip(t *testing.T) { { name: "request immutability - original request unchanged", target: &vmcp.BackendTarget{ - WorkloadID: "backend-1", - AuthStrategy: "header_injection", - AuthMetadata: map[string]any{"key": "value"}, + WorkloadID: "backend-1", + AuthConfig: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-API-Key", + HeaderValue: "test-key", + }, + }, }, setupStrategy: func(ctrl *gomock.Controller) auth.Strategy { mockStrategy := authmocks.NewMockStrategy(ctrl) mockStrategy.EXPECT(). Name(). - Return("header_injection"). + Return(authtypes.StrategyTypeHeaderInjection). AnyTimes() mockStrategy.EXPECT(). Authenticate( gomock.Any(), gomock.Any(), - map[string]any{"key": "value"}, + gomock.Any(), ). - DoAndReturn(func(_ context.Context, req *http.Request, _ map[string]any) error { + DoAndReturn(func(_ context.Context, req *http.Request, _ *authtypes.BackendAuthStrategy) error { // Modify the cloned request req.Header.Set("Authorization", "Bearer modified-token") req.Header.Set("X-Custom-Header", "custom-value") @@ -473,7 +495,7 @@ func TestAuthRoundTripper_RoundTrip(t *testing.T) { authRT := &authRoundTripper{ base: baseTransport, authStrategy: mockStrategy, - authMetadata: tt.target.AuthMetadata, + authConfig: tt.target.AuthConfig, target: tt.target, } @@ -548,51 +570,56 @@ func TestResolveAuthStrategy(t *testing.T) { { name: "defaults to unauthenticated when strategy is empty", target: &vmcp.BackendTarget{ - WorkloadID: "backend-1", - AuthStrategy: "", - AuthMetadata: nil, + WorkloadID: "backend-1", + AuthConfig: nil, // nil AuthConfig defaults to unauthenticated }, setupRegistry: func() auth.OutgoingAuthRegistry { registry := auth.NewDefaultOutgoingAuthRegistry() - err := registry.RegisterStrategy("unauthenticated", &strategies.UnauthenticatedStrategy{}) + err := registry.RegisterStrategy(authtypes.StrategyTypeUnauthenticated, &strategies.UnauthenticatedStrategy{}) require.NoError(t, err) return registry }, expectError: false, checkStrategy: func(t *testing.T, strategy auth.Strategy) { t.Helper() - assert.Equal(t, "unauthenticated", strategy.Name()) + assert.Equal(t, authtypes.StrategyTypeUnauthenticated, strategy.Name()) }, }, { name: "resolves explicitly configured strategy", target: &vmcp.BackendTarget{ - WorkloadID: "backend-1", - AuthStrategy: "header_injection", - AuthMetadata: map[string]any{"header_name": "X-API-Key", "header_value": "test-key"}, + WorkloadID: "backend-1", + AuthConfig: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-API-Key", + HeaderValue: "test-key", + }, + }, }, setupRegistry: func() auth.OutgoingAuthRegistry { registry := auth.NewDefaultOutgoingAuthRegistry() - err := registry.RegisterStrategy("header_injection", strategies.NewHeaderInjectionStrategy()) + err := registry.RegisterStrategy(authtypes.StrategyTypeHeaderInjection, strategies.NewHeaderInjectionStrategy()) require.NoError(t, err) return registry }, expectError: false, checkStrategy: func(t *testing.T, strategy auth.Strategy) { t.Helper() - assert.Equal(t, "header_injection", strategy.Name()) + assert.Equal(t, authtypes.StrategyTypeHeaderInjection, strategy.Name()) }, }, { name: "returns error for unknown strategy", target: &vmcp.BackendTarget{ - WorkloadID: "backend-1", - AuthStrategy: "unknown_strategy", - AuthMetadata: nil, + WorkloadID: "backend-1", + AuthConfig: &authtypes.BackendAuthStrategy{ + Type: "unknown_strategy", + }, }, setupRegistry: func() auth.OutgoingAuthRegistry { registry := auth.NewDefaultOutgoingAuthRegistry() - err := registry.RegisterStrategy("unauthenticated", &strategies.UnauthenticatedStrategy{}) + err := registry.RegisterStrategy(authtypes.StrategyTypeUnauthenticated, &strategies.UnauthenticatedStrategy{}) require.NoError(t, err) return registry }, @@ -602,9 +629,8 @@ func TestResolveAuthStrategy(t *testing.T) { { name: "returns error when unauthenticated strategy not registered", target: &vmcp.BackendTarget{ - WorkloadID: "backend-1", - AuthStrategy: "", // Empty strategy defaults to unauthenticated - AuthMetadata: nil, + WorkloadID: "backend-1", + AuthConfig: nil, // nil AuthConfig defaults to unauthenticated }, setupRegistry: func() auth.OutgoingAuthRegistry { // Don't register unauthenticated strategy diff --git a/pkg/vmcp/config/config_test.go b/pkg/vmcp/config/config_test.go index bdbdd0163..053842a45 100644 --- a/pkg/vmcp/config/config_test.go +++ b/pkg/vmcp/config/config_test.go @@ -4,132 +4,143 @@ import ( "testing" "github.com/stretchr/testify/assert" + + authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" ) func TestOutgoingAuthConfig_ResolveForBackend(t *testing.T) { t.Parallel() tests := []struct { - name string - config *OutgoingAuthConfig - backendID string - wantStrategy string - wantMetadata map[string]any - description string + name string + config *OutgoingAuthConfig + backendID string + wantStrategy *authtypes.BackendAuthStrategy + description string }{ { - name: "nil config returns empty", - config: nil, - backendID: "backend1", - wantStrategy: "", - wantMetadata: nil, - description: "When config is nil, should return empty values", + name: "nil config returns nil", + config: nil, + backendID: "backend1", + wantStrategy: nil, + description: "When config is nil, should return nil", }, { name: "backend-specific config takes precedence", config: &OutgoingAuthConfig{ - Default: &BackendAuthStrategy{ - Type: "default-strategy", - Metadata: map[string]any{"key": "default-value"}, + Default: &authtypes.BackendAuthStrategy{ + Type: "default-strategy", }, - Backends: map[string]*BackendAuthStrategy{ + Backends: map[string]*authtypes.BackendAuthStrategy{ "backend1": { - Type: "backend-specific-strategy", - Metadata: map[string]any{"key": "backend-value"}, + Type: "backend-specific-strategy", }, }, }, - backendID: "backend1", - wantStrategy: "backend-specific-strategy", - wantMetadata: map[string]any{"key": "backend-value"}, - description: "Backend-specific config should override default", + backendID: "backend1", + wantStrategy: &authtypes.BackendAuthStrategy{ + Type: "backend-specific-strategy", + }, + description: "Backend-specific config should override default", }, { name: "falls back to default when backend not configured", config: &OutgoingAuthConfig{ - Default: &BackendAuthStrategy{ - Type: "default-strategy", - Metadata: map[string]any{"key": "default-value"}, + Default: &authtypes.BackendAuthStrategy{ + Type: "default-strategy", }, - Backends: map[string]*BackendAuthStrategy{ + Backends: map[string]*authtypes.BackendAuthStrategy{ "backend1": { - Type: "backend-specific-strategy", - Metadata: map[string]any{"key": "backend-value"}, + Type: "backend-specific-strategy", }, }, }, - backendID: "backend2", - wantStrategy: "default-strategy", - wantMetadata: map[string]any{"key": "default-value"}, - description: "Should use default when specific backend not configured", + backendID: "backend2", + wantStrategy: &authtypes.BackendAuthStrategy{ + Type: "default-strategy", + }, + description: "Should use default when specific backend not configured", }, { - name: "returns empty when no default and backend not configured", + name: "returns nil when no default and backend not configured", config: &OutgoingAuthConfig{ - Backends: map[string]*BackendAuthStrategy{ + Backends: map[string]*authtypes.BackendAuthStrategy{ "backend1": { - Type: "backend-specific-strategy", - Metadata: map[string]any{"key": "backend-value"}, + Type: "backend-specific-strategy", }, }, }, backendID: "backend2", - wantStrategy: "", - wantMetadata: nil, - description: "Should return empty when no default and backend not in map", + wantStrategy: nil, + description: "Should return nil when no default and backend not in map", }, { name: "handles nil backend strategy in map", config: &OutgoingAuthConfig{ - Default: &BackendAuthStrategy{ - Type: "default-strategy", - Metadata: map[string]any{"key": "default-value"}, + Default: &authtypes.BackendAuthStrategy{ + Type: "default-strategy", }, - Backends: map[string]*BackendAuthStrategy{ + Backends: map[string]*authtypes.BackendAuthStrategy{ "backend1": nil, }, }, - backendID: "backend1", - wantStrategy: "default-strategy", - wantMetadata: map[string]any{"key": "default-value"}, - description: "Should fall back to default when backend strategy is nil", + backendID: "backend1", + wantStrategy: &authtypes.BackendAuthStrategy{ + Type: "default-strategy", + }, + description: "Should fall back to default when backend strategy is nil", }, { - name: "returns empty when only default is nil", + name: "returns nil when only default is nil", config: &OutgoingAuthConfig{ Default: nil, - Backends: map[string]*BackendAuthStrategy{}, + Backends: map[string]*authtypes.BackendAuthStrategy{}, }, backendID: "backend1", - wantStrategy: "", - wantMetadata: nil, - description: "Should return empty when default is nil and backend not found", + wantStrategy: nil, + description: "Should return nil when default is nil and backend not found", }, { - name: "handles strategy with nil metadata", + name: "handles header injection strategy", config: &OutgoingAuthConfig{ - Default: &BackendAuthStrategy{ - Type: "default-strategy", - Metadata: nil, + Default: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-API-Key", + HeaderValue: "test-value", + }, }, }, - backendID: "backend1", - wantStrategy: "default-strategy", - wantMetadata: nil, - description: "Should handle nil metadata correctly", + backendID: "backend1", + wantStrategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-API-Key", + HeaderValue: "test-value", + }, + }, + description: "Should return full header injection strategy", }, { - name: "handles strategy with empty metadata", + name: "handles token exchange strategy", config: &OutgoingAuthConfig{ - Default: &BackendAuthStrategy{ - Type: "default-strategy", - Metadata: map[string]any{}, + Default: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "https://example.com/token", + ClientID: "client-id", + }, }, }, - backendID: "backend1", - wantStrategy: "default-strategy", - wantMetadata: map[string]any{}, - description: "Should return empty map when metadata is empty", + backendID: "backend1", + wantStrategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "https://example.com/token", + ClientID: "client-id", + }, + }, + description: "Should return full token exchange strategy", }, } @@ -137,10 +148,9 @@ func TestOutgoingAuthConfig_ResolveForBackend(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - gotStrategy, gotMetadata := tt.config.ResolveForBackend(tt.backendID) + gotStrategy := tt.config.ResolveForBackend(tt.backendID) assert.Equal(t, tt.wantStrategy, gotStrategy, "Strategy mismatch: %s", tt.description) - assert.Equal(t, tt.wantMetadata, gotMetadata, "Metadata mismatch: %s", tt.description) }) } } diff --git a/pkg/vmcp/config/validator_test.go b/pkg/vmcp/config/validator_test.go index 1ab7dd8f4..cfa6f0dad 100644 --- a/pkg/vmcp/config/validator_test.go +++ b/pkg/vmcp/config/validator_test.go @@ -6,6 +6,7 @@ import ( "time" "github.com/stacklok/toolhive/pkg/vmcp" + authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" ) func TestValidator_ValidateBasicFields(t *testing.T) { @@ -190,8 +191,8 @@ func TestValidator_ValidateOutgoingAuth(t *testing.T) { name: "valid inline source with unauthenticated default", auth: &OutgoingAuthConfig{ Source: "inline", - Default: &BackendAuthStrategy{ - Type: "unauthenticated", + Default: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeUnauthenticated, }, }, wantErr: false, @@ -200,12 +201,12 @@ func TestValidator_ValidateOutgoingAuth(t *testing.T) { name: "valid header_injection backend", auth: &OutgoingAuthConfig{ Source: "inline", - Backends: map[string]*BackendAuthStrategy{ + Backends: map[string]*authtypes.BackendAuthStrategy{ "github": { - Type: "header_injection", - Metadata: map[string]any{ - "header_name": "Authorization", - "header_value": "secret-token", + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "Authorization", + HeaderValue: "secret-token", }, }, }, @@ -242,7 +243,7 @@ func TestValidator_ValidateOutgoingAuth(t *testing.T) { name: "invalid backend auth type", auth: &OutgoingAuthConfig{ Source: "inline", - Backends: map[string]*BackendAuthStrategy{ + Backends: map[string]*authtypes.BackendAuthStrategy{ "test": { Type: "invalid", }, diff --git a/pkg/vmcp/config/yaml_loader_test.go b/pkg/vmcp/config/yaml_loader_test.go index 7df5e1caf..56aea19c0 100644 --- a/pkg/vmcp/config/yaml_loader_test.go +++ b/pkg/vmcp/config/yaml_loader_test.go @@ -378,11 +378,11 @@ aggregation: if backend.Type != "header_injection" { t.Errorf("Backend.Type = %v, want header_injection", backend.Type) } - // Verify the resolved value is in metadata - headerValue, ok := backend.Metadata["header_value"].(string) - if !ok { - t.Fatal("header_value not found in metadata") + // Verify the resolved value is in the typed config + if backend.HeaderInjection == nil { + t.Fatal("HeaderInjection config not found") } + headerValue := backend.HeaderInjection.HeaderValue if headerValue != "secret-token-123" { t.Errorf("header_value = %v, want secret-token-123", headerValue) } @@ -418,10 +418,10 @@ aggregation: if !ok { t.Fatal("api-service backend not found") } - headerValue, ok := backend.Metadata["header_value"].(string) - if !ok { - t.Fatal("header_value not found in metadata") + if backend.HeaderInjection == nil { + t.Fatal("HeaderInjection config not found") } + headerValue := backend.HeaderInjection.HeaderValue if headerValue != "v1" { t.Errorf("header_value = %v, want v1", headerValue) } diff --git a/pkg/vmcp/config/yaml_loader_transform_test.go b/pkg/vmcp/config/yaml_loader_transform_test.go index b3fe17aab..ea1dacb83 100644 --- a/pkg/vmcp/config/yaml_loader_transform_test.go +++ b/pkg/vmcp/config/yaml_loader_transform_test.go @@ -9,7 +9,7 @@ import ( "go.uber.org/mock/gomock" "github.com/stacklok/toolhive/pkg/env/mocks" - "github.com/stacklok/toolhive/pkg/vmcp/auth/strategies" + authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" ) // TestYAMLLoader_transformBackendAuthStrategy tests the critical auth strategy transformation logic @@ -21,28 +21,29 @@ func TestYAMLLoader_transformBackendAuthStrategy(t *testing.T) { name string raw *rawBackendAuthStrategy envVars map[string]string - verify func(t *testing.T, strategy *BackendAuthStrategy) + verify func(t *testing.T, strategy *authtypes.BackendAuthStrategy) wantErr bool errMsg string }{ { name: "header_injection with literal value", raw: &rawBackendAuthStrategy{ - Type: strategies.StrategyTypeHeaderInjection, + Type: authtypes.StrategyTypeHeaderInjection, HeaderInjection: &rawHeaderInjectionAuth{ HeaderName: "Authorization", HeaderValue: "Bearer token123", }, }, - verify: func(t *testing.T, strategy *BackendAuthStrategy) { + verify: func(t *testing.T, strategy *authtypes.BackendAuthStrategy) { t.Helper() - assert.Equal(t, "Bearer token123", strategy.Metadata[strategies.MetadataHeaderValue]) + require.NotNil(t, strategy.HeaderInjection) + assert.Equal(t, "Bearer token123", strategy.HeaderInjection.HeaderValue) }, }, { name: "header_injection resolves env var correctly", raw: &rawBackendAuthStrategy{ - Type: strategies.StrategyTypeHeaderInjection, + Type: authtypes.StrategyTypeHeaderInjection, HeaderInjection: &rawHeaderInjectionAuth{ HeaderName: "X-API-Key", HeaderValueEnv: "API_KEY", @@ -51,15 +52,16 @@ func TestYAMLLoader_transformBackendAuthStrategy(t *testing.T) { envVars: map[string]string{ "API_KEY": "secret-key-value", }, - verify: func(t *testing.T, strategy *BackendAuthStrategy) { + verify: func(t *testing.T, strategy *authtypes.BackendAuthStrategy) { t.Helper() - assert.Equal(t, "secret-key-value", strategy.Metadata[strategies.MetadataHeaderValue]) + require.NotNil(t, strategy.HeaderInjection) + assert.Equal(t, "secret-key-value", strategy.HeaderInjection.HeaderValue) }, }, { name: "header_injection fails when both value and env set (mutual exclusivity)", raw: &rawBackendAuthStrategy{ - Type: strategies.StrategyTypeHeaderInjection, + Type: authtypes.StrategyTypeHeaderInjection, HeaderInjection: &rawHeaderInjectionAuth{ HeaderName: "Authorization", HeaderValue: "literal", @@ -72,7 +74,7 @@ func TestYAMLLoader_transformBackendAuthStrategy(t *testing.T) { { name: "header_injection fails when neither value nor env set", raw: &rawBackendAuthStrategy{ - Type: strategies.StrategyTypeHeaderInjection, + Type: authtypes.StrategyTypeHeaderInjection, HeaderInjection: &rawHeaderInjectionAuth{ HeaderName: "Authorization", }, @@ -83,7 +85,7 @@ func TestYAMLLoader_transformBackendAuthStrategy(t *testing.T) { { name: "header_injection fails when env var not set", raw: &rawBackendAuthStrategy{ - Type: strategies.StrategyTypeHeaderInjection, + Type: authtypes.StrategyTypeHeaderInjection, HeaderInjection: &rawHeaderInjectionAuth{ HeaderName: "Authorization", HeaderValueEnv: "MISSING_VAR", @@ -95,7 +97,7 @@ func TestYAMLLoader_transformBackendAuthStrategy(t *testing.T) { { name: "header_injection fails when env var is empty string", raw: &rawBackendAuthStrategy{ - Type: strategies.StrategyTypeHeaderInjection, + Type: authtypes.StrategyTypeHeaderInjection, HeaderInjection: &rawHeaderInjectionAuth{ HeaderName: "Authorization", HeaderValueEnv: "EMPTY_VAR", @@ -110,7 +112,7 @@ func TestYAMLLoader_transformBackendAuthStrategy(t *testing.T) { { name: "header_injection fails when config block missing", raw: &rawBackendAuthStrategy{ - Type: strategies.StrategyTypeHeaderInjection, + Type: authtypes.StrategyTypeHeaderInjection, }, wantErr: true, errMsg: "header_injection configuration is required", @@ -118,7 +120,7 @@ func TestYAMLLoader_transformBackendAuthStrategy(t *testing.T) { { name: "token_exchange validates env var is set", raw: &rawBackendAuthStrategy{ - Type: "token_exchange", + Type: authtypes.StrategyTypeTokenExchange, TokenExchange: &rawTokenExchangeAuth{ TokenURL: "https://auth.example.com/token", ClientID: "client-123", @@ -128,16 +130,17 @@ func TestYAMLLoader_transformBackendAuthStrategy(t *testing.T) { envVars: map[string]string{ "CLIENT_SECRET": "secret-value", }, - verify: func(t *testing.T, strategy *BackendAuthStrategy) { + verify: func(t *testing.T, strategy *authtypes.BackendAuthStrategy) { t.Helper() + require.NotNil(t, strategy.TokenExchange) // Verify env var name is stored (not resolved) for lazy evaluation - assert.Equal(t, "CLIENT_SECRET", strategy.Metadata["client_secret_env"]) + assert.Equal(t, "CLIENT_SECRET", strategy.TokenExchange.ClientSecretEnv) }, }, { name: "token_exchange fails when env var not set", raw: &rawBackendAuthStrategy{ - Type: "token_exchange", + Type: authtypes.StrategyTypeTokenExchange, TokenExchange: &rawTokenExchangeAuth{ TokenURL: "https://auth.example.com/token", ClientID: "client-123", @@ -150,7 +153,7 @@ func TestYAMLLoader_transformBackendAuthStrategy(t *testing.T) { { name: "token_exchange fails when config block missing", raw: &rawBackendAuthStrategy{ - Type: "token_exchange", + Type: authtypes.StrategyTypeTokenExchange, }, wantErr: true, errMsg: "token_exchange configuration is required", @@ -158,11 +161,13 @@ func TestYAMLLoader_transformBackendAuthStrategy(t *testing.T) { { name: "unauthenticated strategy requires no extra config", raw: &rawBackendAuthStrategy{ - Type: strategies.StrategyTypeUnauthenticated, + Type: authtypes.StrategyTypeUnauthenticated, }, - verify: func(t *testing.T, strategy *BackendAuthStrategy) { + verify: func(t *testing.T, strategy *authtypes.BackendAuthStrategy) { t.Helper() - assert.Empty(t, strategy.Metadata) + assert.Equal(t, authtypes.StrategyTypeUnauthenticated, strategy.Type) + assert.Nil(t, strategy.HeaderInjection) + assert.Nil(t, strategy.TokenExchange) }, }, } diff --git a/pkg/vmcp/registry_test.go b/pkg/vmcp/registry_test.go index b7bd59a38..2a461b85f 100644 --- a/pkg/vmcp/registry_test.go +++ b/pkg/vmcp/registry_test.go @@ -6,6 +6,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" ) const ( @@ -30,9 +32,10 @@ func TestNewImmutableRegistry(t *testing.T) { BaseURL: "http://localhost:8080", TransportType: "streamable-http", HealthStatus: BackendHealthy, - AuthStrategy: "unauthenticated", - AuthMetadata: map[string]any{"key": "value"}, - Metadata: map[string]string{"env": "production"}, + AuthConfig: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeUnauthenticated, + }, + Metadata: map[string]string{"env": "production"}, }, }, expectedCount: 1, @@ -79,9 +82,9 @@ func TestNewImmutableRegistry(t *testing.T) { name: "nil metadata maps", backends: []Backend{ { - ID: "backend-1", - AuthMetadata: nil, - Metadata: nil, + ID: "backend-1", + AuthConfig: nil, + Metadata: nil, }, }, expectedCount: 1, @@ -130,7 +133,7 @@ func TestNewImmutableRegistry(t *testing.T) { if tt.name == "nil metadata maps" { backend := registry.Get(ctx, "backend-1") require.NotNil(t, backend) - assert.Nil(t, backend.AuthMetadata) + assert.Nil(t, backend.AuthConfig) assert.Nil(t, backend.Metadata) } @@ -156,9 +159,13 @@ func TestBackendRegistry_Get(t *testing.T) { BaseURL: "http://localhost:8080", TransportType: "streamable-http", HealthStatus: BackendHealthy, - AuthStrategy: "token_exchange", - AuthMetadata: map[string]any{"audience": "github-api"}, - Metadata: map[string]string{"env": "production"}, + AuthConfig: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + Audience: "github-api", + }, + }, + Metadata: map[string]string{"env": "production"}, }, { ID: "jira-mcp", @@ -185,8 +192,10 @@ func TestBackendRegistry_Get(t *testing.T) { assert.Equal(t, "http://localhost:8080", b.BaseURL) assert.Equal(t, "streamable-http", b.TransportType) assert.Equal(t, BackendHealthy, b.HealthStatus) - assert.Equal(t, "token_exchange", b.AuthStrategy) - assert.Equal(t, "github-api", b.AuthMetadata["audience"]) + require.NotNil(t, b.AuthConfig) + assert.Equal(t, authtypes.StrategyTypeTokenExchange, b.AuthConfig.Type) + require.NotNil(t, b.AuthConfig.TokenExchange) + assert.Equal(t, "github-api", b.AuthConfig.TokenExchange.Audience) assert.Equal(t, "production", b.Metadata["env"]) }, }, @@ -309,8 +318,13 @@ func TestBackendRegistry_List(t *testing.T) { ID: "github-mcp", Name: "GitHub MCP", TransportType: "streamable-http", - AuthMetadata: map[string]any{"audience": "github-api"}, - Metadata: map[string]string{"env": "production"}, + AuthConfig: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + Audience: "github-api", + }, + }, + Metadata: map[string]string{"env": "production"}, }, } registry := NewImmutableRegistry(backends) @@ -319,7 +333,9 @@ func TestBackendRegistry_List(t *testing.T) { require.Len(t, result, 1) assert.Equal(t, "github-mcp", result[0].ID) - assert.Equal(t, "github-api", result[0].AuthMetadata["audience"]) + require.NotNil(t, result[0].AuthConfig) + require.NotNil(t, result[0].AuthConfig.TokenExchange) + assert.Equal(t, "github-api", result[0].AuthConfig.TokenExchange.Audience) assert.Equal(t, "production", result[0].Metadata["env"]) }) @@ -444,9 +460,14 @@ func TestBackendToTarget(t *testing.T) { BaseURL: "http://localhost:8080", TransportType: "streamable-http", HealthStatus: BackendHealthy, - AuthStrategy: "token_exchange", - AuthMetadata: map[string]any{"audience": "github-api", "scopes": []string{"repo"}}, - Metadata: map[string]string{"env": "production"}, + AuthConfig: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + Audience: "github-api", + Scopes: []string{"repo"}, + }, + }, + Metadata: map[string]string{"env": "production"}, }, wantNil: false, validate: func(t *testing.T, target *BackendTarget) { @@ -456,8 +477,10 @@ func TestBackendToTarget(t *testing.T) { assert.Equal(t, "http://localhost:8080", target.BaseURL) assert.Equal(t, "streamable-http", target.TransportType) assert.Equal(t, BackendHealthy, target.HealthStatus) - assert.Equal(t, "token_exchange", target.AuthStrategy) - assert.Equal(t, "github-api", target.AuthMetadata["audience"]) + require.NotNil(t, target.AuthConfig) + assert.Equal(t, authtypes.StrategyTypeTokenExchange, target.AuthConfig.Type) + require.NotNil(t, target.AuthConfig.TokenExchange) + assert.Equal(t, "github-api", target.AuthConfig.TokenExchange.Audience) assert.Equal(t, "production", target.Metadata["env"]) assert.False(t, target.SessionAffinity) }, @@ -465,16 +488,23 @@ func TestBackendToTarget(t *testing.T) { { name: "preserves metadata", backend: &Backend{ - ID: "test", - AuthMetadata: map[string]any{"token": "secret", "timeout": 30, "retries": 3}, - Metadata: map[string]string{"env": "staging", "region": "us-west-2", "version": "2.0.0"}, + ID: "test", + AuthConfig: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "Authorization", + HeaderValue: "Bearer secret", + }, + }, + Metadata: map[string]string{"env": "staging", "region": "us-west-2", "version": "2.0.0"}, }, wantNil: false, validate: func(t *testing.T, target *BackendTarget) { t.Helper() - assert.Equal(t, "secret", target.AuthMetadata["token"]) - assert.Equal(t, 30, target.AuthMetadata["timeout"]) - assert.Equal(t, 3, target.AuthMetadata["retries"]) + require.NotNil(t, target.AuthConfig) + assert.Equal(t, authtypes.StrategyTypeHeaderInjection, target.AuthConfig.Type) + require.NotNil(t, target.AuthConfig.HeaderInjection) + assert.Equal(t, "Bearer secret", target.AuthConfig.HeaderInjection.HeaderValue) assert.Equal(t, "staging", target.Metadata["env"]) assert.Equal(t, "us-west-2", target.Metadata["region"]) assert.Equal(t, "2.0.0", target.Metadata["version"]) @@ -493,8 +523,7 @@ func TestBackendToTarget(t *testing.T) { assert.Empty(t, target.BaseURL) assert.Empty(t, target.TransportType) assert.Equal(t, BackendHealthStatus(""), target.HealthStatus) - assert.Empty(t, target.AuthStrategy) - assert.Nil(t, target.AuthMetadata) + assert.Nil(t, target.AuthConfig) assert.Nil(t, target.Metadata) }, }, diff --git a/pkg/vmcp/workloads/k8s_test.go b/pkg/vmcp/workloads/k8s_test.go index d3c3331db..c720731ca 100644 --- a/pkg/vmcp/workloads/k8s_test.go +++ b/pkg/vmcp/workloads/k8s_test.go @@ -14,6 +14,7 @@ import ( mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" "github.com/stacklok/toolhive/pkg/vmcp" + authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" ) const testNamespace = "test-namespace" @@ -99,18 +100,19 @@ func TestDiscoverAuth_TokenExchange(t *testing.T) { require.NoError(t, err) require.NotNil(t, backend) - // Verify backend has auth populated - assert.Equal(t, "token_exchange", backend.AuthStrategy) - assert.NotNil(t, backend.AuthMetadata) - - // Verify metadata contains expected fields - metadata := backend.AuthMetadata - assert.Equal(t, "https://auth.example.com/token", metadata["token_url"]) - assert.Equal(t, "test-client", metadata["client_id"]) - assert.Equal(t, "my-secret-value", metadata["client_secret"]) - assert.Equal(t, "https://api.example.com", metadata["audience"]) - assert.Equal(t, []string{"read", "write"}, metadata["scopes"]) - assert.Equal(t, "urn:ietf:params:oauth:token-type:access_token", metadata["subject_token_type"]) + // Verify backend has typed auth populated + require.NotNil(t, backend.AuthConfig) + assert.Equal(t, authtypes.StrategyTypeTokenExchange, backend.AuthConfig.Type) + require.NotNil(t, backend.AuthConfig.TokenExchange) + + // Verify typed token exchange config + tokenExchange := backend.AuthConfig.TokenExchange + assert.Equal(t, "https://auth.example.com/token", tokenExchange.TokenURL) + assert.Equal(t, "test-client", tokenExchange.ClientID) + assert.Equal(t, "my-secret-value", tokenExchange.ClientSecret) + assert.Equal(t, "https://api.example.com", tokenExchange.Audience) + assert.Equal(t, []string{"read", "write"}, tokenExchange.Scopes) + assert.Equal(t, "urn:ietf:params:oauth:token-type:access_token", tokenExchange.SubjectTokenType) } func TestDiscoverAuth_HeaderInjection(t *testing.T) { @@ -176,16 +178,15 @@ func TestDiscoverAuth_HeaderInjection(t *testing.T) { require.NoError(t, err) require.NotNil(t, backend) - // Verify backend has auth populated - assert.Equal(t, "header_injection", backend.AuthStrategy) - assert.NotNil(t, backend.AuthMetadata) + // Verify backend has typed auth populated + require.NotNil(t, backend.AuthConfig) + assert.Equal(t, authtypes.StrategyTypeHeaderInjection, backend.AuthConfig.Type) + require.NotNil(t, backend.AuthConfig.HeaderInjection) - // Verify metadata contains expected fields - metadata := backend.AuthMetadata - assert.Equal(t, "X-API-Key", metadata["header_name"]) - assert.Equal(t, "my-api-key-value", metadata["header_value"]) - // Env var reference should be removed after secret resolution - assert.NotContains(t, metadata, "header_value_env") + // Verify typed header injection config + headerInjection := backend.AuthConfig.HeaderInjection + assert.Equal(t, "X-API-Key", headerInjection.HeaderName) + assert.Equal(t, "my-api-key-value", headerInjection.HeaderValue) } func TestDiscoverAuth_NoAuthConfig(t *testing.T) { @@ -220,8 +221,7 @@ func TestDiscoverAuth_NoAuthConfig(t *testing.T) { require.NotNil(t, backend) // Verify backend has no auth - assert.Empty(t, backend.AuthStrategy) - assert.Nil(t, backend.AuthMetadata) + assert.Nil(t, backend.AuthConfig) } func TestDiscoverAuth_AuthConfigNotFound(t *testing.T) { diff --git a/test/integration/vmcp/helpers/vmcp_server.go b/test/integration/vmcp/helpers/vmcp_server.go index 92b95561e..b961b3fff 100644 --- a/test/integration/vmcp/helpers/vmcp_server.go +++ b/test/integration/vmcp/helpers/vmcp_server.go @@ -13,6 +13,7 @@ import ( vmcptypes "github.com/stacklok/toolhive/pkg/vmcp" "github.com/stacklok/toolhive/pkg/vmcp/aggregator" "github.com/stacklok/toolhive/pkg/vmcp/auth/factory" + authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" vmcpclient "github.com/stacklok/toolhive/pkg/vmcp/client" "github.com/stacklok/toolhive/pkg/vmcp/discovery" "github.com/stacklok/toolhive/pkg/vmcp/router" @@ -29,7 +30,6 @@ func NewBackend(id string, opts ...func(*vmcptypes.Backend)) vmcptypes.Backend { TransportType: "streamable-http", HealthStatus: vmcptypes.BackendHealthy, Metadata: make(map[string]string), - AuthMetadata: make(map[string]any), } for _, opt := range opts { opt(&b) @@ -44,11 +44,10 @@ func WithURL(url string) func(*vmcptypes.Backend) { } } -// WithAuth configures authentication. -func WithAuth(strategy string, metadata map[string]any) func(*vmcptypes.Backend) { +// WithAuth configures typed authentication. +func WithAuth(authConfig *authtypes.BackendAuthStrategy) func(*vmcptypes.Backend) { return func(b *vmcptypes.Backend) { - b.AuthStrategy = strategy - b.AuthMetadata = metadata + b.AuthConfig = authConfig } } diff --git a/test/integration/vmcp/vmcp_integration_test.go b/test/integration/vmcp/vmcp_integration_test.go index d29a1dffc..57dd164e5 100644 --- a/test/integration/vmcp/vmcp_integration_test.go +++ b/test/integration/vmcp/vmcp_integration_test.go @@ -7,6 +7,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stacklok/toolhive/pkg/vmcp" + authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" "github.com/stacklok/toolhive/test/integration/vmcp/helpers" ) @@ -170,16 +171,22 @@ func TestVMCPServer_TwoBoundaryAuth_HeaderInjection(t *testing.T) { backends := []vmcp.Backend{ helpers.NewBackend("gitlab", helpers.WithURL(gitlabServer.URL+"/mcp"), - helpers.WithAuth("header_injection", map[string]any{ - "header_name": "X-GitLab-Token", - "header_value": "secret-123", + helpers.WithAuth(&authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-GitLab-Token", + HeaderValue: "secret-123", + }, }), ), helpers.NewBackend("github", helpers.WithURL(githubServer.URL+"/mcp"), - helpers.WithAuth("header_injection", map[string]any{ - "header_name": "Authorization", - "header_value": "Bearer token-456", + helpers.WithAuth(&authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "Authorization", + HeaderValue: "Bearer token-456", + }, }), ), } From d8bebfd67875eceb8aee9e7d4760f7c45d0b77b8 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Fri, 28 Nov 2025 15:53:14 +0000 Subject: [PATCH 8/8] test(vmcp): add CRD-to-CLI config roundtrip tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add integration tests that verify vMCP configuration serialized from Kubernetes CRDs can be properly deserialized and used by the CLI. Tests cover: - HeaderInjection config roundtrip (literal values, env vars) - TokenExchange config roundtrip (all fields, minimal, secrets) - Full OutgoingAuthConfig with default and per-backend strategies - Unauthenticated strategy roundtrip - YAML field naming consistency between operator and CLI types These tests prove that the typed BackendAuthStrategy fields serialize correctly to YAML and can be parsed by the CLI's yaml_loader.go code, validating the fix for the CRD-to-CLI serialization issue. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .../auth/strategies/tokenexchange_test.go | 46 +- pkg/vmcp/config/config_test.go | 20 +- pkg/vmcp/config/crd_cli_roundtrip_test.go | 612 ++++++++++++++++++ 3 files changed, 645 insertions(+), 33 deletions(-) create mode 100644 pkg/vmcp/config/crd_cli_roundtrip_test.go diff --git a/pkg/vmcp/auth/strategies/tokenexchange_test.go b/pkg/vmcp/auth/strategies/tokenexchange_test.go index 8153f8ad3..593158e0a 100644 --- a/pkg/vmcp/auth/strategies/tokenexchange_test.go +++ b/pkg/vmcp/auth/strategies/tokenexchange_test.go @@ -67,14 +67,14 @@ func TestTokenExchangeStrategy_Authenticate(t *testing.T) { t.Parallel() tests := []struct { - name string - setupCtx func() context.Context - strategy *authtypes.BackendAuthStrategy + name string + setupCtx func() context.Context + strategy *authtypes.BackendAuthStrategy tokenURLOverride string // If set, replace the strategy's TokenURL with this server URL - setupServer func() *httptest.Server - expectError bool - errorContains string - checkAuthHeader func(t *testing.T, req *http.Request) + setupServer func() *httptest.Server + expectError bool + errorContains string + checkAuthHeader func(t *testing.T, req *http.Request) }{ { name: "successfully exchanges token", @@ -93,7 +93,7 @@ func TestTokenExchangeStrategy_Authenticate(t *testing.T) { }, }, tokenURLOverride: "PLACEHOLDER", - expectError: false, + expectError: false, checkAuthHeader: func(t *testing.T, req *http.Request) { t.Helper() assert.Equal(t, "Bearer backend-token-123", req.Header.Get("Authorization")) @@ -116,7 +116,7 @@ func TestTokenExchangeStrategy_Authenticate(t *testing.T) { }, }, tokenURLOverride: "PLACEHOLDER", - expectError: false, + expectError: false, }, { name: "includes scopes in token exchange", @@ -135,7 +135,7 @@ func TestTokenExchangeStrategy_Authenticate(t *testing.T) { }, }, tokenURLOverride: "PLACEHOLDER", - expectError: false, + expectError: false, }, { name: "includes client credentials in token exchange", @@ -165,7 +165,7 @@ func TestTokenExchangeStrategy_Authenticate(t *testing.T) { }, }, tokenURLOverride: "PLACEHOLDER", - expectError: false, + expectError: false, }, { name: "returns error when no identity in context", @@ -182,8 +182,8 @@ func TestTokenExchangeStrategy_Authenticate(t *testing.T) { }, }, tokenURLOverride: "PLACEHOLDER", - expectError: true, - errorContains: "no identity", + expectError: true, + errorContains: "no identity", }, { name: "returns error when identity has no token", @@ -200,8 +200,8 @@ func TestTokenExchangeStrategy_Authenticate(t *testing.T) { }, }, tokenURLOverride: "PLACEHOLDER", - expectError: true, - errorContains: "no token", + expectError: true, + errorContains: "no token", }, { name: "returns error when strategy is nil", @@ -230,8 +230,8 @@ func TestTokenExchangeStrategy_Authenticate(t *testing.T) { }, }, tokenURLOverride: "PLACEHOLDER", - expectError: true, - errorContains: "token exchange failed", + expectError: true, + errorContains: "token exchange failed", }, { name: "returns error when response is missing access_token", @@ -252,8 +252,8 @@ func TestTokenExchangeStrategy_Authenticate(t *testing.T) { }, }, tokenURLOverride: "PLACEHOLDER", - expectError: true, - errorContains: "empty access_token", + expectError: true, + errorContains: "empty access_token", }, } @@ -349,14 +349,14 @@ func TestTokenExchangeStrategy_Validate(t *testing.T) { }, }, { - name: "error on nil strategy", - strategy: nil, + name: "error on nil strategy", + strategy: nil, expectError: "token_exchange configuration required", }, { name: "error on nil TokenExchange config", strategy: &authtypes.BackendAuthStrategy{ - Type: authtypes.StrategyTypeTokenExchange, + Type: authtypes.StrategyTypeTokenExchange, TokenExchange: nil, }, expectError: "token_exchange configuration required", @@ -364,7 +364,7 @@ func TestTokenExchangeStrategy_Validate(t *testing.T) { { name: "error on missing token_url", strategy: &authtypes.BackendAuthStrategy{ - Type: authtypes.StrategyTypeTokenExchange, + Type: authtypes.StrategyTypeTokenExchange, TokenExchange: &authtypes.TokenExchangeConfig{}, }, expectError: "token_url required", diff --git a/pkg/vmcp/config/config_test.go b/pkg/vmcp/config/config_test.go index 053842a45..f617b3f10 100644 --- a/pkg/vmcp/config/config_test.go +++ b/pkg/vmcp/config/config_test.go @@ -12,18 +12,18 @@ func TestOutgoingAuthConfig_ResolveForBackend(t *testing.T) { t.Parallel() tests := []struct { - name string - config *OutgoingAuthConfig - backendID string - wantStrategy *authtypes.BackendAuthStrategy - description string + name string + config *OutgoingAuthConfig + backendID string + wantStrategy *authtypes.BackendAuthStrategy + description string }{ { - name: "nil config returns nil", - config: nil, - backendID: "backend1", - wantStrategy: nil, - description: "When config is nil, should return nil", + name: "nil config returns nil", + config: nil, + backendID: "backend1", + wantStrategy: nil, + description: "When config is nil, should return nil", }, { name: "backend-specific config takes precedence", diff --git a/pkg/vmcp/config/crd_cli_roundtrip_test.go b/pkg/vmcp/config/crd_cli_roundtrip_test.go new file mode 100644 index 000000000..708610794 --- /dev/null +++ b/pkg/vmcp/config/crd_cli_roundtrip_test.go @@ -0,0 +1,612 @@ +package config + +import ( + "testing" + + "go.uber.org/mock/gomock" + "gopkg.in/yaml.v3" + + "github.com/stacklok/toolhive/pkg/env/mocks" + authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" +) + +// TestCRDToCliRoundtrip_HeaderInjection verifies that a BackendAuthStrategy with +// HeaderInjection config can be serialized to YAML and correctly deserialized +// by the CLI's yaml_loader.go code. +// +// This test simulates the flow: +// 1. Operator creates BackendAuthStrategy with HeaderInjection (like converters/header_injection.go does) +// 2. Config is serialized to YAML (for mounting as ConfigMap) +// 3. CLI parses YAML using rawBackendAuthStrategy + transformBackendAuthStrategy +// 4. All fields are correctly preserved +func TestCRDToCliRoundtrip_HeaderInjection(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + operatorStrategy *authtypes.BackendAuthStrategy + envVars map[string]string + wantType string + wantHeaderName string + wantHeaderValue string + wantErr bool + errContains string + }{ + { + name: "header injection with literal value", + operatorStrategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "Authorization", + HeaderValue: "Bearer secret-token-123", + }, + }, + wantType: authtypes.StrategyTypeHeaderInjection, + wantHeaderName: "Authorization", + wantHeaderValue: "Bearer secret-token-123", + }, + { + name: "header injection with custom header name", + operatorStrategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-API-Key", + HeaderValue: "api-key-value", + }, + }, + wantType: authtypes.StrategyTypeHeaderInjection, + wantHeaderName: "X-API-Key", + wantHeaderValue: "api-key-value", + }, + { + name: "header injection with env var reference", + operatorStrategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "Authorization", + HeaderValueEnv: "MY_SECRET_TOKEN", + }, + }, + envVars: map[string]string{ + "MY_SECRET_TOKEN": "resolved-secret-value", + }, + wantType: authtypes.StrategyTypeHeaderInjection, + wantHeaderName: "Authorization", + wantHeaderValue: "resolved-secret-value", + }, + { + name: "header injection with missing env var fails", + operatorStrategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "Authorization", + HeaderValueEnv: "MISSING_VAR", + }, + }, + wantErr: true, + errContains: "environment variable MISSING_VAR not set", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + // Step 1: Marshal the operator's BackendAuthStrategy to YAML + yamlBytes, err := yaml.Marshal(tt.operatorStrategy) + if err != nil { + t.Fatalf("failed to marshal operator strategy to YAML: %v", err) + } + + // Step 2: Unmarshal into CLI's raw struct (simulating YAML parsing) + var rawStrategy rawBackendAuthStrategy + if err := yaml.Unmarshal(yamlBytes, &rawStrategy); err != nil { + t.Fatalf("failed to unmarshal YAML to raw strategy: %v", err) + } + + // Step 3: Create mock env reader and YAMLLoader to use transform function + ctrl := gomock.NewController(t) + mockEnv := mocks.NewMockReader(ctrl) + + // Set up expectations for env vars + for key, value := range tt.envVars { + mockEnv.EXPECT().Getenv(key).Return(value).AnyTimes() + } + // Return empty for any other env var lookups + mockEnv.EXPECT().Getenv(gomock.Any()).Return("").AnyTimes() + + loader := NewYAMLLoader("", mockEnv) + + // Step 4: Transform raw struct to typed BackendAuthStrategy + cliStrategy, err := loader.transformBackendAuthStrategy(&rawStrategy) + + // Check error expectations + if tt.wantErr { + if err == nil { + t.Fatalf("expected error containing %q, got nil", tt.errContains) + } + if tt.errContains != "" && !containsString(err.Error(), tt.errContains) { + t.Errorf("error = %q, want to contain %q", err.Error(), tt.errContains) + } + return + } + + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // Step 5: Verify all fields are preserved + if cliStrategy.Type != tt.wantType { + t.Errorf("Type = %q, want %q", cliStrategy.Type, tt.wantType) + } + + if cliStrategy.HeaderInjection == nil { + t.Fatalf("HeaderInjection config is nil") + } + + if cliStrategy.HeaderInjection.HeaderName != tt.wantHeaderName { + t.Errorf("HeaderName = %q, want %q", + cliStrategy.HeaderInjection.HeaderName, tt.wantHeaderName) + } + + if cliStrategy.HeaderInjection.HeaderValue != tt.wantHeaderValue { + t.Errorf("HeaderValue = %q, want %q", + cliStrategy.HeaderInjection.HeaderValue, tt.wantHeaderValue) + } + }) + } +} + +// TestCRDToCliRoundtrip_TokenExchange verifies that a BackendAuthStrategy with +// TokenExchange config can be serialized to YAML and correctly deserialized +// by the CLI's yaml_loader.go code. +func TestCRDToCliRoundtrip_TokenExchange(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + operatorStrategy *authtypes.BackendAuthStrategy + envVars map[string]string + wantType string + wantTokenURL string + wantClientID string + wantAudience string + wantScopes []string + wantSubjectType string + wantErr bool + errContains string + }{ + { + name: "token exchange with all fields", + operatorStrategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "https://auth.example.com/oauth/token", + ClientID: "my-client-id", + ClientSecretEnv: "TOKEN_EXCHANGE_SECRET", + Audience: "https://api.example.com", + Scopes: []string{"read", "write"}, + SubjectTokenType: "urn:ietf:params:oauth:token-type:access_token", + }, + }, + envVars: map[string]string{ + "TOKEN_EXCHANGE_SECRET": "client-secret-value", + }, + wantType: authtypes.StrategyTypeTokenExchange, + wantTokenURL: "https://auth.example.com/oauth/token", + wantClientID: "my-client-id", + wantAudience: "https://api.example.com", + wantScopes: []string{"read", "write"}, + wantSubjectType: "urn:ietf:params:oauth:token-type:access_token", + }, + { + name: "token exchange with minimal fields", + operatorStrategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "https://auth.example.com/token", + }, + }, + wantType: authtypes.StrategyTypeTokenExchange, + wantTokenURL: "https://auth.example.com/token", + }, + { + name: "token exchange with client secret directly set", + operatorStrategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "https://auth.example.com/token", + ClientID: "direct-client", + ClientSecret: "direct-secret-value", + Audience: "https://backend.example.com", + }, + }, + wantType: authtypes.StrategyTypeTokenExchange, + wantTokenURL: "https://auth.example.com/token", + wantClientID: "direct-client", + wantAudience: "https://backend.example.com", + }, + { + name: "token exchange with missing env var fails", + operatorStrategy: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "https://auth.example.com/token", + ClientSecretEnv: "MISSING_SECRET_VAR", + }, + }, + wantErr: true, + errContains: "environment variable MISSING_SECRET_VAR not set", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + // Step 1: Marshal the operator's BackendAuthStrategy to YAML + yamlBytes, err := yaml.Marshal(tt.operatorStrategy) + if err != nil { + t.Fatalf("failed to marshal operator strategy to YAML: %v", err) + } + + // Step 2: Unmarshal into CLI's raw struct + var rawStrategy rawBackendAuthStrategy + if err := yaml.Unmarshal(yamlBytes, &rawStrategy); err != nil { + t.Fatalf("failed to unmarshal YAML to raw strategy: %v", err) + } + + // Step 3: Create mock env reader + ctrl := gomock.NewController(t) + mockEnv := mocks.NewMockReader(ctrl) + + for key, value := range tt.envVars { + mockEnv.EXPECT().Getenv(key).Return(value).AnyTimes() + } + mockEnv.EXPECT().Getenv(gomock.Any()).Return("").AnyTimes() + + loader := NewYAMLLoader("", mockEnv) + + // Step 4: Transform + cliStrategy, err := loader.transformBackendAuthStrategy(&rawStrategy) + + if tt.wantErr { + if err == nil { + t.Fatalf("expected error containing %q, got nil", tt.errContains) + } + if tt.errContains != "" && !containsString(err.Error(), tt.errContains) { + t.Errorf("error = %q, want to contain %q", err.Error(), tt.errContains) + } + return + } + + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // Step 5: Verify fields + if cliStrategy.Type != tt.wantType { + t.Errorf("Type = %q, want %q", cliStrategy.Type, tt.wantType) + } + + if cliStrategy.TokenExchange == nil { + t.Fatalf("TokenExchange config is nil") + } + + if cliStrategy.TokenExchange.TokenURL != tt.wantTokenURL { + t.Errorf("TokenURL = %q, want %q", + cliStrategy.TokenExchange.TokenURL, tt.wantTokenURL) + } + + if cliStrategy.TokenExchange.ClientID != tt.wantClientID { + t.Errorf("ClientID = %q, want %q", + cliStrategy.TokenExchange.ClientID, tt.wantClientID) + } + + if cliStrategy.TokenExchange.Audience != tt.wantAudience { + t.Errorf("Audience = %q, want %q", + cliStrategy.TokenExchange.Audience, tt.wantAudience) + } + + if !stringSliceEqual(cliStrategy.TokenExchange.Scopes, tt.wantScopes) { + t.Errorf("Scopes = %v, want %v", + cliStrategy.TokenExchange.Scopes, tt.wantScopes) + } + + if cliStrategy.TokenExchange.SubjectTokenType != tt.wantSubjectType { + t.Errorf("SubjectTokenType = %q, want %q", + cliStrategy.TokenExchange.SubjectTokenType, tt.wantSubjectType) + } + }) + } +} + +// TestCRDToCliRoundtrip_FullOutgoingAuthConfig verifies that a complete OutgoingAuthConfig +// with both Default and per-backend strategies can be serialized and deserialized correctly. +func TestCRDToCliRoundtrip_FullOutgoingAuthConfig(t *testing.T) { + t.Parallel() + + // Simulate operator creating OutgoingAuthConfig + operatorConfig := &OutgoingAuthConfig{ + Source: "inline", + Default: &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "Authorization", + HeaderValue: "Bearer default-token", + }, + }, + Backends: map[string]*authtypes.BackendAuthStrategy{ + "github": { + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "Authorization", + HeaderValue: "Bearer github-token", + }, + }, + "internal-api": { + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "https://auth.internal.com/token", + ClientID: "internal-client", + ClientSecretEnv: "INTERNAL_SECRET", + Audience: "https://api.internal.com", + Scopes: []string{"api.read", "api.write"}, + SubjectTokenType: "urn:ietf:params:oauth:token-type:access_token", + }, + }, + "public-api": { + Type: authtypes.StrategyTypeUnauthenticated, + }, + }, + } + + // Step 1: Marshal to YAML + yamlBytes, err := yaml.Marshal(operatorConfig) + if err != nil { + t.Fatalf("failed to marshal config to YAML: %v", err) + } + + // Step 2: Unmarshal into raw struct + var rawConfig rawOutgoingAuth + if err := yaml.Unmarshal(yamlBytes, &rawConfig); err != nil { + t.Fatalf("failed to unmarshal YAML: %v", err) + } + + // Step 3: Create mock env reader + ctrl := gomock.NewController(t) + mockEnv := mocks.NewMockReader(ctrl) + mockEnv.EXPECT().Getenv("INTERNAL_SECRET").Return("internal-secret-value").AnyTimes() + mockEnv.EXPECT().Getenv(gomock.Any()).Return("").AnyTimes() + + loader := NewYAMLLoader("", mockEnv) + + // Step 4: Transform + cliConfig, err := loader.transformOutgoingAuth(&rawConfig) + if err != nil { + t.Fatalf("failed to transform config: %v", err) + } + + // Step 5: Verify structure + if cliConfig.Source != "inline" { + t.Errorf("Source = %q, want %q", cliConfig.Source, "inline") + } + + // Verify default strategy + if cliConfig.Default == nil { + t.Fatal("Default strategy is nil") + } + if cliConfig.Default.Type != authtypes.StrategyTypeHeaderInjection { + t.Errorf("Default.Type = %q, want %q", + cliConfig.Default.Type, authtypes.StrategyTypeHeaderInjection) + } + if cliConfig.Default.HeaderInjection == nil { + t.Fatal("Default.HeaderInjection is nil") + } + if cliConfig.Default.HeaderInjection.HeaderValue != "Bearer default-token" { + t.Errorf("Default header value = %q, want %q", + cliConfig.Default.HeaderInjection.HeaderValue, "Bearer default-token") + } + + // Verify github backend + github, ok := cliConfig.Backends["github"] + if !ok { + t.Fatal("github backend not found") + } + if github.Type != authtypes.StrategyTypeHeaderInjection { + t.Errorf("github.Type = %q, want %q", github.Type, authtypes.StrategyTypeHeaderInjection) + } + if github.HeaderInjection == nil || github.HeaderInjection.HeaderValue != "Bearer github-token" { + t.Errorf("github header value = %v, want %q", + github.HeaderInjection, "Bearer github-token") + } + + // Verify internal-api backend (token exchange) + internalAPI, ok := cliConfig.Backends["internal-api"] + if !ok { + t.Fatal("internal-api backend not found") + } + if internalAPI.Type != authtypes.StrategyTypeTokenExchange { + t.Errorf("internal-api.Type = %q, want %q", + internalAPI.Type, authtypes.StrategyTypeTokenExchange) + } + if internalAPI.TokenExchange == nil { + t.Fatal("internal-api.TokenExchange is nil") + } + if internalAPI.TokenExchange.TokenURL != "https://auth.internal.com/token" { + t.Errorf("internal-api.TokenURL = %q, want %q", + internalAPI.TokenExchange.TokenURL, "https://auth.internal.com/token") + } + if internalAPI.TokenExchange.ClientID != "internal-client" { + t.Errorf("internal-api.ClientID = %q, want %q", + internalAPI.TokenExchange.ClientID, "internal-client") + } + if internalAPI.TokenExchange.Audience != "https://api.internal.com" { + t.Errorf("internal-api.Audience = %q, want %q", + internalAPI.TokenExchange.Audience, "https://api.internal.com") + } + if !stringSliceEqual(internalAPI.TokenExchange.Scopes, []string{"api.read", "api.write"}) { + t.Errorf("internal-api.Scopes = %v, want %v", + internalAPI.TokenExchange.Scopes, []string{"api.read", "api.write"}) + } + + // Verify public-api backend (unauthenticated) + publicAPI, ok := cliConfig.Backends["public-api"] + if !ok { + t.Fatal("public-api backend not found") + } + if publicAPI.Type != authtypes.StrategyTypeUnauthenticated { + t.Errorf("public-api.Type = %q, want %q", + publicAPI.Type, authtypes.StrategyTypeUnauthenticated) + } +} + +// TestCRDToCliRoundtrip_Unauthenticated verifies the unauthenticated strategy roundtrip. +func TestCRDToCliRoundtrip_Unauthenticated(t *testing.T) { + t.Parallel() + + operatorStrategy := &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeUnauthenticated, + } + + // Step 1: Marshal to YAML + yamlBytes, err := yaml.Marshal(operatorStrategy) + if err != nil { + t.Fatalf("failed to marshal: %v", err) + } + + // Step 2: Unmarshal to raw struct + var rawStrategy rawBackendAuthStrategy + if err := yaml.Unmarshal(yamlBytes, &rawStrategy); err != nil { + t.Fatalf("failed to unmarshal: %v", err) + } + + // Step 3: Transform + ctrl := gomock.NewController(t) + mockEnv := mocks.NewMockReader(ctrl) + mockEnv.EXPECT().Getenv(gomock.Any()).Return("").AnyTimes() + + loader := NewYAMLLoader("", mockEnv) + cliStrategy, err := loader.transformBackendAuthStrategy(&rawStrategy) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // Step 4: Verify + if cliStrategy.Type != authtypes.StrategyTypeUnauthenticated { + t.Errorf("Type = %q, want %q", cliStrategy.Type, authtypes.StrategyTypeUnauthenticated) + } + + // Unauthenticated should have no HeaderInjection or TokenExchange config + if cliStrategy.HeaderInjection != nil { + t.Errorf("HeaderInjection should be nil for unauthenticated, got %+v", + cliStrategy.HeaderInjection) + } + if cliStrategy.TokenExchange != nil { + t.Errorf("TokenExchange should be nil for unauthenticated, got %+v", + cliStrategy.TokenExchange) + } +} + +// TestYAMLFieldNaming verifies that YAML field names match between operator and CLI. +// This is a sanity check to ensure struct tags are consistent. +func TestYAMLFieldNaming(t *testing.T) { + t.Parallel() + + // Create a strategy with all fields populated + operatorStrategy := &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeHeaderInjection, + HeaderInjection: &authtypes.HeaderInjectionConfig{ + HeaderName: "X-Custom-Header", + HeaderValue: "custom-value", + HeaderValueEnv: "CUSTOM_ENV", + }, + } + + // Marshal to YAML + yamlBytes, err := yaml.Marshal(operatorStrategy) + if err != nil { + t.Fatalf("failed to marshal: %v", err) + } + + yamlStr := string(yamlBytes) + + // Verify expected field names are present in YAML + expectedFields := []string{ + "type:", + "header_injection:", + "header_name:", + "header_value:", + "header_value_env:", + } + + for _, field := range expectedFields { + if !containsString(yamlStr, field) { + t.Errorf("YAML missing expected field %q in:\n%s", field, yamlStr) + } + } + + // Verify JSON tags produce same field names when using yaml.v3 with json tags + // (yaml.v3 can use json tags as fallback) + tokenStrategy := &authtypes.BackendAuthStrategy{ + Type: authtypes.StrategyTypeTokenExchange, + TokenExchange: &authtypes.TokenExchangeConfig{ + TokenURL: "https://example.com/token", + ClientID: "client-123", + ClientSecretEnv: "SECRET_ENV", + Audience: "https://api.example.com", + Scopes: []string{"read", "write"}, + SubjectTokenType: "access_token", + }, + } + + tokenYamlBytes, err := yaml.Marshal(tokenStrategy) + if err != nil { + t.Fatalf("failed to marshal token strategy: %v", err) + } + + tokenYamlStr := string(tokenYamlBytes) + + expectedTokenFields := []string{ + "token_exchange:", + "token_url:", + "client_id:", + "client_secret_env:", + "audience:", + "scopes:", + "subject_token_type:", + } + + for _, field := range expectedTokenFields { + if !containsString(tokenYamlStr, field) { + t.Errorf("YAML missing expected field %q in:\n%s", field, tokenYamlStr) + } + } +} + +// containsString checks if s contains substr. +func containsString(s, substr string) bool { + return len(s) >= len(substr) && (s == substr || len(s) > 0 && containsSubstring(s, substr)) +} + +func containsSubstring(s, substr string) bool { + for i := 0; i <= len(s)-len(substr); i++ { + if s[i:i+len(substr)] == substr { + return true + } + } + return false +} + +// stringSliceEqual compares two string slices for equality. +func stringSliceEqual(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +}