Fix definition order of modsecurity directives

kubernetes · Apr 2, 2020 · c3d811d · c3d811d
1 parent 680d801
commit c3d811d
Show file tree

Hide file tree

Showing 3 changed files with 85 additions and 11 deletions.
diff --git a/internal/ingress/controller/template/template.go b/internal/ingress/controller/template/template.go
@@ -1360,12 +1360,6 @@ func buildModSecurityForLocation(cfg config.Configuration, location *ingress.Loc
 
 	if !isMSEnabled {
 		buffer.WriteString(`modsecurity on;
-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;
 `)
 	}
 
@@ -1381,6 +1375,16 @@ modsecurity_rules_file /etc/nginx/modsecurity/modsecurity.conf;
 `, 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;
+`)
+	}
+
 	return buffer.String()
 }
 

diff --git a/internal/ingress/controller/template/template_test.go b/internal/ingress/controller/template/template_test.go
@@ -1373,7 +1373,9 @@ func TestShouldLoadOpentracingModule(t *testing.T) {
 
 func TestModSecurityForLocation(t *testing.T) {
 	loadModule := `modsecurity on;
-modsecurity_rules_file /etc/nginx/modsecurity/modsecurity.conf;
+`
+
+	modSecCfg := `modsecurity_rules_file /etc/nginx/modsecurity/modsecurity.conf;
 `
 
 	modsecOff := "modsecurity off;"
@@ -1411,12 +1413,12 @@ modsecurity_rules_file /etc/nginx/modsecurity/modsecurity.conf;
 		{"configmap enabled, configmap OWASP disabled, annotation enabled, OWASP enabled, no snippet and no transaction ID", true, false, true, true, true, "", "", owaspRules},
 		{"configmap enabled, configmap OWASP disabled, annotation disabled, OWASP disabled, no snippet and no transaction ID", true, false, false, true, false, "", "", modsecOff},
 		{"configmap enabled, configmap OWASP disabled, annotation enabled, OWASP enabled, with snippet and no transaction ID", true, false, true, true, true, "", "", owaspRules},
-		{"configmap enabled, configmap OWASP disabled, annotation enabled, OWASP enabled, with snippet and transaction ID", true, false, true, true, true, "", transactionID, fmt.Sprintf("%v%v", owaspRules, transactionCfg)},
+		{"configmap enabled, configmap OWASP disabled, annotation enabled, OWASP enabled, with snippet and transaction ID", true, false, true, true, true, "", transactionID, fmt.Sprintf("%v%v", transactionCfg, owaspRules)},
 		{"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, "", "", loadModule},
+		{"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", loadModule, modsecRule)},
-		{"configmap disabled, annotation enabled, OWASP enabled", false, false, true, true, false, testRule, "", fmt.Sprintf("%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 {

diff --git a/test/e2e/annotations/modsecurity.go b/test/e2e/annotations/modsecurity.go
@@ -17,6 +17,7 @@ limitations under the License.
 package annotations
 
 import (
+	"net/http"
 	"strings"
 
 	"github.com/onsi/ginkgo"
@@ -148,4 +149,71 @@ var _ = framework.DescribeAnnotation("modsecurity owasp", func() {
 			})
 	})
 
+	ginkgo.It("should enable modsecurity with snippet and block requests", func() {
+		host := "modsecurity.foo.com"
+		nameSpace := f.Namespace
+
+		snippet := `SecRuleEngine On
+		SecRequestBodyAccess On
+		SecAuditEngine RelevantOnly
+		SecAuditLogParts ABIJDEFHZ
+		SecAuditLog /dev/stdout
+		SecAuditLogType Serial
+		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,
+		}
+
+		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 on;") &&
+					strings.Contains(server, "SecRuleEngine On")
+			})
+
+		f.HTTPTestClient().
+			GET("/").
+			WithHeader("Host", host).
+			WithHeader("User-Agent", "block-ua").
+			Expect().
+			Status(http.StatusForbidden)
+	})
+
+	ginkgo.It("should enable modsecurity globally and with modsecurity-snippet block requests", func() {
+		host := "modsecurity.foo.com"
+		nameSpace := f.Namespace
+
+		snippet := `SecRuleEngine On
+		SecRequestBodyAccess On
+		SecAuditEngine RelevantOnly
+		SecAuditLogParts ABIJDEFHZ
+		SecAuditLog /dev/stdout
+		SecAuditLogType Serial
+		SecRule REQUEST_HEADERS:User-Agent \"block-ua\" \"log,deny,id:107,status:403,msg:\'UA blocked\'\"`
+
+		annotations := map[string]string{
+			"nginx.ingress.kubernetes.io/modsecurity-snippet": snippet,
+		}
+
+		ing := framework.NewSingleIngress(host, "/", host, nameSpace, framework.EchoService, 80, annotations)
+		f.EnsureIngress(ing)
+
+		f.UpdateNginxConfigMapData("enable-modsecurity", "true")
+
+		f.WaitForNginxServer(host,
+			func(server string) bool {
+				return strings.Contains(server, "SecRuleEngine On")
+			})
+
+		f.HTTPTestClient().
+			GET("/").
+			WithHeader("Host", host).
+			WithHeader("User-Agent", "block-ua").
+			Expect().
+			Status(http.StatusForbidden)
+	})
 })