Adding authentication information to access token create APIs #62490

BigPandaToo · 2020-09-16T17:11:01Z

Adding authentication object to following APIs:
/_security/oauth2/token
/_security/delegate_pki
/_security/saml/authenticate
/_security/oidc/authenticate

Resolves: #59685

elasticmachine · 2020-09-17T08:08:42Z

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

ywangd

The changes are in the right direction. A few general things can be improved upon:

Tests
- For existing tests that use the updated response classes, we should use actual authentication object to fill the new field instead of null. I commented it for DelegatePkiAuthenticationResponseTests, but there are a few more places like it.
- The YAML test failures are genuine. Those tests try to match the response object literally. The fix should be along the line of adding the authentication field into the response samples.
- Tests for TransportXxxAction classes. This could get a bit tricky because there are not many unit tests for these type of classes. So you'll need add to the following integration tests. The following is a rough list for where some assertions could be added:
  - OpenIdConnectAuthIT#completeAuthentication
  - SamlAuthenticationIT#submitSamlResponse
  - TokenAuthIntegTests
  - PkiAuthDelegationIntegTests
Client code - Many of the Request and Response classes have their correspondences in the High Level Rest Client (HLRC) package, e.g. For DelegatePkiAuthenticationResponse, there is one in the org.elasticsearch.client.security package, which is different from the one in org.elasticsearch.xpack.core.security.action. They also need to be updated with the new fields. Naturally, the relevant tests also need to be updated after the main code changes.
New field and BackWards Compatibility (BWC) - When fields are added or removed from existing entitiy classes, extra handling are mostly (if not always) needed to ensure things work correctly in a mixed-version cluster environment. The most common changes are with the Reader and Writer interfaces, where the field is handled differently based on the in/out stream version. An example of this can be seen in SamlAuthenticateResponse, which is one of the class you need update as well.

.../src/test/java/org/elasticsearch/client/security/DelegatePkiAuthenticationResponseTests.java

...ava/org/elasticsearch/xpack/core/security/action/oidc/OpenIdConnectAuthenticateResponse.java

...rc/test/java/org/elasticsearch/xpack/core/action/DelegatePkiAuthenticationResponseTests.java

...src/main/java/org/elasticsearch/xpack/security/action/token/TransportRefreshTokenAction.java

BigPandaToo · 2020-10-01T06:58:00Z

@elasticmachine update branch

BigPandaToo · 2020-10-02T10:08:55Z

@elasticmachine update branch

BigPandaToo · 2020-10-02T17:51:10Z

Fixed tests, added changes to RHL APIs + test, backward compatibility, updated TransportRefreshTokenAction

ywangd

Thanks @BigPandaToo The latest changes are looking great. This PR is turning out to be of decent size because of heavy involvement of the request/response objects. There is only one thing from my previous comment still needs to be addressed: "Tests for TransportXxxAction classes". What needs to be done is essentially adding assertions into the existing test methods that perform token related authentication to assert that the response does contain the new authentication field.

I also left a few other comments.

...est-high-level/src/test/java/org/elasticsearch/client/security/CreateTokenResponseTests.java

.../src/test/java/org/elasticsearch/client/security/DelegatePkiAuthenticationResponseTests.java

docs/java-rest/high-level/security/create-token.asciidoc

x-pack/docs/en/rest-api/security/delegate-pki-authentication.asciidoc

...ain/java/org/elasticsearch/xpack/core/security/action/DelegatePkiAuthenticationResponse.java

...ava/org/elasticsearch/xpack/core/security/action/oidc/OpenIdConnectAuthenticateResponse.java

...rc/main/java/org/elasticsearch/xpack/core/security/action/saml/SamlAuthenticateResponse.java

...re/src/main/java/org/elasticsearch/xpack/core/security/action/token/CreateTokenResponse.java

...rc/test/java/org/elasticsearch/xpack/core/action/DelegatePkiAuthenticationResponseTests.java

...c/test/java/org/elasticsearch/xpack/security/rest/action/oauth2/RestGetTokenActionTests.java

BigPandaToo · 2020-10-08T19:58:26Z

@elasticmachine update branch

BigPandaToo · 2020-10-08T20:48:44Z

@elasticmachine update branch

BigPandaToo · 2020-10-11T18:45:18Z

@elasticmachine update branch

BigPandaToo · 2020-10-12T12:52:47Z

@elasticmachine update branch

BigPandaToo · 2020-10-12T13:24:43Z

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

BigPandaToo · 2020-10-12T14:17:37Z

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

ywangd

Thanks for the iteration. It's looking good. One of my initial comments still needs to be addressed. I am copying/pasting it here for your convenience:

Tests for TransportXxxAction classes. This could get a bit tricky because there are not many unit tests for these type of classes. So you'll need add them to the following integration tests. The following is a rough list for where some assertions could be added:
- OpenIdConnectAuthIT#completeAuthentication
- SamlAuthenticationIT#submitSamlResponse
- TokenAuthIntegTests
- PkiAuthDelegationIntegTests

Other than the above, I have a few other minor comments.

docs/java-rest/high-level/security/create-token.asciidoc

docs/java-rest/high-level/security/delegate-pki-authentication.asciidoc

...re/src/main/java/org/elasticsearch/xpack/core/security/action/token/CreateTokenResponse.java

Adding authentication object to following APIs: /_security/oauth2/token /_security/delegate_pki /_security/saml/authenticate /_security/oidc/authenticate Resolves: elastic#59685 (cherry picked from commit 51dbd9e)

…AD metadata) Addressing PR comments

…AD metadata) Update version check

BigPandaToo · 2020-10-14T17:49:24Z

@elasticmachine run elasticsearch-ci/2

BigPandaToo · 2020-10-15T17:54:06Z

@elasticmachine run elasticsearch-ci/1

ywangd

LGTM pending on resolving following comments. Please note some of the comments are applicable to multiple places. I didn't comment on all the places to avoid duplication.

Thanks for the iterations!

client/rest-high-level/src/main/java/org/elasticsearch/client/security/CreateTokenResponse.java

x-pack/docs/en/rest-api/security/get-tokens.asciidoc

ywangd · 2020-10-16T00:15:53Z

...src/internalClusterTest/java/org/elasticsearch/xpack/security/authc/TokenAuthIntegTests.java

@@ -104,6 +104,7 @@ public void testTokenServiceBootstrapOnNodeJoin() throws Exception {
        PlainActionFuture<UserToken> userTokenFuture = new PlainActionFuture<>();
        tokenService.decodeToken(response.getAccessToken(), userTokenFuture);
        assertNotNull(userTokenFuture.actionGet());
+        assertNotNull(response.getAuthenticationResponse());


Nit: I think this assertion is better to be close to where the response object is defined, i.e. straight after CreateTokenResponse response = restClient.... It reads better this way since the test is close to the target. Also, in case it fails, it fails quickly before having to go through the token decrpytion which involves cluster traffic.

ywangd · 2020-10-16T00:19:21Z

...src/internalClusterTest/java/org/elasticsearch/xpack/security/authc/TokenAuthIntegTests.java

@@ -133,6 +134,7 @@ public void testTokenServiceCanRotateKeys() throws Exception {
            assertNotNull(userTokenFuture.actionGet());
            assertNotEquals(activeKeyHash, tokenService.getActiveKeyHash());
        }
+        assertNotNull(response.getAuthenticationResponse());


We should assert the authentication object's principal name as well. This is because the token creation call involves two credentials, one is the invoking user and the other is the user whom the token is created for. We can assert that the authentication's principal is the second user, not the first one.

ywangd · 2020-10-16T00:35:55Z

...a/org/elasticsearch/xpack/security/rest/action/oidc/RestOpenIdConnectAuthenticateAction.java

+                        builder.field("access_token", response.getAccessTokenString());
+                        builder.field("refresh_token", response.getRefreshTokenString());
+                        builder.field("expires_in", response.getExpiresIn().seconds());
+                        builder.field("authentication", response.getAuthentication());


I wonder whether we should have null check here as well similar to how it is done in the toXContent, writeTo methods. It may not be absolutely necessary since this is a rest action and odic cannot be perform with transport client only. But then I am not sure whether there could be some weird combination that could lead to a null value here. Overall I'd say it is easier to just add a null check here so we are sure it is covered in any cases.

ywangd · 2020-10-16T00:36:05Z

.../main/java/org/elasticsearch/xpack/security/rest/action/saml/RestSamlAuthenticateAction.java

+                        builder.field("access_token", response.getTokenString());
+                        builder.field("refresh_token", response.getRefreshToken());
+                        builder.field("expires_in", response.getExpiresIn().seconds());
+                        builder.field("authentication", response.getAuthentication());


Same here as above.

Addressed in #63792

jkakavas · 2020-10-16T07:58:42Z

Heya @BigPandaToo , I think you missed some of the comments from @ywangd in the last review round! We can either revert and redo this or handle in a followup, before backporting the changes to 7.x ?

BigPandaToo · 2020-10-16T08:49:39Z

Yep, sorry about that, already working on follow up

Resolves: elastic#63792

Fixing username

Fixing formatting

* Nit fixes and formatting following #62490 comments Resolves: #63792 * Nit fixes and formatting following #62490 comments Resolves: #63792 * Nit fixes and formatting following #62490 comments Fixing username * Nit fixes and formatting following #62490 comments Fixing formatting

…c#62490) * Adding authentication information to access token create APIs Adding authentication object to following APIs: /_security/oauth2/token /_security/delegate_pki /_security/saml/authenticate /_security/oidc/authenticate Resolves: elastic#59685 (cherry picked from commit 51dbd9e) * Addressing PR commends, fixing tests * Returning tokenGroups attribute as SID string instead of byte array (AD metadata) Addressing PR comments * Returning tokenGroups attribute as SID string instead of byte array (AD metadata) Update version check * Returning tokenGroups attribute as SID string instead of byte array (AD metadata) Update version check * Addressing more PR comments * Adding more to integration tests + some small fixes

) * Nit fixes and formatting following elastic#62490 comments Resolves: elastic#63792 * Nit fixes and formatting following elastic#62490 comments Resolves: elastic#63792 * Nit fixes and formatting following elastic#62490 comments Fixing username * Nit fixes and formatting following elastic#62490 comments Fixing formatting

#63841) * Adding authentication information to access token create APIs (#62490) * Adding authentication information to access token create APIs Adding authentication object to following APIs: /_security/oauth2/token /_security/delegate_pki /_security/saml/authenticate /_security/oidc/authenticate Resolves: #59685 (cherry picked from commit 51dbd9e) * Addressing PR commends, fixing tests * Returning tokenGroups attribute as SID string instead of byte array (AD metadata) Addressing PR comments * Returning tokenGroups attribute as SID string instead of byte array (AD metadata) Update version check * Returning tokenGroups attribute as SID string instead of byte array (AD metadata) Update version check * Addressing more PR comments * Adding more to integration tests + some small fixes * Nit fixes and formatting following #62490 comments (#63797) * Nit fixes and formatting following #62490 comments Resolves: #63792 * Nit fixes and formatting following #62490 comments Resolves: #63792 * Nit fixes and formatting following #62490 comments Fixing username * Nit fixes and formatting following #62490 comments Fixing formatting * Fixing merge conflicts * Fixing merge conflicts

BigPandaToo requested a review from ywangd September 16, 2020 17:11

BigPandaToo added :Security/Client Security in clients (Transport, Rest) >enhancement Team:Security Meta label for security team v7.10.0 v8.0.0 labels Sep 17, 2020

ywangd requested changes Sep 18, 2020

View reviewed changes

BigPandaToo requested a review from ywangd October 2, 2020 17:51

ywangd reviewed Oct 5, 2020

View reviewed changes

andreidan added v7.11.0 and removed v7.10.0 labels Oct 7, 2020

BigPandaToo requested a review from ywangd October 12, 2020 08:01

ywangd reviewed Oct 13, 2020

View reviewed changes

BigPandaToo added 6 commits October 14, 2020 18:18

Adding authentication information to access token create APIs

a917570

Adding authentication object to following APIs: /_security/oauth2/token /_security/delegate_pki /_security/saml/authenticate /_security/oidc/authenticate Resolves: elastic#59685 (cherry picked from commit 51dbd9e)

Addressing PR commends, fixing tests

a1f7c1f

Returning tokenGroups attribute as SID string instead of byte array (…

0966f92

…AD metadata) Addressing PR comments

Returning tokenGroups attribute as SID string instead of byte array (…

bbcb68b

…AD metadata) Update version check

Returning tokenGroups attribute as SID string instead of byte array (…

eba3272

…AD metadata) Update version check

Addressing more PR comments

58a8c7b

BigPandaToo force-pushed the APIresponse branch from 20b3799 to 58a8c7b Compare October 14, 2020 17:28

BigPandaToo requested a review from ywangd October 14, 2020 18:39

Adding more to integration tests + some small fixes

49fbded

ywangd approved these changes Oct 16, 2020

View reviewed changes

BigPandaToo merged commit 2351bb3 into elastic:master Oct 16, 2020

BigPandaToo mentioned this pull request Oct 16, 2020

Followup for #62490 to address all the comments #63792

Closed

BigPandaToo added a commit to BigPandaToo/elasticsearch that referenced this pull request Oct 16, 2020

Nit fixes and formatting following elastic#62490 comments

e17c289

Resolves: elastic#63792

BigPandaToo added a commit to BigPandaToo/elasticsearch that referenced this pull request Oct 16, 2020

Nit fixes and formatting following elastic#62490 comments

bc27589

Resolves: elastic#63792

BigPandaToo added a commit to BigPandaToo/elasticsearch that referenced this pull request Oct 16, 2020

Nit fixes and formatting following elastic#62490 comments

7dd0839

Fixing username

BigPandaToo added a commit to BigPandaToo/elasticsearch that referenced this pull request Oct 16, 2020

Nit fixes and formatting following elastic#62490 comments

a4dd6e2

Fixing formatting

BigPandaToo mentioned this pull request Oct 16, 2020

Adding authentication information to access token create APIs (#62490) #63841

Merged

azasypkin mentioned this pull request Oct 19, 2020

Remove redundant call to _authenticate API after access token is created elastic/kibana#80952

Closed

jkakavas mentioned this pull request Oct 20, 2020

TokenAuthIntegTests#testRefreshingMultipleTimesWithinWindowSucceeds failures #55816

Closed

azasypkin mentioned this pull request Nov 9, 2020

Remove redundant call to _authenticate API after access token is created. elastic/kibana#82980

Merged

stevejgordon mentioned this pull request Dec 17, 2020

7.11.0 Meta Ticket elastic/elasticsearch-net#5198

Closed

stevejgordon mentioned this pull request Mar 1, 2021

Update get token response to include auth info elastic/elasticsearch-net#5378

Merged

BigPandaToo deleted the APIresponse branch April 19, 2021 13:49

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

ywangd mentioned this pull request Jan 31, 2023

[Doc] Authentication information is missing certain API responses #93366

Open

Adding authentication information to access token create APIs #62490

Adding authentication information to access token create APIs #62490

Uh oh!

Conversation

BigPandaToo commented Sep 16, 2020

Uh oh!

elasticmachine commented Sep 17, 2020

Uh oh!

ywangd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BigPandaToo commented Oct 1, 2020

Uh oh!

BigPandaToo commented Oct 2, 2020

Uh oh!

BigPandaToo commented Oct 2, 2020

Uh oh!

ywangd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BigPandaToo commented Oct 8, 2020

Uh oh!

BigPandaToo commented Oct 8, 2020

Uh oh!

BigPandaToo commented Oct 11, 2020

Uh oh!

BigPandaToo commented Oct 12, 2020

Uh oh!

BigPandaToo commented Oct 12, 2020

Uh oh!

BigPandaToo commented Oct 12, 2020

Uh oh!

ywangd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BigPandaToo commented Oct 14, 2020

Uh oh!

BigPandaToo commented Oct 15, 2020

Uh oh!

ywangd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ywangd Oct 16, 2020

Choose a reason for hiding this comment

Uh oh!

ywangd Oct 16, 2020

Choose a reason for hiding this comment

Uh oh!

ywangd Oct 16, 2020

Choose a reason for hiding this comment

Uh oh!

ywangd Oct 16, 2020

Choose a reason for hiding this comment

Uh oh!

BigPandaToo Oct 16, 2020

Choose a reason for hiding this comment

Uh oh!

jkakavas commented Oct 16, 2020

Uh oh!