From dd43d9198b5df655b41ceb7d00abb0ad920f7bfc Mon Sep 17 00:00:00 2001 From: Ashley Scopes <73482956+ascopes@users.noreply.github.com> Date: Sun, 15 Aug 2021 14:38:13 +0100 Subject: [PATCH] Amended treatment of OAuth2 'iss' claim Prior to this commit, the OAuth2 resource server code is failing any issuer that is not a valid URL. This does not correspond to https://datatracker.ietf.org/doc/html/rfc7662#page-7 which redirects to https://datatracker.ietf.org/doc/html/rfc7519#section-4.1.1, defining an issuer as being a "StringOrURI", which is defined at https://datatracker.ietf.org/doc/html/rfc7519#page-5 as being an "arbitrary string value" that "MUST be a URI" only for "any value containing a ':'". The issue currently is that an issuer that is not a valid URL may be provided, which will automatically result in the request being aborted due to being invalid. I have removed the check entirely, since while the claim could be invalid, it is still a response that the OAuth2 introspection endpoint has provided. In the liklihood that interpretations of this behaviour are different for the OAuth2 server implementation in use, this currently stops Spring Security from being able to be used at all without implementing a custom introspector from scratch. It is also worth noting that the spec does not specify whether it is valid to normalize issuers or not if they are valid URLs. This may cause other unintended side effects as a result of this change, so it is safer to disable it entirely. --- .../NimbusOpaqueTokenIntrospector.java | 31 ++++++++++++------- ...NimbusReactiveOpaqueTokenIntrospector.java | 31 ++++++++++++------- .../SpringOpaqueTokenIntrospector.java | 31 ++++++++++++------- ...SpringReactiveOpaqueTokenIntrospector.java | 31 ++++++++++++------- .../NimbusOpaqueTokenIntrospectorTests.java | 3 +- ...sReactiveOpaqueTokenIntrospectorTests.java | 3 +- .../SpringOpaqueTokenIntrospectorTests.java | 3 +- ...gReactiveOpaqueTokenIntrospectorTests.java | 3 +- 8 files changed, 80 insertions(+), 56 deletions(-) diff --git a/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/introspection/NimbusOpaqueTokenIntrospector.java b/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/introspection/NimbusOpaqueTokenIntrospector.java index 830ec77ee5..907f6f26fb 100644 --- a/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/introspection/NimbusOpaqueTokenIntrospector.java +++ b/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/introspection/NimbusOpaqueTokenIntrospector.java @@ -17,7 +17,6 @@ package org.springframework.security.oauth2.server.resource.introspection; import java.net.URI; -import java.net.URL; import java.time.Instant; import java.util.ArrayList; import java.util.Collection; @@ -207,7 +206,25 @@ public class NimbusOpaqueTokenIntrospector implements OpaqueTokenIntrospector { claims.put(OAuth2TokenIntrospectionClaimNames.IAT, iat); } if (response.getIssuer() != null) { - claims.put(OAuth2TokenIntrospectionClaimNames.ISS, issuer(response.getIssuer().getValue())); + // RFC-7662 page 7 directs users to RFC-7519 for defining the values of these + // issuer fields. + // https://datatracker.ietf.org/doc/html/rfc7662#page-7 + // + // RFC-7519 page 9 defines issuer fields as being 'case-sensitive' strings + // containing + // a 'StringOrURI', which is defined on page 5 as being any string, but + // strings containing ':' + // should be treated as valid URIs. + // https://datatracker.ietf.org/doc/html/rfc7519#section-2 + // + // It is not defined however as to whether-or-not normalized URIs should be + // treated as the same literal + // value. It only defines validation itself, so to avoid potential ambiguity + // or unwanted side effects that + // may be awkward to debug, we do not want to manipulate this value. Previous + // versions of Spring Security + // would *only* allow valid URLs, which is not what we wish to achieve here. + claims.put(OAuth2TokenIntrospectionClaimNames.ISS, response.getIssuer().getValue()); } if (response.getNotBeforeTime() != null) { claims.put(OAuth2TokenIntrospectionClaimNames.NBF, response.getNotBeforeTime().toInstant()); @@ -222,14 +239,4 @@ public class NimbusOpaqueTokenIntrospector implements OpaqueTokenIntrospector { return new OAuth2IntrospectionAuthenticatedPrincipal(claims, authorities); } - private URL issuer(String uri) { - try { - return new URL(uri); - } - catch (Exception ex) { - throw new OAuth2IntrospectionException( - "Invalid " + OAuth2TokenIntrospectionClaimNames.ISS + " value: " + uri); - } - } - } diff --git a/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/introspection/NimbusReactiveOpaqueTokenIntrospector.java b/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/introspection/NimbusReactiveOpaqueTokenIntrospector.java index b562c86308..a09f0c1c65 100644 --- a/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/introspection/NimbusReactiveOpaqueTokenIntrospector.java +++ b/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/introspection/NimbusReactiveOpaqueTokenIntrospector.java @@ -17,7 +17,6 @@ package org.springframework.security.oauth2.server.resource.introspection; import java.net.URI; -import java.net.URL; import java.time.Instant; import java.util.ArrayList; import java.util.Collection; @@ -174,7 +173,25 @@ public class NimbusReactiveOpaqueTokenIntrospector implements ReactiveOpaqueToke claims.put(OAuth2TokenIntrospectionClaimNames.IAT, iat); } if (response.getIssuer() != null) { - claims.put(OAuth2TokenIntrospectionClaimNames.ISS, issuer(response.getIssuer().getValue())); + // RFC-7662 page 7 directs users to RFC-7519 for defining the values of these + // issuer fields. + // https://datatracker.ietf.org/doc/html/rfc7662#page-7 + // + // RFC-7519 page 9 defines issuer fields as being 'case-sensitive' strings + // containing + // a 'StringOrURI', which is defined on page 5 as being any string, but + // strings containing ':' + // should be treated as valid URIs. + // https://datatracker.ietf.org/doc/html/rfc7519#section-2 + // + // It is not defined however as to whether-or-not normalized URIs should be + // treated as the same literal + // value. It only defines validation itself, so to avoid potential ambiguity + // or unwanted side effects that + // may be awkward to debug, we do not want to manipulate this value. Previous + // versions of Spring Security + // would *only* allow valid URLs, which is not what we wish to achieve here. + claims.put(OAuth2TokenIntrospectionClaimNames.ISS, response.getIssuer().getValue()); } if (response.getNotBeforeTime() != null) { claims.put(OAuth2TokenIntrospectionClaimNames.NBF, response.getNotBeforeTime().toInstant()); @@ -190,16 +207,6 @@ public class NimbusReactiveOpaqueTokenIntrospector implements ReactiveOpaqueToke return new OAuth2IntrospectionAuthenticatedPrincipal(claims, authorities); } - private URL issuer(String uri) { - try { - return new URL(uri); - } - catch (Exception ex) { - throw new OAuth2IntrospectionException( - "Invalid " + OAuth2TokenIntrospectionClaimNames.ISS + " value: " + uri); - } - } - private OAuth2IntrospectionException onError(Throwable ex) { return new OAuth2IntrospectionException(ex.getMessage(), ex); } diff --git a/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/introspection/SpringOpaqueTokenIntrospector.java b/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/introspection/SpringOpaqueTokenIntrospector.java index 81e9795d0b..a07ea82da0 100644 --- a/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/introspection/SpringOpaqueTokenIntrospector.java +++ b/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/introspection/SpringOpaqueTokenIntrospector.java @@ -17,7 +17,6 @@ package org.springframework.security.oauth2.server.resource.introspection; import java.net.URI; -import java.net.URL; import java.time.Instant; import java.util.ArrayList; import java.util.Arrays; @@ -187,7 +186,25 @@ public class SpringOpaqueTokenIntrospector implements OpaqueTokenIntrospector { (k, v) -> Instant.ofEpochSecond(((Number) v).longValue())); claims.computeIfPresent(OAuth2TokenIntrospectionClaimNames.IAT, (k, v) -> Instant.ofEpochSecond(((Number) v).longValue())); - claims.computeIfPresent(OAuth2TokenIntrospectionClaimNames.ISS, (k, v) -> issuer(v.toString())); + // RFC-7662 page 7 directs users to RFC-7519 for defining the values of these + // issuer fields. + // https://datatracker.ietf.org/doc/html/rfc7662#page-7 + // + // RFC-7519 page 9 defines issuer fields as being 'case-sensitive' strings + // containing + // a 'StringOrURI', which is defined on page 5 as being any string, but strings + // containing ':' + // should be treated as valid URIs. + // https://datatracker.ietf.org/doc/html/rfc7519#section-2 + // + // It is not defined however as to whether-or-not normalized URIs should be + // treated as the same literal + // value. It only defines validation itself, so to avoid potential ambiguity or + // unwanted side effects that + // may be awkward to debug, we do not want to manipulate this value. Previous + // versions of Spring Security + // would *only* allow valid URLs, which is not what we wish to achieve here. + claims.computeIfPresent(OAuth2TokenIntrospectionClaimNames.ISS, (k, v) -> v.toString()); claims.computeIfPresent(OAuth2TokenIntrospectionClaimNames.NBF, (k, v) -> Instant.ofEpochSecond(((Number) v).longValue())); Collection authorities = new ArrayList<>(); @@ -204,14 +221,4 @@ public class SpringOpaqueTokenIntrospector implements OpaqueTokenIntrospector { return new OAuth2IntrospectionAuthenticatedPrincipal(claims, authorities); } - private URL issuer(String uri) { - try { - return new URL(uri); - } - catch (Exception ex) { - throw new OAuth2IntrospectionException( - "Invalid " + OAuth2TokenIntrospectionClaimNames.ISS + " value: " + uri); - } - } - } diff --git a/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/introspection/SpringReactiveOpaqueTokenIntrospector.java b/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/introspection/SpringReactiveOpaqueTokenIntrospector.java index edc0965a6a..1f0ac77e7f 100644 --- a/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/introspection/SpringReactiveOpaqueTokenIntrospector.java +++ b/oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/introspection/SpringReactiveOpaqueTokenIntrospector.java @@ -17,7 +17,6 @@ package org.springframework.security.oauth2.server.resource.introspection; import java.net.URI; -import java.net.URL; import java.time.Instant; import java.util.ArrayList; import java.util.Arrays; @@ -146,7 +145,25 @@ public class SpringReactiveOpaqueTokenIntrospector implements ReactiveOpaqueToke (k, v) -> Instant.ofEpochSecond(((Number) v).longValue())); claims.computeIfPresent(OAuth2IntrospectionClaimNames.ISSUED_AT, (k, v) -> Instant.ofEpochSecond(((Number) v).longValue())); - claims.computeIfPresent(OAuth2IntrospectionClaimNames.ISSUER, (k, v) -> issuer(v.toString())); + // RFC-7662 page 7 directs users to RFC-7519 for defining the values of these + // issuer fields. + // https://datatracker.ietf.org/doc/html/rfc7662#page-7 + // + // RFC-7519 page 9 defines issuer fields as being 'case-sensitive' strings + // containing + // a 'StringOrURI', which is defined on page 5 as being any string, but strings + // containing ':' + // should be treated as valid URIs. + // https://datatracker.ietf.org/doc/html/rfc7519#section-2 + // + // It is not defined however as to whether-or-not normalized URIs should be + // treated as the same literal + // value. It only defines validation itself, so to avoid potential ambiguity or + // unwanted side effects that + // may be awkward to debug, we do not want to manipulate this value. Previous + // versions of Spring Security + // would *only* allow valid URLs, which is not what we wish to achieve here. + claims.computeIfPresent(OAuth2IntrospectionClaimNames.ISSUER, (k, v) -> v.toString()); claims.computeIfPresent(OAuth2IntrospectionClaimNames.NOT_BEFORE, (k, v) -> Instant.ofEpochSecond(((Number) v).longValue())); Collection authorities = new ArrayList<>(); @@ -163,16 +180,6 @@ public class SpringReactiveOpaqueTokenIntrospector implements ReactiveOpaqueToke return new OAuth2IntrospectionAuthenticatedPrincipal(claims, authorities); } - private URL issuer(String uri) { - try { - return new URL(uri); - } - catch (Exception ex) { - throw new OAuth2IntrospectionException( - "Invalid " + OAuth2IntrospectionClaimNames.ISSUER + " value: " + uri); - } - } - private OAuth2IntrospectionException onError(Throwable ex) { return new OAuth2IntrospectionException(ex.getMessage(), ex); } diff --git a/oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/introspection/NimbusOpaqueTokenIntrospectorTests.java b/oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/introspection/NimbusOpaqueTokenIntrospectorTests.java index 89215cefbe..e65971ca79 100644 --- a/oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/introspection/NimbusOpaqueTokenIntrospectorTests.java +++ b/oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/introspection/NimbusOpaqueTokenIntrospectorTests.java @@ -17,7 +17,6 @@ package org.springframework.security.oauth2.server.resource.introspection; import java.io.IOException; -import java.net.URL; import java.time.Instant; import java.util.Arrays; import java.util.Base64; @@ -146,7 +145,7 @@ public class NimbusOpaqueTokenIntrospectorTests { Arrays.asList("https://protected.example.net/resource")) .containsEntry(OAuth2TokenIntrospectionClaimNames.CLIENT_ID, "l238j323ds-23ij4") .containsEntry(OAuth2TokenIntrospectionClaimNames.EXP, Instant.ofEpochSecond(1419356238)) - .containsEntry(OAuth2TokenIntrospectionClaimNames.ISS, new URL("https://server.example.com/")) + .containsEntry(OAuth2TokenIntrospectionClaimNames.ISS, "https://server.example.com/") .containsEntry(OAuth2TokenIntrospectionClaimNames.SCOPE, Arrays.asList("read", "write", "dolphin")) .containsEntry(OAuth2TokenIntrospectionClaimNames.SUB, "Z5O3upPC88QrAjx00dis") .containsEntry(OAuth2TokenIntrospectionClaimNames.USERNAME, "jdoe") diff --git a/oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/introspection/NimbusReactiveOpaqueTokenIntrospectorTests.java b/oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/introspection/NimbusReactiveOpaqueTokenIntrospectorTests.java index e299d9be31..d72a77a408 100644 --- a/oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/introspection/NimbusReactiveOpaqueTokenIntrospectorTests.java +++ b/oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/introspection/NimbusReactiveOpaqueTokenIntrospectorTests.java @@ -17,7 +17,6 @@ package org.springframework.security.oauth2.server.resource.introspection; import java.io.IOException; -import java.net.URL; import java.time.Instant; import java.util.Arrays; import java.util.Base64; @@ -117,7 +116,7 @@ public class NimbusReactiveOpaqueTokenIntrospectorTests { Arrays.asList("https://protected.example.net/resource")) .containsEntry(OAuth2TokenIntrospectionClaimNames.CLIENT_ID, "l238j323ds-23ij4") .containsEntry(OAuth2TokenIntrospectionClaimNames.EXP, Instant.ofEpochSecond(1419356238)) - .containsEntry(OAuth2TokenIntrospectionClaimNames.ISS, new URL("https://server.example.com/")) + .containsEntry(OAuth2TokenIntrospectionClaimNames.ISS, "https://server.example.com/") .containsEntry(OAuth2TokenIntrospectionClaimNames.SCOPE, Arrays.asList("read", "write", "dolphin")) .containsEntry(OAuth2TokenIntrospectionClaimNames.SUB, "Z5O3upPC88QrAjx00dis") .containsEntry(OAuth2TokenIntrospectionClaimNames.USERNAME, "jdoe") diff --git a/oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/introspection/SpringOpaqueTokenIntrospectorTests.java b/oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/introspection/SpringOpaqueTokenIntrospectorTests.java index 0fd6a4934f..641e467dff 100644 --- a/oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/introspection/SpringOpaqueTokenIntrospectorTests.java +++ b/oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/introspection/SpringOpaqueTokenIntrospectorTests.java @@ -17,7 +17,6 @@ package org.springframework.security.oauth2.server.resource.introspection; import java.io.IOException; -import java.net.URL; import java.time.Instant; import java.util.Arrays; import java.util.Base64; @@ -150,7 +149,7 @@ public class SpringOpaqueTokenIntrospectorTests { Arrays.asList("https://protected.example.net/resource")) .containsEntry(OAuth2TokenIntrospectionClaimNames.CLIENT_ID, "l238j323ds-23ij4") .containsEntry(OAuth2TokenIntrospectionClaimNames.EXP, Instant.ofEpochSecond(1419356238)) - .containsEntry(OAuth2TokenIntrospectionClaimNames.ISS, new URL("https://server.example.com/")) + .containsEntry(OAuth2TokenIntrospectionClaimNames.ISS, "https://server.example.com/") .containsEntry(OAuth2TokenIntrospectionClaimNames.SCOPE, Arrays.asList("read", "write", "dolphin")) .containsEntry(OAuth2TokenIntrospectionClaimNames.SUB, "Z5O3upPC88QrAjx00dis") .containsEntry(OAuth2TokenIntrospectionClaimNames.USERNAME, "jdoe") diff --git a/oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/introspection/SpringReactiveOpaqueTokenIntrospectorTests.java b/oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/introspection/SpringReactiveOpaqueTokenIntrospectorTests.java index 7ff5b4ae5b..4511696b6d 100644 --- a/oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/introspection/SpringReactiveOpaqueTokenIntrospectorTests.java +++ b/oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/introspection/SpringReactiveOpaqueTokenIntrospectorTests.java @@ -17,7 +17,6 @@ package org.springframework.security.oauth2.server.resource.introspection; import java.io.IOException; -import java.net.URL; import java.time.Instant; import java.util.Arrays; import java.util.Base64; @@ -122,7 +121,7 @@ public class SpringReactiveOpaqueTokenIntrospectorTests { Arrays.asList("https://protected.example.net/resource")) .containsEntry(OAuth2IntrospectionClaimNames.CLIENT_ID, "l238j323ds-23ij4") .containsEntry(OAuth2IntrospectionClaimNames.EXPIRES_AT, Instant.ofEpochSecond(1419356238)) - .containsEntry(OAuth2IntrospectionClaimNames.ISSUER, new URL("https://server.example.com/")) + .containsEntry(OAuth2IntrospectionClaimNames.ISSUER, "https://server.example.com/") .containsEntry(OAuth2IntrospectionClaimNames.SCOPE, Arrays.asList("read", "write", "dolphin")) .containsEntry(OAuth2IntrospectionClaimNames.SUBJECT, "Z5O3upPC88QrAjx00dis") .containsEntry(OAuth2IntrospectionClaimNames.USERNAME, "jdoe")