Module configuration refactoring #139
Conversation
It is being used for creating ModSecurity instance via msc_init() call, as well as setting up necessary cleanups for the instance. While here: - refactored cleanup handler for common context to highlight that its primary goal is to release memory consumed by rules; - made all sanity checks related code to be included only when MODSECURITY_SANITY_CHECKS is true.
Also, there is no need to set corresponding pointers to NULL as cleanup handler should be called only once (per context).
No functional changes.
|
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 owasp-modsecurity/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. |
|
@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. |
|
@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. |
There was a problem hiding this comment.
Oh man, I really like that the code is simplified 👍 Also loved the number of loaded rules in log message at start. Will certainly be helpful in a number of cases, including SecRemoteRules 👍
Looks good to me :)
@zimmerle what do you think about merging this and tag a release as 1.1 or 1.0.1? :)
|
Merged! ;) thanks! |
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.: