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 - v2 #12120

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jasonish
Copy link
Member

Just a rebase and non-draft PR now that the support work has been merged into master.

cc: @lucaderi @cardigliano

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

@jasonish jasonish requested review from jufajardini and a team as code owners November 14, 2024 08:48
@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 23321

@victorjulien
Copy link
Member

How would testing this in SV work? Is there a way to know in a test that ndpi is available?

@jasonish
Copy link
Member Author

How would testing this in SV work? Is there a way to know in a test that ndpi is available?

I think S-V will need a small update to pick out enabled built-in plugins. I'm not sure if we have to add a "HAVE_XXX" for something that is not statically enabled.

We already have some support for this in S-V, but it just doesn't pickup stuff under the "Plugin" section of --build-info, but trivial to add.

The other question is if fields that are added to the logs by plugins should go into the schema as well?

@victorjulien
Copy link
Member

How would testing this in SV work? Is there a way to know in a test that ndpi is available?

I think S-V will need a small update to pick out enabled built-in plugins. I'm not sure if we have to add a "HAVE_XXX" for something that is not statically enabled.

We already have some support for this in S-V, but it just doesn't pickup stuff under the "Plugin" section of --build-info, but trivial to add.

The other question is if fields that are added to the logs by plugins should go into the schema as well?

This will require us to load the plugins and check the config before build info, right?

Wrt the schema, would it make sense to have a per plugin additional schema that is somehow used for those records?

@jasonish
Copy link
Member Author

This will require us to load the plugins and check the config before build info, right?

This PR has the following in --build-info:

Plugins:
  nDPI:                                    yes

S-V could also test that the plugin.so file exists as well.

@jasonish
Copy link
Member Author

jasonish commented Nov 14, 2024

Wrt the schema, would it make sense to have a per plugin additional schema that is somehow used for those records?

Will require research to see if JSON schema can be extended from another file, and if the schema validation libs we use would support doing that as well.

@victorjulien
Copy link
Member

This will require us to load the plugins and check the config before build info, right?

This PR has the following in --build-info:

Plugins:
  nDPI:                                    yes

S-V could also test that the plugin.so file exists as well.

So this tells us it is built, but not necessary loaded, right? So then the SV test would either need to load the plugin, or it would need to be able to check if it is loaded?

@jasonish
Copy link
Member Author

jasonish commented Nov 15, 2024

This will require us to load the plugins and check the config before build info, right?

This PR has the following in --build-info:

Plugins:
  nDPI:                                    yes

S-V could also test that the plugin.so file exists as well.

So this tells us it is built, but not necessary loaded, right? So then the SV test would either need to load the plugin, or it would need to be able to check if it is loaded?

If this is yes, the plugin path in suricata.yaml is the install path, so still not good enough to check if loaded.

So probably have to do something like:

requires:
  files:
    - plugins/ndpi/ndpi.so

args:
  - --set plugins.0=./plugins/ndpi/ndpi.so

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