Migrate the rest of the API endpoints to the New Platform plugin #50695

azasypkin · 2019-11-14T18:01:30Z

In this PR, I'm moving the rest of the API endpoints to the New Platform:

api keys (done)
indices routes (done)
users routes (done)
authentication routes (done)

NOTE: It'd be much easier to review PR commit by commit.

Fixes: #43258

"Release note: /api/security/v1/me endpoint is deprecated and will be completely replaced with internal-use-only endpoint in the next major version."

"Release note: /api/security/v1/logout endpoint is deprecated in favor of /api/security/logout and will be removed completely in the next major version. Please make sure to switch any Single sign-on providers you use with Kibana to /api/security/logout instead."

"Release note: /api/security/v1/oidc/implicit endpoint is deprecated in favor of /api/security/implicit and will be removed completely in the next major version. Please make sure to switch Elasticsearch and any OpenID Connect providers you use with Kibana to /api/security/implicit instead."

"Release note: support of Third-Party Initiated login via /api/security/v1/oidc endpoint is deprecated and will be removed completely in the next major version. Please make sure to switch OpenID Connect providers you use with Kibana for Third-Party Initiated login to /api/security/initiate_login instead."

elasticmachine · 2019-11-14T18:01:32Z

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

elasticmachine · 2019-11-26T15:31:24Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: a70c46de6ec564c7e79eeda72cb4b14ad6ffcc38

azasypkin · 2019-11-27T17:48:36Z

x-pack/legacy/plugins/security/public/lib/api_keys_api.ts

@@ -5,8 +5,7 @@
 */

 import { kfetch } from 'ui/kfetch';
-import { ApiKey, ApiKeyToInvalidate } from '../../common/model/api_key';
-import { INTERNAL_API_BASE_PATH } from '../../common/constants';


note: tbh I don't see much value in INTERNAL_API_BASE_PATH constant and it makes it harder to copy-paste URL to find where it's defined. But let me know if you prefer to have it.

I'm in favor of removing this as well. I enjoy being able to search on full API paths in code

azasypkin · 2019-11-27T17:55:04Z

x-pack/legacy/plugins/siem/cypress/integration/lib/login/helpers.ts

@@ -39,7 +39,7 @@ const ELASTICSEARCH_PASSWORD = 'ELASTICSEARCH_PASSWORD';
 /**
 * The Kibana server endpoint used for authentication
 */
-const LOGIN_API_ENDPOINT = '/api/security/v1/login';
+const LOGIN_API_ENDPOINT = '/internal/security/login';


question: @andrew-goldstein, this code looks a bit suspicious to me, I'd love to hear what it's doing and why? Or is it just test only code?

P.S. I don't see that anyone owns this plugin in .github/CODEOWNERS.

elasticmachine · 2019-11-27T18:30:47Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 88e35f77c7f24e460b46bfb3816eddbf3fc34512

azasypkin · 2019-11-28T13:25:17Z

x-pack/plugins/security/server/elasticsearch_client_plugin.ts

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

