diff --git a/pkg/auth/token.go b/pkg/auth/token.go index 1102df1879..7147daefc4 100644 --- a/pkg/auth/token.go +++ b/pkg/auth/token.go @@ -587,35 +587,18 @@ func resolveClientSecret(configSecret string, envReader env.Reader) string { return "" } -// NewTokenValidator creates a new token validator. -func NewTokenValidator(ctx context.Context, config TokenValidatorConfig, opts ...TokenValidatorOption) (*TokenValidator, error) { - // Apply functional options - o := &tokenValidatorOptions{envReader: &env.OSReader{}} - for _, opt := range opts { - opt(o) - } - - // Log warning if insecure HTTP is enabled - if config.InsecureAllowHTTP { - slog.Warn( - "insecure HTTP is enabled - "+ - "HTTP OIDC URLs are allowed - this is INSECURE and should NEVER be used in production", - "issuer", config.Issuer, - ) - } - +// resolveJWKSDiscovery determines the JWKS URL to use, and whether lazy OIDC +// discovery is needed. Discovery is deferred when JWKS URL is not provided +// but issuer is, allowing the validator to start before the OIDC provider is +// ready; ensureOIDCDiscovered populates jwksURL on first use in that case. +func resolveJWKSDiscovery(config TokenValidatorConfig, o *tokenValidatorOptions) (string, error) { jwksURL := config.JWKSURL - // Determine if we need lazy OIDC discovery. - // Discovery is deferred when JWKS URL is not provided but issuer is, - // allowing the validator to start before the OIDC provider is ready. - // When set, ensureOIDCDiscovered will populate jwksURL on first use. - // Skip discovery if TOOLHIVE_SKIP_OIDC_DISCOVERY is set (for testing only) skipDiscovery := o.envReader.Getenv("TOOLHIVE_SKIP_OIDC_DISCOVERY") == "true" if skipDiscovery && config.Issuer != "" { if jwksURL == "" { - return nil, fmt.Errorf( + return "", fmt.Errorf( "TOOLHIVE_SKIP_OIDC_DISCOVERY=true requires explicit JWKSURL in config. " + "This env var is for testing only and cannot guess provider-specific JWKS URLs", ) @@ -637,7 +620,36 @@ func NewTokenValidator(ctx context.Context, config TokenValidatorConfig, opts .. // Ensure we have either an explicit JWKS URL, an issuer to discover from, // or a local key provider (embedded auth server). if jwksURL == "" && config.Issuer == "" && o.keyProvider == nil { - return nil, ErrMissingIssuerAndJWKSURL + return "", ErrMissingIssuerAndJWKSURL + } + + return jwksURL, nil +} + +// NewTokenValidator creates a new token validator. +func NewTokenValidator(ctx context.Context, config TokenValidatorConfig, opts ...TokenValidatorOption) (*TokenValidator, error) { + // Apply functional options + o := &tokenValidatorOptions{envReader: &env.OSReader{}} + for _, opt := range opts { + opt(o) + } + + // Log warning if insecure HTTP is enabled + if config.InsecureAllowHTTP { + slog.Warn( + "insecure HTTP is enabled - "+ + "HTTP OIDC URLs are allowed - this is INSECURE and should NEVER be used in production", + "issuer", config.Issuer, + ) + } + + jwksURL, err := resolveJWKSDiscovery(config, o) + if err != nil { + return nil, err + } + + if err := validateGoogleTokeninfoAudience(config); err != nil { + return nil, err } // Create HTTP client with CA bundle and auth token support for JWKS @@ -691,6 +703,23 @@ func NewTokenValidator(ctx context.Context, config TokenValidatorConfig, opts .. return validator, nil } +// validateGoogleTokeninfoAudience rejects a Google tokeninfo introspection +// config with no audience. Google's tokeninfo endpoint returns no iss claim - +// parseGoogleResponse synthesises iss locally, so a configured-issuer check +// against it is self-satisfying and proves nothing about which OAuth client +// the token was minted for. The only real binding to this deployment is the +// audience check, which is skipped when audience is empty. Without it, ANY +// valid Google access token (e.g. one minted for an attacker's own unrelated +// OAuth client) passes validation. Refuse the combination at startup instead +// of silently accepting cross-client tokens at runtime. +func validateGoogleTokeninfoAudience(config TokenValidatorConfig) error { + if config.IntrospectionURL == GoogleTokeninfoURL && strings.TrimSpace(config.Audience) == "" { + return fmt.Errorf("audience is required when using Google's tokeninfo endpoint for introspection: " + + "without it any valid Google access token is accepted regardless of the OAuth client it was minted for") + } + return nil +} + // ensureJWKSRegistered ensures that the JWKS URL is registered with the cache. // This is called lazily on first use to avoid blocking startup. // On failure, registration is retried on subsequent calls (transient failures diff --git a/pkg/auth/token_test.go b/pkg/auth/token_test.go index e4c4ee910b..5f5aaa8fee 100644 --- a/pkg/auth/token_test.go +++ b/pkg/auth/token_test.go @@ -3075,3 +3075,39 @@ func TestEnsureJWKSRegistered_NonFatalRegistrationErrors(t *testing.T) { require.True(t, validator.jwksRegistered) }) } + +func TestNewTokenValidator_GoogleTokeninfoRequiresAudience(t *testing.T) { + t.Parallel() + + // Google tokeninfo returns no iss claim (the provider synthesises it + // locally), so an issuer check is self-satisfying and the audience check + // is the only binding to this deployment. Without it, any valid Google + // access token passes regardless of which OAuth client minted it. + tests := []struct { + name string + audience string + expectErr bool + }{ + {name: "empty audience rejected", audience: "", expectErr: true}, + {name: "whitespace-only audience rejected", audience: " ", expectErr: true}, + {name: "audience configured accepted", audience: "my-client-id.apps.googleusercontent.com", expectErr: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + _, err := NewTokenValidator(context.Background(), TokenValidatorConfig{ + Issuer: "https://accounts.google.com", + JWKSURL: "https://www.googleapis.com/oauth2/v3/certs", + IntrospectionURL: GoogleTokeninfoURL, + Audience: tt.audience, + }) + if tt.expectErr { + require.Error(t, err) + require.Contains(t, err.Error(), "audience is required") + } else { + require.NoError(t, err) + } + }) + } +}