tests: verify pcap_filename on alert and flow events - v2#2932
tests: verify pcap_filename on alert and flow events - v2#2932oferda4 wants to merge 1 commit intoOISF:masterfrom
Conversation
| match: | ||
| event_type: alert | ||
| alert.signature_id: 2200005 | ||
| pcap_filename.__endswith: "ip_secopt.pcap" |
There was a problem hiding this comment.
What else could we get since there is only one pcap in this test ?
There was a problem hiding this comment.
Not sure I understand what you mean. We want to test the field shows the correct value. If you mean as why using _endswith it's because the field contains the absolute path.
There was a problem hiding this comment.
Test pcap-filename-recursive has 2 packs, so I see we can be wrong for pcap_filename
But how could we be wrong when there is only one pcap like in this test ?
There was a problem hiding this comment.
This test is just a sanity the filename is properly added. Do you think the test is redundant?
There was a problem hiding this comment.
Maybe indeed. What does it cover that pcap-filename-recursive does not cover ?
There was a problem hiding this comment.
So regarding code paths it doesn't add any more coverage. It's just giving a simpler test for the basic functionality (that works even from older versions) to be easier to debug rather the complex recursive test.
But I don't mind to remove the test entirely if we are trying to be more strict on tests usage.
There was a problem hiding this comment.
Ok, I would say let's keep it, thanks
oferda4
force-pushed
the
test/eve-pcap-filename-alert-and-flow
branch
from
175ef09 to
3fe2659
Compare
oferda4
force-pushed
the
test/eve-pcap-filename-alert-and-flow
branch
from
3fe2659 to
f9b8f46
Compare
Add three suricata-verify tests that together cover all code paths in the pcap_filename output for pcap-file mode: pcap-filename-alert-and-flow: single pcap, verifies pcap_filename is present in alert events (p != NULL path in OutputJsonBuilderBuffer) and flow events (p == NULL, falls back to FlowGetPcapFileVars). pcap-filename-pseudo-pkts: verifies pcap_filename is present in fileinfo events generated by stream pseudo-packets (PKT_SRC_STREAM_TCP_DETECTLOG_FLUSH), which carry pcap_v.pfv propagated from the flow. pcap-filename-recursive: two pcap files fed with --pcap-file-recursive. alert.pcap is processed first; http.pcap is processed last. Verifies that the flow event for the alert.pcap connection still reports alert.pcap (via the per-flow pfv), not the stale global that has advanced to http.pcap. This is the primary race-condition regression test for #5255. Ticket: #5255
oferda4
force-pushed
the
test/eve-pcap-filename-alert-and-flow
branch
from
f9b8f46 to
69bdcb6
Compare
| count: 1 | ||
| match: | ||
| event_type: flow | ||
| pcap_filename.__endswith: "http.pcap" |
There was a problem hiding this comment.
Do we want the stats event as well ?
There was a problem hiding this comment.
Good idea - I've added stats coverage to the single-pcap test (pcap-filename-alert-and-flow). Stats events hit the p == NULL, f == NULL global fallback path in OutputJsonBuilderBuffer, which was the only path without test coverage. With one pcap the global is unambiguous so it's a clean verification.
| - --pcap-file-recursive | ||
|
|
||
| checks: | ||
| # Race-condition regression: alert.pcap is processed FIRST; http.pcap is |
There was a problem hiding this comment.
Why is alert processed first ? Alphabetical order ?
There was a problem hiding this comment.
The code in source-pcap-file-directory-helper.c sorts by modification time (CompareTimes), not alphabetically. After a git checkout, both alert.pcap and http.pcap get the same mtime (I verified they're identical), so the order is actually determined by readdir() - which is filesystem-dependent and not guaranteed.
However, the test assertions are order-independent: they just verify that each event has the correct pcap_filename regardless of which pcap was processed first. The comment here is misleading because it claims a specific processing order. I'll update the comment.
2 participants
Add a regression test that checks the pcap_filename field is present and correct in EVE JSON output for both alert events (packet-based) and flow events (non-packet-based).
These two code paths use different sources for the filename in OutputJsonBuilderBuffer: per-packet pfv->filename when a packet is available, and the global PcapFileGetFilename() fallback for flow/netflow events where no packet exists.
Previous PR: #2927
v2:
- Fix recursive test and readme.
Redmine ticket: https://redmine.openinfosecfoundation.org/issues/5255
Suricata PR: OISF/suricata#15116