Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Client credentials not correctly encoded in Basic Auth #9610

Closed
DrZ7 opened this issue Apr 12, 2021 · 11 comments · Fixed by #9791
Closed

Client credentials not correctly encoded in Basic Auth #9610

DrZ7 opened this issue Apr 12, 2021 · 11 comments · Fixed by #9791
Assignees
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: breaks-passivity A change that breaks passivity with the previous release type: bug A general bug
Milestone

Comments

@DrZ7
Copy link

DrZ7 commented Apr 12, 2021

Summary
OAuth2AuthorizationGrantRequestEntityUtils.getTokenRequestHeaders does not work properly if client credentials contain special characters. From RFC 6749:

Clients in possession of a client password 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
password is encoded using the same algorithm and used as the
password.

Actual Behavior
The client with client name or password containing special characters cannot login. The provider returns exception.

Expected Behavior
The client with client name or password containing special characters can be authenticated.

Configuration Sample
spring.security.oauth2.client.registration.sth.client-secret = sthUI=+2~fubar

Where
org.springframework.security.oauth2.client.endpoint.OAuth2AuthorizationGrantRequestEntityUtils.getTokenRequestHeaders(ClientRegistration)

Related
This is related to spring-attic/spring-security-oauth#1826

@DrZ7 DrZ7 added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Apr 12, 2021
@eleftherias
Copy link
Contributor

Thanks for reaching out @DrZ7.
Which provider are you using when you see the exception?
Could you also share the stacktrace here?

@eleftherias eleftherias added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Apr 12, 2021
@DrZ7
Copy link
Author

DrZ7 commented Apr 13, 2021

This ticket is the counterpart to spring-attic/spring-security-oauth#1826...
Our provider works according to specification, it decodes client name and password.
Replacing
headers.setBasicAuth(clientRegistration.getClientId(), clientRegistration.getClientSecret());
with
headers.setBasicAuth(URLEncoder.encode(clientRegistration.getClientId(), StandardCharsets.UTF_8), URLEncoder.encode(clientRegistration.getClientSecret(), StandardCharsets.UTF_8));
fixes the issue...

stack with special character password and without encoding:

org.springframework.security.oauth2.client.ClientAuthorizationException: [invalid_token_response] An error occurred while attempting to retrieve the OAuth 2.0 Access Token Response: 401 : [no body]
at org.springframework.security.oauth2.client.ClientCredentialsOAuth2AuthorizedClientProvider.getTokenResponse(ClientCredentialsOAuth2AuthorizedClientProvider.java:96)
at org.springframework.security.oauth2.client.ClientCredentialsOAuth2AuthorizedClientProvider.authorize(ClientCredentialsOAuth2AuthorizedClientProvider.java:85)
at org.springframework.security.oauth2.client.DelegatingOAuth2AuthorizedClientProvider.authorize(DelegatingOAuth2AuthorizedClientProvider.java:71)
at org.springframework.security.oauth2.client.AuthorizedClientServiceOAuth2AuthorizedClientManager.authorize(AuthorizedClientServiceOAuth2AuthorizedClientManager.java:142)

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Apr 13, 2021
@jzheaux
Copy link
Contributor

jzheaux commented Apr 16, 2021

I believe this can be addressed by supplying a custom headers converter:

@Bean
public OAuth2AuthorizedClientManager authorizedClientManager(
    ClientRegistrationRepository registrations,
    OAuth2AuthorizedClientRepository clients) {
  Converter<OAuth2ClientCredentialsGrantRequest, HttpHeaders> headersConverter = (request) -> {
      ClientRegistration registration = request.getClientRegistration();
      HttpHeaders headers = new HttpHeaders();
      headers.setAccept(Collections.singletonList(MediaType.APPLICATION_JSON));
      headers.setContentType(MediaType.APPLICATION_FORM_URLENCODED);
      headers.setBasicAuth(
          URLEncoder.encode(registration.getClientId(), StandardCharsets.UTF_8), 
          URLEncoder.encode(registration.getClientSecret(), StandardCharsets.UTF_8));
      return headers;
  };
  OAuth2ClientCredentialsGrantRequestEntityConverter entityConverter =
      new OAuth2ClientCredentialsGrantRequestEntityConverter();
  entityConverter.setHeadersConverter(headersConverter);
  DefaultClientCredentialsTokenResponseClient client = 
      new DefaultClientCredentialsTokenResponseClient();
  client.setRequestEntityConverter(entityConverter);
  OAuth2AuthorizedClientProvider authorizedClientProvider = 
      OAuth2AuthorizedClientProviderBuilder.builder()
          .clientCredentials((clientCredentials) -> clientCredentials
              .accessTokenResponseClient(client))
          .build();
  DefaultOAuth2AuthorizedClientManager authorizedClientManager =
      new DefaultOAuth2AuthorizedClientManager(registrations, clients);
  authorizedClientManager.setAuthorizedClientProvider(authorizedClientProvider);
  return authorizedClientManager;
}

