Repository navigation
Addon to PR 323: Moving Exec Args from NA to Storage - #328
Conversation
…ow various wildcards Signed-off-by: entlein <einentlein@gmail.com>
📝 WalkthroughWalkthroughThis PR refactors exec-argument token matching in the dynamic path detector by centralizing matching logic into new helper functions, adds comprehensive test coverage for embedded wildcard/dynamic tokens and differential verification, and removes upstream PR attribution from documentation throughout the package. ChangesDynamic Path Detector - Exec Args Token Matching and Package Documentation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Reviewed the relocation of the exec-arg matcher into storage. The refactor reads clean, build is green, and go test ./pkg/registry/file/dynamicpathdetector/... passes. The doc-comment cleanup is fine. One blocker to resolve before merge, plus a nit.
Blocker — argMatches reinterprets literal * / ⋯ in argv as wildcards
compare_exec_args.go:
if strings.Contains(profileArg, DynamicIdentifier) || strings.Contains(profileArg, WildcardIdentifier) {
return CompareDynamic(profileArg, runtimeArg)
}The strings.Contains gate can't tell an analyzer-emitted wildcard token from a * that a process was genuinely invoked with. Any profile arg in which * (or ⋯) lands on a /-delimited segment boundary now matches via CompareDynamic instead of literal equality, so the anchored match silently broadens. Confirmed against this branch:
CompareExecArgs(["/bin/*"], ["/bin/ls"])→ true (was literal-only: a profile that recorded the literal arg/bin/*now matches/bin/<anything>)CompareExecArgs(["cmd","/var/log/*/a"], ["cmd","/var/log/X/a"])→ true
(An embedded * that is not a whole segment, e.g. echo *, still compares literally, so the divergence is specifically the segment-boundary * / trailing /* case.)
For a detection matcher this is a false-negative direction: a profiled exec entry matches more runtime argv vectors than before, so a malicious exec that happens to fall under a literal-glob arg won't trip R0040. The argMatches doc asserts "any * seen is embedded in a path token (the form the analyzer emits)" — that assumption holds for analyzer output but not for user-authored profiles or recorded literal-glob args, and there's no escaping (\* is noted as a v0.0.2 future).
Can you confirm whether this matches node-agent's current behavior (i.e. this is intended parity, not a new broadening), and whether literal-* exec args are considered out of scope? If it's intended, a one-line note on argMatches stating literal */⋯ in argv are deliberately treated as wildcards would close it. If not, the gate needs to distinguish a real token from a literal character.
Nit
analyzer.go hasPrefixAtBoundary: the reflowed empty-prefix comment now breaks mid-sentence ("falls through to" / "pathPrefix[0] == '/'") and reads awkwardly — worth re-wrapping.
Signed-off-by: entlein <einentlein@gmail.com>
Signed-off-by: entlein <einentlein@gmail.com>
|
New plan, we use a different non-overloaded character. I ll next update NA 805 and then ... we retest it all :) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pkg/registry/file/dynamicpathdetector/tests/matchexecargs_property_test.go (1)
97-119: ⚡ Quick winBound default runtime of the exhaustive differential test.
This loop currently checks
2801 × 2801 = 7,845,601pairs, each with a naive recursive oracle call. Great for deep validation, but expensive for routine CI. Consider a smaller default bound and an opt-in exhaustive mode.Suggested change
import ( + "os" "strings" "testing" @@ func TestMatchExecArgs_DifferentialAgainstNaiveOracle(t *testing.T) { profAlphabet := []string{"a", "b", "*", dp.ExecArgsWildcard, dp.DynamicIdentifier, "/x/" + dp.DynamicIdentifier, "/x/*"} runAlphabet := []string{"a", "b", "c", "*", "/x/y", "/x/y/z", "/x/*"} - profiles := enumerate(profAlphabet, 4) - runtimes := enumerate(runAlphabet, 4) + maxLen := 3 + if os.Getenv("DP_EXHAUSTIVE_EXECARGS") == "1" { + maxLen = 4 + } + profiles := enumerate(profAlphabet, maxLen) + runtimes := enumerate(runAlphabet, maxLen)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/registry/file/dynamicpathdetector/tests/matchexecargs_property_test.go` around lines 97 - 119, The TestMatchExecArgs_DifferentialAgainstNaiveOracle function currently enumerates profiles and runtimes with depth 4, creating over 7.8 million test pairs which is expensive for routine CI. Reduce the default enumeration depth to a smaller value (such as 2 or 3) to speed up the test, and add a conditional check using testing.Short() to enable the exhaustive depth-4 enumeration only when opt-in exhaustive testing is desired (i.e., when not running in short mode). This way the function will run quickly by default but allow for deep validation when explicitly requested.pkg/registry/file/dynamicpathdetector/tests/compare_exec_args_test.go (1)
464-470: ⚡ Quick winReduce wall-clock flake risk in the ReDoS regression assertion.
Line 468’s fixed
100msbudget is likely to flap on contended runners even when behavior is correct. Consider a looser budget (or benchmark-only enforcement) so this remains a semantic regression test, not a scheduler-speed test.Suggested change
- if elapsed > 100*time.Millisecond { + const redosBudget = 500 * time.Millisecond + if elapsed > redosBudget { t.Errorf("matcher took %v on adversarial input — memoisation regression?", elapsed) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/registry/file/dynamicpathdetector/tests/compare_exec_args_test.go` around lines 464 - 470, The fixed 100ms timeout in the elapsed time check is causing test flakiness on contended runners due to scheduler variability. To fix this, increase the time budget significantly (e.g., to 500ms or 1s) to account for system contention while still catching actual performance regressions in the memoised matcher logic, or alternatively move this timing assertion to a benchmark test instead where it can be properly controlled. Update the condition comparing elapsed time and the corresponding error message to reflect the looser budget or remove the wall-clock timing check entirely from the test if moving to a benchmark-based approach.pkg/registry/file/dynamicpathdetector/tests/glob_semantics_demo_test.go (1)
17-20: ⚡ Quick winAlign opens
*comments with actual trailing-star semantics.The comments currently describe
*as uniformly “zero-or-more,” but inpkg/registry/file/dynamicpathdetector/analyzer.go(compareSegmentsIndex), trailing*matches one-or-more remaining segments. This demo text should reflect that distinction to avoid misleading future test authors.Suggested comment fix
-// "*" matches ZERO-OR-MORE path segments +// "*" matches ZERO-OR-MORE path segments in the middle, and +// ONE-OR-MORE segments when trailing @@ - "/var/log/*", // zero-or-more trailing segments + "/var/log/*", // one-or-more trailing segmentsAlso applies to: 39-40
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/registry/file/dynamicpathdetector/tests/glob_semantics_demo_test.go` around lines 17 - 20, The comments describing the `*` wildcard semantics in the test file incorrectly state that `*` matches "ZERO-OR-MORE" path segments, but the actual implementation in the `compareSegmentsIndex` function in the analyzer module shows that a trailing `*` matches one-or-more remaining segments. Update the comment text to accurately reflect that trailing `*` matches one-or-more segments rather than zero-or-more, ensuring the documentation aligns with the actual implementation behavior and prevents misleading future readers of these test semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/registry/file/dynamicpathdetector/tests/compare_exec_args_test.go`:
- Around line 464-470: The fixed 100ms timeout in the elapsed time check is
causing test flakiness on contended runners due to scheduler variability. To fix
this, increase the time budget significantly (e.g., to 500ms or 1s) to account
for system contention while still catching actual performance regressions in the
memoised matcher logic, or alternatively move this timing assertion to a
benchmark test instead where it can be properly controlled. Update the condition
comparing elapsed time and the corresponding error message to reflect the looser
budget or remove the wall-clock timing check entirely from the test if moving to
a benchmark-based approach.
In `@pkg/registry/file/dynamicpathdetector/tests/glob_semantics_demo_test.go`:
- Around line 17-20: The comments describing the `*` wildcard semantics in the
test file incorrectly state that `*` matches "ZERO-OR-MORE" path segments, but
the actual implementation in the `compareSegmentsIndex` function in the analyzer
module shows that a trailing `*` matches one-or-more remaining segments. Update
the comment text to accurately reflect that trailing `*` matches one-or-more
segments rather than zero-or-more, ensuring the documentation aligns with the
actual implementation behavior and prevents misleading future readers of these
test semantics.
In `@pkg/registry/file/dynamicpathdetector/tests/matchexecargs_property_test.go`:
- Around line 97-119: The TestMatchExecArgs_DifferentialAgainstNaiveOracle
function currently enumerates profiles and runtimes with depth 4, creating over
7.8 million test pairs which is expensive for routine CI. Reduce the default
enumeration depth to a smaller value (such as 2 or 3) to speed up the test, and
add a conditional check using testing.Short() to enable the exhaustive depth-4
enumeration only when opt-in exhaustive testing is desired (i.e., when not
running in short mode). This way the function will run quickly by default but
allow for deep validation when explicitly requested.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d52bd55-70b7-4a51-9e37-612e841b5d01
📒 Files selected for processing (9)
pkg/registry/file/dynamicpathdetector/analyzer.gopkg/registry/file/dynamicpathdetector/compare_exec_args.gopkg/registry/file/dynamicpathdetector/tests/benchmark_test.gopkg/registry/file/dynamicpathdetector/tests/compare_dynamic_adversarial_test.gopkg/registry/file/dynamicpathdetector/tests/compare_exec_args_test.gopkg/registry/file/dynamicpathdetector/tests/execargs_wildcard_ap_test.gopkg/registry/file/dynamicpathdetector/tests/glob_semantics_demo_test.gopkg/registry/file/dynamicpathdetector/tests/matchexecargs_property_test.gopkg/registry/file/dynamicpathdetector/types.go
✅ Files skipped from review due to trivial changes (3)
- pkg/registry/file/dynamicpathdetector/tests/compare_dynamic_adversarial_test.go
- pkg/registry/file/dynamicpathdetector/analyzer.go
- pkg/registry/file/dynamicpathdetector/types.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/registry/file/dynamicpathdetector/tests/benchmark_test.go
- pkg/registry/file/dynamicpathdetector/compare_exec_args.go
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Re-reviewed at 35603ac. The blocker is resolved — thanks for the thorough rework.
The exec matcher no longer overloads *. ⋯ (one arg / one segment) and the new ExecArgsWildcard ⋯⋯ (zero-or-more whole args) are dedicated sentinels, and argMatches now treats * as literal data. matchExecArgSegments does its own equal-count, literal-segment compare instead of delegating to CompareDynamic, so the file-path * semantics no longer leak into argv matching. Verified the previously over-broad cases are now literal:
CompareExecArgs(["/bin/*"], ["/bin/ls"])→ false (matches only literal/bin/*)CompareExecArgs(["/plugins/*"], ["/plugins/evil.so"])→ falseCompareExecArgs([..,"/var/log/*/a"], [..,"/var/log/X/a"])→ false
…while the intended ⋯ path-token matching still works (/usr/lib/postgresql/⋯/bin/postgres matches the versioned runtime path and rejects an extra segment). Build is green and the full package test suite passes, including the new property / AP / glob-semantics tests. The comment-attribution nit is cleaned up too.
One coordination note (not blocking this PR): since * is now data and ⋯⋯ is the positional zero-or-more sentinel, node-agent (#805) must emit ⋯⋯ — not * — when it generates the "zero-or-more args" form, or learned profiles won't match. Worth a line in the NA PR description so the two land together.
LGTM on the storage side once #805 is aligned.
I dont think so , node-agent was never supposed to pre-aggregate the APs into I m assuming the |
currently kubescape/node-agent#805 is built against my mirror of this branch, and the test results and benchmarks are at the bottom.
There are edits to outdated comments, the only
relevantpart of this PR is moving this function from NA to storage to avoid duplication of logic.Summary by CodeRabbit
Refactor
Tests
*staying literal, and boundary/adversarial cases to prevent unintended matches.Documentation