Skip to content

Commit 8cee8d5

Browse files
authored
Merge pull request #4119 from tammert/conditional-modsecurity-module-load
Only load module ngx_http_modsecurity_module.so when option enable-mo…
2 parents 2879309 + c11583d commit 8cee8d5

File tree

3 files changed

+99
-8
lines changed

3 files changed

+99
-8
lines changed

internal/ingress/controller/template/template.go

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,7 @@ var (
170170
"buildCustomErrorDeps": buildCustomErrorDeps,
171171
"opentracingPropagateContext": opentracingPropagateContext,
172172
"buildCustomErrorLocationsPerServer": buildCustomErrorLocationsPerServer,
173+
"shouldLoadModSecurityModule": shouldLoadModSecurityModule,
173174
}
174175
)
175176

@@ -1043,3 +1044,37 @@ func opentracingPropagateContext(loc interface{}) string {
10431044

10441045
return "opentracing_propagate_context"
10451046
}
1047+
1048+
// shouldLoadModSecurityModule determines whether or not the ModSecurity module needs to be loaded.
1049+
// First, it checks if `enable-modsecurity` is set in the ConfigMap. If it is not, it iterates over all locations to
1050+
// check if ModSecurity is enabled by the annotation `nginx.ingress.kubernetes.io/enable-modsecurity`.
1051+
func shouldLoadModSecurityModule(c interface{}, s interface{}) bool {
1052+
cfg, ok := c.(config.Configuration)
1053+
if !ok {
1054+
klog.Errorf("expected a 'config.Configuration' type but %T was returned", c)
1055+
return false
1056+
}
1057+
1058+
servers, ok := s.([]*ingress.Server)
1059+
if !ok {
1060+
klog.Errorf("expected an '[]*ingress.Server' type but %T was returned", s)
1061+
return false
1062+
}
1063+
1064+
// Determine if ModSecurity is enabled globally.
1065+
if cfg.EnableModsecurity {
1066+
return true
1067+
}
1068+
1069+
// If ModSecurity is not enabled globally, check if any location has it enabled via annotation.
1070+
for _, server := range servers {
1071+
for _, location := range server.Locations {
1072+
if location.ModSecurity.Enable {
1073+
return true
1074+
}
1075+
}
1076+
}
1077+
1078+
// Not enabled globally nor via annotation on a location, no need to load the module.
1079+
return false
1080+
}

internal/ingress/controller/template/template_test.go

Lines changed: 62 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ import (
3535
"k8s.io/ingress-nginx/internal/ingress/annotations/authreq"
3636
"k8s.io/ingress-nginx/internal/ingress/annotations/influxdb"
3737
"k8s.io/ingress-nginx/internal/ingress/annotations/luarestywaf"
38+
"k8s.io/ingress-nginx/internal/ingress/annotations/modsecurity"
3839
"k8s.io/ingress-nginx/internal/ingress/annotations/ratelimit"
3940
"k8s.io/ingress-nginx/internal/ingress/annotations/rewrite"
4041
"k8s.io/ingress-nginx/internal/ingress/controller/config"
@@ -184,18 +185,18 @@ func TestBuildLuaSharedDictionaries(t *testing.T) {
184185
},
185186
}
186187

187-
config := buildLuaSharedDictionaries(servers, false)
188-
if !strings.Contains(config, "lua_shared_dict configuration_data") {
189-
t.Errorf("expected to include 'configuration_data' but got %s", config)
188+
configuration := buildLuaSharedDictionaries(servers, false)
189+
if !strings.Contains(configuration, "lua_shared_dict configuration_data") {
190+
t.Errorf("expected to include 'configuration_data' but got %s", configuration)
190191
}
191-
if strings.Contains(config, "waf_storage") {
192-
t.Errorf("expected to not include 'waf_storage' but got %s", config)
192+
if strings.Contains(configuration, "waf_storage") {
193+
t.Errorf("expected to not include 'waf_storage' but got %s", configuration)
193194
}
194195

195196
servers[1].Locations[0].LuaRestyWAF = luarestywaf.Config{Mode: "ACTIVE"}
196-
config = buildLuaSharedDictionaries(servers, false)
197-
if !strings.Contains(config, "lua_shared_dict waf_storage") {
198-
t.Errorf("expected to configure 'waf_storage', but got %s", config)
197+
configuration = buildLuaSharedDictionaries(servers, false)
198+
if !strings.Contains(configuration, "lua_shared_dict waf_storage") {
199+
t.Errorf("expected to configure 'waf_storage', but got %s", configuration)
199200
}
200201
}
201202

@@ -1212,3 +1213,56 @@ func TestStripLocationModifer(t *testing.T) {
12121213
t.Errorf("Expected '%v' but returned '%v'", expected, actual)
12131214
}
12141215
}
1216+
1217+
func TestShouldLoadModSecurityModule(t *testing.T) {
1218+
// ### Invalid argument type tests ###
1219+
// The first tests should return false.
1220+
expected := false
1221+
1222+
invalidType := &ingress.Ingress{}
1223+
actual := shouldLoadModSecurityModule(config.Configuration{}, invalidType)
1224+
if expected != actual {
1225+
t.Errorf("Expected '%v' but returned '%v'", expected, actual)
1226+
}
1227+
1228+
actual = shouldLoadModSecurityModule(invalidType, []*ingress.Server{})
1229+
if expected != actual {
1230+
t.Errorf("Expected '%v' but returned '%v'", expected, actual)
1231+
}
1232+
1233+
// ### Functional tests ###
1234+
actual = shouldLoadModSecurityModule(config.Configuration{}, []*ingress.Server{})
1235+
if expected != actual {
1236+
t.Errorf("Expected '%v' but returned '%v'", expected, actual)
1237+
}
1238+
1239+
// All further tests should return true.
1240+
expected = true
1241+
1242+
configuration := config.Configuration{EnableModsecurity: true}
1243+
actual = shouldLoadModSecurityModule(configuration, []*ingress.Server{})
1244+
if expected != actual {
1245+
t.Errorf("Expected '%v' but returned '%v'", expected, actual)
1246+
}
1247+
1248+
servers := []*ingress.Server{
1249+
{
1250+
Locations: []*ingress.Location{
1251+
{
1252+
ModSecurity: modsecurity.Config{
1253+
Enable: true,
1254+
},
1255+
},
1256+
},
1257+
},
1258+
}
1259+
actual = shouldLoadModSecurityModule(config.Configuration{}, servers)
1260+
if expected != actual {
1261+
t.Errorf("Expected '%v' but returned '%v'", expected, actual)
1262+
}
1263+
1264+
actual = shouldLoadModSecurityModule(configuration, servers)
1265+
if expected != actual {
1266+
t.Errorf("Expected '%v' but returned '%v'", expected, actual)
1267+
}
1268+
}

rootfs/etc/nginx/template/nginx.tmpl

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,9 @@ pid {{ .PID }};
1616
load_module /etc/nginx/modules/ngx_http_geoip2_module.so;
1717
{{ end }}
1818

19+
{{ if (shouldLoadModSecurityModule $cfg $servers) }}
1920
load_module /etc/nginx/modules/ngx_http_modsecurity_module.so;
21+
{{ end }}
2022

2123
{{ if $cfg.EnableOpentracing }}
2224
load_module /etc/nginx/modules/ngx_http_opentracing_module.so;

0 commit comments

Comments
 (0)