profile-compaction: CollapseConfig CRD + projection overlay + user-ma…#808
Open
entlein wants to merge 1 commit into
Open
profile-compaction: CollapseConfig CRD + projection overlay + user-ma…#808entlein wants to merge 1 commit into
entlein wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Cosign-based signing and verification for profile and rules objects, exposes a ChangesProfile Signing and Tamper Detection
Sequence Diagram(s)ContainerProfileCache tamper flow: sequenceDiagram
participant ContainerProfileCacheImpl
participant verifyUserApplicationProfile
participant emitTamperAlert
participant tamperAlertExporter
participant projectUserProfiles
participant stampOverlayIdentity
ContainerProfileCacheImpl->>verifyUserApplicationProfile: verify user ApplicationProfile overlay
verifyUserApplicationProfile->>emitTamperAlert: emit R1016 on signature mismatch
emitTamperAlert->>tamperAlertExporter: SendRuleAlert
ContainerProfileCacheImpl->>projectUserProfiles: merge overlays into ContainerProfile
projectUserProfiles->>stampOverlayIdentity: append ap=/nn= to kubescape.io/sync-checksum
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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[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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/objectcache/containerprofilecache/containerprofilecache.go`:
- Around line 412-414: The verification calls to
c.verifyUserApplicationProfile(userAP, sharedData.Wlid) currently ignore its
boolean result; update both call sites (the one using userAP at lines ~412 and
the similar one around ~428-430) to check the returned bool and, when it is
false and the cache is running in strict verification mode (e.g., the instance
flag controlling strict verification such as c.strictVerify or equivalent),
avoid projecting/merging the overlay (skip the merge/projectOverlay path), log
the verification failure with context (Wlid and which overlay), and return or
surface an error instead of continuing; if not in strict mode, continue but log
a warning. Ensure you reference and use c.verifyUserApplicationProfile(...) and
the strict-mode flag when implementing the conditional.
🪄 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: acff329b-9e89-4538-a272-7877e0ad1e70
📒 Files selected for processing (12)
pkg/objectcache/containerprofilecache/containerprofilecache.gopkg/objectcache/containerprofilecache/projection.gopkg/objectcache/containerprofilecache/projection_apply.gopkg/objectcache/containerprofilecache/tamper_alert.gopkg/objectcache/containerprofilecache/tamper_alert_test.gopkg/objectcache/containerprofilecache/test32_projection_test.gopkg/objectcache/projection_types.gopkg/objectcache/shared_container_data.gopkg/objectcache/v1/mock.gopkg/rulebindingmanager/cache/cache.gopkg/rulebindingmanager/cache/cache_test.gopkg/rulemanager/cel/libraries/cache/function_cache.go
entlein
pushed a commit
to k8sstormcenter/node-agent
that referenced
this pull request
May 16, 2026
…verlay CodeRabbit upstream PR kubescape#808 / containerprofilecache.go:414 (Major). The verifyUserApplicationProfile and verifyUserNetworkNeighborhood methods already return a boolean reflecting verification outcome — true when the overlay is unsigned OR when verification succeeded OR in permissive mode (EnableSignatureVerification=false); false only in strict mode on actual tamper. The two call sites in projection-load were discarding that return, so tampered overlays in strict mode silently merged anyway. The R1016 alert was emitted but the protection was advisory only. Now: when verify returns false (strict mode + tamper detected) the overlay is nilled out before the merge step so the cache never projects a known-tampered profile. Permissive mode is unchanged — verify always returns true, the overlay still merges, R1016 still fires. New tests: - TestVerifyAP_StrictMode_ReturnsFalseOnTamper — sign + tamper an ApplicationProfile, construct a cache with EnableSignatureVerification=true, and assert verifyUserApplicationProfile returns false (caller drops overlay). - TestVerifyNN_StrictMode_ReturnsFalseOnTamper — symmetric pin for the NetworkNeighborhood path. The existing legacy-permissive tamper test (TestVerifyAP_TamperedProfile_PopulatesDedupMap) continues to pass unchanged — that path still returns true with the R1016 emitted.
4 tasks
This was referenced May 16, 2026
Contributor
Author
|
@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. |
This was referenced May 16, 2026
|
PR
|
| File | Issue | Belongs to |
|---|---|---|
projection_types.go |
Defines ExecsByPath map[string][]string on ProjectedContainerProfile |
Tier 2 · #812 ("ExecsByPath composite-key surface") |
projection_apply.go |
Consumes ExecsByPath, containsDynamicSegment, isNetworkIPWildcard, isNetworkDNSWildcard |
Tier 2 (#812) for exec surface; Tier 3 (#806/#324) for network wildcards |
v1/mock.go |
Updated to populate ExecsByPath + wildcard network patterns |
Tier 2 + Tier 3 |
rulebindingmanager/cache/cache.go + test |
Non-blocking fanout refactor — purely infrastructure, no tamper/sign semantics | Pre-Tier 4 (Tier 0 or standalone) |
Specific recommendations
-
ExecsByPathtype definition should either live in#812(Tier 2) with#808taking a compile dependency on it, or you should explicitly note that#808is the defining PR and#812merely consumes it. Right now the tier map implies the reverse, which is confusing. -
isNetworkIPWildcard/isNetworkDNSWildcardinprojection_apply.goduplicate classifier logic that Tier 3 (#806,#324) is supposed to own. Either import from the Tier 3 package or note these are intentionally local shims with a// TODO: consolidate with pkg/objectcache/networkmatchcomment. -
Non-blocking fanout (
rulebindingmanager/cache) is fully orthogonal to tamper/sign. If this was included because it was required to prevent deadlocks when emitting tamper alerts under load, that's a valid reason — but it should be called out explicitly in the PR description as a required infrastructure prerequisite, not just bundled silently.
Bottom line: The tamper/sign core is clean. The main atomicity leaks are the ExecsByPath surface definition crossing from Tier 4 into what the map says is Tier 2, and the fanout refactor having no declared relationship to the Tier 4 scope. If you can clarify the ExecsByPath ownership direction and add a sentence to the PR description explaining why the fanout refactor is here, the remaining coupling is defensible.
✅ 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.
entlein
added a commit
to k8sstormcenter/node-agent
that referenced
this pull request
May 16, 2026
…e#807, kubescape#808) (#44) * fix(containerprofilecache): honor verify result before merging user overlay CodeRabbit upstream PR kubescape#808 / containerprofilecache.go:414 (Major). The verifyUserApplicationProfile and verifyUserNetworkNeighborhood methods already return a boolean reflecting verification outcome — true when the overlay is unsigned OR when verification succeeded OR in permissive mode (EnableSignatureVerification=false); false only in strict mode on actual tamper. The two call sites in projection-load were discarding that return, so tampered overlays in strict mode silently merged anyway. The R1016 alert was emitted but the protection was advisory only. Now: when verify returns false (strict mode + tamper detected) the overlay is nilled out before the merge step so the cache never projects a known-tampered profile. Permissive mode is unchanged — verify always returns true, the overlay still merges, R1016 still fires. New tests: - TestVerifyAP_StrictMode_ReturnsFalseOnTamper — sign + tamper an ApplicationProfile, construct a cache with EnableSignatureVerification=true, and assert verifyUserApplicationProfile returns false (caller drops overlay). - TestVerifyNN_StrictMode_ReturnsFalseOnTamper — symmetric pin for the NetworkNeighborhood path. The existing legacy-permissive tamper test (TestVerifyAP_TamperedProfile_PopulatesDedupMap) continues to pass unchanged — that path still returns true with the R1016 emitted. * docs(applicationprofile): rule-author contracts on opens-with-suffix/prefix + exec-args Two doc-only fixes for CodeRabbit cross-PR advisory: #7 (NA kubescape#807): ap.was_path_opened_with_suffix / _prefix — explicitly document the false-negative gap when the projection is in pass-through mode (cp.Opens.All=true). Wildcard Patterns are skipped from the suffix/prefix scan because their token-bearing text doesn't safely answer suffix questions. Rule authors who need wildcard-aware coverage should either declare an Opens-projection slice (so SuffixHits/PrefixHits become authoritative for the literals they care about) or use ap.was_path_opened (which runs CompareDynamic over Patterns). #8 (NA kubescape#807): wasExecutedWithArgs — document the three states of ExecsByPath: 1. Path absent from Execs.Values → exec not allowed, fall through. 2. Path in Values, ABSENT from ExecsByPath → legacy back-compat "no argv constraint", match. 3. Path in Values, PRESENT with empty arg list [] → explicit "ran with no args" constraint, NOT a wildcard. The distinction is load-bearing for profile authors: an entry of {Path: ..., Args: []} is a constraint, not a free pass. No behavioural change. Tests pass unchanged. * deps: pin stereoscope v0.1.9 + runtime-spec v1.2.1 (compat with kubescape/syft fork) Storage rc1 bumped to syft v1.42.4 (CVE-2026-33481), which transitively requires stereoscope v0.1.22 + runtime-spec v1.3.0. Those versions use the new moby/moby/client submodule API, which is incompatible with inspektor-gadget's moby/moby umbrella requirement on the node-agent side (ambiguous-import wall — see issue #45). Node-agent stays on kubescape/syft v1.32.0-ks.2 via the existing replace, but transitive resolution from storage's go.mod pulls the newer stereoscope into the build, breaking the build with: undefined: client.New undefined: client.PingOptions Adds two replace directives to force the older transitive chain that matches kubescape/syft v1.32.0-ks.2's expectations: github.com/anchore/stereoscope => v0.1.9-0.20250826202322-... github.com/opencontainers/runtime-spec => v1.2.1 This is the minimum set needed for node-agent to build cleanly against storage rc1 (with syft v1.42.4) while still using kubescape/syft on its own side. Storage's CVE fix remains in effect at the storage binary; node-agent's syft surface is unchanged. Verified locally: go build ./... ok go test ./pkg/objectcache/... ./pkg/rulemanager/... -count=1 → 30+ packages ok --------- Co-authored-by: Entlein <eineintlein@gmail.com>
matthyx
requested changes
May 27, 2026
matthyx
left a comment
matthyx
left a comment
Contributor
There was a problem hiding this comment.
I found three merge-blocking build issues and left inline notes on each failing spot.
| "github.com/kubescape/node-agent/pkg/exporters" | ||
| "github.com/kubescape/node-agent/pkg/rulemanager/types" | ||
| "github.com/kubescape/node-agent/pkg/signature" | ||
| "github.com/kubescape/node-agent/pkg/signature/profiles" |
Contributor
There was a problem hiding this comment.
go test ./... fails on this new file because github.com/kubescape/node-agent/pkg/signature and .../pkg/signature/profiles do not exist in the checked-in module (pkg/**/signature/** is empty here). As written, this breaks package setup for every importer of containerprofilecache, so the PR will not build until these imports point at a real package or the code is wired through an existing dependency.
| } | ||
| // v0.0.2 IPAddresses[] — list form supporting CIDRs and '*' sentinel. | ||
| // Same semantics as the deprecated singular IPAddress, just plural. | ||
| addrs = append(addrs, n.IPAddresses...) |
Contributor
There was a problem hiding this comment.
The current github.com/kubescape/storage/pkg/apis/softwarecomposition/v1beta1.NetworkNeighbor type only exposes IPAddress (singular) — go doc ...NetworkNeighbor has no IPAddresses field. That makes this append, and the matching ingress append below, a compile error against the current dependency, so this needs to stay on the singular field unless the storage API is updated first.
| pcp.EgressAddresses.All = true | ||
| pcp.EgressAddresses.Values = make(map[string]struct{}) | ||
| if !specInstalled || spec.EgressAddresses.InUse { | ||
| addrs := make([]string, 0, len(n.IPAddresses)+1) |
Contributor
There was a problem hiding this comment.
Same blocker in the mock path: NetworkNeighbor does not have an IPAddresses field, so len(n.IPAddresses) and the append here fail to compile (same issue repeats in the analogous ingress block below). go test ./pkg/objectcache/v1 is currently red because of this.
entlein
pushed a commit
to k8sstormcenter/node-agent
that referenced
this pull request
May 27, 2026
Three matthyx blockers (2026-05-27): (1) tamper_alert.go:28 — imports pkg/signature and pkg/signature/profiles which ship in node-agent#809 (not yet merged). Adds minimum-surface stubs (pkg/signature/stub.go, pkg/signature/profiles/stub.go) so the PR compiles standalone. With IsSigned returning false, the tamper path short-circuits and never invokes Verify — the no-op stub is behavior- safe: signed-profile detection is dormant until kubescape#809 lands and replaces these files with the real implementation. (2) projection_apply.go:265 — NetworkNeighbor.IPAddresses missing. (3) mock.go:202 — same IPAddresses issue. Both (2) and (3) resolved by replacing kubescape/storage with k8sstormcenter/storage's upstream-pr/sbob-network tip 46f37d32 (sibling of kubescape/storage#324). Removed when kubescape#324 merges and a release ships IPAddresses.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/rulebindingmanager/cache/cache.go (2)
188-189:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't multiplex refresh pulses onto the pod-delta notifier.
rulebindingmanager.RuleBindingNotify{}is a real zero-value event here (Action == Addedwith an empty Pod).pkg/containerwatcher/v2/container_watcher_collection.go:144-173treats every notification as a concrete pod update, so this pulse will mark rule bindings initialized and add"/"toruleManagedPods. Ifpkg/rulemanager/rule_manager.go:112-133needs a coalesced refresh signal, give it a separate notifier path or extend the payload with an explicit refresh action.🤖 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/rulebindingmanager/cache/cache.go` around lines 188 - 189, The refresh pulse is being sent as an empty rulebindingmanager.RuleBindingNotify via dispatchNonBlocking(notifiers, ...), which the container watcher treats as a real Pod Added event and corrupts ruleManagedPods; stop multiplexing refreshes onto the pod-delta notifier by adding a dedicated refresh path or explicit action field: either add a new notifier channel specifically for coalesced refreshes and call dispatchNonBlocking(refreshNotifiers, nil, "refresh pulse"), or extend rulebindingmanager.RuleBindingNotify with an explicit Action enum/value (e.g., Refresh) and emit RuleBindingNotify{Action: Refresh} from the caller and update the consumers in container_watcher_collection and rule_manager.go to ignore Refresh as a pod update and handle it as a coalesced refresh signal.
201-219:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDrop-on-full is unsafe for the multi-pod add/modify/delete paths.
This helper is fine for idempotent pulses, but the handlers pass slices of per-pod notifications. Once a subscriber channel fills, later pod events are silently lost, and
pkg/containerwatcher/v2/container_watcher_collection.go:144-173does not resync from cache — it updatesruleManagedPodsfrom each message. A single large rule-binding change can therefore leave a slow subscriber permanently missing pods. Consider coalescing those handlers to one resync event or moving delivery behind a per-subscriber queue/worker that preserves at-least-once delivery.🤖 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/rulebindingmanager/cache/cache.go` around lines 201 - 219, The current dispatchNonBlocking drops per-pod notifications which breaks callers that expect at-least-once delivery (the add/modify/delete handlers that send slices of per-pod RuleBindingNotify and consumers that update ruleManagedPods). Fix by changing delivery semantics: either (A) coalesce the handlers so they emit one consolidated resync-style RuleBindingNotify instead of many per-pod messages (update the add/modify/delete handlers to produce a single resync message), or (B) implement a per-subscriber worker/queue: create a goroutine per notifier that owns a buffered channel and serializes delivery (modify dispatchNonBlocking to enqueue into each notifier's dedicated queue/channel and have the worker perform the actual send with retries/blocking to guarantee at-least-once), and update indexOfNotifier usage accordingly; pick one approach and adjust all callers of dispatchNonBlocking and the add/modify/delete handlers to match.
🤖 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/rulebindingmanager/cache/cache.go`:
- Line 140: The AddHandler and ModifyHandler implementations hold c.mutex and
call addRuleBinding/modifiedRuleBinding while those helpers use
context.Background() for Namespaces().List and Pods(...).List which prevents
cancellation; change the signatures of addRuleBinding(ctx context.Context, ...)
and modifiedRuleBinding(ctx context.Context, ...) and pass the incoming watch
context from AddHandler/ModifyHandler into those calls, then replace
context.Background() uses in both helpers with the passed ctx for both
Namespaces().List and Pods(...).List; finally update cache_test.go to call the
new function signatures accordingly.
---
Outside diff comments:
In `@pkg/rulebindingmanager/cache/cache.go`:
- Around line 188-189: The refresh pulse is being sent as an empty
rulebindingmanager.RuleBindingNotify via dispatchNonBlocking(notifiers, ...),
which the container watcher treats as a real Pod Added event and corrupts
ruleManagedPods; stop multiplexing refreshes onto the pod-delta notifier by
adding a dedicated refresh path or explicit action field: either add a new
notifier channel specifically for coalesced refreshes and call
dispatchNonBlocking(refreshNotifiers, nil, "refresh pulse"), or extend
rulebindingmanager.RuleBindingNotify with an explicit Action enum/value (e.g.,
Refresh) and emit RuleBindingNotify{Action: Refresh} from the caller and update
the consumers in container_watcher_collection and rule_manager.go to ignore
Refresh as a pod update and handle it as a coalesced refresh signal.
- Around line 201-219: The current dispatchNonBlocking drops per-pod
notifications which breaks callers that expect at-least-once delivery (the
add/modify/delete handlers that send slices of per-pod RuleBindingNotify and
consumers that update ruleManagedPods). Fix by changing delivery semantics:
either (A) coalesce the handlers so they emit one consolidated resync-style
RuleBindingNotify instead of many per-pod messages (update the add/modify/delete
handlers to produce a single resync message), or (B) implement a per-subscriber
worker/queue: create a goroutine per notifier that owns a buffered channel and
serializes delivery (modify dispatchNonBlocking to enqueue into each notifier's
dedicated queue/channel and have the worker perform the actual send with
retries/blocking to guarantee at-least-once), and update indexOfNotifier usage
accordingly; pick one approach and adjust all callers of dispatchNonBlocking and
the add/modify/delete handlers to match.
🪄 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: 1b640f7e-c5dd-4b9f-a2b1-436aa9cc7564
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
go.modpkg/objectcache/containerprofilecache/containerprofilecache.gopkg/rulebindingmanager/cache/cache.gopkg/rulebindingmanager/cache/cache_test.gopkg/signature/profiles/stub.gopkg/signature/stub.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/objectcache/containerprofilecache/containerprofilecache.go
| c.mutex.Unlock() | ||
| return | ||
| } | ||
| rbs = c.addRuleBinding(ruleBinding) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show repo file
ls -la
# Print the relevant portions of the file around the referenced lines
FILE="pkg/rulebindingmanager/cache/cache.go"
echo "=== File stats ==="
wc -l "$FILE"
echo
echo "=== Snippet around line 120-180 ==="
nl -ba "$FILE" | sed -n '110,190p'
echo
echo "=== Snippet around line 220-310 ==="
nl -ba "$FILE" | sed -n '220,310p'
echo
echo "=== Snippet around line 320-380 ==="
nl -ba "$FILE" | sed -n '320,380p'
echo
echo "=== Find all context.Background() uses in this file ==="
rg -n "context\.Background\(\)" "$FILE" || true
echo
echo "=== Find definitions/usages of addRuleBinding / modifiedRuleBinding ==="
rg -n "func \(c \*RBCache\) (addRuleBinding|modifiedRuleBinding)\b" "$FILE" || true
rg -n "\.addRuleBinding\(" "$FILE" || true
rg -n "\.modifiedRuleBinding\(" "$FILE" || true
echo
echo "=== Show where List() calls happen in addRuleBinding/modifiedRuleBinding ==="
# Print whole functions if not too large
perl -ne 'print if $.>=0' "$FILE" >/dev/null
# Use ast-grep? Fallback to sed by locating function start lines
ADD_START=$(nl -ba "$FILE" | rg -n "func \(c \*RBCache\) addRuleBinding" | head -n1 | cut -d: -f1 || true)
MOD_START=$(nl -ba "$FILE" | rg -n "func \(c \*RBCache\) modifiedRuleBinding" | head -n1 | cut -d: -f1 || true)
if [ -n "${ADD_START:-}" ]; then
echo "=== addRuleBinding from $ADD_START (print until next func) ==="
nl -ba "$FILE" | sed -n "${ADD_START},$((ADD_START+160))p"
fi
if [ -n "${MOD_START:-}" ]; then
echo "=== modifiedRuleBinding from $MOD_START (print until next func) ==="
nl -ba "$FILE" | sed -n "${MOD_START},$((MOD_START+160))p"
fiRepository: kubescape/node-agent
Length of output: 2017
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="pkg/rulebindingmanager/cache/cache.go"
echo "=== File stats ==="
wc -l "$FILE"
echo
echo "=== Snippet around line 110-190 ==="
awk 'NR>=110 && NR<=190 {printf "%d\t%s\n", NR, $0}' "$FILE"
echo
echo "=== Snippet around line 220-310 ==="
awk 'NR>=220 && NR<=310 {printf "%d\t%s\n", NR, $0}' "$FILE"
echo
echo "=== Snippet around line 320-380 ==="
awk 'NR>=320 && NR<=380 {printf "%d\t%s\n", NR, $0}' "$FILE"
echo
echo "=== Find all context.Background() uses in this file ==="
rg -n "context\.Background\(\)" "$FILE" || true
echo
echo "=== Find definitions/usages of addRuleBinding / modifiedRuleBinding ==="
rg -n "func \(c \*RBCache\) (addRuleBinding|modifiedRuleBinding)\b" "$FILE" || true
rg -n "\.addRuleBinding\(" "$FILE" || true
rg -n "\.modifiedRuleBinding\(" "$FILE" || true
echo
echo "=== Function bodies (trimmed) ==="
ADD_START=$(rg -n "func \(c \*RBCache\) addRuleBinding\b" "$FILE" | head -n1 | cut -d: -f1 || true)
MOD_START=$(rg -n "func \(c \*RBCache\) modifiedRuleBinding\b" "$FILE" | head -n1 | cut -d: -f1 || true)
if [ -n "${ADD_START:-}" ]; then
echo "--- addRuleBinding starting at $ADD_START ---"
awk -v s="$ADD_START" 'NR>=s && NR<=s+220 {printf "%d\t%s\n", NR, $0}' "$FILE"
fi
if [ -n "${MOD_START:-}" ]; then
echo "--- modifiedRuleBinding starting at $MOD_START ---"
awk -v s="$MOD_START" 'NR>=s && NR<=s+220 {printf "%d\t%s\n", NR, $0}' "$FILE"
fiRepository: kubescape/node-agent
Length of output: 24559
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "\.addRuleBinding\(" -S . || true
rg -n "\.modifiedRuleBinding\(" -S . || true
rg -n "func \(c \*RBCache\) addRuleBinding\(" pkg/rulebindingmanager/cache/cache.go
rg -n "func \(c \*RBCache\) modifiedRuleBinding\(" pkg/rulebindingmanager/cache/cache.goRepository: kubescape/node-agent
Length of output: 739
Restore watch-context cancellation for RB list calls.
pkg/rulebindingmanager/cache/cache.go keeps c.mutex held in AddHandler/ModifyHandler while it calls addRuleBinding/modifiedRuleBinding, and addRuleBinding currently uses context.Background() for Namespaces().List and Pods(...).List (lines 140, 159, 237-281, 343-345). This prevents list requests from being canceled when the watch context is canceled and can stall cache operations during/after shutdown or restart. Thread ctx through addRuleBinding(ctx, ...) / modifiedRuleBinding(ctx, ...), pass it from AddHandler/ModifyHandler, and use it for both List calls; update cache_test.go call sites for the signature change.
🤖 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/rulebindingmanager/cache/cache.go` at line 140, The AddHandler and
ModifyHandler implementations hold c.mutex and call
addRuleBinding/modifiedRuleBinding while those helpers use context.Background()
for Namespaces().List and Pods(...).List which prevents cancellation; change the
signatures of addRuleBinding(ctx context.Context, ...) and
modifiedRuleBinding(ctx context.Context, ...) and pass the incoming watch
context from AddHandler/ModifyHandler into those calls, then replace
context.Background() uses in both helpers with the passed ctx for both
Namespaces().List and Pods(...).List; finally update cache_test.go to call the
new function signatures accordingly.
matthyx
reviewed
May 28, 2026
| helpers.Error(err)) | ||
| // Honour strict-mode: refuse to load on any verification failure, | ||
| // but do NOT touch the dedup map or emit R1016. | ||
| return !c.cfg.EnableSignatureVerification |
Contributor
There was a problem hiding this comment.
c.cfg.EnableSignatureVerification still does not exist on config.Config, so this file has four hard compiler errors at the strict-mode return sites (for example here, and again at lines 108/136/146). Please either add/plumb that config field in this PR or remove the strict-mode branches so the tamper path matches currently available config.
matthyx
reviewed
May 28, 2026
| // signature-payload accessors. | ||
| type Signable interface{} | ||
|
|
||
| func IsSigned(_ Signable) bool { return false } |
Contributor
There was a problem hiding this comment.
These stubs still leave the new tamper-detection path effectively disabled: IsSigned always returns false, VerifyObjectAllowUntrusted always returns nil, and the adapters in pkg/signature/profiles/stub.go return nil. That means the runtime path never verifies anything and the new tamper tests are not exercising the intended behavior. This needs the real signature implementation (or the feature/tests need to stay out of this PR).
Contributor
Author
There was a problem hiding this comment.
Yes, those are just to make the PR "atomic" they need to be deleted after merge. I havent yet found a way to completely seperate the PRs 808 and 809
matthyx
requested changes
May 28, 2026
matthyx
left a comment
matthyx
left a comment
Contributor
There was a problem hiding this comment.
I re-checked this PR and I’m still seeing three merge blockers:
pkg/objectcache/containerprofilecache/tamper_alert.goreferencesc.cfg.EnableSignatureVerification, butpkg/config/config.gostill has no such field. LSP flags four compiler errors at the strict-mode returns.cmd/main.gocreates both the runtime alert exporter and the container profile cache, but never callscpc.SetTamperAlertExporter(exporter). As written,emitTamperAlert()is a no-op in production, so the new R1016 path never actually emits alerts end-to-end.pkg/signature/stub.go/pkg/signature/profiles/stub.gostill short-circuit the feature (IsSigned == false,VerifyObjectAllowUntrusted == nil, adapters returnnil), so the new tamper-detection behavior is effectively dormant and the added tamper tests are not validating real verification behavior.
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
matthyx
reviewed
May 28, 2026
matthyx
left a comment
matthyx
left a comment
Contributor
There was a problem hiding this comment.
Checked again.
The earlier import/IPAddresses blockers are addressed now: the PR has local pkg/signature stubs, and the storage replace brings NetworkNeighbor.IPAddresses.
I’m still seeing three blockers before merge:
pkg/objectcache/containerprofilecache/tamper_alert.gostill readsc.cfg.EnableSignatureVerificationat the strict-mode return sites, butpkg/config/config.gostill has no such field onconfig.Config.cmd/main.gocreates both the runtime alert exporter and the container profile cache, but never callscpc.SetTamperAlertExporter(exporter), so the new R1016 tamper path still never emits alerts end-to-end.pkg/signature/stub.goandpkg/signature/profiles/stub.gostill short-circuit the feature (IsSigned == false,VerifyObjectAllowUntrusted == nil, adapters returnnil), so the tamper-detection path remains dormant and the added tamper tests are not validating a real verification flow.
Contributor
Author
|
Component tests are green end-to-end for the tamper-alert wiring in Root cause that |
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (1)
tests/component_test.go (1)
2624-2644: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the stale duplicated enforcement notes.
These bullets still say no R-number fires on signature verification failure, but this PR adds
R1016coverage inTest_31. The block is also duplicated.🤖 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/component_test.go` around lines 2624 - 2644, Update the duplicated enforcement note block so it matches the current behavior: remove the stale claim that no R-number rule fires on signature verification failure, since `Test_31` now covers `R1016` for that path. Keep the description in `Test_30_TamperedSignedProfiles` accurate to the remaining behavior, and delete the repeated copy of the same bullets so the comment block appears only once.
🤖 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 `@cmd/sign-object/main.go`:
- Around line 400-443: The detectObjectType function only uses top-level
apiVersion and kind, so empty-TypeMeta profiles hit the unable to auto-detect
object type fallback even though loadApplicationProfile, loadSeccompProfile, and
loadNetworkNeighborhood can handle missing metadata. Update detectObjectType to
add a deterministic fallback path for the auto case when kind/apiVersion are
empty, using another discriminator from the decoded object shape or known
profile-specific fields instead of trying all loaders blindly. Keep the existing
explicit objectType handling unchanged and make sure the fallback resolves the
same empty-TypeMeta scenario covered by the cluster flow.
In `@pkg/rulemanager/ruleswatcher/watcher.go`:
- Around line 99-101: The skipped verification metric is counting rejected Rules
CRs instead of individual rules, which underreports failures when one invalid
resource contains multiple enabled rules. Update the verification path in
watcher.go around verifyRules so skippedVerificationCount is incremented per
skipped rule, matching the per-rule behavior used by enabledRules and
skippedByVersion. Use the same rule iteration/selection logic in the
RulesWatcher flow to count each enabled rule from a rejected Rules object,
including the similar code path referenced by the other affected block.
In `@pkg/signature/cosign_adapter_test.go`:
- Around line 128-137: The “Certificate is CA” negative test is using raw DER
from x509.CreateCertificate directly in Signature.Certificate, so the decode
step can fail before the CA rejection logic is exercised. Update the subtest in
cosign_adapter_test.go to PEM-encode the generated CA certificate before
assigning it to Signature.Certificate, using the existing certificate handling
path in Signature.Verify/parse logic so the intended “CA certificate rejected”
branch is actually covered.
In `@pkg/signature/cosign_adapter.go`:
- Around line 210-220: The strict verification path is rejecting signatures
created by this adapter because `signKeyless` returns no `RekorBundle` and
`signWithKey` is still treated like a Fulcio-issued cert before the local-issuer
branch runs. Update the verification logic in `VerifySignature`,
`verifyCertificates`, and the related keyless/local certificate handling so
adapter-produced signatures are accepted in strict mode by properly
short-circuiting or exempting local/self-signed issuer flows, and make sure the
round-trip path validates the `Signature` output from `signKeyless` and
`signWithKey` without requiring Rekor or Fulcio roots for those cases.
- Around line 126-185: The keyless signing flow in signKeyless always uses
context.Background(), so token acquisition and Fulcio/interactive auth can hang
indefinitely in non-interactive runs. Update signKeyless to accept and use a
caller-provided context, propagate it to c.tokenProvider, providers.Provide, and
oauthflow.OIDConnect, and wrap the signing path with a bounded timeout or
cancellation handling. Make the interactive fallback opt-in or gated so
tests/agents do not drop into browser-based OAuth by default.
In `@pkg/signature/profiles/adapter_test.go`:
- Around line 276-335: TestAdapterUniqueness currently only verifies both
adapters can be signed, not that signatures are unique per adapter/content.
Update the test to assert uniqueness by comparing the signatures returned from
GetObjectSignature for NewApplicationProfileAdapter and
NewSeccompProfileAdapter, or by signing two distinct adapter payloads and
confirming the resulting signature values differ. Keep the existing
SignObjectDisableKeyless setup, but add an explicit assertion in
TestAdapterUniqueness that proves the signature is bound to each adapter’s
content rather than just being locally signed.
In `@pkg/signature/profiles/applicationprofile_adapter.go`:
- Around line 40-57: GetContent() is mutating the wrapped ApplicationProfile
spec while normalizing PolicyByRuleId, which should not happen in the
hash/sign/verify path. Update ApplicationProfileAdapter.GetContent to build the
normalized payload from a copy of a.profile.Spec, and apply the nil-to-empty-map
normalization only on that copy for Containers, InitContainers, and
EphemeralContainers so the live spec remains unchanged.
- Around line 17-21: GetAnnotations currently mutates the wrapped profile by
initializing Annotations on read, which breaks read-only access patterns used by
VerifyObject and GetObjectSignature. Update
ApplicationProfileAdapter.GetAnnotations to return the existing map without
allocating or assigning, and move any nil-map initialization to the write path
or mutating methods; apply the same read-only behavior to the other
storage-backed adapter GetAnnotations implementations.
In `@pkg/signature/profiles/rules_adapter_test.go`:
- Around line 4-5: Update the rules adapter test to assert the returned error
matches the exported sentinel signature.ErrSignatureMismatch rather than only
checking the error text. In the test case around the verifier call in
rules_adapter_test.go (including the related assertions near the later lines
noted in the comment), keep any string checks only if needed for context, but
make the primary expectation use the exported tamper error so the contract stays
stable even if wrapping or wording changes.
In `@pkg/signature/sign_test.go`:
- Around line 204-208: The keyless signing setup in the sign test is swallowing
failures from SignObjectKeyless, which delays the real error until
GetObjectSignature. Update the setup path in the sign_test subtest so the result
of SignObjectKeyless is checked and any error is surfaced immediately with a
test failure, using the existing tt.setupSign / tt.profile flow to locate the
change.
In `@pkg/signature/signer.go`:
- Around line 18-19: The CosignSigner.Sign method currently dereferences
s.adapter without checking the receiver, so it can panic when the signer is nil
or not fully initialized. Add a nil/invalid receiver guard at the start of Sign,
matching the error-returning pattern used by CosignVerifier.Verify*, and return
a regular error instead of calling adapter.SignData when the signer is unusable.
In `@pkg/signature/verify_test.go`:
- Around line 216-231: The captureLogOutput helper in verify_test.go leaves the
global logger writer and pipe descriptors un-restored if fn fails or panics, so
move the cleanup into deferred teardown around logger.L().SetWriter and os.Pipe
handling. In captureLogOutput, save oldWriter, then defer restoring
logger.L().SetWriter(oldWriter) and closing w/r after the pipe is created, while
still copying the buffer after fn returns, so later tests never inherit the
temporary writer.
In `@pkg/signature/verify.go`:
- Around line 38-40: Treat malformed signature annotations as
ErrSignatureMismatch instead of returning a plain wrapped decode error. In
verify.go, update the error handling around
adapter.DecodeSignatureFromAnnotations in the verification path (and the same
decode handling in the other affected block) so that any decode failure after
AnnotationSignature is present is converted to ErrSignatureMismatch, preserving
the underlying decode details only as context if needed. Keep the fix localized
to the verification flow by using the existing verify logic and
ErrSignatureMismatch symbol so callers using errors.Is can route corrupted
signatures through the tamper path.
In `@tests/component_test.go`:
- Around line 2129-2130: The alert helper logic is masking Alertmanager fetch
failures as empty results, which can make negative assertions pass incorrectly.
Update the alert retrieval path in the helpers that use testutils.GetAlerts,
especially waitAlerts and countR1016, so the final GetAlerts call must succeed
instead of ignoring its error or returning 0/empty alerts on failure. Keep the
“no alerts” case only for a successful empty response, and surface any retrieval
error directly so tests fail when Alertmanager is unavailable.
- Around line 2798-2857: The test currently proves only that wget still triggers
R0001, which does not confirm the tampered exec entry was honored by the loaded
profile. In the same flow in the tamper-verification block, after
`storageClient.ApplicationProfiles(...).Get`,
`profiles.NewApplicationProfileAdapter`, and the `require.Eventually` cache-load
check, add an assertion using `wl.ExecIntoPod` and `testutils.GetAlerts` that
`nslookup` does not emit `R0001`, so the test verifies the node-agent actually
loaded the tampered `ApplicationProfile` rather than the original signed
baseline.
- Around line 2591-2593: The R0001 check is too broad and can pass on an
unrelated alert from the earlier allowed exec. Tighten the assertion in the test
that iterates over alerts so it only matches the anomalous nslookup command’s
alert, using the existing alert fields in that loop and the surrounding
Eventually/final count logic to verify the specific command rather than any
R0001 in the namespace. Apply the same scoping fix in the later assertion block
that has the same issue.
- Around line 2149-2164: The no-alert subtest can pass even when the allowed
traffic never happened because `wl.ExecIntoPod` failures for `nslookup` and
`curl` are only logged. In this test block, make the `fusioncore.ai` checks in
`Test...`/the relevant subtest fail fast if either allowed operation returns an
error, so `waitAlerts`, `countByRule`, and the `R0005`/`R0011` assertions only
run after successful DNS and HTTP traffic generation.
---
Nitpick comments:
In `@tests/component_test.go`:
- Around line 2624-2644: Update the duplicated enforcement note block so it
matches the current behavior: remove the stale claim that no R-number rule fires
on signature verification failure, since `Test_31` now covers `R1016` for that
path. Keep the description in `Test_30_TamperedSignedProfiles` accurate to the
remaining behavior, and delete the repeated copy of the same bullets so the
comment block appears only once.
🪄 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: 89702d91-ffab-479e-a7cf-ff04223b892a
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (35)
cmd/main.gocmd/sign-object/Dockerfilecmd/sign-object/main.gogo.modpkg/config/config.gopkg/containerprofilemanager/v1/lifecycle.gopkg/exporters/alert_manager.gopkg/objectcache/containerprofilecache/tamper_alert_test.gopkg/rulemanager/cel/libraries/applicationprofile/ap.gopkg/rulemanager/ruleswatcher/watcher.gopkg/rulemanager/ruleswatcher/watcher_signature_test.gopkg/signature/annotations.gopkg/signature/cluster_flow_test.gopkg/signature/cluster_scenario_test.gopkg/signature/cosign_adapter.gopkg/signature/cosign_adapter_test.gopkg/signature/interface.gopkg/signature/profiles/adapter_test.gopkg/signature/profiles/applicationprofile_adapter.gopkg/signature/profiles/empty_typemeta_test.gopkg/signature/profiles/networkneighborhood_adapter.gopkg/signature/profiles/networkneighborhood_adapter_test.gopkg/signature/profiles/rules_adapter.gopkg/signature/profiles/rules_adapter_test.gopkg/signature/profiles/seccompprofile_adapter.gopkg/signature/sign.gopkg/signature/sign_test.gopkg/signature/signer.gopkg/signature/verifier.gopkg/signature/verify.gopkg/signature/verify_test.gotests/component_test.gotests/resources/aplint_test.gotests/resources/curl-signed-deployment.yamltests/resources/known-application-profile.yaml
✅ Files skipped from review due to trivial changes (5)
- cmd/sign-object/Dockerfile
- pkg/signature/annotations.go
- pkg/containerprofilemanager/v1/lifecycle.go
- pkg/rulemanager/cel/libraries/applicationprofile/ap.go
- pkg/signature/profiles/networkneighborhood_adapter_test.go
Comment on lines
+400
to
+443
| func detectObjectType(objectType string, data []byte) (signature.SignableObject, error) { | ||
| var decoded map[string]interface{} | ||
| if err := k8syaml.Unmarshal(data, &decoded); err != nil { | ||
| return nil, fmt.Errorf("failed to unmarshal YAML: %w", err) | ||
| } | ||
|
|
||
| kind, _ := decoded["kind"].(string) | ||
| apiVersion, _ := decoded["apiVersion"].(string) | ||
|
|
||
| if verbose { | ||
| fmt.Printf("Detected API: %s, Kind: %s\n", apiVersion, kind) | ||
| } | ||
|
|
||
| if objectType != "auto" { | ||
| switch strings.ToLower(objectType) { | ||
| case "applicationprofile", "application-profile", "ap": | ||
| return loadApplicationProfile(data) | ||
| case "seccompprofile", "seccomp-profile", "sp": | ||
| return loadSeccompProfile(data) | ||
| case "networkneighborhood", "network-neighborhood", "nn": | ||
| return loadNetworkNeighborhood(data) | ||
| case "rules", "rule", "r": | ||
| return loadRules(data) | ||
| default: | ||
| return nil, fmt.Errorf("unknown object type: %s", objectType) | ||
| } | ||
| } | ||
|
|
||
| if strings.Contains(strings.ToLower(apiVersion), "softwarecomposition") { | ||
| switch strings.ToLower(kind) { | ||
| case "applicationprofile", "application-profile": | ||
| return loadApplicationProfile(data) | ||
| case "seccompprofile", "seccomp-profile": | ||
| return loadSeccompProfile(data) | ||
| case "networkneighborhood", "network-neighborhood": | ||
| return loadNetworkNeighborhood(data) | ||
| } | ||
| } | ||
|
|
||
| if strings.Contains(strings.ToLower(apiVersion), "kubescape.io") && strings.ToLower(kind) == "rules" { | ||
| return loadRules(data) | ||
| } | ||
|
|
||
| return nil, fmt.Errorf("unable to auto-detect object type") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
--type auto breaks on the empty-TypeMeta profiles this PR explicitly supports.
detectObjectType only dispatches from top-level apiVersion/kind, so profiles loaded from the cluster with both fields empty fall through to unable to auto-detect object type. That makes the default CLI path fail for the same production scenario covered in pkg/signature/cluster_scenario_test.go, even though the adapters themselves can normalize missing TypeMeta.
Please add a real fallback discriminator here for empty-TypeMeta objects rather than relying solely on apiVersion/kind. Avoid a naive “try all unmarshals” loop, since these structs will happily decode unknown YAML into zero values and can false-positive.
🤖 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 `@cmd/sign-object/main.go` around lines 400 - 443, The detectObjectType
function only uses top-level apiVersion and kind, so empty-TypeMeta profiles hit
the unable to auto-detect object type fallback even though
loadApplicationProfile, loadSeccompProfile, and loadNetworkNeighborhood can
handle missing metadata. Update detectObjectType to add a deterministic fallback
path for the auto case when kind/apiVersion are empty, using another
discriminator from the decoded object shape or known profile-specific fields
instead of trying all loaders blindly. Keep the existing explicit objectType
handling unchanged and make sure the fallback resolves the same empty-TypeMeta
scenario covered by the cluster flow.
Comment on lines
+128
to
+137
| t.Run("Certificate is CA", func(t *testing.T) { | ||
| // Create a CA certificate | ||
| template := x509.Certificate{ | ||
| IsCA: true, | ||
| } | ||
| certDER, _ := x509.CreateCertificate(rand.Reader, &template, &template, &privKey.PublicKey, privKey) | ||
| sig := &Signature{ | ||
| Signature: []byte("sig"), | ||
| Certificate: certDER, | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
PEM-encode the CA cert in this negative test.
x509.CreateCertificate returns DER, but this subtest passes it straight into Signature.Certificate. That means Line 138 can fail on certificate decoding instead of the intended “CA certificate rejected” path, so this branch is not actually covered.
Proposed fix
t.Run("Certificate is CA", func(t *testing.T) {
// Create a CA certificate
template := x509.Certificate{
IsCA: true,
}
certDER, _ := x509.CreateCertificate(rand.Reader, &template, &template, &privKey.PublicKey, privKey)
sig := &Signature{
Signature: []byte("sig"),
- Certificate: certDER,
+ Certificate: pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: certDER}),
}
err := adapter.VerifyData(data, sig, false)
if err == nil {
t.Error("Expected error for CA certificate, got nil")
}
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| t.Run("Certificate is CA", func(t *testing.T) { | |
| // Create a CA certificate | |
| template := x509.Certificate{ | |
| IsCA: true, | |
| } | |
| certDER, _ := x509.CreateCertificate(rand.Reader, &template, &template, &privKey.PublicKey, privKey) | |
| sig := &Signature{ | |
| Signature: []byte("sig"), | |
| Certificate: certDER, | |
| } | |
| t.Run("Certificate is CA", func(t *testing.T) { | |
| // Create a CA certificate | |
| template := x509.Certificate{ | |
| IsCA: true, | |
| } | |
| certDER, _ := x509.CreateCertificate(rand.Reader, &template, &template, &privKey.PublicKey, privKey) | |
| sig := &Signature{ | |
| Signature: []byte("sig"), | |
| Certificate: pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: certDER}), | |
| } |
🤖 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/signature/cosign_adapter_test.go` around lines 128 - 137, The
“Certificate is CA” negative test is using raw DER from x509.CreateCertificate
directly in Signature.Certificate, so the decode step can fail before the CA
rejection logic is exercised. Update the subtest in cosign_adapter_test.go to
PEM-encode the generated CA certificate before assigning it to
Signature.Certificate, using the existing certificate handling path in
Signature.Verify/parse logic so the intended “CA certificate rejected” branch is
actually covered.
Comment on lines
+38
to
+40
| sig, err := adapter.DecodeSignatureFromAnnotations(annotations) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to decode signature from annotations: %w", err) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Treat malformed signature annotations as ErrSignatureMismatch.
Once AnnotationSignature is present, a decode failure is no longer “unsigned” — it is a broken or tampered signed payload. Returning a plain error here means downstream callers that rely on errors.Is(err, ErrSignatureMismatch) will classify corrupted signature metadata as operational and skip the tamper path/R1016 emission.
Suggested fix
sig, err := adapter.DecodeSignatureFromAnnotations(annotations)
if err != nil {
- return fmt.Errorf("failed to decode signature from annotations: %w", err)
+ return fmt.Errorf("%w: failed to decode signature from annotations: %w", ErrSignatureMismatch, err)
}Also applies to: 67-71
🤖 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/signature/verify.go` around lines 38 - 40, Treat malformed signature
annotations as ErrSignatureMismatch instead of returning a plain wrapped decode
error. In verify.go, update the error handling around
adapter.DecodeSignatureFromAnnotations in the verification path (and the same
decode handling in the other affected block) so that any decode failure after
AnnotationSignature is present is converted to ErrSignatureMismatch, preserving
the underlying decode details only as context if needed. Keep the fix localized
to the verification flow by using the existing verify logic and
ErrSignatureMismatch symbol so callers using errors.Is can route corrupted
signatures through the tamper path.
Comment on lines
+2129
to
+2130
| alerts, _ = testutils.GetAlerts(ns) | ||
| return alerts |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don’t convert alert retrieval failures into “no alerts”.
Both helpers can make negative assertions pass when Alertmanager is unavailable: waitAlerts drops the final GetAlerts error, and countR1016 returns 0 on error. Require the final fetch to succeed so “no alert” is meaningful.
Proposed fix
- alerts, _ = testutils.GetAlerts(ns)
+ alerts, err = testutils.GetAlerts(ns)
+ require.NoError(t, err, "get alerts from ns %s", ns)
return alerts alerts, err := testutils.GetAlerts(nsName)
- if err != nil {
- t.Logf("GetAlerts error: %v", err)
- return 0
- }
+ require.NoError(t, err, "get alerts from ns %s", nsName)Also applies to: 3047-3050
🤖 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/component_test.go` around lines 2129 - 2130, The alert helper logic is
masking Alertmanager fetch failures as empty results, which can make negative
assertions pass incorrectly. Update the alert retrieval path in the helpers that
use testutils.GetAlerts, especially waitAlerts and countR1016, so the final
GetAlerts call must succeed instead of ignoring its error or returning 0/empty
alerts on failure. Keep the “no alerts” case only for a successful empty
response, and surface any retrieval error directly so tests fail when
Alertmanager is unavailable.
Comment on lines
+2149
to
+2164
| // DNS lookup via nslookup (domain in NN). | ||
| stdout, stderr, err := wl.ExecIntoPod([]string{"nslookup", "fusioncore.ai"}, "curl") | ||
| t.Logf("nslookup fusioncore.ai → err=%v stdout=%q stderr=%q", err, stdout, stderr) | ||
|
|
||
| // HTTP via curl (domain + IP in NN). | ||
| stdout, stderr, err = wl.ExecIntoPod([]string{"curl", "-sm5", "http://fusioncore.ai"}, "curl") | ||
| t.Logf("curl fusioncore.ai → err=%v stdout=%q stderr=%q", err, stdout, stderr) | ||
|
|
||
| alerts := waitAlerts(t, wl.Namespace) | ||
| t.Logf("=== %d alerts ===", len(alerts)) | ||
| logAlerts(t, alerts) | ||
|
|
||
| assert.Equal(t, 0, countByRule(alerts, "R0005"), | ||
| "fusioncore.ai is in NN — should NOT fire R0005") | ||
| assert.Equal(t, 0, countByRule(alerts, "R0011"), | ||
| "fusioncore.ai IP is in NN — should NOT fire R0011") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail the no-alert case when allowed traffic was never generated.
This subtest only logs nslookup/curl errors, so DNS or egress failures can still pass as “no R0005/R0011”. Require the allowed operations to succeed before asserting no alerts.
Proposed fix
stdout, stderr, err := wl.ExecIntoPod([]string{"nslookup", "fusioncore.ai"}, "curl")
t.Logf("nslookup fusioncore.ai → err=%v stdout=%q stderr=%q", err, stdout, stderr)
+ require.NoError(t, err, "allowed DNS lookup must succeed before asserting no R0005")
// HTTP via curl (domain + IP in NN).
stdout, stderr, err = wl.ExecIntoPod([]string{"curl", "-sm5", "http://fusioncore.ai"}, "curl")
t.Logf("curl fusioncore.ai → err=%v stdout=%q stderr=%q", err, stdout, stderr)
+ require.NoError(t, err, "allowed HTTP egress must succeed before asserting no R0011")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // DNS lookup via nslookup (domain in NN). | |
| stdout, stderr, err := wl.ExecIntoPod([]string{"nslookup", "fusioncore.ai"}, "curl") | |
| t.Logf("nslookup fusioncore.ai → err=%v stdout=%q stderr=%q", err, stdout, stderr) | |
| // HTTP via curl (domain + IP in NN). | |
| stdout, stderr, err = wl.ExecIntoPod([]string{"curl", "-sm5", "http://fusioncore.ai"}, "curl") | |
| t.Logf("curl fusioncore.ai → err=%v stdout=%q stderr=%q", err, stdout, stderr) | |
| alerts := waitAlerts(t, wl.Namespace) | |
| t.Logf("=== %d alerts ===", len(alerts)) | |
| logAlerts(t, alerts) | |
| assert.Equal(t, 0, countByRule(alerts, "R0005"), | |
| "fusioncore.ai is in NN — should NOT fire R0005") | |
| assert.Equal(t, 0, countByRule(alerts, "R0011"), | |
| "fusioncore.ai IP is in NN — should NOT fire R0011") | |
| // DNS lookup via nslookup (domain in NN). | |
| stdout, stderr, err := wl.ExecIntoPod([]string{"nslookup", "fusioncore.ai"}, "curl") | |
| t.Logf("nslookup fusioncore.ai → err=%v stdout=%q stderr=%q", err, stdout, stderr) | |
| require.NoError(t, err, "allowed DNS lookup must succeed before asserting no R0005") | |
| // HTTP via curl (domain + IP in NN). | |
| stdout, stderr, err = wl.ExecIntoPod([]string{"curl", "-sm5", "http://fusioncore.ai"}, "curl") | |
| t.Logf("curl fusioncore.ai → err=%v stdout=%q stderr=%q", err, stdout, stderr) | |
| require.NoError(t, err, "allowed HTTP egress must succeed before asserting no R0011") | |
| alerts := waitAlerts(t, wl.Namespace) | |
| t.Logf("=== %d alerts ===", len(alerts)) | |
| logAlerts(t, alerts) | |
| assert.Equal(t, 0, countByRule(alerts, "R0005"), | |
| "fusioncore.ai is in NN — should NOT fire R0005") | |
| assert.Equal(t, 0, countByRule(alerts, "R0011"), | |
| "fusioncore.ai IP is in NN — should NOT fire R0011") |
🤖 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/component_test.go` around lines 2149 - 2164, The no-alert subtest can
pass even when the allowed traffic never happened because `wl.ExecIntoPod`
failures for `nslookup` and `curl` are only logged. In this test block, make the
`fusioncore.ai` checks in `Test...`/the relevant subtest fail fast if either
allowed operation returns an error, so `waitAlerts`, `countByRule`, and the
`R0005`/`R0011` assertions only run after successful DNS and HTTP traffic
generation.
Comment on lines
+2591
to
+2593
| for _, a := range alerts { | ||
| if a.Labels["rule_id"] == "R0001" { | ||
| return true |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Filter the R0001 assertion to the anomalous command.
The test intends to prove nslookup is anomalous, but any R0001 in the namespace satisfies the Eventually and final count. A false positive from the earlier allowed curl exec would still pass.
Proposed fix
- if a.Labels["rule_id"] == "R0001" {
+ if a.Labels["rule_id"] == "R0001" && a.Labels["comm"] == "nslookup" {
return true
} for _, a := range alerts {
- if a.Labels["rule_id"] == "R0001" {
+ if a.Labels["rule_id"] == "R0001" && a.Labels["comm"] == "nslookup" {
r0001Count++
}
}Also applies to: 2611-2617
🤖 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/component_test.go` around lines 2591 - 2593, The R0001 check is too
broad and can pass on an unrelated alert from the earlier allowed exec. Tighten
the assertion in the test that iterates over alerts so it only matches the
anomalous nslookup command’s alert, using the existing alert fields in that loop
and the surrounding Eventually/final count logic to verify the specific command
rather than any R0001 in the namespace. Apply the same scoping fix in the later
assertion block that has the same issue.
Comment on lines
+2798
to
+2857
| // Tamper: attacker adds nslookup to the whitelist. | ||
| ap.Spec.Containers[0].Execs = append(ap.Spec.Containers[0].Execs, | ||
| v1beta1.ExecCalls{Path: "/usr/bin/nslookup"}) | ||
|
|
||
| // Signature is now invalid. | ||
| tamperedAdapter := profiles.NewApplicationProfileAdapter(ap) | ||
| require.Error(t, signature.VerifyObjectAllowUntrusted(tamperedAdapter), | ||
| "tampered AP must fail verification") | ||
|
|
||
| // Push tampered AP to storage (signature annotations are stale). | ||
| _, err := storageClient.ApplicationProfiles(ns.Name).Create( | ||
| context.Background(), ap, metav1.CreateOptions{}) | ||
| require.NoError(t, err, "push tampered AP to storage") | ||
|
|
||
| // Verify stored AP has stale signature. | ||
| require.Eventually(t, func() bool { | ||
| stored, getErr := storageClient.ApplicationProfiles(ns.Name).Get( | ||
| context.Background(), "signed-ap", v1.GetOptions{}) | ||
| if getErr != nil { | ||
| return false | ||
| } | ||
| storedAdapter := profiles.NewApplicationProfileAdapter(stored) | ||
| // Signature annotation exists but verification should fail. | ||
| if !signature.IsSigned(storedAdapter) { | ||
| return false | ||
| } | ||
| return signature.VerifyObjectAllowUntrusted(storedAdapter) != nil | ||
| }, 30*time.Second, 1*time.Second, "stored AP must have stale signature that fails verification") | ||
| t.Log("Stored AP has invalid signature (tamper detected at crypto layer)") | ||
|
|
||
| // Deploy pod referencing the tampered profile. | ||
| wl, err := testutils.NewTestWorkload(ns.Name, | ||
| path.Join(utils.CurrentDir(), "resources/curl-signed-deployment.yaml")) | ||
| require.NoError(t, err) | ||
| require.NoError(t, wl.WaitForReady(80)) | ||
|
|
||
| // Drive the unexpected exec inside Eventually so cache-load latency | ||
| // is absorbed by retries instead of a blind sleep. Same pattern as | ||
| // Test_29 (signed AP, anomalous exec) — without it, the first exec | ||
| // can land before the CP cache projects the user-defined AP, the | ||
| // rule manager evaluates against an empty baseline, and R0001 never | ||
| // fires within the polling window. | ||
| // | ||
| // wget is NOT in the AP (even after the attacker added nslookup), so | ||
| // once the cache loads, every wget exec produces an R0001 alert. | ||
| var alerts []testutils.Alert | ||
| require.Eventually(t, func() bool { | ||
| wl.ExecIntoPod([]string{"wget", "-qO-", "--timeout=2", "http://ebpf.io"}, "curl") | ||
| alerts, err = testutils.GetAlerts(ns.Name) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| for _, a := range alerts { | ||
| if a.Labels["rule_id"] == "R0001" && a.Labels["comm"] == "wget" { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| }, 120*time.Second, 10*time.Second, | ||
| "wget not in tampered AP must fire R0001 — proves tampered profile was loaded (enforcement off)") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert the tampered exec is actually honored.
The test appends nslookup to the tampered AP, but only proves wget still alerts. If node-agent loaded the original signed baseline or dropped the tampered exec, wget would still fire and this test would pass. After cache load is proven, also assert nslookup does not produce R0001.
🤖 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/component_test.go` around lines 2798 - 2857, The test currently proves
only that wget still triggers R0001, which does not confirm the tampered exec
entry was honored by the loaded profile. In the same flow in the
tamper-verification block, after `storageClient.ApplicationProfiles(...).Get`,
`profiles.NewApplicationProfileAdapter`, and the `require.Eventually` cache-load
check, add an assertion using `wl.ExecIntoPod` and `testutils.GetAlerts` that
`nslookup` does not emit `R0001`, so the test verifies the node-agent actually
loaded the tampered `ApplicationProfile` rather than the original signed
baseline.
entlein
pushed a commit
to k8sstormcenter/node-agent
that referenced
this pull request
Jun 25, 2026
Three matthyx blockers (2026-05-27): (1) tamper_alert.go:28 — imports pkg/signature and pkg/signature/profiles which ship in node-agent#809 (not yet merged). Adds minimum-surface stubs (pkg/signature/stub.go, pkg/signature/profiles/stub.go) so the PR compiles standalone. With IsSigned returning false, the tamper path short-circuits and never invokes Verify — the no-op stub is behavior- safe: signed-profile detection is dormant until kubescape#809 lands and replaces these files with the real implementation. (2) projection_apply.go:265 — NetworkNeighbor.IPAddresses missing. (3) mock.go:202 — same IPAddresses issue. Both (2) and (3) resolved by replacing kubescape/storage with k8sstormcenter/storage's upstream-pr/sbob-network tip 46f37d32 (sibling of kubescape/storage#324). Removed when kubescape#324 merges and a release ships IPAddresses. Signed-off-by: entlein <einentlein@gmail.com>
entlein
force-pushed
the
upstream-pr/sbob-crd-compaction
branch
from
6e0d0c3 to
44dec25
Compare
matthyx
pushed a commit
to k8sstormcenter/node-agent
that referenced
this pull request
Jun 26, 2026
…naged lifecycle + signing/tamper detection Squashed and rebased onto main (resolving conflicts against the merged kubescape#806 network-wildcards and kubescape#807 exec-args work). Adds, on top of main's projection surface: - Cryptographic signing/verification of ApplicationProfile, NetworkNeighborhood, seccomp and rules profiles (pkg/signature, cosign-backed, key-based + keyless). - Tamper detection for signed user overlays: re-verifies on every ContainerProfileCache load and emits R1016 "Signed profile tampered". - Rule-signature verification in ruleswatcher. - sign-object CLI (sign / verify / generate-keypair / extract-signature). - User-defined NetworkNeighborhood overlay support + SyncChecksum overlay-identity stamping. - enableSignatureVerification config flag. Conflict resolution: kept main's tested projection design (projectField isPathSurface, ExecsByPath [][]string) and rule-binding notificationQueue; the PR's redundant reimplementation of that surface was dropped in favour of the merged kubescape#806/kubescape#807 versions. Only the net-new features above were grafted on top. go.mod: pins the official kubescape/storage v0.0.291 (carries storage kubescape#325), dropping the temporary k8sstormcenter/storage fork replace; adds sigstore (fulcio, rekor) dependencies for signing. Feature documented in docs/features/profile-signing-and-tamper-detection.md. Squashed from PR kubescape#808 (kubescape/node-agent). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Duck <70207455+entlein@users.noreply.github.com>
matthyx
force-pushed
the
upstream-pr/sbob-crd-compaction
branch
from
44dec25 to
432fef2
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/objectcache/containerprofilecache/containerprofilecache.go (1)
417-450: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winVerify the
ug-<workload>overlays too.The new tamper gate only covers label-referenced
userAP/userNN.userManagedAP/userManagedNNfetched above still flow intoprojectUserProfilesat Lines 493-497 without R1016 emission or strict-mode dropping, so signed user-managed overlays can bypass tamper detection.Proposed fix
if ugNNErr != nil { if shouldLogOptionalUserManagedFetchError(ugNNErr) { logger.L().Debug("failed to fetch user-managed NetworkNeighborhood", helpers.String("containerID", containerID), helpers.String("namespace", ns), helpers.String("name", ugNNName), helpers.Error(ugNNErr)) } userManagedNN = nil } + if userManagedAP != nil { + if !c.verifyUserApplicationProfile(userManagedAP, sharedData.Wlid) { + userManagedAP = nil + } + } + if userManagedNN != nil { + if !c.verifyUserNetworkNeighborhood(userManagedNN, sharedData.Wlid) { + userManagedNN = nil + } + } }🤖 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/objectcache/containerprofilecache/containerprofilecache.go` around lines 417 - 450, The tamper-check logic in containerprofilecache should also cover the ug-<workload> overlays, not just the label-referenced userAP and userNN paths. Update the overlay-loading flow around verifyUserApplicationProfile and verifyUserNetworkNeighborhood so the userManagedAP and userManagedNN values are verified with the same signature/tamper gate before projectUserProfiles consumes them, and drop the managed overlays in strict mode when verification fails. Keep the existing permissive behavior for unsigned overlays, but ensure signed user-managed overlays cannot bypass R1016 emission or silently merge into the cache.
🧹 Nitpick comments (3)
pkg/objectcache/containerprofilecache/tamper_alert_test.go (1)
53-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese tests don't exercise the cache's classification path.
This block only proves
errors.Isandsync.Mapsemantics locally, so it will keep passing even ifverifyUserApplicationProfilestops consultingErrSignatureMismatchor the dedup wiring regresses. I'd replace it with a seam-injected verifier test or fold the intent into the end-to-end cases below.🤖 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/objectcache/containerprofilecache/tamper_alert_test.go` around lines 53 - 102, The new tests only validate local errors.Is and sync.Map behavior, so they do not cover the real classification flow in ContainerProfileCacheImpl. Replace these with a seam-injected test that drives verifyUserApplicationProfile (or the existing end-to-end tamper cases) and assert that signature.ErrSignatureMismatch reaches the tamper dedup path while operational errors do not. Use the existing tamperEmitted and emitTamperAlert-related flow to verify the cache’s actual routing instead of standalone fixtures.pkg/objectcache/containerprofilecache/tamper_alert.go (1)
32-38: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftPrune stale resource-version entries from
tamperEmitted.The dedup key includes
ResourceVersion, but the cleanup path only deletes the current key on a clean verify. Every tampered edit at a new RV leaves the old entry behind forever, so a frequently updated signed overlay can grow this map monotonically for the lifetime of the agent.Also applies to: 77-79, 105-108, 122-146
🤖 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/objectcache/containerprofilecache/tamper_alert.go` around lines 32 - 38, The tamper dedup map is leaking stale entries because `tamperKey` includes `ResourceVersion`, but the cleanup path only removes the current key after a clean verify. Update the `tamperEmitted` handling in `verify`, `recordTamper`, and the clean-up logic to prune old keys for the same profile identity (kind/namespace/name) when a clean reconcile succeeds, while still preserving per-RV re-flagging for new tampered edits.pkg/signature/verify_test.go (1)
84-87: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert
ErrSignatureMismatchhere, not just “any error”.This test currently passes on verifier setup failures too. The tamper path depends on
errors.Is(err, ErrSignatureMismatch), so this case should lock that contract down explicitly.Suggested change
import ( + "errors" "io" "os" "strings" "testing" @@ err = VerifyObjectStrict(profile) if err == nil { t.Error("Expected verification failure for tampered profile, got success") + } else if !errors.Is(err, ErrSignatureMismatch) { + t.Fatalf("expected ErrSignatureMismatch, got: %v", err) } }🤖 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/signature/verify_test.go` around lines 84 - 87, The tampered-profile check in VerifyObjectStrict only asserts that some error occurred, so it can pass on unrelated setup failures. Update the test to explicitly verify the returned error matches ErrSignatureMismatch using errors.Is, keeping the existing VerifyObjectStrict path and the tamper case focused on the signature-mismatch contract.
🤖 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 `@cmd/main.go`:
- Around line 544-550: The best-effort pod lookup in resolveOwnContainerID can
block startup because it uses the unbounded context from main for the Kubernetes
GET call. Update resolveOwnContainerID to use a bounded context for the pod
lookup, such as a timeout or deadline around
k8sClient.GetKubernetesClient().CoreV1().Pods(namespace).Get, so startup cannot
hang if the API server or network is slow. Keep the change localized to
resolveOwnContainerID and preserve the existing fallback behavior of returning
an empty container ID on lookup failure.
In `@cmd/sign-object/Dockerfile`:
- Around line 17-20: The runtime image for sign-object still starts as root, so
update the final stage in the Dockerfile to run the copied sign-object binary as
a non-root user. Use the existing builder/runtime stages and the ENTRYPOINT
setup as the place to switch users, and make sure the chosen user can still
access /work and execute /usr/local/bin/sign-object.
In `@cmd/sign-object/main.go`:
- Line 549: The help text in the sign-object command still points to the removed
signing README, so update the reference in the sign-object help string to the
new profile signing doc. Locate the help/documentation text near the sign-object
command setup in main.go and replace the docs/signing/README.md reference with
docs/features/profile-signing-and-tamper-detection.md so --help points to the
correct document.
In `@pkg/objectcache/containerprofilecache/containerprofilecache.go`:
- Around line 699-710: The retry budget comment in
ContainerProfileCacheImpl.NotifyContainerCompleted does not match the actual
loop behavior, since the goroutine currently retries 20 times while the comment
says 5 attempts × 3 s. Update the comment to reflect the real retry count or,
preferably, extract the retry count and delay into named constants used by the
loop so the documented budget stays consistent with the implementation.
In `@pkg/objectcache/containerprofilecache/tamper_alert_test.go`:
- Around line 256-282: The test currently never enters the operational-error
path because the ApplicationProfile is unsigned and verifyUserApplicationProfile
exits early at the IsSigned check. Update
TestVerifyAP_OperationalError_DoesNotEmit to use a signed profile with malformed
signature data, or otherwise inject a verifier/decode/hash error, so the failure
flows through the non-tamper verification branch and still asserts that
captureExporter receives no R1016 alerts.
In `@pkg/signature/cosign_adapter.go`:
- Around line 402-458: The strict verification path in cosign_adapter.go is
trusting mutable annotation data from sig.Timestamp and sig.RekorBundle, which
can be rewritten by a client. Update the verification flow around the
certificate checks in the signature verification logic to derive signing time
from trusted Rekor/CT evidence instead of using sig.Timestamp, or fail
closed/use current time until that evidence is actually validated. Also ensure
the Rekor bundle handling does not merely accept any non-empty sig.RekorBundle;
validate it before treating it as proof, and keep the identity and certificate
checks in the same strict path.
- Around line 541-551: The signature parsing in cosign_adapter.go should fail
fast instead of falling back to raw bytes when base64 decoding
AnnotationSignature fails. Update the annotation handling in the signature
decode path around the signatureB64/base64.StdEncoding.DecodeString logic so
malformed signatures return an error immediately, preserving the
ErrSignatureMismatch contract for actual content mismatches only. Use the
existing signature decode flow in the function that reads annotations and keep
the error path explicit for invalid AnnotationSignature values.
In `@tests/resources/aplint_test.go`:
- Around line 243-249: The YAML validation in the fixture walk is happening
before the template-fixture skip, so templated files still fail in
yaml.Unmarshal and never reach the later R-AP-00/templatePlaceholderRe
exclusion. Update the logic in aplint_test.go around the directory scan to
detect and skip template placeholders before calling yaml.Unmarshal, and apply
the same ordering to the other affected validation block so only concrete
fixtures are parsed.
In `@tests/resources/known-application-profile.yaml`:
- Around line 42-43: The fixture entry for the setgroups path is missing the
procfs prefix and should be corrected in the known-application profile data.
Update the path value in the procfs fixture so it matches the surrounding
entries under the same recorded profile shape, using the existing setgroups
record as the target to locate it.
---
Outside diff comments:
In `@pkg/objectcache/containerprofilecache/containerprofilecache.go`:
- Around line 417-450: The tamper-check logic in containerprofilecache should
also cover the ug-<workload> overlays, not just the label-referenced userAP and
userNN paths. Update the overlay-loading flow around
verifyUserApplicationProfile and verifyUserNetworkNeighborhood so the
userManagedAP and userManagedNN values are verified with the same
signature/tamper gate before projectUserProfiles consumes them, and drop the
managed overlays in strict mode when verification fails. Keep the existing
permissive behavior for unsigned overlays, but ensure signed user-managed
overlays cannot bypass R1016 emission or silently merge into the cache.
---
Nitpick comments:
In `@pkg/objectcache/containerprofilecache/tamper_alert_test.go`:
- Around line 53-102: The new tests only validate local errors.Is and sync.Map
behavior, so they do not cover the real classification flow in
ContainerProfileCacheImpl. Replace these with a seam-injected test that drives
verifyUserApplicationProfile (or the existing end-to-end tamper cases) and
assert that signature.ErrSignatureMismatch reaches the tamper dedup path while
operational errors do not. Use the existing tamperEmitted and
emitTamperAlert-related flow to verify the cache’s actual routing instead of
standalone fixtures.
In `@pkg/objectcache/containerprofilecache/tamper_alert.go`:
- Around line 32-38: The tamper dedup map is leaking stale entries because
`tamperKey` includes `ResourceVersion`, but the cleanup path only removes the
current key after a clean verify. Update the `tamperEmitted` handling in
`verify`, `recordTamper`, and the clean-up logic to prune old keys for the same
profile identity (kind/namespace/name) when a clean reconcile succeeds, while
still preserving per-RV re-flagging for new tampered edits.
In `@pkg/signature/verify_test.go`:
- Around line 84-87: The tampered-profile check in VerifyObjectStrict only
asserts that some error occurred, so it can pass on unrelated setup failures.
Update the test to explicitly verify the returned error matches
ErrSignatureMismatch using errors.Is, keeping the existing VerifyObjectStrict
path and the tamper case focused on the signature-mismatch contract.
🪄 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: ef38baee-92ab-4905-8bc9-2558f657b460
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (42)
cmd/main.gocmd/sign-object/Dockerfilecmd/sign-object/main.godocs/features/profile-signing-and-tamper-detection.mdgo.modpkg/config/config.gopkg/containerprofilemanager/v1/lifecycle.gopkg/exporters/alert_manager.gopkg/objectcache/containerprofilecache/containerprofilecache.gopkg/objectcache/containerprofilecache/projection.gopkg/objectcache/containerprofilecache/tamper_alert.gopkg/objectcache/containerprofilecache/tamper_alert_test.gopkg/objectcache/containerprofilecache/test32_projection_test.gopkg/objectcache/shared_container_data.gopkg/rulemanager/cel/libraries/applicationprofile/ap.gopkg/rulemanager/cel/libraries/cache/function_cache.gopkg/rulemanager/ruleswatcher/watcher.gopkg/rulemanager/ruleswatcher/watcher_signature_test.gopkg/signature/annotations.gopkg/signature/cluster_flow_test.gopkg/signature/cluster_scenario_test.gopkg/signature/cosign_adapter.gopkg/signature/cosign_adapter_test.gopkg/signature/interface.gopkg/signature/profiles/adapter_test.gopkg/signature/profiles/applicationprofile_adapter.gopkg/signature/profiles/empty_typemeta_test.gopkg/signature/profiles/networkneighborhood_adapter.gopkg/signature/profiles/networkneighborhood_adapter_test.gopkg/signature/profiles/rules_adapter.gopkg/signature/profiles/rules_adapter_test.gopkg/signature/profiles/seccompprofile_adapter.gopkg/signature/sign.gopkg/signature/sign_test.gopkg/signature/signer.gopkg/signature/verifier.gopkg/signature/verify.gopkg/signature/verify_test.gotests/component_test.gotests/resources/aplint_test.gotests/resources/curl-signed-deployment.yamltests/resources/known-application-profile.yaml
💤 Files with no reviewable changes (1)
- tests/component_test.go
✅ Files skipped from review due to trivial changes (2)
- pkg/rulemanager/cel/libraries/applicationprofile/ap.go
- pkg/containerprofilemanager/v1/lifecycle.go
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/rulemanager/cel/libraries/cache/function_cache.go
- pkg/exporters/alert_manager.go
- pkg/config/config.go
- pkg/objectcache/shared_container_data.go
- pkg/objectcache/containerprofilecache/projection.go
- pkg/rulemanager/ruleswatcher/watcher.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/objectcache/containerprofilecache/containerprofilecache.go (1)
417-450: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winVerify the
ug-<workload>overlays too.The new tamper gate only covers label-referenced
userAP/userNN.userManagedAP/userManagedNNfetched above still flow intoprojectUserProfilesat Lines 493-497 without R1016 emission or strict-mode dropping, so signed user-managed overlays can bypass tamper detection.Proposed fix
if ugNNErr != nil { if shouldLogOptionalUserManagedFetchError(ugNNErr) { logger.L().Debug("failed to fetch user-managed NetworkNeighborhood", helpers.String("containerID", containerID), helpers.String("namespace", ns), helpers.String("name", ugNNName), helpers.Error(ugNNErr)) } userManagedNN = nil } + if userManagedAP != nil { + if !c.verifyUserApplicationProfile(userManagedAP, sharedData.Wlid) { + userManagedAP = nil + } + } + if userManagedNN != nil { + if !c.verifyUserNetworkNeighborhood(userManagedNN, sharedData.Wlid) { + userManagedNN = nil + } + } }🤖 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/objectcache/containerprofilecache/containerprofilecache.go` around lines 417 - 450, The tamper-check logic in containerprofilecache should also cover the ug-<workload> overlays, not just the label-referenced userAP and userNN paths. Update the overlay-loading flow around verifyUserApplicationProfile and verifyUserNetworkNeighborhood so the userManagedAP and userManagedNN values are verified with the same signature/tamper gate before projectUserProfiles consumes them, and drop the managed overlays in strict mode when verification fails. Keep the existing permissive behavior for unsigned overlays, but ensure signed user-managed overlays cannot bypass R1016 emission or silently merge into the cache.
🧹 Nitpick comments (3)
pkg/objectcache/containerprofilecache/tamper_alert_test.go (1)
53-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese tests don't exercise the cache's classification path.
This block only proves
errors.Isandsync.Mapsemantics locally, so it will keep passing even ifverifyUserApplicationProfilestops consultingErrSignatureMismatchor the dedup wiring regresses. I'd replace it with a seam-injected verifier test or fold the intent into the end-to-end cases below.🤖 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/objectcache/containerprofilecache/tamper_alert_test.go` around lines 53 - 102, The new tests only validate local errors.Is and sync.Map behavior, so they do not cover the real classification flow in ContainerProfileCacheImpl. Replace these with a seam-injected test that drives verifyUserApplicationProfile (or the existing end-to-end tamper cases) and assert that signature.ErrSignatureMismatch reaches the tamper dedup path while operational errors do not. Use the existing tamperEmitted and emitTamperAlert-related flow to verify the cache’s actual routing instead of standalone fixtures.pkg/objectcache/containerprofilecache/tamper_alert.go (1)
32-38: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftPrune stale resource-version entries from
tamperEmitted.The dedup key includes
ResourceVersion, but the cleanup path only deletes the current key on a clean verify. Every tampered edit at a new RV leaves the old entry behind forever, so a frequently updated signed overlay can grow this map monotonically for the lifetime of the agent.Also applies to: 77-79, 105-108, 122-146
🤖 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/objectcache/containerprofilecache/tamper_alert.go` around lines 32 - 38, The tamper dedup map is leaking stale entries because `tamperKey` includes `ResourceVersion`, but the cleanup path only removes the current key after a clean verify. Update the `tamperEmitted` handling in `verify`, `recordTamper`, and the clean-up logic to prune old keys for the same profile identity (kind/namespace/name) when a clean reconcile succeeds, while still preserving per-RV re-flagging for new tampered edits.pkg/signature/verify_test.go (1)
84-87: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert
ErrSignatureMismatchhere, not just “any error”.This test currently passes on verifier setup failures too. The tamper path depends on
errors.Is(err, ErrSignatureMismatch), so this case should lock that contract down explicitly.Suggested change
import ( + "errors" "io" "os" "strings" "testing" @@ err = VerifyObjectStrict(profile) if err == nil { t.Error("Expected verification failure for tampered profile, got success") + } else if !errors.Is(err, ErrSignatureMismatch) { + t.Fatalf("expected ErrSignatureMismatch, got: %v", err) } }🤖 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/signature/verify_test.go` around lines 84 - 87, The tampered-profile check in VerifyObjectStrict only asserts that some error occurred, so it can pass on unrelated setup failures. Update the test to explicitly verify the returned error matches ErrSignatureMismatch using errors.Is, keeping the existing VerifyObjectStrict path and the tamper case focused on the signature-mismatch contract.
🤖 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 `@cmd/main.go`:
- Around line 544-550: The best-effort pod lookup in resolveOwnContainerID can
block startup because it uses the unbounded context from main for the Kubernetes
GET call. Update resolveOwnContainerID to use a bounded context for the pod
lookup, such as a timeout or deadline around
k8sClient.GetKubernetesClient().CoreV1().Pods(namespace).Get, so startup cannot
hang if the API server or network is slow. Keep the change localized to
resolveOwnContainerID and preserve the existing fallback behavior of returning
an empty container ID on lookup failure.
In `@cmd/sign-object/Dockerfile`:
- Around line 17-20: The runtime image for sign-object still starts as root, so
update the final stage in the Dockerfile to run the copied sign-object binary as
a non-root user. Use the existing builder/runtime stages and the ENTRYPOINT
setup as the place to switch users, and make sure the chosen user can still
access /work and execute /usr/local/bin/sign-object.
In `@cmd/sign-object/main.go`:
- Line 549: The help text in the sign-object command still points to the removed
signing README, so update the reference in the sign-object help string to the
new profile signing doc. Locate the help/documentation text near the sign-object
command setup in main.go and replace the docs/signing/README.md reference with
docs/features/profile-signing-and-tamper-detection.md so --help points to the
correct document.
In `@pkg/objectcache/containerprofilecache/containerprofilecache.go`:
- Around line 699-710: The retry budget comment in
ContainerProfileCacheImpl.NotifyContainerCompleted does not match the actual
loop behavior, since the goroutine currently retries 20 times while the comment
says 5 attempts × 3 s. Update the comment to reflect the real retry count or,
preferably, extract the retry count and delay into named constants used by the
loop so the documented budget stays consistent with the implementation.
In `@pkg/objectcache/containerprofilecache/tamper_alert_test.go`:
- Around line 256-282: The test currently never enters the operational-error
path because the ApplicationProfile is unsigned and verifyUserApplicationProfile
exits early at the IsSigned check. Update
TestVerifyAP_OperationalError_DoesNotEmit to use a signed profile with malformed
signature data, or otherwise inject a verifier/decode/hash error, so the failure
flows through the non-tamper verification branch and still asserts that
captureExporter receives no R1016 alerts.
In `@pkg/signature/cosign_adapter.go`:
- Around line 402-458: The strict verification path in cosign_adapter.go is
trusting mutable annotation data from sig.Timestamp and sig.RekorBundle, which
can be rewritten by a client. Update the verification flow around the
certificate checks in the signature verification logic to derive signing time
from trusted Rekor/CT evidence instead of using sig.Timestamp, or fail
closed/use current time until that evidence is actually validated. Also ensure
the Rekor bundle handling does not merely accept any non-empty sig.RekorBundle;
validate it before treating it as proof, and keep the identity and certificate
checks in the same strict path.
- Around line 541-551: The signature parsing in cosign_adapter.go should fail
fast instead of falling back to raw bytes when base64 decoding
AnnotationSignature fails. Update the annotation handling in the signature
decode path around the signatureB64/base64.StdEncoding.DecodeString logic so
malformed signatures return an error immediately, preserving the
ErrSignatureMismatch contract for actual content mismatches only. Use the
existing signature decode flow in the function that reads annotations and keep
the error path explicit for invalid AnnotationSignature values.
In `@tests/resources/aplint_test.go`:
- Around line 243-249: The YAML validation in the fixture walk is happening
before the template-fixture skip, so templated files still fail in
yaml.Unmarshal and never reach the later R-AP-00/templatePlaceholderRe
exclusion. Update the logic in aplint_test.go around the directory scan to
detect and skip template placeholders before calling yaml.Unmarshal, and apply
the same ordering to the other affected validation block so only concrete
fixtures are parsed.
In `@tests/resources/known-application-profile.yaml`:
- Around line 42-43: The fixture entry for the setgroups path is missing the
procfs prefix and should be corrected in the known-application profile data.
Update the path value in the procfs fixture so it matches the surrounding
entries under the same recorded profile shape, using the existing setgroups
record as the target to locate it.
---
Outside diff comments:
In `@pkg/objectcache/containerprofilecache/containerprofilecache.go`:
- Around line 417-450: The tamper-check logic in containerprofilecache should
also cover the ug-<workload> overlays, not just the label-referenced userAP and
userNN paths. Update the overlay-loading flow around
verifyUserApplicationProfile and verifyUserNetworkNeighborhood so the
userManagedAP and userManagedNN values are verified with the same
signature/tamper gate before projectUserProfiles consumes them, and drop the
managed overlays in strict mode when verification fails. Keep the existing
permissive behavior for unsigned overlays, but ensure signed user-managed
overlays cannot bypass R1016 emission or silently merge into the cache.
---
Nitpick comments:
In `@pkg/objectcache/containerprofilecache/tamper_alert_test.go`:
- Around line 53-102: The new tests only validate local errors.Is and sync.Map
behavior, so they do not cover the real classification flow in
ContainerProfileCacheImpl. Replace these with a seam-injected test that drives
verifyUserApplicationProfile (or the existing end-to-end tamper cases) and
assert that signature.ErrSignatureMismatch reaches the tamper dedup path while
operational errors do not. Use the existing tamperEmitted and
emitTamperAlert-related flow to verify the cache’s actual routing instead of
standalone fixtures.
In `@pkg/objectcache/containerprofilecache/tamper_alert.go`:
- Around line 32-38: The tamper dedup map is leaking stale entries because
`tamperKey` includes `ResourceVersion`, but the cleanup path only removes the
current key after a clean verify. Update the `tamperEmitted` handling in
`verify`, `recordTamper`, and the clean-up logic to prune old keys for the same
profile identity (kind/namespace/name) when a clean reconcile succeeds, while
still preserving per-RV re-flagging for new tampered edits.
In `@pkg/signature/verify_test.go`:
- Around line 84-87: The tampered-profile check in VerifyObjectStrict only
asserts that some error occurred, so it can pass on unrelated setup failures.
Update the test to explicitly verify the returned error matches
ErrSignatureMismatch using errors.Is, keeping the existing VerifyObjectStrict
path and the tamper case focused on the signature-mismatch contract.
🪄 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: ef38baee-92ab-4905-8bc9-2558f657b460
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (42)
cmd/main.gocmd/sign-object/Dockerfilecmd/sign-object/main.godocs/features/profile-signing-and-tamper-detection.mdgo.modpkg/config/config.gopkg/containerprofilemanager/v1/lifecycle.gopkg/exporters/alert_manager.gopkg/objectcache/containerprofilecache/containerprofilecache.gopkg/objectcache/containerprofilecache/projection.gopkg/objectcache/containerprofilecache/tamper_alert.gopkg/objectcache/containerprofilecache/tamper_alert_test.gopkg/objectcache/containerprofilecache/test32_projection_test.gopkg/objectcache/shared_container_data.gopkg/rulemanager/cel/libraries/applicationprofile/ap.gopkg/rulemanager/cel/libraries/cache/function_cache.gopkg/rulemanager/ruleswatcher/watcher.gopkg/rulemanager/ruleswatcher/watcher_signature_test.gopkg/signature/annotations.gopkg/signature/cluster_flow_test.gopkg/signature/cluster_scenario_test.gopkg/signature/cosign_adapter.gopkg/signature/cosign_adapter_test.gopkg/signature/interface.gopkg/signature/profiles/adapter_test.gopkg/signature/profiles/applicationprofile_adapter.gopkg/signature/profiles/empty_typemeta_test.gopkg/signature/profiles/networkneighborhood_adapter.gopkg/signature/profiles/networkneighborhood_adapter_test.gopkg/signature/profiles/rules_adapter.gopkg/signature/profiles/rules_adapter_test.gopkg/signature/profiles/seccompprofile_adapter.gopkg/signature/sign.gopkg/signature/sign_test.gopkg/signature/signer.gopkg/signature/verifier.gopkg/signature/verify.gopkg/signature/verify_test.gotests/component_test.gotests/resources/aplint_test.gotests/resources/curl-signed-deployment.yamltests/resources/known-application-profile.yaml
💤 Files with no reviewable changes (1)
- tests/component_test.go
✅ Files skipped from review due to trivial changes (2)
- pkg/rulemanager/cel/libraries/applicationprofile/ap.go
- pkg/containerprofilemanager/v1/lifecycle.go
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/rulemanager/cel/libraries/cache/function_cache.go
- pkg/exporters/alert_manager.go
- pkg/config/config.go
- pkg/objectcache/shared_container_data.go
- pkg/objectcache/containerprofilecache/projection.go
- pkg/rulemanager/ruleswatcher/watcher.go
🛑 Comments failed to post (9)
cmd/main.go (1)
544-550: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the best-effort pod lookup.
Line 550 uses
main’s background context for a Kubernetes API call during startup. If the API server or network stalls, this best-effort cgroup metric lookup can block node-agent startup.Proposed fix
func resolveOwnContainerID(ctx context.Context, k8sClient *k8sinterface.KubernetesApi) string { const containerName = "node-agent" podName, namespace := os.Getenv("POD_NAME"), os.Getenv("NAMESPACE_NAME") if podName == "" || namespace == "" { return "" } - pod, err := k8sClient.GetKubernetesClient().CoreV1().Pods(namespace).Get(ctx, podName, metav1.GetOptions{}) + lookupCtx, cancel := context.WithTimeout(ctx, 3*time.Second) + defer cancel() + pod, err := k8sClient.GetKubernetesClient().CoreV1().Pods(namespace).Get(lookupCtx, podName, metav1.GetOptions{})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func resolveOwnContainerID(ctx context.Context, k8sClient *k8sinterface.KubernetesApi) string { const containerName = "node-agent" podName, namespace := os.Getenv("POD_NAME"), os.Getenv("NAMESPACE_NAME") if podName == "" || namespace == "" { return "" } lookupCtx, cancel := context.WithTimeout(ctx, 3*time.Second) defer cancel() pod, err := k8sClient.GetKubernetesClient().CoreV1().Pods(namespace).Get(lookupCtx, podName, metav1.GetOptions{})🤖 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 `@cmd/main.go` around lines 544 - 550, The best-effort pod lookup in resolveOwnContainerID can block startup because it uses the unbounded context from main for the Kubernetes GET call. Update resolveOwnContainerID to use a bounded context for the pod lookup, such as a timeout or deadline around k8sClient.GetKubernetesClient().CoreV1().Pods(namespace).Get, so startup cannot hang if the API server or network is slow. Keep the change localized to resolveOwnContainerID and preserve the existing fallback behavior of returning an empty container ID on lookup failure.cmd/sign-object/Dockerfile (1)
17-20: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Drop root in the runtime image.
The final image still runs as root, which expands the blast radius of a compromised CLI container and will create root-owned outputs on mounted workdirs. Please switch to a non-root runtime user here.
Suggested change
FROM gcr.io/distroless/static-debian13:latest COPY --from=builder /sign-object /usr/local/bin/sign-object WORKDIR /work +USER 65532:65532 ENTRYPOINT ["sign-object"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.FROM gcr.io/distroless/static-debian13:latest COPY --from=builder /sign-object /usr/local/bin/sign-object WORKDIR /work USER 65532:65532 ENTRYPOINT ["sign-object"]🤖 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 `@cmd/sign-object/Dockerfile` around lines 17 - 20, The runtime image for sign-object still starts as root, so update the final stage in the Dockerfile to run the copied sign-object binary as a non-root user. Use the existing builder/runtime stages and the ENTRYPOINT setup as the place to switch users, and make sure the chosen user can still access /work and execute /usr/local/bin/sign-object.Source: Linters/SAST tools
cmd/sign-object/main.go (1)
549-549: 📐 Maintainability & Code Quality | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash fd -p 'README.md|profile-signing-and-tamper-detection.md' docs rg -n 'For more information, see:' cmd/sign-object/main.goRepository: kubescape/node-agent
Length of output: 268
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n-- docs/signing files --\n' git ls-files 'docs/signing/*' 'docs/features/*' | sort printf '\n-- specific path existence --\n' for p in docs/signing/README.md docs/features/profile-signing-and-tamper-detection.md; do if [ -e "$p" ]; then printf '%s: exists\n' "$p" else printf '%s: missing\n' "$p" fi done printf '\n-- nearby help text --\n' sed -n '530,555p' cmd/sign-object/main.goRepository: kubescape/node-agent
Length of output: 1136
Point the help text at the new doc
sign-object --helpstill referencesdocs/signing/README.md, which no longer exists. Update it todocs/features/profile-signing-and-tamper-detection.md.🤖 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 `@cmd/sign-object/main.go` at line 549, The help text in the sign-object command still points to the removed signing README, so update the reference in the sign-object help string to the new profile signing doc. Locate the help/documentation text near the sign-object command setup in main.go and replace the docs/signing/README.md reference with docs/features/profile-signing-and-tamper-detection.md so --help points to the correct document.pkg/objectcache/containerprofilecache/containerprofilecache.go (1)
699-710: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the retry budget comment with the loop.
The comment says “5 attempts × 3 s”, but Line 710 runs 20 attempts. Please update the comment or extract the value to a named constant so the retry behavior stays clear.
Proposed fix
// NotifyContainerCompleted is called by containerprofilemanager when it writes a // CP with status="completed". If the container is still pending it launches a -// bounded retry goroutine (up to 5 attempts × 3 s) so the cache entry is +// bounded retry goroutine (up to 20 attempts × 3 s) so the cache entry is // promoted within seconds of the consolidation cycle completing, without waiting // for the next 30 s reconciler tick. func (c *ContainerProfileCacheImpl) NotifyContainerCompleted(containerID string) { p, pending := c.pending.Load(containerID) if !pending { return } + const maxCompletionPromotionAttempts = 20 go func() { - for i := 0; i < 20; i++ { + for i := 0; i < maxCompletionPromotionAttempts; i++ {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// NotifyContainerCompleted is called by containerprofilemanager when it writes a // CP with status="completed". If the container is still pending it launches a // bounded retry goroutine (up to 20 attempts × 3 s) so the cache entry is // promoted within seconds of the consolidation cycle completing, without waiting // for the next 30 s reconciler tick. func (c *ContainerProfileCacheImpl) NotifyContainerCompleted(containerID string) { p, pending := c.pending.Load(containerID) if !pending { return } const maxCompletionPromotionAttempts = 20 go func() { for i := 0; i < maxCompletionPromotionAttempts; i++ {🤖 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/objectcache/containerprofilecache/containerprofilecache.go` around lines 699 - 710, The retry budget comment in ContainerProfileCacheImpl.NotifyContainerCompleted does not match the actual loop behavior, since the goroutine currently retries 20 times while the comment says 5 attempts × 3 s. Update the comment to reflect the real retry count or, preferably, extract the retry count and delay into named constants used by the loop so the documented budget stays consistent with the implementation.pkg/objectcache/containerprofilecache/tamper_alert_test.go (1)
256-282: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
This never reaches the operational-error branch.
The profile is completely unsigned, so
verifyUserApplicationProfilereturns at!signature.IsSigned(adapter)and never exercises the non-tamper verification failure path the test name/comment describe. Use a signed object with malformed signature material, or inject a verifier error, so this actually pins “operational failure != R1016”.🤖 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/objectcache/containerprofilecache/tamper_alert_test.go` around lines 256 - 282, The test currently never enters the operational-error path because the ApplicationProfile is unsigned and verifyUserApplicationProfile exits early at the IsSigned check. Update TestVerifyAP_OperationalError_DoesNotEmit to use a signed profile with malformed signature data, or otherwise inject a verifier/decode/hash error, so the failure flows through the non-tamper verification branch and still asserts that captureExporter receives no R1016 alerts.pkg/signature/cosign_adapter.go (2)
402-458: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Don't treat unsigned annotations as trusted proof in strict mode.
Lines 402-415 use
sig.Timestampas the certificate verification time, and Lines 449-458 accept any non-emptysig.RekorBundlewithout validating it. Both values come from mutable annotations, so a client can rewrite them and still satisfy the "strict" path with the same Fulcio cert. Strict verification needs to derive signing time from verified Rekor/CT evidence; until that exists, fail closed or validate against current time instead of annotation data.🤖 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/signature/cosign_adapter.go` around lines 402 - 458, The strict verification path in cosign_adapter.go is trusting mutable annotation data from sig.Timestamp and sig.RekorBundle, which can be rewritten by a client. Update the verification flow around the certificate checks in the signature verification logic to derive signing time from trusted Rekor/CT evidence instead of using sig.Timestamp, or fail closed/use current time until that evidence is actually validated. Also ensure the Rekor bundle handling does not merely accept any non-empty sig.RekorBundle; validate it before treating it as proof, and keep the identity and certificate checks in the same strict path.
541-551: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fail fast on a malformed signature annotation.
EncodeSignatureToAnnotationsalways base64-encodesAnnotationSignature, so the raw fallback here only turns corrupt input into a downstream signature mismatch. That collapses “malformed annotation” and “tampered content” into the same path, which breaks theErrSignatureMismatchcontract.Suggested fix
signatureB64, ok := annotations[AnnotationSignature] if !ok { return nil, fmt.Errorf("missing %s annotation", AnnotationSignature) } var err error sig.Signature, err = base64.StdEncoding.DecodeString(signatureB64) if err != nil { - // Try raw if base64 fails - sig.Signature = []byte(signatureB64) + return nil, fmt.Errorf("invalid %s annotation: %w", AnnotationSignature, err) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.signatureB64, ok := annotations[AnnotationSignature] if !ok { return nil, fmt.Errorf("missing %s annotation", AnnotationSignature) } var err error sig.Signature, err = base64.StdEncoding.DecodeString(signatureB64) if err != nil { return nil, fmt.Errorf("invalid %s annotation: %w", AnnotationSignature, err) }🤖 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/signature/cosign_adapter.go` around lines 541 - 551, The signature parsing in cosign_adapter.go should fail fast instead of falling back to raw bytes when base64 decoding AnnotationSignature fails. Update the annotation handling in the signature decode path around the signatureB64/base64.StdEncoding.DecodeString logic so malformed signatures return an error immediately, preserving the ErrSignatureMismatch contract for actual content mismatches only. Use the existing signature decode flow in the function that reads annotations and keep the error path explicit for invalid AnnotationSignature values.tests/resources/aplint_test.go (1)
243-249: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Template-fixture skipping happens too late.
yaml.Unmarshal(data, &head)validates the entire document, so any unsubstituted placeholder that makes the YAML non-concrete will hitt.Fatalfhere before the laterR-AP-00 + templatePlaceholderReskip can run. That means the directory walk still breaks on the templated fixtures this test says it should ignore.Also applies to: 263-265
🤖 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/aplint_test.go` around lines 243 - 249, The YAML validation in the fixture walk is happening before the template-fixture skip, so templated files still fail in yaml.Unmarshal and never reach the later R-AP-00/templatePlaceholderRe exclusion. Update the logic in aplint_test.go around the directory scan to detect and skip template placeholders before calling yaml.Unmarshal, and apply the same ordering to the other affected validation block so only concrete fixtures are parsed.tests/resources/known-application-profile.yaml (1)
42-43: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restore the missing
/procprefix on this fixture path.
/7/setgroupsis inconsistent with the surrounding procfs entries and does not look like a real recorded path shape. Keeping the typo in the canonical fixture bakes bad data into the linter’s “gold standard”.🤖 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/known-application-profile.yaml` around lines 42 - 43, The fixture entry for the setgroups path is missing the procfs prefix and should be corrected in the known-application profile data. Update the path value in the procfs fixture so it matches the surrounding entries under the same recorded profile shape, using the existing setgroups record as the target to locate it.
matthyx
pushed a commit
to k8sstormcenter/node-agent
that referenced
this pull request
Jun 26, 2026
…naged lifecycle + signing/tamper detection Squashed and rebased onto main (resolving conflicts against the merged kubescape#806 network-wildcards and kubescape#807 exec-args work). Adds, on top of main's projection surface: - Cryptographic signing/verification of ApplicationProfile, NetworkNeighborhood, seccomp and rules profiles (pkg/signature, cosign-backed, key-based + keyless). - Tamper detection for signed user overlays: re-verifies on every ContainerProfileCache load and emits R1016 "Signed profile tampered". - Rule-signature verification in ruleswatcher. - sign-object CLI (sign / verify / generate-keypair / extract-signature). - User-defined NetworkNeighborhood overlay support + SyncChecksum overlay-identity stamping. - enableSignatureVerification config flag. Conflict resolution: kept main's tested projection design (projectField isPathSurface, ExecsByPath [][]string) and rule-binding notificationQueue; the PR's redundant reimplementation of that surface was dropped in favour of the merged kubescape#806/kubescape#807 versions. Only the net-new features above were grafted on top. go.mod: pins the official kubescape/storage v0.0.291 (carries storage kubescape#325), dropping the temporary k8sstormcenter/storage fork replace; adds sigstore (fulcio, rekor) dependencies for signing. Feature documented in docs/features/profile-signing-and-tamper-detection.md. Syncs R1016 "Signed profile tampered" into the bundled default-rules.yaml from kubescape/rulelibrary (metadata-only entry, empty ruleExpression since R1016 is code-emitted, not CEL-evaluated). Squashed from PR kubescape#808 (kubescape/node-agent). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Duck <70207455+entlein@users.noreply.github.com>
matthyx
force-pushed
the
upstream-pr/sbob-crd-compaction
branch
from
432fef2 to
e643967
Compare
matthyx
pushed a commit
to k8sstormcenter/node-agent
that referenced
this pull request
Jun 26, 2026
…naged lifecycle + signing/tamper detection Squashed and rebased onto main (resolving conflicts against the merged kubescape#806 network-wildcards and kubescape#807 exec-args work). Adds, on top of main's projection surface: - Cryptographic signing/verification of ApplicationProfile, NetworkNeighborhood, seccomp and rules profiles (pkg/signature, cosign-backed, key-based + keyless). - Tamper detection for signed user overlays: re-verifies on every ContainerProfileCache load and emits R1016 "Signed profile tampered". - Rule-signature verification in ruleswatcher. - sign-object CLI (sign / verify / generate-keypair / extract-signature). - User-defined NetworkNeighborhood overlay support + SyncChecksum overlay-identity stamping. - enableSignatureVerification config flag. Conflict resolution: kept main's tested projection design (projectField isPathSurface, ExecsByPath [][]string) and rule-binding notificationQueue; the PR's redundant reimplementation of that surface was dropped in favour of the merged kubescape#806/kubescape#807 versions. Only the net-new features above were grafted on top. go.mod: pins the official kubescape/storage v0.0.291 (carries storage kubescape#325), dropping the temporary k8sstormcenter/storage fork replace; adds sigstore (fulcio, rekor) dependencies for signing. Feature documented in docs/features/profile-signing-and-tamper-detection.md. Syncs R1016 "Signed profile tampered" into the bundled default-rules.yaml from kubescape/rulelibrary (metadata-only entry, empty ruleExpression since R1016 is code-emitted, not CEL-evaluated). Squashed from PR kubescape#808 (kubescape/node-agent). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Duck <70207455+entlein@users.noreply.github.com>
matthyx
force-pushed
the
upstream-pr/sbob-crd-compaction
branch
from
e643967 to
a460877
Compare
matthyx
pushed a commit
to k8sstormcenter/node-agent
that referenced
this pull request
Jun 26, 2026
…naged lifecycle + signing/tamper detection Squashed and rebased onto main (resolving conflicts against the merged kubescape#806 network-wildcards and kubescape#807 exec-args work). Adds, on top of main's projection surface: - Cryptographic signing/verification of ApplicationProfile, NetworkNeighborhood, seccomp and rules profiles (pkg/signature, cosign-backed, key-based + keyless). - Tamper detection for signed user overlays: re-verifies on every ContainerProfileCache load and emits R1016 "Signed profile tampered". - Rule-signature verification in ruleswatcher. - sign-object CLI (sign / verify / generate-keypair / extract-signature). - User-defined NetworkNeighborhood overlay support + SyncChecksum overlay-identity stamping. - enableSignatureVerification config flag. Conflict resolution: kept main's tested projection design (projectField isPathSurface, ExecsByPath [][]string) and rule-binding notificationQueue; the PR's redundant reimplementation of that surface was dropped in favour of the merged kubescape#806/kubescape#807 versions. Only the net-new features above were grafted on top. go.mod: pins the official kubescape/storage v0.0.291 (carries storage kubescape#325), dropping the temporary k8sstormcenter/storage fork replace; adds sigstore (fulcio, rekor) dependencies for signing. Feature documented in docs/features/profile-signing-and-tamper-detection.md. Syncs R1016 "Signed profile tampered" into the bundled default-rules.yaml from kubescape/rulelibrary (metadata-only entry, empty ruleExpression since R1016 is code-emitted, not CEL-evaluated). Squashed from PR kubescape#808 (kubescape/node-agent). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Duck <70207455+entlein@users.noreply.github.com>
matthyx
force-pushed
the
upstream-pr/sbob-crd-compaction
branch
from
a460877 to
eb14aa8
Compare
…naged lifecycle + signing/tamper detection Squashed and rebased onto main (resolving conflicts against the merged kubescape#806 network-wildcards and kubescape#807 exec-args work). Adds, on top of main's projection surface: - Cryptographic signing/verification of ApplicationProfile, NetworkNeighborhood, seccomp and rules profiles (pkg/signature, cosign-backed, key-based + keyless). - Tamper detection for signed user overlays: re-verifies on every ContainerProfileCache load and emits R1016 "Signed profile tampered". - Rule-signature verification in ruleswatcher. - sign-object CLI (sign / verify / generate-keypair / extract-signature). - User-defined NetworkNeighborhood overlay support + SyncChecksum overlay-identity stamping. - enableSignatureVerification config flag. Conflict resolution: kept main's tested projection design (projectField isPathSurface, ExecsByPath [][]string) and rule-binding notificationQueue; the PR's redundant reimplementation of that surface was dropped in favour of the merged kubescape#806/kubescape#807 versions. Only the net-new features above were grafted on top. go.mod: pins the official kubescape/storage v0.0.291 (carries storage kubescape#325), dropping the temporary k8sstormcenter/storage fork replace; adds sigstore (fulcio, rekor) dependencies for signing. Feature documented in docs/features/profile-signing-and-tamper-detection.md. Syncs R1016 "Signed profile tampered" into the bundled default-rules.yaml from kubescape/rulelibrary (metadata-only entry, empty ruleExpression since R1016 is code-emitted, not CEL-evaluated). Squashed from PR kubescape#808 (kubescape/node-agent). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Duck <70207455+entlein@users.noreply.github.com>
matthyx
force-pushed
the
upstream-pr/sbob-crd-compaction
branch
from
eb14aa8 to
ea2d117
Compare
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
2 participants
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.
here things got confusing during the rebase, this is the sister PR to storage and need the signature PR (from matthyx)
Summary by CodeRabbit
New Features
Bug Fixes
Documentation