From 717008cf9bf699f01c024b0865f69ad511a97507 Mon Sep 17 00:00:00 2001 From: James Elliott Date: Sun, 1 Sep 2024 11:52:03 +1000 Subject: [PATCH] fix(authentication): jwt assertions audience too strict This fixes an issue where all JWT assertions are strictly checked against the Token URL. RFC7523 Section 3 states that the JWT must contain an 'aud' claim that identifies the authorization server and that the token endpoint URL may be used, not that it must be used. RFC9126 clarifies this that it should be the issuer value, and that both the token endpoint URL and pushed authorization request endpoint URL must also be accepted. This fix facilitate this. --- client_authentication_strategy.go | 49 +++++++++++++++++-- client_authentication_test.go | 12 ++--- config.go | 8 +-- config_default.go | 14 +++--- fosite.go | 2 +- handler/rfc7523/handler.go | 28 +++++++++-- handler/rfc7523/handler_test.go | 7 +-- .../authorize_jwt_bearer_required_iat_test.go | 2 +- .../authorize_jwt_bearer_required_jti_test.go | 2 +- integration/authorize_jwt_bearer_test.go | 2 +- .../introspect_jwt_bearer_token_test.go | 2 +- 11 files changed, 96 insertions(+), 32 deletions(-) diff --git a/client_authentication_strategy.go b/client_authentication_strategy.go index fe098c39..9f63f7d3 100644 --- a/client_authentication_strategy.go +++ b/client_authentication_strategy.go @@ -22,7 +22,7 @@ type DefaultClientAuthenticationStrategy struct { } Config interface { JWKSFetcherStrategyProvider - TokenURLProvider + AllowedJWTAssertionAudiencesProvider } } @@ -277,15 +277,15 @@ func (s *DefaultClientAuthenticationStrategy) doAuthenticateAssertionJWTBearer(c } func (s *DefaultClientAuthenticationStrategy) doAuthenticateAssertionParseAssertionJWTBearer(ctx context.Context, client Client, assertion *ClientAssertion, resolver EndpointClientAuthHandler) (method, kid, alg string, token *xjwt.Token, claims *xjwt.RegisteredClaims, err error) { - var tokenURI string + audience := s.Config.GetAllowedJWTAssertionAudiences(ctx) - if tokenURI = s.Config.GetTokenURL(ctx); tokenURI == "" { - return "", "", "", nil, nil, errorsx.WithStack(ErrMisconfiguration.WithHint("The authorization server does not support OAuth 2.0 JWT Profile Client Authentication RFC7523 or OpenID Connect 1.0 specific authentication methods.").WithDebug("The authorization server Token URL was empty but it's required to validate the RFC7523 audience claim.")) + if len(audience) == 0 { + return "", "", "", nil, nil, errorsx.WithStack(ErrMisconfiguration.WithHint("The authorization server does not support OAuth 2.0 JWT Profile Client Authentication RFC7523 or OpenID Connect 1.0 specific authentication methods.").WithDebug("The authorization server could not determine any safe value for it's audience but it's required to validate the RFC7523 client assertions.")) } opts := []xjwt.ParserOption{ xjwt.WithStrictDecoding(), - xjwt.WithAudience(tokenURI), // Satisfies RFC7523 Section 3 Point 3. + //xjwt.WithAudience(tokenURI), // Satisfies RFC7523 Section 3 Point 3. xjwt.WithExpirationRequired(), // Satisfies RFC7523 Section 3 Point 4. xjwt.WithIssuedAt(), // Satisfies RFC7523 Section 3 Point 6. } @@ -320,9 +320,48 @@ func (s *DefaultClientAuthenticationStrategy) doAuthenticateAssertionParseAssert return "", "", "", nil, nil, resolveJWTErrorToRFCError(err) } + // Satisfies RFC7523 Section 3 Point 3. + if err = s.doAuthenticateAssertionJWTBearerClaimAudience(ctx, audience, claims); err != nil { + return "", "", "", nil, nil, err + } + return method, kid, alg, token, claims, nil } +func (s *DefaultClientAuthenticationStrategy) doAuthenticateAssertionJWTBearerClaimAudience(ctx context.Context, audience []string, claims *xjwt.RegisteredClaims) (err error) { + if len(claims.Audience) == 0 { + return errorsx.WithStack( + ErrInvalidClient. + WithHint("Unable to verify the integrity of the 'client_assertion' value. It may have been used before it was issued, may have been used before it's allowed to be used, may have been used after it's expired, or otherwise doesn't meet a particular validation constraint."). + WithDebug("Unable to validate the 'aud' claim of the 'client_assertion' as it was empty."), + ) + } + + validAudience := false + + var aud, unverified string + +verification: + for _, unverified = range claims.Audience { + for _, aud = range audience { + if subtle.ConstantTimeCompare([]byte(aud), []byte(unverified)) == 1 { + validAudience = true + break verification + } + } + } + + if !validAudience { + return errorsx.WithStack( + ErrInvalidClient. + WithHint("Unable to verify the integrity of the 'client_assertion' value. It may have been used before it was issued, may have been used before it's allowed to be used, may have been used after it's expired, or otherwise doesn't meet a particular validation constraint."). + WithDebugf("Unable to validate the 'aud' claim of the 'client_assertion' value '%s' as it doesn't match any of the expected values '%s'.", strings.Join(claims.Audience, "', '"), strings.Join(audience, "', '")), + ) + } + + return nil +} + func (s *DefaultClientAuthenticationStrategy) doAuthenticateAssertionParseAssertionJWTBearerFindKey(ctx context.Context, header map[string]any, client AuthenticationMethodClient, handler EndpointClientAuthHandler) (key any, err error) { var kid, alg, method string diff --git a/client_authentication_test.go b/client_authentication_test.go index 24468859..6ce06cfd 100644 --- a/client_authentication_test.go +++ b/client_authentication_test.go @@ -571,7 +571,7 @@ func TestAuthenticateClient(t *testing.T) { }, keyRSA, "kid-foo")}, "client_assertion_type": []string{consts.ClientAssertionTypeJWTBearer}}, r: new(http.Request), expectErr: ErrInvalidClient, - err: "Client authentication failed (e.g., unknown client, no client authentication included, or unsupported authentication method). Unable to decode 'client_assertion' value for an unknown reason. token has invalid claims: token has invalid audience", + err: "Client authentication failed (e.g., unknown client, no client authentication included, or unsupported authentication method). Unable to verify the integrity of the 'client_assertion' value. It may have been used before it was issued, may have been used before it's allowed to be used, may have been used after it's expired, or otherwise doesn't meet a particular validation constraint. Unable to validate the 'aud' claim of the 'client_assertion' value 'token-url-1', 'token-url-2' as it doesn't match any of the expected values 'token-url'.", }, { name: "ShouldPassWithProperAssertionWhenJWKsAreSetWithinTheClient", @@ -697,9 +697,9 @@ func TestAuthenticateClient(t *testing.T) { provider := &Fosite{ Store: storage.NewMemoryStore(), Config: &Config{ - JWKSFetcherStrategy: NewDefaultJWKSFetcherStrategy(), - TokenURL: "token-url", - HTTPClient: retryablehttp.NewClient(), + JWKSFetcherStrategy: NewDefaultJWKSFetcherStrategy(), + AllowedJWTAssertionAudiences: []string{"token-url"}, + HTTPClient: retryablehttp.NewClient(), }, } @@ -764,8 +764,8 @@ func TestAuthenticateClientTwice(t *testing.T) { provider := &Fosite{ Store: store, Config: &Config{ - JWKSFetcherStrategy: NewDefaultJWKSFetcherStrategy(), - TokenURL: "token-url", + JWKSFetcherStrategy: NewDefaultJWKSFetcherStrategy(), + AllowedJWTAssertionAudiences: []string{"token-url"}, }, } diff --git a/config.go b/config.go index 001167d5..7c16abd8 100644 --- a/config.go +++ b/config.go @@ -300,9 +300,11 @@ type FormPostResponseProvider interface { GetFormPostResponseWriter(ctx context.Context) FormPostResponseWriter } -type TokenURLProvider interface { - // GetTokenURL returns the token URL. - GetTokenURL(ctx context.Context) (url string) +// AllowedJWTAssertionAudiencesProvider is a provider used in contexts where the permitted audiences for a JWT assertion +// is required to validate a request. +type AllowedJWTAssertionAudiencesProvider interface { + // GetAllowedJWTAssertionAudiences returns the permitted audience list for JWT Assertions. + GetAllowedJWTAssertionAudiences(ctx context.Context) (audiences []string) } // AuthorizeEndpointHandlersProvider returns the provider for configuring the authorize endpoint handlers. diff --git a/config_default.go b/config_default.go index 8a4297e0..f1a105f2 100644 --- a/config_default.go +++ b/config_default.go @@ -95,10 +95,10 @@ type Config struct { // AllowedPromptValues sets which OpenID Connect prompt values the server supports. Defaults to []string{"login", "none", "consent", "select_account"}. AllowedPromptValues []string - // TokenURL is the the URL of the Authorization Server's Token Endpoint. If the authorization server is intended - // to be compatible with the private_key_jwt client authentication method (see http://openid.net/specs/openid-connect-core-1_0.html#CodeFlowAuth), - // this value MUST be set. - TokenURL string + // AllowedJWTAssertionAudiences is a list of permitted client assertion audiences. If the authorization server is + // intended to be compatible with the client_secret_jwt or private_key_jwt client authentication methods + // (see http://openid.net/specs/openid-connect-core-1_0.html#CodeFlowAuth), this value MUST be set. + AllowedJWTAssertionAudiences []string // JWKSFetcherStrategy is responsible for fetching JSON Web Keys from remote URLs. This is required when the private_key_jwt // client authentication method is used. Defaults to oauth2.DefaultJWKSFetcherStrategy. @@ -277,8 +277,8 @@ func (c *Config) GetHTTPClient(ctx context.Context) *retryablehttp.Client { return c.HTTPClient } -func (c *Config) GetTokenURL(ctx context.Context) string { - return c.TokenURL +func (c *Config) GetAllowedJWTAssertionAudiences(ctx context.Context) []string { + return c.AllowedJWTAssertionAudiences } func (c *Config) GetFormPostHTMLTemplate(ctx context.Context) *template.Template { @@ -652,7 +652,7 @@ var ( _ MessageCatalogProvider = (*Config)(nil) _ FormPostHTMLTemplateProvider = (*Config)(nil) _ FormPostResponseProvider = (*Config)(nil) - _ TokenURLProvider = (*Config)(nil) + _ AllowedJWTAssertionAudiencesProvider = (*Config)(nil) _ HTTPClientProvider = (*Config)(nil) _ HMACHashingProvider = (*Config)(nil) _ AuthorizeEndpointHandlersProvider = (*Config)(nil) diff --git a/fosite.go b/fosite.go index 8f3417eb..8c78bbf3 100644 --- a/fosite.go +++ b/fosite.go @@ -184,7 +184,7 @@ type Configurator interface { MessageCatalogProvider FormPostHTMLTemplateProvider FormPostResponseProvider - TokenURLProvider + AllowedJWTAssertionAudiencesProvider AuthorizeEndpointHandlersProvider TokenEndpointHandlersProvider TokenIntrospectionHandlersProvider diff --git a/handler/rfc7523/handler.go b/handler/rfc7523/handler.go index 254407c8..166320d4 100644 --- a/handler/rfc7523/handler.go +++ b/handler/rfc7523/handler.go @@ -5,6 +5,8 @@ package rfc7523 import ( "context" + "crypto/subtle" + "strings" "time" "github.com/go-jose/go-jose/v4" @@ -21,7 +23,7 @@ type Handler struct { Config interface { oauth2.AccessTokenLifespanProvider - oauth2.TokenURLProvider + oauth2.AllowedJWTAssertionAudiencesProvider oauth2.GrantTypeJWTBearerCanSkipClientAuthProvider oauth2.GrantTypeJWTBearerIDOptionalProvider oauth2.GrantTypeJWTBearerIssuedDateOptionalProvider @@ -229,17 +231,37 @@ func (c *Handler) findPublicKeyForToken(ctx context.Context, token *jwt.JSONWebT // //nolint:gocyclo,unparam func (c *Handler) validateTokenClaims(ctx context.Context, claims jwt.Claims, key *jose.JSONWebKey) error { + audiences := c.Config.GetAllowedJWTAssertionAudiences(ctx) + + if len(audiences) == 0 { + return errorsx.WithStack(oauth2.ErrServerError. + WithHint("The JWT in 'assertion' request parameter can't be validated as the authorization server isn't configured to accept JWT assertions."), + ) + } + if len(claims.Audience) == 0 { return errorsx.WithStack(oauth2.ErrInvalidGrant. WithHint("The JWT in 'assertion' request parameter MUST contain an 'aud' (audience) claim."), ) } - if !claims.Audience.Contains(c.Config.GetTokenURL(ctx)) { + validAudience := false + +verify: + for _, unverified := range claims.Audience { + for _, aud := range audiences { + if subtle.ConstantTimeCompare([]byte(aud), []byte(unverified)) == 1 { + validAudience = true + break verify + } + } + } + + if !validAudience { return errorsx.WithStack(oauth2.ErrInvalidGrant. WithHintf( "The JWT in 'assertion' request parameter MUST contain an 'aud' (audience) claim containing a value '%s' that identifies the authorization server as an intended audience.", - c.Config.GetTokenURL(ctx), + strings.Join(audiences, "', '"), ), ) } diff --git a/handler/rfc7523/handler_test.go b/handler/rfc7523/handler_test.go index ed98e4f0..66b66a9c 100644 --- a/handler/rfc7523/handler_test.go +++ b/handler/rfc7523/handler_test.go @@ -12,6 +12,7 @@ import ( mrand "math/rand" "net/url" "strconv" + "strings" "testing" "time" @@ -74,7 +75,7 @@ func (s *AuthorizeJWTGrantRequestHandlerTestSuite) SetupTest() { Config: &oauth2.Config{ ScopeStrategy: oauth2.HierarchicScopeStrategy, AudienceMatchingStrategy: oauth2.DefaultAudienceMatchingStrategy, - TokenURL: "https://www.example.com/token", + AllowedJWTAssertionAudiences: []string{"https://www.example.com/token"}, GrantTypeJWTBearerCanSkipClientAuth: false, GrantTypeJWTBearerIDOptional: false, GrantTypeJWTBearerIssuedDateOptional: false, @@ -333,7 +334,7 @@ func (s *AuthorizeJWTGrantRequestHandlerTestSuite) TestNotValidAudienceInAsserti s.Equal( fmt.Sprintf( "The JWT in 'assertion' request parameter MUST contain an 'aud' (audience) claim containing a value '%s' that identifies the authorization server as an intended audience.", - s.handler.Config.GetTokenURL(ctx), + strings.Join(s.handler.Config.GetAllowedJWTAssertionAudiences(ctx), "', '"), ), oauth2.ErrorToRFC6749Error(err).HintField, ) @@ -851,7 +852,7 @@ func (s *AuthorizeJWTGrantPopulateTokenEndpointTestSuite) SetupTest() { Config: &oauth2.Config{ ScopeStrategy: oauth2.HierarchicScopeStrategy, AudienceMatchingStrategy: oauth2.DefaultAudienceMatchingStrategy, - TokenURL: "https://www.example.com/token", + AllowedJWTAssertionAudiences: []string{"https://www.example.com/token"}, GrantTypeJWTBearerCanSkipClientAuth: false, GrantTypeJWTBearerIDOptional: false, GrantTypeJWTBearerIssuedDateOptional: false, diff --git a/integration/authorize_jwt_bearer_required_iat_test.go b/integration/authorize_jwt_bearer_required_iat_test.go index 9ce653e5..fe17ee85 100644 --- a/integration/authorize_jwt_bearer_required_iat_test.go +++ b/integration/authorize_jwt_bearer_required_iat_test.go @@ -90,7 +90,7 @@ func TestAuthorizeJWTBearerRequiredIATSuite(t *testing.T) { GrantTypeJWTBearerCanSkipClientAuth: true, GrantTypeJWTBearerIDOptional: true, GrantTypeJWTBearerIssuedDateOptional: false, - TokenURL: tokenURL, + AllowedJWTAssertionAudiences: []string{tokenURL}, }, store, jwtStrategy, diff --git a/integration/authorize_jwt_bearer_required_jti_test.go b/integration/authorize_jwt_bearer_required_jti_test.go index 0f92a439..d6bb4be7 100644 --- a/integration/authorize_jwt_bearer_required_jti_test.go +++ b/integration/authorize_jwt_bearer_required_jti_test.go @@ -89,7 +89,7 @@ func TestAuthorizeJWTBearerRequiredJtiSuite(t *testing.T) { GrantTypeJWTBearerCanSkipClientAuth: true, GrantTypeJWTBearerIDOptional: false, GrantTypeJWTBearerIssuedDateOptional: true, - TokenURL: tokenURL, + AllowedJWTAssertionAudiences: []string{tokenURL}, }, store, jwtStrategy, diff --git a/integration/authorize_jwt_bearer_test.go b/integration/authorize_jwt_bearer_test.go index 6d7ba0c8..eb81943d 100644 --- a/integration/authorize_jwt_bearer_test.go +++ b/integration/authorize_jwt_bearer_test.go @@ -412,7 +412,7 @@ func TestAuthorizeJWTBearerSuite(t *testing.T) { GrantTypeJWTBearerIDOptional: true, GrantTypeJWTBearerIssuedDateOptional: true, GrantTypeJWTBearerMaxDuration: 24 * time.Hour, - TokenURL: tokenURL, + AllowedJWTAssertionAudiences: []string{tokenURL}, }, store, jwtStrategy, diff --git a/integration/introspect_jwt_bearer_token_test.go b/integration/introspect_jwt_bearer_token_test.go index 5f83479e..ebff676c 100644 --- a/integration/introspect_jwt_bearer_token_test.go +++ b/integration/introspect_jwt_bearer_token_test.go @@ -233,7 +233,7 @@ func TestIntrospectJWTBearerTokenSuite(t *testing.T) { GrantTypeJWTBearerIDOptional: true, GrantTypeJWTBearerIssuedDateOptional: true, AccessTokenLifespan: time.Hour, - TokenURL: tokenURL, + AllowedJWTAssertionAudiences: []string{tokenURL}, }, store, jwtStrategy,