-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every S-V test that doesn't specify a custom suricata.yaml
is run with this plugin enabled. Which is why we needed the entry in the schema.
@@ -38,6 +38,8 @@ Suricata Rules | |||
smtp-keywords | |||
websocket-keywords | |||
app-layer | |||
ndpi-protocol |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have only one ndpi entry here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
And CLA is ok for the author of the plugin as well :) |
@@ -38,6 +38,8 @@ Suricata Rules | |||
smtp-keywords | |||
websocket-keywords | |||
app-layer | |||
ndpi-protocol |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
Example of configuring Suricata to be compiled with nDPI support: | ||
|
||
.. code-block:: console | ||
|
||
./configure --enable-ndpi --with-ndpi=/home/user/nDPI | ||
|
||
Note that rules using the ``ndpi-protocol`` should check if the | ||
``ndpi-protocol`` keyword exists with ``requires``, for example:: | ||
|
||
requires: keyword ndpi-protocol | ||
|
||
Example of suricata.yaml configuration file to load the ``ndpi`` plugin:: | ||
|
||
plugins: | ||
- /usr/lib/suricata/ndpi.so | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion to move the portions of both sections that are related to configuration to the configuration
chapter and then link to that, here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main configuration chapter? As a plugin, I think the documentation should be self contained, and our main configuration should not contain anything that is provided by a plugin, bundled with Suricata or not. Just to provide a clear separation.
Replaced by #12560. |
Previous PR: #12423
Changes:
Ticket: https://redmine.openinfosecfoundation.org/issues/7231