THREAT-395 Correlation Rule Style Guide in repo #1376
Conversation
|
Should we create a |
CORRELATION_RULES_STYLE_GUIDE.md
Outdated
|
|
||
| This style guide highlights essential best practices for writing correlation rules. For a more detailed guide, visit [Correlation Rules](https://docs.panther.com/detections/correlation-rules) in the Panther documentation. | ||
|
|
||
| ## General advises |
There was a problem hiding this comment.
I would maybe use "guidelines" instead of "advises"
CORRELATION_RULES_STYLE_GUIDE.md
Outdated
|
|
||
| ## General advises | ||
|
|
||
| - Transition names in rule IDs, and transition IDs should be all caps. |
There was a problem hiding this comment.
- comma not necessary
- "be in all-caps"
- this might be just my personal preference, but using "Transition names" here is a little confusing because it makes me wonder whether
Transition.Nameis an actual Transition field, like ID. you could rephrase to say, "In a rule ID or transition ID, when describing a transition, put that portion of the ID in all-caps."
CORRELATION_RULES_STYLE_GUIDE.md
Outdated
| @@ -0,0 +1,70 @@ | |||
| # panther-analysis Correlation Rule (CR) Style Guide and Best Practices | |||
|
|
|||
| This style guide highlights essential best practices for writing correlation rules. For a more detailed guide, visit [Correlation Rules](https://docs.panther.com/detections/correlation-rules) in the Panther documentation. | |||
There was a problem hiding this comment.
I loved that you added the acronym "(CR)," but we typically don't add those in titles. so like the title would just say Correlation Rule, and the first use of it (in this sentence) would have (CR) after
CORRELATION_RULES_STYLE_GUIDE.md
Outdated
| - e.g. `"GitHub Advanced Security Change NOT FOLLOWED BY repo archived"` | ||
| - Boilerplate comments from the CR template in the UI should be removed before committing to PA. | ||
| - e.g. remove `# Create a list of rules to correlate` | ||
| - Sequence, Group, and Transition IDs should have meaningful names. |
There was a problem hiding this comment.
in the docs, we don't capitalize group or transition (or sequence). the more important thing, though, is consistency—if you are capitalizing transition here, e.g., be sure to do it everywhere
CORRELATION_RULES_STYLE_GUIDE.md
Outdated
| - e.g. remove `# Create a list of rules to correlate` | ||
| - Sequence, Group, and Transition IDs should have meaningful names. | ||
| - e.g. `- ID: GHASChange` instead of `- ID: TR.1` | ||
| - Check for Signal rules that already exist. |
There was a problem hiding this comment.
should this be a top-level bullet instead of a sub-bullet of line 11?
CORRELATION_RULES_STYLE_GUIDE.md
Outdated
|
|
||
| ### Testing | ||
|
|
||
| You must run `pat validate` against a live Panther instance to test, `pat test` is not sufficient. |
There was a problem hiding this comment.
To test your correlation rules before uploading them, run pat validate against a live Panther instance. Running pat test is not sufficient.
CORRELATION_RULES_STYLE_GUIDE.md
Outdated
| - Guidance for multi-logtype pack is do not enable the pack, just enable the individual detections you have logs for | ||
| - A pack with CRs should also contain the sub-rules referenced by those CRs | ||
| - AND any globals, data models, etc. that the sub-rules reference | ||
| - can the pack-checker be updated to check for these dependencies? |
CORRELATION_RULES_STYLE_GUIDE.md
Outdated
|
|
||
| ### Placing CRs in packs | ||
|
|
||
| - If a CR only references 1 LogType it can go in that LogType’s pack |
There was a problem hiding this comment.
"one" instead of "1"
ref: https://developers.google.com/style/numbers#numbers-as-words
There was a problem hiding this comment.
instead of "it can go in" maybe say "store it in"
CORRELATION_RULES_STYLE_GUIDE.md
Outdated
| ### WithinTimeFrameMinutes | ||
|
|
||
| - `WithinTimeFrameMinutes` is not required and defaults to `LookbackWindowMinutes`. | ||
| - Omit `WithinTimeFrameMinutes` in Sequences with only 2 rules |
There was a problem hiding this comment.
lower-case sequences, spell out two
CORRELATION_RULES_STYLE_GUIDE.md
Outdated
|
|
||
| ### Match On | ||
|
|
||
| - `Match On` fields are not required to be `p_` fields. Especially for CRs where each subrule is for the same LogType, use the regular field instead of the alert context field. |
There was a problem hiding this comment.
in other places you included a dash in "sub-rules"
CORRELATION_RULES_STYLE_GUIDE.md
Outdated
|
|
||
| - If a CR only references 1 LogType it can go in that LogType’s pack | ||
| - If a CR spans multiple LogTypes, put it in the multi-logtype pack | ||
| - Guidance for multi-logtype pack is do not enable the pack, just enable the individual detections you have logs for |
There was a problem hiding this comment.
maybe make clearer here that you are sharing guidance for end-users, not p-a contributors
like "Customers are advised not to enable multi-LogType packs, and to instead enable the individual detections within them that they have log sources set up for."
CORRELATION_RULES_STYLE_GUIDE.md
Outdated
|
|
||
| ### MinMatchCount | ||
|
|
||
| `MinMatchCount: 1` is the default value, this can be omitted. |
There was a problem hiding this comment.
in other sections within "Correlation rule fields," the body is formatted as bullets. I would suggest making this line a bullet, too
There was a problem hiding this comment.
suggested rewrite: If you are setting MinMatchCount to 1, you can omit it completely, as 1 is the default value.
CORRELATION_RULES_STYLE_GUIDE.md
Outdated
|
|
||
| ### LookbackWindowMinutes | ||
|
|
||
| - `LookbackWindowMinutes: 15` is the default value, but this feels valuable to keep. |
There was a problem hiding this comment.
suggested rewrite: For ease of understanding, it's valuable to include LookbackWindowMinutes even if you are setting it to 15, which is the default value.
I would say: yes! I also want to advocate for each guide linking to one another. because even if I know both guides exist, if I'm writing a CR, I might think oh, I only have to reference/abide by the CR guide. so it would be nice to have something near the top of the CR guide that says, like, "This guide provides specialized guidelines on writing CRs, which build upon the general detection writing best practices outlined in /regular-style-guide." and then in the regular guide, I would love to see like "For specialized guidelines on writing correlation rules, see /cr_guide." or whatever |
|
@cara-panther Thank you for your comments! |
|
😱 |
|
😱 |
arielkr256
deleted the
THREAT-395-Correlation-Rule-Style-Guide-in-repo
branch
Changes