fix(authentication): jwt assertions audience too strict #125
Conversation
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.
WalkthroughThe changes across the codebase involve a transition from using a single Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- client_authentication_strategy.go (3 hunks)
- client_authentication_test.go (3 hunks)
- config.go (1 hunks)
- config_default.go (3 hunks)
- fosite.go (1 hunks)
- handler/rfc7523/handler.go (3 hunks)
- handler/rfc7523/handler_test.go (4 hunks)
- integration/authorize_jwt_bearer_required_iat_test.go (1 hunks)
- integration/authorize_jwt_bearer_required_jti_test.go (1 hunks)
- integration/authorize_jwt_bearer_test.go (1 hunks)
- integration/introspect_jwt_bearer_token_test.go (1 hunks)
Additional comments not posted (21)
integration/authorize_jwt_bearer_required_jti_test.go (1)
92-92: LGTM! The change enhances the flexibility and security of the audience validation mechanism.The replacement of
TokenURLwithAllowedJWTAssertionAudiencesallows for multiple acceptable audience values for JWT assertions. This change improves the flexibility and security of the audience validation mechanism.Please verify the impact of this change on downstream functionality that relies on the audience validation mechanism. Consider running integration tests or manually testing relevant scenarios to ensure the expected behavior.
integration/authorize_jwt_bearer_required_iat_test.go (1)
93-93: LGTM! The change enhances the flexibility of the audience validation mechanism.The replacement of
TokenURLwithAllowedJWTAssertionAudiencesallows for multiple acceptable audience values for JWT assertions. This change improves the flexibility of the audience validation mechanism.Please verify the impact of this change on the validation logic related to JWT processing. Consider running integration tests or manually testing relevant scenarios to ensure the expected behavior.
fosite.go (1)
187-187: LGTM! The change aligns with the modifications made in the test files.The replacement of
TokenURLProviderwithAllowedJWTAssertionAudiencesProviderin theConfiguratorinterface aligns with the changes made in the test files. This change indicates a shift in the responsibilities of the interface to handle allowed audiences for JWT assertions instead of providing a token URL.Please verify the impact of this change on components that implement or interact with the
Configuratorinterface. Consider running integration tests or manually testing relevant scenarios to ensure the expected behavior.integration/introspect_jwt_bearer_token_test.go (1)
236-236: LGTM! The change aligns with the new audience validation approach.The modification replaces the
TokenURLfield withAllowedJWTAssertionAudiences, initialized with a slice containingtokenURL. This change indicates a shift towards validating JWT assertions against allowed audiences, rather than a single token URL.handler/rfc7523/handler.go (2)
26-26: LGTM! The change in theConfiginterface aligns with the new audience validation approach.The
Configinterface now includesoauth2.AllowedJWTAssertionAudiencesProviderinstead ofoauth2.TokenURLProvider, which aligns with the shift towards validating JWT assertions against allowed audiences.
Line range hint
234-268: LGTM! The changes in thevalidateTokenClaimsmethod improve audience validation and error handling.The modifications in the
validateTokenClaimsmethod include:
- Retrieving the allowed JWT assertion audiences from the configuration.
- Returning an error if no audiences are configured, indicating that the JWT cannot be validated.
- Iterating through the audience claims and comparing them against the allowed audiences using a constant-time comparison.
These changes enhance error handling by providing clearer feedback when the server is not set up to accept JWT assertions. The audience validation logic has been improved to ensure that the JWT assertion contains at least one of the allowed audience values, preventing timing attacks.
integration/authorize_jwt_bearer_test.go (1)
415-415: LGTM! The change aligns with the new audience validation approach.The modification replaces the
TokenURLfield in the configuration structure withAllowedJWTAssertionAudiences, initialized with a slice containingtokenURL. This change indicates a shift towards validating JWT assertions against allowed audiences, rather than a single token URL, which may impact how the authorization logic is tested.config.go (2)
305-305: LGTM!The new interface
AllowedJWTAssertionAudiencesProvideris a good addition to enhance the functionality related to JWT validation by allowing contexts to specify which audiences are acceptable when validating requests.
306-307: LGTM!The new method
GetAllowedJWTAssertionAudiencesaligns with the purpose of the new interfaceAllowedJWTAssertionAudiencesProvider.client_authentication_strategy.go (5)
25-25: LGTM!The change in the
Configinterface to includeAllowedJWTAssertionAudiencesProviderinstead ofTokenURLProvideris a good shift in focus towards audience validation for JWT assertions.
280-280: LGTM!The changes in the
doAuthenticateAssertionParseAssertionJWTBearermethod to prioritize audience validation and return an error if the audience list is empty enhance the security of the authentication process by ensuring that only assertions with valid audiences are accepted.Also applies to: 282-283, 288-288
331-363: LGTM!The new method
doAuthenticateAssertionJWTBearerClaimAudienceenhances the security of the authentication process by ensuring that only assertions with valid audiences are accepted. It iterates through the claims' audience and checks against the provided audience list, returning an error if no valid match is found.
323-326: LGTM!Calling the new
doAuthenticateAssertionJWTBearerClaimAudiencemethod in thedoAuthenticateAssertionParseAssertionJWTBearermethod to verify the audience claim aligns with the purpose of the new method and enhances the security of the authentication process.
331-331: LGTM!The addition of the new method
doAuthenticateAssertionJWTBearerClaimAudienceto theDefaultClientAuthenticationStrategystruct aligns with the purpose of the struct and enhances its functionality.config_default.go (3)
98-101: LGTM!The change in the
Configstruct to replace theTokenURLfield withAllowedJWTAssertionAudiencesreflects a shift from a single token URL to a more flexible configuration that accommodates multiple audiences for JWT assertions, enhancing the security and compatibility of the authorization server with various client authentication methods.
280-281: LGTM!The change in the
GetAllowedJWTAssertionAudiencesmethod to return the newAllowedJWTAssertionAudiencesfield instead of the deprecatedTokenURLindicates a broader adjustment in the API, aligning it with the new configuration structure.
655-655: LGTM!The change in the type assertion in the variable declarations at the end of the file to reflect the new provider for the allowed JWT assertion audiences, replacing the previous
TokenURLProviderwithAllowedJWTAssertionAudiencesProvider, aligns with the new configuration structure and interface changes.handler/rfc7523/handler_test.go (2)
78-78: LGTM!The changes to the
oauth2.Configstructure and the corresponding updates to the test assertions align with the transition from using a singleTokenURLfor JWT validation to implementing a more flexible approach withAllowedJWTAssertionAudiences. This enhances audience validation and improves the overall security framework for client authentication.Also applies to: 337-339
855-858: LGTM!The changes to the
oauth2.Configstructure align with the transition from using a singleTokenURLfor JWT validation to implementing a more flexible approach withAllowedJWTAssertionAudiences. This enhances audience validation and improves the overall security framework for client authentication.client_authentication_test.go (2)
700-702: LGTM!The changes to the
Configstruct, including the addition of theAllowedJWTAssertionAudiencesfield and the removal of theTokenURLfield, indicate a shift towards stricter validation of JWT assertions by explicitly defining acceptable audiences. This improves security and compliance with authentication standards.
574-576: LGTM!The expanded error message associated with the
expectErrfield provides more detailed information regarding potential issues with theclient_assertionvalue, specifying various reasons for failure, including integrity verification issues, usage timing constraints, and validation of the 'aud' claim against expected values. This enhances the clarity of error reporting, making it easier for developers to diagnose authentication failures.
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.