Merge pull request #4119 from tammert/conditional-modsecurity-module-load

Only load module ngx_http_modsecurity_module.so when option enable-mo…

internal/ingress/controller/template/template.go

@@ -170,6 +170,7 @@ var (
 		"buildCustomErrorDeps":               buildCustomErrorDeps,
 		"opentracingPropagateContext":        opentracingPropagateContext,
 		"buildCustomErrorLocationsPerServer": buildCustomErrorLocationsPerServer,
+		"shouldLoadModSecurityModule":        shouldLoadModSecurityModule,
 	}
 )
 
@@ -1043,3 +1044,37 @@ func opentracingPropagateContext(loc interface{}) string {
 
 	return "opentracing_propagate_context"
 }
+
+// shouldLoadModSecurityModule determines whether or not the ModSecurity module needs to be loaded.
+// First, it checks if `enable-modsecurity` is set in the ConfigMap. If it is not, it iterates over all locations to
+// check if ModSecurity is enabled by the annotation `nginx.ingress.kubernetes.io/enable-modsecurity`.
+func shouldLoadModSecurityModule(c interface{}, s interface{}) bool {
+	cfg, ok := c.(config.Configuration)
+	if !ok {
+		klog.Errorf("expected a 'config.Configuration' type but %T was returned", c)
+		return false
+	}
+
+	servers, ok := s.([]*ingress.Server)
+	if !ok {
+		klog.Errorf("expected an '[]*ingress.Server' type but %T was returned", s)
+		return false
+	}
+
+	// Determine if ModSecurity is enabled globally.
+	if cfg.EnableModsecurity {
+		return true
+	}
+
+	// If ModSecurity is not enabled globally, check if any location has it enabled via annotation.
+	for _, server := range servers {
+		for _, location := range server.Locations {
+			if location.ModSecurity.Enable {
+				return true
+			}
+		}
+	}
+
+	// Not enabled globally nor via annotation on a location, no need to load the module.
+	return false
+}

internal/ingress/controller/template/template_test.go

@@ -35,6 +35,7 @@ import (
 	"k8s.io/ingress-nginx/internal/ingress/annotations/authreq"
 	"k8s.io/ingress-nginx/internal/ingress/annotations/influxdb"
 	"k8s.io/ingress-nginx/internal/ingress/annotations/luarestywaf"
+	"k8s.io/ingress-nginx/internal/ingress/annotations/modsecurity"
 	"k8s.io/ingress-nginx/internal/ingress/annotations/ratelimit"
 	"k8s.io/ingress-nginx/internal/ingress/annotations/rewrite"
 	"k8s.io/ingress-nginx/internal/ingress/controller/config"
@@ -184,18 +185,18 @@ func TestBuildLuaSharedDictionaries(t *testing.T) {
 		},
 	}
 
-	config := buildLuaSharedDictionaries(servers, false)
-	if !strings.Contains(config, "lua_shared_dict configuration_data") {
-		t.Errorf("expected to include 'configuration_data' but got %s", config)
+	configuration := buildLuaSharedDictionaries(servers, false)
+	if !strings.Contains(configuration, "lua_shared_dict configuration_data") {
+		t.Errorf("expected to include 'configuration_data' but got %s", configuration)
 	}
-	if strings.Contains(config, "waf_storage") {
-		t.Errorf("expected to not include 'waf_storage' but got %s", config)
+	if strings.Contains(configuration, "waf_storage") {
+		t.Errorf("expected to not include 'waf_storage' but got %s", configuration)
 	}
 
 	servers[1].Locations[0].LuaRestyWAF = luarestywaf.Config{Mode: "ACTIVE"}
-	config = buildLuaSharedDictionaries(servers, false)
-	if !strings.Contains(config, "lua_shared_dict waf_storage") {
-		t.Errorf("expected to configure 'waf_storage', but got %s", config)
+	configuration = buildLuaSharedDictionaries(servers, false)
+	if !strings.Contains(configuration, "lua_shared_dict waf_storage") {
+		t.Errorf("expected to configure 'waf_storage', but got %s", configuration)
 	}
 }
 
@@ -1212,3 +1213,56 @@ func TestStripLocationModifer(t *testing.T) {
 		t.Errorf("Expected '%v' but returned '%v'", expected, actual)
 	}
 }
+
+func TestShouldLoadModSecurityModule(t *testing.T) {
+	// ### Invalid argument type tests ###
+	// The first tests should return false.
+	expected := false
+
+	invalidType := &ingress.Ingress{}
+	actual := shouldLoadModSecurityModule(config.Configuration{}, invalidType)
+	if expected != actual {
+		t.Errorf("Expected '%v' but returned '%v'", expected, actual)
+	}
+
+	actual = shouldLoadModSecurityModule(invalidType, []*ingress.Server{})
+	if expected != actual {
+		t.Errorf("Expected '%v' but returned '%v'", expected, actual)
+	}
+
+	// ### Functional tests ###
+	actual = shouldLoadModSecurityModule(config.Configuration{}, []*ingress.Server{})
+	if expected != actual {
+		t.Errorf("Expected '%v' but returned '%v'", expected, actual)
+	}
+
+	// All further tests should return true.
+	expected = true
+
+	configuration := config.Configuration{EnableModsecurity: true}
+	actual = shouldLoadModSecurityModule(configuration, []*ingress.Server{})
+	if expected != actual {
+		t.Errorf("Expected '%v' but returned '%v'", expected, actual)
+	}
+
+	servers := []*ingress.Server{
+		{
+			Locations: []*ingress.Location{
+				{
+					ModSecurity: modsecurity.Config{
+						Enable: true,
+					},
+				},
+			},
+		},
+	}
+	actual = shouldLoadModSecurityModule(config.Configuration{}, servers)
+	if expected != actual {
+		t.Errorf("Expected '%v' but returned '%v'", expected, actual)
+	}
+
+	actual = shouldLoadModSecurityModule(configuration, servers)
+	if expected != actual {
+		t.Errorf("Expected '%v' but returned '%v'", expected, actual)
+	}
+}

rootfs/etc/nginx/template/nginx.tmpl

@@ -16,7 +16,9 @@ pid {{ .PID }};
 load_module /etc/nginx/modules/ngx_http_geoip2_module.so;
 {{ end }}
 
+{{ if (shouldLoadModSecurityModule $cfg $servers) }}
 load_module /etc/nginx/modules/ngx_http_modsecurity_module.so;
+{{ end }}
 
 {{ if $cfg.EnableOpentracing }}
 load_module /etc/nginx/modules/ngx_http_opentracing_module.so;