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

Jump to bottom

[Security Solution][Exceptions] Implement exceptions for ML rules #84006

Merged
merged 12 commits into from
Dec 2, 2020
Merged

[Security Solution][Exceptions] Implement exceptions for ML rules #84006

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
8525db7
Implement exceptions for ML rules
marshallmain Nov 20, 2020
09682d5
Merge branch 'master' into ml-exceptions
marshallmain Nov 20, 2020
7a20f78
Remove unused import
marshallmain Nov 21, 2020
50306cb
Merge branch 'master' into ml-exceptions
kibanamachine Nov 23, 2020
c5ad7c0
Better implicit types
marshallmain Nov 25, 2020
abbc51a
Merge branch 'ml-exceptions' of github.com:marshallmain/kibana into m…
marshallmain Nov 25, 2020
5e5a9b0
Merge branch 'master' into ml-exceptions
kibanamachine Nov 26, 2020
87f817e
Merge branch 'master' into ml-exceptions
kibanamachine Nov 30, 2020
0b3c064
Retrieve ML rule index pattern for exception field suggestions and au…
marshallmain Dec 2, 2020
d52702c
Add ML job logic to edit exception modal
marshallmain Dec 2, 2020
fe820c3
Remove unnecessary logic change
marshallmain Dec 2, 2020
61299d5
Merge branch 'master' into ml-exceptions
kibanamachine Dec 2, 2020
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
72 changes: 46 additions & 26 deletions x-pack/plugins/security_solution/common/detection_engine/get_query_filter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import { getQueryFilter, buildExceptionFilter, buildEqlSearchRequest } from './get_query_filter';
import { Filter, EsQueryConfig } from 'src/plugins/data/public';
import { getExceptionListItemSchemaMock } from '../../../lists/common/schemas/response/exception_list_item_schema.mock';
import { ExceptionListItemSchema } from '../shared_imports';

describe('get_filter', () => {
describe('getQueryFilter', () => {
Expand Down Expand Up @@ -919,19 +920,27 @@ describe('get_filter', () => {
dateFormatTZ: 'Zulu',
};
test('it should build a filter without chunking exception items', () => {
const exceptionFilter = buildExceptionFilter(
[
{ language: 'kuery', query: 'host.name: linux and some.field: value' },
{ language: 'kuery', query: 'user.name: name' },
const exceptionItem1: ExceptionListItemSchema = {
...getExceptionListItemSchemaMock(),
entries: [
{ field: 'host.name', operator: 'included', type: 'match', value: 'linux' },
{ field: 'some.field', operator: 'included', type: 'match', value: 'value' },
],
{
};
const exceptionItem2: ExceptionListItemSchema = {
...getExceptionListItemSchemaMock(),
entries: [{ field: 'user.name', operator: 'included', type: 'match', value: 'name' }],
};
const exceptionFilter = buildExceptionFilter({
lists: [exceptionItem1, exceptionItem2],
config,
excludeExceptions: true,
chunkSize: 2,
indexPattern: {
fields: [],
title: 'auditbeat-*',
},
config,
true,
2
);
});
expect(exceptionFilter).toEqual({
meta: {
alias: null,
Expand All @@ -949,7 +958,7 @@ describe('get_filter', () => {
minimum_should_match: 1,
should: [
{
match: {
match_phrase: {
'host.name': 'linux',
},
},
Expand All @@ -961,7 +970,7 @@ describe('get_filter', () => {
minimum_should_match: 1,
should: [
{
match: {
match_phrase: {
'some.field': 'value',
},
},
Expand All @@ -976,7 +985,7 @@ describe('get_filter', () => {
minimum_should_match: 1,
should: [
{
match: {
match_phrase: {
'user.name': 'name',
},
},
Expand All @@ -990,20 +999,31 @@ describe('get_filter', () => {
});

test('it should properly chunk exception items', () => {
const exceptionFilter = buildExceptionFilter(
[
{ language: 'kuery', query: 'host.name: linux and some.field: value' },
{ language: 'kuery', query: 'user.name: name' },
{ language: 'kuery', query: 'file.path: /safe/path' },
const exceptionItem1: ExceptionListItemSchema = {
...getExceptionListItemSchemaMock(),
entries: [
{ field: 'host.name', operator: 'included', type: 'match', value: 'linux' },
{ field: 'some.field', operator: 'included', type: 'match', value: 'value' },
],
{
};
const exceptionItem2: ExceptionListItemSchema = {
...getExceptionListItemSchemaMock(),
entries: [{ field: 'user.name', operator: 'included', type: 'match', value: 'name' }],
};
const exceptionItem3: ExceptionListItemSchema = {
...getExceptionListItemSchemaMock(),
entries: [{ field: 'file.path', operator: 'included', type: 'match', value: '/safe/path' }],
};
const exceptionFilter = buildExceptionFilter({
lists: [exceptionItem1, exceptionItem2, exceptionItem3],
config,
excludeExceptions: true,
chunkSize: 2,
indexPattern: {
fields: [],
title: 'auditbeat-*',
},
config,
true,
2
);
});
expect(exceptionFilter).toEqual({
meta: {
alias: null,
Expand All @@ -1024,7 +1044,7 @@ describe('get_filter', () => {
minimum_should_match: 1,
should: [
{
match: {
match_phrase: {
'host.name': 'linux',
},
},
Expand All @@ -1036,7 +1056,7 @@ describe('get_filter', () => {
minimum_should_match: 1,
should: [
{
match: {
match_phrase: {
'some.field': 'value',
},
},
Expand All @@ -1051,7 +1071,7 @@ describe('get_filter', () => {
minimum_should_match: 1,
should: [
{
match: {
match_phrase: {
'user.name': 'name',
},
},
Expand All @@ -1069,7 +1089,7 @@ describe('get_filter', () => {
minimum_should_match: 1,
should: [
{
match: {
match_phrase: {
'file.path': '/safe/path',
},
},
Expand Down
69 changes: 40 additions & 29 deletions x-pack/plugins/security_solution/common/detection_engine/get_query_filter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,19 +53,18 @@ export const getQueryFilter = (
* buildEsQuery, this allows us to offer nested queries
* regardless
*/
const exceptionQueries = buildExceptionListQueries({ language: 'kuery', lists });
if (exceptionQueries.length > 0) {
// Assume that `indices.query.bool.max_clause_count` is at least 1024 (the default value),
// allowing us to make 1024-item chunks of exception list items.
// Discussion at https://issues.apache.org/jira/browse/LUCENE-4835 indicates that 1024 is a
// very conservative value.
const exceptionFilter = buildExceptionFilter(
exceptionQueries,
indexPattern,
config,
excludeExceptions,
1024
);
// Assume that `indices.query.bool.max_clause_count` is at least 1024 (the default value),
// allowing us to make 1024-item chunks of exception list items.
// Discussion at https://issues.apache.org/jira/browse/LUCENE-4835 indicates that 1024 is a
// very conservative value.
const exceptionFilter = buildExceptionFilter({
lists,
config,
excludeExceptions,
chunkSize: 1024,
indexPattern,
});
if (exceptionFilter !== undefined) {
enabledFilters.push(exceptionFilter);
}
const initialQuery = { query, language };
Expand Down Expand Up @@ -101,15 +100,17 @@ export const buildEqlSearchRequest = (
ignoreFilterIfFieldNotInIndex: false,
dateFormatTZ: 'Zulu',
};
const exceptionQueries = buildExceptionListQueries({ language: 'kuery', lists: exceptionLists });
let exceptionFilter: Filter | undefined;
if (exceptionQueries.length > 0) {
// Assume that `indices.query.bool.max_clause_count` is at least 1024 (the default value),
// allowing us to make 1024-item chunks of exception list items.
// Discussion at https://issues.apache.org/jira/browse/LUCENE-4835 indicates that 1024 is a
// very conservative value.
exceptionFilter = buildExceptionFilter(exceptionQueries, indexPattern, config, true, 1024);
}
// Assume that `indices.query.bool.max_clause_count` is at least 1024 (the default value),
// allowing us to make 1024-item chunks of exception list items.
// Discussion at https://issues.apache.org/jira/browse/LUCENE-4835 indicates that 1024 is a
// very conservative value.
const exceptionFilter = buildExceptionFilter({
lists: exceptionLists,
config,
excludeExceptions: true,
chunkSize: 1024,
indexPattern,
});
const indexString = index.join();
const requestFilter: unknown[] = [
{
Expand Down Expand Up @@ -154,13 +155,23 @@ export const buildEqlSearchRequest = (
}
};

export const buildExceptionFilter = (
exceptionQueries: Query[],
indexPattern: IIndexPattern,
config: EsQueryConfig,
excludeExceptions: boolean,
chunkSize: number
) => {
export const buildExceptionFilter = ({
lists,
config,
excludeExceptions,
chunkSize,
indexPattern,
}: {
lists: Array<ExceptionListItemSchema | CreateExceptionListItemSchema>;
config: EsQueryConfig;
excludeExceptions: boolean;
chunkSize: number;
indexPattern?: IIndexPattern;
}) => {
const exceptionQueries = buildExceptionListQueries({ language: 'kuery', lists });
if (exceptionQueries.length === 0) {
return undefined;
}
const exceptionFilter: Filter = {
meta: {
alias: null,
Expand Down
4 changes: 3 additions & 1 deletion x-pack/plugins/security_solution/public/common/containers/source/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,8 +197,10 @@ export const useFetchIndex = (
);

useEffect(() => {
if (!isEmpty(indexNames) && !isEqual(previousIndexesName.current, indexNames)) {
if (!isEqual(previousIndexesName.current, indexNames)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This change might be redundant with mine

indexFieldsSearch(indexNames);
} else {
setLoading(false);
}
}, [indexNames, indexFieldsSearch, previousIndexesName]);

Expand Down
19 changes: 12 additions & 7 deletions ...olution/public/detections/components/alerts_table/timeline_actions/alert_context_menu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import { TimelineId } from '../../../../../common/types/timeline';
import { DEFAULT_INDEX_PATTERN } from '../../../../../common/constants';
import { Status, Type } from '../../../../../common/detection_engine/schemas/common/schemas';
import { isThresholdRule } from '../../../../../common/detection_engine/utils';
import { isMlRule } from '../../../../../common/machine_learning/helpers';
import { timelineActions } from '../../../../timelines/store/timeline';
import { EventsTd, EventsTdContent } from '../../../../timelines/components/timeline/styles';
import { DEFAULT_ICON_BUTTON_WIDTH } from '../../../../timelines/components/timeline/helpers';
Expand Down Expand Up @@ -75,11 +74,17 @@ const AlertContextMenuComponent: React.FC<AlertContextMenuProps> = ({
'',
[ecsRowData]
);
const ruleIndices = useMemo(
(): string[] =>
(ecsRowData.signal?.rule && ecsRowData.signal.rule.index) ?? DEFAULT_INDEX_PATTERN,
[ecsRowData]
);
const ruleIndices = useMemo((): string[] => {
if (
ecsRowData.signal?.rule &&
ecsRowData.signal.rule.index &&
ecsRowData.signal.rule.index.length > 0
) {
return ecsRowData.signal.rule.index;
} else {
return DEFAULT_INDEX_PATTERN;
}
}, [ecsRowData]);

const { addWarning } = useAppToasts();

Expand Down Expand Up @@ -317,7 +322,7 @@ const AlertContextMenuComponent: React.FC<AlertContextMenuProps> = ({
const areExceptionsAllowed = useMemo((): boolean => {
const ruleTypes = getOr([], 'signal.rule.type', ecsRowData);
const [ruleType] = ruleTypes as Type[];
return !isMlRule(ruleType) && !isThresholdRule(ruleType);
return !isThresholdRule(ruleType);
}, [ecsRowData]);

const addExceptionComponent = (
Expand Down
6 changes: 1 addition & 5 deletions ...ck/plugins/security_solution/public/detections/components/rules/step_about_rule/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import { EuiAccordion, EuiFlexItem, EuiSpacer, EuiFormRow } from '@elastic/eui';
import React, { FC, memo, useCallback, useEffect, useState } from 'react';
import styled from 'styled-components';

import { isMlRule } from '../../../../../common/machine_learning/helpers';
import { isThresholdRule } from '../../../../../common/detection_engine/utils';
import {
RuleStepProps,
Expand Down Expand Up @@ -76,10 +75,7 @@ const StepAboutRuleComponent: FC<StepAboutRuleProps> = ({
const [severityValue, setSeverityValue] = useState<string>(initialState.severity.value);
const [indexPatternLoading, { indexPatterns }] = useFetchIndex(defineRuleData?.index ?? []);

const canUseExceptions =
defineRuleData?.ruleType &&
!isMlRule(defineRuleData.ruleType) &&
!isThresholdRule(defineRuleData.ruleType);
const canUseExceptions = defineRuleData?.ruleType && !isThresholdRule(defineRuleData.ruleType);

const { form } = useForm<AboutStepRule>({
defaultValue: initialState,
Expand Down
3 changes: 1 addition & 2 deletions ...lugins/security_solution/public/detections/pages/detection_engine/rules/details/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@ import { DEFAULT_INDEX_PATTERN } from '../../../../../../common/constants';
import { useFullScreen } from '../../../../../common/containers/use_full_screen';
import { Display } from '../../../../../hosts/pages/display';
import { ExceptionListTypeEnum, ExceptionListIdentifiers } from '../../../../../shared_imports';
import { isMlRule } from '../../../../../../common/machine_learning/helpers';
import { isThresholdRule } from '../../../../../../common/detection_engine/utils';
import { useRuleAsync } from '../../../../containers/detection_engine/rules/use_rule_async';
import { showGlobalFilters } from '../../../../../timelines/components/timeline/helpers';
Expand All @@ -104,7 +103,7 @@ enum RuleDetailTabs {
}

const getRuleDetailsTabs = (rule: Rule | null) => {
const canUseExceptions = rule && !isMlRule(rule.type) && !isThresholdRule(rule.type);
const canUseExceptions = rule && !isThresholdRule(rule.type);
return [
{
id: RuleDetailTabs.alerts,
Expand Down
8 changes: 4 additions & 4 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 @@ -149,8 +149,8 @@ export const sampleDocNoSortIdNoVersion = (someUuid: string = sampleIdGuid): Sig

export const sampleDocWithSortId = (
someUuid: string = sampleIdGuid,
ip?: string,
destIp?: string
ip?: string | string[],
destIp?: string | string[]
): SignalSourceHit => ({
_index: 'myFakeSignalIndex',
_type: 'doc',
Expand Down Expand Up @@ -485,8 +485,8 @@ export const repeatedSearchResultsWithSortId = (
total: number,
pageSize: number,
guids: string[],
ips?: string[],
destIps?: string[]
ips?: Array<string | string[]>,
destIps?: Array<string | string[]>
): SignalSearchResponse => ({
took: 10,
timed_out: false,
Expand Down
2 changes: 1 addition & 1 deletion ...k/plugins/security_solution/server/lib/detection_engine/signals/bulk_create_ml_signals.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@

import { flow, omit } from 'lodash/fp';
import set from 'set-value';
import { SearchResponse } from 'elasticsearch';

import { Logger } from '../../../../../../../src/core/server';
import { AlertServices } from '../../../../../alerts/server';
Expand All @@ -15,6 +14,7 @@ import { RuleTypeParams, RefreshTypes } from '../types';
import { singleBulkCreate, SingleBulkCreateResponse } from './single_bulk_create';
import { AnomalyResults, Anomaly } from '../../machine_learning';
import { BuildRuleMessage } from './rule_messages';
import { SearchResponse } from '../../types';
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that we're continuing to diverge from the official typings, but I couldn't find an explanation as to why (I was able to revert these without type errors). If we have an unsupported use case we should probably file an issue upstream.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I was playing around with the types as well and forgot to switch this one back...got curious about the duplicates

Copy link
Contributor Author

@marshallmain marshallmain Nov 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Ah I got an error when changing it back and tracked it down. The search response for KQL query rules is typed based on the custom SearchResponse type because it is usinghits.total which looks to be typed (incorrectly) as number in the official SearchResponse type. To share the "filter against lists" code with the KQL rules then the ML bulk create logic has to accept our custom SearchResponse.

Need to investigate further why the upstream SearchResponse.hits.total type doesn't match the expected type based on https://www.elastic.co/guide/en/elasticsearch/reference/current/search-search.html

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it was a number before 7.0: https://www.elastic.co/guide/en/elasticsearch/reference/current/breaking-changes-7.0.html#hits-total-now-object-search-response

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The types package is out of date by over a year.

There's an issue to include up-to-date types with the client, but nothing has been merged. We may be better off preferring to use our own SearchResponse type in general over the unmaintained SearchResponse from ES 5 - until there are centrally maintained response types.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this has been an issue for me in the past too. I think we're on our own until those are updated...


interface BulkCreateMlSignalsParams {
actions: RuleAlertAction[];
Expand Down
Loading