-
Notifications
You must be signed in to change notification settings - Fork 24.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support handling LogoutResponse from SAML idP #56316
Changes from all commits
a7187ca
3180c86
e6d4d5a
3ef50a1
c5633e5
c1ea969
bcaf87d
824b560
41494c4
c0e4089
48c27d6
17903a3
e297fa5
c22674d
a56bbcc
f2ed7df
04abf77
2b85c07
a9b890a
99704d2
88862ab
5128737
19a0afc
c244cb6
75abd7b
b82275d
f6b4fcb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
package org.elasticsearch.xpack.core.security.action.saml; | ||
|
||
import org.elasticsearch.action.ActionType; | ||
|
||
/** | ||
* ActionType for completing SAML LogoutResponse | ||
*/ | ||
public final class SamlCompleteLogoutAction extends ActionType<SamlCompleteLogoutResponse> { | ||
|
||
public static final String NAME = "cluster:admin/xpack/security/saml/complete_logout"; | ||
public static final SamlCompleteLogoutAction INSTANCE = new SamlCompleteLogoutAction(); | ||
|
||
private SamlCompleteLogoutAction() { | ||
super(NAME, SamlCompleteLogoutResponse::new); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,92 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
package org.elasticsearch.xpack.core.security.action.saml; | ||
|
||
import org.elasticsearch.action.ActionRequest; | ||
import org.elasticsearch.action.ActionRequestValidationException; | ||
import org.elasticsearch.common.Nullable; | ||
import org.elasticsearch.common.Strings; | ||
import org.elasticsearch.common.io.stream.StreamInput; | ||
|
||
import java.io.IOException; | ||
import java.util.List; | ||
|
||
import static org.elasticsearch.action.ValidateActions.addValidationError; | ||
|
||
/** | ||
* Represents a request to complete SAML LogoutResponse | ||
*/ | ||
public final class SamlCompleteLogoutRequest extends ActionRequest { | ||
|
||
@Nullable | ||
private String queryString; | ||
@Nullable | ||
private String content; | ||
private List<String> validRequestIds; | ||
private String realm; | ||
|
||
public SamlCompleteLogoutRequest(StreamInput in) throws IOException { | ||
super(in); | ||
} | ||
|
||
public SamlCompleteLogoutRequest() { | ||
} | ||
|
||
@Override | ||
public ActionRequestValidationException validate() { | ||
ActionRequestValidationException validationException = null; | ||
if (Strings.hasText(realm) == false) { | ||
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); | ||
} | ||
if (Strings.hasText(queryString) && Strings.hasText(content)) { | ||
validationException = addValidationError("queryString and content may not both present", validationException); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
Is that our intent? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 I would personally prefer to ignore the body for |
||
} | ||
return validationException; | ||
} | ||
|
||
public String getQueryString() { | ||
return queryString; | ||
} | ||
|
||
public void setQueryString(String queryString) { | ||
this.queryString = queryString; | ||
} | ||
|
||
public String getContent() { | ||
return content; | ||
} | ||
|
||
public void setContent(String content) { | ||
this.content = content; | ||
} | ||
|
||
public List<String> getValidRequestIds() { | ||
return validRequestIds; | ||
} | ||
|
||
public void setValidRequestIds(List<String> validRequestIds) { | ||
this.validRequestIds = validRequestIds; | ||
} | ||
|
||
public String getRealm() { | ||
return realm; | ||
} | ||
|
||
public void setRealm(String realm) { | ||
this.realm = realm; | ||
} | ||
|
||
public boolean isHttpRedirect() { | ||
return queryString != null; | ||
} | ||
|
||
public String getPayload() { | ||
return isHttpRedirect() ? queryString : content; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
package org.elasticsearch.xpack.core.security.action.saml; | ||
|
||
import org.elasticsearch.action.ActionResponse; | ||
import org.elasticsearch.common.io.stream.StreamInput; | ||
import org.elasticsearch.common.io.stream.StreamOutput; | ||
|
||
import java.io.IOException; | ||
|
||
/** | ||
* A response to complete the LogoutResponse from idP | ||
*/ | ||
public final class SamlCompleteLogoutResponse extends ActionResponse { | ||
|
||
public SamlCompleteLogoutResponse(StreamInput in) throws IOException { | ||
super(in); | ||
} | ||
|
||
public SamlCompleteLogoutResponse() { | ||
} | ||
|
||
@Override | ||
public void writeTo(StreamOutput out) throws IOException { | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,24 +16,32 @@ | |
*/ | ||
public final class SamlLogoutResponse extends ActionResponse { | ||
|
||
private String redirectUrl; | ||
private final String requestId; | ||
private final String redirectUrl; | ||
|
||
public SamlLogoutResponse(StreamInput in) throws IOException { | ||
super(in); | ||
requestId = in.readString(); | ||
redirectUrl = in.readString(); | ||
} | ||
|
||
public SamlLogoutResponse(String redirectUrl) { | ||
public SamlLogoutResponse(String requestId, String redirectUrl) { | ||
this.requestId = requestId; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). |
||
this.redirectUrl = redirectUrl; | ||
} | ||
|
||
public String getRequestId() { | ||
return requestId; | ||
} | ||
|
||
public String getRedirectUrl() { | ||
return redirectUrl; | ||
} | ||
|
||
@Override | ||
public void writeTo(StreamOutput out) throws IOException { | ||
out.writeString(requestId); | ||
out.writeString(redirectUrl); | ||
} | ||
|
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,4 +48,4 @@ public void writeTo(StreamOutput out) throws IOException { | |
out.writeString(redirectUrl); | ||
} | ||
|
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
package org.elasticsearch.xpack.core.security.action.saml; | ||
|
||
import org.elasticsearch.action.ActionRequestValidationException; | ||
import org.elasticsearch.test.ESTestCase; | ||
|
||
import static org.hamcrest.Matchers.containsString; | ||
|
||
public class SamlCompleteLogoutRequestTests extends ESTestCase { | ||
|
||
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")); | ||
} | ||
|
||
public void testValidateFailsWhenQueryAndBodyBothSet() { | ||
final SamlCompleteLogoutRequest samlCompleteLogoutRequest = new SamlCompleteLogoutRequest(); | ||
samlCompleteLogoutRequest.setRealm("realm"); | ||
samlCompleteLogoutRequest.setQueryString("queryString"); | ||
samlCompleteLogoutRequest.setContent("content"); | ||
final ActionRequestValidationException validationException = samlCompleteLogoutRequest.validate(); | ||
assertThat(validationException.getMessage(), containsString("queryString and content may not both present")); | ||
} | ||
|
||
public void testValidateFailsWhenRealmIsNotSet() { | ||
final SamlCompleteLogoutRequest samlCompleteLogoutRequest = new SamlCompleteLogoutRequest(); | ||
samlCompleteLogoutRequest.setQueryString("queryString"); | ||
final ActionRequestValidationException validationException = samlCompleteLogoutRequest.validate(); | ||
assertThat(validationException.getMessage(), containsString("realm may not be empty")); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
package org.elasticsearch.xpack.security.action.saml; | ||
|
||
import org.elasticsearch.action.ActionListener; | ||
import org.elasticsearch.action.support.ActionFilters; | ||
import org.elasticsearch.action.support.HandledTransportAction; | ||
import org.elasticsearch.common.inject.Inject; | ||
import org.elasticsearch.tasks.Task; | ||
import org.elasticsearch.transport.TransportService; | ||
import org.elasticsearch.xpack.core.security.action.saml.SamlCompleteLogoutAction; | ||
import org.elasticsearch.xpack.core.security.action.saml.SamlCompleteLogoutRequest; | ||
import org.elasticsearch.xpack.core.security.action.saml.SamlCompleteLogoutResponse; | ||
import org.elasticsearch.xpack.security.authc.Realms; | ||
import org.elasticsearch.xpack.security.authc.saml.SamlLogoutResponseHandler; | ||
import org.elasticsearch.xpack.security.authc.saml.SamlRealm; | ||
import org.elasticsearch.xpack.security.authc.saml.SamlUtils; | ||
|
||
import java.util.List; | ||
|
||
import static org.elasticsearch.xpack.security.authc.saml.SamlRealm.findSamlRealms; | ||
|
||
/** | ||
* Transport action responsible for completing SAML LogoutResponse | ||
*/ | ||
public final class TransportSamlCompleteLogoutAction extends HandledTransportAction<SamlCompleteLogoutRequest, SamlCompleteLogoutResponse> { | ||
|
||
private final Realms realms; | ||
|
||
@Inject | ||
public TransportSamlCompleteLogoutAction(TransportService transportService, ActionFilters actionFilters, Realms realms) { | ||
super(SamlCompleteLogoutAction.NAME, transportService, actionFilters, SamlCompleteLogoutRequest::new); | ||
this.realms = realms; | ||
} | ||
|
||
@Override | ||
protected void doExecute(Task task, SamlCompleteLogoutRequest request, ActionListener<SamlCompleteLogoutResponse> listener) { | ||
List<SamlRealm> realms = findSamlRealms(this.realms, request.getRealm(), null); | ||
if (realms.isEmpty()) { | ||
listener.onFailure(SamlUtils.samlException("Cannot find any matching realm with name [{}]", request.getRealm())); | ||
} else if (realms.size() > 1) { | ||
listener.onFailure(SamlUtils.samlException("Found multiple matching realms [{}] with name [{}]", realms, request.getRealm())); | ||
} else { | ||
processLogoutResponse(realms.get(0), request, listener); | ||
} | ||
} | ||
|
||
private void processLogoutResponse(SamlRealm samlRealm, SamlCompleteLogoutRequest request, | ||
ActionListener<SamlCompleteLogoutResponse> listener) { | ||
|
||
final SamlLogoutResponseHandler logoutResponseHandler = samlRealm.getLogoutResponseHandler(); | ||
try { | ||
logoutResponseHandler.handle(request.isHttpRedirect(), request.getPayload(), request.getValidRequestIds()); | ||
listener.onResponse(new SamlCompleteLogoutResponse()); | ||
} catch (Exception e) { | ||
listener.onFailure(e); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we also validate that the realm is not an empty string here so that we fail explicitly here instead of in the Transport action's
doExecute
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added. Also removed the
@Nullable
annotation from therealm
field.