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

Proposal: integrate nginx ingress controller with Modsecurity #116

Closed
mqliang opened this issue Jan 9, 2017 · 17 comments · Fixed by #1500
Closed

Proposal: integrate nginx ingress controller with Modsecurity #116

mqliang opened this issue Jan 9, 2017 · 17 comments · Fixed by #1500
Assignees
Labels

Comments

@mqliang
Copy link

mqliang commented Jan 9, 2017

It would be wonderful if nginx ingress controller could integrate Modsecurity to provide web application firewall (WAF) functionality, and the repository owasp-modsecurity-crs contains a collection of rules that works out of the box.

We could add two knobs to nginx-ingress controller:

  • --enable-modsecurity, bool, “if true, enable modsecurity, currently only support OWASP ModSecurity Core Rule Set”
  • --modsecurity-rule-set, []string, “set of OWASP modsecurity rules to include”

@aledbf @bprashanth

@aledbf
Copy link
Member

aledbf commented Jan 9, 2017

@mqliang I've been following the new modsecuritty for a while. Once they release v3.0 I will add the option.

@rikatz
Copy link
Contributor

rikatz commented Jan 9, 2017

@mqliang I was thinking about NAXSI (https://github.com/nbs-system/naxsi) as this is much simpler than mod_security.

I think the bigger issue with mod_security would be the false positives, as mod_security even doesn't have a pretty way to visualize threats and so.

Anyway, this is a great idea. Would add also a --modsecurity-rule-exclude to specify an array of excluded rules (that cause false positives and so), and an operation mode, so as SELinux we can put mod_security on enforcing, reporting only and disabled :)

@pieterlange
Copy link
Contributor

pieterlange commented Jan 9, 2017

mod_security added json logging a while ago, so visualizing these logs is much easier using some SIEM or ELK stack nowadays.

Edited to add: If we add this, we should take care not to introduce a false sense of safety as most of these WAF products don't play well with HTTP2 yet. (notably naxsi, but i'd bet the same goes for mod_security)

@rikatz
Copy link
Contributor

rikatz commented Jan 9, 2017

Hum, this is new for me :) will take a look then :)

@aledbf
Copy link
Member

aledbf commented Jan 9, 2017

@rikatz naxsi does not supports http2.

Edit: I missed @pieterlange comment about it.

@tomazzaman
Copy link

3.0 has been out since November? Or am I missing something?

@mqliang
Copy link
Author

mqliang commented Mar 20, 2017

cc @kdada

@maikotz
Copy link

maikotz commented Apr 25, 2017

+1 following

@rikatz
Copy link
Contributor

rikatz commented Apr 25, 2017

@tomazzaman I think this is the Core Rule Set, not mod_security 3 itself.

The code of this version is here: https://github.com/SpiderLabs/ModSecurity/tree/v3/master

But it wasn't released yet.

@ghost
Copy link

ghost commented Aug 29, 2017

@mschmidt291
Copy link

Any update about this? Would be really nice if ModSecurity would finally get implemented.

@aledbf
Copy link
Member

aledbf commented Sep 19, 2017

This is a no go for now. Just adding the library and module to nginx increases the size in 220MB

gcr.io/google_containers/nginx-slim           test                617a104421a2        20 minutes ago      341MB

and also impacts nginx throughput https://github.com/defanator/modsecurity-performance/wiki#2017-08-28

@rikatz
Copy link
Contributor

rikatz commented Sep 28, 2017

@aledbf For this feature, I'm thinking about this:

  1. Create an 'init' container with the resulting libraries from the dynamic compilation of mod_security for nginx (following this in alpine), but creating the module with the compilation of NGINX with this (--add-dynamic-module=/opt/nginx/ModSecurity-nginx/).

This will create libs only, and the libraries necessary for mod_security to work are ngx_http_modsecurity_module.so (215kb) and libmodsecurity.so.3.0.0 (30M)

So, we're going to have an 'init' container, with a shared volume containing only those libraries. This could allow us to make a different deployment file, being one with mod_security (a POD with an initContainer and the ingress-controller itself) and one without it (nginx-ingress-controller only)

  1. Create a new argument while starting ingress-controller, to enable mod_security. When this is done, we create a flag in $all like modsecurity_enabled, that could be used in a conditional in nginx.tmpl to load mod_security module with the following directive:

loadmodule $SHARED_INIT_VOLUME/ngx_http_modsecurity_module.so

Also, this boolean could be used latelly in the feature implementation itself, like enabling mod_security per ingress/location and also in directives that disables ModSecurity rules.

  1. Points that needs to be reviewed:
  • the initContainer should be recompiled always following the nginx version used in nginx-slim
  • How to load the CRS rules and keep them up to date. Those rules have ~600kb, but I don't know how much they change.

Is that a good approach? I was doing some experiments here (using CentOS, not alpine) and this works fine. If this is ok I'll start a PoC with alpine and a different deployment, not changing anything in Ingress Controller code, but only in deployment itself. And then we can start thinking about nice features that we could have in ingress controller.

Thanks

@ghost
Copy link

ghost commented Oct 24, 2017

This should be re-opened pending owasp-modsecurity/ModSecurity#1590

@bramvdklinkenberg
Copy link

Any update on this? Would be nice to see waf functionality in the nginx controller.

@aledbf
Copy link
Member

aledbf commented Dec 15, 2017

@bramvdklinkenberg
Copy link

Hi @aledbf, I saw.. I realized I need to ask the question at the kubernetes helm project :).
I deploy the controller via helm on my kubernetes clusters but the waf functionality is not available by default yet in the nginx helm chart.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants