From 380ef3a92cedc778289681c48baf41515c11b357 Mon Sep 17 00:00:00 2001 From: Bhavin Gandhi Date: Sun, 22 Mar 2020 01:05:07 +0530 Subject: [PATCH] Fix the ability to disable ModSecurity at location level - Adds 'modsecurity off;' to the nginx config if the 'enable-modsecurity' annotation is set to false. - Update tests and e2e tests accordingly Signed-off-by: Bhavin Gandhi --- .../ingress/annotations/modsecurity/main.go | 6 +++ .../annotations/modsecurity/main_test.go | 24 +++++----- .../ingress/controller/template/template.go | 5 ++- .../controller/template/template_test.go | 32 ++++++++------ test/e2e/annotations/modsecurity.go | 44 +++++++++++++++++++ 5 files changed, 84 insertions(+), 27 deletions(-) diff --git a/internal/ingress/annotations/modsecurity/main.go b/internal/ingress/annotations/modsecurity/main.go index 85fd71573e1..91a0a5a9472 100644 --- a/internal/ingress/annotations/modsecurity/main.go +++ b/internal/ingress/annotations/modsecurity/main.go @@ -25,6 +25,7 @@ import ( // Config contains ModSecurity Configuration items type Config struct { Enable bool `json:"enable-modsecurity"` + EnableSet bool `json:"enable-modsecurity-set"` OWASPRules bool `json:"enable-owasp-core-rules"` TransactionID string `json:"modsecurity-transaction-id"` Snippet string `json:"modsecurity-snippet"` @@ -41,6 +42,9 @@ func (modsec1 *Config) Equal(modsec2 *Config) bool { if modsec1.Enable != modsec2.Enable { return false } + if modsec1.EnableSet != modsec2.EnableSet { + return false + } if modsec1.OWASPRules != modsec2.OWASPRules { return false } @@ -69,9 +73,11 @@ func (a modSecurity) Parse(ing *networking.Ingress) (interface{}, error) { var err error config := &Config{} + config.EnableSet = true config.Enable, err = parser.GetBoolAnnotation("enable-modsecurity", ing) if err != nil { config.Enable = false + config.EnableSet = false } config.OWASPRules, err = parser.GetBoolAnnotation("enable-owasp-core-rules", ing) diff --git a/internal/ingress/annotations/modsecurity/main_test.go b/internal/ingress/annotations/modsecurity/main_test.go index e609c8b00cb..34d92533dcf 100644 --- a/internal/ingress/annotations/modsecurity/main_test.go +++ b/internal/ingress/annotations/modsecurity/main_test.go @@ -41,22 +41,22 @@ func TestParse(t *testing.T) { annotations map[string]string expected Config }{ - {map[string]string{enable: "true"}, Config{true, false, "", ""}}, - {map[string]string{enable: "false"}, Config{false, false, "", ""}}, - {map[string]string{enable: ""}, Config{false, false, "", ""}}, + {map[string]string{enable: "true"}, Config{true, true, false, "", ""}}, + {map[string]string{enable: "false"}, Config{false, true, false, "", ""}}, + {map[string]string{enable: ""}, Config{false, false, false, "", ""}}, - {map[string]string{owasp: "true"}, Config{false, true, "", ""}}, - {map[string]string{owasp: "false"}, Config{false, false, "", ""}}, - {map[string]string{owasp: ""}, Config{false, false, "", ""}}, + {map[string]string{owasp: "true"}, Config{false, false, true, "", ""}}, + {map[string]string{owasp: "false"}, Config{false, false, false, "", ""}}, + {map[string]string{owasp: ""}, Config{false, false, false, "", ""}}, - {map[string]string{transID: "ok"}, Config{false, false, "ok", ""}}, - {map[string]string{transID: ""}, Config{false, false, "", ""}}, + {map[string]string{transID: "ok"}, Config{false, false, false, "ok", ""}}, + {map[string]string{transID: ""}, Config{false, false, false, "", ""}}, - {map[string]string{snippet: "ModSecurity Rule"}, Config{false, false, "", "ModSecurity Rule"}}, - {map[string]string{snippet: ""}, Config{false, false, "", ""}}, + {map[string]string{snippet: "ModSecurity Rule"}, Config{false, false, false, "", "ModSecurity Rule"}}, + {map[string]string{snippet: ""}, Config{false, false, false, "", ""}}, - {map[string]string{}, Config{false, false, "", ""}}, - {nil, Config{false, false, "", ""}}, + {map[string]string{}, Config{false, false, false, "", ""}}, + {nil, Config{false, false, false, "", ""}}, } ing := &networking.Ingress{ diff --git a/internal/ingress/controller/template/template.go b/internal/ingress/controller/template/template.go index afe9fedeb88..a423b86a291 100644 --- a/internal/ingress/controller/template/template.go +++ b/internal/ingress/controller/template/template.go @@ -1345,14 +1345,15 @@ func shouldLoadOpentracingModule(c interface{}, s interface{}) bool { func buildModSecurityForLocation(cfg config.Configuration, location *ingress.Location) string { isMSEnabledInLoc := location.ModSecurity.Enable + isMSEnableSetInLoc := location.ModSecurity.EnableSet isMSEnabled := cfg.EnableModsecurity if !isMSEnabled && !isMSEnabledInLoc { return "" } - if !isMSEnabledInLoc { - return "" + if isMSEnableSetInLoc && !isMSEnabledInLoc { + return "modsecurity off;" } var buffer bytes.Buffer diff --git a/internal/ingress/controller/template/template_test.go b/internal/ingress/controller/template/template_test.go index 7e32eb188cb..782a54d4a77 100644 --- a/internal/ingress/controller/template/template_test.go +++ b/internal/ingress/controller/template/template_test.go @@ -1376,6 +1376,8 @@ func TestModSecurityForLocation(t *testing.T) { modsecurity_rules_file /etc/nginx/modsecurity/modsecurity.conf; ` + modsecOff := "modsecurity off;" + modsecRule := `modsecurity_rules ' #RULE# '; @@ -1394,30 +1396,34 @@ modsecurity_rules_file /etc/nginx/modsecurity/modsecurity.conf; isEnabledInCM bool isOwaspEnabledInCM bool isEnabledInLoc bool + isEnableSetInLoc bool isOwaspEnabledInLoc bool snippet string transactionID string expected string }{ - {"configmap enabled, configmap OWASP disabled, without annotation, snippet or transaction ID", true, false, false, false, "", "", ""}, - {"configmap enabled, configmap OWASP disabled, without annotation, snippet and with transaction ID", true, false, false, false, "", transactionID, ""}, - {"configmap enabled, configmap OWASP enabled, without annotation, OWASP enabled", true, true, false, false, "", "", ""}, - {"configmap enabled, configmap OWASP enabled, without annotation, OWASP disabled, with snippet and no transaction ID", true, true, true, false, testRule, "", modsecRule}, - {"configmap enabled, configmap OWASP enabled, without annotation, OWASP disabled, with snippet and transaction ID", true, true, true, false, testRule, transactionID, fmt.Sprintf("%v%v", modsecRule, transactionCfg)}, - {"configmap enabled, with annotation, OWASP disabled", true, false, true, false, "", "", ""}, - {"configmap enabled, configmap OWASP disabled, with annotation, OWASP enabled, no snippet and no transaction ID", true, false, true, true, "", "", owaspRules}, - {"configmap enabled, configmap OWASP disabled, with annotation, OWASP enabled, with snippet and no transaction ID", true, false, true, true, "", "", owaspRules}, - {"configmap enabled, configmap OWASP disabled, with annotation, OWASP enabled, with snippet and transaction ID", true, false, true, true, "", transactionID, fmt.Sprintf("%v%v", owaspRules, transactionCfg)}, - {"configmap enabled, OWASP configmap enabled, with annotation, OWASP disabled", true, true, true, false, "", "", ""}, - {"configmap disabled, with annotation, OWASP disabled", false, false, true, false, "", "", loadModule}, - {"configmap disabled, with annotation, OWASP disabled", false, false, true, false, testRule, "", fmt.Sprintf("%v%v", loadModule, modsecRule)}, - {"configmap disabled, with annotation, OWASP enabled", false, false, true, false, testRule, "", fmt.Sprintf("%v%v", loadModule, modsecRule)}, + {"configmap enabled, configmap OWASP disabled, without annotation, snippet or transaction ID", true, false, false, false, false, "", "", ""}, + {"configmap enabled, configmap OWASP disabled, without annotation, snippet and with transaction ID", true, false, false, false, false, "", transactionID, transactionCfg}, + {"configmap enabled, configmap OWASP enabled, without annotation, OWASP enabled", true, true, false, false, false, "", "", ""}, + {"configmap enabled, configmap OWASP enabled, without annotation, OWASP disabled, with snippet and no transaction ID", true, true, false, false, false, testRule, "", modsecRule}, + {"configmap enabled, configmap OWASP enabled, without annotation, OWASP disabled, with snippet and transaction ID", true, true, false, false, false, testRule, transactionID, fmt.Sprintf("%v%v", modsecRule, transactionCfg)}, + {"configmap enabled, configmap OWASP disabled, annotation enabled, OWASP disabled", true, false, true, true, false, "", "", ""}, + {"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 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 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)}, } for _, testCase := range testCases { il := &ingress.Location{ ModSecurity: modsecurity.Config{ Enable: testCase.isEnabledInLoc, + EnableSet: testCase.isEnableSetInLoc, OWASPRules: testCase.isOwaspEnabledInLoc, Snippet: testCase.snippet, TransactionID: testCase.transactionID, diff --git a/test/e2e/annotations/modsecurity.go b/test/e2e/annotations/modsecurity.go index 08a3cccc8ed..a3682243f25 100644 --- a/test/e2e/annotations/modsecurity.go +++ b/test/e2e/annotations/modsecurity.go @@ -104,4 +104,48 @@ var _ = framework.DescribeAnnotation("modsecurity owasp", func() { strings.Contains(server, "SecRuleEngine On") }) }) + + ginkgo.It("should enable modsecurity without using 'modsecurity on;'", func() { + f.SetNginxConfigMapData(map[string]string{ + "enable-modsecurity": "true"}, + ) + + host := "modsecurity.foo.com" + nameSpace := f.Namespace + + annotations := map[string]string{ + "nginx.ingress.kubernetes.io/enable-modsecurity": "true", + } + + 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, "modsecurity_rules_file /etc/nginx/modsecurity/modsecurity.conf;") + }) + }) + + ginkgo.It("should disable modsecurity using 'modsecurity off;'", func() { + f.SetNginxConfigMapData(map[string]string{ + "enable-modsecurity": "true"}, + ) + + host := "modsecurity.foo.com" + nameSpace := f.Namespace + + annotations := map[string]string{ + "nginx.ingress.kubernetes.io/enable-modsecurity": "false", + } + + 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 off;") + }) + }) + })