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

Extend Suricata support #8372

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

Conversation

0xThiebaut
Copy link

@0xThiebaut 0xThiebaut commented Feb 22, 2025

Extend Suricata support in OPNsense to increase the monitoring capabilities (e.g., integrate with Malcolm)

Features:

  • Extend support for EVE log, supporting all available protocols (RDP, DNS, ...).
  • Add support for PCAP logs.

Subsequent changes:

  • Update configuration template to align with Suricata 7.0.8 (e.g., add new fields such as ja4).
  • Disable stats.log (stats can be enabled in EVE).
  • Add migration logic.

@0xThiebaut 0xThiebaut force-pushed the suricata branch 2 times, most recently from fff9706 to aa83b54 Compare February 27, 2025 14:40
@0xThiebaut
Copy link
Author

Thanks for the reviews so far!

I get that every help or label string I change bounces the translations of other languages, which we typically want to avoid. However I believe the benefit of properly (re)naming the options and help messages will benefit the users on the long run as the current strings are not consistent and occasionally obscure or incorrectly translated (e.g., Output TLS transaction where the session [...] is translated to French as Exit a TLS transaction where a session [...]).

I'd love to refactor the IDS UI strings to have them be consistent and helpful for the users (instead of requiring them to find the equivalent features in the Suricata documentation). I'd obviously be happy to perform the French translations on any bumped string.

@0xThiebaut 0xThiebaut requested a review from Monviech February 27, 2025 15:09
@Monviech
Copy link
Member

If you limit this PR only to the new functionality you want to add it will be simpler to review.

If you want to change existing language strings it could be in a separate PR after this one.

@0xThiebaut
Copy link
Author

Thanks for the feedback; I rolled-back any rewording not related to the new features.

use OPNsense\Base\BaseModelMigration;
use OPNSense\IDS\IDS;

class M1_1_1 extends BaseModelMigration
Copy link
Member

Choose a reason for hiding this comment

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

Just asking but isn't there a way to add the new features that you need inline without needing a migration or altering existing options? That would make reviewing far easier and we could still move settings around later.

Migrations are always a risk due to possible impacts on existing installations.

Copy link
Author

Choose a reason for hiding this comment

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

We definitely could add it without a migration; I focused on having a reasonable code-base with pleasant UI but if avoiding a migration is a priority I can add the new features without requiring a migration.

Could you confirm you'd prefer this? This would involve having a drop-down for new protocol types (e.g.,DNS) while retaining check boxes for already-supported log types (i.e., HTTP & TLS).

This wouldn't be consistent but would avoid the need for a migration.

Copy link
Member

Choose a reason for hiding this comment

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

I prefer not having a migration here.

@fichtner whats your opinion?

Copy link
Member

@fichtner fichtner Mar 14, 2025

Choose a reason for hiding this comment

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

We don't have this policy in place (yet) but it would be a very good idea to either only introduce new changes or only move existing data, but not both in the same PR. This is beneficial in reducing risk of regression and considering backporting. On top of that suricata.yaml has broken a few times already so you have that risk as well on top.

Copy link
Author

Choose a reason for hiding this comment

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

Alright, with that guideline I'll make the necessary modifications to have this PR introduce the required functionality without it involving a migration.

I'll leave the refactoring for better UX for a later time.

@@ -174,22 +174,6 @@
</Model>
<ValidationMessage>Related cron not found.</ValidationMessage>
</UpdateCron>
<AlertLogrotate type="OptionField">
Copy link
Member

Choose a reason for hiding this comment

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

Is it really necessary to alter the model like that? Can't we go for a minimal impact approach first?

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