Skip to content
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

[Security Solution][Detections] Reduce detection engine reliance on _source #89371

Merged
merged 9 commits into from
Feb 4, 2021
Merged

[Security Solution][Detections] Reduce detection engine reliance on _source #89371

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
1a0a6ed
First pass at switching rules to depend on fields instead of _source
marshallmain Jan 26, 2021
084ba5e
Merge branch 'master' into drop-source
marshallmain Jan 26, 2021
2ff44e1
Fix tests
marshallmain Jan 27, 2021
8d8b62a
Change operator: excluded logic so missing fields are allowlisted
marshallmain Jan 27, 2021
286e8d5
Merge branch 'master' into drop-source
kibanamachine Jan 28, 2021
47297b6
Merge branch 'master' into drop-source
kibanamachine Jan 29, 2021
6420cff
Merge branch 'master' into drop-source
kibanamachine Feb 1, 2021
9b9d1b6
Merge branch 'master' into drop-source
kibanamachine Feb 2, 2021
9479bfa
Merge branch 'master' into drop-source
kibanamachine Feb 4, 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
11 changes: 11 additions & 0 deletions x-pack/plugins/security_solution/server/lib/detection_engine/signals/__mocks__/es_results.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,12 @@ export const sampleDocWithSortId = (
ip: destIp ?? '127.0.0.1',
},
},
fields: {
someKey: ['someValue'],
'@timestamp': ['2020-04-20T21:27:45+0000'],
'source.ip': ip ? (Array.isArray(ip) ? ip : [ip]) : ['127.0.0.1'],
'destination.ip': destIp ? (Array.isArray(destIp) ? destIp : [destIp]) : ['127.0.0.1'],
},
sort: ['1234567891111'],
});

Expand All @@ -185,6 +191,11 @@ export const sampleDocNoSortId = (
ip: ip ?? '127.0.0.1',
},
},
fields: {
someKey: ['someValue'],
'@timestamp': ['2020-04-20T21:27:45+0000'],
'source.ip': [ip ?? '127.0.0.1'],
},
sort: [],
});

Expand Down
41 changes: 36 additions & 5 deletions .../plugins/security_solution/server/lib/detection_engine/signals/build_events_query.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,12 @@ describe('create_signals', () => {
],
},
},

