[SIEM][Detection Engine] Removes technical debt and minor bug fixes (#…

…50111)

## Summary

* Removes technical debt of name being in the params now that the regular alerting has name as a first class citizen.
* Only creates new signals on an update if it sees that saved objects return 404
* Changes the conversion script of saved searches to rules to be every 5 minutes for tests.
* Changes the logger levels to be mostly quiet by using debug instead of info.
* Small fixes for when we return false for errors on 0 found signals when that is not an error. 
* Added a delete all api keys for more cleanups when developing 

For testing things on the backend this is the `kibana.dev.yml` settings I use for this PR which enables the siem debug but filters out the others and enables additional request information for testing:

```yml
logging.verbose: true
logging.events:
  {
    log: ['siem', 'info', 'warning', 'error', 'fatal'],
    request: ['info', 'warning', 'error', 'fatal'],
    error: '*',
    ops: __no-ops__,
  }
```

### Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~- [ ] This was checked for cross-browser compatibility, [including a check against IE11](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility)~~

~~- [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md)~~

~~- [ ] [Documentation](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#writing-documentation) was added for features that require explanation or tutorials~~

- [x] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios

~~- [ ] This was checked for [keyboard-only and screenreader accessibility](https://developer.mozilla.org/en-US/docs/Learn/Tools_and_testing/Cross_browser_testing/Accessibility#Accessibility_testing_checklist)~~

### For maintainers

~~- [ ] This was checked for breaking API changes and was [labeled appropriately](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#release-notes-process)~~

~~- [ ] This includes a feature addition or change that requires a release note and was [labeled appropriately](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#release-notes-process)~~

x-pack/legacy/plugins/siem/scripts/convert_saved_search_to_signals.js

@@ -28,10 +28,10 @@ const path = require('path');
 // this type of information. You usually will want to make any hand edits after
 // doing a search to KQL conversion before posting it as a signal or checking it
 // into another repository.
-const INTERVAL = '24h';
+const INTERVAL = '5m';
 const SEVERITY = 'low';
 const TYPE = 'query';
-const FROM = 'now-24h';
+const FROM = 'now-6m';
 const TO = 'now';
 const INDEX = ['auditbeat-*', 'filebeat-*', 'packetbeat-*', 'winlogbeat-*'];
 

x-pack/legacy/plugins/siem/server/lib/detection_engine/alerts/create_signals.ts

@@ -57,10 +57,12 @@ export const updateIfIdExists = async ({
     });
     return signal;
   } catch (err) {
-    // This happens when we cannot get a saved object back from reading a signal.
-    // So we continue normally as we have nothing we can upsert.
+    if (err.output.statusCode === 404) {
+      return null;
+    } else {
+      return err;
+    }
   }
-  return null;
 };
 
 export const createSignals = async ({
@@ -124,7 +126,7 @@ export const createSignals = async ({
 
     return alertsClient.create({
       data: {
-        name: 'SIEM Alert',
+        name,
         alertTypeId: SIGNALS_ID,
         alertTypeParams: {
           description,
@@ -137,7 +139,6 @@ export const createSignals = async ({
           savedId,
           filters,
           maxSignals,
-          name,
           severity,
           to,
           type,

x-pack/legacy/plugins/siem/server/lib/detection_engine/alerts/find_signals.ts

@@ -7,16 +7,12 @@
 import { SIGNALS_ID } from '../../../../common/constants';
 import { FindSignalParams } from './types';
 
-// TODO: Change this from a search to a filter once this ticket is solved:
-// https://github.com/elastic/kibana/projects/26#card-27462236
-export const findSignals = async ({ alertsClient, perPage, page, fields }: FindSignalParams) => {
-  return alertsClient.find({
+export const findSignals = async ({ alertsClient, perPage, page, fields }: FindSignalParams) =>
+  alertsClient.find({
     options: {
       fields,
       page,
       perPage,
-      searchFields: ['alertTypeId'],
-      search: SIGNALS_ID,
+      filter: `alert.attributes.alertTypeId: ${SIGNALS_ID}`,
     },
   });
-};

x-pack/legacy/plugins/siem/server/lib/detection_engine/alerts/signals_alert_type.ts

@@ -11,11 +11,7 @@ import { Logger } from '../../../../../../../../src/core/server';
 // TODO: Remove this for the build_events_query call eventually
 import { buildEventsReIndex } from './build_events_reindex';
 
-// TODO: Comment this in and use this instead of the reIndex API
-// once scrolling and other things are done with it.
 import { buildEventsSearchQuery } from './build_events_query';
-
-// bulk scroll class
 import { searchAfterAndBulkIndex } from './utils';
 import { SignalAlertTypeDefinition } from './types';
 import { getFilter } from './get_filter';
@@ -37,7 +33,6 @@ export const signalsAlertType = ({ logger }: { logger: Logger }): SignalAlertTyp
         query: schema.nullable(schema.string()),
         filters: schema.nullable(schema.arrayOf(schema.object({}, { allowUnknowns: true }))),
         maxSignals: schema.number({ defaultValue: 100 }),
-        name: schema.string(),
         severity: schema.string(),
         to: schema.string(),
         type: schema.string(),
@@ -46,8 +41,6 @@ export const signalsAlertType = ({ logger }: { logger: Logger }): SignalAlertTyp
       }),
     },
     async executor({ services, params }) {
-      const instance = services.alertInstanceFactory('siem-signals');
-
       const {
         description,
         filter,
@@ -80,7 +73,6 @@ export const signalsAlertType = ({ logger }: { logger: Logger }): SignalAlertTyp
         index,
       });
 
-      // TODO: Turn these options being sent in into a template for the alert type
       const noReIndex = buildEventsSearchQuery({
         index,
         from,
@@ -90,11 +82,7 @@ export const signalsAlertType = ({ logger }: { logger: Logger }): SignalAlertTyp
       });
 
       try {
-        logger.info('Starting SIEM signal job');
-
-        // TODO: Comment this in eventually and use this for manual insertion of the
-        // signals instead of the ReIndex() api
-
+        logger.debug(`Starting signal rule "${id}"`);
         if (process.env.USE_REINDEX_API === 'true') {
           const reIndex = buildEventsReIndex({
             index,
@@ -115,14 +103,19 @@ export const signalsAlertType = ({ logger }: { logger: Logger }): SignalAlertTyp
             references,
           });
           const result = await services.callCluster('reindex', reIndex);
-
-          // TODO: Error handling here and writing of any errors that come back from ES by
-          logger.info(`Result of reindex: ${JSON.stringify(result, null, 2)}`);
+          if (result.total > 0) {
+            logger.info(
+              `Total signals found from signal rule "${id}" (reindex algorithm): ${result.total}`
+            );
+          }
         } else {
-          logger.info(`[+] Initial search call`);
-
+          logger.debug(`[+] Initial search call of signal rule "${id}"`);
           const noReIndexResult = await services.callCluster('search', noReIndex);
-          logger.info(`Total docs to reindex: ${noReIndexResult.hits.total.value}`);
+          if (noReIndexResult.hits.total.value !== 0) {
+            logger.info(
+              `Total signals found from signal rule "${id}": ${noReIndexResult.hits.total.value}`
+            );
+          }
 
           const bulkIndexResult = await searchAfterAndBulkIndex(
             noReIndexResult,
@@ -132,19 +125,23 @@ export const signalsAlertType = ({ logger }: { logger: Logger }): SignalAlertTyp
           );
 
           if (bulkIndexResult) {
-            logger.info('Finished SIEM signal job');
+            logger.debug(`Finished signal rule "${id}"`);
           } else {
-            logger.error('Error processing SIEM signal job');
+            logger.error(`Error processing signal rule "${id}"`);
           }
         }
       } catch (err) {
         // TODO: Error handling and writing of errors into a signal that has error
         // handling/conditions
-        logger.error(`You encountered an error of: ${err.message}`);
+        logger.error(`Error from signal rule "${id}", ${err.message}`);
       }
 
+      // TODO: Schedule and fire any and all actions configured for the signals rule
+      // such as email/slack/etc... Note you will not be able to save in-memory state
+      // without calling this at least once but we are not using in-memory state at the moment.
       // Schedule the default action which is nothing if it's a plain signal.
-      instance.scheduleActions('default');
+      // const instance = services.alertInstanceFactory('siem-signals');
+      // instance.scheduleActions('default');
     },
   };
 };

x-pack/legacy/plugins/siem/server/lib/detection_engine/alerts/types.ts

@@ -47,6 +47,10 @@ export type SignalAlertParamsRest = Omit<SignalAlertParams, 'maxSignals' | 'save
   max_signals: SignalAlertParams['maxSignals'];
 };
 
+export type UpdateSignalAlertParamsRest = Partial<Omit<SignalAlertParamsRest, 'id'>> & {
+  id: SignalAlertParams['id'];
+};
+
 export interface FindParamsRest {
   per_page: number;
   page: number;
@@ -61,6 +65,10 @@ export interface Clients {
 
 export type SignalParams = SignalAlertParams & Clients;
 
+export type UpdateSignalParams = Partial<Omit<SignalAlertParams, 'id'>> & {
+  id: SignalAlertParams['id'];
+} & Clients;
+
 export type DeleteSignalParams = Clients & { id: string };
 
 export interface FindSignalsRequest extends Omit<Hapi.Request, 'query'> {
@@ -95,6 +103,10 @@ export interface SignalsRequest extends Hapi.Request {
   payload: SignalAlertParamsRest;
 }
 
+export interface UpdateSignalsRequest extends Hapi.Request {
+  payload: UpdateSignalAlertParamsRest;
+}
+
 export type SignalExecutorOptions = Omit<AlertExecutorOptions, 'params'> & {
   params: SignalAlertParams & {
     scrollSize: number;

x-pack/legacy/plugins/siem/server/lib/detection_engine/alerts/update_signals.test.ts

@@ -4,7 +4,7 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
-import { calculateInterval } from './update_signals';
+import { calculateInterval, calculateName } from './update_signals';
 
 describe('update_signals', () => {
   describe('#calculateInterval', () => {
@@ -23,4 +23,26 @@ describe('update_signals', () => {
       expect(interval).toEqual('5m');
     });
   });
+
+  describe('#calculateName', () => {
+    test('should return the updated name when it and originalName is there', () => {
+      const name = calculateName({ updatedName: 'updated', originalName: 'original' });
+      expect(name).toEqual('updated');
+    });
+
+    test('should return the updated name when originalName is undefined', () => {
+      const name = calculateName({ updatedName: 'updated', originalName: undefined });
+      expect(name).toEqual('updated');
+    });
+
+    test('should return the original name when updatedName is undefined', () => {
+      const name = calculateName({ updatedName: undefined, originalName: 'original' });
+      expect(name).toEqual('original');
+    });
+
+    test('should return untitled when both updatedName and originalName is undefined', () => {
+      const name = calculateName({ updatedName: undefined, originalName: undefined });
+      expect(name).toEqual('untitled');
+    });
+  });
 });

x-pack/legacy/plugins/siem/server/lib/detection_engine/alerts/update_signals.ts

@@ -7,7 +7,7 @@
 import { defaults } from 'lodash/fp';
 import { AlertAction } from '../../../../../alerting/server/types';
 import { readSignals } from './read_signals';
-import { SignalParams } from './types';
+import { UpdateSignalParams } from './types';
 
 export const calculateInterval = (
   interval: string | undefined,
@@ -22,6 +22,25 @@ export const calculateInterval = (
   }
 };
 
+export const calculateName = ({
+  updatedName,
+  originalName,
+}: {
+  updatedName: string | undefined;
+  originalName: string | undefined;
+}): string => {
+  if (updatedName != null) {
+    return updatedName;
+  } else if (originalName != null) {
+    return originalName;
+  } else {
+    // You really should never get to this point. This is a fail safe way to send back
+    // the name of "untitled" just in case a signal rule name became null or undefined at
+    // some point since TypeScript allows it.
+    return 'untitled';
+  }
+};
+
 export const updateSignal = async ({
   alertsClient,
   actionsClient, // TODO: Use this whenever we add feature support for different action types
@@ -42,7 +61,7 @@ export const updateSignal = async ({
   to,
   type,
   references,
-}: SignalParams) => {
+}: UpdateSignalParams) => {
   // TODO: Error handling and abstraction. Right now if this is an error then what happens is we get the error of
   // "message": "Saved object [alert/{id}] not found"
   const signal = await readSignals({ alertsClient, id });
@@ -67,7 +86,6 @@ export const updateSignal = async ({
       filters,
       index,
       maxSignals,
-      name,
       severity,
       to,
       type,
@@ -84,7 +102,7 @@ export const updateSignal = async ({
   return alertsClient.update({
     id: signal.id,
     data: {
-      name: 'SIEM Alert',
+      name: calculateName({ updatedName: name, originalName: signal.name }),
       interval: calculateInterval(interval, signal.interval),
       actions,
       alertTypeParams: nextAlertTypeParams,

x-pack/legacy/plugins/siem/server/lib/detection_engine/alerts/utils.ts

@@ -43,8 +43,7 @@ export const singleBulkIndex = async (
   logger: Logger
 ): Promise<boolean> => {
   if (sr.hits.hits.length === 0) {
-    logger.warn('First search result yielded 0 documents');
-    return false;
+    return true;
   }
   const bulkBody = sr.hits.hits.flatMap(doc => [
     {
@@ -62,8 +61,8 @@ export const singleBulkIndex = async (
     body: bulkBody,
   });
   const time2 = performance.now();
-  logger.info(`individual bulk process time took: ${time2 - time1} milliseconds`);
-  logger.info(`took property says bulk took: ${firstResult.took} milliseconds`);
+  logger.debug(`individual bulk process time took: ${time2 - time1} milliseconds`);
+  logger.debug(`took property says bulk took: ${firstResult.took} milliseconds`);
   if (firstResult.errors) {
     logger.error(`[-] bulkResponse had errors: ${JSON.stringify(firstResult.errors, null, 2)}}`);
     return false;
@@ -105,20 +104,24 @@ export const searchAfterAndBulkIndex = async (
   service: AlertServices,
   logger: Logger
 ): Promise<boolean> => {
-  logger.info('[+] starting bulk insertion');
+  if (someResult.hits.hits.length === 0) {
+    return true;
+  }
+
+  logger.debug('[+] starting bulk insertion');
   const firstBulkIndexSuccess = await singleBulkIndex(someResult, params, service, logger);
   if (!firstBulkIndexSuccess) {
-    logger.warn('First bulk index was unsuccessful');
+    logger.error('First bulk index was unsuccessful');
     return false;
   }
 
   const totalHits =
     typeof someResult.hits.total === 'number' ? someResult.hits.total : someResult.hits.total.value;
   let size = someResult.hits.hits.length - 1;
-  logger.info(`first size: ${size}`);
+  logger.debug(`first size: ${size}`);
   let sortIds = someResult.hits.hits[0].sort;
   if (sortIds == null && totalHits > 0) {
-    logger.warn('sortIds was empty on first search but expected more ');
+    logger.error(`sortIds was empty on first search when encountering ${totalHits}`);
     return false;
   } else if (sortIds == null && totalHits === 0) {
     return true;
@@ -130,32 +133,32 @@ export const searchAfterAndBulkIndex = async (
   while (size < totalHits) {
     // utilize track_total_hits instead of true
     try {
-      logger.info(`sortIds: ${sortIds}`);
+      logger.debug(`sortIds: ${sortIds}`);
       const searchAfterResult: SignalSearchResponse = await singleSearchAfter(
         sortId,
         params,
         service,
         logger
       );
       size += searchAfterResult.hits.hits.length - 1;
-      logger.info(`size: ${size}`);
+      logger.debug(`size adjusted: ${size}`);
       sortIds = searchAfterResult.hits.hits[0].sort;
       if (sortIds == null) {
-        logger.warn('sortIds was empty search');
+        logger.error('sortIds was empty search when running a signal rule');
         return false;
       }
       sortId = sortIds[0];
-      logger.info('next bulk index');
+      logger.debug('next bulk index');
       const bulkSuccess = await singleBulkIndex(searchAfterResult, params, service, logger);
-      logger.info('finished next bulk index');
+      logger.debug('finished next bulk index');
       if (!bulkSuccess) {
-        logger.error('[-] bulk index failed');
+        logger.error('[-] bulk index failed but continuing');
       }
     } catch (exc) {
       logger.error(`[-] search_after and bulk threw an error ${exc}`);
       return false;
     }
   }
-  logger.info(`[+] completed bulk index of ${totalHits}`);
+  logger.debug(`[+] completed bulk index of ${totalHits}`);
   return true;
 };