mqtt: enable limiting of logged message length - v2 #11054
Conversation
satta
requested review from
jasonish,
jufajardini,
victorjulien and
a team
as code owners
satta
force-pushed
the
6984-limit-mqtt-logs-v2
branch
from
af29595
to
aa47dca
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #11054 +/- ##
==========================================
+ Coverage 80.63% 83.65% +3.01%
==========================================
Files 922 922
Lines 250137 250338 +201
==========================================
+ Hits 201699 209417 +7718
+ Misses 48438 40921 -7517
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Are there other MQTT fields that should go into this limitation ? Like topics... |
There was a problem hiding this comment.
Thanks for the work :-)
I guess the first thing to do is to determine the scope : are there other fields than msg that can be logged unbounded ?
But there is already a limit on the parsed PDU size, which in turn limits the log length, right ?
- CI : 🟠 probably just need a rebase
- Code :Checking now
- Commits segmentation : ok
- Commit messages : ok for me
- Git ID set : looks fine for me
- CLA : you already contributed
- Doc update : @jufajardini what do you think about the commit doc: update example
outputssection ? How should we make sure we do not lag behind again, as I did not add websocket-payload for instance - Redmine ticket : updated it ;-)
- Rustfmt : you ran it and I like it, but I am not sure it is relevant...
- Tests : 🟠 you tested test: test for lua rules erroring out - v2 suricata-verify#1826 instead of mqtt: add tests for MQTT log limiting suricata-verify#1828
- Dependencies added: none
| @@ -112,6 +115,15 @@ static void JsonMQTTLogParseConfig(ConfNode *conf, LogMQTTFileCtx *mqttlog_ctx) | |||
| } else { | |||
| mqttlog_ctx->flags |= MQTT_LOG_PASSWORDS; | |||
| } | |||
| uint32_t max_msg_log_len = 0; | |||
| query = ConfNodeLookupChildValue(conf, "msg-log-limit"); | |||
There was a problem hiding this comment.
@jasonish what do you think about doing this in rust ? and setting a global variable with this field
There was a problem hiding this comment.
Probably only worth it if the whole Config function can be moved to Rust.
There was a problem hiding this comment.
the whole Config function can be moved to Rust.
Yes, we can, but the way I see it means using a global variable, so unsafe static mut...
For our devguide, we use the I wonder if something like that could be used with our suricata.yaml file, and then we'd reference that, instead of the |
This commit adds config items for features that have been introduced in the meantime but not added to the example configuration in the documentation.
satta
force-pushed
the
6984-limit-mqtt-logs-v2
branch
from
aa47dca
to
a023878
Compare
|
Never mind the push, will send new MR soon |
|
Next PR: #11194 |
Previous PR: #11053
Changes to previous PR:
suricata.yamloptions to EVE Output documentation page.outputssection in documentation.Ticket
Redmine ticket: https://redmine.openinfosecfoundation.org/issues/6984
SV_BRANCH=OISF/suricata-verify#1826