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.

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] adds WrapSequences method (RAC) #102106

Merged
merged 13 commits into from
Jun 17, 2021
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@ describe('eql_executor', () => {
logger,
searchAfterSize,
bulkCreate: jest.fn(),
wrapHits: jest.fn(),
wrapSequences: jest.fn(),
});
expect(response.warningMessages.length).toEqual(1);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,18 @@ import { isOutdated } from '../../migrations/helpers';
import { getIndexVersion } from '../../routes/index/get_index_version';
import { MIN_EQL_RULE_INDEX_VERSION } from '../../routes/index/get_signals_template';
import { EqlRuleParams } from '../../schemas/rule_schemas';
import { buildSignalFromEvent, buildSignalGroupFromSequence } from '../build_bulk_body';
import { getInputIndex } from '../get_input_output_index';
import { filterDuplicateSignals } from '../filter_duplicate_signals';

import {
AlertAttributes,
BulkCreate,
WrapHits,
WrapSequences,
EqlSignalSearchResponse,
SearchAfterAndBulkCreateReturnType,
WrappedSignalHit,
SimpleHit,
} from '../types';
import { createSearchAfterReturnType, makeFloatString, wrapSignal } from '../utils';
import { createSearchAfterReturnType, makeFloatString } from '../utils';

export const eqlExecutor = async ({
rule,
Expand All @@ -41,6 +42,8 @@ export const eqlExecutor = async ({
logger,
searchAfterSize,
bulkCreate,
wrapHits,
wrapSequences,
}: {
rule: SavedObject<AlertAttributes<EqlRuleParams>>;
exceptionItems: ExceptionListItemSchema[];
Expand All @@ -49,6 +52,8 @@ export const eqlExecutor = async ({
logger: Logger;
searchAfterSize: number;
bulkCreate: BulkCreate;
wrapHits: WrapHits;
wrapSequences: WrapSequences;
}): Promise<SearchAfterAndBulkCreateReturnType> => {
const result = createSearchAfterReturnType();
const ruleParams = rule.attributes.params;
Expand Down Expand Up @@ -101,27 +106,18 @@ export const eqlExecutor = async ({
const eqlSignalSearchEnd = performance.now();
const eqlSearchDuration = makeFloatString(eqlSignalSearchEnd - eqlSignalSearchStart);
result.searchAfterTimes = [eqlSearchDuration];
let newSignals: WrappedSignalHit[] | undefined;
let newSignals: SimpleHit[] | undefined;
if (response.hits.sequences !== undefined) {
newSignals = response.hits.sequences.reduce(
(acc: WrappedSignalHit[], sequence) =>
acc.concat(buildSignalGroupFromSequence(sequence, rule, ruleParams.outputIndex)),
[]
);
newSignals = wrapSequences(response.hits.sequences);
} else if (response.hits.events !== undefined) {
newSignals = filterDuplicateSignals(
rule.id,
response.hits.events.map((event) =>
wrapSignal(buildSignalFromEvent(event, rule, true), ruleParams.outputIndex)
)
);
newSignals = wrapHits(response.hits.events);
} else {
throw new Error(
'eql query response should have either `sequences` or `events` but had neither'
);
}

if (newSignals.length > 0) {
if (newSignals?.length) {
const insertResult = await bulkCreate(newSignals);
result.bulkCreateTimes.push(insertResult.bulkCreateDuration);
result.createdSignalsCount += insertResult.createdItemsCount;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,13 @@ const mockSignals = [
];

describe('filterDuplicateSignals', () => {
it('filters duplicate signals', () => {
expect(filterDuplicateSignals(mockRuleId1, mockSignals).length).toEqual(1);
});
describe('detection engine implementation', () => {
it('filters duplicate signals', () => {
expect(filterDuplicateSignals(mockRuleId1, mockSignals, false).length).toEqual(1);
});

it('does not filter non-duplicate signals', () => {
expect(filterDuplicateSignals(mockRuleId3, mockSignals).length).toEqual(2);
it('does not filter non-duplicate signals', () => {
expect(filterDuplicateSignals(mockRuleId3, mockSignals, false).length).toEqual(2);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,26 @@
* 2.0.
*/

import { WrappedSignalHit } from './types';
import { SimpleHit, WrappedSignalHit } from './types';

export const filterDuplicateSignals = (ruleId: string, signals: WrappedSignalHit[]) => {
return signals.filter(
(doc) => !doc._source.signal?.ancestors.some((ancestor) => ancestor.rule === ruleId)
);
const isWrappedSignalHit = (
signals: SimpleHit[],
isRuleRegistryEnabled: boolean
): signals is WrappedSignalHit[] => {
return !isRuleRegistryEnabled;
};

export const filterDuplicateSignals = (
ruleId: string,
signals: SimpleHit[],
isRuleRegistryEnabled: boolean
) => {
if (isWrappedSignalHit(signals, isRuleRegistryEnabled)) {
return signals.filter(
(doc) => !doc._source.signal?.ancestors.some((ancestor) => ancestor.rule === ruleId)
);
} else {
// TODO: filter duplicate signals for RAC
return [];
}
};
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,10 @@ describe('searchAfterAndBulkCreate', () => {
buildRuleMessage,
false
);
wrapHits = wrapHitsFactory({ ruleSO, signalsIndex: DEFAULT_SIGNALS_INDEX });
wrapHits = wrapHitsFactory({
ruleSO,
signalsIndex: DEFAULT_SIGNALS_INDEX,
});
});

test('should return success with number of searches less than max signals', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ import {
} from '../schemas/rule_schemas';
import { bulkCreateFactory } from './bulk_create_factory';
import { wrapHitsFactory } from './wrap_hits_factory';
import { wrapSequencesFactory } from './wrap_sequences_factory';

export const signalRulesAlertType = ({
logger,
Expand Down Expand Up @@ -233,6 +234,11 @@ export const signalRulesAlertType = ({
signalsIndex: params.outputIndex,
});

const wrapSequences = wrapSequencesFactory({
ruleSO: savedObject,
signalsIndex: params.outputIndex,
});

if (isMlRule(type)) {
const mlRuleSO = asTypeSpecificSO(savedObject, machineLearningRuleParams);
result = await mlExecutor({
Expand Down Expand Up @@ -302,6 +308,8 @@ export const signalRulesAlertType = ({
searchAfterSize,
bulkCreate,
logger,
wrapHits,
wrapSequences,
});
} else {
throw new Error(`unknown rule type ${type}`);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {
BaseHit,
RuleAlertAction,
SearchTypes,
EqlSequence,
} from '../../../../common/detection_engine/types';
import { ListClient } from '../../../../../lists/server';
import { Logger, SavedObject } from '../../../../../../../src/core/server';
Expand Down Expand Up @@ -257,9 +258,11 @@ export type SignalsEnrichment = (signals: SignalSearchResponse) => Promise<Signa

export type BulkCreate = <T>(docs: Array<BaseHit<T>>) => Promise<GenericBulkCreateResponse<T>>;

export type WrapHits = (
hits: Array<estypes.SearchHit<unknown>>
) => Array<BaseHit<{ '@timestamp': string }>>;
export type SimpleHit = BaseHit<{ '@timestamp': string }>;

export type WrapHits = (hits: Array<estypes.SearchHit<unknown>>) => SimpleHit[];
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this unknown be typed and then we avoid the as casting below.

_source: buildBulkBody(ruleSO, doc as SignalSourceHit),


export type WrapSequences = (sequences: Array<EqlSequence<SignalSource>>) => SimpleHit[];

export interface SearchAfterAndBulkCreateParams {
tuples: Array<{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,15 @@ export const wrapHitsFactory = ({
const wrappedDocs: WrappedSignalHit[] = events.flatMap((doc) => [
{
_index: signalsIndex,
// TODO: bring back doc._version
_id: generateId(doc._index, doc._id, '', ruleSO.attributes.params.ruleId ?? ''),
_id: generateId(
doc._index,
doc._id,
doc._version ? doc._version.toString() : '',
Copy link
Contributor

Choose a reason for hiding this comment

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

fwiw a lot of people use the constructor variant such as String(doc._version) because it's safer if someone changes this code and accidentally pushes in a non string.

One other thing is that I do not think the doc._version is the best usage of identifying unique document changes anymore within elastic.

In Saved Objects and other places if we are trying to identify if a document has changed and key off of that it's better to use _seq_no and _primary_term

Ref: https://www.elastic.co/guide/en/elasticsearch/reference/current/optimistic-concurrency-control.html

The caveat being we have to ensure we are passing the correct flags to return these two fields with our search results which you would have to check. You could fall back on doc._version if you do not see them being set to double check things or write an e2e test to ensure we do return them as expected as well at some point.

However, overall I think we have older deprecated patterns of using doc._version instead of using those two other fields and should prefer using those.

Let me know if that doesn't sound right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just pushed some changes and updated it as String(doc._version) for this moment. It looks like we would need to use the seq_no_primary_term flag, I haven't found out exactly how just yet, looking into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

created #102395

ruleSO.attributes.params.ruleId ?? ''
),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to type the line below in the file above and then we avoid this as cast?

_source: buildBulkBody(ruleSO, doc as SignalSourceHit),

_source: buildBulkBody(ruleSO, doc as SignalSourceHit),
},
]);

return filterDuplicateSignals(ruleSO.id, wrappedDocs);
return filterDuplicateSignals(ruleSO.id, wrappedDocs, false);
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { SearchAfterAndBulkCreateParams, WrappedSignalHit, WrapSequences } from './types';
import { buildSignalGroupFromSequence } from './build_bulk_body';

export const wrapSequencesFactory = ({
ruleSO,
signalsIndex,
}: {
ruleSO: SearchAfterAndBulkCreateParams['ruleSO'];
signalsIndex: string;
}): WrapSequences => (sequences) => {
const wrappedDocs = sequences.reduce(
(acc: WrappedSignalHit[], sequence) =>
acc.concat(buildSignalGroupFromSequence(sequence, ruleSO, signalsIndex)),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can we do a splat and avoid the concat here? Something like this? (warning I did not test this, but this is usually the format I see in the code base).

[...acc, buildSignalGroupFromSequence(sequence, ruleSO, signalsIndex)]

[]
);

return wrappedDocs;
};
6 changes: 4 additions & 2 deletions x-pack/plugins/security_solution/server/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -208,8 +208,10 @@ export class Plugin implements IPlugin<PluginSetup, PluginStart, SetupPlugins, S
});

// TODO: Once we are past experimental phase this check can be removed along with legacy registration of rules
const isRuleRegistryEnabled = experimentalFeatures.ruleRegistryEnabled;

let ruleDataClient: RuleDataClient | null = null;
if (experimentalFeatures.ruleRegistryEnabled) {
if (isRuleRegistryEnabled) {
const { ruleDataService } = plugins.ruleRegistry;
const start = () => core.getStartServices().then(([coreStart]) => coreStart);

Expand Down Expand Up @@ -293,7 +295,7 @@ export class Plugin implements IPlugin<PluginSetup, PluginStart, SetupPlugins, S
const ruleTypes = [
SIGNALS_ID,
NOTIFICATIONS_ID,
...(experimentalFeatures.ruleRegistryEnabled ? referenceRuleTypes : []),
...(isRuleRegistryEnabled ? referenceRuleTypes : []),
];

plugins.features.registerKibanaFeature({
Expand Down