Skip to content

[Bug] [DaC] Metadata maturity field default mismatch and poor enforcement of rule naming conventions #4285

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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions detection_rules/cli_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
DEFAULT_PREBUILT_RULES_DIRS, RuleCollection,
dict_filter)
from .schemas import definitions
from .utils import clear_caches
from .utils import clear_caches, rulename_to_filename


def single_collection(f):
Expand Down Expand Up @@ -95,6 +95,16 @@ def get_collection(*args, **kwargs):
if len(rules) == 0:
client_error("No rules found")

# Warn that if the path does not match the expected path, it will be saved to the expected path
for rule in rules:
threat = rule.contents.data.get("threat")
first_tactic = threat[0].tactic.name if threat else ""
rule_name = rulename_to_filename(rule.contents.data.name, tactic_name=first_tactic)
if rule.path.name != rule_name:
click.secho(
f"WARNING: Rule path does not match required path: {rule.path.name} != {rule_name}", fg="yellow"
)
Comment on lines +104 to +106
Copy link
Contributor

Choose a reason for hiding this comment

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

Is click the best option we have without adding new dependencies for warnings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do not have a logging standard for Detection Rules as far as I am aware, and each entry point for the CLI commands uses click. Other than introducing something new that would break current convention, I do not see a strong alternative, but certainly open to suggestions.


kwargs["rules"] = rules
return f(*args, **kwargs)

Expand Down Expand Up @@ -200,7 +210,19 @@ def rule_prompt(path=None, rule_type=None, required_only=True, save=True, verbos
# DEFAULT_PREBUILT_RULES_DIRS[0] is a required directory just as a suggestion
suggested_path = Path(DEFAULT_PREBUILT_RULES_DIRS[0]) / contents['name']
path = Path(path or input(f'File path for rule [{suggested_path}]: ') or suggested_path).resolve()
meta = {'creation_date': creation_date, 'updated_date': creation_date, 'maturity': 'development'}
# Inherit maturity from the rule already exists
maturity = "development"
if path.exists():
rules = RuleCollection()
rules.load_file(path)
if rules:
maturity = rules.rules[0].contents.metadata.maturity

meta = {
"creation_date": creation_date,
"updated_date": creation_date,
"maturity": maturity,
}

try:
rule = TOMLRule(path=Path(path), contents=TOMLRuleContents.from_dict({'rule': contents, 'metadata': meta}))
Expand Down
19 changes: 15 additions & 4 deletions detection_rules/kbwrap.py
Original file line number Diff line number Diff line change
Expand Up @@ -237,10 +237,21 @@ def kibana_export_rules(ctx: click.Context, directory: Path, action_connectors_d
rule_resource["author"] = rule_resource.get("author") or default_author or [rule_resource.get("created_by")]
if isinstance(rule_resource["author"], str):
rule_resource["author"] = [rule_resource["author"]]
contents = TOMLRuleContents.from_rule_resource(rule_resource, maturity="production")
threat = contents.data.get("threat")
first_tactic = threat[0].tactic.name if threat else ""
rule_name = rulename_to_filename(contents.data.name, tactic_name=first_tactic)
# Inherit maturity from the rule already exists
maturity = "development"
threat = rule_resource.get("threat")
first_tactic = threat[0].get("tactic").get("name") if threat else ""
rule_name = rulename_to_filename(rule_resource.get("name"), tactic_name=first_tactic)
# check if directory / f"{rule_name}" exists
if (directory / f"{rule_name}").exists():
rules = RuleCollection()
rules.load_file(directory / f"{rule_name}")
if rules:
maturity = rules.rules[0].contents.metadata.maturity
Comment on lines +247 to +250
Copy link
Contributor

Choose a reason for hiding this comment

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

I may be reading this wrong.

  1. For a given rule, get the first tactic and build expected file naming convention
  2. If that rule exists, initialize rule collection and load that specific rule
  3. If we successfully load that rule, overwrite maturity with what the rule has statically defined
  4. Overwrite the updated and created dates by formatting what is statically in the fine --> Is this necessary?

Other than my question on 4. it looks good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For question 4, short answer is no. This is not necessary and we may not want to do it, looking for input as to other's thoughts on this. More context from the PR summary above.

There is also an update that we may or may not want to include that is currently in this PR which, in addition to maturity, the creation and modified dates are now inherited from the file if they exist, and if not, the dates from Kibana are used. However, as this represents a potential change to how dates are calculated (before the date is specific to the date that the rule existed on disk in the repo) we may or may not want to include these changes and I would ask reviewers to leave their thoughts on this change in the PR.


contents = TOMLRuleContents.from_rule_resource(
rule_resource, maturity=maturity
)
rule = TOMLRule(contents=contents, path=directory / f"{rule_name}")
except Exception as e:
if skip_errors:
Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[project]
name = "detection_rules"
version = "0.3.19"
version = "0.4.0"
description = "Detection Rules is the home for rules used by Elastic Security. This repository is used for the development, maintenance, testing, validation, and release of rules for Elastic Security’s Detection Engine."
readme = "README.md"
requires-python = ">=3.12"
Expand Down
Loading