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

snort3: add missing config include and general cleanup #22830

Merged
merged 1 commit into from
Dec 16, 2023

Conversation

efahl
Copy link
Contributor

@efahl efahl commented Dec 6, 2023

  • Add snort config 'include' to allow user customizations in the lua
  • Enhance 'check' to test generated nftables file
  • Suppress warnings on configuration check unless '-v'erbose
  • Replace text logging with json logging to reduce footprint and make reports easier
  • Fix up some error messages suggesting solutions

Maintainer: @flyn-org
Compile tested:
Run tested: x86/64

Description:

@efahl
Copy link
Contributor Author

efahl commented Dec 6, 2023

What are the general guidelines that suggest a bump for PKG_RELEASE? I've read through the dev guidelines a couple times, and I can't find a mention of it (should this be in the wiki, if so let me know and I'll add it).

@efahl efahl force-pushed the snort3-fixes branch 2 times, most recently from 37a958d to c29f544 Compare December 8, 2023 17:33
@graysky2
Copy link
Contributor

graysky2 commented Dec 9, 2023

@efahl - I have two suggestions:

  1. Consider removing local.lua and homenet.lua since neither is used by the new configuration.
  2. Is there is an easy way you can avoid the lengthy validation process if nothing in the config files changed? Or maybe have it do a more minimal check. The majority of the time is spent going through the rules file. On my system this adds ~30 seconds to the startup process each time.

@efahl
Copy link
Contributor Author

efahl commented Dec 9, 2023

@graysky2 , re no.1 yeah, I considered removing both of those files, but didn't want to break the manual setup. But now that you mention it, if someone already has a manual setup, then those files will be retained by sysupgrade -b, so it shouldn't matter. I will delete them and add some comments in the snort.lua about homenet being superseded.

@graysky2
Copy link
Contributor

graysky2 commented Dec 9, 2023

That's perfect. Again, if the default package is using your new code, it should not be providing legacy files IMO.

@efahl
Copy link
Contributor Author

efahl commented Dec 9, 2023

For no.2, YOW 30 seconds! I just ran time snort-mgr check and it's 0.27s real time for me (community ruleset only), so I'm glad you tested this. I dug through the docs and don't see anything on the snort side itself that could help, but I think maybe just generating the test config without the rules might help.

Could you do a quick test? Edit /usr/share/snort/templates/snort.uc comment out line 57 where the rules are included:

--include         = '{{ snort.config_dir }}/' .. RULE_PATH .. '/snort.rules',

Then run

time snort-mgr check

Immediately undo the edit to snort.uc, otherwise you'll have no rules next time you restart...

@graysky2
Copy link
Contributor

graysky2 commented Dec 9, 2023

Yes, when I swapped out my primary snort.rules (40416 lines/21M) for the test rule that just blocks ping (1 line) it was nearly instantaneous.

@graysky2
Copy link
Contributor

graysky2 commented Dec 9, 2023

I too didn't see anything in the snort CLI that would work here... perhaps your strategy to not pass along the rules is good one. That way it is just the config that is checked for sanity.

@efahl
Copy link
Contributor Author

efahl commented Dec 9, 2023

I'm thinking that the '-v' would do all the rules, and without it, just check a rule-less config. I'll give that a try and see how it works out. I expect most of the rules people use are downloaded and have already been checked, so no real loss.

@graysky2
Copy link
Contributor

graysky2 commented Dec 9, 2023

No, the -v just prints the output... the check still takes ~30 s with or without it.

@efahl
Copy link
Contributor Author

efahl commented Dec 9, 2023

Right, I'm modifying the check function so

  1. snort-mgr check generates a config without rules, so runs fast (this is what gets used internally when you restart).
  2. snort-mgr -v check includes the rules, so it spews all sorts of stuff and runs slow. This would only be used from the command line, when user needs extra help debugging a config.

Does that seem reasonable?

@graysky2
Copy link
Contributor

graysky2 commented Dec 9, 2023

Perfect! Ping the PR when you force push so I can test.

@efahl
Copy link
Contributor Author

efahl commented Dec 9, 2023

Ping! Should all be there.

@efahl
Copy link
Contributor Author

efahl commented Dec 9, 2023

Oops, had to merge with the version update from a couple days ago.

@graysky2
Copy link
Contributor

graysky2 commented Dec 9, 2023

Works as expected.

  1. Snort daemon starts much faster
  2. snort-mgr check is fast without checking rules
  3. snort-mgr -v check is slower as it checks rules

@flyn-org
Copy link
Contributor

Looks good. Recommend merge.

@graysky2, are you going to submit a pull request to co-maintain snort3?

@graysky2
Copy link
Contributor

graysky2 commented Dec 10, 2023

I haven't considered it, but if you are open to that idea, I can @flyn-org

EDIT: #22855

I would really like to get both #21627 and #21471 merged and then update snort3 to use them. Do you know what can be done to accelerate?

graysky2 added a commit to graysky2/packages that referenced this pull request Dec 10, 2023
Michael invited me to co-maintain[1].

1. openwrt#22830 (comment)

Signed-off-by: John Audia <therealgraysky@proton.me>
1715173329 pushed a commit that referenced this pull request Dec 13, 2023
Michael invited me to co-maintain[1].

1. #22830 (comment)

Signed-off-by: John Audia <therealgraysky@proton.me>
@graysky2
Copy link
Contributor

Looks good. Recommend merge as well.

net/snort3/Makefile Outdated Show resolved Hide resolved
- Delete legacy configuration files homenet.lua and local.lua
- Add snort config 'include' to allow user customizations in the lua
- Enhance 'check' to test generated nftables file
- Suppress inclusion of rules file when doing silent config check
- Suppress warnings on configuration check unless '-v'erbose
- Replace text logging with json logging to reduce footprint and make reports easier
- Fix some typos in the snort.uc template
- Fix up some error messages suggesting solutions

Signed-off-by: Eric Fahlgren <ericfahlgren@gmail.com>
@1715173329 1715173329 merged commit 0d2dac8 into openwrt:master Dec 16, 2023
12 checks passed
@efahl efahl deleted the snort3-fixes branch December 16, 2023 21:41
tiagogaspar8 pushed a commit to tiagogaspar8/packages that referenced this pull request Dec 20, 2023
Michael invited me to co-maintain[1].

1. openwrt#22830 (comment)

Signed-off-by: John Audia <therealgraysky@proton.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants