Support RP initated single log out #38475

jkakavas · 2019-02-05T23:40:55Z

https://openid.net/specs/openid-connect-session-1_0.html#RPLogout

- Use nimbus oidc sdk - JWE not handled - UserInfo requests not handled

…cation-flows

…alm-authentication-flows

- Make the calls to TokenEndpoint and UserInfoEndpoint asynchrously - Move IdTokenValidator to an instance variable

…alm-authentication-flows

Access Token will not be returned in the implicit flow if the response type is set to "id_token" (as opposed to "id_token token") In such cases, there is no access token to validate and we cannot make requests to the UserInfo endpoint, even if the user has configured the Userinfo endpoint in the configuration

Our tests now handle all applicable known attacks. References: - https://www.nds.ruhr-uni-bochum.de/media/ei/veroeffentlichungen/2017/01/30/oidc-security.pdf - https://www.nds.ruhr-uni-bochum.de/media/ei/veroeffentlichungen/2017/01/13/OIDCSecurity_1.pdf - https://auth0.com/blog/critical-vulnerabilities-in-json-web-token-libraries/

…alm-authentication-flows

Implements RP initiated SLO as described in OpenID Connect Session Management 1.0 Draft 38 https://openid.net/specs/openid-connect-session-1_0.html#RPLogout

elasticmachine · 2019-02-05T23:40:57Z

Pinging @elastic/es-security

albertzaharovits · 2019-02-11T16:56:21Z

...n/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealm.java

@@ -178,8 +184,21 @@ private void buildUserFromClaims(JWTClaimsSet claims, ActionListener<Authenticat
            return;
        }

+        final Map<String, Object> tokenMetadata = new HashMap<>();
+        tokenMetadata.put("id_token_hint", claims.getClaim("id_token_hint"));


Does this claim exist? I believe not. I think we need to put the JWT id token in here.

It;s not a standard claim not. This was handled in 3e67b2b#diff-548117e16c773d2ce359cfabe8ef8a69R222 where the serialized token was added in as a synthetic claim during authentication but I guess this was messed up in a git merge effort somehow. I'll reintroduce this and check that nothing else was inadvertently removed

albertzaharovits · 2019-02-11T16:57:21Z

...n/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealm.java

@@ -178,8 +184,21 @@ private void buildUserFromClaims(JWTClaimsSet claims, ActionListener<Authenticat
            return;
        }

+        final Map<String, Object> tokenMetadata = new HashMap<>();
+        tokenMetadata.put("id_token_hint", claims.getClaim("id_token_hint"));
+        tokenMetadata.put("oidc_realm", this.name());


I think this is redundant since we have the realm name in the Authentication which is serialized together with this metadata.

albertzaharovits · 2019-02-11T16:59:27Z

...in/java/org/elasticsearch/xpack/security/action/oidc/TransportOpenIdConnectLogoutAction.java

+    }
+
+    private OpenIdConnectLogoutResponse buildResponse(Authentication authentication, Map<String, Object> tokenMetadata) {
+        validateAuthenticationAndMetadata(authentication, tokenMetadata);


Feels a bit off to do this validation here, after invalidation succeeded.

Agreed, not sure what I was thinking

albertzaharovits · 2019-02-11T17:11:28Z

...in/java/org/elasticsearch/xpack/security/action/oidc/TransportOpenIdConnectLogoutAction.java

+
+    @Override
+    protected void doExecute(Task task, OpenIdConnectLogoutRequest request, ActionListener<OpenIdConnectLogoutResponse> listener) {
+        invalidateRefreshToken(request.getRefreshToken(), ActionListener.wrap(ignore -> {


I would leave this last, after we make sure the token is valid and the authn token has been invalidated. This way the refresh_token is usable even when this API returns an error (validation or otherwise).
Do you think otherwise?

The idea behind invalidating the refresh token first (both here and in the SAML logout , or the invalidate API) is that since our refresh tokens and access tokens are linked, invalidation usually means both tokens. Now if we invalidate the access token before the refresh token, theoretically we allow for an extra window (while we invalidate the access token) where the refresh token can be used to get a new refresh token, thus persisting access.

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?

to not allow refreshing invalidated access tokens

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.

or that invalidating the refresh token would also invalidate the access token.

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 invalidatedTokenDocument method in the TokenService and call that from the various TransportRealmLogoutAction's. I'll address this in a follow up.

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.

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 think there is value in your suggestion and we could add an invalidatedTokenDocument method in the TokenService and call that from the various TransportRealmLogoutAction's.

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:

Depending on the authorization server's revocation policy, the
revocation of a particular token may cause the revocation of related
tokens and the underlying authorization grant. If the particular
token is a refresh token and the authorization server supports the
revocation of access tokens, then the authorization server SHOULD
also invalidate all access tokens based on the same authorization
grant (see Implementation Note). If the token passed to the request
is an access token, the server MAY revoke the respective refresh
token as well.

I think we should go with SHOULD and invalidate the access token when the refresh token is invalidated.

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

albertzaharovits

Left a few comments, I hope my understanding is correct.

jkakavas · 2019-02-11T20:19:01Z

ci/2 was my bad, but ci/1 failed with

20:17:29 FAILURE 0.75s J0 | SimpleClusterStateIT.testIndicesOptions <<< FAILURES!
20:17:29    > Throwable #1: java.lang.AssertionError: 
20:17:29    > Expected: is <1>
20:17:29    >      but: was <2>
20:17:29    > 	at __randomizedtesting.SeedInfo.seed([816343E7777D5AE4:407A45CD64145436]:0)
20:17:29    > 	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
20:17:29    > 	at org.elasticsearch.cluster.SimpleClusterStateIT.testIndicesOptions(SimpleClusterStateIT.java:264)
20:17:29    > 	at java.lang.Thread.run(Thread.java:748)

albertzaharovits

Left one important change request and some suggestions. Besides that, LGTM for now!

albertzaharovits · 2019-02-13T13:04:48Z

...ty/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java

            } else if (accessToken == null && opConfig.getUserinfoEndpoint() != null) {
                LOGGER.debug("UserInfo endpoint is configured but the OP didn't return an access token so we can't query it");
            } else if (accessToken != null && opConfig.getUserinfoEndpoint() == null) {
                LOGGER.debug("OP returned an access token but the UserInfo endpoint is not configured.");
            }
-            claimsListener.onResponse(verifiedIdTokenClaims);
+            claimsListener.onResponse(enrichedVerifiedIdTokenClaims);


this has to be on the else branch of the if, to avoid erroneously calling claimsListener.onResponse twice.

albertzaharovits · 2019-02-13T14:15:17Z

...ty/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java

+            final JSONObject verifiedIdTokenClaimsObject = verifiedIdTokenClaims.toJSONObject();
+            final JWTClaimsSet idTokenClaim = new JWTClaimsSet.Builder().claim("id_token_hint", idToken.serialize()).build();
+            verifiedIdTokenClaimsObject.merge(idTokenClaim.toJSONObject());
+            final JWTClaimsSet enrichedVerifiedIdTokenClaims = JWTClaimsSet.parse(verifiedIdTokenClaimsObject);


This feels like a hack to me. I would prefer a ActionListener<Tuple<JWT , JWTClaimsSet>> claimsListener listener. This also feels a bit hacky, so you could create a container class, and pass that as the type of the listener. If you don't dig this too, you could use the threadContext.

From my perspective, this would've been the last alternative I would consider, but as it stands it's not a reason against LGTM.

I don't necessarily agree on the order of preference, but this is not a strong opinion. I'm gonna go with using the listener to return the serialized JWT as a String

That was a poor initial choice I think. It ends up with passing around the serialized string or the JWT in internal methods all over the OpenIdConnectAuthenticator.java along with the listener. Even after we have consumed the Id token :/ How would you use the threadContext for this ?

You need to carry the ThreadPool in the OpenIdConnectRealm and into the OpenIdConnectAuthenticatior, and then have threadPool.getThreadContext().putTransient("_oidc_received_id_token", itToken.serialize()) (untested).

I think the threadContext is not a great idea. It can easily be abused and I feel like that would be what this is doing. I wonder if we can pass a Function that takes the verified claims and returns an enriched verified claims?

@jaymode Could you slightly elaborate on what you mean with "pass a Function that takes the verified claims and returns an enriched verified claims" ? I'm thinking that if we will "enrich" the JwtClaimsSet eitherway, what are the benefits compared to the way it's implemented now?

I was thinking about this again and the synthetic claim approach doesn't seem hacky after all. At least not hacky enough to justify the listener or the threadContext approach

I was thinking having a parameter of type Function<JWTClaimsSet, JWTClaimsSet> claimsEnricher that did what this was doing but it’s not necessary. Just a alternative to the other alternatives. I am fine with this as is

@albertzaharovits I'm going to merge this as - is to get the Realm effort moving. I'm open to discussing this again though, this is in no way a dismissal of the feedback. My current thinking is that the current implementation is not hacky enough, especially considering the alternatives, but I'd be open to be swayed

albertzaharovits · 2019-02-13T14:41:15Z

...in/java/org/elasticsearch/xpack/security/action/oidc/TransportOpenIdConnectLogoutAction.java

+                        );
+                    }, listener::onFailure));
+            } catch (IOException e) {
+                logger.debug("Internal error during OpenID Connect Logout");


tracing this back to TokenService#decodeToke (which throws and has a listener... Grrr) it looks like IOException is actually a sign of an invalid token, so I would drop this log line because the error is not internal.

albertzaharovits · 2019-02-13T14:52:24Z

...in/java/org/elasticsearch/xpack/security/action/oidc/TransportOpenIdConnectLogoutAction.java

+            throw new ElasticsearchSecurityException("Authentication did not contain metadata");
+        }
+        if (authentication == null) {
+            throw new ElasticsearchSecurityException("No active authentication");


nit: I see these map to HTTP 500, I think 400 is better. So maybe throw IllegalArgumentException. I don't have a consistent perspective over these stuff (across all APIs), but 500 is the server's fault and 400 is the client's .

This is the client giving the server a valid access token and the server fetching a UserToken from .security that has no Authentication. I would claim this is the server's error if the UserToken was malformed in storage or if it originally created one with no Authentication

You're correct, I was putting all the exceptions thrown in this method in the same bucket. This one is indeed internal server error.

I'll change the following:

if (realm instanceof OpenIdConnectRealm == false) { throw new IllegalArgumentException("Access token is not valid for an OpenID Connect realm"); }

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

jaymode · 2019-02-13T21:57:13Z

...c/main/java/org/elasticsearch/xpack/core/security/action/oidc/OpenIdConnectLogoutAction.java

+
+    @Override
+    public OpenIdConnectLogoutResponse newResponse() {
+        throw new UnsupportedOperationException("usage of Streamable is to be replaced by Writeable");


jaymode · 2019-02-13T21:58:40Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java

@@ -745,6 +748,7 @@ public void onIndexModule(IndexModule module) {
                new ActionHandler<>(OpenIdConnectPrepareAuthenticationAction.INSTANCE,
                    TransportOpenIdConnectPrepareAuthenticationAction.class),
                new ActionHandler<>(OpenIdConnectAuthenticateAction.INSTANCE, TransportOpenIdConnectAuthenticateAction.class),
+            new ActionHandler<>(OpenIdConnectLogoutAction.INSTANCE, TransportOpenIdConnectLogoutAction.class),


nit: can we line these all up?

jaymode · 2019-02-13T21:58:51Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java

@@ -799,6 +803,7 @@ public void onIndexModule(IndexModule module) {
                new RestSamlInvalidateSessionAction(settings, restController, getLicenseState()),
                new RestOpenIdConnectPrepareAuthenticationAction(settings, restController, getLicenseState()),
                new RestOpenIdConnectAuthenticateAction(settings, restController, getLicenseState()),
+            new RestOpenIdConnectLogoutAction(settings, restController, getLicenseState()),


nit: can we line these all up

jaymode · 2019-02-13T22:31:36Z

...ty/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java

+            final JSONObject verifiedIdTokenClaimsObject = verifiedIdTokenClaims.toJSONObject();
+            final JWTClaimsSet idTokenClaim = new JWTClaimsSet.Builder().claim("id_token_hint", idToken.serialize()).build();
+            verifiedIdTokenClaimsObject.merge(idTokenClaim.toJSONObject());
+            final JWTClaimsSet enrichedVerifiedIdTokenClaims = JWTClaimsSet.parse(verifiedIdTokenClaimsObject);


I think the threadContext is not a great idea. It can easily be abused and I feel like that would be what this is doing. I wonder if we can pass a Function that takes the verified claims and returns an enriched verified claims?

…alm-logout

This commit adds an OpenID Connect authentication realm to elasticsearch. Elasticsearch (with the assistance of kibana or another web component) acts as an OpenID Connect Relying Party and supports the Authorization Code Grant and Implicit flows as described in http://ela.st/oidc-spec. It adds support for consuming and verifying signed ID Tokens, both RP initiated and 3rd party initiated Single Sign on and RP initiated signle logout. It also adds an OpenID Connect Provider in the idp-fixture to be used for the associated integration tests. The code in this commit has been tracked in a feature branch and has been previously reviewed and approved in : #37009 #37787 #38474 #38475 #40262

This commit adds an OpenID Connect authentication realm to elasticsearch. Elasticsearch (with the assistance of kibana or another web component) acts as an OpenID Connect Relying Party and supports the Authorization Code Grant and Implicit flows as described in http://ela.st/oidc-spec. It adds support for consuming and verifying signed ID Tokens, both RP initiated and 3rd party initiated Single Sign on and RP initiated signle logout. It also adds an OpenID Connect Provider in the idp-fixture to be used for the associated integration tests. The code in this commit has been tracked in a feature branch and has been previously reviewed and approved in : elastic#37009 elastic#37787 elastic#38474 elastic#38475 elastic#40262

jkakavas added 30 commits January 23, 2019 11:04

Implement code and implicit flows

6448271

- Use nimbus oidc sdk - JWE not handled - UserInfo requests not handled

fix licenses and SHAs for introduced dependencies

92f63ef

Support userinfo requests

19034ce

Merge remote-tracking branch 'origin/master' into oidc-realm-authenti…

aaa5426

…cation-flows

Add few tests

6614057

Fix bugs and add unit tests

b70526a

Merge remote-tracking branch 'origin/feature-oidc-realm' into oidc-re…

422319d

…alm-authentication-flows

Address Feedback

05910d9

- Make the calls to TokenEndpoint and UserInfoEndpoint asynchrously - Move IdTokenValidator to an instance variable

Merge remote-tracking branch 'origin/feature-oidc-realm' into oidc-re…

22e8bb7

…alm-authentication-flows

Merge remote-tracking branch 'origin/feature-oidc-realm' into oidc-re…

ad493a9

…alm-authentication-flows

Fix checkstyle

b3cbd8d

Make JWKSource reloading async

0a0ae0e

Merge remote-tracking branch 'origin/feature-oidc-realm' into oidc-re…

bc1f552

…alm-authentication-flows

fix thirdPartyAudit

baadcdb

Adds logout support to the OIDC realm

56b86eb

address feedback

09413b5

Merge branch 'oidc-realm-authentication-flows' into oidc-realm-logout

300f523

Finalizes logout support

3e67b2b

Implements RP initiated SLO as described in OpenID Connect Session Management 1.0 Draft 38 https://openid.net/specs/openid-connect-session-1_0.html#RPLogout

cleanup

59bc122

Merge branch 'oidc-realm-authentication-flows' into oidc-realm-logout

58e62a9

handle not existing end session url in config

924f427

re-introduce the option for facilitators to pass state and nonce values

86e2ca4

Merge branch 'oidc-realm-authentication-flows' into oidc-realm-logout

79d5971

re-introduce the option for facilitators to pass state and nonce values

dd7023d

Merge branch 'oidc-realm-authentication-flows' into oidc-realm-logout

5db0c04

re-introduce the option for facilitators to pass state and nonce values

b368ffc

Merge branch 'oidc-realm-authentication-flows' into oidc-realm-logout

509c141

remove unused import

81a1770

jkakavas added 3 commits February 3, 2019 21:44

fix tests

3450541

Merge branch 'oidc-realm-authentication-flows' into oidc-realm-logout

42b48b8

Merge branch 'feature-oidc-realm' into oidc-realm-logout

fdf6f9b

jkakavas added >feature :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) labels Feb 5, 2019

jkakavas requested review from jaymode and albertzaharovits February 5, 2019 23:40

albertzaharovits reviewed Feb 11, 2019

View reviewed changes

albertzaharovits requested changes Feb 11, 2019

View reviewed changes

jkakavas added 2 commits February 11, 2019 19:54

address feedback

838b941

Merge branch 'feature-oidc-realm' into oidc-realm-logout

9d92b1c

fix test

a9b37ca

albertzaharovits approved these changes Feb 13, 2019

View reviewed changes

address feedback

4b0e14c

jaymode reviewed Feb 13, 2019

View reviewed changes

jkakavas added 3 commits February 22, 2019 15:15

Merge remote-tracking branch 'origin/feature-oidc-realm' into oidc-re…

25f59bc

…alm-logout

allign

8683b44

Merge branch 'feature-oidc-realm' into oidc-realm-logout

2e3c7d5

jkakavas merged commit 54a0448 into elastic:feature-oidc-realm Feb 26, 2019

jkakavas deleted the oidc-realm-logout branch February 26, 2019 08:49

This was referenced Mar 27, 2019

SAML logout implicitly invalidate the associated refresh token #40523

Open

Add an OpenID Connect authentication realm #40674

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support RP initated single log out #38475

Support RP initated single log out #38475

jkakavas commented Feb 5, 2019

elasticmachine commented Feb 5, 2019

albertzaharovits Feb 11, 2019

jkakavas Feb 11, 2019

albertzaharovits Feb 11, 2019

albertzaharovits Feb 11, 2019

jkakavas Feb 11, 2019

albertzaharovits Feb 11, 2019

jkakavas Feb 11, 2019

albertzaharovits Feb 11, 2019

jkakavas Feb 11, 2019

albertzaharovits Feb 11, 2019

jkakavas Feb 11, 2019

albertzaharovits left a comment

jkakavas commented Feb 11, 2019

albertzaharovits left a comment

albertzaharovits Feb 13, 2019

albertzaharovits Feb 13, 2019

jkakavas Feb 13, 2019

jkakavas Feb 13, 2019

albertzaharovits Feb 13, 2019

jaymode Feb 13, 2019

jkakavas Feb 22, 2019

jaymode Feb 22, 2019

jkakavas Feb 26, 2019

albertzaharovits Feb 13, 2019

albertzaharovits Feb 13, 2019

jkakavas Feb 13, 2019

albertzaharovits Feb 13, 2019

jkakavas Feb 13, 2019

jaymode Feb 13, 2019

jaymode Feb 13, 2019

jaymode Feb 13, 2019

jaymode Feb 13, 2019

Support RP initated single log out #38475

Support RP initated single log out #38475

Conversation

jkakavas commented Feb 5, 2019

elasticmachine commented Feb 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertzaharovits left a comment

Choose a reason for hiding this comment

jkakavas commented Feb 11, 2019

albertzaharovits left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment