From f290d4d5faf2667b232bd5f381d2817be556516b Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Wed, 9 Jun 2021 21:56:46 +1000 Subject: [PATCH 1/4] Deprecate queryString and replace it with query_string --- .../security/saml-complete-logout-api.asciidoc | 8 ++++++-- .../en/rest-api/security/saml-invalidate-api.asciidoc | 10 +++++++--- .../action/saml/SamlCompleteLogoutRequest.java | 10 +++++++--- .../action/saml/SamlInvalidateSessionRequest.java | 6 +++++- .../rest/action/saml/RestSamlCompleteLogoutAction.java | 4 +++- .../action/saml/RestSamlInvalidateSessionAction.java | 4 +++- 6 files changed, 31 insertions(+), 11 deletions(-) diff --git a/x-pack/docs/en/rest-api/security/saml-complete-logout-api.asciidoc b/x-pack/docs/en/rest-api/security/saml-complete-logout-api.asciidoc index 7969e723c2bf6..7ebb49494d9bf 100644 --- a/x-pack/docs/en/rest-api/security/saml-complete-logout-api.asciidoc +++ b/x-pack/docs/en/rest-api/security/saml-complete-logout-api.asciidoc @@ -46,10 +46,14 @@ clients. See also <>, (Required, array) A json array with all the valid SAML Request Ids that the caller of the API has for the current user. -`queryString`:: +`query_string`:: (Optional, string) If the SAML IdP sends the logout response with the HTTP-Redirect binding, this field must be set to the query string of the redirect URI. +`queryString`:: +deprecated:[7.14.0, "Use query_string instead"] + See `query_string` + `content`:: (Optional, string) If the SAML IdP sends the logout response with the HTTP-Post binding, this field must be set to the value of the `SAMLResponse` form parameter @@ -67,7 +71,7 @@ POST /_security/saml/complete_logout { "realm": "saml1", "ids": [ "_1c368075e0b3..." ], - "queryString": "SAMLResponse=fZHLasMwEEVbfb1bf...&SigAlg=http%3A%2F%2Fwww.w3.org%2F2000%2F09%2Fxmldsig%23rsa-sha1&Signature=CuCmFn%2BLqnaZGZJqK..." + "query_string": "SAMLResponse=fZHLasMwEEVbfb1bf...&SigAlg=http%3A%2F%2Fwww.w3.org%2F2000%2F09%2Fxmldsig%23rsa-sha1&Signature=CuCmFn%2BLqnaZGZJqK..." } -------------------------------------------------- // TEST[skip:can't test this without a valid SAML Logout Response] diff --git a/x-pack/docs/en/rest-api/security/saml-invalidate-api.asciidoc b/x-pack/docs/en/rest-api/security/saml-invalidate-api.asciidoc index 8e43d3175a9b3..dc663ce787862 100644 --- a/x-pack/docs/en/rest-api/security/saml-invalidate-api.asciidoc +++ b/x-pack/docs/en/rest-api/security/saml-invalidate-api.asciidoc @@ -40,16 +40,20 @@ clients. See also <>, (Optional, string) The Assertion Consumer Service URL that matches the one of the SAML realm in {es} that should be used. You must specify either this parameter or the `realm` parameter. -`queryString`:: +`query_string`:: (Required, string) The query part of the URL that the user was redirected to by the SAML IdP to initiate the Single Logout. This query should include a single parameter named `SAMLRequest` that contains a SAML logout request that is deflated and Base64 encoded. If the SAML IdP has signed the logout request, the URL should include two extra parameters named `SigAlg` and `Signature` that contain the algorithm used for the signature and the signature value itself. -In order for {es} to be able to verify the IdP's signature, the value of the queryString field must be an exact match to the string provided by the browser. +In order for {es} to be able to verify the IdP's signature, the value of the query_string field must be an exact match to the string provided by the browser. The client application must not attempt to parse or process the string in any way. +`queryString`:: +deprecated:[7.14.0, "Use query_string instead"] + See `query_string`. + `realm`:: (Optional, string) The name of the SAML realm in {es} the configuration. You must specify either this parameter or the `acs` parameter. @@ -78,7 +82,7 @@ the user that is identified in the SAML Logout Request: -------------------------------------------------- POST /_security/saml/invalidate { - "queryString" : "SAMLRequest=nZFda4MwFIb%2FiuS%2BmviRpqFaClKQdbvo2g12M2KMraCJ9cRR9utnW4Wyi13sMie873MeznJ1aWrnS3VQGR0j4mLkKC1NUeljjA77zYyhVbIE0dR%2By7fmaHq7U%2BdegXWGpAZ%2B%2F4pR32luBFTAtWgUcCv56%2Fp5y30X87Yz1khTIycdgpUW9kY7WdsC9zxoXTvMvWuVV98YyMnSGH2SYE5pwALBIr9QKiwDGpW0oGVUznGeMyJZKFkQ4jBf5HnhUymjIhzCAL3KNFihbYx8TBYzzGaY7EnIyZwHzCWMfiDnbRIftkSjJr%2BFu0e9v%2B0EgOquRiiZjKpiVFp6j50T4WXoyNJ%2FEWC9fdqc1t%2F1%2B2F3aUpjzhPiXpqMz1%2FHSn4A&SigAlg=http%3A%2F%2Fwww.w3.org%2F2001%2F04%2Fxmldsig-more%23rsa-sha256&Signature=MsAYz2NFdovMG2mXf6TSpu5vlQQyEJAg%2B4KCwBqJTmrb3yGXKUtIgvjqf88eCAK32v3eN8vupjPC8LglYmke1ZnjK0%2FKxzkvSjTVA7mMQe2AQdKbkyC038zzRq%2FYHcjFDE%2Bz0qISwSHZY2NyLePmwU7SexEXnIz37jKC6NMEhus%3D", + "query_string" : "SAMLRequest=nZFda4MwFIb%2FiuS%2BmviRpqFaClKQdbvo2g12M2KMraCJ9cRR9utnW4Wyi13sMie873MeznJ1aWrnS3VQGR0j4mLkKC1NUeljjA77zYyhVbIE0dR%2By7fmaHq7U%2BdegXWGpAZ%2B%2F4pR32luBFTAtWgUcCv56%2Fp5y30X87Yz1khTIycdgpUW9kY7WdsC9zxoXTvMvWuVV98YyMnSGH2SYE5pwALBIr9QKiwDGpW0oGVUznGeMyJZKFkQ4jBf5HnhUymjIhzCAL3KNFihbYx8TBYzzGaY7EnIyZwHzCWMfiDnbRIftkSjJr%2BFu0e9v%2B0EgOquRiiZjKpiVFp6j50T4WXoyNJ%2FEWC9fdqc1t%2F1%2B2F3aUpjzhPiXpqMz1%2FHSn4A&SigAlg=http%3A%2F%2Fwww.w3.org%2F2001%2F04%2Fxmldsig-more%23rsa-sha256&Signature=MsAYz2NFdovMG2mXf6TSpu5vlQQyEJAg%2B4KCwBqJTmrb3yGXKUtIgvjqf88eCAK32v3eN8vupjPC8LglYmke1ZnjK0%2FKxzkvSjTVA7mMQe2AQdKbkyC038zzRq%2FYHcjFDE%2Bz0qISwSHZY2NyLePmwU7SexEXnIz37jKC6NMEhus%3D", "realm" : "saml1" } -------------------------------------------------- diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutRequest.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutRequest.java index d885105fe76f0..fa3107660dbc1 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutRequest.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutRequest.java @@ -43,10 +43,10 @@ public ActionRequestValidationException validate() { validationException = addValidationError("realm may not be empty", validationException); } if (Strings.hasText(queryString) == false && Strings.hasText(content) == false) { - validationException = addValidationError("queryString and content may not both be empty", validationException); + validationException = addValidationError("query_string and content may not both be empty", validationException); } if (Strings.hasText(queryString) && Strings.hasText(content)) { - validationException = addValidationError("queryString and content may not both present", validationException); + validationException = addValidationError("query_string and content may not both present", validationException); } return validationException; } @@ -56,7 +56,11 @@ public String getQueryString() { } public void setQueryString(String queryString) { - this.queryString = queryString; + if (this.queryString == null) { + this.queryString = queryString; + } else { + throw new IllegalArgumentException("Must use either [queryString] or [query_string], not both at the same time"); + } } public String getContent() { diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlInvalidateSessionRequest.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlInvalidateSessionRequest.java index 3d715198d380a..efd060832e590 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlInvalidateSessionRequest.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlInvalidateSessionRequest.java @@ -50,7 +50,11 @@ public String getQueryString() { } public void setQueryString(String queryString) { - this.queryString = queryString; + if (this.queryString == null) { + this.queryString = queryString; + } else { + throw new IllegalArgumentException("Must use either [queryString] or [query_string], not both at the same time"); + } } public String getRealmName() { diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlCompleteLogoutAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlCompleteLogoutAction.java index 7eb75fc4e8eee..d1489da370f4c 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlCompleteLogoutAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlCompleteLogoutAction.java @@ -47,10 +47,12 @@ public class RestSamlCompleteLogoutAction extends SamlBaseRestHandler{ PARSER = new ObjectParser<>("saml_complete_logout", SamlCompleteLogoutRequest::new); static { - PARSER.declareStringOrNull(SamlCompleteLogoutRequest::setQueryString, new ParseField("queryString")); + PARSER.declareStringOrNull(SamlCompleteLogoutRequest::setQueryString, + new ParseField("queryString").withAllDeprecated("query_string")); PARSER.declareStringOrNull(SamlCompleteLogoutRequest::setContent, new ParseField("content")); PARSER.declareStringArray(SamlCompleteLogoutRequest::setValidRequestIds, new ParseField("ids")); PARSER.declareString(SamlCompleteLogoutRequest::setRealm, new ParseField("realm")); + PARSER.declareStringOrNull(SamlCompleteLogoutRequest::setQueryString, new ParseField("query_string")); } public RestSamlCompleteLogoutAction(Settings settings, XPackLicenseState licenseState) { diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlInvalidateSessionAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlInvalidateSessionAction.java index 0d28f9e126fc9..063823199735e 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlInvalidateSessionAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlInvalidateSessionAction.java @@ -38,9 +38,11 @@ public class RestSamlInvalidateSessionAction extends SamlBaseRestHandler { new ObjectParser<>("saml_invalidate_session", SamlInvalidateSessionRequest::new); static { - PARSER.declareString(SamlInvalidateSessionRequest::setQueryString, new ParseField("queryString")); + PARSER.declareString(SamlInvalidateSessionRequest::setQueryString, + new ParseField("queryString").withAllDeprecated("query_string")); PARSER.declareString(SamlInvalidateSessionRequest::setAssertionConsumerServiceURL, new ParseField("acs")); PARSER.declareString(SamlInvalidateSessionRequest::setRealmName, new ParseField("realm")); + PARSER.declareString(SamlInvalidateSessionRequest::setQueryString, new ParseField("query_string")); } public RestSamlInvalidateSessionAction(Settings settings, XPackLicenseState licenseState) { From 3f4a3a5683baf9602763b73a211c95dd91f6d813 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Thu, 10 Jun 2021 16:13:31 +1000 Subject: [PATCH 2/4] Add tests --- .../saml/SamlCompleteLogoutRequest.java | 2 +- .../saml/SamlInvalidateSessionRequest.java | 4 ++-- .../saml/SamlCompleteLogoutRequestTests.java | 12 ++++++++-- .../SamlInvalidateSessionRequestTests.java | 23 +++++++++++++++++++ 4 files changed, 36 insertions(+), 5 deletions(-) create mode 100644 x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/saml/SamlInvalidateSessionRequestTests.java diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutRequest.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutRequest.java index fa3107660dbc1..b19b13a9716a2 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutRequest.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutRequest.java @@ -59,7 +59,7 @@ public void setQueryString(String queryString) { if (this.queryString == null) { this.queryString = queryString; } else { - throw new IllegalArgumentException("Must use either [queryString] or [query_string], not both at the same time"); + throw new IllegalArgumentException("Must use either [query_string] or [queryString], not both at the same time"); } } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlInvalidateSessionRequest.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlInvalidateSessionRequest.java index efd060832e590..2531589342738 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlInvalidateSessionRequest.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlInvalidateSessionRequest.java @@ -40,7 +40,7 @@ public SamlInvalidateSessionRequest() { public ActionRequestValidationException validate() { ActionRequestValidationException validationException = null; if (Strings.isNullOrEmpty(queryString)) { - validationException = addValidationError("queryString is missing", validationException); + validationException = addValidationError("query_string is missing", validationException); } return validationException; } @@ -53,7 +53,7 @@ public void setQueryString(String queryString) { if (this.queryString == null) { this.queryString = queryString; } else { - throw new IllegalArgumentException("Must use either [queryString] or [query_string], not both at the same time"); + throw new IllegalArgumentException("Must use either [query_string] or [queryString], not both at the same time"); } } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutRequestTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutRequestTests.java index dc59bf5f1ba44..b80a6ee6618d9 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutRequestTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutRequestTests.java @@ -18,7 +18,7 @@ public void testValidateFailsWhenQueryAndBodyBothNotExist() { final SamlCompleteLogoutRequest samlCompleteLogoutRequest = new SamlCompleteLogoutRequest(); samlCompleteLogoutRequest.setRealm("realm"); final ActionRequestValidationException validationException = samlCompleteLogoutRequest.validate(); - assertThat(validationException.getMessage(), containsString("queryString and content may not both be empty")); + assertThat(validationException.getMessage(), containsString("query_string and content may not both be empty")); } public void testValidateFailsWhenQueryAndBodyBothSet() { @@ -27,7 +27,7 @@ public void testValidateFailsWhenQueryAndBodyBothSet() { samlCompleteLogoutRequest.setQueryString("queryString"); samlCompleteLogoutRequest.setContent("content"); final ActionRequestValidationException validationException = samlCompleteLogoutRequest.validate(); - assertThat(validationException.getMessage(), containsString("queryString and content may not both present")); + assertThat(validationException.getMessage(), containsString("query_string and content may not both present")); } public void testValidateFailsWhenRealmIsNotSet() { @@ -36,4 +36,12 @@ public void testValidateFailsWhenRealmIsNotSet() { final ActionRequestValidationException validationException = samlCompleteLogoutRequest.validate(); assertThat(validationException.getMessage(), containsString("realm may not be empty")); } + + public void testCannotSetQueryStringTwice() { + final SamlCompleteLogoutRequest samlCompleteLogoutRequest = new SamlCompleteLogoutRequest(); + samlCompleteLogoutRequest.setQueryString("query_string"); + final IllegalArgumentException e = + expectThrows(IllegalArgumentException.class, () -> samlCompleteLogoutRequest.setQueryString("queryString")); + assertThat(e.getMessage(), containsString("Must use either [query_string] or [queryString], not both at the same time")); + } } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/saml/SamlInvalidateSessionRequestTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/saml/SamlInvalidateSessionRequestTests.java new file mode 100644 index 0000000000000..0331880b2bce4 --- /dev/null +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/saml/SamlInvalidateSessionRequestTests.java @@ -0,0 +1,23 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +package org.elasticsearch.xpack.core.security.action.saml; + +import org.elasticsearch.test.ESTestCase; + +import static org.hamcrest.Matchers.containsString; + +public class SamlInvalidateSessionRequestTests extends ESTestCase { + + public void testCannotSetQueryStringTwice() { + final SamlInvalidateSessionRequest samlInvalidateSessionRequest = new SamlInvalidateSessionRequest(); + samlInvalidateSessionRequest.setQueryString("query_string"); + final IllegalArgumentException e = + expectThrows(IllegalArgumentException.class, () -> samlInvalidateSessionRequest.setQueryString("queryString")); + assertThat(e.getMessage(), containsString("Must use either [query_string] or [queryString], not both at the same time")); + } +} From 32e7778ca5f2e0610848f6d6e56b406475c16551 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Tue, 22 Jun 2021 21:13:13 +1000 Subject: [PATCH 3/4] Update x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlCompleteLogoutAction.java Co-authored-by: Tim Vernum --- .../rest/action/saml/RestSamlCompleteLogoutAction.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlCompleteLogoutAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlCompleteLogoutAction.java index d1489da370f4c..4c74bd4c30985 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlCompleteLogoutAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlCompleteLogoutAction.java @@ -47,12 +47,10 @@ public class RestSamlCompleteLogoutAction extends SamlBaseRestHandler{ PARSER = new ObjectParser<>("saml_complete_logout", SamlCompleteLogoutRequest::new); static { - PARSER.declareStringOrNull(SamlCompleteLogoutRequest::setQueryString, - new ParseField("queryString").withAllDeprecated("query_string")); + PARSER.declareStringOrNull(SamlCompleteLogoutRequest::setQueryString, new ParseField("query_string", "queryString")); PARSER.declareStringOrNull(SamlCompleteLogoutRequest::setContent, new ParseField("content")); PARSER.declareStringArray(SamlCompleteLogoutRequest::setValidRequestIds, new ParseField("ids")); PARSER.declareString(SamlCompleteLogoutRequest::setRealm, new ParseField("realm")); - PARSER.declareStringOrNull(SamlCompleteLogoutRequest::setQueryString, new ParseField("query_string")); } public RestSamlCompleteLogoutAction(Settings settings, XPackLicenseState licenseState) { From a2f09cafba33360553357712d575dac6694a6e77 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Tue, 22 Jun 2021 21:15:40 +1000 Subject: [PATCH 4/4] address feedback --- .../rest/action/saml/RestSamlInvalidateSessionAction.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlInvalidateSessionAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlInvalidateSessionAction.java index 063823199735e..3e2ac5b718d53 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlInvalidateSessionAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlInvalidateSessionAction.java @@ -38,11 +38,9 @@ public class RestSamlInvalidateSessionAction extends SamlBaseRestHandler { new ObjectParser<>("saml_invalidate_session", SamlInvalidateSessionRequest::new); static { - PARSER.declareString(SamlInvalidateSessionRequest::setQueryString, - new ParseField("queryString").withAllDeprecated("query_string")); + PARSER.declareString(SamlInvalidateSessionRequest::setQueryString, new ParseField("query_string", "queryString")); PARSER.declareString(SamlInvalidateSessionRequest::setAssertionConsumerServiceURL, new ParseField("acs")); PARSER.declareString(SamlInvalidateSessionRequest::setRealmName, new ParseField("realm")); - PARSER.declareString(SamlInvalidateSessionRequest::setQueryString, new ParseField("query_string")); } public RestSamlInvalidateSessionAction(Settings settings, XPackLicenseState licenseState) {