Support alternate OIDC providers, to prepare for the switch from Keycloak to `fabric8_auth` #8650

davidfestal · 2018-02-07T10:15:46Z

What does this PR do?

This PR allows switching to an alternate OIDC provider (provided that it emits
access tokens as JWT tokens).

This is the CHE 6 equivalent of PR #8614 that has already been created against the CHE 5 maintenance branch.

What issues does this PR fix or reference?

This is the implementation required in upstream Che, for issues
redhat-developer/rh-che#502 and
redhat-developer/rh-che#525

Changes in dependencies

This PR depends on PR eclipse-che/che-dependencies#96 and the associated CQ.

codenvy-ci · 2018-02-07T10:16:01Z

Can one of the admins verify this patch?

codenvy-ci · 2018-02-07T10:19:01Z

Can one of the admins verify this patch?

codenvy-ci · 2018-02-07T10:19:05Z

Can one of the admins verify this patch?

benoitf · 2018-02-07T10:25:10Z

assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/che/multiuser.properties

@@ -117,3 +121,18 @@ che.keycloak.github.endpoint=NULL

 #     The number of seconds to tolerate for clock skew when verifying exp or nbf claims.
 che.keycloak.allowed_clock_skew_sec=3
+
+#     Use the OIDC optional `nonce` feature to increase security.
+che.keycloak.use_nonce=true


Hello @davidfestal , it seems it's missing properties in che.env

wdym ? I see them in che.env here

benoitf · 2018-02-07T10:26:33Z

dockerfiles/init/manifests/che.env

@@ -543,6 +543,9 @@ CHE_SINGLE_PORT=false
 #CHE_KEYCLOAK_CLIENT__ID=che-public
 #CHE_KEYCLOAK_ALLOWED__CLOCK__SKEW__SEC=3
 #CHE_KEYCLOAK_ADMIN_REQUIRE_UPDATE_PASSWORD=true
+#CHE_KEYCLOAK_USE__NONCE=true


properties should be documented as it's for user editing (while the properties file is quite hidden)

documented in the che.env file or in the docs as a separate PR ?

in che.env like https://github.com/eclipse/che/blob/6897a6e8b45c3084639d748edc802247a90a6bc9/dockerfiles/init/manifests/che.env#L18-L21 (based on what you've commented on properties file

Added the comments in commit 4ab6687

is it what you were expecting @benoitf ?

mshaposhnik · 2018-02-07T12:02:53Z

...er/src/main/java/org/eclipse/che/multiuser/keycloak/server/KeycloakConfigurationService.java

@@ -43,4 +48,25 @@ public KeycloakConfigurationService(KeycloakSettings keycloakSettings) {
  public Map<String, String> settings() {
    return keycloakSettings.get();
  }
+
+  @GET
+  @Path("/OIDCKeycloak.js")


I propose to make it in such way:

@Path("/{filename}") public String javascriptAdapter(@PathParam("filename") String filename) .... Thread.currentThread().getContextClassLoader().getResource("keycloak/" + filename); ... if (file_not_found) throw NotFoundException //(to be like 404 instead of epmty file)

I would left it as is. Your proposal could be dangerous.

@mshaposhnik what happens if I use ../ on filename ? could I get some private files ?

@skabashnyuk @mshaposhnik I'll leave it as is then.

Very unlikely. They should be under java classpath. But *.properties can be read, probably.

mshaposhnik · 2018-02-07T12:53:02Z

assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/che/multiuser.properties

+#     URL to the Keycloak Javascript adapter we want to use.
+#     if set to NULL, then the default used value is  
+#     `${che.keycloak.auth_server_url}/js/keycloak.js`,
+#     or `<che-server>/wsmaster/keycloak/OIDCKeycloak.js`


Maybe call it something like OIDC_client.js ?

well, it's the keycloak.js file modified to be OIDC compatible, but still the Keycloak Javascript adaper, hence this name.

mshaposhnik · 2018-02-07T12:54:52Z

assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/che/multiuser.properties

+#     URL to the Keycloak Javascript adapter we want to use.
+#     if set to NULL, then the default used value is  
+#     `${che.keycloak.auth_server_url}/js/keycloak.js`,
+#     or `<che-server>/wsmaster/keycloak/OIDCKeycloak.js`


There is no /wsmaster URL's. Correct is /api

Fixed in commit 8951eef

Documentation related to PR eclipse-che/che#8650

davidfestal · 2018-03-19T13:29:41Z

Related documentation PR is here

davidfestal · 2018-03-20T18:06:01Z

ci-test

skabashnyuk · 2018-03-20T21:13:47Z

...eycloak-server/src/main/java/org/eclipse/che/multiuser/keycloak/server/KeycloakSettings.java

      @Named(CLIENT_ID_SETTING) String clientId,
      @Nullable @Named(OIDC_PROVIDER_SETTING) String oidcProvider,
      @Named(USE_NONCE_SETTING) boolean useNonce,
      @Nullable @Named(OSO_ENDPOINT_SETTING) String osoEndpoint,
      @Nullable @Named(GITHUB_ENDPOINT_SETTING) String gitHubEndpoint) {

+    if (serverURL == null && oidcProvider == null) {
+        LOG.error("Either the '" + AUTH_SERVER_URL_SETTING + "' or '" + OIDC_PROVIDER_SETTING + "' property should be set");
+        this.settings = Collections.unmodifiableMap(new HashMap<String, String>());


just throw throw new RuntimeExeception("Either the '" + AUTH_SERVER_URL_SETTING + "' or '" + OIDC_PROVIDER_SETTING + "' property should be set")

or this an acceptable mode or not? maybe Collections.emptyMap()?

if it's acceptable why log.error?

Fixed in commit e1e4001

skabashnyuk · 2018-03-20T21:14:00Z

...eycloak-server/src/main/java/org/eclipse/che/multiuser/keycloak/server/KeycloakSettings.java

+
+    if (oidcProvider == null && realm == null) {
+        LOG.error("The '" + REALM_SETTING + "' property should be set");
+        this.settings = Collections.unmodifiableMap(new HashMap<String, String>());


just throw throw new RuntimeExeception

Fixed in commit e1e4001

skabashnyuk · 2018-03-20T21:14:18Z

...eycloak-server/src/main/java/org/eclipse/che/multiuser/keycloak/server/KeycloakSettings.java

+    }
+
+    if (oidcProvider == null && realm == null) {
+        LOG.error("The '" + REALM_SETTING + "' property should be set");


It's better to use parametrised method

skabashnyuk · 2018-03-20T21:18:17Z

Docs pr?

codenvy-ci · 2018-03-20T23:31:38Z

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:8650
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

davidfestal · 2018-03-21T08:05:09Z

Docs pr?

@skabashnyuk see comment #8650 (comment)

Signed-off-by: David Festal <dfestal@redhat.com>

These are exactly the same changes as those already done / approved in the GWT application main HTML file. Signed-off-by: David Festal <dfestal@redhat.com>

Signed-off-by: David Festal <dfestal@redhat.com>

davidfestal · 2018-03-21T15:40:41Z

ci-test

codenvy-ci · 2018-03-21T16:25:49Z

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:8650
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

davidfestal · 2018-03-21T17:25:40Z

ci-test

codenvy-ci · 2018-03-21T19:56:20Z

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:8650
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

dmytro-ndp · 2018-03-22T08:52:59Z

@davidfestal: selenium test launcher failed to create test users because it didn't find che realm "Resource not found for url: http://localhost:8080/auth/admin/realms/che/users".
Also URL to get auth token didn't work as well:

java.io.IOException: Failed access: http://che-eclipse-che.172.30.3.246.nip.io/api/keycloak/settings/?token
...
Application is not available

The application is currently not serving requests at this endpoint. It may not have been started or is still starting.

riuvshin · 2018-03-22T09:33:51Z

@davidfestal from what I see che is not able to star from your branch on ocp
http://pastebin.test.redhat.com/567786

l0rd · 2018-03-22T09:43:38Z

@riuvshin David is in PTO until monday but had commented that yesterday:

It seems that the Keycloak server used to run tests has a problem
Is it expected :

18:24:06 [TEST] Creating default test user with name 'user1521649448'...
18:24:09 [WARN] There is a problem with creation of default test user in Keycloak server: ''.�[0m
18:24:09 Admin user will be used as default test user.
18:24:09 [TEST] Creating second test user with name 'user1521649449'...
18:24:10 [WARN] There is a problem with creation of second test user in Keycloak server: ''.�[0m
18:24:10 Admin user will be used as a second test user.
18:24:11 Logging into http://localhost:8080/auth as user admin of realm master
18:24:11 Resource not found for url: http://localhost:8080/auth/admin/realms/che/users? username=admin&first=0&max=2

It seems that the Keycloak server is not correctly started, so that, finally the .../keycloak/settings endpoint doesn't work correctly.

riuvshin · 2018-03-22T09:58:10Z

@l0rd I deployed it locally, KC server is OK Im able to login as admin to KC but CHE server is down due to problems on start up

in che logs:

2018-03-22 09:31:53,667[ost-startStop-1]  [ERROR] [o.a.c.c.C.[.[localhost].[/api] 4752] - Exception sending context initialized event to listener instance of class [org.eclipse.che.inject.CheBootstrap]
com.google.inject.CreationException: Unable to create injector, see the following errors:
 
1) Error injecting constructor, java.lang.RuntimeException: Exception while retrieving OpenId configuration from endpoint: http://keycloak-eclipse-che.192.168.65.2.nip.io/auth/realms/che/.well-known/openid-configuration
  at org.eclipse.che.multiuser.keycloak.server.KeycloakSettings.<init>(KeycloakSettings.java:61)
  at org.eclipse.che.multiuser.keycloak.server.KeycloakSettings.class(KeycloakSettings.java:48)
  while locating org.eclipse.che.multiuser.keycloak.server.KeycloakSettings
    for the 1st parameter of org.eclipse.che.multiuser.keycloak.server.dao.KeycloakProfileDao.<init>(KeycloakProfileDao.java:46)
  while locating org.eclipse.che.multiuser.keycloak.server.dao.KeycloakProfileDao
  while locating org.eclipse.che.api.user.server.spi.ProfileDao
    for the 2nd parameter of org.eclipse.che.multiuser.api.account.personal.PersonalAccountUserManager.<init>(PersonalAccountUserManager.java:50)
  at org.eclipse.che.multiuser.api.account.personal.PersonalAccountUserManager.class(PersonalAccountUserManager.java:50)
  while locating org.eclipse.che.multiuser.api.account.personal.PersonalAccountUserManager
  while locating org.eclipse.che.api.user.server.UserManager
Caused by: java.lang.RuntimeException: Exception while retrieving OpenId configuration from endpoint: http://keycloak-eclipse-che.192.168.65.2.nip.io/auth/realms/che/.well-known/openid-configuration
        at org.eclipse.che.multiuser.keycloak.server.KeycloakSettings.<init>(KeycloakSettings.java:95)

is that expected url http://keycloak-eclipse-che.192.168.65.2.nip.io/auth/realms/che/.well-known/openid-configuration ?

based on that I can guess that it is calling wrong url OR this PR missing some other changes related to KC

…rs_che6

riuvshin · 2018-03-22T12:46:04Z

ci-test

codenvy-ci · 2018-03-22T18:38:06Z

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:8650
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

artaleks9 · 2018-03-23T13:16:30Z

Test report is OK

* Add documentation for alternate OIDC providers Documentation related to PR eclipse-che/che#8650 * small type fix

…loak to `fabric8_auth` (#8650) Allow switching to an alternate OIDC provider (provided that it emits access tokens as JWT tokens). This is the implementation required in upstream Che, for issues redhat-developer/rh-che#502 and redhat-developer/rh-che#525 Signed-off-by: David Festal <dfestal@redhat.com>

…loak to `fabric8_auth` (eclipse-che#8650) Allow switching to an alternate OIDC provider (provided that it emits access tokens as JWT tokens). This is the implementation required in upstream Che, for issues redhat-developer/rh-che#502 and redhat-developer/rh-che#525 Signed-off-by: David Festal <dfestal@redhat.com>

davidfestal requested review from ashumilova, benoitf, riuvshin, skabashnyuk and vparfonov as code owners February 7, 2018 10:15

davidfestal requested review from l0rd and ibuziuk February 7, 2018 10:16

benoitf added the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Feb 7, 2018

benoitf reviewed Feb 7, 2018

View reviewed changes

skabashnyuk approved these changes Feb 7, 2018

View reviewed changes

skabashnyuk requested a review from mshaposhnik February 7, 2018 10:42

mshaposhnik reviewed Feb 7, 2018

View reviewed changes

davidfestal self-assigned this Feb 7, 2018

benoitf approved these changes Feb 7, 2018

View reviewed changes

mshaposhnik approved these changes Feb 7, 2018

View reviewed changes

davidfestal added a commit to eclipse-che/che-docs that referenced this pull request Mar 19, 2018

Add documentation for alternate OIDC providers

905e8ef

Documentation related to PR eclipse-che/che#8650

davidfestal mentioned this pull request Mar 19, 2018

Add documentation for alternate OIDC providers eclipse-che/che-docs#375

Merged

davidfestal force-pushed the enable_oidc_providers_che6 branch from 8951eef to bf36a52 Compare March 19, 2018 13:27

skabashnyuk requested changes Mar 20, 2018

View reviewed changes

davidfestal added 4 commits March 21, 2018 16:38

Fix @skabashnyuk comments

66b1e14

Signed-off-by: David Festal <dfestal@redhat.com>

Last requested change + formatting

1516b88

Signed-off-by: David Festal <dfestal@redhat.com>

Add required Keycloak changes to the workspace-loader

130576d

These are exactly the same changes as those already done / approved in the GWT application main HTML file. Signed-off-by: David Festal <dfestal@redhat.com>

Missing transitive dependency

7d24572

Signed-off-by: David Festal <dfestal@redhat.com>

davidfestal force-pushed the enable_oidc_providers_che6 branch from 5ca0128 to 7d24572 Compare March 21, 2018 15:39

riuvshin mentioned this pull request Mar 22, 2018

make KC configuration before CHE start in case of deploy to OS #9181

Merged

Merge remote-tracking branch 'origin/master' into enable_oidc_provide…

11e4f12

…rs_che6

artaleks9 approved these changes Mar 23, 2018

View reviewed changes

riuvshin approved these changes Mar 23, 2018

View reviewed changes

davidfestal merged commit ff3459d into master Mar 23, 2018

davidfestal deleted the enable_oidc_providers_che6 branch March 23, 2018 13:45

benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Mar 23, 2018

benoitf added this to the 6.3.0 milestone Mar 23, 2018

bmicklea pushed a commit to eclipse-che/che-docs that referenced this pull request Mar 23, 2018

Add documentation for alternate OIDC providers (#375)

8707b39

* Add documentation for alternate OIDC providers Documentation related to PR eclipse-che/che#8650 * small type fix

This was referenced Mar 23, 2018

Fix deployment che-multi on docker #9222

Merged

CHE tomcat should not fail if KC is not accessible on starting #9235

Closed

davidfestal mentioned this pull request Mar 26, 2018

Che should use Auth API instead of Keycloak for authentication redhat-developer/rh-che#502

Closed

Support alternate OIDC providers, to prepare for the switch from Keycloak to fabric8_auth #8650

Support alternate OIDC providers, to prepare for the switch from Keycloak to fabric8_auth #8650

Conversation

davidfestal commented Feb 7, 2018

What does this PR do?

What issues does this PR fix or reference?

Changes in dependencies

codenvy-ci commented Feb 7, 2018

codenvy-ci commented Feb 7, 2018

codenvy-ci commented Feb 7, 2018

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

mshaposhnik Feb 7, 2018 • 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

mshaposhnik Feb 7, 2018 • edited Loading

Choose a reason for hiding this comment

mshaposhnik Feb 7, 2018 • 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

davidfestal commented Mar 19, 2018

davidfestal commented Mar 20, 2018

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

skabashnyuk commented Mar 20, 2018

codenvy-ci commented Mar 20, 2018

davidfestal commented Mar 21, 2018

davidfestal commented Mar 21, 2018

codenvy-ci commented Mar 21, 2018

davidfestal commented Mar 21, 2018

codenvy-ci commented Mar 21, 2018

dmytro-ndp commented Mar 22, 2018 • edited Loading

riuvshin commented Mar 22, 2018

l0rd commented Mar 22, 2018

riuvshin commented Mar 22, 2018 • edited Loading

riuvshin commented Mar 22, 2018

codenvy-ci commented Mar 22, 2018

artaleks9 commented Mar 23, 2018

Support alternate OIDC providers, to prepare for the switch from Keycloak to `fabric8_auth` #8650

Support alternate OIDC providers, to prepare for the switch from Keycloak to `fabric8_auth` #8650

mshaposhnik Feb 7, 2018 •

edited

Loading

mshaposhnik Feb 7, 2018 •

edited

Loading

mshaposhnik Feb 7, 2018 •

edited

Loading

dmytro-ndp commented Mar 22, 2018 •

edited

Loading

riuvshin commented Mar 22, 2018 •

edited

Loading