rebase storage, CRDs, exec arg wildcard#27
Merged
Conversation
…in exec arg vectors
User-defined ApplicationProfile entries can now express argument-vector
patterns containing two wildcard tokens, exposed via the existing
constants:
DynamicIdentifier ("⋯") — matches exactly one argument position.
WildcardIdentifier ("*") — matches zero or more consecutive args.
Anything else is a literal-equality match. The match is anchored at
both ends — every runtime arg must be consumed by the profile vector,
either by a literal, a single-position ⋯, or a *-absorbing run.
Implementation is recursive backtracking. Real exec arg vectors are
short (typically ≤ a dozen entries) with at most a handful of
wildcards, so the worst case stays well below a regex compile and we
avoid the ReDoS surface that comes with regex.
40 unit subtests cover empty/literal/dynamic/wildcard/mixed/realistic
patterns; matcher is consumer-ready for node-agent's CEL exec rule.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds ChangesArgument Vector Matching
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/registry/file/dynamicpathdetector/compare_exec_args.go`:
- Around line 26-33: The wildcard branch in CompareExecArgs can cause
exponential backtracking; add memoization keyed by (profileIndex, runtimeIndex)
to avoid re-evaluating the same subproblem. Implement a small helper (e.g.,
compareExecArgsMemo or make CompareExecArgs accept indices) that takes
profileArgs, runtimeArgs, current indices (i, j) and a memo map[uint64]bool or
map[string]bool, check the memo at the start and return cached result if
present, compute the result (including the WildcardIdentifier loop), store the
result in memo before returning, and have CompareExecArgs call this helper—use
the pair (i,j) as the unique key to identify subcalls.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c0f9768b-8f84-4d37-9995-6a7cdcc0be60
📒 Files selected for processing (2)
pkg/registry/file/dynamicpathdetector/compare_exec_args.gopkg/registry/file/dynamicpathdetector/tests/compare_exec_args_test.go
|
Summary:
|
CodeRabbit (node-agent PR kubescape#38). Path-only Execs entries in user-defined ApplicationProfiles (the common case) carry a nil/empty Args slice. The strict empty-vs-empty semantics caused R0040 to fire on every legit exec of those paths because was_executed_with_args returned false even when the profile had explicitly allowed the path. Split CompareExecArgs into a thin outer wrapper (empty profile → match) and a strict inner matcher (matchExecArgs, retains the original empty- empty anchor). This preserves the recursive backtracking's anchoring contract — profile fully consumed in the middle of recursion still fails when runtime has args left — while letting the public API treat empty input as 'no constraint'. Test pin: empty profile + multi-arg runtime returns true; both-empty still true (degenerate); non-empty profile + empty runtime stays false. Existing 30+ subtests for literal/dynamic/wildcard/mixed all pass.
|
Summary:
|
Test Results 9 files 9 suites 1h 6m 43s ⏱️ For more details on these failures, see this check. Results for commit b0d68d3. ♻️ This comment has been updated with latest results. |
CodeRabbit Major finding on storage PR #27 (compare_exec_args.go:56). Naïve recursive backtracking on '[*, *, *, …, literal]' patterns re-explores every prefix split before the trailing-literal mismatch surfaces, degrading to exponential time on adversarial inputs. This is the canonical ReDoS-style hazard for wildcard matchers. Switch to index-based recursion with memoisation on (profileIndex, runtimeIndex) state pairs. Both indices only shrink during recursion so the reachable state space is bounded by (len(profile)+1) * (len(runtime)+1), giving O(P*R) total work. Behaviour unchanged: all 30+ existing subtests across literal / DynamicIdentifier / WildcardIdentifier / mixed / realistic groups still pass. Outer empty-profile-as-wildcard short-circuit retained. Add TestCompareExecArgs_ReDoSResistance: 20 leading wildcards + a non-matching trailing literal vs a 50-element runtime. The naïve matcher would take seconds (~2^20 path splits); the memoised matcher completes in microseconds. Test asserts <100ms — generous budget that catches any regression to the unmemoised form.
|
Summary:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…in exec arg vectors
User-defined ApplicationProfile entries can now express argument-vector patterns containing two wildcard tokens, exposed via the existing constants:
DynamicIdentifier ("⋯") — matches exactly one argument position.
WildcardIdentifier ("*") — matches zero or more consecutive args.
Anything else is a literal-equality match. The match is anchored at both ends — every runtime arg must be consumed by the profile vector, either by a literal, a single-position ⋯, or a *-absorbing run.
Implementation is recursive backtracking. Real exec arg vectors are short (typically ≤ a dozen entries) with at most a handful of wildcards, so the worst case stays well below a regex compile and we avoid the ReDoS surface that comes with regex.
40 unit subtests cover empty/literal/dynamic/wildcard/mixed/realistic patterns; matcher is consumer-ready for node-agent's CEL exec rule.
Sorry, we do not accept changes directly against this repository. Please see
CONTRIBUTING.md for information on where and how to contribute instead.