PoC: Add PROXY protocol support

[moorereason]

This commit adds a custom net.Listener to the webhook HTTP server to
enable PROXY protocol support. I've copied in the keep-alive listener
from the Go net/http package, so the non-PROXY server should behave just
like the stdlib.

Proof of Concept

This PR is a proof of concept at the moment. I haven't tested webhook behind a PROXY-enabled proxy. The go-proxyproto package looked simple enough and does all the heavy lifting, so I thought I'd just take a stab at an implementation. Interested parties need to help test it out (@stephenreay).

TODO:

• Actually test it behind a PROXY-enabled proxy
• Update docs
• Vendor dependencies
• Tests
• Add IP whitelisting option

Fixes #152

[stephenreay]

Thanks @moorereason , I'll try to look at this sometime this weekend and let you know how it goes.

I assume from the comments on the issue that the go proxy protocol lib only supports PROXYv1 (plain text), not PROXYv2 (binary) ?

I'll setup a vagrantfile to test this with a couple of different "upstreams" sending PROXY requests. Once it works (to the point of sending the request to webhook) I'll share it to make maintenance/testing easier.

[adnanh]

👍

[stephenreay]

Ok, so.. I have updates.

The short version is, this mostly WORKS. 🎉 Thanks @moorereason for your work on this so far!

The longer version is definitely somewhat driven by my inexperience (and resulting frustration) building a go package without detailed instructions.

This took me a while to get to even compile (e.g. it grabs the 'hook' package from GitHub not the local copy, and there is apparently a version mismatch in a method signature). I also had to comment out the svr.ServeTLS call, as it complains the method doesn't exist? I'm sure these are not technical faults, but lack of "how to build" when working from a local clone.

Anyway - so it seems I was correct, this is PROXY v1 only, but there is a bigger problem, which is referenced in a comment on the proxy package's repo: armon/go-proxyproto#6 (comment)

The underlying PROXY package explicitly allows non-PROXY connections to work transparently.
As was mentioned earlier, misuse of this protocol is dangerous, and allowing regular HTTP and PROXY+HTTP connections on the same port is so dangerous the protocol explicitly forbids it:

The receiver MUST be configured to only receive the protocol described in this
specification and MUST not try to guess whether the protocol header is present
or not. This means that the protocol explicitly prevents port sharing between
public and private access. Otherwise it would open a major security breach by
allowing untrusted parties to spoof their connection addresses. The receiver
SHOULD ensure proper access filtering so that only trusted proxies are allowed
to use this protocol.

Given my golang experience level (zero) I don't know if its realistic to do so, but a patched (presumably just removing the special-casing in https://github.com/pires/go-proxyproto-fork/blob/a284d5f458358c96eef5ce15cf36eaf7cc1ee8a4/protocol.go#L75) version might be usable, to retain protocol compatibility?

I do have the aforementioned Vagrant config (plus a test hook & a HAProxy config file) I'm happy to share, if it helps with testing/dev on this feature.

[moorereason]

@stephenreay, can you put your test configs into a gist? Looks like I'm going to have to get more familiar with the PROXY protocol. 😃

[stephenreay]

Hey @moorereason, see https://gist.github.com/stephenreay/391a94c7f435c53959e67f63a9fde6b4 - it assumes you want to test in Vagrant, but it should run OK on any platform that will run HAProxy if you point to the haproxy.cfg in the gist.

[stephenreay]

I also found a reference to https://github.com/pires/go-proxyproto in that other comment thread - not sure on the stability/usability of that lib, but it specifically says it supports PROXY v1 and v2.

[moorereason]

@stephenreay, I've confirmed the behavior you describe. The armon package silently accepts non-PROXY protocol connections. I've submitted a PR to give us the option to require PROXY.

The pires package looks half-baked. I may give it a try over the holidays. We'll see.

It looks like most users of the armon package setup IP whitelisting. We likely need to add that feature to this PR to further secure webhook without needing outside help (from firewalls or whatnot).

webhook -proxyProtocol -proxyWhitelist 10.0.0.11/32,10.0.0.22/32

[stephenreay]

Hi @moorereason

Thanks for the update.

Re: non-PROXY connections: it's worrying that the upstream PROXY lib is deliberately breaking the protocol like this.

Re: whitelisting: this seems like something that would be best left as a documentation item, surely: in the docs where it covers -proxyProtocol, an entry specifying that the service should only be accessible by trusted addresses - either bind to loopback (e.g. if HAproxy is running locally), or use an OS provided ip firewall etc. Implementing an IP firewall in this tool seems like an unnecessary duplication that's going to have multiple orders of magnitude less eyeballs looking at it.

Thanks again!

[moorereason]

@stephenreay wrote:

Re: whitelisting: this seems like something that would be best left as a documentation item, surely: in the docs where it covers -proxyProtocol, an entry specifying that the service should only be accessible by trusted addresses - either bind to loopback (e.g. if HAproxy is running locally), or use an OS provided ip firewall etc. Implementing an IP firewall in this tool seems like an unnecessary duplication that's going to have multiple orders of magnitude less eyeballs looking at it.

Good point. I'll hold off on that idea. 👍

[moorereason]

Closing. I have no intention of completing this PR in the foreseeable future.

[pires]

If anyone gets back to this, github.com/pires/go-proxyproto is stable (94% test coverage) and maintained. It supports v1 and v2, TLS and more TLVs. It also includes a policy mechanism for strict/relaxed handling of potentially proxy proto connections. Good luck!