feat: add event enable/disable #3466

josedonizetti · 2023-09-12T21:38:53Z

The event manager manages the state (disabled/enabled) of events globably on tracee.

1. Explain what the PR does

This PR is part of the work to support a v1 for GRPC, it adds an enable/disable method to the policyManager, with a state struct to hold the information about the event that is enable/disbale globally as well as per policy, this struct in the future can hold the event emit/submit mask and also a policyMask to enable/disable policies globally.

yanivagman

Not sure we need an event manager - can you please explain what other responsibilities will it have in the future other than enabling/disabling an event?
If we choose to keep the event manager, I think it should include all the state about the events:

The events that were selected by the user
The event configuration (submit/emit)
The policies that have this event enabled (so this means moving the policy bitmap of an event from policy manager to the event manager).

One thing that I didn't see here is calling the existing API to unload/load a signature event - do you plan to add it as well?

josedonizetti · 2023-09-13T11:34:10Z

@yanivagman I think in the future the event manager should be the one responsible for eventsState, which hold the events the user selected and also the the emit, submit mask.

But I don't think it should have the policyBitmap, because disabling an event globably is different than disabling a rule in a policy, if I have a mask for the event.SecurityBPF of 0b0011 on the policyManager which means we have this event as a rule into policy 0 and 1, if I disable the event, I should stop emitting/submitting it, but later when I enable it back event should still have the mask of the policies 0b0011, right? We shouldn't change the policyBitmask for an event disable/enable. That is why I didn't want to do a enable/disable event into the policymanager.

Also, not refactoring the eventsState now is a way to simplify this change to deliver the API, because eventsState is used everywhere, so encapsulating it first, and refactoring later should be the approach here.

Unloading/loading a signature should be done later, I want to first finish the API to have it ready in the case we want to release an RC, the unloading/loading can't be done without changing any public API.

WDTY?

yanivagman · 2023-09-13T11:59:23Z

But I don't think it should have the policyBitmap, because disabling an event globably is different than disabling a rule in a policy, if I have a mask for the event.SecurityBPF of 0b0011 on the policyManager which means we have this event as a rule into policy 0 and 1, if I disable the event, I should stop emitting/submitting it, but later when I enable it back event should still have the mask of the policies 0b0011, right? We shouldn't change the policyBitmask for an event disable/enable. That is why I didn't want to do a enable/disable event into the policymanager.

The fact that we can keep the policy bitmap in the event state doesn't say we should modify it if the event was disable/enabled. Per each event, its state can be composed of:

Which policies selected it - emit, submit, etc
Is the event enabled/disabled globally

Also, not refactoring the eventsState now is a way to simplify this change to deliver the API, because eventsState is used everywhere, so encapsulating it first, and refactoring later should be the approach here.

Ok

Unloading/loading a signature should be done later, I want to first finish the API to have it ready in the case we want to release an RC, the unloading/loading can't be done without changing any public API.

I guess you meant CAN be done, right? If so then yes, I agree

geyslan · 2023-09-13T13:27:54Z

pkg/ebpf/events_pipeline.go

@@ -561,6 +561,13 @@ func (t *Tracee) sinkEvents(ctx context.Context, in <-chan *trace.Event) <-chan
 				continue // might happen during initialization (ctrl+c seg faults)
 			}

+			// Is the event disabled?
+			if !t.eventManager.IsEventEnabled(events.ID(event.EventID)) {
+				logger.Debugw("event dropped because it is disabled", "event", event.EventName)


An idea for the future: instead of logging the drop (which is noisy, even in a debugging, in a huge submission of a disabled event), we could count them like we do with lost events.

Nice! Will add a TODO/issue about it, should be part of the metrics.

josedonizetti · 2023-09-13T13:53:47Z

But I don't think it should have the policyBitmap, because disabling an event globably is different than disabling a rule in a policy, if I have a mask for the event.SecurityBPF of 0b0011 on the policyManager which means we have this event as a rule into policy 0 and 1, if I disable the event, I should stop emitting/submitting it, but later when I enable it back event should still have the mask of the policies 0b0011, right? We shouldn't change the policyBitmask for an event disable/enable. That is why I didn't want to do a enable/disable event into the policymanager.

The fact that we can keep the policy bitmap in the event state doesn't say we should modify it if the event was disable/enabled. Per each event, its state can be composed of:

Which policies selected it - emit, submit, etc

Is the event enabled/disabled globally

Ah! Ok, so let's merge both policyManager and eventManager? I think I prefer the name policyManager, and it has both states inside a structure like eventState{policy.Bitmap, bool (for enabled/disabled which later becomes emit/subimt}, and we have one check only check if an event is enabled instead of checking first if the event is enabled, and then if the rule is enabled (later if the policy is enabled). WDYT? or you think two maps, one for each is best?

Unloading/loading a signature should be done later, I want to first finish the API to have it ready in the case we want to release an RC, the unloading/loading can't be done without changing any public API.

I guess you meant CAN be done, right? If so then yes, I agree

Yes, exactly, we do it after having the API working as an internal change.

geyslan · 2023-09-14T20:12:16Z

Ah! Ok, so let's merge both policyManager and eventManager? I think I prefer the name policyManager, and it has both states inside a structure like eventState{policy.Bitmap, bool (for enabled/disabled which later becomes emit/subimt}, ...

I also do prefer naming it policyManager. 👍🏼

After that type merge I'll start the relocation of the scattered Policies and EventState logic into the policyManager, as pointed in this EPIC issue: #3239

geyslan

LGTM. There's a question ahead.

pkg/ebpf/policy_manager.go

yanivagman · 2023-09-19T10:42:56Z

pkg/ebpf/policy_manager.go

 	}
+
+	return pm.isRuleEnabled(matchedPolicies, ruleId)


Should it be ruleId or eventId then?
Should it be isRuleEnabled or isEventEnabled?

I'm using ruleId where in the future it is a rule, although now it is type events.ID and I'm using eventId where it is an event, rule depends on passing policy information, event doesn't

And we have both concepts, Rules can be enabled/disabled, and Events can be enabled/disable, so we are exposing synchronized methods to deal with it (specially good to test both concepts), but in the pipeline we want to do a single test, get the mutex only one time, so we use IsEnabled which in the future should also cover the case of Policies enabled/disabled

yanivagman · 2023-09-19T10:47:50Z

pkg/ebpf/policy_manager.go

+
+// not synchronized, use IsEventEnabled instead
+func (pm *policyManager) isEventEnabled(evenId events.ID) bool {
+	state, ok := pm.rules[evenId]


we use pm.rules once with eventId (typo: eventId) and above with ruleId - which one is correct?
Reminder that in the future rule id will be composed of event id and some index

I'm using ruleId where in the future it is a rule, although now it is type events.ID and I'm using eventId where it is an event, rule depends on passing policy information, event doesn't

pkg/ebpf/policy_manager.go

The event manager manages the state (disabled/enabled) of events globably on tracee.

yanivagman · 2023-09-19T12:01:33Z

pkg/ebpf/policy_manager.go

+	pm.mutex.Lock()
+	defer pm.mutex.Unlock()
+
+	state, ok := pm.rules[eventId]


So how will this code look like in the future when we will have rule ids different than event id?
We will not be able to use pm.rules map anymore, right?

Probably, but it is an internal structure we should be able to refactor it without affecting anything that is consuming it. Right? It is encapsulated.

yanivagman

LGTM

github-actions bot assigned josedonizetti Sep 12, 2023

github-actions bot added area/ebpf area/testing labels Sep 12, 2023

josedonizetti requested review from yanivagman and geyslan September 12, 2023 22:10

josedonizetti marked this pull request as ready for review September 12, 2023 22:11

yanivagman reviewed Sep 13, 2023

View reviewed changes

geyslan reviewed Sep 13, 2023

View reviewed changes

josedonizetti added the milestone/v0.18.0 label Sep 13, 2023

josedonizetti force-pushed the add-event-manager branch 3 times, most recently from a74f957 to 4da8a82 Compare September 19, 2023 00:59

josedonizetti changed the title ~~feat: add event manager~~ feat: add event enable/disable Sep 19, 2023

josedonizetti requested review from geyslan and yanivagman September 19, 2023 01:36

geyslan approved these changes Sep 19, 2023

View reviewed changes

pkg/ebpf/policy_manager.go Show resolved Hide resolved

yanivagman reviewed Sep 19, 2023

View reviewed changes

feat: add event manager

d75aad6

The event manager manages the state (disabled/enabled) of events globably on tracee.

josedonizetti force-pushed the add-event-manager branch from 4da8a82 to d75aad6 Compare September 19, 2023 12:00

yanivagman reviewed Sep 19, 2023

View reviewed changes

yanivagman approved these changes Sep 19, 2023

View reviewed changes

josedonizetti merged commit d6962cd into aquasecurity:main Sep 19, 2023
25 checks passed

josedonizetti deleted the add-event-manager branch September 19, 2023 17:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add event enable/disable #3466

feat: add event enable/disable #3466

josedonizetti commented Sep 12, 2023 •

edited

Loading

yanivagman left a comment •

edited

Loading

josedonizetti commented Sep 13, 2023 •

edited

Loading

yanivagman commented Sep 13, 2023

geyslan Sep 13, 2023

josedonizetti Sep 13, 2023 •

edited

Loading

josedonizetti commented Sep 13, 2023

geyslan commented Sep 14, 2023

geyslan left a comment

yanivagman Sep 19, 2023

josedonizetti Sep 19, 2023 •

edited

Loading

yanivagman Sep 19, 2023

josedonizetti Sep 19, 2023 •

edited

Loading

yanivagman Sep 19, 2023

josedonizetti Sep 19, 2023

yanivagman left a comment

feat: add event enable/disable #3466

feat: add event enable/disable #3466

Conversation

josedonizetti commented Sep 12, 2023 • edited Loading

1. Explain what the PR does

yanivagman left a comment • edited Loading

Choose a reason for hiding this comment

josedonizetti commented Sep 13, 2023 • edited Loading

yanivagman commented Sep 13, 2023

geyslan Sep 13, 2023

Choose a reason for hiding this comment

josedonizetti Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

josedonizetti commented Sep 13, 2023

geyslan commented Sep 14, 2023

geyslan left a comment

Choose a reason for hiding this comment

yanivagman Sep 19, 2023

Choose a reason for hiding this comment

josedonizetti Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

yanivagman Sep 19, 2023

Choose a reason for hiding this comment

josedonizetti Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

yanivagman Sep 19, 2023

Choose a reason for hiding this comment

josedonizetti Sep 19, 2023

Choose a reason for hiding this comment

yanivagman left a comment

Choose a reason for hiding this comment

josedonizetti commented Sep 12, 2023 •

edited

Loading

yanivagman left a comment •

edited

Loading

josedonizetti commented Sep 13, 2023 •

edited

Loading

josedonizetti Sep 13, 2023 •

edited

Loading

josedonizetti Sep 19, 2023 •

edited

Loading

josedonizetti Sep 19, 2023 •

edited

Loading