Introduce Policies versioning (map of maps) #3305

geyslan · 2023-07-08T00:14:18Z

1. Explain what the PR does

df1f1f9 feat(userland): add support for policy versioning

Add policy versioning, which will support runtime policy updates.

This also:

- Adds the singleton Snapshots circular buffer (for policy versioning).
- Introduces the Cloner interface (used for all policy objects).
- Moves the filters' ebpf logic to a new ebpf file (filter types are now
  ebpf-agnostic).
- Splits filters_test.go into multiple files.
- Removes the unused containers filter (it was just a wrapper for
  StringFilter).
- Removes unused RandStringRunes function from sets_test.go.
- Makes other minor refactorings/changes.

53dd343 feat(ebpf): add support for policy versioning

Make use of map of maps to versionize policies. This is a first step
towards supporting runtime policy updates.

2. Explain how to test it

3. Other comments

geyslan · 2024-01-02T15:52:47Z

It's ready for review again.

pkg/policy/snapshots.go

pkg/policy/policies.go

pkg/cmd/cobra/cobra.go

rafaeldtinoco · 2024-01-05T01:27:44Z

Im literally missing only the eBPF filters portion (file) now, will finish tomorrow.

Things I like from the PR so far:

the singleton initialization (Once)
multiple tests added to each filter type
filter eBPF logic split from regular filter logic

Left some questions/comments.

pkg/policy/ebpf.go

rafaeldtinoco · 2024-01-05T16:15:47Z

@geyslan Im done with my review. I cannot find any flaw (have not tested it, just read and made logic judgement). I believe it touches sensitive filtering logic, so I would make sure to do lots of tests before merging (not only unit testing, but making sure each policy has a specific filter, or set of filters, including == and !=s and making sure at least 5 consecutive version changes are good, things like that).

Either way, it was a very nice work of you, as usual. Congrasts!

rafaeldtinoco

LGTM after concurrency issues are addressed, documentation is improved and some of my comments are taken into consideration. I would recommend some testing like said before. Other than that, it's a very nice work.

NDStrahilevitz

Really nice work. I hope what I've written below will be useful.

pkg/ebpf/c/common/filtering.h

pkg/ebpf/c/tracee.bpf.c

NDStrahilevitz · 2024-01-06T14:42:08Z

pkg/ebpf/c/tracee.bpf.c

@@ -981,6 +989,7 @@ int uprobe_lkm_seeker_submitter(struct pt_regs *ctx)

    // Uprobes are not triggered by syscalls, so we need to override the false value.
    p.event->context.syscall = NO_SYSCALL;
+    p.event->context.policies_version = p.config->policies_version;


A note about uprobe triggered events in general:
The way you added policies versions to them is by checking the config at time of ebpf context. But recall that triggering them is done in userland, meaning that it may be more appropriate to pass the policies version at time of userland trigger to the ebpf program. Because note that there can be a policy version change in the time between the uprobe trigger and the ebpf program starting.

I'm not saying this isn't a reasonable pragmatic choice to do as you did, but I would argue that there is a kind of inaccuracy in doing it like this. @OriGlassman @AlonZivony since uprobe events are basically all for your domain, your take would be good.

Edit: I see that you reinject the policy version later in the "trigger context store". Note how this doesn't solve the issue here, that the applied policy filters in eBPF are not guaranteed to be with the same policy version later injected.

Regarding this, I'll wait @OriGlassman @AlonZivony feedback.

Because note that there can be a policy version change in the time between the uprobe trigger and the ebpf program starting.

I had put a check on the last stage of the event submission, checking the event context version against the current one from config_entry; if different discard the event. We had discussions about that and agreed that it would be better to send the event even with wrong version. But we could reintroduce it again to set the version as 0, so userland would know that it's a rogue event.

checking the event context version against the current one from config_entry;

But even the event context version can be later than the one that should be injected from the trigger. So that kind of corruption would be missed. This isn't a huge problem for our trigger events right now since they are infrequent, but if we had events which are more frequent, it may be more noticeable.
I'd at least document the limitation, it may not be worth fixing (I think eBPF cookies in later kernels could be used to solve this for example).

pkg/policy/ebpf.go

pkg/policy/snapshots.go

common/interfaces.go

pkg/events/derive/derive.go

NDStrahilevitz · 2024-01-06T15:04:08Z

pkg/ebpf/tracee.go

@@ -815,7 +832,7 @@ func (t *Tracee) getOptionsConfig() uint32 {
 	return cOptVal
 }

-func (t *Tracee) computeConfigValues() []byte {
+func (t *Tracee) computeConfigValues(newPolicies *policy.Policies) []byte {


I think this function could (and should) be moved to the policy snapshots.

Perhaps to Policies, as it's more correlated.

Well, the problem is this would reinject the bpf logic into the Policies/Snapshots types, which is what we're trying to avoid: coupling things together.

I think that the existence of UpdateBPF in snapshots show that the snapshot object is the coupling point. That's not a bad thing, there must be some point of coupling if we want to make policies meaningful in bpf logic.
Maybe, the UpdateBPF function should be taken out of the snapshots object so that the coupling will be out of the object and into some "effect function".

I'm keeping the point on because the tracee file is huge, and we should aim to refactor these kind of functions out given the opportunity.

@NDStrahilevitz after analyse this better I agree with you. We really need to split logic and diminish the tracee file. However, the complexity involved is also huge. For example, the computeConfigValues takes care of "config_map" which has a lot from policies but it's a tracee generic config place. And splitting or moving its logic may compromise the configVal config_entry encode understanding. What I propose now is to update this EPIC with next steps, so we can tackle then meanwhile.

pkg/ebpf/tracee.go

geyslan · 2024-01-09T17:54:02Z

Moving it to v0.21.0 as discussed offline.

Make use of map of maps to versionize policies. This is a first step towards supporting runtime policy updates.

yanivagman

This is a very nice work Geyslan!
A few comments ahead and we can merge this soon

yanivagman · 2024-01-14T12:57:29Z

pkg/ebpf/c/common/filtering.h

@@ -189,6 +217,10 @@ statfunc u64 compute_scopes(program_data_t *p)
        return 0;
    }

+    //
+    // unversioned filters


If I understand correctly, we should add boolean filters versioning in the future by adding a dedicated map for each one of them. If we won't do it, there might by a mismatch between the policies version used by the versioned filters to those that are not versioned. Is that right?
If so, it is ok that we postpone this for a future PR, but we should add a comment explaining this (and an issue) to track this required change

yanivagman · 2024-01-14T16:35:20Z

pkg/ebpf/c/common/network.h

@@ -153,8 +153,10 @@ cgrpctxmap_t cgrpctxmap_eg SEC(".maps");    // saved info SKB caller <=> SKB egr
 typedef struct net_task_context {
    struct task_struct *task;
    task_context_t taskctx;
+    s32 syscall;


any reason for using s32 and not u32?

IIRC due to respective userland EventContext field:

tracee/pkg/bufferdecoder/protocol.go

Line 47 in c6d88a0

Syscall int32

yanivagman · 2024-01-14T16:43:22Z

pkg/ebpf/c/maps.h

+BPF_HASH_INNER(uid_filter, u32, eq_t, 256);                        // filter events by UID prototype, for specific UIDs either by == or !=
+BPF_HASH_OUTER(uid_filter_version, uid_filter, 64);                // map of UID filters maps
+BPF_HASH_INNER(pid_filter, u32, eq_t, 256);                        // filter events by PID prototype
+BPF_HASH_OUTER(pid_filter_version, pid_filter, 64);                // map of PID filters maps
+BPF_HASH_INNER(mnt_ns_filter, u64, eq_t, 256);                     // filter events by mount namespace id prototype
+BPF_HASH_OUTER(mnt_ns_filter_version, mnt_ns_filter, 64);          // map of mount namespace filters maps
+BPF_HASH_INNER(pid_ns_filter, u64, eq_t, 256);                     // filter events by pid namespace id prototype
+BPF_HASH_OUTER(pid_ns_filter_version, pid_ns_filter, 64);          // map of pid namespace filters maps
+BPF_HASH_INNER(uts_ns_filter, string_filter_t, eq_t, 256);         // filter events by uts namespace name prototype
+BPF_HASH_OUTER(uts_ns_filter_version, uts_ns_filter, 64);          // map of uts namespace filters maps
+BPF_HASH_INNER(comm_filter, string_filter_t, eq_t, 256);           // filter events by command name prototype
+BPF_HASH_OUTER(comm_filter_version, comm_filter, 64);              // map of command name filters maps
+BPF_HASH_INNER(cgroup_id_filter, u32, eq_t, 256);                  // filter events by cgroup id prototype
+BPF_HASH_OUTER(cgroup_id_filter_version, cgroup_id_filter, 64);    // map of cgroup id filters maps
+BPF_HASH_INNER(binary_filter, binary_t, eq_t, 256);                // filter events by binary path and mount namespace prototype
+BPF_HASH_OUTER(binary_filter_version, binary_filter, 64);          // map of binary filters maps
+BPF_HASH_INNER(process_tree_map, u32, eq_t, 10240);                // filter events by the ancestry of the traced process
+BPF_HASH_OUTER(process_tree_map_version, process_tree_map, 64);    // map of process tree maps
+BPF_HASH_INNER(events_map, u32, event_config_t, MAX_EVENT_ID);     // map to persist event configuration data
+BPF_HASH_OUTER(events_map_version, events_map, 64);                // map of events maps


Does it mean those filter maps now take 64 times more space than before? Or is it according to the amount of inner maps that are populated in a given time?

It's according the new map creation/insertion. Rafael also noticed this size. It was put as 64 just to align the max number of policies, however, we can reduce it to 4-8. I assume it would be unlikely to have to keep more than that, even if we compute new policies very quickly.

Anyway, it would keep up to 64 references to the respective filter map:

❯ sudo bpftool map show name events_map_vers 78: hash_of_maps name events_map_vers flags 0x0 key 2B value 4B max_entries 64 memlock 9280B pids tracee(51042)

Changing max_entries to 2 only reduced memlock by ~53%. The usage is minimal.

❯ sudo bpftool map show name events_map_vers 188: hash_of_maps name events_map_vers flags 0x0 key 2B value 4B max_entries 2 memlock 4320B pids tracee(53850)

yanivagman · 2024-01-14T17:09:31Z

pkg/ebpf/tracee.go

@@ -436,6 +443,16 @@ func (t *Tracee) Init(ctx gocontext.Context) error {
 		return errfmt.Errorf("error initializing event derivation map: %v", err)
 	}

+	// Initialize events parameter types map
+	t.eventsParams = make(map[events.ID][]bufferdecoder.ArgType)


I was first confused from the name eventsParams, but now I understand its about the event parameter types,
so maybe rename to eventsParamTypes?

yanivagman · 2024-01-14T17:27:19Z

pkg/ebpf/tracee.go

@@ -1691,6 +1677,7 @@ func (t *Tracee) triggerMemDump(event trace.Event) []error {

 	errs := []error{}

+	// TODO: consider to iterate over given policies when policies are changed


So do I understand correctly that this will break the Subscribe API if merged as is?

Add policy versioning, which will support runtime policy updates. This also: - Adds the singleton Snapshots circular buffer (for policy versioning). - Introduces the Cloner interface (used for all policy objects). - Moves the filters' ebpf logic to a new ebpf file (filter types are now ebpf-agnostic). - Splits filters_test.go into multiple files. - Removes the unused containers filter (it was just a wrapper for StringFilter). - Removes unused RandStringRunes function from sets_test.go. - Makes other minor refactorings/changes.

yanivagman

LGTM!

geyslan added the milestone/v0.17.0 label Jul 8, 2023

geyslan self-assigned this Jul 8, 2023

github-actions bot added area/ebpf area/testing area/UX area/events area/filtering area/build area/flags labels Jul 8, 2023

geyslan force-pushed the 3239-pol-version branch from 34f4c5e to 7c569b1 Compare July 10, 2023 19:46

github-actions bot removed the area/flags label Jul 10, 2023

geyslan force-pushed the 3239-pol-version branch 6 times, most recently from e3f65d3 to 062bcbe Compare July 17, 2023 19:31

geyslan force-pushed the 3239-pol-version branch 6 times, most recently from 469a4c7 to 03d7ef9 Compare July 18, 2023 22:43

geyslan added milestone/v0.18.0 and removed milestone/v0.17.0 labels Jul 19, 2023

geyslan force-pushed the 3239-pol-version branch 2 times, most recently from 1c717b0 to d04dde7 Compare August 1, 2023 15:49

geyslan force-pushed the 3239-pol-version branch 3 times, most recently from 6e27a92 to c19e788 Compare August 16, 2023 20:44

geyslan force-pushed the 3239-pol-version branch 2 times, most recently from cc4f828 to 45ebe93 Compare January 2, 2024 14:45

NDStrahilevitz self-requested a review January 4, 2024 15:24

geyslan force-pushed the 3239-pol-version branch from 45ebe93 to 5b58ac8 Compare January 4, 2024 17:11

rafaeldtinoco reviewed Jan 4, 2024

View reviewed changes

pkg/policy/snapshots.go Show resolved Hide resolved

rafaeldtinoco reviewed Jan 4, 2024

View reviewed changes

pkg/policy/policies.go Show resolved Hide resolved

rafaeldtinoco reviewed Jan 5, 2024

View reviewed changes

pkg/cmd/cobra/cobra.go Show resolved Hide resolved

geyslan force-pushed the 3239-pol-version branch from 5b58ac8 to debccc4 Compare January 5, 2024 13:26

rafaeldtinoco reviewed Jan 5, 2024

View reviewed changes

pkg/policy/ebpf.go Outdated Show resolved Hide resolved

rafaeldtinoco reviewed Jan 5, 2024

View reviewed changes

pkg/policy/ebpf.go Show resolved Hide resolved

rafaeldtinoco reviewed Jan 5, 2024

View reviewed changes

pkg/policy/ebpf.go Show resolved Hide resolved

rafaeldtinoco approved these changes Jan 5, 2024

View reviewed changes

NDStrahilevitz reviewed Jan 6, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

geyslan added milestone/v0.21.0 and removed milestone/v0.20.0 labels Jan 9, 2024

feat(ebpf): add support for policy versioning

53dd343

Make use of map of maps to versionize policies. This is a first step towards supporting runtime policy updates.

geyslan force-pushed the 3239-pol-version branch from a9d6a3b to d80c9a4 Compare January 10, 2024 19:11

yanivagman reviewed Jan 15, 2024

View reviewed changes

geyslan force-pushed the 3239-pol-version branch from d80c9a4 to df1f1f9 Compare January 15, 2024 13:01

yanivagman approved these changes Jan 15, 2024

View reviewed changes

geyslan merged commit 71c58ac into aquasecurity:main Jan 15, 2024
30 checks passed

geyslan deleted the 3239-pol-version branch January 15, 2024 16:40

geyslan added milestone/v0.20.0 and removed milestone/v0.21.0 labels Jan 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Policies versioning (map of maps) #3305

Introduce Policies versioning (map of maps) #3305

geyslan commented Jul 8, 2023 •

edited

Loading

geyslan commented Jan 2, 2024

rafaeldtinoco commented Jan 5, 2024

rafaeldtinoco commented Jan 5, 2024

rafaeldtinoco left a comment

NDStrahilevitz left a comment

NDStrahilevitz Jan 6, 2024 •

edited

Loading

geyslan Jan 9, 2024

geyslan Jan 9, 2024

NDStrahilevitz Jan 9, 2024 •

edited

Loading

NDStrahilevitz Jan 6, 2024

geyslan Jan 8, 2024

geyslan Jan 8, 2024

NDStrahilevitz Jan 8, 2024 •

edited

Loading

geyslan Jan 9, 2024 •

edited

Loading

This comment was marked as outdated.

geyslan commented Jan 9, 2024 •

edited

Loading

yanivagman left a comment

yanivagman Jan 14, 2024 •

edited

Loading

yanivagman Jan 14, 2024

geyslan Jan 15, 2024

yanivagman Jan 14, 2024

geyslan Jan 15, 2024

geyslan Jan 15, 2024

yanivagman Jan 14, 2024

yanivagman Jan 14, 2024

yanivagman left a comment •

edited

Loading

		@@ -1691,6 +1677,7 @@ func (t *Tracee) triggerMemDump(event trace.Event) []error {

		errs := []error{}

		// TODO: consider to iterate over given policies when policies are changed

Introduce Policies versioning (map of maps) #3305

Introduce Policies versioning (map of maps) #3305

Conversation

geyslan commented Jul 8, 2023 • edited Loading

1. Explain what the PR does

2. Explain how to test it

3. Other comments

geyslan commented Jan 2, 2024

rafaeldtinoco commented Jan 5, 2024

rafaeldtinoco commented Jan 5, 2024

rafaeldtinoco left a comment

Choose a reason for hiding this comment

NDStrahilevitz left a comment

Choose a reason for hiding this comment

NDStrahilevitz Jan 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NDStrahilevitz Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NDStrahilevitz Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

geyslan Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

This comment was marked as outdated.

geyslan commented Jan 9, 2024 • edited Loading

yanivagman left a comment

Choose a reason for hiding this comment

yanivagman Jan 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanivagman left a comment • edited Loading

Choose a reason for hiding this comment

geyslan commented Jul 8, 2023 •

edited

Loading

NDStrahilevitz Jan 6, 2024 •

edited

Loading

NDStrahilevitz Jan 9, 2024 •

edited

Loading

NDStrahilevitz Jan 8, 2024 •

edited

Loading

geyslan Jan 9, 2024 •

edited

Loading

geyslan commented Jan 9, 2024 •

edited

Loading

yanivagman Jan 14, 2024 •

edited

Loading

yanivagman left a comment •

edited

Loading