Spring Security Claims contain an instance of java.net.URL and are used in hash-based containers

Hi,

class org.springframework.security.oauth2.core.user.OAuth2UserAuthority includes its attributes for the calculation of hashCode and equals. In case of org.springframework.security.oauth2.core.oidc.user.OidcUserAuthority extends OAuth2UserAuthority those attributes come from org.springframework.security.oauth2.core.oidc.OidcIdToken and org.springframework.security.oauth2.core.oidc.OidcUserInfo via org.springframework.security.oauth2.core.oidc.user.OidcUserAuthority.collectClaims(OidcIdToken, OidcUserInfo) function. It seems that due to https://github.com/spring-projects/spring-security/blob/main/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenDecoderFactory.java#L101 whenever (always?) the claims contain a org.springframework.security.oauth2.core.oidc.IdTokenClaimNames.ISS, it is there as an instance of class java.net.URL.

This is a problem, since due to historical reasons, URL performs DNS lookups whenever its equals/hashCode is used. Instances of URL must not be used in any containers requiring use of equals/hashCode.

A simple solution would be to change it to be an instance of java.net.URI. Would this break something? I didn't craft a pull request since I have no idea if this kind of change would be a major breaking change for spring-security...?

Comment From: jgrandja

Thanks for the report @jyrimatti. This will be a breaking change so I've scheduled it for 6.0.x.

I think the required change is to ensure the Map returned from OidcUserAuthority.collectClaims() replaces all URL values with URL.toExternalForm().

Would you be interested in submitting a PR for this?

Comment From: jyrimatti

Yes, sure, I can submit one.

Just to make sure: Are you certain this fix is enough? java.net.URL is not deprecated and I guess it's never going to be, but as https://bugs.java.com/bugdatabase/view_bug.do?bug_id=4434494 says:

People are encouraged to use URI for parsing and URI comparison, and leave URL class for accessing the URI itself, getting at the protocol handler, interacting with the protocol etc.

Thus, another possibility (?) would be to change createDefaultClaimTypeConverters methods in OidcIdTokenDecoderFactory and ReactiveOidcIdTokenDecoderFactory to use an uriConverter (or stringConverter if String is wanted) instead of urlConverter for IdTokenClaimNames.ISS.

Comment From: jgrandja

@jyrimatti Take a look at the OpenID Connect Core 1.0 spec, in section 2. ID Token:

iss REQUIRED. Issuer Identifier for the Issuer of the response. The iss value is a case sensitive URL using the https scheme that contains scheme, host, and optionally, port number and path components and no query or fragment components.

The iss is a URL so URL OidcIdToken.getIssuer() is correct and no changes are required in OidcIdTokenDecoderFactory or ReactiveOidcIdTokenDecoderFactory.

The only change required is what I proposed in previous comment - either URL.toExternalForm() OR convert to URI.

Another possible solution is to change the OAuth2UserAuthority.equals() and OAuth2UserAuthority.hashCode() methods to iterate over this.getAttributes() and do the conversion there instead. I think I like this option best.

Comment From: jyrimatti

Ok, sure. Made a PR: https://github.com/spring-projects/spring-security/pull/11030 Please let me know if there's something I can improve!