Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Route security management by end user #14536

Merged
merged 1 commit into from
Jun 15, 2017

Conversation

pecameron
Copy link
Contributor

@pecameron pecameron commented Jun 8, 2017

Add a new route annotation "haproxy.router.openshift.io/ip_whitelist"
that specifies a space separated list of white listed source IP
addresses and/or CIDRs. Requests from IP addresses that are not in
the whitelist are dropped.

When the annotation is present for a route a acl is set up in the
backend with the whitelist.

This PR addresses issue #13709

Some examples:

When editing a route add the following annotation to define the desired
source ip's.

  1. allow only one ip

haproxy.router.openshift.io/whitelist:
192.168.1.10

  1. several ip's

haproxy.router.openshift.io/whitelist:
192.168.1.10 192.168.1.11 192.168.1.12

  1. ip ranges

haproxy.router.openshift.io/whitelist:
192.168.1.0/24

  1. ip's and ranges

haproxy.router.openshift.io/whitelist:
180.5.61.153 192.168.1.0/24 10.0.0.0/8

Trello: TbZPhHKE Route security management by end user
https://trello.com/c/TbZPhHKE/

Bug: 1426562
https://bugzilla.redhat.com/show_bug.cgi?id=1426562

This was ported from PR 13729

Committer: [email protected]
Author: [email protected]

@pecameron
Copy link
Contributor Author

@openshift/networking @knobunc @jorgemoralespou PTAL
@fbac

@jorgemoralespou
Copy link

@fbac Take a look at this PR

@fbac
Copy link
Contributor

fbac commented Jun 9, 2017

Great work, thank guys! 👍

Copy link
Contributor

@knobunc knobunc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, woke up this morning with some more thoughts about this. Just minor stuff. The general thrust is good.

@@ -6,6 +6,9 @@
{{- define "/var/lib/haproxy/conf/haproxy.config" }}
{{- $workingDir := .WorkingDir }}
{{- $defaultDestinationCA := .DefaultDestinationCA }}
{{- $cidrPattern := "(([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\\.){3}([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])(/([0-9]|[1-2][0-9]|3[0-2]))?" -}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could make this a hair less ugly by having:

{{- $quadPattern := "(?:[0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])" -}}
{{- $ipPattern := printf "(?:%s\.%s\.%s\.%s)" $quadPattern $quadPattern $quadPattern $quadPattern -}}
{{- $cidrPattern := printf "(?:%s(?:/(?:[0-9]|[1-2][0-9]|3[0-2]))?)" $ipPattern -}}

I also think we should wrap all named regexes in ()s (as I did above) so that they are more easily composed into other regexes without the alternations getting messed up. (i.e. if I have $foo := "a|b"; then that is risky to use in a subsequent regex.

Also all regexes should not be using capturing ()s. We should say (?:) instead.

Finally, all regexes should have a comment explaining what they match, with a sample match.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@@ -6,6 +6,9 @@
{{- define "/var/lib/haproxy/conf/haproxy.config" }}
{{- $workingDir := .WorkingDir }}
{{- $defaultDestinationCA := .DefaultDestinationCA }}
{{- $cidrPattern := "(([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\\.){3}([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])(/([0-9]|[1-2][0-9]|3[0-2]))?" -}}
{{- $cidrListPattern := printf "%s( +(%s))*" $cidrPattern $cidrPattern -}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the above rules, this should be:
{{- $cidrListPattern := printf "(?:%s(?: +%s)*)" $cidrPattern $cidrPattern -}}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

@pecameron
Copy link
Contributor Author

@knobunc is this what you are looking for? PTAL

Copy link
Contributor

@knobunc knobunc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I'd really like to see some tests... but I'm not sure how


{{/* A bunch of regular expressions. Each should be wrapped in (?:) so that it is safe to include bare */}}
{{/* quadPattern: Match a quad in an IP address; e.g. 123 */}}
{{- $quadPattern := "(?:[0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])" -}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better. But can we flip all of these regexes to be enclosed in backticks, that way we can lose any double \ stuff. Do it to all of the ones you are adding just for consistency.

Add a new route annotation "haproxy.router.openshift.io/ip_whitelist"
that specifies a space separated list of white listed source IP
addresses and/or CIDRs. Requests from IP addresses that are not in
the whitelist are dropped.

When the annotation is present for a route a acl is set up in the
backend with the whitelist.

This PR addresses issue openshift#13709

Some examples:

When editing a route add the following annotation to define the desired
source ip's.

1) allow only one ip

haproxy.router.openshift.io/whitelist:
192.168.1.10

2) several ip's

haproxy.router.openshift.io/whitelist:
192.168.1.10 192.168.1.11 192.168.1.12

3) ip ranges

haproxy.router.openshift.io/whitelist:
192.168.1.0/24

4) ip's and ranges

haproxy.router.openshift.io/whitelist:
180.5.61.153 192.168.1.0/24 10.0.0.0/8

Trello: TbZPhHKE Route security management by end user
https://trello.com/c/TbZPhHKE/

Bug: 1426562
https://bugzilla.redhat.com/show_bug.cgi?id=1426562

Committer: [email protected]
Author: [email protected]
@pecameron
Copy link
Contributor Author

pecameron commented Jun 12, 2017

@knobunc I put in the `, is this ready to merge?

@knobunc knobunc requested a review from rajatchopra June 12, 2017 14:27
@knobunc
Copy link
Contributor

knobunc commented Jun 12, 2017

@pecameron I'd like someone else to weigh in. I flagged @rajatchopra for review too.

Copy link

@pravisankar pravisankar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes looks good to me.

@knobunc
Copy link
Contributor

knobunc commented Jun 14, 2017

[merge]

@openshift-bot
Copy link
Contributor

[Test]ing while waiting on the merge queue

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 7c4719f

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2229/) (Base Commit: ee9f436)

@pecameron
Copy link
Contributor Author

@knobunc
Copy link
Contributor

knobunc commented Jun 15, 2017

[merge][severity: bug]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 7c4719f

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge FAILURE (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/1010/) (Base Commit: 98b2737) (PR Branch Commit: 7c4719f) (Extended Tests: bug)

@dobbymoodge
Copy link
Contributor

False merge failure, merging anyway

@dobbymoodge dobbymoodge merged commit 34764f7 into openshift:master Jun 15, 2017
@pecameron pecameron deleted the bz1426562 branch June 19, 2017 15:07
pecameron added a commit to pecameron/openshift-docs that referenced this pull request Jun 19, 2017
Openshift 3.6

Add a new route annotation "haproxy.router.openshift.io/ip_whitelist"
that specifies a space separated list of white listed source IP
addresses and/or CIDRs. Requests from IP addresses that are not in
the whitelist are dropped.

origin PR 14536
openshift/origin#14536

Trello: TbZPhHKE Route security management by end user
https://trello.com/c/TbZPhHKE/

Bug: 1426562
https://bugzilla.redhat.com/show_bug.cgi?id=1426562
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants