Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
update formatIssuer to describeIssuer
  • Loading branch information
richard-dennehy committed Apr 7, 2025
commit f5ff8b17db4f2938233cfa42b9e715ba4cc44457
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ protected static String describe(Collection<X509Credential> credentials) {
return credentials.stream().map(credential -> describe(credential.getEntityCertificate())).collect(Collectors.joining(","));
}

void validateSignature(Signature signature, Issuer issuer) {
void validateSignature(Signature signature, @Nullable Issuer issuer) {
final String signatureText = text(signature, 32);
SAMLSignatureProfileValidator profileValidator = new SAMLSignatureProfileValidator();
try {
Expand Down Expand Up @@ -200,7 +200,7 @@ void validateSignature(Signature signature, Issuer issuer) {
);
return true;
} catch (PrivilegedActionException e) {
logger.warn("SecurityException while attempting to validate SAML signature" + formatIssuer(issuer), e);
logger.warn("SecurityException while attempting to validate SAML signature." + describeIssuer(issuer), e);
return false;
}
});
Expand All @@ -214,7 +214,7 @@ void validateSignature(Signature signature, Issuer issuer) {
* Tests whether the provided function returns {@code true} for any of the IdP's signing credentials.
* @throws ElasticsearchSecurityException - A SAML exception if no matching credential is found.
*/
protected void checkIdpSignature(CheckedFunction<Credential, Boolean, Exception> check, String signatureText, Issuer issuer) {
protected void checkIdpSignature(CheckedFunction<Credential, Boolean, Exception> check, String signatureText, @Nullable Issuer issuer) {
final Predicate<Credential> predicate = credential -> {
try {
return check.apply(credential);
Expand All @@ -231,7 +231,7 @@ 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" + formatIssuer(issuer), e);
logger.warn("Exception while attempting to validate SAML Signature." + describeIssuer(issuer), e);
return false;
}
};
Expand All @@ -245,15 +245,15 @@ protected void checkIdpSignature(CheckedFunction<Credential, Boolean, Exception>
* Constructs a SAML specific exception with a consistent message regarding SAML Signature validation failures
*/
private ElasticsearchSecurityException samlSignatureException(
Issuer issuer,
@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{}",
formatIssuer(issuer)
+ "metadata file/URL for this Identity Provider.{}",
describeIssuer(issuer)
);
final String msg = "SAML Signature [{}] could not be validated against [{}]";
if (cause != null) {
Expand All @@ -267,8 +267,8 @@ private ElasticsearchSecurityException samlSignatureException(Issuer issuer, Lis
return samlSignatureException(issuer, credentials, signature, null);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove duplication (since I need to edit the exact same log message twice)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

}

private String formatIssuer(Issuer issuer) {
return issuer != null ? Strings.format(" [%s]", issuer.getValue()) : "";
private String describeIssuer(@Nullable Issuer issuer) {
return issuer != null ? Strings.format(" The issuer included in the SAML message was [%s]", issuer.getValue()) : "";
}

private static String describeCredentials(List<Credential> credentials) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,8 @@ public class SamlAuthenticatorTests extends SamlResponseHandlerTests {
+ "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 "
+ "[https://idp.saml.elastic.test/]";
+ "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;

Expand Down Expand Up @@ -1356,7 +1356,8 @@ public void testFailureWhenIdPCredentialsAreNull() throws Exception {
"Null credentials",
authenticator.getClass().getName(),
Level.WARN,
"Exception while attempting to validate SAML Signature [https://idp.saml.elastic.test/]"
"Exception while attempting to validate SAML Signature. " +
"The issuer included in the SAML message was [https://idp.saml.elastic.test/]"
)
);

Expand Down
Loading