feat(cel): was_path_opened* CEL helpers in applicationprofile library#811
feat(cel): was_path_opened* CEL helpers in applicationprofile library#811entlein wants to merge 4 commits into
Conversation
Signed-off-by: entlein <einentlein@gmail.com>
|
Warning Review limit reached
More reviews will be available in 59 minutes and 56 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthroughThe PR refines path-pattern matching in the applicationprofile CEL library to correctly evaluate suffix/prefix patterns containing wildcard segments. Two new helper functions extract concrete literal portions from dynamic patterns; matching becomes permissive when patterns end or start with wildcards. Unit tests, benchmarks, and Kubernetes deployments validate the new behavior. ChangesWildcard-aware path-pattern matching
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
|
@coderabbitai review Collective re-review request after applying your atomic-split advisory + rabbit-feedback fixes across the full SBOB upstream PR set. Each PR is now scoped to a single tier per your dependency map: Tier 0 (opens):
Tier 2 (execs):
Tier 3 (network):
Tier 4 (tamper/sign):
Umbrellas:
Please confirm atomicity per your earlier advisory and flag any remaining cross-PR coupling we missed. |
🧩 Analysis chain🏁 Script executed: Length of output: 1979 🏁 Script executed: Length of output: 29788
One soft compile-time coupling to flag: Triggering a full review now. Findings follow below. Review findings1.
|
| This PR → | Coupling type | Safe to merge independently? |
|---|---|---|
kubescape/storage#323 |
Soft compile-time (field shapes on cp.Opens) |
✅ Yes, provided field names are stable |
kubescape/node-agent#805 (exec) |
None | ✅ |
kubescape/node-agent#807 (R0040) |
None | ✅ |
kubescape/node-agent#812 (ExecsByPath) |
None | ✅ |
kubescape/node-agent#806 (network CEL) |
None | ✅ |
kubescape/node-agent#808/809 (tamper/sign) |
None | ✅ |
Summary: PR #811 is atomic and correctly placed at Tier 0. The three substantive items above (silent flag degradation, cost overestimate, and future flag validation) are all containable within this PR without touching any other tier.
✅ Actions performed
Review triggered.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
| // 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). | ||
| for openPath := range cp.Opens.Values { |
There was a problem hiding this comment.
Blocking: projectField() still keeps dynamic open paths only in cp.Opens.Patterns when cp.Opens.All == true (see TestApply_DynamicRetainedInPassThrough). 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.
Removing the Patterns scan when cp.Opens.All==true (PR kubescape#811 original diff) regressed projectField()'s pass-through behavior: ap.was_path_opened_with_suffix/_prefix returned false for dynamic entries kept only in cp.Opens.Patterns when the rule omitted profileDataRequired.opens. Restores a narrower Patterns scan that, in All-mode, considers each pattern's CONCRETE SUFFIX/PREFIX (text up to the first '*' or '⋯') rather than the full pattern body. This keeps the spoof-resistance intent matthyx noted (rejecting raw HasPrefix/HasSuffix over attacker-influenced pattern strings) while restoring matches for the dynamic entries the All-mode contract is supposed to cover. The two segment-iteration sites that classify wildcard tokens now match the bare "*" literal directly rather than depending on dynamicpathdetector.WildcardIdentifier (added in storage kubescape#323, not yet in NA's pinned storage v0.0.258). DynamicIdentifier reference stays — it's been in storage since long before v0.0.258. Resolves matthyx review on open.go:125 (2026-05-19).
…ardAnchoring + fixtures
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/rulemanager/cel/libraries/applicationprofile/open.go (1)
71-74:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEmit a degradation signal when flags are ignored.
At Line 71,
flagsare parsed and then dropped without any metric/log signal. That makes path-only fallback invisible in production, unlike the undeclared-literal paths used in suffix/prefix helpers.Suggested patch
- // flags projection (OpenFlagsByPath) is out of scope for v1; degrade to path-only matching. - if _, err := celparse.ParseList[string](flags); err != nil { + // flags projection (OpenFlagsByPath) is out of scope for v1; degrade to path-only matching. + parsedFlags, err := celparse.ParseList[string](flags) + if err != nil { return types.NewErr("failed to parse flags: %v", err) } + if len(parsedFlags) > 0 && l.metrics != nil { + l.metrics.IncProjectionUndeclaredLiteral("ap.was_path_opened_with_flags") + }🤖 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/rulemanager/cel/libraries/applicationprofile/open.go` around lines 71 - 74, The code parses OpenFlagsByPath via celparse.ParseList[string](flags) but then silently drops flags to degrade to path-only matching; emit a clear degradation signal when this happens by logging or incrementing a metric right after successful parse and before returning, e.g., call your existing logger/metrics helper to record "flags ignored, falling back to path-only" (include the parsed flags value) so operators can detect the fallback from OpenFlagsByPath; update the branch handling celparse.ParseList[string](flags) to log/metricize the degradation before continuing.pkg/rulemanager/cel/libraries/applicationprofile/ap.go (1)
379-381:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
ap.was_path_opened_with_flagscost estimate overstates v1 work.Line 380 documents O(f*p) flag comparison, but v1 currently parses flags and performs path-only matching. Keeping cost at 40 can unnecessarily penalize valid expressions under cost budgets.
Suggested patch
case "ap.was_path_opened_with_flags": - // Cache lookup + O(n) search + dynamic path comparison + O(f*p) flag comparison - cost = 40 + // v1: cache lookup + O(n) path search + dynamic path comparison (+ list parse) + // keep aligned with runtime until OpenFlagsByPath matching is implemented. + cost = 25🤖 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/rulemanager/cel/libraries/applicationprofile/ap.go` around lines 379 - 381, The cost for the case "ap.was_path_opened_with_flags" overstates current v1 work; update the case to reduce the cost (e.g., from 40 to a lower value such as 10) and amend the comment to accurately state that v1 parses flags but only performs path-only matching (no O(f*p) flag comparison). Locate the switch case for "ap.was_path_opened_with_flags" and change the numeric cost and the explanatory comment to reflect the actual operations: cache lookup + O(n) path search + flag parsing (not full flag-pair comparisons).
🧹 Nitpick comments (2)
tests/resources/nginx-user-profile-deployment.yaml (2)
20-20: ⚡ Quick winConsider using a more recent nginx version.
nginx:1.14.2 was released in December 2018. While this may be intentional for specific test scenarios, using a more recent version (e.g., nginx:1.27 or nginx:stable) would avoid potential issues from known vulnerabilities in the 8-year-old image.
🤖 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 `@tests/resources/nginx-user-profile-deployment.yaml` at line 20, Update the nginx image tag in tests/resources/nginx-user-profile-deployment.yaml: replace the outdated "image: nginx:1.14.2" with a newer, supported tag such as "nginx:1.27" or "nginx:stable"; after changing the image line, run the test suite that references this YAML and verify any other manifests or image-pinning (e.g., deployment templates or CI fixtures) are updated to the same newer tag to keep tests consistent.
20-20: ⚡ Quick winConsider digest-pinning for test reproducibility.
The
curldeployment uses digest-pinned images while this uses a tag. For consistent integration test behavior, consider pinning to a specific digest like the curl deployment does.📌 Example: Convert tag to digest
You can get the current digest for nginx:1.14.2:
docker pull nginx:1.14.2 docker inspect nginx:1.14.2 --format='{{index .RepoDigests 0}}'Then update the manifest:
- image: nginx:1.14.2 + image: nginx@sha256:<digest>🤖 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 `@tests/resources/nginx-user-profile-deployment.yaml` at line 20, Replace the tag-based image reference "image: nginx:1.14.2" with a digest-pinned reference (same format used by the curl deployment) to ensure reproducible tests; fetch the current digest with docker pull nginx:1.14.2 and docker inspect nginx:1.14.2 --format='{{index .RepoDigests 0}}' then update the manifest's image value to that returned digest (e.g., nginx@sha256:...), keeping the rest of the deployment manifest unchanged.
🤖 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.
Inline comments:
In `@pkg/rulemanager/cel/libraries/applicationprofile/ap.go`:
- Around line 133-154: The comment for
ap.was_path_opened_with_suffix/ap.was_path_opened_with_prefix is stale: update
the doc to reflect that when the projector is in pass-through mode
cp.Opens.Patterns ARE scanned for concrete suffix/prefix matches via the
patternConcreteSuffix and patternConcretePrefix helpers in open.go (so Patterns
can contribute to suffix/prefix answers), and adjust the guidance about when to
declare an Opens projection slice or use ap.was_path_opened(path) accordingly;
reference ap.was_path_opened_with_suffix, ap.was_path_opened_with_prefix,
cp.Opens.Patterns, patternConcreteSuffix, patternConcretePrefix and
SuffixHits/PrefixHits in the updated comment so rule authors aren’t misled.
In `@pkg/rulemanager/cel/libraries/applicationprofile/open_test.go`:
- Around line 16-47: The top-level test comment contradicts the test body:
update the opening paragraph to state that Patterns ARE scanned when
Opens.All==true but using the narrower fallback (concrete tail/head matches via
HasSuffix/HasPrefix; wildcard-only patterns are permissive), to match the
behavior validated by
TestWasPathOpenedWithSuffix_PatternsScannedWithConcreteTail and the related
tests (TestOpenWithSuffixInProfile / TestOpenWithPrefixInProfile) and the
explanations about Opens.Patterns, Opens.Values, Opens.All and precomputed
Opens.SuffixHits/PrefixHits; remove or rewrite the lines that claim Patterns
must not contribute to suffix/prefix answers so the entire comment asserts the
single consistent contract exercised by the test.
---
Outside diff comments:
In `@pkg/rulemanager/cel/libraries/applicationprofile/ap.go`:
- Around line 379-381: The cost for the case "ap.was_path_opened_with_flags"
overstates current v1 work; update the case to reduce the cost (e.g., from 40 to
a lower value such as 10) and amend the comment to accurately state that v1
parses flags but only performs path-only matching (no O(f*p) flag comparison).
Locate the switch case for "ap.was_path_opened_with_flags" and change the
numeric cost and the explanatory comment to reflect the actual operations: cache
lookup + O(n) path search + flag parsing (not full flag-pair comparisons).
In `@pkg/rulemanager/cel/libraries/applicationprofile/open.go`:
- Around line 71-74: The code parses OpenFlagsByPath via
celparse.ParseList[string](flags) but then silently drops flags to degrade to
path-only matching; emit a clear degradation signal when this happens by logging
or incrementing a metric right after successful parse and before returning,
e.g., call your existing logger/metrics helper to record "flags ignored, falling
back to path-only" (include the parsed flags value) so operators can detect the
fallback from OpenFlagsByPath; update the branch handling
celparse.ParseList[string](flags) to log/metricize the degradation before
continuing.
---
Nitpick comments:
In `@tests/resources/nginx-user-profile-deployment.yaml`:
- Line 20: Update the nginx image tag in
tests/resources/nginx-user-profile-deployment.yaml: replace the outdated "image:
nginx:1.14.2" with a newer, supported tag such as "nginx:1.27" or
"nginx:stable"; after changing the image line, run the test suite that
references this YAML and verify any other manifests or image-pinning (e.g.,
deployment templates or CI fixtures) are updated to the same newer tag to keep
tests consistent.
- Line 20: Replace the tag-based image reference "image: nginx:1.14.2" with a
digest-pinned reference (same format used by the curl deployment) to ensure
reproducible tests; fetch the current digest with docker pull nginx:1.14.2 and
docker inspect nginx:1.14.2 --format='{{index .RepoDigests 0}}' then update the
manifest's image value to that returned digest (e.g., nginx@sha256:...), keeping
the rest of the deployment manifest unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd7d7db5-3c3c-4941-a83a-8ca9d4f74a01
📒 Files selected for processing (6)
pkg/rulemanager/cel/libraries/applicationprofile/ap.gopkg/rulemanager/cel/libraries/applicationprofile/open.gopkg/rulemanager/cel/libraries/applicationprofile/open_bench_test.gopkg/rulemanager/cel/libraries/applicationprofile/open_test.gotests/resources/curl-user-profile-wildcards-deployment.yamltests/resources/nginx-user-profile-deployment.yaml
| // ap.was_path_opened_with_suffix and ap.was_path_opened_with_prefix | ||
| // — rule-author contract (CodeRabbit upstream PR #807 finding #7): | ||
| // | ||
| // These helpers answer "did any RECORDED concrete path open match | ||
| // this suffix/prefix?". When the profile-projection cache is in | ||
| // pass-through mode (no rule declared an Opens-projection slice, | ||
| // so cp.Opens.All == true), wildcard patterns in cp.Opens.Patterns | ||
| // are NOT scanned via string-level HasSuffix/HasPrefix because the | ||
| // pattern text contains '*' / '⋯' tokens whose string shape doesn't | ||
| // safely answer suffix/prefix questions (see open.go comment). | ||
| // Concrete-only Values are scanned. | ||
| // | ||
| // False-negative gap: if a profile entry is `/var/log/pods/*/foo.log`, | ||
| // the runtime path `/var/log/pods/web-7d6f/volumes/foo.log` actually | ||
| // matches this pattern, but `was_path_opened_with_suffix("/foo.log")` | ||
| // returns FALSE because the pattern text doesn't end in `/foo.log` | ||
| // literally. Rule authors who need wildcard-aware coverage should | ||
| // either: (a) declare an Opens projection slice in the rule's | ||
| // ProfileDataRequired (then SuffixHits/PrefixHits become authoritative | ||
| // and the projector pre-computes the hit map for wildcard entries), | ||
| // or (b) use ap.was_path_opened(path) which DOES run dynamic-segment | ||
| // matching over Patterns via CompareDynamic. |
There was a problem hiding this comment.
Helper contract comment is out of sync with runtime behavior.
Lines 139-154 still describe cp.Opens.Patterns as effectively not participating in suffix/prefix answers, but open.go now scans patterns via patternConcreteSuffix/patternConcretePrefix in pass-through mode. This doc mismatch can mislead rule authors.
🤖 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/rulemanager/cel/libraries/applicationprofile/ap.go` around lines 133 -
154, The comment for
ap.was_path_opened_with_suffix/ap.was_path_opened_with_prefix is stale: update
the doc to reflect that when the projector is in pass-through mode
cp.Opens.Patterns ARE scanned for concrete suffix/prefix matches via the
patternConcreteSuffix and patternConcretePrefix helpers in open.go (so Patterns
can contribute to suffix/prefix answers), and adjust the guidance about when to
declare an Opens projection slice or use ap.was_path_opened(path) accordingly;
reference ap.was_path_opened_with_suffix, ap.was_path_opened_with_prefix,
cp.Opens.Patterns, patternConcreteSuffix, patternConcretePrefix and
SuffixHits/PrefixHits in the updated comment so rule authors aren’t misled.
| // 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. | ||
| // | ||
| // 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. | ||
| // | ||
| // 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) { |
There was a problem hiding this comment.
Top-level test comment is internally contradictory.
Lines 16-23 say patterns must not contribute to suffix/prefix answers, while Lines 32-43 and the test body assert the opposite (patterns are scanned with narrower fallback). Please reconcile this block to a single contract.
🤖 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/rulemanager/cel/libraries/applicationprofile/open_test.go` around lines
16 - 47, The top-level test comment contradicts the test body: update the
opening paragraph to state that Patterns ARE scanned when Opens.All==true but
using the narrower fallback (concrete tail/head matches via HasSuffix/HasPrefix;
wildcard-only patterns are permissive), to match the behavior validated by
TestWasPathOpenedWithSuffix_PatternsScannedWithConcreteTail and the related
tests (TestOpenWithSuffixInProfile / TestOpenWithPrefixInProfile) and the
explanations about Opens.Patterns, Opens.Values, Opens.All and precomputed
Opens.SuffixHits/PrefixHits; remove or rewrite the lines that claim Patterns
must not contribute to suffix/prefix answers so the entire comment asserts the
single consistent contract exercised by the test.
…ardAnchoring + fixtures
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
2 participants
Layer 1 (path opens) — adds the four
was_path_opened*CEL helpers to the applicationprofile library so rule authors can express open-event matching against the projected profile.CEL surface added
ap.was_path_opened(containerID, path)ap.was_path_opened_with_flags(containerID, path, flags)flagswere seenap.was_path_opened_with_suffix(containerID, suffix)suffixap.was_path_opened_with_prefix(containerID, prefix)prefixAll four route through
pkg/rulemanager/cel/libraries/applicationprofile/open.go. The flags variant accepts a[]stringso rules can scope to e.g.["O_RDWR", "O_CREAT"]. Cost-estimator entries are added inap.goalongside the existing helper costs.Dependencies
None on the node-agent side. Layer 1 — Tier 0 in the dependency advisory CodeRabbit posted on
kubescape/storage#323. Companion to storage'sanalyze_opens.goanchored-*bugfix.Tests
Unit coverage for each helper in
open_test.go. Suite-level integration tests inintegration_test.goexercising one open-event call per helper, including thewas_path_opened_with_flagstest case that demonstrates flag-list parsing.Summary by CodeRabbit
Bug Fixes
Documentation
Tests