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 https://github.com/spring-projects/spring-security/commit/e6c268add00bef40cc6f47d8963176f43b8a1de1 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 https://github.com/spring-projects/spring-security-oauth/issues/1826 is still open, indicating that there hasn't been a fix there for the server. Reading the comment in https://github.com/spring-projects/spring-security/issues/9610#issuecomment-822526400:
applying the fix now may break existing applications that are using non-compliant providers.
which is exactly what has happened to us.
Comment From: 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.
Comment From: rainerfrey-inxmail
See also https://github.com/spring-projects/spring-security/issues/9610#issuecomment-826911006
We'll come up with a strategy that does not break existing applications.
Comment From: rpaasche
BTW. Same for 5.4.7.
Comment From: 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.
Comment From: 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());
}
}
Comment From: 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.
Comment From: 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 https://github.com/GluuFederation/oxAuth/issues/677) to be lenient and try the client secret twice (once as is and then once decoded first).
Comment From: 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.
Comment From: 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.
Comment From: 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:
- What provider are you using, including version and relevant settings or brief description of settings?
- What backend or service is used to manage credentials, and how are clients configured to utilize those credentials?
- 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:
- 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.
Comment From: 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
Comment From: mpsz76
The OAuth application is .NET box. Receiving 400 Error, BAD_REQUEST
Comment From: 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.
Comment From: sjohnr
Thanks for the feedback. If anyone has a different setup, feel free to add. In the meantime, please see the following stackoverflow answers:
- How to customize the Authorization header of the OAuth2 token request (Servlet)
- How to customize the Authorization header of the OAuth2 token request (Reactive)
While we should be able to provide a temporary solution to revert to the old behavior for the 5.5.x upgrade path (and 5.4.x, 5.3.x, 5.2.x which were back-ported to), this issue will be permanent starting with 5.6. I'd like to know if my suggestions work to solve the issue permanently for you, or if I've missed anything and the answers could be enhanced to do so.
Note: I've successfully tested these suggestions against the spring-security-oauth project and am working to test them against Keycloak.
Comment From: boydingham
Howdy @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. ...
applying the fix now may break existing applications that are using non-compliant providers.
which is exactly what has happened to us.
So I'm guessing this is that same issue I told you about in Slack a couple weeks ago. Remember the one? Where I talked about trying to quote special characters in curl, sed, awk, Git Bash, etc?
So it wasn't a Windoze exclusive thing after all?
Comment From: rainerfrey-inxmail
Thanks for your efforts to improve this, very much appreciated.
Thanks for the feedback. If anyone has a different setup, feel free to add. In the meantime, please see the following stackoverflow answers:
That's more or less what I did (slightly adapted from a comment in the original issue), and it works fine. Only thing I did differently was to apply the adapted token response client conditionally on a custom environment property. I don't have a case using reactive right now, so haven't looked at that.
While we should be able to provide a temporary solution to revert to the old behavior for the 5.5.x upgrade path (and 5.4.x, 5.3.x, 5.2.x which were back-ported to), this issue will be permanent starting with 5.6.
To be honest, if you are able to provide a means to revert the behaviour in 5.5, I don't understand why that can't remain available in 5.x.
If someone from the Spring Security team could file an issue with Keycloak, it would possibly find a different level of attention than if I do it myself, and I believe you'd be better at describing it anyway. So if you have the time to do so, it would also be a big help.
Comment From: petergphillips
Sorry for delay in replying. 1. We are using our own deployment of https://spring.io/projects/spring-security-oauth (version 2.5.1.RELEASE) as an oauth2 provider. 2. We store the credentials in a database with our own maintenance for the credentials. We either manually inform the clients of the credentials or inject them automatically into kubernetes secrets for them. 3. Spring Security OAuth will return a 401 to the client. 4. We have our own custom client credentials policy so could technically adjust that strategy to remove non URL-safe characters. However we have around 120 different client credentials in use spread across three different environments so regenerating would take considerable time and effort. Furthermore this would represent a reduction in security by limiting the number of characters that could be used.
Luckily in our case we are in charge of the provider so were able to implement a retry policy on the client credentials in the provider. The generated client ids are already URL-safe so therefore we could just catch the BadCredentialsException and retry with a decoded client secret. This has then meant that we have been able to upgrade our internal clients to Spring Security 5.5.1 without any further issues.
Comment From: 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.
Comment From: 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 {
Comment From: sjohnr
@frankjkelly Sorry to hear that you're having issues. I responded to your comment on stackoverflow.
Comment From: frankjkelly
@sjohnr Thanks! Apologies for the cross-post
Comment From: 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?
Comment From: 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.