Customize when UserInfo is called

[youlabi]

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:
fde26e0

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.

[youlabi]

See related issues:

• Incorrect scope map fix #12144
• Incorrect scope map fix #12205

[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?

[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 ?

[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.

[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.

[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<Set> accessibleScopes that is empty by default
• They both have a setter setAccessibleScopes(Set accessibleScopes)
• Depending on whether accessibleScopes is empty or set, either call OidcUserRequestUtils.shouldRetrieveUserInfo(OidcUserRequest userRequest) or OidcUserRequestUtils.shouldRetrieveUserInfo(OidcUserRequest userRequest, Set accessibleScopes)
• OidcUserRequestUtils stores the default accessibleAcopes and uses them if none are provided (i.e. when calling the first method)

[youlabi]

@sjohnr , I have a question regardingyour fix fde26e0 for #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.

[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 :)

[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.

[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.

[mikelemikelo]

Any updates on this one?

[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.userInfoEndpoint is defined
• The userRequest.clientRegistration.authorizationGrantType is authorization_code (an access token is issued)
• The userRequest.clientRegistration.scopes contains any of the following (these scopes were configured to be requested):
  • profile
  • email
  • address
  • phone
• The userRequest.accessToken.scopes indicates any of the following scopes have been granted (contains any of the following):
  • profile
  • email
  • address
  • phone

--

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

[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?

[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.

[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.

[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 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.

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?

[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.

[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).

[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....

[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

[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);
		};
	}
}

[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.