-
Notifications
You must be signed in to change notification settings - Fork 392
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
refactor(engine): feed engine with signatures events #3681
refactor(engine): feed engine with signatures events #3681
Conversation
I am debating if we should maybe change the |
cfa2e5d
to
cec0b6c
Compare
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.
I like the idea of deriving the signatures (I believe the proctree example had a good "usecase" or, at least, an idea that could be used in that regards).
I think the PR needs improvements:
-
You need to better document existing and new logic and functions (please watch out for formatting/syntax, capitalize sentences, keep good line height, etc).
-
I'm afraid of the loop reintroduction. That could lead us to infinite loops if the mask isn't cleared by the signature engine in some occasion. Could you at least document (in a comment ?) what condition makes it safe ? Maybe we should have a counter for the event (can't rely in the event pointer in a LRU map because every time the event comes it has a new address since it was io.copied()).
-
I think we need to fix the "Args" part. When reintroducing the event, the
FindingToEvent->NewEvent->getArguments
logic nests theArgs->TriggeredBy
over and over and over. This is actually a good question, @NDStrahilevitz, you asked me about multiple derivations from a single event. Lets assume we will allow that to happen (for both events and signature events), how should "Args" and "triggeredBy" look like in that case ?
{
"DifferencesInArgs": {
"Event1": {
"Args": [
{
"Name": "triggeredBy",
"Value": {
"args": [
{
"Name": "file_path",
"Type": "const char*",
"Value": "/home/rafaeldtinoco/work/projects/tracee/file_modification.txt"
},
{
"Name": "dev",
"Type": "dev_t",
"Value": 271581188
},
{
"Name": "inode",
"Type": "unsigned long",
"Value": 17214308
},
{
"Name": "old_ctime",
"Type": "unsigned long",
"Value": 1699921967398957690
},
{
"Name": "new_ctime",
"Type": "unsigned long",
"Value": 1699921967402291023
}
],
"id": 774,
"name": "file_modification",
"returnValue": 0
}
}
]
},
"Event2": {
"Args": [
{
"Name": "triggeredBy",
"Value": {
"args": [
{
"Name": "triggeredBy",
"Value": {
"args": [
{
"Name": "file_path",
"Type": "const char*",
"Value": "/home/rafaeldtinoco/work/projects/tracee/file_modification.txt"
},
{
"Name": "dev",
"Type": "dev_t",
"Value": 271581188
},
{
"Name": "inode",
"Type": "unsigned long",
"Value": 17214308
},
{
"Name": "old_ctime",
"Type": "unsigned long",
"Value": 1699921967398957690
},
{
"Name": "new_ctime",
"Type": "unsigned long",
"Value": 1699921967402291023
}
],
"id": 774,
"name": "file_modification",
"returnValue": 0
}
}
],
"id": 6001,
"name": "FILE_MODIFICATION",
"returnValue": 0
}
}
]
}
}
}
Sure, though I feel it is already well documented in the code. You want in the documentation man pages?
I don't really see a reason to be concerned here.
I don't see a problem with nested |
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.
Not -1ing because @rafaeldtinoco raised the concerns relevant for it already.
I'll answer to the points:
- Documentation - 100% required for this kind of change. Inside the code it didn't seem bad at all to me, but I think that the actual docs should be updated on this.
- I tend to agree with @AlonZivony on the risk of an infinite loop, but just to be sure we should test this by loading two signatures, one regular one derived, add a print in the engine post selector stage, and trigger the initial signature. Then we can see if an infinite loop is generated anyway.
- While I think one nested triggeredBy is ok, i'm concerned with any more than that. I'm not sure it's worthwhile for us to limit it though, so it should be left to the user's concern when writing signatures, and documented as a potential "risk" factor of the feature.
@rafaeldtinoco @NDStrahilevitz |
Feed the signatures engine with its own output instead of feeding it to the output channel. This allows signatures to use other signatures as their selector.
cec0b6c
to
47a5468
Compare
We have: https://aquasecurity.github.io/tracee/dev/docs/events/custom/overview/ but they are very small. The fact that golang signatures aren't good end-user experience is definitely what made us not to extend them (as we had the rego x go-cel x golang signatures discussions).
Sure, and this was only "a thing" after the "everything as event" feature was done (very recently) anyway.
Nah, its alright from myside. |
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.
LGTM. Nadav, Alon and I discussed this offline and agreed that the recursive triggeredBy is ok for now, all other items were discussed and already taken care.
Feed the signatures engine with its own output instead of feeding it to the output channel.
This allows signatures to use other signatures as their selector.
1. Explain what the PR does
cfa2e5d refactor(engine): feed engine with signatures events
fix #3680
2. Explain how to test it
3. Other comments