Add Issuer to failed SAML Signature validation logs when available (elastic#126310)

richard-dennehy · elasticsearchmachine · richard-dennehy · commit 4013f479ee96 · 2025-04-23T09:49:54.000+01:00
* Add Issuer to failed SAML Signature validation logs when available * [CI] Auto commit changes from spotless * Fix tests * Update docs/changelog/126310.yaml * address review comments * replace String.format call * update formatIssuer to describeIssuer * [CI] Auto commit changes from spotless * truncate long issuers in log messages * [CI] Auto commit changes from spotless * handle null issuer value * address review comments --------- Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co> (cherry picked from commit ceaa01a)
diff --git a/docs/changelog/126310.yaml b/docs/changelog/126310.yaml
@@ -0,0 +1,6 @@
+pr: 126310
+summary: Add Issuer to failed SAML Signature validation logs when available
+area: Security
+type: enhancement
+issues:
+ - 111022
diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlAuthenticator.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlAuthenticator.java
@@ -93,7 +93,7 @@ private SamlAttributes authenticateResponse(Element element, Collection<String>
         }
         final boolean requireSignedAssertions;
         if (response.isSigned()) {
-            validateSignature(response.getSignature());
+            validateSignature(response.getSignature(), response.getIssuer());
             requireSignedAssertions = false;
         } else {
             requireSignedAssertions = true;
@@ -199,7 +199,7 @@ private List<Attribute> processAssertion(Assertion assertion, boolean requireSig
         }
         // Do not further process unsigned Assertions
         if (assertion.isSigned()) {
-            validateSignature(assertion.getSignature());
+            validateSignature(assertion.getSignature(), assertion.getIssuer());
         } else if (requireSignature) {
             throw samlException("Assertion [{}] is not signed, but a signature is required", assertion.getElementQName());
         }
diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutRequestHandler.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutRequestHandler.java
@@ -73,7 +73,7 @@ private Result parseLogout(LogoutRequest logoutRequest, boolean requireSignature
                 throw samlException("Logout request is not signed");
             }
         } else {
-            validateSignature(signature);
+            validateSignature(signature, logoutRequest.getIssuer());
         }
 
         checkIssuer(logoutRequest.getIssuer(), logoutRequest);
diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java
@@ -45,7 +45,7 @@ public void handle(boolean httpRedirect, String payload, Collection<String> allo
                 if (logoutResponse.getSignature() == null) {
                     throw samlException("LogoutResponse is not signed, but is required for HTTP-Post binding");
                 }
-                validateSignature(logoutResponse.getSignature());
+                validateSignature(logoutResponse.getSignature(), logoutResponse.getIssuer());
             }
             checkInResponseTo(logoutResponse, allowedSamlRequestIds);
             checkStatus(logoutResponse.getStatus());
diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlObjectHandler.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlObjectHandler.java
@@ -95,6 +95,8 @@ public class SamlObjectHandler {
         }
     });
 
+    private static final int ISSUER_VALUE_MAX_LENGTH = 512;
+
     protected final Logger logger = LogManager.getLogger(getClass());
 
     @Nullable
@@ -161,13 +163,13 @@ protected static String describe(Collection<X509Credential> credentials) {
         return credentials.stream().map(credential -> describe(credential.getEntityCertificate())).collect(Collectors.joining(","));
     }
 
-    void validateSignature(Signature signature) {
+    void validateSignature(Signature signature, @Nullable Issuer issuer) {
         final String signatureText = text(signature, 32);
         SAMLSignatureProfileValidator profileValidator = new SAMLSignatureProfileValidator();
         try {
             profileValidator.validate(signature);
         } catch (SignatureException e) {
-            throw samlSignatureException(idp.getSigningCredentials(), signatureText, e);
+            throw samlSignatureException(issuer, idp.getSigningCredentials(), signatureText, e);
         }
 
         checkIdpSignature(credential -> {
@@ -200,21 +202,21 @@ void validateSignature(Signature signature) {
                         );
                         return true;
                     } catch (PrivilegedActionException e) {
-                        logger.warn("SecurityException while attempting to validate SAML signature", e);
+                        logger.warn("SecurityException while attempting to validate SAML signature." + describeIssuer(issuer), e);
                         return false;
                     }
                 });
             } catch (PrivilegedActionException e) {
                 throw new SecurityException("SecurityException while attempting to validate SAML signature", e);
             }
-        }, signatureText);
+        }, signatureText, issuer);
     }
 
     /**
      * Tests whether the provided function returns {@code true} for any of the IdP's signing credentials.
-     * @throws ElasticsearchSecurityException - A SAML exception if not matching credential is found.
+     * @throws ElasticsearchSecurityException - A SAML exception if no matching credential is found.
      */
-    protected void checkIdpSignature(CheckedFunction<Credential, Boolean, Exception> check, String signatureText) {
+    protected void checkIdpSignature(CheckedFunction<Credential, Boolean, Exception> check, String signatureText, @Nullable Issuer issuer) {
         final Predicate<Credential> predicate = credential -> {
             try {
                 return check.apply(credential);
@@ -231,35 +233,52 @@ protected void checkIdpSignature(CheckedFunction<Credential, Boolean, Exception>
                 logger.trace("SAML Signature failure caused by", e);
                 return false;
             } catch (Exception e) {
-                logger.warn("Exception while attempting to validate SAML Signature", e);
+                logger.warn("Exception while attempting to validate SAML Signature." + describeIssuer(issuer), e);
                 return false;
             }
         };
         final List<Credential> credentials = idp.getSigningCredentials();
         if (credentials.stream().anyMatch(predicate) == false) {
-            throw samlSignatureException(credentials, signatureText);
+            throw samlSignatureException(issuer, credentials, signatureText);
         }
     }
 
     /**
      * Constructs a SAML specific exception with a consistent message regarding SAML Signature validation failures
      */
-    private ElasticsearchSecurityException samlSignatureException(List<Credential> credentials, String signature, Exception cause) {
+    private ElasticsearchSecurityException samlSignatureException(
+        @Nullable Issuer issuer,
+        List<Credential> credentials,
+        String signature,
+        Exception cause
+    ) {
         logger.warn(
-            "The XML Signature of this SAML message cannot be validated. Please verify that the saml realm uses the correct SAML"
-                + "metadata file/URL for this Identity Provider"
+            "The XML Signature of this SAML message cannot be validated. Please verify that the saml realm uses the correct SAML "
+                + "metadata file/URL for this Identity Provider.{}",
+            describeIssuer(issuer)
         );
         final String msg = "SAML Signature [{}] could not be validated against [{}]";
-        return samlException(msg, cause, signature, describeCredentials(credentials));
+        if (cause != null) {
+            return samlException(msg, cause, signature, describeCredentials(credentials));
+        } else {
+            return samlException(msg, signature, describeCredentials(credentials));
+        }
     }
 
-    private ElasticsearchSecurityException samlSignatureException(List<Credential> credentials, String signature) {
-        logger.warn(
-            "The XML Signature of this SAML message cannot be validated. Please verify that the saml realm uses the correct SAML"
-                + "metadata file/URL for this Identity Provider"
-        );
-        final String msg = "SAML Signature [{}] could not be validated against [{}]";
-        return samlException(msg, signature, describeCredentials(credentials));
+    private ElasticsearchSecurityException samlSignatureException(Issuer issuer, List<Credential> credentials, String signature) {
+        return samlSignatureException(issuer, credentials, signature, null);
+    }
+
+    // package private for testing
+    static String describeIssuer(@Nullable Issuer issuer) {
+        if (issuer == null || issuer.getValue() == null) {
+            return "";
+        }
+        final String msg = " The issuer included in the SAML message was [%s]";
+        if (issuer.getValue().length() > ISSUER_VALUE_MAX_LENGTH) {
+            return Strings.format(msg + "...", Strings.cleanTruncate(issuer.getValue(), ISSUER_VALUE_MAX_LENGTH));
+        }
+        return Strings.format(msg, issuer.getValue());
     }
 
     private static String describeCredentials(List<Credential> credentials) {
@@ -423,7 +442,7 @@ private void validateSignature(String inputString, String signatureAlgorithm, St
                 );
                 return false;
             }
-        }, signatureText);
+        }, signatureText, null);
     }
 
     protected byte[] decodeBase64(String content) {
diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlAuthenticatorTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlAuthenticatorTests.java
@@ -39,6 +39,7 @@
 import org.opensaml.saml.saml2.core.SubjectConfirmation;
 import org.opensaml.saml.saml2.core.SubjectConfirmationData;
 import org.opensaml.saml.saml2.core.impl.AuthnStatementBuilder;
+import org.opensaml.saml.saml2.core.impl.IssuerBuilder;
 import org.opensaml.saml.saml2.encryption.Encrypter;
 import org.opensaml.security.credential.BasicCredential;
 import org.opensaml.security.credential.Credential;
@@ -83,7 +84,9 @@
 import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.endsWith;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.hasLength;
 import static org.hamcrest.Matchers.instanceOf;
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.iterableWithSize;
@@ -106,6 +109,9 @@ public class SamlAuthenticatorTests extends SamlResponseHandlerTests {
             + "Attributes with a name clash may prevent authentication or interfere will role mapping. "
             + "Change your IdP configuration to use a different attribute *"
             + " that will not clash with any of [*]";
+    private static final String SIGNATURE_VALIDATION_FAILED_LOG_MESSAGE = "The XML Signature of this SAML message cannot be validated. "
+        + "Please verify that the saml realm uses the correct SAML metadata file/URL for this Identity Provider. "
+        + "The issuer included in the SAML message was [https://idp.saml.elastic.test/]";
 
     private SamlAuthenticator authenticator;
 
@@ -741,16 +747,29 @@ public void testIncorrectSigningKeyIsRejected() throws Exception {
         // check that the content is valid when signed by the correct key-pair
         assertThat(authenticator.authenticate(token(signer.transform(xml, idpSigningCertificatePair))), notNullValue());
 
-        // check is rejected when signed by a different key-pair
-        final Tuple<X509Certificate, PrivateKey> wrongKey = readKeyPair("RSA_4096_updated");
-        final ElasticsearchSecurityException exception = expectThrows(
-            ElasticsearchSecurityException.class,
-            () -> authenticator.authenticate(token(signer.transform(xml, wrongKey)))
-        );
-        assertThat(exception.getMessage(), containsString("SAML Signature"));
-        assertThat(exception.getMessage(), containsString("could not be validated"));
-        assertThat(exception.getCause(), nullValue());
-        assertThat(SamlUtils.isSamlException(exception), is(true));
+        try (var mockLog = MockLog.capture(authenticator.getClass())) {
+            mockLog.addExpectation(
+                new MockLog.SeenEventExpectation(
+                    "Invalid Signature",
+                    authenticator.getClass().getName(),
+                    Level.WARN,
+                    SIGNATURE_VALIDATION_FAILED_LOG_MESSAGE
+                )
+            );
+
+            // check is rejected when signed by a different key-pair
+            final Tuple<X509Certificate, PrivateKey> wrongKey = readKeyPair("RSA_4096_updated");
+            final ElasticsearchSecurityException exception = expectThrows(
+                ElasticsearchSecurityException.class,
+                () -> authenticator.authenticate(token(signer.transform(xml, wrongKey)))
+            );
+            assertThat(exception.getMessage(), containsString("SAML Signature"));
+            assertThat(exception.getMessage(), containsString("could not be validated"));
+            assertThat(exception.getCause(), nullValue());
+            assertThat(SamlUtils.isSamlException(exception), is(true));
+
+            mockLog.assertAllExpectationsMatched();
+        }
     }
 
     public void testSigningKeyIsReloadedForEachRequest() throws Exception {
@@ -1301,24 +1320,80 @@ public void testFailureWhenIdPCredentialsAreEmpty() throws Exception {
         authenticator = buildAuthenticator(() -> emptyList(), emptyList());
         final String xml = getSimpleResponseAsString(clock.instant());
         final SamlToken token = token(signResponse(xml));
-        final ElasticsearchSecurityException exception = expectSamlException(() -> authenticator.authenticate(token));
-        assertThat(exception.getCause(), nullValue());
-        assertThat(exception.getMessage(), containsString("SAML Signature"));
-        assertThat(exception.getMessage(), containsString("could not be validated"));
-        // Restore the authenticator with credentials for the rest of the test cases
-        authenticator = buildAuthenticator(() -> buildOpenSamlCredential(idpSigningCertificatePair), emptyList());
+
+        try (var mockLog = MockLog.capture(authenticator.getClass())) {
+            mockLog.addExpectation(
+                new MockLog.SeenEventExpectation(
+                    "Invalid signature",
+                    authenticator.getClass().getName(),
+                    Level.WARN,
+                    SIGNATURE_VALIDATION_FAILED_LOG_MESSAGE
+                )
+            );
+
+            final ElasticsearchSecurityException exception = expectSamlException(() -> authenticator.authenticate(token));
+            assertThat(exception.getCause(), nullValue());
+            assertThat(exception.getMessage(), containsString("SAML Signature"));
+            assertThat(exception.getMessage(), containsString("could not be validated"));
+
+            mockLog.awaitAllExpectationsMatched();
+        }
     }
 
     public void testFailureWhenIdPCredentialsAreNull() throws Exception {
         authenticator = buildAuthenticator(() -> singletonList(null), emptyList());
         final String xml = getSimpleResponseAsString(clock.instant());
         final SamlToken token = token(signResponse(xml));
-        final ElasticsearchSecurityException exception = expectSamlException(() -> authenticator.authenticate(token));
-        assertThat(exception.getCause(), nullValue());
-        assertThat(exception.getMessage(), containsString("SAML Signature"));
-        assertThat(exception.getMessage(), containsString("could not be validated"));
-        // Restore the authenticator with credentials for the rest of the test cases
-        authenticator = buildAuthenticator(() -> buildOpenSamlCredential(idpSigningCertificatePair), emptyList());
+
+        try (var mockLog = MockLog.capture(authenticator.getClass())) {
+            mockLog.addExpectation(
+                new MockLog.SeenEventExpectation(
+                    "Invalid signature",
+                    authenticator.getClass().getName(),
+                    Level.WARN,
+                    SIGNATURE_VALIDATION_FAILED_LOG_MESSAGE
+                )
+            );
+            mockLog.addExpectation(
+                new MockLog.SeenEventExpectation(
+                    "Null credentials",
+                    authenticator.getClass().getName(),
+                    Level.WARN,
+                    "Exception while attempting to validate SAML Signature. "
+                        + "The issuer included in the SAML message was [https://idp.saml.elastic.test/]"
+                )
+            );
+
+            final ElasticsearchSecurityException exception = expectSamlException(() -> authenticator.authenticate(token));
+            assertThat(exception.getCause(), nullValue());
+            assertThat(exception.getMessage(), containsString("SAML Signature"));
+            assertThat(exception.getMessage(), containsString("could not be validated"));
+
+            mockLog.awaitAllExpectationsMatched();
+        }
+    }
+
+    public void testDescribeNullIssuer() {
+        final Issuer issuer = randomFrom(new IssuerBuilder().buildObject(), null);
+        assertThat(SamlAuthenticator.describeIssuer(issuer), equalTo(""));
+    }
+
+    public void testDescribeIssuer() {
+        final Issuer issuer = new IssuerBuilder().buildObject();
+        issuer.setValue("https://idp.saml.elastic.test/");
+        assertThat(
+            SamlAuthenticator.describeIssuer(issuer),
+            equalTo(" The issuer included in the SAML message was [https://idp.saml.elastic.test/]")
+        );
+    }
+
+    public void testDescribeVeryLongIssuer() {
+        final Issuer issuer = new IssuerBuilder().buildObject();
+        issuer.setValue("https://idp.saml.elastic.test/" + randomAlphaOfLength(512));
+
+        final String description = SamlAuthenticator.describeIssuer(issuer);
+        assertThat(description, hasLength(562));
+        assertThat(description, endsWith("..."));
     }
 
     private interface CryptoTransform {

Original file line number	Diff line number	Diff line change
`@@ -93,7 +93,7 @@ private SamlAttributes authenticateResponse(Element element, Collection<String>`
`93`	`93`	`}`
`94`	`94`	`final boolean requireSignedAssertions;`
`95`	`95`	`if (response.isSigned()) {`
`96`		`- validateSignature(response.getSignature());`
	`96`	`+ validateSignature(response.getSignature(), response.getIssuer());`
`97`	`97`	`requireSignedAssertions = false;`
`98`	`98`	`} else {`
`99`	`99`	`requireSignedAssertions = true;`
`@@ -199,7 +199,7 @@ private List<Attribute> processAssertion(Assertion assertion, boolean requireSig`
`199`	`199`	`}`
`200`	`200`	`// Do not further process unsigned Assertions`
`201`	`201`	`if (assertion.isSigned()) {`
`202`		`- validateSignature(assertion.getSignature());`
	`202`	`+ validateSignature(assertion.getSignature(), assertion.getIssuer());`
`203`	`203`	`} else if (requireSignature) {`
`204`	`204`	`throw samlException("Assertion [{}] is not signed, but a signature is required", assertion.getElementQName());`
`205`	`205`	`}`
Original file line number	Diff line number	Diff line change
`@@ -73,7 +73,7 @@ private Result parseLogout(LogoutRequest logoutRequest, boolean requireSignature`
`73`	`73`	`throw samlException("Logout request is not signed");`
`74`	`74`	`}`
`75`	`75`	`} else {`
`76`		`- validateSignature(signature);`
	`76`	`+ validateSignature(signature, logoutRequest.getIssuer());`
`77`	`77`	`}`
`78`	`78`
`79`	`79`	`checkIssuer(logoutRequest.getIssuer(), logoutRequest);`
Original file line number	Diff line number	Diff line change
`@@ -45,7 +45,7 @@ public void handle(boolean httpRedirect, String payload, Collection<String> allo`
`45`	`45`	`if (logoutResponse.getSignature() == null) {`
`46`	`46`	`throw samlException("LogoutResponse is not signed, but is required for HTTP-Post binding");`
`47`	`47`	`}`
`48`		`- validateSignature(logoutResponse.getSignature());`
	`48`	`+ validateSignature(logoutResponse.getSignature(), logoutResponse.getIssuer());`
`49`	`49`	`}`
`50`	`50`	`checkInResponseTo(logoutResponse, allowedSamlRequestIds);`
`51`	`51`	`checkStatus(logoutResponse.getStatus());`