Draft: enable output-specific metadata logging configuration in alerts

[satta]

• [X] I have read the contributing guide lines at https://docs.suricata.io/en/latest/devguide/contributing/contribution-process.html
• [X] I have signed the Open Information Security Foundation contribution agreement at https://suricata.io/about/contribution-agreement/ (note: this is only required once)

This draft outlines a potential approach to making app-layer logging options (like mqtt.msg-log-limit (#11054)) available to metadata logging in alerts. The problem there was so far that there is no way of passing these settings to the metadata logging code, since a EveJsonSimpleTxLogFunc only gets the transaction and the JsonBuilder as parameters. Using a global static data structure is only half a solution since it is well possible to have multiple EVE outputs that can have different settings for the same event type, and we would obviously like the alerts in EVE output A use the same options as all the app-layer logging in output A, and the same behaviour for different settings in EVE outpub B and C. With such a global variable we can't reflect that.

Describe changes:

• Introduce interface change for EveJsonSimpleTxLogFunc passing a metadata-specific context into the log function. This is done in the C function declaration and also in all current implementations, Rust and C.
• Add a new pointer array (one pointer per alproto) to the output-specific OutputJsonCtx to store metadata-specific context pointers.
• Use this construct to assign a struct with settings after parsing to that pointer array, and read and use it in EveJsonSimpleTxLogFunc if it is set. Memory-wise, the context struct is already handled (allocated and freed) using the regular InitSub/DeinitCtxSub and all access is expected to only occur during the lifetime defined through this mechanism.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 83.87097% with 5 lines in your changes missing coverage. Please review.

Project coverage is 84.29%. Comparing base (e041187) to head (296bdca). Report is 1452 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #11160   +/-   ##
=======================================
  Coverage   84.28%   84.29%           
=======================================
  Files         926      926           
  Lines      243303   243315   +12     
=======================================
+ Hits       205076   205092   +16     
+ Misses      38227    38223    -4     

Flag	Coverage Δ
fuzzcorpus	64.18% <67.74%> (-0.01%)	:arrow_down:
livemode	19.79% <3.22%> (+0.12%)	:arrow_up:
pcap	46.64% <58.06%> (-0.03%)	:arrow_down:
suricata-verify	63.04% <80.64%> (+0.01%)	:arrow_up:
unittests	61.74% <0.00%> (-0.01%)	:arrow_down:

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

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[catenacyber]

Thanks Sacha, left a few comments/questions...

[catenacyber]

@satta will you work on this again ?

[satta]

@satta will you work on this again ?

Very far down my email inbox aka TODO list ;) Not something I currently plan doing soon. For instance, it would take me quite some time to find out how to do that in Rust. I'd be fine with having a pointer to squeeze things through. If anything, I wanted to be as little invasive as possible.

[catenacyber]

Thanks for the reply. It is low priority for me... Should we create a redmine ticket to reference the work already done ? (and close this GitHub PR)

[jasonish]

and we would obviously like the alerts in EVE output A use the same options as all the app-layer logging in output A

My question is, do we?

My train of thought here is when logging records for protocol X, we might want a "brief" mode, or whatever, however, when logging as part of alert, full detail would be nice.

[catenacyber]

My train of thought here is when logging records for protocol X, we might want a "brief" mode, or whatever, however, when logging as part of alert, full detail would be nice.

I think this PR would help in that direction anyways

[glongo]

I was planning to extend the SIP logger to enable logging of headers and wanted to add options similar to those available for HTTP. However, it seems that this is not currently possible, and this PR could be a good starting point. Considering there are some open questions, if we can determine how it should be done and no one else is working on it, I'm available to pick it up.

[jasonish]

@satta Can you share what was missing from the alert metadata that you are after? It always worth considering whats in our default, and it it could be made better as well.

[satta]

This PR is about making metadata logging configuration available. E.g. if we set mqtt.msg-log-limit in the EVE output to limit the length of metadata output, we also want that setting to apply to metadata fields that are logged in alerts. Currently we have to hardcode a default there because there is no way to share this setting. But I can imagine that the default there is not what all users want, as it may depend on their use case. Also, it's about consistency -- I can imagine users being surprised if they configured MQTT output in a particular way and it still does not match their preference in one spot compared to another.

I hope I understood correctly what you were asking about...

[jasonish]

Possibly another challenge.. In one eve-log, I can specify something like http multiple times with different options, for example:

        - http:
            extended: yes
        - http:
            extended: no
            dump-all-headers: both

I'm pretty sure the types was made a list by accident way back as we chose visual pleasantness over meaning.

[catenacyber]

I think we need something like that

[catenacyber]

Do we have a ticket about this ?

[catenacyber]

Closing as stale

@satta could you put more details in https://redmine.openinfosecfoundation.org/issues/7696 ?