spring-projects / spring-security

Spring Security
http://spring.io/projects/spring-security
Apache License 2.0
8.45k stars 5.76k forks source link

Tokens for Client Credentials grant type should be cached only based on clientRegistrationId #14991

Closed pzgadzaj-equinix closed 4 days ago

pzgadzaj-equinix commented 2 weeks ago

Expected Behavior

  1. Configure spring oauth2 registration with authorization grant type set to "client credentials"
  2. Use Code described in https://docs.spring.io/spring-security/reference/servlet/oauth2/client/authorization-grants.html#_using_the_access_token to get "client credentials" Access token

Execution of the above code (from the link) twice, each time for different principal will result in two "client credentials" calls. I would expect that as client credentials token is user-agnostic, then Spring security does not cache such tokens based on principal

Current Behavior

Current implementation caches the "client credentials" tokens in a map which key is based on principal name (both JdbcOAuth2AuthorizedClientService and InMemoryOAuth2AuthorizedClientService)

Context

Workaround which might be an option is to extend InMemoryOAuth2AuthorizedClientService in a way that it considers "authorizationGrantType" of a clientRegistration. If it's "client_credentials" then the token is stored in a cache with key containing only the registration id, otherwise, current behavior would apply

sjohnr commented 1 week ago

@pzgadzaj-equinix thanks for reaching out!

The code example you linked from the docs shows binding the access token to a principal using an Authentication injected into the method. You can bind the token to any authentication by simply constructing your own instead. Even easier, you can simply provide a principal(String) (e.g. principal("anonymous") to the builder instead of a principal(Authentication). So the library already has this capability, I don't think we need to make any changes.

Do you feel that the docs should have a clear example of this use case? Would that have helped you find the solution more easily?

pzgadzaj-equinix commented 1 week ago

Initially i reported that as an improvement, but this is not an improvement but bug in my opinion

One more time, tokens retrieved in scope oauth flow with Client credentials grant are user agnostic. So if we imagine following situation:

  1. We have spring boot application which user OIDC login flow, and it communicates with other applications via two kind of REST rest endpoints:
    • one kind which uses m2m tokens
    • second kind which uses user tokens
  2. Both of clients are registered in separate entries within spring securty, with different grant type
  3. Now let's imagine that application is serving traffic for 1h. and within that one 1h it will serve traffic for 3600 distinctive users. When serving each user, application will make one call which requires m2m token and one call which will require user based token
  4. Serving such traffic will lead to storing of 3600 m2m tokens (and making 3600 token endpoint calls) in the cache and 3600 user based tokens

As for user based tokens, it's completely understandable that each user will have it;s own token stored in cache. But for m2m token is really bizarre, that when serving each distinctive user a fresh m2m token will be obtain from IDP. Especially that:

sjohnr commented 1 week ago

@pzgadzaj-equinix I'm sorry you're having trouble with the client_credentials support in Spring Security.

Initially i reported that as an improvement, but this is not an improvement but bug in my opinion

As I mentioned, the link you shared points to an example in the reference for binding the token to an Authentication which is injected into a Spring MVC @Controller endpoint. This would not be a bug as you describe, but an intentional choice by the user. The docs do not state that the behavior you expect would be achieved by this configuration. This is why I mention a documentation improvement as a possible solution.

In order to proceed, please share the code you are using as a minimal, reproducible sample if you believe you have found a bug, and we can discuss it further.

pzgadzaj-equinix commented 4 days ago

@sjohnr

I can provide minimal reproducible sample, but first i spotted one issue with my initial description. We don't use DefaultOAuth2AuthorizedClientManager (how it's described in https://docs.spring.io/spring-security/reference/servlet/oauth2/client/authorization-grants.html#_using_the_access_token).

Instead, we use: https://docs.spring.io/spring-security/reference/servlet/oauth2/client/authorized-clients.html - so ServletOAuth2AuthorizedClientExchangeFilterFunction which does not provide a way to nullify (or set to anonymous) the Authentication. It always rely on SecurityContextHolder (thread local storage)

sjohnr commented 4 days ago

@pzgadzaj-equinix

ServletOAuth2AuthorizedClientExchangeFilterFunction which does not provide a way to nullify (or set to anonymous) the Authentication. It always rely on SecurityContextHolder (thread local storage)

You can actually still affect the Authentication used by providing it to attributes via the ServletOAuth2AuthorizedClientExchangeFilterFunction.authentication() helper method (covered in the docs page you linked).

pzgadzaj-equinix commented 4 days ago

So when i have 10 m2m calls to be made, and each of them uses web client, then in each of them i have to put that code:

`@GetMapping("/") public String index() { String resourceUri = ...

Authentication anonymousAuthentication = new AnonymousAuthenticationToken(
        "anonymous", "anonymousUser", AuthorityUtils.createAuthorityList("ROLE_ANONYMOUS"));
String body = webClient
        .get()
        .uri(resourceUri)
        .attributes(authentication(anonymousAuthentication))
        .retrieve()
        .bodyToMono(String.class)
        .block();

...

return "index";

}`

There is no other way? And also, one more time, it's illogical that the solution which should serve use M2M tokens, cache the token on "user level"

sjohnr commented 4 days ago

@pzgadzaj-equinix thanks for your response.

At this point, I am going to close this issue since there is nothing to do. I do feel that there is room for improvement in the docs, but I will open specific issues for that at a later time as we are mostly enhancing the docs on a release-by-release basis. If you have any further questions, please open a stackoverflow question and share the link to that (so that others can find it) and I will be happy to answer.