Describe the bug org.springframework.security.oauth2.client.oidc.userinfo.OidcReactiveOAuth2UserService#getUserInfo calls OidcUserRequestUtils::shouldRetrieveUserInfo that uses the scopes in the OAuth2AccessToken to determine whether it should fetch user infos or not.
In the non-reactive OidcUserService shouldRetrieveUserInfo was extended to return true if either, the access token has no scopes or the accessibleScopes is empty: https://github.com/spring-projects/spring-security/commit/fde26e003a648a670be40c690a028970f0d1137b
This fix was not applied to the reactive version
To Reproduce Set up OIDC server to return an Opaque Token, which automatically has no scopes.
Expected behavior Userinfo endpoint is called
Sample
No sample present
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.
Comment From: youlabi
See related issues:
- https://github.com/spring-projects/spring-security/issues/12144
- https://github.com/spring-projects/spring-security/issues/12205
Comment From: sjohnr
Thanks for the report, @youlabi! I think the OidcReactiveOAuth2UserService needs to be aligned with OidcUserService by applying both this fix and the addition of accessibleScopes.
Would you be interested in submitting a PR?
Comment From: youlabi
I can not promise anything :). Can you please forward me to some resources and things I need to know, on how to submit a PR ?
Regarding the actual change. This seems to me like it is exactly the same logic duplicated in two places. Should this be kept this way? This seems highly inefficient, and the next fix will again have to be transferred and likely be forgotten.
Another question I am unsure about: why does the reactive stack make use of a static class? Is this just legacy ?
Comment From: sjohnr
Hi @youlabi, thanks for the reply.
Another question I am unsure about: why does the reactive stack make use of a static class? Is this just legacy ?
Sadly, I don't know the answer to that question. It's just however the developer chose to do it.
Regarding the actual change. This seems to me like it is exactly the same logic duplicated in two places. Should this be kept this way? This seems highly inefficient, and the next fix will again have to be transferred and likely be forgotten.
It is OK to duplicate logic when needed. It is not a goal of Spring Security to eliminate all such duplication as this would be an untenable goal. However, I see your point that duplication can cause issues like this. In this case, it looks like the static class can be used to reduce duplication, so if you wish, you can refactor the code so both OidcReactiveOAuth2UserService and OidcUserService use the same static class and same logic. This refactoring is possible because the static class is package-private.
I can not promise anything :). Can you please forward me to some resources and things I need to know, on how to submit a PR ?
Thank you so much! That's completely okay, I'm here to help. Take a look at the contributing document which will walk you through most of what you need to know.
I will mention that because you have correctly categorized this issue as a bug, it would be nice to base the change on the 5.7.x branch, but this is not required. If it's easier for you to use main, I can rebase the change for you. We also use a forward-port process to propagate changes from 5.7.x to main. This is again something you don't need to worry about, I can handle it for you!
Any other questions, please let me know.
Comment From: sjohnr
Oh, I just remembered one more thing. If/when we make a fix on the 5.7.x branch, we probably will not want to add the setter for accessibleScopes because it would be a new API, and bug fixes should not introduce new APIs. So when performing the change, make sure not to add a setter for it. We can add the setter in the 6.2 release since the change will not be a breaking one.
Looking over the code in OidcUserRequestUtils, I'm also noticing that the logic does not line up with what's in OidcUserService. That makes this change a little trickier. Feel free to continue discussing if you want to talk through the changes.
Comment From: youlabi
Thanks for all the input.
I noticed that shouldRetrieveUserInfo is alwyas private and or final, or to put it differently, it is behaviour that can not be overwritten by the user. So I can not extend OidcUserService and adapt the behaviour.
Is this because, this is standardized OIDC / OAuth2.0 behaviour? This might explain why it is private / static code that can not be changed.
What do you think about this implementation:
- Use OidcUserRequestUtils for both OidcUserService and OidcReactiveOAuth2UserService
- Both OidcUserService and OidcReactiveOAuth2UserService have a property Optional
Comment From: youlabi
@sjohnr , I have a question regardingyour fix https://github.com/spring-projects/spring-security/commit/fde26e003a648a670be40c690a028970f0d1137b for https://github.com/spring-projects/spring-security/issues/12144
If https://www.rfc-editor.org/rfc/rfc6749#section-5.1 says, that "returning no scopes means all requested scopes were granted", shouldn't the AccessToken actually be populated with all of those granted scopes earlier, if the scopes in the response was not set. It seems to me, that the fix is in fact incorrect, and more of a workaround because the way I understand it, we should have probably filled the at some place earlier.
What if the server returns an access token with an empty scope list. Then assuming that all scopes were granted is not right.
Comment From: youlabi
Unless of course an empty scope list is not permitted, in which case the workaround is fine, however, it still seems like a workaround instead of a fix :)
Comment From: youlabi
Also, I know see what you mean by
"Looking over the code in OidcUserRequestUtils, I'm also noticing that the logic does not line up with what's in OidcUserService."
In my opinion the only way to not introduce a severe breaking change in older versions while somehow solving the bug is to: - For OidcReactiveOAuth2UserService set the default accessibleScopes to the scopes provided by the getClientRegistration (incorrect behaviour, however perevents breaking change). This is done in OidcReactiveOAuth2UserService , so that OidcUserRequestUtils is "clean" - Allow this default to be overwritten using setAccessibleScopes
As a result, we have no change in behaviour, however, we can set accessibleScopes to empty if we like, so that there is some alignement between the two.
The accessToken scopes = empty check however needs to be introduced. This is a breaking change, in the sense that, more requests are done to the userInfo endpoint than before.
Comment From: sjohnr
Hi @youlabi.
If https://www.rfc-editor.org/rfc/rfc6749#section-5.1 says, that "returning no scopes means all requested scopes were granted", shouldn't the AccessToken actually be populated with all of those granted scopes earlier, if the scopes in the response was not set.
Just to point this out (in case you weren't aware), this behavior was changed previously for security reasons, so we won't be following the spec here. We don't want to assume that you were granted all the scopes you requested, we have no way of getting that assumption right.
I'm still working through your other comments and will follow up this week.
Comment From: mikelemikelo
Any updates on this one?
Comment From: sjohnr
No updates @mikelemikelo. I didn't get around to responding to questions from @youlabi. Apologies for that.
Actually, as I look at this again I'm not sure there is a path forward that doesn't introduce a breaking change. While I understand that there is an expectation to call the UserInfo endpoint in certain situations, I don't see a way to harmonize that with the situations where the reactive implementation currently calls UserInfo. Also, because the reactive version's behavior is not compatible with the non-reactive one, I don't think we can/should share any code between the two right now.
Before we proceed, I'd like to get feedback on whether the following outlined behavior would result in the UserInfo endpoint being called when expected. For this discussion, please ignore what either implementation currently does.
--
The UserInfo Endpoint should be called if all of the following conditions are true:
- The
userRequest.clientRegistration.providerDetails.userInfoEndpointis defined - The
userRequest.clientRegistration.authorizationGrantTypeisauthorization_code(an access token is issued) - The
userRequest.clientRegistration.scopescontains any of the following (these scopes were configured to be requested): profileemailaddressphone- The
userRequest.accessToken.scopesindicates any of the following scopes have been granted (contains any of the following): profileemailaddressphone
--
If this would not be the correct behavior for your use case, can you provide clarification on what specific situation you're aiming to solve? @mikelemikelo @youlabi
Comment From: sjohnr
@youlabi do you have any feedback on the above? Does the above set of conditions describe the situation in which you're wanting to see the UserInfo endpoint called?
Comment From: DarrenForsythe
Following on this as my company has started to see some significant movement to Spring Cloud Gateway and the switch to Reactive implementations have started to hit this.
@sjohnr Those assumptions do not work for our use case. Scopes are simply not returned in this case from our IdP, and I'd assume enterprise users are expecting the same functionality in this circumstance between imperative and reactive implementations given how much spring security handles in this area.
Additionally there is no hint provided why the userInfo endpoint isn't called when switching, given how deep and the lack of configuration options to override this check I've resorted to a hack classloading so a custom implementation is loaded before the libraries... which isn't great.
Just to point this out (in case you weren't aware), this behavior was changed previously for security reasons, so we won't be following the spec here.
Is this documented anywhere? I am a bit shocked on not following the spec, but then not allowing configurations to revert back to spec behaviour.
Happy to contribute a PR on this if there is a consensus on how.
Comment From: mrlonis
Just to point this out (in case you weren't aware), this behavior was changed previously for security reasons, so we won't be following the spec here.
Is this the referenced security issue? https://spring.io/blog/2022/10/31/cve-2022-31690-privilege-escalation-in-spring-security-oauth2-client
This seems to be the offending lines: https://github.com/spring-projects/spring-security/blob/main/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/userinfo/OidcUserRequestUtils.java#L63-L64
By overriding the if (AuthorizationGrantType.AUTHORIZATION_CODE.equals(clientRegistration.getAuthorizationGrantType())) { to just always return true; I can work around this issue by duplicating this file/package in my local project with the custom code to hack the class loading. This forces it always to be called.
In the case of our enterprise authorization server, I believe this comment is true: AS returned no scopes, either because none were granted or because requested == granted. All scopes are granted for the request and not returned.
This was a wild issue to chase down. Previously, the application was running spring-security 5.4.6 as a servlet application and is now running 5.7.11 as a reactive application. I believe the reason I am seeing this crop up is because of CVE-2022-31690, since the old code would map the authorization scopes to the principal when the Authorization Server would respond with an empty or missing scope parameter.
Comment From: sjohnr
@sjohnr Those assumptions do not work for our use case. Scopes are simply not returned in this case from our IdP, and I'd assume enterprise users are expecting the same functionality in this circumstance between imperative and reactive implementations given how much spring security handles in this area.
@DarrenForsythe, apologies as the above flow in my comment was from some time ago, so I am not fully remembering what the intent of my question to the reporter was, but I think I was describing the "normal" flow.
The flow I proposed should allow for the UserInfo endpoint to be called in your case, assuming we introduce a setter for accessibleScopes as the servlet implementation has, which would allow you to customize such that the UserInfo endpoint would be called for an empty list. The issue with the change in logic is that it would be breaking for the reactive version.
Is this the referenced security issue? https://spring.io/blog/2022/10/31/cve-2022-31690-privilege-escalation-in-spring-security-oauth2-client
@mrlonis that's correct.
By overriding the
if (AuthorizationGrantType.AUTHORIZATION_CODE.equals(clientRegistration.getAuthorizationGrantType())) {to just alwaysreturn true;I can work around this issue by duplicating this file/package in my local project with the custom code to hack the class loading. This forces it always to be called.
I'm wondering if part of the (or the entire) solution here would be to introduce a way to simply override the logic of shouldRetrieveUserInfo. This would add the needed flexibility and not be a breaking change, as we wouldn't need to introduce a change to the logic at all. It also seems like it would remove the need for your workaround. What do you think?
Comment From: mrlonis
@sjohnr That would work well. Would the addition of a new property be considered "non-breaking"?
I am imagining something like this:
return CollectionUtils.containsAny(userRequest.getAccessToken().getScopes(),
userRequest.getClientRegistration().getScopes()) ||
NEW_PROPERY;
where NEW_PROPERTY is a boolean value mapped to something like spring.security.oauth2.client.call-user-info-for-empty-scopes.
I'm sure there is a better way to do it but that's just something off the top of my head.
Comment From: sjohnr
@mrlonis we could add the ability to set a lambda to replace the logic of the existing method. I created a branch to illustrate my idea (see this commit).
Would the addition of a new property be considered "non-breaking"?
It would not be a new property (see above), so adding a new setter would be considered a new feature but it is non-breaking. So we would need to put this in the next feature release (6.3).
Comment From: knoobie
@sjohnr I would highly highly appreciate this change; I had to resort to go this route to enforce the call to user info..
setAccessibleScopes(Collections.emptySet()); // Make it empty so that ALL claims; even openid enforces a call to /userinfo....
Comment From: shiven1703
Just in case if someone is looking for the version which is working and makes the call to userinfo endpoint -> Try version 5.7.0.
I was facing issues in version 5.7.10
Comment From: sjohnr
@shiven1703,
Just in case if someone is looking for the version which is working
Thanks, but we do not recommend downgrading versions as a workaround, because older versions do not include security fixes like the one mentioned above (see blog post).
If you need to work around the issue while waiting for this enhancement, please give the following configuration a try and let me know if it helps you:
@Configuration
@EnableWebFluxSecurity
public class SecurityConfiguration {
private static final Set<String> ACCESSIBLE_SCOPES = Set.of(OidcScopes.OPENID, OidcScopes.PROFILE);
@Bean
public SecurityWebFilterChain securityWebFilterChain(ServerHttpSecurity http) {
// ...
}
@Bean
public ReactiveOAuth2UserService<OidcUserRequest, OidcUser> userService() {
var delegate = new OidcReactiveOAuth2UserService();
return (userRequest) -> {
var accessToken = userRequest.getAccessToken();
if (!CollectionUtils.isEmpty(accessToken.getScopes())) {
return delegate.loadUser(userRequest);
}
var modifiedAccessToken = new OAuth2AccessToken(
accessToken.getTokenType(),
accessToken.getTokenValue(),
accessToken.getIssuedAt(),
accessToken.getExpiresAt(),
ACCESSIBLE_SCOPES);
var modifiedUserRequest = new OidcUserRequest(
userRequest.getClientRegistration(),
modifiedAccessToken,
userRequest.getIdToken());
return delegate.loadUser(modifiedUserRequest);
};
}
}
Comment From: sjohnr
Note: I have converted this issue into an enhancement as discussed above. See workaround for a possible way to cause UserInfo to be called prior to 6.3.