eve: share logger id to allow for more loggers - v4 #8006
Conversation
This is to avoid the tx logging code that doesn't support LoggerId values over 31 at this time. The simplest fix for now is to just have all JSON (eve) loggers use the same ID. DNP3 is left as-is for now as it needs some extra support in the parser.
Sort the LoggerId's in the order they are define in suricata-common.h.
Update DNP3 to work with a single TX logger, and just register one logger instead of 2. This primarily creates a TX per message instead of correlating replies to requests, which fits the DNP3 model better, but we didn't really have this concept nailed down when DNP3 was written.
| if (flags & STREAM_TOSERVER && tx->is_request) { | ||
| objects = &tx->objects; | ||
| } else if (flags & STREAM_TOCLIENT && !tx->is_request) { | ||
| objects = &tx->objects; | ||
| } |
There was a problem hiding this comment.
Its code like this that fails when APP_LAYER_PARSER_OPT_UNIDIR_TXS is set for the protocol. When set the direction logic appears to be inversed. The detect function is never called with STREAM_TOSERVER for a request transaction, and vice-versa, as these detect functions are being called the TCP ACK. Running in IPS mode makes the direction logic correct, which I'm still trying to figure out. I believe the progress logic is correct at https://github.com/OISF/suricata/pull/8006/files#diff-e6b6ada9b3a17879876cc1c3a1db5f162e3f834cc731bf3eab4d02bde39ae424R1437
However, the parser, logging and detect seems to work find without setting the APP_LAYER_PARSER_OPT_UNIDIR_TXS, so it might not be needed.
| #if 0 | ||
| /* While this parser is now fully unidirectional. setting this | ||
| * flag breaks detection at this time. */ | ||
| AppLayerParserRegisterOptionFlags( | ||
| IPPROTO_TCP, ALPROTO_DNP3, APP_LAYER_PARSER_OPT_UNIDIR_TXS); | ||
| #endif |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #8006 +/- ##
==========================================
- Coverage 81.65% 81.61% -0.04%
==========================================
Files 953 953
Lines 275749 275691 -58
==========================================
- Hits 225158 225006 -152
- Misses 50591 50685 +94
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
WARNING:
Pipeline 10007 |
|
Merged in #8097, thanks! I left out the DNP3 patch as I had no time yet to review that. |
|
Rebased: #8101 |
As we've hit the maximum number of transaction loggers for the current method
of tx logging, we either need to restructure or reduce the number of loggers.
It looks like all JSON tx loggers can share the same ID, which seems to be the
least intrusive approach at this time.
This will allow the bittorrent parser work, and other parsers to proceed.
Previous PR: #7999
Changes from last PR:
registered.