[Security Solution][Endpoint][Admin][Policy List] GET endpoint package policy api by parkiino · Pull Request #119545 · elastic/kibana

parkiino · 2021-11-23T21:11:09Z

Summary

Creates a new decoupled endpoint api: GET /api/endpoint/policy : which returns a list of package policies
Swaps out the original fleet api endpoint with the new endpoint api in the service: sendGetEndpointSpecificPackagePolicies
Unit tests

elasticmachine · 2021-11-23T21:11:12Z

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

elasticmachine · 2021-11-23T21:11:12Z

Pinging @elastic/esecurity-onboarding-and-lifecycle-mgt (Feature:Endpoint)

parkiino · 2021-11-23T21:11:46Z

x-pack/plugins/security_solution/server/endpoint/routes/policy/index.ts

+  router.get(
+    {
+      path: BASE_POLICY_ROUTE,
+      validate: false,


need to add the schema from fleet

Can you include the schema validation prior to committing this?
The schema you want is GetPackagePoliciesSchema located here:

kibana/x-pack/plugins/fleet/server/types/rest_spec/package_policy.ts

Lines 14 to 16 in b1253db

export const GetPackagePoliciesRequestSchema = {

query: ListWithKuerySchema,

};

I'm not srue if you can import directly from fleet/server/types. if not, then just export it out of fleet's server/index

paul-tavares

Can you also please add some tests?

Its looking good. I left some comments. Let me know if you have any questions.

paul-tavares · 2021-11-24T14:56:08Z

x-pack/plugins/security_solution/server/endpoint/routes/policy/handlers.ts

+        body: doc,
+      });
+    }
+    return response.notFound({ body: 'Failed to retrieve package policy list' });


I don't think this is right. If the above query did not throw and it returned null then that implies that are none, right? on a list type of API when this is case, we should return the normal list response with an empty array for the items.

paul-tavares · 2021-11-24T15:05:42Z

x-pack/plugins/security_solution/server/endpoint/routes/policy/handlers.ts

+    const soClient = context.core.savedObjects.client;
+    const packagePolicyService = endpointAppContext.service.getPackagePolicyService();
+    const doc = await packagePolicyService.list(soClient, request.query);
+    if (doc) {
+      return response.ok({
+        body: doc,
+      });
+    }


this should be wrapped in a try {} catch (error) {} and the catch of the error should then do this:

Suggested change

const soClient = context.core.savedObjects.client;

const packagePolicyService = endpointAppContext.service.getPackagePolicyService();

const doc = await packagePolicyService.list(soClient, request.query);

if (doc) {

return response.ok({

body: doc,

});

}

return response.notFound({ body: new EndpointError('Failed to retrieve package policy list', error) });

Which does a few thing:

it returns back to the consumer of this API a body that is consistent with other errors (normally it reaches the UI code as an object containing the message property)

it helps the kibana logs to see the original code that was encurred when we called the fleet service

paul-tavares · 2021-11-24T15:11:00Z

x-pack/plugins/security_solution/server/endpoint/routes/policy/index.ts

