ndpi: ndpi as a plugin - v6 #12476
ndpi: ndpi as a plugin - v6 #12476
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.
The eve callback in ndpi requires a flow, so bail earlier if there is no flow.
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.
Suggest that rules using ndpi keywords should also test for existence of the keyword with requires.
- remove duplicate calls to ndpi_init_detection_module - cleanup ndpi_init_detection_module when no longer needed
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 #12476 +/- ##
=======================================
Coverage 80.52% 80.53%
=======================================
Files 923 923
Lines 259176 259176
=======================================
+ Hits 208708 208716 +8
+ Misses 50468 50460 -8
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Information: QA ran without warnings. Pipeline 24349 |
There was a problem hiding this comment.
Thanks for the work :-)
CI should run the nDPI plugin, right ?
CI : 🟢
Code : looking
Commits segmentation : ok even if I may have squashed some together
Commit messages : good
Git ID set : looks fine for me
CLA : you already contributed :-p
Doc update : I wonder if a plugin keywords belong to the base suricata doc... Has this already been discussed ? And I would put both keywords on the same ndpi.rst page
Redmine ticket : ok, I closed https://redmine.openinfosecfoundation.org/issues/511
Rustfmt : no rust
Tests :
Dependencies added: none
| @@ -648,6 +658,8 @@ jobs: | |||
| with: | |||
| name: prep | |||
| path: prep | |||
| - name: Check if the nDPI plugin was installed | |||
| run: test -e /usr/local/lib/suricata/ndpi.so | |||
There was a problem hiding this comment.
We should run some pcap with it like https://github.com/OISF/suricata/pull/12471/files#diff-a76cf7978f0a981f911e8d68d2351a72a268977304612226433df4fb8203b06fR194 (this is done for other plugins of yours if I am correct)
| @@ -38,6 +38,8 @@ Suricata Rules | |||
| smtp-keywords | |||
| websocket-keywords | |||
| app-layer | |||
| ndpi-protocol | |||
There was a problem hiding this comment.
I think we should have only one ndpi entry here
|
|
||
| Match on the Layer-7 protocol detected by nDPI. | ||
|
|
||
| Suricata should be compiled with the nDPI support and the ``ndpi`` |
There was a problem hiding this comment.
nit space at end of line
| @@ -6866,6 +6866,10 @@ | |||
| } | |||
| }, | |||
| "additionalProperties": false | |||
| }, | |||
| "ndpi": { | |||
There was a problem hiding this comment.
Same question than for doc : does this plugin stuff belong in base suricata etc schema.json ?
Can a plugin propose its json schema extension ?
| @@ -95,7 +95,7 @@ int DetectHelperMultiBufferMpmRegister(const char *name, const char *desc, AppPr | |||
| return DetectBufferTypeGetByName(name); | |||
| } | |||
|
|
|||
| int DetectHelperKeywordRegister(const SCSigTableElmt *kw) | |||
| int SCDetectHelperNewKeywordId(void) | |||
| { | |||
There was a problem hiding this comment.
Jason, I recommend adding commit 8582f99 ;-)
| static int ndpi_protocol_keyword_id = -1; | ||
| static int ndpi_risk_keyword_id = -1; | ||
|
|
||
| struct NdpiThreadContext { |
There was a problem hiding this comment.
Does it make sense to break this plugin into multiple C files ? like a rust app-layer with detection, logging, etc..?
Previous PR: #12423
Changes:
Ticket: https://redmine.openinfosecfoundation.org/issues/7231