Skip to content

Output alert applayer v13.3#9807

Closed
catenacyber wants to merge 12 commits intoOISF:masterfrom
catenacyber:output-alert-applayer-v13.3
Closed

Output alert applayer v13.3#9807
catenacyber wants to merge 12 commits intoOISF:masterfrom
catenacyber:output-alert-applayer-v13.3

Conversation

@catenacyber
Copy link
Contributor

Link to redmine tickets:
https://redmine.openinfosecfoundation.org/issues/3827
https://redmine.openinfosecfoundation.org/issues/5977
https://redmine.openinfosecfoundation.org/issues/6500
https://redmine.openinfosecfoundation.org/issues/6501
https://redmine.openinfosecfoundation.org/issues/5053

Describe changes:

  • Fix setup-app-layer script so that it adds app-layer metadata to alerts
  • add krb5 metadata to alerts
  • add ftp metadata to alerts
  • add tftp metadata to alerts
  • output/dns: do not add empty app-layer metadata
  • app-layer: do not require probing parser as fixed patterns can be enough
  • app-layer plugins

#9799 with more commits

SV_BRANCH=pr/1465

OISF/suricata-verify#1465

Draft cc @jasonish
I think the first commits ie #9768 should be merged first in its own PR
But that gives an overview of the POC

catenacyber and others added 12 commits November 16, 2023 11:46
Especially fix setup-app-layer script to not forget this part

This allows, for simple loggers, to have a unique definition
of the actual logging function with the jsonbuilder.
This way, alerts, files, and app-layer event can share the code
to output the same data.

Ticket: OISF#3827
as fixed patterns can be enough
Only implemented for snmp.version and mqtt.password
But should be implemented for more
So that we can have dynamically registered protocols.
Doing it at compile time, with CFLAGS=-DALPROTO_DYNAMIC_NB=1,
allows to keep the rest of the code using ALPROTO_MAX

Ticket: 5053
@catenacyber
Copy link
Contributor Author

Also draft :
Should we completely remove src/detect-mqtt-connect-password.c to have keyword registration and all done in rust only ?
Does it matter if DETECT_AL_MQTT_CONNECT_PASSWORD is no longer a constant ?

@catenacyber
Copy link
Contributor Author

Replaced by #9812

static size_t app_layer_plugins_nb = 0;
#endif

int SCPluginRegisterAppLayer(SCAppLayerPlugin *plugin)
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to not think about registering an app-layer as a plugin. Could app-layers not be registered the same being included or as plugins? For plugin support we just expose the API that Suricata would already be using.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not understand your sentences here... Could you rephrase ?

(Plus I need to push a plugin example)

Copy link
Member

@jasonish jasonish Nov 16, 2023

Choose a reason for hiding this comment

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

Why couldn't a non-plugin AppLayer not register itself through this API? In which case its no longer a plugin API, its just the API for registering app-layers. In short, putting Plugin in here seems a little artificial.

It was a mistake in the EVE file type API which I've already started to undo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so this is just about renaming SCPluginRegisterAppLayer to SCRegisterDynamicAppLayer, do I get it ?

Would we want to get rid of all the static ones ?

Copy link
Member

Choose a reason for hiding this comment

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

Would we want to get rid of all the static ones ?

Personally I'd think so unless there is a performance hit?

I like to think of plugin support as just exposing the APIs we already use internally. Then there is nothing really different from a built-in or plugin, and we're constantly dog-fooding the APIs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And so, should this be moved out of util-plugin.c to app-layer-register.c ?

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.

2 participants

Comments