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

Added includes needed for plugins #66

Merged
merged 4 commits into from
Apr 6, 2022

Conversation

theseion
Copy link
Contributor

No description provided.

@theseion theseion requested a review from fzipi March 22, 2022 19:31
@fzipi
Copy link
Member

fzipi commented Mar 22, 2022

@theseion Does this work locally?

@theseion
Copy link
Contributor Author

Not yet. Just discovered that I need to create that directory first.

fzipi
fzipi previously approved these changes Mar 22, 2022
Copy link
Member

@fzipi fzipi left a comment

Choose a reason for hiding this comment

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

Looks good. We need to think about how plugins will get inside the container. Mounting perhaps?

@theseion
Copy link
Contributor Author

At least for the tests: yes. That's what coreruleset/coreruleset#2448 is for.

includes for plugins are now also disabled if no appropriate rules could be found

this is necessary because plugins are likely to be added and removed, depending on the test
Copy link
Member

@fzipi fzipi left a comment

Choose a reason for hiding this comment

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

After taking a look at https://coreruleset.org/docs/configuring/plugins/#how-to-install-a-plugin, do we want to create

crs/plugins/empty-config.conf
crs/plugins/empty-before.conf
crs/plugins/empty-after.conf

?

@theseion
Copy link
Contributor Author

I had the same idea but it doesn't work (at least not by itself). As soon as you mount a plugins directory you have the same issue. Hence, we'd have to add dummy files to the CRS repo and I didn't want to do that. That would only make sense, IMO, if we supplied a prepared configuration file with the Include ... lines in it. But since we don't, I'm not sure we should.

@theseion
Copy link
Contributor Author

I need this for coreruleset/coreruleset#2394. So it would be good if we could come to a conclusion.

Copy link
Member

@fzipi fzipi left a comment

Choose a reason for hiding this comment

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

I think this will fail if:

  • there is an /opt/owasp-crs/this-nice-plugins/my-plugin-config.conf file
  • no corresponding symlink in the /opt/owasp-crs/plugins directory

If there is a plugin subdirectory infrastructure, e.g: /opt/owasp-crs/XYZ-plugin/xyz-config,after,before,config}.conf, do we recommend symlinking from the /opt/owasp-crs/plugins/xyz-*.conf files?

The crs blog post says also:

Future CRS releases will come with a plugins folder next to the rules folder. If you do not have that yet then create it and place three empty config files in it (Shell command touch is your friend):

crs/plugins/empty-config.conf
crs/plugins/empty-before.conf
crs/plugins/empty-after.conf

These empty rule files make sure that the web server does not fail when including *.conf if there are no plugins present. (We're aware that Apache supports the IncludeOptional directive, but that is not available on all web servers, so we prefer to use Include for documentation purposes.)

Also, plugins directory might not exist, and be volume mounted read-only in docker. So writing by "activating" plugins might fail. 🤔

The expressions used with sed require extended regex support
@theseion
Copy link
Contributor Author

theseion commented Apr 1, 2022

Well spotted! I fixed the find operation to look a the full path. I also saw that there was an issue with the second sed command (requires -E).

The code otherwise accounts for the possibility of the plugins directory being absent (the inlcudes are commented by default and find will not fail).

@theseion theseion requested a review from fzipi April 6, 2022 13:20
Copy link
Member

@fzipi fzipi left a comment

Choose a reason for hiding this comment

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

Good, let's see how this works.

@theseion
Copy link
Contributor Author

theseion commented Apr 6, 2022

I don't have write access to the repo, so you'll have to merge.

@theseion theseion merged commit 5eae48e into coreruleset:master Apr 6, 2022
@theseion theseion deleted the add-plugin-support branch April 6, 2022 14:00
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

2 participants