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

* 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 ceaa01a538)
2025-04-24 15:17:30 -04:00 · 2025-04-23 10:57:27 +01:00 · 2025-04-23 10:57:27 +01:00 · cbf36bf2e2
commit cbf36bf2e2
parent b2529edd50
6 changed files with 146 additions and 46 deletions
--- 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
--- 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 @@ class SamlAuthenticator extends SamlResponseHandler {
        }
        final boolean requireSignedAssertions;
        if (response.isSigned()) {
-            validateSignature(response.getSignature());
+            validateSignature(response.getSignature(), response.getIssuer());
            requireSignedAssertions = false;
        } else {
            requireSignedAssertions = true;
@ -199,7 +199,7 @@ class SamlAuthenticator extends SamlResponseHandler {
        }
        // 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());
        }
--- 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 @@ public class SamlLogoutRequestHandler extends SamlObjectHandler {
                throw samlException("Logout request is not signed");
            }
        } else {
-            validateSignature(signature);
+            validateSignature(signature, logoutRequest.getIssuer());
        }

        checkIssuer(logoutRequest.getIssuer(), logoutRequest);
--- 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 class SamlLogoutResponseHandler extends SamlResponseHandler {
                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());
--- 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 @@ public class SamlObjectHandler {
        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 @@ public class SamlObjectHandler {
                        );
                        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 @@ public class SamlObjectHandler {
                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 @@ public class SamlObjectHandler {
                );
                return false;
            }
-        }, signatureText);
+        }, signatureText, null);
    }

    protected byte[] decodeBase64(String content) {
--- 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.Subject;
 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.CoreMatchers.not;
 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 class SamlAuthenticatorTests extends SamlResponseHandlerTests {
        // 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 class SamlAuthenticatorTests extends SamlResponseHandlerTests {
        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 {