Skip to content

fix(cel): scan Opens.Patterns in was_path_opened_with_suffix/prefix (#98) - #99

Open
ConstanzeTU wants to merge 1 commit into
mainfrom
fix/suffix-prefix-scan-patterns
Open

fix(cel): scan Opens.Patterns in was_path_opened_with_suffix/prefix (#98)#99
ConstanzeTU wants to merge 1 commit into
mainfrom
fix/suffix-prefix-scan-patterns

Conversation

@ConstanzeTU

Copy link
Copy Markdown

Draft / stub for discussion. Fixes #98.

The failure

R0006 gates on !cp.was_path_opened_with_suffix(containerId, '/token').

The kubelet writes projected volumes through an atomic writer, so a ServiceAccount token is read at …/serviceaccount/..2026_08_27_14_27_52.163845901/token. The timestamped segment is volatile, dynamicpathdetector collapses it, and a correctly learned profile stores

/run/secrets/kubernetes.io/serviceaccount/⋯/token

Because that entry contains a collapse token it lives in Opens.Patterns. The Opens.All branch of the helper scanned Opens.Values only, answered false, and R0006 fired on every token read for the life of the workload. R0008 is the same mechanism via /proc/⋯/environ.

Nothing in the profile or the rule is wrong — the helper simply did not consult what was recorded.

Why scanning Patterns is safe rather than a widening

A pattern's trailing segments after the last collapse token are literal. If they end with the queried suffix, every concrete path that pattern stands for ends with it too. A pattern whose leaf is itself a wildcard (/var/log/pods/⋯) returns false — the same answer as skipping it — so the change is never worse than the previous behaviour. The prefix side is the mirror image: the segments before the first collapse token are literal, so /var/⋯/log genuinely does have prefix /var/.

The removed rationale did not support the code it justified. It warned that HasSuffix on a pattern "returns false and produces a false negative" — and then avoided that by skipping Patterns, which also returns false. The blanket skip guaranteed the false negative it was meant to prevent, and did so for concrete-leaf patterns where HasSuffix would have been correct.

The two branches disagreed

With projection active, projection_apply.go builds SuffixHits/PrefixHits with HasSuffix/HasPrefix over every raw entry including dynamic ones. So the projected branch already answered true for ⋯/token; only Opens.All disagreed. Now pinned by TestSuffix_AllBranchAgreesWithProjectedBranch.

Tests

open_atomicwriter_test.go (new). Verified against unmodified main first:

test on main after
TestSuffix_AtomicWriterServiceAccountToken FAIL PASS
TestSuffix_ProcfsEnviron FAIL PASS
TestPrefix_ConcreteHeadOfPattern FAIL PASS
TestSuffix_AllBranchAgreesWithProjectedBranch FAIL PASS
TestSuffix_WildcardLeafStillUnmatched PASS PASS
TestPrefix_UnrelatedHeadStillUnmatched PASS PASS
TestSuffix_ConcreteValueStillMatches PASS PASS

The three that pass in both columns are the point: they show the fix does not widen matching.

⚠️ This changes a deliberately-pinned contract — look here first

Issue #98 does not mention it, and it is the reason this is a contract change rather than a straightforward oversight.

TestWasPathOpenedWithSuffix_PatternsNotScanned and TestWasPathOpenedWithPrefix_PatternsNotScanned existed specifically to assert "patterns must not be scanned" (from the CodeRabbit PR #43 review on open.go:79). They are replaced by _ConcreteLeafPatternMatches / _ConcreteHeadPatternMatches, which keep the wildcard-leaf and past-the-collapse-token cases asserting false and add the concrete-leaf/head cases asserting true.

Worth noting what the old prefix test asserted: that /var/⋯/log/foo does not have prefix /var/. It literally does, and so does every concrete path it stands for.

If the reviewers want the old contract preserved, the alternative is to leave the helper alone and have R0006/R0008 declare their suffixes so projection is always active — but that leaves two branches of one helper permanently disagreeing, which seems worse.

Regression check

./pkg/rulemanager/cel/libraries/containerprofile/... and ./pkg/objectcache/... all pass, including the pre-existing TestOpenWithSuffixInProfile / TestOpenWithPrefixInProfile and containerprofilecache (2.459s). go build ./... clean, gofmt clean.

Not covered here

No component/e2e test yet — reproducing needs a real cluster with a learned profile and a workload reading its token. Happy to add one under tests/ if you want it before this leaves draft.

The Opens.All branch of both helpers scanned Opens.Values only. Volatile paths
are always stored as Patterns, so a correctly learned ContainerProfile records
the kubelet atomic-writer ServiceAccount token open as

  /run/secrets/kubernetes.io/serviceaccount/⋯/token

with the timestamped directory collapsed and the /token leaf preserved. R0006
gates on !cp.was_path_opened_with_suffix(containerId, '/token'), the helper
answered false for that profile, and the rule fired on every token read for the
life of the workload. R0008 has the same shape via /proc/⋯/environ.

Fixes #98.

Why scanning Patterns is safe, not a widening:

A pattern's trailing segments after the last collapse token are literal. If they
end with the queried suffix then every concrete path the pattern stands for ends
with it too, so HasSuffix answers a real question. A pattern whose leaf is itself
a wildcard ("/var/log/pods/⋯") returns false — the same answer as skipping it —
so this is never worse than the previous behaviour. The prefix side is the
mirror image: the segments before the first collapse token are literal.

The rationale in the removed comment did not support the code it justified. It
warned that HasSuffix on a pattern "returns false and produces a false negative",
then avoided that by skipping Patterns — which returns false as well. The blanket
skip therefore guaranteed the false negative it was meant to prevent, and did so
for concrete-leaf patterns too, where HasSuffix would have been correct.

This also makes the helper self-consistent. With projection active,
projection_apply.go builds SuffixHits/PrefixHits with HasSuffix/HasPrefix over
every raw entry INCLUDING dynamic ones, so the projected branch already answered
true for "⋯/token". Only the Opens.All branch disagreed. Pinned by
TestSuffix_AllBranchAgreesWithProjectedBranch.

Tests:

  open_atomicwriter_test.go — new. Four cases fail on unmodified main
  (SA-token suffix, procfs environ suffix, concrete-head prefix, and the
  two-branch agreement check) and pass after. Three guard cases
  (wildcard-leaf suffix, unrelated-head prefix, concrete Values) pass both
  before and after, so the fix is shown not to widen matching.

  open_test.go — TestWasPathOpenedWithSuffix_PatternsNotScanned and
  TestWasPathOpenedWithPrefix_PatternsNotScanned pinned the old contract and
  are replaced by _ConcreteLeafPatternMatches / _ConcreteHeadPatternMatches.
  These keep the wildcard-leaf and past-the-collapse-token cases asserting
  false, and add the concrete-leaf/head cases asserting true. Issue #98 does
  not mention these tests; they are the reason this is a deliberate contract
  change rather than an oversight, and a reviewer should look here first.

Regression check: ./pkg/rulemanager/cel/libraries/containerprofile/... and
./pkg/objectcache/... all pass, including the pre-existing
TestOpenWithSuffixInProfile / TestOpenWithPrefixInProfile projection tests and
containerprofilecache (2.459s).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

wasPathOpenedWithSuffix skips Opens.Patterns → R0006/R0008 false-positive on every atomic-writer SA-token / procfs read

2 participants