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

[Change Proposal] Allow security_rule objects to have rule IDs different from the object IDs #459

Closed
Tracked by #174166
xcrzx opened this issue Jan 10, 2023 · 2 comments · Fixed by #463
Closed
Tracked by #174166
Labels
discuss Issue needs discussion

Comments

@xcrzx
Copy link
Contributor

xcrzx commented Jan 10, 2023

Security Solution migrates the detection rules package from storing a single saved object per rule to multiple saved objects. See elastic/kibana#137420 for more context regarding the change.

The package will contain rule saved objects with rule id and version in the name (security_rule/[ruleId]_[ruleVersion].json) with the following content:

{
  "id": "[ruleId]_[ruleVersion]",
  "type": "security-rule",
  "attributes": {
    "rule_id": "[ruleId]",
    "version": "[ruleVersion]",
    // Other rule attributes
  }
}

So the saved object ID and the rule ID do not match anymore, making this validation check always return an error:

if ruleID != objectID {
errs = append(errs, errors.New("rule ID is different from the object ID"))
continue
}

I would like to know why that validation exists in the first place and if we could remove or update it.

@xcrzx xcrzx added the discuss Issue needs discussion label Jan 10, 2023
@xcrzx xcrzx changed the title [Change Proposal] Allow security_rule objects to rule IDs different from the object IDs [Change Proposal] Allow security_rule objects to have rule IDs different from the object IDs Jan 10, 2023
@spong
Copy link
Member

spong commented Jan 11, 2023

I would like to know why that validation exists in the first place and if we could remove or update it.

So when this check was first added, @rw-access (who did the original implementation on the security side) commented:

There's no reason these have to match.

The rule_id is only relevant to the Security solution.
The top level id is the saved object ID. It's possible that in the future we want better named SO ids. But we can loosen up this spec up then if that happens.

That said, as we were discussing earlier, there is some inherent use to this check as it ensures that all security-rule assets included as part of an integration are guaranteed to have a unique rule_id, a requirement of the Security Solution app. If rule_id must equal id, and the SO filename must be the id, and since all SO's are stored in the same directory, you can't have duplicates, and so can't have duplicate rule_id's. Turns out this ended up being a roundabout and accidental safety check it seems.

I'd say Ross's comment stands then, as this check doesn't need to be happening this way, and can be removed. Security Solution needs to handle the case correctly where there are multiple security-rule assets with the same rule_id, but it would still be nice if there was some guard in place at the package layer that prevented developers from shipping packages that would be known to have invalid rules that would result in a more complicated UX (making the user pick which rule to install or something).

Please see this related discussion (elastic/kibana#128202) with regards to enforcing unique rule_id's among a package's assets, which originally surfaced when we started shipping additional packages that contain security-rule assets (i.e. LotL Attack Detection package).

@xcrzx
Copy link
Contributor Author

xcrzx commented Jan 12, 2023

I'd say Ross's comment stands then, as this check doesn't need to be happening this way, and can be removed. Security Solution needs to handle the case correctly where there are multiple security-rule assets with the same rule_id, but it would still be nice if there was some guard in place at the package layer that prevented developers from shipping packages that would be known to have invalid rules that would result in a more complicated UX (making the user pick which rule to install or something).

So with the introduction of historical rule versions, rule_ids will no longer be unique within a single package as we're starting to ship multiple saved objects per rule, all of which will have the same rule_id. So that draws the uniqueness check redundant. We should probably leave it up to package authors to ensure they don't ship rules with the same rule_ids.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issue needs discussion
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants