8000 Plugins: Config file is not loaded before rules · Issue #2246 · coreruleset/coreruleset · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
8000

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

Closed
azurit opened this issue Oct 22, 2021 · 13 comments
Closed

Plugins: Config file is not loaded before rules #2246

azurit opened this issue Oct 22, 2021 · 13 comments

Comments

@azurit
Copy link
Member
azurit commented Oct 22, 2021

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):

<plugin_name>-before.conf
<plugin_name>-config-before.conf

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:

<plugin_name>-after.conf
<plugin_name>-before.conf
<plugin_name>-config.conf

Includes:

Include modsecurity.d/owasp-modsecurity-crs/crs-setup.conf
IncludeOptional modsecurity.d/owasp-modsecurity-crs/plugins/*-config.conf
IncludeOptional modsecurity.d/owasp-modsecurity-crs/plugins/*-before.conf
Include modsecurity.d/owasp-modsecurity-crs/rules/*.conf
IncludeOptional modsecurity.d/owasp-modsecurity-crs/plugins/*-after.conf

Prefix plugin files with numbers

Plugin files:

10-<plugin_name>-config-before.conf
20-<plugin_name>-before.conf
30-<plugin_name>-after.conf

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).

@dune73
Copy link
Member
dune73 commented Nov 15, 2021

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:

dummy-after.conf
dummy-before.conf
dummy-config-before.conf

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:

<plugin_name>-after.conf
<plugin_name>-before.conf
<plugin_name>-config.conf

Loaded via:

Include modsecurity.d/owasp-modsecurity-crs/crs-setup.conf

Include modsecurity.d/owasp-modsecurity-crs/plugins/*-config.conf
Include modsecurity.d/owasp-modsecurity-crs/plugins/*-before.conf

Include modsecurity.d/owasp-modsecurity-crs/rules/*.conf

Include modsecurity.d/owasp-modsecurity-crs/plugins/*-after.conf

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.

@azurit
Copy link
Member Author
azurit commented Nov 15, 2021

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:

  • we need to distribute also configuration file (as this is the place where plugin variables are initialized)
  • update process may override local configuration file
  • if new version of plugin contains new configuration directives, these must be added by hand by user or the plugin may stop working without any error beeing logged

With some 'initialization' file, config file even don't have to exists to everything work ok (and it should be shipped with .example extension).

@dune73
Copy link
Member
dune73 commented Nov 15, 2021

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.

@dune73
Copy link
Member
dune73 commented Nov 15, 2021

OK, so we'll continue the conversation here. @airween and @franbuehler volunteered to join the discussion with @53cur3M3 and @RedXanadu standing on the sidelines.

@dune73
Copy link
Member
dune73 commented Nov 15, 2021

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?

@azurit
Copy link
Member Author
azurit commented Nov 16, 2021

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.

@dune73
Copy link
Member
dune73 commented Dec 20, 2021

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:

SecAction "id:1004,phase:2,pass,nolog,setvar:tx.var1=foo"
SecAction "id:1005,phase:2,pass,nolog,setvar:tx.var2=bar"
SecRule &TX:var1|&TX:var2|&TX:var3 "!@eq 1"     "id:1006,phase:2,pass,log,msg:'Variable uninitialized: %{MATCHED_VAR_NAME}. Deactivating plugin.'"

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.

@fzipi
Copy link
Member
fzipi commented Jan 2, 2022

@dune73 @azurit There is no clear owner, or next steps here. Can someone summarize what should be done now, next steps?

@dune73
Copy link
Member
dune73 commented Jan 3, 2022

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).

@azurit
Copy link
Member Author
azurit commented Jan 11, 2022

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).

Why?

@fzipi
Copy link
Member
fzipi commented Apr 5, 2022

@dune73 This was part of the discussion in the monthly meeting yesterday, right?

@dune73
Copy link
Member
dune73 commented Apr 5, 2022

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.

@azurit
Copy link
Member Author
azurit commented May 6, 2022

Fixed, closing.

@azurit azurit closed this as completed May 6, 2022
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

No branches or pull requests

3 participants
0