diff --git a/pkg/rulemanager/cel/libraries/containerprofile/open.go b/pkg/rulemanager/cel/libraries/containerprofile/open.go index 45d89634de..84bfd89f1b 100644 --- a/pkg/rulemanager/cel/libraries/containerprofile/open.go +++ b/pkg/rulemanager/cel/libraries/containerprofile/open.go @@ -112,21 +112,40 @@ func (l *containerProfileLibrary) wasPathOpenedWithSuffix(containerID, suffix re } 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). + // All entries retained (no rule declared SuffixHits-style projection). + // Scan Values first, then Patterns. + // + // Patterns must be scanned. Volatile paths are ALWAYS stored as patterns, + // so a correctly learned profile records the kubelet atomic-writer token + // open as "/run/secrets/kubernetes.io/serviceaccount/⋯/token" — the + // timestamped directory collapses, the "/token" leaf survives. Skipping + // Patterns answers "no" for that profile and R0006 fires on every + // SA-token read for the life of the workload; R0008 does the same via + // "/proc/⋯/environ". See issue #98. + // + // strings.HasSuffix against pattern text is sound for the concrete + // suffixes rules actually query: a pattern's trailing segments after the + // last collapse token are literal, so if they end with the suffix then + // every concrete path the pattern stands for ends with it too. A pattern + // whose LEAF is itself a wildcard ("/var/log/pods/⋯") simply returns + // false — the same answer as not scanning it, so this is never worse than + // the previous behaviour. + // + // This also makes the two branches of this helper agree. When projection + // is active, projection_apply.go builds SuffixHits with strings.HasSuffix + // over every raw entry INCLUDING dynamic ones, so the projected branch + // already answers true for "⋯/token". The Opens.All branch answering + // false for the same profile was the inconsistency. for openPath := range cp.Opens.Values { if strings.HasSuffix(openPath, suffixStr) { return types.Bool(true) } } + for _, openPath := range cp.Opens.Patterns { + if strings.HasSuffix(openPath, suffixStr) { + return types.Bool(true) + } + } return types.Bool(false) } // Projection applied — SuffixHits is authoritative; absent key = undeclared. @@ -160,18 +179,25 @@ func (l *containerProfileLibrary) wasPathOpenedWithPrefix(containerID, prefix re } 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 Values, then Patterns. Symmetric with + // wasPathOpenedWithSuffix above; see issue #98. + // + // A pattern's segments BEFORE the first collapse token are literal, so + // strings.HasPrefix is sound for the concrete prefixes rules query: if the + // pattern text starts with the prefix, every concrete path that pattern + // stands for starts with it too. "/var/⋯/log" genuinely does have prefix + // "/var/". A prefix that would reach past the first collapse token simply + // fails to match — the same answer as not scanning, never worse. for openPath := range cp.Opens.Values { if strings.HasPrefix(openPath, prefixStr) { return types.Bool(true) } } + for _, openPath := range cp.Opens.Patterns { + if strings.HasPrefix(openPath, prefixStr) { + return types.Bool(true) + } + } return types.Bool(false) } // Projection applied — PrefixHits is authoritative; absent key = undeclared. diff --git a/pkg/rulemanager/cel/libraries/containerprofile/open_atomicwriter_test.go b/pkg/rulemanager/cel/libraries/containerprofile/open_atomicwriter_test.go new file mode 100644 index 0000000000..f563aff694 --- /dev/null +++ b/pkg/rulemanager/cel/libraries/containerprofile/open_atomicwriter_test.go @@ -0,0 +1,148 @@ +package containerprofile + +import ( + "testing" + + "github.com/google/cel-go/common/types" + "github.com/kubescape/node-agent/pkg/objectcache" +) + +// The kubelet writes projected volumes through an "atomic writer": the real files +// live under a timestamped directory that is replaced wholesale on rotation, and a +// `..data` symlink points at the current one. A ServiceAccount token is therefore +// read at a path like +// +// /run/secrets/kubernetes.io/serviceaccount/..2026_08_27_14_27_52.163845901/token +// +// The timestamped segment is volatile, so dynamicpathdetector collapses it and the +// learned ContainerProfile records +// +// /run/secrets/kubernetes.io/serviceaccount/⋯/token +// +// which — because it contains a collapse token — is stored in Opens.Patterns, not +// Opens.Values. +// +// R0006 gates on `!cp.was_path_opened_with_suffix(containerId, '/token')`. With the +// Opens.All branch scanning Values only, a correctly-learned profile answers "no" +// and R0006 fires on every SA-token read for the life of the workload. R0008 has the +// same shape via /proc/⋯/environ. +// +// These tests pin the behaviour the rules actually need. They fail before the +// Patterns scan is added to wasPathOpenedWithSuffix / wasPathOpenedWithPrefix. +// See k8sstormcenter/node-agent#98. + +func newPatternProfile(patterns []string, values ...string) *containerProfileLibrary { + vals := map[string]struct{}{} + for _, v := range values { + vals[v] = struct{}{} + } + if len(vals) == 0 { + vals = nil + } + pcp := &objectcache.ProjectedContainerProfile{ + Opens: objectcache.ProjectedField{ + All: true, + Values: vals, + Patterns: patterns, + }, + } + return &containerProfileLibrary{objectCache: &mockObjectCacheForPattern{pcp: pcp}} +} + +func boolOf(t *testing.T, v interface{ Value() any }) bool { + t.Helper() + b, ok := v.Value().(bool) + if !ok { + t.Fatalf("expected a bool result, got %T (%v)", v.Value(), v) + } + return b +} + +// R0006: the profile records the token open as a pattern with a concrete leaf. +func TestSuffix_AtomicWriterServiceAccountToken(t *testing.T) { + lib := newPatternProfile([]string{ + "/run/secrets/kubernetes.io/serviceaccount/⋯/token", + }) + if !boolOf(t, lib.wasPathOpenedWithSuffix(types.String("cid"), types.String("/token"))) { + t.Error("suffix '/token' against recorded pattern " + + "'/run/secrets/kubernetes.io/serviceaccount/⋯/token': expected true. " + + "Returning false makes R0006 fire on every SA-token read of a correctly " + + "learned profile (issue #98)") + } +} + +// R0008: same mechanism, /proc//environ. +func TestSuffix_ProcfsEnviron(t *testing.T) { + lib := newPatternProfile([]string{"/proc/⋯/environ"}) + if !boolOf(t, lib.wasPathOpenedWithSuffix(types.String("cid"), types.String("/environ"))) { + t.Error("suffix '/environ' against recorded pattern '/proc/⋯/environ': " + + "expected true (R0008 false-positive otherwise)") + } +} + +// A pattern whose LEAF is itself a wildcard cannot answer a concrete suffix +// question. HasSuffix returns false, which is the same answer as skipping the +// pattern entirely — so scanning Patterns is never worse than not scanning them. +func TestSuffix_WildcardLeafStillUnmatched(t *testing.T) { + lib := newPatternProfile([]string{"/var/log/pods/⋯"}) + if boolOf(t, lib.wasPathOpenedWithSuffix(types.String("cid"), types.String("/foo.log"))) { + t.Error("suffix '/foo.log' against wildcard-leaf pattern '/var/log/pods/⋯': " + + "expected false; the pattern text cannot answer this") + } +} + +// Prefix side: the segments before the first collapse token are concrete, so every +// concrete path the pattern stands for really does start with them. +func TestPrefix_ConcreteHeadOfPattern(t *testing.T) { + lib := newPatternProfile([]string{"/run/secrets/kubernetes.io/serviceaccount/⋯/token"}) + if !boolOf(t, lib.wasPathOpenedWithPrefix(types.String("cid"), + types.String("/run/secrets/"))) { + t.Error("prefix '/run/secrets/' against pattern " + + "'/run/secrets/kubernetes.io/serviceaccount/⋯/token': expected true; " + + "the pattern head is concrete") + } +} + +func TestPrefix_UnrelatedHeadStillUnmatched(t *testing.T) { + lib := newPatternProfile([]string{"/run/secrets/kubernetes.io/serviceaccount/⋯/token"}) + if boolOf(t, lib.wasPathOpenedWithPrefix(types.String("cid"), types.String("/etc/"))) { + t.Error("prefix '/etc/' against a /run/... pattern: expected false") + } +} + +// Values must keep working, and must still win without consulting Patterns. +func TestSuffix_ConcreteValueStillMatches(t *testing.T) { + lib := newPatternProfile(nil, "/var/log/concrete.log") + if !boolOf(t, lib.wasPathOpenedWithSuffix(types.String("cid"), types.String(".log"))) { + t.Error("suffix '.log' against concrete value '/var/log/concrete.log': expected true") + } +} + +// The two code paths for the same helper must agree. projection_apply.go builds +// SuffixHits with strings.HasSuffix over EVERY raw entry including dynamic ones, so +// a projected profile already answers true for ⋯/token. The Opens.All branch +// answering false for the same profile is the inconsistency issue #98 reports. +func TestSuffix_AllBranchAgreesWithProjectedBranch(t *testing.T) { + const entry = "/run/secrets/kubernetes.io/serviceaccount/⋯/token" + const suffix = "/token" + + all := newPatternProfile([]string{entry}) + allAnswer := boolOf(t, all.wasPathOpenedWithSuffix(types.String("cid"), types.String(suffix))) + + // what projection_apply.go would compute for the same raw entry + projected := &containerProfileLibrary{objectCache: &mockObjectCacheForPattern{ + pcp: &objectcache.ProjectedContainerProfile{ + Opens: objectcache.ProjectedField{ + All: false, + SuffixHits: map[string]bool{suffix: true}, + }, + }, + }} + projectedAnswer := boolOf(t, + projected.wasPathOpenedWithSuffix(types.String("cid"), types.String(suffix))) + + if allAnswer != projectedAnswer { + t.Errorf("same profile, two code paths, different answers: "+ + "Opens.All branch=%v, projected branch=%v", allAnswer, projectedAnswer) + } +} diff --git a/pkg/rulemanager/cel/libraries/containerprofile/open_test.go b/pkg/rulemanager/cel/libraries/containerprofile/open_test.go index 8907194fb9..40f8eca4e3 100644 --- a/pkg/rulemanager/cel/libraries/containerprofile/open_test.go +++ b/pkg/rulemanager/cel/libraries/containerprofile/open_test.go @@ -13,32 +13,33 @@ import ( "github.com/stretchr/testify/assert" ) -// TestWasPathOpenedWithSuffix_PatternsNotScanned pins the contract from -// the CodeRabbit PR #43 review on open.go:79 (Major). Wildcard-shaped -// entries in cp.Opens.Patterns MUST NOT contribute to suffix/prefix -// answers — their literal text answers the wrong question. A retained -// pattern "/var/log/pods/*/volumes/...." doesn't END with "foo.log" -// even though the concrete open it stands in for might. Only concrete -// paths in cp.Opens.Values are valid sources of suffix/prefix truth in -// pass-through (Opens.All=true) mode. +// These two tests previously pinned the opposite contract — that Patterns must +// never contribute to suffix/prefix answers (CodeRabbit PR #43 review on +// open.go:79). That contract caused a permanent false positive: volatile paths are +// always stored as Patterns, so a correctly learned profile records the kubelet +// atomic-writer SA-token open as "/run/secrets/.../serviceaccount/⋯/token" and the +// helper answered "not opened", firing R0006 on every read. Issue #98. // -// In projection-active mode (Opens.All=false), the rule manager -// precomputes Opens.SuffixHits / PrefixHits from the spec, which is -// the correct mechanism — those are exercised in -// TestOpenWithSuffixInProfile / TestOpenWithPrefixInProfile. +// The stated rationale did not support the code it justified. It warned that +// strings.HasSuffix on a pattern "returns false and produces a false negative" — +// but skipping Patterns returns false too, so the blanket skip GUARANTEED the very +// false negative it was meant to avoid, for concrete-leaf patterns as well as +// wildcard-leaf ones. // -// 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. +// The corrected contract, pinned below: +// +// - a pattern whose trailing text after the last collapse token ends with the +// queried suffix DOES answer true — every concrete path it stands for ends +// that way ("⋯/token" really does end with "/token"); +// - a pattern whose LEAF is a wildcard still answers false, which is exactly the +// old behaviour, so scanning Patterns is never worse than skipping them; +// - the same, mirrored, for prefixes against a pattern's concrete head. +// +// This also brings the Opens.All branch into agreement with the projected branch, +// where projection_apply.go already computes SuffixHits/PrefixHits over every raw +// entry including dynamic ones. + +func TestWasPathOpenedWithSuffix_ConcreteLeafPatternMatches(t *testing.T) { pcp := &objectcache.ProjectedContainerProfile{ Opens: objectcache.ProjectedField{ All: true, @@ -49,27 +50,30 @@ func TestWasPathOpenedWithSuffix_PatternsNotScanned(t *testing.T) { objCache := &mockObjectCacheForPattern{pcp: pcp} lib := &containerProfileLibrary{objectCache: objCache} - // 1) With concrete in Values: returns true. + // concrete Values entry still answers 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). + // with Values emptied, the concrete-leaf pattern must now answer 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 concrete-leaf pattern /var/log/⋯/foo.log: "+ + "expected true (its leaf really does end in .log), got %v", got) + } + + // a wildcard LEAF still cannot answer — unchanged from the old behaviour + pcp.Opens.Patterns = []string{"/var/log/pods/⋯"} + got = lib.wasPathOpenedWithSuffix(types.String("test-cid"), types.String(".log")) 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 '.log' against wildcard-leaf pattern /var/log/pods/⋯: "+ + "expected false, got %v", 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) { +func TestWasPathOpenedWithPrefix_ConcreteHeadPatternMatches(t *testing.T) { pcp := &objectcache.ProjectedContainerProfile{ Opens: objectcache.ProjectedField{ All: true, @@ -85,11 +89,18 @@ func TestWasPathOpenedWithPrefix_PatternsNotScanned(t *testing.T) { t.Fatalf("prefix '/var/' against concrete /var/concrete/foo: expected true, got %v", got) } + // with Values emptied, the pattern's concrete head must answer 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 "+ + "(the head before the collapse token is literal), got %v", got) + } + + // a prefix reaching past the collapse token still cannot be answered + got = lib.wasPathOpenedWithPrefix(types.String("test-cid"), types.String("/var/spool/")) 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 '/var/spool/' against pattern /var/⋯/log/foo: expected false, got %v", got) } }