Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions docs/server/docs.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions docs/server/swagger.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions docs/server/swagger.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

30 changes: 30 additions & 0 deletions pkg/authserver/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Expand Down Expand Up @@ -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
Expand Down
62 changes: 62 additions & 0 deletions pkg/authserver/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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()

Expand Down
29 changes: 29 additions & 0 deletions pkg/authserver/runner/embeddedauthserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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 {
Expand All @@ -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
Expand All @@ -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,
Expand Down Expand Up @@ -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
Expand Down
38 changes: 38 additions & 0 deletions pkg/authserver/runner/embeddedauthserver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})

Expand Down Expand Up @@ -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) {
Expand Down
18 changes: 6 additions & 12 deletions pkg/authserver/upstream/oauth2.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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"`
Expand Down
36 changes: 18 additions & 18 deletions pkg/authserver/upstream/oauth2_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
13 changes: 8 additions & 5 deletions pkg/authserver/upstream/oidc.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -290,7 +293,7 @@ func NewOIDCProvider(
Endpoint: oauth2.Endpoint{
AuthURL: providerEndpoint.AuthURL,
TokenURL: providerEndpoint.TokenURL,
AuthStyle: oauth2.AuthStyleInParams,
AuthStyle: authStyle,
},
}

Expand Down
Loading