+  router.get(
+    {
+      path: BASE_POLICY_ROUTE,
+      validate: false,


Can you include the schema validation prior to committing this?
The schema you want is GetPackagePoliciesSchema located here:

kibana/x-pack/plugins/fleet/server/types/rest_spec/package_policy.ts

Lines 14 to 16 in b1253db

export const GetPackagePoliciesRequestSchema = {

query: ListWithKuerySchema,

};

I'm not srue if you can import directly from fleet/server/types. if not, then just export it out of fleet's server/index

paul-tavares · 2021-11-24T15:19:09Z

x-pack/plugins/security_solution/server/endpoint/routes/policy/handlers.ts

+  return async (context, request, response) => {
+    const soClient = context.core.savedObjects.client;
+    const packagePolicyService = endpointAppContext.service.getPackagePolicyService();
+    const doc = await packagePolicyService.list(soClient, request.query);


doc is a bit misleading since we're getting a list of docs. maybe rename it to listResponse?
Also, we should put a type on this that matches the type of the data this API is meant to return (GetPackagePoliciesResponse). That will ensure that if the service return value is ever changed in fleet, that we catch that here and ensure that the API response is returning the expected structure.

I'm now wondering if we should create a new service to handle fleet "stuff". We can't just pass through the request.query here because we need to ensure that all calls to fleet for data is always filtered to retrieve only endpoint related data. so this request needs to ensure that we are requesting package policies whose package.name is endpoint. This additional filter needs to be included in the call or added to any kuery that came in on the Request.: ex:

kuery = `${request.query.kuery ? `(${request.query.kuery}) AND ` : ''}(${fleet_package_policy_so_type}.package.name: endpoint)`

because of this, I'm thinking we want to create our own service (ex. FleetDataService)

cc/ @pzl

in my poc this route will be called in the existing sendGetEndpointSpecificPackagePolicies which is in public/management/services/policies.ts which does add an additional filter for endpoint specific policies. Although I guess if people tried using the api directly without using that service, it doesn't pass along the endpoint filtering query

…ackagepolicy-api

academo · 2021-11-29T17:41:41Z

x-pack/plugins/security_solution/server/endpoint/routes/policy/handlers.ts

+        body: listResponse,
+      });
+    } catch (error) {
+      return response.notFound({


I am not yet familiar with the way we do server API so let me know is this is the way we do things:

You are assuming here the reason why the request failed is because it was not found (404) but the error can be that and anything else. I'd argue that a 500 error makes more sense here since the error was unexpected and the API itself does exist.

agreed - this should be a 500 here.

academo

Looks good to me. Check if the 404 error really makes sense for the API otherwise good.

paul-tavares

can you add some tests?

paul-tavares · 2021-11-29T18:38:49Z

x-pack/plugins/security_solution/server/endpoint/routes/policy/handlers.ts

+        body: listResponse,
+      });
+    } catch (error) {
+      return response.notFound({


agreed - this should be a 500 here.

paul-tavares · 2021-11-29T18:41:12Z

x-pack/plugins/security_solution/server/endpoint/routes/policy/handlers.ts

+    const soClient = context.core.savedObjects.client;
+    const packagePolicyService = endpointAppContext.service.getPackagePolicyService();
+    try {
+      const listResponse = await packagePolicyService.list(soClient, request.query);


See my prior comment around ensuring we are not creating a path to retrieve non-endpoint packages with this API. All calls to this API need to ensure they are being wrapped with a filter that looks only at endpoint integrations. I'm ok if you want to do that here for now, but we may want to create a service module in our code base that can do that.

joeypoon · 2021-11-30T22:28:13Z

x-pack/plugins/security_solution/common/endpoint/schema/policy.ts

+
+const ListWithKuerySchema = schema.object({
+  page: schema.maybe(schema.number({ defaultValue: 1 })),
+  perPage: schema.maybe(schema.number({ defaultValue: 20 })),


can we rename this to pageSize for consistency

kibana/x-pack/plugins/security_solution/common/endpoint/schema/metadata.ts

Line 16 in 3d0bc8b

pageSize: schema.number({ defaultValue: ENDPOINT_DEFAULT_PAGE_SIZE, min: 1, max: 10000 }),

kibana/x-pack/plugins/security_solution/common/endpoint/types/index.ts

Line 1241 in 3d0bc8b

pageSize: number;

joeypoon · 2021-11-30T22:34:46Z

x-pack/plugins/security_solution/common/endpoint/schema/policy.ts

+const ListWithKuerySchema = schema.object({
+  page: schema.maybe(schema.number({ defaultValue: 1 })),
+  perPage: schema.maybe(schema.number({ defaultValue: 20 })),
+  sortField: schema.maybe(schema.string()),


I think we agreed on sort for this https://github.com/elastic/security-team/issues/2149 though I don't mind this more explicit naming.

…ackagepolicy-api

…for now

…ackagepolicy-api

joeypoon · 2021-12-09T16:08:03Z

x-pack/plugins/security_solution/server/endpoint/routes/policy/handlers.test.ts

+      });
+    });
+
+    it('should add endpoint-specific kuery to the requests kuery', async () => {


would be good to also add tests for the other query params too.

joeypoon · 2021-12-09T16:17:27Z

x-pack/plugins/security_solution/common/endpoint/schema/policy.ts

  }),
 };
+
+const ListWithKuerySchema = schema.object({


would be good to add tests for these. I have a suspicion that these default values might not work since they're wrapped in a maybe.

example schema tests: https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/common/endpoint/schema/metadata.test.ts.

joeypoon · 2021-12-09T16:20:17Z

x-pack/plugins/security_solution/server/endpoint/routes/policy/handlers.test.ts

      });
    });
  });
+  describe('test GET policy list handler', () => {


we should add some integration tests as well. I'm guessing in here: https://github.com/elastic/kibana/blob/main/x-pack/test/security_solution_endpoint_api_int/apis/policy.ts.

academo · 2021-12-13T09:24:41Z

x-pack/plugins/security_solution/server/endpoint/routes/policy/handlers.test.ts

        ? [
            {
-              _index: 'metrics-endpoint.policy-default-1',
+              _index: 'ingest-package-policies',


I am not familiar with this code but just checking this change is intended.

yeah, this is probably not intended. This function seems to be mocking the ES response for when searching for the Endpoint's Policy Response document.

paul-tavares · 2021-12-13T14:41:01Z

x-pack/plugins/security_solution/server/endpoint/routes/policy/handlers.test.ts

        ? [
            {
-              _index: 'metrics-endpoint.policy-default-1',
+              _index: 'ingest-package-policies',


yeah, this is probably not intended. This function seems to be mocking the ES response for when searching for the Endpoint's Policy Response document.

paul-tavares · 2021-12-13T14:48:22Z

x-pack/plugins/security_solution/server/endpoint/routes/policy/handlers.ts

+  return async (context, request, response) => {
+    const soClient = context.core.savedObjects.client;
+    const fleetServices = endpointAppContext.service.getScopedFleetServices(request);
+    const endpointFilteredKuery = `${


This does not look correct - you need to ensure that both side of the AND are wrapped in parentheses

Suggested change

const endpointFilteredKuery = `${

const endpointFilteredKuery = `${request?.query?.kuery ? `(${request.query.kuery}) AND ` : ''

}(${PACKAGE_POLICY_SAVED_OBJECT_TYPE}.package.name: endpoint)`

Here is a test that you can do to see why this is important (note: i did not actually execute this, but you should be able to and get the results I'm thinking you should get):

call this API (prior to my suggestion above) with: query.kuery: '(ingest-package-policies.package.name: nginx) OR '

This will likely match and return package policies for nginx (if you have any) instead of only looking for endpoint. Thats because the parentheses are evaluated first and if true, then the other side of the OR will never be eval'd and thus the API can be abused to actually query for non-endpoint stuff.

I worked with @parkiino yesterday on this and we validated that what i thought was going to happen (retrieval of non-endpoint package policies) did not actually happen. that's because even if a user of the API ends their kql with 0R, when we append the AND package.name: endpoint to it, you get a KQL parsing error (...OR AND...). So I think we're good, especially since parentheses were added around the request's kuery value (if any).

paul-tavares · 2021-12-13T14:53:15Z

x-pack/plugins/security_solution/server/endpoint/routes/policy/handlers.test.ts

+        mockResponse
+      );
+      expect(mockPackagePolicyService.list.mock.calls[0][1]).toEqual({
+        kuery: `some query and ${PACKAGE_POLICY_SAVED_OBJECT_TYPE}.package.name: endpoint`,


This test assertion is not correct. the kuery should have both sides of the AND wrapped in parentheses - like: (some query) AND (${PACKAGE_POLICY_SAVED_OBJECT_TYPE}.package.name: endpoint)

paul-tavares

🔥
Thanks @parkiino for working on this and following through on all the feedback. Like @joeypoon mentioned, please create an issue to add more tests - including integration tests and follow through on that prior to 8.1.

Also - reminder: this probably does not need to go into 8.0. only 8.1

…ackagepolicy-api

pzl

🚋

kibana-ci · 2021-12-14T21:44:50Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💔 Build #13175 failed 7a7ae9b
💚 Build #12283 succeeded d1e827a
💔 Build #12029 failed 7405ead
💚 Build #11626 succeeded d52464a
💚 Build #10861 succeeded e8497a1

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

kibanamachine · 2021-12-14T21:51:24Z

💔 Backport failed

The backport operation could not be completed due to the following error:
There are no branches to backport to. Aborting.

The backport PRs will be merged automatically after passing CI.

To backport manually run:
node scripts/backport --pr 119545

…e policy api (elastic#119545)

parkiino added 2 commits November 23, 2021 15:30

api changes

74ba729

swap out services url from ingest to our own endpoint api

b195533

parkiino requested a review from paul-tavares November 23, 2021 21:11

parkiino requested review from a team as code owners November 23, 2021 21:11

parkiino requested a review from pzl November 23, 2021 21:11

parkiino commented Nov 23, 2021

View reviewed changes

paul-tavares reviewed Nov 24, 2021

View reviewed changes

parkiino added 2 commits November 29, 2021 11:21

Merge remote-tracking branch 'upstream/main' into task/get-endpoint-p…

a0e7470

…ackagepolicy-api

add request schema, try catch bloack

b4cbe40

parkiino requested a review from academo November 29, 2021 17:30

academo reviewed Nov 29, 2021

View reviewed changes

academo approved these changes Nov 29, 2021

View reviewed changes

paul-tavares reviewed Nov 29, 2021

View reviewed changes

parkiino added 2 commits November 30, 2021 16:29

copy fleet package policy request schema to security solution

074a51f

add endpoint filtering to handler, throw error if needed

efb6b85

joeypoon reviewed Nov 30, 2021

View reviewed changes

parkiino added 3 commits December 1, 2021 16:37

Merge remote-tracking branch 'upstream/main' into task/get-endpoint-p…

06fc5cb

…ackagepolicy-api

Merge remote-tracking branch 'upstream/main' into task/get-endpoint-p…

dbd2072

…ackagepolicy-api

modify schema to follow endpoint api patterns, revert policy/service …

e8497a1

…for now

pzl approved these changes Dec 3, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/main' into task/get-endpoint-p…

8df1bc9

…ackagepolicy-api

parkiino added 7 commits December 6, 2021 17:01

adjust kuery to include parentheses

7f672d3

a single passing unit test

c4b4845

add another expect, remove unnecessary lines?

d52464a

adding package policy service back, deleting unnecessary code

c885bb5

Merge remote-tracking branch 'upstream/main' into task/get-endpoint-p…

7405ead

…ackagepolicy-api

Merge remote-tracking branch 'upstream/main' into task/get-endpoint-p…

7c189d1

…ackagepolicy-api

finished tests

d1e827a

parkiino requested review from academo, joeypoon, paul-tavares and pzl December 8, 2021 20:51

joeypoon reviewed Dec 9, 2021

View reviewed changes

academo reviewed Dec 13, 2021

View reviewed changes

paul-tavares reviewed Dec 13, 2021

View reviewed changes

parkiino removed the v8.0.0 label Dec 13, 2021

paul-tavares approved these changes Dec 14, 2021

View reviewed changes

parkiino added 2 commits December 14, 2021 11:15

Merge remote-tracking branch 'upstream/main' into task/get-endpoint-p…

14980a1

…ackagepolicy-api

fix change and add parens

7a7ae9b

pzl approved these changes Dec 14, 2021

View reviewed changes

fix jest test

1695276

parkiino merged commit 9b15697 into elastic:main Dec 14, 2021

kibanamachine added the backport:skip This PR does not require backporting label Dec 14, 2021

parkiino deleted the task/get-endpoint-packagepolicy-api branch December 14, 2021 21:51

TinLe pushed a commit to TinLe/kibana that referenced this pull request Dec 22, 2021

[Security Solution][Endpoint][Admin][Policy List] GET endpoint packag…

255ad7f

…e policy api (elastic#119545)

parkiino mentioned this pull request Jan 26, 2022

[Security Solution][Endpoint][Policy] Remove GET policy list api route #123873

Merged

	export const GetPackagePoliciesRequestSchema = {
	query: ListWithKuerySchema,
	};

	const endpointFilteredKuery = `${
	const endpointFilteredKuery = `${request?.query?.kuery ? `(${request.query.kuery}) AND ` : ''
	}(${PACKAGE_POLICY_SAVED_OBJECT_TYPE}.package.name: endpoint)`

Conversation

parkiino commented Nov 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

elasticmachine commented Nov 23, 2021

Uh oh!

elasticmachine commented Nov 23, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

academo left a comment

Choose a reason for hiding this comment

Uh oh!

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

joeypoon Dec 9, 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!

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!

paul-tavares left a comment

Choose a reason for hiding this comment

Uh oh!

pzl left a comment

Choose a reason for hiding this comment

Uh oh!

kibana-ci commented Dec 14, 2021

💚 Build Succeeded

Metrics [docs]

History

Uh oh!

kibanamachine commented Dec 14, 2021

parkiino commented Nov 23, 2021 •

edited

Loading

joeypoon Dec 9, 2021 •

edited

Loading