Content-Length: 453891 | pFad | http://github.com/OISF/suricata/pull/12476

11 ndpi: ndpi as a plugin - v6 by jasonish · Pull Request #12476 · OISF/suricata · GitHub
Skip to content
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

Closed
wants to merge 7 commits into from
Closed

Conversation

jasonish
Copy link
Member

Previous PR: #12423

Changes:

  • Pull in code cleanups from ntop branch
  • Add note about "requires" keyword to doc
  • Fix memory leak in keyword setup.

Ticket: https://redmine.openinfosecfoundation.org/issues/7231

cardigliano and others added 7 commits January 24, 2025 10:39
- 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 jasonish requested review from jufajardini, victorjulien and a team as code owners January 24, 2025 17:53
Copy link

codecov bot commented Jan 24, 2025

Codecov Report

Attention: Patch coverage is 92.30769% with 2 lines in your changes missing coverage. Please review.

Project coverage is 80.53%. Comparing base (d63ad75) to head (26611f6).
Report is 51 commits behind head on master.

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     
Flag Coverage Δ
fuzzcorpus 56.06% <92.30%> (+<0.01%) ⬆️
livemode 19.38% <92.30%> (-0.03%) ⬇️
pcap 44.21% <92.30%> (+0.01%) ⬆️
suricata-verify 63.33% <92.30%> (+<0.01%) ⬆️
unittests 58.44% <92.30%> (-0.01%) ⬇️

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

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 24349

Copy link
Contributor

@catenacyber catenacyber left a 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 : ⚠️ should we not run the ndpi plugin in CI ?
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
Copy link
Contributor

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)

Copy link
Member Author

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
Copy link
Contributor

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

@jasonish
Copy link
Member Author

CLA : you already contributed :-p

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

etc/schema.json Show resolved Hide resolved
Comment on lines +12 to +27
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

Copy link
Contributor

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.

Copy link
Member Author

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.

doc/userguide/rules/ndpi-protocol.rst Show resolved Hide resolved
doc/userguide/rules/ndpi-protocol.rst Show resolved Hide resolved
doc/userguide/rules/ndpi-risk.rst Show resolved Hide resolved
@jasonish
Copy link
Member Author

Replaced by #12560.

@jasonish jasonish closed this Feb 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants








ApplySandwichStrip

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier!      Saves Data!


--- a PPN by Garber Painting Akron. With Image Size Reduction included!

Fetched URL: http://github.com/OISF/suricata/pull/12476

Alternative Proxies:

Alternative Proxy

pFad Proxy

pFad v3 Proxy

pFad v4 Proxy