OIDC realm authentication flows #37787

jkakavas · 2019-01-23T21:31:18Z

Adds implementation for authentication in the OIDC realm using the authorization code grant and the implicit flows as described in https://openid.net/specs/openid-connect-core-1_0.html

TODO: ~~Tests~~ Most unit tests are added here, IT will be on its own PR.

Documentation will be handled in a different PR.

Better late than never, the flow diagrams for the implicit flow in https://drive.google.com/open?id=1FF9LltN7qBUdGO98zLfCB8R55WQc1spL might help with the review

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

elasticmachine · 2019-01-23T21:31:19Z

Pinging @elastic/es-security

…cation-flows

albertzaharovits · 2019-01-25T14:01:53Z

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

+            AccessToken accessToken;
+            if (rpConfig.getResponseType().impliesCodeFlow()) {
+                final AuthorizationCode code = response.getAuthorizationCode();
+                Tuple<AccessToken, JWT> tokens = exchangeCodeForToken(code);


I believe this should be async.

…alm-authentication-flows

jaymode

I think we'll need to put some more work in unfortunately to prevent blocking of network threads. Authentication will typically happen on a network thread and operations on a network thread should be non-blocking otherwise there is the potential to make a node unresponsive.

...c/main/java/org/elasticsearch/xpack/core/security/authc/oidc/OpenIdConnectRealmSettings.java

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

jaymode · 2019-01-25T20:02:14Z

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

+            final ClientAuthentication clientAuth = new ClientSecretBasic(rpConfig.getClientId(), clientSecret);
+            final AuthorizationGrant codeGrant = new AuthorizationCodeGrant(code, rpConfig.getRedirectUri());
+            final TokenRequest tokenRequest = new TokenRequest(opConfig.getTokenEndpoint(), clientAuth, codeGrant);
+            final HTTPResponse httpResponse = getResponse(buildRequest(tokenRequest));


as @albertzaharovits mentioned already, we need to make this asynchronous. We have a few options:

fork to the generic threadpool and execute this blocking call there

add HTTPRequest#send as a forbidden api, add http-asyncclient as a dependency and use it to execute asynchronously.

I'm in favor of option 2 even though it is more work. It feels more like the right thing to do.

I went with option 2. Do you think it's necessary to add the HTTPRequest#send as a forbidden API?

Yeah I do think we should. The reason being that it might not be obvious to someone else that it should not be used

jaymode · 2019-01-25T20:09:25Z

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

+            } else {
+                String jwkSetPath = opConfig.getJwkSetPath();
+                if (jwkSetPath.startsWith("https://")) {
+                    validator = new IDTokenValidator(opConfig.getIssuer(), rpConfig.getClientId(), requestedAlgorithm,


If we want to use this, we need to fork to a different threadpool as they expect a synchronous resolution or go async to retrieve the content ourselves and then use JWKSet.parse on the returned body content

I think we would have to do some caching to not reach out for JWKSet for each new authn. For that matter I think it's better to let the IDTokenValidator handle its business on a separate thread and not build the IDTokenValidator everytime (make it an instance variable).

Good point regarding making IDTokenValidator an instance variable. It is thread safe. I do wonder if we should have some sort of built in update mechanism for obtaining the jwkset rather than having the token validator reach out for every auth.

I moved IDTokenValidator an instance variable and since it's thread safe I think we'll be fine. WDYT ?
It won't reach out for the jwkset on each authn, as there is a simple built in caching mechanism. It will only reach out again, in case it receives a JWT that carries a kid in its Header that the IDTokenValidator has not seen before ( the use case is OP key rotation )

I also added a FileWatcher to make sure that we pick up changes to the JWKSet if it's provided in a local file

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

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

...c/main/java/org/elasticsearch/xpack/core/security/authc/oidc/OpenIdConnectRealmSettings.java

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

...c/main/java/org/elasticsearch/xpack/core/security/authc/oidc/OpenIdConnectRealmSettings.java

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

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

...c/main/java/org/elasticsearch/xpack/core/security/authc/oidc/OpenIdConnectRealmSettings.java

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

jkakavas · 2019-01-29T15:28:45Z

@elasticmachine test this please

…alm-authentication-flows

jkakavas · 2019-02-01T12:35:11Z

@jaymode I'd appreciate a round of 👀

jaymode

LGTM. Lets see if @albertzaharovits can also take a final pass

jaymode · 2019-02-01T19:59:06Z

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

@@ -219,11 +225,26 @@ private void getUserClaims(@Nullable AccessToken accessToken, JWT idToken, Nonce
                claimsListener.onResponse(verifiedIdTokenClaims);
            }
        } catch (com.nimbusds.oauth2.sdk.ParseException | JOSEException | BadJOSEException e) {


I think it is easier to read if we catch BadJOSEException separately:

try { ... } catch (BadJOSEException e) { if (shouldRetry && ... ) { } else { claimsListener.onFailure(new ElasticsearchSecurityException("Failed to parse or validate the ID Token", e)); } } catch (com.nimbusds.oauth2.sdk.ParseException | JOSEException e) { claimsListener.onFailure(new ElasticsearchSecurityException("Failed to parse or validate the ID Token", e)); }

jaymode · 2019-02-01T20:04:15Z

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

-        private SSLContext clientContext;
-        private HostnameVerifier verifier;
-        private RealmConfig config;
+        private volatile JWKSet cachedJwkSet = null;


can we use new JWKSet() instead of null? Otherwise there is a chance we get a NPE in the get method. If you prefer null, then the get method should be something like:

JWKSet local = this.jwkSet; if (local != null) { return jwkSelector.select(local); } else { return Collections.emptyList(); }

I will change it but I think null is fine too because JWKSelector#select explicitly handles it

/** * Selects the keys from the specified JWK set according to the * matcher's criteria. * * @param jwkSet The JWK set. May be {@code null}. * * @return The selected keys, ordered by their position in the JWK set, * empty list if none were matched or the JWK is {@code null}. */

jkakavas · 2019-02-04T05:44:49Z

@elasticmachine run elasticsearch-ci/1

jkakavas · 2019-02-04T05:46:21Z

@elasticmachine run elasticsearch-ci/2

jkakavas · 2019-02-04T07:39:39Z

@albertzaharovits I'd appreciate a final round as I have a couple of small PRs I want to open that depend on this one 🙏

albertzaharovits

LGTM, This is solid work Ioannis!
I have left a few suggestions, but I don't think another review round is necessary.

albertzaharovits · 2019-02-04T19:38:36Z

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

+                                    + claimValueObject.getClass().getName());
+                            } else {
+                                values = (List<String>) claimValueObject;
+                            }


nit:

} else if (claimValueObject instanceof List) { values = (List<String>) claimValueObject; } else { throw new SettingsException("Setting [" + RealmSettings.getFullSettingKey(realmConfig, setting.getClaim()) + " expects a claim with String or a String Array value but found a " + claimValueObject.getClass().getName()); }

