Support handling LogoutResponse from SAML idP #56316

ywangd · 2020-05-07T02:34:38Z

SAML idP sends back a LogoutResponse at the end of the logout workflow. It can be sent via either HTTP-Redirect binding or HTTP-POST binding. Currently, the HTTP-Redirect request is simply ignored by Kibana and never reaches ES. It does not cause any obvious issue and the workflow is completed normally from user's perspective. For completeness, we have an open issue #40901 for handling it on the ES side.

The HTTP-POST request results in a 404 error because POST request is not accepted by Kibana's logout end-point. This causes a non-trivial issue because it renders an error page in user's browser. In addition, some resources do not seem to be fully cleaned up due to the error, e.g. the username will be pre-filled when trying to login again after the 404 error.

This PR tries to solve both of the above issues with a new _security/saml/complete_logout end-point. The function is largely a subset of the API that checks SAML authentication response.

NOTE: This PR is for elasticsearch only. To have complete end-to-end support for SAML logout response, changes need to be made for Kibana to handle (forward) the response message to elasticsearch.

Related to: #40901
Resolves: #43264

elasticmachine · 2020-05-07T02:34:40Z

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

ywangd · 2020-05-07T02:42:42Z

This PR is not entirely done yet. But I'd appreciate some feedback on the overall approach since this is my first saml related PR and also it involves quite a few refactor for code reuse. Thanks.

Still need:

Docs
More tests
API spec? (but we don't seem to have it other than for idP)
yaml tests (again don't seem to have it?)

Changes are also needed on Kibana side to really make the flow work. I could create an issue in Kibana repo (a related issue is elastic/kibana#27156)?

ywangd · 2020-05-07T03:03:20Z

...ity/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java

+            if (logoutResponse.isSigned()) {
+                validateSignature(logoutResponse.getSignature());
+            }
+            checkInResponseTo(logoutResponse, allowedSamlRequestIds);


This requires Kibana to know the _ID of the logout request crafted by ES and sent to the idP. The current SamlLogoutResponse does not give this information in a way that Kibana can easily access.

We could either change SamlLogoutResponse to include a separate field of _ID similar to what we do with SamlPrepareAuthenticationResponse. Or we could simply skip this check?

The spec (4.4.4.2) doesn't talk about validating the InResponseTo parameter, but I've seen other libraries / impementation do that. In all honesty SAML SLO is such a wild wild west, everyone does pretty much what they feel like :/

Given that it's not dictated by the specification and that our API has a binary response of "The Logout Response is fine" vs "the Logout Response is not fine" which cannot be abused somehow ( ie. open redirect etc) I think it would be fine to not validate but then again the implementation of doing it is already there so why not ? ( Kibana would need to make changes either way )

It's straightforward to add a new requestId field to the SamlLogoutResponse ES produces (via the /_security/saml/logout API). Given it is a change to an existing API, we may need to communicate to the Kibana team earlier on. @legrego Could you please comment on this? The requestId value is then required to be passed back to ES from Kibana to verify the LogoutResponse from the idP.

Given it is a change to an existing API, we may need to communicate to the Kibana team earlier on

In general, adding additional fields to our responses is not considered to be a breaking change so I believe Kibana will be fine with it even if we don't sync merging support for this.

The requestId value is then required to be passed back to ES from Kibana to verify the LogoutResponse from the idP.

Kibana does that already for the request ID of the authentication response and send this back to ES in the _security/saml/authenticate API so I believe it would be fine to do here too.

Thanks @jkakavas I'll make the change accordingly.

In general, adding additional fields to our responses is not considered to be a breaking change so I believe Kibana will be fine with it even if we don't sync merging support for this.

That's correct.

Kibana does that already for the request ID of the authentication response and send this back to ES in the _security/saml/authenticate API so I believe it would be fine to do here too.

The main difference here is that request ID returned from _security/saml/prepare is stored in the cookie. But during logout flow we used to clear cookie when user is redirected to IdP with LogoutRequest. Having said that, we can change that, we'll just need to carefully fix all the remaining places that still treat any cookie as a sign of authenticated user.

Since the specification doesn't explicitly call this out AND the outcome is in any case a no-op, I would be ok with not validating this. Especially since it sounds possible that the risk of introducing a bug on Kibana's side by changing this behavior is not minimal. On the other hand, treating "any cookie as a sign of authenticated user." is something worth fixing regardless of that change so if this is an extra driver for that change, let's do it !

jkakavas · 2020-05-07T06:26:01Z

...ity/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java

+            if (logoutResponse == null) {
+                throw samlException("Cannot convert element {} into LogoutResponse object", root);
+            }
+            if (logoutResponse.isSigned()) {


The response MUST be signed (4.4.4.2) so we should fail if it is not signed.

The spec says "Must authenticate itself either by signing or a binding specific mechansim". While the HTTP-POST LogoutResponse is signed, the LogoutResponse from HTTP-Redirect binding is not. So I would prefer it to be optional.

or a binding specific mechanism

this alludes to the artifact binding that is not used/supported here

. While the HTTP-POST LogoutResponse is signed, the LogoutResponse from HTTP-Redirect binding is not.

Why are you stating that as a fact ? It can very well be signed and according to the specification it MUST be.

Why are you stating that as a fact ?

It's not an universal fact. I was trying to avoid explicitly mentioning a popular idP's name. Sorry about the confusion. If we mandate signature verification and Kibana starts to redirect the response to ES, the verification will fail since it does not have signature.

In addition, including signature in HTTP-Redirect request may risk hitting the (practical) limit of URL length. I guess this is probably the reason why it is not signed by the aforementioned idP.

If we mandate signature verification and Kibana starts to redirect the response to ES, the verification will fail since it does not have signature.

It all boils down to how permissive we want to be with things that go against the specification. If a popular IDP doesn't adhere to the specification, do we happily consume what they are sending us or do we not ? And if another popular IDP comes along and doesn't adhere to the standards in a different way, do we happily accept that too? Where do we draw the line? I believe that drawing the line at what the specification says is easier to argue about than drawing the line in an arbitrary leniency we deem acceptable at some point.

In addition, including signature in HTTP-Redirect request may risk hitting the (practical) limit of URL length. I guess this is probably the reason why it is not signed by the aforementioned idP

I doubt that. SAML Logout Response messages are quite small ( comparable to authentication requests that also use the HTTP Redirect binding and are signed )

First of all, thanks for indulging me with the discussions. I really appreciate it. It helps me understanding the issue better ❤️

It is when it is configured to be

Thanks for pointing this out. I completely ignored this part when setting up saml locally. But isn't it actually an example that we do allow unsigned LogoutResponse (the difference being we are the sender in this case)?

SAML with authentication requests ( using the HTTP Redirect binding) being signed has been out there for 15 years now, this is heavily supported and used

This is a good point as well. Given authn request can be signed and use HTTP Redirect, I agree there will be no issue for LogoutResponse.

I don't think this is true.

You are right that it is not "optional". The word used in the spec 6.4.2 is "SHOULD". It is stronger than optional, but not MUST. Our implementation for this part treats it as optional:

elasticsearch/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlAuthenticator.java

Lines 90 to 92 in a443e13

if (response.isSigned()) {

validateSignature(response.getSignature());

requireSignedAssertions = false;

But isn't it actually an example that we do allow unsigned LogoutResponse (the difference being we are the sender in this case)?

I don't remember the discussions we had while implementing this , I can assume we made a decision that interoperability is more important than specification adherence at that time. We can revisit that decision too :)

You are right that it is not "optional". The word used in the spec 6.4.2 is "SHOULD".

We only support the web browser single sign on profile, so 4.1 in that spec, see 4.1.3.5

Our implementation for this part treats it as optional:

It -hopefully- doesn't!!! A SAML response contains SAML assertions. What we do support is either the response or the assertion is signed :) There must be an integrity protection implemented, otherwise it would be trivial for attackers to bypass authentication

We only support the web browser single sign on profile, so 4.1 in that spec, see 4.1.3.5

You are right. I read the wrong section 🤦

What we do support is either the response or the assertion is signed

Yes that's what I figured as well. There is no risk in this part.

I think the only thing that bothers me is this "popular idP". The original issue #43264 is created with it as an example of why we consider to support HTTP-POST LogoutResponse. So it feels rather unsatifying if we fix one binding and breaks the other (the current working one).

The code change itself either way is trivial. The decision making part is more important. I feel we could talk about this over sync time.

I think the only thing that bothers me is this "popular idP".

I don't think we need to make changes that satisfy any "popular IDP", we should make changes that make sense for the users and are correct. What's more, I see no indication whatsoever that any IDP doesn't support signed Logout Responses so I'm not sure what we would be actually breaking with this.

I think we should not be lenient here for no reason, but happy to discuss this more on the sync.

Sync update: we decided to conform to the spec and perform full validation. Caller of the API can decide what to do with validation errors if any. Thanks for the discussion.

ywangd · 2020-05-08T01:36:59Z

...core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponse.java

        redirectUrl = in.readString();
    }

-    public SamlLogoutResponse(String redirectUrl) {
+    public SamlLogoutResponse(String requestId, String redirectUrl) {
+        this.requestId = requestId;


I don't think we need handle BWC here since this response never goes across nodes. The only consumer is Kibana (or other external system that integrates with ES).

ywangd · 2020-05-11T05:15:40Z

Updated to ensure support of handling SAML LogoutResponse via both HTTP-Redirect and HTTP-Post bindings. The new /_security/saml/verify_logout API is used to handle both scenarios. In both cases, it takes a POST request with following JSON payload:

{
  "ids": ["..."],
  "realm": "saml1",
  "content": "..."
}

where ids is a list of request IDs that InResponseTo value should be verified against, and content is the raw URI query string of HTTP-Redirect binding or raw body of HTTP-Post binding.

The code examines the content to see whether an URL level signature is available. If it is found, HTTP-Redirect is assumed and verified accordingly. If no URL level signature is found, HTTP-Post is assumed, which mandates the signature at XML level.

jkakavas

Looking good Yang! I added some comments , a couple of these I believe we should address, the rest are suggestions that we can iterate on

.../src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutAction.java

jkakavas · 2020-05-15T09:28:08Z

...src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutRequest.java

+ */
+public final class SamlVerifyLogoutRequest extends ActionRequest {
+
+    private String content;


I would prefer if we have different fields for this depending on the binding

I was intentionally trying to make it the same to handle both bindings. The current approach is to have a single POST endpoint that Kibana can call for both HTTP-Redirect and HTTP-Post bindings from the idP, i.e. Kibana acts as a multiplexer here. So what ES receives is an uniform POST message. The handler (SamlLogoutResponseHandler) is able to tell whether the paylod (content) itself is signed and proceeds as HTTP-Redirect. If it is not, it proceeds as HTTP-POST and requires the XML to be signed, which is more or less similar to what SamlAuthenticator does.

I can be convinced to have two separate endpoints, one for HTTP-Redirect and one for HTTP-POST. If we do that, we can even have two separate Request classes as well. But if we choose to have a single endpoint, I'd prefer to also not differentiate it at the Request object level.

Whether we should have one or two end-points also depends on what Kibana can do (@azasypkin). My thinking is that Kibana will have to prepare the requset to ES anyhow, i.e. it has to add the realm and requestId information. So Kibana might as well just act as the multiplexer and prepare an uniform message.

After reading your comment below. I am convinced to have separate fields at request level, queryString and content for HTTP-Redirect and HTTP-POST, respectively. The names are chosen to be consistent with RestSamlInvalidateSessionAction and RestSamlAuthenticateAction.

The SamlCompleteLogoutRequest will validate that the two fields cannot both be null nor both be set. It also has convenient methods, getPayload and isHttpRedirect, so the information is clear at handler level. Thanks.

jkakavas · 2020-05-15T09:31:08Z

...src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutRequest.java

+    @Nullable
+    private String realm;
+    @Nullable
+    private String assertionConsumerServiceURL;


I don't think we need the assertionConsumerServiceURL in this case. IIUC ( @azasypkin can keep me honest ) , kibana's current versions can be made to always send the realm name in the request which is enough for us to get the realm by name

Correct, starting from 7.7 we store realm returned from the _security/saml/authenticate in the cookie/session and we'll keep it when store Logout Request ID there as well.

Thanks. Good to know that. Removed.

jkakavas · 2020-05-15T09:40:35Z

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

@@ -333,7 +277,13 @@ private void checkSubject(Subject assertionSubject, XMLObject parent, Collection
        }
        checkRecipient(confirmationData.get(0));
        checkLifetimeRestrictions(confirmationData.get(0));
-        checkInResponseTo(confirmationData.get(0), allowedSamlRequestIds);
+        SubjectConfirmationData subjectConfirmationData = confirmationData.get(0);


Leave this in a method as it was and just name it differently ? WDYT ?

Sure, named it checkSubjectInResponseTo. Let me know if you have a better name.

jkakavas · 2020-05-15T09:53:42Z

.../security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlResponseHandler.java

+
+import static org.elasticsearch.xpack.security.authc.saml.SamlUtils.samlException;
+
+public class SamlResponseHandler extends SamlObjectHandler {


I think I don't see the value in splitting up SamlObjectHandler and SamlResponseHandler. Can you elaborate ? It also doesn't help that all the objects that are responses for SAML , are passed in a request to elasticsearch and that makes naming hard but I'm equally happy with SamlResponseHandler and SamlRequestHandler

This is mainly because we do have a real Request object from idP, i.e.SamlInvalidateSessionRequest. Its associated handler is SamlLogoutRequestHandler, which is a subclass of SamlObjectHandler. In another word, SamlObjectHandler is the base class for both Request and Response handlers. So I did not merge SamlResponseHandler into SamlObjectHandler.

Also I did a few refactoring which is likely made the changes hard to track, here is a summary:

SamlObjectHandler was renamed from SamlRequestHandler (It seems to be a good idea to me since its the base for both Request and Response handlers. But I can revert it if it does not sound right to you).

SamlResponseHandler is extracted from SamlAuthenticator since many code can be reused by the new SamlLogoutResponseHandler

The class hierarchy is as follows:

SamlObjectHandler ├── SamlLogoutRequestHandler └── SamlResponseHandler ├── SamlAuthenticator └── SamlLogoutResponseHandler

jkakavas · 2020-05-15T09:58:27Z

...rity/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlResponseHandlerTests.java

+import static javax.xml.crypto.dsig.Transform.ENVELOPED;
+import static org.opensaml.saml.common.xml.SAMLConstants.SAML20_NS;
+
+public class SamlResponseHandlerTests extends SamlTestCase {


I assume these are carried over as is from SamlAuthenticatorTests so I'm fine with these, let me know if you've made any significant changes somehow

You are right. They are split with the IDE refactoring tool. So nothing should be really different.

jkakavas · 2020-05-15T10:07:59Z

.../org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpRedirectTests.java

+        final String url = new SamlRedirect(logoutResponse, signingConfiguration).getRedirectUrl();
+        final ElasticsearchSecurityException e =
+            expectSamlException(() -> samlLogoutResponseHandler.handle(new URI(url).getRawQuery(), List.of(requestId)));
+        assertThat(e.getMessage(), containsString("Failed to parse SAML message"));


related to the discussion above, this is a case where we don't want to fail to parse the SAML message because we will try to parse it as if it came from the HTTP POST binding. We want to fail this because it is an unsigned request that came via the HTTP-Redirect binding and we disallow that.

I think I see your point here. I'll move away from auto-detect and have two separate fields at request level.

jkakavas · 2020-05-15T10:08:39Z

...ity/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java

+    public void handle(String content, Collection<String> allowedSamlRequestIds) {
+        final ParsedQueryString parsed = parseQueryStringAndValidateSignature(content, "SAMLResponse");
+        final Element root;
+        if (parsed.hasSignature) {


I would very much prefer that we are explicit here and not try to deduce and imply checks. I think we need two parameters for the REST layer, one for the body of a response that comes via the HTTP-POST binding and a different one for the query string of a response that comes via the HTTP-Redirect binding. Then we can be very explicit here with regards to what we expect to have and what we need to process/check/validate

jkakavas · 2020-05-15T10:10:18Z

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

@@ -322,4 +331,74 @@ protected void validateNotOnOrAfter(DateTime notOnOrAfter) {
            throw samlException("Rejecting SAML assertion because [{}] is on/after [{}]", pastNow, notOnOrAfter);
        }
    }
+
+    protected ParsedQueryString parseQueryStringAndValidateSignature(String queryString, String samlMessageParameterName) {


Again, assuming that you didn't change much when you copied this over ( apart from introducing samlMessageParameterName ) , please do flag if otherwise ! ( diffs are not that helpful in spotting this kind of things :) )

Good spot! Indeed this is the only significant change as part of the refactoring. Next time I'll compile the list of changes before hand to save you some time. Sorry about that. Here is a list for rest of the changes (they are all very minor):

method visibility from private to protect due to superclass extraction.

This includes: SamlResponseHandler#isSuccess, SamlResponseHandler#getStatusCodeMessage, SamlObjectHandler#decodeBase64, SamlObjectHandler#inflate, and SamlObjectHandler#parseQueryStringAndValidateSignature.

Reverse the order of String equals compare in SamlResponseHandler#isSuccess.

Changed from status.getStatusCode().getValue().equals(StatusCode.SUCCESS) to StatusCode.SUCCESS.equals(status.getStatusCode().getValue()).

ywangd · 2020-05-18T02:41:23Z

Thanks a lot @jkakavas I have addressed your feedbacks.

There seem to be some inconsistency in how we throw exceptions for saml handling. The method SamlUtils#samlException(String msg, Object... args) is used most often. I also used them in the new handler. This method implies 500 for HTTP response status, which does not feel right in many places. I am aware there is an overloaded version of this method where you can pass another cause exception for other status like 400. I didn't use it mainly because it would be "inconsistent" with usages in many other places. I'd like to hear your opinions. Thanks

jkakavas

Added a few more comments

jkakavas · 2020-05-26T05:30:42Z

...ity/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java

+    public void handle(boolean httpRedirect, String payload, Collection<String> allowedSamlRequestIds) {
+        final ParsedQueryString parsed = parseQueryStringAndValidateSignature(payload, "SAMLResponse");
+        if (httpRedirect && parsed.hasSignature == false) {
+            throw samlException("URL is not signed, but is required for HTTP-Redirect binding");


It's not the URL that is signed, but rather the - encoded - SAML response.

You are right that it is not the entire URL that gets signed. But it is not just the encoded SAMLResponse either. It is rather a string taking the form of SAMLResponse=xxx&RelayState=xxx&SigAlg=xx. How about changing it to "Query string is not signed, but is required for HTTP-Redirect binding"?

The most important part here is for the user/administrator to realize what is wrong and why this failed. I think that "The SAML logout response is not signed" does that in a better way than "the URL is not signed" or "The query string is not signed" but I won't insist too much :)

...ity/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java

jkakavas · 2020-05-26T05:34:14Z

...ity/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java

+    }
+
+    public void handle(boolean httpRedirect, String payload, Collection<String> allowedSamlRequestIds) {
+        final ParsedQueryString parsed = parseQueryStringAndValidateSignature(payload, "SAMLResponse");


It reads strange that we "parse the query string" when payload might very well be the body of HTTP POST

Reading through the tests below, it looks like there is an implicit assumption that even when HTTP POST is used, we will receive SAMLResponse=<Base64Endoded saml logout response here> from Kibana and thus we could "parse" this as if it was a query string. I think we should be explicit and

a) Declare that we expect kibana(or any caller) to pass the value of the SAMLResponse POST parameter
b) Base64 decode this on the RestSamlLogoutAction
c) Operate on the raw bytes after that

similar to what we do for the SamlAuthenticateAction's parameter named content

The payload/data returned from a SAML idP with HTTP-Post binding uses application/x-www-form-urlencoded encoding, which takes the form of SAMLResponse=<base64>&RelayState=xxxx. So it can indeed be processed as if it is a query string.

But I think it is good point that we should keep this consistent with how SAMLReseponse is handled by SamlAuthenticationAction, for which I think Kibana performs the action of extracting the value of SAMLResponse parameter and passes it to ES so that we do not have to parse it (as query parameter) again. I will make the change and add a note in the code to clarify the expectation for Kibana.

...ity/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java

jkakavas · 2020-05-26T05:44:32Z

...ity/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java

+        final ParsedQueryString parsed = parseQueryStringAndValidateSignature(payload, "SAMLResponse");
+        if (httpRedirect && parsed.hasSignature == false) {
+            throw samlException("URL is not signed, but is required for HTTP-Redirect binding");
+        } else if (httpRedirect == false && parsed.hasSignature) {
+            throw samlException("URL is signed, but binding is HTTP-POST");
+        }
+
+        final Element root;
+        if (httpRedirect) {
+            logger.info("Process SAML LogoutResponse with HTTP-Redirect binding");
+            root = parseSamlMessage(inflate(decodeBase64(parsed.samlMessage)));
+        } else {
+            logger.info("Process SAML LogoutResponse with HTTP-POST binding");
+            root = parseSamlMessage(decodeBase64(parsed.samlMessage));
+        }


Given that I had 4 comments in 10 lines ( Woke up picky today, apologies 🙏 ) think we could do this is a simpler way, something like

final Element root; if (httpRedirect) { logger.debug("Process SAML LogoutResponse with HTTP-Redirect binding"); final ParsedQueryString parsed = parseQueryStringAndValidateSignature(payload, "SAMLResponse"); if (parsed.hasSignature == false){ throw samlException("SAML LogoutResponse messages must be signed"); } root = parseSamlMessage(inflate(decodeBase64(parsed.samlMessage))); } else { logger.info("Process SAML LogoutResponse with HTTP-POST binding"); root = parseSamlMessage(decodeBase64(payload)); }

...ity/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java

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

...c/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutRequest.java

.../org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpRedirectTests.java

…ecurity/authc/saml/SamlLogoutResponseHandler.java Co-authored-by: Ioannis Kakavas <[email protected]>

…ecurity/action/saml/TransportSamlCompleteLogoutAction.java Co-authored-by: Ioannis Kakavas <[email protected]>

…logout

ywangd · 2020-05-29T06:29:57Z

Thanks @jkakavas :)

The reason for samlException() is https://github.com/jkakavas/elasticsearch/blob/a7187ca208b4a7815d8be38c6214cecb610e3376/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlRealm.java#L423 . If we're using it in other places and this affects the response and status code of our APIs, this is probably wrong and we should fix this. I'm not entirely certain what we should be returning in each case but we can start an issue and gather all places we throw such an Exception and see how we can "fix' them across the SAML APIs ( in a followup )

The usage is indeed largely covered by the above exception check, which makes it sensible to use in SamlAuthenticator and SamlRealm. There are however many other places where it also gets used. I created #57331 to track potential improvement.

…logout

tvernum · 2020-06-01T01:37:37Z

...c/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutRequest.java

+            validationException = addValidationError("queryString 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);


What is our expectation for how a WebApp should behave if it gets a LogoutResponse over a HTTP-POST binding that also has URL parameters?

I think the rule for Kibana is:

If the http method is GET send the queryString to Elasticsearch, and ignore (or reject) any body (which is very unlikely to exist)

if the http method is POST ignore (or reject) the query parameters, and send the body to Elasticsearch.

Is that our intent?

A good question. To be honest, I haven't really considered this thoroughly. I think your suggestion makes sense because the code proceeds as HTTP-POST, e.g. base64 encode, no deflate and XML signature, when content is present. This basically says content implies POST, which in turn means queryString implies GET.

I would personally prefer to ignore the body for GET and query string for POST since the spec does not explicitly forbid them.

tvernum · 2020-06-01T01:50:51Z

...ain/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlCompleteLogoutAction.java

+
+    private static final Logger logger = LogManager.getLogger(RestSamlCompleteLogoutAction.class);
+
+    static class Input {


Why do we have another class here, rather than just using SamlCompleteLogoutRequest ?

Because of copy/paste ... removed it. thanks

…logout

ywangd · 2020-06-17T12:41:59Z

@tvernum just ping you so this PR remains on your radar. thanks

…logout

SAML idP sends back a LogoutResponse at the end of the logout workflow. It can be sent via either HTTP-Redirect binding or HTTP-POST binding. Currently, the HTTP-Redirect request is simply ignored by Kibana and never reaches ES. It does not cause any obvious issue and the workflow is completed normally from user's perspective. The HTTP-POST request results in a 404 error because POST request is not accepted by Kibana's logout end-point. This causes a non-trivial issue because it renders an error page in user's browser. In addition, some resources do not seem to be fully cleaned up due to the error, e.g. the username will be pre-filled when trying to login again after the 404 error. This PR solves both of the above issues from ES side with a new /_security/saml/complete_logout end-point. Changes are still needed on Kibana side to relay the messages.

This PR adds the documentation and Rest API spec file for the SAML complete logout API. It is a (overdued) follow up for #56316

This PR adds the documentation and Rest API spec file for the SAML complete logout API. It is a (overdued) follow up for elastic#56316

) This PR adds the documentation and Rest API spec file for the SAML complete logout API. It is a (overdued) follow up for #56316

ywangd added >enhancement :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) v8.0.0 v7.9.0 labels May 7, 2020

ywangd requested review from tvernum and jkakavas May 7, 2020 02:34

elasticmachine added the Team:Security Meta label for security team label May 7, 2020

ywangd commented May 7, 2020

View reviewed changes

jkakavas reviewed May 7, 2020

View reviewed changes

ywangd commented May 8, 2020

View reviewed changes

jkakavas self-requested a review May 15, 2020 09:17

jkakavas requested changes May 15, 2020

View reviewed changes

ywangd requested a review from jkakavas May 18, 2020 04:19

jkakavas requested changes May 26, 2020

View reviewed changes

ywangd added 12 commits May 27, 2020 16:53

Initial working version. No tests yet

a7187ca

Refactor and add tests

3180c86

fix copy/paste text

e6d4d5a

Add requestId to logout response for kibana to pass it back

3ef50a1

checkstyle

c5633e5

Support LogoutResponse with both HTTP-Redirect and HTTP-Post bindings

c1ea969

Rename and checkstyle

bcaf87d

Forbidden API

824b560

Address feedback

41494c4

Address feedback for removing auto-detect of redirect and post

c0e4089

Fix test convention

48c27d6

checkstyle

17903a3

ywangd and others added 5 commits May 29, 2020 15:28

Update x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/s…

a9b890a

…ecurity/authc/saml/SamlLogoutResponseHandler.java Co-authored-by: Ioannis Kakavas <[email protected]>

Update x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/s…

99704d2

…ecurity/action/saml/TransportSamlCompleteLogoutAction.java Co-authored-by: Ioannis Kakavas <[email protected]>

Address feedback

88862ab

Add one more test for non-null realm field

5128737

Merge remote-tracking branch 'origin/master' into es-43264-saml-post-…

19a0afc

…logout

Merge remote-tracking branch 'origin/master' into es-43264-saml-post-…

c244cb6

…logout

tvernum reviewed Jun 1, 2020

View reviewed changes

ywangd added 2 commits June 1, 2020 22:59

Address feedback

75abd7b

Merge remote-tracking branch 'origin/master' into es-43264-saml-post-…

b82275d

…logout

ywangd requested a review from tvernum June 1, 2020 13:03

azasypkin mentioned this pull request Jun 18, 2020

Properly handle Logout Response coming from SAML IdP. elastic/kibana#69506

Open

tvernum approved these changes Jun 24, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into es-43264-saml-post-…

f6b4fcb

…logout

ywangd merged commit e966155 into elastic:master Jun 29, 2020

ywangd added the backport pending label Jun 29, 2020

ywangd removed the backport pending label Sep 10, 2020

This was referenced May 9, 2021

Add REST API specification for SAML APIs #72839

Merged

Add docs and rest api spec for saml complete logout API #72867

Merged

ywangd added a commit that referenced this pull request May 31, 2021

Add docs and rest api spec for saml complete logout API (#72867)

da3d72c

This PR adds the documentation and Rest API spec file for the SAML complete logout API. It is a (overdued) follow up for #56316

ywangd mentioned this pull request May 31, 2021

Add docs and rest api spec for saml complete logout API (#72867) #73544

Merged

ywangd added a commit that referenced this pull request May 31, 2021

Add docs and rest api spec for saml complete logout API (#72867) (#73544

d225297

) This PR adds the documentation and Rest API spec file for the SAML complete logout API. It is a (overdued) follow up for #56316

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

This was referenced Feb 16, 2022

Correctly handle <LogoutResponse> messages for the SAML Realm #40901

Closed

Support HTTP-POST binding for SAML logout #43264

Open

	if (response.isSigned()) {
	validateSignature(response.getSignature());
	requireSignedAssertions = false;


		import static org.elasticsearch.xpack.security.authc.saml.SamlUtils.samlException;

		public class SamlResponseHandler extends SamlObjectHandler {


		private static final Logger logger = LogManager.getLogger(RestSamlCompleteLogoutAction.class);

		static class Input {

Support handling LogoutResponse from SAML idP #56316

Support handling LogoutResponse from SAML idP #56316

Conversation

ywangd commented May 7, 2020 • edited Loading

elasticmachine commented May 7, 2020

ywangd commented May 7, 2020 • edited Loading

Choose a reason for hiding this comment

jkakavas May 7, 2020 • 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

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

ywangd May 7, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywangd commented May 11, 2020 • edited Loading

jkakavas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywangd May 18, 2020 • 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

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

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

ywangd commented May 18, 2020

jkakavas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywangd May 27, 2020 • 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

Choose a reason for hiding this comment

ywangd commented May 29, 2020

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

ywangd commented Jun 17, 2020

ywangd commented May 7, 2020 •

edited

Loading

ywangd commented May 7, 2020 •

edited

Loading

jkakavas May 7, 2020 •

edited

Loading

ywangd May 7, 2020 •

edited

Loading

ywangd commented May 11, 2020 •

edited

Loading

ywangd May 18, 2020 •

edited

Loading

ywangd May 27, 2020 •

edited

Loading