-
-
Notifications
You must be signed in to change notification settings - Fork 402
Plugins: Config file is not loaded before rules #2246
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
Comments
OK. I've now tested this. Seriously my bad. I thought everything was in order until I realized it's only in order as long as config-before is phase 1 and before is in phase 2. With the naming of the dummy plugin, this is not working for obvious reasons:
I do not really think that 3 separate plugin Includes are particularly elegant. But it would be very clear what is loaded and in which order. Plugin file names thus as @azurit proposes:
Loaded via:
I'm leaving away the IncludeOptional since NGINX is not supporting it. This will force us to do 3 empty plugin files in the plugin folder of the release. |
Maybe off-topic: I would suggest creating another file with default configuration (something like configuration initialization). This file will contain all configuration options (with their default values), even the 'missing' one from configuration file. With current plugin files format, it will be very hard doing automatic updates, because:
With some 'initialization' file, config file even don't have to exists to everything work ok (and it should be shipped with |
You're right about the configuration. And we need to think this through. As automatic updates would be cool, but some updates will demand users read the release notes and adopt the configuration. Maybe something we can cover in the documentation. Like a best practice when working with plugins. |
OK, so we'll continue the conversation here. @airween and @franbuehler volunteered to join the discussion with @53cur3M3 and @RedXanadu standing on the sidelines. |
A plugin consists of 3 files like here: https://github.com/coreruleset/dummy-plugin/tree/main/plugins We've discussed this several times, so Walter and I finally discussed it separately and we agreed that plugins would all be thrown into the same folder (alternatively symlinked) and then loaded separately via Includes. Loading in the correct order is of course a problem. So 3 separate Includes as discussed tonight. Now @azurit is right to point out that the update path is rocky if an update overwrites the existing plugin config. That's the reason for the crs-setup.conf.example. So we need an elegant solution for this problem too. What do you think? |
My current solution (as i'm already having problems with mass updates) for plugins i'm using is creating some kind of 'initialization' file which is setting all config variables with its default values. This file, which looks the same as config file, is loaded before the config file, so everything can be overridden using a config file. I understand that not every update can be done automatically but, on the other hand, we should't ask from our users to manualy fix plugins after every update. Updates must be smooth or users will stop doing them. |
The initialization should be handled like CRS initialization. One can simply integrate the functionality of the CRS 901 into the rules-before file. For the record, you can test for the existence of all config variables via a single rule:
The 2nd problem is per-domain activation or deactivation. This can be achieved with specially constructed ctl:ruleRemoveById statements - a setup that deserves separate documentation. If we want to offer per-domain activation or deactivation, SecRuleUpdateTargetById won't be usable in rule exclusion plugins (and that was seen as an advantage of rule exclusion plugins). @azurit: Are all your concerns addresses this way. |
Next step is to adopt this into the dummy plugin. That way it is documented. I think I will also extend my upcoming plugins blog post with this information. This is aiming for a release this week (it's done but was postponed due to log4j). |
Why? |
@dune73 This was part of the discussion in the monthly meeting yesterday, right? |
Yes, exactly. Config-time directives like SecRuleUpdateTargetById and SecRemoveTargetById are always context-wide. So VH wide exclusions force you to load the rule set in that context, which is very bad for large number of VHs on a host. On such a machine you want serverwide rules. |
Fixed, closing. |
Uh oh!
There was an error while loading. Please reload this page.
Describe the bug
Config file must be always read/processed as first file as it is setting variables used by plugin rules. Our current plugin structure prevents this behavior (files are processed in alphabetical order):
Current plugins are working only thanks to the fact that config rule is run in phase 1 and all other rules in phase 2.
Solutions
Add another include command and rename config file
Plugin files:
Includes:
Prefix plugin files with numbers
Plugin files:
This solution sounds too complex but it has one benefit: It allows plugin creator to include also other files and decide when they should be loaded, even before config file (use-case: second config file with default values loaded before real config file [will make plugin updates easier]).
EDIT: This solution will also allows doing dependencies for plugins (i.e. one plugin depends on another, which must be loaded first).
The text was updated successfully, but these errors were encountered: