Implementations of OpaqueTokenIntrospector fail to URL encode client secret

[joelossher]

Describe the bug
Both the SpringOpaqueTokenIntrospector and NimbusOpaqueTokenIntrospector use the clientId and clientSecret to authenticate the calls to the authorization server.

This is done via basic authentication added using a BasicAuthenticationInterceptor. This does not perform any URL encoding.

This issue was addressed in #9610 for the token granting client, but persists for the introspection client.

The workaround at the moment is to manually encode the secret when instantiating the introspector.

To Reproduce

1. Set up a Spring Authorization Server with a client with a secret such as badSecret%
2. Configure a SpringOpaqueTokenIntrospector or NimbusOpaqueTokenIntrospector to use that client
3. Attempt to use the introspector with the Spring Authorization Server.
4. See the server respond with a 400 invalid_request error and see the following cause in the logs:

Caused by: java.lang.IllegalArgumentException: URLDecoder: Incomplete trailing escape (%) pattern
        at java.base/java.net.URLDecoder.decode(URLDecoder.java:230) ~[?:?]
        at java.base/java.net.URLDecoder.decode(URLDecoder.java:147) ~[?:?]
        at org.springframework.security.oauth2.server.authorization.web.authentication.ClientSecretBasicAuthenticationConverter.convert(ClientSecretBasicAuthenticationConverter.java:85) ~[spring-security-oauth2-authorization-server-1.3.2.jar!/:1.3.2]
        ... 103 more

Expected behavior
The token introspector should URL encode the secret.

[jzheaux]

Thanks for this report, @joelossher. I agree that this should be taken care of.

My primary concern is that there are applications like yours that are self-encoding already (like you are). To start encoding by default at this point would break those applications.

Because there is a constructor that accepts a RestOperations instance, there is already a workaround, should the fix cause any issues; we just don't want to break folks in a point release.

For that reason, I'll schedule this fix for 6.5.

[jzheaux]

Are you able to provide a PR to add the encoding and a test?

[ngocnhan-tran1996]

@jzheaux

SpringOpaqueTokenIntrospector use BasicAuthenticationInterceptor for encoding clientId and clientSecret.
So clientSecret will be encoded before calling BasicAuthenticationInterceptor. Please correct me if I am wrong

[jzheaux]

@ngocnhan-tran1996, my understanding is that the OAuth 2.0 spec indicates that the client id and secret should be URL encoded before performing the Basic Auth encoding of username and password together (pseudocode follows):

header = base64(urlencode(username) + ':' + urlencode(password))

So this ticket would make it so that the values are URL encoded before formulating the Authorization: Basic header.

Does that clarify, or am I misunderstanding your question?

[ngocnhan-tran1996]

@jzheaux

Let me make more clearly
Currently, SpringOpaqueTokenIntrospector use BasicAuthenticationInterceptor for encoding clientId and clientSecret

spring-security/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/introspection/SpringOpaqueTokenIntrospector.java

Line 88 in 9d2ca3d

restTemplate.getInterceptors().add(new BasicAuthenticationInterceptor(clientId, clientSecret));

And deep into this, it use HttpHeaders#encodeBasicAuth (belongs to Spring Framework) like your pseudocode

String credentialsString = username + ":" + password;
byte[] encodedBytes = Base64.getEncoder().encode(credentialsString.getBytes(charset));
return new String(encodedBytes, charset);

So if we want to encode password in side SpringOpaqueTokenIntrospector, we will do something like

restTemplate.getInterceptors().add(new BasicAuthenticationInterceptor(urlencode(clientId), urlencode(clientSecret)));

This part confuses me, we need to encode from Spring Framework side or Spring Security side if somewhere in Spring Security also use HttpHeaders#encodeBasicAuth

[jzheaux]

The encoding that Spring Framework provides is Base64 encoding. The OAuth spec indicates that the values should be URL encoded and then also thereafter Base64 encoded.

If you look carefully at the Framework code, you'll see that it is doing:

header = base64(username + ':' + password)

which is different from

header = base64(urlencode(username) + ':' + urlencode(password))

[ngocnhan-tran1996]

Thank for your explantion, can I work on this?

[jzheaux]

Sure! Note that it is targeted for 6.5, so the PR wouldn't merge for a couple of months.