Applayer plugin 5053 v3.9 by catenacyber · Pull Request #12066 · OISF/suricata

catenacyber · 2024-10-30T14:25:59Z

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

Describe changes:

get ready to use dynamic number of app-layer protos (also work with static constant) in some places
preventive fix of macro with parenthesis cc @jufajardini

#12060 with profiling fix and clean history

Example plugin at https://github.com/catenacyber/suricata-zabbix

instead of a global variable. For easier initialization with dynamic number of protocols

for expectation_proto Ticket: 5053

for alproto_names Ticket: 5053

Ticket: 5053

so that we can use safely EXCEPTION_POLICY_MAX*sizeof(x)

Ticket: 5053

Ticket: 5053 delay after initialization so that StringToAppProto works

Ticket: 5053

As too many cases are found when splitting tcp payload

As it is also used for HTTP/1

Ticket: 5053 The names are nwo dynamic

Ticket: 5053

codecov · 2024-10-30T15:19:06Z

Codecov Report

Attention: Patch coverage is 74.43182% with 90 lines in your changes missing coverage. Please review.

Project coverage is 83.29%. Comparing base (3a7eef8) to head (b34acb1).
Report is 33 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #12066      +/-   ##
==========================================
- Coverage   83.42%   83.29%   -0.14%     
==========================================
  Files         910      910              
  Lines      257642   257757     +115     
==========================================
- Hits       214949   214698     -251     
- Misses      42693    43059     +366

Flag	Coverage Δ
fuzzcorpus	`61.06% <68.01%> (-0.59%)`	⬇️
livemode	`19.42% <57.92%> (+0.01%)`	⬆️
pcap	`44.48% <70.02%> (+<0.01%)`	⬆️
suricata-verify	`62.76% <74.06%> (+<0.01%)`	⬆️
unittests	`59.35% <57.67%> (-0.02%)`	⬇️

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

suricata-qa · 2024-10-30T19:40:03Z

Information: QA ran without warnings.

Pipeline 23220

victorjulien

Code looks mostly ok, have some style issues and minor other things inline.

Does need a lot more documentation in the commit messages, as well as more comments in the new functions, structs and global variables.

victorjulien · 2024-11-06T18:50:13Z

src/app-layer-protos.c

 #include "app-layer-protos.h"
+#include "rust.h"
+
+AppProto ALPROTO_FAILED = ALPROTO_MAX_STATIC;


I don't like the coding style violations here, macro style name for vars, function style name for a var, etc.

can also imagine that the now global var ALPROTO_MAX would be replaced by a function AppProtoMax() or something.

Why a function instead of a variable ?

victorjulien · 2024-11-06T19:02:04Z

src/app-layer-protos.c

    return ALPROTO_UNKNOWN;
 }
+
+void RegisterAppProtoString(AppProto alproto, const char *proto_name)


can ALPROTO_FAILED just be a constant instead of having it be a dynamic value for no real reason I can see?

ALPROTO_FAILED is used in comparison like < ALPROTO_FAILED ...
Will think about it again

Looks like we can move ALPROTO_FAILED to 1 and change the comparisons... in next PR version

rust/src/sip/sip.rs

rust/src/core.rs

src/app-layer-protos.h

victorjulien · 2024-11-06T19:08:02Z

src/app-layer.c

 /***** Setup/General Registration *****/
+static void AppLayerNamesSetup(void)
+{
+    RegisterAppProtoString(ALPROTO_UNKNOWN, "unknown");


should this move into the actual parser implementations?

Maybe as a next step that makes a protocol like SNMP behave like a plugin ? (no static definition of ALPROTO_SNMP)

src/app-layer-protos.c

src/detect-engine-file.h

victorjulien · 2024-11-06T19:11:13Z

src/suricata-plugin.h


 int SCPluginRegisterCapture(SCCapturePlugin *);

+typedef struct SCAppLayerPlugin_ {


needs some struct docs

@catenacyber Do you see this as being only used for app-layer plugins? Or become the new way to register app-layers? If the latter, it should be more generic than plugin context. Also its a lot odd to be here, but then used in detect-engine-register.c, I think it should be localized to plugin specific code.

Just for some context, if "plugin" is mentioned outside of a file with plugin in the name, its probably suspect to a leaky abstraction. We have this for capture plugins still, but have got rid of it for other types of plugins.

Do you see this as being only used for app-layer plugins?

That is what I thought at first

Ok, then I'd move the usage of the Plugin struct out of the app-layer. Keep it within the plugin API, and not the app-layer API. Then register it from there so the plugin interface doesn't leak into the app-layer API if that makes sense.

What do you mean ? For example, Which line of code should I move from which file to which file ?

I guess this:

#ifdef HAVE_PLUGINS for (size_t i = 0; i < app_layer_plugins_nb; i++) { SCAppLayerPlugin *app_layer_plugin = SCPluginFindAppLayerByIndex(i); if (app_layer_plugin == NULL) { break; } app_layer_plugin->Register(); } #endif

it doesn't really need HAVE_PLUGINS, its simply dynamic app-layer registration right?

It just feels weird that the app-layer "layer" is plugin aware, instead the plugin-layer would be app-layer aware.

Can we get a sample plugin for CI purposes to make sure it works? Might help reason about it.

Can we get a sample plugin for CI purposes to make sure it works?

Example plugin at https://github.com/catenacyber/suricata-zabbix ;-)

It just feels weird that the app-layer "layer" is plugin aware, instead the plugin-layer would be app-layer aware.

I see what you mean.
The problem is different initialization timings :

plugin is initialized

app-layer are registered (and then keywords, and then outputs)

So the plugin init cannot call directly its app-layer registration.
Instead, it registers a callback that will be called during app-layer registration...

Example plugin at https://github.com/catenacyber/suricata-zabbix ;-)

I think we need some minimal in-tree so we can make sure it works. For example, this is the reason why examples/plugins/ci-capture/ exists - however we also now have 2 in-tree capture methods that are plugins as well.

Done in #12116

catenacyber · 2024-11-11T16:46:55Z

Replaced by #12110

catenacyber added 14 commits October 29, 2024 11:13

protodetect: make expectation_proto part of AppLayerProtoDetectCtx

5ddb2ba

instead of a global variable. For easier initialization with dynamic number of protocols

protodetect: use dynamic number of app-layer protos

c6bd4ef

for expectation_proto Ticket: 5053

protodetect: use dynamic number of app-layer protos

1a39105

for alproto_names Ticket: 5053

frames: use dynamic number of app-layer protos

3338c82

Ticket: 5053

util: parenthesis for macro

4b08f81

so that we can use safely EXCEPTION_POLICY_MAX*sizeof(x)

app-layer/stats: use dynamic number of app-layer protos

aac2715

Ticket: 5053

fuzz: use dynamic number of app-layer protos

2545897

Ticket: 5053 delay after initialization so that StringToAppProto works

app-layer/parser: use dynamic number of app-layer protos

42db243

Ticket: 5053

profiling: use dynamic number of app-layer protos

eb2c7f8

Ticket: 5053

fuzz: better init for protocol detection

45679d1

fuzz: simplify target for protocol detection

df05782

As too many cases are found when splitting tcp payload

sip: remove UPDATE method for detection

ff98a23

As it is also used for HTTP/1

app-layer: make number of alprotos dynamic

867f486

Ticket: 5053 The names are nwo dynamic

plugins: app-layer plugins

b34acb1

Ticket: 5053

catenacyber requested review from a team, jasonish and victorjulien as code owners October 30, 2024 14:26

catenacyber mentioned this pull request Oct 30, 2024

Applayer plugin 5053 v3.8 #12060

Closed

victorjulien requested changes Nov 6, 2024

View reviewed changes

victorjulien added the needs rebase Needs rebase to main label Nov 6, 2024

catenacyber mentioned this pull request Nov 11, 2024

Applayer plugin 5053 v3.11 #12110

Closed

catenacyber closed this Nov 11, 2024


		int SCPluginRegisterCapture(SCCapturePlugin *);

		typedef struct SCAppLayerPlugin_ {

Conversation

catenacyber commented Oct 30, 2024

Uh oh!

codecov bot commented Oct 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

suricata-qa commented Oct 30, 2024

Uh oh!

victorjulien left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

catenacyber commented Nov 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

codecov bot commented Oct 30, 2024 •

edited

Loading