Add OIDC authentication Integration Tests by jkakavas · Pull Request #40262 · elastic/elasticsearch

jkakavas · 2019-03-20T14:48:06Z

This change introduces an OpenID Connect Provider in idp-fixture
that uses the existing openldap server for authentication and
as a user info repository.

It also adds tests where a mock facilitator application (HttpClient)
performs authentication using Elasticsearch's oidc realm against
that OpenID Connect Provider, using both the authorization code
and the implicit grant flows.

This change introduces an OpenID Connect Provider in idp-fixture that uses the existing openldap server for authentication and as a user info repository. It also adds tests where a mock facilitator application (HttpClient) performs authentication using Elasticsearch's oidc realm against that OpenID Connect Provider, using both the authorization code and the implicit grant flows.

elasticmachine · 2019-03-20T14:48:08Z

Pinging @elastic/es-security

jkakavas · 2019-03-20T16:33:21Z

@elasticmachine test this please

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

jaymode · 2019-03-21T20:47:19Z

x-pack/test/idp-fixture/docker-compose.yml

+    depends_on:
+      - openldap
+    ports:
+      - "8080:8080"


can we avoid the hardcoded port? See #40333 for my suggestion on how to do it for the rest of the fixture.

yes! Will do :) I wasn't aware this could be handled this way, thanks!

x-pack/plugin/security/build.gradle

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

...-op-tests/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthIT.java

bizybot

LGTM, Thank you.

alpar-t

I left some comments on the build side.

x-pack/qa/oidc-op-tests/build.gradle

alpar-t · 2019-03-28T10:44:49Z

x-pack/qa/oidc-op-tests/build.gradle

+task setupPorts {
+    dependsOn idpFixtureProject.postProcessFixture
+    doLast {
+        ephemeralPort = idpFixtureProject.postProcessFixture.ext."test.fixtures.oidc-provider.tcp.8080"


This won't work on hosts without docker -Dtests.fixture.enabled=false can be used to simulate it.
I think it also needs a fix from #40297

I raised #40585 because I wasn't sure how to do this properly and will be integrating whatever solution we come up with here to. Did you mean something like https://github.com/elastic/elasticsearch/pull/40297/files#diff-4fbfb78d054fdb66e0b02f321c8d63c5R79?

This won't work on hosts without docker

@atorok these kind of projects only contain integration tests that depend on systems running on Docker so could we have a generic way of "Don't even consider this project if Docker is not available" instead of simply disabling the integTest task as I did in #40585 ?

…alm-IT

jaymode

I do not like the idea of checking in a web-app, ldapAuth. Is this something we could provision and/or build a image with it already there and just override the configuration?

jaymode · 2019-03-28T14:05:19Z

x-pack/qa/oidc-op-tests/build.gradle

+    setting 'xpack.security.authc.realms.oidc.c2id-implicit.claims.name', 'name'
+    setting 'xpack.security.authc.realms.oidc.c2id-implicit.claims.mail', 'email'
+    // Allow for troubleshooting CI errors
+    setting 'logger.org.elasticsearch.xpack.security.authc', 'TRACE'


Are you expecting this to fail in CI? I'd much rather leave this out because who knows what this could hide; maybe it slows things down enough that we miss a concurrency bug

Are you expecting this to fail in CI?

No :) , the idea was to have logs available for when it will - inevitably - fail . I see your point though, will remove this

jaymode · 2019-03-28T14:08:24Z

...-op-tests/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthIT.java

+    }
+
+    private CloseableHttpClient getHttpClient() {
+        return HttpClients.custom().build();


Suggested change

return HttpClients.custom().build();

return HttpClients.createDefault();

jkakavas · 2019-03-28T14:39:32Z

I do not like the idea of checking in a web-app, ldapAuth.

Well, we don't need to check the web app in - it is part of the image. The only reason we map this volume is so that we can replace the configuration of it (ldapAuth/WEB-INF/ldapAuth.properties) and since the authors of the image didn't make that configurable (like they did with override.properties for the c2id server ) I couldn't find a better way to do this.
What I have tried :

try and map only the file with
```
- ./oidc/ldapAuth.properties:/c2id-server/tomcat/webapps/ldapauth/WEB-INF/ldapAuth.properties
```
This doesn't work as when docker maps this, the /c2id-server/tomcat/webapps/ is not yet created ( Tomcat hasn't still deployed the webapp ) and this means that we get /c2id-server/tomcat/webapps/ldapauth that only contains /c2id-server/tomcat/webapps/ldapauth/WEB-INF/ldapAuth.properties and no deployed application - tomcat fails to deploy this.

Is this something we could provision and/or build a image with it already there and just override the configuration?

Yes I can build an image based on theirs and replace ldapAuth.properties in the Dockerfile. I tried to stay away from building the image at runtime (as @atorok mentioned this would be slower and thus not desirable in general ), but where would you see us hosting the docker image ?

jaymode · 2019-03-28T15:00:05Z

