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][Detections] Adds Indicator path config for indicator match rules #91260

Merged
merged 9 commits into from
Feb 17, 2021

Conversation

rylnd
Copy link
Contributor

@rylnd rylnd commented Feb 12, 2021

Summary

This adds a new rule field, threat_indicator_path, that applies only to indicator match rules. Much like an ingest processor, users can use this field to define where their threat indicator can be found on their indicator documents. The value defaults to threat.indicator, the proposed ECS path, if unspecified.

Regardless of where the enrichment fields are found, they will be placed within the prospective ECS field, threat.indicator, as a nested object in the generated detection alert.

This configuration is critical for 7.12 to allow users to receive enrichments from the new filebeat modules, which currently do not conform to the proposed ECS but instead use threatintel.indicator.

Reviewer Notes:

In general: create an indicator index, create a rule, generate some events, and observe the resultant, enriched signals. Specifically:

  1. Generate a custom indicator index:

    PUT /custom_indicators
    {
      "mappings": {
        "properties": {
          "@timestamp": {
            "type": "date"
          },
          "custom_indicator": {
            "properties": {
              "path": {
                "properties": {
                  "domain": {
                    "type": "keyword"
                  },
                  "description": {
                    "type": "text"
                  }
                }
              }
            }
          }
        }
      }
    }
  2. Create some indicator data. The simplest approach here is to create an indicator(s) with some known value (e.g. your computer's hostname) that we can leverage later. In dev tools:

    POST custom_indicators/_doc
    {
      "@timestamp": "2021-02-10T00:41:06.527Z",
      "custom_indicator": {
        "path": {
          "domain": "my-laptop-hostname.local",
          "description": "This should also appear in the alert's threat.indicator!"
        }
      }
    }
  3. Generate source events with values matching your indicator fields above. As an example, I run auditbeat, knowing that it will generate events with our laptop's hostname in host.name. However, you can also manually insert some documents into an index of your own creation; just ensure that they contain values that will cause the rule to generate a signal.

  4. Create an indicator match rule against your indicator index and event data:

40f208d0-6a6f-11eb-b40c-7157997aa0ef_-_Kibana

40f208d0-6a6f-11eb-b40c-7157997aa0ef_-_Kibana

  1. Peep those enriched signals:
    Note: we're temporarily using dev tools here due to some ongoing fields display issues, and the _source filter is for demo cleanliness, please remove
    GET .siem-signals-default/_search
    {
      "query": {
        "term": {
          "signal.rule.name": {
            "value": "Review Rule Name"
          }
        }
      },
      "_source": "threat.indicator"
    }
Dev_Tools_-_Elastic

In the interest of getting this reviewed/merged I'm going to defer integration/UI tests to a subsequent PR.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

There is no UI for this currently, nor is it used during rule execution.
Also adds missing plumbing that was preventing the new field from being
persisted to the alert/returned in the response.
We always persist to `threat.indicator.*` on the signal, but this allows
users to specify where the enrichment fields can be found on the matched
indicator document.
We were not passing this from the rule itself into the threat matching
logic, and so were merely getting the default value.

An integration test will fix this. Incoming!
This happens closer to where we pass data from the rule to our helpers,
and will prevent errors/bugs due to defaulting logic down the road.

It makes tests a little more verbose, but that's okay.
@rylnd rylnd added release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detections and Resp Security Detection Response Team v7.12.0 v8.0.0 labels Feb 12, 2021
@rylnd rylnd self-assigned this Feb 12, 2021
Always sending along this field, but only allowing it for threat match
rules was implicitly breaking the workflow of otther rule types. By
making the field conditional on the rule type, this field only impacts
threat match rules.

This also fixes some types and tests accordingly.
@rylnd
Copy link
Contributor Author

rylnd commented Feb 16, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 7.5MB 7.5MB +1.7KB

History

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

@rylnd rylnd marked this pull request as ready for review February 16, 2021 14:42
@rylnd rylnd requested a review from a team as a code owner February 16, 2021 14:42
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detections-response (Team:Detections and Resp)

};
const result = formatAboutStepData(mockData);
expect(result.threat_indicator_path).toEqual('my_custom.path');
});
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 You have some testing! Thanks

