Repository: SAP/cloud-security-xsuaa-integration
Pull request #309 (2020-06-19 14:18:27)

refactor jku validation
- increased test coverage


Patch:
diff --git a/java-security/src/main/java/com/sap/cloud/security/token/validation/validators/XsuaaJwtIssuerValidator.java b/java-security/src/main/java/com/sap/cloud/security/token/validation/validators/XsuaaJwtIssuerValidator.java
index ea437fb5..2605c963 100644
--- a/java-security/src/main/java/com/sap/cloud/security/token/validation/validators/XsuaaJwtIssuerValidator.java
+++ b/java-security/src/main/java/com/sap/cloud/security/token/validation/validators/XsuaaJwtIssuerValidator.java
@@ -4,7 +4,6 @@ import static com.sap.cloud.security.token.validation.ValidationResults.*;
 import static com.sap.cloud.security.xsuaa.Assertions.assertHasText;
 
 import java.net.URI;
-import java.net.URISyntaxException;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -38,27 +37,37 @@ class XsuaaJwtIssuerValidator implements Validator<Token> {
 	@Override
 	public ValidationResult validate(Token token) {
 		String tokenKeyUrl = token.getHeaderParameterAsString(TokenHeader.JWKS_URL);
+		URI jkuUri;
+
 		if (tokenKeyUrl == null || tokenKeyUrl.trim().isEmpty()) {
 			return createInvalid(
 					"Issuer validation can not be performed because Jwt token does not contain 'jku' header parameter.");
 		}
-		return matchesTokenKeyUrlDomain(tokenKeyUrl);
-	}
-
-	private ValidationResult matchesTokenKeyUrlDomain(String tokenKeyUrl) {
-		URI jkuUri;
 		try {
-			jkuUri = new URI(tokenKeyUrl);
-			if (jkuUri.getHost() != null && jkuUri.getHost().endsWith(domain)) {
-				return createValid();
-			}
-		} catch (URISyntaxException e) {
-			logger.error("Error: 'jku' header parameter '{}' does not provide a valid URI: {}.", tokenKeyUrl,
-					e.getMessage(), e);
+			jkuUri = URI.create(tokenKeyUrl);
+		} catch (IllegalArgumentException e) {
+			return createInvalid(
+					"Issuer validation can not be performed because Jwt token does not contain a valid uri as 'jku' header parameter.");
+		}
+		if (!matchesTokenKeyUrlDomain(jkuUri)) {
+			return createInvalid(
+					"Issuer is not trusted because 'jku' '{}' does not match uaa domain '{}' of the identity service.",
+					jkuUri, domain);
+		}
+		if (!matchesTokenKeyEndpoint(jkuUri)) {
+			return createInvalid(
+					"Jwt token does not contain a valid 'jku' header parameter.",
+					jkuUri, domain);
 		}
-		return createInvalid(
-				"Issuer is not trusted because 'jku' '{}' does not match uaa domain '{}' of the identity service.",
-				tokenKeyUrl, domain);
+		return createValid();
+	}
+
+	private boolean matchesTokenKeyUrlDomain(URI jkuUri) {
+		return jkuUri.getHost() != null && jkuUri.getHost().endsWith(domain);
+	}
+
+	private boolean matchesTokenKeyEndpoint(URI jkuUri) {
+		return jkuUri.getPath().endsWith("token_keys") && jkuUri.getQuery() == null && jkuUri.getFragment() == null;
 	}
 
 	@Override
diff --git a/spring-xsuaa/src/main/java/com/sap/cloud/security/xsuaa/token/authentication/XsuaaJwtDecoder.java b/spring-xsuaa/src/main/java/com/sap/cloud/security/xsuaa/token/authentication/XsuaaJwtDecoder.java
index 536cd5e3..52088de0 100644
--- a/spring-xsuaa/src/main/java/com/sap/cloud/security/xsuaa/token/authentication/XsuaaJwtDecoder.java
+++ b/spring-xsuaa/src/main/java/com/sap/cloud/security/xsuaa/token/authentication/XsuaaJwtDecoder.java
@@ -2,6 +2,7 @@ package com.sap.cloud.security.xsuaa.token.authentication;
 
 import static com.sap.cloud.security.xsuaa.token.TokenClaims.CLAIM_JKU;
 import static com.sap.cloud.security.xsuaa.token.TokenClaims.CLAIM_KID;
+import static org.springframework.util.StringUtils.isEmpty;
 
 import java.net.URI;
 import java.net.URISyntaxException;
@@ -18,6 +19,7 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.springframework.security.oauth2.core.OAuth2TokenValidator;
 import org.springframework.security.oauth2.jwt.*;
+import org.springframework.security.oauth2.jwt.NimbusJwtDecoder.JwkSetUriJwtDecoderBuilder;
 import org.springframework.util.Assert;
 
 import com.github.benmanes.caffeine.cache.Cache;
@@ -93,17 +95,17 @@ public class XsuaaJwtDecoder implements JwtDecoder {
 			String jku = tokenInfoExtractor.getJku(jwt);
 			String kid = tokenInfoExtractor.getKid(jwt);
 			String uaaDomain = tokenInfoExtractor.getUaaDomain(jwt);
-			return verifyTokenOnline(jwt.getParsedString(), jku, kid, uaaDomain);
+			return verifyToken(jwt.getParsedString(), jku, kid, uaaDomain);
 		} catch (JwtException e) {
-			return tryToVerifyWithOfflineKey(jwt.getParsedString(), e);
+			return tryToVerifyWithVerificationKey(jwt.getParsedString(), e);
 		}
 	}
 
-	private Jwt verifyTokenOnline(String token, String jku, String kid, String uaaDomain) {
+	private Jwt verifyToken(String token, String jku, String kid, String uaaDomain) {
 		try {
-			canVerifyWithOnlineKey(jku, kid, uaaDomain);
-			validateJKU(jku, uaaDomain);
-			Jwt verifiedToken = verifyWithOnlineKey(token, jku, kid);
+			canVerifyWithKey(jku, kid, uaaDomain);
+			validateJku(jku, uaaDomain);
+			Jwt verifiedToken = verifyWithKey(token, jku, kid);
 
 			return verifiedToken;
 		} catch (JwtValidationException ex) {
@@ -113,7 +115,7 @@ public class XsuaaJwtDecoder implements JwtDecoder {
 		}
 	}
 
-	private void canVerifyWithOnlineKey(String jku, String kid, String uaadomain) {
+	private void canVerifyWithKey(String jku, String kid, String uaadomain) {
 		if (jku != null && kid != null && uaadomain != null) {
 			return;
 		}
@@ -129,15 +131,18 @@ public class XsuaaJwtDecoder implements JwtDecoder {
 				String.join(", ", nullParams)));
 	}
 
-	private void validateJKU(String jku, String uaadomain) {
+	private void validateJku(String jku, String uaadomain) {
 		try {
 			URI jkuUri = new URI(jku);
 			if (jkuUri.getHost() == null) {
 				throw new JwtException("JKU of token is not valid");
 			} else if (!jkuUri.getHost().endsWith(uaadomain)) {
-				logger.warn("Error: Do not trust jku '{}' because it does not match uaa domain '{}'",
+				logger.warn("Error: Do not trust jku '{}' because it does not match uaa domain '{}'.",
 						jku, uaadomain);
-				throw new JwtException("JKU of token header is not trusted");
+				throw new JwtException("Do not trust 'jku' token header.");
+			} else if (!jkuUri.getPath().endsWith("token_keys") || !isEmpty(jkuUri.getQuery()) || !isEmpty(jkuUri.getFragment())) {
+				logger.warn("Error: Do not trust jku '{}' because it contains invalid path, query or fragment.", jku);
+				throw new JwtException("Jwt token does not contain a valid 'jku' header parameter: " + jkuUri);
 			}
 		} catch (URISyntaxException e) {
 			throw new JwtException("JKU of token header is not valid");
@@ -145,14 +150,14 @@ public class XsuaaJwtDecoder implements JwtDecoder {
 	}
 
 	@java.lang.SuppressWarnings("squid:S2259")
-	private Jwt verifyWithOnlineKey(String token, String jku, String kid) {
+	private Jwt verifyWithKey(String token, String jku, String kid) {
 		String cacheKey = jku + kid;
 		JwtDecoder decoder = cache.get(cacheKey, k -> this.getDecoder(jku));
 		return decoder.decode(token);
 	}
 
 	private JwtDecoder getDecoder(String jku) {
-		NimbusJwtDecoder.JwkSetUriJwtDecoderBuilder jwkSetUriJwtDecoderBuilder = NimbusJwtDecoder.withJwkSetUri(jku);
+		JwkSetUriJwtDecoderBuilder jwkSetUriJwtDecoderBuilder = NimbusJwtDecoder.withJwkSetUri(jku);
 		if (restOperations != null) {
 			jwkSetUriJwtDecoderBuilder.restOperations(restOperations);
 		}
@@ -161,15 +166,15 @@ public class XsuaaJwtDecoder implements JwtDecoder {
 		return jwtDecoder;
 	}
 
-	private Jwt tryToVerifyWithOfflineKey(String token, JwtException onlineVerificationException) {
+	private Jwt tryToVerifyWithVerificationKey(String token, JwtException verificationException) {
 		String verificationKey = xsuaaServiceConfiguration.getVerificationKey();
-		if (verificationKey == null || verificationKey.isEmpty()) {
-			throw onlineVerificationException;
+		if (isEmpty(verificationKey)) {
+			throw verificationException;
 		}
-		return verifyWithOfflineKey(token, verificationKey);
+		return verifyWithVerificationKey(token, verificationKey);
 	}
 
-	private Jwt verifyWithOfflineKey(String token, String verificationKey) {
+	private Jwt verifyWithVerificationKey(String token, String verificationKey) {
 		try {
 			RSAPublicKey verficationKey = createPublicKey(verificationKey);
 			NimbusJwtDecoder decoder = NimbusJwtDecoder.withPublicKey(verficationKey).build();


Test patch:
diff --git a/java-security/src/test/java/com/sap/cloud/security/token/validation/validators/XsuaaJwtIssuerValidatorTest.java b/java-security/src/test/java/com/sap/cloud/security/token/validation/validators/XsuaaJwtIssuerValidatorTest.java
index d1c6cd03..57a02c05 100644
--- a/java-security/src/test/java/com/sap/cloud/security/token/validation/validators/XsuaaJwtIssuerValidatorTest.java
+++ b/java-security/src/test/java/com/sap/cloud/security/token/validation/validators/XsuaaJwtIssuerValidatorTest.java
@@ -8,8 +8,7 @@ import org.mockito.Mockito;
 
 import static com.sap.cloud.security.token.TokenHeader.JWKS_URL;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
-import static org.hamcrest.CoreMatchers.is;
-import static org.hamcrest.CoreMatchers.startsWith;
+import static org.hamcrest.CoreMatchers.*;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.mockito.Mockito.when;
 
@@ -35,13 +34,13 @@ public class XsuaaJwtIssuerValidatorTest {
 	}
 
 	@Test
-	public void tokenIssuerMatchesIdentityServiceDomain() {
-		when(token.getHeaderParameterAsString(JWKS_URL)).thenReturn("https://subdomain.myauth.ondemand.com");
+	public void jwksMatchesIdentityServiceDomain() {
+		when(token.getHeaderParameterAsString(JWKS_URL)).thenReturn("https://subdomain.myauth.ondemand.com/token_keys");
 		assertThat(cut.validate(token).isValid(), is(true));
 	}
 
 	@Test
-	public void validationFails_whenTokenIssuerDoesNotMatchIdentityServiceDomain() {
+	public void validationFails_whenJwksDoesNotMatchIdentityServiceDomain() {
 		when(token.getHeaderParameterAsString(JWKS_URL)).thenReturn("https://subdomain.any.ondemand.com");
 		ValidationResult validationResult = cut.validate(token);
 		assertThat(validationResult.isErroneous(), is(true));
@@ -50,7 +49,7 @@ public class XsuaaJwtIssuerValidatorTest {
 	}
 
 	@Test
-	public void validationFails_whenTokenIssuerIsEmpty() {
+	public void validationFails_whenJwksIsEmpty() {
 		when(token.getHeaderParameterAsString(JWKS_URL)).thenReturn(" ");
 		ValidationResult validationResult = cut.validate(token);
 		assertThat(validationResult.isErroneous(), is(true));
@@ -59,7 +58,7 @@ public class XsuaaJwtIssuerValidatorTest {
 	}
 
 	@Test
-	public void validationFails_whenTokenIssuerIsNull() {
+	public void validationFails_whenJwksIsNull() {
 		when(token.getHeaderParameterAsString(JWKS_URL)).thenReturn(null);
 		ValidationResult validationResult = cut.validate(token);
 		assertThat(validationResult.isErroneous(), is(true));
@@ -68,10 +67,43 @@ public class XsuaaJwtIssuerValidatorTest {
 	}
 
 	@Test
-	public void validationFails_whenTokenIssuerIsNotAValidUri() {
+	public void validationFails_whenJwksIsNotAValidUri() {
 		when(token.getHeaderParameterAsString(JWKS_URL)).thenReturn("\0://myauth.com");
 		ValidationResult validationResult = cut.validate(token);
 		assertThat(validationResult.isErroneous(), is(true));
-		assertThat(validationResult.getErrorDescription(), startsWith("Issuer is not trusted because"));
+		assertThat(validationResult.getErrorDescription(), containsString("Jwt token does not contain a valid uri as 'jku' header parameter"));
 	}
+
+	@Test
+	public void validationFails_whenJwksContainsParameter() {
+		when(token.getHeaderParameterAsString(JWKS_URL)).thenReturn("\0://myauth.com");
+		ValidationResult validationResult = cut.validate(token);
+		assertThat(validationResult.isErroneous(), is(true));
+		assertThat(validationResult.getErrorDescription(), containsString("Jwt token does not contain a valid uri as 'jku' header parameter"));
+	}
+
+	@Test
+	public void validationFails_whenJwksDoesNotContainAValidPath() {
+		when(token.getHeaderParameterAsString(JWKS_URL)).thenReturn("https://subdomain.myauth.ondemand.com/wrong_endpoint");
+		ValidationResult validationResult = cut.validate(token);
+		assertThat(validationResult.isErroneous(), is(true));
+		assertThat(validationResult.getErrorDescription(), containsString("Jwt token does not contain a valid 'jku' header parameter"));
+	}
+
+	@Test
+	public void validationFails_whenJwksContainQueryParameters() {
+		when(token.getHeaderParameterAsString(JWKS_URL)).thenReturn("https://subdomain.myauth.ondemand.com/token_keys?a=b");
+		ValidationResult validationResult = cut.validate(token);
+		assertThat(validationResult.isErroneous(), is(true));
+		assertThat(validationResult.getErrorDescription(), containsString("Jwt token does not contain a valid 'jku' header parameter"));
+	}
+
+	@Test
+	public void validationFails_whenJwksContainsFragment() {
+		when(token.getHeaderParameterAsString(JWKS_URL)).thenReturn("https://subdomain.myauth.ondemand.com/token_keys#token_keys");
+		ValidationResult validationResult = cut.validate(token);
+		assertThat(validationResult.isErroneous(), is(true));
+		assertThat(validationResult.getErrorDescription(), containsString("Jwt token does not contain a valid 'jku' header parameter"));
+	}
+
 }
diff --git a/spring-xsuaa/src/test/java/com/sap/cloud/security/xsuaa/token/authentication/XsuaaJwtDecoderTest.java b/spring-xsuaa/src/test/java/com/sap/cloud/security/xsuaa/token/authentication/XsuaaJwtDecoderTest.java
index 957fe8f4..ecf5c204 100644
--- a/spring-xsuaa/src/test/java/com/sap/cloud/security/xsuaa/token/authentication/XsuaaJwtDecoderTest.java
+++ b/spring-xsuaa/src/test/java/com/sap/cloud/security/xsuaa/token/authentication/XsuaaJwtDecoderTest.java
@@ -1,6 +1,9 @@
 package com.sap.cloud.security.xsuaa.token.authentication;
 
+import com.nimbusds.jwt.JWT;
+import com.sap.cloud.security.xsuaa.XsuaaCredentials;
 import com.sap.cloud.security.xsuaa.XsuaaServiceConfiguration;
+import com.sap.cloud.security.xsuaa.XsuaaServiceConfigurationCustom;
 import com.sap.cloud.security.xsuaa.XsuaaServiceConfigurationDefault;
 import com.sap.cloud.security.xsuaa.token.TokenClaims;
 import org.apache.commons.io.IOUtils;
@@ -21,17 +24,19 @@ import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
 @RunWith(SpringRunner.class)
-@TestPropertySource(locations = "/XsuaaJwtDecoderTest.properties")
+@TestPropertySource("/XsuaaJwtDecoderTest.properties")
 @ContextConfiguration(classes = XsuaaServiceConfigurationDefault.class)
 public class XsuaaJwtDecoderTest {
 
 	@Autowired
-	XsuaaServiceConfiguration configuration;
+	XsuaaServiceConfiguration configurationWithVerificationKey;
+
+	XsuaaServiceConfiguration configuration = new XsuaaServiceConfigurationCustom(new XsuaaCredentials());
 
 	@Test
 	public void decode_withVerficationKey() throws IOException {
 		String token = IOUtils.resourceToString("/accessTokenRSA256WithVerificationKey.txt", StandardCharsets.UTF_8);
-		final JwtDecoder cut = new XsuaaJwtDecoderBuilder(configuration).build();
+		final JwtDecoder cut = new XsuaaJwtDecoderBuilder(configurationWithVerificationKey).build();
 
 		final Jwt jwt = cut.decode(token);
 
@@ -44,6 +49,76 @@ public class XsuaaJwtDecoderTest {
 
 		final JwtDecoder cut = new XsuaaJwtDecoderBuilder(configuration).build();
 
-		assertThatThrownBy(() -> cut.decode(token)).isInstanceOf(JwtException.class);
+		assertThatThrownBy(() -> cut.decode(token)).isInstanceOf(JwtException.class)
+				.hasMessageContaining("Cannot verify with online token key, jku, kid, uaadomain is null");
+	}
+
+	@Test
+	public void decode_whenJwksContainsInvalidJwksDomain_throwsException() throws IOException {
+		String token = IOUtils.resourceToString("/token_user.txt", StandardCharsets.UTF_8);
+
+		XsuaaJwtDecoder cut = (XsuaaJwtDecoder)new XsuaaJwtDecoderBuilder(configuration).build();
+		cut.setTokenInfoExtractor(new TokenInfoExtractorImpl("https://subdomain.wrongoauth.ondemand.com/token_keys"));
+
+		assertThatThrownBy(() -> cut.decode(token)).isInstanceOf(JwtException.class)
+				.hasMessageContaining("JWT verification failed: Do not trust 'jku' token header");
+	}
+
+	@Test
+	public void decode_whenJwksContainsInvalidPath_throwsException() throws IOException {
+		String token = IOUtils.resourceToString("/token_cc.txt", StandardCharsets.UTF_8);
+
+		XsuaaJwtDecoder cut = (XsuaaJwtDecoder)new XsuaaJwtDecoderBuilder(configuration).build();
+		cut.setTokenInfoExtractor(new TokenInfoExtractorImpl("https://subdomain.myauth.ondemand.com/wrong_endpoint"));
+
+		assertThatThrownBy(() -> cut.decode(token)).isInstanceOf(JwtException.class)
+				.hasMessageContaining("Jwt token does not contain a valid 'jku' header parameter");
+	}
+
+	@Test
+	public void decode_whenJwksContainQueryParameters_throwsException() throws IOException {
+		String token = IOUtils.resourceToString("/token_cc.txt", StandardCharsets.UTF_8);
+
+		XsuaaJwtDecoder cut = (XsuaaJwtDecoder)new XsuaaJwtDecoderBuilder(configuration).build();
+		cut.setTokenInfoExtractor(new TokenInfoExtractorImpl("https://subdomain.myauth.ondemand.com/token_keys?a=b"));
+
+		assertThatThrownBy(() -> cut.decode(token)).isInstanceOf(JwtException.class)
+				.hasMessageContaining("Jwt token does not contain a valid 'jku' header parameter: ");
+
+	}
+
+	@Test
+	public void decode_whenJwksContainsFragment_throwsException() throws IOException {
+		String token = IOUtils.resourceToString("/token_cc.txt", StandardCharsets.UTF_8);
+
+		XsuaaJwtDecoder cut = (XsuaaJwtDecoder)new XsuaaJwtDecoderBuilder(configuration).build();
+		cut.setTokenInfoExtractor(new TokenInfoExtractorImpl("https://subdomain.myauth.ondemand.com/token_keys#token_keys"));
+
+		assertThatThrownBy(() -> cut.decode(token)).isInstanceOf(JwtException.class)
+				.hasMessageContaining("Jwt token does not contain a valid 'jku' header parameter:");
+
+	}
+
+	private static class TokenInfoExtractorImpl implements com.sap.cloud.security.xsuaa.token.authentication.TokenInfoExtractor {
+		private String jku;
+
+		public TokenInfoExtractorImpl(String jku) {
+			this.jku = jku;
+		}
+
+		@Override
+		public String getJku(JWT jwt) {
+			return jku;
+		}
+
+		@Override
+		public String getKid(JWT jwt) {
+			return "kid";
+		}
+
+		@Override
+		public String getUaaDomain(JWT jwt) {
+			return "myauth.ondemand.com";
+		}
 	}
 }
\ No newline at end of file