Maybe there is a way to add a doLast on the postProcessFixture that calls docker cp?

jkakavas · 2019-03-28T15:08:11Z

Maybe there is a way to add a doLast on the postProcessFixture that calls docker cp?

That wouldn't work as ldapAuth would already be loaded by tomcat at that point and it doesn't reload config changes without restart. I'd need to see if the provided tomcat exposes its manager on 8080 and try and also do a curl to localhost:ephemeralPort/manager/text/reload?path=/ldapAuth

This change makes it so we use the internal test user (alice) of c2id image for testing. This in turn removes the need to configure ldapAuth and the dependency to openldap.

jkakavas · 2019-03-29T10:11:21Z

@jaymode I stopped using the ldapAuth application for our testing. What we care about in this context is the retrieved claims we get from the Provider, and not how the provider sourced them (internal file, ldap, etc) . This means we don't have to deal with the issue of configuring ldapAuth anymore.

jaymode

I left one question, otherwise LGTM

x-pack/qa/oidc-op-tests/build.gradle

alpar-t

Left a comment, other than that the build LGTM

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 2 commits March 20, 2019 16:40

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

67c3f63

jkakavas added :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) v8.0.0 v7.2.0 labels Mar 20, 2019

jkakavas requested review from bizybot and jaymode March 20, 2019 14:48

remove superfluous config examples

5419310

jkakavas added 2 commits March 20, 2019 20:47

Replace java mail with jakarta mail (see elastic#40088)

cd0b221

update jar SHA

312807f

jaymode reviewed Mar 21, 2019

View reviewed changes

bizybot reviewed Mar 26, 2019

View reviewed changes

jkakavas added 6 commits March 26, 2019 16:27

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

368f0f6

address feedback

6bd3ecb

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

50e8741

Adjust after TokenService refactoring

e76f1da

add necessary files for jakarta.mail

670df71

use ephemeral ports instead of hardcoded ones for oidc-provider

c583a9a

jkakavas requested review from bizybot and jaymode and removed request for bizybot March 27, 2019 08:33

Adjust time fixing in tests to reduce possibility for random failures

5147c2c

bizybot approved these changes Mar 28, 2019

View reviewed changes

alpar-t reviewed Mar 28, 2019

View reviewed changes

jkakavas added 2 commits March 28, 2019 13:42

address feedback

a6ce674

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

9f24682

…alm-IT

jkakavas requested a review from alpar-t March 28, 2019 13:25

jaymode requested changes Mar 28, 2019

View reviewed changes

Use c2id internal user for testing

fb76140

This change makes it so we use the internal test user (alice) of c2id image for testing. This in turn removes the need to configure ldapAuth and the dependency to openldap.

jkakavas requested a review from jaymode March 29, 2019 10:11

jaymode approved these changes Mar 29, 2019

View reviewed changes

x-pack/qa/oidc-op-tests/build.gradle Outdated Show resolved Hide resolved

address feedback

b21abec

alpar-t approved these changes Mar 29, 2019

View reviewed changes

move onlyIf in the task

b4fac58

jkakavas merged commit 948aa39 into elastic:feature-oidc-realm Mar 30, 2019

jkakavas deleted the oidc-realm-IT branch March 30, 2019 09:33

jkakavas mentioned this pull request Apr 1, 2019

Add an OpenID Connect authentication realm #40674

Merged

jkakavas mentioned this pull request Apr 17, 2019

Security Tokens moved to a new separate index #40742

Merged

colings86 added >non-issue >test Issues or PRs that are addressing/adding tests and removed >non-issue labels Jun 18, 2019

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

	return HttpClients.custom().build();
	return HttpClients.createDefault();

Conversation

jkakavas commented Mar 20, 2019

Uh oh!

elasticmachine commented Mar 20, 2019

Uh oh!

jkakavas commented Mar 20, 2019

Uh oh!

Uh oh!

jaymode Mar 21, 2019

Choose a reason for hiding this comment

Uh oh!

jkakavas Mar 21, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bizybot left a comment

Choose a reason for hiding this comment

Uh oh!

alpar-t left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alpar-t Mar 28, 2019

Choose a reason for hiding this comment

Uh oh!

jkakavas Mar 28, 2019

Choose a reason for hiding this comment

Uh oh!

jkakavas Mar 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaymode left a comment

Choose a reason for hiding this comment

Uh oh!

jaymode Mar 28, 2019

Choose a reason for hiding this comment

Uh oh!

jkakavas Mar 28, 2019

Choose a reason for hiding this comment

Uh oh!

jaymode Mar 28, 2019

Choose a reason for hiding this comment

Uh oh!

jkakavas commented Mar 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaymode commented Mar 28, 2019

Uh oh!

jkakavas commented Mar 28, 2019

Uh oh!

jkakavas commented Mar 29, 2019

Uh oh!

jaymode left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alpar-t left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jkakavas Mar 28, 2019 •

edited

Loading

jkakavas commented Mar 28, 2019 •

edited

Loading