Oidc additional client auth types #58708

jkakavas · 2020-06-29T22:56:15Z

The OpenID Connect specification defines a number of ways for a
client (RP) to authenticate itself to the OP when accessing the
Token Endpoint. We currently only support client_secret_basic.

This change introduces support for 2 additional authentication
methods, namely client_secret_post (where the client credentials
are passed in the body of the POST request to the OP) and
client_secret_jwt where the client constructs a JWT and signs
it using the the client secret as a key.

Support for the above, and especially client_secret_jwt in our
integration tests meant that the OP we use ( Connect2id server )
should be able to validate the JWT that we send it from the RP
(which contains the Token Endpoint as the aud claim ).
Since we run the OP in docker and it listens on an ephemeral port
we would have no way of knowing the port so that we can configure
the ES running via the testcluster to know the "correct" Token
Endpoint, and even if we did, this would not be the Token Endpoint
URL that the OP would think it listens on. To alleviate this, we
run an ES single node cluster in docker, alongside the OP so that
we can configured it with the correct hostname and port within
the docker network.

The OpenID Connect specification defines a number of ways for a client (RP) to authenticate itself to the OP when accessing the Token Endpoint. We currently only support `client_secret_basic`. This change introduces support for 2 additional authentication methods, namely `client_secret_post` (where the client credentials are passed in the body of the POST request to the OP) and `client_secret_jwt` where the client constructs a JWT and signs it using the the client secret as a key. Support for the above, and especially `client_secret_jwt` in our integration tests meant that the OP we use ( Connect2id server ) should be able to validate the JWT that we send it from the RP. Since we run the OP in docker and it listens on an ephemeral port we would have no way of knowing the port so that we can configure the ES running via the testcluster to know the "correct" Token Endpoint, and even if we did, this would not be the Token Endpoint URL that the OP would think it listens on. To alleviate this, we run an ES single node cluster in docker, alongside the OP so that we can configured it with the correct hostname and port within the docker network.

…ent-auth-types

elasticmachine · 2020-06-29T22:56:17Z

Pinging @elastic/es-security (:Security/Authentication)

jkakavas · 2020-06-29T22:58:06Z

@mark-vieira added you for a +1 for the docker-compose changes since we chatted about this

ywangd

I still need go through the tests more carefully (they are new to me). Here are a few comments/questions to start. Thanks!

ywangd · 2020-06-30T13:28:48Z

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

+            } else {
+                tokensListener.onFailure(new ElasticsearchSecurityException("Failed to exchange code for Id Token using Token Endpoint." +
+                    "Expected client authentication method to be one of " + OpenIdConnectRealmSettings.CLIENT_AUTH_METHODS + " but was " +
+                    rpConfig.getClientAuthenticationMethod()));


Nit: The name or value is usually placed inside a pair of brackets in error messages, .e.g ... but was [xxx].

ywangd · 2020-06-30T13:33:49Z

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

+                for (Map.Entry<String, List<String>> entry : clientSecretJWT.toParameters().entrySet()) {
+                    // Both client_assertion and client_assertion_type are singleton lists
+                    params.add(new BasicNameValuePair(entry.getKey(), entry.getValue().get(0)));
+                }


Maybe I am missing something obvious. But client_id is not added as one of the request parameters? The map returned from ClientSecretJWT.toParameters() only contains client_assertion and client_assertion_type if I read the code correctly.

But client_id is not added as one of the request parameters?

It's not, it doesn't need to be. It is added as the iss claim of the JWT and validated from there.

only contains client_assertion and client_assertion_type

yes, these are the two parameters needed, see https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication

You are right. For a moment, I forgot how JWT is organized. It's too late for me ...

I know that feeling !!! :) No need to go through this today, it can definitely wait. I raised the PR because I had it ready since last week

ywangd · 2020-06-30T13:37:11Z

...c/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticatorTests.java

@@ -841,6 +842,8 @@ private RelyingPartyConfiguration getDefaultRpConfig() throws URISyntaxException
            new ResponseType("id_token", "token"),
            new Scope("openid"),
            JWSAlgorithm.RS384,
+            ClientAuthenticationMethod.CLIENT_SECRET_BASIC,
+            JWSAlgorithm.HS384,
            new URI("https://rp.elastic.co/successfull_logout"));
    }
    private RelyingPartyConfiguration getRpConfig(String alg) throws URISyntaxException {


Nit: need a blank line before this method.

ywangd · 2020-06-30T13:46:37Z

...c/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealmSettingsTests.java

+            .put(getFullSettingKey(REALM_NAME, OpenIdConnectRealmSettings.PRINCIPAL_CLAIM.getClaim()), "sub")
+            .put(getFullSettingKey(REALM_NAME, OpenIdConnectRealmSettings.RP_REDIRECT_URI), "https://rp.my.com")
+            .put(getFullSettingKey(REALM_NAME, OpenIdConnectRealmSettings.RP_CLIENT_ID), "rp-my")
+            .put(getFullSettingKey(REALM_NAME, OpenIdConnectRealmSettings.RP_CLIENT_AUTH_METHOD), "none")


"none", "None", "noNe", "nOnE", "NonE" ... 👍 that we will not have this issue.

This would be testing the OP though, not our implementation, as it is the OP that should not validate a client JWT with none algorithm. We do not disallow none specifically, we use an allowlist that only allows "HS256", "HS384", "HS512"

I like the suggestion though, we can use it in OpenIdConnectAuthenticatorTests.testImplicitFlowFailsWithNoneAlgorithm which tests our implementation

ywangd · 2020-06-30T14:11:54Z

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

+        + "/c2id/clients";
+    private static final String LOGIN_API = "http://127.0.0.1:" + getEphemeralTcpPortFromProperty("oidc-provider", "8080")
+        + "/c2id-login/api/";
+    private static final String ES_PORT = getEphemeralTcpPortFromProperty("elasticsearch-node", "9200");


I don't see test.fixtures.elasticsearch-node.9200 get set anywhere. How does this work?

elasticsearch-node is the name of the elasticsearch service in https://github.com/elastic/elasticsearch/pull/58708/files#diff-2be63811028f90650408e594c29099b9R3 and 9200 is defined there in the ports section. I assume it is the gradle docker compose plugin that handles this magic or something in our buildSrc , Mark can probably share more details

Ah awesome! buildSrc magic. TIL

ywangd · 2020-06-30T14:19:01Z

I assume this PR will close #54582 ? Or do you plan to add more auth methods after this one? Anyway, I am tagging it here for cross-reference.

jkakavas · 2020-06-30T14:30:38Z

I still need go through the tests more carefully (they are new to me). Here are a few comments/questions to start. Thanks!

we can wait until I return, this is not urgent, no worries.

mark-vieira

One comment, otherwise LGTM.

mark-vieira · 2020-06-30T19:00:13Z

x-pack/test/idp-fixture/build.gradle

@@ -1,4 +1,36 @@
+import org.elasticsearch.gradle.VersionProperties
+import org.elasticsearch.gradle.Architecture
+
 apply plugin: 'elasticsearch.build'


I wonder if we can remove elasticsearch.build plugin. This project contains no code or tests, it's simply a test fixture.

ywangd

LGTM. I left a few questions about the c2id-server setup for my learning purpose. Thanks!

ywangd · 2020-07-01T03:56:33Z

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

@@ -40,11 +153,13 @@ services:
      - ./idp/shib-jetty-base/start.d/ssl.ini:/opt/shib-jetty-base/start.d/ssl.ini

  oidc-provider:
-    image: "c2id/c2id-server:7.8"
+    image: "c2id/c2id-server:9.5"


Is this an image that we maintain? If so, where can I find the Dockerfile that builds the image?

Nope, we use the official one that the folks from connect2id ( who implement the OP ) publish in Dockerhub. I dont think they publish the Dockerfile, some details are in https://connect2id.com/products/server/docs/guides/docker

Thanks! I checked the image on Dockerhub and it has zero documentation. I found it hard to believe it was the official image ... apparently I am wrong.

ywangd · 2020-07-01T04:23:43Z

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

+            httpPost3.setEntity(new StringEntity(postClient, ContentType.APPLICATION_JSON));
+            httpPost3.setHeader("Accept", "application/json");
+            httpPost3.setHeader("Content-type", "application/json");
+            httpPost3.setHeader("Authorization", "Bearer 811fa888f3e0fdc9e01d4201bfeee46a");


Not sure where this Bearer token is picked from. I guess it has something to do with the docker image?

We define this in override.properties which we bind mount to the container, see the docker compose file and it is used as a static credential to talk to the OPs API , I'll add a comment here when I get to merge this

ywangd · 2020-07-01T04:28:01Z

x-pack/test/idp-fixture/oidc/override.properties

+op.issuer=http://oidc-provider:8080/c2id
+op.authz.endpoint=http://oidc-provider:8080/c2id-login/


Same here. Would be great to learn how these configurations work.

See https://connect2id.com/products/server/docs/guides/docker for some details. Issuer is the OPs issuer string that it will use to populate the iss claim in the ID Token. The OP also uses this to determine the token endpoint and userinfo endpoint it expects to be listening on by adding the proper suffix.

The authz.endpoint is an API endpoint that we use to programmatically authenticate users with the OP.

We use the docker compose service name oidc-provider so that es which also runs in docker can access it via it (docker networking magic :) )

Sorry for not being clear here. I wasn't asking about oidc-provider, which I understand it's docker's doing. Was more about property names, e.g. op.authz.endpoint and what do they mean to the OP. The guide you link should answer all my questions. Thanks!

…ent-auth-types

jkakavas · 2020-08-19T09:58:42Z

@elasticmachine run elasticsearch-ci/2

jkakavas · 2020-08-19T14:43:54Z

:x-pack:test:idp-fixture:composeUp fails for some reason, @breskeby is kindly assisting me pinpointing the cause

breskeby · 2020-08-19T16:03:51Z

Hey looking at the composeUp issue I noticed two things:

As far as I understand the composeUp task should depend on preProcessFixture but it seems this is not wired up correctly.
triggering preProcessFixture might not fix the problem as it seems the up-to-date checking of this and its depending tasks returns false positives.

I need some more time to narrow down the proper fix.

@jkakavas As a workaround, for now running ./gradlew preProcessFixture --rerun-tasks triggers the preProcessFixture task and its dependencies with ignoring the up-to-date check and should fix the problem for you locally.

jkakavas · 2020-08-31T10:29:16Z

@breskeby any updates on this ? Did you find sometime to look into the composeUp task? Anything I could do to help with this? Thanks!

breskeby · 2020-08-31T12:52:49Z

@jkakavas I pushed a fix on task dependencies setup for test clusters earlier today. Can you rebase against master and see if that fixes your issue?

jkakavas · 2020-09-01T09:33:30Z

@elasticmachine update branch

jkakavas · 2020-09-01T09:35:39Z

@elasticmachine run elasticsearch-ci/packaging-sample-windows

jkakavas · 2020-09-01T10:27:26Z

Still:

http-proxy uses an image, skipping
oidc-provider uses an image, skipping
Creating network "89c62680e863be49b17213e549106bdb_idp-fixture__default" with the default driver
Pulling elasticsearch-node (elasticsearch:test)...
manifest for elasticsearch:test not found
Removing network 89c62680e863be49b17213e549106bdb_idp-fixture__default

@breskeby

breskeby · 2020-09-01T12:32:01Z

@mark-vieira I looked briefly into this but couldn't find a root cause for this. do you have an idea what's going wrong here?

ywangd · 2020-09-07T12:08:27Z

Would be very much appreciated, if ElasticSearch 7.9.x could support client_secret_jwt instead of only plain secret-id.

The PR targets v7.10.0. Also patch releases, e.g. v7.9.2, normally only get bug fixes. Since this PR is new features, it is unlikely to be part of v7.9.2. But I'll double confirm with the team and get back to you.

…ent-auth-types

From breskeby@de079d6

The OpenID Connect specification defines a number of ways for a client (RP) to authenticate itself to the OP when accessing the Token Endpoint. We currently only support `client_secret_basic`. This change introduces support for 2 additional authentication methods, namely `client_secret_post` (where the client credentials are passed in the body of the POST request to the OP) and `client_secret_jwt` where the client constructs a JWT and signs it using the the client secret as a key. Support for the above, and especially `client_secret_jwt` in our integration tests meant that the OP we use ( Connect2id server ) should be able to validate the JWT that we send it from the RP. Since we run the OP in docker and it listens on an ephemeral port we would have no way of knowing the port so that we can configure the ES running via the testcluster to know the "correct" Token Endpoint, and even if we did, this would not be the Token Endpoint URL that the OP would think it listens on. To alleviate this, we run an ES single node cluster in docker, alongside the OP so that we can configured it with the correct hostname and port within the docker network.

ywangd · 2020-09-14T06:30:43Z

Backport #62289
The change should be part of the upcoming v7.10.0 release.

The OpenID Connect specification defines a number of ways for a client (RP) to authenticate itself to the OP when accessing the Token Endpoint. We currently only support `client_secret_basic`. This change introduces support for 2 additional authentication methods, namely `client_secret_post` (where the client credentials are passed in the body of the POST request to the OP) and `client_secret_jwt` where the client constructs a JWT and signs it using the the client secret as a key. Support for the above, and especially `client_secret_jwt` in our integration tests meant that the OP we use ( Connect2id server ) should be able to validate the JWT that we send it from the RP. Since we run the OP in docker and it listens on an ephemeral port we would have no way of knowing the port so that we can configure the ES running via the testcluster to know the "correct" Token Endpoint, and even if we did, this would not be the Token Endpoint URL that the OP would think it listens on. To alleviate this, we run an ES single node cluster in docker, alongside the OP so that we can configured it with the correct hostname and port within the docker network. Co-authored-by: Ioannis Kakavas <[email protected]>

tvernum · 2020-10-07T07:26:19Z

@jkakavas @ywangd Can we check the labels here?
I think we need to add v8.0.0 and remove backport pending

ywangd · 2020-10-07T07:29:03Z

@tvernum You are right. They are corrected now.

Support for additional Client authentication methods was added in the OIDC realm in elastic#58708. This change adds the `rp.client_auth_method` in the realm settings reference doc.

Support for additional Client authentication methods was added in the OIDC realm in #58708. This change adds the `rp.client_auth_method` and `rp.client_auth_signature_algorithm` settings in the realm settings reference doc.

Support for additional Client authentication methods was added in the OIDC realm in elastic#58708. This change adds the `rp.client_auth_method` and `rp.client_auth_signature_algorithm` settings in the realm settings reference doc.

Support for additional Client authentication methods was added in the OIDC realm in #58708. This change adds the `rp.client_auth_method` and `rp.client_auth_signature_algorithm` settings in the realm settings reference doc.

jkakavas added 3 commits June 25, 2020 08:52

wip

3360fa1

Merge remote-tracking branch 'origin/master' into oidc-additional-cli…

45d9a74

…ent-auth-types

jkakavas added >enhancement :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) labels Jun 29, 2020

jkakavas requested review from ywangd and mark-vieira June 29, 2020 22:56

elasticmachine added the Team:Security Meta label for security team label Jun 29, 2020

remove random characters from config

0467b14

ywangd reviewed Jun 30, 2020

View reviewed changes

address feedback

e1e5537

mark-vieira approved these changes Jun 30, 2020

View reviewed changes

ywangd approved these changes Jul 1, 2020

View reviewed changes

jkakavas added 2 commits August 19, 2020 11:54

Merge remote-tracking branch 'origin/master' into oidc-additional-cli…

e66c6e5

…ent-auth-types

address feedback

49cd438

jkakavas added the v7.10.0 label Aug 19, 2020

Merge branch 'master' into oidc-additional-client-auth-types

0636079

ywangd added 2 commits September 9, 2020 18:53

Merge remote-tracking branch 'origin/master' into oidc-additional-cli…

52a4c80

…ent-auth-types

Apply Rene's fix for test fixture

1dcd6f3

From breskeby@de079d6

ywangd mentioned this pull request Sep 10, 2020

More general fix for dependencies of the composeUp task #62203

Closed

ywangd merged commit 5d341dc into elastic:master Sep 14, 2020

ywangd added the backport pending label Sep 14, 2020

ywangd added v8.0.0 and removed backport pending labels Oct 7, 2020

Mpdreamz mentioned this pull request Nov 16, 2020

7.10.1 Meta Ticket elastic/elasticsearch-net#5096

Closed

61 tasks

stevejgordon mentioned this pull request Dec 17, 2020

7.11.0 Meta Ticket elastic/elasticsearch-net#5198

Closed

jkakavas mentioned this pull request Mar 9, 2021

Add doc reference for client_auth_method #70124

Merged

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

jkakavas mentioned this pull request Jan 28, 2022

OpenID Connect realm should support more client authentication methods #54582

Closed

		op.issuer=http://oidc-provider:8080/c2id
		op.authz.endpoint=http://oidc-provider:8080/c2id-login/

Oidc additional client auth types #58708

Oidc additional client auth types #58708

Conversation

jkakavas commented Jun 29, 2020 • edited Loading

elasticmachine commented Jun 29, 2020

jkakavas commented Jun 29, 2020

ywangd 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

jkakavas Jun 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywangd commented Jun 30, 2020

jkakavas commented Jun 30, 2020

mark-vieira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywangd 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

jkakavas commented Aug 19, 2020

jkakavas commented Aug 19, 2020

breskeby commented Aug 19, 2020

jkakavas commented Aug 31, 2020

breskeby commented Aug 31, 2020

jkakavas commented Sep 1, 2020

jkakavas commented Sep 1, 2020

jkakavas commented Sep 1, 2020

breskeby commented Sep 1, 2020

ywangd commented Sep 7, 2020

ywangd commented Sep 14, 2020

tvernum commented Oct 7, 2020

ywangd commented Oct 7, 2020

jkakavas commented Jun 29, 2020 •

edited

Loading

jkakavas Jun 30, 2020 •

edited

Loading