From a7187ca208b4a7815d8be38c6214cecb610e3376 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Wed, 6 May 2020 22:42:06 +1000 Subject: [PATCH 01/22] Initial working version. No tests yet --- .../action/saml/SamlLogoutResponseAction.java | 21 ++++ .../saml/SamlLogoutResponseRequest.java | 71 +++++++++++ .../SamlLogoutResponseRequestBuilder.java | 37 ++++++ .../saml/SamlLogoutResponseResponse.java | 29 +++++ .../xpack/security/Security.java | 5 + .../TransportSamlLogoutResponseAction.java | 61 +++++++++ .../authc/saml/SamlLogoutResponseHandler.java | 118 ++++++++++++++++++ .../xpack/security/authc/saml/SamlRealm.java | 21 +++- .../saml/RestSamlLogoutResponseAction.java | 112 +++++++++++++++++ .../authc/saml/SamlRealmTestHelper.java | 3 +- .../security/authc/saml/SamlRealmTests.java | 2 +- 11 files changed, 475 insertions(+), 5 deletions(-) create mode 100644 x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponseAction.java create mode 100644 x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponseRequest.java create mode 100644 x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponseRequestBuilder.java create mode 100644 x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponseResponse.java create mode 100644 x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlLogoutResponseAction.java create mode 100644 x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java create mode 100644 x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlLogoutResponseAction.java diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponseAction.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponseAction.java new file mode 100644 index 0000000000000..8dea3ddd6f80c --- /dev/null +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponseAction.java @@ -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 authenticating using SAML assertions + */ +public final class SamlLogoutResponseAction extends ActionType { + + public static final String NAME = "cluster:admin/xpack/security/saml/logout_response"; + public static final SamlLogoutResponseAction INSTANCE = new SamlLogoutResponseAction(); + + private SamlLogoutResponseAction() { + super(NAME, SamlLogoutResponseResponse::new); + } +} diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponseRequest.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponseRequest.java new file mode 100644 index 0000000000000..d8299223e6d8c --- /dev/null +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponseRequest.java @@ -0,0 +1,71 @@ +/* + * 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.io.stream.StreamInput; + +import java.io.IOException; +import java.util.List; + +/** + * Represents a request to handle LogoutResponse using SAML assertions. + */ +public final class SamlLogoutResponseRequest extends ActionRequest { + + private byte[] saml; + private List validRequestIds; + @Nullable + private String realm; + @Nullable + private String assertionConsumerServiceURL; + + public SamlLogoutResponseRequest(StreamInput in) throws IOException { + super(in); + } + + public SamlLogoutResponseRequest() { + } + + @Override + public ActionRequestValidationException validate() { + return null; + } + + public byte[] getSaml() { + return saml; + } + + public void setSaml(byte[] saml) { + this.saml = saml; + } + + public List getValidRequestIds() { + return validRequestIds; + } + + public void setValidRequestIds(List validRequestIds) { + this.validRequestIds = validRequestIds; + } + + public String getRealm() { + return realm; + } + + public void setRealm(String realm) { + this.realm = realm; + } + + public String getAssertionConsumerServiceURL() { + return assertionConsumerServiceURL; + } + + public void setAssertionConsumerServiceURL(String assertionConsumerServiceURL) { + this.assertionConsumerServiceURL = assertionConsumerServiceURL; + } +} diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponseRequestBuilder.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponseRequestBuilder.java new file mode 100644 index 0000000000000..65fcc105f97bc --- /dev/null +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponseRequestBuilder.java @@ -0,0 +1,37 @@ +/* + * 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.ActionRequestBuilder; +import org.elasticsearch.client.ElasticsearchClient; + +import java.util.List; + +/** + * Request builder used to populate a {@link SamlLogoutResponseRequest} + */ +public final class SamlLogoutResponseRequestBuilder + extends ActionRequestBuilder { + + public SamlLogoutResponseRequestBuilder(ElasticsearchClient client) { + super(client, SamlLogoutResponseAction.INSTANCE, new SamlLogoutResponseRequest()); + } + + public SamlLogoutResponseRequestBuilder saml(byte[] saml) { + request.setSaml(saml); + return this; + } + + public SamlLogoutResponseRequestBuilder validRequestIds(List validRequestIds) { + request.setValidRequestIds(validRequestIds); + return this; + } + + public SamlLogoutResponseRequestBuilder authenticatingRealm(String realm) { + request.setRealm(realm); + return this; + } +} diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponseResponse.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponseResponse.java new file mode 100644 index 0000000000000..33ca399f51d50 --- /dev/null +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponseResponse.java @@ -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; + +/** + * The response to the LogoutResponse from idP + */ +public final class SamlLogoutResponseResponse extends ActionResponse { + + public SamlLogoutResponseResponse(StreamInput in) throws IOException { + super(in); + } + + public SamlLogoutResponseResponse() { + } + + @Override + public void writeTo(StreamOutput out) throws IOException { + } +} diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java index 1a601df5cbb38..8279a32284e3f 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java @@ -104,6 +104,7 @@ import org.elasticsearch.xpack.core.security.action.saml.SamlAuthenticateAction; import org.elasticsearch.xpack.core.security.action.saml.SamlInvalidateSessionAction; import org.elasticsearch.xpack.core.security.action.saml.SamlLogoutAction; +import org.elasticsearch.xpack.core.security.action.saml.SamlLogoutResponseAction; import org.elasticsearch.xpack.core.security.action.saml.SamlPrepareAuthenticationAction; import org.elasticsearch.xpack.core.security.action.token.CreateTokenAction; import org.elasticsearch.xpack.core.security.action.token.InvalidateTokenAction; @@ -167,6 +168,7 @@ import org.elasticsearch.xpack.security.action.saml.TransportSamlAuthenticateAction; import org.elasticsearch.xpack.security.action.saml.TransportSamlInvalidateSessionAction; import org.elasticsearch.xpack.security.action.saml.TransportSamlLogoutAction; +import org.elasticsearch.xpack.security.action.saml.TransportSamlLogoutResponseAction; import org.elasticsearch.xpack.security.action.saml.TransportSamlPrepareAuthenticationAction; import org.elasticsearch.xpack.security.action.token.TransportCreateTokenAction; import org.elasticsearch.xpack.security.action.token.TransportInvalidateTokenAction; @@ -233,6 +235,7 @@ import org.elasticsearch.xpack.security.rest.action.saml.RestSamlAuthenticateAction; import org.elasticsearch.xpack.security.rest.action.saml.RestSamlInvalidateSessionAction; import org.elasticsearch.xpack.security.rest.action.saml.RestSamlLogoutAction; +import org.elasticsearch.xpack.security.rest.action.saml.RestSamlLogoutResponseAction; import org.elasticsearch.xpack.security.rest.action.saml.RestSamlPrepareAuthenticationAction; import org.elasticsearch.xpack.security.rest.action.user.RestChangePasswordAction; import org.elasticsearch.xpack.security.rest.action.user.RestDeleteUserAction; @@ -751,6 +754,7 @@ public void onIndexModule(IndexModule module) { new ActionHandler<>(SamlAuthenticateAction.INSTANCE, TransportSamlAuthenticateAction.class), new ActionHandler<>(SamlLogoutAction.INSTANCE, TransportSamlLogoutAction.class), new ActionHandler<>(SamlInvalidateSessionAction.INSTANCE, TransportSamlInvalidateSessionAction.class), + new ActionHandler<>(SamlLogoutResponseAction.INSTANCE, TransportSamlLogoutResponseAction.class), new ActionHandler<>(OpenIdConnectPrepareAuthenticationAction.INSTANCE, TransportOpenIdConnectPrepareAuthenticationAction.class), new ActionHandler<>(OpenIdConnectAuthenticateAction.INSTANCE, TransportOpenIdConnectAuthenticateAction.class), @@ -808,6 +812,7 @@ public List getRestHandlers(Settings settings, RestController restC new RestSamlAuthenticateAction(settings, getLicenseState()), new RestSamlLogoutAction(settings, getLicenseState()), new RestSamlInvalidateSessionAction(settings, getLicenseState()), + new RestSamlLogoutResponseAction(settings, getLicenseState()), new RestOpenIdConnectPrepareAuthenticationAction(settings, getLicenseState()), new RestOpenIdConnectAuthenticateAction(settings, getLicenseState()), new RestOpenIdConnectLogoutAction(settings, getLicenseState()), diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlLogoutResponseAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlLogoutResponseAction.java new file mode 100644 index 0000000000000..7e3483f6d6de5 --- /dev/null +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlLogoutResponseAction.java @@ -0,0 +1,61 @@ +/* + * 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.SamlLogoutResponseAction; +import org.elasticsearch.xpack.core.security.action.saml.SamlLogoutResponseRequest; +import org.elasticsearch.xpack.core.security.action.saml.SamlLogoutResponseResponse; +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 taking saml content and turning it into a token. + */ +public final class TransportSamlLogoutResponseAction extends HandledTransportAction { + + private final Realms realms; + + @Inject + public TransportSamlLogoutResponseAction(TransportService transportService, ActionFilters actionFilters, Realms realms) { + super(SamlLogoutResponseAction.NAME, transportService, actionFilters, SamlLogoutResponseRequest::new); + this.realms = realms; + } + + @Override + protected void doExecute(Task task, SamlLogoutResponseRequest request, ActionListener listener) { + List realms = findSamlRealms(this.realms, request.getRealm(), request.getAssertionConsumerServiceURL()); + if (realms.isEmpty()) { + listener.onFailure(SamlUtils.samlException("Cannot find any matching realm for [{}]", request)); + } else if (realms.size() > 1) { + listener.onFailure(SamlUtils.samlException("Found multiple matching realms [{}] for [{}]", realms, request)); + } else { + processLogoutResponse(realms.get(0), request, listener); + } + } + + private void processLogoutResponse(SamlRealm samlRealm, SamlLogoutResponseRequest request, + ActionListener listener) { + + final SamlLogoutResponseHandler logoutResponseHandler = samlRealm.getLogoutResponseHandler(); + try { + listener.onResponse(logoutResponseHandler.buildResponse(request.getSaml(), request.getValidRequestIds())); + } catch (Exception e) { + listener.onFailure(e); + } + } +} diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java new file mode 100644 index 0000000000000..191b5a8f22471 --- /dev/null +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java @@ -0,0 +1,118 @@ +/* + * 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.authc.saml; + +import org.elasticsearch.common.Strings; +import org.elasticsearch.common.unit.TimeValue; +import org.elasticsearch.xpack.core.security.action.saml.SamlLogoutResponseResponse; +import org.opensaml.saml.saml2.core.LogoutResponse; +import org.opensaml.saml.saml2.core.Status; +import org.opensaml.saml.saml2.core.StatusCode; +import org.opensaml.saml.saml2.core.StatusDetail; +import org.opensaml.saml.saml2.core.StatusMessage; +import org.opensaml.saml.saml2.core.StatusResponseType; +import org.w3c.dom.Element; + +import java.time.Clock; +import java.util.Collection; + +import static org.elasticsearch.xpack.security.authc.saml.SamlUtils.samlException; + +public class SamlLogoutResponseHandler extends SamlRequestHandler { + + private static final String LOGOUT_RESPONSE_TAG_NAME = "LogoutResponse"; + + public SamlLogoutResponseHandler( + Clock clock, IdpConfiguration idp, SpConfiguration sp, TimeValue maxSkew) { + super(clock, idp, sp, maxSkew); + } + + public SamlLogoutResponseResponse buildResponse(byte[] payload, Collection allowedSamlRequestIds) { + final Element root = parseSamlMessage(payload); + if (LOGOUT_RESPONSE_TAG_NAME.equals(root.getLocalName()) && SAML_NAMESPACE.equals(root.getNamespaceURI())) { + final LogoutResponse logoutResponse = buildXmlObject(root, LogoutResponse.class); + if (logoutResponse == null) { + throw samlException("Cannot convert element {} into LogoutResponse object", root); + } + if (logoutResponse.isSigned()) { + validateSignature(logoutResponse.getSignature()); + } + + checkInResponseTo(logoutResponse, allowedSamlRequestIds); + + final Status status = logoutResponse.getStatus(); + if (status == null || status.getStatusCode() == null) { + throw samlException("SAML Response has no status code"); + } + if (StatusCode.SUCCESS.equals(status.getStatusCode().getValue()) == false) { + throw samlException("SAML Response is not a 'success' response: {}", getStatusCodeMessage(status)); + } + + checkIssuer(logoutResponse.getIssuer(), logoutResponse); + checkResponseDestination(logoutResponse, getSpConfiguration().getLogoutUrl()); + return new SamlLogoutResponseResponse(); + } else { + throw samlException("SAML content [{}] should have a root element of Namespace=[{}] Tag=[{}]", + root, SAML_NAMESPACE, LOGOUT_RESPONSE_TAG_NAME); + } + + } + + private void checkInResponseTo(StatusResponseType response, Collection allowedSamlRequestIds) { + 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()); + throw samlException("SAML content is in-response-to [{}] but expected one of {} ", + response.getInResponseTo(), allowedSamlRequestIds); + } + } + + 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 void checkResponseDestination(StatusResponseType response, String spConfiguredUrl) { + if (spConfiguredUrl.equals(response.getDestination()) == false) { + if (response.isSigned() || Strings.hasText(response.getDestination())) { + throw samlException("SAML response " + response.getID() + " is for destination " + response.getDestination() + + " but this realm uses " + spConfiguredUrl); + } + } + } + + private String getMessage(Status status) { + final StatusMessage sm = status.getStatusMessage(); + return sm == null ? null : sm.getMessage(); + } + + private String getDetail(Status status) { + final StatusDetail sd = status.getStatusDetail(); + return sd == null ? null : SamlUtils.toString(sd.getDOM()); + } +} diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlRealm.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlRealm.java index 5969bb9aeecd0..e5e5e9edde663 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlRealm.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlRealm.java @@ -152,6 +152,7 @@ public final class SamlRealm extends Realm implements Releasable { private final SamlLogoutRequestHandler logoutHandler; private final UserRoleMapper roleMapper; + private final SamlLogoutResponseHandler logoutResponseHandler; private final Supplier idpDescriptor; private final SpConfiguration serviceProvider; @@ -195,8 +196,11 @@ public static SamlRealm create(RealmConfig config, SSLService sslService, Resour final SamlAuthenticator authenticator = new SamlAuthenticator(clock, idpConfiguration, serviceProvider, maxSkew); final SamlLogoutRequestHandler logoutHandler = new SamlLogoutRequestHandler(clock, idpConfiguration, serviceProvider, maxSkew); + final SamlLogoutResponseHandler logoutResponseHandler = + new SamlLogoutResponseHandler(clock, idpConfiguration, serviceProvider, maxSkew); - final SamlRealm realm = new SamlRealm(config, roleMapper, authenticator, logoutHandler, idpDescriptor, serviceProvider); + final SamlRealm realm = new SamlRealm(config, roleMapper, authenticator, logoutHandler, + logoutResponseHandler, idpDescriptor, serviceProvider); // the metadata resolver needs to be destroyed since it runs a timer task in the background and destroying stops it! realm.releasables.add(() -> metadataResolver.destroy()); @@ -205,13 +209,20 @@ public static SamlRealm create(RealmConfig config, SSLService sslService, Resour } // For testing - SamlRealm(RealmConfig config, UserRoleMapper roleMapper, SamlAuthenticator authenticator, SamlLogoutRequestHandler logoutHandler, - Supplier idpDescriptor, SpConfiguration spConfiguration) throws Exception { + SamlRealm( + RealmConfig config, + UserRoleMapper roleMapper, + SamlAuthenticator authenticator, + SamlLogoutRequestHandler logoutHandler, + SamlLogoutResponseHandler logoutResponseHandler, + Supplier idpDescriptor, + SpConfiguration spConfiguration) throws Exception { super(config); this.roleMapper = roleMapper; this.authenticator = authenticator; this.logoutHandler = logoutHandler; + this.logoutResponseHandler = logoutResponseHandler; this.idpDescriptor = idpDescriptor; this.serviceProvider = spConfiguration; @@ -701,6 +712,10 @@ public SamlLogoutRequestHandler getLogoutHandler() { return this.logoutHandler; } + public SamlLogoutResponseHandler getLogoutResponseHandler() { + return logoutResponseHandler; + } + private static class FileListener implements FileChangesListener { private final Logger logger; diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlLogoutResponseAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlLogoutResponseAction.java new file mode 100644 index 0000000000000..82861f387d90c --- /dev/null +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlLogoutResponseAction.java @@ -0,0 +1,112 @@ +/* + * 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.rest.action.saml; + +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.elasticsearch.client.node.NodeClient; +import org.elasticsearch.common.ParseField; +import org.elasticsearch.common.Strings; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.xcontent.ObjectParser; +import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.license.XPackLicenseState; +import org.elasticsearch.rest.BytesRestResponse; +import org.elasticsearch.rest.RestRequest; +import org.elasticsearch.rest.RestResponse; +import org.elasticsearch.rest.RestStatus; +import org.elasticsearch.rest.action.RestBuilderListener; +import org.elasticsearch.xpack.core.security.action.saml.SamlLogoutResponseRequestBuilder; +import org.elasticsearch.xpack.core.security.action.saml.SamlLogoutResponseResponse; + +import java.io.IOException; +import java.util.Base64; +import java.util.List; + +import static org.elasticsearch.rest.RestRequest.Method.POST; + +public class RestSamlLogoutResponseAction extends SamlBaseRestHandler{ + + private static final Logger logger = LogManager.getLogger(RestSamlLogoutResponseAction.class); + + static class Input { + String content; + List ids; + String realm; + String assertionConsumerServiceURL; + + void setContent(String content) { + this.content = content; + } + + void setIds(List ids) { + this.ids = ids; + } + + void setRealm(String realm) { this.realm = realm;} + + void setAssertionConsumerServiceURL(String assertionConsumerServiceURL) { + this.assertionConsumerServiceURL = assertionConsumerServiceURL; + } + } + + static final ObjectParser + PARSER = new ObjectParser<>("saml_logout_response", RestSamlLogoutResponseAction.Input::new); + + static { + PARSER.declareString(RestSamlLogoutResponseAction.Input::setContent, new ParseField("content")); + PARSER.declareStringArray(RestSamlLogoutResponseAction.Input::setIds, new ParseField("ids")); + PARSER.declareStringOrNull(RestSamlLogoutResponseAction.Input::setRealm, new ParseField("realm")); + PARSER.declareString(RestSamlLogoutResponseAction.Input::setAssertionConsumerServiceURL, new ParseField("acs")); + } + + public RestSamlLogoutResponseAction(Settings settings, XPackLicenseState licenseState) { + super(settings, licenseState); + } + + @Override + public String getName() { + return "security_saml_logout_response_action"; + } + + @Override + public List routes() { + return List.of(new Route(POST, "/_security/saml/logout_response")); + } + + @Override + protected RestChannelConsumer innerPrepareRequest(RestRequest request, NodeClient client) throws IOException { + try (XContentParser parser = request.contentParser()) { + final RestSamlLogoutResponseAction.Input input = PARSER.parse(parser, null); + logger.trace("SAML LogoutResponse: [{}...] [{}]", Strings.cleanTruncate(input.content, 128), input.ids); + return channel -> { + final byte[] bytes = decodeBase64(input.content); + final SamlLogoutResponseRequestBuilder requestBuilder = + new SamlLogoutResponseRequestBuilder(client).saml(bytes).validRequestIds(input.ids).authenticatingRealm(input.realm); + requestBuilder.execute(new RestBuilderListener<>(channel) { + @Override + public RestResponse buildResponse(SamlLogoutResponseResponse response, XContentBuilder builder) throws Exception { + builder.startObject() + .endObject(); + return new BytesRestResponse(RestStatus.OK, builder); + } + }); + }; + } + } + + private byte[] decodeBase64(String content) { + content = content.replaceAll("\\s+", ""); + try { + return Base64.getDecoder().decode(content); + } catch (IllegalArgumentException e) { + logger.info("Failed to decode base64 string [{}] - {}", content, e.toString()); + throw e; + } + } +} diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlRealmTestHelper.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlRealmTestHelper.java index c030e96f0900b..e6c7dc373f18e 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlRealmTestHelper.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlRealmTestHelper.java @@ -43,7 +43,8 @@ public static SamlRealm buildRealm(RealmConfig realmConfig, @Nullable X509Creden final SpConfiguration spConfiguration = new SpConfiguration(SP_ENTITY_ID, SP_ACS_URL, SP_LOGOUT_URL, new SigningConfiguration(Collections.singleton("*"), credential), Arrays.asList(credential), Collections.emptyList()); return new SamlRealm(realmConfig, mock(UserRoleMapper.class), mock(SamlAuthenticator.class), - mock(SamlLogoutRequestHandler.class), () -> idpDescriptor, spConfiguration); + mock(SamlLogoutRequestHandler.class), mock(SamlLogoutResponseHandler.class), + () -> idpDescriptor, spConfiguration); } public static void writeIdpMetadata(Path path, String idpEntityId) throws IOException { diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlRealmTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlRealmTests.java index fa538c8b3187e..6493f78d4be32 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlRealmTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlRealmTests.java @@ -309,7 +309,7 @@ private void initializeRealms(Realm... realms) { public SamlRealm buildRealm(RealmConfig config, UserRoleMapper roleMapper, SamlAuthenticator authenticator, SamlLogoutRequestHandler logoutHandler, EntityDescriptor idp, SpConfiguration sp) throws Exception { try { - return new SamlRealm(config, roleMapper, authenticator, logoutHandler, () -> idp, sp); + return new SamlRealm(config, roleMapper, authenticator, logoutHandler, mock(SamlLogoutResponseHandler.class), () -> idp, sp); } catch (SettingsException e) { logger.info(new ParameterizedMessage("Settings are invalid:\n{}", config.settings().toDelimitedString('\n')), e); throw e; From 3180c86397690d742e4fff48b9bc98b0e70f8229 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Thu, 7 May 2020 12:20:36 +1000 Subject: [PATCH 02/22] Refactor and add tests --- ...ction.java => SamlVerifyLogoutAction.java} | 10 +- ...uest.java => SamlVerifyLogoutRequest.java} | 6 +- ...va => SamlVerifyLogoutRequestBuilder.java} | 16 +- ...nse.java => SamlVerifyLogoutResponse.java} | 6 +- .../xpack/security/Security.java | 10 +- ...a => TransportSamlVerifyLogoutAction.java} | 21 +- .../authc/saml/SamlAuthenticator.java | 79 +----- .../authc/saml/SamlLogoutResponseHandler.java | 78 +----- .../authc/saml/SamlResponseHandler.java | 93 +++++++ ...n.java => RestSamlVerifyLogoutAction.java} | 34 +-- .../authc/saml/SamlAuthenticatorTests.java | 110 +-------- .../saml/SamlLogoutResponseHandlerTests.java | 112 +++++++++ .../authc/saml/SamlResponseHandlerTests.java | 231 ++++++++++++++++++ 13 files changed, 502 insertions(+), 304 deletions(-) rename x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/{SamlLogoutResponseAction.java => SamlVerifyLogoutAction.java} (61%) rename x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/{SamlLogoutResponseRequest.java => SamlVerifyLogoutRequest.java} (90%) rename x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/{SamlLogoutResponseRequestBuilder.java => SamlVerifyLogoutRequestBuilder.java} (51%) rename x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/{SamlLogoutResponseResponse.java => SamlVerifyLogoutResponse.java} (77%) rename x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/{TransportSamlLogoutResponseAction.java => TransportSamlVerifyLogoutAction.java} (65%) create mode 100644 x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlResponseHandler.java rename x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/{RestSamlLogoutResponseAction.java => RestSamlVerifyLogoutAction.java} (66%) create mode 100644 x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerTests.java create mode 100644 x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlResponseHandlerTests.java diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponseAction.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutAction.java similarity index 61% rename from x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponseAction.java rename to x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutAction.java index 8dea3ddd6f80c..6d0e0506577b0 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponseAction.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutAction.java @@ -10,12 +10,12 @@ /** * ActionType for authenticating using SAML assertions */ -public final class SamlLogoutResponseAction extends ActionType { +public final class SamlVerifyLogoutAction extends ActionType { - public static final String NAME = "cluster:admin/xpack/security/saml/logout_response"; - public static final SamlLogoutResponseAction INSTANCE = new SamlLogoutResponseAction(); + public static final String NAME = "cluster:admin/xpack/security/saml/verify_logout"; + public static final SamlVerifyLogoutAction INSTANCE = new SamlVerifyLogoutAction(); - private SamlLogoutResponseAction() { - super(NAME, SamlLogoutResponseResponse::new); + private SamlVerifyLogoutAction() { + super(NAME, SamlVerifyLogoutResponse::new); } } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponseRequest.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutRequest.java similarity index 90% rename from x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponseRequest.java rename to x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutRequest.java index d8299223e6d8c..681bf03abec80 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponseRequest.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutRequest.java @@ -16,7 +16,7 @@ /** * Represents a request to handle LogoutResponse using SAML assertions. */ -public final class SamlLogoutResponseRequest extends ActionRequest { +public final class SamlVerifyLogoutRequest extends ActionRequest { private byte[] saml; private List validRequestIds; @@ -25,11 +25,11 @@ public final class SamlLogoutResponseRequest extends ActionRequest { @Nullable private String assertionConsumerServiceURL; - public SamlLogoutResponseRequest(StreamInput in) throws IOException { + public SamlVerifyLogoutRequest(StreamInput in) throws IOException { super(in); } - public SamlLogoutResponseRequest() { + public SamlVerifyLogoutRequest() { } @Override diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponseRequestBuilder.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutRequestBuilder.java similarity index 51% rename from x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponseRequestBuilder.java rename to x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutRequestBuilder.java index 65fcc105f97bc..e9616fda74348 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponseRequestBuilder.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutRequestBuilder.java @@ -11,26 +11,26 @@ import java.util.List; /** - * Request builder used to populate a {@link SamlLogoutResponseRequest} + * Request builder used to populate a {@link SamlVerifyLogoutRequest} */ -public final class SamlLogoutResponseRequestBuilder - extends ActionRequestBuilder { +public final class SamlVerifyLogoutRequestBuilder + extends ActionRequestBuilder { - public SamlLogoutResponseRequestBuilder(ElasticsearchClient client) { - super(client, SamlLogoutResponseAction.INSTANCE, new SamlLogoutResponseRequest()); + public SamlVerifyLogoutRequestBuilder(ElasticsearchClient client) { + super(client, SamlVerifyLogoutAction.INSTANCE, new SamlVerifyLogoutRequest()); } - public SamlLogoutResponseRequestBuilder saml(byte[] saml) { + public SamlVerifyLogoutRequestBuilder saml(byte[] saml) { request.setSaml(saml); return this; } - public SamlLogoutResponseRequestBuilder validRequestIds(List validRequestIds) { + public SamlVerifyLogoutRequestBuilder validRequestIds(List validRequestIds) { request.setValidRequestIds(validRequestIds); return this; } - public SamlLogoutResponseRequestBuilder authenticatingRealm(String realm) { + public SamlVerifyLogoutRequestBuilder authenticatingRealm(String realm) { request.setRealm(realm); return this; } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponseResponse.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutResponse.java similarity index 77% rename from x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponseResponse.java rename to x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutResponse.java index 33ca399f51d50..8bcfb4f25b39c 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponseResponse.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutResponse.java @@ -14,13 +14,13 @@ /** * The response to the LogoutResponse from idP */ -public final class SamlLogoutResponseResponse extends ActionResponse { +public final class SamlVerifyLogoutResponse extends ActionResponse { - public SamlLogoutResponseResponse(StreamInput in) throws IOException { + public SamlVerifyLogoutResponse(StreamInput in) throws IOException { super(in); } - public SamlLogoutResponseResponse() { + public SamlVerifyLogoutResponse() { } @Override diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java index 8279a32284e3f..138a1a0a7714f 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java @@ -104,7 +104,7 @@ import org.elasticsearch.xpack.core.security.action.saml.SamlAuthenticateAction; import org.elasticsearch.xpack.core.security.action.saml.SamlInvalidateSessionAction; import org.elasticsearch.xpack.core.security.action.saml.SamlLogoutAction; -import org.elasticsearch.xpack.core.security.action.saml.SamlLogoutResponseAction; +import org.elasticsearch.xpack.core.security.action.saml.SamlVerifyLogoutAction; import org.elasticsearch.xpack.core.security.action.saml.SamlPrepareAuthenticationAction; import org.elasticsearch.xpack.core.security.action.token.CreateTokenAction; import org.elasticsearch.xpack.core.security.action.token.InvalidateTokenAction; @@ -168,7 +168,7 @@ import org.elasticsearch.xpack.security.action.saml.TransportSamlAuthenticateAction; import org.elasticsearch.xpack.security.action.saml.TransportSamlInvalidateSessionAction; import org.elasticsearch.xpack.security.action.saml.TransportSamlLogoutAction; -import org.elasticsearch.xpack.security.action.saml.TransportSamlLogoutResponseAction; +import org.elasticsearch.xpack.security.action.saml.TransportSamlVerifyLogoutAction; import org.elasticsearch.xpack.security.action.saml.TransportSamlPrepareAuthenticationAction; import org.elasticsearch.xpack.security.action.token.TransportCreateTokenAction; import org.elasticsearch.xpack.security.action.token.TransportInvalidateTokenAction; @@ -235,7 +235,7 @@ import org.elasticsearch.xpack.security.rest.action.saml.RestSamlAuthenticateAction; import org.elasticsearch.xpack.security.rest.action.saml.RestSamlInvalidateSessionAction; import org.elasticsearch.xpack.security.rest.action.saml.RestSamlLogoutAction; -import org.elasticsearch.xpack.security.rest.action.saml.RestSamlLogoutResponseAction; +import org.elasticsearch.xpack.security.rest.action.saml.RestSamlVerifyLogoutAction; import org.elasticsearch.xpack.security.rest.action.saml.RestSamlPrepareAuthenticationAction; import org.elasticsearch.xpack.security.rest.action.user.RestChangePasswordAction; import org.elasticsearch.xpack.security.rest.action.user.RestDeleteUserAction; @@ -754,7 +754,7 @@ public void onIndexModule(IndexModule module) { new ActionHandler<>(SamlAuthenticateAction.INSTANCE, TransportSamlAuthenticateAction.class), new ActionHandler<>(SamlLogoutAction.INSTANCE, TransportSamlLogoutAction.class), new ActionHandler<>(SamlInvalidateSessionAction.INSTANCE, TransportSamlInvalidateSessionAction.class), - new ActionHandler<>(SamlLogoutResponseAction.INSTANCE, TransportSamlLogoutResponseAction.class), + new ActionHandler<>(SamlVerifyLogoutAction.INSTANCE, TransportSamlVerifyLogoutAction.class), new ActionHandler<>(OpenIdConnectPrepareAuthenticationAction.INSTANCE, TransportOpenIdConnectPrepareAuthenticationAction.class), new ActionHandler<>(OpenIdConnectAuthenticateAction.INSTANCE, TransportOpenIdConnectAuthenticateAction.class), @@ -812,7 +812,7 @@ public List getRestHandlers(Settings settings, RestController restC new RestSamlAuthenticateAction(settings, getLicenseState()), new RestSamlLogoutAction(settings, getLicenseState()), new RestSamlInvalidateSessionAction(settings, getLicenseState()), - new RestSamlLogoutResponseAction(settings, getLicenseState()), + new RestSamlVerifyLogoutAction(settings, getLicenseState()), new RestOpenIdConnectPrepareAuthenticationAction(settings, getLicenseState()), new RestOpenIdConnectAuthenticateAction(settings, getLicenseState()), new RestOpenIdConnectLogoutAction(settings, getLicenseState()), diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlLogoutResponseAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlVerifyLogoutAction.java similarity index 65% rename from x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlLogoutResponseAction.java rename to x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlVerifyLogoutAction.java index 7e3483f6d6de5..6d10cb879993d 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlLogoutResponseAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlVerifyLogoutAction.java @@ -11,9 +11,9 @@ import org.elasticsearch.common.inject.Inject; import org.elasticsearch.tasks.Task; import org.elasticsearch.transport.TransportService; -import org.elasticsearch.xpack.core.security.action.saml.SamlLogoutResponseAction; -import org.elasticsearch.xpack.core.security.action.saml.SamlLogoutResponseRequest; -import org.elasticsearch.xpack.core.security.action.saml.SamlLogoutResponseResponse; +import org.elasticsearch.xpack.core.security.action.saml.SamlVerifyLogoutAction; +import org.elasticsearch.xpack.core.security.action.saml.SamlVerifyLogoutRequest; +import org.elasticsearch.xpack.core.security.action.saml.SamlVerifyLogoutResponse; import org.elasticsearch.xpack.security.authc.Realms; import org.elasticsearch.xpack.security.authc.saml.SamlLogoutResponseHandler; import org.elasticsearch.xpack.security.authc.saml.SamlRealm; @@ -26,18 +26,18 @@ /** * Transport action responsible for taking saml content and turning it into a token. */ -public final class TransportSamlLogoutResponseAction extends HandledTransportAction { +public final class TransportSamlVerifyLogoutAction extends HandledTransportAction { private final Realms realms; @Inject - public TransportSamlLogoutResponseAction(TransportService transportService, ActionFilters actionFilters, Realms realms) { - super(SamlLogoutResponseAction.NAME, transportService, actionFilters, SamlLogoutResponseRequest::new); + public TransportSamlVerifyLogoutAction(TransportService transportService, ActionFilters actionFilters, Realms realms) { + super(SamlVerifyLogoutAction.NAME, transportService, actionFilters, SamlVerifyLogoutRequest::new); this.realms = realms; } @Override - protected void doExecute(Task task, SamlLogoutResponseRequest request, ActionListener listener) { + protected void doExecute(Task task, SamlVerifyLogoutRequest request, ActionListener listener) { List realms = findSamlRealms(this.realms, request.getRealm(), request.getAssertionConsumerServiceURL()); if (realms.isEmpty()) { listener.onFailure(SamlUtils.samlException("Cannot find any matching realm for [{}]", request)); @@ -48,12 +48,13 @@ protected void doExecute(Task task, SamlLogoutResponseRequest request, ActionLis } } - private void processLogoutResponse(SamlRealm samlRealm, SamlLogoutResponseRequest request, - ActionListener listener) { + private void processLogoutResponse(SamlRealm samlRealm, SamlVerifyLogoutRequest request, + ActionListener listener) { final SamlLogoutResponseHandler logoutResponseHandler = samlRealm.getLogoutResponseHandler(); try { - listener.onResponse(logoutResponseHandler.buildResponse(request.getSaml(), request.getValidRequestIds())); + logoutResponseHandler.handle(request.getSaml(), request.getValidRequestIds()); + listener.onResponse(new SamlVerifyLogoutResponse()); } catch (Exception e) { listener.onFailure(e); } 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 de5dbde2c7e44..e676698b2b542 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 @@ -21,10 +21,6 @@ import org.opensaml.saml.saml2.core.EncryptedAssertion; import org.opensaml.saml.saml2.core.EncryptedAttribute; import org.opensaml.saml.saml2.core.Response; -import org.opensaml.saml.saml2.core.Status; -import org.opensaml.saml.saml2.core.StatusCode; -import org.opensaml.saml.saml2.core.StatusDetail; -import org.opensaml.saml.saml2.core.StatusMessage; import org.opensaml.saml.saml2.core.Subject; import org.opensaml.saml.saml2.core.SubjectConfirmation; import org.opensaml.saml.saml2.core.SubjectConfirmationData; @@ -46,7 +42,7 @@ /** * Processes the IdP's SAML Response for our AuthnRequest, validates it, and extracts the relevant properties. */ -class SamlAuthenticator extends SamlRequestHandler { +class SamlAuthenticator extends SamlResponseHandler { private static final String RESPONSE_TAG_NAME = "Response"; @@ -94,20 +90,8 @@ private SamlAttributes authenticateResponse(Element element, Collection 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()); - throw samlException("SAML content is in-response-to [{}] but expected one of {} ", - response.getInResponseTo(), allowedSamlRequestIds); - } - - final Status status = response.getStatus(); - if (status == null || status.getStatusCode() == null) { - throw samlException("SAML Response has no status code"); - } - if (isSuccess(status) == false) { - throw samlException("SAML Response is not a 'success' response: {}", getStatusCodeMessage(status)); - } + checkInResponseTo(response, allowedSamlRequestIds); + checkStatus(response.getStatus()); checkIssuer(response.getIssuer(), response); checkResponseDestination(response); @@ -136,46 +120,6 @@ private SamlAttributes authenticateResponse(Element element, Collection 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(); - } - - private String getDetail(Status status) { - final StatusDetail sd = status.getStatusDetail(); - return sd == null ? null : SamlUtils.toString(sd.getDOM()); - } - - private boolean isSuccess(Status status) { - return status.getStatusCode().getValue().equals(StatusCode.SUCCESS); - } - private String getSessionIndex(Assertion assertion) { return assertion.getAuthnStatements().stream().map(as -> as.getSessionIndex()).filter(Objects::nonNull).findFirst().orElse(null); } @@ -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); + // Allow for IdP initiated SSO where InResponseTo MUST be missing + if (Strings.hasText(subjectConfirmationData.getInResponseTo()) + && allowedSamlRequestIds.contains(subjectConfirmationData.getInResponseTo()) == false) { + throw samlException("SAML Assertion SubjectConfirmationData is in-response-to [{}] but expected one of [{}]", + subjectConfirmationData.getInResponseTo(), allowedSamlRequestIds); + } } private void checkRecipient(SubjectConfirmationData subjectConfirmationData) { @@ -344,15 +294,6 @@ private void checkRecipient(SubjectConfirmationData subjectConfirmationData) { } } - private void checkInResponseTo(SubjectConfirmationData subjectConfirmationData, Collection allowedSamlRequestIds) { - // Allow for IdP initiated SSO where InResponseTo MUST be missing - if (Strings.hasText(subjectConfirmationData.getInResponseTo()) - && allowedSamlRequestIds.contains(subjectConfirmationData.getInResponseTo()) == false) { - throw samlException("SAML Assertion SubjectConfirmationData is in-response-to [{}] but expected one of [{}]", - subjectConfirmationData.getInResponseTo(), allowedSamlRequestIds); - } - } - private void checkAudienceRestrictions(List restrictions) { if (restrictions.stream().allMatch(this::checkAudienceRestriction) == false) { throw samlException("Conditions [{}] do not match required audience [{}]", diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java index 191b5a8f22471..760c62d2d2898 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java @@ -6,15 +6,8 @@ package org.elasticsearch.xpack.security.authc.saml; -import org.elasticsearch.common.Strings; import org.elasticsearch.common.unit.TimeValue; -import org.elasticsearch.xpack.core.security.action.saml.SamlLogoutResponseResponse; import org.opensaml.saml.saml2.core.LogoutResponse; -import org.opensaml.saml.saml2.core.Status; -import org.opensaml.saml.saml2.core.StatusCode; -import org.opensaml.saml.saml2.core.StatusDetail; -import org.opensaml.saml.saml2.core.StatusMessage; -import org.opensaml.saml.saml2.core.StatusResponseType; import org.w3c.dom.Element; import java.time.Clock; @@ -22,7 +15,7 @@ import static org.elasticsearch.xpack.security.authc.saml.SamlUtils.samlException; -public class SamlLogoutResponseHandler extends SamlRequestHandler { +public class SamlLogoutResponseHandler extends SamlResponseHandler { private static final String LOGOUT_RESPONSE_TAG_NAME = "LogoutResponse"; @@ -31,7 +24,7 @@ public SamlLogoutResponseHandler( super(clock, idp, sp, maxSkew); } - public SamlLogoutResponseResponse buildResponse(byte[] payload, Collection allowedSamlRequestIds) { + public void handle(byte[] payload, Collection allowedSamlRequestIds) { final Element root = parseSamlMessage(payload); if (LOGOUT_RESPONSE_TAG_NAME.equals(root.getLocalName()) && SAML_NAMESPACE.equals(root.getNamespaceURI())) { final LogoutResponse logoutResponse = buildXmlObject(root, LogoutResponse.class); @@ -41,78 +34,13 @@ public SamlLogoutResponseResponse buildResponse(byte[] payload, Collection allowedSamlRequestIds) { - 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()); - throw samlException("SAML content is in-response-to [{}] but expected one of {} ", - response.getInResponseTo(), allowedSamlRequestIds); - } - } - - 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 void checkResponseDestination(StatusResponseType response, String spConfiguredUrl) { - if (spConfiguredUrl.equals(response.getDestination()) == false) { - if (response.isSigned() || Strings.hasText(response.getDestination())) { - throw samlException("SAML response " + response.getID() + " is for destination " + response.getDestination() - + " but this realm uses " + spConfiguredUrl); - } - } - } - - private String getMessage(Status status) { - final StatusMessage sm = status.getStatusMessage(); - return sm == null ? null : sm.getMessage(); - } - - private String getDetail(Status status) { - final StatusDetail sd = status.getStatusDetail(); - return sd == null ? null : SamlUtils.toString(sd.getDOM()); } } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlResponseHandler.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlResponseHandler.java new file mode 100644 index 0000000000000..ebbfb4f6f9ef7 --- /dev/null +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlResponseHandler.java @@ -0,0 +1,93 @@ +/* + * 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.authc.saml; + +import org.elasticsearch.common.Strings; +import org.elasticsearch.common.unit.TimeValue; +import org.opensaml.saml.saml2.core.Status; +import org.opensaml.saml.saml2.core.StatusCode; +import org.opensaml.saml.saml2.core.StatusDetail; +import org.opensaml.saml.saml2.core.StatusMessage; +import org.opensaml.saml.saml2.core.StatusResponseType; + +import java.time.Clock; +import java.util.Collection; + +import static org.elasticsearch.xpack.security.authc.saml.SamlUtils.samlException; + +public class SamlResponseHandler extends SamlRequestHandler { + public SamlResponseHandler(Clock clock, IdpConfiguration idp, SpConfiguration sp, TimeValue maxSkew) { + super(clock, idp, sp, maxSkew); + } + + protected void checkInResponseTo(StatusResponseType response, Collection allowedSamlRequestIds) { + 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()); + throw samlException("SAML content is in-response-to [{}] but expected one of {} ", + response.getInResponseTo(), allowedSamlRequestIds); + } + } + + protected 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(); + } + + protected void checkResponseDestination(StatusResponseType response, String spConfiguredUrl) { + if (spConfiguredUrl.equals(response.getDestination()) == false) { + if (response.isSigned() || Strings.hasText(response.getDestination())) { + throw samlException("SAML response " + response.getID() + " is for destination " + response.getDestination() + + " but this realm uses " + spConfiguredUrl); + } + } + } + + protected void checkStatus(Status status) { + if (status == null || status.getStatusCode() == null) { + throw samlException("SAML Response has no status code"); + } + if (isSuccess(status) == false) { + throw samlException("SAML Response is not a 'success' response: {}", getStatusCodeMessage(status)); + } + } + + protected boolean isSuccess(Status status) { + return StatusCode.SUCCESS.equals(status.getStatusCode().getValue()); + } + + private String getMessage(Status status) { + final StatusMessage sm = status.getStatusMessage(); + return sm == null ? null : sm.getMessage(); + } + + private String getDetail(Status status) { + final StatusDetail sd = status.getStatusDetail(); + return sd == null ? null : SamlUtils.toString(sd.getDOM()); + } +} diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlLogoutResponseAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlVerifyLogoutAction.java similarity index 66% rename from x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlLogoutResponseAction.java rename to x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlVerifyLogoutAction.java index 82861f387d90c..6ad4f68099cc7 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlLogoutResponseAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlVerifyLogoutAction.java @@ -21,8 +21,8 @@ import org.elasticsearch.rest.RestResponse; import org.elasticsearch.rest.RestStatus; import org.elasticsearch.rest.action.RestBuilderListener; -import org.elasticsearch.xpack.core.security.action.saml.SamlLogoutResponseRequestBuilder; -import org.elasticsearch.xpack.core.security.action.saml.SamlLogoutResponseResponse; +import org.elasticsearch.xpack.core.security.action.saml.SamlVerifyLogoutRequestBuilder; +import org.elasticsearch.xpack.core.security.action.saml.SamlVerifyLogoutResponse; import java.io.IOException; import java.util.Base64; @@ -30,9 +30,9 @@ import static org.elasticsearch.rest.RestRequest.Method.POST; -public class RestSamlLogoutResponseAction extends SamlBaseRestHandler{ +public class RestSamlVerifyLogoutAction extends SamlBaseRestHandler{ - private static final Logger logger = LogManager.getLogger(RestSamlLogoutResponseAction.class); + private static final Logger logger = LogManager.getLogger(RestSamlVerifyLogoutAction.class); static class Input { String content; @@ -55,42 +55,42 @@ void setAssertionConsumerServiceURL(String assertionConsumerServiceURL) { } } - static final ObjectParser - PARSER = new ObjectParser<>("saml_logout_response", RestSamlLogoutResponseAction.Input::new); + static final ObjectParser + PARSER = new ObjectParser<>("saml_verify_logout", RestSamlVerifyLogoutAction.Input::new); static { - PARSER.declareString(RestSamlLogoutResponseAction.Input::setContent, new ParseField("content")); - PARSER.declareStringArray(RestSamlLogoutResponseAction.Input::setIds, new ParseField("ids")); - PARSER.declareStringOrNull(RestSamlLogoutResponseAction.Input::setRealm, new ParseField("realm")); - PARSER.declareString(RestSamlLogoutResponseAction.Input::setAssertionConsumerServiceURL, new ParseField("acs")); + PARSER.declareString(RestSamlVerifyLogoutAction.Input::setContent, new ParseField("content")); + PARSER.declareStringArray(RestSamlVerifyLogoutAction.Input::setIds, new ParseField("ids")); + PARSER.declareStringOrNull(RestSamlVerifyLogoutAction.Input::setRealm, new ParseField("realm")); + PARSER.declareString(RestSamlVerifyLogoutAction.Input::setAssertionConsumerServiceURL, new ParseField("acs")); } - public RestSamlLogoutResponseAction(Settings settings, XPackLicenseState licenseState) { + public RestSamlVerifyLogoutAction(Settings settings, XPackLicenseState licenseState) { super(settings, licenseState); } @Override public String getName() { - return "security_saml_logout_response_action"; + return "security_saml_verify_logout_action"; } @Override public List routes() { - return List.of(new Route(POST, "/_security/saml/logout_response")); + return List.of(new Route(POST, "/_security/saml/verify_logout")); } @Override protected RestChannelConsumer innerPrepareRequest(RestRequest request, NodeClient client) throws IOException { try (XContentParser parser = request.contentParser()) { - final RestSamlLogoutResponseAction.Input input = PARSER.parse(parser, null); + final RestSamlVerifyLogoutAction.Input input = PARSER.parse(parser, null); logger.trace("SAML LogoutResponse: [{}...] [{}]", Strings.cleanTruncate(input.content, 128), input.ids); return channel -> { final byte[] bytes = decodeBase64(input.content); - final SamlLogoutResponseRequestBuilder requestBuilder = - new SamlLogoutResponseRequestBuilder(client).saml(bytes).validRequestIds(input.ids).authenticatingRealm(input.realm); + final SamlVerifyLogoutRequestBuilder requestBuilder = + new SamlVerifyLogoutRequestBuilder(client).saml(bytes).validRequestIds(input.ids).authenticatingRealm(input.realm); requestBuilder.execute(new RestBuilderListener<>(channel) { @Override - public RestResponse buildResponse(SamlLogoutResponseResponse response, XContentBuilder builder) throws Exception { + public RestResponse buildResponse(SamlVerifyLogoutResponse response, XContentBuilder builder) throws Exception { builder.startObject() .endObject(); return new BytesRestResponse(RestStatus.OK, builder); 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 f18a8a74ae484..847862d79710e 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 @@ -8,8 +8,6 @@ import org.apache.logging.log4j.Level; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; -import org.apache.xml.security.Init; -import org.apache.xml.security.encryption.XMLCipher; import org.elasticsearch.ElasticsearchSecurityException; import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.logging.Loggers; @@ -19,9 +17,7 @@ import org.elasticsearch.xpack.core.watcher.watch.ClockMock; import org.hamcrest.Matchers; import org.joda.time.DateTime; -import org.junit.AfterClass; import org.junit.Before; -import org.junit.BeforeClass; import org.opensaml.core.xml.config.XMLObjectProviderRegistrySupport; import org.opensaml.core.xml.schema.XSString; import org.opensaml.core.xml.schema.impl.XSStringBuilder; @@ -64,7 +60,6 @@ import org.xml.sax.InputSource; import org.xml.sax.SAXException; -import javax.crypto.Cipher; import javax.crypto.KeyGenerator; import javax.crypto.SecretKey; import javax.xml.crypto.dsig.CanonicalizationMethod; @@ -80,13 +75,11 @@ import javax.xml.crypto.dsig.keyinfo.KeyInfoFactory; import javax.xml.crypto.dsig.spec.C14NMethodParameterSpec; import javax.xml.crypto.dsig.spec.TransformParameterSpec; -import javax.xml.parsers.DocumentBuilder; import javax.xml.parsers.DocumentBuilderFactory; import javax.xml.parsers.ParserConfigurationException; import java.io.IOException; import java.io.StringReader; import java.nio.charset.StandardCharsets; -import java.security.NoSuchAlgorithmException; import java.security.PrivateKey; import java.security.cert.X509Certificate; import java.time.Instant; @@ -123,77 +116,16 @@ import static org.opensaml.saml.saml2.core.SubjectConfirmation.METHOD_BEARER; import static org.opensaml.saml.saml2.core.SubjectConfirmation.METHOD_HOLDER_OF_KEY; -public class SamlAuthenticatorTests extends SamlTestCase { +public class SamlAuthenticatorTests extends SamlResponseHandlerTests { - private static final String SP_ENTITY_ID = "https://sp.saml.elastic.test/"; - private static final String IDP_ENTITY_ID = "https://idp.saml.elastic.test/"; - private static final String SP_ACS_URL = SP_ENTITY_ID + "sso/post"; private static final String UID_OID = "urn:oid:0.9.2342.19200300.100.1.1"; - private static Tuple idpSigningCertificatePair; - private static Tuple spSigningCertificatePair; - private static List> spEncryptionCertificatePairs; - - private static List supportedAesKeyLengths; - private static List supportedAesTransformations; - - private ClockMock clock; private SamlAuthenticator authenticator; - private String requestId; - private TimeValue maxSkew; - - @BeforeClass - public static void init() throws Exception { - SamlUtils.initialize(LogManager.getLogger(SamlAuthenticatorTests.class)); - // Initialise Apache XML security so that the signDoc methods work correctly. - Init.init(); - } - - @BeforeClass - public static void calculateAesLength() throws NoSuchAlgorithmException { - supportedAesKeyLengths = new ArrayList<>(); - supportedAesTransformations = new ArrayList<>(); - supportedAesKeyLengths.add(128); - supportedAesTransformations.add(XMLCipher.AES_128); - supportedAesTransformations.add(XMLCipher.AES_128_GCM); - if (Cipher.getMaxAllowedKeyLength("AES") > 128) { - supportedAesKeyLengths.add(192); - supportedAesKeyLengths.add(256); - supportedAesTransformations.add(XMLCipher.AES_192); - supportedAesTransformations.add(XMLCipher.AES_192_GCM); - supportedAesTransformations.add(XMLCipher.AES_256); - supportedAesTransformations.add(XMLCipher.AES_256_GCM); - } - } - - /** - * Generating X.509 credentials can be CPU intensive and slow, so we only want to do it once per class. - */ - @BeforeClass - public static void initCredentials() throws Exception { - idpSigningCertificatePair = readRandomKeyPair(randomSigningAlgorithm()); - spSigningCertificatePair = readRandomKeyPair(randomSigningAlgorithm()); - spEncryptionCertificatePairs = Arrays.asList(readKeyPair("ENCRYPTION_RSA_2048"), readKeyPair("ENCRYPTION_RSA_4096")); - } - - private static String randomSigningAlgorithm() { - return randomFrom("RSA", "DSA", "EC"); - } - - @AfterClass - public static void cleanup() { - idpSigningCertificatePair = null; - spSigningCertificatePair = null; - spEncryptionCertificatePairs = null; - supportedAesKeyLengths = null; - supportedAesTransformations = null; - } @Before public void setupAuthenticator() throws Exception { this.clock = new ClockMock(); this.maxSkew = TimeValue.timeValueMinutes(1); - this.authenticator = buildAuthenticator(() -> buildOpenSamlCredential(idpSigningCertificatePair), emptyList()); this.requestId = randomId(); } @@ -1266,42 +1198,6 @@ private String signResponse(String xml, String c14nMethod, Tuple buildOpenSamlCredential(idpSigningCertificatePair)), + getSpConfiguration(emptyList()), + maxSkew); + } + + public void testHandleWorksWithoutSignature() { + final String xml = "\n" + + "\n" + + " %(IDP_ENTITY_ID)\n" + + " \n" + + " \n" + + " \n" + + ""; + + final Map replacements = new HashMap<>(); + replacements.put("IDP_ENTITY_ID", IDP_ENTITY_ID); + replacements.put("now", clock.instant()); + replacements.put("randomId", requestId); + replacements.put("requestId", requestId); + replacements.put("SP_LOGOUT_URL", SP_LOGOUT_URL); + + final String payload = NamedFormatter.format(xml, replacements); + samlLogoutResponseHandler.handle(payload.getBytes(StandardCharsets.UTF_8), List.of(requestId)); + } + + public void testHandleWorksWithSignature() throws Exception { + final String xml = "\n" + + "\n" + + " %(IDP_ENTITY_ID)\n" + + " \n" + + " \n" + + " \n" + + ""; + + final Map replacements = new HashMap<>(); + replacements.put("IDP_ENTITY_ID", IDP_ENTITY_ID); + replacements.put("now", clock.instant()); + replacements.put("randomId", requestId); + replacements.put("requestId", requestId); + replacements.put("SP_LOGOUT_URL", SP_LOGOUT_URL); + + final String payload = signDoc(NamedFormatter.format(xml, replacements)); + samlLogoutResponseHandler.handle(payload.getBytes(StandardCharsets.UTF_8), List.of(requestId)); + } + + public void testHandleWillThrowWhenStatusIsNotSuccess() { + final String xml = "\n" + + "\n" + + " %(IDP_ENTITY_ID)\n" + + " \n" + + " \n" + + " \n" + + ""; + + final Map replacements = new HashMap<>(); + replacements.put("IDP_ENTITY_ID", IDP_ENTITY_ID); + replacements.put("now", clock.instant()); + replacements.put("randomId", requestId); + replacements.put("requestId", requestId); + replacements.put("SP_LOGOUT_URL", SP_LOGOUT_URL); + + final String payload = NamedFormatter.format(xml, replacements); + final ElasticsearchSecurityException e = expectSamlException(() -> + samlLogoutResponseHandler.handle(payload.getBytes(StandardCharsets.UTF_8), List.of(requestId))); + assertThat(e.getMessage(), containsString("not a 'success' response")); + } +} diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlResponseHandlerTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlResponseHandlerTests.java new file mode 100644 index 0000000000000..d4cdd775a36cd --- /dev/null +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlResponseHandlerTests.java @@ -0,0 +1,231 @@ +/* + * 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.authc.saml; + +import org.apache.logging.log4j.LogManager; +import org.apache.xml.security.Init; +import org.apache.xml.security.encryption.XMLCipher; +import org.elasticsearch.common.collect.Tuple; +import org.elasticsearch.common.unit.TimeValue; +import org.elasticsearch.xpack.core.watcher.watch.ClockMock; +import org.junit.AfterClass; +import org.junit.BeforeClass; +import org.opensaml.security.credential.Credential; +import org.opensaml.security.x509.X509Credential; +import org.w3c.dom.Document; +import org.w3c.dom.Element; +import org.w3c.dom.NodeList; +import org.xml.sax.InputSource; +import org.xml.sax.SAXException; + +import java.io.IOException; +import java.io.StringReader; +import java.security.KeyException; +import java.security.NoSuchAlgorithmException; +import java.security.PrivateKey; +import java.security.cert.X509Certificate; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import java.util.function.Supplier; +import java.util.stream.Collectors; +import javax.crypto.Cipher; +import javax.xml.crypto.dsig.CanonicalizationMethod; +import javax.xml.crypto.dsig.DigestMethod; +import javax.xml.crypto.dsig.Reference; +import javax.xml.crypto.dsig.SignatureMethod; +import javax.xml.crypto.dsig.SignedInfo; +import javax.xml.crypto.dsig.Transform; +import javax.xml.crypto.dsig.XMLSignature; +import javax.xml.crypto.dsig.XMLSignatureFactory; +import javax.xml.crypto.dsig.dom.DOMSignContext; +import javax.xml.crypto.dsig.keyinfo.KeyInfo; +import javax.xml.crypto.dsig.keyinfo.KeyInfoFactory; +import javax.xml.crypto.dsig.spec.C14NMethodParameterSpec; +import javax.xml.crypto.dsig.spec.TransformParameterSpec; +import javax.xml.parsers.DocumentBuilder; +import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.parsers.ParserConfigurationException; + +import static java.util.Collections.singletonList; +import static javax.xml.crypto.dsig.CanonicalizationMethod.EXCLUSIVE; +import static javax.xml.crypto.dsig.Transform.ENVELOPED; +import static org.opensaml.saml.common.xml.SAMLConstants.SAML20_NS; + +public class SamlResponseHandlerTests extends SamlTestCase { + protected static final String SP_ENTITY_ID = "https://sp.saml.elastic.test/"; + protected static final String IDP_ENTITY_ID = "https://idp.saml.elastic.test/"; + protected static final String SP_ACS_URL = SP_ENTITY_ID + "sso/post"; + protected static final String SP_LOGOUT_URL = SP_ENTITY_ID + "sso/logout"; + protected static Tuple idpSigningCertificatePair; + protected static Tuple spSigningCertificatePair; + protected static List> spEncryptionCertificatePairs; + protected static List supportedAesKeyLengths; + protected static List supportedAesTransformations; + protected ClockMock clock; + protected String requestId; + protected TimeValue maxSkew; + + @BeforeClass + public static void init() throws Exception { + SamlUtils.initialize(LogManager.getLogger(SamlResponseHandlerTests.class)); + // Initialise Apache XML security so that the signDoc methods work correctly. + Init.init(); + } + + @BeforeClass + public static void calculateAesLength() throws NoSuchAlgorithmException { + supportedAesKeyLengths = new ArrayList<>(); + supportedAesTransformations = new ArrayList<>(); + supportedAesKeyLengths.add(128); + supportedAesTransformations.add(XMLCipher.AES_128); + supportedAesTransformations.add(XMLCipher.AES_128_GCM); + if (Cipher.getMaxAllowedKeyLength("AES") > 128) { + supportedAesKeyLengths.add(192); + supportedAesKeyLengths.add(256); + supportedAesTransformations.add(XMLCipher.AES_192); + supportedAesTransformations.add(XMLCipher.AES_192_GCM); + supportedAesTransformations.add(XMLCipher.AES_256); + supportedAesTransformations.add(XMLCipher.AES_256_GCM); + } + } + + /** + * Generating X.509 credentials can be CPU intensive and slow, so we only want to do it once per class. + */ + @BeforeClass + public static void initCredentials() throws Exception { + idpSigningCertificatePair = readRandomKeyPair(SamlResponseHandlerTests.randomSigningAlgorithm()); + spSigningCertificatePair = readRandomKeyPair(SamlResponseHandlerTests.randomSigningAlgorithm()); + spEncryptionCertificatePairs = Arrays.asList(readKeyPair("RSA_2048"), readKeyPair("RSA_4096")); + } + + protected static String randomSigningAlgorithm() { + return randomFrom("RSA", "DSA", "EC"); + } + + @AfterClass + public static void cleanup() { + idpSigningCertificatePair = null; + spSigningCertificatePair = null; + spEncryptionCertificatePairs = null; + supportedAesKeyLengths = null; + supportedAesTransformations = null; + } + + private static KeyInfo getKeyInfo(XMLSignatureFactory factory, X509Certificate certificate) throws KeyException { + KeyInfoFactory kif = factory.getKeyInfoFactory(); + javax.xml.crypto.dsig.keyinfo.X509Data data = kif.newX509Data(Collections.singletonList(certificate)); + return kif.newKeyInfo(singletonList(data)); + } + + protected SpConfiguration getSpConfiguration(List reqAuthnCtxClassRef) { + final SigningConfiguration signingConfiguration = new SigningConfiguration( + Collections.singleton("*"), + (X509Credential) buildOpenSamlCredential(spSigningCertificatePair).get(0)); + final List spEncryptionCredentials = buildOpenSamlCredential(spEncryptionCertificatePairs).stream() + .map((cred) -> (X509Credential) cred).collect(Collectors.toList()); + return new SpConfiguration(SP_ENTITY_ID, SP_ACS_URL, SP_LOGOUT_URL, signingConfiguration, spEncryptionCredentials, + reqAuthnCtxClassRef); + } + + protected IdpConfiguration getIdpConfiguration(Supplier> credentials) { + return new IdpConfiguration(IDP_ENTITY_ID, credentials); + } + + protected String randomId() { + return SamlUtils.generateSecureNCName(randomIntBetween(12, 36)); + } + + protected String signDoc(String xml) throws Exception { + return signDoc(xml, EXCLUSIVE, SamlResponseHandlerTests.idpSigningCertificatePair); + } + + protected String signDoc(String xml, Tuple keyPair) throws Exception { + return signDoc(xml, EXCLUSIVE, keyPair); + } + + protected String signDoc(String xml, String c14nMethod) throws Exception { + return signDoc(xml, c14nMethod, SamlResponseHandlerTests.idpSigningCertificatePair); + } + + private String signDoc(String xml, String c14nMethod, Tuple keyPair) throws Exception { + final Document doc = parseDocument(xml); + signElement(doc.getDocumentElement(), keyPair, c14nMethod); + return SamlUtils.toString(doc.getDocumentElement()); + } + + protected Document parseDocument(String xml) throws ParserConfigurationException, SAXException, IOException { + final DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance(); + dbf.setNamespaceAware(true); + final DocumentBuilder documentBuilder = dbf.newDocumentBuilder(); + return documentBuilder.parse(new InputSource(new StringReader(xml))); + } + + /** + * Randomly selects digital signature algorithm URI for given private key + * algorithm ({@link PrivateKey#getAlgorithm()}). + * + * @param key + * {@link PrivateKey} + * @return algorithm URI + */ + protected String getSignatureAlgorithmURI(PrivateKey key) { + String algoUri = null; + switch (key.getAlgorithm()) { + case "RSA": + algoUri = randomFrom("http://www.w3.org/2001/04/xmldsig-more#rsa-sha256", + "http://www.w3.org/2001/04/xmldsig-more#rsa-sha512"); + break; + case "DSA": + algoUri = "http://www.w3.org/2009/xmldsig11#dsa-sha256"; + break; + case "EC": + algoUri = randomFrom("http://www.w3.org/2001/04/xmldsig-more#ecdsa-sha256", + "http://www.w3.org/2001/04/xmldsig-more#ecdsa-sha512"); + break; + default: + throw new IllegalArgumentException("Unsupported algorithm : " + key.getAlgorithm() + + " for signature, allowed values for private key algorithm are [RSA, DSA, EC]"); + } + return algoUri; + } + + protected void signElement(Element parent, Tuple keyPair, String c14nMethod) throws Exception { + //We need to explicitly set the Id attribute, "ID" is just our convention + parent.setIdAttribute("ID", true); + final String refID = "#" + parent.getAttribute("ID"); + final X509Certificate certificate = keyPair.v1(); + final PrivateKey privateKey = keyPair.v2(); + final XMLSignatureFactory fac = XMLSignatureFactory.getInstance("DOM"); + final DigestMethod digestMethod = fac.newDigestMethod(randomFrom(DigestMethod.SHA256, DigestMethod.SHA512), null); + final Transform transform = fac.newTransform(ENVELOPED, (TransformParameterSpec) null); + // We don't "have to" set the reference explicitly since we're using enveloped signatures, but it helps with + // creating the XSW test cases + final Reference reference = fac.newReference(refID, digestMethod, singletonList(transform), null, null); + final SignatureMethod signatureMethod = fac.newSignatureMethod(getSignatureAlgorithmURI(privateKey), null); + final CanonicalizationMethod canonicalizationMethod = fac.newCanonicalizationMethod(c14nMethod, (C14NMethodParameterSpec) null); + + final SignedInfo signedInfo = fac.newSignedInfo(canonicalizationMethod, signatureMethod, singletonList(reference)); + + final KeyInfo keyInfo = SamlResponseHandlerTests.getKeyInfo(fac, certificate); + + final DOMSignContext dsc = new DOMSignContext(privateKey, parent); + dsc.setDefaultNamespacePrefix("ds"); + // According to the schema, the signature needs to be placed after the if there is one in the document + // If there are more than one we are dealing with a so we sign the Response and add the + // Signature after the Response + NodeList issuersList = parent.getElementsByTagNameNS(SAML20_NS, "Issuer"); + if (issuersList.getLength() > 0) { + dsc.setNextSibling(issuersList.item(0).getNextSibling()); + } + + final XMLSignature signature = fac.newXMLSignature(signedInfo, keyInfo); + signature.sign(dsc); + } +} From e6d4d5a4e5068870a86e7e8e37cccb1548c76da0 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Thu, 7 May 2020 14:08:39 +1000 Subject: [PATCH 03/22] fix copy/paste text --- .../xpack/core/security/action/saml/SamlVerifyLogoutAction.java | 2 +- .../core/security/action/saml/SamlVerifyLogoutRequest.java | 2 +- .../core/security/action/saml/SamlVerifyLogoutResponse.java | 2 +- .../security/action/saml/TransportSamlVerifyLogoutAction.java | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutAction.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutAction.java index 6d0e0506577b0..e327ba12f27f7 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutAction.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutAction.java @@ -8,7 +8,7 @@ import org.elasticsearch.action.ActionType; /** - * ActionType for authenticating using SAML assertions + * ActionType for verifying SAML LogoutResponse */ public final class SamlVerifyLogoutAction extends ActionType { diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutRequest.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutRequest.java index 681bf03abec80..855111c90c3a5 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutRequest.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutRequest.java @@ -14,7 +14,7 @@ import java.util.List; /** - * Represents a request to handle LogoutResponse using SAML assertions. + * Represents a request to verify SAML LogoutResponse */ public final class SamlVerifyLogoutRequest extends ActionRequest { diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutResponse.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutResponse.java index 8bcfb4f25b39c..b7038d5099951 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutResponse.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutResponse.java @@ -12,7 +12,7 @@ import java.io.IOException; /** - * The response to the LogoutResponse from idP + * The verification response to the LogoutResponse from idP */ public final class SamlVerifyLogoutResponse extends ActionResponse { diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlVerifyLogoutAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlVerifyLogoutAction.java index 6d10cb879993d..a7a937fe4ec16 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlVerifyLogoutAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlVerifyLogoutAction.java @@ -24,7 +24,7 @@ import static org.elasticsearch.xpack.security.authc.saml.SamlRealm.findSamlRealms; /** - * Transport action responsible for taking saml content and turning it into a token. + * Transport action responsible for verifying SAML LogoutResponse */ public final class TransportSamlVerifyLogoutAction extends HandledTransportAction { From 3ef50a1329c35e0197f0b0e41601c64c3ec6af28 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Fri, 8 May 2020 11:34:34 +1000 Subject: [PATCH 04/22] Add requestId to logout response for kibana to pass it back --- .../security/action/saml/SamlLogoutResponse.java | 15 ++++++++++++--- .../saml/SamlPrepareAuthenticationResponse.java | 2 +- .../action/saml/TransportSamlLogoutAction.java | 4 ++-- .../rest/action/saml/RestSamlLogoutAction.java | 1 + .../saml/TransportSamlLogoutActionTests.java | 1 + 5 files changed, 17 insertions(+), 6 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponse.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponse.java index dc176b6113451..a63a81fc7dc09 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponse.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponse.java @@ -16,24 +16,33 @@ */ 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; 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); } - } +} diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlPrepareAuthenticationResponse.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlPrepareAuthenticationResponse.java index 942d7d0b0afef..69adb7ac3df6c 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlPrepareAuthenticationResponse.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlPrepareAuthenticationResponse.java @@ -48,4 +48,4 @@ public void writeTo(StreamOutput out) throws IOException { out.writeString(redirectUrl); } - } +} diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlLogoutAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlLogoutAction.java index c7df4c34337bd..7611340e47788 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlLogoutAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlLogoutAction.java @@ -112,10 +112,10 @@ private SamlLogoutResponse buildResponse(Authentication authentication, Map metadata, String key) { diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlLogoutAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlLogoutAction.java index a7836f9ce059c..311b2c87b4866 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlLogoutAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlLogoutAction.java @@ -74,6 +74,7 @@ public RestChannelConsumer innerPrepareRequest(RestRequest request, NodeClient c @Override public RestResponse buildResponse(SamlLogoutResponse response, XContentBuilder builder) throws Exception { builder.startObject(); + builder.field("id", response.getRequestId()); builder.field("redirect", response.getRedirectUrl()); builder.endObject(); return new BytesRestResponse(RestStatus.OK, builder); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/saml/TransportSamlLogoutActionTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/saml/TransportSamlLogoutActionTests.java index 6af1ac328d3ee..b455e4d091c2b 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/saml/TransportSamlLogoutActionTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/saml/TransportSamlLogoutActionTests.java @@ -259,6 +259,7 @@ public void testLogoutInvalidatesToken() throws Exception { action.doExecute(mock(Task.class), request, listener); final SamlLogoutResponse response = listener.get(); assertThat(response, notNullValue()); + assertThat(response.getRequestId(), notNullValue()); assertThat(response.getRedirectUrl(), notNullValue()); final IndexRequest indexRequest1 = indexRequests.get(0); From c5633e57b5839e0065a4c1a29902f31b47a344d3 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Fri, 8 May 2020 11:35:27 +1000 Subject: [PATCH 05/22] checkstyle --- .../xpack/core/security/action/saml/SamlLogoutResponse.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponse.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponse.java index a63a81fc7dc09..c6599b7db1d9e 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponse.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlLogoutResponse.java @@ -25,8 +25,7 @@ public SamlLogoutResponse(StreamInput in) throws IOException { redirectUrl = in.readString(); } - public - SamlLogoutResponse(String requestId, String redirectUrl) { + public SamlLogoutResponse(String requestId, String redirectUrl) { this.requestId = requestId; this.redirectUrl = redirectUrl; } From c1ea969b20ffe19c4c73e8ed51d3ec2cbb9209c4 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Mon, 11 May 2020 15:09:04 +1000 Subject: [PATCH 06/22] Support LogoutResponse with both HTTP-Redirect and HTTP-Post bindings --- .../action/saml/SamlVerifyLogoutRequest.java | 10 +- .../saml/SamlVerifyLogoutRequestBuilder.java | 4 +- .../saml/TransportSamlVerifyLogoutAction.java | 2 +- .../authc/saml/SamlLogoutRequestHandler.java | 74 +--------- .../authc/saml/SamlLogoutResponseHandler.java | 19 ++- .../authc/saml/SamlRequestHandler.java | 79 +++++++++++ .../saml/RestSamlVerifyLogoutAction.java | 15 +- ...amlLogoutResponseHandlerHttpPostTests.java | 87 ++++++++++++ ...ogoutResponseHandlerHttpRedirectTests.java | 132 ++++++++++++++++++ .../saml/SamlLogoutResponseHandlerTests.java | 112 --------------- 10 files changed, 326 insertions(+), 208 deletions(-) create mode 100644 x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpPostTests.java create mode 100644 x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpRedirectTests.java delete mode 100644 x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerTests.java diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutRequest.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutRequest.java index 855111c90c3a5..cdbd5c9fc5e2e 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutRequest.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutRequest.java @@ -18,7 +18,7 @@ */ public final class SamlVerifyLogoutRequest extends ActionRequest { - private byte[] saml; + private String content; private List validRequestIds; @Nullable private String realm; @@ -37,12 +37,12 @@ public ActionRequestValidationException validate() { return null; } - public byte[] getSaml() { - return saml; + public String getContent() { + return content; } - public void setSaml(byte[] saml) { - this.saml = saml; + public void setContent(String content) { + this.content = content; } public List getValidRequestIds() { diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutRequestBuilder.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutRequestBuilder.java index e9616fda74348..22d8b2bbf021a 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutRequestBuilder.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutRequestBuilder.java @@ -20,8 +20,8 @@ public SamlVerifyLogoutRequestBuilder(ElasticsearchClient client) { super(client, SamlVerifyLogoutAction.INSTANCE, new SamlVerifyLogoutRequest()); } - public SamlVerifyLogoutRequestBuilder saml(byte[] saml) { - request.setSaml(saml); + public SamlVerifyLogoutRequestBuilder content(String content) { + request.setContent(content); return this; } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlVerifyLogoutAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlVerifyLogoutAction.java index a7a937fe4ec16..f057f0952f620 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlVerifyLogoutAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlVerifyLogoutAction.java @@ -53,7 +53,7 @@ private void processLogoutResponse(SamlRealm samlRealm, SamlVerifyLogoutRequest final SamlLogoutResponseHandler logoutResponseHandler = samlRealm.getLogoutResponseHandler(); try { - logoutResponseHandler.handle(request.getSaml(), request.getValidRequestIds()); + logoutResponseHandler.handle(request.getContent(), request.getValidRequestIds()); listener.onResponse(new SamlVerifyLogoutResponse()); } catch (Exception e) { listener.onFailure(e); diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutRequestHandler.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutRequestHandler.java index 9bd8527e373b2..30d43774bb990 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutRequestHandler.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutRequestHandler.java @@ -57,9 +57,9 @@ public class SamlLogoutRequestHandler extends SamlRequestHandler { * @throws ElasticsearchSecurityException If the SAML is invalid for this realm/configuration */ public Result parseFromQueryString(String queryString) { - final ParsedQueryString parsed = parseQueryStringAndValidateSignature(queryString); + final ParsedQueryString parsed = parseQueryStringAndValidateSignature(queryString, "SAMLRequest"); - final Element root = parseSamlMessage(inflate(decodeBase64(parsed.samlRequest))); + final Element root = parseSamlMessage(inflate(decodeBase64(parsed.samlMessage))); if (REQUEST_TAG_NAME.equals(root.getLocalName()) && SAML_NAMESPACE.equals(root.getNamespaceURI())) { try { final LogoutRequest logoutRequest = buildXmlObject(root, LogoutRequest.class); @@ -74,26 +74,6 @@ public Result parseFromQueryString(String queryString) { } } - private ParsedQueryString parseQueryStringAndValidateSignature(String queryString) { - final String signatureInput = queryString.replaceAll("&Signature=.*$", ""); - final Map parameters = new HashMap<>(); - RestUtils.decodeQueryString(queryString, 0, parameters); - final String samlRequest = parameters.get("SAMLRequest"); - if (samlRequest == null) { - throw samlException("Could not parse SAMLRequest from query string: [{}]", queryString); - } - - final String relayState = parameters.get("RelayState"); - final String signatureAlgorithm = parameters.get("SigAlg"); - final String signature = parameters.get("Signature"); - if (signature == null || signatureAlgorithm == null) { - return new ParsedQueryString(samlRequest, false, relayState); - } - - validateSignature(signatureInput, signatureAlgorithm, signature); - return new ParsedQueryString(samlRequest, true, relayState); - } - private Result parseLogout(LogoutRequest logoutRequest, boolean requireSignature, String relayState) { final Signature signature = logoutRequest.getSignature(); if (signature == null) { @@ -111,44 +91,6 @@ private Result parseLogout(LogoutRequest logoutRequest, boolean requireSignature return new Result(logoutRequest.getID(), SamlNameId.fromXml(getNameID(logoutRequest)), getSessionIndex(logoutRequest), relayState); } - private void validateSignature(String inputString, String signatureAlgorithm, String signature) { - final byte[] sigBytes = decodeBase64(signature); - final byte[] inputBytes = inputString.getBytes(StandardCharsets.US_ASCII); - final String signatureText = Strings.cleanTruncate(signature, 32); - checkIdpSignature(credential -> { - if (XMLSigningUtil.verifyWithURI(credential, signatureAlgorithm, sigBytes, inputBytes)) { - logger.debug(() -> new ParameterizedMessage("SAML Signature [{}] matches credentials [{}] [{}]", - signatureText, credential.getEntityId(), credential.getPublicKey())); - return true; - } else { - logger.debug(() -> new ParameterizedMessage("SAML Signature [{}] failed against credentials [{}] [{}]", - signatureText, credential.getEntityId(), credential.getPublicKey())); - return false; - } - }, signatureText); - } - - private byte[] decodeBase64(String content) { - try { - return Base64.getDecoder().decode(content.replaceAll("\\s+", "")); - } catch (IllegalArgumentException e) { - logger.info("Failed to decode base64 string [{}] - {}", content, e.toString()); - throw samlException("SAML message cannot be Base64 decoded", e); - } - } - - private byte[] inflate(byte[] bytes) { - Inflater inflater = new Inflater(true); - try (ByteArrayInputStream in = new ByteArrayInputStream(bytes); - InflaterInputStream inflate = new InflaterInputStream(in, inflater); - ByteArrayOutputStream out = new ByteArrayOutputStream(bytes.length * 3 / 2)) { - Streams.copy(inflate, out); - return out.toByteArray(); - } catch (IOException e) { - throw samlException("SAML message cannot be inflated", e); - } - } - private NameID getNameID(LogoutRequest logoutRequest) { final NameID nameID = logoutRequest.getNameID(); if (nameID == null) { @@ -197,18 +139,6 @@ private void checkDestination(LogoutRequest request) { } } - static class ParsedQueryString { - final String samlRequest; - final boolean hasSignature; - final String relayState; - - ParsedQueryString(String samlRequest, boolean hasSignature, String relayState) { - this.samlRequest = samlRequest; - this.hasSignature = hasSignature; - this.relayState = relayState; - } - } - public static class Result { private final String requestId; private final SamlNameId nameId; diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java index 760c62d2d2898..fa1dadfa38916 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java @@ -24,14 +24,27 @@ public SamlLogoutResponseHandler( super(clock, idp, sp, maxSkew); } - public void handle(byte[] payload, Collection allowedSamlRequestIds) { - final Element root = parseSamlMessage(payload); + public void handle(String content, Collection allowedSamlRequestIds) { + final ParsedQueryString parsed = parseQueryStringAndValidateSignature(content, "SAMLResponse"); + final Element root; + if (parsed.hasSignature) { + logger.info("Content is signed at URL level. Proceed as HTTP-Redirect binding"); + root = parseSamlMessage(inflate(decodeBase64(parsed.samlMessage))); + } else { + logger.info("Content is not signed at URL level. Proceed as HTTP-POST binding"); + root = parseSamlMessage(decodeBase64(parsed.samlMessage)); + } + if (LOGOUT_RESPONSE_TAG_NAME.equals(root.getLocalName()) && SAML_NAMESPACE.equals(root.getNamespaceURI())) { final LogoutResponse logoutResponse = buildXmlObject(root, LogoutResponse.class); if (logoutResponse == null) { throw samlException("Cannot convert element {} into LogoutResponse object", root); } - if (logoutResponse.isSigned()) { + if (logoutResponse.getSignature() == null) { + if (parsed.hasSignature == false) { + throw samlException("LogoutResponse is not signed for HTTP-Post binding"); + } + } else { validateSignature(logoutResponse.getSignature()); } checkInResponseTo(logoutResponse, allowedSamlRequestIds); diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlRequestHandler.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlRequestHandler.java index ccd0ec22f20ab..8fddcbe958953 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlRequestHandler.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlRequestHandler.java @@ -13,6 +13,8 @@ import org.elasticsearch.common.Nullable; import org.elasticsearch.common.Strings; import org.elasticsearch.common.unit.TimeValue; +import org.elasticsearch.core.internal.io.Streams; +import org.elasticsearch.rest.RestUtils; import org.elasticsearch.xpack.core.security.support.RestorableContextClassLoader; import org.joda.time.DateTime; import org.opensaml.core.xml.XMLObject; @@ -24,6 +26,7 @@ import org.opensaml.saml.security.impl.SAMLSignatureProfileValidator; import org.opensaml.security.credential.Credential; import org.opensaml.security.x509.X509Credential; +import org.opensaml.xmlsec.crypto.XMLSigningUtil; import org.opensaml.xmlsec.encryption.support.ChainingEncryptedKeyResolver; import org.opensaml.xmlsec.encryption.support.EncryptedKeyResolver; import org.opensaml.xmlsec.encryption.support.InlineEncryptedKeyResolver; @@ -46,7 +49,9 @@ import javax.xml.parsers.DocumentBuilder; import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; import java.io.IOException; +import java.nio.charset.StandardCharsets; import java.security.AccessController; import java.security.PrivilegedActionException; import java.security.PrivilegedExceptionAction; @@ -58,9 +63,13 @@ import java.util.Base64; import java.util.Collection; import java.util.Collections; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.function.Predicate; import java.util.stream.Collectors; +import java.util.zip.Inflater; +import java.util.zip.InflaterInputStream; import static org.elasticsearch.xpack.security.authc.saml.SamlUtils.samlException; import static org.opensaml.core.xml.config.XMLObjectProviderRegistrySupport.getUnmarshallerFactory; @@ -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) { + final String signatureInput = queryString.replaceAll("&Signature=.*$", ""); + final Map parameters = new HashMap<>(); + RestUtils.decodeQueryString(queryString, 0, parameters); + final String samlMessage = parameters.get(samlMessageParameterName); + if (samlMessage == null) { + throw samlException("Could not parse {} from query string: [{}]", samlMessageParameterName, queryString); + } + + final String relayState = parameters.get("RelayState"); + final String signatureAlgorithm = parameters.get("SigAlg"); + final String signature = parameters.get("Signature"); + if (signature == null || signatureAlgorithm == null) { + return new ParsedQueryString(samlMessage, false, relayState); + } + + validateSignature(signatureInput, signatureAlgorithm, signature); + return new ParsedQueryString(samlMessage, true, relayState); + } + + private void validateSignature(String inputString, String signatureAlgorithm, String signature) { + final byte[] sigBytes = decodeBase64(signature); + final byte[] inputBytes = inputString.getBytes(StandardCharsets.US_ASCII); + final String signatureText = Strings.cleanTruncate(signature, 32); + checkIdpSignature(credential -> { + if (XMLSigningUtil.verifyWithURI(credential, signatureAlgorithm, sigBytes, inputBytes)) { + logger.debug(() -> new ParameterizedMessage("SAML Signature [{}] matches credentials [{}] [{}]", + signatureText, credential.getEntityId(), credential.getPublicKey())); + return true; + } else { + logger.debug(() -> new ParameterizedMessage("SAML Signature [{}] failed against credentials [{}] [{}]", + signatureText, credential.getEntityId(), credential.getPublicKey())); + return false; + } + }, signatureText); + } + + protected byte[] decodeBase64(String content) { + try { + return Base64.getDecoder().decode(content.replaceAll("\\s+", "")); + } catch (IllegalArgumentException e) { + logger.info("Failed to decode base64 string [{}] - {}", content, e.toString()); + throw samlException("SAML message cannot be Base64 decoded", e); + } + } + + protected byte[] inflate(byte[] bytes) { + Inflater inflater = new Inflater(true); + try (ByteArrayInputStream in = new ByteArrayInputStream(bytes); + InflaterInputStream inflate = new InflaterInputStream(in, inflater); + ByteArrayOutputStream out = new ByteArrayOutputStream(bytes.length * 3 / 2)) { + Streams.copy(inflate, out); + return out.toByteArray(); + } catch (IOException e) { + throw samlException("SAML message cannot be inflated", e); + } + } + + static class ParsedQueryString { + final String samlMessage; + final boolean hasSignature; + final String relayState; + + ParsedQueryString(String samlMessage, boolean hasSignature, String relayState) { + this.samlMessage = samlMessage; + this.hasSignature = hasSignature; + this.relayState = relayState; + } + } } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlVerifyLogoutAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlVerifyLogoutAction.java index 6ad4f68099cc7..0a16eee4866d3 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlVerifyLogoutAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlVerifyLogoutAction.java @@ -25,7 +25,6 @@ import org.elasticsearch.xpack.core.security.action.saml.SamlVerifyLogoutResponse; import java.io.IOException; -import java.util.Base64; import java.util.List; import static org.elasticsearch.rest.RestRequest.Method.POST; @@ -85,9 +84,9 @@ protected RestChannelConsumer innerPrepareRequest(RestRequest request, NodeClien final RestSamlVerifyLogoutAction.Input input = PARSER.parse(parser, null); logger.trace("SAML LogoutResponse: [{}...] [{}]", Strings.cleanTruncate(input.content, 128), input.ids); return channel -> { - final byte[] bytes = decodeBase64(input.content); final SamlVerifyLogoutRequestBuilder requestBuilder = - new SamlVerifyLogoutRequestBuilder(client).saml(bytes).validRequestIds(input.ids).authenticatingRealm(input.realm); + new SamlVerifyLogoutRequestBuilder(client) + .content(input.content).validRequestIds(input.ids).authenticatingRealm(input.realm); requestBuilder.execute(new RestBuilderListener<>(channel) { @Override public RestResponse buildResponse(SamlVerifyLogoutResponse response, XContentBuilder builder) throws Exception { @@ -99,14 +98,4 @@ public RestResponse buildResponse(SamlVerifyLogoutResponse response, XContentBui }; } } - - private byte[] decodeBase64(String content) { - content = content.replaceAll("\\s+", ""); - try { - return Base64.getDecoder().decode(content); - } catch (IllegalArgumentException e) { - logger.info("Failed to decode base64 string [{}] - {}", content, e.toString()); - throw e; - } - } } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpPostTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpPostTests.java new file mode 100644 index 0000000000000..ad83029008632 --- /dev/null +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpPostTests.java @@ -0,0 +1,87 @@ +/* + * 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.authc.saml; + +import org.elasticsearch.ElasticsearchSecurityException; +import org.elasticsearch.common.unit.TimeValue; +import org.elasticsearch.common.util.NamedFormatter; +import org.elasticsearch.xpack.core.watcher.watch.ClockMock; +import org.junit.Before; + +import java.net.URLEncoder; +import java.nio.charset.StandardCharsets; +import java.util.Base64; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import static java.util.Collections.emptyList; +import static java.util.Collections.emptyMap; +import static org.hamcrest.Matchers.containsString; + +public class SamlLogoutResponseHandlerHttpPostTests extends SamlResponseHandlerTests { + + private SamlLogoutResponseHandler samlLogoutResponseHandler; + + @Before + public void setupHandler() { + clock = new ClockMock(); + maxSkew = TimeValue.timeValueMinutes(1); + requestId = randomId(); + samlLogoutResponseHandler = new SamlLogoutResponseHandler(clock, + getIdpConfiguration(() -> buildOpenSamlCredential(idpSigningCertificatePair)), + getSpConfiguration(emptyList()), + maxSkew); + } + + public void testHandlerWorksWithHttpPostBinding() throws Exception { + final String payload = buildLogoutResponsePayload(emptyMap(), true); + samlLogoutResponseHandler.handle(payload, List.of(requestId)); + } + + public void testHandlerFailsWithHttpPostBindingAndNoSignature() throws Exception { + final String payload = buildLogoutResponsePayload(emptyMap(), false); + final ElasticsearchSecurityException e = expectSamlException(() -> samlLogoutResponseHandler.handle(payload, List.of(requestId))); + assertThat(e.getMessage(), containsString("is not signed")); + } + + public void testHandlerWillThrowWhenStatusIsNotSuccess() throws Exception { + final Map replacements = new HashMap<>(); + replacements.put("status", "urn:oasis:names:tc:SAML:2.0:status:Requester"); + final String payload = buildLogoutResponsePayload(replacements, true); + final ElasticsearchSecurityException e = + expectSamlException(() -> samlLogoutResponseHandler.handle(payload, List.of(requestId))); + assertThat(e.getMessage(), containsString("not a 'success' response")); + } + + private String buildLogoutResponsePayload(Map data, boolean shouldSign) throws Exception { + final String template = "\n" + + "\n" + + " %(IDP_ENTITY_ID)\n" + + " \n" + + " \n" + + " \n" + + ""; + + Map replacements = new HashMap<>(data); + replacements.putIfAbsent("IDP_ENTITY_ID", IDP_ENTITY_ID); + replacements.putIfAbsent("now", clock.instant()); + replacements.putIfAbsent("randomId", requestId); + replacements.putIfAbsent("requestId", requestId); + replacements.putIfAbsent("SP_LOGOUT_URL", SP_LOGOUT_URL); + replacements.putIfAbsent("status", "urn:oasis:names:tc:SAML:2.0:status:Success"); + final String xml = shouldSign + ? signDoc(NamedFormatter.format(template, replacements)) : NamedFormatter.format(template, replacements); + String encoded = URLEncoder.encode(Base64.getEncoder().encodeToString(xml.getBytes(StandardCharsets.UTF_8)), + StandardCharsets.US_ASCII.name()); + return String.format("SAMLResponse=%s", encoded); + } +} diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpRedirectTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpRedirectTests.java new file mode 100644 index 0000000000000..d47fb0cafe50d --- /dev/null +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpRedirectTests.java @@ -0,0 +1,132 @@ +/* + * 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.authc.saml; + +import org.elasticsearch.ElasticsearchSecurityException; +import org.elasticsearch.common.unit.TimeValue; +import org.elasticsearch.common.util.set.Sets; +import org.joda.time.DateTime; +import org.junit.AfterClass; +import org.junit.Before; +import org.junit.BeforeClass; +import org.opensaml.saml.saml2.core.Issuer; +import org.opensaml.saml.saml2.core.LogoutResponse; +import org.opensaml.saml.saml2.core.Status; +import org.opensaml.saml.saml2.core.StatusCode; +import org.opensaml.saml.saml2.core.impl.StatusBuilder; +import org.opensaml.saml.saml2.core.impl.StatusCodeBuilder; +import org.opensaml.security.x509.X509Credential; + +import java.net.URI; +import java.net.URISyntaxException; +import java.time.Clock; +import java.util.Collections; +import java.util.List; + +import static org.hamcrest.Matchers.containsString; + +public class SamlLogoutResponseHandlerHttpRedirectTests extends SamlTestCase { + + private static final String IDP_ENTITY_ID = "https://idp.test/"; + private static final String LOGOUT_URL = "https://sp.test/saml/logout"; + + private Clock clock; + private SamlLogoutResponseHandler samlLogoutResponseHandler; + + private static X509Credential credential; + + @BeforeClass + public static void setupCredential() throws Exception { + credential = (X509Credential) buildOpenSamlCredential(readRandomKeyPair()).get(0); + } + + @AfterClass + public static void clearCredential() { + credential = null; + } + + @Before + public void setupHandler() throws Exception { + clock = Clock.systemUTC(); + final IdpConfiguration idp = new IdpConfiguration(IDP_ENTITY_ID, () -> Collections.singletonList(credential)); + final X509Credential spCredential = (X509Credential) buildOpenSamlCredential(readRandomKeyPair()).get(0); + final SigningConfiguration signingConfiguration = new SigningConfiguration(Collections.singleton("*"), spCredential); + final SpConfiguration sp = new SpConfiguration( + "https://sp.test/", + "https://sp.test/saml/asc", + LOGOUT_URL, + signingConfiguration, + List.of(spCredential), + Collections.emptyList()); + samlLogoutResponseHandler = new SamlLogoutResponseHandler(clock, idp, sp, TimeValue.timeValueSeconds(1)); + } + + public void testHandlerWorks() throws URISyntaxException { + final String requestId = SamlUtils.generateSecureNCName(randomIntBetween(8, 30)); + final SigningConfiguration signingConfiguration = new SigningConfiguration(Sets.newHashSet("*"), credential); + final LogoutResponse logoutResponse = SamlUtils.buildObject(LogoutResponse.class, LogoutResponse.DEFAULT_ELEMENT_NAME); + logoutResponse.setDestination(LOGOUT_URL); + logoutResponse.setIssueInstant(new DateTime(clock.millis())); + logoutResponse.setID(SamlUtils.generateSecureNCName(randomIntBetween(8, 30))); + logoutResponse.setInResponseTo(requestId); + logoutResponse.setStatus(buildStatus(StatusCode.SUCCESS)); + + final Issuer issuer = SamlUtils.buildObject(Issuer.class, Issuer.DEFAULT_ELEMENT_NAME); + issuer.setValue(IDP_ENTITY_ID); + logoutResponse.setIssuer(issuer); + final String url = new SamlRedirect(logoutResponse, signingConfiguration).getRedirectUrl(); + samlLogoutResponseHandler.handle(new URI(url).getRawQuery(), List.of(requestId)); + } + + public void testHandlerFailsIfStatusIsNotSuccess() { + final String requestId = SamlUtils.generateSecureNCName(randomIntBetween(8, 30)); + final SigningConfiguration signingConfiguration = new SigningConfiguration(Sets.newHashSet("*"), credential); + final LogoutResponse logoutResponse = SamlUtils.buildObject(LogoutResponse.class, LogoutResponse.DEFAULT_ELEMENT_NAME); + logoutResponse.setDestination(LOGOUT_URL); + logoutResponse.setIssueInstant(new DateTime(clock.millis())); + logoutResponse.setID(SamlUtils.generateSecureNCName(randomIntBetween(8, 30))); + logoutResponse.setInResponseTo(requestId); + logoutResponse.setStatus(buildStatus(randomFrom(StatusCode.REQUESTER, StatusCode.RESPONDER))); + + final Issuer issuer = SamlUtils.buildObject(Issuer.class, Issuer.DEFAULT_ELEMENT_NAME); + issuer.setValue(IDP_ENTITY_ID); + logoutResponse.setIssuer(issuer); + 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("is not a 'success' response")); + } + + public void testHandlerWillUseHttpPostBindingWhenUrlNotSigned() { + final String requestId = SamlUtils.generateSecureNCName(randomIntBetween(8, 30)); + final SigningConfiguration signingConfiguration = new SigningConfiguration(Sets.newHashSet("*"), null); + final LogoutResponse logoutResponse = SamlUtils.buildObject(LogoutResponse.class, LogoutResponse.DEFAULT_ELEMENT_NAME); + logoutResponse.setDestination(LOGOUT_URL); + logoutResponse.setIssueInstant(new DateTime(clock.millis())); + logoutResponse.setID(SamlUtils.generateSecureNCName(randomIntBetween(8, 30))); + logoutResponse.setInResponseTo(requestId); + logoutResponse.setStatus(buildStatus(randomFrom(StatusCode.REQUESTER, StatusCode.RESPONDER))); + + final Issuer issuer = SamlUtils.buildObject(Issuer.class, Issuer.DEFAULT_ELEMENT_NAME); + issuer.setValue(IDP_ENTITY_ID); + logoutResponse.setIssuer(issuer); + 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")); + } + + private Status buildStatus(String statusCodeValue) { + final Status status = new StatusBuilder().buildObject(); + final StatusCode statusCode = new StatusCodeBuilder().buildObject(); + statusCode.setValue(statusCodeValue); + status.setStatusCode(statusCode); + return status; + } + +} diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerTests.java deleted file mode 100644 index 0fe9caee14f91..0000000000000 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerTests.java +++ /dev/null @@ -1,112 +0,0 @@ -/* - * 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.authc.saml; - -import org.elasticsearch.ElasticsearchSecurityException; -import org.elasticsearch.common.unit.TimeValue; -import org.elasticsearch.common.util.NamedFormatter; -import org.elasticsearch.xpack.core.watcher.watch.ClockMock; -import org.junit.Before; - -import java.nio.charset.StandardCharsets; -import java.util.HashMap; -import java.util.List; -import java.util.Map; - -import static java.util.Collections.emptyList; -import static org.hamcrest.Matchers.containsString; - -public class SamlLogoutResponseHandlerTests extends SamlResponseHandlerTests { - - private SamlLogoutResponseHandler samlLogoutResponseHandler; - - @Before - public void setupHandler() { - clock = new ClockMock(); - maxSkew = TimeValue.timeValueMinutes(1); - requestId = randomId(); - samlLogoutResponseHandler = new SamlLogoutResponseHandler( - clock, - getIdpConfiguration(() -> buildOpenSamlCredential(idpSigningCertificatePair)), - getSpConfiguration(emptyList()), - maxSkew); - } - - public void testHandleWorksWithoutSignature() { - final String xml = "\n" - + "\n" - + " %(IDP_ENTITY_ID)\n" - + " \n" - + " \n" - + " \n" - + ""; - - final Map replacements = new HashMap<>(); - replacements.put("IDP_ENTITY_ID", IDP_ENTITY_ID); - replacements.put("now", clock.instant()); - replacements.put("randomId", requestId); - replacements.put("requestId", requestId); - replacements.put("SP_LOGOUT_URL", SP_LOGOUT_URL); - - final String payload = NamedFormatter.format(xml, replacements); - samlLogoutResponseHandler.handle(payload.getBytes(StandardCharsets.UTF_8), List.of(requestId)); - } - - public void testHandleWorksWithSignature() throws Exception { - final String xml = "\n" - + "\n" - + " %(IDP_ENTITY_ID)\n" - + " \n" - + " \n" - + " \n" - + ""; - - final Map replacements = new HashMap<>(); - replacements.put("IDP_ENTITY_ID", IDP_ENTITY_ID); - replacements.put("now", clock.instant()); - replacements.put("randomId", requestId); - replacements.put("requestId", requestId); - replacements.put("SP_LOGOUT_URL", SP_LOGOUT_URL); - - final String payload = signDoc(NamedFormatter.format(xml, replacements)); - samlLogoutResponseHandler.handle(payload.getBytes(StandardCharsets.UTF_8), List.of(requestId)); - } - - public void testHandleWillThrowWhenStatusIsNotSuccess() { - final String xml = "\n" - + "\n" - + " %(IDP_ENTITY_ID)\n" - + " \n" - + " \n" - + " \n" - + ""; - - final Map replacements = new HashMap<>(); - replacements.put("IDP_ENTITY_ID", IDP_ENTITY_ID); - replacements.put("now", clock.instant()); - replacements.put("randomId", requestId); - replacements.put("requestId", requestId); - replacements.put("SP_LOGOUT_URL", SP_LOGOUT_URL); - - final String payload = NamedFormatter.format(xml, replacements); - final ElasticsearchSecurityException e = expectSamlException(() -> - samlLogoutResponseHandler.handle(payload.getBytes(StandardCharsets.UTF_8), List.of(requestId))); - assertThat(e.getMessage(), containsString("not a 'success' response")); - } -} From bcaf87da675548fef2357052f0b9baf8e413408a Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Mon, 11 May 2020 15:19:18 +1000 Subject: [PATCH 07/22] Rename and checkstyle --- .../authc/saml/SamlLogoutRequestHandler.java | 15 +-------------- ...RequestHandler.java => SamlObjectHandler.java} | 4 ++-- .../security/authc/saml/SamlResponseHandler.java | 2 +- ...dlerTests.java => SamlObjectHandlerTests.java} | 8 ++++---- 4 files changed, 8 insertions(+), 21 deletions(-) rename x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/{SamlRequestHandler.java => SamlObjectHandler.java} (99%) rename x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/{SamlRequestHandlerTests.java => SamlObjectHandlerTests.java} (85%) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutRequestHandler.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutRequestHandler.java index 30d43774bb990..97f406b56a2b9 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutRequestHandler.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutRequestHandler.java @@ -5,29 +5,16 @@ */ package org.elasticsearch.xpack.security.authc.saml; -import java.io.ByteArrayInputStream; -import java.io.ByteArrayOutputStream; -import java.io.IOException; -import java.nio.charset.StandardCharsets; import java.time.Clock; -import java.util.Base64; -import java.util.HashMap; -import java.util.Map; import java.util.Objects; -import java.util.zip.Inflater; -import java.util.zip.InflaterInputStream; import org.apache.logging.log4j.message.ParameterizedMessage; import org.elasticsearch.ElasticsearchSecurityException; -import org.elasticsearch.common.Strings; import org.elasticsearch.common.unit.TimeValue; -import org.elasticsearch.core.internal.io.Streams; -import org.elasticsearch.rest.RestUtils; import org.opensaml.saml.common.SAMLObject; import org.opensaml.saml.saml2.core.EncryptedID; import org.opensaml.saml.saml2.core.LogoutRequest; import org.opensaml.saml.saml2.core.NameID; -import org.opensaml.xmlsec.crypto.XMLSigningUtil; import org.opensaml.xmlsec.encryption.support.DecryptionException; import org.opensaml.xmlsec.signature.Signature; import org.w3c.dom.Element; @@ -37,7 +24,7 @@ /** * Processes a LogoutRequest for an IdP-initiated logout. */ -public class SamlLogoutRequestHandler extends SamlRequestHandler { +public class SamlLogoutRequestHandler extends SamlObjectHandler { private static final String REQUEST_TAG_NAME = "LogoutRequest"; diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlRequestHandler.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlObjectHandler.java similarity index 99% rename from x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlRequestHandler.java rename to x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlObjectHandler.java index 8fddcbe958953..4f68d5e380a53 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlRequestHandler.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlObjectHandler.java @@ -74,7 +74,7 @@ import static org.elasticsearch.xpack.security.authc.saml.SamlUtils.samlException; import static org.opensaml.core.xml.config.XMLObjectProviderRegistrySupport.getUnmarshallerFactory; -public class SamlRequestHandler { +public class SamlObjectHandler { protected static final String SAML_NAMESPACE = "urn:oasis:names:tc:SAML:2.0:protocol"; @@ -102,7 +102,7 @@ public class SamlRequestHandler { private final TimeValue maxSkew; private final UnmarshallerFactory unmarshallerFactory; - public SamlRequestHandler(Clock clock, IdpConfiguration idp, SpConfiguration sp, TimeValue maxSkew) { + public SamlObjectHandler(Clock clock, IdpConfiguration idp, SpConfiguration sp, TimeValue maxSkew) { this.clock = clock; this.idp = idp; this.sp = sp; diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlResponseHandler.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlResponseHandler.java index ebbfb4f6f9ef7..8508218368c69 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlResponseHandler.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlResponseHandler.java @@ -19,7 +19,7 @@ import static org.elasticsearch.xpack.security.authc.saml.SamlUtils.samlException; -public class SamlResponseHandler extends SamlRequestHandler { +public class SamlResponseHandler extends SamlObjectHandler { public SamlResponseHandler(Clock clock, IdpConfiguration idp, SpConfiguration sp, TimeValue maxSkew) { super(clock, idp, sp, maxSkew); } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlRequestHandlerTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlObjectHandlerTests.java similarity index 85% rename from x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlRequestHandlerTests.java rename to x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlObjectHandlerTests.java index b5391ba861d0f..2eee62dabe335 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlRequestHandlerTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlObjectHandlerTests.java @@ -14,7 +14,7 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; -public class SamlRequestHandlerTests extends ESTestCase { +public class SamlObjectHandlerTests extends ESTestCase { public void testXmlObjectToTextWhenExceedsLength() { final int prefixLength = randomIntBetween(10, 30); @@ -28,7 +28,7 @@ public void testXmlObjectToTextWhenExceedsLength() { when(xml.getDOM()).thenReturn(element); when(element.getTextContent()).thenReturn(text); - assertThat(SamlRequestHandler.text(xml, prefixLength, suffixLength), equalTo(prefix + "..." + suffix)); + assertThat(SamlObjectHandler.text(xml, prefixLength, suffixLength), equalTo(prefix + "..." + suffix)); } public void testXmlObjectToTextPrefixOnly() { @@ -41,7 +41,7 @@ public void testXmlObjectToTextPrefixOnly() { when(xml.getDOM()).thenReturn(element); when(element.getTextContent()).thenReturn(text); - assertThat(SamlRequestHandler.text(xml, length, 0), equalTo(prefix + "...")); + assertThat(SamlObjectHandler.text(xml, length, 0), equalTo(prefix + "...")); } public void testXmlObjectToTextWhenShortedThanRequiredLength() { @@ -54,7 +54,7 @@ public void testXmlObjectToTextWhenShortedThanRequiredLength() { when(xml.getDOM()).thenReturn(element); when(element.getTextContent()).thenReturn(text); - assertThat(SamlRequestHandler.text(xml, prefixLength, suffixLength), equalTo(text)); + assertThat(SamlObjectHandler.text(xml, prefixLength, suffixLength), equalTo(text)); } } From 824b5608185c23acbae62374e0d060e6df500696 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Mon, 11 May 2020 15:33:41 +1000 Subject: [PATCH 08/22] Forbidden API --- .../authc/saml/SamlLogoutResponseHandlerHttpPostTests.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpPostTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpPostTests.java index ad83029008632..6987bbf2d4be4 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpPostTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpPostTests.java @@ -17,6 +17,7 @@ import java.util.Base64; import java.util.HashMap; import java.util.List; +import java.util.Locale; import java.util.Map; import static java.util.Collections.emptyList; @@ -82,6 +83,6 @@ private String buildLogoutResponsePayload(Map data, boolean shou ? signDoc(NamedFormatter.format(template, replacements)) : NamedFormatter.format(template, replacements); String encoded = URLEncoder.encode(Base64.getEncoder().encodeToString(xml.getBytes(StandardCharsets.UTF_8)), StandardCharsets.US_ASCII.name()); - return String.format("SAMLResponse=%s", encoded); + return String.format(Locale.ROOT, "SAMLResponse=%s", encoded); } } From 41494c4192829e70803de7c21c376d5ceb0bf958 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Mon, 18 May 2020 10:59:14 +1000 Subject: [PATCH 09/22] Address feedback --- ...ion.java => SamlCompleteLogoutAction.java} | 12 +++--- ...st.java => SamlCompleteLogoutRequest.java} | 18 ++------- ... => SamlCompleteLogoutRequestBuilder.java} | 16 ++++---- ...e.java => SamlCompleteLogoutResponse.java} | 8 ++-- .../xpack/security/Security.java | 10 ++--- ...=> TransportSamlCompleteLogoutAction.java} | 24 ++++++------ .../authc/saml/SamlAuthenticator.java | 6 ++- ...java => RestSamlCompleteLogoutAction.java} | 38 ++++++++----------- 8 files changed, 60 insertions(+), 72 deletions(-) rename x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/{SamlVerifyLogoutAction.java => SamlCompleteLogoutAction.java} (54%) rename x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/{SamlVerifyLogoutRequest.java => SamlCompleteLogoutRequest.java} (69%) rename x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/{SamlVerifyLogoutRequestBuilder.java => SamlCompleteLogoutRequestBuilder.java} (51%) rename x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/{SamlVerifyLogoutResponse.java => SamlCompleteLogoutResponse.java} (71%) rename x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/{TransportSamlVerifyLogoutAction.java => TransportSamlCompleteLogoutAction.java} (64%) rename x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/{RestSamlVerifyLogoutAction.java => RestSamlCompleteLogoutAction.java} (60%) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutAction.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutAction.java similarity index 54% rename from x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutAction.java rename to x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutAction.java index e327ba12f27f7..c0e95bd100a12 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutAction.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutAction.java @@ -8,14 +8,14 @@ import org.elasticsearch.action.ActionType; /** - * ActionType for verifying SAML LogoutResponse + * ActionType for completing SAML LogoutResponse */ -public final class SamlVerifyLogoutAction extends ActionType { +public final class SamlCompleteLogoutAction extends ActionType { - public static final String NAME = "cluster:admin/xpack/security/saml/verify_logout"; - public static final SamlVerifyLogoutAction INSTANCE = new SamlVerifyLogoutAction(); + public static final String NAME = "cluster:admin/xpack/security/saml/complete_logout"; + public static final SamlCompleteLogoutAction INSTANCE = new SamlCompleteLogoutAction(); - private SamlVerifyLogoutAction() { - super(NAME, SamlVerifyLogoutResponse::new); + private SamlCompleteLogoutAction() { + super(NAME, SamlCompleteLogoutResponse::new); } } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutRequest.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutRequest.java similarity index 69% rename from x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutRequest.java rename to x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutRequest.java index cdbd5c9fc5e2e..93941ce8537d0 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutRequest.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutRequest.java @@ -14,22 +14,20 @@ import java.util.List; /** - * Represents a request to verify SAML LogoutResponse + * Represents a request to complete SAML LogoutResponse */ -public final class SamlVerifyLogoutRequest extends ActionRequest { +public final class SamlCompleteLogoutRequest extends ActionRequest { private String content; private List validRequestIds; @Nullable private String realm; - @Nullable - private String assertionConsumerServiceURL; - public SamlVerifyLogoutRequest(StreamInput in) throws IOException { + public SamlCompleteLogoutRequest(StreamInput in) throws IOException { super(in); } - public SamlVerifyLogoutRequest() { + public SamlCompleteLogoutRequest() { } @Override @@ -60,12 +58,4 @@ public String getRealm() { public void setRealm(String realm) { this.realm = realm; } - - public String getAssertionConsumerServiceURL() { - return assertionConsumerServiceURL; - } - - public void setAssertionConsumerServiceURL(String assertionConsumerServiceURL) { - this.assertionConsumerServiceURL = assertionConsumerServiceURL; - } } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutRequestBuilder.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutRequestBuilder.java similarity index 51% rename from x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutRequestBuilder.java rename to x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutRequestBuilder.java index 22d8b2bbf021a..9d08dcd6fb988 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutRequestBuilder.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutRequestBuilder.java @@ -11,26 +11,26 @@ import java.util.List; /** - * Request builder used to populate a {@link SamlVerifyLogoutRequest} + * Request builder used to populate a {@link SamlCompleteLogoutRequest} */ -public final class SamlVerifyLogoutRequestBuilder - extends ActionRequestBuilder { +public final class SamlCompleteLogoutRequestBuilder + extends ActionRequestBuilder { - public SamlVerifyLogoutRequestBuilder(ElasticsearchClient client) { - super(client, SamlVerifyLogoutAction.INSTANCE, new SamlVerifyLogoutRequest()); + public SamlCompleteLogoutRequestBuilder(ElasticsearchClient client) { + super(client, SamlCompleteLogoutAction.INSTANCE, new SamlCompleteLogoutRequest()); } - public SamlVerifyLogoutRequestBuilder content(String content) { + public SamlCompleteLogoutRequestBuilder content(String content) { request.setContent(content); return this; } - public SamlVerifyLogoutRequestBuilder validRequestIds(List validRequestIds) { + public SamlCompleteLogoutRequestBuilder validRequestIds(List validRequestIds) { request.setValidRequestIds(validRequestIds); return this; } - public SamlVerifyLogoutRequestBuilder authenticatingRealm(String realm) { + public SamlCompleteLogoutRequestBuilder authenticatingRealm(String realm) { request.setRealm(realm); return this; } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutResponse.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutResponse.java similarity index 71% rename from x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutResponse.java rename to x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutResponse.java index b7038d5099951..cc38b17a1dd20 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlVerifyLogoutResponse.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutResponse.java @@ -12,15 +12,15 @@ import java.io.IOException; /** - * The verification response to the LogoutResponse from idP + * A response to complete the LogoutResponse from idP */ -public final class SamlVerifyLogoutResponse extends ActionResponse { +public final class SamlCompleteLogoutResponse extends ActionResponse { - public SamlVerifyLogoutResponse(StreamInput in) throws IOException { + public SamlCompleteLogoutResponse(StreamInput in) throws IOException { super(in); } - public SamlVerifyLogoutResponse() { + public SamlCompleteLogoutResponse() { } @Override diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java index 138a1a0a7714f..e00fa9ba39304 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java @@ -104,7 +104,7 @@ import org.elasticsearch.xpack.core.security.action.saml.SamlAuthenticateAction; import org.elasticsearch.xpack.core.security.action.saml.SamlInvalidateSessionAction; import org.elasticsearch.xpack.core.security.action.saml.SamlLogoutAction; -import org.elasticsearch.xpack.core.security.action.saml.SamlVerifyLogoutAction; +import org.elasticsearch.xpack.core.security.action.saml.SamlCompleteLogoutAction; import org.elasticsearch.xpack.core.security.action.saml.SamlPrepareAuthenticationAction; import org.elasticsearch.xpack.core.security.action.token.CreateTokenAction; import org.elasticsearch.xpack.core.security.action.token.InvalidateTokenAction; @@ -168,7 +168,7 @@ import org.elasticsearch.xpack.security.action.saml.TransportSamlAuthenticateAction; import org.elasticsearch.xpack.security.action.saml.TransportSamlInvalidateSessionAction; import org.elasticsearch.xpack.security.action.saml.TransportSamlLogoutAction; -import org.elasticsearch.xpack.security.action.saml.TransportSamlVerifyLogoutAction; +import org.elasticsearch.xpack.security.action.saml.TransportSamlCompleteLogoutAction; import org.elasticsearch.xpack.security.action.saml.TransportSamlPrepareAuthenticationAction; import org.elasticsearch.xpack.security.action.token.TransportCreateTokenAction; import org.elasticsearch.xpack.security.action.token.TransportInvalidateTokenAction; @@ -235,7 +235,7 @@ import org.elasticsearch.xpack.security.rest.action.saml.RestSamlAuthenticateAction; import org.elasticsearch.xpack.security.rest.action.saml.RestSamlInvalidateSessionAction; import org.elasticsearch.xpack.security.rest.action.saml.RestSamlLogoutAction; -import org.elasticsearch.xpack.security.rest.action.saml.RestSamlVerifyLogoutAction; +import org.elasticsearch.xpack.security.rest.action.saml.RestSamlCompleteLogoutAction; import org.elasticsearch.xpack.security.rest.action.saml.RestSamlPrepareAuthenticationAction; import org.elasticsearch.xpack.security.rest.action.user.RestChangePasswordAction; import org.elasticsearch.xpack.security.rest.action.user.RestDeleteUserAction; @@ -754,7 +754,7 @@ public void onIndexModule(IndexModule module) { new ActionHandler<>(SamlAuthenticateAction.INSTANCE, TransportSamlAuthenticateAction.class), new ActionHandler<>(SamlLogoutAction.INSTANCE, TransportSamlLogoutAction.class), new ActionHandler<>(SamlInvalidateSessionAction.INSTANCE, TransportSamlInvalidateSessionAction.class), - new ActionHandler<>(SamlVerifyLogoutAction.INSTANCE, TransportSamlVerifyLogoutAction.class), + new ActionHandler<>(SamlCompleteLogoutAction.INSTANCE, TransportSamlCompleteLogoutAction.class), new ActionHandler<>(OpenIdConnectPrepareAuthenticationAction.INSTANCE, TransportOpenIdConnectPrepareAuthenticationAction.class), new ActionHandler<>(OpenIdConnectAuthenticateAction.INSTANCE, TransportOpenIdConnectAuthenticateAction.class), @@ -812,7 +812,7 @@ public List getRestHandlers(Settings settings, RestController restC new RestSamlAuthenticateAction(settings, getLicenseState()), new RestSamlLogoutAction(settings, getLicenseState()), new RestSamlInvalidateSessionAction(settings, getLicenseState()), - new RestSamlVerifyLogoutAction(settings, getLicenseState()), + new RestSamlCompleteLogoutAction(settings, getLicenseState()), new RestOpenIdConnectPrepareAuthenticationAction(settings, getLicenseState()), new RestOpenIdConnectAuthenticateAction(settings, getLicenseState()), new RestOpenIdConnectLogoutAction(settings, getLicenseState()), diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlVerifyLogoutAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlCompleteLogoutAction.java similarity index 64% rename from x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlVerifyLogoutAction.java rename to x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlCompleteLogoutAction.java index f057f0952f620..c752b8191d5e3 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlVerifyLogoutAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlCompleteLogoutAction.java @@ -11,9 +11,9 @@ import org.elasticsearch.common.inject.Inject; import org.elasticsearch.tasks.Task; import org.elasticsearch.transport.TransportService; -import org.elasticsearch.xpack.core.security.action.saml.SamlVerifyLogoutAction; -import org.elasticsearch.xpack.core.security.action.saml.SamlVerifyLogoutRequest; -import org.elasticsearch.xpack.core.security.action.saml.SamlVerifyLogoutResponse; +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; @@ -24,21 +24,21 @@ import static org.elasticsearch.xpack.security.authc.saml.SamlRealm.findSamlRealms; /** - * Transport action responsible for verifying SAML LogoutResponse + * Transport action responsible for completing SAML LogoutResponse */ -public final class TransportSamlVerifyLogoutAction extends HandledTransportAction { +public final class TransportSamlCompleteLogoutAction extends HandledTransportAction { private final Realms realms; @Inject - public TransportSamlVerifyLogoutAction(TransportService transportService, ActionFilters actionFilters, Realms realms) { - super(SamlVerifyLogoutAction.NAME, transportService, actionFilters, SamlVerifyLogoutRequest::new); + public TransportSamlCompleteLogoutAction(TransportService transportService, ActionFilters actionFilters, Realms realms) { + super(SamlCompleteLogoutAction.NAME, transportService, actionFilters, SamlCompleteLogoutRequest::new); this.realms = realms; } @Override - protected void doExecute(Task task, SamlVerifyLogoutRequest request, ActionListener listener) { - List realms = findSamlRealms(this.realms, request.getRealm(), request.getAssertionConsumerServiceURL()); + protected void doExecute(Task task, SamlCompleteLogoutRequest request, ActionListener listener) { + List realms = findSamlRealms(this.realms, request.getRealm(), null); if (realms.isEmpty()) { listener.onFailure(SamlUtils.samlException("Cannot find any matching realm for [{}]", request)); } else if (realms.size() > 1) { @@ -48,13 +48,13 @@ protected void doExecute(Task task, SamlVerifyLogoutRequest request, ActionListe } } - private void processLogoutResponse(SamlRealm samlRealm, SamlVerifyLogoutRequest request, - ActionListener listener) { + private void processLogoutResponse(SamlRealm samlRealm, SamlCompleteLogoutRequest request, + ActionListener listener) { final SamlLogoutResponseHandler logoutResponseHandler = samlRealm.getLogoutResponseHandler(); try { logoutResponseHandler.handle(request.getContent(), request.getValidRequestIds()); - listener.onResponse(new SamlVerifyLogoutResponse()); + listener.onResponse(new SamlCompleteLogoutResponse()); } catch (Exception e) { listener.onFailure(e); } 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 e676698b2b542..fab8b265a43b6 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 @@ -277,7 +277,11 @@ private void checkSubject(Subject assertionSubject, XMLObject parent, Collection } checkRecipient(confirmationData.get(0)); checkLifetimeRestrictions(confirmationData.get(0)); - SubjectConfirmationData subjectConfirmationData = confirmationData.get(0); + checkSubjectInResponseTo(confirmationData.get(0), allowedSamlRequestIds); + } + + private void checkSubjectInResponseTo( + SubjectConfirmationData subjectConfirmationData, Collection allowedSamlRequestIds) { // Allow for IdP initiated SSO where InResponseTo MUST be missing if (Strings.hasText(subjectConfirmationData.getInResponseTo()) && allowedSamlRequestIds.contains(subjectConfirmationData.getInResponseTo()) == false) { diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlVerifyLogoutAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlCompleteLogoutAction.java similarity index 60% rename from x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlVerifyLogoutAction.java rename to x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlCompleteLogoutAction.java index 0a16eee4866d3..ff7fd60823dd5 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlVerifyLogoutAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlCompleteLogoutAction.java @@ -21,23 +21,22 @@ import org.elasticsearch.rest.RestResponse; import org.elasticsearch.rest.RestStatus; import org.elasticsearch.rest.action.RestBuilderListener; -import org.elasticsearch.xpack.core.security.action.saml.SamlVerifyLogoutRequestBuilder; -import org.elasticsearch.xpack.core.security.action.saml.SamlVerifyLogoutResponse; +import org.elasticsearch.xpack.core.security.action.saml.SamlCompleteLogoutRequestBuilder; +import org.elasticsearch.xpack.core.security.action.saml.SamlCompleteLogoutResponse; import java.io.IOException; import java.util.List; import static org.elasticsearch.rest.RestRequest.Method.POST; -public class RestSamlVerifyLogoutAction extends SamlBaseRestHandler{ +public class RestSamlCompleteLogoutAction extends SamlBaseRestHandler{ - private static final Logger logger = LogManager.getLogger(RestSamlVerifyLogoutAction.class); + private static final Logger logger = LogManager.getLogger(RestSamlCompleteLogoutAction.class); static class Input { String content; List ids; String realm; - String assertionConsumerServiceURL; void setContent(String content) { this.content = content; @@ -48,48 +47,43 @@ void setIds(List ids) { } void setRealm(String realm) { this.realm = realm;} - - void setAssertionConsumerServiceURL(String assertionConsumerServiceURL) { - this.assertionConsumerServiceURL = assertionConsumerServiceURL; - } } - static final ObjectParser - PARSER = new ObjectParser<>("saml_verify_logout", RestSamlVerifyLogoutAction.Input::new); + static final ObjectParser + PARSER = new ObjectParser<>("saml_complete_logout", RestSamlCompleteLogoutAction.Input::new); static { - PARSER.declareString(RestSamlVerifyLogoutAction.Input::setContent, new ParseField("content")); - PARSER.declareStringArray(RestSamlVerifyLogoutAction.Input::setIds, new ParseField("ids")); - PARSER.declareStringOrNull(RestSamlVerifyLogoutAction.Input::setRealm, new ParseField("realm")); - PARSER.declareString(RestSamlVerifyLogoutAction.Input::setAssertionConsumerServiceURL, new ParseField("acs")); + PARSER.declareString(RestSamlCompleteLogoutAction.Input::setContent, new ParseField("content")); + PARSER.declareStringArray(RestSamlCompleteLogoutAction.Input::setIds, new ParseField("ids")); + PARSER.declareString(RestSamlCompleteLogoutAction.Input::setRealm, new ParseField("realm")); } - public RestSamlVerifyLogoutAction(Settings settings, XPackLicenseState licenseState) { + public RestSamlCompleteLogoutAction(Settings settings, XPackLicenseState licenseState) { super(settings, licenseState); } @Override public String getName() { - return "security_saml_verify_logout_action"; + return "security_saml_complete_logout_action"; } @Override public List routes() { - return List.of(new Route(POST, "/_security/saml/verify_logout")); + return List.of(new Route(POST, "/_security/saml/complete_logout")); } @Override protected RestChannelConsumer innerPrepareRequest(RestRequest request, NodeClient client) throws IOException { try (XContentParser parser = request.contentParser()) { - final RestSamlVerifyLogoutAction.Input input = PARSER.parse(parser, null); + final RestSamlCompleteLogoutAction.Input input = PARSER.parse(parser, null); logger.trace("SAML LogoutResponse: [{}...] [{}]", Strings.cleanTruncate(input.content, 128), input.ids); return channel -> { - final SamlVerifyLogoutRequestBuilder requestBuilder = - new SamlVerifyLogoutRequestBuilder(client) + final SamlCompleteLogoutRequestBuilder requestBuilder = + new SamlCompleteLogoutRequestBuilder(client) .content(input.content).validRequestIds(input.ids).authenticatingRealm(input.realm); requestBuilder.execute(new RestBuilderListener<>(channel) { @Override - public RestResponse buildResponse(SamlVerifyLogoutResponse response, XContentBuilder builder) throws Exception { + public RestResponse buildResponse(SamlCompleteLogoutResponse response, XContentBuilder builder) throws Exception { builder.startObject() .endObject(); return new BytesRestResponse(RestStatus.OK, builder); From c0e4089adb6dac6851b8e518b6b480efebf91d52 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Mon, 18 May 2020 12:03:52 +1000 Subject: [PATCH 10/22] Address feedback for removing auto-detect of redirect and post --- .../saml/SamlCompleteLogoutRequest.java | 28 ++++++++++++++--- .../SamlCompleteLogoutRequestBuilder.java | 5 +++ .../saml/SamlCompleteLogoutRequestTests.java | 31 +++++++++++++++++++ .../TransportSamlCompleteLogoutAction.java | 2 +- .../authc/saml/SamlLogoutResponseHandler.java | 24 ++++++++------ .../saml/RestSamlCompleteLogoutAction.java | 14 +++++++-- ...amlLogoutResponseHandlerHttpPostTests.java | 6 ++-- ...ogoutResponseHandlerHttpRedirectTests.java | 29 ++++++++++++++--- 8 files changed, 112 insertions(+), 27 deletions(-) create mode 100644 x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutRequestTests.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 93941ce8537d0..b539dcc8ae7e0 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 @@ -13,11 +13,14 @@ 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 { + private String queryString; private String content; private List validRequestIds; @Nullable @@ -32,17 +35,24 @@ public SamlCompleteLogoutRequest() { @Override public ActionRequestValidationException validate() { - return null; - } - - public String getContent() { - return content; + ActionRequestValidationException validationException = null; + if (queryString == null && content == null) { + validationException = addValidationError("queryString and content may not both be null", validationException); + } + if (queryString != null && content != null) { + validationException = addValidationError("queryString and content may not both present", validationException); + } + return validationException; } public void setContent(String content) { this.content = content; } + public void setQueryString(String queryString) { + this.queryString = queryString; + } + public List getValidRequestIds() { return validRequestIds; } @@ -58,4 +68,12 @@ public String getRealm() { public void setRealm(String realm) { this.realm = realm; } + + public boolean isHttpRedirect() { + return queryString != null; + } + + public String getPayload() { + return isHttpRedirect() ? queryString : content; + } } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutRequestBuilder.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutRequestBuilder.java index 9d08dcd6fb988..0270e5b6e5986 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutRequestBuilder.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutRequestBuilder.java @@ -20,6 +20,11 @@ public SamlCompleteLogoutRequestBuilder(ElasticsearchClient client) { super(client, SamlCompleteLogoutAction.INSTANCE, new SamlCompleteLogoutRequest()); } + public SamlCompleteLogoutRequestBuilder queryString(String queryString) { + request.setQueryString(queryString); + return this; + } + public SamlCompleteLogoutRequestBuilder content(String content) { request.setContent(content); return this; 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 new file mode 100644 index 0000000000000..098560af1ed44 --- /dev/null +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutRequestTests.java @@ -0,0 +1,31 @@ +/* + * 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 junit.framework.TestCase; +import org.elasticsearch.action.ActionRequestValidationException; + +import static org.hamcrest.Matchers.containsString; +import static org.junit.Assert.assertThat; + +public class SamlCompleteLogoutRequestTests extends TestCase { + + public void testValidateFailsWhenQueryAndBodyBothNotExist() { + final SamlCompleteLogoutRequest samlCompleteLogoutRequest = new SamlCompleteLogoutRequest(); + final ActionRequestValidationException validationException = samlCompleteLogoutRequest.validate(); + assertThat(validationException.getMessage(), containsString("queryString and content may not both be null")); + } + + public void testValidateFailsWhenQueryAndBodyBothSet() { + final SamlCompleteLogoutRequest samlCompleteLogoutRequest = new SamlCompleteLogoutRequest(); + samlCompleteLogoutRequest.setQueryString("queryString"); + samlCompleteLogoutRequest.setContent("content"); + final ActionRequestValidationException validationException = samlCompleteLogoutRequest.validate(); + assertThat(validationException.getMessage(), containsString("queryString and content may not both present")); + } + +} diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlCompleteLogoutAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlCompleteLogoutAction.java index c752b8191d5e3..811baf044e863 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlCompleteLogoutAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlCompleteLogoutAction.java @@ -53,7 +53,7 @@ private void processLogoutResponse(SamlRealm samlRealm, SamlCompleteLogoutReques final SamlLogoutResponseHandler logoutResponseHandler = samlRealm.getLogoutResponseHandler(); try { - logoutResponseHandler.handle(request.getContent(), request.getValidRequestIds()); + logoutResponseHandler.handle(request.isHttpRedirect(), request.getPayload(), request.getValidRequestIds()); listener.onResponse(new SamlCompleteLogoutResponse()); } catch (Exception e) { listener.onFailure(e); diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java index fa1dadfa38916..7abccd389cc0b 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java @@ -24,14 +24,20 @@ public SamlLogoutResponseHandler( super(clock, idp, sp, maxSkew); } - public void handle(String content, Collection allowedSamlRequestIds) { - final ParsedQueryString parsed = parseQueryStringAndValidateSignature(content, "SAMLResponse"); + public void handle(boolean httpRedirect, String payload, Collection 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"); + } else if (httpRedirect == false && parsed.hasSignature) { + throw samlException("URL is signed, but binding is HTTP-POST"); + } + final Element root; - if (parsed.hasSignature) { - logger.info("Content is signed at URL level. Proceed as HTTP-Redirect binding"); + if (httpRedirect) { + logger.info("Process SAML LogoutResponse with HTTP-Redirect binding"); root = parseSamlMessage(inflate(decodeBase64(parsed.samlMessage))); } else { - logger.info("Content is not signed at URL level. Proceed as HTTP-POST binding"); + logger.info("Process SAML LogoutResponse with HTTP-POST binding"); root = parseSamlMessage(decodeBase64(parsed.samlMessage)); } @@ -40,11 +46,9 @@ public void handle(String content, Collection allowedSamlRequestIds) { if (logoutResponse == null) { throw samlException("Cannot convert element {} into LogoutResponse object", root); } - if (logoutResponse.getSignature() == null) { - if (parsed.hasSignature == false) { - throw samlException("LogoutResponse is not signed for HTTP-Post binding"); - } - } else { + if (httpRedirect == false && logoutResponse.getSignature() == null) { + throw samlException("LogoutResponse is not signed, but a signature is required for HTTP-Post binding"); + } else if (httpRedirect == false) { validateSignature(logoutResponse.getSignature()); } checkInResponseTo(logoutResponse, allowedSamlRequestIds); 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 ff7fd60823dd5..8020f7679b003 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 @@ -34,10 +34,15 @@ public class RestSamlCompleteLogoutAction extends SamlBaseRestHandler{ private static final Logger logger = LogManager.getLogger(RestSamlCompleteLogoutAction.class); static class Input { + String queryString; String content; List ids; String realm; + void setQueryString(String queryString) { + this.queryString = queryString; + } + void setContent(String content) { this.content = content; } @@ -53,7 +58,8 @@ void setIds(List ids) { PARSER = new ObjectParser<>("saml_complete_logout", RestSamlCompleteLogoutAction.Input::new); static { - PARSER.declareString(RestSamlCompleteLogoutAction.Input::setContent, new ParseField("content")); + PARSER.declareStringOrNull(RestSamlCompleteLogoutAction.Input::setQueryString, new ParseField("queryString")); + PARSER.declareStringOrNull(RestSamlCompleteLogoutAction.Input::setContent, new ParseField("content")); PARSER.declareStringArray(RestSamlCompleteLogoutAction.Input::setIds, new ParseField("ids")); PARSER.declareString(RestSamlCompleteLogoutAction.Input::setRealm, new ParseField("realm")); } @@ -76,11 +82,13 @@ public List routes() { protected RestChannelConsumer innerPrepareRequest(RestRequest request, NodeClient client) throws IOException { try (XContentParser parser = request.contentParser()) { final RestSamlCompleteLogoutAction.Input input = PARSER.parse(parser, null); - logger.trace("SAML LogoutResponse: [{}...] [{}]", Strings.cleanTruncate(input.content, 128), input.ids); + logger.trace("SAML LogoutResponse: [{}...] [{}...] [{}]", + Strings.cleanTruncate(input.queryString, 128), Strings.cleanTruncate(input.content, 128), input.ids); return channel -> { final SamlCompleteLogoutRequestBuilder requestBuilder = new SamlCompleteLogoutRequestBuilder(client) - .content(input.content).validRequestIds(input.ids).authenticatingRealm(input.realm); + .queryString(input.queryString).content(input.content) + .validRequestIds(input.ids).authenticatingRealm(input.realm); requestBuilder.execute(new RestBuilderListener<>(channel) { @Override public RestResponse buildResponse(SamlCompleteLogoutResponse response, XContentBuilder builder) throws Exception { diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpPostTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpPostTests.java index 6987bbf2d4be4..fb868f5dab850 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpPostTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpPostTests.java @@ -41,12 +41,12 @@ public void setupHandler() { public void testHandlerWorksWithHttpPostBinding() throws Exception { final String payload = buildLogoutResponsePayload(emptyMap(), true); - samlLogoutResponseHandler.handle(payload, List.of(requestId)); + samlLogoutResponseHandler.handle(false, payload, List.of(requestId)); } public void testHandlerFailsWithHttpPostBindingAndNoSignature() throws Exception { final String payload = buildLogoutResponsePayload(emptyMap(), false); - final ElasticsearchSecurityException e = expectSamlException(() -> samlLogoutResponseHandler.handle(payload, List.of(requestId))); + final ElasticsearchSecurityException e = expectSamlException(() -> samlLogoutResponseHandler.handle(false, payload, List.of(requestId))); assertThat(e.getMessage(), containsString("is not signed")); } @@ -55,7 +55,7 @@ public void testHandlerWillThrowWhenStatusIsNotSuccess() throws Exception { replacements.put("status", "urn:oasis:names:tc:SAML:2.0:status:Requester"); final String payload = buildLogoutResponsePayload(replacements, true); final ElasticsearchSecurityException e = - expectSamlException(() -> samlLogoutResponseHandler.handle(payload, List.of(requestId))); + expectSamlException(() -> samlLogoutResponseHandler.handle(false, payload, List.of(requestId))); assertThat(e.getMessage(), containsString("not a 'success' response")); } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpRedirectTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpRedirectTests.java index d47fb0cafe50d..7055769e8d1a7 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpRedirectTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpRedirectTests.java @@ -79,7 +79,7 @@ public void testHandlerWorks() throws URISyntaxException { issuer.setValue(IDP_ENTITY_ID); logoutResponse.setIssuer(issuer); final String url = new SamlRedirect(logoutResponse, signingConfiguration).getRedirectUrl(); - samlLogoutResponseHandler.handle(new URI(url).getRawQuery(), List.of(requestId)); + samlLogoutResponseHandler.handle(true, new URI(url).getRawQuery(), List.of(requestId)); } public void testHandlerFailsIfStatusIsNotSuccess() { @@ -98,11 +98,11 @@ public void testHandlerFailsIfStatusIsNotSuccess() { final String url = new SamlRedirect(logoutResponse, signingConfiguration).getRedirectUrl(); final ElasticsearchSecurityException e = - expectSamlException(() -> samlLogoutResponseHandler.handle(new URI(url).getRawQuery(), List.of(requestId))); + expectSamlException(() -> samlLogoutResponseHandler.handle(true, new URI(url).getRawQuery(), List.of(requestId))); assertThat(e.getMessage(), containsString("is not a 'success' response")); } - public void testHandlerWillUseHttpPostBindingWhenUrlNotSigned() { + public void testHandlerWillFailWhenUrlNotSigned() { final String requestId = SamlUtils.generateSecureNCName(randomIntBetween(8, 30)); final SigningConfiguration signingConfiguration = new SigningConfiguration(Sets.newHashSet("*"), null); final LogoutResponse logoutResponse = SamlUtils.buildObject(LogoutResponse.class, LogoutResponse.DEFAULT_ELEMENT_NAME); @@ -117,8 +117,27 @@ public void testHandlerWillUseHttpPostBindingWhenUrlNotSigned() { logoutResponse.setIssuer(issuer); 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")); + expectSamlException(() -> samlLogoutResponseHandler.handle(true, new URI(url).getRawQuery(), List.of(requestId))); + assertThat(e.getMessage(), containsString("URL is not signed, but is required for HTTP-Redirect binding")); + } + + public void testHandlerWillFailWhenUrlIsSignedButBindingIsHttpPost() throws URISyntaxException { + final String requestId = SamlUtils.generateSecureNCName(randomIntBetween(8, 30)); + final SigningConfiguration signingConfiguration = new SigningConfiguration(Sets.newHashSet("*"), credential); + final LogoutResponse logoutResponse = SamlUtils.buildObject(LogoutResponse.class, LogoutResponse.DEFAULT_ELEMENT_NAME); + logoutResponse.setDestination(LOGOUT_URL); + logoutResponse.setIssueInstant(new DateTime(clock.millis())); + logoutResponse.setID(SamlUtils.generateSecureNCName(randomIntBetween(8, 30))); + logoutResponse.setInResponseTo(requestId); + logoutResponse.setStatus(buildStatus(StatusCode.SUCCESS)); + + final Issuer issuer = SamlUtils.buildObject(Issuer.class, Issuer.DEFAULT_ELEMENT_NAME); + issuer.setValue(IDP_ENTITY_ID); + logoutResponse.setIssuer(issuer); + final String url = new SamlRedirect(logoutResponse, signingConfiguration).getRedirectUrl(); + final ElasticsearchSecurityException e = + expectSamlException(() -> samlLogoutResponseHandler.handle(false, new URI(url).getRawQuery(), List.of(requestId))); + assertThat(e.getMessage(), containsString("URL is signed, but binding is HTTP-POST")); } private Status buildStatus(String statusCodeValue) { From 48c27d6a32b3657ce3011ca642b5ddf59c5a1089 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Mon, 18 May 2020 12:43:51 +1000 Subject: [PATCH 11/22] Fix test convention --- .../security/action/saml/SamlCompleteLogoutRequestTests.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) 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 098560af1ed44..c270ebbca52d5 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 @@ -6,13 +6,12 @@ package org.elasticsearch.xpack.core.security.action.saml; -import junit.framework.TestCase; import org.elasticsearch.action.ActionRequestValidationException; +import org.elasticsearch.test.ESTestCase; import static org.hamcrest.Matchers.containsString; -import static org.junit.Assert.assertThat; -public class SamlCompleteLogoutRequestTests extends TestCase { +public class SamlCompleteLogoutRequestTests extends ESTestCase { public void testValidateFailsWhenQueryAndBodyBothNotExist() { final SamlCompleteLogoutRequest samlCompleteLogoutRequest = new SamlCompleteLogoutRequest(); From 17903a363f26f4748f2e4b87bbc0b586f51665dc Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Mon, 18 May 2020 13:20:46 +1000 Subject: [PATCH 12/22] checkstyle --- .../authc/saml/SamlLogoutResponseHandlerHttpPostTests.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpPostTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpPostTests.java index fb868f5dab850..3a1330d200463 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpPostTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpPostTests.java @@ -46,7 +46,8 @@ public void testHandlerWorksWithHttpPostBinding() throws Exception { public void testHandlerFailsWithHttpPostBindingAndNoSignature() throws Exception { final String payload = buildLogoutResponsePayload(emptyMap(), false); - final ElasticsearchSecurityException e = expectSamlException(() -> samlLogoutResponseHandler.handle(false, payload, List.of(requestId))); + final ElasticsearchSecurityException e = + expectSamlException(() -> samlLogoutResponseHandler.handle(false, payload, List.of(requestId))); assertThat(e.getMessage(), containsString("is not signed")); } From e297fa5eaa84fb16904d6045e1a0b32309d7e4d3 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Wed, 27 May 2020 15:49:24 +1000 Subject: [PATCH 13/22] Update x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java Co-authored-by: Ioannis Kakavas --- .../xpack/security/authc/saml/SamlLogoutResponseHandler.java | 1 + 1 file changed, 1 insertion(+) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java index 7abccd389cc0b..6bf19ebac8158 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java @@ -46,6 +46,7 @@ public void handle(boolean httpRedirect, String payload, Collection allo if (logoutResponse == null) { throw samlException("Cannot convert element {} into LogoutResponse object", root); } + // For HTTP-Redirect, we validate the signature while parsing the object from the query string if (httpRedirect == false && logoutResponse.getSignature() == null) { throw samlException("LogoutResponse is not signed, but a signature is required for HTTP-Post binding"); } else if (httpRedirect == false) { From c22674ddbbba0e678e305556c9a9c3c858be463d Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Wed, 27 May 2020 23:33:36 +1000 Subject: [PATCH 14/22] Tidy up a bit due to latest change in master --- .../authc/saml/SamlAuthenticatorTests.java | 79 ++----------------- ...amlLogoutResponseHandlerHttpPostTests.java | 15 +++- .../authc/saml/SamlResponseHandlerTests.java | 60 +++++++------- 3 files changed, 47 insertions(+), 107 deletions(-) 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 847862d79710e..5d6bce11fd9a4 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 @@ -18,7 +18,6 @@ import org.hamcrest.Matchers; import org.joda.time.DateTime; import org.junit.Before; -import org.opensaml.core.xml.config.XMLObjectProviderRegistrySupport; import org.opensaml.core.xml.schema.XSString; import org.opensaml.core.xml.schema.impl.XSStringBuilder; import org.opensaml.saml.saml2.core.Assertion; @@ -49,32 +48,15 @@ import org.opensaml.xmlsec.encryption.support.DecryptionException; import org.opensaml.xmlsec.encryption.support.EncryptionConstants; import org.opensaml.xmlsec.encryption.support.KeyEncryptionParameters; -import org.opensaml.xmlsec.keyinfo.KeyInfoSupport; -import org.opensaml.xmlsec.signature.Signature; import org.opensaml.xmlsec.signature.support.SignatureException; -import org.opensaml.xmlsec.signature.support.Signer; import org.w3c.dom.Document; import org.w3c.dom.Element; -import org.w3c.dom.NodeList; import org.xml.sax.InputSource; import org.xml.sax.SAXException; import javax.crypto.KeyGenerator; import javax.crypto.SecretKey; -import javax.xml.crypto.dsig.CanonicalizationMethod; -import javax.xml.crypto.dsig.DigestMethod; -import javax.xml.crypto.dsig.Reference; -import javax.xml.crypto.dsig.SignatureMethod; -import javax.xml.crypto.dsig.SignedInfo; -import javax.xml.crypto.dsig.Transform; -import javax.xml.crypto.dsig.XMLSignature; -import javax.xml.crypto.dsig.XMLSignatureFactory; -import javax.xml.crypto.dsig.dom.DOMSignContext; -import javax.xml.crypto.dsig.keyinfo.KeyInfo; -import javax.xml.crypto.dsig.keyinfo.KeyInfoFactory; -import javax.xml.crypto.dsig.spec.C14NMethodParameterSpec; -import javax.xml.crypto.dsig.spec.TransformParameterSpec; import javax.xml.parsers.DocumentBuilderFactory; import javax.xml.parsers.ParserConfigurationException; import java.io.IOException; @@ -96,7 +78,6 @@ import static java.util.Collections.singletonList; import static javax.xml.crypto.dsig.CanonicalizationMethod.EXCLUSIVE; import static javax.xml.crypto.dsig.CanonicalizationMethod.EXCLUSIVE_WITH_COMMENTS; -import static javax.xml.crypto.dsig.Transform.ENVELOPED; import static org.hamcrest.CoreMatchers.not; import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.containsInAnyOrder; @@ -126,6 +107,7 @@ public class SamlAuthenticatorTests extends SamlResponseHandlerTests { public void setupAuthenticator() throws Exception { this.clock = new ClockMock(); this.maxSkew = TimeValue.timeValueMinutes(1); + this.authenticator = buildAuthenticator(() -> buildOpenSamlCredential(idpSigningCertificatePair), emptyList()); this.requestId = randomId(); } @@ -1183,7 +1165,8 @@ private interface CryptoTransform { } private String signResponse(Response response) throws Exception { - return signResponseElement(response, EXCLUSIVE, SamlAuthenticatorTests.idpSigningCertificatePair, true); + signSignableObject(response, EXCLUSIVE, SamlAuthenticatorTests.idpSigningCertificatePair); + return SamlUtils.getXmlContent(response, false); } private String signResponse(String xml) throws Exception { @@ -1208,65 +1191,15 @@ private String signAssertions(String xml, Tuple key private String signResponseString(String xml, String c14nMethod, Tuple keyPair, boolean onlyResponse) throws Exception { - return signResponseElement(toResponse(xml), c14nMethod, keyPair, onlyResponse); - } - - private String signResponseElement(Response response, String c14nMethod, Tuple keyPair, - boolean onlyResponse) - throws Exception { - final Signature signature = SamlUtils.buildObject(Signature.class, Signature.DEFAULT_ELEMENT_NAME); - final Credential credential = new BasicCredential(keyPair.v1().getPublicKey(), keyPair.v2()); - final org.opensaml.xmlsec.signature.KeyInfo kf = SamlUtils.buildObject(org.opensaml.xmlsec.signature.KeyInfo.class, - org.opensaml.xmlsec.signature.KeyInfo.DEFAULT_ELEMENT_NAME); - KeyInfoSupport.addCertificate(kf, keyPair.v1()); - signature.setSigningCredential(credential); - signature.setSignatureAlgorithm(getSignatureAlgorithmURI(keyPair.v2())); - signature.setCanonicalizationAlgorithm(c14nMethod); - signature.setKeyInfo(kf); + final Response response = toResponse(xml); if (onlyResponse) { - response.setSignature(signature); + signSignableObject(response, c14nMethod, keyPair); } else { - response.getAssertions().get(0).setSignature(signature); + signSignableObject(response.getAssertions().get(0), c14nMethod, keyPair); } - XMLObjectProviderRegistrySupport.getMarshallerFactory().getMarshaller(response).marshall(response); - Signer.signObject(signature); return SamlUtils.getXmlContent(response, false); } - private void signElement(Element parent, String c14nMethod) throws Exception { - //We need to explicitly set the Id attribute, "ID" is just our convention - parent.setIdAttribute("ID", true); - final String refID = "#" + parent.getAttribute("ID"); - final X509Certificate certificate = idpSigningCertificatePair.v1(); - final PrivateKey privateKey = idpSigningCertificatePair.v2(); - final XMLSignatureFactory fac = XMLSignatureFactory.getInstance("DOM"); - final DigestMethod digestMethod = fac.newDigestMethod(randomFrom(DigestMethod.SHA256, DigestMethod.SHA512), null); - final Transform transform = fac.newTransform(ENVELOPED, (TransformParameterSpec) null); - // We don't "have to" set the reference explicitly since we're using enveloped signatures, but it helps with - // creating the XSW test cases - final Reference reference = fac.newReference(refID, digestMethod, singletonList(transform), null, null); - final SignatureMethod signatureMethod = fac.newSignatureMethod(getSignatureAlgorithmURI(privateKey), null); - final CanonicalizationMethod canonicalizationMethod = fac.newCanonicalizationMethod(c14nMethod, (C14NMethodParameterSpec) null); - - final SignedInfo signedInfo = fac.newSignedInfo(canonicalizationMethod, signatureMethod, singletonList(reference)); - KeyInfoFactory kif = fac.getKeyInfoFactory(); - javax.xml.crypto.dsig.keyinfo.X509Data data = kif.newX509Data(Collections.singletonList(certificate)); - final KeyInfo keyInfo = kif.newKeyInfo(singletonList(data)); - - final DOMSignContext dsc = new DOMSignContext(privateKey, parent); - dsc.setDefaultNamespacePrefix("ds"); - // According to the schema, the signature needs to be placed after the if there is one in the document - // If there are more than one we are dealing with a so we sign the Response and add the - // Signature after the Response - NodeList issuersList = parent.getElementsByTagNameNS(SAML20_NS, "Issuer"); - if (issuersList.getLength() > 0) { - dsc.setNextSibling(issuersList.item(0).getNextSibling()); - } - - final XMLSignature signature = fac.newXMLSignature(signedInfo, keyInfo); - signature.sign(dsc); - } - private Response encryptAssertions(String xml, Tuple keyPair) throws Exception { final Response response = toResponse(xml); final Encrypter samlEncrypter = getEncrypter(keyPair); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpPostTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpPostTests.java index 3a1330d200463..049df3ed94a9a 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpPostTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpPostTests.java @@ -11,6 +11,7 @@ import org.elasticsearch.common.util.NamedFormatter; import org.elasticsearch.xpack.core.watcher.watch.ClockMock; import org.junit.Before; +import org.opensaml.saml.saml2.core.LogoutResponse; import java.net.URLEncoder; import java.nio.charset.StandardCharsets; @@ -22,6 +23,7 @@ import static java.util.Collections.emptyList; import static java.util.Collections.emptyMap; +import static javax.xml.crypto.dsig.CanonicalizationMethod.EXCLUSIVE; import static org.hamcrest.Matchers.containsString; public class SamlLogoutResponseHandlerHttpPostTests extends SamlResponseHandlerTests { @@ -80,10 +82,17 @@ private String buildLogoutResponsePayload(Map data, boolean shou replacements.putIfAbsent("requestId", requestId); replacements.putIfAbsent("SP_LOGOUT_URL", SP_LOGOUT_URL); replacements.putIfAbsent("status", "urn:oasis:names:tc:SAML:2.0:status:Success"); - final String xml = shouldSign - ? signDoc(NamedFormatter.format(template, replacements)) : NamedFormatter.format(template, replacements); - String encoded = URLEncoder.encode(Base64.getEncoder().encodeToString(xml.getBytes(StandardCharsets.UTF_8)), + final String xml = NamedFormatter.format(template, replacements); + final String signed = shouldSign ? signLogoutResponseString(xml) : xml; + String encoded = URLEncoder.encode(Base64.getEncoder().encodeToString(signed.getBytes(StandardCharsets.UTF_8)), StandardCharsets.US_ASCII.name()); return String.format(Locale.ROOT, "SAMLResponse=%s", encoded); } + + private String signLogoutResponseString(String xml) throws Exception { + final LogoutResponse logoutResponse = + samlLogoutResponseHandler.buildXmlObject(parseDocument(xml).getDocumentElement(), LogoutResponse.class); + signSignableObject(logoutResponse, EXCLUSIVE, idpSigningCertificatePair); + return SamlUtils.getXmlContent(logoutResponse, false); + } } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlResponseHandlerTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlResponseHandlerTests.java index d4cdd775a36cd..ccc6b2f978677 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlResponseHandlerTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlResponseHandlerTests.java @@ -14,8 +14,14 @@ import org.elasticsearch.xpack.core.watcher.watch.ClockMock; import org.junit.AfterClass; import org.junit.BeforeClass; +import org.opensaml.core.xml.config.XMLObjectProviderRegistrySupport; +import org.opensaml.saml.common.SignableSAMLObject; +import org.opensaml.security.credential.BasicCredential; import org.opensaml.security.credential.Credential; import org.opensaml.security.x509.X509Credential; +import org.opensaml.xmlsec.keyinfo.KeyInfoSupport; +import org.opensaml.xmlsec.signature.Signature; +import org.opensaml.xmlsec.signature.support.Signer; import org.w3c.dom.Document; import org.w3c.dom.Element; import org.w3c.dom.NodeList; @@ -24,7 +30,6 @@ import java.io.IOException; import java.io.StringReader; -import java.security.KeyException; import java.security.NoSuchAlgorithmException; import java.security.PrivateKey; import java.security.cert.X509Certificate; @@ -53,7 +58,6 @@ import javax.xml.parsers.ParserConfigurationException; import static java.util.Collections.singletonList; -import static javax.xml.crypto.dsig.CanonicalizationMethod.EXCLUSIVE; import static javax.xml.crypto.dsig.Transform.ENVELOPED; import static org.opensaml.saml.common.xml.SAMLConstants.SAML20_NS; @@ -118,12 +122,6 @@ public static void cleanup() { supportedAesTransformations = null; } - private static KeyInfo getKeyInfo(XMLSignatureFactory factory, X509Certificate certificate) throws KeyException { - KeyInfoFactory kif = factory.getKeyInfoFactory(); - javax.xml.crypto.dsig.keyinfo.X509Data data = kif.newX509Data(Collections.singletonList(certificate)); - return kif.newKeyInfo(singletonList(data)); - } - protected SpConfiguration getSpConfiguration(List reqAuthnCtxClassRef) { final SigningConfiguration signingConfiguration = new SigningConfiguration( Collections.singleton("*"), @@ -142,24 +140,6 @@ protected String randomId() { return SamlUtils.generateSecureNCName(randomIntBetween(12, 36)); } - protected String signDoc(String xml) throws Exception { - return signDoc(xml, EXCLUSIVE, SamlResponseHandlerTests.idpSigningCertificatePair); - } - - protected String signDoc(String xml, Tuple keyPair) throws Exception { - return signDoc(xml, EXCLUSIVE, keyPair); - } - - protected String signDoc(String xml, String c14nMethod) throws Exception { - return signDoc(xml, c14nMethod, SamlResponseHandlerTests.idpSigningCertificatePair); - } - - private String signDoc(String xml, String c14nMethod, Tuple keyPair) throws Exception { - final Document doc = parseDocument(xml); - signElement(doc.getDocumentElement(), keyPair, c14nMethod); - return SamlUtils.toString(doc.getDocumentElement()); - } - protected Document parseDocument(String xml) throws ParserConfigurationException, SAXException, IOException { final DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance(); dbf.setNamespaceAware(true); @@ -196,12 +176,12 @@ protected String getSignatureAlgorithmURI(PrivateKey key) { return algoUri; } - protected void signElement(Element parent, Tuple keyPair, String c14nMethod) throws Exception { + protected void signElement(Element parent, String c14nMethod) throws Exception { //We need to explicitly set the Id attribute, "ID" is just our convention parent.setIdAttribute("ID", true); final String refID = "#" + parent.getAttribute("ID"); - final X509Certificate certificate = keyPair.v1(); - final PrivateKey privateKey = keyPair.v2(); + final X509Certificate certificate = idpSigningCertificatePair.v1(); + final PrivateKey privateKey = idpSigningCertificatePair.v2(); final XMLSignatureFactory fac = XMLSignatureFactory.getInstance("DOM"); final DigestMethod digestMethod = fac.newDigestMethod(randomFrom(DigestMethod.SHA256, DigestMethod.SHA512), null); final Transform transform = fac.newTransform(ENVELOPED, (TransformParameterSpec) null); @@ -212,8 +192,9 @@ protected void signElement(Element parent, Tuple ke final CanonicalizationMethod canonicalizationMethod = fac.newCanonicalizationMethod(c14nMethod, (C14NMethodParameterSpec) null); final SignedInfo signedInfo = fac.newSignedInfo(canonicalizationMethod, signatureMethod, singletonList(reference)); - - final KeyInfo keyInfo = SamlResponseHandlerTests.getKeyInfo(fac, certificate); + KeyInfoFactory kif = fac.getKeyInfoFactory(); + javax.xml.crypto.dsig.keyinfo.X509Data data = kif.newX509Data(Collections.singletonList(certificate)); + final KeyInfo keyInfo = kif.newKeyInfo(singletonList(data)); final DOMSignContext dsc = new DOMSignContext(privateKey, parent); dsc.setDefaultNamespacePrefix("ds"); @@ -228,4 +209,21 @@ protected void signElement(Element parent, Tuple ke final XMLSignature signature = fac.newXMLSignature(signedInfo, keyInfo); signature.sign(dsc); } + + protected void signSignableObject( + SignableSAMLObject signableObject, String c14nMethod, Tuple keyPair) + throws Exception { + final Signature signature = SamlUtils.buildObject(Signature.class, Signature.DEFAULT_ELEMENT_NAME); + final Credential credential = new BasicCredential(keyPair.v1().getPublicKey(), keyPair.v2()); + final org.opensaml.xmlsec.signature.KeyInfo kf = SamlUtils.buildObject(org.opensaml.xmlsec.signature.KeyInfo.class, + org.opensaml.xmlsec.signature.KeyInfo.DEFAULT_ELEMENT_NAME); + KeyInfoSupport.addCertificate(kf, keyPair.v1()); + signature.setSigningCredential(credential); + signature.setSignatureAlgorithm(getSignatureAlgorithmURI(keyPair.v2())); + signature.setCanonicalizationAlgorithm(c14nMethod); + signature.setKeyInfo(kf); + signableObject.setSignature(signature); + XMLObjectProviderRegistrySupport.getMarshallerFactory().getMarshaller(signableObject).marshall(signableObject); + Signer.signObject(signature); + } } From a56bbccb3693a728dfe204f30e527e1fbec959f7 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Wed, 27 May 2020 23:50:11 +1000 Subject: [PATCH 15/22] Fix randomize of signer --- .../xpack/security/authc/saml/SamlAuthenticatorTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 5d6bce11fd9a4..d6bb9a1af7778 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 @@ -262,7 +262,7 @@ public void testSuccessfullyParseContentFromEncryptedAndSignedAssertion() throws } public void testSuccessfullyParseContentFromEncryptedAttribute() throws Exception { - final CryptoTransform signer = randomBoolean() ? this::signResponse : this::signResponse; + final CryptoTransform signer = randomBoolean() ? this::signResponse : this::signAssertions; final Instant now = clock.instant(); String xml = getSimpleResponseAsString(now); /** From f2ed7dfc7a45bb2a06c9ed1a0eae5382864d5ef8 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Thu, 28 May 2020 10:54:52 +1000 Subject: [PATCH 16/22] Address feedback --- .../saml/SamlCompleteLogoutRequest.java | 9 +++++--- .../saml/SamlCompleteLogoutRequestTests.java | 3 +-- .../authc/saml/SamlLogoutResponseHandler.java | 20 ++++++---------- .../saml/RestSamlCompleteLogoutAction.java | 7 ++++++ ...amlLogoutResponseHandlerHttpPostTests.java | 6 +---- ...ogoutResponseHandlerHttpRedirectTests.java | 23 ++----------------- 6 files changed, 24 insertions(+), 44 deletions(-) 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 b539dcc8ae7e0..6ea37232135b0 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 @@ -8,6 +8,7 @@ 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; @@ -20,7 +21,9 @@ */ public final class SamlCompleteLogoutRequest extends ActionRequest { + @Nullable private String queryString; + @Nullable private String content; private List validRequestIds; @Nullable @@ -36,10 +39,10 @@ public SamlCompleteLogoutRequest() { @Override public ActionRequestValidationException validate() { ActionRequestValidationException validationException = null; - if (queryString == null && content == null) { - validationException = addValidationError("queryString and content may not both be null", validationException); + if (Strings.hasText(queryString) == false && Strings.hasText(content) == false) { + validationException = addValidationError("queryString and content may not both be empty", validationException); } - if (queryString != null && content != null) { + if (Strings.hasText(queryString) && Strings.hasText(content)) { validationException = addValidationError("queryString and content may not both present", validationException); } return validationException; 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 c270ebbca52d5..520f5d88e4ff4 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 @@ -16,7 +16,7 @@ public class SamlCompleteLogoutRequestTests extends ESTestCase { public void testValidateFailsWhenQueryAndBodyBothNotExist() { final SamlCompleteLogoutRequest samlCompleteLogoutRequest = new SamlCompleteLogoutRequest(); final ActionRequestValidationException validationException = samlCompleteLogoutRequest.validate(); - assertThat(validationException.getMessage(), containsString("queryString and content may not both be null")); + assertThat(validationException.getMessage(), containsString("queryString and content may not both be empty")); } public void testValidateFailsWhenQueryAndBodyBothSet() { @@ -26,5 +26,4 @@ public void testValidateFailsWhenQueryAndBodyBothSet() { final ActionRequestValidationException validationException = samlCompleteLogoutRequest.validate(); assertThat(validationException.getMessage(), containsString("queryString and content may not both present")); } - } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java index 6bf19ebac8158..3ff02e5e52e3d 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java @@ -25,27 +25,21 @@ public SamlLogoutResponseHandler( } public void handle(boolean httpRedirect, String payload, Collection 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"); - } 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"); + logger.debug("Process SAML LogoutResponse with HTTP-Redirect binding"); + final ParsedQueryString parsed = parseQueryStringAndValidateSignature(payload, "SAMLResponse"); + if (parsed.hasSignature == false){ + throw samlException("Query string is not signed, but is required for HTTP-Redirect binding"); + } root = parseSamlMessage(inflate(decodeBase64(parsed.samlMessage))); } else { - logger.info("Process SAML LogoutResponse with HTTP-POST binding"); - root = parseSamlMessage(decodeBase64(parsed.samlMessage)); + logger.debug("Process SAML LogoutResponse with HTTP-POST binding"); + root = parseSamlMessage(decodeBase64(payload)); } if (LOGOUT_RESPONSE_TAG_NAME.equals(root.getLocalName()) && SAML_NAMESPACE.equals(root.getNamespaceURI())) { final LogoutResponse logoutResponse = buildXmlObject(root, LogoutResponse.class); - if (logoutResponse == null) { - throw samlException("Cannot convert element {} into LogoutResponse object", root); - } // For HTTP-Redirect, we validate the signature while parsing the object from the query string if (httpRedirect == false && logoutResponse.getSignature() == null) { throw samlException("LogoutResponse is not signed, but a signature is required for HTTP-Post binding"); 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 8020f7679b003..6c73a95eb8ad7 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 @@ -29,6 +29,13 @@ import static org.elasticsearch.rest.RestRequest.Method.POST; +/** + * This Rest endpoint handles SAML LogoutResponse sent from idP with either HTTP-Redirect or HTTP-Post binding. + * For HTTP-Redirect binding, it expects {@link Input#queryString} be set to the query string of the redirect URI. + * For HTTP-Post binding, it expects {@link Input#content} be set to the value of SAMLResponse form parameter, i.e. + * caller of this API must do the work to extract the SAMLResponse value from body of the HTTP-Post request. The + * value must also be URL decoded if necessary. + */ public class RestSamlCompleteLogoutAction extends SamlBaseRestHandler{ private static final Logger logger = LogManager.getLogger(RestSamlCompleteLogoutAction.class); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpPostTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpPostTests.java index 049df3ed94a9a..aff3e0aa610b9 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpPostTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpPostTests.java @@ -13,12 +13,10 @@ import org.junit.Before; import org.opensaml.saml.saml2.core.LogoutResponse; -import java.net.URLEncoder; import java.nio.charset.StandardCharsets; import java.util.Base64; import java.util.HashMap; import java.util.List; -import java.util.Locale; import java.util.Map; import static java.util.Collections.emptyList; @@ -84,9 +82,7 @@ private String buildLogoutResponsePayload(Map data, boolean shou replacements.putIfAbsent("status", "urn:oasis:names:tc:SAML:2.0:status:Success"); final String xml = NamedFormatter.format(template, replacements); final String signed = shouldSign ? signLogoutResponseString(xml) : xml; - String encoded = URLEncoder.encode(Base64.getEncoder().encodeToString(signed.getBytes(StandardCharsets.UTF_8)), - StandardCharsets.US_ASCII.name()); - return String.format(Locale.ROOT, "SAMLResponse=%s", encoded); + return Base64.getEncoder().encodeToString(signed.getBytes(StandardCharsets.UTF_8)); } private String signLogoutResponseString(String xml) throws Exception { diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpRedirectTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpRedirectTests.java index 7055769e8d1a7..a8fdc4c50e784 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpRedirectTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandlerHttpRedirectTests.java @@ -102,7 +102,7 @@ public void testHandlerFailsIfStatusIsNotSuccess() { assertThat(e.getMessage(), containsString("is not a 'success' response")); } - public void testHandlerWillFailWhenUrlNotSigned() { + public void testHandlerWillFailWhenQueryStringNotSigned() { final String requestId = SamlUtils.generateSecureNCName(randomIntBetween(8, 30)); final SigningConfiguration signingConfiguration = new SigningConfiguration(Sets.newHashSet("*"), null); final LogoutResponse logoutResponse = SamlUtils.buildObject(LogoutResponse.class, LogoutResponse.DEFAULT_ELEMENT_NAME); @@ -118,26 +118,7 @@ public void testHandlerWillFailWhenUrlNotSigned() { final String url = new SamlRedirect(logoutResponse, signingConfiguration).getRedirectUrl(); final ElasticsearchSecurityException e = expectSamlException(() -> samlLogoutResponseHandler.handle(true, new URI(url).getRawQuery(), List.of(requestId))); - assertThat(e.getMessage(), containsString("URL is not signed, but is required for HTTP-Redirect binding")); - } - - public void testHandlerWillFailWhenUrlIsSignedButBindingIsHttpPost() throws URISyntaxException { - final String requestId = SamlUtils.generateSecureNCName(randomIntBetween(8, 30)); - final SigningConfiguration signingConfiguration = new SigningConfiguration(Sets.newHashSet("*"), credential); - final LogoutResponse logoutResponse = SamlUtils.buildObject(LogoutResponse.class, LogoutResponse.DEFAULT_ELEMENT_NAME); - logoutResponse.setDestination(LOGOUT_URL); - logoutResponse.setIssueInstant(new DateTime(clock.millis())); - logoutResponse.setID(SamlUtils.generateSecureNCName(randomIntBetween(8, 30))); - logoutResponse.setInResponseTo(requestId); - logoutResponse.setStatus(buildStatus(StatusCode.SUCCESS)); - - final Issuer issuer = SamlUtils.buildObject(Issuer.class, Issuer.DEFAULT_ELEMENT_NAME); - issuer.setValue(IDP_ENTITY_ID); - logoutResponse.setIssuer(issuer); - final String url = new SamlRedirect(logoutResponse, signingConfiguration).getRedirectUrl(); - final ElasticsearchSecurityException e = - expectSamlException(() -> samlLogoutResponseHandler.handle(false, new URI(url).getRawQuery(), List.of(requestId))); - assertThat(e.getMessage(), containsString("URL is signed, but binding is HTTP-POST")); + assertThat(e.getMessage(), containsString("Query string is not signed, but is required for HTTP-Redirect binding")); } private Status buildStatus(String statusCodeValue) { From 2b85c075176c0ac6c0709703cc857e034ec5d5dd Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Fri, 29 May 2020 15:24:19 +1000 Subject: [PATCH 17/22] Update x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlCompleteLogoutAction.java Co-authored-by: Ioannis Kakavas --- .../security/action/saml/TransportSamlCompleteLogoutAction.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlCompleteLogoutAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlCompleteLogoutAction.java index 811baf044e863..63db889b79c20 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlCompleteLogoutAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlCompleteLogoutAction.java @@ -40,7 +40,7 @@ public TransportSamlCompleteLogoutAction(TransportService transportService, Acti protected void doExecute(Task task, SamlCompleteLogoutRequest request, ActionListener listener) { List realms = findSamlRealms(this.realms, request.getRealm(), null); if (realms.isEmpty()) { - listener.onFailure(SamlUtils.samlException("Cannot find any matching realm for [{}]", request)); + 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 [{}] for [{}]", realms, request)); } else { From a9b890a255bbfb3c541788fa8cfbbc72353a6226 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Fri, 29 May 2020 15:28:31 +1000 Subject: [PATCH 18/22] Update x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java Co-authored-by: Ioannis Kakavas --- .../security/authc/saml/SamlLogoutResponseHandler.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java index 3ff02e5e52e3d..9f12127ea09a1 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java @@ -41,9 +41,10 @@ public void handle(boolean httpRedirect, String payload, Collection allo if (LOGOUT_RESPONSE_TAG_NAME.equals(root.getLocalName()) && SAML_NAMESPACE.equals(root.getNamespaceURI())) { final LogoutResponse logoutResponse = buildXmlObject(root, LogoutResponse.class); // For HTTP-Redirect, we validate the signature while parsing the object from the query string - if (httpRedirect == false && logoutResponse.getSignature() == null) { - throw samlException("LogoutResponse is not signed, but a signature is required for HTTP-Post binding"); - } else if (httpRedirect == false) { + if (httpRedirect == false) { + if (logoutResponse.getSignature() == null) { + throw samlException("LogoutResponse is not signed, but a signature is required for HTTP-Post binding"); + } validateSignature(logoutResponse.getSignature()); } checkInResponseTo(logoutResponse, allowedSamlRequestIds); From 99704d23893c98394ab19f0fe768bb2036be7431 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Fri, 29 May 2020 15:32:26 +1000 Subject: [PATCH 19/22] Update x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlCompleteLogoutAction.java Co-authored-by: Ioannis Kakavas --- .../security/action/saml/TransportSamlCompleteLogoutAction.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlCompleteLogoutAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlCompleteLogoutAction.java index 63db889b79c20..f4893c15f1cae 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlCompleteLogoutAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlCompleteLogoutAction.java @@ -42,7 +42,7 @@ protected void doExecute(Task task, SamlCompleteLogoutRequest request, ActionLis 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 [{}] for [{}]", realms, request)); + listener.onFailure(SamlUtils.samlException("Found multiple matching realms [{}] with name [{}]", realms, request.getRealm())); } else { processLogoutResponse(realms.get(0), request, listener); } From 88862ab16b9fc0a928192706ef44f2e39202f97a Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Fri, 29 May 2020 15:33:25 +1000 Subject: [PATCH 20/22] Address feedback --- .../security/action/saml/SamlCompleteLogoutRequest.java | 4 +++- .../security/authc/saml/SamlLogoutResponseHandler.java | 6 +++--- 2 files changed, 6 insertions(+), 4 deletions(-) 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 6ea37232135b0..714c714a104e6 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 @@ -26,7 +26,6 @@ public final class SamlCompleteLogoutRequest extends ActionRequest { @Nullable private String content; private List validRequestIds; - @Nullable private String realm; public SamlCompleteLogoutRequest(StreamInput in) throws IOException { @@ -39,6 +38,9 @@ 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); } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java index 9f12127ea09a1..7e17cb6c84dfa 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlLogoutResponseHandler.java @@ -40,11 +40,11 @@ public void handle(boolean httpRedirect, String payload, Collection allo if (LOGOUT_RESPONSE_TAG_NAME.equals(root.getLocalName()) && SAML_NAMESPACE.equals(root.getNamespaceURI())) { final LogoutResponse logoutResponse = buildXmlObject(root, LogoutResponse.class); - // For HTTP-Redirect, we validate the signature while parsing the object from the query string + // For HTTP-Redirect, the signature is already validated when parsing the query string if (httpRedirect == false) { if (logoutResponse.getSignature() == null) { - throw samlException("LogoutResponse is not signed, but a signature is required for HTTP-Post binding"); - } + throw samlException("LogoutResponse is not signed, but is required for HTTP-Post binding"); + } validateSignature(logoutResponse.getSignature()); } checkInResponseTo(logoutResponse, allowedSamlRequestIds); From 512873794161a1de417420cf43e68b3b001b9d66 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Fri, 29 May 2020 15:35:32 +1000 Subject: [PATCH 21/22] Add one more test for non-null realm field --- .../action/saml/SamlCompleteLogoutRequestTests.java | 9 +++++++++ 1 file changed, 9 insertions(+) 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 520f5d88e4ff4..3b9cbef75c13c 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 @@ -15,15 +15,24 @@ 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")); + } } From 75abd7b2686bd8869f4529ed248cf735c7ba6a74 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Mon, 1 Jun 2020 22:59:25 +1000 Subject: [PATCH 22/22] Address feedback --- .../saml/SamlCompleteLogoutRequest.java | 12 +++- .../SamlCompleteLogoutRequestBuilder.java | 42 ------------- .../saml/RestSamlCompleteLogoutAction.java | 63 ++++++------------- 3 files changed, 30 insertions(+), 87 deletions(-) delete mode 100644 x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutRequestBuilder.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 714c714a104e6..cd3d8e2dec590 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 @@ -50,14 +50,22 @@ public ActionRequestValidationException validate() { return validationException; } - public void setContent(String content) { - this.content = content; + 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 getValidRequestIds() { return validRequestIds; } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutRequestBuilder.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutRequestBuilder.java deleted file mode 100644 index 0270e5b6e5986..0000000000000 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlCompleteLogoutRequestBuilder.java +++ /dev/null @@ -1,42 +0,0 @@ -/* - * 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.ActionRequestBuilder; -import org.elasticsearch.client.ElasticsearchClient; - -import java.util.List; - -/** - * Request builder used to populate a {@link SamlCompleteLogoutRequest} - */ -public final class SamlCompleteLogoutRequestBuilder - extends ActionRequestBuilder { - - public SamlCompleteLogoutRequestBuilder(ElasticsearchClient client) { - super(client, SamlCompleteLogoutAction.INSTANCE, new SamlCompleteLogoutRequest()); - } - - public SamlCompleteLogoutRequestBuilder queryString(String queryString) { - request.setQueryString(queryString); - return this; - } - - public SamlCompleteLogoutRequestBuilder content(String content) { - request.setContent(content); - return this; - } - - public SamlCompleteLogoutRequestBuilder validRequestIds(List validRequestIds) { - request.setValidRequestIds(validRequestIds); - return this; - } - - public SamlCompleteLogoutRequestBuilder authenticatingRealm(String realm) { - request.setRealm(realm); - return this; - } -} 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 6c73a95eb8ad7..dfee78471e236 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 @@ -21,7 +21,8 @@ import org.elasticsearch.rest.RestResponse; import org.elasticsearch.rest.RestStatus; import org.elasticsearch.rest.action.RestBuilderListener; -import org.elasticsearch.xpack.core.security.action.saml.SamlCompleteLogoutRequestBuilder; +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 java.io.IOException; @@ -31,44 +32,24 @@ /** * This Rest endpoint handles SAML LogoutResponse sent from idP with either HTTP-Redirect or HTTP-Post binding. - * For HTTP-Redirect binding, it expects {@link Input#queryString} be set to the query string of the redirect URI. - * For HTTP-Post binding, it expects {@link Input#content} be set to the value of SAMLResponse form parameter, i.e. - * caller of this API must do the work to extract the SAMLResponse value from body of the HTTP-Post request. The - * value must also be URL decoded if necessary. + * For HTTP-Redirect binding, it expects {@link SamlCompleteLogoutRequest#getPayload()} be set to the query + * string of the redirect URI. + * For HTTP-Post binding, it expects {@link SamlCompleteLogoutRequest#getPayload} be set to the value of + * SAMLResponse form parameter, i.e. caller of this API must do the work to extract the SAMLResponse value + * from body of the HTTP-Post request. The value must also be URL decoded if necessary. */ public class RestSamlCompleteLogoutAction extends SamlBaseRestHandler{ private static final Logger logger = LogManager.getLogger(RestSamlCompleteLogoutAction.class); - static class Input { - String queryString; - String content; - List ids; - String realm; - - void setQueryString(String queryString) { - this.queryString = queryString; - } - - void setContent(String content) { - this.content = content; - } - - void setIds(List ids) { - this.ids = ids; - } - - void setRealm(String realm) { this.realm = realm;} - } - - static final ObjectParser - PARSER = new ObjectParser<>("saml_complete_logout", RestSamlCompleteLogoutAction.Input::new); + static final ObjectParser + PARSER = new ObjectParser<>("saml_complete_logout", SamlCompleteLogoutRequest::new); static { - PARSER.declareStringOrNull(RestSamlCompleteLogoutAction.Input::setQueryString, new ParseField("queryString")); - PARSER.declareStringOrNull(RestSamlCompleteLogoutAction.Input::setContent, new ParseField("content")); - PARSER.declareStringArray(RestSamlCompleteLogoutAction.Input::setIds, new ParseField("ids")); - PARSER.declareString(RestSamlCompleteLogoutAction.Input::setRealm, new ParseField("realm")); + PARSER.declareStringOrNull(SamlCompleteLogoutRequest::setQueryString, new ParseField("queryString")); + PARSER.declareStringOrNull(SamlCompleteLogoutRequest::setContent, new ParseField("content")); + PARSER.declareStringArray(SamlCompleteLogoutRequest::setValidRequestIds, new ParseField("ids")); + PARSER.declareString(SamlCompleteLogoutRequest::setRealm, new ParseField("realm")); } public RestSamlCompleteLogoutAction(Settings settings, XPackLicenseState licenseState) { @@ -88,23 +69,19 @@ public List routes() { @Override protected RestChannelConsumer innerPrepareRequest(RestRequest request, NodeClient client) throws IOException { try (XContentParser parser = request.contentParser()) { - final RestSamlCompleteLogoutAction.Input input = PARSER.parse(parser, null); + final SamlCompleteLogoutRequest samlCompleteLogoutRequest = PARSER.parse(parser, null); logger.trace("SAML LogoutResponse: [{}...] [{}...] [{}]", - Strings.cleanTruncate(input.queryString, 128), Strings.cleanTruncate(input.content, 128), input.ids); - return channel -> { - final SamlCompleteLogoutRequestBuilder requestBuilder = - new SamlCompleteLogoutRequestBuilder(client) - .queryString(input.queryString).content(input.content) - .validRequestIds(input.ids).authenticatingRealm(input.realm); - requestBuilder.execute(new RestBuilderListener<>(channel) { + Strings.cleanTruncate(samlCompleteLogoutRequest.getQueryString(), 128), + Strings.cleanTruncate(samlCompleteLogoutRequest.getContent(), 128), + samlCompleteLogoutRequest.getValidRequestIds()); + return channel -> client.execute(SamlCompleteLogoutAction.INSTANCE, samlCompleteLogoutRequest, + new RestBuilderListener<>(channel) { @Override public RestResponse buildResponse(SamlCompleteLogoutResponse response, XContentBuilder builder) throws Exception { - builder.startObject() - .endObject(); + builder.startObject().endObject(); return new BytesRestResponse(RestStatus.OK, builder); } }); - }; } } }