IP Whitelists for Frontend (with Docker- & Kubernetes-Provider Support) #1332

MaZderMind · 2017-03-22T20:56:22Z

This PR fixes #816. It adds the ability to specify a List of CIDR Whitelists per Frontend. If Whitelists are configured and the Request-IP does not match one of them, it is aborted with a 403 Forbidden.

This obviously requires traefik to be directly mapped on public ips, if the incoming requests are nat'ed, ie through dockers publish-functionality or docker-compose ports, traefik will only see an ip of the internal network and won't be able to filter according to the source-ip.

This PR adds support for the Docker-Provider and the Kubernetes-Provider, but it should be trivial to extend the support to other Providers, I just have no test-setup for those. For both it parses the Annoatation/Label "traefik.frontend.whitelistSourceRange", for Kubernetes it also accepts the quasi-standard "ingress.kubernetes.io/whitelist-source-range". They can take a Comma-Separated List of CIDR Nets (IPv4 & v6), like this:

machine:
  image: katacoda/docker-http-server
  labels:
    - "traefik.backend=local-only-echo"
    - "traefik.frontend.whitelistSourceRange="10.42.0.0/16, 152.89.1.33/32, afed:be44::/16"
    - "traefik.frontend.rule=Host:local-only-echo.example.com"

or for Kubernetes:

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: hello-minikube
  annotations:
    traefik.frontend.whitelistSourceRange: "10.42.0.0/16, 152.89.1.33/32, afed:be44::/16"
spec:
  rules:
  - host: echo.kube
    http:
      paths:
      - path: /
        backend:
          serviceName: hello-minikube
          servicePort: web

For File-Based Config, there's the whitelistSourceRange property, which takes an array of CIDR Net-Strings:

[frontends]
  [frontends.frontend2]
  backend = "backend1"
  passHostHeader = true
  priority = 10
  whitelistSourceRange = ["10.42.0.0/16", "152.89.1.33/32", "afed:be44::/16"]

The applied Whitelists are Displayed on the Dashboard:

For my own personal use and for easy testing, I pushed a Docker-Image based on this PR:
https://hub.docker.com/r/mazdermind/traefik/tags/

I'd be happy to receive some feedback, because Go is still fairly new.
Best Regards,
Peter

timoreimann · 2017-03-22T21:07:39Z

@MaZderMind thanks for your contribution!

I'll try to review the code. In the meantime, could you please run make fmt and commit your changes in order to please the style gods (and our CI)?

MaZderMind · 2017-03-22T21:08:46Z

@timoreimann Thank you, I'm already at it. Sorry for not checking this before pushing, I'm quite new to the Go Ecosystem

timoreimann · 2017-03-22T21:10:22Z

@MaZderMind no worries. The CI is our friend. :-)

timoreimann · 2017-03-22T21:19:34Z

middlewares/ip_witelister.go

+func NewIPWhitelister(whitelistStrings []string) (*IPWhitelister, error) {
+	whitelister := IPWhitelister{}
+
+	if len(whitelistStrings) < 1 {


Let's simplify and compare against a length of zero.

timoreimann · 2017-03-22T21:20:04Z

middlewares/ip_witelister.go

+	}
+
+	whitelister.whitelists = whitelists
+	whitelister.handler = negroni.HandlerFunc(func(w http.ResponseWriter, r *http.Request, next http.HandlerFunc) {


I suggest we use a dedicated function for the handler func as opposed to defining it inline.

If you define it on the IPWhitelister struct, you gain access to the configured whitelists.

timoreimann · 2017-03-22T21:20:23Z

middlewares/ip_witelister.go

+	whitelister := IPWhitelister{}
+
+	if len(whitelistStrings) < 1 {
+		return nil, fmt.Errorf("Error creating IpWhitelister: no whitelists provided")


Do not capitalize error strings.

Also, no need to mention that this is an error. (After all, we'll be returning an error.) Just say no whitelists provided. It's only at the call site where we should have something like

if err != nil { log.Warnf("failed to initialize white lists: %s", err) }

Similar for other errors.

timoreimann · 2017-03-22T21:21:35Z

middlewares/ip_witelister.go

+	for _, whitelistString := range whitelistStrings {
+		_, whitelist, err := net.ParseCIDR(whitelistString)
+		if err != nil {
+			return nil, fmt.Errorf("Error PArsing CIDR Whitelist %s: %s", whitelist, err)


One extra capital A.

Please decapitalize Whitelist.

timoreimann · 2017-03-22T21:23:09Z

middlewares/ip_witelister.go

+		whitelists = append(whitelists, whitelist)
+	}
+
+	whitelister.whitelists = whitelists


We can append directly into the struct field in line 31 to avoid this assignment.

timoreimann · 2017-03-23T00:17:35Z

provider/kubernetes_test.go

+								Paths: []v1beta1.HTTPIngressPath{
+									{
+										Path: "/ip-source-range",
+										Backend: v1beta1.IngressBackend{


s/ip/whitelist/ for naming consistency.

timoreimann · 2017-03-23T00:22:43Z

server.go

@@ -740,6 +740,17 @@ func (server *Server) loadConfig(configurations configs, globalConfiguration Glo
 								negroni.Use(metricsMiddlewareBackend)
 							}
 						}
+
+						if len(frontend.WhitelistSourceRange) > 0 {
+							log.Info("Configured IP Whitelists: ", frontend.WhitelistSourceRange)


I'd place this log messages only after we are clear there won't be an error returned by NewIPWhitelister.

Please use log.Infof and apply the correct verb (%s I'd think).

timoreimann · 2017-03-23T00:24:08Z

server.go

+							ipSourceRanges := frontend.WhitelistSourceRange
+							ipWhitelistMiddleware, err := middlewares.NewIPWhitelister(ipSourceRanges)
+							if err != nil {
+								log.Fatal("Error creating IP Whitelister: ", err)


Please use log.Fatalf.

timoreimann · 2017-03-23T00:31:33Z

server.go

+							}
+							negroni.Use(ipWhitelistMiddleware)
+						}
+


This part needs to be covered by a test. I know it's tricky because we have zero tests yet, but we need to start somewhere.

For a starter, here are my tests from a different PR.

The Server-Construction-Code is not really nice to be tested, is it? ;)
I extracted the configuration of the middleware into a smaller, easy to test function and tested that. I think this is the better approach instead of configuring a complete Server and trying to fiddle out which middlewares are configured.

timoreimann · 2017-03-23T00:33:56Z

middlewares/ip_witelister.go

+			log.Debugf("Source-IP %s matched none of the %d whitelists, rejecting", remoteIP, len(whitelists))
+			reject(w)
+		}
+	})


The middleware needs testing. Once you have moved the inline function into a dedicated one, you can test it easily against an http/httptest server.

Russell-IO · 2017-03-23T06:53:07Z

This concept is purely awesome

MaZderMind · 2017-03-24T21:52:08Z

@timoreimann Thank you for your in-depth review. It'll take me some time to get through it, though.

errm · 2017-03-25T01:58:48Z

Could we add support for X-Forwarded-For here?

It would make sense for environments were there is some other load balancer in front of Treafik

MaZderMind · 2017-03-26T10:56:18Z

I'm sure we could. How would you see that working? Silently using the foremost Forwarded-For-Header instead of the original IP would AFAIS allow Spoofing any IP when not running behind a Proxy. An extra Annotation would be required to turn this on and off. I'd rather like to get this feature done first and then have another issue/PR for the follow-up discussion.

timoreimann · 2017-03-26T10:59:41Z

I'm with @MaZderMind: Support for the header would be great, but this PR can stand on its own without it.

timoreimann · 2017-04-05T19:02:23Z

hey @MaZderMind, did you have an opportunity yet to address the comments?

I believe the feature you've built is being asked for a few times per week. Would be awesome to have it. :-)

MaZderMind · 2017-04-06T06:20:51Z

Unfortunately i haven't found the time to work on. It's stil on my agenda, though.

MaZderMind · 2017-04-09T22:40:00Z

@timoreimann I addressed all your change requests (despite the one on removing the empty string-slices on which I commented above) and merged the current master into this PR.

MaZderMind · 2017-04-10T09:37:46Z

It seems, the build failed with "The job exceeded the maximum time limit for jobs, and has been terminated.". I'll try to retrigger.

timoreimann

Second round -- things are already looking pretty good. 👍

I need to apologize for a wrong advise I gave in my previous review: While error messages should not start with a capital letter, log messages indeed should (source).

I'm okay with keeping log messages non-capitalized though as our styling is fairly inconsistent anyway.

Sorry about that! 😞

timoreimann · 2017-04-10T19:36:08Z

middlewares/ip_whitelister_test.go

+	"errors"
+	"fmt"
+	"github.com/codegangsta/negroni"
+	"github.com/stretchr/testify/assert"


Please group the non-stdlib imports below the stdlib ones, like here.

timoreimann · 2017-04-10T19:37:56Z

middlewares/ip_whitelister_test.go

+)
+
+func TestNewIPWhitelister(t *testing.T) {
+	cases := map[string]struct {


A map of strings to structs does not seem very idiomatic. Could you please use a simplified slice of structs and have one (the first) field be your test description?

Here's an example.

Same for other tests.

In my mind the tests are grouped by name, just as you would do it when writing them down on paper. You would not have a Headline "1" with the title as Content under it. But I'm ok following your style guide and will change this :)

Just to add a bit more reasoning on my end: I agree with you that a map could make sense if every test had a name. However, the parameter you pass to t.Run() does not have to be static (as in our case) but could also be the result of some "dynamic computation". For instance, the tests for a math utility function could use the input parameters to define the description.

Looking at it from this angle, employing maps where a static name is needed and a slice of structs where it's not would lead to an inconsistent picture. I prefer tests to look boringly similar. :-)

timoreimann · 2017-04-10T19:44:36Z

middlewares/ip_whitelister_test.go

+	}
+
+	for index, e := range cases {
+		t.Run(index, func(t *testing.T) {


You need to say e := e here to capture the range variable. See the Run a group of tests in parallel in the official sub-tests blog post and this Go wiki post (note that t.Parallel() creates goroutines under the hood).

Same for other tests.

By the way, what is e supposed to stand for? I immediately thought of error.

Maybe call it case or tc (for test case) instead?

I copied that from the docker tests, but it is there as wrong as it is here. fixed

timoreimann · 2017-04-10T19:45:54Z

middlewares/ip_whitelister_test.go

+		t.Run(index, func(t *testing.T) {
+			t.Parallel()
+			whitelister, err := NewIPWhitelister(e.whitelistStrings)
+			if !reflect.DeepEqual(err, e.err) {


Error values should be comparable using ==.

timoreimann · 2017-04-10T19:47:13Z

middlewares/ip_whitelister_test.go

+			t.Parallel()
+			whitelister, err := NewIPWhitelister(e.whitelistStrings)
+			if !reflect.DeepEqual(err, e.err) {
+				t.Errorf("expected err: %+v, got: %+v", e.err, err)


One advantage of using sub-tests is that you can use t.Fatal*, and it will only bail out of the current test. So no need to to t.Error* + return.

timoreimann · 2017-04-10T20:17:41Z

middlewares/ip_witelister.go

+	for _, whitelistString := range whitelistStrings {
+		_, whitelist, err := net.ParseCIDR(whitelistString)
+		if err != nil {
+			return nil, fmt.Errorf("parsing CIDR whitelist %s: %+v", whitelist, err)


Error messages should just be formatted using %v or %s (no plus sign) as they are meant for humans.

timoreimann · 2017-04-10T20:25:43Z

provider/docker_test.go

+					Backend:              "backend-test",
+					PassHostHeader:       true,
+					EntryPoints:          []string{},
+					WhitelistSourceRange: []string{},


I'd still remove the empty slice from the test case definition and sanitize inside the test body; that is, check if the value is nil and assign the empty slice to it. That keeps the test cases clean and moves the sanitization logic into a central location.

As far as being able to easily spot what's wrong in case of a test failure, you should use something like go-spew's Sdump. We already use it at other occasions, so you should be able to use it in your tests too.

timoreimann · 2017-04-10T20:26:40Z

server_test.go

+)
+
+func TestNewServerWithoutWhitelistSourceRange(t *testing.T) {
+	cases := map[string]struct {


Convert to struct slices here too please.

timoreimann · 2017-04-10T20:29:27Z

docs/toml.md

@@ -455,6 +455,7 @@ defaultEntryPoints = ["http", "https"]
  backend = "backend1"
  passHostHeader = true
  priority = 10
+  whitelistSourceRange = ["10.42.0.0/16", "152.89.1.33/32", "afed:be44::/16"]


I suppose a bit of extra prose might be nice that explains things like

what does this feature do;

how does it behave (including error cases); and

what providers are supported.

timoreimann · 2017-04-10T20:31:13Z

middlewares/ip_witelister.go

+}
+
+func (whitelister *IPWhitelister) handle(w http.ResponseWriter, r *http.Request, next http.HandlerFunc) {
+	var match *net.IPNet


Unused (except for printing)?

MaZderMind · 2017-04-11T16:10:37Z

@timoreimann I think i changed all the things requested. The build failed with a timeout bit I'm sure retriggering it would make it succeed.

timoreimann · 2017-04-12T22:53:35Z

middlewares/ip_whitelister_test.go

+		desc               string
+		whitelistStrings   []string
+		expectedWhitelists []*net.IPNet
+		err                string


Can we rename this to errMessage? err is usually reserved for variables of type error.

timoreimann · 2017-04-12T22:58:02Z

server_test.go

+			t.Parallel()
+			middleware, err := configureIPWhitelistMiddleware(tc.whitelistStrings)
+
+			if !reflect.DeepEqual(err, tc.err) {


Couldn't we use assert.Equal() here?

timoreimann · 2017-04-12T22:59:41Z

middlewares/ip_whitelister_test.go

+
+			assert.NoError(t, err, "there should be no error")
+			if !assert.NotNil(t, whitelister, "this should not be nil") {
+				return


This and this also seem like they would benefit from require.

timoreimann

Apart from the assert/require question, I have only two minor points left.

I think we're getting close to wrapping up. :-)

timoreimann · 2017-04-12T23:24:04Z

provider/string_util.go

+
+import "strings"
+
+func splitAndTrimString(separatedString string) []string {


I just realize we don't have any dedicated tests for this function. Could you please add some? That way, we guarantee the function does what it should independent of any of its current users.

timoreimann · 2017-05-05T07:46:51Z

Yes, vendor management is awful. :-(

All hopes lie on golang/dep...

MaZderMind · 2017-05-09T15:09:44Z

@timoreimann any news on this? I might do another rebase and merge, but it's not a task I'm very keen on, not knowing if it will be merged then.

timoreimann · 2017-05-10T20:02:59Z

@MaZderMind we need another review for this PR for process reasons, and due to its mere size and impact. I think it's fine to refrain from rebasing until things have been finalized.

Thanks!

emilevauge

Awesome job @MaZderMind ❤️
Few comments, but overall, looks great :)

emilevauge · 2017-05-10T22:09:27Z

middlewares/ip_witelister.go

+func (whitelister *IPWhitelister) handle(w http.ResponseWriter, r *http.Request, next http.HandlerFunc) {
+	remoteIP, err := ipFromRemoteAddr(r.RemoteAddr)
+	if err != nil {
+		log.Warnf("unable to parse remote-address from header: %s - rejecting", r.RemoteAddr)


Shouldn't you use Debugf everywhere (like in https://github.com/containous/traefik/pull/1332/files#diff-251fc09a93a45584efee4df208e9373cR56)?

In my opinion this is a more dangerous problem than the others, because a single wrong syntax IP will stop the complete filter from being applied, which possibly means your private frontend is now exposed to the internet.

Got it 👍

emilevauge · 2017-05-10T22:13:30Z

middlewares/ip_witelister.go

+
+func reject(w http.ResponseWriter) {
+	statusCode := http.StatusForbidden
+	statusText := "request from prohibited source"


Just asking, is it a safe practice to give a reason on this kind of error? I wonder if an empty statusTextwould't be better 🤔

In my opinion, security by obscurity is never good practice. Our service should be save, even if an attacker knows how we secured it.

But is there a need to print this text?

no, not really a need. Other Webservers just repeat the HTTP Status-Code, that would probably be a string like "Forbidden". Generally I think being more verbose is better in case of debugging a problem, but If you want me to change that, I'll do so.

@MaZderMind IMHO, we should not use HTTP error messages for debugging: we already have logs for this :) As we don't set any custom error message in Traefik for now, I would prefer to just remove it. @timoreimann WDYT?

I'm also inclined to say there's no need to give away too much information, so either leave it empty or stick with "Forbidden" -- whatever is more consistent with Traefik's current behavior.

Then, we remove the text :) We will be able to customize the error page in the future.

emilevauge · 2017-05-10T22:15:54Z

server/server.go

@@ -774,6 +782,21 @@ func (server *Server) loadConfig(configurations configs, globalConfiguration Glo
 	return serverEntryPoints, nil
 }

+func configureIPWhitelistMiddleware(whitelistSourceRangs []string) (negroni.Handler, error) {


s/whitelistSourceRangs/whitelistSourceRanges

ouch, will fix that

ldez · 2017-05-18T15:25:49Z

Due to SemaphoreCI, I close and reopen the PR.

emilevauge

Thanks a lot @MaZderMind !
LGTM
(@ldez will change the status text)

MaZderMind · 2017-05-19T12:42:48Z

@emilevauge Sorry for not being responsive with this and previous changes. It's only one small side project for me. Thanks to @ldez for jumping in!

MaZderMind changed the title ~~Ingress ip whitelist~~ IP Whitelists for Frontend (with Docker- & Kuberentes-Provider Support) Mar 22, 2017

timoreimann self-requested a review March 22, 2017 21:07

timoreimann added kind/enhancement a new or improved feature. area/provider/docker area/provider/k8s/ingress status/1-needs-design-review status/2-needs-review labels Mar 22, 2017

timoreimann suggested changes Mar 23, 2017

View reviewed changes

emilevauge added the contributor/waiting-for-corrections label Apr 6, 2017

MaZderMind force-pushed the ingress-ip-whitelist branch from d73554d to 664b496 Compare April 10, 2017 09:41

timoreimann suggested changes Apr 10, 2017

View reviewed changes

MaZderMind force-pushed the ingress-ip-whitelist branch from 664b496 to 746a5e1 Compare April 11, 2017 14:34

timoreimann reviewed Apr 12, 2017

View reviewed changes

timoreimann suggested changes Apr 12, 2017

View reviewed changes

timoreimann reviewed Apr 12, 2017

View reviewed changes

This was referenced Apr 15, 2017

Feature Request: set allowed ips #946

Closed

add ability to accept connections from specific subnet / ip address #623

Closed

MaZderMind force-pushed the ingress-ip-whitelist branch from ed75dbe to 67bbe5a Compare May 9, 2017 13:23

ldez removed the status/2-needs-review label May 10, 2017

emilevauge reviewed May 10, 2017

View reviewed changes

MaZderMind force-pushed the ingress-ip-whitelist branch from 67bbe5a to afbda42 Compare May 11, 2017 06:59

ldez added status/2-needs-review and removed status/1-needs-design-review labels May 12, 2017

emilevauge added this to the 1.4 milestone May 18, 2017

ldez closed this May 18, 2017

ldez reopened this May 18, 2017

Install github.com/stretchr/testify/require.

7f767a4

ldez force-pushed the ingress-ip-whitelist branch from afbda42 to 7a3bf37 Compare May 19, 2017 09:03

ldez added status/3-needs-merge status/2-needs-review and removed status/2-needs-review status/3-needs-merge labels May 19, 2017

emilevauge approved these changes May 19, 2017

View reviewed changes

IP Whitelists for Frontend (with Docker- & Kubernetes-Provider Support)

88fa306

ldez force-pushed the ingress-ip-whitelist branch from 7a3bf37 to 88fa306 Compare May 19, 2017 12:58

ldez added status/3-needs-merge and removed status/2-needs-review labels May 19, 2017

ldez merged commit 5f0b215 into traefik:master May 19, 2017

ldez removed the status/3-needs-merge label May 19, 2017

christopherobin mentioned this pull request Jun 2, 2017

Add whitelist configuration option for entrypoints #1702

Merged

This was referenced Jun 9, 2017

DataDog and StatsD Metrics Support #1701

Merged

glide.lock outdated, I can't install new dep using ./script/glide.sh get some_repo #1734

Closed


		import "strings"

		func splitAndTrimString(separatedString string) []string {

IP Whitelists for Frontend (with Docker- & Kubernetes-Provider Support) #1332

IP Whitelists for Frontend (with Docker- & Kubernetes-Provider Support) #1332

Conversation

MaZderMind commented Mar 22, 2017 • edited by ldez Loading

timoreimann commented Mar 22, 2017

MaZderMind commented Mar 22, 2017

timoreimann commented Mar 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Russell-IO commented Mar 23, 2017

MaZderMind commented Mar 24, 2017

errm commented Mar 25, 2017

MaZderMind commented Mar 26, 2017 via email

timoreimann commented Mar 26, 2017

timoreimann commented Apr 5, 2017

MaZderMind commented Apr 6, 2017 via email

MaZderMind commented Apr 9, 2017

MaZderMind commented Apr 10, 2017

timoreimann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaZderMind commented Apr 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann commented May 5, 2017

MaZderMind commented May 9, 2017

timoreimann commented May 10, 2017 • edited Loading

emilevauge left a comment

Choose a reason for hiding this comment

emilevauge May 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaZderMind May 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann May 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldez commented May 18, 2017

emilevauge left a comment

Choose a reason for hiding this comment

MaZderMind commented May 19, 2017

MaZderMind commented Mar 22, 2017 •

edited by ldez

Loading

timoreimann commented May 10, 2017 •

edited

Loading

emilevauge May 10, 2017 •

edited

Loading

MaZderMind May 11, 2017 •

edited

Loading

timoreimann May 12, 2017 •

edited

Loading