From f9b6e70d01a2c4edb779511878ab189b3717cf45 Mon Sep 17 00:00:00 2001 From: besha100 <36448614+besha100@users.noreply.github.com> Date: Wed, 8 Dec 2021 13:45:58 +0100 Subject: [PATCH 01/15] 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 --- rootfs/etc/nginx/template/nginx.tmpl | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/rootfs/etc/nginx/template/nginx.tmpl b/rootfs/etc/nginx/template/nginx.tmpl index c6e978ffeb..c71c22a739 100755 --- a/rootfs/etc/nginx/template/nginx.tmpl +++ b/rootfs/etc/nginx/template/nginx.tmpl @@ -154,9 +154,12 @@ http { {{ $all.Cfg.ModsecuritySnippet }} '; {{ end }} - + + {{ if (not (empty $all.Cfg.ModsecuritySnippet)) }} + # modsecurity_rules_file /etc/nginx/modsecurity/modsecurity.conf; + {{ 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 }} From bcda35ef6b8d97cd7d8a7983051500b4e4822aa6 Mon Sep 17 00:00:00 2001 From: besha100 <36448614+besha100@users.noreply.github.com> Date: Wed, 8 Dec 2021 23:28:01 +0100 Subject: [PATCH 02/15] Remove unnecessary comments Only have the default Modsecurity conf settings in case Modsecurity configuration snippet is not present and remove unnecessary comments --- rootfs/etc/nginx/template/nginx.tmpl | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/rootfs/etc/nginx/template/nginx.tmpl b/rootfs/etc/nginx/template/nginx.tmpl index c71c22a739..b3df55df20 100755 --- a/rootfs/etc/nginx/template/nginx.tmpl +++ b/rootfs/etc/nginx/template/nginx.tmpl @@ -155,11 +155,10 @@ http { '; {{ end }} - {{ if (not (empty $all.Cfg.ModsecuritySnippet)) }} - # modsecurity_rules_file /etc/nginx/modsecurity/modsecurity.conf; - {{ else }} + {{ if (empty $all.Cfg.ModsecuritySnippet) }} 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 }} From 48c0742d3cae27b95ccc2f0e943d50ab0c3068d3 Mon Sep 17 00:00:00 2001 From: besha100 <36448614+besha100@users.noreply.github.com> Date: Thu, 9 Dec 2021 10:45:46 +0100 Subject: [PATCH 03/15] Fixed modsecurity default file only if Modsecurity snippet present Fixed if condition Modsecurity snippet present have modsecurity default config file --- rootfs/etc/nginx/template/nginx.tmpl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rootfs/etc/nginx/template/nginx.tmpl b/rootfs/etc/nginx/template/nginx.tmpl index b3df55df20..30a1294521 100755 --- a/rootfs/etc/nginx/template/nginx.tmpl +++ b/rootfs/etc/nginx/template/nginx.tmpl @@ -155,7 +155,7 @@ http { '; {{ end }} - {{ if (empty $all.Cfg.ModsecuritySnippet) }} + {{ if not $all.Cfg.ModsecuritySnippet }} modsecurity_rules_file /etc/nginx/modsecurity/modsecurity.conf; {{ end }} From e485345224122aeaabfaf0d111d0b76a4b789dfa Mon Sep 17 00:00:00 2001 From: besha100 <36448614+besha100@users.noreply.github.com> Date: Sun, 12 Dec 2021 14:42:46 +0100 Subject: [PATCH 04/15] Added e2e test to disabling modsecurity conf Added e2e in case modsecurity-snippet enabled to disable settings in default modsecurity.conf --- .../annotations/modsecurity/modsecurity.go | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/test/e2e/annotations/modsecurity/modsecurity.go b/test/e2e/annotations/modsecurity/modsecurity.go index f88d6541e2..4c8a85a7e7 100644 --- a/test/e2e/annotations/modsecurity/modsecurity.go +++ b/test/e2e/annotations/modsecurity/modsecurity.go @@ -342,4 +342,40 @@ 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;") + }) + + f.HTTPTestClient(). + GET("/"). + WithHeader("Host", host). + WithHeader("User-Agent", "block-ua"). + Expect(). + Status(http.StatusForbidden) + }) }) From fa0fcf36ead4059b5441940d7656364b2f95a2d2 Mon Sep 17 00:00:00 2001 From: besha100 <36448614+besha100@users.noreply.github.com> Date: Sun, 12 Dec 2021 14:48:04 +0100 Subject: [PATCH 05/15] Validate writing to a different location Validate also modsecurity to write to a different location instead of the default directory --- test/e2e/annotations/modsecurity/modsecurity.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/e2e/annotations/modsecurity/modsecurity.go b/test/e2e/annotations/modsecurity/modsecurity.go index 4c8a85a7e7..bfec2ec310 100644 --- a/test/e2e/annotations/modsecurity/modsecurity.go +++ b/test/e2e/annotations/modsecurity/modsecurity.go @@ -368,7 +368,8 @@ var _ = framework.DescribeAnnotation("modsecurity owasp", func() { f.WaitForNginxServer(host, func(server string) bool { - return !strings.Contains(server, "modsecurity_rules_file /etc/nginx/modsecurity/modsecurity.conf;") + return !strings.Contains(server, "modsecurity_rules_file /etc/nginx/modsecurity/modsecurity.conf;") && + strings.Contains(server, "SecAuditLog /var/tmp/modsec_audit.log") }) f.HTTPTestClient(). From 112c3e4291cecf9a602f6b4294dbd7eb240907fe Mon Sep 17 00:00:00 2001 From: besha100 Date: Wed, 15 Dec 2021 17:33:50 +0100 Subject: [PATCH 06/15] Fixed the formatting --- test/e2e/annotations/modsecurity/modsecurity.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e/annotations/modsecurity/modsecurity.go b/test/e2e/annotations/modsecurity/modsecurity.go index bfec2ec310..4de85818db 100644 --- a/test/e2e/annotations/modsecurity/modsecurity.go +++ b/test/e2e/annotations/modsecurity/modsecurity.go @@ -342,7 +342,7 @@ 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 From b86c2199edd9adc561534b9985521781fc937370 Mon Sep 17 00:00:00 2001 From: besha100 Date: Wed, 15 Dec 2021 18:48:57 +0100 Subject: [PATCH 07/15] Fixed if empty ModsecuritySnippet --- rootfs/etc/nginx/template/nginx.tmpl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rootfs/etc/nginx/template/nginx.tmpl b/rootfs/etc/nginx/template/nginx.tmpl index 30a1294521..b3df55df20 100755 --- a/rootfs/etc/nginx/template/nginx.tmpl +++ b/rootfs/etc/nginx/template/nginx.tmpl @@ -155,7 +155,7 @@ http { '; {{ end }} - {{ if not $all.Cfg.ModsecuritySnippet }} + {{ if (empty $all.Cfg.ModsecuritySnippet) }} modsecurity_rules_file /etc/nginx/modsecurity/modsecurity.conf; {{ end }} From 3d1f84787110d927fe825b158b359af4a0188ecd Mon Sep 17 00:00:00 2001 From: besha100 Date: Wed, 15 Dec 2021 19:33:33 +0100 Subject: [PATCH 08/15] Fixed ModsecuritySnippet condition --- rootfs/etc/nginx/template/nginx.tmpl | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/rootfs/etc/nginx/template/nginx.tmpl b/rootfs/etc/nginx/template/nginx.tmpl index b3df55df20..0cc8d3cabe 100755 --- a/rootfs/etc/nginx/template/nginx.tmpl +++ b/rootfs/etc/nginx/template/nginx.tmpl @@ -153,9 +153,7 @@ http { modsecurity_rules ' {{ $all.Cfg.ModsecuritySnippet }} '; - {{ end }} - - {{ if (empty $all.Cfg.ModsecuritySnippet) }} + {{ else }} modsecurity_rules_file /etc/nginx/modsecurity/modsecurity.conf; {{ end }} From 63152368d71f99fad22a7117f4be1c106b342a42 Mon Sep 17 00:00:00 2001 From: besha100 Date: Wed, 15 Dec 2021 20:22:42 +0100 Subject: [PATCH 09/15] Fixed the condition also in ingress controller template --- internal/ingress/controller/template/template.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/internal/ingress/controller/template/template.go b/internal/ingress/controller/template/template.go index ae5ec259a2..d8d275caa6 100644 --- a/internal/ingress/controller/template/template.go +++ b/internal/ingress/controller/template/template.go @@ -1530,6 +1530,9 @@ func buildModSecurityForLocation(cfg config.Configuration, location *ingress.Loc %v '; `, location.ModSecurity.Snippet)) + } else { + buffer.WriteString(`modsecurity_rules_file /etc/nginx/modsecurity/modsecurity.conf; +`) } if location.ModSecurity.TransactionID != "" { @@ -1537,11 +1540,6 @@ func buildModSecurityForLocation(cfg config.Configuration, location *ingress.Loc `, location.ModSecurity.TransactionID)) } - if !isMSEnabled { - buffer.WriteString(`modsecurity_rules_file /etc/nginx/modsecurity/modsecurity.conf; -`) - } - if !cfg.EnableOWASPCoreRules && location.ModSecurity.OWASPRules { buffer.WriteString(`modsecurity_rules_file /etc/nginx/owasp-modsecurity-crs/nginx-modsecurity.conf; `) From fed84ef5cfc9fab6928016e126de92f37572c03f Mon Sep 17 00:00:00 2001 From: besha100 Date: Wed, 15 Dec 2021 21:32:16 +0100 Subject: [PATCH 10/15] Removed the default config condition in ingress controller template --- internal/ingress/controller/template/template.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/internal/ingress/controller/template/template.go b/internal/ingress/controller/template/template.go index d8d275caa6..5db9fd3ec9 100644 --- a/internal/ingress/controller/template/template.go +++ b/internal/ingress/controller/template/template.go @@ -1530,9 +1530,6 @@ func buildModSecurityForLocation(cfg config.Configuration, location *ingress.Loc %v '; `, location.ModSecurity.Snippet)) - } else { - buffer.WriteString(`modsecurity_rules_file /etc/nginx/modsecurity/modsecurity.conf; -`) } if location.ModSecurity.TransactionID != "" { From 9d38eca40fe615a4c756500ca57b05634240edde Mon Sep 17 00:00:00 2001 From: besha100 Date: Wed, 15 Dec 2021 23:00:24 +0100 Subject: [PATCH 11/15] Fixed the default config condition in ingress controller template --- internal/ingress/controller/template/template.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/internal/ingress/controller/template/template.go b/internal/ingress/controller/template/template.go index 5db9fd3ec9..e5be1490a3 100644 --- a/internal/ingress/controller/template/template.go +++ b/internal/ingress/controller/template/template.go @@ -1537,6 +1537,11 @@ func buildModSecurityForLocation(cfg config.Configuration, location *ingress.Loc `, location.ModSecurity.TransactionID)) } + if !isMSEnabled && location.ModSecurity.Snippet == "" { + buffer.WriteString(`modsecurity_rules_file /etc/nginx/modsecurity/modsecurity.conf; +`) + } + if !cfg.EnableOWASPCoreRules && location.ModSecurity.OWASPRules { buffer.WriteString(`modsecurity_rules_file /etc/nginx/owasp-modsecurity-crs/nginx-modsecurity.conf; `) From d1defc15cf7d3098d4a7e2774cf360823f3f02ad Mon Sep 17 00:00:00 2001 From: besha100 Date: Thu, 16 Dec 2021 00:48:13 +0100 Subject: [PATCH 12/15] Fixed pull-ingress-nginx-test --- internal/ingress/controller/template/template_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/ingress/controller/template/template_test.go b/internal/ingress/controller/template/template_test.go index cfb65c08e3..c34f8dad31 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%v", loadModule, modsecRule)}, + {"configmap disabled, annotation enabled, OWASP enabled", false, false, true, true, false, testRule, "", fmt.Sprintf("%v%v%v", loadModule, modsecRule)}, } for _, testCase := range testCases { From c60b207f2d5551d0aefc25c9d436229ecccfe574 Mon Sep 17 00:00:00 2001 From: besha100 Date: Thu, 16 Dec 2021 13:19:50 +0100 Subject: [PATCH 13/15] Revert "Fixed the default config condition in ingress controller template" This reverts commit 9d38eca40fe615a4c756500ca57b05634240edde. --- internal/ingress/controller/template/template.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/internal/ingress/controller/template/template.go b/internal/ingress/controller/template/template.go index e5be1490a3..5db9fd3ec9 100644 --- a/internal/ingress/controller/template/template.go +++ b/internal/ingress/controller/template/template.go @@ -1537,11 +1537,6 @@ func buildModSecurityForLocation(cfg config.Configuration, location *ingress.Loc `, location.ModSecurity.TransactionID)) } - if !isMSEnabled && location.ModSecurity.Snippet == "" { - buffer.WriteString(`modsecurity_rules_file /etc/nginx/modsecurity/modsecurity.conf; -`) - } - if !cfg.EnableOWASPCoreRules && location.ModSecurity.OWASPRules { buffer.WriteString(`modsecurity_rules_file /etc/nginx/owasp-modsecurity-crs/nginx-modsecurity.conf; `) From 00f8dffc783ec5f2ca80b149884195ad6117f3c9 Mon Sep 17 00:00:00 2001 From: besha100 Date: Thu, 16 Dec 2021 13:24:12 +0100 Subject: [PATCH 14/15] Revert template_test --- internal/ingress/controller/template/template.go | 5 +++++ internal/ingress/controller/template/template_test.go | 4 ++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/internal/ingress/controller/template/template.go b/internal/ingress/controller/template/template.go index 5db9fd3ec9..e5be1490a3 100644 --- a/internal/ingress/controller/template/template.go +++ b/internal/ingress/controller/template/template.go @@ -1537,6 +1537,11 @@ func buildModSecurityForLocation(cfg config.Configuration, location *ingress.Loc `, location.ModSecurity.TransactionID)) } + if !isMSEnabled && location.ModSecurity.Snippet == "" { + buffer.WriteString(`modsecurity_rules_file /etc/nginx/modsecurity/modsecurity.conf; +`) + } + if !cfg.EnableOWASPCoreRules && location.ModSecurity.OWASPRules { buffer.WriteString(`modsecurity_rules_file /etc/nginx/owasp-modsecurity-crs/nginx-modsecurity.conf; `) diff --git a/internal/ingress/controller/template/template_test.go b/internal/ingress/controller/template/template_test.go index c34f8dad31..cfb65c08e3 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)}, - {"configmap disabled, annotation enabled, OWASP enabled", false, false, true, true, false, testRule, "", fmt.Sprintf("%v%v%v", loadModule, modsecRule)}, + {"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)}, } for _, testCase := range testCases { From 54263d0142b7f08c22a4534f98d2b3d4b2384b92 Mon Sep 17 00:00:00 2001 From: besha100 Date: Thu, 16 Dec 2021 18:42:02 +0100 Subject: [PATCH 15/15] Adjusted the formating %v --- internal/ingress/controller/template/template_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/ingress/controller/template/template_test.go b/internal/ingress/controller/template/template_test.go index cfb65c08e3..b65e33c320 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 {