handle indeterminate length proxy chains - fixes #449

[aaronhurt]

No description provided.

[aaronhurt]

@magiconair What are your thoughts on this as a solution for handling layered proxies?

[magiconair]

This looks sane but I want to let the problem sink in a bit more. Both of you have discussed this in detail and I don't think there is anything to add right now.

As a general rule, Fabio should do the 'right thing' if possible and should pick the 'right' default if there are multiple options.

If I can't think of a glaring omission I'll merge this.

[magiconair]

I'd prefer not to nest the ifs.

if xip == host {
    continue
}
if ip = net.ParseIP(xip); ip == nil {
    log.Printf("[WARN] failed to parse xff address %s", xip)
    continue
}
if t.denyByIP(ip) {
    return true
}

[aaronhurt]

Can do.

[magiconair]

I think this is the right approach.

The chain of ips you have to validate is client, proxy1, proxy2, ...., proxyN. The chain is composed of the xff header and the remote ip of your incoming connection which cannot be spoofed. Also, previous proxies only append the remote ip of their incoming connection to the xff header.

For this to work you have to trust a chain of proxies in front of you. The end of your trust chain contains the ip address of the inbound connection you care about and it cannot be spoofed. If someone sends a bogus xff header from an ip that is not in the list then this ip will be added to the xff header by your trusted proxy and the other trusted proxies in front of you will pass on that information to fabio which will see the ip and deny access.

[magiconair]

don't you need to continue here?

[aaronhurt]

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.

[magiconair]

With that in mind I wouldn't call this a work around. This is how this should work I think

[aaronhurt]

I updated the commit description and PR title to reflect the discussion.

[aaronhurt]

If someone sends a bogus xff header from an ip that is not in the list then this ip will be added to the xff header by your trusted proxy and the other trusted proxies in front of you will pass on that information to fabio which will see the ip and deny access.

Yes, exactly which is why I settled on validating all the elements contained within the XFF header. If some client wants to be clever and inject a bogus XFF header, including a header of an allowed netblock, they will still be denied if any member of the header fails to match.

[aaronhurt]

@magiconair anything else that is still needed with this one?

[magiconair]

Two small comments and then we're good. In the test itself you're not handling the error from url.Parse. In picker_test.go there is a mustParse function which panics which you could use.

[magiconair]

Is there a reason the order changed?

[aaronhurt]

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

[aaronhurt]

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.

[magiconair]

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

[aaronhurt]

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

[aaronhurt]

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

[magiconair]

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.

[aaronhurt]

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.