albertzaharovits · 2019-02-04T20:13:09Z

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

+        this.rpConfiguration = buildRelyingPartyConfiguration(config);
+        this.opConfiguration = buildOpenIdConnectProviderConfiguration(config);
+        this.openIdConnectAuthenticator =
+            new OpenIdConnectAuthenticator(config, opConfiguration, rpConfiguration, sslService, watcherService);


nit:
As a best practice, I try to put everything that throws in a constructor, first, and any Closeable last (OpenIdConnectAuthenticator).

albertzaharovits · 2019-02-04T20:15:28Z

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

+    private final CloseableHttpAsyncClient httpClient;
+    private final ResourceWatcherService watcherService;
+
+    protected final Logger logger = LogManager.getLogger(getClass());


nit:
We prefer these static.

albertzaharovits · 2019-02-04T21:39:15Z

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

+                    CloseableHttpAsyncClient httpAsyncClient = HttpAsyncClients.custom()
+                        .setConnectionManager(connectionManager)
+                        .setSSLContext(clientContext)
+                        .setSSLHostnameVerifier(verifier)


Can you please double check that hostnameVerifier works together with connectionManger. I see they overlap a little.

Good catch !!! Setting the connection manager explicitly overrides the SslContext :/

albertzaharovits · 2019-02-04T22:14:10Z

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

+    private void setMetadataFileWatcher(String jwkSetPath) throws IOException {
+        final Path path = realmConfig.env().configFile().resolve(jwkSetPath);
+        FileWatcher watcher = new FileWatcher(path);
+        watcher.addListener(new FileListener(logger, () -> this.idTokenValidator = createIdTokenValidator()));


idTokenValidator should be AtomicReference .

albertzaharovits · 2019-02-04T22:27:53Z

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

+                    }, ex -> {
+                        logger.trace("Attempted and failed to refresh JWK cache upon token validation failure", e);
+                        claimsListener.onFailure(ex);
+                    }));


Can we please, please move this "should retry" check inside a method in idTokenValidator.

And I would leave the messageSigned JWT rejected: Another algorithm expected, or no matching key(s) found out, because I think in the case of the HTTP JWTKeySets the Exception type is enough of a check. If they change the message when we update the library we would blindly loose the ability to rotate keys on OP side.

And I would leave the messageSigned JWT rejected: Another algorithm expected, or no matching key(s) found out, because I think in the case of the HTTP JWTKeySets the Exception type is enough of a check. If they change the message when we update the library we would blindly loose the ability to rotate keys on OP side.

Yeap, makes sense.

Can we please, please move this "should retry" check inside a method in idTokenValidator.

You mean extend IDTokenValidator and add the method there or inside a method in idTokenValidator. == inside a method in OpenIdConnectAuthenticator ?.

albertzaharovits · 2019-02-04T22:30:17Z

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

+            if (logger.isTraceEnabled()) {
+                logger.trace("Received and validated the Id Token for the user: [{}]", verifiedIdTokenClaims);
+            }
+            if (accessToken != null && opConfig.getUserinfoEndpoint() != null) {


if accessToken != null && opConfig.getUserinfoEndpoint() == null we do nothing. I think at least logging should be done. For example if the URL is wrong, getAndCombineUserInfoClaims would throw ElasticsearchSecurityException .

accessToken != null && opConfig.getUserinfoEndpoint() == null is a valid case where the OP returned an access token but we haven't configured a userInfoEndpoint in our settings because we are not interested in hitting it ( all necessary claims are in the ID token ). The same goes for accessToken == null && opConfig.getUserinfoEndpoint() != null: We have configured an userinfo endpoint but the OP didn't return an access token (maybe we misconfigured our response type to id_token instead of token id_token, so we can't make a request to the user info endpoint. Logging the latter wouldn't hurt, you're right :)

For example if the URL is wrong, getAndCombineUserInfoClaims would throw ElasticsearchSecurityException .

If the URL is invalid, we would throw on building the realm config. If it is valid but wrong, we would catch that in getAndCombineUserInfoClaims and call onFailure(new ElasticsearchSecurityException("Failed to get claims from the Userinfo Endpoint.",

jkakavas · 2019-02-05T00:08:59Z

Thanks for the feedback Albert, that was really valuable. I 'll merge this to the feature branch on green CI and we can re-iterate any of the points in the merge of the feature branch to master

jkakavas · 2019-02-05T07:14:12Z

I keep hitting #36816, I'll try and get this muted before re-invoking the CI

jkakavas · 2019-02-05T10:48:51Z

@elasticmachine please run elasticsearch-ci/packaging-sample

This reverts commit 6098583.

jkakavas · 2019-02-05T14:25:15Z

16:07:29 Tests with failures:
16:07:29 
16:07:29   - org.elasticsearch.xpack.monitoring.collector.indices.IndexRecoveryMonitoringDocTests.testToUTC

REPRODUCE WITH: ./gradlew :x-pack:plugin:monitoring:unitTest -Dtests.seed=B110FE213E2190D6 -Dtests.class=org.elasticsearch.xpack.monitoring.collector.indices.IndexRecoveryMonitoringDocTests -Dtests.method="testToUTC" -Dtests.security.manager=true -Dtests.locale=sr-CS -Dtests.timezone=Africa/Lubumbashi -Dcompiler.java=11 -Druntime.java=8

which doesn't reproduce locally, so I'll roll the dice and ask @elasticmachine to run elasticsearch-ci/2 once more

jkakavas · 2019-02-05T14:30:35Z

@elasticmachine run elasticsearch-ci/2 ?

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 3 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

jkakavas added >feature WIP v7.0.0 :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) labels Jan 23, 2019

jkakavas mentioned this pull request Jan 24, 2019

[WIP] OIDC Realm JWT+JWS related functionality #37272

Closed

jkakavas added 2 commits January 24, 2019 14:13

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

aaa5426

…cation-flows

Add few tests

6614057

albertzaharovits reviewed Jan 25, 2019

View reviewed changes

jkakavas added 2 commits January 25, 2019 18:02

Fix bugs and add unit tests

b70526a

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

422319d

…alm-authentication-flows

jkakavas force-pushed the oidc-realm-authentication-flows branch from 8d50fb1 to 422319d Compare January 25, 2019 16:12

jkakavas requested review from jaymode and albertzaharovits January 25, 2019 16:13

jkakavas changed the title ~~[WIP] OIDC realm authentication flows~~ OIDC realm authentication flows Jan 25, 2019

jkakavas removed the WIP label Jan 25, 2019

jaymode requested changes Jan 25, 2019

View reviewed changes

albertzaharovits reviewed Jan 27, 2019

View reviewed changes

...c/main/java/org/elasticsearch/xpack/core/security/authc/oidc/OpenIdConnectRealmSettings.java Outdated Show resolved Hide resolved

albertzaharovits reviewed Jan 27, 2019

View reviewed changes

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

albertzaharovits reviewed Jan 27, 2019

View reviewed changes

...ty/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java Outdated Show resolved Hide resolved

albertzaharovits reviewed Jan 27, 2019

View reviewed changes

...ty/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java Outdated Show resolved Hide resolved

albertzaharovits reviewed Jan 27, 2019

View reviewed changes

...c/main/java/org/elasticsearch/xpack/core/security/authc/oidc/OpenIdConnectRealmSettings.java Outdated Show resolved Hide resolved

albertzaharovits reviewed Jan 27, 2019

View reviewed changes

...n/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealm.java Outdated Show resolved Hide resolved

albertzaharovits reviewed Jan 27, 2019

View reviewed changes

...ty/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java Outdated Show resolved Hide resolved

albertzaharovits reviewed Jan 27, 2019

View reviewed changes

...c/main/java/org/elasticsearch/xpack/core/security/authc/oidc/OpenIdConnectRealmSettings.java Show resolved Hide resolved

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…

bc1f552

…alm-authentication-flows

fix thirdPartyAudit

baadcdb

jaymode approved these changes Feb 1, 2019

View reviewed changes

jkakavas added 6 commits February 2, 2019 00:22

address feedback

09413b5

cleanup

59bc122

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

b368ffc

remove unused import

81a1770

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

c9c1cf7

fix tests

3450541

albertzaharovits approved these changes Feb 4, 2019

View reviewed changes

address feedback

9d30bd2

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

82d23a6

Handle deprecation header-AbstractUpgradeTestCase

6098583

Revert "Handle deprecation header-AbstractUpgradeTestCase"

fa7c024

This reverts commit 6098583.

jkakavas merged commit e5859fd into elastic:feature-oidc-realm Feb 5, 2019

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

jkakavas mentioned this pull request Apr 1, 2019

Add an OpenID Connect authentication realm #40674

Merged

OIDC realm authentication flows #37787

OIDC realm authentication flows #37787

Conversation

jkakavas commented Jan 23, 2019 • edited Loading

elasticmachine commented Jan 23, 2019

Choose a reason for hiding this comment

jaymode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkakavas Jan 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkakavas commented Jan 29, 2019 • edited Loading

jkakavas commented Feb 1, 2019

jaymode 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

jkakavas commented Feb 4, 2019

jkakavas commented Feb 4, 2019

jkakavas commented Feb 4, 2019 • edited Loading

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

jkakavas commented Feb 5, 2019 • edited Loading

jkakavas commented Feb 5, 2019

jkakavas commented Feb 5, 2019

jkakavas commented Feb 5, 2019

jkakavas commented Feb 5, 2019

jkakavas commented Jan 23, 2019 •

edited

Loading

jkakavas Jan 29, 2019 •

edited

Loading

jkakavas commented Jan 29, 2019 •

edited

Loading

jkakavas commented Feb 4, 2019 •

edited

Loading

jkakavas commented Feb 5, 2019 •

edited

Loading