Skip to content

[Security Solution][Endpoint][Admin][Policy List] GET endpoint package policy api #119545

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
parkiino merged 20 commits into from
Dec 14, 2021
Merged

[Security Solution][Endpoint][Admin][Policy List] GET endpoint package policy api #119545

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
74ba729
api changes
Nov 23, 2021
b195533
swap out services url from ingest to our own endpoint api
Nov 23, 2021
a0e7470
Merge remote-tracking branch 'upstream/main' into task/get-endpoint-p…
Nov 29, 2021
b4cbe40
add request schema, try catch bloack
Nov 29, 2021
074a51f
copy fleet package policy request schema to security solution
Nov 30, 2021
efb6b85
add endpoint filtering to handler, throw error if needed
Nov 30, 2021
06fc5cb
Merge remote-tracking branch 'upstream/main' into task/get-endpoint-p…
Dec 1, 2021
dbd2072
Merge remote-tracking branch 'upstream/main' into task/get-endpoint-p…
Dec 2, 2021
e8497a1
modify schema to follow endpoint api patterns, revert policy/service …
Dec 3, 2021
8df1bc9
Merge remote-tracking branch 'upstream/main' into task/get-endpoint-p…
Dec 6, 2021
7f672d3
adjust kuery to include parentheses
Dec 6, 2021
c4b4845
a single passing unit test
Dec 7, 2021
d52464a
add another expect, remove unnecessary lines?
Dec 7, 2021
c885bb5
adding package policy service back, deleting unnecessary code
Dec 8, 2021
7405ead
Merge remote-tracking branch 'upstream/main' into task/get-endpoint-p…
Dec 8, 2021
7c189d1
Merge remote-tracking branch 'upstream/main' into task/get-endpoint-p…
Dec 8, 2021
d1e827a
finished tests
Dec 8, 2021
14980a1
Merge remote-tracking branch 'upstream/main' into task/get-endpoint-p…
Dec 14, 2021
7a7ae9b
fix change and add parens
Dec 14, 2021
1695276
fix jest test
Dec 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions x-pack/plugins/security_solution/common/endpoint/schema/policy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/

import { schema } from '@kbn/config-schema';
import type { TypeOf } from '@kbn/config-schema';

export const GetPolicyResponseSchema = {
query: schema.object({
Expand All @@ -19,3 +20,21 @@ export const GetAgentPolicySummaryRequestSchema = {
policy_id: schema.nullable(schema.string()),
}),
};

const ListWithKuerySchema = schema.object({

@joeypoon joeypoon Dec 9, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@joeypoon joeypoon Dec 9, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

page: schema.maybe(schema.number({ defaultValue: 1 })),
perPage: schema.maybe(schema.number({ defaultValue: 20 })),

@joeypoon joeypoon Nov 30, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

sortField: schema.maybe(schema.string()),

@joeypoon joeypoon Nov 30, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

sortOrder: schema.maybe(schema.oneOf([schema.literal('desc'), schema.literal('asc')])),
showUpgradeable: schema.maybe(schema.boolean()),
kuery: schema.maybe(
schema.oneOf([
schema.string(),
schema.any(), // KueryNode
])
),
});

export const GetEndpointPackagePolicyRequestSchema = {
query: ListWithKuerySchema,
};
4 changes: 2 additions & 2 deletions x-pack/plugins/security_solution/public/management/services/policies.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {
GetPackagePoliciesRequest,
PACKAGE_POLICY_SAVED_OBJECT_TYPE,
} from '../../../../fleet/common';
import { INGEST_API_PACKAGE_POLICIES } from '../pages/policy/store/services/ingest';
import { BASE_POLICY_ROUTE } from '../../../common/endpoint/constants';
import { GetPolicyListResponse } from '../pages/policy/types';

/**
Expand All @@ -23,7 +23,7 @@ export const sendGetEndpointSpecificPackagePolicies = (
http: HttpStart,
options: HttpFetchOptions & Partial<GetPackagePoliciesRequest> = {}
): Promise<GetPolicyListResponse> => {
return http.get<GetPolicyListResponse>(INGEST_API_PACKAGE_POLICIES, {
return http.get<GetPolicyListResponse>(BASE_POLICY_ROUTE, {
...options,
query: {
...options.query,
Expand Down
26 changes: 26 additions & 0 deletions x-pack/plugins/security_solution/server/endpoint/routes/policy/handlers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ import {
import { EndpointAppContext } from '../../types';
import { getAgentPolicySummary, getPolicyResponseByAgentId } from './service';
import { GetAgentSummaryResponse } from '../../../../common/endpoint/types';
import { GetPackagePoliciesRequest } from '../../../../../fleet/common/types/rest_spec';
import { EndpointError } from '../../../../common/endpoint/errors';
import { wrapErrorIfNeeded } from '../../utils';
import { PACKAGE_POLICY_SAVED_OBJECT_TYPE } from '../../../../../fleet/common';

export const getHostPolicyResponseHandler = function (): RequestHandler<
undefined,
Expand Down Expand Up @@ -63,3 +67,25 @@ export const getAgentPolicySummaryHandler = function (
});
};
};

export const getPolicyListHandler = function (
endpointAppContext: EndpointAppContext
): RequestHandler<undefined, GetPackagePoliciesRequest['query'], undefined> {
return async (context, request, response) => {
const soClient = context.core.savedObjects.client;
const packagePolicyService = endpointAppContext.service.getPackagePolicyService();
const endpointFilteredKuery = `${

@paul-tavares paul-tavares Dec 13, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@paul-tavares paul-tavares Dec 14, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

request?.query?.kuery ? `${request.query.kuery} and ` : ''
}${PACKAGE_POLICY_SAVED_OBJECT_TYPE}.package.name: endpoint`;
request.query.kuery = endpointFilteredKuery;
try {
const listResponse = await packagePolicyService.list(soClient, request.query);

@paul-tavares paul-tavares Nov 29, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


return response.ok({
body: listResponse,
});
} catch (error) {
throw wrapErrorIfNeeded(error);
}
Comment on lines +72 to +97

@paul-tavares paul-tavares Nov 24, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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)
  2. it helps the kibana logs to see the original code that was encurred when we called the fleet service

};
};
17 changes: 16 additions & 1 deletion x-pack/plugins/security_solution/server/endpoint/routes/policy/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,16 @@ import { EndpointAppContext } from '../../types';
import {
GetPolicyResponseSchema,
GetAgentPolicySummaryRequestSchema,
GetEndpointPackagePolicyRequestSchema,
} from '../../../../common/endpoint/schema/policy';
import { getHostPolicyResponseHandler, getAgentPolicySummaryHandler } from './handlers';
import {
getHostPolicyResponseHandler,
getAgentPolicySummaryHandler,
getPolicyListHandler,
} from './handlers';
import {
AGENT_POLICY_SUMMARY_ROUTE,
BASE_POLICY_ROUTE,
BASE_POLICY_RESPONSE_ROUTE,
} from '../../../../common/endpoint/constants';

Expand All @@ -37,4 +43,13 @@ export function registerPolicyRoutes(router: IRouter, endpointAppContext: Endpoi
},
getAgentPolicySummaryHandler(endpointAppContext)
);

router.get(
{
path: BASE_POLICY_ROUTE,
validate: GetEndpointPackagePolicyRequestSchema,
options: { authRequired: true },
},
getPolicyListHandler(endpointAppContext)
);
}