Disable default modsecurity_rules_file if modsecurity-snippet is specified #8021

besha100 · 2021-12-08T12:48:12Z

The default modsecurity_rules_file (/etc/nginx/modsecurity/modsecurity.conf;) has settings that override the ModSecurity-snippet if it is specified with custom config settings like "SecAuditLog", "SecAuditLogStorageDir" and "SecRuleEngine On".

What this PR does / why we need it:

To disable the default modsecurity_rules_file (/etc/nginx/modsecurity/modsecurity.conf;) if custom modsecurity-snippet is specified

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation only

Which issue/s this PR fixes

6307
6438
4640

How Has This Been Tested?

In the values yaml file have the config below
config:
enable-modsecurity: "true"
enable-owasp-modsecurity-crs: "false"
modsecurity-snippet: |-
SecRuleEngine On
SecAuditEngine RelevantOnly
SecAuditLogParts ABIJDEFHZ
SecAuditLogRelevantStatus "^(?:05912300)"
SecAuditLogType Concurrent
SecAuditLog /var/tmp/modsec_audit.log
SecAuditLogStorageDir /var/tmp/

Then deploying the latest nginx controller
helm upgrade --install --values your-values-with-config-above.yaml your-release-name ingress-nginx/ingress-nginx --version 4.0.13

After you deploy, exec into one nginx pod and browse " /etc/nginx/nginx.conf ". You will find after your modsecurity custom config the below line is included

  modsecurity_rules_file /etc/nginx/modsecurity/modsecurity.conf;

Browsing this file, you will find some settings that conflict with the settings in your "modsecurity-snippet: | " such as:

    SecRuleEngine DetectionOnly
    SecAuditLog /var/log/modsec_audit.log
    SecAuditLogStorageDir /var/log/audit/

Verification:
make some suspicious traffic to your nginx ingress such as

curl http://your-url/index.html\?exec\=/bin/bash

Then when you exec into the nginx pod, you will see the logs are in "/var/log/modsec_audit.log" (from the default modsecurity file) instead of the customized configured one "/var/tmp/modsec_audit.log" in the (modsecurity-snippet)

This PR will not have this include file " modsecurity_rules_file /etc/nginx/modsecurity/modsecurity.conf" in the nginx configuration file. After deploying this PR, all the custom modsecurity-snippet will take effect and not be overridden.
Then if you do suspicious traffic to your nginx ingress like the one above,t he audit log will be /var/tmp/modsec_audit.log , and SecRuleEngine On will be on the blocking mode according to the rules in your modsecurity-snippet

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I've read the CONTRIBUTION guide
I have added tests to cover my changes.
All new and existing tests passed.

…cifed The default modsecurity_rules_file overwrites the ModSecurity-snippet if it is specified with custom config settings like "SecRuleEngine On". This will not let Modsecurity be in blocking mode even if "SecRuleEngine On" is specified in the ModSecurity-snippet configuration

linux-foundation-easycla · 2021-12-08T12:48:15Z

The committers are authorized under a signed CLA.

✅ besha100 (f9b6e70)

k8s-ci-robot · 2021-12-08T12:48:16Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot · 2021-12-08T12:48:21Z

Welcome @besha100!

It looks like this is your first PR to kubernetes/ingress-nginx 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/ingress-nginx has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2021-12-08T12:48:21Z

Hi @besha100. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

besha100 · 2021-12-08T13:03:02Z

/assign @thockin

iamNoah1

#6307

I find it strange that you add a commented line that does nothing if modsecurity snippet is speficied and only enable modsecuritx rules file in the else case. IMHO it would be more straight forward if we would have only the if case where we add the rules file if snippet is empty and have no else case at all.

Also I would expect some tests that cover this change.

iamNoah1 · 2021-12-08T18:19:35Z

/triage accepted
/priority important-longterm
/kind bug

Only have the default Modsecurity conf settings in case Modsecurity configuration snippet is not present and remove unnecessary comments

besha100 · 2021-12-16T08:12:54Z

/test pull-ingress-nginx-test

besha100 · 2021-12-16T09:54:39Z

@iamNoah1 I need help here. All e2e tests have passed here .
but in the build log this one failed and couldn't figure out why? can you please help? it doesn't look like one of my changes

iamNoah1 · 2021-12-16T09:57:19Z

/retest-required

