Skip to content

Commit

Permalink
Fix the ability to disable ModSecurity at location level
Browse files Browse the repository at this point in the history 
- Adds 'modsecurity off;' to the nginx config if the
  'enable-modsecurity' annotation is set to false.
- Update tests accordingly

Signed-off-by: Bhavin Gandhi <bhavin7392@gmail.com>
  • Loading branch information
@bhavin192
bhavin192 committed Mar 21, 2020
1 parent 8f4d5f8 commit 1cf22e4
Show file tree
Hide file tree
Showing 4 changed files with 40 additions and 27 deletions.
6 changes: 6 additions & 0 deletions internal/ingress/annotations/modsecurity/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Expand All @@ -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
}
Expand Down Expand Up @@ -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)
Expand Down
24 changes: 12 additions & 12 deletions internal/ingress/annotations/modsecurity/main_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
5 changes: 3 additions & 2 deletions internal/ingress/controller/template/template.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
32 changes: 19 additions & 13 deletions internal/ingress/controller/template/template_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1376,6 +1376,8 @@ func TestModSecurityForLocation(t *testing.T) {
modsecurity_rules_file /etc/nginx/modsecurity/modsecurity.conf;
`

modsecOff := "modsecurity off;"

modsecRule := `modsecurity_rules '
#RULE#
';
Expand All @@ -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,
Expand Down

0 comments on commit 1cf22e4

Please sign in to comment.