From 3dd59323c245e5d12b32872387a9a30406ac27be Mon Sep 17 00:00:00 2001 From: Entlein Date: Tue, 19 May 2026 21:31:34 +0200 Subject: [PATCH 1/2] open: narrower-fallback Patterns scan for suffix/prefix queries (All=true) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Matthias's upstream PR #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-#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-#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 --- .../cel/libraries/applicationprofile/open.go | 129 ++++++++++++++-- .../applicationprofile/open_bench_test.go | 129 ++++++++++++++++ .../libraries/applicationprofile/open_test.go | 142 +++++++++++++++--- 3 files changed, 360 insertions(+), 40 deletions(-) create mode 100644 pkg/rulemanager/cel/libraries/applicationprofile/open_bench_test.go diff --git a/pkg/rulemanager/cel/libraries/applicationprofile/open.go b/pkg/rulemanager/cel/libraries/applicationprofile/open.go index 62a4abedf..30ce3c2c2 100644 --- a/pkg/rulemanager/cel/libraries/applicationprofile/open.go +++ b/pkg/rulemanager/cel/libraries/applicationprofile/open.go @@ -113,20 +113,34 @@ func (l *apLibrary) wasPathOpenedWithSuffix(containerID, suffix ref.Val) ref.Val if cp.Opens.All { // All entries retained (no rule declared SuffixHits-style - // projection). Scan ONLY concrete entries in Values — Patterns - // contain wildcard tokens ('*' / '⋯') whose text doesn't safely - // answer suffix questions. CodeRabbit PR #43 open.go:79: a - // retained Pattern like "/var/log/pods/*/volumes/..." doesn't - // end with the concrete suffix "foo.log", but the concrete open - // it stands in for might — strings.HasSuffix on the pattern - // text returns false and produces a false negative. Patterns - // are inherently wildcard-shaped; concrete-path semantics live - // in Values (and in SuffixHits when projection is active). + // projection). Scan concrete entries in Values first — exact + // strings.HasSuffix is correct for those. for openPath := range cp.Opens.Values { if strings.HasSuffix(openPath, suffixStr) { return types.Bool(true) } } + // Patterns hold dynamic entries (containing `*` / `⋯`). We + // can't run strings.HasSuffix on the raw pattern text — a + // pattern like "/var/log/pods/*/volumes/..." has wildcard + // tokens that don't textually end with "foo.log" even though + // its concrete realisations might. Matthias upstream PR #811 + // review: a NARROWER fallback is the answer here — split off + // the pattern's concrete tail (the literal text after the + // last wildcard segment) and only check HasSuffix against + // that. If the pattern ends in a wildcard segment, the tail + // is empty and concrete realisations could match ANY suffix — + // be permissive (return true) to avoid the false-negative on + // rules that omit profileDataRequired.opens. + for _, openPattern := range cp.Opens.Patterns { + tail := patternConcreteSuffix(openPattern) + if tail == "" { + return types.Bool(true) + } + if strings.HasSuffix(tail, suffixStr) { + return types.Bool(true) + } + } return types.Bool(false) } // Projection applied — SuffixHits is authoritative; absent key = undeclared. @@ -160,18 +174,27 @@ func (l *apLibrary) wasPathOpenedWithPrefix(containerID, prefix ref.Val) ref.Val } if cp.Opens.All { - // All entries retained — scan ONLY Values (concrete paths). - // Patterns contain wildcard tokens whose text doesn't safely - // answer prefix questions; a pattern starting with "/var/⋯/log" - // matches concrete paths starting with "/var/anything/log" but - // strings.HasPrefix against the pattern text returns false for - // "/var/foo/log...". Same fix as wasPathOpenedWithSuffix above. - // CodeRabbit PR #43 open.go:79 (Also applies to 111-123). + // All entries retained. Scan concrete entries in Values first — + // exact strings.HasPrefix is correct for those. for openPath := range cp.Opens.Values { if strings.HasPrefix(openPath, prefixStr) { return types.Bool(true) } } + // Patterns: same narrower-fallback strategy as the suffix path. + // Split off the pattern's concrete head (the literal text + // BEFORE the first wildcard segment). If the pattern starts + // with a wildcard, concrete realisations could match ANY + // prefix — be permissive. Matthias upstream PR #811 review. + for _, openPattern := range cp.Opens.Patterns { + head := patternConcretePrefix(openPattern) + if head == "" { + return types.Bool(true) + } + if strings.HasPrefix(head, prefixStr) { + return types.Bool(true) + } + } return types.Bool(false) } // Projection applied — PrefixHits is authoritative; absent key = undeclared. @@ -185,3 +208,77 @@ func (l *apLibrary) wasPathOpenedWithPrefix(containerID, prefix ref.Val) ref.Val return types.Bool(hit) } +// patternConcreteSuffix returns the literal text at the tail of a +// wildcard-bearing path pattern, dropped to start after the LAST +// wildcard segment's trailing `/`. Returns the input unchanged when +// no wildcard segments are present, or "" when the pattern ends in +// a wildcard segment (concrete realisations could match any suffix). +// +// Examples: +// +// "/var/log/⋯/foo.log" → "foo.log" (last wildcard `⋯`, concrete tail follows) +// "/var/log/pods/*" → "" (trailing wildcard, permissive caller) +// "/var/log/foo.log" → "/var/log/foo.log" (no wildcards, whole pattern) +// "*" → "" (lone wildcard) +// +// Matthias upstream PR #811 review. +func patternConcreteSuffix(p string) string { + lastWildEnd := -1 + i := 0 + for i < len(p) { + segStart := i + for i < len(p) && p[i] != '/' { + i++ + } + seg := p[segStart:i] + if seg == dynamicpathdetector.WildcardIdentifier || seg == dynamicpathdetector.DynamicIdentifier { + lastWildEnd = i + } + if i < len(p) { + i++ // skip `/` + } + } + if lastWildEnd < 0 { + return p + } + if lastWildEnd >= len(p) { + return "" + } + // lastWildEnd points at the `/` after the wildcard segment. Keep + // the slash so callers querying with leading-slash suffixes match + // correctly (every concrete realisation has that slash too). + return p[lastWildEnd:] +} + +// patternConcretePrefix is the mirror of patternConcreteSuffix — +// returns the literal text at the HEAD of the pattern up to (but not +// including) the first wildcard segment. Returns the input unchanged +// when no wildcard segments are present, or "" when the pattern starts +// with a wildcard segment. +// +// Matthias upstream PR #811 review. +func patternConcretePrefix(p string) string { + i := 0 + for i < len(p) { + segStart := i + for i < len(p) && p[i] != '/' { + i++ + } + seg := p[segStart:i] + if seg == dynamicpathdetector.WildcardIdentifier || seg == dynamicpathdetector.DynamicIdentifier { + if segStart == 0 { + return "" + } + // segStart is at the wildcard segment; the byte BEFORE it + // is the `/` separator. Keep the slash in the returned + // prefix so callers querying with trailing-slash prefixes + // match (every concrete realisation has that slash too). + return p[:segStart] + } + if i < len(p) { + i++ // skip `/` + } + } + return p +} + diff --git a/pkg/rulemanager/cel/libraries/applicationprofile/open_bench_test.go b/pkg/rulemanager/cel/libraries/applicationprofile/open_bench_test.go new file mode 100644 index 000000000..29b740e0f --- /dev/null +++ b/pkg/rulemanager/cel/libraries/applicationprofile/open_bench_test.go @@ -0,0 +1,129 @@ +package applicationprofile + +import ( + "strconv" + "testing" + + "github.com/google/cel-go/common/types" + "github.com/kubescape/node-agent/pkg/objectcache" +) + +// BenchmarkWasPathOpenedWithSuffix_AllMode exercises the pass-through +// (Opens.All == true) suffix path under three representative profile +// shapes: +// +// - values_only: 50 concrete entries, no Patterns +// - patterns_concrete: 50 concrete entries + 10 Patterns whose tail +// is literal (the typical /var/log/⋯/foo.log shape) +// - patterns_wildcard: 50 concrete entries + 10 Patterns ending in a +// wildcard segment (the permissive-arm shape) +// +// Captures Matthias's upstream PR #811 contract numbers for the PR +// description. +func BenchmarkWasPathOpenedWithSuffix_AllMode(b *testing.B) { + shapes := []struct { + name string + values int + patterns []string + }{ + {"values_only", 50, nil}, + {"patterns_concrete", 50, []string{ + "/var/log/⋯/access.log", "/var/log/⋯/error.log", "/opt/⋯/server.log", + "/etc/⋯/audit.log", "/var/run/⋯/state.log", "/srv/⋯/app.log", + "/var/cache/⋯/tmp.log", "/usr/share/⋯/data.log", "/home/⋯/user.log", + "/proc/⋯/status.log", + }}, + {"patterns_wildcard", 50, []string{ + "/var/log/pods/*", "/var/log/containers/*", "/etc/cron.d/*", + "/opt/⋯", "/srv/*", "/var/run/*", + "/usr/local/⋯", "/home/⋯", "/tmp/⋯", "/run/⋯", + }}, + } + for _, sh := range shapes { + b.Run(sh.name, func(b *testing.B) { + values := make(map[string]struct{}, sh.values) + for i := 0; i < sh.values; i++ { + values["/usr/lib/x86_64-linux-gnu/libcrypto.so."+strconv.Itoa(i)] = struct{}{} + } + pcp := &objectcache.ProjectedContainerProfile{ + Opens: objectcache.ProjectedField{ + All: true, + Values: values, + Patterns: sh.patterns, + }, + } + lib := &apLibrary{objectCache: &mockObjectCacheForPattern{pcp: pcp}} + suffix := types.String(".log") + cid := types.String("bench-cid") + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + _ = lib.wasPathOpenedWithSuffix(cid, suffix) + } + }) + } +} + +// BenchmarkWasPathOpenedWithPrefix_AllMode mirrors the suffix bench +// for the prefix path. +func BenchmarkWasPathOpenedWithPrefix_AllMode(b *testing.B) { + shapes := []struct { + name string + values int + patterns []string + }{ + {"values_only", 50, nil}, + {"patterns_concrete", 50, []string{ + "/var/log/⋯/access.log", "/var/log/⋯/error.log", "/opt/⋯/server.log", + "/etc/⋯/audit.log", "/var/run/⋯/state.log", + }}, + {"patterns_wildcard", 50, []string{ + "*/run", "*/log", "*/cache", + "⋯", "*", + }}, + } + for _, sh := range shapes { + b.Run(sh.name, func(b *testing.B) { + values := make(map[string]struct{}, sh.values) + for i := 0; i < sh.values; i++ { + values["/usr/lib/x86_64-linux-gnu/libcrypto.so."+strconv.Itoa(i)] = struct{}{} + } + pcp := &objectcache.ProjectedContainerProfile{ + Opens: objectcache.ProjectedField{ + All: true, + Values: values, + Patterns: sh.patterns, + }, + } + lib := &apLibrary{objectCache: &mockObjectCacheForPattern{pcp: pcp}} + prefix := types.String("/var/") + cid := types.String("bench-cid") + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + _ = lib.wasPathOpenedWithPrefix(cid, prefix) + } + }) + } +} + +// BenchmarkPatternConcreteSuffix isolates the helper to confirm zero +// allocation regardless of pattern shape. +func BenchmarkPatternConcreteSuffix(b *testing.B) { + cases := []string{ + "/var/log/⋯/foo.log", + "/var/log/pods/*", + "/var/log/foo.log", + "*", + "/var/⋯/log/⋯/foo.log", + } + for _, c := range cases { + b.Run(c, func(b *testing.B) { + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + _ = patternConcreteSuffix(c) + } + }) + } +} diff --git a/pkg/rulemanager/cel/libraries/applicationprofile/open_test.go b/pkg/rulemanager/cel/libraries/applicationprofile/open_test.go index 9fce787ae..c0cf6f6e6 100644 --- a/pkg/rulemanager/cel/libraries/applicationprofile/open_test.go +++ b/pkg/rulemanager/cel/libraries/applicationprofile/open_test.go @@ -27,18 +27,24 @@ import ( // the correct mechanism — those are exercised in // TestOpenWithSuffixInProfile / TestOpenWithPrefixInProfile. // -// This test exercises the pass-through path directly by setting a -// ProjectedContainerProfile where Opens.All=true, Values contains a -// concrete path with the queried suffix, and Patterns contains a -// wildcard-pattern that ALSO appears to satisfy strings.HasSuffix -// against the queried suffix. The pattern must be ignored. -func TestWasPathOpenedWithSuffix_PatternsNotScanned(t *testing.T) { - // Pass-through pcp (Opens.All=true): - // Values: ["/var/log/concrete.log"] — concrete, ends with ".log" - // Patterns: ["/var/log/⋯/foo.log"] — wildcard, ALSO ends with ".log" - // Querying suffix=".log" should match Values; we then strip - // concrete.log from Values and assert suffix doesn't match - // through Patterns alone. +// These tests pin Matthias's upstream PR #811 review contract: +// +// Patterns ARE scanned when Opens.All == true, but with a NARROWER +// fallback than text-level strings.HasSuffix/HasPrefix: +// +// * Pattern with a concrete tail/head (text after/before the last/first +// wildcard segment): match via HasSuffix/HasPrefix on the concrete +// piece — every realisation has that text textually. +// * Pattern ending/starting with a wildcard segment: be PERMISSIVE +// (return true). The concrete realisations could match ANY +// suffix/prefix; refusing would silently regress rules that omit +// profileDataRequired.opens (Matthias's "we still need a narrower +// fallback here instead of ignoring Patterns entirely"). +// +// Pre-PR-#811 (CR's HasSuffix-on-Patterns concern) the matcher SKIPPED +// Patterns entirely. That made wildcard-only profiles silently fail +// suffix/prefix queries — the regression Matthias's review reverses. +func TestWasPathOpenedWithSuffix_PatternsScannedWithConcreteTail(t *testing.T) { pcp := &objectcache.ProjectedContainerProfile{ Opens: objectcache.ProjectedField{ All: true, @@ -49,27 +55,57 @@ func TestWasPathOpenedWithSuffix_PatternsNotScanned(t *testing.T) { objCache := &mockObjectCacheForPattern{pcp: pcp} lib := &apLibrary{objectCache: objCache} - // 1) With concrete in Values: returns true. + // 1) Concrete in Values: returns true via Values scan. got := lib.wasPathOpenedWithSuffix(types.String("test-cid"), types.String(".log")) if b, _ := got.Value().(bool); !b { t.Fatalf("suffix '.log' against concrete /var/log/concrete.log: expected true, got %v", got) } - // 2) Strip Values; only the wildcard Pattern remains. Suffix '.log' - // text-matches the pattern but the pattern is wildcardised — the - // correct answer is false (no concrete observation supports it). + // 2) Strip Values; only the wildcard Pattern remains. The pattern's + // concrete tail (text after the last wildcard segment) is + // "/foo.log" which DOES end with ".log" → expect true. pcp.Opens.Values = map[string]struct{}{} got = lib.wasPathOpenedWithSuffix(types.String("test-cid"), types.String(".log")) + if b, _ := got.Value().(bool); !b { + t.Errorf("suffix '.log' against pattern /var/log/⋯/foo.log: "+ + "expected true (concrete tail '/foo.log' has suffix '.log'), got %v", got) + } + + // 3) Same pattern, query suffix that DOESN'T match the concrete tail. + got = lib.wasPathOpenedWithSuffix(types.String("test-cid"), types.String(".txt")) if b, _ := got.Value().(bool); b { - t.Errorf("suffix '.log' against ONLY wildcard pattern /var/log/⋯/foo.log: "+ - "expected false (patterns must not be scanned), got %v", got) + t.Errorf("suffix '.txt' against pattern /var/log/⋯/foo.log: "+ + "expected false (concrete tail '/foo.log' doesn't have suffix '.txt'), got %v", got) + } +} + +// TestWasPathOpenedWithSuffix_PatternWildcardTail_Permissive pins the +// permissive arm of Matthias's contract: a pattern ending in a wildcard +// segment can match ANY suffix because its concrete realisations are +// unconstrained at the tail. +func TestWasPathOpenedWithSuffix_PatternWildcardTail_Permissive(t *testing.T) { + pcp := &objectcache.ProjectedContainerProfile{ + Opens: objectcache.ProjectedField{ + All: true, + Values: map[string]struct{}{}, + Patterns: []string{"/var/log/pods/*"}, + }, + } + objCache := &mockObjectCacheForPattern{pcp: pcp} + lib := &apLibrary{objectCache: objCache} + + for _, suffix := range []string{".log", "/foo.log", "kube-system"} { + got := lib.wasPathOpenedWithSuffix(types.String("test-cid"), types.String(suffix)) + if b, _ := got.Value().(bool); !b { + t.Errorf("suffix %q against pattern /var/log/pods/*: "+ + "expected true (permissive — wildcard tail), got %v", suffix, got) + } } } -// TestWasPathOpenedWithPrefix_PatternsNotScanned mirrors the suffix -// test for the prefix path. Same rabbit finding (open.go:79 Also -// applies to: 111-123). -func TestWasPathOpenedWithPrefix_PatternsNotScanned(t *testing.T) { +// TestWasPathOpenedWithPrefix_PatternsScannedWithConcreteHead mirrors +// the suffix test for the prefix path. +func TestWasPathOpenedWithPrefix_PatternsScannedWithConcreteHead(t *testing.T) { pcp := &objectcache.ProjectedContainerProfile{ Opens: objectcache.ProjectedField{ All: true, @@ -85,11 +121,69 @@ func TestWasPathOpenedWithPrefix_PatternsNotScanned(t *testing.T) { t.Fatalf("prefix '/var/' against concrete /var/concrete/foo: expected true, got %v", got) } + // Strip Values; the pattern's concrete head is "/var/" which DOES + // start with the queried prefix "/var/" → expect true. pcp.Opens.Values = map[string]struct{}{} got = lib.wasPathOpenedWithPrefix(types.String("test-cid"), types.String("/var/")) + if b, _ := got.Value().(bool); !b { + t.Errorf("prefix '/var/' against pattern /var/⋯/log/foo: "+ + "expected true (concrete head '/var/' starts with '/var/'), got %v", got) + } + + // Query prefix that doesn't match the concrete head. + got = lib.wasPathOpenedWithPrefix(types.String("test-cid"), types.String("/etc/")) if b, _ := got.Value().(bool); b { - t.Errorf("prefix '/var/' against ONLY wildcard pattern /var/⋯/log/foo: "+ - "expected false (patterns must not be scanned), got %v", got) + t.Errorf("prefix '/etc/' against pattern /var/⋯/log/foo: "+ + "expected false (concrete head '/var/' doesn't start with '/etc/'), got %v", got) + } +} + +// TestWasPathOpenedWithPrefix_PatternWildcardHead_Permissive pins the +// permissive arm of the prefix path. +func TestWasPathOpenedWithPrefix_PatternWildcardHead_Permissive(t *testing.T) { + pcp := &objectcache.ProjectedContainerProfile{ + Opens: objectcache.ProjectedField{ + All: true, + Values: map[string]struct{}{}, + Patterns: []string{"*/run"}, + }, + } + objCache := &mockObjectCacheForPattern{pcp: pcp} + lib := &apLibrary{objectCache: objCache} + + for _, prefix := range []string{"/var/lib", "/etc", "/anything"} { + got := lib.wasPathOpenedWithPrefix(types.String("test-cid"), types.String(prefix)) + if b, _ := got.Value().(bool); !b { + t.Errorf("prefix %q against pattern */run: "+ + "expected true (permissive — wildcard head), got %v", prefix, got) + } + } +} + +// TestPatternConcreteSuffix_AndPrefix pins the helper-level contract +// for the narrower-fallback splitters. Standalone test on the helpers +// so failures localise to the splitter logic rather than the matcher. +func TestPatternConcreteSuffix_AndPrefix(t *testing.T) { + cases := []struct { + name, in, wantSuffix, wantPrefix string + }{ + {"no_wildcards", "/var/log/foo.log", "/var/log/foo.log", "/var/log/foo.log"}, + {"trailing_star", "/var/log/pods/*", "", "/var/log/pods/"}, + {"leading_star", "*/run", "/run", ""}, + {"mid_ellipsis", "/var/⋯/log", "/log", "/var/"}, + {"both_mid", "/var/⋯/log/⋯/foo.log", "/foo.log", "/var/"}, + {"lone_star", "*", "", ""}, + {"lone_ellipsis", "⋯", "", ""}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := patternConcreteSuffix(tc.in); got != tc.wantSuffix { + t.Errorf("patternConcreteSuffix(%q) = %q, want %q", tc.in, got, tc.wantSuffix) + } + if got := patternConcretePrefix(tc.in); got != tc.wantPrefix { + t.Errorf("patternConcretePrefix(%q) = %q, want %q", tc.in, got, tc.wantPrefix) + } + }) } } From 9e79d1b04e2cf921fb006f2b3ec52107a5a9da7a Mon Sep 17 00:00:00 2001 From: Entlein Date: Wed, 20 May 2026 10:56:16 +0200 Subject: [PATCH 2/2] =?UTF-8?q?exec:=20exact-path=20args=20constraint=20is?= =?UTF-8?q?=20authoritative=20=E2=80=94=20no=20fall-through?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- .../cel/libraries/applicationprofile/exec.go | 28 +++++-- .../applicationprofile/exec_bench_test.go | 79 +++++++++++++++++++ 2 files changed, 99 insertions(+), 8 deletions(-) create mode 100644 pkg/rulemanager/cel/libraries/applicationprofile/exec_bench_test.go diff --git a/pkg/rulemanager/cel/libraries/applicationprofile/exec.go b/pkg/rulemanager/cel/libraries/applicationprofile/exec.go index 10e8d4c97..578b9b8df 100644 --- a/pkg/rulemanager/cel/libraries/applicationprofile/exec.go +++ b/pkg/rulemanager/cel/libraries/applicationprofile/exec.go @@ -121,16 +121,28 @@ func (l *apLibrary) wasExecutedWithArgs(containerID, path, args ref.Val) ref.Val // wildcard token in Args. if _, ok := cp.Execs.Values[pathStr]; ok { if profileArgs, ok := cp.ExecsByPath[pathStr]; ok { - if dynamicpathdetector.CompareExecArgs(profileArgs, runtimeArgs) { - return types.Bool(true) - } - } else { - // State 2: ExecsByPath absent → back-compat "no argv constraint". - return types.Bool(true) + // Exact path IS in the profile WITH an authored argv + // constraint. The constraint is authoritative — its match + // result IS the answer for this path. Do NOT fall through + // to pattern matching: a looser pattern (e.g. `/usr/bin/*` + // with profileArgs `[*]`) would silently override the + // exact-path's stricter args constraint, defeating + // rule-author intent. + // + // CodeRabbit upstream PR #43 exec.go:144 (Major). Before + // this fix the function fell through on CompareExecArgs + // false, letting a permissive Pattern shadow the strict + // exact-path constraint. + return types.Bool(dynamicpathdetector.CompareExecArgs(profileArgs, runtimeArgs)) } + // State 2: ExecsByPath absent → back-compat "no argv constraint". + return types.Bool(true) } - // Pattern path match: dynamic-segment paths in cp.Execs.Patterns. - // Args matching mirrors the exact-path case. + // Path NOT in Values — try Pattern path match: dynamic-segment paths + // in cp.Execs.Patterns. Args matching mirrors the exact-path case + // EXCEPT here the pattern is the only constraint, so a mismatched + // args fall-through to the NEXT pattern is acceptable (different + // patterns can encode different (path, args) shapes). for _, execPath := range cp.Execs.Patterns { if dynamicpathdetector.CompareDynamic(execPath, pathStr) { if profileArgs, ok := cp.ExecsByPath[execPath]; ok { diff --git a/pkg/rulemanager/cel/libraries/applicationprofile/exec_bench_test.go b/pkg/rulemanager/cel/libraries/applicationprofile/exec_bench_test.go new file mode 100644 index 000000000..c9cea80c6 --- /dev/null +++ b/pkg/rulemanager/cel/libraries/applicationprofile/exec_bench_test.go @@ -0,0 +1,79 @@ +package applicationprofile + +import ( + "testing" + + "github.com/google/cel-go/common/types" + "github.com/kubescape/node-agent/pkg/objectcache" +) + + +// BenchmarkWasExecutedWithArgs covers the R0040 hot path in the four +// representative shapes: +// +// - exact_path_args_match: literal path in Values + matching argv +// - exact_path_args_mismatch: literal path in Values + mismatching argv +// (post-tier-2 fix: returns false without +// falling through to pattern matching) +// - pattern_path_args_match: path absent from Values, matches a Pattern, +// ExecsByPath has the matching argv +// - no_match: path absent from Values AND no Pattern +// matches (terminal false) +// +// Reports allocs/op for the steady-state matcher. The CEL value-wrapping +// floor is unchanged by this PR — only the exec.go control flow changed +// (no fall-through on exact-path strict-args mismatch). +func BenchmarkWasExecutedWithArgs(b *testing.B) { + values := map[string]struct{}{ + "/usr/bin/curl": {}, + "/usr/sbin/sshd": {}, + "/bin/ls": {}, + "/usr/bin/python": {}, + } + patterns := []string{ + "/usr/local/bin/⋯", + "/opt/⋯/bin/⋯", + } + execsByPath := map[string][]string{ + "/usr/bin/curl": {"-X", "GET", "*"}, + "/usr/sbin/sshd": {"-D"}, + "/bin/ls": {"-la", "⋯"}, + "/usr/bin/python": {"⋯", "*"}, + "/usr/local/bin/⋯": {"*"}, + "/opt/⋯/bin/⋯": {"*"}, + } + pcp := &objectcache.ProjectedContainerProfile{ + Execs: objectcache.ProjectedField{ + All: true, + Values: values, + Patterns: patterns, + }, + ExecsByPath: execsByPath, + } + lib := &apLibrary{objectCache: &mockObjectCacheForPattern{pcp: pcp}} + + cases := []struct { + name, path string + args []string + }{ + {"exact_path_args_match", "/usr/bin/curl", []string{"-X", "GET", "https://api.example.com"}}, + {"exact_path_args_mismatch", "/usr/bin/curl", []string{"-X", "DELETE", "https://api.example.com"}}, + {"pattern_path_args_match", "/usr/local/bin/myhelper", []string{"--config", "/etc/myhelper.yaml"}}, + // `no_match` would fall through to isExecInPodSpec which needs + // K8sObjectCache wiring — out of scope for a matcher bench. + } + cid := types.String("bench-cid") + for _, c := range cases { + path := types.String(c.path) + // Encode argv as a CEL list — let the matcher parse it like + // the real CEL call site does. + argsRefVal := types.NewStringList(types.DefaultTypeAdapter, c.args) + b.Run(c.name, func(b *testing.B) { + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + _ = lib.wasExecutedWithArgs(cid, path, argsRefVal) + } + }) + } +}