Revoke tokens when code is reused

[neochae1]

This PR adds an enhancement recommended per spec:

https://www.rfc-editor.org/rfc/rfc6749#section-4.1.2
"If an authorization code is used more than once,
the authorization server MUST deny the request and
SHOULD revoke (when possible) all tokens previously
issued based on that authorization code."

[jgrandja]

Thanks for the PR @neochae1.

I'd like to better understand the main use cases you require for custom validation.

the Spec recommends revoking the issued Token for a reused Code. However, the current library only checks if the Code has been reused.

The token is invalidated if another client attempted to use it. See this line.

spring-authorization-server/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/authentication/OAuth2AuthorizationCodeAuthenticationProvider.java

Line 137 in 13a6103

authorization = OAuth2AuthenticationProviderUtils.invalidate(authorization, authorizationCode.getToken());

Customization is necessary for more powerful or flexible Redirect URL and Code validation

The redirect_uri is validated in the Authorization Request and you're able to provide your own custom validator via OAuth2AuthorizationCodeRequestAuthenticationProvider.setAuthenticationValidator(). I don't see any reason for double validation in OAuth2AuthorizationCodeAuthenticationProvider other than exact matching between authorizationRequest.getRedirectUri() and redirect_uri parameter. Can you provide more details on why you need to also validate the redirect_uri in the token request?

[neochae1]

Not different client,
"The token is invalidated if another client attempted to use it. See this line."

If an authorization code is used multiple times with same client, the current implementation will reject the request with an "INVALID_GRANT" error because the code has already been invalidated.

spring-authorization-server/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/authentication/OAuth2AuthorizationCodeAuthenticationProvider.java

Line 152 in 13a6103

throw new OAuth2AuthenticationException(OAuth2ErrorCodes.INVALID_GRANT);

The specification recommends that all previously issued tokens based on that authorization code should be revoked when possible, as the code may have been used in an unusual or potentially exploitative manner.
Customizable validator can check the code more strong way and revoke token.

redirect_uri is is already validated as you mentiond.
But, default validator can check existing validating logic not a customizable reason.

[jgrandja]

@neochae1

The specification recommends that all previously issued tokens based on that authorization code should be revoked when possible ...

Yes, the specification recommends this (noted SHOULD) but it does not state MUST:

The client MUST NOT use the authorization code
more than once. If an authorization code is used more than
once, the authorization server MUST deny the request and SHOULD
revoke (when possible) all tokens previously issued based on
that authorization code.

IMO, a client attempting to use the code more than once may simply be an error on client side. If the client was successful on the initial exchange then it authenticating successfully, the token request validated and the authorization was successful. If a subsequent attempt occurs, then the request is rejected, as implemented per spec:

If an authorization code is used more than once, the authorization server MUST deny the request

However, I decided not to implement:

SHOULD revoke (when possible) all tokens previously issued based on that authorization code.

because it is a recommendation only and I felt it was too restrictive to invalidate the tokens for the same authenticated client. However, I could be convinced to enhance the existing implementation to align with the recommendation if you can provide a scenario (or test) that would support the recommendation?

But, default validator can check existing validating logic not a customizable reason.

I'm not sure I understand? What I'm looking for is a specific validation scenario that may be needed to enhance security.

FYI, I'm not against exposing a custom validator hook, but we need to verify concrete use cases where this would be used, otherwise the API might not be used that often. Furthermore, we prefer issues being logged first to see if there is demand for a specific feature before we accept/merge a PR. Since you submitted the PR first, we will consider it, but we need to verify the concrete use cases where this may be used.

[neochae1]

We are currently preparing for OpenID Certification. In the Base OP Profile, "Should" items in the Spec are categorized as Fail or Warning items. We are working on modifying the Revoking-related item in the "oidcc-codereuse-30seconds" test item, which is currently categorized as a Warning.

TC checks wether access token was revoked or not after duplicated using code.
https://www.certification.openid.net/log-detail.html?log=QhS1r8HrzY1Eufs&public=true

As you mentioned, this modification could be seen as a client-side error, but if we view it more aggressively, it could be seen as an error leading to token invalidation due to code leakage. Therefore, instead of directly including the code of the "SHOULD" item in the library code, we submitted a PR to add a customizable validator.

[jgrandja]

@neochae1 Thanks for the explanation. I now have a better understanding for the motivation of this change. I certainly want to help to get your product certified.

However, I'm still not convinced that exposing an authenticationValidator is needed at this point. What I would like to propose is if you can update the PR to conform to the recommendation:

The client MUST NOT use the authorization code more than once. If an authorization code is used more than once, the authorization server MUST deny the request and SHOULD revoke (when possible) all access tokens and refresh tokens previously issued based on that authorization code.

This would allow the conformance test to pass so you can move forward. Does this work for you?

[rishiraj88]

Thanks, @neochae1 , for explanation.

[neochae1]

@jgrandja

This PR was written to enable a custom validator,
which is not a part of the PR itself but rather a configuration for the tokenEndpoint.
The configuration could not be a part of PR like existing validator(OAuth2AuthorizationCodeRequestAuthenticationValidator)
It is a custom configuration.

Are you suggesting that the PR includes the necessary configuration?
We have verified that combining the PR with a custom validator could help resolve the issue.

However, I'm still not convinced that exposing an authenticationValidator is needed at this point. What I would like to propose is if you can update the PR to conform to the recommendation:

Custom validator can work like below,

1. Register validator to tokenEndpoint
2. Custom validator works at exchanging token with code
3. Custom validator extract OAuth2AuthorizationCode from context
4. Custom validator check whether OAuth2AuthorizationCode is isInvalidated()
5. If it was invalidated then revoke access/refresh token and throw OAuth2AuthenticationException
   Custom validator revokes token using bean OAuth2AuthorizationService.

Fail logs(original)
https://www.certification.openid.net/log-detail.html?log=pCW49zqhr6P9PGw&public=true

Success logs with custom validator
https://www.certification.openid.net/log-detail.html?log=TgifiT7DqcbatPG&public=true

Code Snippet

	@Bean
	@Order(Ordered.HIGHEST_PRECEDENCE)
	public SecurityFilterChain authorizationServerSecurityFilterChain(HttpSecurity http) throws Exception {
		OAuth2AuthorizationServerConfiguration.applyDefaultSecurity(http);
		http.getConfigurer(OAuth2AuthorizationServerConfigurer.class)
				.oidc(Customizer.withDefaults())
				.tokenEndpoint(tokenEndpoint ->
						tokenEndpoint.authenticationProviders(authenticationValidator()));
        ...
    }

	private Consumer<List<AuthenticationProvider>> authenticationValidator() {
		return (authenticationProviders) ->
				authenticationProviders.forEach((authenticationProvider) -> {
					if (authenticationProvider instanceof OAuth2AuthorizationCodeAuthenticationProvider provider) {
						Consumer<OAuth2AuthorizationCodeAuthenticationContext> validator =
								new CodeAuthenticationValidator(this.authorizationService)
									.andThen(OAuth2AuthorizationCodeAuthenticationValidator.DEFAULT_AUTHORIZATION_CODE_VALIDATOR)
									.andThen(OAuth2AuthorizationCodeAuthenticationValidator.DEFAULT_REDIRECT_URI_VALIDATOR
								);
						provider.setAuthenticationValidator(validator);
					}
				});
	}
...
    public class CodeAuthenticationValidator implements Consumer<OAuth2AuthorizationCodeAuthenticationContext> {
    
        @Override
        public void accept(OAuth2AuthorizationCodeAuthenticationContext context) {
            this.validateAuthorizationCode(context);
        }
    
        private void validateAuthorizationCode(OAuth2AuthorizationCodeAuthenticationContext authenticationContext) {
            OAuth2Authorization.Token<OAuth2AuthorizationCode> authorizationCode = authenticationContext.getAuthorizationCode();
            if (authorizationCode.isInvalidated()) {
                Optional.ofNullable(this.authorizationService.findByToken(authorizationCode.getToken().getTokenValue(), AUTHORIZATION_CODE_TOKEN_TYPE))
                    .ifPresent(authorization -> {
                        //revoke all tokens related with the token
                        if (authorization.getAccessToken() != null) {
                            authorization = this.invalidateToken(authorization, authorization.getAccessToken().getToken());
                        }
                        if (authorization.getRefreshToken() != null) {
                            authorization = this.invalidateToken(authorization, authorization.getRefreshToken().getToken());
                        }
                        this.authorizationService.save(authorization);
                    });
    
                throw new OAuth2AuthenticationException(OAuth2ErrorCodes.INVALID_GRANT);
            }
        }

[jgrandja]

@neochae1 Apologies, as I don't think I was clear with my proposal:

What I would like to propose is if you can update the PR to conform to the recommendation....

As mentioned, I'm not convinced exposing an authenticationValidator() is needed at this point. So what I'm proposing is to remove the authenticationValidator() code and directly implement the access/refresh token invalidation in OAuth2AuthorizationCodeAuthenticationProvider, as per spec:

If an authorization code is used more than once, the authorization server MUST deny the request and SHOULD revoke (when possible) all access tokens and refresh tokens previously issued based on that authorization code.

Steps 4 and 5 as you noted:

4. Custom validator check whether OAuth2AuthorizationCode is isInvalidated()
5. If it was invalidated then revoke access/refresh token and throw OAuth2AuthenticationException
   Custom validator revokes token using bean OAuth2AuthorizationService.

Makes sense?

[neochae1]

@jgrandja
I have reviewed your proposal and updated the PR to incorporate your suggestions.

With this change conformance test pass without any warning.
https://www.certification.openid.net/log-detail.html?log=2v3wKyrtLNxQvQT&public=true

[jgrandja]

Thanks for the updates @neochae1. Please see review comments.

[jgrandja]

Can you try to integrate this logic into OAuth2AuthenticationProviderUtils.invalidate() so we can isolate the logic in one spot?

[neochae1]

It is difficult to merge the added logic with the existing one, since the existing "invalidate()" method invalidates a single token, typically an authorization code or a refresh token, and sometimes invalidates access tokens or authorization codes in addition to the refresh token.

Our requirement is to invalidate both access tokens and refresh tokens. Therefore, integrating the added logic with the existing "invalidate()" method is not feasible. Instead, we created a new method called "invalidateAuthorizationTokens()" that invalidates two types of tokens.

[jgrandja]

Our requirement is to invalidate both access tokens and refresh tokens. Therefore, integrating the added logic with the existing "invalidate()" method is not feasible.

The addition of the following should work?

static <T extends OAuth2Token> OAuth2Authorization invalidate(
		OAuth2Authorization authorization, T token) {

	OAuth2Authorization.Builder authorizationBuilder = OAuth2Authorization.from(authorization)
			.token(token,
					(metadata) ->
							metadata.put(OAuth2Authorization.Token.INVALIDATED_METADATA_NAME, true));

	if (OAuth2AuthorizationCode.class.isAssignableFrom(token.getClass())) {
		// TODO Invalidate access token (if available)


		// TODO Invalidate refresh token (if available)

	}

	return authorizationBuilder.build();
}

[neochae1]

It might work, but modifying invalidate() could cause it to lose its original functionality of invalidating a single token
For example, there may be code where invalidate() should only invalidate one token without affecting any other tokens in the sequence.

spring-authorization-server/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/authentication/OAuth2AuthorizationCodeAuthenticationProvider.java

Line 254 in 13a6103

authorization = OAuth2AuthenticationProviderUtils.invalidate(authorization, authorizationCode.getToken());

[neochae1]

@jgrandja
What do you think about my opinion regarding the requested modification for invalidate()?

[jgrandja]

@neochae1

modifying invalidate() could cause it to lose its original functionality of invalidating a single token

There are existing test cases that would fail if the current logic was changed incorrectly. I believe enhancing the logic will work for this additional use case. Please give it a try. My preference is to keep the logic in one spot.

[neochae1]

@jgrandja
Please review code.

I have merged the logic for invalidating tokens into the existing invalidate().
Also, I have relocated the logic for invalidating the code to the builder creation process for success scenario.

[neochae1]

@jgrandja
Please provide additional review feedback regarding the content you revised based on the method you proposed.

[jgrandja]

Please add a test for this case.

[neochae1]

I added logic to an existing test case to verify whether the access and refresh tokens have been invalidated.
(authenticateWhenInvalidatedCodeThenThrowOAuth2AuthenticationException)

[jgrandja]

Thanks for the updates @neochae1. After reviewing the changes, I was able to simplify the logic so I went ahead and applied the changes. This is now merged.

[neochae1]

Thank you for your code review and advice.
I have reviewed the code you rewrote, unlike my original code,
and confirmed that it works without any issues with the 1.1.0 library version.
https://www.certification.openid.net/log-detail.html?log=MztiVWHAbHDGO46&public=true

Thanks.