[Security Solution][Endpoint] New route for create an exception list and return the existing one if it already exists #139618

dasansol92 · 2022-08-29T09:07:47Z

Summary

Adds new internal route for creating an exception list that skips the list creation if it already exists so we avoid 409 errors.
Common code has been moved into a handler that is used in both routes.
Exception list api client (management) has been updated to use the new route and avoid 409 errors in frontend when checking if the list has been created.

Solves #137577

For maintainers

This was checked for breaking API changes and was labeled appropriately

… alredy exists

dasansol92 · 2022-08-29T10:15:33Z

@elasticmachine merge upstream

…ption_lists-137577

… there is a conflict

paul-tavares

awesome job at extracting out the portion of code that actually creates the list.

My main comment is around avoid having to define the URL param for the internal route (no need for it in my mind) and maybe renaming the route to provide a clearer picture of what it does.

paul-tavares · 2022-08-29T21:46:55Z

...securitysolution-io-ts-list-types/src/request/internal/create_exception_list_schema/index.ts

+
+export const internalCreateExceptionListQuerySchema = t.exact(
+  t.partial({
+    ignore_existing: DefaultStringBooleanFalse,


Since the new route is internal, why now just make the POST API call behave the way we want it by default and avoid this param all together?

I would say: remove this URL param and just have the internal handler NOT return a 409. YOu might need to add another param to the createExceptionListHandler() function you created that contains the extracted/reusable code.

Also - if you decide to keep this, can you add documentation to this prop that states what it does when defined in the API params

Yeah, that make sense to me. My intention here was having something more generic but it does not make sense right now and it also adds an extra level of complexity we can get rid of.
Will remove the param and adjust the route naming to be more specific. Thanks!

paul-tavares · 2022-08-29T21:48:24Z

packages/kbn-securitysolution-list-constants/src/index.ts

+/**
+ * Internal exception list routes
+ */
+export const INTERNAL_EXCEPTION_LIST_URL = `/internal${EXCEPTION_LIST_URL}`;


Maybe name the route more specific so that it's clear to us that it differs from the normal "create"? Maybe:

/internal${EXCEPTION_LIST_URL}/_ensure_created;

??

Just noticed further below that the actual route has /_create on it. Can you instead define that here so that we don't have to hard coded everywhere we need it? (and still consider the name change if it makes sense)

Like:

/** * Ensure that a given list is created. If already created, then the existing list definition is returned (no 409 error). * If not yet created, then it will be created. */ export const INTERNAL_EXCEPTIONS_LIST_ENSURE_CREATED_URL = `${INTERNAL_EXCEPTION_LIST_URL}/_ensure_created`;

Would we consider not adding _ensure... if we foresee using this pattern for other routes - like an internal _update route that's for internally managed exception lists.

paul-tavares · 2022-08-29T21:57:22Z

x-pack/plugins/lists/server/routes/internal/create_exceptions_list_route.ts

+      const siemResponse = buildSiemResponse(response);
+      try {
+        return await createExceptionListHandler(context, request, response, siemResponse);
+      } catch (err) {


Do you need the try {} catch ()?
createExceptionListHandler() seems to already do all fo this.

Yeah, it will catch all errors inside the handler and will return a proper http error if something throws.

paul-tavares · 2022-08-29T21:59:56Z

x-pack/plugins/lists/server/routes/internal/create_exceptions_list_route.ts

+      options: {
+        tags: ['access:lists-all'],
+      },
+      path: `${INTERNAL_EXCEPTION_LIST_URL}/_create`,


Instead of defining this actual route here, can you create a const for it next to INTERNAL_EXCEPTION_LIST_URL?

elasticmachine · 2022-08-30T08:55:47Z

Pinging @elastic/security-onboarding-and-lifecycle-mgt (Team:Onboarding and Lifecycle Mgt)

dasansol92 · 2022-08-30T09:45:39Z

@elasticmachine merge upstream

…ption_lists-137577

x-pack/plugins/lists/server/routes/index.ts

yctercero · 2022-08-30T15:41:22Z

x-pack/plugins/lists/server/routes/internal/create_exceptions_list_route.ts

+      path: INTERNAL_EXCEPTIONS_LIST_ENSURE_CREATED_URL,
+      validate: {
+        body: buildRouteValidation<
+          typeof createExceptionListSchema,


What do you think of the following:

Update the public create route to restrict the list types that can be used to only those that can be publicly managed (which I think right now is just detection?)

Update this path to accept only list types that are internally managed

Might make each routes intent very clear to devs and avoid someone accidentally using them interchangeably?

This sounds good to me. Will add it for the internal one.

yctercero · 2022-08-30T15:46:13Z

...s/security_solution/public/management/services/exceptions_list/exceptions_list_api_client.ts

+  EXCEPTION_LIST_ITEM_URL,
+  EXCEPTION_LIST_URL,
+  INTERNAL_EXCEPTIONS_LIST_ENSURE_CREATED_URL,
+} from '@kbn/securitysolution-list-constants';
 import type { HttpStart } from '@kbn/core/public';
 import { MANAGEMENT_DEFAULT_PAGE, MANAGEMENT_DEFAULT_PAGE_SIZE } from '../../common/constants';



Is there any chance of updating this client name from ExceptionsListApiClient to something like ArtifactManagementExceptionsListApiClient - something indicating it's use for artifacts. At first glance I thought you were updating the main exceptions list client (an instance of which is also used throughout detections).

Yeah! Let me discuss this with the rest of the team. I'm not 100% sure but this refactor could be big so I would suggest doing it in another pr soon instead of including this change here.

…-137577' of github.com:dasansol92/kibana into feat/olm-new_list_endpoint_for_creating_exception_lists-137577

…ption_lists-137577

yctercero · 2022-08-31T16:16:36Z

From our team sync today:

What about using the GET to check in all these places instead of creating a new route?

cc @elastic/security-solution-platform

paul-tavares · 2022-08-31T19:07:07Z

...securitysolution-io-ts-list-types/src/request/internal/create_exception_list_schema/index.ts

+    t.partial({
+      // TODO: Move the ALL_ENDPOINT_ARTIFACT_LIST_IDS inside the package and use it here instead
+      list_id: t.keyof({
+        endpoint_trusted_apps: null,


Looks like we're limiting the use of this API to only these list_id's, correct?

if so, could we use the const that have the list id's defined here so that they remain in sync?

dasansol92 · 2022-09-01T07:31:01Z

@yctercero

What about using the GET to check in all these places instead of creating a new route?

We have already talked about this and using the existing GET to check if a list exists means having to do an extra call to create it if it does not exists yet.
That being said, this extra call would be done only the first time user visits an artifact page, once it is created, only the GET to ensure it exists would be done.
cc: @paul-tavares

paul-tavares · 2022-09-01T14:00:37Z

@yctercero confused on what you mean by GET?

We're trying to avoid having the API send back an Error if the list already exists. If we do a GET against a list that does not exists, we get an API error.

So today, we do a POST to the create list and ignore 409 in our code - but this still sends back an API error, which if you look at the original issue that triggered this PR, that is what we are trying to avoid. Having that Error be return in the API is giving others in Kibana (when investigating issues) a false sense that perhaps whatever issue they are investigating is due to our API error.

yctercero · 2022-09-01T16:29:54Z

@paul-tavares we were suggesting what @dasansol92 had mentioned in that you would only get an error once using a GET on the initial call to see if it exists and then every subsequent call to it would not return an error, just the list you're asking for. It seemed that core's issue with it was that you were seeing that 409 on every route change.

Just a suggestion as it avoids making any changes other than one additional call on your end. I'm not opposed to continuing with this PR but @nkhristinin had asked about just using a GET and I didn't have a good reason of why not so was curious what you all thought.

paul-tavares · 2022-09-01T18:13:01Z

@yctercero It will not address the original problem - we don't want to get an API error for something that we feel is a work-around to limitations in the Lists functionality (more on this below). We want to: check if our internal use only list exists and if not, then created it. Doing a GET instead of a POST to create the list does not resolve the problem because it still returns an error if the list does not exist.

To clarify: our UI code has no issues with the error that currently comes back (409). But because we now have to do some checks upfront when kibana first loads in support of the global search and navigation bars, this API error is being seen in the browser developer tools console and network tabs by other apps in kibana and that leads folks to believe there is a problem (see my comment here)

We can revert these changes here if the team does not feel this is needed in Lists plugin and by its consumers. We can always just create our own API under /api/endpoint that can do this for us. But it really does does feel like it belongs here.

About the Root Cause

The root cause of this work-around is the fact that Lists does not allow/support "reserved" list IDs. We have to ensure these types of internal lists are first created before we can use them. Because of this limitation, we are forced to work-around this problem by ensuring that we create the List (and ignore 409 if it already exists) before we can start to use it.

Personally - I would have liked that List introduced support for "internal" list names. I suggested this in multiple exchanges in the past and issue 109 (internal) and issue 982 (internal) remain open with that suggestion.

Some history

When our team first started working with the Lists plugins (Trusted apps) we actually went down the path of ensuring the list is created as soon as Kibana comes up so that we were not forced to always first ensure the list is created when attempting to consume the data, but ended up reverting that based on this comment from Frank

yctercero · 2022-09-03T19:10:17Z

...securitysolution-io-ts-list-types/src/request/internal/create_exception_list_schema/index.ts

+
+export const internalCreateExceptionListSchema = t.intersection([
+  t.exact(
+    t.type({


Nice - thanks for these updates!

yctercero

Code LGTM! Thanks @paul-tavares for the back and forth - just wanted to make sure we'd explored using what we already have before adding more code. But changes look good and clean!

dasansol92 · 2022-09-05T11:40:31Z

@elasticmachine merge upstream

…ption_lists-137577

paul-tavares

Changes look goo to me.

Q. Did you test the original issue when the user has no permissions to endpoint? Wondering if the creation of the list will actually cause an Error due to Auth. And if so, maybe we need to make some additional changes on the UI code to not call the API if user has no Authz. to endpoint (or maybe that is already done?)

…hecking links availability

dasansol92 · 2022-09-07T10:24:35Z

@paul-tavares Did a fix here cf631c4 to avoid doing create call when it is not needed or does not has permissions to do it.

It will still being an API error when accessing security solutions (without permissions) because this hook as the instance is created and the API call done at the beginning. Let me know if that make sense.

dasansol92 · 2022-09-07T12:25:30Z

@elasticmachine merge upstream

…ption_lists-137577

kibana-ci · 2022-09-07T13:31:14Z

💚 Build Succeeded

Buildkite Build
Commit: b41a27f

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`lists`	308	309	+1
`securitySolution`	3054	3055	+1
total			+2

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/securitysolution-io-ts-list-types`	449	452	+3
`@kbn/securitysolution-list-constants`	12	13	+1
total			+4

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`lists`	148.4KB	148.8KB	+449.0B
`securitySolution`	6.4MB	6.4MB	+347.0B
total			+796.0B

Unknown metric groups

API count

id	before	after	diff
`@kbn/securitysolution-io-ts-list-types`	462	465	+3
`@kbn/securitysolution-list-constants`	26	28	+2
total			+5

History

💚 Build #69996 succeeded cf631c4
💚 Build #69400 succeeded 19d9f82
💚 Build #69109 succeeded 72145b1
💚 Build #68609 succeeded 2288d28
💚 Build #68229 succeeded 418afef
💔 Build #68215 failed 88cafae

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

paul-tavares

🎉
🚢 it

paul-tavares · 2022-09-07T13:45:15Z

x-pack/plugins/security_solution/public/management/links.ts

-      core.http
-    );
-
+    if (!privileges.canAccessEndpointManagement) {


ashokaditya

Tested it out and it works as it should. I only have a minor nit. Feel free to 🚢 it.

ashokaditya · 2022-09-07T15:10:45Z

x-pack/plugins/lists/server/handlers/create_exception_list_handler.ts

+    type,
+    version,
+  } = request.body;
+  const exceptionLists = await getExceptionListClient(context);


nit: Better to name this exceptionListsClient.

new route for create an exception list and return the existing one if…

3aaeb59

… alredy exists

dasansol92 added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.5.0 labels Aug 29, 2022

dasansol92 mentioned this pull request Aug 29, 2022

409 error hitting /api/exception_lists on various screens across Kibana #137577

Closed

kibanamachine and others added 2 commits August 29, 2022 06:15

Merge branch 'main' into feat/olm-new_list_endpoint_for_creating_exce…

049fe51

…ption_lists-137577

Fixes unit test and shows error when ignore_existing set to false and…

d576e6d

… there is a conflict

paul-tavares reviewed Aug 29, 2022

View reviewed changes

Remove query param and update route name to be more specific

3591c3f

dasansol92 requested a review from paul-tavares August 30, 2022 08:53

Fixes unit test

88cafae

dasansol92 marked this pull request as ready for review August 30, 2022 08:55

dasansol92 requested review from a team as code owners August 30, 2022 08:55

dasansol92 requested a review from ashokaditya August 30, 2022 08:55

dasansol92 requested a review from yctercero August 30, 2022 08:56

Merge branch 'main' into feat/olm-new_list_endpoint_for_creating_exce…

418afef

…ption_lists-137577

yctercero reviewed Aug 30, 2022

View reviewed changes

x-pack/plugins/lists/server/routes/index.ts Show resolved Hide resolved

yctercero reviewed Aug 30, 2022

View reviewed changes

dasansol92 added 2 commits August 31, 2022 12:39

Enforce list_id and type types for internal route. Added unit tests

42ed063

Merge branch 'feat/olm-new_list_endpoint_for_creating_exception_lists…

827f563

…-137577' of github.com:dasansol92/kibana into feat/olm-new_list_endpoint_for_creating_exception_lists-137577

dasansol92 requested a review from yctercero August 31, 2022 10:41

Merge branch 'main' into feat/olm-new_list_endpoint_for_creating_exce…

2288d28

…ption_lists-137577

paul-tavares reviewed Aug 31, 2022

View reviewed changes

Uses existing constants to define list_ids

72145b1

dasansol92 requested a review from paul-tavares September 1, 2022 15:00

yctercero reviewed Sep 3, 2022

View reviewed changes

yctercero approved these changes Sep 3, 2022

View reviewed changes

Merge branch 'main' into feat/olm-new_list_endpoint_for_creating_exce…

19d9f82

…ption_lists-137577

paul-tavares approved these changes Sep 6, 2022

View reviewed changes

Don't create host isolation exeptions api client if not needed when c…

cf631c4

…hecking links availability

Merge branch 'main' into feat/olm-new_list_endpoint_for_creating_exce…

b41a27f

…ption_lists-137577

dasansol92 requested a review from paul-tavares September 7, 2022 12:25

paul-tavares approved these changes Sep 7, 2022

View reviewed changes

ashokaditya approved these changes Sep 7, 2022

View reviewed changes

dasansol92 merged commit e459752 into elastic:main Sep 8, 2022

[Security Solution][Endpoint] New route for create an exception list and return the existing one if it already exists #139618

[Security Solution][Endpoint] New route for create an exception list and return the existing one if it already exists #139618

Conversation

dasansol92 commented Aug 29, 2022 • edited Loading

Summary

For maintainers

dasansol92 commented Aug 29, 2022

paul-tavares 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

elasticmachine commented Aug 30, 2022

dasansol92 commented Aug 30, 2022

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

yctercero commented Aug 31, 2022

Choose a reason for hiding this comment

dasansol92 commented Sep 1, 2022

paul-tavares commented Sep 1, 2022

yctercero commented Sep 1, 2022

paul-tavares commented Sep 1, 2022

About the Root Cause

Some history

Choose a reason for hiding this comment

yctercero left a comment

Choose a reason for hiding this comment

dasansol92 commented Sep 5, 2022

paul-tavares left a comment

Choose a reason for hiding this comment

dasansol92 commented Sep 7, 2022

dasansol92 commented Sep 7, 2022

kibana-ci commented Sep 7, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

API count

History

paul-tavares left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashokaditya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dasansol92 commented Aug 29, 2022 •

edited

Loading