Applayer plugin 5053 v3.19 #12364
Applayer plugin 5053 v3.19 #12364
Conversation
Because some alprotos will remain static and defined as a constant, such as ALPROTO_UNKNOWN=0, or ALPROTO_FAILED. The regular already used protocols keep for now their static identifier such as ALPROTO_SNMP, but this could be made more dynamic in a later commit. ALPROTO_FAILED was used in comparison and these needed to change to use either ALPROTO_MAX or use standard function AppProtoIsValid
Ticket: 5053 The names are now dynamically registered at runtime. The AppProto alproto enum identifiers are still static for now. This is the final step before app-layer plugins.
Ticket: 5053
catenacyber
requested review from
victorjulien,
jasonish and
a team
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12364 +/- ##
==========================================
- Coverage 82.54% 80.50% -2.05%
==========================================
Files 912 913 +1
Lines 258028 258152 +124
==========================================
- Hits 212988 207818 -5170
- Misses 45040 50334 +5294
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Information: QA ran without warnings. Pipeline 24156 |
| @@ -24,53 +24,18 @@ | |||
|
|
|||
| #include "suricata-common.h" | |||
| #include "app-layer-protos.h" | |||
| #include "rust.h" | |||
|
|
|||
| AppProto AlprotoMax = ALPROTO_MAX_STATIC; | |||
There was a problem hiding this comment.
style: should be snake case, probably g_alproto_max here. The g_ to indicate it is a global.
Alternatively it could be replaced by a inline func AlprotoMax() which would return a lower scoped static variable.
| @@ -27,6 +27,11 @@ | |||
|
|
|||
| enum AppProtoEnum { | |||
| ALPROTO_UNKNOWN = 0, | |||
| /* used by the probing parser when alproto detection fails | |||
| * permanently for that particular stream */ | |||
| ALPROTO_FAILED = 1, | |||
There was a problem hiding this comment.
should have a comment that any update to the value should also be updated in rust I think
| /* keep last */ | ||
| ALPROTO_MAX, | ||
| ALPROTO_MAX_STATIC, |
There was a problem hiding this comment.
Can you add a comment that after this there will be dynamic id's?
| @@ -1029,11 +1029,54 @@ void AppLayerListSupportedProtocols(void) | |||
| } | |||
|
|
|||
| /***** Setup/General Registration *****/ | |||
| static void AppLayerNamesSetup(void) | |||
| { | |||
| AppProtoRegisterProtoString(ALPROTO_UNKNOWN, "unknown"); | |||
There was a problem hiding this comment.
can these move into the parser registrations? E.g.
/**
* \brief Register the SMTP Protocol parser.
*/
void RegisterSMTPParsers(void)
{
const char *proto_name = "smtp";
AppProtoRegisterProtoString(ALPROTO_SMTP, proto_name);| static size_t preregistered_callbacks_nb = 0; | ||
| static size_t preregistered_callbacks_cap = 0; | ||
|
|
||
| int SigTablePreRegister(void (*KeywordsRegister)(void)) |
There was a problem hiding this comment.
we need some explanation of what this is here
| @@ -708,6 +729,10 @@ void SigTableSetup(void) | |||
| ScDetectSipRegister(); | |||
| ScDetectTemplateRegister(); | |||
|
|
|||
| for (size_t i = 0; i < preregistered_callbacks_nb; i++) { | |||
| preregistered_callbacks[i](); | |||
There was a problem hiding this comment.
style: function pointers using FunctionPointer style
| static size_t preregistered_loggers_nb = 0; | ||
| static size_t preregistered_loggers_cap = 0; | ||
|
|
||
| int OutputPreRegisterLogger(EveJsonLoggerRegistrationData reg_data) |
There was a problem hiding this comment.
needs doc, like with detect
| preregistered_loggers[i].confname, OutputJsonLogInitSub, | ||
| preregistered_loggers[i].alproto, JsonGenericDirFlowLogger, JsonLogThreadInit, | ||
| JsonLogThreadDeinit); | ||
| SCLogNotice( |
There was a problem hiding this comment.
don't think we want to keep this as notice
| const char *logname; | ||
| AppProto alproto; | ||
| EveJsonSimpleTxLogFunc LogTx; | ||
| } EveJsonLoggerRegistrationData; |
There was a problem hiding this comment.
name is already long, but should it include an indication that it is about Tx logging and/or app-layer logging?
Link to ticket: https://redmine.openinfosecfoundation.org/issues/
https://redmine.openinfosecfoundation.org/issues/5053
Describe changes:
#12363 without zabbix plugin in tree, and test fix due to splitting with #12307
Note that there is still #12307 to fix the limitation of probing parsers against 32 protocols (meaning any new app-layer like one in a plugin may be affected by this bug if it uses probing parsers for protocol detection)