From e7b68e33f2f539b6c74e7b218f1431805871cedf Mon Sep 17 00:00:00 2001 From: Frank Hassanabad Date: Mon, 11 Nov 2019 13:02:09 -0500 Subject: [PATCH] [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)~~ --- .../convert_saved_search_to_signals.js | 4 +- .../detection_engine/alerts/create_signals.ts | 11 ++--- .../detection_engine/alerts/find_signals.ts | 10 ++--- .../alerts/signals_alert_type.ts | 43 +++++++++---------- .../lib/detection_engine/alerts/types.ts | 12 ++++++ .../alerts/update_signals.test.ts | 24 ++++++++++- .../detection_engine/alerts/update_signals.ts | 26 +++++++++-- .../lib/detection_engine/alerts/utils.ts | 33 +++++++------- .../routes/update_signals_route.ts | 7 ++- .../scripts/delete_all_api_keys.sh | 21 +++++++++ .../detection_engine/scripts/hard_reset.sh | 1 + .../signals/root_or_admin_update_1.json | 14 +++--- 12 files changed, 138 insertions(+), 68 deletions(-) create mode 100755 x-pack/legacy/plugins/siem/server/lib/detection_engine/scripts/delete_all_api_keys.sh diff --git a/x-pack/legacy/plugins/siem/scripts/convert_saved_search_to_signals.js b/x-pack/legacy/plugins/siem/scripts/convert_saved_search_to_signals.js index 5692ce27218701..9b2a38e372ae3c 100644 --- a/x-pack/legacy/plugins/siem/scripts/convert_saved_search_to_signals.js +++ b/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-*']; diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/alerts/create_signals.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/alerts/create_signals.ts index 2caf00ed0179cf..038effbd30086d 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/alerts/create_signals.ts +++ b/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, diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/alerts/find_signals.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/alerts/find_signals.ts index ebddf6ac5b3c74..23f4e38a95eeab 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/alerts/find_signals.ts +++ b/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}`, }, }); -}; diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/alerts/signals_alert_type.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/alerts/signals_alert_type.ts index fc18c1b552198e..6673b7bd1423b6 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/alerts/signals_alert_type.ts +++ b/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'); }, }; }; diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/alerts/types.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/alerts/types.ts index b8d7af5c453033..96a319c944bd90 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/alerts/types.ts +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/alerts/types.ts @@ -47,6 +47,10 @@ export type SignalAlertParamsRest = Omit> & { + 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> & { + id: SignalAlertParams['id']; +} & Clients; + export type DeleteSignalParams = Clients & { id: string }; export interface FindSignalsRequest extends Omit { @@ -95,6 +103,10 @@ export interface SignalsRequest extends Hapi.Request { payload: SignalAlertParamsRest; } +export interface UpdateSignalsRequest extends Hapi.Request { + payload: UpdateSignalAlertParamsRest; +} + export type SignalExecutorOptions = Omit & { params: SignalAlertParams & { scrollSize: number; diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/alerts/update_signals.test.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/alerts/update_signals.test.ts index 46ebfe98ce3d98..39f7951a8eab98 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/alerts/update_signals.test.ts +++ b/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'); + }); + }); }); diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/alerts/update_signals.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/alerts/update_signals.ts index 9cb30a29173d70..422d9123a286bb 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/alerts/update_signals.ts +++ b/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, diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/alerts/utils.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/alerts/utils.ts index 08e99de0f2581d..a514baa186fd26 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/alerts/utils.ts +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/alerts/utils.ts @@ -43,8 +43,7 @@ export const singleBulkIndex = async ( logger: Logger ): Promise => { 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 => { - 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,7 +133,7 @@ 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, @@ -138,24 +141,24 @@ export const searchAfterAndBulkIndex = async ( 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; }; diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/update_signals_route.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/update_signals_route.ts index 08307ef633ffe2..8248d1505143a5 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/update_signals_route.ts +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/update_signals_route.ts @@ -8,7 +8,7 @@ import Hapi from 'hapi'; import Joi from 'joi'; import { isFunction } from 'lodash/fp'; import { updateSignal } from '../alerts/update_signals'; -import { SignalsRequest } from '../alerts/types'; +import { UpdateSignalsRequest } from '../alerts/types'; import { updateSignalSchema } from './schemas'; export const createUpdateSignalsRoute: Hapi.ServerRoute = { @@ -30,7 +30,7 @@ export const createUpdateSignalsRoute: Hapi.ServerRoute = { payload: updateSignalSchema, }, }, - async handler(request: SignalsRequest, headers) { + async handler(request: UpdateSignalsRequest, headers) { const { description, enabled, @@ -53,14 +53,13 @@ export const createUpdateSignalsRoute: Hapi.ServerRoute = { type, references, } = request.payload; - const alertsClient = isFunction(request.getAlertsClient) ? request.getAlertsClient() : null; + const alertsClient = isFunction(request.getAlertsClient) ? request.getAlertsClient() : null; const actionsClient = isFunction(request.getActionsClient) ? request.getActionsClient() : null; if (!alertsClient || !actionsClient) { return headers.response().code(404); } - return updateSignal({ alertsClient, actionsClient, diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/scripts/delete_all_api_keys.sh b/x-pack/legacy/plugins/siem/server/lib/detection_engine/scripts/delete_all_api_keys.sh new file mode 100755 index 00000000000000..efcee69a0152c6 --- /dev/null +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/scripts/delete_all_api_keys.sh @@ -0,0 +1,21 @@ +#!/bin/sh + +# +# Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one +# or more contributor license agreements. Licensed under the Elastic License; +# you may not use this file except in compliance with the Elastic License. +# + +set -e +./check_env_variables.sh + +# Example: ./delete_all_api_keys.sh +# https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-invalidate-api-key.html +curl -s -k \ + -H "Content-Type: application/json" \ + -u ${ELASTICSEARCH_USERNAME}:${ELASTICSEARCH_PASSWORD} \ + -X DELETE ${ELASTICSEARCH_URL}/_security/api_key \ + --data "{ + \"username\": \"${ELASTICSEARCH_USERNAME}\" + }" \ + | jq . diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/scripts/hard_reset.sh b/x-pack/legacy/plugins/siem/server/lib/detection_engine/scripts/hard_reset.sh index ee8fa18e1234d3..9c37e93831ccbe 100755 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/scripts/hard_reset.sh +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/scripts/hard_reset.sh @@ -14,3 +14,4 @@ set -e ./delete_all_alert_tasks.sh ./delete_signal_index.sh ./put_signal_index.sh +./delete_all_api_keys.sh diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/scripts/signals/root_or_admin_update_1.json b/x-pack/legacy/plugins/siem/server/lib/detection_engine/scripts/signals/root_or_admin_update_1.json index 71d79903a01db1..589583d417a133 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/scripts/signals/root_or_admin_update_1.json +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/scripts/signals/root_or_admin_update_1.json @@ -1,14 +1,14 @@ { "id": "rule-1", - "description": "Detecting root and admin users", - "index": ["auditbeat-*", "filebeat-*", "packetbeat-*", "winlogbeat-*"], - "interval": "5m", - "name": "Detect Root/Admin Users", + "description": "Changed Description of only detecting root user", + "index": ["auditbeat-*"], + "interval": "50m", + "name": "A different name", "severity": "high", "type": "query", "from": "now-6m", - "to": "now", - "query": "user.name: root or user.name: admin", + "to": "now-5m", + "query": "user.name: root", "language": "kuery", - "references": [] + "references": ["https://update1.example.com", "https://update2.example.com"] }