Migrate authentication functionality to a new Elasticsearch client. by azasypkin · Pull Request #87094 · elastic/kibana

azasypkin · 2020-12-31T15:51:46Z

The last PR in a series of PRs to migrate Security plugin to a new Elasticsearch client:

Authentication sub-system is migrated to a new ES client
Session management sub-system is migrated to a new ES client
Completely removed ElasticsearchClientPlugin as we can now use either Client's default method or transport.request (for SAML/OIDC/PKI only)

~~Blocked by: #87091~~

azasypkin · 2021-01-15T10:09:04Z

   */
-  private getNegotiateChallenge(error: LegacyElasticsearchError) {
+  private getNegotiateChallenge(error: errors.ResponseError) {
+    // We extract headers from the original Elasticsearch error and not from the top-level `headers`


note: well, in fact it's NodeJS that merges these headers, but we can pretend we don't know that 🙂

azasypkin · 2021-01-15T10:28:10Z

-        body: { x509_certificate_chain: certificateChain },
-      });
+      result = (
+        await this.options.client.asInternalUser.transport.request({


note: hopefully we can publish spec for this API in the future (being discussed in https://github.com/elastic/elasticsearch/issues/67189)

azasypkin · 2021-01-15T10:55:05Z

+      const { body: response } = await this.options.elasticsearchClient.deleteByQuery(
+        {
+          index: this.indexName,
+          refresh: true,


note: wait_for is actually not supported by the deleteByQuery

elasticmachine · 2021-01-15T12:32:35Z

Pinging @elastic/kibana-security (Team:Security)

legrego · 2021-01-21T14:40:33Z

ACK: reviewing

legrego

This was a ton of work, nice job! Just a few optional nits and questions, so I won't hold up approval. Thanks for taking care of this 💚

We also have some stray setup of the legacy client in plugin.test.ts that we can cleanup:

kibana/x-pack/plugins/security/server/plugin.test.ts

Lines 46 to 47 in 951aa66

    
           mockClusterClient = elasticsearchServiceMock.createLegacyCustomClusterClient(); 
        
           mockCoreSetup.elasticsearch.legacy.createClient.mockReturnValue(mockClusterClient);

legrego · 2021-01-21T14:42:30Z

   * @deprecated Use `authc` methods from the `SecurityServiceStart` contract instead.
   */
-  authc: Pick<AuthenticationServiceSetup, 'getCurrentUser'>;
+  authc: Pick<AuthenticationServiceStart, 'getCurrentUser'>;


It feels strange to export a start service from our setup contract. I can live with this since it's deprecated though

Yeah, you're right, I'll just duplicate definition here.

legrego · 2021-01-21T14:48:44Z

- * you may not use this file except in compliance with the Elastic License.
- */
-
-export function elasticsearchClientPlugin(Client: any, config: unknown, components: any) {


legrego · 2021-01-21T14:50:36Z

+      const { body: response } = await this.options.elasticsearchClient.deleteByQuery(
+        {
+          index: this.indexName,
+          refresh: true,


legrego · 2021-01-21T14:55:38Z

@@ -4,50 +4,52 @@
 * you may not use this file except in compliance with the Elastic License.
 */


I really like how we can mock the explicit API calls that we expect to make, without having to mock the generic callAsInternalUser anymore and hope that we're calling it correctly 🏅

legrego · 2021-01-21T15:11:07Z

-      });
+      } = (
+        await this.options.client.security.getToken<{
+          access_token: string;


question Do you know if there type improvements we can eventually upstream to the ES client, or is this intentionally generic? I know there are varied responses from ES that we can get here, but there appers to be some common fields that we could potentially leverage.

AFAIK we'll get much better type definitions soon, see #83808 for more details.

Once this merges I think we can improve missing pieces on case-by-case basis.

legrego · 2021-01-21T15:17:44Z


+      if (!this.authenticator) {
+        this.logger.error('Authenticator is not initialized yet.');
+        return response.internalError();


Would 503 be more appropriate?

Yep, agree, will change, thanks!

legrego · 2021-01-21T15:19:47Z

    });

+    const getCurrentUser = (request: KibanaRequest) => {
+      if (!this.license.isEnabled()) {


question I know you just moved this as-is from setup, but is there a reason we need to perform a license check here?

If the license doesn't enable security, then we'd presumably not have an AuthenticatedUser on the request state, so we'd still end up returning null.

Good point, let me double check.

legrego · 2021-01-21T15:26:56Z

+    // since `WWW-Authenticate` values can also include commas.
    const challenges = ([] as string[]).concat(
-      (error.output.headers as { [key: string]: string })[WWWAuthenticateHeaderName]
+      error.body?.error?.header?.[WWWAuthenticateHeaderName] || []


note: I didn't test this as part of my review, but I will if you'd like me to.

No worries, I specifically checked this. We have integration tests that test this behavior and they failed when I initially tried to use error.headers.

legrego · 2021-01-21T15:30:35Z

-          realm: this.realm,
-        },
-      });
+      result = (


nit: for the endpoints where we need to use transport.request, can we add a comment linking to https://github.com/elastic/elasticsearch/issues/67189? That will be really helpful for future me to remember why we did it this way 🙂

Yep, will do, thanks!

legrego · 2021-01-21T15:35:10Z

+          path: '/_security/oidc/prepare',
+          body: params,
+        })
+      ).body as any;


I kid, I think this is tolerable for now given our usage

🤣 Yeah, that's exactly how I feel about it 🙈

kibanamachine · 2021-01-21T19:35:45Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 6a68861

Metrics [docs]

✅ unchanged

History

💚 Build elastic/elasticsearch#99871 succeeded 6210f6d
💛 Build elastic/elasticsearch#99514 was flaky f997558
💚 Build elastic/elasticsearch#99074 succeeded c85de9d
💚 Build elastic/elasticsearch#98241 succeeded 3d97e8c01b86d83f0e20dfdf9e93be969acc8b7a
💔 Build elastic/elasticsearch#98217 failed 07dbc21f2ac20a9eb598bde734940f90cb7a7535

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…lastic#87094) # Conflicts: # x-pack/plugins/security/server/authentication/authentication_service.ts # x-pack/plugins/security/server/authentication/providers/base.mock.ts # x-pack/plugins/security/server/authentication/providers/saml.test.ts # x-pack/plugins/security/server/authentication/providers/saml.ts

azasypkin · 2021-01-25T07:45:15Z

7.x/7.12.0: 22d85e7

azasypkin added chore Feature:New Platform Team:Security Platform Security: Auth, Users, Roles, Spaces, Audit Logging, etc t// Feature:Security/Authentication Platform Security - Authentication release_note:skip Skip the PR/issue when compiling release notes v7.12.0 labels Dec 31, 2020

azasypkin force-pushed the issue-xxx-new-es-client branch 3 times, most recently from c587775 to a585f9c Compare January 11, 2021 13:47

azasypkin added the blocked label Jan 11, 2021

azasypkin force-pushed the issue-xxx-new-es-client branch 2 times, most recently from 07dbc21 to 3d97e8c Compare January 12, 2021 14:58

Migrate authentication functionality to a new Elasticsearch client.

c85de9d

azasypkin force-pushed the issue-xxx-new-es-client branch from 3d97e8c to c85de9d Compare January 15, 2021 09:58

azasypkin commented Jan 15, 2021

View reviewed changes

azasypkin removed the blocked label Jan 15, 2021

azasypkin marked this pull request as ready for review January 15, 2021 12:32

azasypkin requested a review from a team as a code owner January 15, 2021 12:32

azasypkin added 2 commits January 19, 2021 10:17

Merge branch 'master' into issue-xxx-new-es-client

f997558

Merge branch 'master' into issue-xxx-new-es-client

6210f6d

legrego approved these changes Jan 21, 2021

View reviewed changes

azasypkin added 2 commits January 21, 2021 17:58

Merge branch 'master' into issue-xxx-new-es-client

85770c1

Review#1: handle review feedback.

6a68861

azasypkin merged commit f668972 into elastic:master Jan 21, 2021

azasypkin deleted the issue-xxx-new-es-client branch January 21, 2021 20:10

azasypkin mentioned this pull request Jan 21, 2021

[7.x] Migrate authentication functionality to a new Elasticsearch client. (#87094) #89027

Merged

azasypkin added the backported label Jan 21, 2021

	mockClusterClient = elasticsearchServiceMock.createLegacyCustomClusterClient();
	mockCoreSetup.elasticsearch.legacy.createClient.mockReturnValue(mockClusterClient);

		@@ -4,50 +4,52 @@
		* you may not use this file except in compliance with the Elastic License.
		*/

Conversation

azasypkin commented Dec 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azasypkin Jan 15, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jan 15, 2021

Uh oh!

legrego commented Jan 21, 2021

Uh oh!

legrego left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jan 21, 2021

💚 Build Succeeded

Metrics [docs]

History

Uh oh!

azasypkin commented Jan 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

azasypkin commented Dec 31, 2020 •

edited

Loading

azasypkin Jan 15, 2021 •

edited

Loading