Skip to content

Output alert applayer v7.1#9005

Closed
catenacyber wants to merge 7 commits intoOISF:masterfrom
catenacyber:output-alert-applayer-v7.1
Closed

Output alert applayer v7.1#9005
catenacyber wants to merge 7 commits intoOISF:masterfrom
catenacyber:output-alert-applayer-v7.1

Conversation

@catenacyber
Copy link
Contributor

Link to redmine ticket:
None, preliminary work for https://redmine.openinfosecfoundation.org/issues/5053 and app-layer plugins
Part of #8961 with rebase

Describe changes:

  • Fix setup-app-layer script so that it adds app-layer metadata to alerts

After that, there is still from #8961

  • addition of protocols missing alert metadata (like krb5) + behavioral change for dns alert metadata
  • reusing these SimpleTxLogFunc from a JsonGenericLogger to remove many C files

Draft looking for review on 3 points:

1.Continuing discussion of #8922 (comment)

I think a centralized store doesn't have to use a table of hardcoded entries, but could also be more dynamic, like with many other parts of the engine. The parsers could then all a registration function that adds their entry into the store.

Should I really do that ?
Having a centralized store of hardcoded entries was the easy way for me to see which protocols are missing alert metadata

  1. Should I squash all these commits together into one ?


/// populate a json object with transactional information, for logging
fn log(tx: &ModbusTransaction, js: &mut JsonBuilder) -> Result<(), JsonError> {
js.open_object("modbus")?;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. What is the right way to do this ?

open the object at the start of the logging function, or should the caller do that ?

Now, half the protocols do one way, and the other half does the other way
This PR makes it that the caller opens the object

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't say there is a right way, but we want to avoid empty objects, or even the overhead of empty objects. So I think the caller should only open the object if it knows it will contain data. If its the logging function that decides whether logging will happen or not, maybe it should open the object, to avoid the case where the caller would have the rewind to a marker.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way you put it, it looks better to open in the logging function which knows better if the object is empty

@codecov
Copy link

codecov bot commented Jun 9, 2023

Codecov Report

Merging #9005 (98e8d07) into master (e30f494) will decrease coverage by 0.10%.
The diff coverage is 96.82%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9005      +/-   ##
==========================================
- Coverage   82.40%   82.30%   -0.10%     
==========================================
  Files         969      969              
  Lines      273608   273462     -146     
==========================================
- Hits       225469   225081     -388     
- Misses      48139    48381     +242     
Flag Coverage Δ
fuzzcorpus 64.71% <87.30%> (-0.21%) ⬇️
suricata-verify 60.44% <96.82%> (-0.02%) ⬇️
unittests 62.95% <0.00%> (+0.03%) ⬆️

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

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 14375

@jasonish
Copy link
Member

Having a centralized store of hardcoded entries was the easy way for me to see which protocols are missing alert metadata

Would it makes sense to do that as a separate issue? Could even wait for 8 as its not needed write now. Otherwise I like what I'm seeing with all this cleanup.

@catenacyber
Copy link
Contributor Author

Replaced by #9034

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Comments