Skip to content

allowing wildcards in exec args#805

Open
entlein wants to merge 1 commit into
kubescape:mainfrom
k8sstormcenter:upstream-pr/sbob-execs
Open

allowing wildcards in exec args#805
entlein wants to merge 1 commit into
kubescape:mainfrom
k8sstormcenter:upstream-pr/sbob-execs

Conversation

@entlein
Copy link
Copy Markdown

@entlein entlein commented May 15, 2026

Overview

Adds a 3-arg parse.get_exec_path(args, comm, exepath) CEL overload
and recording-side resolveExecPath so rules that need to distinguish
a symlinked exec from its underlying binary (busybox utilities,
buildroot-style image where every /bin/* is a symlink to /bin/busybox)
get the absolute argv[0] path when available, falling back to exepath
then the bare comm.

Additional Information

has a sister PR in storage

Checklist before requesting a review

put an [x] in the box to get it checked

  • My code follows the style guidelines of this project
  • I have commented on my code, particularly in hard-to-understand areas
  • I have performed a self-review of my code
  • If it is a core feature, I have added thorough tests.
  • New and existing unit tests pass locally with my changes

Benchmark: all incoming PRs have been benchmarked

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

coderabbitai Bot commented May 15, 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: 93ba05d7-42e8-48d0-8900-8ebbbef927ef

📥 Commits

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

📒 Files selected for processing (5)
  • pkg/containerprofilemanager/v1/event_reporting.go
  • pkg/containerprofilemanager/v1/event_reporting_test.go
  • pkg/rulemanager/cel/libraries/parse/parse.go
  • pkg/rulemanager/cel/libraries/parse/parselib.go
  • pkg/rulemanager/cel/libraries/parse/parsing_test.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

@entlein, I'll kick off a fresh full review of PR #805 right away. Let me also pull the PR details to check atomicity and cross-PR coupling.


🏁 Script executed:

#!/bin/bash
# Get PR `#805` details and files changed
gh pr view 805 --repo kubescape/node-agent --json title,body,files,baseRefName,headRefName

echo "---FILES CHANGED---"
gh pr diff 805 --repo kubescape/node-agent --name-only

echo "---PR `#807` files (R0040 args consumer)---"
gh pr diff 807 --repo kubescape/node-agent --name-only 2>/dev/null || echo "Could not fetch `#807`"

echo "---PR `#812` files (ExecsByPath composite-key surface)---"
gh pr diff 812 --repo kubescape/node-agent --name-only 2>/dev/null || echo "Could not fetch `#812`"

echo "---PR `#811` files (was_path_opened CEL helpers)---"
gh pr diff 811 --repo kubescape/node-agent --name-only 2>/dev/null || echo "Could not fetch `#811`"

Length of output: 3046


🏁 Script executed:

#!/bin/bash
echo "=== PR `#805` DIFF ==="
gh pr diff 805 --repo kubescape/node-agent

echo ""
echo "=== PR `#811` open.go section (for overlap check with `#807`) ==="
gh pr diff 811 --repo kubescape/node-agent -- pkg/rulemanager/cel/libraries/applicationprofile/open.go 2>/dev/null || echo "Could not fetch"

echo ""
echo "=== PR `#807` open.go section ==="
gh pr diff 807 --repo kubescape/node-agent -- pkg/rulemanager/cel/libraries/applicationprofile/open.go 2>/dev/null || echo "Could not fetch"

Length of output: 12669


@entlein, the full review has been triggered. Here is my atomicity confirmation and coupling analysis for #805 (resolveExecPath + 3-arg parse):


✅ Atomicity: Confirmed Clean

PR #805 touches exactly two packages:

  • pkg/rulemanager/cel/libraries/parse/ — CEL 3-arg overload
  • pkg/containerprofilemanager/v1/ — recording-side resolveExecPath

No file overlap with any sibling PR in the tier map:

PR Files Overlap with #805
#807 applicationprofile/exec.go, open.go ✅ None
#811 applicationprofile/ap.go, open.go ✅ None
#812 objectcache/containerprofilecache/, projection_types.go ✅ None

🔗 Cross-PR Coupling Flags

Expected (clean tier dependency):

  • #805#807: #807 (R0040 args consumer) consumes the 3-arg parse.get_exec_path overload introduced here. This is correct Tier 2 → Tier 2 sequential dependency. #807 cannot be merged before #805.

Rebase risk (not #805's fault):

  • #811#807: Both PRs modify pkg/rulemanager/cel/libraries/applicationprofile/open.go. Even with correct tier ordering (Tier 0 before Tier 2), #807 will need a rebase on that file after #811 merges.

Cross-repo Tier 2 sibling:

  • #805 ↔ storage#322: The PR body names storage#322 as the sister. resolveExecPath on the recording side produces the key that CompareExecArgs (storage#322) operates on. These need to land together or storage#322 must tolerate the pre-allowing wildcards in exec args #805 key format.

🐛 Code-Level Issues Found

1. Stale docstring in parsing_test.go (misleading)

TestGetExecPath_SymmetryWithRecordingSide's block comment describes the old recording-side precedence:

// That recording function uses
//   1. exepath (kernel-authoritative)
//   2. argv[0] when non-empty
//   3. comm

But after this PR, resolveExecPath uses: absolute argv[0] → exepath → bare argv[0] → comm. The docstring should be updated to match the new 4-tier precedence to avoid confusing future readers about which direction the "symmetry" runs.

2. 2-arg overload back-compat gap (silent footgun)

The existing parse.get_exec_path(args, comm) 2-arg overload still uses:

if argsList[0] != "" {
    return types.String(argsList[0])
}

For the busybox case, this coincidentally returns the right answer (/bin/sh is non-empty). But for spoofing protection (argv[0]="sshd" with no exepath available), it returns the wrong value with no recourse. The SHOULD use 3-arg comment documents the intent, but any existing rules silently get degraded behavior on busybox images without a compile error or warning. Worth considering whether to emit a CEL deprecation notice or add a test that explicitly documents the 2-arg limitation.

3. Minor: CEL arity guard is redundant in parselib.go

if len(values) != 3 {
    return types.NewErr("expected 3 arguments, got %d", len(values))
}

CEL's overload dispatch already enforces the 3-arg signature declared in []*cel.Type{...}, so this guard is unreachable in normal operation. Harmless, but it adds noise.


The PR is structurally sound for its tier scope. The stale docstring (issue 1) is the most actionable item before merge since it will confuse readers tracing the symmetry invariant.

✅ Actions performed

Full review triggered.

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