From af134ef7365a0856b4b455761e2147baa9436bb8 Mon Sep 17 00:00:00 2001 From: koiakoia Date: Wed, 29 Apr 2026 12:45:38 -0400 Subject: [PATCH 1/3] Add STYLE_GUIDE.md for rules doc comments Implements the convention discussed in #150 (doc comments vs section comments, override examples in tuning macros, source attribution for list-derived data, "explain or delete" for commented-out items). Builds on @leogr's Go-Doc-inspired proposal from Feb 2026, fleshed out with GOOD/BAD examples drawn from running ~77 custom rules in production on OKD. Closes #150 Signed-off-by: koiakoia --- CONTRIBUTING.md | 4 + STYLE_GUIDE.md | 388 ++++++++++++++++++++++++++++++++++++++++++++++++ mkdocs.yml | 1 + 3 files changed, 393 insertions(+) create mode 100644 STYLE_GUIDE.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b2fee004b..3fb02de5e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -6,6 +6,10 @@ This repository includes a dedicated guide for contributing rules, outlining the All rules must be licensed under the [Apache 2.0 License](./LICENSE). +For comment and documentation conventions within rule files (doc comments, +section comments, tuning-macro override examples, and list provenance), see +**[STYLE_GUIDE.md](STYLE_GUIDE.md)**. + **Table of Contents** diff --git a/STYLE_GUIDE.md b/STYLE_GUIDE.md new file mode 100644 index 000000000..7cef81bb2 --- /dev/null +++ b/STYLE_GUIDE.md @@ -0,0 +1,388 @@ +# Falco Rules Doc Comment Style Guide + +> **Status:** Draft — opened in PR for discussion with maintainers. +> Feedback and revisions welcome; this guide is a starting point, not a mandate. + +--- + +## Motivation + +Falco rule files contain two kinds of comments: comments that document an +individual item (a rule, macro, or list), and comments that annotate a section +of the file. Without a convention to distinguish them, it's ambiguous whether a +comment describes the item immediately below it or the group that follows. + +This guide proposes a small set of lightweight conventions borrowed from the +[Go Doc Comments](https://go.dev/doc/comment) model and shaped by the discussion +in [issue #150](https://github.com/falcosecurity/rules/issues/150). The goal is +to make rule files easier to read, easier to tune, and easier to review — without +imposing heavy overhead on contributors. + +The conventions apply to the YAML rule files under `rules/`. They complement +(rather than replace) the [Falco Rules Style Guide](https://falco.org/docs/rules/style-guide/). + +--- + +## 1. Doc Comments vs. Section Comments + +This is the central rule. There are exactly two kinds of comments: + +- **Doc comment** — documents a single item (rule, macro, list). No blank line + between the comment and the item. +- **Section comment** — annotates a group of items or a region of the file. Has + a blank line above it *and* at least one blank line between it and the first + item in the group. + +### Why this matters + +Without the blank-line rule, a comment that sits three lines above a macro might +be describing the macro, the previous macro, or a logical group. The blank-line +rule resolves this unambiguously: if there's no blank line between a comment and +an item, the comment belongs to that item. + +### GOOD — doc comment (no blank line between comment and item) + +```yaml +# True when the event occurs inside a container (container.id != host). +# Used widely to scope rules to containerized workloads only. +- macro: container + condition: (container.id != host) +``` + +### BAD — ambiguous gap + +```yaml +# True when the event occurs inside a container (container.id != host). + +- macro: container + condition: (container.id != host) +``` + +The blank line above signals "section comment," but this comment is clearly +about `container`, not a group. A reviewer cannot tell without reading the +condition. + +### GOOD — section comment (blank line on both sides) + +```yaml +# ─── File access helpers ──────────────────────────────────────────────────── + +- macro: open_write + condition: (evt.type in (open,openat,openat2) and evt.is_open_write=true and fd.typechar='f' and fd.num>=0) + +- macro: open_read + condition: (evt.type in (open,openat,openat2) and evt.is_open_read=true and fd.typechar='f' and fd.num>=0) +``` + +The blank line separating the section comment from the items makes clear it +applies to the group, not specifically to `open_write`. + +### BAD — section comment pasted to the first item + +```yaml +# File access helpers +- macro: open_write + condition: (evt.type in (open,openat,openat2) and evt.is_open_write=true and fd.typechar='f' and fd.num>=0) +``` + +This looks like a doc comment for `open_write` alone, when the intent was to +label the section. + +--- + +## 2. Doc Comment Structure + +A doc comment's **first sentence** should summarize what the item does — concise +enough to serve as a tooltip or search result. Additional sentences can cover +tuning guidance, rationale, related items, or caveats. + +The first sentence ends at the first period followed by a space or newline. +Don't pad with introductory phrases like "This macro…" or "This list…" — start +with the substance. + +### GOOD + +```yaml +# Detects process name existence; required before identity-based conditions. +# Handles the edge case of dropped syscall events where proc.name may be "". +# TODO: Remove the "N/A" variant once scap-file compatibility is no longer needed. +- macro: proc_name_exists + condition: (not proc.name in ("","N/A")) +``` + +- First sentence states the purpose directly. +- Second sentence explains the rationale. +- TODO is explicit and actionable. + +### BAD + +```yaml +# This macro is about the process name +- macro: proc_name_exists + condition: (not proc.name in ("","N/A")) +``` + +"About the process name" tells a reviewer nothing they couldn't infer from +the macro name itself. + +--- + +## 3. Tuning Macros (`user_*`, `allowed_*`) + +Tuning macros are placeholders — their default condition is `(never_true)` or +`(always_true)`, and operators are expected to override them locally to suppress +false positives. Because these macros have no `desc` field, their doc comment is +the *only* documentation an operator will read before deciding how to override. + +**Required in every tuning macro doc comment:** + +1. What the macro does (one sentence). +2. How to override it — include a concrete `append: true` snippet. +3. The field or fields most commonly used in overrides (e.g., `proc.name`, + `container.image.repository`). + +### GOOD + +```yaml +# Suppresses "Read sensitive file untrusted" for known-legitimate readers. +# Default is never_true (no suppression). To allow specific processes, append: +# +# - macro: user_known_read_sensitive_files_activities +# condition: (proc.name in (my_backup_agent, my_audit_tool)) +# append: true +# +# Common override fields: proc.name, proc.exepath, container.image.repository. +- macro: user_known_read_sensitive_files_activities + condition: (never_true) +``` + +### BAD + +```yaml +# Add conditions here to suppress false positives. +- macro: user_known_read_sensitive_files_activities + condition: (never_true) +``` + +An operator hitting a false positive at 2 AM needs the override snippet, not +a paraphrase of what overriding means. + +--- + +### GOOD — `allowed_*` style (negative-logic placeholder) + +```yaml +# Allowlist of hosts permitted to initiate SSH connections in the monitored rule. +# Default is never_true; the rule uses double negation (and not allowed_ssh_hosts), +# so this macro effectively allows everything when unset. +# +# To restrict SSH to known management hosts: +# +# - macro: allowed_ssh_hosts +# condition: (evt.hostname in (bastion.example.com, mgmt.example.com)) +# append: true # or override entirely — remove append: true to replace default +# +# See also: macro never_true for the placeholder pattern. +- macro: allowed_ssh_hosts + condition: (never_true) +``` + +### BAD + +```yaml +# Placeholder for SSH host allowlist. +- macro: allowed_ssh_hosts + condition: (never_true) +``` + +--- + +## 4. Lists with Derived Data + +When a list's contents were generated or sourced from an external reference +(a package manager query, a vendor document, a command), record that provenance +in the doc comment. This helps future contributors verify and update the list +without having to reverse-engineer how it was built. + +### GOOD + +```yaml +# Password and account-management binaries on Debian/Ubuntu systems. +# Generated with: +# dpkg -L passwd | grep bin | xargs ls -ld | grep -v '^d' \ +# | awk '{print $9}' | xargs -L 1 basename | tr '\n' ',' +# Last verified against passwd 1:4.13+dfsg1-4 (Debian bookworm). +- list: passwd_binaries + items: [ + shadowconfig, grpck, pwunconv, grpconv, pwck, + groupmod, vipw, pwconv, useradd, newusers, cppw, chpasswd, usermod, + groupadd, groupdel, grpunconv, chgpasswd, userdel, chage, chsh, + gpasswd, chfn, expiry, passwd, vigr, cpgr, adduser, addgroup, deluser, delgroup + ] +``` + +### BAD + +```yaml +# passwd binaries +- list: passwd_binaries + items: [ + shadowconfig, grpck, pwunconv, ... + ] +``` + +Without the source command, the next contributor who needs to update this list +for a new distro version has no starting point. + +--- + +### GOOD — vendor / upstream URL attribution + +```yaml +# NFS external-provisioner image; sourced from upstream chart values: +# https://github.com/kubernetes-sigs/nfs-subdir-external-provisioner/blob/master/charts/nfs-subdir-external-provisioner/values.yaml +- list: nfs_provisioner_images + items: [registry.k8s.io/sig-storage/nfs-subdir-external-provisioner] +``` + +--- + +## 5. Commented-Out Code + +Commented-out rules, macros, and lists accumulate quickly. At scale they become +noise that reviewers skip, and nobody knows whether they were intentionally +disabled, broken, or simply forgotten. + +**Convention:** if you keep a commented-out item, its doc comment must explain +*why* it is disabled and under what conditions it should be re-enabled. +If you cannot write that explanation, delete the item — git history preserves it. + +### GOOD — intentionally disabled with clear reason + +```yaml +# Disabled: read and write syscalls are ignored in the current event source. +# The open_write/open_read macros cover the relevant cases via the open* family. +# Re-enable if a future Falco version surfaces raw read/write as distinct events. +# +# - macro: write +# condition: (syscall.type=write and fd.type in (file, directory)) +# - macro: read +# condition: (syscall.type=read and fd.type in (file, directory)) +``` + +### BAD — unexplained comment + +```yaml +# - macro: write +# condition: (syscall.type=write and fd.type in (file, directory)) +# - macro: read +# condition: (syscall.type=read and fd.type in (file, directory)) +``` + +A reviewer seeing this has no idea if the macros are intentionally disabled, +temporarily scaffolded, or simply left from a refactor. + +--- + +### GOOD — tuning template with opt-in explanation + +```yaml +# Optional: treat any node process in a container as a protected shell spawner. +# Disabled by default because node is also widely used as a build-pipeline tool +# where spawning shells is expected. Enable by overriding this macro to remove +# the never_true guard: +# +# - macro: possibly_node_in_container +# condition: (proc.pname=node and proc.aname[3]=docker-containe) +# # no append: true — replace the default entirely +# +- macro: possibly_node_in_container + condition: (never_true and (proc.pname=node and proc.aname[3]=docker-containe)) +``` + +--- + +## 6. Section Comments + +Section comments group related items visually. They are separated from the items +they introduce by a blank line (so they are not mistaken for a doc comment on the +first item in the group). + +A section comment should describe the logical grouping, not re-list the items. + +### GOOD + +```yaml +# ─── Shell-spawning protection ─────────────────────────────────────────────── +# Macros and lists used by "Run shell untrusted" to identify non-shell parent +# processes that should never spawn a shell. + +- list: protected_shell_spawning_binaries + items: [ + http_server_binaries, db_server_binaries, nosql_server_binaries, mail_binaries, + fluentd, flanneld, splunkd, consul, smbd, runsv, PM2 + ] + +- macro: protected_shell_spawner + condition: (proc.pname exists and proc.pname in (protected_shell_spawning_binaries)) +``` + +### BAD — section header attached to first item + +```yaml +# Shell-spawning protection macros and lists +- list: protected_shell_spawning_binaries + items: [...] +``` + +--- + +## 7. Field Order within a Rule + +The [Falco Rules Style Guide](https://falco.org/docs/rules/style-guide/) is +authoritative on field ordering. For reference, the order used in existing +`maturity_stable` rules is: + +```yaml +- rule: + desc: > + + condition: > + + output: + priority: + tags: [, , ] + enabled: # omit when true (the default) + source: # omit when syscall (the default) +``` + +`enabled: false` and `source:` are placed after `tags` and only included when +they differ from the default. This ordering is a convention observed in the +existing rule corpus; defer to maintainers if guidance changes. + +--- + +## Quick Reference + +| Situation | Convention | +|-----------|------------| +| Comment immediately precedes an item (no blank line) | Doc comment — documents that item | +| Comment has a blank line before it AND below it | Section comment — labels a group | +| Tuning macro (`user_*`, `allowed_*`) | Doc comment must include override snippet with `append: true` | +| List with generated or sourced content | Doc comment must include source command or URL | +| Commented-out item with no explanation | Delete it — use git history | +| Commented-out item kept intentionally | Doc comment must state why and when to re-enable | +| First sentence of any doc comment | Summarize purpose; avoid "This macro…" preamble | + +--- + +## Relationship to the Existing Style Guide + +This guide covers **comment conventions only**. For rule expression style, +output format, tag requirements, and maturity-level criteria, see the +[Falco Rules Style Guide](https://falco.org/docs/rules/style-guide/) and +[CONTRIBUTING.md](CONTRIBUTING.md). + +--- + +*Feedback on these guidelines is welcome in the PR discussion.* diff --git a/mkdocs.yml b/mkdocs.yml index a90851db0..e4ec43083 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -2,5 +2,6 @@ site_name: Falcosecurity Rules site_url: https://github.com/falcosecurity/rules nav: - Home: index.md + - Style Guide: STYLE_GUIDE.md theme: material From be40a7eafc0eb11113a4ba6c57de2feb6cdddede Mon Sep 17 00:00:00 2001 From: koiakoia Date: Tue, 19 May 2026 09:06:26 -0400 Subject: [PATCH 2/3] Address review feedback: override syntax + correct SSH allowlist example - Replace deprecated `append: true` with `override: append` throughout the examples and Quick Reference, per @ekoops review feedback. Add a note that `override: replace` is the swap-default counterpart. - Rewrite the `allowed_*` style example: the previous version used `evt.hostname`, which is the Falco runner's own hostname (not the destination), so the macro could never restrict SSH peers as described. Renamed to `allowed_ssh_initiators` and switched the condition to `proc.name`, with a callout that `fd.rip` is the right field for peer-IP allowlists. Reviewer thread: https://github.com/falcosecurity/rules/pull/ Signed-off-by: koiakoia --- STYLE_GUIDE.md | 41 ++++++++++++++++++++++++----------------- 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/STYLE_GUIDE.md b/STYLE_GUIDE.md index 7cef81bb2..a13c0d9a3 100644 --- a/STYLE_GUIDE.md +++ b/STYLE_GUIDE.md @@ -137,7 +137,7 @@ the *only* documentation an operator will read before deciding how to override. **Required in every tuning macro doc comment:** 1. What the macro does (one sentence). -2. How to override it — include a concrete `append: true` snippet. +2. How to override it — include a concrete `override: append` snippet. 3. The field or fields most commonly used in overrides (e.g., `proc.name`, `container.image.repository`). @@ -145,13 +145,15 @@ the *only* documentation an operator will read before deciding how to override. ```yaml # Suppresses "Read sensitive file untrusted" for known-legitimate readers. -# Default is never_true (no suppression). To allow specific processes, append: +# Default is never_true (no suppression). To allow specific processes: # # - macro: user_known_read_sensitive_files_activities # condition: (proc.name in (my_backup_agent, my_audit_tool)) -# append: true +# override: append # -# Common override fields: proc.name, proc.exepath, container.image.repository. +# Use `override: replace` to swap the default condition entirely instead of +# extending it. Common override fields: proc.name, proc.exepath, +# container.image.repository. - macro: user_known_read_sensitive_files_activities condition: (never_true) ``` @@ -172,26 +174,31 @@ a paraphrase of what overriding means. ### GOOD — `allowed_*` style (negative-logic placeholder) ```yaml -# Allowlist of hosts permitted to initiate SSH connections in the monitored rule. -# Default is never_true; the rule uses double negation (and not allowed_ssh_hosts), -# so this macro effectively allows everything when unset. +# Allowlist of process names permitted to initiate outbound SSH connections. +# Default is never_true; the rule uses double negation +# (and not allowed_ssh_initiators), so this macro effectively allows nothing +# when unset and the rule's negative gate stays open. Operators override to +# narrow the set of expected SSH-initiating processes. # -# To restrict SSH to known management hosts: +# To restrict SSH initiation to known automation tooling: # -# - macro: allowed_ssh_hosts -# condition: (evt.hostname in (bastion.example.com, mgmt.example.com)) -# append: true # or override entirely — remove append: true to replace default +# - macro: allowed_ssh_initiators +# condition: (proc.name in (ansible-playbook, scp, rsync)) +# override: append # -# See also: macro never_true for the placeholder pattern. -- macro: allowed_ssh_hosts +# Use `override: replace` to swap the default condition entirely instead of +# extending it. Common override fields: proc.name, proc.exepath, fd.rip +# (for peer-IP allowlists). See also: macro never_true for the placeholder +# pattern. +- macro: allowed_ssh_initiators condition: (never_true) ``` ### BAD ```yaml -# Placeholder for SSH host allowlist. -- macro: allowed_ssh_hosts +# Placeholder for SSH initiator allowlist. +- macro: allowed_ssh_initiators condition: (never_true) ``` @@ -294,7 +301,7 @@ temporarily scaffolded, or simply left from a refactor. # # - macro: possibly_node_in_container # condition: (proc.pname=node and proc.aname[3]=docker-containe) -# # no append: true — replace the default entirely +# override: replace # swap the default never_true guard entirely # - macro: possibly_node_in_container condition: (never_true and (proc.pname=node and proc.aname[3]=docker-containe)) @@ -368,7 +375,7 @@ existing rule corpus; defer to maintainers if guidance changes. |-----------|------------| | Comment immediately precedes an item (no blank line) | Doc comment — documents that item | | Comment has a blank line before it AND below it | Section comment — labels a group | -| Tuning macro (`user_*`, `allowed_*`) | Doc comment must include override snippet with `append: true` | +| Tuning macro (`user_*`, `allowed_*`) | Doc comment must include override snippet with `override: append` (or `override: replace`) | | List with generated or sourced content | Doc comment must include source command or URL | | Commented-out item with no explanation | Delete it — use git history | | Commented-out item kept intentionally | Doc comment must state why and when to re-enable | From 79f6c33b73963f849c5e6e87e96c70d1ae283a39 Mon Sep 17 00:00:00 2001 From: koiakoia Date: Tue, 19 May 2026 09:08:54 -0400 Subject: [PATCH 3/3] Move STYLE_GUIDE.md into docs/ for mkdocs discovery Per review feedback from @ekoops: the top-level STYLE_GUIDE.md isn't found by mkdocs since the project's docs_dir defaults to docs/. Moving the file into docs/ aligns with mkdocs convention and matches the existing docs/images/ placement. The mkdocs.yml nav entry `Style Guide: STYLE_GUIDE.md` already resolves to docs/STYLE_GUIDE.md under docs_dir, so no nav rewrite is needed; added a clarifying comment. Signed-off-by: koiakoia --- STYLE_GUIDE.md => docs/STYLE_GUIDE.md | 0 mkdocs.yml | 2 +- 2 files changed, 1 insertion(+), 1 deletion(-) rename STYLE_GUIDE.md => docs/STYLE_GUIDE.md (100%) diff --git a/STYLE_GUIDE.md b/docs/STYLE_GUIDE.md similarity index 100% rename from STYLE_GUIDE.md rename to docs/STYLE_GUIDE.md diff --git a/mkdocs.yml b/mkdocs.yml index e4ec43083..01762d72c 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -2,6 +2,6 @@ site_name: Falcosecurity Rules site_url: https://github.com/falcosecurity/rules nav: - Home: index.md - - Style Guide: STYLE_GUIDE.md + - Style Guide: STYLE_GUIDE.md # served from docs/STYLE_GUIDE.md (mkdocs docs_dir default) theme: material