diff --git a/docs/server/docs.go b/docs/server/docs.go index 878fc6acb9..e2977e7c0f 100644 --- a/docs/server/docs.go +++ b/docs/server/docs.go @@ -400,6 +400,10 @@ const docTemplate = `{ "description": "SubjectClaim names the validated ID-token claim to use as the upstream\nsubject. Defaults to \"sub\" when empty. Set for IdPs where \"sub\" isn't\nstable per user (e.g. Entra/Azure AD's \"oid\"). See upstream.OIDCConfig.", "type": "string" }, + "token_endpoint_auth_method": { + "description": "TokenEndpointAuthMethod is the client authentication method used at the token endpoint.\nWhen empty and a client secret is configured, client_secret_basic is used.", + "type": "string" + }, "userinfo_override": { "$ref": "#/components/schemas/authserver.UserInfoRunConfig" } diff --git a/docs/server/swagger.json b/docs/server/swagger.json index d0e8a2ef1f..df8efd6a72 100644 --- a/docs/server/swagger.json +++ b/docs/server/swagger.json @@ -393,6 +393,10 @@ "description": "SubjectClaim names the validated ID-token claim to use as the upstream\nsubject. Defaults to \"sub\" when empty. Set for IdPs where \"sub\" isn't\nstable per user (e.g. Entra/Azure AD's \"oid\"). See upstream.OIDCConfig.", "type": "string" }, + "token_endpoint_auth_method": { + "description": "TokenEndpointAuthMethod is the client authentication method used at the token endpoint.\nWhen empty and a client secret is configured, client_secret_basic is used.", + "type": "string" + }, "userinfo_override": { "$ref": "#/components/schemas/authserver.UserInfoRunConfig" } diff --git a/docs/server/swagger.yaml b/docs/server/swagger.yaml index 7ad50b8294..0fe2441957 100644 --- a/docs/server/swagger.yaml +++ b/docs/server/swagger.yaml @@ -470,6 +470,11 @@ components: subject. Defaults to "sub" when empty. Set for IdPs where "sub" isn't stable per user (e.g. Entra/Azure AD's "oid"). See upstream.OIDCConfig. type: string + token_endpoint_auth_method: + description: |- + TokenEndpointAuthMethod is the client authentication method used at the token endpoint. + When empty and a client secret is configured, client_secret_basic is used. + type: string userinfo_override: $ref: '#/components/schemas/authserver.UserInfoRunConfig' type: object diff --git a/pkg/authserver/config.go b/pkg/authserver/config.go index bd7a756258..f01325d575 100644 --- a/pkg/authserver/config.go +++ b/pkg/authserver/config.go @@ -637,6 +637,10 @@ type OIDCUpstreamRunConfig struct { // Mutually exclusive with ClientSecretFile. Optional for public clients using PKCE. ClientSecretEnvVar string `json:"client_secret_env_var,omitempty" yaml:"client_secret_env_var,omitempty"` + // TokenEndpointAuthMethod is the client authentication method used at the token endpoint. + // When empty and a client secret is configured, client_secret_basic is used. + TokenEndpointAuthMethod string `json:"token_endpoint_auth_method,omitempty" yaml:"token_endpoint_auth_method,omitempty"` + // RedirectURI is the callback URL where the upstream IDP will redirect after authentication. // When not specified, defaults to `{issuer}/oauth/callback`. RedirectURI string `json:"redirect_uri,omitempty" yaml:"redirect_uri,omitempty"` @@ -1370,6 +1374,32 @@ func (c *Config) warnTrustedIssuerAudiences() { } } +// Validate checks that the OIDCUpstreamRunConfig has a supported token endpoint +// authentication method and does not combine public-client authentication with a +// configured client secret. +func (c *OIDCUpstreamRunConfig) Validate() error { + hasSecretSource := c.ClientSecretFile != "" || c.ClientSecretEnvVar != "" + + switch c.TokenEndpointAuthMethod { + case "": + // Resolved from the presence of a secret in buildOIDCConfig. + case oauthproto.TokenEndpointAuthMethodNone: + if hasSecretSource { + return fmt.Errorf("oidc upstream: token_endpoint_auth_method none cannot be used with a client secret") + } + case oauthproto.TokenEndpointAuthMethodClientSecretBasic, oauthproto.TokenEndpointAuthMethodClientSecretPost: + if !hasSecretSource { + return fmt.Errorf( + "oidc upstream: token_endpoint_auth_method %q requires client_secret_file or client_secret_env_var", + c.TokenEndpointAuthMethod) + } + default: + return fmt.Errorf("oidc upstream: unsupported token_endpoint_auth_method %q", c.TokenEndpointAuthMethod) + } + + return nil +} + // Validate checks that the OAuth2UpstreamRunConfig is internally consistent. // It enforces the mutual exclusivity of ClientID and DCRConfig: exactly one must // be set. A ClientID is required for pre-provisioned clients; a DCRConfig is diff --git a/pkg/authserver/config_test.go b/pkg/authserver/config_test.go index eca7dfe62d..444fd4be22 100644 --- a/pkg/authserver/config_test.go +++ b/pkg/authserver/config_test.go @@ -18,6 +18,7 @@ import ( "github.com/stacklok/toolhive/pkg/authserver/server/registration" "github.com/stacklok/toolhive/pkg/authserver/server/tokenexchange" "github.com/stacklok/toolhive/pkg/authserver/upstream" + "github.com/stacklok/toolhive/pkg/oauthproto" ) func TestValidateIssuerURL(t *testing.T) { @@ -390,6 +391,67 @@ func assertError(t *testing.T, err error, wantErr bool, errMsg string) { } } +func TestOIDCUpstreamRunConfigValidate(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + config OIDCUpstreamRunConfig + wantErr bool + errMsg string + }{ + {name: "empty method is valid", config: OIDCUpstreamRunConfig{}}, + {name: "none is valid without secret", config: OIDCUpstreamRunConfig{TokenEndpointAuthMethod: oauthproto.TokenEndpointAuthMethodNone}}, + {name: "basic is valid", config: OIDCUpstreamRunConfig{TokenEndpointAuthMethod: oauthproto.TokenEndpointAuthMethodClientSecretBasic}}, + {name: "post is valid", config: OIDCUpstreamRunConfig{TokenEndpointAuthMethod: oauthproto.TokenEndpointAuthMethodClientSecretPost}}, + { + name: "unknown method rejects", + config: OIDCUpstreamRunConfig{TokenEndpointAuthMethod: "private_key_jwt"}, + wantErr: true, + errMsg: "unsupported token_endpoint_auth_method", + }, + { + name: "none with secret file rejects", + config: OIDCUpstreamRunConfig{TokenEndpointAuthMethod: oauthproto.TokenEndpointAuthMethodNone, ClientSecretFile: "secret"}, + wantErr: true, + errMsg: "none cannot be used with a client secret", + }, + { + name: "none with secret env rejects", + config: OIDCUpstreamRunConfig{TokenEndpointAuthMethod: oauthproto.TokenEndpointAuthMethodNone, ClientSecretEnvVar: "SECRET"}, + wantErr: true, + errMsg: "none cannot be used with a client secret", + }, + { + name: "basic without a secret source rejects", + config: OIDCUpstreamRunConfig{TokenEndpointAuthMethod: oauthproto.TokenEndpointAuthMethodClientSecretBasic}, + wantErr: true, + errMsg: `requires client_secret_file or client_secret_env_var`, + }, + { + name: "post without a secret source rejects", + config: OIDCUpstreamRunConfig{TokenEndpointAuthMethod: oauthproto.TokenEndpointAuthMethodClientSecretPost}, + wantErr: true, + errMsg: `requires client_secret_file or client_secret_env_var`, + }, + { + name: "basic with a client secret env var configured is valid", + config: OIDCUpstreamRunConfig{ + TokenEndpointAuthMethod: oauthproto.TokenEndpointAuthMethodClientSecretBasic, + ClientSecretEnvVar: "MY_CLIENT_SECRET", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + err := tt.config.Validate() + assertError(t, err, tt.wantErr, tt.errMsg) + }) + } +} + func TestOAuth2UpstreamRunConfigValidate(t *testing.T) { t.Parallel() diff --git a/pkg/authserver/runner/embeddedauthserver.go b/pkg/authserver/runner/embeddedauthserver.go index 073162d8d4..e0ebf5d355 100644 --- a/pkg/authserver/runner/embeddedauthserver.go +++ b/pkg/authserver/runner/embeddedauthserver.go @@ -26,6 +26,7 @@ import ( "github.com/stacklok/toolhive/pkg/authserver/storage" "github.com/stacklok/toolhive/pkg/authserver/upstream" "github.com/stacklok/toolhive/pkg/bodylimit" + "github.com/stacklok/toolhive/pkg/oauthproto" ) // Redis ACL credential environment variable names. @@ -689,6 +690,9 @@ func buildOIDCConfig(rc *authserver.UpstreamRunConfig, insecureAllowHTTP bool) ( } oidc := rc.OIDCConfig + if err := oidc.Validate(); err != nil { + return nil, err + } // Warn if UserInfoOverride is configured but won't be used if oidc.UserInfoOverride != nil { @@ -703,6 +707,16 @@ func buildOIDCConfig(rc *authserver.UpstreamRunConfig, insecureAllowHTTP bool) ( return nil, fmt.Errorf("failed to resolve OIDC client secret: %w", err) } + authMethod := oidc.TokenEndpointAuthMethod + if authMethod == "" && clientSecret != "" { + authMethod = oauthproto.TokenEndpointAuthMethodClientSecretBasic + } + if isConfidentialAuthMethod(authMethod) && clientSecret == "" { + return nil, fmt.Errorf( + "oidc upstream: token_endpoint_auth_method %q requires a non-empty client secret, "+ + "but the configured secret resolved to an empty value", authMethod) + } + // Default scopes if not specified. The default includes offline_access // (standard OIDC mechanism for refresh tokens). Providers like Google that // use access_type=offline instead should specify explicit scopes in their @@ -716,6 +730,7 @@ func buildOIDCConfig(rc *authserver.UpstreamRunConfig, insecureAllowHTTP bool) ( CommonOAuthConfig: upstream.CommonOAuthConfig{ ClientID: oidc.ClientID, ClientSecret: clientSecret, + TokenEndpointAuthMethod: authMethod, RedirectURI: oidc.RedirectURI, Scopes: scopes, AdditionalAuthorizationParams: oidc.AdditionalAuthorizationParams, @@ -784,6 +799,20 @@ func buildPureOAuth2Config(rc *authserver.UpstreamRunConfig, insecureAllowHTTP b return cfg, nil } +// isConfidentialAuthMethod reports whether method requires a client secret to +// be presented at the token endpoint. Used to catch a secret file that reads +// successfully but is empty after trimming -- a case Validate cannot see, since +// it only knows whether a secret source is configured, not what it resolves to. +// Shared by buildPureOAuth2Config and buildOIDCConfig. +func isConfidentialAuthMethod(method string) bool { + switch method { + case oauthproto.TokenEndpointAuthMethodClientSecretBasic, oauthproto.TokenEndpointAuthMethodClientSecretPost: + return true + default: + return false + } +} + // resolveSecret reads a secret from file or environment variable. // File takes precedence over env var. Returns an error if file is specified but // unreadable, or if envVar is specified but not set. Returns empty string with diff --git a/pkg/authserver/runner/embeddedauthserver_test.go b/pkg/authserver/runner/embeddedauthserver_test.go index f6d3dd691f..f27ef485f6 100644 --- a/pkg/authserver/runner/embeddedauthserver_test.go +++ b/pkg/authserver/runner/embeddedauthserver_test.go @@ -1100,6 +1100,7 @@ func TestBuildOIDCConfig(t *testing.T) { // Verify client config is passed through assert.Equal(t, "test-client-id", cfg.ClientID) assert.Equal(t, "http://localhost:8080/callback", cfg.RedirectURI) + assert.Empty(t, cfg.TokenEndpointAuthMethod) assert.Equal(t, []string{"openid", "profile"}, cfg.Scopes) }) @@ -1147,6 +1148,43 @@ func TestBuildOIDCConfig(t *testing.T) { require.NotNil(t, cfg) assert.Equal(t, "my-oidc-client-secret", cfg.ClientSecret) + assert.Equal(t, oauthproto.TokenEndpointAuthMethodClientSecretBasic, cfg.TokenEndpointAuthMethod) + }) + + t.Run("preserves explicit client_secret_post", func(t *testing.T) { + t.Parallel() + + rc := &authserver.UpstreamRunConfig{ + Type: authserver.UpstreamProviderTypeOIDC, + OIDCConfig: &authserver.OIDCUpstreamRunConfig{ + IssuerURL: "https://example.com", + ClientID: "test-client-id", + TokenEndpointAuthMethod: oauthproto.TokenEndpointAuthMethodClientSecretPost, + RedirectURI: "http://localhost:8080/callback", + }, + } + + cfg, err := buildOIDCConfig(rc, false) + require.NoError(t, err) + assert.Equal(t, oauthproto.TokenEndpointAuthMethodClientSecretPost, cfg.TokenEndpointAuthMethod) + }) + + t.Run("rejects invalid token endpoint auth method", func(t *testing.T) { + t.Parallel() + + rc := &authserver.UpstreamRunConfig{ + Type: authserver.UpstreamProviderTypeOIDC, + OIDCConfig: &authserver.OIDCUpstreamRunConfig{ + IssuerURL: "https://example.com", + ClientID: "test-client-id", + TokenEndpointAuthMethod: "private_key_jwt", + RedirectURI: "http://localhost:8080/callback", + }, + } + + _, err := buildOIDCConfig(rc, false) + require.Error(t, err) + assert.Contains(t, err.Error(), "unsupported token_endpoint_auth_method") }) t.Run("missing secret file returns error", func(t *testing.T) { diff --git a/pkg/authserver/upstream/oauth2.go b/pkg/authserver/upstream/oauth2.go index 337e413b55..98374c39c2 100644 --- a/pkg/authserver/upstream/oauth2.go +++ b/pkg/authserver/upstream/oauth2.go @@ -109,6 +109,12 @@ type CommonOAuthConfig struct { // after authentication. RedirectURI string `json:"redirect_uri" yaml:"redirect_uri"` + // TokenEndpointAuthMethod is the RFC 7591 client authentication method used + // at the token endpoint; see authStyleFromMethod for the mapping to + // oauth2.AuthStyle and the rationale. + //nolint:lll // field tags require full JSON+YAML names + TokenEndpointAuthMethod string `json:"token_endpoint_auth_method,omitempty" yaml:"token_endpoint_auth_method,omitempty"` + // AdditionalAuthorizationParams are extra query parameters to include in the // authorization URL. This is useful for providers that require custom parameters // such as Google's access_type=offline for obtaining refresh tokens. @@ -147,18 +153,6 @@ type OAuth2Config struct { // TokenEndpoint is the URL for the OAuth token endpoint. TokenEndpoint string `json:"token_endpoint" yaml:"token_endpoint"` - // TokenEndpointAuthMethod is the RFC 7591 client authentication method used - // at the token endpoint; see authStyleFromMethod for the mapping to - // oauth2.AuthStyle and the rationale. When empty, the historical default - // (POST body) is used. - // - // Only the DCR path populates this, via applyResolutionToOAuth2Config. - // OAuth2UpstreamRunConfig has no corresponding field, so a statically- - // configured upstream cannot set it and always gets the default — an - // intentional limitation scoped to issue #5865 (DCR-negotiated clients). - //nolint:lll // field tags require full JSON+YAML names - TokenEndpointAuthMethod string `json:"token_endpoint_auth_method,omitempty" yaml:"token_endpoint_auth_method,omitempty"` - // UserInfo contains configuration for fetching user information (optional). // When nil, the provider does not support UserInfo fetching. UserInfo *UserInfoConfig `json:"userinfo,omitempty" yaml:"userinfo,omitempty"` diff --git a/pkg/authserver/upstream/oauth2_test.go b/pkg/authserver/upstream/oauth2_test.go index 5ffe5b41ee..15a6973504 100644 --- a/pkg/authserver/upstream/oauth2_test.go +++ b/pkg/authserver/upstream/oauth2_test.go @@ -231,13 +231,13 @@ func TestNewOAuth2Provider(t *testing.T) { config := &OAuth2Config{ CommonOAuthConfig: CommonOAuthConfig{ - ClientID: "test-client", - ClientSecret: "test-secret", - RedirectURI: "http://localhost:8080/callback", + ClientID: "test-client", + ClientSecret: "test-secret", + RedirectURI: "http://localhost:8080/callback", + TokenEndpointAuthMethod: "private_key_jwt", }, - AuthorizationEndpoint: mock.URL + "/authorize", - TokenEndpoint: mock.URL + "/token", - TokenEndpointAuthMethod: "private_key_jwt", + AuthorizationEndpoint: mock.URL + "/authorize", + TokenEndpoint: mock.URL + "/token", } _, err := NewOAuth2Provider(config) @@ -357,13 +357,13 @@ func TestNewOAuth2Provider_TokenEndpointAuthMethod(t *testing.T) { config := &OAuth2Config{ CommonOAuthConfig: CommonOAuthConfig{ - ClientID: clientID, - ClientSecret: clientSecret, - RedirectURI: "http://localhost:8080/callback", + ClientID: clientID, + ClientSecret: clientSecret, + RedirectURI: "http://localhost:8080/callback", + TokenEndpointAuthMethod: tt.authMethod, }, - AuthorizationEndpoint: mock.URL + "/authorize", - TokenEndpoint: mock.URL + "/token", - TokenEndpointAuthMethod: tt.authMethod, + AuthorizationEndpoint: mock.URL + "/authorize", + TokenEndpoint: mock.URL + "/token", } provider, err := NewOAuth2Provider(config) @@ -429,13 +429,13 @@ func TestBaseOAuth2Provider_RefreshTokens_TokenEndpointAuthMethod(t *testing.T) config := &OAuth2Config{ CommonOAuthConfig: CommonOAuthConfig{ - ClientID: clientID, - ClientSecret: clientSecret, - RedirectURI: "http://localhost:8080/callback", + ClientID: clientID, + ClientSecret: clientSecret, + RedirectURI: "http://localhost:8080/callback", + TokenEndpointAuthMethod: oauthproto.TokenEndpointAuthMethodClientSecretBasic, }, - AuthorizationEndpoint: mock.URL + "/authorize", - TokenEndpoint: mock.URL + "/token", - TokenEndpointAuthMethod: oauthproto.TokenEndpointAuthMethodClientSecretBasic, + AuthorizationEndpoint: mock.URL + "/authorize", + TokenEndpoint: mock.URL + "/token", } provider, err := NewOAuth2Provider(config) diff --git a/pkg/authserver/upstream/oidc.go b/pkg/authserver/upstream/oidc.go index 38ab8157f0..8770a1a8d2 100644 --- a/pkg/authserver/upstream/oidc.go +++ b/pkg/authserver/upstream/oidc.go @@ -277,11 +277,14 @@ func NewOIDCProvider( } p.config = oauth2Config - // Create the oauth2.Config for use with golang.org/x/oauth2 library - // Use go-oidc's endpoint which handles discovery, but explicitly set AuthStyle - // to ensure client credentials are sent in the request body (not Basic auth header) - // for consistent behavior across different IDP implementations. + // Create the oauth2.Config for use with golang.org/x/oauth2 library. + // Use go-oidc's discovered endpoint URLs, but explicitly set AuthStyle from + // the configured method so client authentication is consistent across IDPs. providerEndpoint := oidcProvider.Endpoint() + authStyle, err := authStyleFromMethod(config.TokenEndpointAuthMethod) + if err != nil { + return nil, err + } p.oauth2Config = &oauth2.Config{ ClientID: config.ClientID, ClientSecret: config.ClientSecret, @@ -290,7 +293,7 @@ func NewOIDCProvider( Endpoint: oauth2.Endpoint{ AuthURL: providerEndpoint.AuthURL, TokenURL: providerEndpoint.TokenURL, - AuthStyle: oauth2.AuthStyleInParams, + AuthStyle: authStyle, }, }