-(function (root, factory) {


note: just removed this part and run the rest of the file through prettier.

elasticmachine · 2019-11-28T18:49:20Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 43280e422e04bd5ae2d0fed89b9242d931fe75d1

azasypkin · 2019-11-29T12:15:30Z

x-pack/plugins/security/server/routes/authentication/oidc.ts

+  getLegacyAPI,
+  basePath,
+}: RouteDefinitionParams) {
+  // Generate two identical routes with new and deprecated URL and issue a warning if route with deprecated URL is ever used.


note: lots of BWC tricks in this file, but while I'm here I wanted to clean it up.....

legrego · 2019-12-09T14:16:41Z

x-pack/plugins/security/server/routes/authentication/common.ts

+      createLicensedRouteHandler(async (context, request, response) => {
+        const serverBasePath = basePath.serverBasePath;
+        if (path === '/api/security/v1/logout') {
+          logger.warn(


What do you think about integrating this with the upgrade assistant in a followup PR? I think it'd be helpful for folks when upgrading to the next major version if we make it known that something in their environment is relying on this deprecated endpoint.

To say this another way, I'd personally be annoyed if I ran the upgrade assistant, and it didn't warn me that this would stop working if I was relying on it 😄

Comment applies to the other deprecated endpoints as well

That's a great idea! I'll try to figure how hard/easy it would be. I tried it once in the distant past and UA wasn't ready for things like this back then, but I believe it's much more advanced now.

legrego

Looks great overall. Just a couple minor questions/nits on my end so far

legrego · 2019-12-09T14:25:12Z

x-pack/plugins/security/server/routes/authentication/common.ts

+        validate: { query: schema.object({}, { allowUnknowns: true }) },
+        options: { authRequired: false },
+      },
+      createLicensedRouteHandler(async (context, request, response) => {


The LP implementation for login/logout endpoints did not perform license checks. I think it makes sense to add it for our login routes, but what do you think about removing the check for the logout endpoints? It's an edge-case for sure, but I worry about blocking logout if there is a hiccup with Kibana's view of the current license.

Yeah, that's a good point, thanks! I'll remove license check from here.

legrego · 2019-12-09T14:30:20Z

x-pack/plugins/security/server/routes/authentication/common.ts

+    { path: '/internal/security/me', validate: false },
+    createLicensedRouteHandler(async (context, request, response) => {
+      try {
+        return response.ok({ body: (await authc.getCurrentUser(request)) as any });


note: authc.getCurrentUser performs an additional round-trip to ES to retrieve the user details. The previous implementation did not have to do this, likely because of the intimate relationship it had with Hapi.

I don't think anything has to change here, but as we move the client-side security plugin to. the NP, we should evaluate all the places we are calling this endpoint. I recall seeing several calls as the page loads which we can likely trim down.

authc.getCurrentUser performs an additional round-trip to ES to retrieve the user details. The previous implementation did not have to do this, likely because of the intimate relationship it had with Hapi.

Yep, that's correct.

I don't think anything has to change here, but as we move the client-side security plugin to. the NP, we should evaluate all the places we are calling this endpoint. I recall seeing several calls as the page loads which we can likely trim down.

If it becomes a perf problem for us, we'll ask platform to expose existing auth information to plugins: security plugin returns user info to core, core stores it in the internal cache, but just doesn't expose it for now.

legrego · 2019-12-09T14:37:54Z

x-pack/plugins/security/server/routes/authentication/oidc.ts

+        validate: {
+          query: schema.object(
+            {
+              authenticationResponseURI: schema.maybe(schema.string({ minLength: 1 })),


question: is there a reason authenticationResponseURI can't be validated via schema.uri() like error_uri is below?

Hmm, I don't think there is a reason, just an oversight 🙈 Will fix.

legrego · 2019-12-09T14:40:01Z

x-pack/plugins/security/server/routes/authentication/oidc.ts

+              target_link_uri: schema.maybe(schema.uri()),
+              state: schema.maybe(schema.string()),
+            },
+            { allowUnknowns: true }


nit: unrelated to the migration, but can we add a comment explaining why we want to allow unknown object properties here? We're trying to reduce the number of places this is required in code, so documenting legitimate uses will be helpful going forward.

Sure, added comments in all 3 places in this file.

legrego · 2019-12-09T14:44:59Z

x-pack/plugins/security/server/routes/authentication/oidc.ts

+    loginAttempt: ProviderLoginAttempt
+  ) {
+    try {
+      // We handle the fact that the user might get redirected to Kibana while already having an session


super-nit

Suggested change

// We handle the fact that the user might get redirected to Kibana while already having an session

// We handle the fact that the user might get redirected to Kibana while already having a session

legrego · 2019-12-09T14:46:40Z

x-pack/plugins/security/server/routes/authentication/oidc.ts

+      if (authenticationResult.succeeded()) {
+        return response.forbidden({
+          body:
+            'Sorry, you already have an active Kibana session. ' +


This can be done in a followup since it also existed in LP, but we should internationalize this message.

I was hesitating to translate error messages as users usually copy-paste-google for similar cases. But this one is clear enough and actionable by itself, so agree we can translate it. Will fix in this PR.

x-pack/plugins/security/server/routes/authentication/oidc.ts

legrego · 2019-12-09T15:07:00Z

x-pack/plugins/security/server/routes/authentication/common.ts

+  }
+
+  router.get(
+    { path: '/internal/security/me', validate: false },


How do you feel about also keeping /api/security/v1/me around for the remainder of 7.x? Although it's not part of our public API, we have instructed both users and customers about this endpoint for quite some time, and I fear we'll break a lot of "custom" configurations by removing this endpoint.

Hmm, interesting, didn't know that. I can definitely keep it in 7.x. Also what was the use case for customers/users? Would it justify having this as a public API?

Also what was the use case for customers/users? Would it justify having this as a public API?

I believe there are some custom/third-party plugins which use this endpoint. At the very least, we've put it out there in the public:

https://discuss.elastic.co/t/retrieving-the-current-session-info-in-kibana-plugin/85041

https://discuss.elastic.co/t/kibana-user-session-tracking/186226

legrego · 2019-12-09T15:10:05Z

x-pack/plugins/security/server/routes/users/create_or_update.ts

+export function defineCreateOrUpdateUserRoutes({ router, clusterClient }: RouteDefinitionParams) {
+  router.post(
+    {
+      path: '/internal/security/users/{username}',


It looks like we have a couple references to the old endpoint here

Oh, good catch, it's a brand new script! Will fix.

…ep `/api/security/v1/me` in 7.x, validate OIDC `authenticationResponseURI` as a proper URI, explain the need for `allowUnknowns` in OIDC routes validation, localize error message, update APM dev script with new `users` internal routes.

azasypkin · 2019-12-10T11:42:40Z

x-pack/plugins/security/server/routes/authentication/common.ts

+      { path, validate: false },
+      createLicensedRouteHandler(async (context, request, response) => {
+        if (path === '/api/security/v1/me') {
+          logger.warn(


note: Assuming we make this route internal I don't mention the name of the internal route as we don't provide any guarantees regarding it.

azasypkin · 2019-12-10T13:02:29Z

@legrego thanks for the review! PR is ready for another pass.

sorenlouv

APM changes LGTM

legrego

LGTM, thanks for the changes! I only tested basic and token auth providers locally -- let me know if you'd like me to test any additional auth providers

azasypkin · 2019-12-11T17:24:12Z

I only tested basic and token auth providers locally -- let me know if you'd like me to test any additional auth providers

Thanks for review and testing! I believe we're good, I tested OIDC and SAML locally as well and we have a good number of integration tests I believe. If we figure out that I broke something, then I'll add even more integration tests. Testing everything manually is hard and tedious and if we can't rely enough on the test coverage we built it's not going to scale anyway 🙈

azasypkin · 2019-12-11T19:54:46Z

7.x/7.6.0: bb8e383

elasticmachine · 2019-12-12T12:39:01Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 9763717

History

💚 Build #14481 succeeded 9763717
💚 Build #14113 succeeded 94018c1

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

chrisronline · 2019-12-17T21:19:45Z

It looks like this moved the shield plugin. Is it possible to still use this within the old platform?

azasypkin · 2019-12-18T07:59:40Z

It looks like this moved the shield plugin. Is it possible to still use this within the old platform?

@chrisronline Technically you can still import x-pack/plugins/security/server/elasticsearch_client_plugin.ts directly, but I'd like to understand what you need this for and whether Security plugin can expose necessary functionality through its contracts?

P.S. The latest version of elasticsearch.js has a different extension/plugin model anyway, so you may need to migrate once again when Kibana switches to it.

chrisronline · 2019-12-18T14:37:36Z

Good question.

I spoke with @kobelb about some other code and he mentioned we shouldn't use transport.request, and that the alternative is using this shield plugin. But looking at the elasticsearch.js apis, we can just use https://www.elastic.co/guide/en/elasticsearch/client/javascript-api/current/api-reference.html#_security_hasprivileges, right @kobelb?

kobelb · 2019-12-18T17:21:28Z

Are we using the non-legacy Elasticsearch client already?

azasypkin · 2019-12-18T17:28:38Z

Are we using the non-legacy Elasticsearch client already?

You mean @elastic/elasticsearch? If so, then not yet, I see we only have @elastic/elasticsearch as a dev dependency and #27051 stalled as well.

azasypkin added Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! Feature:NP Migration labels Nov 14, 2019

azasypkin force-pushed the issue-xxx-migrate-np-api branch from 23c9ff1 to a70c46d Compare November 26, 2019 14:16

elastic deleted a comment from elasticmachine Nov 26, 2019

azasypkin force-pushed the issue-xxx-migrate-np-api branch from a70c46d to 88e35f7 Compare November 27, 2019 17:14

azasypkin added release_note:deprecation v7.6.0 labels Nov 27, 2019

azasypkin commented Nov 27, 2019

View reviewed changes

azasypkin marked this pull request as ready for review November 27, 2019 18:35

azasypkin requested a review from a team as a code owner November 27, 2019 18:35

azasypkin commented Nov 28, 2019

View reviewed changes

azasypkin force-pushed the issue-xxx-migrate-np-api branch from 88e35f7 to 43280e4 Compare November 28, 2019 17:29

azasypkin commented Nov 29, 2019

View reviewed changes

azasypkin added 2 commits December 9, 2019 08:55

Move files.

4189b08

Change files.

94018c1

azasypkin force-pushed the issue-xxx-migrate-np-api branch from 43280e4 to 94018c1 Compare December 9, 2019 08:09

azasypkin requested a review from legrego December 9, 2019 13:16

legrego reviewed Dec 9, 2019

View reviewed changes

azasypkin added 2 commits December 10, 2019 11:00

Merge branch 'master' into issue-xxx-migrate-np-api

43bb372

azasypkin requested a review from a team as a code owner December 10, 2019 11:40

azasypkin commented Dec 10, 2019

View reviewed changes

azasypkin requested a review from legrego December 10, 2019 13:02

sorenlouv approved these changes Dec 10, 2019

View reviewed changes

legrego approved these changes Dec 10, 2019

View reviewed changes

azasypkin merged commit 2ec82d3 into elastic:master Dec 11, 2019

azasypkin deleted the issue-xxx-migrate-np-api branch December 11, 2019 17:35

azasypkin mentioned this pull request Dec 11, 2019

[7.x] Migrate the rest of the API endpoints to the New Platform plugin (#50695) #52789

Merged

azasypkin added the backport pending label Dec 11, 2019

azasypkin added backported and removed backport pending labels Dec 11, 2019

azasypkin mentioned this pull request Jan 2, 2020

Rename /api/security/oidc to /api/security/oidc/callback. #53886

Merged

jportner mentioned this pull request Mar 7, 2022

Append Kibana Security APIs elastic/support-diagnostics#580

Merged

	// We handle the fact that the user might get redirected to Kibana while already having an session
	// We handle the fact that the user might get redirected to Kibana while already having a session

Migrate the rest of the API endpoints to the New Platform plugin #50695

Migrate the rest of the API endpoints to the New Platform plugin #50695

Conversation

azasypkin commented Nov 14, 2019 • edited Loading

elasticmachine commented Nov 14, 2019

elasticmachine commented Nov 26, 2019

💚 Build Succeeded

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin Nov 27, 2019 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Nov 27, 2019

💚 Build Succeeded

Choose a reason for hiding this comment

elasticmachine commented Nov 28, 2019

💚 Build Succeeded

azasypkin Nov 29, 2019 • edited Loading

Choose a reason for hiding this comment

legrego Dec 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legrego 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin Dec 10, 2019 • 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

Choose a reason for hiding this comment

azasypkin commented Dec 10, 2019

sorenlouv left a comment

Choose a reason for hiding this comment

legrego left a comment

Choose a reason for hiding this comment

azasypkin commented Dec 11, 2019

azasypkin commented Dec 11, 2019

elasticmachine commented Dec 12, 2019

💚 Build Succeeded

History

chrisronline commented Dec 17, 2019

azasypkin commented Dec 18, 2019

chrisronline commented Dec 18, 2019

kobelb commented Dec 18, 2019

azasypkin commented Dec 18, 2019

azasypkin commented Nov 14, 2019 •

edited

Loading

azasypkin Nov 27, 2019 •

edited

Loading

azasypkin Nov 29, 2019 •

edited

Loading

legrego Dec 9, 2019 •

edited

Loading

azasypkin Dec 10, 2019 •

edited

Loading