Feature #4247

PATCH: enable PROXY protocol and X-Forwarded-For

Added by DSA APF 10 months ago. Updated 9 months ago.

Status:FixedStart date:2016-11-14
Priority:NormalDue date:
Assignee:-% Done:

100%

Category:General
Target version:-

Description

Dear developers,

For a research demo we have implemented the PROXY protocol support in HTTP connections. It’s based on the X-Forwarded-For patch from Steffen Vogel. And as someone has requested this functionality, we want to share our patch. It’s fully tested and functional.

Title: [TVHE-devel] enable PROXY protocol and X-Forwarded-For

Description: Adds support for the PROXY protocol in incoming TCP connections. Also adds support for the ‘X-Forwarded-For’ header in HTTP connections.

Comments: This new expert functionality is completely optional. By default is disabled. If you enable it, then it’s fully transparent. However, when enabled you can connect to any TCP listening port and use the PROXY protocol to first send the original IP address of the remote client. Also you can use the HTTP protocol header ‘X-Forwarded-For’ for the same functionality. If both are used in the same HTTP connection (PROXY & 'X-Forwarded-For') then the HTTP header has preference.

User case: When you are running the TVHE in a container with a virtual IP address and use a proxy server to route incoming connections. This functionality shows the correct IP address of the client if the proxy server forwards it.

Warning: It’s recommended to only enable this option when the TVHE server is behind a firewall forwarding the client address. If not, a malicious client might spoof the source address.

Related work:
- PROXY protocol description:
http://www.haproxy.org/download/1.8/doc/proxy-protocol.txt

- 'X-Forwarded-For' header description:
http://tools.ietf.org/html/rfc7239

- Original X-Forwarded-For patch from Steffen Vogel:
http://github.com/tvheadend/tvheadend/pull/640

You can merge it with the main branch if you want. As by default it’s disabled, it doesn’t creates a security hole.

Regards!
D.


Subtasks

Feature #4078: Add support for PROXY-PROTOCOLRejected

History

#1 Updated by Mono Polimorph 10 months ago

Great!

Someone has listening my request at #4078.
I hope Jaroslav merges this patch soon.
Thank you guys! ;)

#2 Updated by DSA APF 9 months ago

Hi,

Any problem with this patch?
We hope it will be accepted before publishing some other patches related to the SAT>IP protocol.

Regards,
D.

#3 Updated by Jaroslav Kysela 9 months ago

Please, create a PR (pull request) on github. I have some comments for the code. Also, you did not resolved the security hole. Only list of allowed (trusted) proxies should be accepted for 'PROXY' requests. So you should check, if the 'PROXY' request comes from an allowed IP list.

#4 Updated by DSA APF 9 months ago

Hi,

Please, create a PR (pull request) on github. I have some comments for the code.

We will do!

Also, you did not resolved the security hole.

Please, review our patch... the proxy mode is DISABLED by default. So, no security hole at all!
The original PROXY protocol RFC advises against using it in mixed environments with proxy/non-proxy connections.

Regards,
D.

#5 Updated by Jaroslav Kysela 9 months ago

DSA APF wrote:

Hi,

Please, create a PR (pull request) on github. I have some comments for the code.

We will do!

Thanks.

Also, you did not resolved the security hole.

Please, review our patch... the proxy mode is DISABLED by default. So, no security hole at all!
The original PROXY protocol RFC advises against using it in mixed environments with proxy/non-proxy connections.

I saw the general on/off switch in your patch, but it's not sufficient. Even if TVH is on the local network, the local clients should not be allowed to claim IP addresses of someone else. I would propose to have a list of known proxies and eventually implement '*' string as a general on without any filtering for special purposes.

#6 Updated by DSA APF 9 months ago

Hi,

Jaroslav Kysela wrote:

Also, you did not resolved the security hole.

Please, review our patch... the proxy mode is DISABLED by default. So, no security hole at all!
The original PROXY protocol RFC advises against using it in mixed environments with proxy/non-proxy connections.

I saw the general on/off switch in your patch, but it's not sufficient. Even if TVH is on the local network, the local clients should not be allowed to claim IP addresses of someone else.

As I commented, the safe way for using the proxy protocol is: use it fully, or not use it at all.
So, if you enable it then is enforced to be behind a proxy for all incomming connections. Then the proxy will pass the correct address of the client, and it can be local or remote.

Jaroslav Kysela wrote:

I would propose to have a list of known proxies and eventually implement '*' string as a general on without any filtering for special purposes.

I agree with this. However, for us isn't trivial to implement. Can you do it (after our pull request), please?

#7 Updated by Jaroslav Kysela 9 months ago

OK, we can start with a simpler version. Please, create PR to get things rolling..

#8 Updated by DSA APF 9 months ago

Jaroslav Kysela wrote:

OK, we can start with a simpler version. Please, create PR to get things rolling..

PR done!
http://github.com/tvheadend/tvheadend/pull/935

#9 Updated by DSA APF 9 months ago

DSA APF wrote:

Jaroslav Kysela wrote:

OK, we can start with a simpler version. Please, create PR to get things rolling..

PR done!
http://github.com/tvheadend/tvheadend/pull/935

Hi Jaroslav,

No interest to commit our patch?

#10 Updated by Mark Clarkstone 9 months ago

DSA APF wrote:

DSA APF wrote:

Jaroslav Kysela wrote:

OK, we can start with a simpler version. Please, create PR to get things rolling..

PR done!
http://github.com/tvheadend/tvheadend/pull/935

Hi Jaroslav,

No interest to commit our patch?

I'm sure he'll get around to it eventually. Like many he's very busy right now :p

#11 Updated by DSA APF 9 months ago

Mark Clarkstone wrote:

DSA APF wrote:

DSA APF wrote:

Jaroslav Kysela wrote:

OK, we can start with a simpler version. Please, create PR to get things rolling..

PR done!
http://github.com/tvheadend/tvheadend/pull/935

Hi Jaroslav,

No interest to commit our patch?

I'm sure he'll get around to it eventually. Like many he's very busy right now :p

I hope the PR is accepted soon... :)

#13 Updated by Jaroslav Kysela 9 months ago

  • Status changed from New to Fixed

Also available in: Atom PDF