Regression with URL encode client credentials

[petergphillips]

Describe the bug
We are using https://spring.io/projects/spring-security-oauth as an oauth2 provider.
In separate projects we are then using the spring security webclient provided by this project.
We tried upgrading to Spring Boot 2.5.2 from 2.5.0, which bundled Spring Security 5.5.1 instead of 5.5.0 and noticed that our applications broke and our client authentication attempts were rejected by our oauth2 provider.

It appears that the change in e6c268a now causes both the client id and client secret to be url encoded. This means that any client secrets containing those special characters then don't work any more as the oauth2 provider doesn't url decode the client id / secret before testing them.

To Reproduce
Upgrade to Spring Security 5.5.1 with a client secret with special characters (e.g. space, quote) that will be url encoded. The basic authentication header will then be sent with the client id / secret url encoded as well as base64 encoded. This breaks oauth2 providers that aren't expecting the basic authentication header elements to be url encoded too.

Expected behavior
We expect that the client id and client secret are not url encoded before being transmitted. That is what the spring security oauth provider is expecting and is current behaviour.
https://datatracker.ietf.org/doc/html/rfc2617#section-2 specifies that the client id and secret are base64 encoded before being transmitted anyway, so it seems odd to url encode them and then base64 encode them immediately afterwards anyway.

The equivalent ticket for our oauth2 provider in spring-attic/spring-security-oauth#1826 is still open, indicating that there hasn't been a fix there for the server. Reading the comment in #9610 (comment):

applying the fix now may break existing applications that are using non-compliant providers.

which is exactly what has happened to us.

[petergphillips]

I guess at the very least, there should be an option in the configuration to choose whether the oauth2 provider is expecting the client id / secret to be url encoded before being base64 encoded so that we can turn it off for our provider.

[rainerfrey-inxmail]

See also #9610 (comment)

We'll come up with a strategy that does not break existing applications.

[rpaasche]

BTW. Same for 5.4.7.

[jgrandja]

@petergphillips @rainerfrey-inxmail @rpaasche

As per spec, in Section 2.3.1. Client Password:

Clients in possession of a client password, also known as a client
secret, MAY use the HTTP Basic authentication scheme as defined in
[RFC2617] to authenticate with the authorization server. The client
identifier is encoded using the "application/x-www-form-urlencoded"
encoding algorithm per Appendix B, and the encoded value is used as
the username; the client secret is encoded using the same algorithm
and used as the password. The authorization server MUST support the
HTTP Basic authentication scheme for authenticating clients that were
issued a client secret.

This bug was reported in gh-9610 and was fixed in 5.5 and backported to older branches.

Regarding my comment:

We're going to apply this fix in 5.5 since this is a bug. We'll come up with a strategy that does not break existing applications.

Unfortunately, this fix has broken your existing applications but this is a bug and needed to be fixed on the client. The provider is non-compliant and should be fixed there.

In order to get the client working with a non-compliant provider, you can customize the Token Request.
See the reference for customizing the DefaultAuthorizationCodeTokenResponseClient.

You would supply a custom Converter<OAuth2AuthorizationCodeGrantRequest, RequestEntity<?>> to DefaultAuthorizationCodeTokenResponseClient.setRequestEntityConverter(). Also see OAuth2AuthorizationCodeGrantRequestEntityConverter.setHeadersConverter().

I'm going to close this since the issue is with the non-compliant provider.

[rpaasche]

For the record the code to restore the old behavior:

 @Bean
    @Order(Ordered.HIGHEST_PRECEDENCE)
    public OAuth2AccessTokenResponseClient<OAuth2AuthorizationCodeGrantRequest> accessTokenResponseClient()
    {
        var accessTokenResponseClient = new DefaultAuthorizationCodeTokenResponseClient();
        accessTokenResponseClient.setRequestEntityConverter(new CustomRequestEntityConverter());

        var tokenResponseHttpMessageConverter = new OAuth2AccessTokenResponseHttpMessageConverter();

        RestTemplate restTemplate = new RestTemplate(Arrays.asList(new FormHttpMessageConverter(), tokenResponseHttpMessageConverter));
        restTemplate.setErrorHandler(new OAuth2ErrorResponseErrorHandler());

        accessTokenResponseClient.setRestOperations(restTemplate);
        accessTokenResponseClient.setRequestEntityConverter(new CustomRequestEntityConverter());
        return accessTokenResponseClient;
    }

public class WebSecurityConfiguration extends WebSecurityConfigurerAdapter
{
    private final CorsProperties corsProperties;

    @Override
    protected void configure(final HttpSecurity http) throws Exception
    {
        http.oauth2Client(client -> client.authorizationCodeGrant(codeGrant -> codeGrant.accessTokenResponseClient(authorizationCodeTokenResponseClient())))
.oauth2Login(oauth2Login -> oauth2Login.loginPage(...).
                                                   .tokenEndpoint(t -> t.accessTokenResponseClient(accessTokenResponseClient())))
[...]
    }

public class CustomRequestEntityConverter implements Converter<OAuth2AuthorizationCodeGrantRequest, RequestEntity<?>>
{
    private OAuth2AuthorizationCodeGrantRequestEntityConverter defaultConverter;

    public CustomRequestEntityConverter()
    {
        defaultConverter = new OAuth2AuthorizationCodeGrantRequestEntityConverter();
    }

    @Override
    public RequestEntity<?> convert(OAuth2AuthorizationCodeGrantRequest req)
    {
        RequestEntity<?> entity = defaultConverter.convert(req);

        var headers = new HttpHeaders();
        headers.addAll(entity.getHeaders());

        if (ClientAuthenticationMethod.CLIENT_SECRET_BASIC.equals(req.getClientRegistration().getClientAuthenticationMethod())
                        || ClientAuthenticationMethod.BASIC.equals(req.getClientRegistration().getClientAuthenticationMethod()))
        {
            String clientId = req.getClientRegistration().getClientId();
            String clientSecret = req.getClientRegistration().getClientSecret();
            headers.setBasicAuth(clientId, clientSecret);
        }

        return new RequestEntity<>(entity.getBody(), headers, entity.getMethod(), entity.getUrl());
    }
}

[rainerfrey-inxmail]

Regarding my comment:

We're going to apply this fix in 5.5 since this is a bug. We'll come up with a strategy that does not break existing applications.

Unfortunately, this fix has broken your existing applications but this is a bug and needed to be fixed on the client. The provider is non-compliant and should be fixed there.

Seeing that this bug has only very recently been fixed in Spring's Provider and client, knowing that the previous version worked fine with most providers (esp. Keycloak that was often referenced by the Spring team before the new authorisation server project), just saying "the provider is wrong, not our fault it broke" sounds fairly smug. Especially after announcing to try to find a "strategy to not break applications". At the very least this should have been clearly marked as breaking change in the release notes and the what's new section of the documentation. It wouldn't have hurt that much to guard it with an environment property either.

[petergphillips]

I would echo those sentiments. We are using the Spring Security OAuth provider that has an outstanding bug for that issue. It is apparently fixed in the replacement, but we're not willing to switch production to using an experimental server.
We've just started getting warnings about CVE-2021-22119 because we've downgraded to 5.5.0 so will have to investigate whether we can put a workaround in Spring Security OAuth (in a similar vein to GluuFederation/oxAuth#677) to be lenient and try the client secret twice (once as is and then once decoded first).

[patrickhuy]

Indeed this is rather disappointing this change breaks all users of the relevant code in Spring security in a patch version without it being labeled as a breaking change.
I understand that you consider it to be a bug, however it was not a regression and I believe many users of Spring security expect the current behavior of NOT encoding the clientId and clientSecret, especially since apparently many OAuth providers don't expect them to be encoded - that might be a bug but it also appears to be reality - and it's kind of unexpected that they need to be form encoded before being base64 encoded.

Therefore I believe this sudden change in behavior is doing more harm than good. Please reconsider your stance on this, at least making it easier to change/toggle (like #10042 suggests) would be greatly appreciated.

[jgrandja]

@petergphillips @rainerfrey-inxmail @rpaasche @patrickhuy

Apologies for the pain in upgrading. This ticket should have been marked as a breaking change so I apologize for that miss.

I'm going to reopen this and we'll investigate what we can do to make everyone happy :)

We'll report back here after we have a change proposal to gather feedback and votes.

[sjohnr]

@petergphillips @rainerfrey-inxmail @rpaasche @patrickhuy

Hello everyone. One thing that would be a big help right now is information around environments that are experiencing this issue. If you wouldn't mind sharing the following items, it would be very helpful for putting together a proposal to fix the regression without reverting the original fix:

1. What provider are you using, including version and relevant settings or brief description of settings?
2. What backend or service is used to manage credentials, and how are clients configured to utilize those credentials?
3. What response is being received on the client side, including http status and oauth error code (if any)?

In particular, we're looking to make sure we know what providers are experiencing the issue to uncover any blind spots in our knowledge of those providers, and also to make sure we fix the issue in a way that supports both compliant and non-compliant providers in the easiest manner possible.

Also, a related question that came up is:

4. Is it feasible or infeasible to regenerate the client credentials such that they only contain URL-safe characters?

If it is infeasible, please share details. This will help in understanding the scope of the issue and difficulty of a workaround in our community.

[rpaasche]

1+2 Sorry but that's an existing OAuth Server from an customer. It's a black box for us.
3) Http-> 403 Authorization failed
4) That's an option indeed, the breaking change was just unexpected

[mpsz76]

The OAuth application is .NET box. Receiving 400 Error, BAD_REQUEST

[rainerfrey-inxmail]

Thanks for looking into this.

1. Keycloak. Currently 13.0.1. I didn't see any related changes in 14 though.
2. Keycloak, Client credentials are in an application.yml in a Kubernetes secret
3. 401 unauthorized, Keycloak logs "invalid client credentials"
4. it involves reconfiguring a bunch of applications managed by different teams, so possible but not desirable. We use a shared library were we applied the workaround of providing a header converter without encoding, enabled by an environment property. A lot of these applications are not affected, as they use Keycloak Spring Security Adapter which is also non-compliant. Only applications using service users for backend-todbackend calls are affected right now.

An environment property that enables encoding would be my preferred solution, I'd be fine with enabled by default.

[sjohnr]

Thanks for everyone's input so far on this. After careful consideration and discussion, we have decided to revert the original bug fix in each of the release branches it was back-ported to, because it breaks passivity. The following versions are affected by the regression:

• 5.5.1
• 5.4.7
• 5.3.10.RELEASE
• 5.2.11.RELEASE

However, the fix will remain applied to main and released with 5.6, and is already available in 5.6.0-M1. See this comment for examples on how to work around this for non-compliant providers once you upgrade to 5.6 or experience the issue in one of the above affected versions.

I do apologize for the inconvenience this has caused. If you have any questions, feel free to reply to this thread.

[frankjkelly]

@sjohnr thanks for the work-around but when I try that my pre-existing SpringBootTest test cases now fail with

Caused by: org.springframework.beans.factory.NoSuchBeanDefinitionException: No qualifying bean of type 'org.springframework.security.oauth2.client.registration.ReactiveClientRegistrationRepository' available: expected at least 1 bean which qualifies as autowire candidate. Dependency annotations: {}
	at app//org.springframework.beans.factory.support.DefaultListableBeanFactory.raiseNoMatchingBeanFound(DefaultListableBeanFactory.java:1801)
	at app//org.springframework.beans.factory.support.DefaultListableBeanFactory.doResolveDependency(DefaultListableBeanFactory.java:1357)

My SpringBootTest classes are annotated as follows

@SpringBootTest
@AutoConfigureMockMvc
@Import({ ServletKeycloakAuthUnitTestingSupport.class })
public class ContainerControllerTest {

[sjohnr]

@frankjkelly Sorry to hear that you're having issues. I responded to your comment on stackoverflow.

[frankjkelly]

@sjohnr Thanks! Apologies for the cross-post

[hannah23280]

I guess at the very least, there should be an option in the configuration to choose whether the oauth2 provider is expecting the client id / secret to be url encoded before being base64 encoded so that we can turn it off for our provider.

So is this suggestion implemented in spring security 5.7 already?

[sjohnr]

@hanct unfortunately 5.7 doesn't have something like that, but for the possibility to add it to a later release, see gh-11440.