iamNoah1 · 2021-12-16T11:53:15Z

internal/ingress/controller/template/template_test.go

@@ -1781,8 +1781,8 @@ func TestModSecurityForLocation(t *testing.T) {
 		{"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, "", "", fmt.Sprintf("%v%v", loadModule, modSecCfg)},
 		{"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%v", loadModule, modsecRule, modSecCfg)},
-		{"configmap disabled, annotation enabled, OWASP enabled", false, false, true, true, false, testRule, "", fmt.Sprintf("%v%v%v", loadModule, modsecRule, modSecCfg)},
+		{"configmap disabled, annotation enabled, OWASP disabled", false, false, true, true, false, testRule, "", fmt.Sprintf("%v%v%v", loadModule, modsecRule)},


Hm, I guess this could be a problem. Why did you remove it?

@iamNoah1 I removed only modSecCfg which is (modsecurity_rules_file /etc/nginx/modsecurity/modsecurity.conf) line. Becasue it shouldn't exist with modsecRule which is (modsecurity_rules).

Now, I reverted this change and now you can see the error is changed and now it is
"
=== RUN TestModSecurityForLocation
template_test.go:1805: configmap disabled, annotation enabled, OWASP disabled: expected 'modsecurity on;
modsecurity_rules '
#RULE#
';
modsecurity_rules_file /etc/nginx/modsecurity/modsecurity.conf;
' but returned 'modsecurity on;
modsecurity_rules '
#RULE#
';
'
template_test.go:1805: configmap disabled, annotation enabled, OWASP enabled: expected 'modsecurity on;
modsecurity_rules '
#RULE#
';
modsecurity_rules_file /etc/nginx/modsecurity/modsecurity.conf;
' but returned 'modsecurity on;
modsecurity_rules '
#RULE#
';
'
--- FAIL: TestModSecurityForLocation (0.00s)

"

These are the two tests that I have removed modSecCfg in the existence of modsecRule.
Can you advise on this.
Thank you:)

ok, then probably I was wrong and your initial change was correct. Anyways I am not that familiar with the Code and will try to drag some more experienced devs in.

Ok I think I found it:

k8s.io/ingress-nginx/internal/ingress/controller/template internal/ingress/controller/template/template_test.go:1784:109: Sprintf format %v reads arg #3, but call has 2 args internal/ingress/controller/template/template_test.go:1785:108: Sprintf format %v reads arg #3, but call has 2 args

---> seems that your initial change was correct but you forgott to adjust the %vs.

…late" This reverts commit 9d38eca.

iamNoah1

see my last comment

besha100 · 2021-12-16T19:14:04Z

@iamNoah1 MAN, YOU ARE THE BEST. I fixed it and now all tests have passed :)
Thank you

besha100 · 2021-12-17T08:03:51Z

@rikatz Can you please review and approve

iamNoah1

/lgtm
As far as I can say

rikatz · 2021-12-17T12:29:07Z

hey, didn't read everything right now (sorry), but a question: I understand this is a bug, but can this also be a breaking change for folks already relying on modsecurity and this behavior?

Thanks!

iamNoah1 · 2021-12-17T17:27:44Z

@rikatz @besha100 As far as I understand and I may be wrong, the behavior is buggy at the moment. People may have implemented a workaround in order to get the expected behavior. But I don't think that this change will breaks those peoples workaround. @besha100 can you please check that.

besha100 · 2021-12-17T21:59:19Z

@rikatz @iamNoah1 I will clarify who, what and how this will affect the folks who use modsecurity.
There are 3 ways different setup/behavior for ModSecurity:

1- Default Configuration: Here Modsecurity is enabled by setting "enable-modsecurity: true". With this ModSecurity will run in “Detection-Only” mode using the default configuration. This PR is not affecting this way, as the line (modsecurity_rules_file /etc/nginx/modsecurity/modsecurity.conf;) will exist in the nginx config.

2- OWASP Core Rule Set: Here Modsecurity is enabled by "enable-modsecurity: true" and "enable-owasp-core-rules: true". Here usually modsecurity-snippet is not specified and thus the change will not affect it. But even if it is specified, there is already a logic to include this file "modsecurity_rules_file /etc/nginx/owasp-modsecurity-crs/nginx-modsecurity.conf;" as configuration/rule file, as this file includes some other rule files and a config file (crs-setup.conf). Thus, This PR is also not affecting this way.

3- Snippet: Here modsecurity is enabled by "enable-modsecurity: true" and "modsecurity-snippet: |-". This PR is only affecting this way of implementing Modsecurity. But this way is currently working as the first approach and not as it meant, because the custom settings you specify under "modsecurity-snippet: |-". will be overridden by the default config file.
This PR will make Modsecurity with the exact config you specify under snippet, but here folks are expected to have their own settings or including/referring to rules files because the default ModSecurity file rules will not be used.

@iamNoah1 you are right:) Here people like these had to mount a volume in the controller container to override the default config in "/etc/nginx/modsecurity/modsecurity.conf". and I agree with you that those who have such workaround solutions won't be affected as they already include custom config/rule files under "modsecurity-snippet: |-".
With this change implemented, Even if someone just want the default modsecurity config, they can include it under "modsecurity-snippet: |-".

To summarize, This PR affects only approcah 3 and approach 3 is currently not working as expected and rather working like approach 1. Folks who want to use approach 3 need to explicitly write the settings/rules or include files under "modsecurity-snippet: |-" that has the custom settings and rules they need. So, I believe this shouldn't be a breaking change for anyone.

PuneeshMotwani · 2021-12-22T11:59:34Z

@rikatz - can you please review and approve, We have been waiting for this change since a while.

Good Work @besha100 in getting this out of the door. 🥇

besha100 · 2021-12-22T22:43:54Z

@rikatz Have you managed to look into this and my answer above?

rikatz · 2021-12-23T11:29:43Z

/approve
Thanks for the explanation and the PR :) happy holidays

k8s-ci-robot · 2021-12-23T11:30:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: besha100, iamNoah1, rikatz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rikatz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…ified (kubernetes#8021) * Disabled default modsecurity_rules_file if modsecurity-snippet is specifed The default modsecurity_rules_file overwrites the ModSecurity-snippet if it is specified with custom config settings like "SecRuleEngine On". This will not let Modsecurity be in blocking mode even if "SecRuleEngine On" is specified in the ModSecurity-snippet configuration * Remove unnecessary comments Only have the default Modsecurity conf settings in case Modsecurity configuration snippet is not present and remove unnecessary comments * Fixed modsecurity default file only if Modsecurity snippet present Fixed if condition Modsecurity snippet present have modsecurity default config file * Added e2e test to disabling modsecurity conf Added e2e in case modsecurity-snippet enabled to disable settings in default modsecurity.conf * Validate writing to a different location Validate also modsecurity to write to a different location instead of the default directory * Fixed the formatting * Fixed if empty ModsecuritySnippet * Fixed ModsecuritySnippet condition * Fixed the condition also in ingress controller template * Removed the default config condition in ingress controller template * Fixed the default config condition in ingress controller template * Fixed pull-ingress-nginx-test * Revert "Fixed the default config condition in ingress controller template" This reverts commit 9d38eca. * Revert template_test * Adjusted the formating %v

k8s-ci-robot added needs-priority size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 8, 2021

k8s-ci-robot requested review from strongjz and tao12345666333 December 8, 2021 12:48

k8s-ci-robot assigned thockin Dec 8, 2021

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Dec 8, 2021

This was referenced Dec 8, 2021

Modesecurity works in "DetectionOnly" mode even if I specify SecRuleEngine to 'On' in my configMap #6307

Closed

[Question]: nginx location configuration owasp-modsecurity/ModSecurity-nginx#183

Closed

Adding modsecurity-snippet to configmap #4902

Closed

iamNoah1 suggested changes Dec 8, 2021

View reviewed changes

Remove unnecessary comments

bcda35e

Only have the default Modsecurity conf settings in case Modsecurity configuration snippet is not present and remove unnecessary comments

besha100 mentioned this pull request Dec 8, 2021

Unable to send modsecurity logs to stdout or different directory #6438

Closed

besha100 requested a review from iamNoah1 December 16, 2021 09:54

iamNoah1 reviewed Dec 16, 2021

View reviewed changes

besha100 added 2 commits December 16, 2021 13:19

Revert "Fixed the default config condition in ingress controller temp…

c60b207

…late" This reverts commit 9d38eca.

Revert template_test

00f8dff

besha100 requested a review from iamNoah1 December 16, 2021 13:17

iamNoah1 suggested changes Dec 16, 2021

View reviewed changes

Adjusted the formating %v

54263d0

besha100 requested a review from iamNoah1 December 16, 2021 19:14

iamNoah1 approved these changes Dec 17, 2021

View reviewed changes

k8s-ci-robot assigned iamNoah1 Dec 17, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 17, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 23, 2021

k8s-ci-robot merged commit fea7fed into kubernetes:main Dec 23, 2021

rikatz added this to the v1.1.1 milestone Jan 9, 2022

damienleger mentioned this pull request Mar 24, 2022

Misleading & clumsy ModSecurity doc #8388

Open

mac-chaffee mentioned this pull request Apr 11, 2022

Question about conflicting modsecurity_rules/modsecurity_rules_file precedence owasp-modsecurity/ModSecurity-nginx#280

Open

vijay-veeranki mentioned this pull request May 19, 2022

Investigate duplicate rule error when switching modsec ingress controller ministryofjustice/cloud-platform#3807

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable default modsecurity_rules_file if modsecurity-snippet is specified #8021

Disable default modsecurity_rules_file if modsecurity-snippet is specified #8021

besha100 commented Dec 8, 2021 •

edited

Loading

linux-foundation-easycla bot commented Dec 8, 2021 •

edited

Loading

k8s-ci-robot commented Dec 8, 2021

k8s-ci-robot commented Dec 8, 2021

k8s-ci-robot commented Dec 8, 2021

besha100 commented Dec 8, 2021

iamNoah1 left a comment

iamNoah1 commented Dec 8, 2021

besha100 commented Dec 16, 2021

besha100 commented Dec 16, 2021

iamNoah1 commented Dec 16, 2021

iamNoah1 Dec 16, 2021

besha100 Dec 16, 2021

iamNoah1 Dec 16, 2021

iamNoah1 Dec 16, 2021 •

edited

Loading

iamNoah1 left a comment

besha100 commented Dec 16, 2021

besha100 commented Dec 17, 2021

iamNoah1 left a comment

rikatz commented Dec 17, 2021

iamNoah1 commented Dec 17, 2021

besha100 commented Dec 17, 2021 •

edited

Loading

PuneeshMotwani commented Dec 22, 2021

besha100 commented Dec 22, 2021

rikatz commented Dec 23, 2021

k8s-ci-robot commented Dec 23, 2021

Disable default modsecurity_rules_file if modsecurity-snippet is specified #8021

Disable default modsecurity_rules_file if modsecurity-snippet is specified #8021

Conversation

besha100 commented Dec 8, 2021 • edited Loading

What this PR does / why we need it:

Types of changes

Which issue/s this PR fixes

How Has This Been Tested?

Checklist:

linux-foundation-easycla bot commented Dec 8, 2021 • edited Loading

k8s-ci-robot commented Dec 8, 2021

k8s-ci-robot commented Dec 8, 2021

k8s-ci-robot commented Dec 8, 2021

besha100 commented Dec 8, 2021

iamNoah1 left a comment

Choose a reason for hiding this comment

iamNoah1 commented Dec 8, 2021

besha100 commented Dec 16, 2021

besha100 commented Dec 16, 2021

iamNoah1 commented Dec 16, 2021

iamNoah1 Dec 16, 2021

Choose a reason for hiding this comment

besha100 Dec 16, 2021

Choose a reason for hiding this comment

iamNoah1 Dec 16, 2021

Choose a reason for hiding this comment

iamNoah1 Dec 16, 2021 • edited Loading

Choose a reason for hiding this comment

iamNoah1 left a comment

Choose a reason for hiding this comment

besha100 commented Dec 16, 2021

besha100 commented Dec 17, 2021

iamNoah1 left a comment

Choose a reason for hiding this comment

rikatz commented Dec 17, 2021

iamNoah1 commented Dec 17, 2021

besha100 commented Dec 17, 2021 • edited Loading

PuneeshMotwani commented Dec 22, 2021

besha100 commented Dec 22, 2021

rikatz commented Dec 23, 2021

k8s-ci-robot commented Dec 23, 2021

besha100 commented Dec 8, 2021 •

edited

Loading

linux-foundation-easycla bot commented Dec 8, 2021 •

edited

Loading

iamNoah1 Dec 16, 2021 •

edited

Loading

besha100 commented Dec 17, 2021 •

edited

Loading