Repository: SAP/cloud-security-xsuaa-integration
Pull request #812 (2022-02-03 17:25:11)

JwtX5tValidator: minor improvements
- avoid nullpointer when calling `JwtX5tValidator.validate(null)`
- Logger.ERROR is not required, log is written with `ValidationResults.createInvalid(...)`
- removed audience check as documented in latest version of proofOfPossession Doc -> see #2-check-certificate-thumbprint-x5t


Patch:
diff --git a/java-security/src/main/java/com/sap/cloud/security/token/validation/validators/JwtX5tValidator.java b/java-security/src/main/java/com/sap/cloud/security/token/validation/validators/JwtX5tValidator.java
index e59ebf10..2bb39ea5 100644
--- a/java-security/src/main/java/com/sap/cloud/security/token/validation/validators/JwtX5tValidator.java
+++ b/java-security/src/main/java/com/sap/cloud/security/token/validation/validators/JwtX5tValidator.java
@@ -9,6 +9,7 @@ import com.sap.cloud.security.token.validation.ValidationResult;
 import com.sap.cloud.security.token.validation.ValidationResults;
 import com.sap.cloud.security.token.validation.Validator;
 import com.sap.cloud.security.x509.Certificate;
+import com.sap.cloud.security.xsuaa.Assertions;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -29,10 +30,10 @@ import javax.annotation.Nullable;
 public class JwtX5tValidator implements Validator<Token> {
 
 	private static final Logger LOGGER = LoggerFactory.getLogger(JwtX5tValidator.class);
-	private final OAuth2ServiceConfiguration config;
+	static final String VALIDATION_FAILED = "Certificate thumbprint validation failed";
 
 	public JwtX5tValidator(OAuth2ServiceConfiguration config) {
-		this.config = config;
+		Assertions.assertNotNull(config, "Service configuration must not be null");
 	}
 
 	/**
@@ -50,29 +51,23 @@ public class JwtX5tValidator implements Validator<Token> {
 	 */
 	@Override
 	public ValidationResult validate(Token token) {
+		if (token == null) {
+			return ValidationResults.createInvalid("No token passed to validate certificate thumbprint");
+		}
 		String tokenX5t = extractCnfThumbprintFromToken(token);
 		LOGGER.debug("Token 'cnf' thumbprint: {}", tokenX5t);
-		if (tokenX5t != null) {
-			Certificate clientCertificate = SecurityContext.getClientCertificate();
-			if (clientCertificate == null) {
-				LOGGER.error("Client certificate missing from SecurityContext");
-				return ValidationResults.createInvalid("Certificate validation failed");
-			}
-			String clientCertificateX5t = clientCertificate.getThumbprint();
-			if (clientCertificateX5t.equals(tokenX5t)) {
-				if (token.getAudiences().size() == 1 && token.getAudiences().contains(config.getClientId())) {
-					return ValidationResults.createValid();
-				}
-				LOGGER.error("Audience validation failed -> \"aud\": {} != \"clientid\": \"{}\"", token.getAudiences(),
-						config.getClientId());
-			} else {
-				LOGGER.error(
-						"Thumbprint validation failed -> x5t from token: \"{}\" != thumbprint from client certificate: \"{}\"",
-						tokenX5t,
-						clientCertificateX5t);
-			}
+		if (tokenX5t == null) {
+			return ValidationResults.createInvalid("Token doesn't contain certificate thumbprint confirmation method");
+		}
+		Certificate clientCertificate = SecurityContext.getClientCertificate();
+		if (clientCertificate == null) {
+			return ValidationResults.createInvalid("Client certificate missing from SecurityContext");
+		}
+		String clientCertificateX5t = clientCertificate.getThumbprint();
+		if (clientCertificateX5t.equals(tokenX5t)) {
+			return ValidationResults.createValid();
 		}
-		return ValidationResults.createInvalid("Certificate validation failed");
+		return ValidationResults.createInvalid(VALIDATION_FAILED);
 	}
 
 	/**


Test patch:
diff --git a/java-security/src/test/java/com/sap/cloud/security/servlet/IasTokenAuthenticatorX509Test.java b/java-security/src/test/java/com/sap/cloud/security/servlet/IasTokenAuthenticatorX509Test.java
index fac26fed..46ba1d7f 100644
--- a/java-security/src/test/java/com/sap/cloud/security/servlet/IasTokenAuthenticatorX509Test.java
+++ b/java-security/src/test/java/com/sap/cloud/security/servlet/IasTokenAuthenticatorX509Test.java
@@ -80,14 +80,14 @@ class IasTokenAuthenticatorX509Test {
 		TokenAuthenticationResult response = cut.validateRequest(httpRequest, HTTP_RESPONSE);
 
 		assertThat(response.getUnauthenticatedReason())
-				.contains("Error during token validation: Certificate validation failed");
+				.contains("Error during token validation: Client certificate missing");
 		assertThat(response.isAuthenticated()).isFalse();
 
 		when(httpRequest.getHeader(FWD_CLIENT_CERT_HEADER)).thenReturn("");
 		TokenAuthenticationResult response2 = cut.validateRequest(httpRequest, HTTP_RESPONSE);
 
 		assertThat(response2.getUnauthenticatedReason())
-				.contains("Error during token validation: Certificate validation failed");
+				.contains("Error during token validation: Client certificate missing");
 		assertThat(response2.isAuthenticated()).isFalse();
 
 	}
@@ -100,7 +100,7 @@ class IasTokenAuthenticatorX509Test {
 		TokenAuthenticationResult response = cut.validateRequest(httpRequest, HTTP_RESPONSE);
 
 		assertThat(response.getUnauthenticatedReason())
-				.contains("Error during token validation: Certificate validation failed");
+				.contains("Error during token validation: Client certificate missing");
 		assertThat(response.isAuthenticated()).isFalse();
 	}
 
@@ -112,7 +112,7 @@ class IasTokenAuthenticatorX509Test {
 		TokenAuthenticationResult response = cut.validateRequest(httpRequest, HTTP_RESPONSE);
 
 		assertThat(response.getUnauthenticatedReason())
-				.contains("Error during token validation: Certificate validation failed");
+				.contains("Error during token validation: Certificate thumbprint validation failed");
 		assertThat(response.isAuthenticated()).isFalse();
 	}
 
diff --git a/java-security/src/test/java/com/sap/cloud/security/token/validation/validators/JwtX5tValidatorTest.java b/java-security/src/test/java/com/sap/cloud/security/token/validation/validators/JwtX5tValidatorTest.java
index ce67c67b..99590205 100644
--- a/java-security/src/test/java/com/sap/cloud/security/token/validation/validators/JwtX5tValidatorTest.java
+++ b/java-security/src/test/java/com/sap/cloud/security/token/validation/validators/JwtX5tValidatorTest.java
@@ -23,10 +23,9 @@ import org.mockito.internal.util.collections.Sets;
 
 import java.io.IOException;
 import java.nio.charset.StandardCharsets;
-import java.util.Collections;
 
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertTrue;
+import static com.sap.cloud.security.token.validation.validators.JwtX5tValidator.VALIDATION_FAILED;
+import static org.junit.jupiter.api.Assertions.*;
 
 class JwtX5tValidatorTest {
 
@@ -36,7 +35,6 @@ class JwtX5tValidatorTest {
 	private static X509Certificate x509;
 	private static final Token TOKEN = Mockito.mock(Token.class);
 	private static final JsonObject JSON_MOCK = Mockito.mock(JsonObject.class);
-	private static final String INVALID_MESSAGE = "Certificate validation failed";
 	private static final String X5T_VALIDATOR_DISABLED = "X5tValidator is not enabled";
 
 	@BeforeAll
@@ -55,90 +53,67 @@ class JwtX5tValidatorTest {
 	}
 
 	@Test
-	void validateToken_ValidToken_validX509() {
-		Token token = Token.create(tokenWithX5t);
-		SecurityContext.setClientCertificate(x509);
-		ValidationResult result = CUT.validate(token);
-		assertTrue(result.isValid());
+	void given_NoConfig() {
+		assertThrows(
+				IllegalArgumentException.class,
+				() -> new JwtX5tValidator(null),
+				"Service configuration must not be null");
 	}
 
 	@Test
-	void validateToken_ValidToken_invalidX509() {
-		Token token = Token.create(tokenWithX5t);
-		SecurityContext.setClientCertificate(null);
-		ValidationResult result = CUT.validate(token);
+	void given_NoToken() {
+		ValidationResult result = CUT.validate(null);
 		assertTrue(result.isErroneous());
-		assertEquals(INVALID_MESSAGE, result.getErrorDescription());
+		assertEquals("No token passed to validate certificate thumbprint", result.getErrorDescription());
 	}
 
 	@Test
-	void validateToken_validCnf_validX509_MultipleAud() {
+	void given_ValidToken_validX509() {
+		Token token = Token.create(tokenWithX5t);
 		SecurityContext.setClientCertificate(x509);
-		Mockito.when(TOKEN.getAudiences()).thenReturn(Sets.newSet("aud1", "aud2"));
-		Mockito.when(TOKEN.getClaimAsJsonObject(TokenClaims.CNF)).thenReturn(JSON_MOCK);
-		ValidationResult result = CUT.validate(TOKEN);
-		assertTrue(result.isErroneous());
-		assertEquals(INVALID_MESSAGE, result.getErrorDescription());
+		ValidationResult result = CUT.validate(token);
+		assertTrue(result.isValid());
 	}
 
 	@Test
-	void validateToken_validCnf_validX509_EmptyAud() {
-		SecurityContext.setClientCertificate(x509);
-		Mockito.when(TOKEN.getAudiences()).thenReturn(Collections.emptySet());
-		Mockito.when(TOKEN.getClaimAsJsonObject(TokenClaims.CNF)).thenReturn(JSON_MOCK);
-		ValidationResult result = CUT.validate(TOKEN);
+	void given_ValidToken_invalidX509() {
+		Token token = Token.create(tokenWithX5t);
+		SecurityContext.setClientCertificate(null);
+		ValidationResult result = CUT.validate(token);
 		assertTrue(result.isErroneous());
-		assertEquals(INVALID_MESSAGE, result.getErrorDescription());
+		assertEquals("Client certificate missing from SecurityContext", result.getErrorDescription());
 	}
 
 	@Test
-	void validateToken_InvalidCnf_validX509() {
+	void given_InvalidCnf_validX509() {
 		Token token = Token.create(tokenWithInvalidX5t);
 		SecurityContext.setClientCertificate(x509);
 		ValidationResult result = CUT.validate(token);
 		assertTrue(result.isErroneous());
-		assertEquals(INVALID_MESSAGE, result.getErrorDescription());
+		assertEquals(VALIDATION_FAILED, result.getErrorDescription());
 	}
 
 	@Test
-	void validateToken_InvalidCnf_invalidX509() {
+	void given_InvalidCnf_invalidX509() {
 		Token token = Token.create(tokenWithInvalidX5t);
 		SecurityContext.setClientCertificate(null);
 		ValidationResult result = CUT.validate(token);
 		assertTrue(result.isErroneous());
-		assertEquals(INVALID_MESSAGE, result.getErrorDescription());
+		assertEquals("Client certificate missing from SecurityContext", result.getErrorDescription());
 	}
 
 	@Test
-	void validateToken_NoCnf_validAud() {
+	void given_NoCnf_validAud() {
 		Mockito.when(JSON_MOCK.getAsString(TokenClaims.CNF_X5T)).thenReturn(null);
 		Mockito.when(TOKEN.getAudiences()).thenReturn(Sets.newSet("myClientId"));
 		ValidationResult result = CUT.validate(TOKEN);
 		assertTrue(result.isErroneous());
-		assertEquals(INVALID_MESSAGE, result.getErrorDescription());
-	}
-
-	@Test
-	void validateToken_NoCnf_MultipleAud() {
-		Mockito.when(JSON_MOCK.getAsString(TokenClaims.CNF_X5T)).thenReturn(null);
-		Mockito.when(TOKEN.getAudiences()).thenReturn(Sets.newSet("aud1", "aud2"));
-		ValidationResult result = CUT.validate(TOKEN);
-		assertTrue(result.isErroneous());
-		assertEquals(INVALID_MESSAGE, result.getErrorDescription());
-	}
-
-	@Test
-	void validateToken_NoCnf_EmptyAud() {
-		Mockito.when(JSON_MOCK.getAsString(TokenClaims.CNF_X5T)).thenReturn(null);
-		Mockito.when(TOKEN.getAudiences()).thenReturn(Collections.emptySet());
-		ValidationResult result = CUT.validate(TOKEN);
-		assertTrue(result.isErroneous());
-		assertEquals(INVALID_MESSAGE, result.getErrorDescription());
+		assertEquals("Token doesn't contain certificate thumbprint confirmation method", result.getErrorDescription());
 	}
 
-	@Disabled("until proofOfPossesion validator subchain is implemented")
+	@Disabled("until proofOfPossession validator subchain is implemented")
 	@Test
-	void validateToken_validatorDisabled() {
+	void given_validatorDisabled() {
 		Token token = Token.create(tokenWithX5t);
 		ValidationResult result = CUT.validate(token);
 		assertTrue(result.isErroneous());