Skip to content
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 - v1 #8318

Closed
wants to merge 3 commits into from

Conversation

jufajardini
Copy link
Contributor

Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/5464

Describe changes:

  • add a new eve field to drop and alert event roots, with possible values of "accept", "drop" or "reject" available in IPS mode, and in case of a final verdict of "reject", also present in IDS mode
  • update alert eve output format documentation to mention new verdict field
  • add section about the drop event type for eve output

The eve logs have a field alert.action that will say something like
'allowed' even if a packet gets blocked by some other rule. To make
this less ambiguous, added a field to the alert and drop events
indicating the final verdict by the engine for a given packet.

Bug OISF#5464
The `fiel action` portion seemed to be comprised of a more generic
section that followed it. Also formatted the section for lines to be
within the character limit.
Comment on lines +161 to +163
const char *verdict = PacketActionVerdictToString(p);
jb_set_string(js, "verdict", verdict);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This felt a bit redundant when I was documenting it...

@codecov
Copy link

codecov bot commented Dec 23, 2022

Codecov Report

Merging #8318 (4cc59a8) into master (4c7ca2c) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8318      +/-   ##
==========================================
- Coverage   81.89%   81.88%   -0.01%     
==========================================
  Files         963      963              
  Lines      277295   277307      +12     
==========================================
- Hits       227082   227074       -8     
- Misses      50213    50233      +20     
Flag Coverage Δ
fuzzcorpus 64.05% <64.28%> (+<0.01%) ⬆️
suricata-verify 59.65% <100.00%> (-0.02%) ⬇️
unittests 63.50% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@suricata-qa
Copy link

Information:

field baseline test %
TREX_GENERIC_stats_chk
.capture.kernel_drops 0 7511 0.00

Pipeline 11285

@@ -168,6 +168,9 @@ outputs:
# Enable the logging of tagged packets for rules using the
# "tag" keyword.
tagged-packets: yes
# Enable logging the final verdict on a packet (e.g. "blocked",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these don't match the actual logged values

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shame on me >____<'

@@ -168,6 +168,9 @@ outputs:
# Enable the logging of tagged packets for rules using the
# "tag" keyword.
tagged-packets: yes
# Enable logging the final verdict on a packet (e.g. "blocked",
# "allowed", "rejected")
verdict: yes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wonder if we should make the action field optional instead. That is the confusing one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then verdict becomes mandatory, and action optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making action optional becomes a bit annoying for the drop events, with our current way of handling optional fields in the alerts (the context flags). Should we keep this field mandatory, there?

@jufajardini
Copy link
Contributor Author

More work to do on this. On it...

@jufajardini
Copy link
Contributor Author

Replaced by: #8347

@jufajardini jufajardini closed this Jan 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants