Add support for PROXY protocol

[stephenreay]

While most of the functionality of the webhook daemon is unaffected by running behind a proxy like HAProxy/Pound/etc, the "check IP address" rule functionality won't work in this setup, because it will have a client IP of the proxy server.

HAProxy introduced the PROXY protocol, which solves this problem, and is implemented by a number of server components.

Documentation for the protocol is here: http://www.haproxy.org/download/1.8/doc/proxy-protocol.txt

[moorereason]

Seems like you should validate IPs at the proxy server instead of within webhook.

But if anyone's interested in implementing this, see https://github.com/armon/go-proxyproto.

[adnanh]

If the server is sending the IP via headers one could use match-regex rule to validate the IP range.

I would accept a good PR for this feature, but I do not have time to implement it myself.

[ivanpesin]

It seems to me that this sort of access control should be implemented at the proxy server.

There are two options discussed:

1. support PROXY protocol
2. support X-Forwarded-For: header

PROXY protocol essentially adds additional instruction when connection is established, just before GET/POST/... request line: PROXY TCP4 192.168.0.1 192.168.0.11 56324 443\r\n

X-Forwarded-For header simply holds a series of IP addresses.

In both cases there is NO way to ensure authenticity of the information passed in and it can easily be used for spoofing. Thus adding support in webhook for these features will only give a false sense that user has access control, when in fact they don't.

Moreover, if a user has access to a proxy server to configure either PROXY protocol support or X-Forwarded-For header, most likely they have access to configure access control there as well which is a more robust way to filter out undesirable requests.

[stephenreay]

If you're using PROXY protocol, it's assumed that the downstream end (ie webhook in this instance) isn't also directly exposed to the network.

Forcing this to be handled upstream in the proxy also doesn't easily allow for multiple hooks with different rules. It means duplicating path inspection.

[ivanpesin]

I see your points and agree that they are convenient. However, security should not be compromised for convenience, especially in a way that gives you false sense of control. You generally don't want your default configuration to rely on other network components' configuration not to be easily compromised. Also, it's not complicated to configure haproxy to have different rules for different URLs.

As a benefit you get better security and much more sophisticated ACL rules available in HAProxy, compared to webhook.

Anyway, that's just my point of view, maybe @adnanh has different opinion.

[stephenreay]

I'm not suggesting to compromise security for convenience.

I'm suggesting there be an option to bind to an address/port with PROXY support, and it should probably default to off.

Allowing fetching the true client IP from a header would be an acceptable alternative, but going forward I would expect PROXY to be the more common method.

Sent from my iPhone

On 16 Sep 2017, at 10:08, Ivan Pesin notifications@github.com wrote:

I see your points and agree that they are convenient. However, security should not be compromised for convenience, especially in a way that gives you false sense of control. You generally don't want your default configuration to rely on other network components' configuration not to be easily compromised. Also, it's not complicated to configure haproxy to have different rules for different URLs.

As a benefit you get better security and much more sophisticated ACL rules available in HAProxy, compared to webhook.

Anyway, that's just my point of view, maybe @adnanh has different opinion.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.