Support RP initated single log out #38475
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License; | ||
| * you may not use this file except in compliance with the Elastic License. | ||
| */ | ||
| package org.elasticsearch.xpack.core.security.action.oidc; | ||
|
|
||
| import org.elasticsearch.action.Action; | ||
| import org.elasticsearch.common.io.stream.Writeable; | ||
|
|
||
| public class OpenIdConnectLogoutAction extends Action<OpenIdConnectLogoutResponse> { | ||
|
|
||
| public static final OpenIdConnectLogoutAction INSTANCE = new OpenIdConnectLogoutAction(); | ||
| public static final String NAME = "cluster:admin/xpack/security/oidc/logout"; | ||
|
|
||
| private OpenIdConnectLogoutAction() { | ||
| super(NAME); | ||
| } | ||
|
|
||
| @Override | ||
| public OpenIdConnectLogoutResponse newResponse() { | ||
| throw new UnsupportedOperationException("usage of Streamable is to be replaced by Writeable"); | ||
| } | ||
|
|
||
| @Override | ||
| public Writeable.Reader<OpenIdConnectLogoutResponse> getResponseReader() { | ||
| return OpenIdConnectLogoutResponse::new; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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.oidc; | ||
|
|
||
| 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 org.elasticsearch.common.io.stream.StreamOutput; | ||
|
|
||
| import java.io.IOException; | ||
|
|
||
| import static org.elasticsearch.action.ValidateActions.addValidationError; | ||
|
|
||
| public final class OpenIdConnectLogoutRequest extends ActionRequest { | ||
|
|
||
| private String token; | ||
| @Nullable | ||
| private String refreshToken; | ||
|
|
||
| public OpenIdConnectLogoutRequest() { | ||
|
|
||
| } | ||
|
|
||
| public OpenIdConnectLogoutRequest(StreamInput in) throws IOException { | ||
| super.readFrom(in); | ||
| token = in.readString(); | ||
| refreshToken = in.readOptionalString(); | ||
| } | ||
|
|
||
| @Override | ||
| public ActionRequestValidationException validate() { | ||
| ActionRequestValidationException validationException = null; | ||
| if (Strings.isNullOrEmpty(token)) { | ||
| validationException = addValidationError("token is missing", validationException); | ||
| } | ||
| return validationException; | ||
| } | ||
|
|
||
| public String getToken() { | ||
| return token; | ||
| } | ||
|
|
||
| public void setToken(String token) { | ||
| this.token = token; | ||
| } | ||
|
|
||
| public String getRefreshToken() { | ||
| return refreshToken; | ||
| } | ||
|
|
||
| public void setRefreshToken(String refreshToken) { | ||
| this.refreshToken = refreshToken; | ||
| } | ||
|
|
||
| @Override | ||
| public void writeTo(StreamOutput out) throws IOException { | ||
| super.writeTo(out); | ||
| out.writeString(token); | ||
| out.writeOptionalString(refreshToken); | ||
| } | ||
|
|
||
| @Override | ||
| public void readFrom(StreamInput in) { | ||
| throw new UnsupportedOperationException("usage of Streamable is to be replaced by Writeable"); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| /* | ||
| * 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.oidc; | ||
|
|
||
| import org.elasticsearch.action.ActionResponse; | ||
| import org.elasticsearch.common.io.stream.StreamInput; | ||
| import org.elasticsearch.common.io.stream.StreamOutput; | ||
|
|
||
| import java.io.IOException; | ||
|
|
||
| public final class OpenIdConnectLogoutResponse extends ActionResponse { | ||
|
|
||
| private String endSessionUrl; | ||
|
|
||
| public OpenIdConnectLogoutResponse(StreamInput in) throws IOException { | ||
| super.readFrom(in); | ||
| this.endSessionUrl = in.readString(); | ||
| } | ||
|
|
||
| public OpenIdConnectLogoutResponse(String endSessionUrl) { | ||
| this.endSessionUrl = endSessionUrl; | ||
| } | ||
|
|
||
| @Override | ||
| public void readFrom(StreamInput in) { | ||
| throw new UnsupportedOperationException("usage of Streamable is to be replaced by Writeable"); | ||
| } | ||
|
|
||
| @Override | ||
| public void writeTo(StreamOutput out) throws IOException { | ||
| super.writeTo(out); | ||
| out.writeString(endSessionUrl); | ||
| } | ||
|
|
||
| public String toString() { | ||
| return "{endSessionUrl=" + endSessionUrl + "}"; | ||
| } | ||
|
|
||
| public String getEndSessionUrl() { | ||
| return endSessionUrl; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,140 @@ | ||
| /* | ||
| * 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.oidc; | ||
|
|
||
| import com.nimbusds.jwt.JWT; | ||
| import com.nimbusds.jwt.JWTParser; | ||
| import org.apache.logging.log4j.LogManager; | ||
| import org.apache.logging.log4j.Logger; | ||
| import org.elasticsearch.ElasticsearchSecurityException; | ||
| import org.elasticsearch.action.ActionListener; | ||
| import org.elasticsearch.action.support.ActionFilters; | ||
| import org.elasticsearch.action.support.HandledTransportAction; | ||
| import org.elasticsearch.common.Strings; | ||
| import org.elasticsearch.common.inject.Inject; | ||
| import org.elasticsearch.common.io.stream.Writeable; | ||
| import org.elasticsearch.tasks.Task; | ||
| import org.elasticsearch.transport.TransportService; | ||
| import org.elasticsearch.xpack.core.security.action.oidc.OpenIdConnectLogoutAction; | ||
| import org.elasticsearch.xpack.core.security.action.oidc.OpenIdConnectLogoutRequest; | ||
| import org.elasticsearch.xpack.core.security.action.oidc.OpenIdConnectLogoutResponse; | ||
| import org.elasticsearch.xpack.core.security.authc.Authentication; | ||
| import org.elasticsearch.xpack.core.security.authc.Realm; | ||
| import org.elasticsearch.xpack.core.security.authc.support.TokensInvalidationResult; | ||
| import org.elasticsearch.xpack.core.security.user.User; | ||
| import org.elasticsearch.xpack.security.authc.Realms; | ||
| import org.elasticsearch.xpack.security.authc.TokenService; | ||
| import org.elasticsearch.xpack.security.authc.oidc.OpenIdConnectRealm; | ||
|
|
||
| import java.io.IOException; | ||
| import java.text.ParseException; | ||
| import java.util.Map; | ||
|
|
||
| /** | ||
| * Transport action responsible for generating an OpenID connect logout request to be sent to an OpenID Connect Provider | ||
| */ | ||
| public class TransportOpenIdConnectLogoutAction extends HandledTransportAction<OpenIdConnectLogoutRequest, OpenIdConnectLogoutResponse> { | ||
|
|
||
| private final Realms realms; | ||
| private final TokenService tokenService; | ||
| private static final Logger logger = LogManager.getLogger(TransportOpenIdConnectLogoutAction.class); | ||
|
|
||
| @Inject | ||
| public TransportOpenIdConnectLogoutAction(TransportService transportService, ActionFilters actionFilters, Realms realms, | ||
| TokenService tokenService) { | ||
| super(OpenIdConnectLogoutAction.NAME, transportService, actionFilters, | ||
| (Writeable.Reader<OpenIdConnectLogoutRequest>) OpenIdConnectLogoutRequest::new); | ||
| this.realms = realms; | ||
| this.tokenService = tokenService; | ||
| } | ||
|
|
||
| @Override | ||
| protected void doExecute(Task task, OpenIdConnectLogoutRequest request, ActionListener<OpenIdConnectLogoutResponse> listener) { | ||
| invalidateRefreshToken(request.getRefreshToken(), ActionListener.wrap(ignore -> { | ||
|
There was a problem hiding this comment. I would leave this last, after we make sure the token is valid and the authn token has been invalidated. This way the There was a problem hiding this comment. The idea behind invalidating the refresh token first (both here and in the SAML logout , or the There was a problem hiding this comment. I see, it makes sense, thanks. I think, it would be better to address the root problem, to not allow refreshing invalidated access tokens or that invalidating the refresh token would also invalidate the access token. Should probably be done in a follow-up, WDYT? There was a problem hiding this comment.
This is not the root problem. The root problem here is that we are "logging out a user" and that user has both an access and a refresh token. Since the refresh token can be used to get a new refresh token (and access token) , and since we use this step invalidation process, it makes sense that the refresh token should be invalidated first.
We don't necessarily want to do this. There is a valid use case where a client might want to invalidate the refresh token so that the user can only use their current access token for the remaining duration of it but not be able to refresh. I think there is value in your suggestion and we could add an There was a problem hiding this comment.
Sure we can be flexible here, but the client should not act like he owns it . The authorization server can invalidate tokens even without the client calling an API (to mitigate threats, etc) I am leaning towards this being the wrong kind of flexibility because it doesn't bring any benefit to the client.
I am 👍 to this, but see below: I flagged this as a problem because it makes things "smart". I understand the reasoning from a protocol point of view, but I don't like it as a programmer that these functions should be called in order if they don't pass a return value from one to the other. As a data point, the RFC https://tools.ietf.org/html/rfc7009#section-2.1 says:
I think we should go with SHOULD and invalidate the access token when the refresh token is invalidated. There was a problem hiding this comment. We're digressing but I'm enjoying the discussion :) We had gone back and forth on this also when the refresh functionality was introduced back in January. I don't have a very clear recollection on whether we consciously decided to not invalidate access tokens when invalidating refresh tokens, I do remember we consciously decided to not invalidate refresh tokens when invalidating the corresponding access token. I think your RTFRFC persuaded me, this could also simplify the TokenService code further. I'll be taking it up as a follow up task and we can iterate on that PR more if needed |
||
| try { | ||
| final String token = request.getToken(); | ||
| tokenService.getAuthenticationAndMetaData(token, ActionListener.wrap( | ||
| tuple -> { | ||
| final Authentication authentication = tuple.v1(); | ||
| final Map<String, Object> tokenMetadata = tuple.v2(); | ||
| validateAuthenticationAndMetadata(authentication, tokenMetadata); | ||
| tokenService.invalidateAccessToken(token, ActionListener.wrap( | ||
| result -> { | ||
| if (logger.isTraceEnabled()) { | ||
| logger.trace("OpenID Connect Logout for user [{}] and token [{}...{}]", | ||
| authentication.getUser().principal(), | ||
| token.substring(0, 8), | ||
| token.substring(token.length() - 8)); | ||
| } | ||
| OpenIdConnectLogoutResponse response = buildResponse(authentication, tokenMetadata); | ||
| listener.onResponse(response); | ||
| }, listener::onFailure) | ||
| ); | ||
| }, listener::onFailure)); | ||
| } catch (IOException e) { | ||
| listener.onFailure(e); | ||
| } | ||
| }, listener::onFailure)); | ||
| } | ||
|
|
||
| private OpenIdConnectLogoutResponse buildResponse(Authentication authentication, Map<String, Object> tokenMetadata) { | ||
| final String idTokenHint = (String) getFromMetadata(tokenMetadata, "id_token_hint"); | ||
| final Realm realm = this.realms.realm(authentication.getAuthenticatedBy().getName()); | ||
| final JWT idToken; | ||
| try { | ||
| idToken = JWTParser.parse(idTokenHint); | ||
| } catch (ParseException e) { | ||
| throw new ElasticsearchSecurityException("Token Metadata did not contain a valid IdToken", e); | ||
| } | ||
| return ((OpenIdConnectRealm) realm).buildLogoutResponse(idToken); | ||
| } | ||
|
|
||
| private void validateAuthenticationAndMetadata(Authentication authentication, Map<String, Object> tokenMetadata) { | ||
| if (tokenMetadata == null) { | ||
| throw new ElasticsearchSecurityException("Authentication did not contain metadata"); | ||
| } | ||
| if (authentication == null) { | ||
| throw new ElasticsearchSecurityException("No active authentication"); | ||
|
There was a problem hiding this comment. nit: I see these map to HTTP 500, I think 400 is better. So maybe throw There was a problem hiding this comment. This is the client giving the server a valid access token and the server fetching a UserToken from .security that has no There was a problem hiding this comment. You're correct, I was putting all the exceptions thrown in this method in the same bucket. This one is indeed internal server error. There was a problem hiding this comment. I'll change the following: This can't happen with Kibana, but we mean to allow these APIs to be used by custom web apps that might support more than one authentication realms and in that case these apps should use the correct rest API for the given realm that authenticated their user. Thanks for the feedback on this one |
||
| } | ||
| final User user = authentication.getUser(); | ||
| if (user == null) { | ||
| throw new ElasticsearchSecurityException("No active user"); | ||
| } | ||
|
|
||
| final Authentication.RealmRef ref = authentication.getAuthenticatedBy(); | ||
| if (ref == null || Strings.isNullOrEmpty(ref.getName())) { | ||
| throw new ElasticsearchSecurityException("Authentication {} has no authenticating realm", | ||
| authentication); | ||
| } | ||
| final Realm realm = this.realms.realm(authentication.getAuthenticatedBy().getName()); | ||
| if (realm == null) { | ||
| throw new ElasticsearchSecurityException("Authenticating realm {} does not exist", ref.getName()); | ||
| } | ||
| if (realm instanceof OpenIdConnectRealm == false) { | ||
| throw new IllegalArgumentException("Access token is not valid for an OpenID Connect realm"); | ||
| } | ||
| } | ||
|
|
||
| private Object getFromMetadata(Map<String, Object> metadata, String key) { | ||
| if (metadata.containsKey(key) == false) { | ||
| throw new ElasticsearchSecurityException("Authentication token does not have OpenID Connect metadata [{}]", key); | ||
| } | ||
| Object value = metadata.get(key); | ||
| if (null != value && value instanceof String == false) { | ||
| throw new ElasticsearchSecurityException("In authentication token, OpenID Connect metadata [{}] is [{}] rather than " + | ||
| "String", key, value.getClass()); | ||
| } | ||
| return value; | ||
|
|
||
| } | ||
| private void invalidateRefreshToken(String refreshToken, ActionListener<TokensInvalidationResult> listener) { | ||
| if (refreshToken == null) { | ||
| listener.onResponse(null); | ||
| } else { | ||
| tokenService.invalidateRefreshToken(refreshToken, listener); | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