ndpi: ndpi as a plugin - v10 #12560
ndpi: ndpi as a plugin - v10 #12560
Conversation
- Download and build nDPI - Enable nDPI during Suricata ./configure - Test that the plugin was built and installed
The format is left free-form, as its controled by a plugin.
Split DetectHelperKeywordRegister into 2 functions, one for acquiring a new keyword ID, and another to perform the registration. This makes it easier to do the traditional C keyword initialization with a dynamic ID.
- remove duplicate calls to ndpi_init_detection_module - cleanup ndpi_init_detection_module when no longer needed
Moves the nDPI documentation to an nDPI page in the plugins section. Remove the duplication of installation and setup documentation.
jasonish
requested review from
jufajardini,
victorjulien and
a team
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12560 +/- ##
==========================================
- Coverage 80.71% 80.70% -0.01%
==========================================
Files 928 928
Lines 259007 259008 +1
==========================================
- Hits 209063 209044 -19
- Misses 49944 49964 +20
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Information: QA ran without warnings. Pipeline 24687 |
doc/userguide/rules/ndpi-risk.rst
Outdated
| - Known Proto on Non Std Port | ||
| - Binary App Transfer | ||
| - Self-signed Certificate | ||
| - Susp DGA Domain name |
There was a problem hiding this comment.
nit: Suggest using full word instead of abbreviation.
doc/userguide/rules/ndpi-risk.rst
Outdated
| Match on the flow risks detected by nDPI. Risks are potential issues detected | ||
| by nDPI during the packet dissection and include: | ||
|
|
||
| - Known Proto on Non Std Port |
There was a problem hiding this comment.
nit: s/Non Std/non standard/
| @@ -0,0 +1,537 @@ | |||
| /* Copyright (C) 2024 Open Information Security Foundation | |||
| } | ||
| } | ||
|
|
||
| if (flowctx->detection_completed) { |
There was a problem hiding this comment.
Since this is DEBUG only, suggest guarding block with if (SCLogDebugEnabled())
| { | ||
| struct NdpiThreadContext *threadctx = ThreadGetStorageById(tv, thread_storage_id); | ||
| struct NdpiFlowContext *flowctx = FlowGetStorageById(f, flow_storage_id); | ||
| uint16_t ip_len = 0; |
There was a problem hiding this comment.
nit -- neither ip_len nor ip_ptr require initialization as they're set by all logic paths before later user
| /* Use ndpi_dpi2json to get a JSON with nDPI metadata */ | ||
| ndpi_dpi2json(threadctx->ndpi, flowctx->ndpi_flow, flowctx->detected_l7_protocol, &serializer); | ||
|
|
||
| buffer = ndpi_serializer_get_buffer(&serializer, &buffer_len); |
There was a problem hiding this comment.
Can ndpi_serializer_get_buffer return NULL?
|
|
||
| - name: Build and install nDPI | ||
| run: | | ||
| curl -OL https://github.com/ntop/nDPI/archive/refs/tags/4.12.tar.gz |
There was a problem hiding this comment.
Can the version number be refactored with a variable so it's easier to maintain across version updates?
| @@ -6873,6 +6873,10 @@ | |||
| } | |||
| }, | |||
| "additionalProperties": false | |||
| }, | |||
| "ndpi": { | |||
| "description": "nDPI plugin, contents provided by 3rd party library", | |||
| ndpi-risk:[!]<risk>; | ||
|
|
||
| Where risk is one (or multiple comma-separated) of the risk codes supported by | ||
| nDPI (e.g. NDPI_BINARY_APPLICATION_TRANSFER). Please check ndpiReader -H for the |
There was a problem hiding this comment.
nit: suggest using backticks with ndpiReader -H.
| Additionally, rules using the ``ndpi-risk`` keyword should check if | ||
| the keyword exists using the ``requires`` keyword, for example:: | ||
|
|
||
| ``requires: keyword ndpi-risk`` |
Previous PR: #12476
Changes:
This is probably my final iteration on this as it handles the "mechanics" of
bundled plugins I think. When reviewing, if you can, please try to consider the
"bundling" of plugins, vs the plugin itself.
Ticket: https://redmine.openinfosecfoundation.org/issues/7231