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

richard-dennehy · 2025-04-04T14:25:28Z

Includes Issuer in warning logs when SAML Signature validation fails (when available) to make it clearer that these logs are false positives and can be ignored when multiple realms are configured.

richard-dennehy · 2025-04-04T15:49:01Z

...in/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlObjectHandler.java

-        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);


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

richard-dennehy · 2025-04-04T15:51:12Z

...in/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlObjectHandler.java

@@ -423,7 +429,7 @@ private void validateSignature(String inputString, String signatureAlgorithm, St
                );
                return false;
            }
-        }, signatureText);
+        }, signatureText, null);


the context of this is SAML Logout with a query string; the message gets parsed into a SAML document after the signature has been verified, so I'm not sure it's possible to pass an Issuer here

Yes, I agree. We could change the order of things here so the issuer is parsed from the message before throwing the error, but if that parsing fails, it ends up causing more confusion than help so I think it makes sense to leave it as it.

richard-dennehy · 2025-04-04T15:51:50Z

...curity/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlAuthenticatorTests.java

-        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())) {


update existing tests to check for log message

...curity/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlAuthenticatorTests.java

elasticsearchmachine · 2025-04-04T15:54:34Z

Hi @richard-dennehy, I've created a changelog YAML for you.

jfreden

Great job on this PR! I've left a couple of small comments. I think the CI failure happens because the branch is outdated.

Can you also create a PR against the docs-content repo as suggested here: #111022 (comment)

I think this PR can be moved to "Ready for Review".

jfreden · 2025-04-07T07:16:38Z

...in/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlObjectHandler.java

        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 -> {


Should the warning in this method also include the issuer? https://github.com/elastic/elasticsearch/pull/126310/files#diff-19351c3f19f00d928645d11a61ef3c3dea10982b13f8f7ed47773c18d034b33dR203

And maybe here too: https://github.com/elastic/elasticsearch/pull/126310/files#diff-19351c3f19f00d928645d11a61ef3c3dea10982b13f8f7ed47773c18d034b33dL234

...curity/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlAuthenticatorTests.java

jfreden · 2025-04-07T08:28:51Z

...curity/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlAuthenticatorTests.java

-        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())) {


jfreden · 2025-04-07T08:52:27Z

...in/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlObjectHandler.java

@@ -423,7 +429,7 @@ private void validateSignature(String inputString, String signatureAlgorithm, St
                );
                return false;
            }
-        }, signatureText);
+        }, signatureText, null);


Yes, I agree. We could change the order of things here so the issuer is parsed from the message before throwing the error, but if that parsing fails, it ends up causing more confusion than help so I think it makes sense to leave it as it.

jfreden · 2025-04-07T08:58:32Z

...in/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlObjectHandler.java

-            "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 {}",


nit: A convention for logging is:

If the log message includes values from your code then you must use placeholders rather than constructing the string yourself using simple concatenation. Consider wrapping the values in [...] to help distinguish them from the static part of the message.

Can you add that around issuerValue (maybe at the null check?)?

jfreden · 2025-04-07T08:58:44Z

...in/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlObjectHandler.java

-        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);


jfreden · 2025-04-07T09:11:26Z

Nit thing about the label. Since this is a Realm change, this could be labeled as :Security/Authentication

elasticsearchmachine · 2025-04-07T09:22:17Z

Pinging @elastic/es-security (Team:Security)

…uer-in-warn-logs

n1v0lg

Thanks for working on this! I'll leave the final review to Johannes -- just adding some additional thoughts/suggestions.

...in/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlObjectHandler.java

n1v0lg · 2025-04-07T10:10:45Z

...in/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlObjectHandler.java

+    }
+
+    private String formatIssuer(Issuer issuer) {
+        return issuer != null ? String.format(" [%s]", issuer.getValue()) : "";


I think we should make it clear that the issuer value is a value we parsed from the request -- esp. since we're logging it in the context where signature verification failed it can potentially be an arbitrary, untrusted value so we should identify that in the log message -- something like "The issuer included in the SAML message was [{}]."

Also, instead of "" I'd go for an explicit value like <missing-issuer> or <null>.

We could call this method describeIssuer to match describeCredentials

One more thought here:

@jfreden @richard-dennehy: WDYT about limiting the length here with a call to cleanTruncate (with a ... if we actually truncated) to avoid excessively long log lines/exception messages?

Could it be more confusing to explicitly describe the issuer as null? In the cases where issuer is null, all it means is we haven't parsed it from the request yet

Also, instead of "" I'd go for an explicit value like or .

For the logout flow, the issuer is probably included (so not missing, we just haven't parsed it yet) and if it's not, validation will fail later (if the signature is correct). Because of that, I think "" is acceptable in this case. My thinking is that null as issuer probably won't happen or at least be extremely rare in production and if it happens, validation will fail because it's a required field.

@jfreden @richard-dennehy: WDYT about limiting the length here with a call to cleanTruncate (with a ... if we actually truncated) to avoid excessively long log lines/exception messages?

Is that necessary for the issuer? Are you worried the issuer is a very long url?

Ah right, good point -- for logout requests we always pass null. Okay, in that case let's go with your approach of not including information about the issuer if the value is null (but the more informative message if it available). We could come up with something more nuanced where we call different methods depending on whether it's a null by definition vs. a null by parsing but no need to over-engineer this thing.

We could call this method describeIssuer to match describeCredentials

++

Is that necessary for the issuer? Are you worried the issuer is a very long url?

Worried primarily about processing a value that's arbitrarily malformed since it's not from a trusted source (we log in cases where the signature verification failed). Limiting it to some high-ish length value would not impede UX but prevent unforeseen issues around large values. It's a smaller concern since for the cases where the value is available, we've already parsed the entire SAML message and thus done much of the heavy lifting.

Thanks for clarifying! Looks like neither opensaml or the saml spec limits the size of the issuer field, so I think adding a cleanTruncate call makes sense.

richard-dennehy · 2025-04-07T12:24:53Z

...curity/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlAuthenticatorTests.java

+        }
+    }
+
+    public void testDescribeNullIssuer() {


added some tests for describeIssuer since it's getting a little complicated

richard-dennehy · 2025-04-07T12:26:06Z

...in/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlObjectHandler.java

+            return "";
+        }
+        final String msg = " The issuer included in the SAML message was [%s]";
+        if (issuer.getValue().length() > 64) {


issuer.getValue() can return null, as I discovered writing the tests - how realistic do we think this is? Should I guard against it?

Yup, lets guard against null here 👍

Nit: I'd move the 64 into a constant and make the threshold higher -- 512 is still negligible in terms of a logging payload but less likely to truncate useful info

jfreden

LGTM pending CI! Just a couple of nits. Good job!

jfreden · 2025-04-07T13:20:14Z

...curity/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlAuthenticatorTests.java

+
+    public void testDescribeVeryLongIssuer() {
+        final Issuer issuer = new IssuerBuilder().buildObject();
+        issuer.setValue("https://idp.saml.elastic.test/" + "a".repeat(128));


nit: there is a utility function called randomAlphaOfLength(128) that could be used here.

jfreden · 2025-04-07T13:22:46Z

...curity/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlAuthenticatorTests.java

+    }
+
+    public void testDescribeNullIssuerValue() {
+        final Issuer issuer = new IssuerBuilder().buildObject();


nit: testDescribeNullIssuer and this one can be combined using randomFrom(null, new IssuerBuilder().buildObject())

jfreden · 2025-04-07T13:23:22Z

...in/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlObjectHandler.java

+    }
+
+    // package private for testing
+    String describeIssuer(@Nullable Issuer issuer) {


nit: this could be static.

…lastic#126310) * 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 <[email protected]> (cherry picked from commit ceaa01a)

richard-dennehy · 2025-04-23T08:50:11Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x
✅	9.0
✅	8.18
✅	8.17

Questions ?

Please refer to the Backport tool documentation

…lastic#126310) * 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 <[email protected]> (cherry picked from commit ceaa01a)

…126310) (#127218) * 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 <[email protected]> (cherry picked from commit ceaa01a)

…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 <[email protected]> (cherry picked from commit ceaa01a)

…126310) (#127221) * 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 <[email protected]> (cherry picked from commit ceaa01a)

…126310) (#127219) * 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 <[email protected]> (cherry picked from commit ceaa01a)

Add Issuer to failed SAML Signature validation logs when available

39fe357

elasticsearchmachine added the v9.1.0 label Apr 4, 2025

elasticsearchmachine and others added 2 commits April 4, 2025 14:34

[CI] Auto commit changes from spotless

8d4d813

Fix tests

8533c8e

richard-dennehy commented Apr 4, 2025

View reviewed changes

richard-dennehy added >enhancement :Security/Security Security issues without another label Team:Security Meta label for security team labels Apr 4, 2025

richard-dennehy requested review from n1v0lg and jfreden April 4, 2025 15:54

Update docs/changelog/126310.yaml

f714d35

jfreden reviewed Apr 7, 2025

View reviewed changes

richard-dennehy added the :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) label Apr 7, 2025

richard-dennehy marked this pull request as ready for review April 7, 2025 09:21

richard-dennehy added 3 commits April 7, 2025 10:51

Merge branch 'main' of github.com:elastic/elasticsearch into saml-iss…

312b2ab

…uer-in-warn-logs

address review comments

198cdf9

replace String.format call

7de3149

n1v0lg reviewed Apr 7, 2025

View reviewed changes

richard-dennehy and others added 3 commits April 7, 2025 12:07

update formatIssuer to describeIssuer

f5ff8b1

[CI] Auto commit changes from spotless

301f6a0

truncate long issuers in log messages

fa9b48c

richard-dennehy commented Apr 7, 2025

View reviewed changes

elasticsearchmachine and others added 2 commits April 7, 2025 12:30

[CI] Auto commit changes from spotless

16cb4b8

handle null issuer value

de44463

jfreden approved these changes Apr 7, 2025

View reviewed changes

address review comments

42ebafc

jfreden removed the :Security/Security Security issues without another label label Apr 7, 2025

richard-dennehy added 2 commits April 7, 2025 17:02

Merge branch 'main' into saml-issuer-in-warn-logs

474a79c

Merge branch 'main' into saml-issuer-in-warn-logs

4893144

richard-dennehy merged commit ceaa01a into elastic:main Apr 8, 2025
22 checks passed

richard-dennehy deleted the saml-issuer-in-warn-logs branch April 8, 2025 09:50

richard-dennehy added v8.18.1 v8.19.0 v9.0.1 v8.17.6 labels Apr 23, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

richard-dennehy commented Apr 4, 2025

richard-dennehy Apr 4, 2025

jfreden Apr 7, 2025

richard-dennehy Apr 4, 2025

jfreden Apr 7, 2025

richard-dennehy Apr 4, 2025

jfreden Apr 7, 2025

elasticsearchmachine commented Apr 4, 2025

jfreden left a comment

jfreden Apr 7, 2025

jfreden Apr 7, 2025

jfreden Apr 7, 2025

jfreden Apr 7, 2025

jfreden Apr 7, 2025

jfreden commented Apr 7, 2025

elasticsearchmachine commented Apr 7, 2025

n1v0lg left a comment

n1v0lg Apr 7, 2025

n1v0lg Apr 7, 2025 •

edited

Loading

richard-dennehy Apr 7, 2025

jfreden Apr 7, 2025

n1v0lg Apr 7, 2025

jfreden Apr 7, 2025

n1v0lg Apr 7, 2025 •

edited

Loading

jfreden Apr 7, 2025

richard-dennehy Apr 7, 2025

richard-dennehy Apr 7, 2025

n1v0lg Apr 7, 2025

n1v0lg Apr 7, 2025 •

edited

Loading

jfreden left a comment

jfreden Apr 7, 2025

jfreden Apr 7, 2025

jfreden Apr 7, 2025

richard-dennehy commented Apr 23, 2025

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

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

Conversation

richard-dennehy commented Apr 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticsearchmachine commented Apr 4, 2025

jfreden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfreden commented Apr 7, 2025

elasticsearchmachine commented Apr 7, 2025

n1v0lg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n1v0lg Apr 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n1v0lg Apr 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n1v0lg Apr 7, 2025 • edited Loading

Choose a reason for hiding this comment

jfreden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richard-dennehy commented Apr 23, 2025

💚 All backports created successfully

Questions ?

n1v0lg Apr 7, 2025 •

edited

Loading

n1v0lg Apr 7, 2025 •

edited

Loading

n1v0lg Apr 7, 2025 •

edited

Loading