fields: [
{
field: '*',
include_unmapped: true,
},
],
sort: [
{
'@timestamp': {
Expand Down Expand Up @@ -115,7 +120,12 @@ describe('create_signals', () => {
],
},
},

fields: [
{
field: '*',
include_unmapped: true,
},
],
sort: [
{
'@timestamp': {
Expand Down Expand Up @@ -175,7 +185,12 @@ describe('create_signals', () => {
],
},
},

fields: [
{
field: '*',
include_unmapped: true,
},
],
sort: [
{
'@timestamp': {
Expand Down Expand Up @@ -236,7 +251,12 @@ describe('create_signals', () => {
],
},
},

fields: [
{
field: '*',
include_unmapped: true,
},
],
sort: [
{
'@timestamp': {
Expand Down Expand Up @@ -296,7 +316,12 @@ describe('create_signals', () => {
],
},
},

fields: [
{
field: '*',
include_unmapped: true,
},
],
sort: [
{
'@timestamp': {
Expand Down Expand Up @@ -358,6 +383,12 @@ describe('create_signals', () => {
],
},
},
fields: [
{
field: '*',
include_unmapped: true,
},
],
aggregations: {
tags: {
terms: {
Expand Down
6 changes: 6 additions & 0 deletions x-pack/plugins/security_solution/server/lib/detection_engine/signals/build_events_query.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,12 @@ export const buildEventsSearchQuery = ({
],
},
},
fields: [
{
field: '*',
include_unmapped: true,
},
],
...(aggregations ? { aggregations } : {}),
sort: [
{
Expand Down
2 changes: 1 addition & 1 deletion ...curity_solution/server/lib/detection_engine/signals/bulk_create_threshold_signals.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ describe('transformThresholdResultsToEcs', () => {
_id,
_index: 'test',
_source: {
'@timestamp': '2020-04-20T21:27:45+0000',
'@timestamp': ['2020-04-20T21:27:45+0000'],
threshold_result: {
count: 1,
value: '127.0.0.1',
Expand Down
6 changes: 3 additions & 3 deletions ...ns/security_solution/server/lib/detection_engine/signals/bulk_create_threshold_signals.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ const getTransformedHits = (
}

const source = {
'@timestamp': get(timestampOverride ?? '@timestamp', hit._source),
'@timestamp': get(timestampOverride ?? '@timestamp', hit.fields),
threshold_result: {
count: totalResults,
value: ruleId,
Expand Down Expand Up @@ -104,10 +104,10 @@ const getTransformedHits = (
}

const source = {
'@timestamp': get(timestampOverride ?? '@timestamp', hit._source),
'@timestamp': get(timestampOverride ?? '@timestamp', hit.fields),
threshold_result: {
count: docCount,
value: get(threshold.field, hit._source),
value: key,
},
};

Expand Down
8 changes: 4 additions & 4 deletions ..._solution/server/lib/detection_engine/signals/filters/create_field_and_set_tuples.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ describe('filterEventsAgainstList', () => {
exceptionItem,
buildRuleMessage,
});
expect([...matchedSet]).toEqual([JSON.stringify('1.1.1.1')]);
expect([...matchedSet]).toEqual([JSON.stringify(['1.1.1.1'])]);
});

test('it returns two matched sets as a JSON.stringify() set from the "events"', async () => {
Expand All @@ -133,7 +133,7 @@ describe('filterEventsAgainstList', () => {
exceptionItem,
buildRuleMessage,
});
expect([...matchedSet]).toEqual([JSON.stringify('1.1.1.1'), JSON.stringify('2.2.2.2')]);
expect([...matchedSet]).toEqual([JSON.stringify(['1.1.1.1']), JSON.stringify(['2.2.2.2'])]);
});

test('it returns an array as a set as a JSON.stringify() array from the "events"', async () => {
Expand Down Expand Up @@ -282,7 +282,7 @@ describe('filterEventsAgainstList', () => {
exceptionItem,
buildRuleMessage,
});
expect([...matchedSet1]).toEqual([JSON.stringify('1.1.1.1'), JSON.stringify('2.2.2.2')]);
expect([...matchedSet2]).toEqual([JSON.stringify('3.3.3.3'), JSON.stringify('5.5.5.5')]);
expect([...matchedSet1]).toEqual([JSON.stringify(['1.1.1.1']), JSON.stringify(['2.2.2.2'])]);
expect([...matchedSet2]).toEqual([JSON.stringify(['3.3.3.3']), JSON.stringify(['5.5.5.5'])]);
});
});
8 changes: 4 additions & 4 deletions ...solution/server/lib/detection_engine/signals/filters/create_set_to_filter_against.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,9 @@ describe('createSetToFilterAgainst', () => {
expect(listClient.searchListItemByValues).toHaveBeenCalledWith({
listId: 'list-123',
type: 'ip',
value: ['1.1.1.1'],
value: [['1.1.1.1']],
});
expect([...field]).toEqual([JSON.stringify('1.1.1.1')]);
expect([...field]).toEqual([JSON.stringify(['1.1.1.1'])]);
});

test('it returns 2 fields if the list returns 2 items', async () => {
Expand All @@ -81,9 +81,9 @@ describe('createSetToFilterAgainst', () => {
expect(listClient.searchListItemByValues).toHaveBeenCalledWith({
listId: 'list-123',
type: 'ip',
value: ['1.1.1.1', '2.2.2.2'],
value: [['1.1.1.1'], ['2.2.2.2']],
});
expect([...field]).toEqual([JSON.stringify('1.1.1.1'), JSON.stringify('2.2.2.2')]);
expect([...field]).toEqual([JSON.stringify(['1.1.1.1']), JSON.stringify(['2.2.2.2'])]);
});

test('it returns 0 fields if the field does not match up to a valid field within the event', async () => {
Expand Down
3 changes: 1 addition & 2 deletions ...rity_solution/server/lib/detection_engine/signals/filters/create_set_to_filter_against.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
* 2.0.
*/

import { get } from 'lodash/fp';
import { CreateSetToFilterAgainstOptions } from './types';

/**
Expand All @@ -31,7 +30,7 @@ export const createSetToFilterAgainst = async <T>({
buildRuleMessage,
}: CreateSetToFilterAgainstOptions<T>): Promise<Set<unknown>> => {
const valuesFromSearchResultField = events.reduce((acc, searchResultItem) => {
const valueField = get(field, searchResultItem._source);
const valueField = searchResultItem.fields ? searchResultItem.fields[field] : undefined;
if (valueField != null) {
acc.add(valueField);
}
Expand Down
10 changes: 5 additions & 5 deletions ...ugins/security_solution/server/lib/detection_engine/signals/filters/filter_events.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ describe('filterEvents', () => {
{
field: 'source.ip',
operator: 'included',
matchedSet: new Set([JSON.stringify('1.1.1.1')]),
matchedSet: new Set([JSON.stringify(['1.1.1.1'])]),
},
];
const field = filterEvents({
Expand All @@ -56,7 +56,7 @@ describe('filterEvents', () => {
{
field: 'source.ip',
operator: 'excluded',
matchedSet: new Set([JSON.stringify('1.1.1.1')]),
matchedSet: new Set([JSON.stringify(['1.1.1.1'])]),
},
];
const field = filterEvents({
Expand All @@ -72,7 +72,7 @@ describe('filterEvents', () => {
{
field: 'madeup.nonexistent', // field does not exist
operator: 'included',
matchedSet: new Set([JSON.stringify('1.1.1.1')]),
matchedSet: new Set([JSON.stringify(['1.1.1.1'])]),
},
];
const field = filterEvents({
Expand All @@ -88,12 +88,12 @@ describe('filterEvents', () => {
{
field: 'source.ip',
operator: 'included',
matchedSet: new Set([JSON.stringify('1.1.1.1')]),
matchedSet: new Set([JSON.stringify(['1.1.1.1'])]),
},
{
field: 'source.ip',
operator: 'excluded',
matchedSet: new Set([JSON.stringify('1.1.1.1')]),
matchedSet: new Set([JSON.stringify(['1.1.1.1'])]),
},
];

Expand Down
13 changes: 8 additions & 5 deletions ...ck/plugins/security_solution/server/lib/detection_engine/signals/filters/filter_events.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
* 2.0.
*/

import { get } from 'lodash/fp';
import { SearchResponse } from '../../../types';
import { FilterEventsOptions } from './types';

Expand All @@ -22,13 +21,17 @@ export const filterEvents = <T>({
return events.filter((item) => {
return fieldAndSetTuples
.map((tuple) => {
const eventItem = get(tuple.field, item._source);
if (eventItem == null) {
return true;
} else if (tuple.operator === 'included') {
const eventItem = item.fields ? item.fields[tuple.field] : undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Super nit: wonder if we're gonna be doing this a lot if it's worth just creating a tiny util to extract x field from fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah if it becomes a common pattern I'd support pulling it out into a function

if (tuple.operator === 'included') {
if (eventItem == null) {
return true;
}
// only create a signal if the event is not in the value list
return !tuple.matchedSet.has(JSON.stringify(eventItem));
} else if (tuple.operator === 'excluded') {
if (eventItem == null) {
return false;
}
// only create a signal if the event is in the value list
return tuple.matchedSet.has(JSON.stringify(eventItem));
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since there's only two operators, not sure this else path ever hits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It shouldn't be hit, but since the operator is a string it's possible for it to get into an invalid state so it's good to handle that possibility. Looking at it again we'd probably want to return true in that case so an invalid exception operator doesn't allowlist everything, but I think that's an issue for a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Wonder if we'd want to log there too. Worry about just letting through invalid states.

Expand Down
16 changes: 8 additions & 8 deletions ...y_solution/server/lib/detection_engine/signals/filters/filter_events_against_list.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,12 +162,12 @@ describe('filterEventsAgainstList', () => {

// this call represents an exception list with a value list containing ['2.2.2.2', '4.4.4.4']
(listClient.searchListItemByValues as jest.Mock).mockResolvedValueOnce([
{ ...getSearchListItemResponseMock(), value: '2.2.2.2' },
{ ...getSearchListItemResponseMock(), value: '4.4.4.4' },
{ ...getSearchListItemResponseMock(), value: ['2.2.2.2'] },
{ ...getSearchListItemResponseMock(), value: ['4.4.4.4'] },
]);
// this call represents an exception list with a value list containing ['6.6.6.6']
(listClient.searchListItemByValues as jest.Mock).mockResolvedValueOnce([
{ ...getSearchListItemResponseMock(), value: '6.6.6.6' },
{ ...getSearchListItemResponseMock(), value: ['6.6.6.6'] },
]);

const res = await filterEventsAgainstList({
Expand Down Expand Up @@ -224,11 +224,11 @@ describe('filterEventsAgainstList', () => {

// this call represents an exception list with a value list containing ['2.2.2.2', '4.4.4.4']
(listClient.searchListItemByValues as jest.Mock).mockResolvedValueOnce([
{ ...getSearchListItemResponseMock(), value: '2.2.2.2' },
{ ...getSearchListItemResponseMock(), value: ['2.2.2.2'] },
]);
// this call represents an exception list with a value list containing ['6.6.6.6']
(listClient.searchListItemByValues as jest.Mock).mockResolvedValueOnce([
{ ...getSearchListItemResponseMock(), value: '6.6.6.6' },
{ ...getSearchListItemResponseMock(), value: ['6.6.6.6'] },
]);

const res = await filterEventsAgainstList({
Expand Down Expand Up @@ -283,11 +283,11 @@ describe('filterEventsAgainstList', () => {

// this call represents an exception list with a value list containing ['2.2.2.2']
(listClient.searchListItemByValues as jest.Mock).mockResolvedValueOnce([
{ ...getSearchListItemResponseMock(), value: '2.2.2.2' },
{ ...getSearchListItemResponseMock(), value: ['2.2.2.2'] },
]);
// this call represents an exception list with a value list containing ['4.4.4.4']
(listClient.searchListItemByValues as jest.Mock).mockResolvedValueOnce([
{ ...getSearchListItemResponseMock(), value: '4.4.4.4' },
{ ...getSearchListItemResponseMock(), value: ['4.4.4.4'] },
]);

const res = await filterEventsAgainstList({
Expand Down Expand Up @@ -365,7 +365,7 @@ describe('filterEventsAgainstList', () => {

// this call represents an exception list with a value list containing ['2.2.2.2', '4.4.4.4']
(listClient.searchListItemByValues as jest.Mock).mockResolvedValue([
{ ...getSearchListItemResponseMock(), value: '2.2.2.2' },
{ ...getSearchListItemResponseMock(), value: ['2.2.2.2'] },
]);

const res = await filterEventsAgainstList({
Expand Down
6 changes: 6 additions & 0 deletions ...k/plugins/security_solution/server/lib/detection_engine/signals/find_threshold_signals.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,12 @@ export const findThresholdSignals = async ({
},
},
],
fields: [
{
field: '*',
include_unmapped: true,
},
],
size: 1,
},
},
Expand Down
14 changes: 7 additions & 7 deletions ...ns/security_solution/server/lib/detection_engine/signals/search_after_bulk_create.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,9 +310,9 @@ describe('searchAfterAndBulkCreate', () => {

test('should return success when all search results are in the allowlist and with sortId present', async () => {
const searchListItems: SearchListItemArraySchema = [
{ ...getSearchListItemResponseMock(), value: '1.1.1.1' },
{ ...getSearchListItemResponseMock(), value: '2.2.2.2' },
{ ...getSearchListItemResponseMock(), value: '3.3.3.3' },
{ ...getSearchListItemResponseMock(), value: ['1.1.1.1'] },
{ ...getSearchListItemResponseMock(), value: ['2.2.2.2'] },
{ ...getSearchListItemResponseMock(), value: ['3.3.3.3'] },
];
listClient.searchListItemByValues = jest.fn().mockResolvedValue(searchListItems);
const sampleParams = sampleRuleAlertParams(30);
Expand Down Expand Up @@ -374,10 +374,10 @@ describe('searchAfterAndBulkCreate', () => {

test('should return success when all search results are in the allowlist and no sortId present', async () => {
const searchListItems: SearchListItemArraySchema = [
{ ...getSearchListItemResponseMock(), value: '1.1.1.1' },
{ ...getSearchListItemResponseMock(), value: '2.2.2.2' },
{ ...getSearchListItemResponseMock(), value: '2.2.2.2' },
{ ...getSearchListItemResponseMock(), value: '2.2.2.2' },
{ ...getSearchListItemResponseMock(), value: ['1.1.1.1'] },
{ ...getSearchListItemResponseMock(), value: ['2.2.2.2'] },
{ ...getSearchListItemResponseMock(), value: ['2.2.2.2'] },
{ ...getSearchListItemResponseMock(), value: ['2.2.2.2'] },
];

listClient.searchListItemByValues = jest.fn().mockResolvedValue(searchListItems);
Expand Down
Loading