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 - v9 #9216

Closed
wants to merge 6 commits into from

Conversation

jufajardini
Copy link
Contributor

@jufajardini jufajardini commented Jul 12, 2023

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

Previous PR: #9162

Describe changes from previous PR:

  • remove changes for adding verdict as new eve type
  • change packet->action to PASS when a pass rule matches for a packet (this wasn't done anywhere, so verdict.action: pass never showed up
  • take into account proto to decide on verdict.reject
  • make verdict.reject an array instead of a string (json)
  • incorporate other feedbacks from previous PR
SV_BRANCH=pr/1308

OISF/suricata-verify#1308

Some output samples:

# IPS mode, reject rule, TCP-reset
  "event_type": "alert",
  "src_ip": "10.16.1.11",
  "src_port": 54186,
  "dest_ip": "82.165.177.154",
  "dest_port": 80,
  "proto": "TCP",
  "pkt_src": "wire/pcap",
  "alert": {
    "action": "blocked",
    "gid": 1,
    "signature_id": 2,
    "rev": 1,
    "signature": "",
    "category": "",
    "severity": 3
  },
  "verdict": {
    "action": "drop",
    "reject_target": "destination",
    "reject": [
      "tcp-reset"
    ]
  }
# packet with alert and pass rules triggered:
{
 "event_type": "alert",
  "verdict": {
    "action": "pass"
  }
}

The `field 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.
If packet->action is never set to 'pass', we won't know if a packet had
a 'pass' verdict.

Related to
Bug OISF#5464
@codecov
Copy link

codecov bot commented Jul 12, 2023

Codecov Report

Merging #9216 (40d30ed) into master (690b65a) will increase coverage by 0.03%.
The diff coverage is 97.72%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9216      +/-   ##
==========================================
+ Coverage   82.36%   82.39%   +0.03%     
==========================================
  Files         968      968              
  Lines      273761   273803      +42     
==========================================
+ Hits       225470   225612     +142     
+ Misses      48291    48191     -100     
Flag Coverage Δ
fuzzcorpus 64.66% <9.09%> (+0.01%) ⬆️
suricata-verify 60.83% <97.72%> (+0.07%) ⬆️
unittests 62.88% <0.00%> (-0.01%) ⬇️

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

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 15124

@@ -416,6 +416,7 @@ void PacketAlertFinalize(DetectEngineCtx *de_ctx, DetectEngineThreadCtx *det_ctx

/* pass "alert" found, we're done */
if (pa->action & ACTION_PASS) {
p->action |= ACTION_PASS;
Copy link
Member

Choose a reason for hiding this comment

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

hmm we had done quite a nice cleanup before, removing the flag from the Packet::action. Since it is only for logging, I wonder if we can keep it out of this field to make clear it's not used otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm we had done quite a nice cleanup before, removing the flag from the Packet::action. Since it is only for logging, I wonder if we can keep it out of this field to make clear it's not used otherwise.

Would it make sense to have a field for the verdict itself? I can try to access the last packet alert in the queue, which should be the one with the pass rule... 🤔

@victorjulien
Copy link
Member

That last "empty" alert to just log verdict feels wrong to me. Maybe this is where we'd use the verdict event type.

@jufajardini
Copy link
Contributor Author

jufajardini commented Jul 12, 2023

Will update the SV tests that are currently failing with windows, I think I inadvertently removed a

requires:
  features:
    - LIBNET1.1

That must be there when we use reject

@jufajardini
Copy link
Contributor Author

Trying a different approach for the empty alert: #9220

@jufajardini jufajardini deleted the alert-action/v9.2 branch July 14, 2023 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants