search

elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.77k stars 8.17k forks source link

[RAM] [META] Make rule params searchable #123982

Closed XavierM closed 2 years ago

XavierM commented 2 years ago

As a team, we decided how we are going to manage some of our field in the params attribute of our rule saved object to be searchable and sortable. To achieve this functionality, we are going to create a new attribute mapped_params in our rule saved object where each fields inside will be define with a specific schema. Also, we will only add field in mapped_params, if they exist in the params attributes. mapped_params will be hidden from our API meaning that our user won't be able to see it. for example imagine that we had risk_score in our params field and we will create a duplicate field in our mapped_params, if a user asks to sort on params.risk_score behind the scene we will use mapped_params.risk_score. That's the basic idea how we are going to make rule params searchable/sortable.

let's describe, what we need to do to do to get there:

Look at this https://github.com/elastic/kibana/issues/123982#issuecomment-1028308593 instead

elasticmachine commented 2 years ago

Pinging @elastic/response-ops (Team:ResponseOps)

XavierM commented 2 years ago

@banderror, can you provide me with a list of all the params who need to be searchable/sortable and let's go over it together to make sure that we are doing the right thing.

xcrzx commented 2 years ago

@XavierM Here's the list of rule fields that we use for sorting, as of now: https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/public/detections/containers/detection_engine/rules/types.ts#L180-L192

export type RulesSortingFields =
  | 'created_at'
  | 'enabled'
  | 'execution_summary.last_execution.date'
  | 'execution_summary.last_execution.metrics.execution_gap_duration_s'
  | 'execution_summary.last_execution.metrics.total_indexing_duration_ms'
  | 'execution_summary.last_execution.metrics.total_search_duration_ms'
  | 'execution_summary.last_execution.status'
  | 'name'
  | 'risk_score'
  | 'severity'
  | 'updated_at'
  | 'version';

Note that all execution_summary.* fields are not part of the rules params.

xcrzx commented 2 years ago

As for filtering, we'll need to filter rules using the following fields:

More info in this epic: https://github.com/elastic/security-team/issues/1972

banderror commented 2 years ago

@XavierM Just expanding on what @xcrzx posted previously:

Framework-level rule fields (not rule params):

Rule params of our SIEM rule types:

Additional fields that we'd like to store in the rule object which are not rule params but dynamic rule execution data. This is what we currently store in the sidecar siem-detection-engine-rule-execution-info SO. Here we might need to think about consolidating Framework and custom statuses before adding it as is:

Something that we need which is not implemented at all:

This is how alert.params.type corresponds to alert.alertTypeId:

/**
 * Maps the legacy SIEM rule type to the new RAC rule type on the Framework level
 * "alert.params.type" -> "alert.alertTypeId"
 */
export const ruleTypeMappings: {
  eql: "siem.eqlRule";
  machine_learning: "siem.mlRule";
  query: "siem.queryRule";
  saved_query: "siem.savedQueryRule";
  threat_match: "siem.indicatorRule";
  threshold: "siem.thresholdRule";
};

Example value of alert.params.threat:

[
  {
    "framework" : "MITRE ATT&CK",
    "tactic" : {
      "id" : "TA0005",
      "name" : "Defense Evasion",
      "reference" : "https://attack.mitre.org/tactics/TA0005/"
    },
    "technique" : [
      {
        "id" : "T1574",
        "name" : "Hijack Execution Flow",
        "reference" : "https://attack.mitre.org/techniques/T1574/",
        "subtechnique" : [
          {
            "id" : "T1574.007",
            "name" : "Path Interception by PATH Environment Variable",
            "reference" : "https://attack.mitre.org/techniques/T1574/007/"
          }
        ]
      }
    ]
  }
]
banderror commented 2 years ago

Currently, when we filter rules by their fields, the filter we pass to the RulesClient looks like that:

alert.attributes.xyz: value

For us as clients of the Framework, are mapped_params going to be hidden implementation details? Meaning that this field won't be exposed by RulesClient when reading rules, and we'd be filtering mapped params just like any other field:

alert.attributes.riskScore: >= 42 and alert.attributes.riskScore: <= 100
XavierM commented 2 years ago

Team had a discussion around it, we agree to set the expectation on just these fields below inside of params

XavierM commented 2 years ago

I think that's the task around this functionality

spong commented 2 years ago

Not sure if it was discussed yesterday as I had to drop early (and not seeing a comment here about it), but we should be able to clean up all our extra logic around the __internal tags hacks that we had to do to work around these searchable params issues. This is the OG PR they were added in (https://github.com/elastic/kibana/pull/52838), but gist is we store both alert.params.ruleId and alert.params.immutable within the root alert.tags field so we can do find's on these fields. This also has spillover to the Stack Mgmt Rule Mgmt UI's as these internal tags were never hidden from users like they were in security.

So I propose we add alert.params.ruleId as one of our mapped fields as well so we can remove this all this tags:__internal_* logic completely. (I suppose TBD on what happens with alert.params.immutable though as we work through what removing immutability looks like from an architectural perspective).

banderror commented 2 years ago

@spong I think filtering by alert.params.ruleId and alert.params.immutable might work as is because alert.params is mapped as a flattened field type and should support filtering for keywords and booleans. So maybe we could remove __internal tags without explicitly mapping these two fields?

spong commented 2 years ago

Oh that's right, I keep forgetting they're mapped as flattened now. Thanks @banderror! 🙂 In that case we should be able to clean up all our extra business logic around __internal without additional changes. I'll create a chore and add to our backlog (https://github.com/elastic/kibana/issues/124899) since this won't require field changes as part of this issue now. 👍

banderror commented 2 years ago

Trying to decompose my previous comment into tickets, I opened these:

And we already had this one:

JiaweiWu commented 2 years ago

I have created a draft PR with the POC implementation (https://github.com/elastic/kibana/pull/125889). In the implementation, I've turned on sorting on the risk score and severity columns. So now they're sorted and filtered via the mapped_params property.

Some challenges that I foresee with this mapped params system mostly has to do with developer/user discoverability. Every time someone wants to promote a params to a searchable/mapped param, we would need to manually add the param to the mapped params mapping allowlist. Alongside that, how are users going to know if something is searchable? We'd have to document the mapped params system without exposing the field itself. Maybe some UX considerations would be needed (some sort of auto complete?)

Now I'm working on making the change to aggregates, will report back with findings.

gmmorris commented 2 years ago

Thanks @JiaweiWu !

Every time someone wants to promote a params to a searchable/mapped param, we would need to manually add the param to the mapped params mapping allowlist.

My understanding was that this was the intended state as we were concerned that if we didn't do this we will end up with a big, hard to maintain, bucket of unrelated params. We wanted to audit every param that is "elevated" to make sure we prevent this from becoming a big mess.

If we find this is a major blocker to progress, we can treat that as a signal that it is worth while investigating a more expensive, but longer-term, approach. Does this match your thinking @XavierM and @mikecote ?

Alongside that, how are users going to know if something is searchable? We'd have to document the mapped params system without exposing the field itself. Maybe some UX considerations would be needed (some sort of auto complete?)

Yes, I have voiced the same concern. I think we should go the UX route as IMHO users shouldn't have to go to the doc to figure this out (this will also avoid unnecessary SDHs). I'd like to hear design's thoughts here (@ryankeairns @mdefazio ), but my ideal is that we provide some sort of autocomplete experience in a search field. Don't we have an equivalent in Discover for fields? 🤔

gmmorris commented 2 years ago

@XavierM @mikecote - what's our plan for the POC? What do we need to feel confident this POC has validated the idea and we can begin productionising the implementation?

I'd like it if we could set some timeframe, so that we can make sure we can begin to tackle this list in a timely manner. 😊

ryankeairns commented 2 years ago

Alongside that, how are users going to know if something is searchable? We'd have to document the mapped params system without exposing the field itself. Maybe some UX considerations would be needed (some sort of auto complete?)

Yes, I have voiced the same concern. I think we should go the UX route as IMHO users shouldn't have to go to the doc to figure this out (this will also avoid unnecessary SDHs). I'd like to hear design's thoughts here (@ryankeairns @mdefazio ), but my ideal is that we provide some sort of autocomplete experience in a search field. Don't we have an equivalent in Discover for fields? 🤔

Surprisingly, we don't have much to lean on here other than the KQL search bar.

We do leverage this key:value sort of solution, but I have not found other places where we do that in conjunction with suggestions/autocomplete (i.e. it may only hint at its existence in placeholder text which is not scalable). That aside, there is the EuiSuggest component (docs) which provides a UI, so perhaps it could be built using this and behave similar to the KQL search bar.

For example:

  1. User starts typing p and the autosuggest shows params.x, params,y, and params.z
  2. User types or selects params.x and autosuggest then shows :
  3. User types : and autosuggest shows possible values for the param

Here's a set of screenshots demonstrating how this looks in Discover:

Screen Shot 2022-02-23 at 7 35 35 PM Screen Shot 2022-02-23 at 7 35 40 PM Screen Shot 2022-02-23 at 7 35 46 PM Screen Shot 2022-02-23 at 7 35 54 PM

Other than this, we only hint at things like:

Screen Shot 2022-02-23 at 7 25 06 PM Screen Shot 2022-02-23 at 7 41 21 PM

@mdefazio feel free to add other examples if I missed something

mdefazio commented 2 years ago

I can't think of additional relevant examples. But I think the KQL example makes sense. We could add params: as a hint like Ryan showed similar to type: and tag:. This then makes the idea that params in general are at least a bit more discoverable as a searchable key:value pair. I'm not sure I understand the steps for a developer wanting additional params added to the mapped / searchable list. Assuming these could / would be rule specific? What's the auditing process on 'elevated' params? Do we think it would frustrate users/developers if we put a cap on which params are searchable—"I see params are searchable, but I've added params and am not seeing them in the results"?