From 8c624e5a20c80905160c4425d672f28dcd73cd9a Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Mon, 11 Feb 2019 11:43:41 +0200 Subject: [PATCH] Enhance parsing of StatusCode in SAML Responses (#38628) * Enhance parsing of StatusCode in SAML Responses elements in a failed response might contain two nested elements. We currently only parse the first one in order to create a message that we attach to the Exception we return and log. However this is generic and only gives out informarion about whether the SAML IDP believes it's an error with the request or if it couldn't handle the request for other reasons. The encapsulated StatusCode has a more interesting error message that potentially gives out the actual error as in Invalid nameid policy, authentication failure etc. This change ensures that we print that information also, and removes Message and Details fields from the message when these are not part of the Status element (which quite often is the case) --- .../authc/saml/SamlAuthenticator.java | 29 +++++++- .../authc/saml/SamlAuthenticatorTests.java | 71 ++++++++++--------- 2 files changed, 63 insertions(+), 37 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 015cb1f8b18..6dd8a971d00 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 @@ -106,8 +106,7 @@ class SamlAuthenticator extends SamlRequestHandler { throw samlException("SAML Response has no status code"); } if (isSuccess(status) == false) { - throw samlException("SAML Response is not a 'success' response: Code={} Message={} Detail={}", - status.getStatusCode().getValue(), getMessage(status), getDetail(status)); + throw samlException("SAML Response is not a 'success' response: {}", getStatusCodeMessage(status)); } checkIssuer(response.getIssuer(), response); checkResponseDestination(response); @@ -137,6 +136,32 @@ class SamlAuthenticator extends SamlRequestHandler { return new SamlAttributes(nameId, session, attributes); } + private String getStatusCodeMessage(Status status) { + StatusCode firstLevel = status.getStatusCode(); + StatusCode subLevel = firstLevel.getStatusCode(); + StringBuilder sb = new StringBuilder(); + if (StatusCode.REQUESTER.equals(firstLevel.getValue())) { + sb.append("The SAML IdP did not grant the request. It indicated that the Elastic Stack side sent something invalid ("); + } else if (StatusCode.RESPONDER.equals(firstLevel.getValue())) { + sb.append("The request could not be granted due to an error in the SAML IDP side ("); + } else if (StatusCode.VERSION_MISMATCH.equals(firstLevel.getValue())) { + sb.append("The request could not be granted because the SAML IDP doesn't support SAML 2.0 ("); + } else { + sb.append("The request could not be granted, the SAML IDP responded with a non-standard Status code ("); + } + sb.append(firstLevel.getValue()).append(")."); + if (getMessage(status) != null) { + sb.append(" Message: [").append(getMessage(status)).append("]"); + } + if (getDetail(status) != null) { + sb.append(" Detail: [").append(getDetail(status)).append("]"); + } + if (null != subLevel) { + sb.append(" Specific status code which might indicate what the issue is: [").append(subLevel.getValue()).append("]"); + } + return sb.toString(); + } + private String getMessage(Status status) { final StatusMessage sm = status.getStatusMessage(); return sm == null ? null : sm.getMessage(); 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 8d10f3ffb69..7d5132ffb9f 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 @@ -1374,7 +1374,7 @@ public class SamlAuthenticatorTests extends SamlTestCase { } public void testContentIsRejectedIfNotMarkedAsSuccess() throws Exception { - final String xml = getSimpleResponse(clock.instant()).replace(StatusCode.SUCCESS, StatusCode.REQUESTER); + final String xml = getStatusFailedResponse(); final SamlToken token = token(signDoc(xml)); final ElasticsearchSecurityException exception = expectSamlException(() -> authenticator.authenticate(token)); assertThat(exception.getMessage(), containsString("not a 'success' response")); @@ -1408,8 +1408,7 @@ public class SamlAuthenticatorTests extends SamlTestCase { */ - final Element response = (Element) legitimateDocument. - getElementsByTagNameNS(SAML20P_NS, "Response").item(0); + final Element response = (Element) legitimateDocument.getElementsByTagNameNS(SAML20P_NS, "Response").item(0); final Element clonedResponse = (Element) response.cloneNode(true); final Element clonedSignature = (Element) clonedResponse. getElementsByTagNameNS("http://www.w3.org/2000/09/xmldsig#", "Signature").item(0); @@ -1443,8 +1442,7 @@ public class SamlAuthenticatorTests extends SamlTestCase { */ - final Element response = (Element) legitimateDocument. - getElementsByTagNameNS(SAML20P_NS, "Response").item(0); + final Element response = (Element) legitimateDocument.getElementsByTagNameNS(SAML20P_NS, "Response").item(0); final Element clonedResponse = (Element) response.cloneNode(true); final Element clonedSignature = (Element) clonedResponse. getElementsByTagNameNS("http://www.w3.org/2000/09/xmldsig#", "Signature").item(0); @@ -1482,8 +1480,7 @@ public class SamlAuthenticatorTests extends SamlTestCase { */ - final Element response = (Element) legitimateDocument. - getElementsByTagNameNS(SAML20P_NS, "Response").item(0); + final Element response = (Element) legitimateDocument.getElementsByTagNameNS(SAML20P_NS, "Response").item(0); final Element assertion = (Element) legitimateDocument. getElementsByTagNameNS(SAML20_NS, "Assertion").item(0); final Element forgedAssertion = (Element) assertion.cloneNode(true); @@ -1522,10 +1519,8 @@ public class SamlAuthenticatorTests extends SamlTestCase { */ - final Element response = (Element) legitimateDocument. - getElementsByTagNameNS(SAML20P_NS, "Response").item(0); - final Element assertion = (Element) legitimateDocument. - getElementsByTagNameNS(SAML20_NS, "Assertion").item(0); + final Element response = (Element) legitimateDocument.getElementsByTagNameNS(SAML20P_NS, "Response").item(0); + final Element assertion = (Element) legitimateDocument.getElementsByTagNameNS(SAML20_NS, "Assertion").item(0); final Element forgedAssertion = (Element) assertion.cloneNode(true); forgedAssertion.setAttribute("ID", "_forged_assertion_id"); final Element clonedSignature = (Element) forgedAssertion. @@ -1559,17 +1554,14 @@ public class SamlAuthenticatorTests extends SamlTestCase { */ - final Element response = (Element) legitimateDocument. - getElementsByTagNameNS(SAML20P_NS, "Response").item(0); - final Element assertion = (Element) legitimateDocument. - getElementsByTagNameNS(SAML20_NS, "Assertion").item(0); + final Element response = (Element) legitimateDocument.getElementsByTagNameNS(SAML20P_NS, "Response").item(0); + final Element assertion = (Element) legitimateDocument.getElementsByTagNameNS(SAML20_NS, "Assertion").item(0); final Element signature = (Element) assertion. - getElementsByTagNameNS("http://www.w3.org/2000/09/xmldsig#", "Signature").item(0); + getElementsByTagNameNS("http://www.w3.org/2000/09/xmldsig#", "Signature").item(0); assertion.removeChild(signature); final Element forgedAssertion = (Element) assertion.cloneNode(true); forgedAssertion.setAttribute("ID", "_forged_assertion_id"); - final Element issuer = (Element) forgedAssertion. - getElementsByTagNameNS(SAML20_NS, "Issuer").item(0); + final Element issuer = (Element) forgedAssertion.getElementsByTagNameNS(SAML20_NS, "Issuer").item(0); forgedAssertion.insertBefore(signature, issuer.getNextSibling()); response.insertBefore(forgedAssertion, assertion); final SamlToken forgedToken = token(SamlUtils.toString((legitimateDocument.getDocumentElement()))); @@ -1598,10 +1590,8 @@ public class SamlAuthenticatorTests extends SamlTestCase { */ - final Element response = (Element) legitimateDocument. - getElementsByTagNameNS(SAML20P_NS, "Response").item(0); - final Element assertion = (Element) legitimateDocument. - getElementsByTagNameNS(SAML20_NS, "Assertion").item(0); + final Element response = (Element) legitimateDocument.getElementsByTagNameNS(SAML20P_NS, "Response").item(0); + final Element assertion = (Element) legitimateDocument.getElementsByTagNameNS(SAML20_NS, "Assertion").item(0); final Element forgedAssertion = (Element) assertion.cloneNode(true); forgedAssertion.setAttribute("ID", "_forged_assertion_id"); final Element signature = (Element) assertion. @@ -1610,8 +1600,7 @@ public class SamlAuthenticatorTests extends SamlTestCase { getElementsByTagNameNS("http://www.w3.org/2000/09/xmldsig#", "Signature").item(0); forgedAssertion.removeChild(forgedSignature); assertion.removeChild(signature); - final Element issuer = (Element) forgedAssertion. - getElementsByTagNameNS(SAML20_NS, "Issuer").item(0); + final Element issuer = (Element) forgedAssertion.getElementsByTagNameNS(SAML20_NS, "Issuer").item(0); forgedAssertion.insertBefore(signature, issuer.getNextSibling()); signature.appendChild(assertion); response.appendChild(forgedAssertion); @@ -1642,11 +1631,9 @@ public class SamlAuthenticatorTests extends SamlTestCase { */ - final Element response = (Element) legitimateDocument. - getElementsByTagNameNS(SAML20P_NS, "Response").item(0); + final Element response = (Element) legitimateDocument.getElementsByTagNameNS(SAML20P_NS, "Response").item(0); final Element extensions = legitimateDocument.createElement("Extensions"); - final Element assertion = (Element) legitimateDocument. - getElementsByTagNameNS(SAML20_NS, "Assertion").item(0); + final Element assertion = (Element) legitimateDocument.getElementsByTagNameNS(SAML20_NS, "Assertion").item(0); response.insertBefore(extensions, assertion); final Element forgedAssertion = (Element) assertion.cloneNode(true); forgedAssertion.setAttribute("ID", "_forged_assertion_id"); @@ -1683,10 +1670,8 @@ public class SamlAuthenticatorTests extends SamlTestCase { */ - final Element response = (Element) legitimateDocument. - getElementsByTagNameNS(SAML20P_NS, "Response").item(0); - final Element assertion = (Element) legitimateDocument. - getElementsByTagNameNS(SAML20_NS, "Assertion").item(0); + final Element response = (Element) legitimateDocument.getElementsByTagNameNS(SAML20P_NS, "Response").item(0); + final Element assertion = (Element) legitimateDocument.getElementsByTagNameNS(SAML20_NS, "Assertion").item(0); final Element forgedAssertion = (Element) assertion.cloneNode(true); forgedAssertion.setAttribute("ID", "_forged_assertion_id"); final Element signature = (Element) assertion. @@ -1695,8 +1680,7 @@ public class SamlAuthenticatorTests extends SamlTestCase { getElementsByTagNameNS("http://www.w3.org/2000/09/xmldsig#", "Signature").item(0); forgedAssertion.removeChild(forgedSignature); assertion.removeChild(signature); - final Element issuer = (Element) forgedAssertion. - getElementsByTagNameNS(SAML20_NS, "Issuer").item(0); + final Element issuer = (Element) forgedAssertion.getElementsByTagNameNS(SAML20_NS, "Issuer").item(0); forgedAssertion.insertBefore(signature, issuer.getNextSibling()); Element object = legitimateDocument.createElement("Object"); object.appendChild(assertion); @@ -2034,7 +2018,7 @@ public class SamlAuthenticatorTests extends SamlTestCase { } private Element buildEncryptedKeyElement(Document document, EncryptedKey encryptedKey, X509Certificate certificate) - throws XMLSecurityException { + throws XMLSecurityException { final XMLCipher cipher = XMLCipher.getInstance(); final org.apache.xml.security.keys.KeyInfo keyInfo = new org.apache.xml.security.keys.KeyInfo(document); final X509Data x509Data = new X509Data(document); @@ -2054,6 +2038,23 @@ public class SamlAuthenticatorTests extends SamlTestCase { return authenticator.buildXmlObject(doc.getDocumentElement(), Response.class); } + private String getStatusFailedResponse() { + final Instant now = clock.instant(); + return "\n" + + "" + + "" + IDP_ENTITY_ID + "" + + "" + + "" + + "" + + ""; + } + private String getSimpleResponse(Instant now) { return getSimpleResponse(now, randomAlphaOfLengthBetween(12, 18), randomId()); }