[SECURITY_SOLUTION][ENDPOINT] Trusted Apps List API #75476

paul-tavares · 2020-08-19T18:27:34Z

Summary

Introduce new API for retrieving list of Trusted Apps. Also includes a script for loading Trusted Apps entries while the Create API is unavailable (may delete that script once that is available).

In addition the endpoint types under common were moved to a directory named types in order to be able to add separate type files per domain (ex. trusted_apps types)

Request:

API route: GET /api/endpoint/trusted_apps
Query Params:

{
    page: number;
    per_page: number;
}

Results will be sorted by name in ascending order

Response:

{
    data: Array<TrustedApp>;
    page: number;
    per_page: number;
    total: number;
}

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

…ted-apps-list-api

efreeti · 2020-08-20T13:53:15Z

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

+export const GetTrustedAppsRequestSchema = {
+  query: schema.object({
+    page: schema.maybe(schema.number({ defaultValue: 1, min: 1 })),
+    per_page: schema.maybe(schema.number({ defaultValue: 20, min: 1 })),


Just out of curiosity and not for bikesheding - why not page_size?

mirror of lists API - they use these values. I also seem to remember that anything outside of code should use _ and only code vars/etc. should camelCase. I think Kibana even has an ESLint rule for it.

efreeti · 2020-08-20T13:54:03Z

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

+
+export const GetTrustedAppsRequestSchema = {
+  query: schema.object({
+    page: schema.maybe(schema.number({ defaultValue: 1, min: 1 })),


I always assumed 0 based is easier to implement, but maybe it's true that we need to show one based.

I'm of the opposite opinion - I find it 1 based to be clearer from a consumer (UI?) standpoint. I want page 1.
The service is 1 based and FYI - we have an issue opened to also refactor Endpoint hosts page to use 1 based pagination.

+1 to 1-based page numbers.

I consider the URL-bar to be human-facing as much as this

efreeti · 2020-08-20T13:54:31Z

x-pack/plugins/security_solution/common/endpoint/constants.ts

@@ -11,3 +11,5 @@ export const policyIndexPattern = 'metrics-endpoint.policy-*';
 export const telemetryIndexPattern = 'metrics-endpoint.telemetry-*';
 export const LIMITED_CONCURRENCY_ENDPOINT_ROUTE_TAG = 'endpoint:limited-concurrency';
 export const LIMITED_CONCURRENCY_ENDPOINT_COUNT = 100;
+
+export const TRUSTED_APPS_LIST_API = '/api/endpoint/trusted_apps';


why /endpoint?

other endpoint apis are mounted at this location

efreeti · 2020-08-20T13:56:36Z

x-pack/plugins/security_solution/common/endpoint/types/trusted_apps.ts

+  ExceptionListItemSchema,
+  'created_at' | 'created_by' | 'name' | 'description' | 'entries'
+> & {
+  os: string;


It feels like enum would be more appropriate.

Yeah, it will probably be an enum - just need to find the one that List is using (if one exists) and reuse it or come up with our own. FYI: this os field does not exist today on the data storage schema - the Trusted Apps API will populate it from the _tags[] array.

we could introduce an enum with with the Create flow and validation

We have a ticket to add the os to the exception list item schema in 7.10

Yes, we can create the Enum either with the UI changes or when we add the Create api.

@peluja1012 sounds good. Madi had mentioned that to me, so we're aware we will have to adjust our "translator" once that gets introduced.

efreeti · 2020-08-20T14:15:20Z

x-pack/plugins/security_solution/common/endpoint/types/trusted_apps.ts

+/** Type for a new Trusted App Entry */
+export type NewTrustedApp = Pick<
+  ExceptionListItemSchema,
+  'created_at' | 'created_by' | 'name' | 'description' | 'entries'


A question and a comment here:

Question: Are we not using camel casing for APIs?

Remark: So sending entries means that we will have to have logic on the client that will have to translate the entries into a single field applicationPath. My personal view on it is that it's unnecessarily complicates client code without good reason - it's more code, it will have to handle error cases (like empty list or missing item with needed field name), it exposes internals (like field names) to the client. In addition it might trigger users to explore what they can submit through that loop hole and it makes it less straight forward to document this API (again binding ourselves to internal event field names that might change, and it's not very obvious that this frontend code should evolve along with the events code).

I really would propose to not use Pick<...> and couple ourselves to how ExceptionListItemSchema is defined but just define our own interface for API data structure. This has multiple benefits:

Even from readability point of view it's way more straight forward and readable. Currently I'll have to navigate to ExceptionListItemSchema to know the types of the fields, which is not very convenient.

We have a freedom to use names that more convenient for our view

It will prevent accidental leak through - assume the case we change created_at from timestamp string to milliseconds number, that means that the code compiles but frontend fails, while having own data structure and translation code will fail on compile.

We don't add complexity to the frontend but rather keep it far away from it.

As mentioned this translation will have to happen somewhere and my experience suggests that it's best to push it as much back in the layers as possible, otherwise the complexity of this data structure complicates all layers.

re: casing - it comes down to what I have seen around in kibana: non code references are camelCased, everything else is snake_cased.

Re:

So sending entries means that we will have to have logic on the client that will have to translate the entries into a single field applicationPath.

Not sure I follow - why would you need logic on the client to translate?

Re: Type and pick from Exceptions
I think our data types should be defined to match the data storage as close as possible. In this case, because we (trusted apps and Exceptions) share the same storage schema, I feel its best to pick the properties that we will reuse from that schema, so that it it changes in the future to something that we don't support, we can get those (TS) errors surfaced at the earlier possible time. This is also consistent with usage patterns I have seen across kibana.

efreeti · 2020-08-20T15:02:34Z

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

+    // FIXME:PT call create list once PR merges
+
+    try {
+      const results = await exceptionsListService?.findExceptionListItem({


Hmm, the exceptionListService is optional?

I was surprise as well. I may replace ? with !.

I fixed it by changing the EndpointAppContextService.getExceptionList()

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

efreeti · 2020-08-20T15:04:34Z

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

+  const logger = endpointAppContext.logFactory.get('trusted_apps');
+
+  return async (context, req, res) => {
+    const { page, per_page: perPage } = req.query;


This is why I'm wondering if it's a convention to follow "_" naming style for REST :)

efreeti · 2020-08-20T15:08:25Z

x-pack/plugins/security_solution/server/endpoint/routes/trusted_apps/utils.ts

+export const exceptionItemToTrustedAppItem = (
+  exceptionListItem: ExceptionListItemSchema
+): TrustedApp => {
+  // eslint-disable-next-line @typescript-eslint/naming-convention


Same point about "_" namings in REST, maybe don't use destruction instead?

efreeti · 2020-08-20T15:15:52Z

x-pack/plugins/security_solution/server/endpoint/routes/trusted_apps/utils.ts

+      return tag;
+    }
+  }
+  return '';


I know it's not "supposed" to happen, but I'd rather fail here than hide the error, there is no meaningful handling of this in UI.

This is always a problem of using generic structures to represent more specific concepts - they leak through and break invariance. Not trying to trigger same discussion again :D just it would be so nicer to just use SavedObject ;)

Actually again a bit on the topic of our own API interface instead of inheriting ExceptionListItem - we now scatter transformations: OS is extracted on the server while path/hash/signer will have to be extracted on the client. I'd again suggest to not send entries but rather translate it to the data structure we can directly consume in UI. This is also why I mentioned in grooming that I would not incorporate signer in API for now.

You are right in that this should NOT happen. But if for some reason that is true, then we are essentially making the list API (and LIST UI) basically unusable. (API will never return if at least 1 entry has no OS.

could we have it return unknown at least so the UI could use it if needed?

Ok. I will set it to a string unknown, but that will likely cause issues once we introduce a type for it like an enum.

'unknown' feels like a magic string waiting to happen (until it is an explicit enum case). Magic strings should not happen from the server. If the front-end ever wanted to display missing OS's differently (like a blank column), one would have to filter for the string value, which is a bit code-smelly. I didn't look up the call chain where this is used, but can this return string | null? The missing case, being null. That would perhaps offer the front-end an opportunity to display/handle explicitly. At the cost of null-checking, which also isn't fun, but more common.

Stop me if this is anti-typescript behavior.

efreeti · 2020-08-20T15:29:57Z

So again on the topic of our own interface for return types from API - I think we should go with that option as it's justifies having own API, otherwise it's a bit half way solution and has less justification for separate API.

In the end think of it like this - we should try to encode as much as possible invariance into API structures so that client has to face way less ambiguity and handle weird invalid cases (like empty OS, empty entries, missing known entries, etc.). I think mostly you already do it and I would suggest to do it also for "entries" field: replace it with data structure that preserves invariance for trusted app matching that we expect in UI.

For example exposing field in entries will not force people's code to fail if they try to send matching signer for Linux.

As a thought I would go even further:

interface WindowsTrustedAppConditions {
  platform: 'windows';
  signer?: string;
  hash?: string;
  path?: string;
}

interface MacTrustedAppConditions {
  platform: 'mac';
  hash?: string;
  path?: string;
}

interface LinuxTrustedAppConditions {
  platform: 'linux';
  hash?: string;
  path?: string;
}

type TrustedAppConditions = WindowsTrustedAppConditions | MacTrustedAppConditions | LinuxTrustedAppConditions;

interface TrustedApp {
  id: string;
  name: string;
  description: string;
  createdAt: Date;
  createdBy: string;
  conditions: TrustedAppConditions;
}

@paul-tavares , @kevinlog , @pzl , @nnamdifrankie what do you guys think? The above could really provide a high level of invariance on the domain entity that we manipulate.

x-pack/plugins/security_solution/common/endpoint/types/trusted_apps.ts

paul-tavares · 2020-08-20T19:59:39Z

@efreeti
I think we are in sync with the exception that you would like to call it conditions and I'm advocating for keeping the same entries property name. I find it to be an unnecessary conversion to be done at the API level or UI because it does not bubble up to the user. They are just entries that defined the conditions for the "exception". It also matches the internal field that is used to send the data down to the endpoint, so does (IMO) help sync thing up for a dev. Also, keeping the same name of this property will assist with possibly re-using the current code in the Manifest Manager component to create the actual artifact, so that we don't have to come up with our own translator there too.

Re: the types you defined above
I'm not sure that lines up with what needs to be supported in the UI. I don't think you can capture the os inside of the condition, because it is something you define at the "exception rule" level, which then dictates what you can store inside of the entries[] array. Might be able to defined multiple Trusted Apps types - that would work. Example:

interface MacosLinuxConditionEntry {
  field: 'hash' | 'path';
  type: 'match';
  operator: 'included';
  value: string;
}

type WindowsConditionEntry = MacosLinuxConditionEntry & {
  field: 'signer';
};

type TrustedApp =
  | {
      id: string;
      name: string;
      description: string;
      createdAt: Date;
      createdBy: string;
      os: 'windows';
      entries: WindowsConditionEntry[];
    }
  | {
      id: string;
      name: string;
      description: string;
      createdAt: Date;
      createdBy: string;
      os: 'linux' | 'macos';
      entries: MacosLinuxConditionEntry[];
    };

We can remove the type and operator from each entry, since this will not be supported via the UI and could have the API automatically add that in on create.

With the above structure, you would get type guards against invalid combinations:

…ted-apps-list-api

…ws if service not available

paul-tavares · 2020-08-24T16:42:23Z

x-pack/plugins/security_solution/scripts/endpoint/trusted_apps/index.ts

@@ -0,0 +1,91 @@
+/*


FYI - this utility is to assist with dev. of the Trusted Apps list until we have a create flow. I would expect this to be deleted once the create flow is merged.

elasticmachine · 2020-08-24T18:32:54Z

Pinging @elastic/endpoint-app-team (Feature:Endpoint)

elasticmachine · 2020-08-24T18:32:56Z

Pinging @elastic/endpoint-management (Team:Endpoint Management)

paul-tavares · 2020-08-24T18:35:05Z

x-pack/plugins/lists/common/schemas/response/exception_list_schema.mock.ts

@@ -42,6 +47,15 @@ export const getExceptionListSchemaMock = (): ExceptionListSchema => ({
  version: VERSION,
 });

+export const getTrustedAppsListSchemaMock = (): ExceptionListSchema => {


@madirey FYI
Created this so that the client mock (below) can include it. I debated whether I should have created a sub-class of the Client under trusted apps and felt it belonged here more than in our feature code

kevinlog · 2020-08-24T21:19:52Z

x-pack/plugins/security_solution/server/endpoint/routes/trusted_apps/utils.ts

+ * Map an ExcptionListItem to a TrustedApp item
+ * @param exceptionListItem
+ */
+export const exceptionItemToTrustedAppItem = (


this is good and can help show how we use Exceptions list

kevinlog

just a couple comments. I'm good with the direction that we agreed on with @peluja1012 and @madirey team to base our structure on Exceptions. The exceptionItemToTrustedAppItem and type are good ways to communicate that relationship.

efreeti · 2020-08-25T09:59:15Z

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

+  // GET list
+  router.get(
+    {
+      path: TRUSTED_APPS_LIST_API,


I haven't seen other server code creating such constants.

I wish they did. Both the UI and the Server can benefit from such constants since both need it. Ingest uses this heavily and it helps allot (ex. we import some of their consts and use it in our API calls - thus if they change, we will not break).

This const should be re-used by the UI's middleware when making the API call.

@efreeti don't treat the existing endpoint/management server code as stylistic gospel. It was developed while we were still new to convention, and right after basic POCs.

An argument can be made to "match what's there" not because it's better but just for the sake of consistency. I can accept that point. Then if we want to change the style for all, it's an easier task if they all have the same pattern to -> from.

However I also like and prefer this idea. We should just have a tech debt item to change the existing patterns to have const routes like this. It can then be quite easy to skim our defined URL endpoints in one place.

efreeti · 2020-08-25T10:28:33Z

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

+  router.get(
+    {
+      path: TRUSTED_APPS_LIST_API,
+      validate: GetTrustedAppsRequestSchema,


I noticed that all other schemas are just made inline instead of separate files. It seems to me to make sense - controller layer should only contain HTTP mapping related functionality hence it is already separate responsibility (separate concern) and splitting it further only creates more indirection, rather keeping path, schema and other things inline here gives a really good overview of API.

I see multiple patterns across the app and Kibana. Defining it inline here makes it harder (impossible?) to test because currently we are not actually simulating an HTTP request to the Kibana server (not sure thats supported in the UT framework - ex. does kibana core provide a mock "server"?), but rather we test against the HTTP handler.

Also - as I learned from Frank, its best to ensure we test our schemas separately so that if we do end up changing schema libraries (ex. move to io-ts), we can ensure that our expectation remain met with the new lib.

One thing I did like about the everything-defined-here pattern was that the schema was close to the URL route. This adds two layers of separation. Need to look in constants to see what the route is, and handlers to see what the params are.

…ted-apps-list-api

paul-tavares · 2020-08-26T16:21:14Z

jenkins test this

paul-tavares · 2020-08-26T17:28:23Z

jenkins test this

…ted-apps-list-api

kibanamachine · 2020-08-26T19:15:47Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 1394a76

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
securitySolution	1923	+1	1922

async chunks size

id	value	diff	baseline
securitySolution	9.5MB	+986.0B	9.5MB

page load bundle size

id	value	diff	baseline
securitySolution	807.8KB	+1.0B	807.8KB

distributable file count

id	value	diff	baseline
total	53204	+7	53197

History

💔 Build #70541 failed b97524b
💔 Build #70569 failed b97524b
💚 Build #70104 succeeded c717647
💔 Build #70066 failed ae8d876
💚 Build #69533 succeeded 0b97949

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

* Trusted Apps initial setup for route registration * Added types for TrustedApp entries * trusted apps list API returns results * use methods and const from latest PR merge to lists * a quick generator for trusted apps entries * support cli options for trusted app data loader * Add mocked `createTrustedAppsList()` method to `ExceptionListClientMock` * tests fro trusted apps route handlers * tests for trusted apps schema * Correct name of mock method * Fix service to ensure return value of `getExceptionList` service throws if service not available * Fix types * Refactor TrustedApp type + code review feedback

* Trusted Apps API for retrieving list * Added types for TrustedApp entries * generator for trusted apps entries * Add mocked `createTrustedAppsList()` method to `ExceptionListClientMock`

paul-tavares added 3 commits August 19, 2020 09:19

Trusted Apps initial setup for route registration

6defd9c

Added types for TrustedApp entries

6f2ce18

trusted apps list API returns results

a835bca

paul-tavares added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:Endpoint Management Feature:Endpoint Elastic Endpoint feature v7.10.0 labels Aug 19, 2020

paul-tavares self-assigned this Aug 19, 2020

paul-tavares added 2 commits August 19, 2020 14:34

Merge remote-tracking branch 'upstream/master' into task/EMT-685-trus…

83739bc

…ted-apps-list-api

use methods and const from latest PR merge to lists

0f4baba

efreeti reviewed Aug 20, 2020

View reviewed changes

a quick generator for trusted apps entries

d0b6ea4

paul-tavares commented Aug 20, 2020

View reviewed changes

x-pack/plugins/security_solution/common/endpoint/types/trusted_apps.ts Outdated Show resolved Hide resolved

paul-tavares added 7 commits August 20, 2020 16:33

support cli options for trusted app data loader

0b97949

Merge remote-tracking branch 'upstream/master' into task/EMT-685-trus…

f7939de

…ted-apps-list-api

Add mocked createTrustedAppsList() method to ExceptionListClientMock

6c94624

tests fro trusted apps route handlers

fafa9d6

tests for trusted apps schema

c66077b

Correct name of mock method

697a57b

Fix service to ensure return value of getExceptionList service thro…

ae8d876

…ws if service not available

paul-tavares commented Aug 24, 2020

View reviewed changes

Fix types

c717647

paul-tavares marked this pull request as ready for review August 24, 2020 18:32

paul-tavares requested review from a team as code owners August 24, 2020 18:32

paul-tavares commented Aug 24, 2020

View reviewed changes

kevinlog reviewed Aug 24, 2020

View reviewed changes

kevinlog approved these changes Aug 24, 2020

View reviewed changes

efreeti reviewed Aug 25, 2020

View reviewed changes

paul-tavares added 2 commits August 26, 2020 10:02

Merge remote-tracking branch 'upstream/master' into task/EMT-685-trus…

6a7fbd6

…ted-apps-list-api

Refactor TrustedApp type + code review feedback

b97524b

Merge remote-tracking branch 'upstream/master' into task/EMT-685-trus…

1394a76

…ted-apps-list-api

paul-tavares merged commit 9873df8 into elastic:master Aug 26, 2020

paul-tavares deleted the task/EMT-685-trusted-apps-list-api branch August 26, 2020 20:02

paul-tavares mentioned this pull request Aug 26, 2020

[7.x] [SECURITY_SOLUTION][ENDPOINT] Trusted Apps List API (#75476) #76029

Merged

[SECURITY_SOLUTION][ENDPOINT] Trusted Apps List API #75476

[SECURITY_SOLUTION][ENDPOINT] Trusted Apps List API #75476

Conversation

paul-tavares commented Aug 19, 2020 • edited Loading

Summary

Request:

Response:

Checklist

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

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

efreeti commented Aug 20, 2020 • edited Loading

paul-tavares commented Aug 20, 2020

Choose a reason for hiding this comment

elasticmachine commented Aug 24, 2020

elasticmachine commented Aug 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinlog 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

paul-tavares commented Aug 26, 2020

paul-tavares commented Aug 26, 2020

kibanamachine commented Aug 26, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

async chunks size

page load bundle size

distributable file count

History

paul-tavares commented Aug 19, 2020 •

edited

Loading

efreeti commented Aug 20, 2020 •

edited

Loading