search

owasp-modsecurity / ModSecurity-nginx

ModSecurity v3 Nginx Connector
Apache License 2.0
1.59k stars 282 forks source link

Module configuration refactoring #139

Closed defanator closed 5 years ago

defanator commented 6 years ago

The proposed changes are mostly aimed to improve overall code readability.

As a bonus, 2ae2a10 introduces a little feature of extending connector banner message with a total number of loaded rules with breakdown by rule source, e.g.:

2018/11/30 08:20:22 [notice] 9116#9116: ModSecurity-nginx v1.0.0 (rules loaded inline/local/remote: 0/1634/0)
zimmerle commented 6 years ago

Hi @defanator,

The only thing that is crucial in terms of Rules merge, is the fact that somehow they have to respect the hierarchy in order to reduce the memory footprint. Two different virtual hosts can make usage of a rules set loaded in server configuration, instead of load two different copies of the rules into memory.

In the effort SpiderLabs/ModSecurity#1970 we are trying to allow the rules to be reloaded is run time, also, we are going to reduce the memory usage by replacing some textual tags with binary properties (good for memory and cpu) and shared_pointers instead of string repetition, pretty simple change that will produce a great result. But, yet, better to load the rules into memory only once, than have it loaded multiple times.

With that said, i think your patch is fine :) Indeed, it does improved the readability. The amount of loaded rules is amazing.

defanator commented 6 years ago

@zimmerle rules merging hasn't changed with the proposed PR, I've just removed duplicate parts of code (i.e. merge_srv_conf and merge_loc_conf were actually doing the same stuff).

In regards to the dynamic modsecurity configuration management, I definitely like the idea. Currently one has to reload nginx by sending SIGHUP to master process after modsecurity configuration has changed; despite such approach can work without breaking live traffic (thanks to nginx' ability to gracefully pass incoming load to new set of workers), avoiding workers restart is a way better thing. It may bring a number of challenges though.

defanator commented 6 years ago

@zimmerle in regards to memory footprint - yes, I'm pretty sure (lib)modsecurity (and/or connector modules) should be smart enough to detect whether the exactly same set of rules is being loaded for multiple entities (servers / locations / virtual hosts / you name it), and avoid any sort of duplicates.

I can imagine how to achieve this with loading rules from file (e.g. you can store and check SHA sums / modification times of a conf and its includes). Using remote sources may require additional checks like passing "If-Modified" and checking response codes, etc. Anyway, there is a room for improvements.

zimmerle commented 5 years ago

Merged! ;) thanks!