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

Applayer plugin example and doc #12441

Closed
wants to merge 4 commits into from

Conversation

catenacyber
Copy link
Contributor

Link to ticket: https://redmine.openinfosecfoundation.org/issues/
https://redmine.openinfosecfoundation.org/issues/4102 with all 6 subtickets

Describe changes:

  • add app-layer plugin example with template protocol
  • document app-layer plugins

@jufajardini what do you think about the doc ?

@jasonish as you created the tickets, is this what you expected ?

Draft: last commit is WIP for a Suricata-plugin crate.

But it recompiles every dependency of Suricata even if I just need some constants !!!
@jasonish do you see a better way ?

and use generic logger callback prototype with later cast
Ticket: 7151
Ticket: 7152
Ticket: 7154
Ticket: 7149
Ticket: 7150
Ticket: 7153
pub const APP_LAYER_EVENT_TYPE_TRANSACTION : AppLayerEventType = AppLayerEventType::APP_LAYER_EVENT_TYPE_TRANSACTION;

pub const SIGMATCH_NOOPT: u16 = detect::SIGMATCH_NOOPT;
pub const SIGMATCH_INFO_STICKY_BUFFER: u16 = detect::SIGMATCH_INFO_STICKY_BUFFER;
Copy link
Member

Choose a reason for hiding this comment

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

Looks like most of this stuff belongs in Suricata proper? I don't think its specific to plugin right? Its just needed by plugins, but its not specific to plugins, which is probably what this crate should be for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's discuss in the main part...

@jasonish
Copy link
Member

But it recompiles every dependency of Suricata even if I just need some constants !!!
@jasonish do you see a better way ?

Maybe not a crate then. We have plugin.rs which is only for use by plugins as well. Crate does seem like a better idea, but probably not if it depends on the suricata crate.

I think we need to focus on:

  • What in your plugin needs to get into suricata rust proper
  • Whats should be provided for plugin support

As this will then be locked down for 8. I haven't put much focus on Rust plugins yet, so don't have much comments on it yet.

@catenacyber
Copy link
Contributor Author

I think we need to focus on:

  • What in your plugin needs to get into suricata rust proper
  • Whats should be provided for plugin support

For plugin support, we should provide what is the PR previous version of suricata.rs :
This includes some type definitions, constant values, and structure definitions...
I want something like a header file...
I do not want my plugin to import and compile 200 dependencies from Suricata to know the value of IPPROTO_TCP = 6

So, do you see a good way ?
The best way I see now is to have this Suricata-plugin crate contain what is in my previous version of the plugin suricata.rs + util.rs

@catenacyber
Copy link
Contributor Author

We could cbindgen a C header file, then bindgen it again to rust to get this rust header-ish file, and have the Suricata-plugin crate define some other methods (like the jsonbuilder rusty feel-like even if we use the C API)

@jasonish
Copy link
Member

So, do you see a good way ?
The best way I see now is to have this Suricata-plugin crate contain what is in my previous version of the plugin suricata.rs + util.rs

But the plugin already requires the suricata crate right? So maybe the extra crate shouldn't be used. It was just a thought - maybe not a good one.

My redis output plugin has some stuff that should be in Suricata proper as well. I'd like to rebase that soon which will give me a better feel for what is going on.

@catenacyber
Copy link
Contributor Author

But the plugin already requires the suricata crate right?

Wrong :-p

@jasonish
Copy link
Member

But the plugin already requires the suricata crate right?

Wrong :-p

Any reason why not? Thats how I'd expect plugins to get access to data structures, SCLog stuff, and so no. At least thats how existing capture and output plugins do it.

@jasonish
Copy link
Member

This PR brings over the custom bindings my redis output has been carrying around for a while: #12446.

@catenacyber
Copy link
Contributor Author

But the plugin already requires the suricata crate right?

Wrong :-p

Any reason why not?

I do not want my plugin to import and compile 200 dependencies from Suricata to know the value of IPPROTO_TCP = 6
( As said above in #12441 (comment) )

@catenacyber
Copy link
Contributor Author

This PR brings over the custom bindings my redis output has been carrying around for a while: #12446.

Thanks

@jasonish
Copy link
Member

But the plugin already requires the suricata crate right?

Wrong :-p

Any reason why not?

I do not want my plugin to import and compile 200 dependencies from Suricata to know the value of IPPROTO_TCP = 6 ( As said above in #12441 (comment) )

But for an example, I don't think providing all that stuff in suricata.rs is ideal as most of it already exists in Suricata, just lacks the Rust bindings. For example, the logging - we already have that exposed in Rust. So what I think you're doing is more likely an edge case, not the normal case so probably not ideal for a an example plugin.

I think the Rust way here would be to break up the Rust part of Suricata into multiple crates, so you can import stuff at a more fine grained level.

@suricata-qa
Copy link

ERROR:

ERROR: QA failed on build_asan.

Pipeline 24315

@catenacyber
Copy link
Contributor Author

But for an example, I don't think providing all that stuff in suricata.rs is ideal

I totally agree.
I thought I had asked you (long ago) about the ideal stuff (see my Suricon talk as well)...

I am working on another POC using cbindgen + bindgen to make a crate Suricata-plugin that exports only a fraction of what Suricata has already available from rust.

For example, the logging - we already have that exposed in Rust. So what I think you're doing is more likely an edge case, not the normal case so probably not ideal for a an example plugin.

I am not sure I follow you.

But logging is a good example :
One plugin cannot use the rust JsonBuilder struct. Because we need it to work at runtime even if the plugin and suricata were compiled with different rust compilers, versions, flags, etc...
So, the plugin has to resort to the C API.
I plan to have my Suricata-plugin crate implement another JsonBuilder struct that calls the C API under the hood even if it feels like the normal rust API from the plugin logger...

@catenacyber
Copy link
Contributor Author

Next in #12448

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.

3 participants