App proto logging

[regit]

There was a regression between Suricata 7 and Suricata 8. The app_proto was logged in almost all events in 7 and is only log in a small subset (fileinfo, flow, frame, netflow) in 8.

This patch updates the code to log app_proto in all events if there is a Flow available. It is making use of EveAddAppProto function to get interesting information such as original application protocol or difference between server and client side.

Contribution style:

• I have read the contributing guide lines at
  https://docs.suricata.io/en/latest/devguide/contributing/contribution-process.html

Our Contribution agreements:

• I have signed the Open Information Security Foundation contribution agreement at
  https://suricata.io/about/contribution-agreement/ (note: this is only required once)

Changes (if applicable):

• I have updated the User Guide (in doc/userguide/) to reflect the changes made
• I have updated the JSON schema (in etc/schema.json) to reflect all logging changes
  (including schema descriptions)
• I have created a ticket at
  https://redmine.openinfosecfoundation.org/projects/suricata/issues

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

Describe changes:

• log app_proto when creating EVE header

SV_BRANCH=OISF/suricata-verify#2640

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.72%. Comparing base (0662736) to head (32634bd).
⚠️ Report is 715 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #13810   +/-   ##
=======================================
  Coverage   83.71%   83.72%           
=======================================
  Files        1011     1011           
  Lines      275116   275115    -1     
=======================================
+ Hits       230321   230330    +9     
+ Misses      44795    44785   -10     

Flag	Coverage Δ
fuzzcorpus	63.02% <100.00%> (+0.02%)	⬆️
livemode	19.00% <53.33%> (+<0.01%)	⬆️
pcap	44.69% <86.66%> (+<0.01%)	⬆️
suricata-verify	65.09% <100.00%> (+<0.01%)	⬆️
unittests	59.16% <0.00%> (+<0.01%)	⬆️

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

🚀 New features to boost your workflow:

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

[victorjulien]

Would be nice to get a Fixes tag for this, do we know which commit(s) broke it?

[regit]

Would be nice to get a Fixes tag for this, do we know which commit(s) broke it?

I did not investigate. I'll try to get that.

[regit]

Would be nice to get a Fixes tag for this, do we know which commit(s) broke it?

I did not investigate. I'll try to get that.

OUTCH, really sorry, this is not a regression. I forgot one patch was bringing that to the 7.0.x branch I did test. So this issue becomes a feature request. I'm updating redmine.

[catenacyber]

https://redmine.openinfosecfoundation.org/issues/7888 for the record ;-)

[catenacyber]

So, the commit message needs to be changed :-p

There was a regression between Suricata 7 and Suricata 8.

Otherwise, code and tests look good...

I wonder if this should be optional (as logs become more verbose)
I guess this deserves an upgrade note

[jufajardini]

So, the commit message needs to be changed :-p

There was a regression between Suricata 7 and Suricata 8.

Otherwise, code and tests look good...

I wonder if this should be optional (as logs become more verbose) I guess this deserves an upgrade note

I agree on this becoming optional. But could there maybe be a shorter version, instead of all or nothing? (Not fully sure if such a thing would make sense 🤔 )

[catenacyber]

@regit will you come back to this ?

[regit]

@regit will you come back to this ?

Hello, yes. I will work on it this week.

[regit]

So, the commit message needs to be changed :-p

There was a regression between Suricata 7 and Suricata 8.

Otherwise, code and tests look good...
I wonder if this should be optional (as logs become more verbose) I guess this deserves an upgrade note

I agree on this becoming optional. But could there maybe be a shorter version, instead of all or nothing? (Not fully sure if such a thing would make sense 🤔 )

I'm not feeling like adding a new option for that. Should we introduce a version in EVE configuration section so we can preserve backward compatibility by reading that in the YAML and only setting up the log if version is above the chosen value ?

[regit]

So, the commit message needs to be changed :-p

There was a regression between Suricata 7 and Suricata 8.

Otherwise, code and tests look good...
I wonder if this should be optional (as logs become more verbose) I guess this deserves an upgrade note

I agree on this becoming optional. But could there maybe be a shorter version, instead of all or nothing? (Not fully sure if such a thing would make sense 🤔 )

I'm not feeling like adding a new option for that. Should we introduce a version in EVE configuration section so we can preserve backward compatibility by reading that in the YAML and only setting up the log if version is above the chosen value ?

I've pushed an implementation of this concept there: https://github.com/regit/suricata/tree/app_proto_logging-v2

[catenacyber]

So, should you open a draft PR with this v2 branch ? Or ?

[regit]

So, should you open a draft PR with this v2 branch ? Or ?

Yes, just wanted to get some feedback on it first but here is the PR: #14760