Terms query for Indicator Match rule

[nkhristinin]

Terms query for Indicator Match rule

TODO: [] need more unit/integrations tests, but ready for review

The indicator match rule will use terms query when it is possible to search for matches for threat-first-search and for events-first-search.

How the match query worked:

Example for threat-first-search.
If we have matching conditions like:
host.ip ==== indicator.host.ip or (source.name === indicator.source.name AND host.name === indicator.host.name)

It will generate queries like:

match: {host.ip: "1"},  
or
match: {host.ip: "2"}
or
match: {host.ip: "3"}
or
(match: {source.name: "1"} and match: {host.name: "1"})
or
(match: {source.name: "2"} and match: {host.name: "2"})
or
(match: {source.name: "3"} and match: {host.name: "3"})

Each match will also have _name fields like: ${threatId}_${threatIndex}_${threatFields}_${sourceField}
So and because it's 1:1 relation between match and response, later at enrichment stage will be clear which threat matches which event.

Terms query.

We do fetch info about mapping for fields which use for match conditions of the IM rule.
Terms query doesn't support all field types, this is why there is some allowed list which field types.
Terms query not applied for AND conditions.

For example:
Fields types

host.ip - ip
user.name - keyword
user.description - text
indicator.host.ip_range - ip_range

host.ip === indicator.host.ip or host.ip_range === indicator.host.ip or (source.name === indicator.source.name AND host.name === indicator.host.name)

It will generate queries like:

terms: {host.ip: ["1","2","3"]},  
or
match: {host.ip_range: "1"} // terms query support range fields, but it will be difficult later to understand which threat match which event, because we can have more than 1 response for this condition
or
match: {host.ip_range: "2"}
or
(match: {source.name: "1"} and match: {host.name: "1"})
or
(match: {source.name: "2"} and match: {host.name: "2"})
or
(match: {source.name: "3"} and match: {host.name: "3"})

For terms query, we don't know which response matches with events, this is why we do match it back in the code.

Other changes

Threat-first-search - will do one extra request to have all matched threats.
For example:
The threat index has 1.000.000 documents.
IM rule gets the first batch of 9.000 threats and builds a query to the events index.
It returns 100 events (max_signal = 100).
Then it tries to enrich those 100 events with threat info.
The problem is that the original implementation will enrich with the only threats from this 9.000 batch.
And it will ignore other matches in 1.000.000 threats.

This way we do one extra request in the end from potential alerts to threat index.

Tests performance

In the best case, it can improve performance by around 3x times.

Base
Threat Indicators - 1.500.000 documents
Source - 1.000.000 documents.
1 field for match condition

This PR:

[nkhristinin]

@elasticmachine merge upstream

[nkhristinin]

@elasticmachine merge upstream

[nkhristinin]

@elasticmachine merge upstream

[nkhristinin]

@elasticmachine merge upstream

[nkhristinin]

@elasticmachine merge upstream

[vitaliidm]

why we started to need casting here? Can it be avoided?

[nkhristinin]

Thanks for notice!

Removed whole uniqueHits.map((signalHit) => ({ signalId: signalHit._id, queries: extractNamedQueries(signalHit), queries: extractNamedQueries(signalHit) as ThreatMatchNamedQuery[], sections, because it's not used anymore

[vitaliidm]

nit:
if wrap comment in JSDoc style, it will be highlighted in place where function is imported.
Will make slightly easier to read code

/**
 * Return map of fields allowed for term query for source and threat indices
 */

[vitaliidm]

@nkhristinin, I've seen you've made this change in some of places, but not others. Would be great if it can be done for other methods for consistency.

[vitaliidm]

Suggested change

	it('return empy object if there some events but no fields', () => {
	it('return empty object if there some events but no fields', () => {

[vitaliidm]

curious, why this check was removed?

[vitaliidm]

does this comment refer to a case, when source's field value could be array?
So, it will be needed to find in that array matched value?

[nkhristinin]

Fixed it

[vitaliidm]

Suggested change

	const indicies = Object.values(indexMapping);
	indicies.forEach((index) => {
	const indices = Object.values(indexMapping);
	indices.forEach((index) => {

[vitaliidm]

why notAllowedTypes is needed? It looks like only check in allowedFieldTypes should be sufficient, because the only way type will be added into notAllowedTypes when it's not in allowedFieldTypes.

as per code below

        if (allowedFieldTypes.includes(fieldType) && !notAllowedTypes.includes(fieldType)) {
          result[field] = true;
        } else {
          notAllowedTypes.push(fieldType);
         ...
        }

[nkhristinin]

Great catch.

Actually, the idea was different indices contain fields, and one of the mappings is not supported by term query we should remove this field, and not add it later.

I fixed the code and change notAllowedTypes to notAllowedFields

[vitaliidm]

putting both requests in Promise.all would allow not wait until the first request is finished, to execute second one

[vitaliidm]

why it becomes unknown? is there according type for should clause? Probably QueryDslQueryContainer?

[vitaliidm]

Probably it should be QueryDslQueryContainer?

[nkhristinin]

@elasticmachine merge upstream

[vitaliidm]

Suggested change

	const singnalValueMap = {
	const signalValueMap = {

[nkhristinin]

@elasticmachine merge upst

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: c5ff4eb
• Cloud Deployment

Metrics [docs]

✅ unchanged

History

• 💔 Build #105085 failed 05754fe
• 💔 Build #104999 failed add3153
• 💚 Build #104982 succeeded 6c44b70
• 💚 Build #104821 succeeded a4cca24
• 💔 Build #104591 failed 35b10b7

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream