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 - v5 #12423

Closed
wants to merge 6 commits into from
Closed

Conversation

jasonish
Copy link
Member

Previous PR: #12412

Changes:

  • Reference ticket for C name collisions
  • Break up keyword registration and keyword ID assignment into 2 commits. This plugin just needs the dynamic ID assignment.

Outstanding comment from previous PR:

  • I guess that last bit we be an S-V test, since we do plan on shipping this in our tree.

cardigliano and others added 6 commits January 17, 2025 14:32
- 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.
By adding no_mangle to non-pub functions they enter the global name
space and can conflict with other functions. In this case another sha
library used by a plugin.

Related ticket: https://redmine.openinfosecfoundation.org/issues/7498
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.
Copy link

codecov bot commented Jan 17, 2025

Codecov Report

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

Project coverage is 80.63%. Comparing base (8f6795d) to head (b7fa276).
Report is 36 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #12423      +/-   ##
==========================================
- Coverage   80.63%   80.63%   -0.01%     
==========================================
  Files         918      918              
  Lines      258696   258696              
==========================================
- Hits       208598   208587      -11     
- Misses      50098    50109      +11     
Flag Coverage Δ
fuzzcorpus 56.81% <92.30%> (+<0.01%) ⬆️
livemode 19.40% <92.30%> (+<0.01%) ⬆️
pcap 44.27% <92.30%> (-0.03%) ⬇️
suricata-verify 63.25% <92.30%> (+0.01%) ⬆️
unittests 58.52% <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 24257

Copy link
Member

@victorjulien victorjulien left a comment

Choose a reason for hiding this comment

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

see inline

Suricata should be compiled with the nDPI support and the ``ndpi``
plugin must be loaded before it can be used.

Example of configuring Suricata to be compiled with nDPI support:
Copy link
Member

Choose a reason for hiding this comment

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

would be good to mention the requires rule support here as well I think

- Malware host contacted
- and many other...

Suricata should be compiled with the nDPI support and the ``ndpi``
Copy link
Member

Choose a reason for hiding this comment

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

requires support

plugins/ndpi/ndpi.c Show resolved Hide resolved
plugins/ndpi/ndpi.c Show resolved Hide resolved
plugins/ndpi/ndpi.c Show resolved Hide resolved
plugins/ndpi/ndpi.c Show resolved Hide resolved
plugins/ndpi/ndpi.c Show resolved Hide resolved
plugins/ndpi/ndpi.c Show resolved Hide resolved
plugins/ndpi/ndpi.c Show resolved Hide resolved
plugins/ndpi/ndpi.c Show resolved Hide resolved
@jasonish
Copy link
Member Author

Ping @cardigliano, can you take a look at these issues? I think the "integration" is more or less complete with this PR, just some code items to deal with in the implementation of the plugin.

@cardigliano
Copy link
Contributor

I fixed the issues in our branch

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.

4 participants