tier-2 (execs): exec.go exact-path args constraint authoritative (no fall-through)#49
Open
entlein wants to merge 2 commits into
Open
tier-2 (execs): exec.go exact-path args constraint authoritative (no fall-through)#49entlein wants to merge 2 commits into
entlein wants to merge 2 commits into
Conversation
added 2 commits
May 19, 2026 21:31
…true) Addresses Matthias's upstream PR kubescape#811 blocking review (2026-05-19T10:35Z): Blocking: projectField() still keeps dynamic open paths only in cp.Opens.Patterns when cp.Opens.All == true. Removing the Patterns scan here makes ap.was_path_opened_with_suffix() / _with_prefix() return false for dynamic entries in pass-through mode, so rules that omit profileDataRequired.opens regress. If the old strings.HasPrefix/HasSuffix behavior on pattern text is too permissive, we still need a narrower fallback here instead of ignoring Patterns entirely. Context ------- Pre-PR-kubescape#811, CodeRabbit (PR #43 review on open.go:79) flagged that running strings.HasSuffix("/var/log/pods/*/volumes/...", "foo.log") returns false even though the pattern's concrete realisations might end in "foo.log" — a false negative. We addressed it by SKIPPING the Patterns scan in the All=true branch. Matthias's review reverses that: skipping is too aggressive and silently regresses rules that omit profileDataRequired.opens (which rely on the All=true pass-through behavior). Fix --- Narrower fallback — when Opens.All == true and the Values scan didn't match, scan Patterns with split semantics: patternConcreteSuffix(p) → literal text after the LAST wildcard segment in p; "" if p ends in a wildcard segment. patternConcretePrefix(p) → literal text before the FIRST wildcard segment in p; "" if p starts with a wildcard segment. Suffix/prefix matcher then: for each pattern: tail/head := patternConcrete{Suffix,Prefix}(pattern) if tail/head == "": return true // wildcard tail/head → permissive (concrete // realisations could match ANY suffix/prefix) if strings.HasSuffix/HasPrefix(tail/head, query): return true Boundary slashes are KEPT in the returned concrete suffix/prefix so queries with leading/trailing slashes match correctly (every concrete realisation has the boundary slash too). Tests ----- Updated previous "PatternsNotScanned" tests (which pinned the pre-kubescape#811 CR fix) to reflect Matthias's new contract: TestWasPathOpenedWithSuffix_PatternsScannedWithConcreteTail pattern "/var/log/⋯/foo.log", query ".log" → true (tail "/foo.log" has suffix ".log") pattern "/var/log/⋯/foo.log", query ".txt" → false TestWasPathOpenedWithSuffix_PatternWildcardTail_Permissive pattern "/var/log/pods/*", any suffix → true TestWasPathOpenedWithPrefix_PatternsScannedWithConcreteHead pattern "/var/⋯/log/foo", query "/var/" → true (head "/var/" has prefix "/var/") pattern "/var/⋯/log/foo", query "/etc/" → false TestWasPathOpenedWithPrefix_PatternWildcardHead_Permissive pattern "*/run", any prefix → true TestPatternConcreteSuffix_AndPrefix pins the splitter helpers against 7 representative shapes (no_wildcards, trailing_star, leading_star, mid_ellipsis, both_mid, lone_star, lone_ellipsis). Benchmarks (new file: open_bench_test.go) ----------------------------------------- Helper itself — zero allocation across all shapes: BenchmarkPatternConcreteSuffix//var/log/⋯/foo.log 11.5 ns/op 0 B/op 0 allocs/op BenchmarkPatternConcreteSuffix//var/log/pods/* 10.0 ns/op 0 B/op 0 allocs/op BenchmarkPatternConcreteSuffix//var/log/foo.log 9.4 ns/op 0 B/op 0 allocs/op BenchmarkPatternConcreteSuffix/* 1.6 ns/op 0 B/op 0 allocs/op BenchmarkPatternConcreteSuffix//var/⋯/log/⋯/foo.log 14.8 ns/op 0 B/op 0 allocs/op Full matcher (50 Values + 10 Patterns) — the Patterns scan adds ~20 ns on top of the Values-only path with no extra allocations (the steady 56 B/op / 3 allocs/op is pre-existing CEL value-wrapping cost, unrelated to this change): BenchmarkWasPathOpenedWithSuffix_AllMode/values_only 372 ns/op 56 B/op 3 allocs/op BenchmarkWasPathOpenedWithSuffix_AllMode/patterns_concrete 391 ns/op 56 B/op 3 allocs/op BenchmarkWasPathOpenedWithSuffix_AllMode/patterns_wildcard 393 ns/op 56 B/op 3 allocs/op BenchmarkWasPathOpenedWithPrefix_AllMode/values_only 351 ns/op 56 B/op 3 allocs/op BenchmarkWasPathOpenedWithPrefix_AllMode/patterns_concrete 395 ns/op 56 B/op 3 allocs/op BenchmarkWasPathOpenedWithPrefix_AllMode/patterns_wildcard 428 ns/op 56 B/op 3 allocs/op
Addresses the CodeRabbit PR #43 Major finding on `pkg/rulemanager/cel/libraries/applicationprofile/exec.go:144`, deferred from the NA #46 batch-1 PR body. Tracked there as: exec.go:144 — Exact path match should return false when args don't match, not fall through to pattern matching. Problem ------- Pre-fix `wasExecutedWithArgs` flow on exact-path hit: if pathStr in cp.Execs.Values: if pathStr in cp.ExecsByPath: if CompareExecArgs(...) → return true // else: FALL THROUGH to pattern matching else: return true (back-compat no-constraint) for pattern in cp.Execs.Patterns: ... The fall-through on `CompareExecArgs == false` would let a LOOSER pattern (e.g. `/usr/bin/*` with profileArgs `[*]`) shadow the exact-path entry's strict args constraint. A rule-author who lists `{Path: /usr/bin/curl, Args: [-X, GET]}` expects that ONLY GET requests pass — but if their profile also contains a permissive `/usr/bin/⋯` pattern, the fall-through silently allows DELETE/POST/… via the pattern. Fix --- When the exact path IS in Values AND ExecsByPath has an entry, the constraint result IS the answer for this path. Return it directly — no fall-through: if pathStr in cp.Execs.Values: if pathStr in cp.ExecsByPath: return CompareExecArgs(profileArgs, runtimeArgs) // else: back-compat no-constraint return true // path NOT in Values — try Patterns for pattern in cp.Execs.Patterns: ... Pattern-loop fall-through stays as-is — different patterns CAN encode different `(path, args)` shapes and a non-matching one shouldn't kill the loop. The asymmetry is intentional: exact-path-with-constraint is producer-authored authoritative, pattern-with-constraint is one candidate among many. Bench (`-count=3 -benchtime=200ms`) ----------------------------------- exact_path_args_match ~890 ns/op 2136 B/op 20 allocs/op exact_path_args_mismatch ~860 ns/op 2136 B/op 20 allocs/op pattern_path_args_match ~450 ns/op 360 B/op 13 allocs/op The exact-path mismatch case is no longer paying the pattern-loop cost it used to (and used to silently return true via the pattern shadow). The remaining alloc/op floor is pre-existing CEL value wrapping (matcher unchanged).
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
✨ 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 |
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup Effectiveness (AFTER only)
Event Counters
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tier 2 of the v0.3.26 release chain
Execs layer. Branches off node-agent#48 (tier-1 path+opens) and pairs with storage#37 (
release/v0.0.240/tier-2-execs).Addresses the CodeRabbit PR #43 Major finding on
pkg/rulemanager/cel/libraries/applicationprofile/exec.go:144, deferred from the NA #46 batch-1 PR body:Problem
Pre-fix
wasExecutedWithArgsflow on exact-path hit:The fall-through on
CompareExecArgs == falselet a LOOSER pattern (e.g./usr/bin/⋯with profileArgs[*]) shadow the exact-path entry's strict args constraint. A rule-author who lists{Path: /usr/bin/curl, Args: [-X, GET]}expects ONLY GET requests to pass — but if their profile also contains a permissive/usr/bin/⋯pattern, the fall-through silently allows DELETE/POST via the pattern.Fix
Exact-path-with-constraint is producer-authored authoritative. Its match result IS the answer for this path — no fall-through:
Pattern-loop fall-through stays as-is — different patterns CAN encode different
(path, args)shapes and a non-matching one shouldn't kill the loop. The asymmetry is intentional.Local benchmarks (
-count=3 -benchtime=200ms)exact_path_args_mismatchis no longer paying the pattern-loop cost it used to — AND no longer returns the wrong answer via Pattern shadow. The 2136 B / 20 allocs/op floor is pre-existing CEL value-wrapping; matcher logic itself didn't add allocations.Companion storage tier-2 (
#37) ships zero-allocCompareExecArgsdispatch — applies to the same R0040 hot path this matcher consumes.Reproduce:
Component test evidence
Run
26152411366(Component Tests onrelease/v0.3.26/tier-2-execswithSTORAGE_TAG=tier2+STORAGE_REF=2db8af1e):wasExecutedWithArgs(the matcher this PR fixes)26121369338and on post-PR#43 main CT25969722538), not introduced by this PRtrigger-integration-testsfailure — same auto-trigger branch-name issue addressed in storage#36 (not a code issue)Tier-1 dependencies still in effect
This branch builds on top of tier-1 path-opens (#48). The narrower-fallback Patterns scan for
was_path_opened_with_suffix/_prefixis included; Test_27 + Test_33 green confirm no regression.Test plan
go build ./...cleango test ./pkg/rulemanager/cel/libraries/applicationprofile/... -count=1greenBenchmarkWasExecutedWithArgsruns clean (table above)