Fix to save after encoding the secret when registering the client

[uc4w6c]

In the Client Registration Endpoint the authentication method is client_secret_basic or client_secret_post or client_secret_jwt When , it should encode the secret and save it.
Since it is currently stored as plain text, the client cannot issue tokens.

[jgrandja]

The extension point for customizing client metadata before it's saved to RegisteredClientRepository is OidcClientRegistrationAuthenticationProvider.setRegisteredClientConverter(). It's not currently documented in the reference manual but there is an open issue gh-1044 that provides sample code on how to configure.

[jgrandja]

@uc4w6c Not all RegisteredClient's will be assigned a client secret during dynamic registration. The extension point OidcClientRegistrationAuthenticationProvider.setRegisteredClientConverter() gives flexibility for consuming applications to assign a client secret (if necessary) and encode using the strategy of their choice. But it's not limited to just the client secret since any client metadata can be customized at that point. Adding setPasswordEncoder() would be redundant as it can be customized using setRegisteredClientConverter().

I'm going to close this PR as the preference is to leverage the extension point setRegisteredClientConverter() for any client metadata customization.

[uc4w6c]

@jgrandja
Thanks for your comment.

I don't want to customize the client metadata before saving it to the RegisteredClientRepository.

I would like to solve the bug that the newly created client cannot be authenticated because the secret is not encoded at the time of client registration, but the secret is decoded at the time of client authentication.

I have created a simple example below.
https://github.com/uc4w6c/spring-authorization-server-pr-1056

OidcClientRegistrationAuthenticationProvider.java does not set the secret encoded with PasswordEncoder.
However, ClientSecretAuthenticationProvider.java decodes the secret with PasswordEncoder after getting the clientSecret from registeredClientRepository and compares it with the secret sent by the user.

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

Line 116 in 67305ab

if (!this.passwordEncoder.matches(clientSecret, registeredClient.getClientSecret())) {

Also, if I were to encode the secret with the RegisteredClientConverter, the encoded secret would be returned as json in the response, which is not a desirable behavior.
So I think should encode the secret only in the argument to RegisteredClientRepository.

[jgrandja]

@uc4w6c

Also, if I were to encode the secret with the RegisteredClientConverter, the encoded secret would be returned as json in the response, which is not a desirable behavior.

You could customize the response by setting a custom OidcClientRegistrationEndpointFilter.setAuthenticationSuccessHandler() that decodes the encoded clientSecret. I realize this might not be ideal but it is a workaround.

I'm really not keen on exposing OidcClientRegistrationAuthenticationProvider.setPasswordEncoder(). However, exposing OidcClientRegistrationAuthenticationProvider.setClientRegistrationConverter() might be the option to consider.

I'll re-open this to look into this further.

[uc4w6c]

@jgrandja
Thanks for reopening.
Sure.I would be happy to have OidcClientRegistrationAuthenticationProvider.setClientRegistrationConverter().
But I think the problem this time is a bug in the default configuration of Spring Authorization Server.
And usually when we encode a secret we do it by hashing.
Hashed values cannot be decoded.
(For example BCryptPasswordEncoder has no decode method.)

My explanation so far may have been difficult to understand, so I will describe the phenomenon again.

---

1. Below is the OIDC configuration.
   Default settings except add a clientRegistrationEndpoint.

    OAuth2AuthorizationServerConfiguration.applyDefaultSecurity(http);
    http.getConfigurer(OAuth2AuthorizationServerConfigurer.class)
      .oidc(oidc -> oidc.clientRegistrationEndpoint(Customizer.withDefaults()));

2. Register a new client with the /connect/register endpoint.
   The parameters look like this.

'{ "redirect_uris": "http://127.0.0.1/callback", "grant_types": ["authorization_code", "client_credentials"] }' 

{
	"client_id": "UuUnmeUMcXzQ7BMYKyIYBsbvuFaP4rbHUYUVSudClqk",
	"client_secret": "o-TpPEQ4UWOLyAcnWqX0vec2S7_A2FjJpL3fVLBAAQJP3WRVAlIB53UmpGD9ydyH",
        // Omit other items
}

3. Issue a token with the newly created client with the /oauth2/token endpoint.

curl -X POST "http://localhost:8080/oauth2/token" -H 'Content-Type: application/x-www-form-urlencoded' -d "grant_type=client_credentials"  -u 'UuUnmeUMcXzQ7BMYKyIYBsbvuFaP4rbHUYUVSudClqk:o-TpPEQ4UWOLyAcnWqX0vec2S7_A2FjJpL3fVLBAAQJP3WRVAlIB53UmpGD9ydyH'

An error occurs.

java.lang.IllegalArgumentException: There is no PasswordEncoder mapped for the id "null"
	at org.springframework.security.crypto.password.DelegatingPasswordEncoder$UnmappedIdPasswordEncoder.matches(DelegatingPasswordEncoder.java:289) ~[spring-security-crypto-6.0.1.jar:6.0.1]
	at org.springframework.security.crypto.password.DelegatingPasswordEncoder.matches(DelegatingPasswordEncoder.java:237) ~[spring-security-crypto-6.0.1.jar:6.0.1]
	at org.springframework.security.oauth2.server.authorization.authentication.ClientSecretAuthenticationProvider.authenticate(ClientSecretAuthenticationProvider.java:116) ~[spring-security-oauth2-authorization-server-1.0.0.jar:1.0.0]
	at org.springframework.security.authentication.ProviderManager.authenticate(ProviderManager.java:182) ~[spring-security-core-6.0.1.jar:6.0.1]
	at org.springframework.security.oauth2.server.authorization.web.OAuth2ClientAuthenticationFilter.doFilterInternal(OAuth2ClientAuthenticationFilter.java:122) ~[spring-security-oauth2-authorization-server-1.0.0.jar:1.0.0]
  // Omit

---

The reason for the error is that the /connect/register endpoint does not encode the secret, but the /oauth2/token endpoint decodes. with default settings.
Why you don't want to add OidcClientRegistrationAuthenticationProvider.setPasswordEncoder() when We already have ClientSecretAuthenticationProvider.setPasswordEncoder()?

Pardon my broken English.

[uc4w6c]

@jgrandja
I really want to solve this problem.
Is there anything I can do?

[jgrandja]

Thank you for your patience @uc4w6c.

I'm really not keen on exposing OidcClientRegistrationAuthenticationProvider.setPasswordEncoder(). However, exposing OidcClientRegistrationAuthenticationProvider.setClientRegistrationConverter() might be the option to consider.

After further analysis, adding OidcClientRegistrationAuthenticationProvider.setPasswordEncoder() is the correct solution.

Please see review comments for requested changes.

[jgrandja]

Please update copyright year to 2023

[jgrandja]

Please revert as wildcard imports are not allowed.

[jgrandja]

Please update the javadoc as the passwordencoder is not used to validate the clientSecret but instead used to encode the clientSecret.
Also, please add @since 1.1.0

[jgrandja]

Use RegisteredClient.from(registeredClient) to make a copy and then update clientSecret. Also, the RegisteredClient should only be copied/updated if the clientSecret is set, otherwise it should use the original returned from the converter.

[jgrandja]

Revert as wildcard imports are not allowed.

[jgrandja]

Please remove this test and add an integration test to OidcClientRegistrationTests.

Here's a template of the test:

// gh-1056
@Test
public void requestWhenClientRegistersWithSecretThenClientAuthenticationSuccess() throws Exception {
	// TODO Update the PasswordEncoder @Bean to PasswordEncoderFactories.createDelegatingPasswordEncoder()
	this.spring.register(AuthorizationServerConfiguration.class).autowire();

	// TODO
	// Create new client with client_credentials grant
	OidcClientRegistration clientRegistration = OidcClientRegistration.builder() ...

	// Register client
	OidcClientRegistration clientRegistrationResponse = registerClient(clientRegistration);

	// TODO
	// Use the newly registered client to obtain an access token using the client_credentials grant
	// if the access token is obtained then client authentication succeeded

}

[uc4w6c]

ClientConfigurationResponse.getClientSecret() returns plain Secret, but ClientRegitionrationRespone. getClientSecret() returns the encoded secret, so do not assert.

[uc4w6c]

@jgrandja
Thank you.
Since I have corrected it, please review it again.

[jgrandja]

Thanks for the updates @uc4w6c. FYI, I added a polish commit with some minor updates.

Also, the setPasswordEncoder() method could not be exposed in the patch releases 0.4.2 and 1.0.2, however, it is available in main targeted for 1.1.0 release.

[uc4w6c]

@jgrandja
Thanks for the merge and update.
Looking forward to the release.