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.

Sign up for GitHub

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

Jump to bottom

handle indeterminate length proxy chains - fixes #449 #453

Merged
merged 3 commits into from
Mar 6, 2018
Merged

handle indeterminate length proxy chains - fixes #449 #453

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cd6b651
handle indeterminate length proxy chains - fixes #449
Feb 26, 2018
3bfe186
flatten conditionals
aaronhurt Feb 27, 2018
a1d7b54
clarify resulting action on failed parse and remove some self-explana…
Feb 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 15 additions & 7 deletions route/access_rules.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,22 @@ func (t *Target) AccessDeniedHTTP(r *http.Request) bool {

// check xff source if present
if xff := r.Header.Get("X-Forwarded-For"); xff != "" {
// only use left-most element (client)
xff = strings.TrimSpace(strings.SplitN(xff, ",", 2)[0])
// only continue if xff differs from host
if xff != host {
if ip = net.ParseIP(xff); ip == nil {
log.Printf("[WARN] failed to parse xff address %s", xff)
// Trusting XFF headers sent from clients is dangerous and generally
// bad practice. Therefore, we cannot assume which if any of the elements
// is the actual client address. To try and avoid the chance of spoofed
// headers and/or loose upstream proxies we validate all elements in the header.
// Specifically AWS does not strip XFF from anonymous internet sources:
// https://docs.aws.amazon.com/elasticloadbalancing/latest/classic/x-forwarded-headers.html#x-forwarded-for
// See lengthy github discussion for more background: https://github.com/fabiolb/fabio/pull/449
for _, xip := range strings.Split(xff, ",") {
xip = strings.TrimSpace(xip)
if xip == host {
continue
}
if ip = net.ParseIP(xip); ip == nil {
log.Printf("[WARN] failed to parse xff address %s", xip)
Copy link
Contributor

Choose a reason for hiding this comment

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

don't you need to continue here?

Copy link
Member Author

Choose a reason for hiding this comment

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

The behavior of denyByIP is to immediately return false when passed a nil ... so it is effectively doing a continue. However, I can see the point that adding a continue would be more explicit and be more future proof should additional code be added between this statement and the denyByIP call.

continue
}
// check xff source and return if denied
if t.denyByIP(ip) {
return true
}
Expand Down
17 changes: 13 additions & 4 deletions route/access_rules_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,11 +165,11 @@ func TestAccessRules_AccessDeniedHTTP(t *testing.T) {
denied bool
}{
{
desc: "denied xff and allowed remote addr",
desc: "single denied xff and allowed remote addr",
target: &Target{
Opts: map[string]string{"allow": "ip:10.0.0.0/8,ip:192.168.0.0/24"},
},
xff: "1.1.1.2, 10.11.12.13, 10.11.12.14",
xff: "10.11.12.13, 1.1.1.2, 10.11.12.14",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason the order changed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not particularly, the result is the same regardless of the order. I can’t remember why it was changed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ohh, the old code only checked the first element. I moved it to ensure it was looping over the elements and still denying on a middle element.

remote: "10.11.12.1:65500",
denied: true,
},
Expand All @@ -183,13 +183,13 @@ func TestAccessRules_AccessDeniedHTTP(t *testing.T) {
denied: true,
},
{
desc: "allowed xff and allowed remote addr",
desc: "single allowed xff and allowed remote addr",
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment doesn't match the result since the behavior changed. Can you double check?

Copy link
Member Author

Choose a reason for hiding this comment

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

Only a single element of the XFF is allowed. The others are denied.

Copy link
Member Author

Choose a reason for hiding this comment

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

In the old code this was allowed as it only checked the first element.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I thought that because the XFF address and the remote address are allowed the request should be allowed. But 1.2.3.4 isn't whitelisted. This wasn't obvious to me. I'll clean up the comments after the merge.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that was part of the new behavior. If any one of the addresses presented in the XFF fail to match an allow/whitelist rule the request is denied. If any one of the addresses match a deny/blacklist rule the request is denied.

target: &Target{
Opts: map[string]string{"allow": "ip:10.0.0.0/8,ip:192.168.0.0/24"},
},
xff: "10.11.12.13, 1.2.3.4",
remote: "192.168.0.12:65500",
denied: false,
denied: true,
},
{
desc: "denied xff and denied remote addr",
Expand All @@ -200,6 +200,15 @@ func TestAccessRules_AccessDeniedHTTP(t *testing.T) {
remote: "200.17.18.20:65500",
denied: true,
},
{
desc: "all allowed xff and allowed remote addr",
target: &Target{
Opts: map[string]string{"allow": "ip:10.0.0.0/8,ip:192.168.0.0/24"},
},
xff: "10.11.12.13, 10.110.120.130",
remote: "192.168.0.12:65500",
denied: false,
},
}

for i, tt := range tests {
Expand Down