Skip to content

feat(cel): was_path_opened* CEL helpers in applicationprofile library#811

Open
entlein wants to merge 1 commit into
kubescape:mainfrom
k8sstormcenter:upstream-pr/opens-na-cel-helpers
Open

feat(cel): was_path_opened* CEL helpers in applicationprofile library#811
entlein wants to merge 1 commit into
kubescape:mainfrom
k8sstormcenter:upstream-pr/opens-na-cel-helpers

Conversation

@entlein
Copy link
Copy Markdown

@entlein entlein commented May 16, 2026

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

Helper Args Returns Use
ap.was_path_opened(containerID, path) (string, string) bool path was opened during learning
ap.was_path_opened_with_flags(containerID, path, flags) (string, string, []string) bool path was opened AND any of flags were seen
ap.was_path_opened_with_suffix(containerID, suffix) (string, string) bool any opened path ends with suffix
ap.was_path_opened_with_prefix(containerID, prefix) (string, string) bool any opened path starts with prefix

All four route through pkg/rulemanager/cel/libraries/applicationprofile/open.go. The flags variant accepts a []string so rules can scope to e.g. ["O_RDWR", "O_CREAT"]. Cost-estimator entries are added in ap.go alongside 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's analyze_opens.go anchored-* bugfix.

Tests

Unit coverage for each helper in open_test.go. Suite-level integration tests in integration_test.go exercising one open-event call per helper, including the was_path_opened_with_flags test case that demonstrates flag-list parsing.

Signed-off-by: entlein <einentlein@gmail.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 16, 2026

Warning

Rate limit exceeded

@entlein has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 58 minutes and 26 seconds before requesting another review.

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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8d284579-1259-4aa3-a3cd-84a4d8849aa1

📥 Commits

Reviewing files that changed from the base of the PR and between cc59fa0 and d78728d.

📒 Files selected for processing (2)
  • pkg/rulemanager/cel/libraries/applicationprofile/ap.go
  • pkg/rulemanager/cel/libraries/applicationprofile/open.go
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@entlein
Copy link
Copy Markdown
Author

entlein commented May 16, 2026

@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.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 16, 2026

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch PR `#811` details
gh pr view 811 --repo kubescape/node-agent --json title,body,files,additions,deletions,changedFiles

# List changed files
gh pr view 811 --repo kubescape/node-agent --json files --jq '.files[].path'

Length of output: 1979


🏁 Script executed:

#!/bin/bash
# Get the full diff for PR `#811`
gh pr diff 811 --repo kubescape/node-agent

echo "===FILE: ap.go==="
cat pkg/rulemanager/cel/libraries/applicationprofile/ap.go

echo "===FILE: open.go==="
cat pkg/rulemanager/cel/libraries/applicationprofile/open.go

Length of output: 29788


@entlein: ✅ Atomicity confirmed — PR #811 is properly scoped. Two files changed, single responsibility (the four was_path_opened* CEL helpers), zero coupling to Tier 2 (execs), Tier 3 (network), or Tier 4 (tamper/sign) PRs.

One soft compile-time coupling to flag: kubescape/storage#323 must be consistent on cp.Opens.{Values, Patterns, All, SuffixHits, PrefixHits} field shapes before this builds. That's expected for Tier 0 and correctly documented in the PR description.


Triggering a full review now. Findings follow below.


Review findings

1. wasPathOpenedWithFlags silently drops the flag constraint with no observable signal (open.go, Lines 58–89)

The v1 degradation (flags parsed, result discarded, falls through to path-only matching) is well-commented in the Go doc, but no metric is emitted for the degradation path — unlike wasPathOpenedWithSuffix/wasPathOpenedWithPrefix which call l.metrics.IncProjectionUndeclaredLiteral(...). In production, a rule author writing:

ap.was_path_opened_with_flags(containerID, "/etc/passwd", ["O_WRONLY"])

…gets a true on any open of /etc/passwd regardless of flags, with no way to observe that the flag constraint was silently dropped.

Suggestion — emit a degradation metric after the flags parse succeeds:

if _, err := celparse.ParseList[string](flags); err != nil {
    return types.NewErr("failed to parse flags: %v", err)
}
// v1: flag-projection not yet implemented; degrade to path-only matching.
if l.detailedMetrics && l.metrics != nil {
    l.metrics.IncProjectionUndeclaredLiteral("ap.was_path_opened_with_flags")
}

Or introduce a dedicated IncFlagsDegradation counter if you want to distinguish this from the projection-undeclared case.


2. Cost estimate for ap.was_path_opened_with_flags overstates v1 actual cost (ap.go, apCostEstimator)

The declared cost is 40 (comment: O(f*p) flag comparison), but in v1 the function performs the identical operations as ap.was_path_opened (cost 25) — no flag comparison happens. This penalises CEL policies using the flags variant until flag projection lands. Consider either:

  • Setting it to 25 (matching v1 reality) and bumping to 40 when the projection slice is added, or
  • Documenting the intentional forward-looking over-estimate in the comment.

3. wasPathOpened / wasPathOpenedWithFlags have no projection fast-path (open.go, Lines 14–89)

The suffix/prefix helpers check cp.Opens.All and use SuffixHits/PrefixHits when projection is active. The path-exact helpers always do a linear scan of both Values and Patterns, even when cp.Opens.All == false. This is presumably by design (no ExactHits map in the projection), but a brief comment mirroring the suffix/prefix block would make the intent clear for future maintainers:

// Note: unlike suffix/prefix, there is no ExactHits projection slice;
// path-exact matching always scans Values + Patterns regardless of cp.Opens.All.

4. Flag-string content not validated against known O_* constants (open.go, Line 71)

celparse.ParseList[string](flags) validates that the argument is a []string, but flag strings like "", "INVALID", or "o_rdwr" (lowercase) are accepted silently. Since the flags list is currently a no-op, this is a v1 carry-forward risk: when flag-projection lands in a future spec revision, existing rules with malformed flag strings will silently miss matches. Consider at least validating non-empty strings, or deferring with a TODO comment anchored to the storage field introduction ticket.


Cross-PR coupling matrix

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants