diff --git a/docs/changelog/126310.yaml b/docs/changelog/126310.yaml new file mode 100644 index 000000000000..a419a1036bd6 --- /dev/null +++ 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 index 71258e07caa5..9a86429c6c33 100644 --- 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()); } 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 index 6c5e6c2d80e2..ff7f0fa0704b 100644 --- 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); 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 index 3a16687f84b3..ad5b1f5a1bf0 100644 --- 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()); 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 index 9f35b32f211d..b118bcef2520 100644 --- 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 check, String signatureText) { + protected void checkIdpSignature(CheckedFunction check, String signatureText, @Nullable Issuer issuer) { final Predicate 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 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 credentials, String signature, Exception cause) { + private ElasticsearchSecurityException samlSignatureException( + @Nullable Issuer issuer, + List 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 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 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 credentials) { @@ -423,7 +442,7 @@ public class SamlObjectHandler { ); 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 index 83f09bad0d27..e0ba9688ef05 100644 --- 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 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 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 {