From 4b319d715183114efe56c3cd94202724e0fb963c Mon Sep 17 00:00:00 2001
From: Ioannis Kakavas <ikakavas@protonmail.com>
Date: Thu, 10 May 2018 11:42:22 +0300
Subject: [PATCH] SAML: Process only signed data (#30420)

As conformance to best practices, this changes ensures that if a
SAML Response is signed, we verify the signature before processing
it any further. We were only checking the InResponseTo and
Destination attributes before potential signature validation but
there was no reason to do that up front either.
---
 .../authc/saml/SamlAuthenticator.java         | 26 +++++++++----------
 1 file changed, 12 insertions(+), 14 deletions(-)

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 93bbe2c1a7567..f8826bebcac71 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
@@ -87,6 +87,14 @@ private SamlAttributes authenticateResponse(Element element, Collection<String>
         if (logger.isTraceEnabled()) {
             logger.trace(SamlUtils.describeSamlObject(response));
         }
+        final boolean requireSignedAssertions;
+        if (response.isSigned()) {
+            validateSignature(response.getSignature());
+            requireSignedAssertions = false;
+        } else {
+            requireSignedAssertions = true;
+        }
+
         if (Strings.hasText(response.getInResponseTo()) && allowedSamlRequestIds.contains(response.getInResponseTo()) == false) {
             logger.debug("The SAML Response with ID {} is unsolicited. A user might have used a stale URL or the Identity Provider " +
                     "incorrectly populates the InResponseTo attribute", response.getID());
@@ -102,10 +110,10 @@ private SamlAttributes authenticateResponse(Element element, Collection<String>
             throw samlException("SAML Response is not a 'success' response: Code={} Message={} Detail={}",
                     status.getStatusCode().getValue(), getMessage(status), getDetail(status));
         }
-
+        checkIssuer(response.getIssuer(), response);
         checkResponseDestination(response);
 
-        Tuple<Assertion, List<Attribute>> details = extractDetails(response, allowedSamlRequestIds);
+        Tuple<Assertion, List<Attribute>> details = extractDetails(response, allowedSamlRequestIds, requireSignedAssertions);
         final Assertion assertion = details.v1();
         final SamlNameId nameId = SamlNameId.forSubject(assertion.getSubject());
         final String session = getSessionIndex(assertion);
@@ -156,17 +164,8 @@ private void checkResponseDestination(Response response) {
         }
     }
 
-    private Tuple<Assertion, List<Attribute>> extractDetails(Response response, Collection<String> allowedSamlRequestIds) {
-        final boolean requireSignedAssertions;
-        if (response.isSigned()) {
-            validateSignature(response.getSignature());
-            requireSignedAssertions = false;
-        } else {
-            requireSignedAssertions = true;
-        }
-
-        checkIssuer(response.getIssuer(), response);
-
+    private Tuple<Assertion, List<Attribute>> extractDetails(Response response, Collection<String> allowedSamlRequestIds,
+                                                             boolean requireSignedAssertions) {
         final int assertionCount = response.getAssertions().size() + response.getEncryptedAssertions().size();
         if (assertionCount > 1) {
             throw samlException("Expecting only 1 assertion, but response contains multiple (" + assertionCount + ")");
@@ -328,5 +327,4 @@ private void checkLifetimeRestrictions(Conditions conditions) {
     private void checkLifetimeRestrictions(SubjectConfirmationData subjectConfirmationData) {
         validateNotOnOrAfter(subjectConfirmationData.getNotOnOrAfter());
     }
-
 }