const enrichedSignals = await enrichSignalThreatMatches(
signals,
getMatchedThreats,
'threat.indicator'
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Suggest using a constant everywhere and put it in the common/constants area we have

Copy link
Contributor

@FrankHassanabad FrankHassanabad left a comment

Choose a reason for hiding this comment

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

LGTM! Good straight forward plumbing.

@rylnd rylnd merged commit 5b97d52 into elastic:master Feb 17, 2021
@rylnd rylnd deleted the indicator_path_config branch February 17, 2021 00:25
rylnd added a commit to rylnd/kibana that referenced this pull request Feb 17, 2021
…tor match rules (elastic#91260)

* Add new field for overriding threat indicator path

There is no UI for this currently, nor is it used during rule execution.

* Adds form field for indicator path parameter

Also adds missing plumbing that was preventing the new field from being
persisted to the alert/returned in the response.

* Wire up our indicator path config to enrichment

* Add unit test for enriching from a custom indicator path

We always persist to `threat.indicator.*` on the signal, but this allows
users to specify where the enrichment fields can be found on the matched
indicator document.

* Wire up the missing piece of our indicator path config

We were not passing this from the rule itself into the threat matching
logic, and so were merely getting the default value.

An integration test will fix this. Incoming!

* Move indicator path defaulting outside of helper functions

This happens closer to where we pass data from the rule to our helpers,
and will prevent errors/bugs due to defaulting logic down the road.

It makes tests a little more verbose, but that's okay.

* Fix remaining type errors around new rule field

* Make threat indicator path a conditional field

Always sending along this field, but only allowing it for threat match
rules was implicitly breaking the workflow of otther rule types. By
making the field conditional on the rule type, this field only impacts
threat match rules.

This also fixes some types and tests accordingly.

Co-authored-by: Kibana Machine <[email protected]>
rylnd added a commit that referenced this pull request Feb 17, 2021
…tor match rules (#91260) (#91593)

* Add new field for overriding threat indicator path

There is no UI for this currently, nor is it used during rule execution.

* Adds form field for indicator path parameter

Also adds missing plumbing that was preventing the new field from being
persisted to the alert/returned in the response.

* Wire up our indicator path config to enrichment

* Add unit test for enriching from a custom indicator path

We always persist to `threat.indicator.*` on the signal, but this allows
users to specify where the enrichment fields can be found on the matched
indicator document.

* Wire up the missing piece of our indicator path config

We were not passing this from the rule itself into the threat matching
logic, and so were merely getting the default value.

An integration test will fix this. Incoming!

* Move indicator path defaulting outside of helper functions

This happens closer to where we pass data from the rule to our helpers,
and will prevent errors/bugs due to defaulting logic down the road.

It makes tests a little more verbose, but that's okay.

* Fix remaining type errors around new rule field

* Make threat indicator path a conditional field

Always sending along this field, but only allowing it for threat match
rules was implicitly breaking the workflow of otther rule types. By
making the field conditional on the rule type, this field only impacts
threat match rules.

This also fixes some types and tests accordingly.

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Feb 17, 2021
* master: (157 commits)
  [DOCS] Adds machine learning to the security section of alerting (elastic#91501)
  [Uptime] Ping list step screenshot caption formatting (elastic#91403)
  [Vislib] Use timestamp on brush event instead of iso dates (elastic#91483)
  [Application Usage] Remove deprecated & unused legacy.appChanged API (elastic#91464)
  Migrate logstash, monitoring, url_drilldowns, xpack_legacy to ts projects (elastic#91194)
  [APM] Wrap Elasticsearch client errors (elastic#91125)
  [APM] Fix optimize-tsconfig script (elastic#91487)
  [Discover][docs] Add searchFieldsFromSource description (elastic#90980)
  Adds support for 'ip' data type (elastic#85087)
  [Detection Rules] Add updates from 7.11.2 rules (elastic#91553)
  [SECURITY SOLUTION] Eql in timeline (elastic#90816)
  [APM] Correlations Beta (elastic#86477) (elastic#89952)
  [Security Solutions][Detection Engine] Adds a warning banner when the alerts data has not been migrated yet. (elastic#90258)
  [Security Solution] [Timeline] Endpoint row renderers (2nd batch) (elastic#91446)
  skip flaky suite (elastic#91450)
  skip flaky suite (elastic#91592)
  [Security Solution][Endpoint][Admin] Endpoint Details UX Enhancements (elastic#90870)
  [ML] Add better UI support for runtime fields Transforms  (elastic#90363)
  [Security Solution] [Detections] Replace 'partial failure' with 'warning' for rule statuses (elastic#91167)
  [Security Solution][Detections] Adds Indicator path config for indicator match rules (elastic#91260)
  ...
@MikePaquette
Copy link

Thanks @rylnd for pulling this together.

Copy: any suggestions for something more descriptive than “Threat Indicator Path”? That’s the name of the underlying field, but it occurred to me that something like “Indicator Path Override” might be more self-explanatory, although that brought up further questions about threat/indicator terminology and how we switched from “threat match” to “indicator” match. Also looking for suggestions on the description text as well.

Generally the dialog in the rule create/edit steps should follow the naming of the rule type, which in this case is "indicator match", so I would vote for using "Indicator" rather than "Threat Indicator." (We expect that this rule can/will be used for matching other indicators besides threat indicators.)

Question, should we use "path" in this context? in ECS, the "path" concept is used for entities such as file system paths, but not generally for fields, field sets, and objects in an ECS-compatible document. In this case, it seems like we are asking the user to specify "where is the field set we should use to find the attributes of the indicator we're matching?" Thus I'm wondering if "Indicator field set" would be better.

The description could be adjusted accordingly, like "Specify the field set in which attributes of the indicator are located. These attributes will be used to enrich alerts that are generated by this rule. Defaults to threat.indicator.

bigger questions
I'm just sipping my first cup of tea, but trying to wrap my head around whether we actually need a separate field for this information in the first place? Is it truly an override? Would it make a simpler user experience if we derived it from the Indicator index field specified in Step 1? For example, if the rule_author specifies threatintel.indicator.ip as the indicator index field, we'd just use threatintel.indicator prefix as the path/field set from which to pull the indicator attributes when enriching the signal document. Similarly, if the data complied with ECS, and the rule_author specified threat.indicator.ip as the indicator index field, then we'd pull the enrichment information from the threat.indicator.* field set. Then having this new specification as an override would make sense - normally not needed, but if there's a use case, we're not thinking about, could give flexibility.

Also, do we need an override field to specify the similar path/field set where the enrichment information will be stored in the alert document? Again, keeping in mind that this rule type is a generic indicator match, and future ECS field sets might define other indicator enrichments that would also be stored in the alert document, we should allow that flexibility. Again, we could infer this value based on what is specified in the indicator index field, but allow an override. For example, if the rule_author specifies threatintel.indicator.ip or threat.indicator.ip as the indicator index field, we'd use threat.indicator.* as the path/field set to copy the indicator attributes to when enriching the signal document.

@MikePaquette
Copy link

@rylnd wondering if you had the chance to see my comments and questions above?

@rylnd
Copy link
Contributor Author

rylnd commented Feb 22, 2021

Question, should we use "path" in this context? in ECS, the "path" concept is used for entities such as file system paths, but not generally for fields, field sets, and objects in an ECS-compatible document. In this case, it seems like we are asking the user to specify "where is the field set we should use to find the attributes of the indicator we're matching?" Thus I'm wondering if "Indicator field set" would be better.

"path" was fresh in my mind as that's what's used for nested queries, but I appreciate using terminology from ECS if possible👍 . My understanding of "field set" is it's literally "a set of fields," often (always?) sharing a common ancestor, rather than the location of that common ancestor as we're trying to describe.

bigger questions
I'm just sipping my first cup of tea, but trying to wrap my head around whether we actually need a separate field for this information in the first place? Is it truly an override? Would it make a simpler user experience if we derived it from the Indicator index field specified in Step 1? For example, if the rule_author specifies threatintel.indicator.ip as the indicator index field, we'd just use threatintel.indicator prefix as the path/field set from which to pull the indicator attributes when enriching the signal document. Similarly, if the data complied with ECS, and the rule_author specified threat.indicator.ip as the indicator index field, then we'd pull the enrichment information from the threat.indicator.* field set. Then having this new specification as an override would make sense - normally not needed, but if there's a use case, we're not thinking about, could give flexibility.

I really like this idea but I'm not sure it's possible without further restricting the structure of an indicator document. Currently the indicator fieldset can exist anywhere in a document (i.e. has n levels above it), and can itself have arbitrary data within it (n levels below it). I don't think we could derive threat.indicator from something like threat.indicator.file.hash unless we restricted the definition to e.g. "fieldset must end with indicator" or something like that.

Additionally, such a derivation would force documents to colocate both indicator values and enrichment metadata; while the current examples assume that the rule is matching within the indicator fieldset (e.g. threat.indicator.domain), there's nothing enforcing this behavior and the user can match on any arbitrary field within the indicator document.

Also, do we need an override field to specify the similar path/field set where the enrichment information will be stored in the alert document? Again, keeping in mind that this rule type is a generic indicator match, and future ECS field sets might define other indicator enrichments that would also be stored in the alert document, we should allow that flexibility. Again, we could infer this value based on what is specified in the indicator index field, but allow an override. For example, if the rule_author specifies threatintel.indicator.ip or threat.indicator.ip as the indicator index field, we'd use threat.indicator.* as the path/field set to copy the indicator attributes to when enriching the signal document.

This is a great point! As we start to provide other enrichment types we may very well need more configuration (or simply more rule types?). For now, the thought was that these rules would be used primarily for CTI and would thus populate threat.indicator.

TL;DR I'm leaning toward "Indicator path override"

spong added a commit to elastic/security-docs that referenced this pull request Mar 9, 2021
… Rules (#522)

This is a rough pass at updating the [Create Rule API docs](https://www.elastic.co/guide/en/security/current/rules-api-create.html#_request_body) for the recent changes surrounding `Threshold` and `Indicator Match` rules. @madirey and @rylnd if you could verify these changes and if any additional fields should be documented that would be greatly appreciated. 🙂 

Resolves: elastic/kibana#91965

Note: The grouping of optional fields per rule type makes these docs difficult to maintain (for me at least 😅), as you need to cross reference each optional field from each type, plus I'd bargain our users might find it useful to be able to look at all fields per each rule type independently. Something to verify with users/discuss, but would be a nice add for maintaining these docs (outside of writing scripts to generate the groupings).


##### Threshold Rules:
For more details on `cardinality_field` and `cardinality_value` see: elastic/kibana#90826


##### Indicator Match Rules:

For more details on `threat_filters` see: elastic/kibana#91260
spong added a commit to spong/security-docs that referenced this pull request Mar 9, 2021
… Rules (elastic#522)

This is a rough pass at updating the [Create Rule API docs](https://www.elastic.co/guide/en/security/current/rules-api-create.html#_request_body) for the recent changes surrounding `Threshold` and `Indicator Match` rules. @madirey and @rylnd if you could verify these changes and if any additional fields should be documented that would be greatly appreciated. 🙂 

Resolves: elastic/kibana#91965

Note: The grouping of optional fields per rule type makes these docs difficult to maintain (for me at least 😅), as you need to cross reference each optional field from each type, plus I'd bargain our users might find it useful to be able to look at all fields per each rule type independently. Something to verify with users/discuss, but would be a nice add for maintaining these docs (outside of writing scripts to generate the groupings).


##### Threshold Rules:
For more details on `cardinality_field` and `cardinality_value` see: elastic/kibana#90826


##### Indicator Match Rules:

For more details on `threat_filters` see: elastic/kibana#91260
spong added a commit to spong/security-docs that referenced this pull request Mar 9, 2021
… Rules (elastic#522)

This is a rough pass at updating the [Create Rule API docs](https://www.elastic.co/guide/en/security/current/rules-api-create.html#_request_body) for the recent changes surrounding `Threshold` and `Indicator Match` rules. @madirey and @rylnd if you could verify these changes and if any additional fields should be documented that would be greatly appreciated. 🙂 

Resolves: elastic/kibana#91965

Note: The grouping of optional fields per rule type makes these docs difficult to maintain (for me at least 😅), as you need to cross reference each optional field from each type, plus I'd bargain our users might find it useful to be able to look at all fields per each rule type independently. Something to verify with users/discuss, but would be a nice add for maintaining these docs (outside of writing scripts to generate the groupings).


##### Threshold Rules:
For more details on `cardinality_field` and `cardinality_value` see: elastic/kibana#90826


##### Indicator Match Rules:

For more details on `threat_filters` see: elastic/kibana#91260
spong added a commit to elastic/security-docs that referenced this pull request Mar 9, 2021
… Rules (#522) (#540)

This is a rough pass at updating the [Create Rule API docs](https://www.elastic.co/guide/en/security/current/rules-api-create.html#_request_body) for the recent changes surrounding `Threshold` and `Indicator Match` rules. @madirey and @rylnd if you could verify these changes and if any additional fields should be documented that would be greatly appreciated. 🙂 

Resolves: elastic/kibana#91965

Note: The grouping of optional fields per rule type makes these docs difficult to maintain (for me at least 😅), as you need to cross reference each optional field from each type, plus I'd bargain our users might find it useful to be able to look at all fields per each rule type independently. Something to verify with users/discuss, but would be a nice add for maintaining these docs (outside of writing scripts to generate the groupings).


##### Threshold Rules:
For more details on `cardinality_field` and `cardinality_value` see: elastic/kibana#90826


##### Indicator Match Rules:

For more details on `threat_filters` see: elastic/kibana#91260
spong added a commit to elastic/security-docs that referenced this pull request Mar 9, 2021
… Rules (#522) (#539)

This is a rough pass at updating the [Create Rule API docs](https://www.elastic.co/guide/en/security/current/rules-api-create.html#_request_body) for the recent changes surrounding `Threshold` and `Indicator Match` rules. @madirey and @rylnd if you could verify these changes and if any additional fields should be documented that would be greatly appreciated. 🙂 

Resolves: elastic/kibana#91965

Note: The grouping of optional fields per rule type makes these docs difficult to maintain (for me at least 😅), as you need to cross reference each optional field from each type, plus I'd bargain our users might find it useful to be able to look at all fields per each rule type independently. Something to verify with users/discuss, but would be a nice add for maintaining these docs (outside of writing scripts to generate the groupings).


##### Threshold Rules:
For more details on `cardinality_field` and `cardinality_value` see: elastic/kibana#90826


##### Indicator Match Rules:

For more details on `threat_filters` see: elastic/kibana#91260
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants