-
Notifications
You must be signed in to change notification settings - Fork 412
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(ebpf): disable PrintSyscallTable as default #3562
fix(ebpf): disable PrintSyscallTable as default #3562
Conversation
E2E 1173 is ✅. |
4daf07b
to
6ce8a36
Compare
6ce8a36
to
7bd94f6
Compare
This comment was marked as outdated.
This comment was marked as outdated.
a2e3e66
to
2a8a096
Compare
This comment was marked as outdated.
This comment was marked as outdated.
yep, Ori has already complained about this on his tests. |
7595b29 solves it. |
@geyslan mind opening a PR solving the imports issue by itself ? THats because we can merge it before this PR is merged and solve other failing tests before anything. |
EvtsToDisable is a hardcoded map of events (key: string) to be disabled by default. It allows to turn off events that may be causing unwanted side effects or can't have their ID predicted (e.g. signature events). If the user sets such an event on the policy, it will be removed from the list of disabled events and the event will be enabled as usual.
This is a temporary fix to silence error messages from GKE kernel.
After the changes of aquasecurity#3262, at this stage, policies.Map() length is always greater than 0.
7595b29
to
e91f15a
Compare
@@ -130,6 +139,7 @@ func prepareEventsToTrace(eventFilter eventFilter, eventsNameToID map[string]eve | |||
return nil, InvalidEventExcludeError(name) | |||
} | |||
isExcluded[id] = true | |||
delete(eventsToDisable, name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the event in "eventsToDisable" is also used in another policy ? You just removed it from the map that is going to be used by the prepareEventsToTracee
when the other policy filters are parsed, no ? The event could have been excluded in this policy, but maybe not the other ? Am I missing something ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed a bit offline with Geyslan. We agreed (and he already thought like that before) that the signature and the event should be disabled if the ksymbol isnt available, but using a debug log only (so it does not scare the end user).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. From the start I didn't like this PR approach, it increases complexity just to silence an error that should be issued.
Please rebase on top of #3601 just to make sure we're good (since the event in question has changed). I'll be adding a INST test to that event as well. |
Closing this. Context: https://github.com/aquasecurity/tracee/pull/3562/files#r1373607623 |
Close: #3397
1. Explain what the PR does
e91f15a chore: change logger levels to debug
286dff2 fix(ebpf): cancel dependencies of the canceled one
19381ad chore(flags): remove leftover
ab97d72 fix: disable syscall_hooking
2e200fd fix(ebpf): set events to be disabled by default
286dff2 fix(ebpf): cancel dependencies of the canceled one
19381ad chore(flags): remove leftover
ab97d72 fix: disable syscall_hooking
2e200fd fix(ebpf): set events to be disabled by default
2. Explain how to test it
When setting
syscall_hooking
, if the symbolAAAsys_call_table
is not found we see the error.Even with the symbol not found, if
syscall_hooking
is NOT required by the user, we don't receive the error any more, just a debug log concerning the disabling.3. Other comments