-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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][Detections] Refactor signal ancestry to allow multiple parents #76531
Changes from 1 commit
39a5f49
de9debb
a3e1559
d818f21
2c9d038
93094ab
08a2a4d
9143432
f3b294e
166f491
0a8caa5
203c363
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,11 +22,33 @@ | |
} | ||
} | ||
}, | ||
"parents": { | ||
"properties": { | ||
"rule": { | ||
"type": "keyword" | ||
}, | ||
"index": { | ||
"type": "keyword" | ||
}, | ||
"id": { | ||
"type": "keyword" | ||
}, | ||
"type": { | ||
"type": "keyword" | ||
}, | ||
"depth": { | ||
"type": "long" | ||
} | ||
} | ||
}, | ||
"ancestors": { | ||
"properties": { | ||
"rule": { | ||
"type": "keyword" | ||
}, | ||
"index": { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
"type": "keyword" | ||
}, | ||
"id": { | ||
"type": "keyword" | ||
}, | ||
|
@@ -299,6 +321,9 @@ | |
}, | ||
"threshold_count": { | ||
"type": "float" | ||
}, | ||
"depth": { | ||
"type": "integer" | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,9 +4,9 @@ | |
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { SignalSourceHit, SignalHit } from './types'; | ||
import { SignalSourceHit, SignalHit, Signal } from './types'; | ||
import { buildRule } from './build_rule'; | ||
import { buildSignal } from './build_signal'; | ||
import { additionalSignalFields, buildSignal } from './build_signal'; | ||
import { buildEventTypeSignal } from './build_event_type_signal'; | ||
import { RuleAlertAction } from '../../../../common/detection_engine/types'; | ||
import { RuleTypeParams } from '../types'; | ||
|
@@ -58,7 +58,10 @@ export const buildBulkBody = ({ | |
tags, | ||
throttle, | ||
}); | ||
const signal = buildSignal(doc, rule); | ||
const signal: Signal = { | ||
...buildSignal([doc], rule), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
...additionalSignalFields(doc), | ||
}; | ||
const event = buildEventTypeSignal(doc); | ||
const signalHit: SignalHit = { | ||
...doc._source, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,10 +4,12 @@ | |
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { buildRule } from './build_rule'; | ||
import { buildRule, removeInternalTagsFromRule } from './build_rule'; | ||
import { sampleDocNoSortId, sampleRuleAlertParams, sampleRuleGuid } from './__mocks__/es_results'; | ||
import { RulesSchema } from '../../../../common/detection_engine/schemas/response/rules_schema'; | ||
import { getListArrayMock } from '../../../../common/detection_engine/schemas/types/lists.mock'; | ||
import { INTERNAL_RULE_ID_KEY, INTERNAL_IMMUTABLE_KEY } from '../../../../common/constants'; | ||
import { getPartialRulesSchemaMock } from '../../../../common/detection_engine/schemas/response/rules_schema.mocks'; | ||
|
||
describe('buildRule', () => { | ||
beforeEach(() => { | ||
|
@@ -208,4 +210,102 @@ describe('buildRule', () => { | |
}; | ||
expect(rule).toEqual(expected); | ||
}); | ||
|
||
test('it builds a rule and removes internal tags', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
const ruleParams = sampleRuleAlertParams(); | ||
const rule = buildRule({ | ||
actions: [], | ||
doc: sampleDocNoSortId(), | ||
ruleParams, | ||
name: 'some-name', | ||
id: sampleRuleGuid, | ||
enabled: false, | ||
createdAt: '2020-01-28T15:58:34.810Z', | ||
updatedAt: '2020-01-28T15:59:14.004Z', | ||
createdBy: 'elastic', | ||
updatedBy: 'elastic', | ||
interval: 'some interval', | ||
tags: [ | ||
'some fake tag 1', | ||
'some fake tag 2', | ||
`${INTERNAL_RULE_ID_KEY}:rule-1`, | ||
`${INTERNAL_IMMUTABLE_KEY}:true`, | ||
], | ||
throttle: 'no_actions', | ||
}); | ||
const expected: Partial<RulesSchema> = { | ||
actions: [], | ||
author: ['Elastic'], | ||
building_block_type: 'default', | ||
created_by: 'elastic', | ||
description: 'Detecting root and admin users', | ||
enabled: false, | ||
false_positives: [], | ||
from: 'now-6m', | ||
id: '04128c15-0d1b-4716-a4c5-46997ac7f3bd', | ||
immutable: false, | ||
index: ['auditbeat-*', 'filebeat-*', 'packetbeat-*', 'winlogbeat-*'], | ||
interval: 'some interval', | ||
language: 'kuery', | ||
license: 'Elastic License', | ||
max_signals: 10000, | ||
name: 'some-name', | ||
output_index: '.siem-signals', | ||
query: 'user.name: root or user.name: admin', | ||
references: ['http://google.com'], | ||
risk_score: 50, | ||
risk_score_mapping: [], | ||
rule_id: 'rule-1', | ||
severity: 'high', | ||
severity_mapping: [], | ||
tags: ['some fake tag 1', 'some fake tag 2'], | ||
threat: [], | ||
to: 'now', | ||
type: 'query', | ||
note: '', | ||
updated_by: 'elastic', | ||
updated_at: rule.updated_at, | ||
created_at: rule.created_at, | ||
throttle: 'no_actions', | ||
exceptions_list: getListArrayMock(), | ||
version: 1, | ||
}; | ||
expect(rule).toEqual(expected); | ||
}); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Next 4 tests are simply moved from |
||
test('it removes internal tags from a typical rule', () => { | ||
const rule = getPartialRulesSchemaMock(); | ||
rule.tags = [ | ||
'some fake tag 1', | ||
'some fake tag 2', | ||
`${INTERNAL_RULE_ID_KEY}:rule-1`, | ||
`${INTERNAL_IMMUTABLE_KEY}:true`, | ||
]; | ||
const noInternals = removeInternalTagsFromRule(rule); | ||
expect(noInternals).toEqual(getPartialRulesSchemaMock()); | ||
}); | ||
|
||
test('it works with an empty array', () => { | ||
const rule = getPartialRulesSchemaMock(); | ||
rule.tags = []; | ||
const noInternals = removeInternalTagsFromRule(rule); | ||
const expected = getPartialRulesSchemaMock(); | ||
expected.tags = []; | ||
expect(noInternals).toEqual(expected); | ||
}); | ||
|
||
test('it works if tags does not exist', () => { | ||
const rule = getPartialRulesSchemaMock(); | ||
delete rule.tags; | ||
const noInternals = removeInternalTagsFromRule(rule); | ||
const expected = getPartialRulesSchemaMock(); | ||
delete expected.tags; | ||
expect(noInternals).toEqual(expected); | ||
}); | ||
|
||
test('it works if tags contains normal values and no internal values', () => { | ||
const rule = getPartialRulesSchemaMock(); | ||
const noInternals = removeInternalTagsFromRule(rule); | ||
expect(noInternals).toEqual(rule); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ import { buildRiskScoreFromMapping } from './mappings/build_risk_score_from_mapp | |
import { SignalSourceHit } from './types'; | ||
import { buildSeverityFromMapping } from './mappings/build_severity_from_mapping'; | ||
import { buildRuleNameFromMapping } from './mappings/build_rule_name_from_mapping'; | ||
import { INTERNAL_IDENTIFIER } from '../../../../common/constants'; | ||
|
||
interface BuildRuleParams { | ||
ruleParams: RuleTypeParams; | ||
|
@@ -64,7 +65,7 @@ export const buildRule = ({ | |
|
||
const meta = { ...ruleParams.meta, ...riskScoreMeta, ...severityMeta, ...ruleNameMeta }; | ||
|
||
return pickBy<RulesSchema>((value: unknown) => value != null, { | ||
const rule = pickBy<RulesSchema>((value: unknown) => value != null, { | ||
id, | ||
rule_id: ruleParams.ruleId ?? '(unknown rule_id)', | ||
actions, | ||
|
@@ -111,4 +112,17 @@ export const buildRule = ({ | |
anomaly_threshold: ruleParams.anomalyThreshold, | ||
threshold: ruleParams.threshold, | ||
}); | ||
return removeInternalTagsFromRule(rule); | ||
}; | ||
|
||
export const removeInternalTagsFromRule = (rule: Partial<RulesSchema>): Partial<RulesSchema> => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cut/paste from |
||
if (rule.tags == null) { | ||
return rule; | ||
} else { | ||
const ruleWithoutInternalTags: Partial<RulesSchema> = { | ||
...rule, | ||
tags: rule.tags.filter((tag) => !tag.startsWith(INTERNAL_IDENTIFIER)), | ||
}; | ||
return ruleWithoutInternalTags; | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the
rule.id
, notrule.rule_id
right? Curious if there's any upside to using one over the other. Like if users often import/export rules, would we lose tracing by using theid
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, this is
rule.id
. I think for the parents/ancestry we want to know which specific instance of a rule generated the alert soid
is a good choice.We could consider making
signal.parent.rule
an object where we could haveid
,rule_id
, and any otherrule
fields that could be useful insignal.parent.rule.*
, but that would be a breaking change with the existingsignal.parent
andsignal.ancestors
so I think we'd need a migration.