diff --git a/internal/ingress/controller/template/template.go b/internal/ingress/controller/template/template.go index ae750d2e6d4..1b2d6a23cf9 100644 --- a/internal/ingress/controller/template/template.go +++ b/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 +} diff --git a/internal/ingress/controller/template/template_test.go b/internal/ingress/controller/template/template_test.go index 5ddc0eef1ad..6944255d2c5 100644 --- a/internal/ingress/controller/template/template_test.go +++ b/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) + } +} diff --git a/rootfs/etc/nginx/template/nginx.tmpl b/rootfs/etc/nginx/template/nginx.tmpl index fccb83cca9e..f012957af1a 100755 --- a/rootfs/etc/nginx/template/nginx.tmpl +++ b/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;