Perhaps @jgrandja can provide a simpler configuration.

The spec does seem to indicate that URL encoding should be performed by default. If so, then applications using a non-compliant provider would need to provide a headers converter instead. @jgrandja do you think the default client behavior should change?

This question has cropped up on the authorization server side of things in a couple of different places recently.

@jzheaux jzheaux added the in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) label Apr 16, 2021
@jgrandja
Copy link
Contributor

jgrandja commented Apr 19, 2021

@DrZ7 The configuration that @jzheaux provided is correct and will work for the compliant provider you are using. I'm assuming you already applied a similar custom configuration and it's working?

I agree that OAuth2AuthorizationGrantRequestEntityUtils.getTokenRequestHeaders() should be updated to comply with the spec, but as @OrangeDog mentioned in this comment, applying the fix now may break existing applications that are using non-compliant providers.

Therefore, I've scheduled this fix for Spring Security 6.0 (Update: Spring Security 5.6).

@jgrandja jgrandja added this to the 6.x milestone Apr 19, 2021
@jgrandja jgrandja added type: enhancement A general enhancement and removed status: feedback-provided Feedback has been provided labels Apr 19, 2021
@jgrandja jgrandja removed their assignment Apr 19, 2021
@DrZ7
Copy link
Author

DrZ7 commented Apr 20, 2021

At the moment we are using our own OAuth2ClientCredentialsGrantRequestEntityConverter b/c setHeadersConverter is not yet available.

@jgrandja jgrandja modified the milestones: 6.x, 5.5.0 Apr 26, 2021
@jgrandja
Copy link
Contributor

@DrZ7 Regarding my last comment:

applying the fix now may break existing applications that are using non-compliant providers. Therefore, I've scheduled this fix for Spring Security 6.0.

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.

@jgrandja jgrandja self-assigned this Apr 26, 2021
@jgrandja jgrandja added type: bug A general bug and removed type: enhancement A general enhancement labels Apr 26, 2021
@jgrandja jgrandja modified the milestones: 5.5.0, 5.5.x May 14, 2021
@jgrandja jgrandja assigned sjohnr and unassigned jgrandja May 18, 2021
@jgrandja jgrandja modified the milestones: 5.5.x, 5.5.1 May 18, 2021
sjohnr added a commit that referenced this issue Jun 3, 2021
sjohnr added a commit that referenced this issue Jun 3, 2021
sjohnr added a commit that referenced this issue Jun 3, 2021
sjohnr added a commit to sjohnr/spring-security that referenced this issue Jun 3, 2021
sjohnr added a commit that referenced this issue Jun 8, 2021
sjohnr added a commit that referenced this issue Jul 20, 2021
sjohnr added a commit that referenced this issue Jul 20, 2021
sjohnr added a commit that referenced this issue Jul 20, 2021
sjohnr added a commit that referenced this issue Jul 20, 2021
@sjohnr sjohnr added type: breaks-passivity A change that breaks passivity with the previous release and removed status: backported An issue that has been backported to maintenance branches labels Jul 20, 2021
akohli96 pushed a commit to akohli96/spring-security that referenced this issue Aug 25, 2021
@AT92
Copy link

AT92 commented Dec 16, 2021

Hello together,
had the same issue with spring-security-oauth2-client-5.6.0 again.
After downgrading to 5.5.3 the problem was resolved, so I think the bug came again with new release.

@sjohnr
Copy link
Member

sjohnr commented Dec 16, 2021

had the same issue with spring-security-oauth2-client-5.6.0 again.

Hi @AT92. Can you describe what you mean by this? Are you saying it did not encode correctly, or it is now encoding and you don't want it to?

@rynoj
Copy link

rynoj commented Dec 22, 2021

Hi, we're facing the same. After updating to 5.6.0 credentials are being encoded again and we don't want it to (since the do contain some special characters)

@sjohnr
Copy link
Member

sjohnr commented Dec 22, 2021

@rynoj that is correct. See this comment on #10018. This change is permanent starting with 5.6. See this comment if you need help working around the issue with a non-compliant provider.

Also note that if you're using reactive, we've merged a change for #10130 which simplifies customizing headers beyond what the stackoverflow post linked above demonstrates.

@rynoj
Copy link

rynoj commented Dec 22, 2021

Ah thanks @sjohnr missed those comments! I'll have a look at the provided workarounds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: breaks-passivity A change that breaks passivity with the previous release type: bug A general bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants