Describe the bug
We have noticed that OidcClientInitiatedLogoutSuccessHandler does double encoding for logout uri. Similarly org.springframework.security.oauth2.client.endpoint.OAuth2PasswordGrantRequestEntityConverter#convert does double encoding.
For example, org.springframework.security.oauth2.client.endpoint.OAuth2PasswordGrantRequestEntityConverter#convert this method gets token uri from client registration. If Token URI is already encoded, it will re-encode it.
@Override
public RequestEntity<?> convert(OAuth2PasswordGrantRequest passwordGrantRequest) {
ClientRegistration clientRegistration = passwordGrantRequest.getClientRegistration();
HttpHeaders headers = OAuth2AuthorizationGrantRequestEntityUtils.getTokenRequestHeaders(clientRegistration);
MultiValueMap<String, String> formParameters = buildFormParameters(passwordGrantRequest);
URI uri = UriComponentsBuilder.fromUriString(clientRegistration.getProviderDetails().getTokenUri())
.build()
.toUri();
return new RequestEntity<>(formParameters, headers, HttpMethod.POST, uri);
}
Similarly, org.springframework.security.oauth2.client.oidc.web.logout.OidcClientInitiatedLogoutSuccessHandler#endpointUri this method causes problem.
It takes has already encoded URIs for endSessionEndpoint and postLogoutRedirectUri as argument and re-encode it again. So, in below key-cloak sample, it will have logout uri as
http://localhost:8080/auth/realms/Test%2520OIDC/protocol/openid-connect/logout. Notice %2520 in URI.
private String endpointUri(URI endSessionEndpoint, String idToken, URI postLogoutRedirectUri) {
UriComponentsBuilder builder = UriComponentsBuilder.fromUri(endSessionEndpoint);
builder.queryParam("id_token_hint", idToken);
if (postLogoutRedirectUri != null) {
builder.queryParam("post_logout_redirect_uri", postLogoutRedirectUri);
}
return builder.encode(StandardCharsets.UTF_8).build().toUriString();
}
To Reproduce
Use keycloak as Identity provider and create a realm with space. For example, "Test OIDC". Well-known configuration provides below data in response with encoded URIs.
{
"issuer": "http://localhost:8080/auth/realms/Test%20OIDC",
"authorization_endpoint": "http://localhost:8080/auth/realms/Test%20OIDC/protocol/openid-connect/auth",
"token_endpoint": "http://localhost:8080/auth/realms/Test%20OIDC/protocol/openid-connect/token",
"token_introspection_endpoint": "http://localhost:8080/auth/realms/Test%20OIDC/protocol/openid-connect/token/introspect",
"userinfo_endpoint": "http://localhost:8080/auth/realms/Test%20OIDC/protocol/openid-connect/userinfo",
"end_session_endpoint": "http://localhost:8080/auth/realms/Test%20OIDC/protocol/openid-connect/logout",
"jwks_uri": "http://localhost:8080/auth/realms/Test%20OIDC/protocol/openid-connect/certs",
"check_session_iframe": "http://localhost:8080/auth/realms/Test%20OIDC/protocol/openid-connect/login-status-iframe.html",
"grant_types_supported": [
"authorization_code",
"implicit",
"refresh_token",
"password",
"client_credentials"
],
"response_types_supported": [
"code",
"none",
"id_token",
"token",
"id_token token",
"code id_token",
"code token",
"code id_token token"
],
"subject_types_supported": [
"public",
"pairwise"
],
"id_token_signing_alg_values_supported": [
"PS384",
"ES384",
"RS384",
"HS256",
"HS512",
"ES256",
"RS256",
"HS384",
"ES512",
"PS256",
"PS512",
"RS512"
],
"id_token_encryption_alg_values_supported": [
"RSA-OAEP",
"RSA1_5"
],
"id_token_encryption_enc_values_supported": [
"A128GCM",
"A128CBC-HS256"
],
"userinfo_signing_alg_values_supported": [
"PS384",
"ES384",
"RS384",
"HS256",
"HS512",
"ES256",
"RS256",
"HS384",
"ES512",
"PS256",
"PS512",
"RS512",
"none"
],
"request_object_signing_alg_values_supported": [
"PS384",
"ES384",
"RS384",
"HS256",
"HS512",
"ES256",
"RS256",
"HS384",
"ES512",
"PS256",
"PS512",
"RS512",
"none"
],
"response_modes_supported": [
"query",
"fragment",
"form_post"
],
"registration_endpoint": "http://localhost:8080/auth/realms/Test%20OIDC/clients-registrations/openid-connect",
"token_endpoint_auth_methods_supported": [
"private_key_jwt",
"client_secret_basic",
"client_secret_post",
"tls_client_auth",
"client_secret_jwt"
],
"token_endpoint_auth_signing_alg_values_supported": [
"PS384",
"ES384",
"RS384",
"HS256",
"HS512",
"ES256",
"RS256",
"HS384",
"ES512",
"PS256",
"PS512",
"RS512"
],
"claims_supported": [
"aud",
"sub",
"iss",
"auth_time",
"name",
"given_name",
"family_name",
"preferred_username",
"email",
"acr"
],
"claim_types_supported": [
"normal"
],
"claims_parameter_supported": false,
"scopes_supported": [
"openid",
"address",
"email",
"microprofile-jwt",
"offline_access",
"phone",
"profile",
"roles",
"web-origins"
],
"request_parameter_supported": true,
"request_uri_parameter_supported": true,
"code_challenge_methods_supported": [
"plain",
"S256"
],
"tls_client_certificate_bound_access_tokens": true,
"introspection_endpoint": "http://localhost:8080/auth/realms/Test%20OIDC/protocol/openid-connect/token/introspect"
}
Expected behavior It should check if uri is already encoded or not instead of re-encoding the same.
Comment From: jgrandja
Thanks for the report @mayur9991
Use keycloak as Identity provider and create a realm with space. For example, "Test OIDC". Well-known configuration provides below data in response with encoded URIs.
This is only an issue when the realm name has a space, e.g. "Test OIDC". However, if the realm name was changed to "TestOIDC", then there is no issue.
Furthermore, the double-encoding happens in UriComponentsBuilder, which is a Spring Framework component.
I would recommend renaming the realm name to NOT have a space to get around this, which I feel makes a lot of sense as far as conventional naming goes.
If you would prefer...
It should check if uri is already encoded or not instead of re-encoding the same.
Please log an issue with Spring Framework issues.
Comment From: mayur9991
@jgrandja Thanks for the update.
Yes, If a space is removed from realm name, then there is no issue as URIs will not have any character which requires encoding.
I would recommend renaming the realm name to NOT have a space to get around this, which I feel makes a lot of sense as far as conventional naming goes.
Yes, that is also what we recommend as well, however we are migrating from spring-security-oauth to spring-security and existing enterprise customers who are already using our integration with keycloak, it would not be ideal upgrade scenario.
I ended up writing my own custom implementation around it to prevent double encoding for now.
Please log an issue with Spring Framework issues.
Thanks, I will do that.