-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
output/eve: add 'verdict' field to 'alert' and 'drop' events - v10 #9220
Conversation
Information: QA ran without warnings. Pipeline 15150 |
alerts: yes # log alerts that caused drops | ||
flows: all # start or all: 'start' logs only a single drop | ||
# per flow direction. All logs each dropped pkt. | ||
|
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.
verdict option is missing here
JB_SET_STRING(jb, "reject_target", "both"); | ||
} | ||
jb_open_array(jb, "reject"); | ||
switch (p->proto) { |
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.
need UDP in here too
JB_SET_STRING(jb, "action", "alert"); | ||
} | ||
if (PacketCheckAction(p, ACTION_REJECT)) { | ||
JB_SET_STRING(jb, "reject_target", "source"); |
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.
wonder if it would be clearer to use to_client
/ to_server
here
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.
Isn't it better to keep closer to what the reject keywords have?
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 think being more explicit about the direction is best, using language we already have. Since we're already thinking about to_server & to_client for most output, I think it makes sense to use it here too
* \param p Pointer to Packet current being logged | ||
* | ||
*/ | ||
void GetVerdictJson(JsonBuilder *jb, const Packet *p) |
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.
since this is a public function, lets name this something like EveAddVerdict
# Enable logging the final action taken on a packet by the engine | ||
# (e.g: the alert may have action 'allowed' but the verdict be | ||
# 'drop' due to another alert. That's the engine's verdict) | ||
verdict: yes |
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 sure about enabling by default here, esp since it will only be slightly useful for the common IDS use case
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.
This was a mistake on my end, will fix.
Is this still current? Not seeing an example of it in the SV tests. # packet with alert and pass rules triggered:
{
"event_type": "alert",
"verdict": {
"action": "pass"
}
} |
Tests {
"timestamp": "2019-05-15T08:11:18.738996+0000",
"flow_id": 1766592176856578,
"pcap_cnt": 1,
"event_type": "alert",
"alert": {
"action": "allowed",
"gid": 1,
"signature_id": 3,
"rev": 0,
"signature": "",
"category": "",
"severity": 3
},
"verdict": {
"action": "pass"
}
} |
Feedback incorporated in #9230 |
Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/5464
Previous PR: #9162
Describe changes from previous PR:
verdict.action: pass
casesSV_BRANCH=pr/1310
OISF/suricata-verify#1310
Some output samples: