spring-projects / spring-security

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

Is it safe to store client password inside clientRegistration inside Authorized Client #15660

Closed dreamstar-enterprises closed 1 month ago

dreamstar-enterprises commented 1 month ago

I noticed that the class DefaultReactiveOAuth2AuthorizedClientManager relies on passing in Authorized Client instances to it, so it can pass this to a provider, that can re-authorize refresh tokens etc. I am assuming the OAuth Client needs the client secret to query the auth server again, e.g. when using refresh tokens to get another access token, and it gets this client secret from inside clientRegistration from the authorized client instance associated with every user session....

Is it safe to store my client_id and client_secret inside clientRegistrations inside every Authorized Client instance. I store these on Redis currently, since i use an OAuthClient, backed. by redis spring session.

Or can I set the password to null, and DefaultReactiveOAuth2AuthorizedClientManager will get the client secret from elsewhere for use by the provider?

Stored in an attribute in a session.

{
   "in-house-auth-server":{
      "@class":"org.springframework.security.oauth2.client.OAuth2AuthorizedClient",
      "clientRegistration":{
         "@class":"org.springframework.security.oauth2.client.registration.ClientRegistration",
         "registrationId":"in-house-auth-server",
         "clientId":"bff_client",
         **"clientSecret":"password",**       <<-- SURELY, NOT SAFE?
         "clientAuthenticationMethod":{
            "value":"client_secret_basic"
         },
         "authorizationGrantType":{
            "value":"authorization_code"
         },
         "redirectUri":"http://localhost:7080/bff/login/oauth2/code/in-house-auth-server",
         "scopes":[
            "openid",
            "offline_access"
         ],
         "providerDetails":{
            "authorizationUri":"http://localhost:7080/auth/oauth2/authorize",
            "tokenUri":"http://localhost:7080/auth/oauth2/token",
            "userInfoEndpoint":{
               "uri":"http://localhost:7080/auth/userinfo",
               "authenticationMethod":{
                  "value":"header"
               },
               "userNameAttributeName":"sub"
            },
            "jwkSetUri":"http://localhost:7080/auth/oauth2/jwks",
            "issuerUri":"http://localhost:7080/auth",
            "configurationMetadata":{
               "issuer":"http://localhost:7080/auth",
               "authorization_endpoint":"http://localhost:7080/auth/oauth2/authorize",
               "token_endpoint":"http://localhost:7080/auth/oauth2/token",
               "userinfo_endpoint":"http://localhost:7080/auth/userinfo",
               "end_session_endpoint":"http://localhost:7080/auth/connect/logout",
               "jwks_uri":"http://localhost:7080/auth/oauth2/jwks",
               "revocation_endpoint":"http://localhost:7080/auth/oauth2/revoke"
            }
         },
         "clientName":"BFF-Server"
      },
      "principalName":"userName.00@gmail.com",
      "accessToken":{
         "tokenType":"Bearer

Describe the bug A clear and concise description of what the bug is.

To Reproduce Steps to reproduce the behavior.

Expected behavior A clear and concise description of what you expected to happen.

Sample

A link to a GitHub repository with a minimal, reproducible sample.

Reports that include a sample will take priority over reports that do not. At times, we may require a sample, so it is good to try and include a sample up front.

sjohnr commented 1 month ago

@dreamstar-enterprises, I believe this is closely related to gh-14360 so I'm going to close this as a duplicate.

If you would like to ask a question, that would be better suited to Stack Overflow. We prefer to use GitHub issues only for bugs and enhancements. Feel free to update this issue with a link to the re-posted question (so that other people can find it) or add a minimal sample that reproduces this issue if you feel this is a genuine bug.

I would also mention that if you have concerns about security issues, those are best raised by following the security policy instead of discussing here or on Stack Overflow.

dreamstar-enterprises commented 1 month ago

Thanks for replying. Since this is security related I will refrain from posting on Github.

I do wonder and ask that if I set the password to null, when serialising it, before it gets to Redis, will the refresh token mechansim, and other mechanisms used by DefaultReactiveOAuth2AuthorizedClientManager still work - if that relies on AuthroizedClient object in each session, to get the client secret?

dreamstar-enterprises commented 1 month ago

I have gotten around this by removing the password at serialisation, and adding it from clientregistrationrepository on deserialiation