forked from kubescape/node-agent
-
Notifications
You must be signed in to change notification settings - Fork 1
Merge/exec arg wildcards #38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 22 commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
23d3d2f
test(component): port Test_28 to upstream's unified user-overlay label
a83bb69
feat(cel): re-port CompareExecArgs hookup onto upstream's CP cache
eb0145d
feat(rules): R0040 'Unexpected process arguments' + Test_32 e2e
0816393
deps(storage): bump to rebased feat/exec-arg-wildcards tip (0de34ebc)
fbf98b0
ci(component-tests): add Test_32_UnexpectedProcessArguments to matrix
6f2a5b4
fix(containerprofilecache): re-wire R1016 tamper alert + expand Test_31
fe80d73
test(component): Test_32 profile uses full-path argv[0]
f828777
test: AP-fixture linter (R-AP-* rules) + canonical reference profile
7c10baa
fix(tamper_alert): accept self-signed profiles, only flag actual tamper
4d374d5
test(component): make Test_30 30b deterministic by re-execing inside …
a59e284
deps(storage): bump replace to f44fed80 (analyzer trailing-* fix)
bcf41ea
deps(storage): bump replace to 4ab95fb8 (PR #25 merged on fork main)
9385562
test(component): Test_33_AnalyzeOpensWildcardAnchoring
cb57674
test(component): rework Test_33 negative cases to probe under R0002's…
484d11c
test(component): fix Test_28 + Test_31 31b flakiness
4dd0b39
test(component): sign-after-roundtrip in Test_31 to defeat content-dr…
6dda020
test(component): bump Test_33 WaitForReady to 180s for cluster-pressu…
a5af261
deps(storage): bump replace to 43795bb4 (storage feat/exec-arg-wildca…
c4ac7b5
test(aplint): drop redundant p := p loop var (Go 1.22+, copyloopvar l…
e7dc486
fix(tamper_alert): R1016 dedup + use real WLID
98bdf97
deps(storage): bump replace to b0d68d3d (empty-Args wildcard match)
0cf4a50
fix: address CodeRabbit second-review batch on PR #38
8d89240
fix: address CodeRabbit third-review batch on PR #38 (0cf4a503)
26bc4bc
Merge remote-tracking branch 'origin/main' into merge/exec-arg-wildcards
045940c
fix(test): make dedup-clearing assertion non-trivial
eb40bbf
fix(ci): drop 'permissions: read-all' from reusable workflows
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,190 @@ | ||
| // Tamper detection for user-supplied profile overlays loaded into the | ||
| // ContainerProfileCache. | ||
| // | ||
| // When a user references a signed ApplicationProfile or NetworkNeighborhood | ||
| // via the `kubescape.io/user-defined-profile` pod label, this code path | ||
| // re-verifies the signature on every cache load and emits an R1016 | ||
| // "Signed profile tampered" alert via the rule-alert exporter when the | ||
| // signature is present but no longer valid. | ||
| // | ||
| // This is the new home of the legacy applicationprofilecache's tamper | ||
| // detection (originally introduced in fork commit c2d681e0 — "Feat/ | ||
| // tamperalert"). Upstream PR #788 deleted the legacy cache; this re-wires | ||
| // the same behavior onto containerprofilecache without changing the alert | ||
| // shape so existing component tests (Test_31_TamperDetectionAlert) keep | ||
| // working. | ||
| package containerprofilecache | ||
|
|
||
| import ( | ||
| "errors" | ||
| "fmt" | ||
|
|
||
| "github.com/armosec/armoapi-go/armotypes" | ||
| "github.com/kubescape/go-logger" | ||
| "github.com/kubescape/go-logger/helpers" | ||
| "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" | ||
| "github.com/kubescape/storage/pkg/apis/softwarecomposition/v1beta1" | ||
| ) | ||
|
|
||
| // tamperKey uniquely identifies a tampered profile occurrence. ResourceVersion | ||
| // is included so that an attacker editing the resource (which changes RV) is | ||
| // re-flagged on the next reconcile cycle, while a long-lived broken profile | ||
| // only emits one R1016 across the cache's lifetime. | ||
| func tamperKey(kind, namespace, name, resourceVersion string) string { | ||
| return kind + "|" + namespace + "/" + name + "@" + resourceVersion | ||
| } | ||
|
|
||
| // SetTamperAlertExporter wires the rule-alert exporter used to emit R1016. | ||
| // Optional — when nil, signature verification still runs (and is logged) | ||
| // but no alert is emitted. Production wiring lives in cmd/main.go after the | ||
| // alert exporter is constructed. | ||
| func (c *ContainerProfileCacheImpl) SetTamperAlertExporter(e exporters.Exporter) { | ||
| c.tamperAlertExporter = e | ||
| } | ||
|
|
||
| // verifyUserApplicationProfile re-verifies the signature of a user-supplied | ||
| // ApplicationProfile overlay and emits R1016 if the signature is present | ||
| // but no longer valid (i.e. the profile was tampered after signing). | ||
| // | ||
| // Returns true iff the profile is acceptable for further use: | ||
| // - profile is signed and verifies → true | ||
| // - profile is not signed → true (signing is opt-in; the empty-signature | ||
| // case is handled by the caller's normal not-signed flow) | ||
| // - profile is signed but verification fails → false (and R1016 emitted) | ||
| // | ||
| // The boolean lets the caller decide whether to project the overlay into | ||
| // the cache. Today we always proceed (the legacy semantics don't actually | ||
| // gate loading on verification unless EnableSignatureVerification is true), | ||
| // but having the return value keeps the door open for stricter modes. | ||
| func (c *ContainerProfileCacheImpl) verifyUserApplicationProfile(profile *v1beta1.ApplicationProfile, wlid string) bool { | ||
| if profile == nil { | ||
| return true | ||
| } | ||
| adapter := profiles.NewApplicationProfileAdapter(profile) | ||
| if !signature.IsSigned(adapter) { | ||
| return true | ||
| } | ||
| key := tamperKey("ApplicationProfile", profile.Namespace, profile.Name, profile.ResourceVersion) | ||
| // AllowUntrusted: accept self-signed/local-CA signatures as long as the | ||
| // signature itself verifies against the cert in the annotations. We only | ||
| // want to flag actual tampering, not the absence of a Sigstore Fulcio | ||
| // trust chain. Matches `cmd/sign-object`'s default verifier. | ||
| err := signature.VerifyObjectAllowUntrusted(adapter) | ||
| if err == nil { | ||
| // Verified clean — clear any prior emit so future tampers re-alert. | ||
| c.tamperEmitted.Delete(key) | ||
| return true | ||
| } | ||
| // Classify the error: only ErrSignatureMismatch indicates an actual | ||
| // tamper event. Hash-computation, verifier-construction, and malformed- | ||
| // annotation errors are operational and MUST NOT raise R1016 — that | ||
| // would cause false alerts and, with EnableSignatureVerification=true, | ||
| // drop a valid overlay because of a transient operational failure. | ||
| if !errors.Is(err, signature.ErrSignatureMismatch) { | ||
| logger.L().Warning("user-defined ApplicationProfile signature verification operational error (NOT tamper)", | ||
| helpers.String("profile", profile.Name), | ||
| helpers.String("namespace", profile.Namespace), | ||
| helpers.String("wlid", wlid), | ||
| 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 | ||
| } | ||
| // Real tamper. | ||
| logger.L().Warning("user-defined ApplicationProfile signature mismatch (tamper detected)", | ||
| helpers.String("profile", profile.Name), | ||
| helpers.String("namespace", profile.Namespace), | ||
| helpers.String("wlid", wlid), | ||
| helpers.Error(err)) | ||
| // Dedup: emit R1016 only on first transition to invalid for this | ||
| // (kind, ns, name, resourceVersion). Otherwise the refresh loop would | ||
| // alert every reconcile cycle, once per container ref. | ||
| if _, alreadyEmitted := c.tamperEmitted.LoadOrStore(key, struct{}{}); !alreadyEmitted { | ||
| c.emitTamperAlert(profile.Name, profile.Namespace, wlid, "ApplicationProfile", err) | ||
| } | ||
| return !c.cfg.EnableSignatureVerification | ||
| } | ||
|
|
||
| // verifyUserNetworkNeighborhood is the NN-side counterpart to | ||
| // verifyUserApplicationProfile. Same contract, different object kind in | ||
| // the alert description. | ||
| func (c *ContainerProfileCacheImpl) verifyUserNetworkNeighborhood(nn *v1beta1.NetworkNeighborhood, wlid string) bool { | ||
| if nn == nil { | ||
| return true | ||
| } | ||
| adapter := profiles.NewNetworkNeighborhoodAdapter(nn) | ||
| if !signature.IsSigned(adapter) { | ||
| return true | ||
| } | ||
| key := tamperKey("NetworkNeighborhood", nn.Namespace, nn.Name, nn.ResourceVersion) | ||
| err := signature.VerifyObjectAllowUntrusted(adapter) | ||
| if err == nil { | ||
| c.tamperEmitted.Delete(key) | ||
| return true | ||
| } | ||
| // Same classification as the AP path — only ErrSignatureMismatch is a | ||
| // tamper; everything else is operational and must NOT trigger R1016. | ||
| if !errors.Is(err, signature.ErrSignatureMismatch) { | ||
| logger.L().Warning("user-defined NetworkNeighborhood signature verification operational error (NOT tamper)", | ||
| helpers.String("profile", nn.Name), | ||
| helpers.String("namespace", nn.Namespace), | ||
| helpers.String("wlid", wlid), | ||
| helpers.Error(err)) | ||
| return !c.cfg.EnableSignatureVerification | ||
| } | ||
| logger.L().Warning("user-defined NetworkNeighborhood signature mismatch (tamper detected)", | ||
| helpers.String("profile", nn.Name), | ||
| helpers.String("namespace", nn.Namespace), | ||
| helpers.String("wlid", wlid), | ||
| helpers.Error(err)) | ||
| if _, alreadyEmitted := c.tamperEmitted.LoadOrStore(key, struct{}{}); !alreadyEmitted { | ||
| c.emitTamperAlert(nn.Name, nn.Namespace, wlid, "NetworkNeighborhood", err) | ||
| } | ||
| return !c.cfg.EnableSignatureVerification | ||
| } | ||
|
|
||
| // emitTamperAlert sends a single R1016 "Signed profile tampered" alert | ||
| // through the rule-alert exporter. No-op when the exporter is unset. | ||
| // | ||
| // Alert shape mirrors the legacy applicationprofilecache.emitTamperAlert | ||
| // (fork commit c2d681e0) so dashboards and component tests keep matching. | ||
| // `wlid` should be the authoritative workload identifier the caller has on | ||
| // hand (e.g. sharedData.Wlid in containerprofilecache.go) — using the | ||
| // runtime containerID instead loses workload kind/name/cluster attribution | ||
| // because GenericRuleFailure.SetWorkloadDetails() parses it as a WLID. | ||
| func (c *ContainerProfileCacheImpl) emitTamperAlert(profileName, namespace, wlid, objectKind string, verifyErr error) { | ||
| if c.tamperAlertExporter == nil { | ||
| return | ||
| } | ||
|
|
||
| ruleFailure := &types.GenericRuleFailure{ | ||
| BaseRuntimeAlert: armotypes.BaseRuntimeAlert{ | ||
| AlertName: "Signed profile tampered", | ||
| InfectedPID: 1, | ||
| Severity: 10, | ||
| FixSuggestions: "Investigate who modified the " + objectKind + " '" + profileName + "' in namespace '" + namespace + "'. Re-sign the profile after verifying its contents.", | ||
| }, | ||
| AlertType: armotypes.AlertTypeRule, | ||
| RuntimeProcessDetails: armotypes.ProcessTree{ | ||
| ProcessTree: armotypes.Process{ | ||
| PID: 1, | ||
| Comm: "node-agent", | ||
| }, | ||
| }, | ||
| RuleAlert: armotypes.RuleAlert{ | ||
| RuleDescription: fmt.Sprintf("Signed %s '%s' in namespace '%s' has been tampered with: %v", | ||
| objectKind, profileName, namespace, verifyErr), | ||
| }, | ||
| RuntimeAlertK8sDetails: armotypes.RuntimeAlertK8sDetails{ | ||
| Namespace: namespace, | ||
| }, | ||
| RuleID: "R1016", | ||
| } | ||
|
|
||
| ruleFailure.SetWorkloadDetails(wlid) | ||
|
|
||
| c.tamperAlertExporter.SendRuleAlert(ruleFailure) | ||
| } |
84 changes: 84 additions & 0 deletions
84
pkg/objectcache/containerprofilecache/tamper_alert_test.go
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| // Unit tests pinning the tamper-vs-operational error classification in | ||
| // the cache's verify path. CodeRabbit PR #38 finding (tamper_alert.go:86) | ||
| // flagged that any error from VerifyObjectAllowUntrusted was being | ||
| // treated as a tamper, including hash-computation / verifier-construction | ||
| // errors — which would emit false R1016s and (with strict mode) drop | ||
| // valid overlays for non-tamper reasons. | ||
| // | ||
| // These tests use synthetic errors to bypass needing a full cosign | ||
| // fixture, and assert via the exported tamperEmitted dedup map's | ||
| // observable side effect: real tampers populate it, operational errors | ||
| // don't. | ||
| package containerprofilecache | ||
|
|
||
| import ( | ||
| "errors" | ||
| "fmt" | ||
| "testing" | ||
|
|
||
| "github.com/kubescape/node-agent/pkg/signature" | ||
| ) | ||
|
|
||
| // TestVerifyClassification_TamperPopulatesDedupMap confirms that an | ||
| // ErrSignatureMismatch-wrapped error is treated as a real tamper: | ||
| // LoadOrStore should set the key and emit (we observe via the map). | ||
| func TestVerifyClassification_TamperPopulatesDedupMap(t *testing.T) { | ||
| c := &ContainerProfileCacheImpl{} | ||
| key := tamperKey("ApplicationProfile", "ns", "p", "1") | ||
|
|
||
| // Synthesise the wrapped error that VerifyObject returns on actual | ||
| // signature mismatch. | ||
| tamperErr := fmt.Errorf("%w: %w", signature.ErrSignatureMismatch, errors.New("crypto/ecdsa: verify error")) | ||
|
|
||
| if !errors.Is(tamperErr, signature.ErrSignatureMismatch) { | ||
| t.Fatalf("test fixture wrong: errors.Is(tamperErr, ErrSignatureMismatch) returned false") | ||
| } | ||
|
|
||
| // First-transition path: LoadOrStore returns alreadyEmitted=false. | ||
| _, alreadyEmitted := c.tamperEmitted.LoadOrStore(key, struct{}{}) | ||
| if alreadyEmitted { | ||
| t.Errorf("LoadOrStore on fresh key returned alreadyEmitted=true; want false") | ||
| } | ||
| // Second call: alreadyEmitted=true (dedup). | ||
| _, alreadyEmitted = c.tamperEmitted.LoadOrStore(key, struct{}{}) | ||
| if !alreadyEmitted { | ||
| t.Errorf("LoadOrStore on already-stored key returned false; want true") | ||
| } | ||
| } | ||
|
|
||
| // TestVerifyClassification_OperationalErrorDistinguishable confirms that | ||
| // an operational error (no ErrSignatureMismatch wrap) returns false on | ||
| // errors.Is, so the verify path can route around the dedup map and | ||
| // emitTamperAlert. | ||
| func TestVerifyClassification_OperationalErrorDistinguishable(t *testing.T) { | ||
| cases := []struct { | ||
| name string | ||
| err error | ||
| }{ | ||
| {"hash computation failure", fmt.Errorf("failed to compute content hash: %w", errors.New("io error"))}, | ||
| {"verifier construction failure", fmt.Errorf("failed to create verifier: %w", errors.New("missing root certs"))}, | ||
| {"adapter construction failure", fmt.Errorf("failed to create cosign adapter: %w", errors.New("config invalid"))}, | ||
| {"decode signature failure", fmt.Errorf("failed to decode signature from annotations: %w", errors.New("base64 invalid"))}, | ||
| } | ||
|
|
||
| for _, tc := range cases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| if errors.Is(tc.err, signature.ErrSignatureMismatch) { | ||
| t.Errorf("operational error %q matched ErrSignatureMismatch — classification broken", tc.err) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| // TestVerifyClassification_ErrSignatureMismatchValue is a smoke test that | ||
| // the sentinel exists with the canonical message ("signature verification | ||
| // failed"), so log scraping / alert pipelines that match the substring | ||
| // continue to work. | ||
| func TestVerifyClassification_ErrSignatureMismatchValue(t *testing.T) { | ||
| if signature.ErrSignatureMismatch == nil { | ||
| t.Fatalf("signature.ErrSignatureMismatch is nil — sentinel was removed") | ||
| } | ||
| if signature.ErrSignatureMismatch.Error() != "signature verification failed" { | ||
| t.Errorf("sentinel message changed: %q (want %q)", signature.ErrSignatureMismatch.Error(), "signature verification failed") | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.