Ability to use "X-Forwarded-For" as a source of IP for white list. #3070

ldez · 2018-03-22T20:05:19Z

What does this PR do?

Adds the ability to use X-Forwarded-For as a source of IP for white list.

Motivation

Fixes #2942

More

Added/updated tests
Added/updated documentation

Additional Notes

Related to #2260

mmatur

Nice job @ldez 👏
First review with few comments :)

mmatur · 2018-03-23T06:31:43Z

docs/configuration/backends/marathon.md

-| `traefik.frontend.rateLimit.rateSet.<name>.period=6`       | See [rate limiting](/configuration/commons/#rate-limiting) section.                                                                                                                                               |
-| `traefik.frontend.rateLimit.rateSet.<name>.average=6`      | See [rate limiting](/configuration/commons/#rate-limiting) section.                                                                                                                                               |
-| `traefik.frontend.rateLimit.rateSet.<name>.burst=6`        | See [rate limiting](/configuration/commons/#rate-limiting) section.                                                                                                                                               |
+| `traefik.frontend.rateLimit.rateSet.<name>.period=6`       | See [rate limiting](/configuration/commons/#rate-limiting) section.                                                                                                                                                    |


Why the order changed between traefik.frontend.rateLimit.rateSet.<name>.period and traefik.frontend.rateLimit.extractorFunc ?

mmatur · 2018-03-23T06:39:30Z

server/header_rewriter.go

@@ -12,7 +11,7 @@ import (

 // NewHeaderRewriter Create a header rewriter
 func NewHeaderRewriter(trustedIPs []string, insecure bool) (forward.ReqRewriter, error) {
-	IPs, err := whitelist.NewIP(trustedIPs, insecure)
+	IPs, err := whitelist.NewIP(trustedIPs, insecure, true) // FIXME


// FIXME ?

mmatur · 2018-03-23T06:41:05Z

server/server_test.go

+			errMessage:           "parsing CIDR whitelist [foo]: parsing CIDR white list <nil>: invalid CIDR address: foo",
+		},
+		{
+			desc: "whitelists configured )",


whitelists configured instead of whitelists configured )

emilevauge

Impressive job @ldez 👏
I think we definitely need a user guide (in the future) on chaining Traefik with another LB that would also explain X-Forwarded-For and white listing.

emilevauge · 2018-03-23T13:05:12Z

whitelist/ip.go

+}
+
+// contains checks if provided address is in the white list
+func (ip *IP) contains(addr string) (bool, net.IP, error) {


Is the returned bool still needed ?

yes due to insecure management.

Sorry, I misunderstood the code initially.

mmatur

Few comments

mmatur · 2018-03-23T13:18:03Z

docs/configuration/backends/file.md

@@ -54,7 +54,10 @@ Træfik can be configured with a file.
      "test:$apr1$H6uskkkW$IgXLP6ewTrSuBkTrqE8wj/",
      "test2:$apr1$d9hr9HBB$4HxwgUir3HP4EsggP/QNo0",
    ]
-    whitelistSourceRange = ["10.42.0.0/16", "152.89.1.33/32", "afed:be44::/16"]
+
+    [frontends.frontend2.whiteList]


I think that it should be frontend1 instead of frontend2

mmatur · 2018-03-23T13:25:28Z

docs/configuration/entrypoints.md

@@ -112,7 +115,8 @@ Redirect.Regex:http://localhost/(.*)
 Redirect.Replacement:http://mydomain/$1
 Redirect.Permanent:true
 Compress:true
-WhiteListSourceRange:10.42.0.0/16,152.89.1.33/32,afed:be44::/16
+WhiteList.SourceRange:10.42.0.0/16,152.89.1.33/32,afed:be44::/16
+WhiteList.useXForwardedFor:true


Should be WhiteList.UseXForwardedFor instead of WhiteList.useXForwardedFor

juliens

LGTM

mmatur

LGTM 👏

emilevauge

LGTM

nemosupremo · 2018-07-13T03:23:38Z

I was looking at this PR as it reflected some functionality we had implemented in a fork, and as it's implemented here it doesn't seem secure (or at least has a large foot gun).

As I understand from this test:

+		{
+			desc:                "allow UseXForwardedFor, remoteAddr not in range, UseXForwardedFor in range",
+			whiteList:           []string{"1.2.3.4/24"},
+			allowXForwardedFor:  true,
+			remoteAddr:          "10.2.3.1:123",
+			xForwardedForValues: []string{"1.2.3.1", "10.2.3.1"},
+			expected:            true,
+		},

If my whitelist is set to 1.2.3.4/24 and the 'attacker' has a remote IP of 10.2.3.1, he can bypass my whitelist by just setting the appropriate X-Forwarded-For header. This seems like a dangerous feature as it means if your the attacker knows your whitelist (which will probably likely be something like 10.0.0.0/8, 172.16.0.0/12, or 192.168.0.0/16), he can essentially access your service from wherever.

What nginx does, which I think it safer, is it only allows you to pass the whitelist if all the IPs pass the whitelist. So in this case, nginx would fail because the remoteAddr is not in the whitelist, so it doesn't trust the X-Forwarded-For header. It also takes into the account the order of the IP's in X-Forwarded-For from right to left. It essentially uses the left-most non-whitelisted IP as the "real IP".

ldez added kind/enhancement a new or improved feature. status/2-needs-review area/middleware labels Mar 22, 2018

ldez added this to the 1.6 milestone Mar 22, 2018

ldez requested review from a team as code owners March 22, 2018 20:05

traefiker added area/provider/consul area/provider/consulcatalog area/provider/docker area/provider/ecs area/provider/k8s/ingress area/provider/marathon area/provider/mesos area/provider/rancher size/L labels Mar 22, 2018

ldez added area/provider/kv and removed area/provider/consul labels Mar 22, 2018

ldez force-pushed the feat/whitelist-xff branch 2 times, most recently from 5ed4797 to 5c3c220 Compare March 22, 2018 20:28

mmatur requested changes Mar 23, 2018

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Mar 23, 2018

ldez removed the contributor/waiting-for-corrections label Mar 23, 2018

ldez added 4 commits March 23, 2018 13:43

feat: support X-Forwarded-For.

6b57157

feat: add labels support.

fe94711

doc: update documentation.

032b17a

review: comments.

f535250

ldez force-pushed the feat/whitelist-xff branch from 3091863 to f535250 Compare March 23, 2018 12:53

feat: add DOcker support.

d654e32

doc: docker.

d48526b

emilevauge requested changes Mar 23, 2018

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Mar 23, 2018

mmatur requested changes Mar 23, 2018

View reviewed changes

ldez added 2 commits March 23, 2018 14:55

review: doc and deprecation.

413ef3c

refactor: rename for SF compatibility.

857e42b

ldez force-pushed the feat/whitelist-xff branch from 83f9a65 to 857e42b Compare March 23, 2018 14:39

review: if.

a8305a1

juliens approved these changes Mar 23, 2018

View reviewed changes

traefiker removed the contributor/waiting-for-corrections label Mar 23, 2018

mmatur approved these changes Mar 23, 2018

View reviewed changes

emilevauge approved these changes Mar 23, 2018

View reviewed changes

traefiker approved these changes Mar 23, 2018

View reviewed changes

ldez added status/3-needs-merge and removed status/2-needs-review labels Mar 23, 2018

traefiker merged commit d2766b1 into traefik:master Mar 23, 2018

traefiker removed the status/3-needs-merge label Mar 23, 2018

ldez deleted the feat/whitelist-xff branch March 23, 2018 21:41

colinhoglund mentioned this pull request Mar 30, 2018

[stable/traefik] Add Entrypoint whiteListSourceRange support helm/charts#4522

Merged

jbdoumenjou mentioned this pull request Oct 2, 2018

X-Forwarded-Proto is overwritten in traefik ≥ 1.6.x #3886

Closed

MaxMood96 mentioned this pull request Jan 1, 2024

[Snyk] Security upgrade axios from 0.19.0 to 0.21.1 MaxMood96/traefik#138

Open

terrorizer1980 mentioned this pull request Jan 2, 2024

[Snyk] Security upgrade axios from 0.19.0 to 0.21.1 terrorizer1980/traefik#146

Open

dwayne45 mentioned this pull request Jan 2, 2024

[Snyk] Security upgrade axios from 0.19.0 to 0.21.1 dwayne45/traefik#113

Open

dwayne45 mentioned this pull request Mar 18, 2024

[Snyk] Security upgrade axios from 0.19.0 to 0.21.1 dwayne45/traefik#119

Open

MaxMood96 mentioned this pull request Mar 18, 2024

[Snyk] Security upgrade axios from 0.19.0 to 0.21.1 MaxMood96/traefik#146

Open

terrorizer1980 mentioned this pull request Mar 18, 2024

[Snyk] Security upgrade axios from 0.19.0 to 0.21.1 terrorizer1980/traefik#155

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ability to use "X-Forwarded-For" as a source of IP for white list. #3070

Ability to use "X-Forwarded-For" as a source of IP for white list. #3070

ldez commented Mar 22, 2018 •

edited

mmatur left a comment

mmatur Mar 23, 2018

mmatur Mar 23, 2018

mmatur Mar 23, 2018

emilevauge left a comment •

edited

emilevauge Mar 23, 2018

ldez Mar 23, 2018

emilevauge Mar 23, 2018

mmatur left a comment

mmatur Mar 23, 2018

mmatur Mar 23, 2018

juliens left a comment

mmatur left a comment

emilevauge left a comment

nemosupremo commented Jul 13, 2018

Ability to use "X-Forwarded-For" as a source of IP for white list. #3070

Ability to use "X-Forwarded-For" as a source of IP for white list. #3070

Conversation

ldez commented Mar 22, 2018 • edited

What does this PR do?

Motivation

More

Additional Notes

mmatur 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

emilevauge left a comment • edited

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

mmatur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliens left a comment

Choose a reason for hiding this comment

mmatur left a comment

Choose a reason for hiding this comment

emilevauge left a comment

Choose a reason for hiding this comment

nemosupremo commented Jul 13, 2018

ldez commented Mar 22, 2018 •

edited

emilevauge left a comment •

edited