new(libsinsp): sinsp_repair_state_sc_set ppm sc API

[incertum]

Signed-off-by: Melissa Kilby melissa.kilby.oss@gmail.com

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area API-version

/area build

/area CI

/area driver-kmod

/area driver-bpf

/area driver-modern-bpf

/area libscap-engine-bpf

/area libscap-engine-gvisor

/area libscap-engine-kmod

/area libscap-engine-modern-bpf

/area libscap-engine-nodriver

/area libscap-engine-noop

/area libscap-engine-source-plugin

/area libscap-engine-savefile

/area libscap-engine-udig

/area libscap

/area libpman

/area libsinsp

/area tests

/area proposals

Does this PR require a change in the driver versions?

/version driver-API-version-major

/version driver-API-version-minor

/version driver-API-version-patch

/version driver-SCHEMA-version-major

/version driver-SCHEMA-version-minor

/version driver-SCHEMA-version-patch

What this PR does / why we need it:

Edited 2023-02-16:

Thanks to refactors in #877 and #854 stars have aligned to build out the adaptive base syscalls enforcement option based on the event types defined in the filter.

Why is this option needed?

Clients can now enable more resourceful kernel tracing in that only syscalls that are needed for sinsp state build-up and life-cycle management are subscribed to in addition to the syscalls defined in the filter string. In short, this new option is sinsp state compliant - conditioned by filter events / current configuration. This option can accomplish large cost savings, especially when the client needs to feature different modes of monitoring according to a cost budget. All configurations are supported, e.g. the most minimal configuration possible is solely monitoring spawned processes nothing else.

Integration proposal:

• Introduce as "experimental"
• Create new e2e test format to verify validity in addition to the new unit tests
• If successful, feature as stable API

Example:

sudo ./libsinsp/examples/sinsp-example -b driver/bpf/probe.o -f "(evt.type in (execve, execveat, connect, accept))" -o "*%proc.exepath %proc.cmdline %proc.exe_ino" -x
-- Filter event types names: accept, connect, execve, execveat
-- Try to open: 'bpf' engine.
-- Option adaptive base syscalls set
-- (25) syscalls activated in total: accept, accept4, bind, capset, chdir, chroot, clone, clone3, close, connect, execve, execveat, fchdir, fork, getsockopt, setgid, setpgid, setresgid, setresgid32, setresuid, setresuid32, setsid, setuid, socket, vfork
driver/bpf/probe.o
-- Engine 'bpf' correctly opened.
-- Start capture

Which issue(s) this PR fixes:

Inability to customize libs clients and run agents in a more cost saving mode.

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

new(libsinsp): feature adaptive base syscalls option in ppm sc API

[FedeDP]

Thank you Melissa! This looks really promising, i think this is what we all had in mind actually :)
There are some minor things to think about:

• We need an event/syscall flag to mark a syscall as heavy
• then, Falco/consumers will need to check if the adaptive set contains any of the heavy syscalls (perhaps a new API for this that return the subset of the ppm_sc_set cotnaining all the heavy ones?)
• in case Falco is not run with -A, we will need to print a warning and remove the heavy syscalls from the set
• if Falco is run with -A, we run adaptive regardless of heavy syscalls

But that can be done in subsequent PRs; let's keep it simple :D

[FedeDP]

Why do we need this API?

[incertum]

Let's see, thought this will likely be refactored anyways after first getting the Falco approach in place. Consider the current approach to extract evttypes from the filter a hack (I don't yet have a full understanding of those parts of the code base) -> for now idea was to have a complete enough POC for testing.

[FedeDP]

Can't we start from enforce_sinsp_state_ppm_sc?
In that case, we can be sure that any syscall needed by sinsp enrichment is actually captured.

[incertum]

Primary goal of this PR is to provide an alternative to enforce_sinsp_state_ppm_sc with the --optimized-syscalls option (opt-in, default to false) 🙃 .
Thinking behind this is that current approach seems to add too many unnecessary syscalls (because of too generic UF_NEVER_DROP or EF_MODIFIES_STATE flags).

Consider the following (attempting to brain dump my current understanding):

• For example, consider a scenario where Falco rules are only around spawned_process and open* syscalls -> hence no need to tap into any network related syscalls at all or other fds.
• Many EF_MODIFIES_STATE do EF_CREATES_FD, but consider a scenario where this syscall isn't part of a Falco rule, hence don't need to add those fds -> for the most part most fd related syscalls contain the needed fd info in the actual live event. Only for example for connect, accept* and listen syscalls absolutely need to also trace socket syscall etc to get the ip tuples. However, interested in learning about concrete examples where this assessment is false -> then extend new alternative enforce_optimized_ppm_sc_set option.
• Very carefully reviewed the parser and the current sets is what came out, but I planned more detailed analyses. The intended new minimum set should truly only address adding or updating fields that are used to create logs when another syscall triggers (e.g. opened a file, but also need current uid of the process).

[The removing and destroying parts should be sufficiently addressed (it's only the proc exit sched events and close syscall)]

[FedeDP]

So, i'd say:

• either we enforce the sinsp state ppm_sc
• or we DO NOT enforce them, and expect the consumer to specify additional syscalls to be tracked, aside from the ruleset ones

If you see, all APIs in that source file but enforce_simple_ppm_sc do not enforce the sinsp state ppm sc!

[FedeDP]

Of course, Falco will eventually enforce the sinsp_state_ppm_sc though :/

[FedeDP]

I think that sooner or later we will need to expose a config option to let users choose the syscalls to be additionally tracked, like
additional_syscalls: [ "open", "close", "execve" ] and when the option is set, we should avoid enforcing the sinsp state ppm sc.

[incertum]

@FedeDP summarized all new suggestions in Falco PR falcosecurity/falco#2361.

• Re additional_syscalls option, I like it, gives more control and even more options to end users
• Re sinsp state enforcement, obviously since I opened this would say we really need this new approach (once finalized and proven to be 100% technically sound), but we can support both: (1) a more efficient sinsp state enforcement option (this one) and (2) the more conservative option (the existing API). Conservative one can be default and the new one opt-in via a flag.

[FedeDP]

Note however that UF_NEVER_DROP are likely to be dropped from the enforced set (i think we will eventually drop both the UF_NEVER_DROP and UF_ALWAYS_DROP flags sooner or later).
In the end, our enforced state ppm sc will just be the list of EF_MODIFIES_STATE events, not that big!

[incertum]

@FedeDP I like the heavy syscall addition as safety guardrails (💯 ) and agreed, could rebase this PR after a new API has been added in a separate PR.

Additional suggestions for consideration:

• Let's simulate everything in sinsp-example, hence also add -A flag or any other new flag to sinsp-example to be able to implement more e2e tests?
• Would it also be acceptable to add a flag --allow-heavy-syscalls instead? That way -A has one clear purpose (no ambiguity), else if you define heavy syscalls in Falco rules, but don't set --allow-heavy-syscalls they would be rejected similar to how you described it. Suggesting this to achieve a ZERO complaints, all configurations supported approach, because end users may complain again and say, hey I only want to add this one heavy syscall, let's say mmap, why can't I just do that and why do I have to pay again the cost of tracing ALL syscalls (and keep in mind this cost increases as we add more syscalls)

[incertum]

Let's see, thought this will likely be refactored anyways after first getting the Falco approach in place. Consider the current approach to extract evttypes from the filter a hack (I don't yet have a full understanding of those parts of the code base) -> for now idea was to have a complete enough POC for testing.

[incertum]

Primary goal of this PR is to provide an alternative to enforce_sinsp_state_ppm_sc with the --optimized-syscalls option (opt-in, default to false) 🙃 .
Thinking behind this is that current approach seems to add too many unnecessary syscalls (because of too generic UF_NEVER_DROP or EF_MODIFIES_STATE flags).

Consider the following (attempting to brain dump my current understanding):

• For example, consider a scenario where Falco rules are only around spawned_process and open* syscalls -> hence no need to tap into any network related syscalls at all or other fds.
• Many EF_MODIFIES_STATE do EF_CREATES_FD, but consider a scenario where this syscall isn't part of a Falco rule, hence don't need to add those fds -> for the most part most fd related syscalls contain the needed fd info in the actual live event. Only for example for connect, accept* and listen syscalls absolutely need to also trace socket syscall etc to get the ip tuples. However, interested in learning about concrete examples where this assessment is false -> then extend new alternative enforce_optimized_ppm_sc_set option.
• Very carefully reviewed the parser and the current sets is what came out, but I planned more detailed analyses. The intended new minimum set should truly only address adding or updating fields that are used to create logs when another syscall triggers (e.g. opened a file, but also need current uid of the process).

[The removing and destroying parts should be sufficiently addressed (it's only the proc exit sched events and close syscall)]

[FedeDP]

Would it also be acceptable to add a flag --allow-heavy-syscalls instead? That way -A has one clear purpose (no ambiguity), else if you define heavy syscalls in Falco rules, but don't set --allow-heavy-syscalls they would be rejected similar to how you described it. Suggesting this to achieve a ZERO complaints, all configurations supported approach, because end users may complain again and say, hey I only want to add this one heavy syscall, let's say mmap, why can't I just do that and why do I have to pay again the cost of tracing ALL syscalls (and keep in mind this cost increases as we add more syscalls)

@incertum note that the -A syscalls won't pull in all syscalls; instead, we will load syscalls from the ruleset, BUT will allow heavy ones. At least, that's the idea :)

Basically, we will have:

• default behavior: adaptive without heavy syscalls. A warning is printed in case any heavy syscall is part of the ruleset
• -A behavior: adaptive but allows heavy syscalls

[leogr]

Would it also be acceptable to add a flag --allow-heavy-syscalls instead? That way -A has one clear purpose (no ambiguity), else if you define heavy syscalls in Falco rules, but don't set --allow-heavy-syscalls they would be rejected similar to how you described it. Suggesting this to achieve a ZERO complaints, all configurations supported approach, because end users may complain again and say, hey I only want to add this one heavy syscall, let's say mmap, why can't I just do that and why do I have to pay again the cost of tracing ALL syscalls (and keep in mind this cost increases as we add more syscalls)

@incertum note that the -A syscalls won't pull in all syscalls; instead, we will load syscalls from the ruleset, BUT will allow heavy ones. At least, that's the idea :)

Basically, we will have:

• default behavior: adaptive without heavy syscalls. A warning is printed in case any heavy syscall is part of the ruleset
• -A behavior: adaptive but allows heavy syscalls

I agree with the solution proposed by @FedeDP, since it basically preserves the previous behaviors from a functional point of view, but at the same time, allows optimization. 👍

[FedeDP]

Minor note: instead of a new EC_HEAVY flag, we could just keep it simple and use the EC_IO event category, ie:

EC_IO_BASE = 32,/* used for masking */
EC_IO_READ = 32,/* General I/O read (can be file, socket, IPC...) */
EC_IO_WRITE = 33,/* General I/O write (can be file, socket, IPC...) */
EC_IO_OTHER = 34,/* General I/O that is neither read not write (can be file, socket, IPC...) */

Most of the "heavy" syscalls are IO related. And this would save us yet another flag.

Basically we could use the set provided by https://github.com/falcosecurity/libs/blob/master/userspace/libsinsp/sinsp_ppm_sc.cpp#L167

[incertum]

ACK, thanks @FedeDP and @leogr for clarifying -A. This sounds great, fully onboard!

@FedeDP smart re using EC_IO flag!

In summary:

• Will continue careful testing to truly only add syscalls absolutely needed, 100% conditioned by the syscalls from the rules set (that's what this PR is all about) -> really need test integrations, will 👀 into it.
• Work with Jason on Falco to extract evttypes from rules satisfactorily
• All other items -> separate PRs by Federico or Jason or more folks; we'll just wing it and find a way to partition work out a bit and make sure all falls together asap after 0.34 is out given the importance / urgency and also allow for some buffer for testing.
• Re naming and exposing stuff over sinsp APIs vs in Falco, all these details I don't have a strong opinion on and happy to keep making changes until everyone thinks it's good enough.

[incertum]

@FedeDP @leogr @Andreagit97 @jasondellaluce this has been re-designed from the ground up thanks to recent refactors.

[incertum]

Created a new Falco ticket falcosecurity/falco#2433 to track this effort and also posted on slack asking for community help to further build this feature out. Thanks.

[incertum]

Rebased and removed changes from sinsp-example as we are reworking the test binary plus the sinsp API will be slightly adjusted as well. Plan would be to add a flag to sinsp-example in a follow up PR and add some behavior similar to below.

This would also clean up the current potentially verbose event names print out ...

if (repair_sinsp_state && !filter_sc_codes.empty())
{
	/* Enable syscalls push down filter in kernel via setting ppm_sc.
	 * Includes syscalls from filter plus syscalls needed for state engine
	 */
	ppm_sc = libsinsp::events::sinsp_repair_state_sc_set(filter_sc_codes).merge(filter_sc_codes);
	auto syscalls_names = libsinsp::events::sc_set_to_names(ppm_sc);
	std::cout << "-- (" + std::to_string(syscalls_names.size()) + ") syscalls activated in total: " + concat_set_in_order(syscalls_names) << std::endl;
}

[Andreagit97]

The PR LGTM left some minor comments :)

A more general question, if I understood correctly now to libs consumer we are offering 3 possible ways:

• simple_sc_set
• repair_sc_set
• YOLO mode (crazy scenarios with just one syscall enabled)

is it true?
When opening the engine with Falco we will always call this repair method using syscalls provided by rulesets, is it true?

[Andreagit97]

Maybe this could be the right place to enforce PPM_SC_SCHED_PROCESS_EXIT?

[incertum]

Yes! Nice, since now we can do it, left a comment stating that it is ok if this enforcement is redundant in case we decide to also enforce PPM_SC_SCHED_PROCESS_EXIT elsewhere.

@jasondellaluce when we refactor sc_names_to_sc_set and/or create event_names_to_sc_set need to also include the tp now ...

[incertum]

A more general question, if I understood correctly now to libs consumer we are offering 3 possible ways:

* `simple_sc_set`

* `repair_sc_set`

* YOLO mode (crazy scenarios with just one syscall enabled)

is it true? When opening the engine with Falco we will always call this repair method using syscalls provided by rulesets, is it true?

Tried to wordsmith the description. We likely need more edits to come up with a clear final description.

• default Falco enforces sinsp_state_sc_set only, simple_set belongs to the past
• base_syscalls.repair = true is opt-in and defaults to false, when true or custom_set not empty the above default behavior sinsp_state_sc_set is overriden -> when true this new API sinsp_repair_state_sc_set is called, it can be used if you want to leave base_syscalls.custom_set empty, but still override default enforcement or if you use base_syscalls.custom_set but you want to ensure that you did not forget a critical system call as it is complete end user override, so yes you can just activate one system call and break Falco. See [New Feature]: User flag for base_syscalls.repair as extension to base_syscalls config overriding default sinsp state enforcement falco#2433 (comment)

[Andreagit97]

/approve

[poiana]

LGTM label has been added.

Git tree hash: aba2e88251a1e3e658253a631c899a3a97d18c46

[Andreagit97]

Tried to wordsmith the description. We likely need more edits to come up with a clear final description.

• default Falco enforces sinsp_state_sc_set only, simple_set belongs to the past

• base_syscalls.repair = true is opt-in and defaults to false, when true or custom_set not empty the above default behavior sinsp_state_sc_set is overriden -> when true this new API sinsp_repair_state_sc_set is called, it can be used if you want to leave base_syscalls.custom_set empty, but still override default enforcement or if you use base_syscalls.custom_set but you want to ensure that you did not forget a critical system call as it is complete end user override, so yes you can just activate one system call and break Falco. See [New Feature]: User flag for base_syscalls.repair as extension to base_syscalls config overriding default sinsp state enforcement falco#2433 (comment)

Makes sense! Thank you for the summary :)

[FedeDP]

/approve

[poiana]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, FedeDP, incertum

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [Andreagit97,FedeDP,incertum]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[FedeDP]

/milestone 0.11.0