From 7605e2292d46d8fe257befe5208f92d945feb8dc Mon Sep 17 00:00:00 2001 From: Ben Date: Thu, 30 Jul 2026 12:11:35 +0300 Subject: [PATCH 01/17] refactor(rulemanager): embed armotypes.RuntimeRule in typesv1.Rule Gives the CRD contract -- including the new StateWrites clause -- exactly one definition shared with the operator, without retiring typesv1.Rule. Expressions and ProfileDataRequired stay shadowed because their types genuinely differ: utils.EventType covers all node-agent event streams, and FieldRequirement carries a Declared flag plus strict unknown-key rejection that armotypes.ProfileDataField has neither of. The two decoders that reach Rule disagree about the shadows. encoding/json resolves same-tag conflicts by depth, so only the depth-0 fields are populated. apimachinery's converter -- the production CRD path, via DefaultUnstructuredConverter.FromUnstructured -- has no depth rule and visits every field independently, so it fills the embedded copies too. Either way the depth-0 fields are what node-agent code reads. rule_embedding_test.go pins both decoders, including the apimachinery path the plan originally left untested. Docs-exempt: struct-embedding refactor; StateWrites is inert until Task 6 reads it Signed-off-by: Ben --- go.mod | 2 +- go.sum | 4 +- .../projection_compile_test.go | 10 +- .../rulecreator/ruleengine_mock.go | 11 +- .../types/v1/rule_embedding_test.go | 168 ++++++++++++++++++ pkg/rulemanager/types/v1/types.go | 42 +++-- 6 files changed, 210 insertions(+), 27 deletions(-) create mode 100644 pkg/rulemanager/types/v1/rule_embedding_test.go diff --git a/go.mod b/go.mod index 9deb035121..794e99f7c9 100644 --- a/go.mod +++ b/go.mod @@ -7,7 +7,7 @@ require ( github.com/Masterminds/semver/v3 v3.4.0 github.com/anchore/syft v1.42.3 github.com/aquilax/truncate v1.0.0 - github.com/armosec/armoapi-go v0.0.696 + github.com/armosec/armoapi-go v0.0.739 github.com/armosec/utils-k8s-go v0.0.35 github.com/cenkalti/backoff v2.2.1+incompatible github.com/cenkalti/backoff/v4 v4.3.0 diff --git a/go.sum b/go.sum index 110dd98c84..16a286a1ba 100644 --- a/go.sum +++ b/go.sum @@ -203,8 +203,8 @@ github.com/armon/go-radix v0.0.0-20180808171621-7fddfc383310/go.mod h1:ufUuZ+zHj github.com/armon/go-radix v1.0.0/go.mod h1:ufUuZ+zHj4x4TnLV4JWEpy2hxWSpsRywHrMgIH9cCH8= github.com/armon/go-socks5 v0.0.0-20160902184237-e75332964ef5 h1:0CwZNZbxp69SHPdPJAN/hZIm0C4OItdklCFmMRWYpio= github.com/armon/go-socks5 v0.0.0-20160902184237-e75332964ef5/go.mod h1:wHh0iHkYZB8zMSxRWpUBQtwG5a7fFgvEO+odwuTv2gs= -github.com/armosec/armoapi-go v0.0.696 h1:+0Ll7y4oWNaKEO47qbGDFIQLxkSJeKYzylS0FwI84XE= -github.com/armosec/armoapi-go v0.0.696/go.mod h1:9jAH0g8ZsryhiBDd/aNMX4+n10bGwTx/doWCyyjSxts= +github.com/armosec/armoapi-go v0.0.739 h1:kviApEaywGpf4oG9Ok5FSq9kije2aUJsF74YqL92YBk= +github.com/armosec/armoapi-go v0.0.739/go.mod h1:1l+70fBK09F7zI2jArrPUWVHaLkijg+sQutFTmE6HRs= github.com/armosec/gojay v1.2.17 h1:VSkLBQzD1c2V+FMtlGFKqWXNsdNvIKygTKJI9ysY8eM= github.com/armosec/gojay v1.2.17/go.mod h1:vuvX3DlY0nbVrJ0qCklSS733AWMoQboq3cFyuQW9ybc= github.com/armosec/utils-go v0.0.58 h1:g9RnRkxZAmzTfPe2ruMo2OXSYLwVSegQSkSavOfmaIE= diff --git a/pkg/objectcache/containerprofilecache/projection_compile_test.go b/pkg/objectcache/containerprofilecache/projection_compile_test.go index fa73e4c0e8..6eae4d6e35 100644 --- a/pkg/objectcache/containerprofilecache/projection_compile_test.go +++ b/pkg/objectcache/containerprofilecache/projection_compile_test.go @@ -3,6 +3,8 @@ package containerprofilecache import ( "testing" + "github.com/armosec/armoapi-go/armotypes" + "github.com/kubescape/node-agent/pkg/objectcache" typesv1 "github.com/kubescape/node-agent/pkg/rulemanager/types/v1" "github.com/stretchr/testify/assert" @@ -12,7 +14,7 @@ import ( // makeRule is a helper that builds a Rule with a ProfileDataRequired. func makeRule(pdr *typesv1.ProfileDataRequired) typesv1.Rule { return typesv1.Rule{ - ID: "test-rule", + RuntimeRule: armotypes.RuntimeRule{ID: "test-rule"}, ProfileDataRequired: pdr, } } @@ -63,8 +65,8 @@ func TestCompileSpec_Empty(t *testing.T) { // ProfileDataRequired do not contribute to the spec. func TestCompileSpec_NilProfileDataRequiredSkipped(t *testing.T) { rules := []typesv1.Rule{ - {ID: "no-pdr", ProfileDataRequired: nil}, - {ID: "also-no-pdr", ProfileDataRequired: nil}, + {RuntimeRule: armotypes.RuntimeRule{ID: "no-pdr"}, ProfileDataRequired: nil}, + {RuntimeRule: armotypes.RuntimeRule{ID: "also-no-pdr"}, ProfileDataRequired: nil}, } spec := CompileSpec(rules) @@ -90,7 +92,7 @@ func TestCompileSpec_DeterministicHash(t *testing.T) { pdr2 := &typesv1.ProfileDataRequired{ Execs: fieldReqAll(), } - rule2 := typesv1.Rule{ID: "r2", ProfileDataRequired: pdr2} + rule2 := typesv1.Rule{RuntimeRule: armotypes.RuntimeRule{ID: "r2"}, ProfileDataRequired: pdr2} specAB := CompileSpec([]typesv1.Rule{rule, rule2}) specBA := CompileSpec([]typesv1.Rule{rule2, rule}) diff --git a/pkg/rulemanager/rulecreator/ruleengine_mock.go b/pkg/rulemanager/rulecreator/ruleengine_mock.go index a56f82f8b0..47086ea73d 100644 --- a/pkg/rulemanager/rulecreator/ruleengine_mock.go +++ b/pkg/rulemanager/rulecreator/ruleengine_mock.go @@ -1,6 +1,7 @@ package rulecreator import ( + "github.com/armosec/armoapi-go/armotypes" typesv1 "github.com/kubescape/node-agent/pkg/rulemanager/types/v1" "github.com/kubescape/node-agent/pkg/utils" ) @@ -15,8 +16,10 @@ func (r *RuleCreatorMock) CreateRulesByTags(tags []string) []typesv1.Rule { var rl []typesv1.Rule for _, t := range tags { rl = append(rl, typesv1.Rule{ - Name: t, - Tags: []string{t}, + RuntimeRule: armotypes.RuntimeRule{ + Name: t, + Tags: []string{t}, + }, }) } return rl @@ -24,13 +27,13 @@ func (r *RuleCreatorMock) CreateRulesByTags(tags []string) []typesv1.Rule { func (r *RuleCreatorMock) CreateRuleByID(id string) typesv1.Rule { return typesv1.Rule{ - ID: id, + RuntimeRule: armotypes.RuntimeRule{ID: id}, } } func (r *RuleCreatorMock) CreateRuleByName(name string) typesv1.Rule { return typesv1.Rule{ - Name: name, + RuntimeRule: armotypes.RuntimeRule{Name: name}, } } diff --git a/pkg/rulemanager/types/v1/rule_embedding_test.go b/pkg/rulemanager/types/v1/rule_embedding_test.go new file mode 100644 index 0000000000..5b9c0a3300 --- /dev/null +++ b/pkg/rulemanager/types/v1/rule_embedding_test.go @@ -0,0 +1,168 @@ +package types + +import ( + "encoding/json" + "testing" + + "github.com/armosec/armoapi-go/armotypes" + "github.com/kubescape/node-agent/pkg/utils" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + k8sruntime "k8s.io/apimachinery/pkg/runtime" +) + +// A representative existing rule (R1004 shape). This must survive embedding +// completely unchanged -- it is the regression gate. +const existingRuleJSON = `{ + "enabled": true, + "id": "R1004", + "name": "Process executed from mount", + "description": "Detecting exec calls from mounted paths.", + "expressions": { + "message": "'msg'", + "uniqueId": "event.comm", + "ruleExpression": [{"eventType": "exec", "expression": "true"}] + }, + "profileDependency": 1, + "profileDataRequired": {"execs": "all"}, + "severity": 5, + "supportPolicy": false, + "isTriggerAlert": true, + "mitreTactic": "TA0002", + "mitreTechnique": "T1059", + "tags": ["exec", "mount"] +}` + +func TestRule_ExistingFieldsUnchangedAfterEmbedding(t *testing.T) { + var r Rule + require.NoError(t, json.Unmarshal([]byte(existingRuleJSON), &r)) + + // Promoted from the embedded RuntimeRule. + assert.True(t, r.Enabled) + assert.Equal(t, "R1004", r.ID) + assert.Equal(t, "Process executed from mount", r.Name) + assert.Equal(t, 5, r.Severity) + assert.Equal(t, armotypes.ProfileDependency(1), r.ProfileDependency) + assert.Equal(t, []string{"exec", "mount"}, r.Tags) + assert.True(t, r.IsTriggerAlert) + assert.Equal(t, "TA0002", r.MitreTactic) + + // Shadowed: still node-agent's own type, still utils.EventType. + require.Len(t, r.Expressions.RuleExpression, 1) + assert.Equal(t, utils.ExecveEventType, r.Expressions.RuleExpression[0].EventType) + assert.Equal(t, "event.comm", r.Expressions.UniqueID) + + // Shadowed: node-agent's FieldRequirement semantics, including Declared. + require.NotNil(t, r.ProfileDataRequired) + assert.True(t, r.ProfileDataRequired.Execs.All) + assert.True(t, r.ProfileDataRequired.Execs.Declared) + assert.False(t, r.ProfileDataRequired.Opens.Declared, + "an absent surface must stay undeclared -- this is the semantics armotypes lacks") +} + +func TestRule_StateWritesAreReadableViaEmbedding(t *testing.T) { + const withState = `{ + "id": "R1089", + "stateWrites": [{ + "eventType": "exec", + "when": "true", + "scope": "container", + "name": "mount_exec", + "key": "string(event.pid)", + "ttl": "10m" + }], + "expressions": {"message": "'m'", "uniqueId": "'u'", "ruleExpression": []} + }` + + var r Rule + require.NoError(t, json.Unmarshal([]byte(withState), &r)) + + require.Len(t, r.StateWrites, 1) + w := r.StateWrites[0] + assert.Equal(t, armotypes.EventTypeExec, w.EventType) + assert.Equal(t, armotypes.StateScopeContainer, w.Scope) + assert.Equal(t, "mount_exec", w.Name) + assert.Equal(t, "10m", w.TTL) +} + +func TestRule_PrefilterStillExcludedFromSerialization(t *testing.T) { + data, err := json.Marshal(Rule{}) + require.NoError(t, err) + assert.NotContains(t, string(data), "Prefilter") + assert.NotContains(t, string(data), "prefilter") +} + +func TestRule_ShadowedFieldWinsOverEmbedded(t *testing.T) { + // Proves the depth rule for encoding/json: the outer Expressions is + // populated, and the embedded RuntimeRule.Expressions is left at its zero + // value. If this ever inverts, EventType silently becomes + // armotypes.EventType and the rule loop stops matching. + var r Rule + require.NoError(t, json.Unmarshal([]byte(existingRuleJSON), &r)) + assert.Len(t, r.Expressions.RuleExpression, 1) + assert.Empty(t, r.RuntimeRule.Expressions.RuleExpression, + "embedded Expressions must stay unused; the shadow is deliberate") +} + +// unstructuredRule is the production decoding path: rules arrive from the CRD as +// unstructured maps and are converted by apimachinery, NOT by encoding/json. +// Unlike encoding/json, apimachinery has no depth-based conflict resolution -- +// it visits every field of the outer struct independently -- so the two decoders +// genuinely disagree about the shadowed fields. What must hold on BOTH paths is +// that the depth-0 shadow (the one all node-agent code reads) is correct. +func unstructuredRule(t *testing.T, raw string) Rule { + t.Helper() + var m map[string]any + require.NoError(t, json.Unmarshal([]byte(raw), &m)) + + var r Rule + require.NoError(t, k8sruntime.DefaultUnstructuredConverter.FromUnstructured(m, &r)) + return r +} + +func TestRule_DecodesViaApimachineryLikeTheCRDPath(t *testing.T) { + r := unstructuredRule(t, existingRuleJSON) + + assert.True(t, r.Enabled) + assert.Equal(t, "R1004", r.ID) + assert.Equal(t, 5, r.Severity) + assert.Equal(t, armotypes.ProfileDependency(1), r.ProfileDependency) + assert.Equal(t, []string{"exec", "mount"}, r.Tags) + assert.Equal(t, "TA0002", r.MitreTactic) + + // The field the rule loop actually compares against. + require.Len(t, r.Expressions.RuleExpression, 1) + assert.Equal(t, utils.ExecveEventType, r.Expressions.RuleExpression[0].EventType) + assert.Equal(t, "event.comm", r.Expressions.UniqueID) + + require.NotNil(t, r.ProfileDataRequired) + assert.True(t, r.ProfileDataRequired.Execs.All) + assert.True(t, r.ProfileDataRequired.Execs.Declared) + assert.False(t, r.ProfileDataRequired.Opens.Declared) +} + +func TestRule_StateWritesDecodeViaApimachinery(t *testing.T) { + const withState = `{ + "id": "R1089", + "stateWrites": [{ + "eventType": "exec", + "scope": "container", + "name": "mount_exec", + "key": "string(event.pid)", + "value": {"argv": "event.args"}, + "ttl": "10m" + }], + "expressions": {"message": "'m'", "uniqueId": "'u'", "ruleExpression": []} + }` + + r := unstructuredRule(t, withState) + + require.Len(t, r.StateWrites, 1) + w := r.StateWrites[0] + assert.Equal(t, armotypes.EventTypeExec, w.EventType) + assert.Equal(t, armotypes.StateScopeContainer, w.Scope) + assert.Equal(t, "mount_exec", w.Name) + assert.Equal(t, "string(event.pid)", w.Key) + assert.Equal(t, "10m", w.TTL) + assert.Equal(t, map[string]any{"argv": "event.args"}, w.Value) +} diff --git a/pkg/rulemanager/types/v1/types.go b/pkg/rulemanager/types/v1/types.go index 20e387552c..90e31ea2d4 100644 --- a/pkg/rulemanager/types/v1/types.go +++ b/pkg/rulemanager/types/v1/types.go @@ -18,23 +18,33 @@ type RulesSpec struct { Rules []Rule `json:"rules" yaml:"rules"` } +// Rule is node-agent's view of a rule from the Rules CRD. +// +// It embeds armotypes.RuntimeRule so the CRD contract -- including StateWrites -- +// has exactly one definition, shared with the operator. Two fields are +// deliberately SHADOWED because their types differ from the shared root: +// +// - Expressions: node-agent's RuleExpression uses utils.EventType, which covers +// all node-agent event streams and is the type the rule loop compares +// against. The embedded RuntimeRule.Expressions is unused. +// - ProfileDataRequired: node-agent's FieldRequirement carries a Declared flag +// distinguishing "absent" from "present but empty", and rejects unknown keys +// at unmarshal. armotypes.ProfileDataField has neither. +// +// The two decoders that reach this struct treat the shadows differently. +// encoding/json resolves same-tag conflicts by depth, so only these depth-0 +// fields are populated. apimachinery's converter -- the production CRD path -- +// has no depth rule and visits every field independently, so it populates the +// embedded copies as well. Either way the depth-0 fields are what all +// node-agent code reads; never read the embedded copies. rule_embedding_test.go +// pins the behaviour of both decoders. type Rule struct { - Enabled bool `json:"enabled" yaml:"enabled"` - ID string `json:"id" yaml:"id"` - Name string `json:"name" yaml:"name"` - Description string `json:"description" yaml:"description"` - Expressions RuleExpressions `json:"expressions" yaml:"expressions"` - ProfileDependency armotypes.ProfileDependency `json:"profileDependency" yaml:"profileDependency"` - ProfileDataRequired *ProfileDataRequired `json:"profileDataRequired,omitempty" yaml:"profileDataRequired,omitempty"` - Severity int `json:"severity" yaml:"severity"` - SupportPolicy bool `json:"supportPolicy" yaml:"supportPolicy"` - Tags []string `json:"tags" yaml:"tags"` - State map[string]any `json:"state,omitempty" yaml:"state,omitempty"` - AgentVersionRequirement string `json:"agentVersionRequirement" yaml:"agentVersionRequirement"` - IsTriggerAlert bool `json:"isTriggerAlert" yaml:"isTriggerAlert"` - MitreTactic string `json:"mitreTactic" yaml:"mitreTactic"` - MitreTechnique string `json:"mitreTechnique" yaml:"mitreTechnique"` - Prefilter *prefilter.Params `json:"-" yaml:"-"` + armotypes.RuntimeRule `json:",inline" yaml:",inline"` + + Expressions RuleExpressions `json:"expressions" yaml:"expressions"` + ProfileDataRequired *ProfileDataRequired `json:"profileDataRequired,omitempty" yaml:"profileDataRequired,omitempty"` + + Prefilter *prefilter.Params `json:"-" yaml:"-"` } type RuleExpressions struct { From 01b5191c668dc1397927daa0761ed1706294f6bf Mon Sep 17 00:00:00 2001 From: Ben Date: Thu, 30 Jul 2026 12:14:54 +0300 Subject: [PATCH 02/17] feat(cel): expose the resolved event timestamp as a CEL variable Ordering guards need to compare when events happened, not when the worker pool observed them. ResolveEventTime prefers the event's kernel timestamp and falls back to enrichment time only when it is zero. Exposed as a top-level 'timestamp' variable rather than an event field: CelFields getters receive an xcel wrapper around the event and cannot reach EnrichedEvent.Timestamp, so a field would be a second, divergent source of truth. The store will stamp entries from this same function. The CEL-level tests use a real utils.StructEvent rather than a fake: the eval context casts the event to utils.CelEvent and calls GetEventType(), so a fake embedding a nil utils.K8sEvent panics before reaching any assertion. They also assert instants rather than rendered text -- time.Unix yields a local-zone Time, so the rendered offset is whatever the node's TZ is. Signed-off-by: Ben --- docs/features/cel-rule-state-store.md | 49 +++++++++++++++ pkg/rulemanager/cel/cel.go | 6 ++ pkg/rulemanager/cel/eventtime.go | 30 +++++++++ pkg/rulemanager/cel/eventtime_test.go | 91 +++++++++++++++++++++++++++ 4 files changed, 176 insertions(+) create mode 100644 docs/features/cel-rule-state-store.md create mode 100644 pkg/rulemanager/cel/eventtime.go create mode 100644 pkg/rulemanager/cel/eventtime_test.go diff --git a/docs/features/cel-rule-state-store.md b/docs/features/cel-rule-state-store.md new file mode 100644 index 0000000000..da9704667d --- /dev/null +++ b/docs/features/cel-rule-state-store.md @@ -0,0 +1,49 @@ +# CEL rule state store + +Gives a CEL rule memory across events, so a detection can span more than one +event: remember something on `exec`, alert on `network`. Without it, every rule +is a pure predicate over a single event and multi-step behaviour — a process +launched from a mount that later connects out, a webshell chain, create/exec/ +delete of a pod — cannot be expressed at all. + +Design: `shared-designs-and-docs/projects/2026-07-28-cel-rule-state-store/spec.md`. + +> **Status: under construction.** This page documents what has landed. Sections +> appear as the implementation does; the state read/write surface itself is not +> usable yet. + +## `timestamp` — the resolved event time + +A top-level CEL variable (not an `event` field) holding the authoritative time +for the event being evaluated: + +```cel +timestamp // a CEL timestamp +timestamp - duration("5m") // arithmetic and comparison work +``` + +It is the event's **kernel** timestamp where the event carries one, falling back +to node-agent's enrichment time only when that is zero. + +Two reasons it is defined this way: + +**Kernel time, not observation time.** Events are processed by a concurrent +worker pool, so the order node-agent *sees* events is not the order they +*happened*. Any ordering comparison has to be against when things happened or it +is meaningless. + +**One source of truth.** The same function populates both this variable and the +timestamp stamped onto stored state entries. If the two could disagree, an +ordering guard would be comparing different clocks and would silently never +fire — the worst failure mode for a detection rule. + +It is a top-level variable rather than `event.timestamp` because the `event` +field getters receive a wrapper around the raw event and cannot see +node-agent's enrichment time; an event field would therefore have to be a +second, divergent source of truth. + +### Timezone + +`string(timestamp)` renders in the node's local zone, so the offset in the text +depends on where the agent runs. Comparisons are instant-based and unaffected. +Assert on instants, not on rendered text. diff --git a/pkg/rulemanager/cel/cel.go b/pkg/rulemanager/cel/cel.go index b064323df9..601d8f701c 100644 --- a/pkg/rulemanager/cel/cel.go +++ b/pkg/rulemanager/cel/cel.go @@ -58,6 +58,11 @@ func NewCEL(objectCache objectcache.ObjectCache, cfg config.Config, mm ...metric cel.Variable("event", eventTyp), // All events accessible via "event" variable cel.Variable("http", eventTyp), // HTTP events also accessible via "http" variable cel.Variable("eventType", cel.StringType), + // The resolved event time, as a top-level variable rather than an event + // field: CelFields getters receive an xcel wrapper around the event and + // cannot reach EnrichedEvent.Timestamp, so a field would be a second, + // divergent source of truth. See ResolveEventTime. + cel.Variable("timestamp", cel.TimestampType), cel.CustomTypeAdapter(ta), cel.CustomTypeProvider(tp), ext.Strings(), @@ -169,6 +174,7 @@ func (c *CEL) CreateEvalContext(event *events.EnrichedEvent) map[string]any { evalContext := map[string]any{ "eventType": string(eventType), "event": obj, + "timestamp": ResolveEventTime(event), } // For HTTP events, also add "http" variable diff --git a/pkg/rulemanager/cel/eventtime.go b/pkg/rulemanager/cel/eventtime.go new file mode 100644 index 0000000000..1ef5492a62 --- /dev/null +++ b/pkg/rulemanager/cel/eventtime.go @@ -0,0 +1,30 @@ +package cel + +import ( + "time" + + "github.com/kubescape/node-agent/pkg/ebpf/events" +) + +// ResolveEventTime returns the single authoritative timestamp for an event. +// +// It prefers the event's own kernel timestamp, because ordering guards must +// compare when things HAPPENED, not when node-agent got around to seeing them -- +// events are processed by a concurrent worker pool, so observation order is not +// causal order. Some events report a zero timestamp; those fall back to the +// enrichment time rather than the epoch. +// +// Both the CEL "timestamp" variable and rulestate.Entry.Timestamp are populated +// from this function. They must never diverge: a mismatch would make the _ts +// join compare different clocks and silently never fire. +func ResolveEventTime(enrichedEvent *events.EnrichedEvent) time.Time { + if enrichedEvent == nil { + return time.Time{} + } + if enrichedEvent.Event != nil { + if ns := int64(enrichedEvent.Event.GetTimestamp()); ns > 0 { + return time.Unix(0, ns) + } + } + return enrichedEvent.Timestamp +} diff --git a/pkg/rulemanager/cel/eventtime_test.go b/pkg/rulemanager/cel/eventtime_test.go new file mode 100644 index 0000000000..dd681bf760 --- /dev/null +++ b/pkg/rulemanager/cel/eventtime_test.go @@ -0,0 +1,91 @@ +package cel + +import ( + "testing" + "time" + + "github.com/kubescape/node-agent/pkg/ebpf/events" + "github.com/kubescape/node-agent/pkg/utils" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// execEventAt builds an exec event whose kernel timestamp is ts. +// +// A real utils.StructEvent is used rather than a hand-rolled fake: the eval +// context casts the event to utils.CelEvent and calls GetEventType() on it, so a +// fake embedding a nil utils.K8sEvent panics before reaching the assertion. +func execEventAt(ts int64) *utils.StructEvent { + return &utils.StructEvent{ + EventType: utils.ExecveEventType, + Comm: "curl", + Timestamp: ts, + } +} + +func TestResolveEventTime_PrefersEventTimestamp(t *testing.T) { + kernelTime := time.Date(2026, 7, 28, 12, 0, 3, 100000000, time.UTC) + observed := kernelTime.Add(5 * time.Millisecond) + + ee := &events.EnrichedEvent{ + Event: execEventAt(kernelTime.UnixNano()), + Timestamp: observed, + } + assert.Equal(t, kernelTime.UTC(), ResolveEventTime(ee).UTC(), + "kernel time must win over observation time") +} + +func TestResolveEventTime_FallsBackWhenEventTimestampIsZero(t *testing.T) { + observed := time.Date(2026, 7, 28, 12, 0, 3, 0, time.UTC) + ee := &events.EnrichedEvent{ + Event: execEventAt(0), + Timestamp: observed, + } + assert.Equal(t, observed.UTC(), ResolveEventTime(ee).UTC(), + "a zero event timestamp must fall back, not yield the epoch") +} + +func TestResolveEventTime_NilSafe(t *testing.T) { + assert.True(t, ResolveEventTime(nil).IsZero()) + assert.True(t, ResolveEventTime(&events.EnrichedEvent{}).IsZero(), + "a nil inner event must not panic on the hot path") +} + +func TestEvalContext_TimestampIsUsableFromCEL(t *testing.T) { + c := newTestCEL(t) + + kernelTime := time.Date(2026, 7, 28, 12, 0, 3, 100000000, time.UTC) + ee := &events.EnrichedEvent{ + Event: execEventAt(kernelTime.UnixNano()), + Timestamp: kernelTime, + } + + out, err := c.EvaluateExpression(ee, `string(timestamp)`) + require.NoError(t, err) + + // CEL renders a timestamp in the location Go gives it, and time.Unix builds + // a local-zone Time -- so the rendered offset depends on the node's TZ. + // Assert the instant, not the spelling, or this test fails everywhere except + // a UTC machine. + got, err := time.Parse(time.RFC3339Nano, out) + require.NoError(t, err, "timestamp must render as RFC3339: %q", out) + assert.True(t, kernelTime.Equal(got), "want %s, got %s", kernelTime, got) +} + +// The whole point of the variable: comparing a remembered time against the +// current event's time. If timestamp were not a CEL timestamp this would not +// compile, and every _ts ordering guard would silently be dead. +func TestEvalContext_TimestampSupportsOrderingComparisons(t *testing.T) { + c := newTestCEL(t) + + kernelTime := time.Date(2026, 7, 28, 12, 0, 3, 0, time.UTC) + ee := &events.EnrichedEvent{ + Event: execEventAt(kernelTime.UnixNano()), + Timestamp: kernelTime, + } + + out, err := c.EvaluateExpression(ee, + `string(timestamp - duration("1m") < timestamp)`) + require.NoError(t, err) + assert.Equal(t, "true", out) +} From 77b8148cdb76488a55c87af3bd9eb586b0d5d0c1 Mon Sep 17 00:00:00 2001 From: Ben Date: Thu, 30 Jul 2026 12:20:06 +0300 Subject: [PATCH 03/17] feat(processtree): add GetAncestorPIDs for ancestry-based rule matching Walks the creator's global process map instead of GetPidBranch, which resolves a container shim and therefore errors for every host / cgroup-0 process -- leaving those events with a zero-value ProcessTree. Ancestor matching must work identically on a VM and in a pod. maxDepth bounds the walk, and a seen-set breaks parent cycles that a reparenting race could produce, so a malformed tree cannot hang rule evaluation. A PPID of 0 terminates the walk: it means "parent unknown", and recording it would put a key in the ancestor list that no state entry can ever be stored under. The manager mock takes a settable ancestor chain rather than always returning nil, because the rule-level tests for ancestor matching need to stub a chain without building a real process tree. Docs-exempt: internal API; the CEL surface that exposes it is documented when it lands Signed-off-by: Ben --- pkg/processtree/ancestors.go | 49 ++++++++ pkg/processtree/ancestors_test.go | 111 ++++++++++++++++++ .../process_tree_manager_interface.go | 3 + pkg/processtree/process_tree_manager_mock.go | 19 +++ 4 files changed, 182 insertions(+) create mode 100644 pkg/processtree/ancestors.go create mode 100644 pkg/processtree/ancestors_test.go diff --git a/pkg/processtree/ancestors.go b/pkg/processtree/ancestors.go new file mode 100644 index 0000000000..c75f052d6d --- /dev/null +++ b/pkg/processtree/ancestors.go @@ -0,0 +1,49 @@ +package processtree + +import ( + "github.com/armosec/armoapi-go/armotypes" +) + +// GetAncestorPIDs returns pid's ancestors, nearest first, up to maxDepth entries. +// pid itself is excluded. +// +// This walks the creator's global process map rather than +// containerTree.GetPidBranch, because GetPidBranch resolves a container shim and +// errors out when there is none -- which is every host / cgroup-0 process. Walking +// the map works identically for containerised and host processes. +// +// maxDepth also bounds the walk defensively: a reparenting race could in +// principle produce a parent cycle, and the evaluator must not hang. +func (ptm *ProcessTreeManagerImpl) GetAncestorPIDs(pid uint32, maxDepth int) []uint32 { + if maxDepth <= 0 { + return nil + } + + var out []uint32 + seen := make(map[uint32]struct{}, maxDepth) + current := pid + + for len(out) < maxDepth { + var node *armotypes.Process + func() { + ptm.mutex.RLock() + defer ptm.mutex.RUnlock() + node, _ = ptm.creator.GetProcessNode(int(current)) + }() + // PPID 0 means "parent unknown", not "parent is pid 0" -- recording it + // would add a key no state entry can ever be stored under. + if node == nil || node.PPID == 0 { + break + } + if _, dup := seen[node.PPID]; dup { + break + } + seen[node.PPID] = struct{}{} + out = append(out, node.PPID) + if node.PPID == 1 { + break + } + current = node.PPID + } + return out +} diff --git a/pkg/processtree/ancestors_test.go b/pkg/processtree/ancestors_test.go new file mode 100644 index 0000000000..f9f91bfe97 --- /dev/null +++ b/pkg/processtree/ancestors_test.go @@ -0,0 +1,111 @@ +package processtree + +import ( + "fmt" + "testing" + + "github.com/armosec/armoapi-go/armotypes" + "github.com/goradd/maps" + "github.com/kubescape/node-agent/pkg/config" + "github.com/kubescape/node-agent/pkg/processtree/conversion" + "github.com/stretchr/testify/assert" +) + +// stubCreator is a ProcessTreeCreator backed by a plain map, so ancestor walking +// can be tested without feeding synthetic events through the real creator. +type stubCreator struct { + tree map[uint32]*armotypes.Process +} + +func (s *stubCreator) FeedEvent(conversion.ProcessEvent) {} +func (s *stubCreator) Start() {} +func (s *stubCreator) Stop() {} + +func (s *stubCreator) GetRootTree() ([]armotypes.Process, error) { return nil, nil } + +func (s *stubCreator) GetProcessMap() *maps.SafeMap[uint32, *armotypes.Process] { + m := &maps.SafeMap[uint32, *armotypes.Process]{} + for pid, p := range s.tree { + m.Set(pid, p) + } + return m +} + +func (s *stubCreator) GetProcessNode(pid int) (*armotypes.Process, error) { + p, ok := s.tree[uint32(pid)] + if !ok { + return nil, fmt.Errorf("process %d not found", pid) + } + return p, nil +} + +func newTestManagerWithTree(t *testing.T, tree map[uint32]*armotypes.Process) *ProcessTreeManagerImpl { + t.Helper() + return &ProcessTreeManagerImpl{ + creator: &stubCreator{tree: tree}, + config: config.Config{}, + } +} + +func TestGetAncestorPIDs(t *testing.T) { + // 900 (bash) -> 4471 (sh) -> 4530 (curl) + tree := map[uint32]*armotypes.Process{ + 900: {PID: 900, PPID: 1}, + 4471: {PID: 4471, PPID: 900}, + 4530: {PID: 4530, PPID: 4471}, + } + + tests := []struct { + name string + pid uint32 + maxDepth int + want []uint32 + }{ + {"full chain", 4530, 8, []uint32{4471, 900, 1}}, + {"depth bound respected", 4530, 2, []uint32{4471, 900}}, + {"leaf with one ancestor", 900, 8, []uint32{1}}, + {"unknown pid yields nothing", 99999, 8, nil}, + {"zero depth yields nothing", 4530, 0, nil}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ptm := newTestManagerWithTree(t, tree) + assert.Equal(t, tt.want, ptm.GetAncestorPIDs(tt.pid, tt.maxDepth)) + }) + } +} + +// Host processes have no container shim, so GetPidBranch errors for them and +// enrichedEvent.ProcessTree is zero-valued. GetAncestorPIDs must still work -- +// this is the whole reason it exists. +func TestGetAncestorPIDs_WorksForHostProcessesWithNoShim(t *testing.T) { + tree := map[uint32]*armotypes.Process{ + 2200: {PID: 2200, PPID: 1}, + 2300: {PID: 2300, PPID: 2200}, + } + ptm := newTestManagerWithTree(t, tree) + assert.Equal(t, []uint32{2200, 1}, ptm.GetAncestorPIDs(2300, 8)) +} + +func TestGetAncestorPIDs_TerminatesOnCycle(t *testing.T) { + // Defensive: reparenting races could in principle produce a loop. The depth + // bound must contain it rather than hanging the evaluator. + tree := map[uint32]*armotypes.Process{ + 10: {PID: 10, PPID: 11}, + 11: {PID: 11, PPID: 10}, + } + ptm := newTestManagerWithTree(t, tree) + assert.LessOrEqual(t, len(ptm.GetAncestorPIDs(10, 8)), 8) +} + +// A PPID of 0 means "parent unknown", not "parent is pid 0". Recording it would +// put a meaningless 0 in the ancestor list and make state lookups probe a key +// that can never exist. +func TestGetAncestorPIDs_StopsAtUnknownParent(t *testing.T) { + tree := map[uint32]*armotypes.Process{ + 7000: {PID: 7000, PPID: 0}, + } + ptm := newTestManagerWithTree(t, tree) + assert.Empty(t, ptm.GetAncestorPIDs(7000, 8)) +} diff --git a/pkg/processtree/process_tree_manager_interface.go b/pkg/processtree/process_tree_manager_interface.go index aa11122288..901a54059d 100644 --- a/pkg/processtree/process_tree_manager_interface.go +++ b/pkg/processtree/process_tree_manager_interface.go @@ -22,4 +22,7 @@ type ProcessTreeManager interface { // inherits btime's whole-second skew, so it must never be compared for // identity. GetProcessBootTimeNs(pid uint32) uint64 + // GetAncestorPIDs returns pid's ancestors, nearest first, bounded by maxDepth. + // Works for host processes too, unlike GetContainerProcessTree. + GetAncestorPIDs(pid uint32, maxDepth int) []uint32 } diff --git a/pkg/processtree/process_tree_manager_mock.go b/pkg/processtree/process_tree_manager_mock.go index 8d04552684..b1df311f8e 100644 --- a/pkg/processtree/process_tree_manager_mock.go +++ b/pkg/processtree/process_tree_manager_mock.go @@ -9,6 +9,7 @@ import ( type ProcessTreeManagerMock struct { pidList []uint32 bootTimeNs map[uint32]uint64 + ancestors []uint32 } var _ ProcessTreeManager = (*ProcessTreeManagerMock)(nil) @@ -63,3 +64,21 @@ func (m *ProcessTreeManagerMock) SetProcessBootTimeNs(pid uint32, ns uint64) { } m.bootTimeNs[pid] = ns } + +// SetAncestors sets the ancestor chain the mock reports, nearest first. Rule +// tests that exercise ancestor matching need to stub a chain rather than build a +// real process tree. +func (m *ProcessTreeManagerMock) SetAncestors(pids []uint32) { + m.ancestors = pids +} + +// GetAncestorPIDs returns the configured ancestor chain, truncated to maxDepth. +func (m *ProcessTreeManagerMock) GetAncestorPIDs(_ uint32, maxDepth int) []uint32 { + if maxDepth <= 0 || len(m.ancestors) == 0 { + return nil + } + if len(m.ancestors) > maxDepth { + return m.ancestors[:maxDepth] + } + return m.ancestors +} From c4227f14fadfa18e74fcbbb9647b94d442bf62c9 Mon Sep 17 00:00:00 2001 From: Ben Date: Thu, 30 Jul 2026 12:23:03 +0300 Subject: [PATCH 04/17] feat(rulestate): add the TTL-bounded rule state store Sharded by scope ID so the per-scope cap is a plain len(), container-removal purge is one map delete, and one container's churn stays off its neighbours' locks. Over-cap writes are REJECTED, never satisfied by eviction: evicting would let a container that sprays events silently disable detection for itself or a neighbour. The host bucket (c:__host__) gets its own larger cap because it holds the whole node's process space and never receives a removal purge. Two properties worth knowing when reading this code: Replacing an existing key bypasses the cap check, because it does not grow the scope. Without that, a scope sitting at its cap could never update its own markers and a bidirectional rule would freeze on stale state. The global ceiling is approximate under concurrency and says so in a comment. Concurrent writers can each pass the size check before any increments, so the store can overshoot MaxSize by up to the number of in-flight writers. Making it exact would serialise every write on one lock. The per-scope cap is exact, and that is the one that bounds a single workload. Knows nothing about CEL or rules, so it is testable without an evaluator. Docs-exempt: internal store; the rule-facing surface is documented in Tasks 5-8 Signed-off-by: Ben --- pkg/rulestate/store.go | 203 +++++++++++++++++++++++++ pkg/rulestate/store_test.go | 293 ++++++++++++++++++++++++++++++++++++ pkg/rulestate/types.go | 100 ++++++++++++ 3 files changed, 596 insertions(+) create mode 100644 pkg/rulestate/store.go create mode 100644 pkg/rulestate/store_test.go create mode 100644 pkg/rulestate/types.go diff --git a/pkg/rulestate/store.go b/pkg/rulestate/store.go new file mode 100644 index 0000000000..b3e6f888d7 --- /dev/null +++ b/pkg/rulestate/store.go @@ -0,0 +1,203 @@ +package rulestate + +import ( + "context" + "hash/fnv" + "sync" + "time" + + "github.com/armosec/armoapi-go/armotypes" +) + +const shardCount = 16 + +type entryKey struct{ ruleID, name, key string } + +type bucket struct { + entries map[entryKey]*Entry +} + +type shard struct { + mu sync.RWMutex + scopes map[string]*bucket +} + +// Store is a bounded, TTL-expiring set of Entries sharded by scope ID. +// +// Sharding by scope ID (not by full key) is deliberate: it makes the per-scope +// cap a plain len(), makes container-removal purge a single map delete, and keeps +// one container's write churn off its neighbours' locks. +type Store struct { + cfg Config + metrics Metrics + shards [shardCount]*shard + + sizeMu sync.Mutex + size int +} + +func NewStore(cfg Config, metrics Metrics) *Store { + s := &Store{cfg: cfg, metrics: metrics} + for i := range s.shards { + s.shards[i] = &shard{scopes: make(map[string]*bucket)} + } + return s +} + +func (s *Store) shardFor(scopeID string) *shard { + h := fnv.New32a() + _, _ = h.Write([]byte(scopeID)) + return s.shards[h.Sum32()%shardCount] +} + +func (s *Store) scopeCap(scopeID string) int { + if IsHostScopeID(scopeID) { + return s.cfg.MaxEntriesForHost + } + return s.cfg.MaxEntriesPerContainer +} + +// Set stores e, replacing any live entry with the same (ruleID, name, key) in the +// same scope -- last write wins, which also resets the TTL. +func (s *Store) Set(e *Entry) error { + if !s.cfg.Enabled { + return nil + } + + // The global check is deliberately not atomic with the insert below: it is a + // backstop, and serialising every write on one lock to make the ceiling exact + // would cost more than the few entries of overshoot it prevents. Concurrent + // writers can each pass this check before any of them increments, so the size + // can exceed MaxSize by up to the number of in-flight writers. The per-scope + // cap, which IS exact, is what bounds any single workload. + if s.currentSize() >= s.cfg.MaxSize { + if s.Sweep() == 0 { + s.metrics.ReportStateWriteRejected(e.RuleID, "global_cap") + return ErrGlobalCapReached + } + } + + sh := s.shardFor(e.ScopeID) + k := entryKey{e.RuleID, e.Name, e.Key} + + sh.mu.Lock() + b, ok := sh.scopes[e.ScopeID] + if !ok { + b = &bucket{entries: make(map[entryKey]*Entry)} + sh.scopes[e.ScopeID] = b + } + // Replacing an existing key does not grow the scope, so the cap must not + // block it -- otherwise a full scope could never update its own markers. + _, replacing := b.entries[k] + if !replacing && len(b.entries) >= s.scopeCap(e.ScopeID) { + sh.mu.Unlock() + s.metrics.ReportStateWriteRejected(e.RuleID, "scope_cap") + return ErrScopeCapReached + } + b.entries[k] = e + sh.mu.Unlock() + + if !replacing { + s.addSize(1) + } + s.metrics.ReportStateWrite(e.RuleID, "ok") + return nil +} + +// Get returns a live entry, or false if absent or expired. Expiry is enforced +// here as well as by the sweeper so a read never sees a stale marker. +func (s *Store) Get(ruleID string, _ armotypes.StateScope, scopeID, name, key string) (*Entry, bool) { + if !s.cfg.Enabled { + return nil, false + } + sh := s.shardFor(scopeID) + + sh.mu.RLock() + b, ok := sh.scopes[scopeID] + if !ok { + sh.mu.RUnlock() + return nil, false + } + e, ok := b.entries[entryKey{ruleID, name, key}] + sh.mu.RUnlock() + + if !ok || e.expired(time.Now()) { + return nil, false + } + return e, true +} + +// PurgeScope drops every entry for a scope. Called on container removal. +func (s *Store) PurgeScope(scopeID string) { + sh := s.shardFor(scopeID) + sh.mu.Lock() + n := 0 + if b, ok := sh.scopes[scopeID]; ok { + n = len(b.entries) + delete(sh.scopes, scopeID) + } + sh.mu.Unlock() + + if n > 0 { + s.addSize(-n) + s.metrics.ReportStatePurged(n) + } +} + +// Sweep removes expired entries and returns how many it reclaimed. Lazy +// expiry on read hides stale entries; only Sweep frees the memory. +func (s *Store) Sweep() int { + now := time.Now() + total := 0 + for _, sh := range s.shards { + sh.mu.Lock() + for scopeID, b := range sh.scopes { + for k, e := range b.entries { + if e.expired(now) { + delete(b.entries, k) + total++ + } + } + if len(b.entries) == 0 { + delete(sh.scopes, scopeID) + } + } + sh.mu.Unlock() + } + if total > 0 { + s.addSize(-total) + s.metrics.ReportStateExpired(total) + } + return total +} + +// Run sweeps until ctx is cancelled. +func (s *Store) Run(ctx context.Context) { + if !s.cfg.Enabled || s.cfg.SweepInterval <= 0 { + return + } + t := time.NewTicker(s.cfg.SweepInterval) + defer t.Stop() + for { + select { + case <-ctx.Done(): + return + case <-t.C: + s.Sweep() + } + } +} + +func (s *Store) Len() int { return s.currentSize() } + +func (s *Store) currentSize() int { + s.sizeMu.Lock() + defer s.sizeMu.Unlock() + return s.size +} + +func (s *Store) addSize(d int) { + s.sizeMu.Lock() + s.size += d + s.sizeMu.Unlock() +} diff --git a/pkg/rulestate/store_test.go b/pkg/rulestate/store_test.go new file mode 100644 index 0000000000..a1431c553d --- /dev/null +++ b/pkg/rulestate/store_test.go @@ -0,0 +1,293 @@ +package rulestate + +import ( + "fmt" + "sync" + "testing" + "time" + + "github.com/armosec/armoapi-go/armotypes" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func testConfig() Config { + return Config{ + Enabled: true, + MaxSize: 1000, + MaxEntriesPerContainer: 4, + MaxEntriesForHost: 8, + MaxTTL: 30 * time.Minute, + SweepInterval: time.Second, + AncestorMaxDepth: 8, + } +} + +func entry(ruleID, scopeID, name, key string, ts time.Time, ttl time.Duration) *Entry { + return &Entry{ + RuleID: ruleID, Name: name, Key: key, + Scope: armotypes.StateScopeContainer, ScopeID: scopeID, + EventType: "exec", + Timestamp: ts, ExpiresAt: ts.Add(ttl), + Process: &armotypes.Process{PID: 4471, Comm: "xmrig"}, + } +} + +func TestStore_SetThenGet(t *testing.T) { + s := NewStore(testConfig(), NoopMetrics{}) + now := time.Now() + require.NoError(t, s.Set(entry("R1089", "c:abc", "mount_exec", "4471", now, time.Minute))) + + got, ok := s.Get("R1089", armotypes.StateScopeContainer, "c:abc", "mount_exec", "4471") + require.True(t, ok) + assert.Equal(t, uint32(4471), got.Process.PID) +} + +func TestStore_IsolationAcrossRulesScopesAndKeys(t *testing.T) { + s := NewStore(testConfig(), NoopMetrics{}) + now := time.Now() + require.NoError(t, s.Set(entry("R1089", "c:abc", "mount_exec", "4471", now, time.Minute))) + + // Different rule: state is rule-private. + _, ok := s.Get("R1090", armotypes.StateScopeContainer, "c:abc", "mount_exec", "4471") + assert.False(t, ok, "another rule must not see this entry") + + // Different container: the security property. + _, ok = s.Get("R1089", armotypes.StateScopeContainer, "c:def", "mount_exec", "4471") + assert.False(t, ok, "a neighbouring container must not see this entry") + + // Different key and different name. + _, ok = s.Get("R1089", armotypes.StateScopeContainer, "c:abc", "mount_exec", "9999") + assert.False(t, ok) + _, ok = s.Get("R1089", armotypes.StateScopeContainer, "c:abc", "other", "4471") + assert.False(t, ok) +} + +func TestStore_ExpiredEntryIsAMissOnRead(t *testing.T) { + s := NewStore(testConfig(), NoopMetrics{}) + past := time.Now().Add(-2 * time.Minute) + require.NoError(t, s.Set(entry("R1089", "c:abc", "mount_exec", "4471", past, time.Minute))) + + _, ok := s.Get("R1089", armotypes.StateScopeContainer, "c:abc", "mount_exec", "4471") + assert.False(t, ok, "TTL must be enforced lazily on read, not only by the sweeper") +} + +func TestStore_SweepReclaimsExpired(t *testing.T) { + s := NewStore(testConfig(), NoopMetrics{}) + past := time.Now().Add(-2 * time.Minute) + now := time.Now() + require.NoError(t, s.Set(entry("R1089", "c:abc", "expired", "1", past, time.Minute))) + require.NoError(t, s.Set(entry("R1089", "c:abc", "live", "1", now, time.Minute))) + + assert.Equal(t, 1, s.Sweep()) + assert.Equal(t, 1, s.Len()) +} + +func TestStore_ScopeCapRejectsRatherThanEvicting(t *testing.T) { + s := NewStore(testConfig(), NoopMetrics{}) // MaxEntriesPerContainer = 4 + now := time.Now() + for i := 0; i < 4; i++ { + require.NoError(t, s.Set(entry("R1089", "c:abc", "n", fmt.Sprint(i), now, time.Minute))) + } + + err := s.Set(entry("R1089", "c:abc", "n", "overflow", now, time.Minute)) + assert.ErrorIs(t, err, ErrScopeCapReached) + + // The critical assertion: nothing already stored was evicted. Evicting would + // let a hostile container silently disable its own -- or a neighbour's -- rules. + for i := 0; i < 4; i++ { + _, ok := s.Get("R1089", armotypes.StateScopeContainer, "c:abc", "n", fmt.Sprint(i)) + assert.True(t, ok, "entry %d was evicted; writes must be rejected instead", i) + } +} + +func TestStore_ScopeCapIsPerScopeNotGlobal(t *testing.T) { + s := NewStore(testConfig(), NoopMetrics{}) + now := time.Now() + for i := 0; i < 4; i++ { + require.NoError(t, s.Set(entry("R1089", "c:abc", "n", fmt.Sprint(i), now, time.Minute))) + } + // A different container is unaffected by its neighbour hitting the cap. + require.NoError(t, s.Set(entry("R1089", "c:def", "n", "0", now, time.Minute))) +} + +// An over-cap scope must still accept an overwrite of a key it already holds: +// refusing would freeze the scope's newest observation out and make a +// bidirectional rule stop updating its own marker. +func TestStore_OverwriteSucceedsEvenAtCap(t *testing.T) { + s := NewStore(testConfig(), NoopMetrics{}) + now := time.Now() + for i := 0; i < 4; i++ { + require.NoError(t, s.Set(entry("R1089", "c:abc", "n", fmt.Sprint(i), now, time.Minute))) + } + later := now.Add(time.Second) + require.NoError(t, s.Set(entry("R1089", "c:abc", "n", "0", later, time.Minute)), + "replacing an existing key does not grow the scope, so the cap must not block it") + + got, ok := s.Get("R1089", armotypes.StateScopeContainer, "c:abc", "n", "0") + require.True(t, ok) + assert.Equal(t, later, got.Timestamp) +} + +func TestStore_OverwriteIsLastWriteWins(t *testing.T) { + s := NewStore(testConfig(), NoopMetrics{}) + t1 := time.Now() + t2 := t1.Add(time.Second) + require.NoError(t, s.Set(entry("R1089", "c:abc", "n", "1", t1, time.Minute))) + require.NoError(t, s.Set(entry("R1089", "c:abc", "n", "1", t2, time.Minute))) + + got, ok := s.Get("R1089", armotypes.StateScopeContainer, "c:abc", "n", "1") + require.True(t, ok) + assert.Equal(t, t2, got.Timestamp) + assert.Equal(t, 1, s.Len(), "overwrite must not grow the store") +} + +func TestStore_HostBucketHasItsOwnLargerCap(t *testing.T) { + s := NewStore(testConfig(), NoopMetrics{}) // host cap 8, container cap 4 + now := time.Now() + for i := 0; i < 8; i++ { + require.NoError(t, s.Set(entry("R1089", HostScopeID(), "n", fmt.Sprint(i), now, time.Minute)), + "host bucket holds the whole node's processes, so it needs a bigger cap than one container") + } + assert.ErrorIs(t, s.Set(entry("R1089", HostScopeID(), "n", "8", now, time.Minute)), ErrScopeCapReached) +} + +func TestScopeIDs_HostAndNodeDoNotCollide(t *testing.T) { + // Host processes carry ContainerID == "", and node scope has no ID. Without + // type prefixes both would be "" and share a bucket. + assert.Equal(t, "c:__host__", ContainerScopeID("")) + assert.Equal(t, "c:abc", ContainerScopeID("abc")) + assert.Equal(t, "n:", NodeScopeID()) + assert.Equal(t, "p:prod/web-1", PodScopeID("prod", "web-1")) + assert.NotEqual(t, ContainerScopeID(""), NodeScopeID()) + assert.True(t, IsHostScopeID(ContainerScopeID(""))) + assert.False(t, IsHostScopeID(ContainerScopeID("abc"))) +} + +func TestStore_PurgeScopeDropsOnlyThatScope(t *testing.T) { + s := NewStore(testConfig(), NoopMetrics{}) + now := time.Now() + require.NoError(t, s.Set(entry("R1089", "c:abc", "n", "1", now, time.Minute))) + require.NoError(t, s.Set(entry("R1089", "c:def", "n", "1", now, time.Minute))) + + s.PurgeScope("c:abc") + _, ok := s.Get("R1089", armotypes.StateScopeContainer, "c:abc", "n", "1") + assert.False(t, ok) + _, ok = s.Get("R1089", armotypes.StateScopeContainer, "c:def", "n", "1") + assert.True(t, ok) + assert.Equal(t, 1, s.Len(), "purge must decrement the global size, not just drop the bucket") +} + +func TestStore_DisabledIsANoop(t *testing.T) { + cfg := testConfig() + cfg.Enabled = false + s := NewStore(cfg, NoopMetrics{}) + require.NoError(t, s.Set(entry("R1089", "c:abc", "n", "1", time.Now(), time.Minute))) + _, ok := s.Get("R1089", armotypes.StateScopeContainer, "c:abc", "n", "1") + assert.False(t, ok, "disabled: writes are no-ops and reads always miss") + assert.Equal(t, 0, s.Len()) +} + +// The global ceiling is a backstop. It must reject rather than evict, for the +// same reason the per-scope cap does. +func TestStore_GlobalCapRejectsWhenNothingCanBeReclaimed(t *testing.T) { + cfg := testConfig() + cfg.MaxSize = 3 + cfg.MaxEntriesPerContainer = 100 + s := NewStore(cfg, NoopMetrics{}) + now := time.Now() + for i := 0; i < 3; i++ { + require.NoError(t, s.Set(entry("R1089", "c:abc", "n", fmt.Sprint(i), now, time.Minute))) + } + + assert.ErrorIs(t, s.Set(entry("R1089", "c:abc", "n", "3", now, time.Minute)), ErrGlobalCapReached) + assert.Equal(t, 3, s.Len()) +} + +// At the ceiling, an expiring entry should make room -- otherwise a node that +// once filled the store would stop correlating forever. +func TestStore_GlobalCapSweepsBeforeRejecting(t *testing.T) { + cfg := testConfig() + cfg.MaxSize = 3 + cfg.MaxEntriesPerContainer = 100 + s := NewStore(cfg, NoopMetrics{}) + now := time.Now() + past := now.Add(-2 * time.Minute) + + require.NoError(t, s.Set(entry("R1089", "c:abc", "n", "0", past, time.Minute))) + require.NoError(t, s.Set(entry("R1089", "c:abc", "n", "1", now, time.Minute))) + require.NoError(t, s.Set(entry("R1089", "c:abc", "n", "2", now, time.Minute))) + + require.NoError(t, s.Set(entry("R1089", "c:abc", "n", "3", now, time.Minute)), + "the expired entry must be reclaimed to admit this write") + assert.Equal(t, 3, s.Len()) +} + +func TestStore_ConcurrentSetGetIsRaceFree(t *testing.T) { + cfg := testConfig() + cfg.MaxEntriesPerContainer = 10000 + // Both caps have to clear 8*200, or the assertion below is really measuring + // the global ceiling rejecting writes rather than concurrent correctness. + cfg.MaxSize = 100000 + s := NewStore(cfg, NoopMetrics{}) + now := time.Now() + + var wg sync.WaitGroup + for c := 0; c < 8; c++ { + wg.Add(1) + go func(c int) { + defer wg.Done() + scopeID := fmt.Sprintf("c:%d", c) + for i := 0; i < 200; i++ { + _ = s.Set(entry("R1089", scopeID, "n", fmt.Sprint(i), now, time.Minute)) + s.Get("R1089", armotypes.StateScopeContainer, scopeID, "n", fmt.Sprint(i)) + } + }(c) + } + wg.Wait() + assert.Equal(t, 8*200, s.Len()) +} + +// Sweep and Set race on the size counter and on bucket maps; a concurrent sweeper +// is exactly what Run does in production. +func TestStore_ConcurrentSweepIsRaceFree(t *testing.T) { + cfg := testConfig() + cfg.MaxEntriesPerContainer = 10000 + s := NewStore(cfg, NoopMetrics{}) + + stop := make(chan struct{}) + + var sweeper sync.WaitGroup + sweeper.Add(1) + go func() { + defer sweeper.Done() + for { + select { + case <-stop: + return + default: + s.Sweep() + } + } + }() + + var writer sync.WaitGroup + writer.Add(1) + go func() { + defer writer.Done() + now := time.Now() + for i := 0; i < 500; i++ { + // Half of these are born expired, so the sweeper has real work. + ttl := time.Minute + if i%2 == 0 { + ttl = -time.Minute + } + _ = s.Set(entry("R1089", "c:abc", "n", fmt.Sprint(i), now, ttl)) + } + }() + + writer.Wait() + close(stop) + sweeper.Wait() + assert.GreaterOrEqual(t, s.Len(), 0, "size must never go negative") +} diff --git a/pkg/rulestate/types.go b/pkg/rulestate/types.go new file mode 100644 index 0000000000..99b3ae7903 --- /dev/null +++ b/pkg/rulestate/types.go @@ -0,0 +1,100 @@ +// Package rulestate holds short-lived, TTL-bounded markers that let a CEL rule +// remember a fact from one event and read it back when a later event arrives. +// +// The package deliberately knows nothing about CEL, rules or Kubernetes: it is a +// bounded map with expiry, so it stays unit-testable without an evaluator. The +// CEL bindings live in pkg/rulemanager/cel/libraries/state, and write-clause +// execution in pkg/rulemanager/statewrites. +package rulestate + +import ( + "errors" + "time" + + "github.com/armosec/armoapi-go/armotypes" +) + +var ( + // ErrScopeCapReached means this scope is at its entry cap. The write is + // rejected -- never satisfied by evicting an existing entry, which would let + // one workload silently disable detection for itself or a neighbour. + ErrScopeCapReached = errors.New("rulestate: scope entry cap reached") + // ErrGlobalCapReached means the node-wide ceiling is reached even after a sweep. + ErrGlobalCapReached = errors.New("rulestate: global entry cap reached") +) + +// Entry is one remembered fact. +// +// Process and Admission are mutually exclusive: node-agent entries carry a +// Process, operator entries carry an Admission. Both map straight onto +// armotypes.CorrelationEvidence, so store -> alert is a copy, not a translation. +type Entry struct { + RuleID string + Name string + Scope armotypes.StateScope + ScopeID string + Key string + EventType armotypes.EventType + + // Timestamp is when the remembered event HAPPENED (see cel.ResolveEventTime), + // not when it was observed. Ordering guards compare against it. + Timestamp time.Time + ExpiresAt time.Time + + Process *armotypes.Process + Admission *armotypes.AdmissionEvidence + Value map[string]any +} + +func (e *Entry) expired(now time.Time) bool { return now.After(e.ExpiresAt) } + +// Config bounds the store. Caps are the anti-abuse mechanism. +type Config struct { + Enabled bool `mapstructure:"enabled"` + // MaxSize is the node-wide ceiling, a backstop above the per-scope caps. + MaxSize int `mapstructure:"maxSize"` + // MaxEntriesPerContainer bounds one container. + MaxEntriesPerContainer int `mapstructure:"maxEntriesPerContainer"` + // MaxEntriesForHost bounds the c:__host__ bucket, which holds the whole + // node's process space rather than one workload, and never receives a + // container-removal purge -- so it needs a larger cap and relies on TTL. + MaxEntriesForHost int `mapstructure:"maxEntriesForHost"` + MaxTTL time.Duration `mapstructure:"maxTtl"` + SweepInterval time.Duration `mapstructure:"sweepInterval"` + AncestorMaxDepth int `mapstructure:"ancestorMaxDepth"` +} + +// Metrics is the observability surface. There is deliberately no per-read +// counter: reads are on the hot path. +type Metrics interface { + ReportStateWrite(ruleID, result string) + ReportStateWriteRejected(ruleID, reason string) + ReportStateExpired(n int) + ReportStatePurged(n int) + ReportStateEntries(scope string, n int) +} + +type NoopMetrics struct{} + +func (NoopMetrics) ReportStateWrite(string, string) {} +func (NoopMetrics) ReportStateWriteRejected(string, string) {} +func (NoopMetrics) ReportStateExpired(int) {} +func (NoopMetrics) ReportStatePurged(int) {} +func (NoopMetrics) ReportStateEntries(string, int) {} + +const hostScopeSuffix = "__host__" + +// ContainerScopeID maps a container ID to its bucket. The empty container ID is +// a host / cgroup-0 process, which gets an explicit pseudo-container bucket -- +// without the type prefix it would collide with node scope, whose ID is also "". +func ContainerScopeID(containerID string) string { + if containerID == "" { + return "c:" + hostScopeSuffix + } + return "c:" + containerID +} + +func HostScopeID() string { return "c:" + hostScopeSuffix } +func NodeScopeID() string { return "n:" } +func PodScopeID(ns, pod string) string { return "p:" + ns + "/" + pod } +func IsHostScopeID(scopeID string) bool { return scopeID == HostScopeID() } From 2cf9d5654e86bb22f2accb28ebf25deb1a34bb8f Mon Sep 17 00:00:00 2001 From: Ben Date: Thu, 30 Jul 2026 14:12:02 +0300 Subject: [PATCH 05/17] feat(cel): add state.has/get/has_ancestor/get_ancestor read functions Reads are pure and rule-private. The plan called for ruleID, scope IDs and the ancestor list to be "injected into the eval context", which needed a concrete mechanism: cel-go hands a function binding only its arguments, never the activation, so a global function named "state.has" cannot discover which rule or container it is evaluating for. So "state" is a VARIABLE whose value is a per-(rule, event) Accessor, and the read functions are member overloads on it. The authored syntax is unchanged -- state.has("mount_exec", "4471") -- but the context now lives in the receiver, which is what makes cross-rule and cross-container reads inexpressible rather than merely forbidden. It also keeps the library itself immutable, so it is safe to share across the worker pool; a library holding per-evaluation state would race, since node-agent evaluates events concurrently against one shared cel.Env. A read resolves its scope by looking the name up in the rule's own stateWrites declarations, per the spec: state is rule-private, so a name determines its scope. An undeclared name reads as a miss rather than an error -- load-time validation is what rejects it, and erroring here would take out a working rule. The member functions are named "has" and "get", which collide with CEL's built-in has() macro by identifier. TestState_DoesNotShadowTheHasMacro pins that has(event.field) still parses and evaluates, because if that ever regressed it would break every existing rule using field presence. The cost estimator keys off overloadID, not the function name, for the same reason: "has" and "get" are too generic to match on. It returns nil for unknown overloads. Note these estimators are currently inert -- nothing in the repo calls NewCompositeCostEstimator -- but it is written to be correct if wired up. Signed-off-by: Ben --- docs/features/cel-rule-state-store.md | 70 +++- pkg/config/config.go | 2 + pkg/rulemanager/cel/cel.go | 5 + .../cel/libraries/state/accessor.go | 184 ++++++++++ .../cel/libraries/state/readtracker.go | 50 +++ .../cel/libraries/state/statelib.go | 217 ++++++++++++ .../cel/libraries/state/statelib_test.go | 317 ++++++++++++++++++ 7 files changed, 842 insertions(+), 3 deletions(-) create mode 100644 pkg/rulemanager/cel/libraries/state/accessor.go create mode 100644 pkg/rulemanager/cel/libraries/state/readtracker.go create mode 100644 pkg/rulemanager/cel/libraries/state/statelib.go create mode 100644 pkg/rulemanager/cel/libraries/state/statelib_test.go diff --git a/docs/features/cel-rule-state-store.md b/docs/features/cel-rule-state-store.md index da9704667d..42f2c8cec4 100644 --- a/docs/features/cel-rule-state-store.md +++ b/docs/features/cel-rule-state-store.md @@ -8,9 +8,73 @@ delete of a pod — cannot be expressed at all. Design: `shared-designs-and-docs/projects/2026-07-28-cel-rule-state-store/spec.md`. -> **Status: under construction.** This page documents what has landed. Sections -> appear as the implementation does; the state read/write surface itself is not -> usable yet. +> **Status: under construction.** This page documents what has landed. The read +> functions below exist and are tested, but nothing populates the store yet — +> writes (`stateWrites:`) are still to come, so in a running agent every read is +> currently a miss. + +## Reading state + +Four functions, on a `state` receiver: + +```cel +state.has(name) // bool — for a fact about the whole scope +state.has(name, key) // bool — for a fact about one subject +state.get(name) // map — empty map on a miss, never an error +state.get(name, key) // map +state.has_ancestor(name) // bool — any ancestor PID carries the marker +state.get_ancestor(name) // map — the NEAREST matching ancestor +``` + +`name` is what kind of fact ("mount_exec"); `key` is who it is about, usually +`string(event.pid)`. A read takes **no scope argument**: state is rule-private, +so the name already determines its scope from the rule's own `stateWrites`. + +`state.get` on a miss returns an **empty map**, so guard provenance access with +`state.has` — `state.get("x", k)._pid` on a miss is a "no such key" error, and +that error fails the whole predicate: + +```cel +state.has("mount_exec", string(event.ppid)) && + state.get("mount_exec", string(event.ppid))._ts < timestamp +``` + +### What `state.get` returns + +Engine-stamped provenance uses reserved `_`-prefixed keys; the rule's own +`value:` entries sit alongside them at the top level. Author keys may not begin +with `_`, so they can never shadow provenance. + +| Key | Type | Meaning | +|---|---|---| +| `_ts` | timestamp | When the remembered event happened. Compare against `timestamp`. | +| `_eventType` | string | The event stream that wrote the entry. | +| `_container` | string | The scope ID the entry lives under. | +| `_pid` / `_ppid` | uint | Process and parent PID. | +| `_comm` / `_pcomm` | string | Process and parent command name. | +| `_exe` | string | Executable path. | +| `_cwd` | string | Working directory. | + +### The ancestor functions assume a PID key + +`has_ancestor` / `get_ancestor` probe each ancestor PID in turn, so they only +find entries whose `key` was a PID. That is an authoring contract, not something +the engine can check — write `key: string(event.pid)` for any name you intend to +read this way. + +They work identically for host and containerised processes. + +### Why `state` is a variable, not a function namespace + +Unlike `process.*` or `net.*`, `state` is a **variable** with member functions. +cel-go hands a function binding only its arguments, never the surrounding +context, so a global `state.has` could not know which rule or which container it +was evaluating for. + +That is also the security property: the rule ID, the scope IDs and the ancestor +list live in the receiver, and no CEL syntax supplies or overrides them. Reading +another rule's state or a neighbouring container's state is not merely forbidden +— it is inexpressible. ## `timestamp` — the resolved event time diff --git a/pkg/config/config.go b/pkg/config/config.go index cec9f41ab6..cda1a5920e 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -16,6 +16,7 @@ import ( processtreecreator "github.com/kubescape/node-agent/pkg/processtree/config" "github.com/kubescape/node-agent/pkg/rulemanager/cel/libraries/cache" "github.com/kubescape/node-agent/pkg/rulemanager/rulecooldown" + "github.com/kubescape/node-agent/pkg/rulestate" "github.com/spf13/viper" ) @@ -54,6 +55,7 @@ type AlertDeduplicationConfig struct { type Config struct { BlockEvents bool `mapstructure:"blockEvents"` CelConfigCache cache.FunctionCacheConfig `mapstructure:"celConfigCache"` + CelStateStore rulestate.Config `mapstructure:"celStateStore"` ContainerEolNotificationBuffer int `mapstructure:"containerEolNotificationBuffer"` DBpf bool `mapstructure:"dBpf"` DCapSys bool `mapstructure:"dCapSys"` diff --git a/pkg/rulemanager/cel/cel.go b/pkg/rulemanager/cel/cel.go index 601d8f701c..1b28500518 100644 --- a/pkg/rulemanager/cel/cel.go +++ b/pkg/rulemanager/cel/cel.go @@ -20,6 +20,7 @@ import ( "github.com/kubescape/node-agent/pkg/rulemanager/cel/libraries/networkneighborhood" "github.com/kubescape/node-agent/pkg/rulemanager/cel/libraries/parse" "github.com/kubescape/node-agent/pkg/rulemanager/cel/libraries/process" + "github.com/kubescape/node-agent/pkg/rulemanager/cel/libraries/state" typesv1 "github.com/kubescape/node-agent/pkg/rulemanager/types/v1" "github.com/kubescape/node-agent/pkg/utils" "github.com/picatz/xcel" @@ -72,6 +73,10 @@ func NewCEL(objectCache objectcache.ObjectCache, cfg config.Config, mm ...metric parse.Parse(cfg), net.Net(cfg), process.Process(cfg), + // Declares the "state" variable and its read functions. The store and the + // per-rule receiver are injected into the eval context, not here -- see + // state.Accessor. + state.State(cfg), } env, err := cel.NewEnv(envOptions...) diff --git a/pkg/rulemanager/cel/libraries/state/accessor.go b/pkg/rulemanager/cel/libraries/state/accessor.go new file mode 100644 index 0000000000..eaaf8c9ccc --- /dev/null +++ b/pkg/rulemanager/cel/libraries/state/accessor.go @@ -0,0 +1,184 @@ +package state + +import ( + "fmt" + "reflect" + + "github.com/armosec/armoapi-go/armotypes" + "github.com/google/cel-go/cel" + "github.com/google/cel-go/common/types" + "github.com/google/cel-go/common/types/ref" + "github.com/kubescape/node-agent/pkg/rulestate" +) + +// AccessorType is the CEL type of the "state" variable. +var AccessorType = cel.ObjectType("rulestate.Accessor") + +// Accessor is the receiver behind the CEL "state" variable: state.has(...) is a +// member call on it, not a global function. +// +// It exists because cel-go hands a function binding only its arguments -- never +// the activation -- so a global function named "state.has" could not discover +// which rule or which container it was evaluating for. Making "state" a variable +// puts that context in the receiver, where the binding can reach it. +// +// The consequence is the security property the design depends on: ruleID, the +// scope IDs and the ancestor list live in the receiver, and there is no CEL +// syntax for supplying or overriding them. A rule author therefore cannot name +// another rule's state or another container's scope -- those reads are not merely +// forbidden, they are inexpressible. +// +// One Accessor is valid for one (rule, event) pair. The rule loop rebuilds it per +// rule, because ruleID and the declared-name scopes change per rule. +type Accessor struct { + store *rulestate.Store + ruleID string + + // scopeOf maps a state name to the scope the rule declared it in. Reads take + // no scope argument: because state is rule-private, a name uniquely + // determines its scope from the rule's own stateWrites. + scopeOf map[string]armotypes.StateScope + + // scopeIDs holds this event's resolved ID for each scope. + scopeIDs map[armotypes.StateScope]string + + // ancestors is called at most once per evaluation, lazily: most rules never + // call has_ancestor, and walking the process tree is not free. + ancestors func() []uint32 + ancestorsMemo []uint32 + ancestorsDone bool + + tracker *ReadTracker + adapter types.Adapter +} + +// NewAccessor builds the receiver for one (rule, event) pair. ancestors is +// invoked lazily and at most once. +func NewAccessor( + store *rulestate.Store, + ruleID string, + scopeOf map[string]armotypes.StateScope, + scopeIDs map[armotypes.StateScope]string, + ancestors func() []uint32, + tracker *ReadTracker, + adapter types.Adapter, +) *Accessor { + if adapter == nil { + adapter = types.DefaultTypeAdapter + } + return &Accessor{ + store: store, + ruleID: ruleID, + scopeOf: scopeOf, + scopeIDs: scopeIDs, + ancestors: ancestors, + tracker: tracker, + adapter: adapter, + } +} + +func (a *Accessor) ConvertToNative(typeDesc reflect.Type) (any, error) { + if typeDesc == reflect.TypeOf(a) { + return a, nil + } + return nil, fmt.Errorf("state accessor cannot be converted to %v", typeDesc) +} + +func (a *Accessor) ConvertToType(t ref.Type) ref.Val { + if t == types.TypeType { + return AccessorType + } + return types.NewErr("state accessor cannot be converted to %v", t) +} + +func (a *Accessor) Equal(other ref.Val) ref.Val { + o, ok := other.(*Accessor) + return types.Bool(ok && o == a) +} + +func (a *Accessor) Type() ref.Type { return AccessorType } +func (a *Accessor) Value() any { return a } + +// lookup resolves one entry. A name the rule never declared is a miss rather +// than an error: load-time validation is what rejects it, and failing the whole +// predicate here would take out an otherwise working rule. +func (a *Accessor) lookup(name, key string) (*rulestate.Entry, bool) { + if a == nil || a.store == nil { + return nil, false + } + scope, ok := a.scopeOf[name] + if !ok { + return nil, false + } + scopeID, ok := a.scopeIDs[scope] + if !ok { + return nil, false + } + e, ok := a.store.Get(a.ruleID, scope, scopeID, name, key) + if !ok { + return nil, false + } + if a.tracker != nil { + a.tracker.record(e) + } + return e, true +} + +// lookupAncestor probes each ancestor PID in order and returns the first hit, so +// get_ancestor yields the NEAREST matching ancestor. +func (a *Accessor) lookupAncestor(name string) (*rulestate.Entry, bool) { + if a == nil { + return nil, false + } + for _, pid := range a.ancestorPIDs() { + if e, ok := a.lookup(name, fmt.Sprint(pid)); ok { + return e, true + } + } + return nil, false +} + +func (a *Accessor) ancestorPIDs() []uint32 { + if a.ancestorsDone { + return a.ancestorsMemo + } + a.ancestorsDone = true + if a.ancestors != nil { + a.ancestorsMemo = a.ancestors() + } + return a.ancestorsMemo +} + +// entryToMap renders an entry for CEL. Engine-stamped provenance uses reserved +// "_" keys; author values from the rule's `value:` sit alongside at top level. +// Author keys beginning with "_" are rejected at rule load, so they cannot +// shadow provenance here. +func entryToMap(e *rulestate.Entry) map[string]any { + m := map[string]any{ + "_ts": e.Timestamp, + "_eventType": string(e.EventType), + "_container": e.ScopeID, + } + if e.Process != nil { + m["_pid"] = e.Process.PID + m["_ppid"] = e.Process.PPID + m["_comm"] = e.Process.Comm + m["_pcomm"] = e.Process.Pcomm + m["_exe"] = e.Process.Path + m["_cwd"] = e.Process.Cwd + } + for k, v := range e.Value { + m[k] = v + } + return m +} + +// emptyMap is what a miss yields. Never an error: a message expression must +// degrade rather than abort evaluation of the whole rule. +func (a *Accessor) emptyMap() ref.Val { + return types.NewStringInterfaceMap(a.adapter, map[string]any{}) +} + +func (a *Accessor) entryVal(e *rulestate.Entry) ref.Val { + return types.NewStringInterfaceMap(a.adapter, entryToMap(e)) +} diff --git a/pkg/rulemanager/cel/libraries/state/readtracker.go b/pkg/rulemanager/cel/libraries/state/readtracker.go new file mode 100644 index 0000000000..ba7e4e7aac --- /dev/null +++ b/pkg/rulemanager/cel/libraries/state/readtracker.go @@ -0,0 +1,50 @@ +package state + +import ( + "sync" + + "github.com/kubescape/node-agent/pkg/rulestate" +) + +// AccessorContextKey is the eval-context key under which the caller injects the +// per-(rule, event) Accessor. It is the CEL variable name authors write as +// "state", and it is the ONLY state-related entry in the eval context. +const AccessorContextKey = "state" + +// ReadTracker records which entries a predicate actually read, so the alert can +// carry them as correlation evidence. Only hits are recorded: a miss is not +// evidence of anything. +// +// MUST be reset between rules. node-agent reuses one eval context across all +// rules for an event, so without a reset rule N inherits rule N-1's hits and +// alerts cite entries they never read. +type ReadTracker struct { + mu sync.Mutex + hits []*rulestate.Entry +} + +func (t *ReadTracker) record(e *rulestate.Entry) { + if e == nil { + return + } + t.mu.Lock() + defer t.mu.Unlock() + for _, h := range t.hits { + if h == e { + return // same entry read twice in one predicate + } + } + t.hits = append(t.hits, e) +} + +func (t *ReadTracker) Hits() []*rulestate.Entry { + t.mu.Lock() + defer t.mu.Unlock() + return append([]*rulestate.Entry(nil), t.hits...) +} + +func (t *ReadTracker) Reset() { + t.mu.Lock() + t.hits = t.hits[:0] + t.mu.Unlock() +} diff --git a/pkg/rulemanager/cel/libraries/state/statelib.go b/pkg/rulemanager/cel/libraries/state/statelib.go new file mode 100644 index 0000000000..79e50f21b0 --- /dev/null +++ b/pkg/rulemanager/cel/libraries/state/statelib.go @@ -0,0 +1,217 @@ +package state + +import ( + "github.com/google/cel-go/cel" + "github.com/google/cel-go/checker" + "github.com/google/cel-go/common/types" + "github.com/google/cel-go/common/types/ref" + "github.com/kubescape/node-agent/pkg/config" + "github.com/kubescape/node-agent/pkg/rulemanager/cel/libraries" +) + +func New(cfg config.Config) libraries.Library { + return &stateLibrary{cfg: cfg} +} + +// State registers the "state" variable and its read functions. +// +// Note what is NOT a parameter here: the store, the rule ID and the process tree. +// Reads are served by the per-(rule, event) Accessor injected into the eval +// context, so the library itself is immutable and safe to share across the +// worker pool. A library holding mutable per-evaluation state would race, since +// node-agent processes events concurrently against one shared cel.Env. +func State(cfg config.Config) cel.EnvOption { + return cel.Lib(New(cfg)) +} + +type stateLibrary struct { + cfg config.Config +} + +func (l *stateLibrary) LibraryName() string { + return "state" +} + +func (l *stateLibrary) Types() []*cel.Type { + return []*cel.Type{AccessorType} +} + +// accessorOf recovers the receiver. A missing or wrong-typed receiver means the +// caller did not seed the eval context; report it as a miss-shaped value rather +// than an error so one misconfigured rule cannot abort evaluation. +func accessorOf(v ref.Val) (*Accessor, bool) { + a, ok := v.Value().(*Accessor) + return a, ok && a != nil +} + +func stringOf(v ref.Val) (string, bool) { + s, ok := v.Value().(string) + return s, ok +} + +func (l *stateLibrary) Declarations() map[string][]cel.FunctionOpt { + return map[string][]cel.FunctionOpt{ + "has": { + cel.MemberOverload("state_has_name", + []*cel.Type{AccessorType, cel.StringType}, cel.BoolType, + cel.BinaryBinding(func(target, name ref.Val) ref.Val { + return hasImpl(target, name, types.String("")) + }), + ), + cel.MemberOverload("state_has_name_key", + []*cel.Type{AccessorType, cel.StringType, cel.StringType}, cel.BoolType, + cel.FunctionBinding(func(values ...ref.Val) ref.Val { + if len(values) != 3 { + return types.Bool(false) + } + return hasImpl(values[0], values[1], values[2]) + }), + ), + }, + "get": { + cel.MemberOverload("state_get_name", + []*cel.Type{AccessorType, cel.StringType}, cel.MapType(cel.StringType, cel.DynType), + cel.BinaryBinding(func(target, name ref.Val) ref.Val { + return getImpl(target, name, types.String("")) + }), + ), + cel.MemberOverload("state_get_name_key", + []*cel.Type{AccessorType, cel.StringType, cel.StringType}, cel.MapType(cel.StringType, cel.DynType), + cel.FunctionBinding(func(values ...ref.Val) ref.Val { + if len(values) != 3 { + return types.NewStringInterfaceMap(types.DefaultTypeAdapter, map[string]any{}) + } + return getImpl(values[0], values[1], values[2]) + }), + ), + }, + "has_ancestor": { + cel.MemberOverload("state_has_ancestor", + []*cel.Type{AccessorType, cel.StringType}, cel.BoolType, + cel.BinaryBinding(func(target, name ref.Val) ref.Val { + a, ok := accessorOf(target) + if !ok { + return types.Bool(false) + } + n, ok := stringOf(name) + if !ok { + return types.Bool(false) + } + _, hit := a.lookupAncestor(n) + return types.Bool(hit) + }), + ), + }, + "get_ancestor": { + cel.MemberOverload("state_get_ancestor", + []*cel.Type{AccessorType, cel.StringType}, cel.MapType(cel.StringType, cel.DynType), + cel.BinaryBinding(func(target, name ref.Val) ref.Val { + a, ok := accessorOf(target) + if !ok { + return types.NewStringInterfaceMap(types.DefaultTypeAdapter, map[string]any{}) + } + n, ok := stringOf(name) + if !ok { + return a.emptyMap() + } + e, hit := a.lookupAncestor(n) + if !hit { + return a.emptyMap() + } + return a.entryVal(e) + }), + ), + }, + } +} + +func hasImpl(target, name, key ref.Val) ref.Val { + a, ok := accessorOf(target) + if !ok { + return types.Bool(false) + } + n, ok := stringOf(name) + if !ok { + return types.Bool(false) + } + k, ok := stringOf(key) + if !ok { + return types.Bool(false) + } + _, hit := a.lookup(n, k) + return types.Bool(hit) +} + +func getImpl(target, name, key ref.Val) ref.Val { + a, ok := accessorOf(target) + if !ok { + return types.NewStringInterfaceMap(types.DefaultTypeAdapter, map[string]any{}) + } + n, ok := stringOf(name) + if !ok { + return a.emptyMap() + } + k, ok := stringOf(key) + if !ok { + return a.emptyMap() + } + e, hit := a.lookup(n, k) + if !hit { + return a.emptyMap() + } + return a.entryVal(e) +} + +func (l *stateLibrary) CompileOptions() []cel.EnvOption { + options := []cel.EnvOption{ + cel.Variable(AccessorContextKey, AccessorType), + } + for name, overloads := range l.Declarations() { + options = append(options, cel.Function(name, overloads...)) + } + return options +} + +func (l *stateLibrary) ProgramOptions() []cel.ProgramOption { + return []cel.ProgramOption{} +} + +func (l *stateLibrary) CostEstimator() checker.CostEstimator { + return &stateCostEstimator{cfg: l.cfg} +} + +// stateCostEstimator implements checker.CostEstimator for the 'state' library. +type stateCostEstimator struct { + cfg config.Config +} + +// EstimateCallCost keys off overloadID, not the function name: this library's +// member functions are called "has" and "get", which are far too generic to +// match on -- another library declaring a "get" would silently receive these +// costs. Unknown overloads return nil so a composite estimator can fall through +// to the library that actually owns the function. +func (e *stateCostEstimator) EstimateCallCost(function, overloadID string, target *checker.AstNode, args []checker.AstNode) *checker.CallEstimate { + var cost int64 + switch overloadID { + case "state_has_name", "state_has_name_key", "state_get_name", "state_get_name_key": + // One hash lookup under an RLock. + cost = 10 + case "state_has_ancestor", "state_get_ancestor": + // One probe per ancestor, so the depth bound is the multiplier. + depth := e.cfg.CelStateStore.AncestorMaxDepth + if depth <= 0 { + depth = 8 + } + cost = int64(10 * depth) + default: + return nil + } + return &checker.CallEstimate{CostEstimate: checker.CostEstimate{Min: uint64(cost), Max: uint64(cost)}} +} + +func (e *stateCostEstimator) EstimateSize(element checker.AstNode) *checker.SizeEstimate { + return nil // Not providing size estimates for now. +} + +var _ checker.CostEstimator = (*stateCostEstimator)(nil) +var _ libraries.Library = (*stateLibrary)(nil) diff --git a/pkg/rulemanager/cel/libraries/state/statelib_test.go b/pkg/rulemanager/cel/libraries/state/statelib_test.go new file mode 100644 index 0000000000..9969058b96 --- /dev/null +++ b/pkg/rulemanager/cel/libraries/state/statelib_test.go @@ -0,0 +1,317 @@ +package state + +import ( + "testing" + "time" + + "github.com/armosec/armoapi-go/armotypes" + "github.com/google/cel-go/cel" + "github.com/google/cel-go/common/types" + "github.com/kubescape/node-agent/pkg/config" + "github.com/kubescape/node-agent/pkg/rulestate" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const testRuleID = "R1089" + +// harness evaluates CEL expressions against a real store and a real cel.Env, so +// the tests exercise the actual dispatch path rather than the impl functions. +type harness struct { + t *testing.T + env *cel.Env + store *rulestate.Store + tracker *ReadTracker + scopeID string + ancestors []uint32 + // scopeOf stands in for the rule's own stateWrites declarations, which is + // what tells a read which scope a name lives in. + scopeOf map[string]armotypes.StateScope + now time.Time +} + +func newHarness(t *testing.T) *harness { + t.Helper() + + cfg := config.Config{} + cfg.CelStateStore = rulestate.Config{ + Enabled: true, + MaxSize: 1000, + MaxEntriesPerContainer: 100, + MaxEntriesForHost: 100, + MaxTTL: 30 * time.Minute, + AncestorMaxDepth: 8, + } + + env, err := cel.NewEnv( + cel.Variable("timestamp", cel.TimestampType), + State(cfg), + ) + require.NoError(t, err) + + return &harness{ + t: t, + env: env, + store: rulestate.NewStore(cfg.CelStateStore, rulestate.NoopMetrics{}), + tracker: &ReadTracker{}, + scopeID: "c:abc", + scopeOf: map[string]armotypes.StateScope{}, + now: time.Now(), + } +} + +// write stores an entry and declares its name as container-scoped, mirroring what +// a rule's stateWrites clause would have done at load time. +// +// ts sets the entry's logical event time, which tests compare against. ExpiresAt +// is deliberately derived from wall-clock now instead: expiry is enforced against +// time.Now(), so deriving it from a ts in the past (any fixed date literal, since +// these tests use them) would store an already-expired entry and every read would +// miss for a reason that has nothing to do with what is being tested. +func (h *harness) write(ruleID, scopeID, name, key string, ts time.Time) *rulestate.Entry { + h.t.Helper() + e := &rulestate.Entry{ + RuleID: ruleID, Name: name, Key: key, + Scope: armotypes.StateScopeContainer, ScopeID: scopeID, + EventType: armotypes.EventTypeExec, + Timestamp: ts, ExpiresAt: time.Now().Add(10 * time.Minute), + Process: &armotypes.Process{ + PID: 4471, PPID: 900, Comm: "xmrig", Pcomm: "sh", + Path: "/mnt/data/xmrig", Cwd: "/mnt/data", + }, + } + require.NoError(h.t, h.store.Set(e)) + h.scopeOf[name] = armotypes.StateScopeContainer + return e +} + +func (h *harness) accessor() *Accessor { + return NewAccessor( + h.store, testRuleID, h.scopeOf, + map[armotypes.StateScope]string{armotypes.StateScopeContainer: h.scopeID}, + func() []uint32 { return h.ancestors }, + h.tracker, + types.DefaultTypeAdapter, + ) +} + +func (h *harness) eval(expr string) any { + h.t.Helper() + ast, iss := h.env.Compile(expr) + require.NoError(h.t, iss.Err(), "expression must compile: %s", expr) + + prg, err := h.env.Program(ast) + require.NoError(h.t, err) + + out, _, err := prg.Eval(map[string]any{ + AccessorContextKey: h.accessor(), + "timestamp": h.now.Add(time.Minute), + }) + require.NoError(h.t, err, "expression must not error: %s", expr) + return out.Value() +} + +func (h *harness) evalBool(expr string) bool { + h.t.Helper() + v, ok := h.eval(expr).(bool) + require.True(h.t, ok, "expression must yield a bool: %s", expr) + return v +} + +func (h *harness) evalString(expr string) string { + h.t.Helper() + v, ok := h.eval(expr).(string) + require.True(h.t, ok, "expression must yield a string: %s", expr) + return v +} + +func TestStateHas_HitAndMiss(t *testing.T) { + h := newHarness(t) + h.write(testRuleID, "c:abc", "mount_exec", "4471", time.Now()) + + assert.True(t, h.evalBool(`state.has("mount_exec", "4471")`)) + assert.False(t, h.evalBool(`state.has("mount_exec", "9999")`)) + assert.False(t, h.evalBool(`state.has("nope", "4471")`)) +} + +func TestStateHas_OneArgFormForScopeWideMarkers(t *testing.T) { + h := newHarness(t) + h.write(testRuleID, "c:abc", "pkg_mgr_ran", "", time.Now()) + assert.True(t, h.evalBool(`state.has("pkg_mgr_ran")`)) +} + +func TestStateGet_ExposesProvenanceAndAuthorValues(t *testing.T) { + h := newHarness(t) + ts := time.Date(2026, 7, 28, 12, 0, 3, 100000000, time.UTC) + e := h.write(testRuleID, "c:abc", "mount_exec", "4471", ts) + e.Value = map[string]any{"argv": "-o pool:4444"} + + assert.Equal(t, "xmrig", h.evalString(`state.get("mount_exec", "4471")._comm`)) + assert.Equal(t, "sh", h.evalString(`state.get("mount_exec", "4471")._pcomm`)) + assert.Equal(t, "/mnt/data/xmrig", h.evalString(`state.get("mount_exec", "4471")._exe`)) + assert.Equal(t, "/mnt/data", h.evalString(`state.get("mount_exec", "4471")._cwd`)) + assert.Equal(t, "exec", h.evalString(`state.get("mount_exec", "4471")._eventType`)) + assert.Equal(t, "-o pool:4444", h.evalString(`state.get("mount_exec", "4471").argv`)) +} + +// _ts must be a CEL timestamp, not a string: the whole ordering-guard idiom is a +// comparison against the current event's time. +func TestStateGet_TimestampIsComparable(t *testing.T) { + h := newHarness(t) + h.now = time.Date(2026, 7, 28, 12, 0, 3, 0, time.UTC) + h.write(testRuleID, "c:abc", "mount_exec", "4471", h.now) + + assert.True(t, h.evalBool(`state.get("mount_exec", "4471")._ts < timestamp`), + "the remembered event happened before the current one") +} + +func TestStateGet_MissReturnsEmptyMapNotError(t *testing.T) { + h := newHarness(t) + // A message expression must degrade, not fail evaluation. + assert.Equal(t, int64(0), h.eval(`size(state.get("absent", "1"))`)) +} + +// A miss must not make a provenance access blow up the whole predicate -- this is +// the difference between a rule that under-fires and a rule that errors out. +func TestStateGet_MissTolerated_WithHasGuard(t *testing.T) { + h := newHarness(t) + assert.False(t, h.evalBool( + `state.has("absent", "1") && state.get("absent", "1")._pid == 1u`)) +} + +func TestStateHasAncestor_MatchesAnAncestorPID(t *testing.T) { + h := newHarness(t) + // nginx 900 -> sh 4471 -> curl 4530; marker is on 4471. + h.ancestors = []uint32{4471, 900, 1} + h.write(testRuleID, "c:abc", "webshell_parent", "4471", time.Now()) + + assert.True(t, h.evalBool(`state.has_ancestor("webshell_parent")`)) + assert.Equal(t, "xmrig", h.evalString(`state.get_ancestor("webshell_parent")._comm`)) +} + +func TestStateHasAncestor_NoMatchWhenNoAncestorCarriesTheMarker(t *testing.T) { + h := newHarness(t) + h.ancestors = []uint32{5000, 5001} + h.write(testRuleID, "c:abc", "webshell_parent", "4471", time.Now()) + assert.False(t, h.evalBool(`state.has_ancestor("webshell_parent")`)) +} + +func TestStateHasAncestor_WorksWithHostScope(t *testing.T) { + h := newHarness(t) + h.scopeID = rulestate.HostScopeID() + h.ancestors = []uint32{2200, 1} + h.write(testRuleID, rulestate.HostScopeID(), "sudo_ran", "2200", time.Now()) + assert.True(t, h.evalBool(`state.has_ancestor("sudo_ran")`)) +} + +// get_ancestor must return the NEAREST match, since the ancestor list is ordered +// nearest-first and a chain can carry the marker at several depths. +func TestStateGetAncestor_ReturnsNearestMatch(t *testing.T) { + h := newHarness(t) + h.ancestors = []uint32{4471, 900} + + near := h.write(testRuleID, "c:abc", "marker", "4471", time.Now()) + near.Process = &armotypes.Process{PID: 4471, Comm: "near"} + far := h.write(testRuleID, "c:abc", "marker", "900", time.Now()) + far.Process = &armotypes.Process{PID: 900, Comm: "far"} + + assert.Equal(t, "near", h.evalString(`state.get_ancestor("marker")._comm`)) +} + +func TestStateHasAncestor_EmptyAncestorListIsAMiss(t *testing.T) { + h := newHarness(t) + h.ancestors = nil + h.write(testRuleID, "c:abc", "marker", "4471", time.Now()) + assert.False(t, h.evalBool(`state.has_ancestor("marker")`)) +} + +func TestReadTracker_RecordsOnlyEntriesActuallyRead(t *testing.T) { + h := newHarness(t) + h.write(testRuleID, "c:abc", "mount_exec", "4471", time.Now()) + h.write(testRuleID, "c:abc", "unrelated", "4471", time.Now()) + + require.True(t, h.evalBool(`state.has("mount_exec", "4471")`)) + hits := h.tracker.Hits() + require.Len(t, hits, 1, "only the entry the predicate touched is evidence") + assert.Equal(t, "mount_exec", hits[0].Name) +} + +func TestReadTracker_MissesAreNotRecorded(t *testing.T) { + h := newHarness(t) + require.False(t, h.evalBool(`state.has("absent", "1")`)) + assert.Empty(t, h.tracker.Hits()) +} + +func TestReadTracker_ResetClearsBetweenRules(t *testing.T) { + h := newHarness(t) + h.write(testRuleID, "c:abc", "mount_exec", "4471", time.Now()) + require.True(t, h.evalBool(`state.has("mount_exec", "4471")`)) + require.Len(t, h.tracker.Hits(), 1) + + h.tracker.Reset() + assert.Empty(t, h.tracker.Hits(), + "without a per-rule reset, rule N inherits rule N-1's evidence") +} + +// Reading the same entry twice in one predicate must cite it once. +func TestReadTracker_DeduplicatesRepeatedReads(t *testing.T) { + h := newHarness(t) + h.write(testRuleID, "c:abc", "mount_exec", "4471", time.Now()) + require.True(t, h.evalBool( + `state.has("mount_exec", "4471") && state.get("mount_exec", "4471")._pid == 4471u`)) + assert.Len(t, h.tracker.Hits(), 1) +} + +func TestState_RuleIDIsNotExpressible(t *testing.T) { + h := newHarness(t) + // Stored under a DIFFERENT rule; the harness evaluates as R1089. There is no + // CEL syntax for naming another rule's state, which is what makes state + // rule-private. + h.write("R9999", "c:abc", "mount_exec", "4471", time.Now()) + assert.False(t, h.evalBool(`state.has("mount_exec", "4471")`)) +} + +// The neighbouring-container case, same argument as above: the scope ID comes +// from the receiver, so no expression can reach another container's bucket. +func TestState_ScopeIDIsNotExpressible(t *testing.T) { + h := newHarness(t) + h.write(testRuleID, "c:other", "mount_exec", "4471", time.Now()) + assert.False(t, h.evalBool(`state.has("mount_exec", "4471")`)) +} + +// A name the rule never declared has no scope to resolve against, so it reads as +// a miss instead of erroring. +func TestState_UndeclaredNameIsAMiss(t *testing.T) { + h := newHarness(t) + h.write(testRuleID, "c:abc", "declared", "1", time.Now()) + delete(h.scopeOf, "declared") + assert.False(t, h.evalBool(`state.has("declared", "1")`)) +} + +func TestState_ExpiredEntryIsAMiss(t *testing.T) { + h := newHarness(t) + e := h.write(testRuleID, "c:abc", "mount_exec", "4471", time.Now()) + e.ExpiresAt = time.Now().Add(-time.Second) + assert.False(t, h.evalBool(`state.has("mount_exec", "4471")`)) +} + +// The library declares member functions called "has" -- the same identifier as +// CEL's built-in has() macro. If declaring it ever shadowed the macro, +// has(event.field) would break across every existing rule. +func TestState_DoesNotShadowTheHasMacro(t *testing.T) { + env, err := cel.NewEnv( + cel.Variable("m", cel.MapType(cel.StringType, cel.StringType)), + State(config.Config{}), + ) + require.NoError(t, err) + + ast, iss := env.Compile(`has(m.present)`) + require.NoError(t, iss.Err(), "the has() macro must still parse alongside state.has") + + prg, err := env.Program(ast) + require.NoError(t, err) + + out, _, err := prg.Eval(map[string]any{"m": map[string]string{"present": "x"}}) + require.NoError(t, err) + assert.Equal(t, true, out.Value()) +} From 54a5b294380da00b201f45851701d74d4c755579 Mon Sep 17 00:00:00 2001 From: Ben Date: Thu, 30 Jul 2026 14:27:32 +0300 Subject: [PATCH 06/17] feat(rulemanager): validate and execute stateWrites clauses Writes run AFTER the predicate, so a predicate only ever sees state from earlier events -- otherwise a rule reading and writing the same name on one event type would trivially satisfy itself. To make that possible the per-rule body of the event loop is now its own function, evaluateRuleAndAlert. Its early exits were continues, which would have skipped the write clause whenever an alert was suppressed -- silently breaking the NEXT leg of the chain. As returns, the caller still runs the writes. Cooldown in particular must not suppress a write: writes are evidence gathering. Validation is at load, not runtime: an unknown event type, the `all` binding wildcard, a bad or non-positive TTL, an identity scope (operator-only) or a reserved _-prefixed name or value key fails loudly instead of producing a rule that silently never matches. A malformed clause degrades that one rule to non-correlating rather than breaking evaluation for the rest of the CRD. ValidateAll also rejects one name declared in two scopes. Reads take no scope argument -- they infer it from the name -- so that would make every read of the name ambiguous. The same name across several event types in one scope is the normal bidirectional idiom and stays legal. isSupportedEventType now also considers stateWrites event types; without that, write-only legs are filtered out before the loop and no chain ever forms. Two deliberate choices worth recording: Compilation happens per event, not once at rule load. It is pure string and duration parsing -- the CEL expressions are compiled and cached by the evaluator, keyed by expression text -- and it only runs for rules that declare writes, a small minority. Caching it on the Rule would need invalidation on every CRD change, and rules reach the loop by two paths of which only one populates load-time derived fields, so the cached field would be silently empty for host rules. utils.IsValidEventType is new and narrower than armotypes.IsKnownEventType, which spans both engines: k8s-admission is a real armotypes event type node-agent never emits, so a node-agent rule naming it has to be rejected at load. Also brings forward the celStateStore config field and the five state metrics from Task 8, since the executor and cost estimator need them to compile. Signed-off-by: Ben --- docs/features/cel-rule-state-store.md | 79 ++++- .../metrics_manager_interface.go | 11 + pkg/metricsmanager/metrics_manager_mock.go | 6 + pkg/metricsmanager/metrics_manager_noop.go | 6 + .../otel/otel_metrics_manager.go | 41 +++ pkg/metricsmanager/prometheus/prometheus.go | 52 +++ pkg/rulemanager/cel/cel.go | 39 +++ pkg/rulemanager/cel/cel_interface.go | 2 + pkg/rulemanager/rule_manager.go | 300 +++++++++++------ pkg/rulemanager/statecontext.go | 118 +++++++ pkg/rulemanager/statecontext_test.go | 181 ++++++++++ pkg/rulemanager/statewrites/executor.go | 198 +++++++++++ pkg/rulemanager/statewrites/executor_test.go | 313 ++++++++++++++++++ pkg/rulemanager/statewrites/validate.go | 152 +++++++++ pkg/rulemanager/statewrites/validate_test.go | 203 ++++++++++++ pkg/utils/events.go | 23 ++ 16 files changed, 1621 insertions(+), 103 deletions(-) create mode 100644 pkg/rulemanager/statecontext.go create mode 100644 pkg/rulemanager/statecontext_test.go create mode 100644 pkg/rulemanager/statewrites/executor.go create mode 100644 pkg/rulemanager/statewrites/executor_test.go create mode 100644 pkg/rulemanager/statewrites/validate.go create mode 100644 pkg/rulemanager/statewrites/validate_test.go diff --git a/docs/features/cel-rule-state-store.md b/docs/features/cel-rule-state-store.md index 42f2c8cec4..b937716f66 100644 --- a/docs/features/cel-rule-state-store.md +++ b/docs/features/cel-rule-state-store.md @@ -8,10 +8,81 @@ delete of a pod — cannot be expressed at all. Design: `shared-designs-and-docs/projects/2026-07-28-cel-rule-state-store/spec.md`. -> **Status: under construction.** This page documents what has landed. The read -> functions below exist and are tested, but nothing populates the store yet — -> writes (`stateWrites:`) are still to come, so in a running agent every read is -> currently a miss. +> **Status: under construction.** Reads and writes both work. Still to land: +> correlation evidence on the emitted alert, and scope purge on container removal. + +## Writing state + +A rule declares what it remembers in a `stateWrites:` block. Each entry is driven +by one event type, which **need not** be an event type the rule alerts on: + +```yaml +id: R1089 +stateWrites: + - eventType: exec # the stream that drives this write + when: "" # optional; absent means always write + scope: container # container | pod | node + name: mount_exec # a literal, never an expression + key: "string(event.pid)" # optional CEL string: who the fact is about + value: # optional extras, CEL expression strings + argv: "event.args" + ttl: 10m # clamped to the configured maxTtl +expressions: + ruleExpression: + - eventType: network # alerts on a DIFFERENT stream + expression: | + state.has("mount_exec", string(event.pid)) && !net.is_private_ip(event.dstIP) +``` + +`name` is a literal rather than an expression on purpose: it stays statically +analysable and is safe to use as a metric label. + +### Writes are declarative, not a CEL setter + +There is no `state.set(...)` function, deliberately. A setter inside a predicate +would be skipped by boolean short-circuiting, could be reordered by the static +optimiser, and could never express "remember this **without** alerting" — which +is exactly what the first leg of a cross-event rule needs. + +### Writes run after the predicate + +For a given event, the rule's predicate is evaluated first and the writes second. +So a predicate only ever sees state from **earlier** events. Otherwise a rule +that reads and writes the same name on the same event type would satisfy itself +from its own write. + +### What suppresses a write + +| Condition | Writes still run? | +|---|---| +| Rule disabled, or does not apply to this context | no | +| `profileDependency: Required` and no profile | no | +| Pre-filter excluded the event | no | +| Rule policy suppressed it | no | +| **Alert cooldown** | **yes** | +| **Predicate returned false** | **yes** | +| Store at capacity | no — write rejected, `state_write_rejected_total` | + +Cooldown suppresses the *alert*, never the write: writes are evidence gathering, +and dropping them would break the next leg of the chain. + +### Validation happens at load + +An unknown event type, `eventType: all` (a binding wildcard, not a stream), +`scope: identity` (operator-only), a bad or non-positive TTL, or a `_`-prefixed +name or value key is rejected when the rule loads. Every one of those mistakes +would otherwise produce a rule that loads cleanly and silently never fires. + +A rule with a malformed clause is degraded to non-correlating and logged; it does +not stop the other rules in the CRD from evaluating. + +### Bounds + +Over-capacity writes are **rejected, never satisfied by evicting** another +entry — eviction would let one container disable detection for its neighbours. +The per-scope cap is exact; the node-wide ceiling is approximate under +concurrency. Host processes share one `c:__host__` bucket with its own larger cap, +since it holds the whole node's process space and gets no removal purge. ## Reading state diff --git a/pkg/metricsmanager/metrics_manager_interface.go b/pkg/metricsmanager/metrics_manager_interface.go index c40dc3d315..84b1b579a4 100644 --- a/pkg/metricsmanager/metrics_manager_interface.go +++ b/pkg/metricsmanager/metrics_manager_interface.go @@ -63,4 +63,15 @@ type MetricsManager interface { // Alert suppression funnel — counts how many alerts were dropped and why. ReportAlertSuppressed(ruleID, reason string) + + // CEL rule state store. Labelled by ruleID only — never by state key, which is + // unbounded cardinality. + // + // ReportStateWriteRejected is the alert-worthy one: it means a rule is being + // silently starved of the state it needs to correlate. + ReportStateWrite(ruleID, result string) + ReportStateWriteRejected(ruleID, reason string) + ReportStateExpired(n int) + ReportStatePurged(n int) + ReportStateEntries(scope string, n int) } diff --git a/pkg/metricsmanager/metrics_manager_mock.go b/pkg/metricsmanager/metrics_manager_mock.go index d33e06428b..df1b388b9d 100644 --- a/pkg/metricsmanager/metrics_manager_mock.go +++ b/pkg/metricsmanager/metrics_manager_mock.go @@ -97,3 +97,9 @@ func (m *MetricsMock) ObserveSBOMScanDuration(_ string, _ time.Duration) func (m *MetricsMock) ReportSBOMScannerRestart() {} func (m *MetricsMock) SetSBOMScannerReady(_ bool) {} func (m *MetricsMock) ReportAlertSuppressed(_, _ string) {} + +func (m *MetricsMock) ReportStateWrite(_, _ string) {} +func (m *MetricsMock) ReportStateWriteRejected(_, _ string) {} +func (m *MetricsMock) ReportStateExpired(_ int) {} +func (m *MetricsMock) ReportStatePurged(_ int) {} +func (m *MetricsMock) ReportStateEntries(_ string, _ int) {} diff --git a/pkg/metricsmanager/metrics_manager_noop.go b/pkg/metricsmanager/metrics_manager_noop.go index a8533de845..88e3a77362 100644 --- a/pkg/metricsmanager/metrics_manager_noop.go +++ b/pkg/metricsmanager/metrics_manager_noop.go @@ -53,3 +53,9 @@ func (m *MetricsNoop) ObserveSBOMScanDuration(_ string, _ time.Duration) func (m *MetricsNoop) ReportSBOMScannerRestart() {} func (m *MetricsNoop) SetSBOMScannerReady(_ bool) {} func (m *MetricsNoop) ReportAlertSuppressed(_, _ string) {} + +func (m *MetricsNoop) ReportStateWrite(_, _ string) {} +func (m *MetricsNoop) ReportStateWriteRejected(_, _ string) {} +func (m *MetricsNoop) ReportStateExpired(_ int) {} +func (m *MetricsNoop) ReportStatePurged(_ int) {} +func (m *MetricsNoop) ReportStateEntries(_ string, _ int) {} diff --git a/pkg/metricsmanager/otel/otel_metrics_manager.go b/pkg/metricsmanager/otel/otel_metrics_manager.go index 784c72eb20..f22ecfca09 100644 --- a/pkg/metricsmanager/otel/otel_metrics_manager.go +++ b/pkg/metricsmanager/otel/otel_metrics_manager.go @@ -75,6 +75,13 @@ type OTELMetricsManager struct { // Alert suppression funnel alertSuppressedTotal metric.Int64Counter + // CEL rule state store + stateWritesTotal metric.Int64Counter + stateWriteRejectedTotal metric.Int64Counter + stateExpiredTotal metric.Int64Counter + statePurgedTotal metric.Int64Counter + stateEntries metric.Float64Gauge + // Live container count — incremented on start, decremented on stop. // Exposed as node_agent.container.count observable gauge. containerCount atomic.Int64 @@ -227,6 +234,16 @@ func NewOTELMetricsManager(ownContainerID string) *OTELMetricsManager { m.alertSuppressedTotal = mustCounter("node_agent.alert.suppressed.total", "Total alerts suppressed before delivery, labeled by rule_id and reason") + m.stateWritesTotal = mustCounter("node_agent.state.writes.total", + "Total CEL rule state entries written, labeled by rule_id") + m.stateWriteRejectedTotal = mustCounter("node_agent.state.write.rejected.total", + "Total CEL rule state writes rejected; a rule is being starved of the state it needs to correlate") + m.stateExpiredTotal = mustCounter("node_agent.state.expired.total", + "Total CEL rule state entries reclaimed by TTL expiry") + m.statePurgedTotal = mustCounter("node_agent.state.purged.total", + "Total CEL rule state entries dropped by scope purge, e.g. container removal") + m.stateEntries = mustGauge("node_agent.state.entries", + "Current CEL rule state entries, labeled by scope") registerResourceMetrics(meter, &m.containerCount, ownContainerID) @@ -525,3 +542,27 @@ func (m *OTELMetricsManager) suppressedOption(ruleID, reason string) metric.Meas func (m *OTELMetricsManager) ReportAlertSuppressed(ruleID, reason string) { m.alertSuppressedTotal.Add(context.Background(), 1, m.suppressedOption(ruleID, reason)) } + +// The state counters reuse suppressedOption: it caches a (ruleID, reason) +// attribute set, which is exactly the label pair these need. Labelling by ruleID +// only is deliberate -- a state key is unbounded cardinality. +func (m *OTELMetricsManager) ReportStateWrite(ruleID, result string) { + m.stateWritesTotal.Add(context.Background(), 1, m.suppressedOption(ruleID, result)) +} + +func (m *OTELMetricsManager) ReportStateWriteRejected(ruleID, reason string) { + m.stateWriteRejectedTotal.Add(context.Background(), 1, m.suppressedOption(ruleID, reason)) +} + +func (m *OTELMetricsManager) ReportStateExpired(n int) { + m.stateExpiredTotal.Add(context.Background(), int64(n)) +} + +func (m *OTELMetricsManager) ReportStatePurged(n int) { + m.statePurgedTotal.Add(context.Background(), int64(n)) +} + +func (m *OTELMetricsManager) ReportStateEntries(scope string, n int) { + m.stateEntries.Record(context.Background(), float64(n), + metric.WithAttributes(attribute.String("scope", scope))) +} diff --git a/pkg/metricsmanager/prometheus/prometheus.go b/pkg/metricsmanager/prometheus/prometheus.go index 36b4e11986..9acb4fafc7 100644 --- a/pkg/metricsmanager/prometheus/prometheus.go +++ b/pkg/metricsmanager/prometheus/prometheus.go @@ -105,6 +105,13 @@ type PrometheusMetric struct { // Alert suppression funnel alertSuppressedCounter *prometheus.CounterVec + // CEL rule state store + stateWritesCounter *prometheus.CounterVec + stateWriteRejectedCounter *prometheus.CounterVec + stateExpiredCounter prometheus.Counter + statePurgedCounter prometheus.Counter + stateEntriesGauge *prometheus.GaugeVec + // Cache to avoid allocating Labels maps on every call ruleCounterCache map[string]prometheus.Counter rulePrefilteredCounterCache map[string]prometheus.Counter @@ -377,6 +384,26 @@ func NewPrometheusMetric() *PrometheusMetric { Name: "node_agent_alert_suppressed_total", Help: "Total alerts suppressed before delivery, labeled by rule_id and reason", }, []string{prometheusRuleIdLabel, "reason"}), + stateWritesCounter: promauto.NewCounterVec(prometheus.CounterOpts{ + Name: "node_agent_state_writes_total", + Help: "Total CEL rule state entries written, labeled by rule_id", + }, []string{prometheusRuleIdLabel, "result"}), + stateWriteRejectedCounter: promauto.NewCounterVec(prometheus.CounterOpts{ + Name: "node_agent_state_write_rejected_total", + Help: "Total CEL rule state writes rejected; a rule is being starved of the state it needs to correlate", + }, []string{prometheusRuleIdLabel, "reason"}), + stateExpiredCounter: promauto.NewCounter(prometheus.CounterOpts{ + Name: "node_agent_state_expired_total", + Help: "Total CEL rule state entries reclaimed by TTL expiry", + }), + statePurgedCounter: promauto.NewCounter(prometheus.CounterOpts{ + Name: "node_agent_state_purged_total", + Help: "Total CEL rule state entries dropped by scope purge, e.g. container removal", + }), + stateEntriesGauge: promauto.NewGaugeVec(prometheus.GaugeOpts{ + Name: "node_agent_state_entries", + Help: "Current CEL rule state entries, labeled by scope", + }, []string{"scope"}), sbomScanDuration: promauto.NewHistogramVec(prometheus.HistogramOpts{ Name: "sbom_scan_duration_seconds", Help: "SBOM scan duration in seconds", @@ -467,6 +494,11 @@ func (p *PrometheusMetric) Destroy() { prometheus.Unregister(p.programPerCpuUsageGauge) prometheus.Unregister(p.sbomScanCounter) prometheus.Unregister(p.alertSuppressedCounter) + prometheus.Unregister(p.stateWritesCounter) + prometheus.Unregister(p.stateWriteRejectedCounter) + prometheus.Unregister(p.stateExpiredCounter) + prometheus.Unregister(p.statePurgedCounter) + prometheus.Unregister(p.stateEntriesGauge) prometheus.Unregister(p.sbomScanDuration) prometheus.Unregister(p.sbomRestarts) prometheus.Unregister(p.sbomReady) @@ -752,3 +784,23 @@ func (p *PrometheusMetric) SetSBOMScannerReady(ready bool) { func (p *PrometheusMetric) ReportAlertSuppressed(ruleID, reason string) { p.alertSuppressedCounter.WithLabelValues(ruleID, reason).Inc() } + +func (p *PrometheusMetric) ReportStateWrite(ruleID, result string) { + p.stateWritesCounter.WithLabelValues(ruleID, result).Inc() +} + +func (p *PrometheusMetric) ReportStateWriteRejected(ruleID, reason string) { + p.stateWriteRejectedCounter.WithLabelValues(ruleID, reason).Inc() +} + +func (p *PrometheusMetric) ReportStateExpired(n int) { + p.stateExpiredCounter.Add(float64(n)) +} + +func (p *PrometheusMetric) ReportStatePurged(n int) { + p.statePurgedCounter.Add(float64(n)) +} + +func (p *PrometheusMetric) ReportStateEntries(scope string, n int) { + p.stateEntriesGauge.WithLabelValues(scope).Set(float64(n)) +} diff --git a/pkg/rulemanager/cel/cel.go b/pkg/rulemanager/cel/cel.go index 1b28500518..2e787bc23d 100644 --- a/pkg/rulemanager/cel/cel.go +++ b/pkg/rulemanager/cel/cel.go @@ -213,6 +213,45 @@ func (c *CEL) evaluateProgramWithContext(expression string, evalContext map[stri return out, nil } +// EvaluateBoolExpressionWithContext evaluates a boolean expression against an +// already-built context. State-write guards use it so the guard sees exactly the +// same event view -- and the same state -- as the predicate did. +func (c *CEL) EvaluateBoolExpressionWithContext(evalContext map[string]any, expression string) (bool, error) { + out, err := c.evaluateProgramWithContext(expression, evalContext) + if err != nil { + return false, err + } + // A nil program means compilation failed and was cached as such. + if out == nil { + return false, nil + } + boolVal, ok := out.Value().(bool) + if !ok { + return false, fmt.Errorf("expression returned %T, expected bool", out.Value()) + } + return boolVal, nil +} + +// EvaluateStringExpressionWithContext evaluates expr against an already-built +// context. Message and uniqueId expressions must reuse the predicate's context so +// state.get() resolves against the same entries -- and so uniqueId can be derived +// from the join key, which is what lets rulecooldown collapse the two legs of a +// bidirectional rule into one alert. +func (c *CEL) EvaluateStringExpressionWithContext(evalContext map[string]any, expression string) (string, error) { + out, err := c.evaluateProgramWithContext(expression, evalContext) + if err != nil { + return "", err + } + if out == nil { + return "", nil + } + strVal, ok := out.Value().(string) + if !ok { + return "", fmt.Errorf("expression returned %T, expected string", out.Value()) + } + return strVal, nil +} + func (c *CEL) EvaluateRule(event *events.EnrichedEvent, expressions []typesv1.RuleExpression) (bool, error) { eventType := event.Event.GetEventType() evalContext := c.CreateEvalContext(event) diff --git a/pkg/rulemanager/cel/cel_interface.go b/pkg/rulemanager/cel/cel_interface.go index 935c7b830f..8a41f0f3a6 100644 --- a/pkg/rulemanager/cel/cel_interface.go +++ b/pkg/rulemanager/cel/cel_interface.go @@ -11,6 +11,8 @@ type RuleEvaluator interface { EvaluateRule(event *events.EnrichedEvent, expressions []typesv1.RuleExpression) (bool, error) EvaluateRuleWithContext(evalContext map[string]any, eventType utils.EventType, expressions []typesv1.RuleExpression) (bool, error) EvaluateExpression(event *events.EnrichedEvent, expression string) (string, error) + EvaluateBoolExpressionWithContext(evalContext map[string]any, expression string) (bool, error) + EvaluateStringExpressionWithContext(evalContext map[string]any, expression string) (string, error) CreateEvalContext(event *events.EnrichedEvent) map[string]any RegisterHelper(function cel.EnvOption) error RegisterCustomType(eventType utils.EventType, obj interface{}) error diff --git a/pkg/rulemanager/rule_manager.go b/pkg/rulemanager/rule_manager.go index a25e5889db..c92dc1fecb 100644 --- a/pkg/rulemanager/rule_manager.go +++ b/pkg/rulemanager/rule_manager.go @@ -27,22 +27,25 @@ import ( "github.com/kubescape/node-agent/pkg/metricsmanager" "github.com/kubescape/node-agent/pkg/objectcache" "github.com/kubescape/node-agent/pkg/objectcache/containerprofilecache" + "github.com/kubescape/node-agent/pkg/otelsetup" "github.com/kubescape/node-agent/pkg/processtree" bindingcache "github.com/kubescape/node-agent/pkg/rulebindingmanager" "github.com/kubescape/node-agent/pkg/rulemanager/cel" + "github.com/kubescape/node-agent/pkg/rulemanager/cel/libraries/state" "github.com/kubescape/node-agent/pkg/rulemanager/prefilter" "github.com/kubescape/node-agent/pkg/rulemanager/profilehelper" "github.com/kubescape/node-agent/pkg/rulemanager/ruleadapters" "github.com/kubescape/node-agent/pkg/rulemanager/rulecooldown" - "github.com/kubescape/node-agent/pkg/otelsetup" + "github.com/kubescape/node-agent/pkg/rulemanager/statewrites" "github.com/kubescape/node-agent/pkg/rulemanager/types" typesv1 "github.com/kubescape/node-agent/pkg/rulemanager/types/v1" + "github.com/kubescape/node-agent/pkg/rulestate" "github.com/kubescape/node-agent/pkg/utils" - corev1 "k8s.io/api/core/v1" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/codes" "go.opentelemetry.io/otel/trace" + corev1 "k8s.io/api/core/v1" ) const ( @@ -72,6 +75,8 @@ type RuleManager struct { detectorManager *detectors.DetectorManager alertLogDedup *expirable.LRU[string, struct{}] alertLogDedupMu sync.Mutex + stateStore *rulestate.Store + stateWrites *statewrites.Executor } var _ RuleManagerClient = (*RuleManager)(nil) @@ -118,6 +123,12 @@ func CreateRuleManager( alertLogDedup: expirable.NewLRU[string, struct{}](1000, nil, 60*time.Second), } + // The state store lives here rather than in main.go because the rule loop is + // its only writer and reader. Sweeping runs for the manager's lifetime. + r.stateStore = rulestate.NewStore(cfg.CelStateStore, newStateMetrics(metrics)) + r.stateWrites = statewrites.NewExecutor(r.stateStore, celEvaluator, newStateMetrics(metrics)) + go r.stateStore.Run(ctx) + // Compile the initial projection spec and start a goroutine that // recompiles whenever rule bindings change. r.recompileProjectionSpec() @@ -322,6 +333,9 @@ func (rm *RuleManager) ReportEnrichedEvent(enrichedEvent *events.EnrichedEvent) var eventFields prefilter.EventFields var evalContext map[string]any + // One tracker per event, reset per rule. Allocating per event rather than per + // rule keeps the common no-state path to a single allocation. + stateTracker := &state.ReadTracker{} for _, rule := range rules { if !rule.Enabled { @@ -339,7 +353,15 @@ func (rm *RuleManager) ReportEnrichedEvent(enrichedEvent *events.EnrichedEvent) } ruleExpressions := rm.getRuleExpressions(rule, eventType) - if len(ruleExpressions) == 0 { + + // Compile the write clause before the no-expressions bail-out below: a rule + // may legitimately have NO ruleExpression for this event type and still need + // to remember something. That is what makes write-without-alerting -- the + // first leg of every cross-event rule -- possible. + stateWrites, stateScopes := rm.compileStateWrites(&rule) + writesThisEvent := hasWriteFor(stateWrites, eventType) + + if len(ruleExpressions) == 0 && !writesThisEvent { continue } @@ -363,112 +385,180 @@ func (rm *RuleManager) ReportEnrichedEvent(enrichedEvent *events.EnrichedEvent) evalContext = rm.celEvaluator.CreateEvalContext(enrichedEvent) } - startTime := time.Now() - var shouldAlert bool - var err error - pprof.Do(context.Background(), pprof.Labels("rule", rule.ID), func(_ context.Context) { - shouldAlert, err = rm.celEvaluator.EvaluateRuleWithContext(evalContext, eventType, ruleExpressions) - }) - evaluationTime := time.Since(startTime) - // Slow-path tracing: only emit a span when evaluation exceeded the threshold. - // This protects the hot path from unconditional tracing overhead on millions of events/sec. - // errCtx tracks the spanned context (when a rule.evaluate span fires) so the - // failure log below inherits its trace_id/span_id — otherwise falls back to rm.ctx. - errCtx := rm.ctx - if evaluationTime >= otelsetup.SlowEvalThreshold() { - evalCtx, span := otelsetup.Tracer().Start(rm.ctx, "rule.evaluate", - trace.WithAttributes( - attribute.String("rule.id", rule.ID), - attribute.String("event.type", string(eventType)), - attribute.String("container.id", enrichedEvent.ContainerID), - attribute.Float64("eval.duration_ms", float64(evaluationTime.Milliseconds())), - attribute.Bool("alert_fired", shouldAlert), - )) - if err != nil { - span.SetStatus(codes.Error, err.Error()) - } - rm.metrics.ReportRuleEvaluationTime(evalCtx, rule.ID, eventType, evaluationTime) - span.End() - errCtx = evalCtx - } else { - rm.metrics.ReportRuleEvaluationTime(rm.ctx, rule.ID, eventType, evaluationTime) + // Rebuild the state receiver per rule: it carries the rule ID and the + // rule's own declared-name scopes, so it cannot be shared between rules. + // Resetting the tracker here is what stops rule N citing rule N-1's + // entries as its own evidence. + stateTracker.Reset() + rm.seedStateContext(evalContext, &rule, enrichedEvent, stateScopes, stateTracker) + + // From here on, alerting must not skip the write clause: writes are + // evidence gathering, and dropping them because an alert was suppressed + // would break the NEXT leg of the chain. The predicate and alert path is + // therefore its own function -- its early exits return, and the writes + // below still run. + if len(ruleExpressions) > 0 { + rm.evaluateRuleAndAlert(evaluateArgs{ + rule: rule, + ruleExpressions: ruleExpressions, + enrichedEvent: enrichedEvent, + evalContext: evalContext, + eventType: eventType, + namespace: namespace, + pod: pod, + details: details, + apChecksum: apChecksum, + tracker: stateTracker, + }) + } + + if writesThisEvent { + // Writes run AFTER the predicate, so a predicate only ever sees state + // from EARLIER events. Otherwise a rule that reads and writes the same + // name on the same event type would trivially satisfy itself. + rm.stateWrites.Apply(stateWrites, rule.ID, enrichedEvent, evalContext, + cel.ResolveEventTime(enrichedEvent)) } + rm.metrics.ReportRuleProcessed(rule.ID) + } +} + +type evaluateArgs struct { + rule typesv1.Rule + ruleExpressions []typesv1.RuleExpression + enrichedEvent *events.EnrichedEvent + evalContext map[string]any + eventType utils.EventType + namespace string + pod string + details string + apChecksum string + tracker *state.ReadTracker +} + +// evaluateRuleAndAlert runs one rule's predicate and emits an alert if it fires. +// +// Split out of the rule loop so that every early exit in here is a return rather +// than a continue, which leaves the caller free to run the rule's state writes +// afterwards regardless of whether an alert was emitted or suppressed. +func (rm *RuleManager) evaluateRuleAndAlert(a evaluateArgs) { + rule := a.rule + enrichedEvent := a.enrichedEvent + evalContext := a.evalContext + eventType := a.eventType + namespace := a.namespace + pod := a.pod + apChecksum := a.apChecksum + + startTime := time.Now() + var shouldAlert bool + var err error + pprof.Do(context.Background(), pprof.Labels("rule", rule.ID), func(_ context.Context) { + shouldAlert, err = rm.celEvaluator.EvaluateRuleWithContext(evalContext, eventType, a.ruleExpressions) + }) + evaluationTime := time.Since(startTime) + // Slow-path tracing: only emit a span when evaluation exceeded the threshold. + // This protects the hot path from unconditional tracing overhead on millions of events/sec. + // errCtx tracks the spanned context (when a rule.evaluate span fires) so the + // failure log below inherits its trace_id/span_id — otherwise falls back to rm.ctx. + errCtx := rm.ctx + if evaluationTime >= otelsetup.SlowEvalThreshold() { + evalCtx, span := otelsetup.Tracer().Start(rm.ctx, "rule.evaluate", + trace.WithAttributes( + attribute.String("rule.id", rule.ID), + attribute.String("event.type", string(eventType)), + attribute.String("container.id", enrichedEvent.ContainerID), + attribute.Float64("eval.duration_ms", float64(evaluationTime.Milliseconds())), + attribute.Bool("alert_fired", shouldAlert), + )) if err != nil { - logger.L().Ctx(errCtx).Error("RuleManager.ReportEnrichedEvent - failed to evaluate rule", helpers.Error(err), helpers.String("rule", rule.ID), helpers.String("eventType", string(eventType))) - rm.metrics.ReportAlertSuppressed(rule.ID, "eval_error") - continue + span.SetStatus(codes.Error, err.Error()) } + rm.metrics.ReportRuleEvaluationTime(evalCtx, rule.ID, eventType, evaluationTime) + span.End() + errCtx = evalCtx + } else { + rm.metrics.ReportRuleEvaluationTime(rm.ctx, rule.ID, eventType, evaluationTime) + } - if shouldAlert { - state := rule.State - if eventType == utils.HTTPEventType { // TODO: Manage state evaluation in a better way (this is abuse of the state map, we need a better way to pass payloads from rules.) - state = rm.evaluateHTTPPayloadState(rule.State, enrichedEvent) - } - rm.metrics.ReportRuleAlert(rule.ID) - message, uniqueID, err := rm.getUniqueIdAndMessage(enrichedEvent, rule) - if err != nil { - logger.L().Error("RuleManager - failed to get unique ID and message", helpers.Error(err)) - continue - } + if err != nil { + logger.L().Ctx(errCtx).Error("RuleManager.ReportEnrichedEvent - failed to evaluate rule", helpers.Error(err), helpers.String("rule", rule.ID), helpers.String("eventType", string(eventType))) + rm.metrics.ReportAlertSuppressed(rule.ID, "eval_error") + return + } - if shouldCooldown, _ := rm.ruleCooldown.ShouldCooldown(uniqueID, enrichedEvent.ContainerID, rule.ID); shouldCooldown { - rm.metrics.ReportAlertSuppressed(rule.ID, "cooldown") - continue - } + if !shouldAlert { + return + } - // Emit OTEL log after cooldown so suppressed alerts are not recorded. - // Dedup key includes eventType to avoid collapsing distinct alert types. - dedupKey := rule.ID + "|" + enrichedEvent.ContainerID + "|" + string(eventType) - rm.alertLogDedupMu.Lock() - alreadySeen := rm.alertLogDedup.Contains(dedupKey) - if !alreadySeen { - rm.alertLogDedup.Add(dedupKey, struct{}{}) - } - rm.alertLogDedupMu.Unlock() - if !alreadySeen { - var image, containerName string - if enrichable, ok := enrichedEvent.Event.(utils.EnrichEvent); ok { - image = enrichable.GetContainerImage() - containerName = enrichable.GetContainer() - } - alertCtx, alertSpan := otelsetup.Tracer().Start(rm.ctx, "rule.alert", - trace.WithAttributes( - attribute.String("rule.id", rule.ID), - attribute.String("rule.name", rule.Name), - attribute.String("k8s.namespace.name", namespace), - attribute.String("k8s.pod.name", pod), - attribute.String("container.id", enrichedEvent.ContainerID), - attribute.String("event.type", string(eventType)), - )) - otelsetup.EmitAlertLogRecord(alertCtx, otelsetup.AlertLogAttrs{ - RuleID: rule.ID, - AlertType: rule.Name, - ContainerID: enrichedEvent.ContainerID, - ContainerName: containerName, - Namespace: namespace, - PodName: pod, - Image: image, - EventType: string(eventType), - }) - alertSpan.End() - } + // ruleState, not "state": the local would otherwise shadow the state + // library package imported for the read tracker. + ruleState := rule.State + if eventType == utils.HTTPEventType { // TODO: Manage state evaluation in a better way (this is abuse of the state map, we need a better way to pass payloads from rules.) + ruleState = rm.evaluateHTTPPayloadState(rule.State, enrichedEvent) + } + rm.metrics.ReportRuleAlert(rule.ID) + message, uniqueID, err := rm.getUniqueIdAndMessage(enrichedEvent, rule) + if err != nil { + logger.L().Error("RuleManager - failed to get unique ID and message", helpers.Error(err)) + return + } - ruleFailure := rm.ruleFailureCreator.CreateRuleFailure(rule, enrichedEvent, rm.objectCache, message, uniqueID, apChecksum, state) - if ruleFailure == nil { - logger.L().Error("RuleManager - failed to create rule failure", helpers.String("rule", rule.Name), - helpers.String("message", message), - helpers.String("uniqueID", uniqueID), - helpers.String("enrichedEvent.EventType", string(eventType)), - ) - continue - } + if shouldCooldown, _ := rm.ruleCooldown.ShouldCooldown(uniqueID, enrichedEvent.ContainerID, rule.ID); shouldCooldown { + rm.metrics.ReportAlertSuppressed(rule.ID, "cooldown") + return + } - ruleFailure.SetWorkloadDetails(details) - rm.exporter.SendRuleAlert(ruleFailure) - } - rm.metrics.ReportRuleProcessed(rule.ID) + // Emit OTEL log after cooldown so suppressed alerts are not recorded. + // Dedup key includes eventType to avoid collapsing distinct alert types. + dedupKey := rule.ID + "|" + enrichedEvent.ContainerID + "|" + string(eventType) + rm.alertLogDedupMu.Lock() + alreadySeen := rm.alertLogDedup.Contains(dedupKey) + if !alreadySeen { + rm.alertLogDedup.Add(dedupKey, struct{}{}) + } + rm.alertLogDedupMu.Unlock() + if !alreadySeen { + var image, containerName string + if enrichable, ok := enrichedEvent.Event.(utils.EnrichEvent); ok { + image = enrichable.GetContainerImage() + containerName = enrichable.GetContainer() + } + alertCtx, alertSpan := otelsetup.Tracer().Start(rm.ctx, "rule.alert", + trace.WithAttributes( + attribute.String("rule.id", rule.ID), + attribute.String("rule.name", rule.Name), + attribute.String("k8s.namespace.name", namespace), + attribute.String("k8s.pod.name", pod), + attribute.String("container.id", enrichedEvent.ContainerID), + attribute.String("event.type", string(eventType)), + )) + otelsetup.EmitAlertLogRecord(alertCtx, otelsetup.AlertLogAttrs{ + RuleID: rule.ID, + AlertType: rule.Name, + ContainerID: enrichedEvent.ContainerID, + ContainerName: containerName, + Namespace: namespace, + PodName: pod, + Image: image, + EventType: string(eventType), + }) + alertSpan.End() } + + ruleFailure := rm.ruleFailureCreator.CreateRuleFailure(rule, enrichedEvent, rm.objectCache, message, uniqueID, apChecksum, ruleState) + if ruleFailure == nil { + logger.L().Error("RuleManager - failed to create rule failure", helpers.String("rule", rule.Name), + helpers.String("message", message), + helpers.String("uniqueID", uniqueID), + helpers.String("enrichedEvent.EventType", string(eventType)), + ) + return + } + + ruleFailure.SetWorkloadDetails(a.details) + rm.exporter.SendRuleAlert(ruleFailure) } func (rm *RuleManager) enrichEventWithContext(enrichedEvent *events.EnrichedEvent) { @@ -627,6 +717,18 @@ func isSupportedEventType(rules []typesv1.Rule, enrichedEvent *events.EnrichedEv return true } } + // A write leg needs no ruleExpression for its event type -- that is what + // makes write-without-alerting possible. Without this, write-only legs are + // dropped before reaching the loop and the chain never forms. + // + // The string() casts are load-bearing: StateWrites carries + // armotypes.EventType while eventType is utils.EventType. They are the same + // strings, but not the same Go type. + for _, w := range rule.StateWrites { + if string(w.EventType) == string(eventType) { + return true + } + } } return false } diff --git a/pkg/rulemanager/statecontext.go b/pkg/rulemanager/statecontext.go new file mode 100644 index 0000000000..05a9ca01e8 --- /dev/null +++ b/pkg/rulemanager/statecontext.go @@ -0,0 +1,118 @@ +package rulemanager + +import ( + "github.com/armosec/armoapi-go/armotypes" + "github.com/kubescape/go-logger" + "github.com/kubescape/go-logger/helpers" + "github.com/kubescape/node-agent/pkg/ebpf/events" + "github.com/kubescape/node-agent/pkg/metricsmanager" + "github.com/kubescape/node-agent/pkg/rulemanager/cel/libraries/state" + "github.com/kubescape/node-agent/pkg/rulemanager/statewrites" + typesv1 "github.com/kubescape/node-agent/pkg/rulemanager/types/v1" + "github.com/kubescape/node-agent/pkg/utils" +) + +// compileStateWrites validates a rule's write clause and returns the compiled +// writes plus the name -> scope map that its reads resolve against. +// +// Compilation happens per event rather than once at rule load. That is deliberate: +// it is pure string and duration parsing (the CEL expressions themselves are +// compiled and cached by the evaluator, keyed by expression text), it only runs +// for rules that declare writes -- a small minority -- and doing it here avoids a +// cache that would have to be invalidated on every CRD change. Rules reach the +// loop from two different paths, only one of which populates load-time derived +// fields, so a cached field would be silently empty for host rules. +// +// A rule whose clause fails validation is treated as having no writes, and the +// failure is logged. It is not fatal: one malformed rule must not stop the other +// forty from evaluating. +func (rm *RuleManager) compileStateWrites(rule *typesv1.Rule) ([]statewrites.Compiled, map[string]armotypes.StateScope) { + if len(rule.StateWrites) == 0 { + return nil, nil + } + + compiled, scopeOf, err := statewrites.ValidateAll(rule.StateWrites, rule.ID, rm.cfg.CelStateStore.MaxTTL) + if err != nil { + logger.L().Error("RuleManager - invalid stateWrites clause; the rule will not correlate", + helpers.Error(err), helpers.String("rule", rule.ID)) + return nil, nil + } + return compiled, scopeOf +} + +func hasWriteFor(compiled []statewrites.Compiled, eventType utils.EventType) bool { + for _, w := range compiled { + if w.EventType == eventType { + return true + } + } + return false +} + +// seedStateContext installs the per-rule state receiver into the eval context. +// +// The ancestor walk is passed as a closure and resolved lazily inside the +// accessor: most rules never call has_ancestor, and walking the process tree per +// event per rule would be a real cost for a feature few rules use. +func (rm *RuleManager) seedStateContext( + evalContext map[string]any, + rule *typesv1.Rule, + enrichedEvent *events.EnrichedEvent, + scopeOf map[string]armotypes.StateScope, + tracker *state.ReadTracker, +) { + evalContext[state.AccessorContextKey] = state.NewAccessor( + rm.stateStore, + rule.ID, + scopeOf, + statewrites.ScopeIDs(enrichedEvent), + func() []uint32 { + return rm.processManager.GetAncestorPIDs( + enrichedEvent.PID, rm.cfg.CelStateStore.AncestorMaxDepth) + }, + tracker, + nil, + ) +} + +// stateMetrics adapts the node-agent metrics manager to rulestate.Metrics. +// +// It is a separate type so pkg/rulestate stays free of any metrics dependency and +// remains unit-testable on its own. +type stateMetrics struct { + mm metricsmanager.MetricsManager +} + +func newStateMetrics(mm metricsmanager.MetricsManager) *stateMetrics { + return &stateMetrics{mm: mm} +} + +func (s *stateMetrics) ReportStateWrite(ruleID, result string) { + if s.mm != nil { + s.mm.ReportStateWrite(ruleID, result) + } +} + +func (s *stateMetrics) ReportStateWriteRejected(ruleID, reason string) { + if s.mm != nil { + s.mm.ReportStateWriteRejected(ruleID, reason) + } +} + +func (s *stateMetrics) ReportStateExpired(n int) { + if s.mm != nil { + s.mm.ReportStateExpired(n) + } +} + +func (s *stateMetrics) ReportStatePurged(n int) { + if s.mm != nil { + s.mm.ReportStatePurged(n) + } +} + +func (s *stateMetrics) ReportStateEntries(scope string, n int) { + if s.mm != nil { + s.mm.ReportStateEntries(scope, n) + } +} diff --git a/pkg/rulemanager/statecontext_test.go b/pkg/rulemanager/statecontext_test.go new file mode 100644 index 0000000000..7fec8ec2bc --- /dev/null +++ b/pkg/rulemanager/statecontext_test.go @@ -0,0 +1,181 @@ +package rulemanager + +import ( + "testing" + "time" + + "github.com/armosec/armoapi-go/armotypes" + "github.com/kubescape/node-agent/pkg/config" + "github.com/kubescape/node-agent/pkg/ebpf/events" + typesv1 "github.com/kubescape/node-agent/pkg/rulemanager/types/v1" + "github.com/kubescape/node-agent/pkg/rulestate" + "github.com/kubescape/node-agent/pkg/utils" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func execEnriched() *events.EnrichedEvent { + return &events.EnrichedEvent{ + Event: &utils.StructEvent{EventType: utils.ExecveEventType}, + } +} + +// The subtle failure this exists to prevent: a rule whose ONLY reference to exec +// is a stateWrites entry must still let exec events reach the rule loop. Without +// it the first leg of every cross-event rule is filtered out before evaluation +// and no chain ever forms. +func TestIsSupportedEventType_WriteOnlyLegIsSupported(t *testing.T) { + rule := typesv1.Rule{ + RuntimeRule: armotypes.RuntimeRule{ + ID: "R1089", + StateWrites: []armotypes.StateWrite{{ + EventType: armotypes.EventTypeExec, + Scope: armotypes.StateScopeContainer, + Name: "mount_exec", + TTL: "10m", + }}, + }, + // Alerts on network only -- there is deliberately no exec expression. + Expressions: typesv1.RuleExpressions{ + RuleExpression: []typesv1.RuleExpression{ + {EventType: utils.NetworkEventType, Expression: "true"}, + }, + }, + } + + assert.True(t, isSupportedEventType([]typesv1.Rule{rule}, execEnriched()), + "an exec event must reach the loop for a rule that only WRITES on exec") +} + +func TestIsSupportedEventType_UnrelatedEventStillUnsupported(t *testing.T) { + rule := typesv1.Rule{ + RuntimeRule: armotypes.RuntimeRule{ + ID: "R1089", + StateWrites: []armotypes.StateWrite{{ + EventType: armotypes.EventTypeDNS, + Scope: armotypes.StateScopeContainer, + Name: "n", + TTL: "10m", + }}, + }, + Expressions: typesv1.RuleExpressions{ + RuleExpression: []typesv1.RuleExpression{ + {EventType: utils.NetworkEventType, Expression: "true"}, + }, + }, + } + + assert.False(t, isSupportedEventType([]typesv1.Rule{rule}, execEnriched()), + "exec appears in neither the expressions nor the writes") +} + +func TestIsSupportedEventType_ExpressionStillWorksWithoutWrites(t *testing.T) { + rule := typesv1.Rule{ + RuntimeRule: armotypes.RuntimeRule{ID: "R1004"}, + Expressions: typesv1.RuleExpressions{ + RuleExpression: []typesv1.RuleExpression{ + {EventType: utils.ExecveEventType, Expression: "true"}, + }, + }, + } + assert.True(t, isSupportedEventType([]typesv1.Rule{rule}, execEnriched())) +} + +func testRuleManager(t *testing.T) *RuleManager { + t.Helper() + cfg := config.Config{} + cfg.CelStateStore = rulestate.Config{ + Enabled: true, + MaxSize: 1000, + MaxEntriesPerContainer: 100, + MaxEntriesForHost: 100, + MaxTTL: 30 * time.Minute, + AncestorMaxDepth: 8, + } + return &RuleManager{cfg: cfg} +} + +func TestCompileStateWrites_NoWritesIsNil(t *testing.T) { + rm := testRuleManager(t) + compiled, scopeOf := rm.compileStateWrites(&typesv1.Rule{ + RuntimeRule: armotypes.RuntimeRule{ID: "R1004"}, + }) + assert.Nil(t, compiled) + assert.Nil(t, scopeOf) +} + +func TestCompileStateWrites_ValidClause(t *testing.T) { + rm := testRuleManager(t) + compiled, scopeOf := rm.compileStateWrites(&typesv1.Rule{ + RuntimeRule: armotypes.RuntimeRule{ + ID: "R1089", + StateWrites: []armotypes.StateWrite{{ + EventType: armotypes.EventTypeExec, + Scope: armotypes.StateScopeContainer, + Name: "mount_exec", + Key: "string(event.pid)", + TTL: "10m", + }}, + }, + }) + require.Len(t, compiled, 1) + assert.Equal(t, utils.ExecveEventType, compiled[0].EventType) + assert.Equal(t, map[string]armotypes.StateScope{ + "mount_exec": armotypes.StateScopeContainer, + }, scopeOf) +} + +// A malformed clause must degrade that one rule to non-correlating, not take down +// evaluation for every other rule in the CRD. +func TestCompileStateWrites_InvalidClauseDegradesToNoWrites(t *testing.T) { + rm := testRuleManager(t) + compiled, scopeOf := rm.compileStateWrites(&typesv1.Rule{ + RuntimeRule: armotypes.RuntimeRule{ + ID: "R1089", + StateWrites: []armotypes.StateWrite{{ + EventType: armotypes.EventTypeExec, + Scope: armotypes.StateScopeIdentity, // operator-only + Name: "mount_exec", + TTL: "10m", + }}, + }, + }) + assert.Nil(t, compiled) + assert.Nil(t, scopeOf) +} + +// TTL clamping has to use the configured max, not the write's own value. +func TestCompileStateWrites_ClampsToConfiguredMaxTTL(t *testing.T) { + rm := testRuleManager(t) + compiled, _ := rm.compileStateWrites(&typesv1.Rule{ + RuntimeRule: armotypes.RuntimeRule{ + ID: "R1089", + StateWrites: []armotypes.StateWrite{{ + EventType: armotypes.EventTypeExec, + Scope: armotypes.StateScopeContainer, + Name: "mount_exec", + TTL: "99h", + }}, + }, + }) + require.Len(t, compiled, 1) + assert.Equal(t, 30*time.Minute, compiled[0].TTL) +} + +func TestHasWriteFor(t *testing.T) { + rm := testRuleManager(t) + compiled, _ := rm.compileStateWrites(&typesv1.Rule{ + RuntimeRule: armotypes.RuntimeRule{ + ID: "R1089", + StateWrites: []armotypes.StateWrite{{ + EventType: armotypes.EventTypeExec, + Scope: armotypes.StateScopeContainer, + Name: "mount_exec", + TTL: "10m", + }}, + }, + }) + assert.True(t, hasWriteFor(compiled, utils.ExecveEventType)) + assert.False(t, hasWriteFor(compiled, utils.NetworkEventType)) + assert.False(t, hasWriteFor(nil, utils.ExecveEventType)) +} diff --git a/pkg/rulemanager/statewrites/executor.go b/pkg/rulemanager/statewrites/executor.go new file mode 100644 index 0000000000..f6762067b4 --- /dev/null +++ b/pkg/rulemanager/statewrites/executor.go @@ -0,0 +1,198 @@ +package statewrites + +import ( + "time" + + "github.com/armosec/armoapi-go/armotypes" + "github.com/kubescape/go-logger" + "github.com/kubescape/go-logger/helpers" + "github.com/kubescape/node-agent/pkg/ebpf/events" + "github.com/kubescape/node-agent/pkg/rulestate" + "github.com/kubescape/node-agent/pkg/utils" +) + +// evaluator is the slice of the CEL evaluator the executor needs. Declared here +// rather than imported so this package does not depend on the whole rule engine. +type evaluator interface { + EvaluateBoolExpressionWithContext(evalContext map[string]any, expression string) (bool, error) + EvaluateStringExpressionWithContext(evalContext map[string]any, expression string) (string, error) +} + +// Executor applies a rule's validated write clause for one event. +type Executor struct { + store *rulestate.Store + eval evaluator + metrics rulestate.Metrics +} + +func NewExecutor(store *rulestate.Store, eval evaluator, metrics rulestate.Metrics) *Executor { + if metrics == nil { + metrics = rulestate.NoopMetrics{} + } + return &Executor{store: store, eval: eval, metrics: metrics} +} + +// ScopeIDs resolves this event's ID for every scope node-agent supports. +// +// Scope IDs are always derived here from the event, never from rule input, which +// is what stops a rule reaching another container's bucket. A host / cgroup-0 +// process has no container ID and maps to the explicit host bucket rather than to +// the empty string, which would otherwise collide with node scope. +func ScopeIDs(enriched *events.EnrichedEvent) map[armotypes.StateScope]string { + ids := map[armotypes.StateScope]string{ + armotypes.StateScopeContainer: rulestate.ContainerScopeID(enriched.ContainerID), + armotypes.StateScopeNode: rulestate.NodeScopeID(), + } + if ns, pod := podIdentity(enriched); pod != "" { + ids[armotypes.StateScopePod] = rulestate.PodScopeID(ns, pod) + } + return ids +} + +func podIdentity(enriched *events.EnrichedEvent) (string, string) { + if enriched.Event == nil { + return "", "" + } + return enriched.Event.GetNamespace(), enriched.Event.GetPod() +} + +// exePathGetter and cwdGetter are satisfied by the process-bearing event types +// (exec, open, dns, ...) but not by all of them, so they are probed rather than +// required. An event without them simply stores no path. +type exePathGetter interface{ GetExePath() string } +type cwdGetter interface{ GetCwd() string } + +// Apply evaluates every write whose event type matches this event and stores the +// ones whose guard passes. +// +// It must be called AFTER the predicate has been evaluated. If a write landed +// first, a rule that reads and writes the same name on the same event type would +// satisfy itself from its own write on a single event. +// +// A store rejection is logged at debug and counted, never propagated: a full +// store must degrade correlation, not break alerting. +func (e *Executor) Apply( + compiled []Compiled, + ruleID string, + enriched *events.EnrichedEvent, + evalContext map[string]any, + eventTime time.Time, +) { + if e == nil || e.store == nil || len(compiled) == 0 || evalContext == nil { + return + } + eventType := enriched.Event.GetEventType() + scopeIDs := ScopeIDs(enriched) + + for _, w := range compiled { + if w.EventType != eventType { + continue + } + + if w.When != "" { + ok, err := e.eval.EvaluateBoolExpressionWithContext(evalContext, w.When) + if err != nil { + logger.L().Debug("statewrites - guard evaluation failed", + helpers.Error(err), + helpers.String("rule", ruleID), + helpers.String("name", w.Name)) + e.metrics.ReportStateWriteRejected(ruleID, "guard_error") + continue + } + if !ok { + continue + } + } + + scopeID, ok := scopeIDs[w.Scope] + if !ok { + // pod scope on an event with no pod identity, e.g. a host process. + e.metrics.ReportStateWriteRejected(ruleID, "scope_unresolved") + continue + } + + key := "" + if w.Key != "" { + k, err := e.eval.EvaluateStringExpressionWithContext(evalContext, w.Key) + if err != nil { + logger.L().Debug("statewrites - key evaluation failed", + helpers.Error(err), + helpers.String("rule", ruleID), + helpers.String("name", w.Name)) + e.metrics.ReportStateWriteRejected(ruleID, "key_error") + continue + } + key = k + } + + entry := &rulestate.Entry{ + RuleID: ruleID, + Name: w.Name, + Scope: w.Scope, + ScopeID: scopeID, + Key: key, + EventType: armotypes.EventType(w.EventType), + Timestamp: eventTime, + ExpiresAt: eventTime.Add(w.TTL), + Process: processOf(enriched), + Value: e.evaluateValues(w, ruleID, evalContext), + } + + if err := e.store.Set(entry); err != nil { + logger.L().Debug("statewrites - store rejected the write", + helpers.Error(err), + helpers.String("rule", ruleID), + helpers.String("name", w.Name)) + } + } +} + +func (e *Executor) evaluateValues(w Compiled, ruleID string, evalContext map[string]any) map[string]any { + if len(w.Value) == 0 { + return nil + } + out := make(map[string]any, len(w.Value)) + for k, expr := range w.Value { + v, err := e.eval.EvaluateStringExpressionWithContext(evalContext, expr) + if err != nil { + logger.L().Debug("statewrites - value evaluation failed", + helpers.Error(err), + helpers.String("rule", ruleID), + helpers.String("name", w.Name), + helpers.String("valueKey", k)) + continue + } + out[k] = v + } + if len(out) == 0 { + return nil + } + return out +} + +// processOf snapshots the acting process onto the entry, so an alert on the far +// leg of a chain can describe the process at the near leg -- which by then may be +// long gone. +func processOf(enriched *events.EnrichedEvent) *armotypes.Process { + p := &armotypes.Process{ + PID: enriched.PID, + PPID: enriched.PPID, + } + if e, ok := enriched.Event.(utils.EnrichEvent); ok { + if p.PID == 0 { + p.PID = e.GetPID() + } + if p.PPID == 0 { + p.PPID = e.GetPpid() + } + p.Comm = e.GetComm() + p.Pcomm = e.GetPcomm() + } + if e, ok := enriched.Event.(exePathGetter); ok { + p.Path = e.GetExePath() + } + if e, ok := enriched.Event.(cwdGetter); ok { + p.Cwd = e.GetCwd() + } + return p +} diff --git a/pkg/rulemanager/statewrites/executor_test.go b/pkg/rulemanager/statewrites/executor_test.go new file mode 100644 index 0000000000..a572334e7c --- /dev/null +++ b/pkg/rulemanager/statewrites/executor_test.go @@ -0,0 +1,313 @@ +package statewrites + +import ( + "errors" + "testing" + "time" + + "github.com/armosec/armoapi-go/armotypes" + "github.com/kubescape/node-agent/pkg/ebpf/events" + "github.com/kubescape/node-agent/pkg/rulestate" + "github.com/kubescape/node-agent/pkg/utils" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// fakeEvaluator returns canned results per expression, so the executor's own +// logic is under test rather than CEL's. +type fakeEvaluator struct { + bools map[string]bool + strings map[string]string + boolErr map[string]bool + strErr map[string]bool +} + +func newFakeEvaluator() *fakeEvaluator { + return &fakeEvaluator{ + bools: map[string]bool{}, + strings: map[string]string{}, + boolErr: map[string]bool{}, + strErr: map[string]bool{}, + } +} + +func (f *fakeEvaluator) EvaluateBoolExpressionWithContext(_ map[string]any, expr string) (bool, error) { + if f.boolErr[expr] { + return false, errors.New("boom") + } + return f.bools[expr], nil +} + +func (f *fakeEvaluator) EvaluateStringExpressionWithContext(_ map[string]any, expr string) (string, error) { + if f.strErr[expr] { + return "", errors.New("boom") + } + return f.strings[expr], nil +} + +func testStore(t *testing.T) *rulestate.Store { + t.Helper() + return rulestate.NewStore(rulestate.Config{ + Enabled: true, + MaxSize: 1000, + MaxEntriesPerContainer: 100, + MaxEntriesForHost: 100, + MaxTTL: 30 * time.Minute, + }, rulestate.NoopMetrics{}) +} + +func execEvent(containerID string) *events.EnrichedEvent { + return &events.EnrichedEvent{ + Event: &utils.StructEvent{ + EventType: utils.ExecveEventType, + ContainerID: containerID, + Comm: "xmrig", + Pcomm: "sh", + ExePath: "/mnt/data/xmrig", + Cwd: "/mnt/data", + Namespace: "prod", + Pod: "web-1", + Pid: 4471, + Ppid: 900, + }, + ContainerID: containerID, + PID: 4471, + PPID: 900, + } +} + +func mustCompile(t *testing.T, w armotypes.StateWrite) []Compiled { + t.Helper() + c, err := Validate(w, "R1089", 30*time.Minute) + require.NoError(t, err) + return []Compiled{c} +} + +func TestApply_GuardTrueStoresEntry(t *testing.T) { + store := testStore(t) + ev := newFakeEvaluator() + ev.bools["is_mount"] = true + ev.strings["string(event.pid)"] = "4471" + + w := base() + w.When = "is_mount" + eventTime := time.Now() + + NewExecutor(store, ev, nil).Apply(mustCompile(t, w), "R1089", execEvent("abc"), + map[string]any{}, eventTime) + + got, ok := store.Get("R1089", armotypes.StateScopeContainer, "c:abc", "mount_exec", "4471") + require.True(t, ok) + assert.Equal(t, eventTime, got.Timestamp, + "the entry must carry the event time, so _ts guards compare the same clock the predicate saw") + assert.Equal(t, eventTime.Add(10*time.Minute), got.ExpiresAt) + require.NotNil(t, got.Process) + assert.Equal(t, uint32(4471), got.Process.PID) + assert.Equal(t, "xmrig", got.Process.Comm) + assert.Equal(t, "/mnt/data/xmrig", got.Process.Path) + assert.Equal(t, armotypes.EventTypeExec, got.EventType) +} + +func TestApply_GuardFalseStoresNothing(t *testing.T) { + store := testStore(t) + ev := newFakeEvaluator() + ev.bools["is_mount"] = false + + w := base() + w.When = "is_mount" + + NewExecutor(store, ev, nil).Apply(mustCompile(t, w), "R1089", execEvent("abc"), + map[string]any{}, time.Now()) + + assert.Equal(t, 0, store.Len()) +} + +func TestApply_AbsentGuardAlwaysStores(t *testing.T) { + store := testStore(t) + ev := newFakeEvaluator() + ev.strings["string(event.pid)"] = "4471" + + w := base() // When == "" + NewExecutor(store, ev, nil).Apply(mustCompile(t, w), "R1089", execEvent("abc"), + map[string]any{}, time.Now()) + + _, ok := store.Get("R1089", armotypes.StateScopeContainer, "c:abc", "mount_exec", "4471") + assert.True(t, ok) +} + +// Host processes carry no container ID and must land in the explicit host bucket, +// not under the empty string. +func TestApply_HostProcessUsesHostScopeID(t *testing.T) { + store := testStore(t) + ev := newFakeEvaluator() + ev.strings["string(event.pid)"] = "4471" + + NewExecutor(store, ev, nil).Apply(mustCompile(t, base()), "R1089", execEvent(""), + map[string]any{}, time.Now()) + + _, ok := store.Get("R1089", armotypes.StateScopeContainer, rulestate.HostScopeID(), "mount_exec", "4471") + assert.True(t, ok, "a host process must be addressable under %q", rulestate.HostScopeID()) +} + +// Write-without-alerting: the write leg's event type need not appear in any +// ruleExpression. The executor sees only compiled writes, so this must hold. +func TestApply_StoresForEventTypeWithNoRuleExpression(t *testing.T) { + store := testStore(t) + ev := newFakeEvaluator() + ev.strings["string(event.pid)"] = "4471" + + // No ruleExpression exists anywhere for this rule; only the write clause. + NewExecutor(store, ev, nil).Apply(mustCompile(t, base()), "R1089", execEvent("abc"), + map[string]any{}, time.Now()) + + assert.Equal(t, 1, store.Len()) +} + +func TestApply_SkipsWritesForOtherEventTypes(t *testing.T) { + store := testStore(t) + ev := newFakeEvaluator() + + w := base() + w.EventType = armotypes.EventTypeNetwork // event is exec + + NewExecutor(store, ev, nil).Apply(mustCompile(t, w), "R1089", execEvent("abc"), + map[string]any{}, time.Now()) + + assert.Equal(t, 0, store.Len()) +} + +func TestApply_ValueExpressionsAreStored(t *testing.T) { + store := testStore(t) + ev := newFakeEvaluator() + ev.strings["string(event.pid)"] = "4471" + ev.strings["event.args"] = "-o pool:4444" + + w := base() + w.Value = map[string]any{"argv": "event.args"} + + NewExecutor(store, ev, nil).Apply(mustCompile(t, w), "R1089", execEvent("abc"), + map[string]any{}, time.Now()) + + got, ok := store.Get("R1089", armotypes.StateScopeContainer, "c:abc", "mount_exec", "4471") + require.True(t, ok) + assert.Equal(t, map[string]any{"argv": "-o pool:4444"}, got.Value) +} + +func TestApply_EmptyKeyYieldsOneScopeWideEntry(t *testing.T) { + store := testStore(t) + ev := newFakeEvaluator() + + w := base() + w.Key = "" + + NewExecutor(store, ev, nil).Apply(mustCompile(t, w), "R1089", execEvent("abc"), + map[string]any{}, time.Now()) + + _, ok := store.Get("R1089", armotypes.StateScopeContainer, "c:abc", "mount_exec", "") + assert.True(t, ok) + assert.Equal(t, 1, store.Len()) +} + +// A broken guard must not store; a broken key must not store under a wrong key. +func TestApply_ExpressionErrorsSkipTheWrite(t *testing.T) { + t.Run("guard error", func(t *testing.T) { + store := testStore(t) + ev := newFakeEvaluator() + ev.boolErr["bad"] = true + w := base() + w.When = "bad" + NewExecutor(store, ev, nil).Apply(mustCompile(t, w), "R1089", execEvent("abc"), + map[string]any{}, time.Now()) + assert.Equal(t, 0, store.Len()) + }) + + t.Run("key error", func(t *testing.T) { + store := testStore(t) + ev := newFakeEvaluator() + ev.strErr["string(event.pid)"] = true + NewExecutor(store, ev, nil).Apply(mustCompile(t, base()), "R1089", execEvent("abc"), + map[string]any{}, time.Now()) + assert.Equal(t, 0, store.Len()) + }) +} + +// Pod scope on an event with no pod identity has nothing to resolve against; the +// write is dropped rather than landing in a bogus bucket. +func TestApply_PodScopeWithoutPodIdentityIsDropped(t *testing.T) { + store := testStore(t) + ev := newFakeEvaluator() + ev.strings["string(event.pid)"] = "4471" + + w := base() + w.Scope = armotypes.StateScopePod + + enriched := execEvent("abc") + enriched.Event.(*utils.StructEvent).Pod = "" + + NewExecutor(store, ev, nil).Apply(mustCompile(t, w), "R1089", enriched, + map[string]any{}, time.Now()) + + assert.Equal(t, 0, store.Len()) +} + +func TestApply_PodScopeUsesNamespacedID(t *testing.T) { + store := testStore(t) + ev := newFakeEvaluator() + ev.strings["string(event.pid)"] = "4471" + + w := base() + w.Scope = armotypes.StateScopePod + + NewExecutor(store, ev, nil).Apply(mustCompile(t, w), "R1089", execEvent("abc"), + map[string]any{}, time.Now()) + + _, ok := store.Get("R1089", armotypes.StateScopePod, rulestate.PodScopeID("prod", "web-1"), + "mount_exec", "4471") + assert.True(t, ok) +} + +// A guard that itself reads state is how multi-step chains are built; the +// executor must not treat a state-reading guard specially. +func TestApply_GuardMayItselfDependOnState(t *testing.T) { + store := testStore(t) + ev := newFakeEvaluator() + ev.bools[`state.has("step1")`] = true + ev.strings["string(event.pid)"] = "4471" + + w := base() + w.Name = "step2" + w.When = `state.has("step1")` + + NewExecutor(store, ev, nil).Apply(mustCompile(t, w), "R1089", execEvent("abc"), + map[string]any{}, time.Now()) + + _, ok := store.Get("R1089", armotypes.StateScopeContainer, "c:abc", "step2", "4471") + assert.True(t, ok) +} + +func TestApply_NilSafeOnMissingPieces(t *testing.T) { + store := testStore(t) + ev := newFakeEvaluator() + e := NewExecutor(store, ev, nil) + + // No writes, and a nil eval context: both must be no-ops, not panics. + e.Apply(nil, "R1089", execEvent("abc"), map[string]any{}, time.Now()) + e.Apply(mustCompile(t, base()), "R1089", execEvent("abc"), nil, time.Now()) + assert.Equal(t, 0, store.Len()) +} + +func TestScopeIDs_ResolvesFromTheEventOnly(t *testing.T) { + ids := ScopeIDs(execEvent("abc")) + assert.Equal(t, "c:abc", ids[armotypes.StateScopeContainer]) + assert.Equal(t, "n:", ids[armotypes.StateScopeNode]) + assert.Equal(t, "p:prod/web-1", ids[armotypes.StateScopePod]) + + // Host: container scope resolves to the host bucket, pod scope is absent. + hostIDs := ScopeIDs(&events.EnrichedEvent{ + Event: &utils.StructEvent{EventType: utils.ExecveEventType}, + ContainerID: "", + }) + assert.Equal(t, rulestate.HostScopeID(), hostIDs[armotypes.StateScopeContainer]) + _, hasPod := hostIDs[armotypes.StateScopePod] + assert.False(t, hasPod) +} diff --git a/pkg/rulemanager/statewrites/validate.go b/pkg/rulemanager/statewrites/validate.go new file mode 100644 index 0000000000..42bcb11d45 --- /dev/null +++ b/pkg/rulemanager/statewrites/validate.go @@ -0,0 +1,152 @@ +// Package statewrites validates and executes a rule's declarative `stateWrites` +// clause: the only way a CEL rule writes to the state store. +// +// Writes are declarative rather than a CEL setter function on purpose. A setter +// could be skipped by boolean short-circuiting, reordered by the static +// optimiser, and could never express "remember this without alerting" -- which is +// exactly what the first leg of a cross-event rule needs. +package statewrites + +import ( + "fmt" + "strings" + "time" + + "github.com/armosec/armoapi-go/armotypes" + "github.com/kubescape/go-logger" + "github.com/kubescape/go-logger/helpers" + "github.com/kubescape/node-agent/pkg/utils" +) + +// Compiled is a validated StateWrite ready for the rule loop. +// +// EventType is utils.EventType, not armotypes.EventType: the rule loop compares +// against utils.EventType, and doing the conversion once here keeps the +// comparison typed rather than a string cast at every event. +type Compiled struct { + EventType utils.EventType + Scope armotypes.StateScope + Name string + Key string + When string + Value map[string]string + TTL time.Duration +} + +// nodeAgentScopes are the scopes node-agent can resolve. identity is +// operator-only: there is no caller identity on an eBPF event. +var nodeAgentScopes = map[armotypes.StateScope]struct{}{ + armotypes.StateScopeContainer: {}, + armotypes.StateScopePod: {}, + armotypes.StateScopeNode: {}, +} + +// Validate checks one write and converts it for the rule loop. +// +// Everything here fails at rule load rather than at runtime, because every one of +// these mistakes otherwise produces a rule that loads cleanly and silently never +// fires -- the worst possible failure for a detection. +func Validate(w armotypes.StateWrite, ruleID string, maxTTL time.Duration) (Compiled, error) { + fail := func(format string, args ...any) (Compiled, error) { + return Compiled{}, fmt.Errorf("rule %s: stateWrites[%q]: "+format, + append([]any{ruleID, w.Name}, args...)...) + } + + if w.Name == "" { + return fail("name must not be empty") + } + if strings.HasPrefix(w.Name, "_") { + return fail("name must not begin with %q, which is reserved for engine provenance", "_") + } + + // IsValidStateWriteEventType, not IsKnownEventType: the latter accepts + // EventTypeAll, which is a rule-binding wildcard rather than an event stream, + // so it would produce a write leg that never matches a concrete event. + if !armotypes.IsValidStateWriteEventType(w.EventType) { + return fail("eventType %q is not an event stream that can drive a write", w.EventType) + } + eventType := utils.EventType(w.EventType) + if !utils.IsValidEventType(eventType) { + return fail("eventType %q is not emitted by node-agent", w.EventType) + } + + if _, ok := nodeAgentScopes[w.Scope]; !ok { + return fail("scope %q is not resolvable by node-agent (want container, pod or node)", w.Scope) + } + + ttl, err := time.ParseDuration(w.TTL) + if err != nil { + return fail("ttl %q is not a duration: %w", w.TTL, err) + } + if ttl <= 0 { + return fail("ttl %q must be positive; an entry born expired is a silently dead rule", w.TTL) + } + if ttl > maxTTL { + logger.L().Warning("statewrites - clamping ttl to the configured maximum", + helpers.String("rule", ruleID), + helpers.String("name", w.Name), + helpers.String("requested", ttl.String()), + helpers.String("maxTtl", maxTTL.String())) + ttl = maxTTL + } + + var values map[string]string + if len(w.Value) > 0 { + values = make(map[string]string, len(w.Value)) + for k, v := range w.Value { + if k == "" { + return fail("value keys must not be empty") + } + if strings.HasPrefix(k, "_") { + return fail("value key %q must not begin with %q, which is reserved for engine provenance", k, "_") + } + s, ok := v.(string) + if !ok { + return fail("value %q must be a CEL expression string, got %T", k, v) + } + values[k] = s + } + } + + return Compiled{ + EventType: eventType, + Scope: w.Scope, + Name: w.Name, + Key: w.Key, + When: w.When, + Value: values, + TTL: ttl, + }, nil +} + +// ValidateAll validates a rule's whole clause and returns the name -> scope map +// that reads resolve against. +// +// A name must map to exactly one scope. Reads take no scope argument -- they infer +// it from the name -- so the same name in two scopes would make every read of it +// ambiguous. Declaring one name across several event types in the SAME scope is +// the normal bidirectional idiom and is allowed. +func ValidateAll(writes []armotypes.StateWrite, ruleID string, maxTTL time.Duration) ([]Compiled, map[string]armotypes.StateScope, error) { + if len(writes) == 0 { + return nil, nil, nil + } + + compiled := make([]Compiled, 0, len(writes)) + scopeOf := make(map[string]armotypes.StateScope, len(writes)) + + for _, w := range writes { + c, err := Validate(w, ruleID, maxTTL) + if err != nil { + return nil, nil, err + } + if existing, ok := scopeOf[c.Name]; ok && existing != c.Scope { + return nil, nil, fmt.Errorf( + "rule %s: stateWrites[%q]: declared in both scope %q and scope %q; a name must have exactly one scope because reads infer it from the name", + ruleID, c.Name, existing, c.Scope) + } + scopeOf[c.Name] = c.Scope + compiled = append(compiled, c) + } + + return compiled, scopeOf, nil +} diff --git a/pkg/rulemanager/statewrites/validate_test.go b/pkg/rulemanager/statewrites/validate_test.go new file mode 100644 index 0000000000..2d576e2f64 --- /dev/null +++ b/pkg/rulemanager/statewrites/validate_test.go @@ -0,0 +1,203 @@ +package statewrites + +import ( + "testing" + "time" + + "github.com/armosec/armoapi-go/armotypes" + "github.com/kubescape/node-agent/pkg/utils" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func base() armotypes.StateWrite { + return armotypes.StateWrite{ + EventType: armotypes.EventTypeExec, + Scope: armotypes.StateScopeContainer, + Name: "mount_exec", + Key: "string(event.pid)", + TTL: "10m", + } +} + +func TestValidate_Accepts(t *testing.T) { + c, err := Validate(base(), "R1089", 30*time.Minute) + require.NoError(t, err) + assert.Equal(t, 10*time.Minute, c.TTL) + assert.Equal(t, "mount_exec", c.Name) + assert.Equal(t, utils.ExecveEventType, c.EventType, + "the rule loop compares utils.EventType, so Validate must convert") +} + +func TestValidate_ClampsTTLToMax(t *testing.T) { + w := base() + w.TTL = "24h" + c, err := Validate(w, "R1089", 30*time.Minute) + require.NoError(t, err) + assert.Equal(t, 30*time.Minute, c.TTL, "no rule may pin memory indefinitely") +} + +func TestValidate_RejectsUnknownEventType(t *testing.T) { + w := base() + w.EventType = armotypes.EventType("nonsense") + _, err := Validate(w, "R1089", 30*time.Minute) + require.Error(t, err, "a rule naming a nonexistent event stream must fail loudly at load, not never match at runtime") +} + +// EventTypeAll is a rule-binding wildcard, not an event stream. Accepting it +// would yield a rule that loads cleanly and then never matches a concrete event. +func TestValidate_RejectsAllWildcardAsDriver(t *testing.T) { + w := base() + w.EventType = armotypes.EventTypeAll + _, err := Validate(w, "R1089", 30*time.Minute) + require.Error(t, err, "the binding wildcard cannot drive a state write") +} + +func TestValidate_RejectsIdentityScopeInNodeAgent(t *testing.T) { + w := base() + w.Scope = armotypes.StateScopeIdentity + _, err := Validate(w, "R1089", 30*time.Minute) + require.Error(t, err, "identity scope belongs to the operator") +} + +func TestValidate_AcceptsContainerPodAndNodeScopes(t *testing.T) { + for _, scope := range []armotypes.StateScope{ + armotypes.StateScopeContainer, + armotypes.StateScopePod, + armotypes.StateScopeNode, + } { + w := base() + w.Scope = scope + c, err := Validate(w, "R1089", 30*time.Minute) + require.NoError(t, err, "scope %q must be accepted", scope) + assert.Equal(t, scope, c.Scope) + } +} + +func TestValidate_RejectsEmptyScope(t *testing.T) { + w := base() + w.Scope = "" + _, err := Validate(w, "R1089", 30*time.Minute) + require.Error(t, err) +} + +func TestValidate_RejectsReservedValueKeys(t *testing.T) { + w := base() + w.Value = map[string]any{"_pid": "event.pid"} + _, err := Validate(w, "R1089", 30*time.Minute) + require.Error(t, err, "author values must not shadow engine provenance") +} + +func TestValidate_RejectsReservedNamePrefix(t *testing.T) { + w := base() + w.Name = "_internal" + _, err := Validate(w, "R1089", 30*time.Minute) + require.Error(t, err) +} + +func TestValidate_RejectsEmptyNameAndBadTTL(t *testing.T) { + w := base() + w.Name = "" + _, err := Validate(w, "R1089", 30*time.Minute) + require.Error(t, err) + + w = base() + w.TTL = "not-a-duration" + _, err = Validate(w, "R1089", 30*time.Minute) + require.Error(t, err) +} + +func TestValidate_RejectsNonPositiveTTL(t *testing.T) { + for _, ttl := range []string{"", "0s", "-5m"} { + w := base() + w.TTL = ttl + _, err := Validate(w, "R1089", 30*time.Minute) + require.Error(t, err, "TTL %q must be rejected: an entry that is born expired is a silently dead rule", ttl) + } +} + +func TestValidate_AllowsEmptyKeyForScopeWideMarker(t *testing.T) { + w := base() + w.Key = "" + c, err := Validate(w, "R1089", 30*time.Minute) + require.NoError(t, err) + assert.Empty(t, c.Key) +} + +func TestValidate_AcceptsStringValueExpressions(t *testing.T) { + w := base() + w.Value = map[string]any{"argv": "event.args"} + c, err := Validate(w, "R1089", 30*time.Minute) + require.NoError(t, err) + assert.Equal(t, map[string]string{"argv": "event.args"}, c.Value) +} + +// Values are CEL expression strings. A non-string would otherwise be stored as a +// literal, which silently is not what the author asked for. +func TestValidate_RejectsNonStringValueExpressions(t *testing.T) { + w := base() + w.Value = map[string]any{"threshold": 5} + _, err := Validate(w, "R1089", 30*time.Minute) + require.Error(t, err) +} + +func TestValidate_RejectsEmptyValueKey(t *testing.T) { + w := base() + w.Value = map[string]any{"": "event.args"} + _, err := Validate(w, "R1089", 30*time.Minute) + require.Error(t, err) +} + +// Errors must name the rule and the write, or a bad rule in a 50-rule CRD is not +// diagnosable from the log line. +func TestValidate_ErrorNamesRuleAndWrite(t *testing.T) { + w := base() + w.TTL = "nope" + _, err := Validate(w, "R1089", 30*time.Minute) + require.Error(t, err) + assert.Contains(t, err.Error(), "R1089") + assert.Contains(t, err.Error(), "mount_exec") +} + +func TestValidateAll_ReportsFirstFailureAndScopeMap(t *testing.T) { + good := base() + other := base() + other.Name = "egress_seen" + other.Scope = armotypes.StateScopeNode + + compiled, scopeOf, err := ValidateAll([]armotypes.StateWrite{good, other}, "R1089", 30*time.Minute) + require.NoError(t, err) + require.Len(t, compiled, 2) + assert.Equal(t, map[string]armotypes.StateScope{ + "mount_exec": armotypes.StateScopeContainer, + "egress_seen": armotypes.StateScopeNode, + }, scopeOf, "the scope map is what lets a read resolve its scope from the name alone") + + bad := base() + bad.Name = "" + _, _, err = ValidateAll([]armotypes.StateWrite{good, bad}, "R1089", 30*time.Minute) + require.Error(t, err) +} + +// The same name declared twice with different scopes makes a read ambiguous. +func TestValidateAll_RejectsSameNameInTwoScopes(t *testing.T) { + a := base() + b := base() + b.EventType = armotypes.EventTypeNetwork + b.Scope = armotypes.StateScopeNode + + _, _, err := ValidateAll([]armotypes.StateWrite{a, b}, "R1089", 30*time.Minute) + require.Error(t, err, "a name must map to exactly one scope, or reads cannot resolve it") +} + +// The bidirectional idiom: one name, two event types, same scope. Must be legal. +func TestValidateAll_AllowsSameNameSameScopeAcrossEventTypes(t *testing.T) { + a := base() + b := base() + b.EventType = armotypes.EventTypeNetwork + + compiled, scopeOf, err := ValidateAll([]armotypes.StateWrite{a, b}, "R1089", 30*time.Minute) + require.NoError(t, err) + assert.Len(t, compiled, 2) + assert.Len(t, scopeOf, 1) +} diff --git a/pkg/utils/events.go b/pkg/utils/events.go index bc0a4cdf58..71683a2444 100644 --- a/pkg/utils/events.go +++ b/pkg/utils/events.go @@ -233,6 +233,29 @@ const ( UnshareEventType EventType = "unshare" ) +// nodeAgentEventTypes is every event stream node-agent can actually deliver. +// AllEventType is deliberately absent: it is a rule-binding wildcard, not a +// stream, so anything that must name a concrete stream has to reject it. +var nodeAgentEventTypes = map[EventType]struct{}{ + BpfEventType: {}, CapabilitiesEventType: {}, DnsEventType: {}, + ExecveEventType: {}, ExitEventType: {}, ForkEventType: {}, + HTTPEventType: {}, HardlinkEventType: {}, IoUringEventType: {}, + KmodEventType: {}, NetworkEventType: {}, OpenEventType: {}, + ProcfsEventType: {}, PtraceEventType: {}, RandomXEventType: {}, + SSHEventType: {}, SymlinkEventType: {}, SyscallEventType: {}, + UnshareEventType: {}, +} + +// IsValidEventType reports whether e names a concrete node-agent event stream. +// +// This is narrower than armotypes.IsKnownEventType, which spans both engines -- +// k8s-admission is a real armotypes event type that node-agent never emits, so a +// node-agent rule naming it must be rejected at load rather than never matching. +func IsValidEventType(e EventType) bool { + _, ok := nodeAgentEventTypes[e] + return ok +} + // Get the path of the file on the node. func GetHostFilePathFromEvent(event EnrichEvent, containerPid uint32) (string, error) { switch event.GetEventType() { From 83cbc7c2f1d93e6283332441f58837cdb5ff7e89 Mon Sep 17 00:00:00 2001 From: Ben Date: Thu, 30 Jul 2026 14:34:07 +0300 Subject: [PATCH 07/17] feat(rulemanager): attach correlation evidence to alerts Entries the predicate actually read become armotypes.CorrelationEvidence on the alert, so a correlation alert describes BOTH ends of the chain -- without it the alert would say only 'a process made an outbound connection' and drop the exec that makes it interesting. Populated in CreateRuleFailure rather than an event adapter, because SetFailureMetadata is per-event-type while correlation is not. InfectedPID and RuntimeProcessDetails still describe the triggering event: correlation enriches an incident, it does not re-key it, so backend grouping is unchanged. TestCorrelationEvidence_DoesNotRekeyTheAlert pins that, and an alert with no correlations still serializes with no correlations key at all. message/uniqueId now reuse the predicate's eval context so state.get() resolves against the same entries, and uniqueId can be derived from the join key -- which is what lets rulecooldown collapse both legs of a bidirectional rule. Note the plan specified `Scope: string(h.Scope)` here; CorrelationEvidence.Scope shipped as a typed armotypes.StateScope in v0.0.739 (a review change on armoapi-go #681), so the copy is direct and the plan text was stale. Signed-off-by: Ben --- docs/features/cel-rule-state-store.md | 27 +++- pkg/exporters/http_exporter.go | 1 + pkg/rulemanager/rule_manager.go | 20 ++- .../ruleadapters/correlation_test.go | 130 ++++++++++++++++++ pkg/rulemanager/ruleadapters/creator.go | 42 +++++- .../ruleadapters/creator_interface.go | 3 +- pkg/rulemanager/types/failure.go | 13 ++ 7 files changed, 227 insertions(+), 9 deletions(-) create mode 100644 pkg/rulemanager/ruleadapters/correlation_test.go diff --git a/docs/features/cel-rule-state-store.md b/docs/features/cel-rule-state-store.md index b937716f66..2866c6ef59 100644 --- a/docs/features/cel-rule-state-store.md +++ b/docs/features/cel-rule-state-store.md @@ -8,8 +8,9 @@ delete of a pod — cannot be expressed at all. Design: `shared-designs-and-docs/projects/2026-07-28-cel-rule-state-store/spec.md`. -> **Status: under construction.** Reads and writes both work. Still to land: -> correlation evidence on the emitted alert, and scope purge on container removal. +> **Status: under construction.** Reads, writes and alert evidence all work. +> Still to land: scope purge on container removal, and the end-to-end component +> test against real eBPF events. Not yet exercised on a live cluster. ## Writing state @@ -182,3 +183,25 @@ second, divergent source of truth. `string(timestamp)` renders in the node's local zone, so the offset in the text depends on where the agent runs. Comparisons are instant-based and unaffected. Assert on instants, not on rendered text. + +## Correlation evidence on the alert + +When a rule fires, the state entries its predicate **actually read** are attached +to the alert as `correlations[]`, so the alert describes both ends of the chain. +Without it, an exec-then-egress alert would say only "a process made an outbound +connection" and drop the exec that makes it interesting. + +Each entry carries `name`, `eventType`, `timestamp`, `scope`, `key`, the +remembered `process`, and any author `values`. Only hits are recorded — a miss is +not evidence of anything — and the record is reset per rule, so one rule never +cites another's entries. + +**Correlation enriches an incident; it does not re-key it.** `InfectedPID` and +`RuntimeProcessDetails` continue to describe the *triggering* event, so backend +incident grouping is unchanged. An alert with no correlations serializes exactly +as before, with no `correlations` key. + +`message` and `uniqueId` are evaluated against the predicate's own context, so +`state.get()` in a message resolves against the same entries the predicate +matched — and `uniqueId` can be derived from the join key, which is what lets +cooldown collapse both legs of a bidirectional rule into one alert. diff --git a/pkg/exporters/http_exporter.go b/pkg/exporters/http_exporter.go index 8ecc9c751f..3305d08000 100644 --- a/pkg/exporters/http_exporter.go +++ b/pkg/exporters/http_exporter.go @@ -346,6 +346,7 @@ func (e *HTTPExporter) createRuleAlert(failedRule types.RuleFailure) armotypes.R RuleID: failedRule.GetRuleId(), IsTriggerAlert: failedRule.GetIsTriggerAlert(), HttpRuleAlert: httpDetails, + CorrelationAlert: failedRule.GetCorrelationAlert(), } } diff --git a/pkg/rulemanager/rule_manager.go b/pkg/rulemanager/rule_manager.go index c92dc1fecb..d00eb13b7d 100644 --- a/pkg/rulemanager/rule_manager.go +++ b/pkg/rulemanager/rule_manager.go @@ -499,7 +499,7 @@ func (rm *RuleManager) evaluateRuleAndAlert(a evaluateArgs) { ruleState = rm.evaluateHTTPPayloadState(rule.State, enrichedEvent) } rm.metrics.ReportRuleAlert(rule.ID) - message, uniqueID, err := rm.getUniqueIdAndMessage(enrichedEvent, rule) + message, uniqueID, err := rm.getUniqueIdAndMessage(evalContext, rule) if err != nil { logger.L().Error("RuleManager - failed to get unique ID and message", helpers.Error(err)) return @@ -547,7 +547,10 @@ func (rm *RuleManager) evaluateRuleAndAlert(a evaluateArgs) { alertSpan.End() } - ruleFailure := rm.ruleFailureCreator.CreateRuleFailure(rule, enrichedEvent, rm.objectCache, message, uniqueID, apChecksum, ruleState) + // The entries this rule's predicate actually read become the alert's + // correlation evidence. Harvested here, after the predicate ran and after + // cooldown, so a suppressed alert costs nothing. + ruleFailure := rm.ruleFailureCreator.CreateRuleFailure(rule, enrichedEvent, rm.objectCache, message, uniqueID, apChecksum, ruleState, a.tracker.Hits()) if ruleFailure == nil { logger.L().Error("RuleManager - failed to create rule failure", helpers.String("rule", rule.Name), helpers.String("message", message), @@ -694,12 +697,19 @@ func (rm *RuleManager) getRuleExpressions(rule typesv1.Rule, eventType utils.Eve return ruleExpressions } -func (rm *RuleManager) getUniqueIdAndMessage(enrichedEvent *events.EnrichedEvent, rule typesv1.Rule) (string, string, error) { - message, err := rm.celEvaluator.EvaluateExpression(enrichedEvent, rule.Expressions.Message) +// getUniqueIdAndMessage renders the alert's message and uniqueId. +// +// It takes the predicate's evalContext rather than rebuilding one. That matters +// for two reasons: state.get() in a message must resolve against the SAME entries +// the predicate matched, and uniqueId can then be derived from the join key -- +// which is what lets rulecooldown collapse both legs of a bidirectional rule into +// a single alert instead of emitting one per leg. +func (rm *RuleManager) getUniqueIdAndMessage(evalContext map[string]any, rule typesv1.Rule) (string, string, error) { + message, err := rm.celEvaluator.EvaluateStringExpressionWithContext(evalContext, rule.Expressions.Message) if err != nil { logger.L().Ctx(rm.ctx).Error("RuleManager - failed to evaluate message", helpers.Error(err)) } - uniqueID, err := rm.celEvaluator.EvaluateExpression(enrichedEvent, rule.Expressions.UniqueID) + uniqueID, err := rm.celEvaluator.EvaluateStringExpressionWithContext(evalContext, rule.Expressions.UniqueID) if err != nil { logger.L().Ctx(rm.ctx).Error("RuleManager - failed to evaluate unique ID", helpers.Error(err)) } diff --git a/pkg/rulemanager/ruleadapters/correlation_test.go b/pkg/rulemanager/ruleadapters/correlation_test.go new file mode 100644 index 0000000000..89d793b18d --- /dev/null +++ b/pkg/rulemanager/ruleadapters/correlation_test.go @@ -0,0 +1,130 @@ +package ruleadapters + +import ( + "encoding/json" + "testing" + "time" + + "github.com/armosec/armoapi-go/armotypes" + "github.com/kubescape/node-agent/pkg/rulemanager/types" + "github.com/kubescape/node-agent/pkg/rulestate" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func execHit() *rulestate.Entry { + return &rulestate.Entry{ + RuleID: "R1089", + Name: "mount_exec", + Scope: armotypes.StateScopeContainer, + ScopeID: "c:abc", + Key: "4471", + EventType: armotypes.EventTypeExec, + Timestamp: time.Date(2026, 7, 28, 12, 0, 3, 0, time.UTC), + Process: &armotypes.Process{ + PID: 4471, Comm: "xmrig", Path: "/mnt/data/xmrig", + }, + Value: map[string]any{"argv": "-o pool:4444"}, + } +} + +func TestCorrelationEvidence_OneHitBecomesOneEvidenceEntry(t *testing.T) { + failure := &types.GenericRuleFailure{} + setCorrelationEvidence(failure, []*rulestate.Entry{execHit()}) + + got := failure.GetCorrelationAlert() + require.Len(t, got.Correlations, 1) + + c := got.Correlations[0] + assert.Equal(t, "mount_exec", c.Name) + assert.Equal(t, armotypes.EventTypeExec, c.EventType) + assert.Equal(t, time.Date(2026, 7, 28, 12, 0, 3, 0, time.UTC), c.Timestamp) + assert.Equal(t, armotypes.StateScopeContainer, c.Scope) + assert.Equal(t, "4471", c.Key) + require.NotNil(t, c.Process) + assert.Equal(t, uint32(4471), c.Process.PID) + assert.Equal(t, "/mnt/data/xmrig", c.Process.Path) + assert.Equal(t, map[string]any{"argv": "-o pool:4444"}, c.Values) + assert.Nil(t, c.Admission, "node-agent entries carry a Process, never an Admission") +} + +func TestCorrelationEvidence_MultipleHitsArePreservedInOrder(t *testing.T) { + second := execHit() + second.Name = "egress_seen" + + failure := &types.GenericRuleFailure{} + setCorrelationEvidence(failure, []*rulestate.Entry{execHit(), second}) + + got := failure.GetCorrelationAlert() + require.Len(t, got.Correlations, 2) + assert.Equal(t, "mount_exec", got.Correlations[0].Name) + assert.Equal(t, "egress_seen", got.Correlations[1].Name) +} + +func TestCorrelationEvidence_NoHitsLeavesTheAlertUntouched(t *testing.T) { + failure := &types.GenericRuleFailure{} + setCorrelationEvidence(failure, nil) + assert.Empty(t, failure.GetCorrelationAlert().Correlations) + + // omitempty means an uncorrelated alert must not gain a "correlations" key -- + // every existing alert on the wire has to stay byte-identical. + data, err := json.Marshal(failure.GetCorrelationAlert()) + require.NoError(t, err) + assert.NotContains(t, string(data), "correlations") +} + +func TestCorrelationEvidence_NilEntriesAreSkipped(t *testing.T) { + failure := &types.GenericRuleFailure{} + setCorrelationEvidence(failure, []*rulestate.Entry{nil, execHit(), nil}) + assert.Len(t, failure.GetCorrelationAlert().Correlations, 1) +} + +func TestCorrelationEvidence_AllNilEntriesAddsNothing(t *testing.T) { + failure := &types.GenericRuleFailure{} + setCorrelationEvidence(failure, []*rulestate.Entry{nil, nil}) + assert.Empty(t, failure.GetCorrelationAlert().Correlations) +} + +// Correlation must ENRICH an incident, never re-key it. If InfectedPID or +// RuntimeProcessDetails shifted to the remembered process, backend incident +// grouping would move the alert to a different incident. +func TestCorrelationEvidence_DoesNotRekeyTheAlert(t *testing.T) { + failure := &types.GenericRuleFailure{ + BaseRuntimeAlert: armotypes.BaseRuntimeAlert{ + InfectedPID: 9999, // the TRIGGERING process + }, + RuntimeProcessDetails: armotypes.ProcessTree{ + ContainerID: "triggering-container", + ProcessTree: armotypes.Process{PID: 9999}, + }, + } + + setCorrelationEvidence(failure, []*rulestate.Entry{execHit()}) // remembered PID 4471 + + assert.Equal(t, uint32(9999), failure.GetBaseRuntimeAlert().InfectedPID, + "InfectedPID must still describe the triggering event") + assert.Equal(t, uint32(9999), failure.GetRuntimeProcessDetails().ProcessTree.PID) + assert.Equal(t, "triggering-container", failure.GetRuntimeProcessDetails().ContainerID) +} + +// The evidence must survive onto the wire alert, not just the internal failure. +func TestCorrelationEvidence_SerializesUnderCorrelations(t *testing.T) { + failure := &types.GenericRuleFailure{} + setCorrelationEvidence(failure, []*rulestate.Entry{execHit()}) + + alert := armotypes.RuntimeAlert{ + CorrelationAlert: failure.GetCorrelationAlert(), + } + data, err := json.Marshal(alert) + require.NoError(t, err) + + var round map[string]any + require.NoError(t, json.Unmarshal(data, &round)) + + raw, ok := round["correlations"] + require.True(t, ok, "CorrelationAlert is inlined, so evidence appears at the alert top level") + entries, ok := raw.([]any) + require.True(t, ok) + require.Len(t, entries, 1) + assert.Equal(t, "mount_exec", entries[0].(map[string]any)["name"]) +} diff --git a/pkg/rulemanager/ruleadapters/creator.go b/pkg/rulemanager/ruleadapters/creator.go index 865eaee3c7..4f91a5b371 100644 --- a/pkg/rulemanager/ruleadapters/creator.go +++ b/pkg/rulemanager/ruleadapters/creator.go @@ -21,6 +21,7 @@ import ( "github.com/kubescape/node-agent/pkg/objectcache" "github.com/kubescape/node-agent/pkg/rulemanager/types" typesv1 "github.com/kubescape/node-agent/pkg/rulemanager/types/v1" + "github.com/kubescape/node-agent/pkg/rulestate" "github.com/kubescape/node-agent/pkg/utils" ) @@ -59,7 +60,7 @@ func NewRuleFailureCreator(enricher types.Enricher, dnsManager dnsmanager.DNSRes } } -func (r *RuleFailureCreator) CreateRuleFailure(rule typesv1.Rule, enrichedEvent *events.EnrichedEvent, objectCache objectcache.ObjectCache, message, uniqueID, apChecksum string, state map[string]any) types.RuleFailure { +func (r *RuleFailureCreator) CreateRuleFailure(rule typesv1.Rule, enrichedEvent *events.EnrichedEvent, objectCache objectcache.ObjectCache, message, uniqueID, apChecksum string, state map[string]any, hits []*rulestate.Entry) types.RuleFailure { eventAdapter, ok := r.adapterFactory.GetAdapter(enrichedEvent.Event.GetEventType()) if !ok { logger.L().Error("RuleFailureCreator - no adapter registered for event type", helpers.String("eventType", string(enrichedEvent.Event.GetEventType()))) @@ -103,9 +104,48 @@ func (r *RuleFailureCreator) CreateRuleFailure(rule typesv1.Rule, enrichedEvent }) } + // Set here rather than in an event adapter: SetFailureMetadata is + // per-event-type, whereas correlation is orthogonal to event type. + // + // Note what is deliberately NOT touched -- InfectedPID and + // RuntimeProcessDetails still describe the TRIGGERING event. Correlation + // enriches an incident, it does not re-key it, so backend incident grouping is + // unchanged by this. + setCorrelationEvidence(ruleFailure, hits) + return ruleFailure } +// setCorrelationEvidence copies the state entries the predicate actually read +// onto the alert, so a correlation alert describes BOTH ends of the chain. +// Without it the alert would say only "a process made an outbound connection" +// and drop the exec that makes it interesting. +func setCorrelationEvidence(ruleFailure *types.GenericRuleFailure, hits []*rulestate.Entry) { + if len(hits) == 0 { + return + } + ev := make([]armotypes.CorrelationEvidence, 0, len(hits)) + for _, h := range hits { + if h == nil { + continue + } + ev = append(ev, armotypes.CorrelationEvidence{ + Name: h.Name, + EventType: h.EventType, + Timestamp: h.Timestamp, + Scope: h.Scope, + Key: h.Key, + Process: h.Process, + Admission: h.Admission, + Values: h.Value, + }) + } + if len(ev) == 0 { + return + } + ruleFailure.SetCorrelationAlert(armotypes.CorrelationAlert{Correlations: ev}) +} + func (r *RuleFailureCreator) enrichRuleFailure(ruleFailure *types.GenericRuleFailure) { if r.enricher != nil && !reflect.ValueOf(r.enricher).IsNil() { if err := r.enricher.EnrichRuleFailure(ruleFailure); err != nil { diff --git a/pkg/rulemanager/ruleadapters/creator_interface.go b/pkg/rulemanager/ruleadapters/creator_interface.go index 3e0781a32e..04c7e613a6 100644 --- a/pkg/rulemanager/ruleadapters/creator_interface.go +++ b/pkg/rulemanager/ruleadapters/creator_interface.go @@ -5,10 +5,11 @@ import ( "github.com/kubescape/node-agent/pkg/objectcache" "github.com/kubescape/node-agent/pkg/rulemanager/types" typesv1 "github.com/kubescape/node-agent/pkg/rulemanager/types/v1" + "github.com/kubescape/node-agent/pkg/rulestate" ) type RuleFailureCreatorInterface interface { - CreateRuleFailure(rule typesv1.Rule, enrichedEvent *events.EnrichedEvent, objectCache objectcache.ObjectCache, message, uniqueID, apChecksum string, state map[string]any) types.RuleFailure + CreateRuleFailure(rule typesv1.Rule, enrichedEvent *events.EnrichedEvent, objectCache objectcache.ObjectCache, message, uniqueID, apChecksum string, state map[string]any, hits []*rulestate.Entry) types.RuleFailure } type EventMetadataSetter interface { diff --git a/pkg/rulemanager/types/failure.go b/pkg/rulemanager/types/failure.go index 9c632ec19a..361fa46f73 100644 --- a/pkg/rulemanager/types/failure.go +++ b/pkg/rulemanager/types/failure.go @@ -27,6 +27,7 @@ type GenericRuleFailure struct { Extra interface{} IsTriggerAlert bool SourceContext contextdetection.EventSourceContext + CorrelationAlert armotypes.CorrelationAlert } type RuleFailure interface { @@ -40,6 +41,8 @@ type RuleFailure interface { GetTriggerEvent() utils.EnrichEvent // Get Rule Description GetRuleAlert() armotypes.RuleAlert + // Get Correlation Alert -- the state entries this rule read to fire + GetCorrelationAlert() armotypes.CorrelationAlert // Get K8s Runtime Details GetRuntimeAlertK8sDetails() armotypes.RuntimeAlertK8sDetails // Get ECS Runtime Details @@ -85,6 +88,8 @@ type RuleFailure interface { SetIsTriggerAlert(isTriggerAlert bool) // Set Source Context SetSourceContext(sourceContext contextdetection.EventSourceContext) + // Set Correlation Alert + SetCorrelationAlert(correlationAlert armotypes.CorrelationAlert) } func (rule *GenericRuleFailure) GetBaseRuntimeAlert() armotypes.BaseRuntimeAlert { @@ -103,6 +108,14 @@ func (rule *GenericRuleFailure) GetRuleAlert() armotypes.RuleAlert { return rule.RuleAlert } +func (rule *GenericRuleFailure) GetCorrelationAlert() armotypes.CorrelationAlert { + return rule.CorrelationAlert +} + +func (rule *GenericRuleFailure) SetCorrelationAlert(correlationAlert armotypes.CorrelationAlert) { + rule.CorrelationAlert = correlationAlert +} + func (rule *GenericRuleFailure) GetRuntimeAlertK8sDetails() armotypes.RuntimeAlertK8sDetails { return rule.RuntimeAlertK8sDetails } From 48145fdbadb6f9ef49c0acd3e8aee8861ae7c015 Mon Sep 17 00:00:00 2001 From: Ben Date: Thu, 30 Jul 2026 14:53:10 +0300 Subject: [PATCH 08/17] feat(rulestate): wire config defaults, metrics and container-removal purge Adds the celStateStore config defaults and immediate scope purge on container removal, so a churning node does not hold markers for containers that no longer exist. The purge deliberately uses Runtime.ContainerID VERBATIM. The plan specified utils.TrimRuntimePrefix here, which would have been actively destructive: that helper returns "" for an ID with no "//" separator, a bare runtime container ID has none, and ContainerScopeID("") resolves to the HOST bucket -- so every container exit would have wiped all host-process state instead of that container's. The write path stores under the untrimmed Runtime.ContainerID (EnrichedEvent.ContainerID is assigned from it in containercallback.go), so untrimmed is also the only form that matches. TestContainerScopeID_TrimmedRuntimeIDWouldHitTheHostBucket pins the trap. main.go needs no change: the store is constructed inside CreateRuleManager, which already has the ctx to run the sweeper on, and NewCEL does not need the store because the per-rule Accessor carries it. state_join_fired_total is NOT added. It has no call site until plan 3's bidirectional component test exercises it, and an unwired metric is worse than a noted gap. Signed-off-by: Ben --- docs/features/cel-rule-state-store.md | 41 +++++++++++++++++++++++-- pkg/config/config.go | 11 +++++++ pkg/config/config_test.go | 10 +++++++ pkg/rulemanager/containercallbacks.go | 16 ++++++++++ pkg/rulemanager/statecontext_test.go | 43 +++++++++++++++++++++++++++ 5 files changed, 118 insertions(+), 3 deletions(-) diff --git a/docs/features/cel-rule-state-store.md b/docs/features/cel-rule-state-store.md index 2866c6ef59..a7cf613111 100644 --- a/docs/features/cel-rule-state-store.md +++ b/docs/features/cel-rule-state-store.md @@ -8,9 +8,11 @@ delete of a pod — cannot be expressed at all. Design: `shared-designs-and-docs/projects/2026-07-28-cel-rule-state-store/spec.md`. -> **Status: under construction.** Reads, writes and alert evidence all work. -> Still to land: scope purge on container removal, and the end-to-end component -> test against real eBPF events. Not yet exercised on a live cluster. +> **Status: complete in-tree, not yet proven on a cluster.** Reads, writes, alert +> evidence, config, metrics and container-removal purge all work and are unit +> tested. Still outstanding: the end-to-end component test against real eBPF +> events. **No rule has been run against a live agent yet**, so treat the +> behaviour described here as tested-by-construction rather than field-proven. ## Writing state @@ -205,3 +207,36 @@ as before, with no `correlations` key. `state.get()` in a message resolves against the same entries the predicate matched — and `uniqueId` can be derived from the join key, which is what lets cooldown collapse both legs of a bidirectional rule into one alert. + +## Configuration + +```yaml +celStateStore: + enabled: true + maxSize: 100000 # node-wide ceiling (approximate under concurrency) + maxEntriesPerContainer: 256 # exact, per container + maxEntriesForHost: 4096 # the c:__host__ bucket + maxTtl: 30m # every rule's ttl is clamped to this + sweepInterval: 30s + ancestorMaxDepth: 8 # probes per has_ancestor call +``` + +Disabling it makes writes no-ops and every read a miss, so correlation rules stop +firing while ordinary rules are unaffected. + +A container's entries are purged as soon as the container is removed, rather than +waiting for TTL. The host bucket gets no such purge — it relies on TTL, which is +why its cap is larger. + +## Metrics + +| Metric | Meaning | +|---|---| +| `node_agent_state_writes_total{rule_id,result}` | Entries written | +| `node_agent_state_write_rejected_total{rule_id,reason}` | **Alert on this** — a rule is being starved of the state it needs | +| `node_agent_state_expired_total` | Reclaimed by TTL | +| `node_agent_state_purged_total` | Dropped by scope purge | +| `node_agent_state_entries{scope}` | Current entry count | + +Counters are labelled by rule ID only, never by state key — a key is unbounded +cardinality. diff --git a/pkg/config/config.go b/pkg/config/config.go index cda1a5920e..dfcd6f39aa 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -211,6 +211,17 @@ func LoadConfigOptional(path string, errNotFound bool) (Config, error) { viper.SetDefault("blockEvents", false) viper.SetDefault("celConfigCache::maxSize", 100000) viper.SetDefault("celConfigCache::ttl", 1*time.Minute) + + // CEL rule state store. maxEntriesForHost is larger than the per-container cap + // because the host bucket holds the whole node's process space rather than one + // workload, and never receives a container-removal purge -- it relies on TTL. + viper.SetDefault("celStateStore::enabled", true) + viper.SetDefault("celStateStore::maxSize", 100000) + viper.SetDefault("celStateStore::maxEntriesPerContainer", 256) + viper.SetDefault("celStateStore::maxEntriesForHost", 4096) + viper.SetDefault("celStateStore::maxTtl", 30*time.Minute) + viper.SetDefault("celStateStore::sweepInterval", 30*time.Second) + viper.SetDefault("celStateStore::ancestorMaxDepth", 8) viper.SetDefault("ignoreRuleBindings", false) viper.SetDefault("eventDedup::enabled", true) diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 754b342279..105e8159fd 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -10,6 +10,7 @@ import ( processtreecreator "github.com/kubescape/node-agent/pkg/processtree/config" "github.com/kubescape/node-agent/pkg/rulemanager/cel/libraries/cache" "github.com/kubescape/node-agent/pkg/rulemanager/rulecooldown" + "github.com/kubescape/node-agent/pkg/rulestate" "github.com/spf13/viper" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -101,6 +102,15 @@ func TestLoadConfig(t *testing.T) { MaxSize: 100000, TTL: 1 * time.Minute, }, + CelStateStore: rulestate.Config{ + Enabled: true, + MaxSize: 100000, + MaxEntriesPerContainer: 256, + MaxEntriesForHost: 4096, + MaxTTL: 30 * time.Minute, + SweepInterval: 30 * time.Second, + AncestorMaxDepth: 8, + }, DNSCacheSize: 50000, ContainerEolNotificationBuffer: 100, FIM: FIMConfig{ diff --git a/pkg/rulemanager/containercallbacks.go b/pkg/rulemanager/containercallbacks.go index f54899ef0f..d0c0344784 100644 --- a/pkg/rulemanager/containercallbacks.go +++ b/pkg/rulemanager/containercallbacks.go @@ -12,6 +12,7 @@ import ( "github.com/kubescape/go-logger/helpers" "github.com/kubescape/node-agent/pkg/contextdetection/detectors" "github.com/kubescape/node-agent/pkg/objectcache" + "github.com/kubescape/node-agent/pkg/rulestate" "github.com/kubescape/node-agent/pkg/utils" ) @@ -102,6 +103,21 @@ func (rm *RuleManager) ContainerCallback(notif containercollection.PubSubEvent) } rm.trackedContainers.Remove(k8sContainerID) + + // Reclaim immediately rather than waiting for TTL: a churning node would + // otherwise hold markers for containers that no longer exist. + // + // This uses Runtime.ContainerID verbatim because that is exactly what the + // write path stored under -- EnrichedEvent.ContainerID is assigned from + // container.Runtime.ContainerID (containercallback.go), untrimmed. Do NOT + // pass it through utils.TrimRuntimePrefix: that helper returns "" for an ID + // with no "//" separator, and ContainerScopeID("") is the HOST bucket, so + // trimming here would purge every host process marker on each container + // exit. + if rm.stateStore != nil { + rm.stateStore.PurgeScope(rulestate.ContainerScopeID(notif.Container.Runtime.ContainerID)) + } + namespace := notif.Container.K8s.Namespace podName := notif.Container.K8s.PodName podID := utils.CreateK8sPodID(namespace, podName) diff --git a/pkg/rulemanager/statecontext_test.go b/pkg/rulemanager/statecontext_test.go index 7fec8ec2bc..618431d7ec 100644 --- a/pkg/rulemanager/statecontext_test.go +++ b/pkg/rulemanager/statecontext_test.go @@ -179,3 +179,46 @@ func TestHasWriteFor(t *testing.T) { assert.False(t, hasWriteFor(compiled, utils.NetworkEventType)) assert.False(t, hasWriteFor(nil, utils.ExecveEventType)) } + +// Container removal must reclaim that container's markers immediately -- a +// churning node would otherwise hold state for containers that no longer exist +// until TTL. +func TestPurgeScope_OnContainerRemovalDropsOnlyThatContainer(t *testing.T) { + rm := testRuleManager(t) + rm.stateStore = rulestate.NewStore(rm.cfg.CelStateStore, rulestate.NoopMetrics{}) + + set := func(scopeID string) { + require.NoError(t, rm.stateStore.Set(&rulestate.Entry{ + RuleID: "R1089", Name: "n", Key: "1", + Scope: armotypes.StateScopeContainer, ScopeID: scopeID, + Timestamp: time.Now(), ExpiresAt: time.Now().Add(time.Minute), + })) + } + set(rulestate.ContainerScopeID("abc")) + set(rulestate.ContainerScopeID("def")) + set(rulestate.HostScopeID()) + + rm.stateStore.PurgeScope(rulestate.ContainerScopeID("abc")) + + _, ok := rm.stateStore.Get("R1089", armotypes.StateScopeContainer, rulestate.ContainerScopeID("abc"), "n", "1") + assert.False(t, ok, "the removed container's markers must be gone") + + _, ok = rm.stateStore.Get("R1089", armotypes.StateScopeContainer, rulestate.ContainerScopeID("def"), "n", "1") + assert.True(t, ok, "a neighbouring container must be untouched") + + _, ok = rm.stateStore.Get("R1089", armotypes.StateScopeContainer, rulestate.HostScopeID(), "n", "1") + assert.True(t, ok, "host markers must survive a container removal") +} + +// The trap this guards: utils.TrimRuntimePrefix returns "" for an ID with no +// "//" separator, and ContainerScopeID("") is the HOST bucket. If the removal +// path ever trims the runtime container ID again, every container exit would wipe +// all host-process state instead of that container's. +func TestContainerScopeID_TrimmedRuntimeIDWouldHitTheHostBucket(t *testing.T) { + bare := "1a2b3c4d5e6f" + assert.Empty(t, utils.TrimRuntimePrefix(bare), + "TrimRuntimePrefix yields empty for a bare runtime ID") + assert.Equal(t, rulestate.HostScopeID(), rulestate.ContainerScopeID(utils.TrimRuntimePrefix(bare)), + "which would resolve to the host bucket -- purge must use the untrimmed ID") + assert.NotEqual(t, rulestate.HostScopeID(), rulestate.ContainerScopeID(bare)) +} From bf88da75ed5d4b985ebf914f1b8ddfc7b614ae99 Mon Sep 17 00:00:00 2001 From: Ben Date: Thu, 30 Jul 2026 14:59:30 +0300 Subject: [PATCH 09/17] fix(rulemanager): keep ReportRuleProcessed semantics across the loop refactor The pre-refactor loop reached ReportRuleProcessed only by falling off the end, so an eval error or a cooldown-suppressed alert did not count as processed. Those were continues; extracting the alert path turned them into returns, which would have silently started counting them -- a changed metric meaning for every existing rule, and a violation of the plan's byte-for-byte constraint for rules with no stateWrites. evaluateRuleAndAlert now reports whether it ran to completion and the caller gates the metric on it. A predicate that simply did not match still counts as processed, exactly as the old fall-through did. Docs-exempt: restores pre-existing metric semantics; no documented behaviour changes Signed-off-by: Ben --- pkg/rulemanager/rule_manager.go | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/pkg/rulemanager/rule_manager.go b/pkg/rulemanager/rule_manager.go index d00eb13b7d..ef1c6167ae 100644 --- a/pkg/rulemanager/rule_manager.go +++ b/pkg/rulemanager/rule_manager.go @@ -397,8 +397,14 @@ func (rm *RuleManager) ReportEnrichedEvent(enrichedEvent *events.EnrichedEvent) // would break the NEXT leg of the chain. The predicate and alert path is // therefore its own function -- its early exits return, and the writes // below still run. + // processed mirrors the pre-refactor control flow exactly: the old loop + // reached ReportRuleProcessed only by falling off the end, so an + // eval error or a cooldown-suppressed alert did NOT count as processed. + // Those were continues; they are returns now, so the metric has to be + // gated or its meaning would silently change for every existing rule. + processed := true if len(ruleExpressions) > 0 { - rm.evaluateRuleAndAlert(evaluateArgs{ + processed = rm.evaluateRuleAndAlert(evaluateArgs{ rule: rule, ruleExpressions: ruleExpressions, enrichedEvent: enrichedEvent, @@ -420,7 +426,9 @@ func (rm *RuleManager) ReportEnrichedEvent(enrichedEvent *events.EnrichedEvent) cel.ResolveEventTime(enrichedEvent)) } - rm.metrics.ReportRuleProcessed(rule.ID) + if processed { + rm.metrics.ReportRuleProcessed(rule.ID) + } } } @@ -442,7 +450,10 @@ type evaluateArgs struct { // Split out of the rule loop so that every early exit in here is a return rather // than a continue, which leaves the caller free to run the rule's state writes // afterwards regardless of whether an alert was emitted or suppressed. -func (rm *RuleManager) evaluateRuleAndAlert(a evaluateArgs) { +// The bool reports whether the path ran to completion. The caller uses it to +// decide whether to count the rule as processed, preserving the metric's +// pre-refactor meaning. +func (rm *RuleManager) evaluateRuleAndAlert(a evaluateArgs) bool { rule := a.rule enrichedEvent := a.enrichedEvent evalContext := a.evalContext @@ -485,11 +496,13 @@ func (rm *RuleManager) evaluateRuleAndAlert(a evaluateArgs) { if err != nil { logger.L().Ctx(errCtx).Error("RuleManager.ReportEnrichedEvent - failed to evaluate rule", helpers.Error(err), helpers.String("rule", rule.ID), helpers.String("eventType", string(eventType))) rm.metrics.ReportAlertSuppressed(rule.ID, "eval_error") - return + return false } + // A predicate that simply did not match still counts as processed, exactly as + // it did when this was a fall-through rather than a return. if !shouldAlert { - return + return true } // ruleState, not "state": the local would otherwise shadow the state @@ -502,12 +515,12 @@ func (rm *RuleManager) evaluateRuleAndAlert(a evaluateArgs) { message, uniqueID, err := rm.getUniqueIdAndMessage(evalContext, rule) if err != nil { logger.L().Error("RuleManager - failed to get unique ID and message", helpers.Error(err)) - return + return false } if shouldCooldown, _ := rm.ruleCooldown.ShouldCooldown(uniqueID, enrichedEvent.ContainerID, rule.ID); shouldCooldown { rm.metrics.ReportAlertSuppressed(rule.ID, "cooldown") - return + return false } // Emit OTEL log after cooldown so suppressed alerts are not recorded. @@ -557,11 +570,12 @@ func (rm *RuleManager) evaluateRuleAndAlert(a evaluateArgs) { helpers.String("uniqueID", uniqueID), helpers.String("enrichedEvent.EventType", string(eventType)), ) - return + return false } ruleFailure.SetWorkloadDetails(a.details) rm.exporter.SendRuleAlert(ruleFailure) + return true } func (rm *RuleManager) enrichEventWithContext(enrichedEvent *events.EnrichedEvent) { From f2905979cd6f466793a9967310a32f5557a9d1c3 Mon Sep 17 00:00:00 2001 From: Ben Date: Mon, 3 Aug 2026 11:56:48 +0300 Subject: [PATCH 10/17] test(component): add CEL state store test rules and workload Four rules rather than one, for the same reason the TTY test has four: a CEL expression that fails to compile returns (false, nil) rather than erroring, so 'no alert' is ambiguous between 'the predicate was false' and 'the rule never ran'. R9912/R9913 must always fire and R9914 must never, which turns a silent R9911 into a diagnosable result instead of a mystery. R9911 deliberately has no exec ruleExpression -- only a stateWrites clause on exec -- so it also exercises write-without-alerting, the shape every cross-event rule depends on. Docs-exempt: test fixtures only Signed-off-by: Ben --- tests/resources/cel-state-deployment.yaml | 22 ++++ tests/resources/cel-state-rulebinding.yaml | 22 ++++ tests/resources/cel-state-rules.yaml | 123 +++++++++++++++++++++ 3 files changed, 167 insertions(+) create mode 100644 tests/resources/cel-state-deployment.yaml create mode 100644 tests/resources/cel-state-rulebinding.yaml create mode 100644 tests/resources/cel-state-rules.yaml diff --git a/tests/resources/cel-state-deployment.yaml b/tests/resources/cel-state-deployment.yaml new file mode 100644 index 0000000000..541fdf57a3 --- /dev/null +++ b/tests/resources/cel-state-deployment.yaml @@ -0,0 +1,22 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + labels: + app: cel-state-app + name: cel-state-deployment +spec: + selector: + matchLabels: + app: cel-state-app + replicas: 1 + template: + metadata: + labels: + app: cel-state-app + spec: + containers: + # alpine's busybox provides nc, so the trigger needs no package install -- + # the kind node may have no outbound internet. + - name: probe + image: alpine:3.20 + command: ["sleep", "infinity"] diff --git a/tests/resources/cel-state-rulebinding.yaml b/tests/resources/cel-state-rulebinding.yaml new file mode 100644 index 0000000000..bc3b7e88d7 --- /dev/null +++ b/tests/resources/cel-state-rulebinding.yaml @@ -0,0 +1,22 @@ +# Binds the test-only state-store rules. The shipped default binding +# (chart/templates/node-agent/default-rule-binding.yaml) lists rules by explicit +# ruleName, so newly added rule IDs are inert until something binds them. +apiVersion: kubescape.io/v1 +kind: RuntimeRuleAlertBinding +metadata: + name: cel-state-test-binding +spec: + namespaceSelector: + matchExpressions: + - key: "kubernetes.io/metadata.name" + operator: "NotIn" + values: + - "kube-system" + - "kube-public" + - "kube-node-lease" + podSelector: + rules: + - ruleName: "TEST state correlation exec then connect" + - ruleName: "TEST state network control" + - ruleName: "TEST state exec control" + - ruleName: "TEST state negative control" diff --git a/tests/resources/cel-state-rules.yaml b/tests/resources/cel-state-rules.yaml new file mode 100644 index 0000000000..c85fafa7b0 --- /dev/null +++ b/tests/resources/cel-state-rules.yaml @@ -0,0 +1,123 @@ +# Test-only rules validating the CEL state store against real eBPF events. +# Applied by Test_36_CelStateStoreCorrelation and deleted on cleanup. IDs are in +# a deliberately test-only 99xx range so they cannot collide with the shipped +# R1xxx/R2xxx ranges. +# +# Four rules, because "no alert" is ambiguous on its own: an unresolvable CEL +# field or function does not error, it fails to compile and silently disables the +# whole expression (pkg/rulemanager/cel returns (false, nil) on compile failure), +# which looks exactly like "the predicate was false". +# +# R9911 The correlation rule under test. Writes on exec, alerts on network. +# It has NO exec ruleExpression at all -- that is the +# write-without-alerting shape every cross-event rule depends on. +# R9912 Network control. Fires on the same connection with no state +# predicate. If R9912 is silent the network leg never reached the rule +# loop, so R9911's silence says nothing about the state store. +# R9913 Exec control. Fires on the marker exec. Its message carries the pid, +# which is how the test verifies the exec and network legs share a pid. +# R9914 Negative control. Reads a name that is never written. Must NOT fire. +# If it does, state.has is returning true spuriously and R9911 proves +# nothing. +# +# All rules use profileDependency 2 (NotRequired) so the test never waits for +# application-profile completion, and uniqueId includes the pid so per-rule +# cooldown cannot swallow a later probe. +apiVersion: kubescape.io/v1 +kind: Rules +metadata: + name: cel-state-test-rules + namespace: kubescape + labels: + app: kubescape +spec: + rules: + - name: "TEST state correlation exec then connect" + enabled: true + id: "R9911" + description: "Test rule: remembers a marker exec, then alerts on an outbound connection from the same pid." + stateWrites: + - eventType: "exec" + scope: "container" + name: "probe_exec" + key: "string(event.pid)" + value: + probeComm: "event.comm" + ttl: "5m" + when: "event.comm == 'sh' && event.args.exists(a, a.contains('CELSTATE_MARKER'))" + expressions: + message: "'state correlation: pid=' + string(event.pid) + ' comm=' + event.comm + ' remembered=' + state.get('probe_exec', string(event.pid)).probeComm" + uniqueId: "'R9911_' + string(event.pid)" + ruleExpression: + - eventType: "network" + expression: | + event.pktType == 'OUTGOING' && state.has('probe_exec', string(event.pid)) + profileDependency: 2 + severity: 1 + supportPolicy: false + isTriggerAlert: true + mitreTactic: "TA0011" + mitreTechnique: "T1071" + tags: + - "test" + - "state" + - name: "TEST state network control" + enabled: true + id: "R9912" + description: "Test control rule: same outbound connection, no state predicate. Must fire whenever the probe connects." + expressions: + message: "'network control: pid=' + string(event.pid) + ' comm=' + event.comm" + uniqueId: "'R9912_' + string(event.pid)" + ruleExpression: + - eventType: "network" + expression: | + event.pktType == 'OUTGOING' && event.comm == 'nc' + profileDependency: 2 + severity: 1 + supportPolicy: false + isTriggerAlert: true + mitreTactic: "TA0011" + mitreTechnique: "T1071" + tags: + - "test" + - "state" + - name: "TEST state exec control" + enabled: true + id: "R9913" + description: "Test control rule: fires on the marker exec. Its message carries the pid, so the test can confirm both legs share one." + expressions: + message: "'exec control: pid=' + string(event.pid) + ' comm=' + event.comm" + uniqueId: "'R9913_' + string(event.pid)" + ruleExpression: + - eventType: "exec" + expression: | + event.comm == 'sh' && event.args.exists(a, a.contains('CELSTATE_MARKER')) + profileDependency: 2 + severity: 1 + supportPolicy: false + isTriggerAlert: true + mitreTactic: "TA0002" + mitreTechnique: "T1059" + tags: + - "test" + - "state" + - name: "TEST state negative control" + enabled: true + id: "R9914" + description: "Test negative control: reads a state name no rule ever writes. Must never fire." + expressions: + message: "'NEGATIVE CONTROL FIRED: pid=' + string(event.pid)" + uniqueId: "'R9914_' + string(event.pid)" + ruleExpression: + - eventType: "network" + expression: | + event.pktType == 'OUTGOING' && state.has('never_written', string(event.pid)) + profileDependency: 2 + severity: 1 + supportPolicy: false + isTriggerAlert: true + mitreTactic: "TA0011" + mitreTechnique: "T1071" + tags: + - "test" + - "state" From c54bb3a4469724b0986faaf519c14eada619380a Mon Sep 17 00:00:00 2001 From: Ben Date: Mon, 3 Aug 2026 11:56:57 +0300 Subject: [PATCH 11/17] test(component): prove the CEL state store end-to-end against real eBPF The alert's existence is the proof: R9911's network-leg predicate is state.has(...), so if the store does not work no alert is emitted. That needs only the existing Alertmanager label assertion -- no payload receiver. The trigger puts 8 seconds between the write and the read by sleeping inside the shell and then exec-ing nc, which replaces the image without forking so the pid is stable across both legs. Without that gap the two events are milliseconds apart and node-agent evaluates on a concurrent worker pool, so a failure could be reordering rather than a defect. Controls are asserted before the correlation rule on purpose: if they are silent their messages are the only diagnostic, and they are gone once the test fails. Docs-exempt: test only Signed-off-by: Ben --- tests/component_test.go | 98 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 98 insertions(+) diff --git a/tests/component_test.go b/tests/component_test.go index 1477a17496..1bd155ccee 100644 --- a/tests/component_test.go +++ b/tests/component_test.go @@ -3595,3 +3595,101 @@ func Test_35_ExecTTYFieldTest(t *testing.T) { assert.Greater(t, total(alerts, "R9904"), 0, "R9904 must fire: !has(event.ttyMajor) proves ttyMajor is a registered field that is honestly absent, not a compile failure") } + +// Test_36_CelStateStoreCorrelation is the end-to-end proof of the CEL state +// store: a rule that remembers a fact on one event stream and reads it back on +// another, against real eBPF events. +// +// The proof does not need to inspect the alert payload. R9911's network-leg +// predicate is state.has(...), so if the store does not work the predicate is +// false and no alert is emitted at all. The alert's EXISTENCE is the proof. +// Asserting the correlations[] evidence payload needs a payload-level receiver +// and is a separate, optional tier. +// +// The trigger puts an 8-second gap between the write and the read: +// +// sh -c '# CELSTATE_MARKER; sleep 8; exec nc -w 3 $HOST $PORT' +// +// T=0 the shell execs with the marker in argv -> R9911 writes state under pid P. +// T=8 `exec nc` replaces the shell's image IN THE SAME PID (exec does not fork), +// and nc connects -> the network event carries pid P and the read hits. +// +// Without that gap the exec and the connect are milliseconds apart, and +// node-agent evaluates events on a concurrent worker pool -- so a failure could +// be reordering rather than a defect. With it, a failure is a real defect. +func Test_36_CelStateStoreCorrelation(t *testing.T) { + start := time.Now() + defer tearDownTest(t, start) + + rulesPath := path.Join(utils.CurrentDir(), "resources/cel-state-rules.yaml") + bindingPath := path.Join(utils.CurrentDir(), "resources/cel-state-rulebinding.yaml") + require.Equal(t, 0, testutils.RunCommand("kubectl", "apply", "--validate=false", "-f", rulesPath), "apply state test rules") + defer testutils.RunCommand("kubectl", "delete", "--ignore-not-found", "-f", rulesPath) + require.Equal(t, 0, testutils.RunCommand("kubectl", "apply", "--validate=false", "-f", bindingPath), "apply state test rule binding") + defer testutils.RunCommand("kubectl", "delete", "--ignore-not-found", "-f", bindingPath) + // let the rules watcher and rule-binding watcher pick the new rules up + time.Sleep(20 * time.Second) + + ns := testutils.NewRandomNamespace() + wl, err := testutils.NewTestWorkload(ns.Name, path.Join(utils.CurrentDir(), "resources/cel-state-deployment.yaml")) + require.NoError(t, err, "Error creating workload") + require.NoError(t, wl.WaitForReady(80)) + time.Sleep(15 * time.Second) + + // Confirm nc exists before relying on it; without this a missing applet looks + // exactly like the state store failing. + _, _, err = wl.ExecIntoPodNoTTY([]string{"sh", "-c", "command -v nc"}, "probe") + require.NoError(t, err, "busybox nc must be present in the probe container") + + // Three probes rather than one. Each is independent (its own pid, its own + // state key), so a single flake does not fail the run, and three silent + // probes is clearly systematic rather than a race. + const probes = 3 + trigger := `# CELSTATE_MARKER +sleep 8 +exec nc -w 3 "$KUBERNETES_SERVICE_HOST" "$KUBERNETES_SERVICE_PORT"` + + for i := 0; i < probes; i++ { + go func() { + // nc is expected to be closed by the peer or time out; the connection + // attempt is the signal, its outcome is irrelevant. + _, _, _ = wl.ExecIntoPodNoTTY([]string{"sh", "-c", trigger}, "probe") + }() + time.Sleep(1 * time.Second) + } + + // 8s sleep + connect + export + Alertmanager group interval. + t.Log("waiting for the probes to connect and their alerts to land") + time.Sleep(60 * time.Second) + + alerts, err := testutils.GetAlerts(wl.Namespace) + require.NoError(t, err, "Error getting alerts") + + count := func(ruleID string) int { + n := 0 + for _, a := range alerts { + if a.Labels["rule_id"] == ruleID { + n++ + } + } + return n + } + for _, a := range alerts { + t.Logf("alert rule_id=%s rule_name=%q", a.Labels["rule_id"], a.Labels["rule_name"]) + } + + // Controls FIRST. If either is silent, R9911's silence says nothing about + // the state store, and these messages are the only diagnostic available. + require.Greater(t, count("R9913"), 0, + "exec control did not fire: the marker exec never reached the rule loop, so this test cannot say anything about state") + require.Greater(t, count("R9912"), 0, + "network control did not fire: the outbound connection never reached the rule loop, so this test cannot say anything about state") + + // The actual proof. + assert.Greater(t, count("R9911"), 0, + "correlation rule never fired: state written on exec was not readable on the network event") + + // And the negative control, which makes the assertion above meaningful. + assert.Equal(t, 0, count("R9914"), + "negative control fired: state.has returned true for a name no rule writes, so R9911 proves nothing") +} From a0aaa7cc5217566635c62482ea26a064ce83e69f Mon Sep 17 00:00:00 2001 From: Ben Date: Mon, 3 Aug 2026 11:56:57 +0300 Subject: [PATCH 12/17] ci: run the state-store and exec-TTY component tests Test_35 was written but never added to the matrix, so the TTY field has only ever been verified by hand. Both are wired in now. Docs-exempt: CI configuration only Signed-off-by: Ben --- .github/workflows/component-tests.yaml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/component-tests.yaml b/.github/workflows/component-tests.yaml index ab410177ab..52d5192c17 100644 --- a/.github/workflows/component-tests.yaml +++ b/.github/workflows/component-tests.yaml @@ -74,7 +74,9 @@ jobs: Test_24_ProcessTreeDepthTest, Test_27_ApplicationProfileOpens, Test_32_UnexpectedProcessArguments, - Test_34_NetworkNeighborsCIDRCollapse + Test_34_NetworkNeighborsCIDRCollapse, + Test_35_ExecTTYFieldTest, + Test_36_CelStateStoreCorrelation ] steps: - name: Checkout code From 577791d2e1555fe871f80865e833e5949d5a754f Mon Sep 17 00:00:00 2001 From: Ben Date: Mon, 3 Aug 2026 12:18:16 +0300 Subject: [PATCH 13/17] fix(crd): add stateWrites to the Rules schema, without which it is pruned The Rules CRD has a structural schema and no x-kubernetes-preserve-unknown-fields at the rule level, so the API server SILENTLY STRIPPED stateWrites on write. The field never reached node-agent, and every correlation rule loaded cleanly and then never fired. Nothing in the Go code was wrong. Every unit test passed -- including a new one added here that drives the real production CEL env -- because none of them go through the API server. Only the component test found it, which is exactly what it was written for. Verified directly: before this change `kubectl get rules ... -o jsonpath={.spec.rules[0].stateWrites}` returned null after a successful apply. statewiring_test.go is the regression test that was missing. The state library's own tests build a bare cel.NewEnv; production builds an env with an xcel TypeAdapter/TypeProvider, every other library and a static optimizer. This drives the write guard, the key expression and the cross-leg read through THAT env, so a wiring problem that only appears in the real evaluator is caught in unit tests rather than on a cluster. IMPORTANT -- this fixes only the copy of the CRD in tests/chart. The canonical Rules CRD ships from the kubescape/helm-charts repo, and the same property must be added there or the feature is inert in production no matter what node-agent does. Docs-exempt: CRD schema fix; the feature page already documents stateWrites Signed-off-by: Ben --- pkg/rulemanager/cel/statewiring_test.go | 144 ++++++++++++++++++++++++ tests/chart/crds/rules.crd.yaml | 39 +++++++ 2 files changed, 183 insertions(+) create mode 100644 pkg/rulemanager/cel/statewiring_test.go diff --git a/pkg/rulemanager/cel/statewiring_test.go b/pkg/rulemanager/cel/statewiring_test.go new file mode 100644 index 0000000000..e1f99537ff --- /dev/null +++ b/pkg/rulemanager/cel/statewiring_test.go @@ -0,0 +1,144 @@ +package cel + +import ( + "testing" + "time" + + "github.com/armosec/armoapi-go/armotypes" + "github.com/kubescape/node-agent/pkg/config" + "github.com/kubescape/node-agent/pkg/ebpf/events" + "github.com/kubescape/node-agent/pkg/objectcache" + "github.com/kubescape/node-agent/pkg/rulemanager/cel/libraries/state" + "github.com/kubescape/node-agent/pkg/rulestate" + "github.com/kubescape/node-agent/pkg/utils" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The state library's own tests build a bare cel.NewEnv. Production does not: +// NewCEL installs an xcel TypeAdapter/TypeProvider, every other library, and a +// static optimizer. This test exercises the state functions through THAT env, so +// a wiring problem that only appears in the real evaluator is caught here rather +// than on a cluster. +func newStateWiringCEL(t *testing.T) (*CEL, *rulestate.Store, config.Config) { + t.Helper() + + cfg := config.Config{} + cfg.CelStateStore = rulestate.Config{ + Enabled: true, + MaxSize: 1000, + MaxEntriesPerContainer: 100, + MaxEntriesForHost: 100, + MaxTTL: 30 * time.Minute, + AncestorMaxDepth: 8, + } + + c, err := NewCEL(objectcache.NewObjectCacheMock(), cfg) + require.NoError(t, err) + + return c, rulestate.NewStore(cfg.CelStateStore, rulestate.NoopMetrics{}), cfg +} + +func execProbeEvent(pid uint32) *events.EnrichedEvent { + return &events.EnrichedEvent{ + Event: &utils.StructEvent{ + EventType: utils.ExecveEventType, + ContainerID: "abc", + Comm: "sh", + Args: []string{"-c", "# CELSTATE_MARKER\nsleep 8\nexec nc -w 3 h p"}, + Pid: pid, + }, + ContainerID: "abc", + PID: pid, + } +} + +func networkProbeEvent(pid uint32) *events.EnrichedEvent { + return &events.EnrichedEvent{ + Event: &utils.StructEvent{ + EventType: utils.NetworkEventType, + ContainerID: "abc", + Comm: "nc", + PktType: "OUTGOING", + Pid: pid, + }, + ContainerID: "abc", + PID: pid, + } +} + +func seedState(c *CEL, ctx map[string]any, store *rulestate.Store, ee *events.EnrichedEvent, tracker *state.ReadTracker) { + ctx[state.AccessorContextKey] = state.NewAccessor( + store, "R9911", + map[string]armotypes.StateScope{"probe_exec": armotypes.StateScopeContainer}, + map[armotypes.StateScope]string{ + armotypes.StateScopeContainer: rulestate.ContainerScopeID(ee.ContainerID), + }, + func() []uint32 { return nil }, + tracker, nil, + ) +} + +// The exact predicates the R9911 component-test rule uses. +const ( + probeGuard = `event.comm == 'sh' && event.args.exists(a, a.contains('CELSTATE_MARKER'))` + probeRead = `event.pktType == 'OUTGOING' && state.has('probe_exec', string(event.pid))` +) + +func TestStateWiring_GuardCompilesAndMatchesInTheRealEnv(t *testing.T) { + c, _, _ := newStateWiringCEL(t) + + ctx := c.CreateEvalContext(execProbeEvent(4471)) + ok, err := c.EvaluateBoolExpressionWithContext(ctx, probeGuard) + require.NoError(t, err) + assert.True(t, ok, "the stateWrites guard must match the marker exec") +} + +func TestStateWiring_KeyExpressionEvaluates(t *testing.T) { + c, _, _ := newStateWiringCEL(t) + + ctx := c.CreateEvalContext(execProbeEvent(4471)) + key, err := c.EvaluateStringExpressionWithContext(ctx, "string(event.pid)") + require.NoError(t, err) + assert.Equal(t, "4471", key, "the join key must render as the bare pid") +} + +// The end-to-end shape: write on exec, read on network, through the production +// evaluator. This is the unit-level equivalent of the component test. +func TestStateWiring_WriteOnExecThenReadOnNetwork(t *testing.T) { + c, store, _ := newStateWiringCEL(t) + tracker := &state.ReadTracker{} + + // --- exec leg: evaluate the guard, then store the entry the executor would. + execEvent := execProbeEvent(4471) + execCtx := c.CreateEvalContext(execEvent) + seedState(c, execCtx, store, execEvent, tracker) + + guardOK, err := c.EvaluateBoolExpressionWithContext(execCtx, probeGuard) + require.NoError(t, err) + require.True(t, guardOK, "guard must match, or the write never happens") + + key, err := c.EvaluateStringExpressionWithContext(execCtx, "string(event.pid)") + require.NoError(t, err) + + now := time.Now() + require.NoError(t, store.Set(&rulestate.Entry{ + RuleID: "R9911", Name: "probe_exec", Key: key, + Scope: armotypes.StateScopeContainer, + ScopeID: rulestate.ContainerScopeID(execEvent.ContainerID), + EventType: armotypes.EventTypeExec, + Timestamp: now, ExpiresAt: now.Add(5 * time.Minute), + Process: &armotypes.Process{PID: 4471, Comm: "sh"}, + Value: map[string]any{"probeComm": "sh"}, + })) + + // --- network leg: the predicate must now see it. + netEvent := networkProbeEvent(4471) + netCtx := c.CreateEvalContext(netEvent) + seedState(c, netCtx, store, netEvent, tracker) + + fired, err := c.EvaluateBoolExpressionWithContext(netCtx, probeRead) + require.NoError(t, err) + assert.True(t, fired, + "state written on the exec leg must be readable on the network leg with the same pid") +} diff --git a/tests/chart/crds/rules.crd.yaml b/tests/chart/crds/rules.crd.yaml index 90d5d56712..94aefa6825 100644 --- a/tests/chart/crds/rules.crd.yaml +++ b/tests/chart/crds/rules.crd.yaml @@ -71,6 +71,45 @@ spec: - message - uniqueId - ruleExpression + stateWrites: + type: array + description: >- + Facts this rule remembers across events, for + cross-event correlation. Each entry is driven by one + event type, which need not be an event type the rule + alerts on -- that is what allows a rule to remember on + exec and alert on network. + items: + type: object + properties: + eventType: + type: string + description: "Event stream that drives this write" + when: + type: string + description: "CEL boolean guard; empty means always write" + scope: + type: string + enum: ["container", "pod", "node", "identity"] + description: "Bucket the entry belongs to. identity is operator-only." + name: + type: string + description: "What kind of fact this is. A literal, never an expression." + key: + type: string + description: "CEL string expression naming the subject. Omit for a scope-wide fact." + value: + type: object + x-kubernetes-preserve-unknown-fields: true + description: "Optional author extras as CEL expressions. Keys may not begin with an underscore." + ttl: + type: string + description: "Go duration string; clamped to the agent's configured maxTtl at load" + required: + - eventType + - scope + - name + - ttl profileDependency: type: integer enum: [0, 1, 2] From 686fbdba26901c2cba400029a68e918fc7605685 Mon Sep 17 00:00:00 2001 From: Ben Date: Mon, 3 Aug 2026 12:18:37 +0300 Subject: [PATCH 14/17] docs: record the cluster verification and the helm-charts CRD prerequisite The status block claimed the feature was untested on a cluster. It is now proven end-to-end against real eBPF by Test_36. Adds the deployment prerequisite that cost this a debugging cycle: the canonical Rules CRD lives in kubescape/helm-charts, and without a stateWrites property there the API server strips the clause silently -- rules load cleanly and never fire, with no error in any log. Signed-off-by: Ben --- docs/features/cel-rule-state-store.md | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/docs/features/cel-rule-state-store.md b/docs/features/cel-rule-state-store.md index a7cf613111..7ed167965d 100644 --- a/docs/features/cel-rule-state-store.md +++ b/docs/features/cel-rule-state-store.md @@ -8,11 +8,16 @@ delete of a pod — cannot be expressed at all. Design: `shared-designs-and-docs/projects/2026-07-28-cel-rule-state-store/spec.md`. -> **Status: complete in-tree, not yet proven on a cluster.** Reads, writes, alert -> evidence, config, metrics and container-removal purge all work and are unit -> tested. Still outstanding: the end-to-end component test against real eBPF -> events. **No rule has been run against a live agent yet**, so treat the -> behaviour described here as tested-by-construction rather than field-proven. +> **Status: proven end-to-end on a cluster.** Verified against real eBPF events +> on kind by `Test_36_CelStateStoreCorrelation`: a rule that writes on `exec` and +> alerts on `network` fires, carries the remembered `value:` through to its +> message, and a negative control confirms it is not firing spuriously. +> +> **Deployment prerequisite:** the `Rules` CRD must declare `stateWrites`. The +> canonical CRD ships from **`kubescape/helm-charts`**, and until the property is +> added there the API server silently strips the clause — rules load cleanly and +> never fire, with no error anywhere. The copy in `tests/chart/crds/` is fixed; +> helm-charts is a separate, required change. ## Writing state From 0a30757af94df3e5c91f4439b0483063be92ac88 Mon Sep 17 00:00:00 2001 From: Ben Date: Mon, 3 Aug 2026 17:49:36 +0300 Subject: [PATCH 15/17] test(processtree): satisfy the creator interface after the boot-time rebase main's SUB-7845 added GetProcessBootTimeNs to ProcessTreeCreator, so the stub creator in ancestors_test.go no longer satisfied the interface. Ancestor walking does not consult start times, so the stub reports "unknown" (0) rather than inventing values. Docs-exempt: test-only rebase integration fix Signed-off-by: Ben --- pkg/processtree/ancestors_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/processtree/ancestors_test.go b/pkg/processtree/ancestors_test.go index f9f91bfe97..1454e1355e 100644 --- a/pkg/processtree/ancestors_test.go +++ b/pkg/processtree/ancestors_test.go @@ -23,6 +23,10 @@ func (s *stubCreator) Stop() {} func (s *stubCreator) GetRootTree() ([]armotypes.Process, error) { return nil, nil } +// Ancestor walking does not consult process start times, so the stub reports +// "unknown" for every pid rather than inventing values. +func (s *stubCreator) GetProcessBootTimeNs(_ uint32) uint64 { return 0 } + func (s *stubCreator) GetProcessMap() *maps.SafeMap[uint32, *armotypes.Process] { m := &maps.SafeMap[uint32, *armotypes.Process]{} for pid, p := range s.tree { From 67a2c6181ca7d0f0d110d487e326ae81a4406c89 Mon Sep 17 00:00:00 2001 From: Ben Date: Mon, 3 Aug 2026 17:51:52 +0300 Subject: [PATCH 16/17] docs: add a troubleshooting order for silent correlation failures Every way a correlation rule can fail to fire is silent -- the rule applies, loads, and never matches. This orders the causes by likelihood and cost to check, leading with CRD pruning because that is the one that cost a debugging cycle and the one nobody would guess: kubectl apply reports success and no log or metric records the loss. Signed-off-by: Ben --- docs/features/cel-rule-state-store.md | 39 +++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/docs/features/cel-rule-state-store.md b/docs/features/cel-rule-state-store.md index 7ed167965d..5a6b1c621e 100644 --- a/docs/features/cel-rule-state-store.md +++ b/docs/features/cel-rule-state-store.md @@ -245,3 +245,42 @@ why its cap is larger. Counters are labelled by rule ID only, never by state key — a key is unbounded cardinality. + +## When a correlation rule does not fire + +Every failure mode here is silent — the rule applies, loads, and simply never +matches. Work down this list in order; each step is cheap and rules out one cause. + +**1. Is the clause even reaching the agent?** The `Rules` CRD must declare +`stateWrites`, or the API server prunes it. This is the most likely cause and the +hardest to guess, because `kubectl apply` reports success: + +```bash +kubectl get rules -n kubescape -o jsonpath='{.spec.rules[0].stateWrites}' +``` + +Empty output after a successful apply means the schema is missing the property. +`--validate=false` does not help — it skips client-side validation only. + +**2. Did the clause fail validation?** node-agent logs +`RuleManager - invalid stateWrites clause` with the rule ID and the offending +write name. A rule that fails validation is degraded to non-correlating; the rest +of the CRD keeps evaluating. + +**3. Is the rule bound?** Rules are inert until a `RuntimeRuleAlertBinding` lists +them by `ruleName`. A new rule ID does nothing on its own. + +**4. Is the write leg reaching the rule loop?** Add a temporary control rule that +alerts on the *write* event type with the same predicate as your `when:` guard. If +the control is silent, the problem is upstream of the state store. + +**5. Do the two legs agree on the join key?** Have the control rules print +`string(event.pid)` in their `message`, and compare. `has_ancestor` is the right +answer when the second leg is a *child* rather than the same process. + +**6. Is the write being rejected?** `state_write_rejected_total` counts caps and +guard errors, labelled by rule ID. + +**7. Could the events be reordered?** node-agent evaluates on a concurrent worker +pool, so two events milliseconds apart can be processed out of order. This is real +but usually not the cause — rule out everything above first. From 811a48264172ea9a1dd6e8647d250894bb5e4218 Mon Sep 17 00:00:00 2001 From: Ben Date: Mon, 3 Aug 2026 19:02:36 +0300 Subject: [PATCH 17/17] =?UTF-8?q?fix(rulestate):=20address=20review=20?= =?UTF-8?q?=E2=80=94=20node-scope=20cap,=20global-cap=20replacements,=20ni?= =?UTF-8?q?l=20guard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five of CodeRabbit's six findings, with tests for the two that were real correctness bugs. Node scope now gets the larger cap. It is a single node-wide bucket shared by every rule and workload, and PurgeScope is only ever called with a container's scope ID so it is never reclaimed on container removal -- exactly the reasoning that already justified the host bucket's headroom. Bounding it by the per-container cap (256) would have starved node-scoped correlation on a busy node. The global ceiling no longer rejects a write that merely REPLACES an existing key. A replacement does not grow the store, so the per-scope cap already exempted it; the ceiling did not, which meant a rule lost the ability to refresh an established marker exactly when the store was under most pressure -- i.e. when an incident is most likely in progress. The existence peek costs an extra RLock but runs only on the already-degraded path (at the ceiling, nothing reclaimable), so the hot path is unchanged. Apply now guards a nil enriched/Event. podIdentity and processOf already tolerate a nil Event, so without this the very next line panicked instead and the package's nil handling was inconsistent. Not reachable from the rule loop, which dereferences Event earlier, but an exported entry point should not depend on that. getUniqueIdAndMessage no longer shadows err. Behaviour is deliberately unchanged: only the uniqueId error is returned and only it drops the alert, because uniqueId drives cooldown and backend dedup while a failed message costs description only. Dropping a real detection because its text did not render is the worse failure. That asymmetry is now stated in a comment rather than being an accident of shadowing. Docs-exempt: review fixes; no change to the documented rule-authoring surface Signed-off-by: Ben --- pkg/rulemanager/rule_manager.go | 19 +++++--- pkg/rulemanager/statewrites/executor.go | 7 +++ pkg/rulemanager/statewrites/executor_test.go | 2 +- pkg/rulestate/store.go | 43 +++++++++++++++--- pkg/rulestate/store_test.go | 46 ++++++++++++++++++++ 5 files changed, 103 insertions(+), 14 deletions(-) diff --git a/pkg/rulemanager/rule_manager.go b/pkg/rulemanager/rule_manager.go index ef1c6167ae..b12f1559b2 100644 --- a/pkg/rulemanager/rule_manager.go +++ b/pkg/rulemanager/rule_manager.go @@ -719,18 +719,23 @@ func (rm *RuleManager) getRuleExpressions(rule typesv1.Rule, eventType utils.Eve // which is what lets rulecooldown collapse both legs of a bidirectional rule into // a single alert instead of emitting one per leg. func (rm *RuleManager) getUniqueIdAndMessage(evalContext map[string]any, rule typesv1.Rule) (string, string, error) { - message, err := rm.celEvaluator.EvaluateStringExpressionWithContext(evalContext, rule.Expressions.Message) - if err != nil { - logger.L().Ctx(rm.ctx).Error("RuleManager - failed to evaluate message", helpers.Error(err)) + message, msgErr := rm.celEvaluator.EvaluateStringExpressionWithContext(evalContext, rule.Expressions.Message) + if msgErr != nil { + logger.L().Ctx(rm.ctx).Error("RuleManager - failed to evaluate message", helpers.Error(msgErr)) } - uniqueID, err := rm.celEvaluator.EvaluateStringExpressionWithContext(evalContext, rule.Expressions.UniqueID) - if err != nil { - logger.L().Ctx(rm.ctx).Error("RuleManager - failed to evaluate unique ID", helpers.Error(err)) + uniqueID, idErr := rm.celEvaluator.EvaluateStringExpressionWithContext(evalContext, rule.Expressions.UniqueID) + if idErr != nil { + logger.L().Ctx(rm.ctx).Error("RuleManager - failed to evaluate unique ID", helpers.Error(idErr)) } uniqueID = hashStringToMD5(uniqueID) - return message, uniqueID, err + // Only the uniqueId error is returned, and the caller drops the alert on it. + // That asymmetry is deliberate: uniqueId drives cooldown and backend dedup, so + // a wrong one corrupts grouping, whereas a failed message costs description + // only. Dropping a real detection because its text did not render would be the + // worse failure, so a message error is logged and the alert still ships. + return message, uniqueID, idErr } func isSupportedEventType(rules []typesv1.Rule, enrichedEvent *events.EnrichedEvent) bool { diff --git a/pkg/rulemanager/statewrites/executor.go b/pkg/rulemanager/statewrites/executor.go index f6762067b4..923c553a02 100644 --- a/pkg/rulemanager/statewrites/executor.go +++ b/pkg/rulemanager/statewrites/executor.go @@ -81,6 +81,13 @@ func (e *Executor) Apply( if e == nil || e.store == nil || len(compiled) == 0 || evalContext == nil { return } + // podIdentity and processOf below both tolerate a nil Event; without this the + // very next line would panic instead, so the package's nil handling would be + // inconsistent. Not reachable from the rule loop, which dereferences Event + // earlier, but the exported entry point should not depend on that. + if enriched == nil || enriched.Event == nil { + return + } eventType := enriched.Event.GetEventType() scopeIDs := ScopeIDs(enriched) diff --git a/pkg/rulemanager/statewrites/executor_test.go b/pkg/rulemanager/statewrites/executor_test.go index a572334e7c..b854f49ec6 100644 --- a/pkg/rulemanager/statewrites/executor_test.go +++ b/pkg/rulemanager/statewrites/executor_test.go @@ -299,7 +299,7 @@ func TestApply_NilSafeOnMissingPieces(t *testing.T) { func TestScopeIDs_ResolvesFromTheEventOnly(t *testing.T) { ids := ScopeIDs(execEvent("abc")) assert.Equal(t, "c:abc", ids[armotypes.StateScopeContainer]) - assert.Equal(t, "n:", ids[armotypes.StateScopeNode]) + assert.Equal(t, rulestate.NodeScopeID(), ids[armotypes.StateScopeNode]) assert.Equal(t, "p:prod/web-1", ids[armotypes.StateScopePod]) // Host: container scope resolves to the host bucket, pod scope is absent. diff --git a/pkg/rulestate/store.go b/pkg/rulestate/store.go index b3e6f888d7..77f734ffe7 100644 --- a/pkg/rulestate/store.go +++ b/pkg/rulestate/store.go @@ -50,8 +50,15 @@ func (s *Store) shardFor(scopeID string) *shard { return s.shards[h.Sum32()%shardCount] } +// scopeCap picks the cap for a bucket. The larger cap applies to both node-wide +// buckets -- the host pseudo-container and node scope itself. Neither holds one +// workload: they are shared by every rule and every process on the node, and +// neither is ever reclaimed by PurgeScope (which is only ever called with a +// container's scope ID), so both rely on TTL and need the headroom. Bounding node +// scope by the per-container cap would starve it far sooner than intended on a +// busy node. func (s *Store) scopeCap(scopeID string) int { - if IsHostScopeID(scopeID) { + if IsHostScopeID(scopeID) || scopeID == NodeScopeID() { return s.cfg.MaxEntriesForHost } return s.cfg.MaxEntriesPerContainer @@ -70,16 +77,27 @@ func (s *Store) Set(e *Entry) error { // writers can each pass this check before any of them increments, so the size // can exceed MaxSize by up to the number of in-flight writers. The per-scope // cap, which IS exact, is what bounds any single workload. + sh := s.shardFor(e.ScopeID) + k := entryKey{e.RuleID, e.Name, e.Key} + if s.currentSize() >= s.cfg.MaxSize { if s.Sweep() == 0 { - s.metrics.ReportStateWriteRejected(e.RuleID, "global_cap") - return ErrGlobalCapReached + // A replacement does not grow the store, so the ceiling must not block + // it -- the same reasoning the per-scope cap already applies below. + // Otherwise a rule loses the ability to refresh an established marker + // exactly when the store is under most pressure, which is when an + // incident is most likely to be in progress. + // + // The peek costs an extra RLock, but only on this already-degraded + // path: at the ceiling with nothing reclaimable. The hot path is + // unchanged. + if !s.holds(sh, e.ScopeID, k) { + s.metrics.ReportStateWriteRejected(e.RuleID, "global_cap") + return ErrGlobalCapReached + } } } - sh := s.shardFor(e.ScopeID) - k := entryKey{e.RuleID, e.Name, e.Key} - sh.mu.Lock() b, ok := sh.scopes[e.ScopeID] if !ok { @@ -104,6 +122,19 @@ func (s *Store) Set(e *Entry) error { return nil } +// holds reports whether a live-or-expired entry already exists under k. Used only +// by the global-cap path to tell a replacement from a genuine insert. +func (s *Store) holds(sh *shard, scopeID string, k entryKey) bool { + sh.mu.RLock() + defer sh.mu.RUnlock() + b, ok := sh.scopes[scopeID] + if !ok { + return false + } + _, exists := b.entries[k] + return exists +} + // Get returns a live entry, or false if absent or expired. Expiry is enforced // here as well as by the sweeper so a read never sees a stale marker. func (s *Store) Get(ruleID string, _ armotypes.StateScope, scopeID, name, key string) (*Entry, bool) { diff --git a/pkg/rulestate/store_test.go b/pkg/rulestate/store_test.go index a1431c553d..0682b70307 100644 --- a/pkg/rulestate/store_test.go +++ b/pkg/rulestate/store_test.go @@ -152,6 +152,52 @@ func TestStore_HostBucketHasItsOwnLargerCap(t *testing.T) { assert.ErrorIs(t, s.Set(entry("R1089", HostScopeID(), "n", "8", now, time.Minute)), ErrScopeCapReached) } +// Node scope is a single node-wide bucket shared by every rule and workload, and +// PurgeScope is only ever called with a container's scope ID, so it is never +// reclaimed on container removal. It therefore needs the same headroom as the +// host bucket -- the per-container cap would starve it on a busy node. +func TestStore_NodeBucketGetsTheLargerCap(t *testing.T) { + s := NewStore(testConfig(), NoopMetrics{}) // host/node cap 8, container cap 4 + now := time.Now() + for i := 0; i < 8; i++ { + e := entry("R1089", NodeScopeID(), "n", fmt.Sprint(i), now, time.Minute) + e.Scope = armotypes.StateScopeNode + require.NoError(t, s.Set(e), + "node scope must not be bounded by the per-container cap") + } + over := entry("R1089", NodeScopeID(), "n", "8", now, time.Minute) + over.Scope = armotypes.StateScopeNode + assert.ErrorIs(t, s.Set(over), ErrScopeCapReached) +} + +// At the global ceiling with nothing reclaimable, a write that only REPLACES an +// existing key does not grow the store, so it must still be admitted -- otherwise +// a rule loses the ability to refresh a marker exactly when the store is under +// most pressure. Mirrors TestStore_OverwriteSucceedsEvenAtCap for the global cap. +func TestStore_GlobalCapAdmitsAReplacement(t *testing.T) { + cfg := testConfig() + cfg.MaxSize = 3 + cfg.MaxEntriesPerContainer = 100 + s := NewStore(cfg, NoopMetrics{}) + now := time.Now() + for i := 0; i < 3; i++ { + require.NoError(t, s.Set(entry("R1089", "c:abc", "n", fmt.Sprint(i), now, time.Minute))) + } + + // A genuine insert is still rejected... + assert.ErrorIs(t, s.Set(entry("R1089", "c:abc", "n", "new", now, time.Minute)), ErrGlobalCapReached) + + // ...but refreshing an existing key is not. + later := now.Add(time.Second) + require.NoError(t, s.Set(entry("R1089", "c:abc", "n", "0", later, time.Minute)), + "a replacement does not grow the store, so the ceiling must not block it") + + got, ok := s.Get("R1089", armotypes.StateScopeContainer, "c:abc", "n", "0") + require.True(t, ok) + assert.Equal(t, later, got.Timestamp) + assert.Equal(t, 3, s.Len()) +} + func TestScopeIDs_HostAndNodeDoNotCollide(t *testing.T) { // Host processes carry ContainerID == "", and node scope has no ID. Without // type prefixes both would be "" and share a bucket.