Provide possibility to block IPs, User-Agents and Referers globally #2997

spa-87 · 2018-08-27T16:06:56Z

Current PR implements possibility to block requests globally based on:

Source IP addresses and subnets
User-Agent header
Referer header

It can be useful for many use-cases:

Block unauthorized SEO and vulnerability scanners
An additional way to fight with DDoS
Protect services from other malicious requests

k8s-ci-robot · 2018-08-27T16:06:59Z

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 email the CNCF helpdesk: helpdesk@rt.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.

aledbf · 2018-08-27T16:08:55Z

@spa-87 please add e2e tests

aledbf · 2018-08-27T16:09:06Z

@spa-87 also, please sign the CLA :)

codecov-io · 2018-08-27T16:32:39Z

Codecov Report

Merging #2997 into master will increase coverage by 0.01%.
The diff coverage is 52.63%.

@@            Coverage Diff             @@
##           master    #2997      +/-   ##
==========================================
+ Coverage   47.54%   47.56%   +0.01%     
==========================================
  Files          77       77              
  Lines        5639     5658      +19     
==========================================
+ Hits         2681     2691      +10     
- Misses       2605     2611       +6     
- Partials      353      356       +3

Impacted Files	Coverage Δ
internal/ingress/controller/template/template.go	`64.97% <ø> (ø)`	⬆️
internal/ingress/controller/config/config.go	`98.36% <100%> (+0.05%)`	⬆️
internal/ingress/controller/template/configmap.go	`73.07% <40%> (-4.32%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 355b793...d8b60ce. Read the comment docs.

ElvinEfendi · 2018-08-27T19:45:59Z

All of these are already possible using https://kubernetes.github.io/ingress-nginx/user-guide/nginx-configuration/annotations/#lua-resty-waf specifically nginx.ingress.kubernetes.io/lua-resty-waf-extra-rules.

I know the rule language is not the best but IMO we should not be introducing yet another logic to do the same thing.

UPDATE: spoke too early. lua-resty-waf is only per location, not global.

aledbf · 2018-08-27T20:01:41Z

I know the rule language is not the best but IMO we should not be introducing yet another logic to do the same thing.

While I share this, I think we cannot say learn this new thing to "just" add a deny section. If we don't merge this then we need to provide such examples using the lua waf module

ElvinEfendi · 2018-08-27T20:30:44Z

@aledbf that makes sense. Also updated my PR, I overlooked the fact that this PR is providing a way to block globally.

spa-87 · 2018-08-29T09:38:16Z

Hey @aledbf, e2e tests are added and CLA is signed.

spa-87 · 2018-08-29T09:55:02Z

@ElvinEfendi yep, the main difference of the current PR is to provide a possibility to block requests globally for the whole cluster.
Our use-case: we have one product implemented by multiple components (microservices architecture). Configuration for components (including annotations) is managed by component developers whereas an infrastructure (my) team is responsible for the whole platform. Thus we need a central place to control access for all components running in the cluster. ConfigMap is the best option.
So it isn't just an additional place to block requests, it's a matter of setting boundaries of responsibility.

Globegitter · 2018-08-30T13:47:10Z

@spa-87 We are doing something quite similar and that should already be possible utilizing the global http-snippet and server-snippet configs together. Or is there something I am missing here?

And a further alternative might be the modsecurity integration - do not have too much experience with it but it is on our roadmap to experiment with it a bit more.

antoineco · 2018-08-30T18:54:34Z

test/e2e/settings/global_access_block.go

+			Get(f.IngressController.HTTPURL).
+			Set("Host", host).
+			End()
+		Expect(len(errs)).Should(BeNumerically("==", 0))


This doesn't print the content of the returned error(s), making it hard to troubleshoot from my experience.
Expect(errs).To(BeNil()) does print the content of the errors and is equally valid.

I know we have been abusing this in other tests, but it's never too late to start improving our tests :)

Hey @antoineco , done

spa-87 · 2018-09-05T11:30:36Z

@Globegitter yeah, you're right. I missed those configs initially. But now I'm able to achieve the same behavior using http-snippet and server-snippet parameters. Many thanks!
On another hand, the parameters I proposed, are obvious and easy to use. Thus they can be kept for the convenience.
@aledbf it's your call. Feel free to close the PR unless you want to add these parameters. :)

aledbf · 2018-09-05T12:40:03Z

Feel free to close the PR unless you want to add these parameters. :)

@spa-87 these new parameters makes sense. Let me test this

aledbf · 2018-09-22T21:47:20Z

internal/ingress/controller/config/config.go

@@ -533,12 +533,22 @@ type Configuration struct {

 	// Checksum contains a checksum of the configmap configuration
 	Checksum string `json:"-"`
+
+	// Block all requests from given IPs
+	BlockIps []string `json:"block-ips"`


Please change to

BlockCIDRs []string `json:"block-cidrs"`

aledbf · 2018-09-22T21:49:49Z

internal/ingress/controller/config/config.go

+	BlockIps []string `json:"block-ips"`
+
+	// Block all requests with given User-Agent headers
+	BlockUas []string `json:"block-user-agents"`


BlockUserAgents

aledbf · 2018-09-22T21:50:01Z

internal/ingress/controller/config/config.go

+	BlockUas []string `json:"block-user-agents"`
+
+	// Block all requests with given Referer headers
+	BlockRefs []string `json:"block-referers"`


BlockReferers

aledbf · 2018-09-22T21:50:59Z

@spa-87 first, apologies for the delay. Just some comments about the naming. If you change that we are ok to merge

spa-87 · 2018-09-25T12:44:51Z

Hey @aledbf, no problem with the delay :)
The parameters are renamed according to your recommendations.

aledbf · 2018-09-25T12:49:20Z

/lgtm
/approve

aledbf · 2018-09-25T12:49:26Z

@spa-87 thanks!

k8s-ci-robot · 2018-09-25T12:49:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, spa-87

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~~ [aledbf]

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

dz0ny · 2018-10-19T20:02:08Z

docs/user-guide/nginx-configuration/configmap.md

+
+## block-referers
+
+A comma-separated list of Referers, requestst from which have to be blocked globally.


Suggested change

A comma-separated list of Referers, requestst from which have to be blocked globally.

A comma-separated list of Referers, requests from which have to be blocked globally.

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 27, 2018

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 Aug 29, 2018

antoineco reviewed Aug 30, 2018

View reviewed changes

aledbf reviewed Sep 22, 2018

View reviewed changes

Provide possibility to block CIDRs, User-Agents and Referers globally

7212d00

k8s-ci-robot assigned aledbf Sep 25, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 25, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 25, 2018

k8s-ci-robot merged commit 6393ca6 into kubernetes:master Sep 25, 2018

dz0ny reviewed Oct 19, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide possibility to block IPs, User-Agents and Referers globally #2997

Provide possibility to block IPs, User-Agents and Referers globally #2997

spa-87 commented Aug 27, 2018

k8s-ci-robot commented Aug 27, 2018

aledbf commented Aug 27, 2018

aledbf commented Aug 27, 2018

codecov-io commented Aug 27, 2018 •

edited

Loading

ElvinEfendi commented Aug 27, 2018 •

edited

Loading

aledbf commented Aug 27, 2018 •

edited

Loading

ElvinEfendi commented Aug 27, 2018

spa-87 commented Aug 29, 2018

spa-87 commented Aug 29, 2018

Globegitter commented Aug 30, 2018 •

edited

Loading

antoineco Aug 30, 2018 •

edited

Loading

spa-87 Sep 5, 2018

spa-87 commented Sep 5, 2018

aledbf commented Sep 5, 2018

aledbf Sep 22, 2018 •

edited

Loading

aledbf Sep 22, 2018

aledbf Sep 22, 2018

aledbf commented Sep 22, 2018

spa-87 commented Sep 25, 2018

aledbf commented Sep 25, 2018

aledbf commented Sep 25, 2018

k8s-ci-robot commented Sep 25, 2018

dz0ny Oct 19, 2018


		## block-referers

		A comma-separated list of Referers, requestst from which have to be blocked globally.

Provide possibility to block IPs, User-Agents and Referers globally #2997

Provide possibility to block IPs, User-Agents and Referers globally #2997

Conversation

spa-87 commented Aug 27, 2018

k8s-ci-robot commented Aug 27, 2018

aledbf commented Aug 27, 2018

aledbf commented Aug 27, 2018

codecov-io commented Aug 27, 2018 • edited Loading

Codecov Report

ElvinEfendi commented Aug 27, 2018 • edited Loading

aledbf commented Aug 27, 2018 • edited Loading

ElvinEfendi commented Aug 27, 2018

spa-87 commented Aug 29, 2018

spa-87 commented Aug 29, 2018

Globegitter commented Aug 30, 2018 • edited Loading

antoineco Aug 30, 2018 • edited Loading

Choose a reason for hiding this comment

spa-87 Sep 5, 2018

Choose a reason for hiding this comment

spa-87 commented Sep 5, 2018

aledbf commented Sep 5, 2018

aledbf Sep 22, 2018 • edited Loading

Choose a reason for hiding this comment

aledbf Sep 22, 2018

Choose a reason for hiding this comment

aledbf Sep 22, 2018

Choose a reason for hiding this comment

aledbf commented Sep 22, 2018

spa-87 commented Sep 25, 2018

aledbf commented Sep 25, 2018

aledbf commented Sep 25, 2018

k8s-ci-robot commented Sep 25, 2018

dz0ny Oct 19, 2018

Choose a reason for hiding this comment

codecov-io commented Aug 27, 2018 •

edited

Loading

ElvinEfendi commented Aug 27, 2018 •

edited

Loading

aledbf commented Aug 27, 2018 •

edited

Loading

Globegitter commented Aug 30, 2018 •

edited

Loading

antoineco Aug 30, 2018 •

edited

Loading

aledbf Sep 22, 2018 •

edited

Loading