Hook into Nginx NGX_HTTP_ACCESS_PHASE

[tomsommer]

Hook into Nginx NGX_HTTP_ACCESS_PHASE instead of NGX_HTTP_PREACCESS_PHASE and NGX_HTTP_REWRITE_PHASE.

Having ModSecurity run before NGX_HTTP_ACCESS_PHASE cripples Nginx during DDOS attacks, because ModSecurity runs before Nginx limit_* functions and consumes too much CPU/memory even in phase:1.

Fixes #293

[airween]

Hi @tomsommer!

Thanks for this PR. Could you add some test(s) (see others here) to check that the behavior is that the modification expects? I mean set up the engine with some very low rate limit and try to send more parallel requests than that. Or you can add a new job in our workflow.

Thanks again!

[tomsommer]

What do you want to test? It sounds like you want to test if Nginx limit_* works?
The current tests should be able to test if this PR breaks anything.

[airween]

What do you want to test? It sounds like you want to test if Nginx limit_* works?

No, I want to test if this patch solves the mentioned issue and that you wrote (in a removed comment): "nginx cannot survive a DDOS if modsec is before limit*". It would be nice to see that if the user sets a limit and requests exceed then Nginx drops the connections and the requests do not reach ModSecurity.

The current tests should be able to test if this PR breaks anything.

Yes, that's a necessary but not sufficient condition.

[tomsommer]

I did a lot of testing and I had to merge the rewrite stuff into the access handler, instead of registering two handlers in the same phase (I don't know if it's the order, or whatever), but it works now. It runs after limit_*

[sonarqubecloud]

Quality Gate passed

Issues
9 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[airween]

Hi @tomsommer,

first of all, many thanks for your fix, and apologize for the late response.

I think this is a very important solution.

I did a lot of testing and I had to merge the rewrite stuff into the access handler, instead of registering two handlers in the same phase (I don't know if it's the order, or whatever), but it works now. It runs after limit_*

I saw the tests - thanks for them - which justify the code works as well. Thank you for them too.

I'm still a little worried (I'm not an Nginx developer), because:

• I can't see (and probably nobody) what other side effects will be there (eg the rewrite hooks; I've made some researches and found that rewrite happens also in pre-access; I don't know how important is that from this aspect)
• I'm sure the original authors (including @defanator, who was an Nginx developer) knew what they were doing, didn't put the connector in pre-access by accident, but on purpose.

If we are lucky now, @defanator will see this comment and may be he will explain the reason.

If there won't be any response or other discussion, I'll merge this into the master, and hopefully many people will test that, which will provide some feedback (as it happened in case of #334 which was reverted in #344).