From fea7fed6dad9e468f46d24a05f13aa4df0e85350 Mon Sep 17 00:00:00 2001 From: Moh Basher <36448614+besha100@users.noreply.github.com> Date: Thu, 23 Dec 2021 12:34:38 +0100 Subject: [PATCH] Disable default modsecurity_rules_file if modsecurity-snippet is specified (#8021) * Disabled default modsecurity_rules_file if modsecurity-snippet is specifed The default modsecurity_rules_file overwrites the ModSecurity-snippet if it is specified with custom config settings like "SecRuleEngine On". This will not let Modsecurity be in blocking mode even if "SecRuleEngine On" is specified in the ModSecurity-snippet configuration * Remove unnecessary comments Only have the default Modsecurity conf settings in case Modsecurity configuration snippet is not present and remove unnecessary comments * Fixed modsecurity default file only if Modsecurity snippet present Fixed if condition Modsecurity snippet present have modsecurity default config file * Added e2e test to disabling modsecurity conf Added e2e in case modsecurity-snippet enabled to disable settings in default modsecurity.conf * Validate writing to a different location Validate also modsecurity to write to a different location instead of the default directory * Fixed the formatting * Fixed if empty ModsecuritySnippet * Fixed ModsecuritySnippet condition * Fixed the condition also in ingress controller template * Removed the default config condition in ingress controller template * Fixed the default config condition in ingress controller template * Fixed pull-ingress-nginx-test * Revert "Fixed the default config condition in ingress controller template" This reverts commit 9d38eca40fe615a4c756500ca57b05634240edde. * Revert template_test * Adjusted the formating %v --- .../ingress/controller/template/template.go | 2 +- .../controller/template/template_test.go | 4 +- rootfs/etc/nginx/template/nginx.tmpl | 6 +-- .../annotations/modsecurity/modsecurity.go | 37 +++++++++++++++++++ 4 files changed, 43 insertions(+), 6 deletions(-) diff --git a/internal/ingress/controller/template/template.go b/internal/ingress/controller/template/template.go index ae5ec259a..e5be1490a 100644 --- a/internal/ingress/controller/template/template.go +++ b/internal/ingress/controller/template/template.go @@ -1537,7 +1537,7 @@ func buildModSecurityForLocation(cfg config.Configuration, location *ingress.Loc `, location.ModSecurity.TransactionID)) } - if !isMSEnabled { + if !isMSEnabled && location.ModSecurity.Snippet == "" { buffer.WriteString(`modsecurity_rules_file /etc/nginx/modsecurity/modsecurity.conf; `) } diff --git a/internal/ingress/controller/template/template_test.go b/internal/ingress/controller/template/template_test.go index cfb65c08e..b65e33c32 100644 --- a/internal/ingress/controller/template/template_test.go +++ b/internal/ingress/controller/template/template_test.go @@ -1781,8 +1781,8 @@ func TestModSecurityForLocation(t *testing.T) { {"configmap enabled, configmap OWASP enabled, annotation enabled, OWASP disabled", true, true, true, true, false, "", "", ""}, {"configmap disabled, annotation enabled, OWASP disabled", false, false, true, true, false, "", "", fmt.Sprintf("%v%v", loadModule, modSecCfg)}, {"configmap disabled, annotation disabled, OWASP disabled", false, false, false, true, false, "", "", ""}, - {"configmap disabled, annotation enabled, OWASP disabled", false, false, true, true, false, testRule, "", fmt.Sprintf("%v%v%v", loadModule, modsecRule, modSecCfg)}, - {"configmap disabled, annotation enabled, OWASP enabled", false, false, true, true, false, testRule, "", fmt.Sprintf("%v%v%v", loadModule, modsecRule, modSecCfg)}, + {"configmap disabled, annotation enabled, OWASP disabled", false, false, true, true, false, testRule, "", fmt.Sprintf("%v%v", loadModule, modsecRule)}, + {"configmap disabled, annotation enabled, OWASP enabled", false, false, true, true, false, testRule, "", fmt.Sprintf("%v%v", loadModule, modsecRule)}, } for _, testCase := range testCases { diff --git a/rootfs/etc/nginx/template/nginx.tmpl b/rootfs/etc/nginx/template/nginx.tmpl index c6e978ffe..0cc8d3cab 100755 --- a/rootfs/etc/nginx/template/nginx.tmpl +++ b/rootfs/etc/nginx/template/nginx.tmpl @@ -153,10 +153,10 @@ http { modsecurity_rules ' {{ $all.Cfg.ModsecuritySnippet }} '; - {{ end }} - + {{ else }} modsecurity_rules_file /etc/nginx/modsecurity/modsecurity.conf; - + {{ end }} + {{ if $all.Cfg.EnableOWASPCoreRules }} modsecurity_rules_file /etc/nginx/owasp-modsecurity-crs/nginx-modsecurity.conf; {{ end }} diff --git a/test/e2e/annotations/modsecurity/modsecurity.go b/test/e2e/annotations/modsecurity/modsecurity.go index f88d6541e..4de85818d 100644 --- a/test/e2e/annotations/modsecurity/modsecurity.go +++ b/test/e2e/annotations/modsecurity/modsecurity.go @@ -342,4 +342,41 @@ var _ = framework.DescribeAnnotation("modsecurity owasp", func() { Expect(). Status(http.StatusOK) }) + + ginkgo.It("should disable default modsecurity conf setting when modsecurity-snippet is specified", func() { + host := "modsecurity.foo.com" + nameSpace := f.Namespace + + snippet := `SecRuleEngine On + SecRequestBodyAccess On + SecAuditEngine RelevantOnly + SecAuditLogParts ABIJDEFHZ + SecAuditLogType Concurrent + SecAuditLog /var/tmp/modsec_audit.log + SecAuditLogStorageDir /var/tmp/ + SecRule REQUEST_HEADERS:User-Agent \"block-ua\" \"log,deny,id:107,status:403,msg:\'UA blocked\'\"` + + annotations := map[string]string{ + "nginx.ingress.kubernetes.io/enable-modsecurity": "true", + "nginx.ingress.kubernetes.io/modsecurity-snippet": snippet, + } + f.UpdateNginxConfigMapData("annotation-value-word-blocklist", "load_module, lua_package, _by_lua, location, root, {, }") + // Sleep a while just to guarantee that the configmap is applied + framework.Sleep() + ing := framework.NewSingleIngress(host, "/", host, nameSpace, framework.EchoService, 80, annotations) + f.EnsureIngress(ing) + + f.WaitForNginxServer(host, + func(server string) bool { + return !strings.Contains(server, "modsecurity_rules_file /etc/nginx/modsecurity/modsecurity.conf;") && + strings.Contains(server, "SecAuditLog /var/tmp/modsec_audit.log") + }) + + f.HTTPTestClient(). + GET("/"). + WithHeader("Host", host). + WithHeader("User-Agent", "block-ua"). + Expect(). + Status(http.StatusForbidden) + }) })