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.

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

Support PROXY protocol #799

Closed
mfischer-zd opened this issue Nov 23, 2015 · 16 comments
Closed

Support PROXY protocol #799

mfischer-zd opened this issue Nov 23, 2015 · 16 comments

Comments

@mfischer-zd
Copy link
Contributor

Amazon ELBs and haproxy support the PROXY protocol for layer 4 (TCP level) load balancing. This protocol presents client IP information to the server upon connecting, before sending the first bytes provided by the actual client (typically the start of a TLS handshake).

Support for this protocol could improve the quality of Vault's audit logs by logging the actual client IP address instead of a proxy's.

References:

@armon
Copy link
Member

armon commented Nov 23, 2015

For context, we discussed X-Forwarded-For previously and it does introduce a security attack against Vault where you poison the log by sending another "client" to Vault by header injection. I think for PROXY support, it would similarly have to be something enabled by an operator server side with the understanding that ELB MUST be used with ProxyProtocol enabled, otherwise they've added an attack vector. We will just need to be very clear in the docs on this.

@mfischer-zd
Copy link
Contributor Author

Perhaps, as with #291 (X-Forwarded-For), trust could be extended to proxies in an IP whitelist.

@jefferai
Copy link
Member

(Moved from a reply on #291) @mfischer-zd it doesn't fix the spoofing problem if the load balancer is not configured to strip existing X-Forwarded-For headers or, in this case, PROXY info. Although at some point you have to put the onus on the user to handle such things.

I think an IP whitelist is a reasonable approach. I'd be interested in the PR in #291 being resurrected with a configurable IP whitelist, support for X-Forwarded-For and PROXY, and if not yet support for https://tools.ietf.org/html/rfc7239, at least the codebase being friendly to later inclusion of it.

@blalor
Copy link
Contributor

blalor commented Dec 4, 2015

I'd like to add a vote for this. AWS ELBs are PCI compliant, but only if you don't terminate SSL at the ELB. The only way to have a usable audit trail when using an ELB in a PCI environment is to support the PROXY protocol and terminate SSL at the Vault instance.

http://docs.aws.amazon.com/ElasticLoadBalancing/latest/DeveloperGuide/enable-proxy-protocol.html

@bkeroackdsc
Copy link

There should be a big warning in the documentation that App-ID auth is totally unusable behind a load balancer/proxy because of this issue. Or at least App-ID with source address restrictions.

In other words, HA setup via LB/proxy (the common case) and App-ID authentication with source address restrictions (also fairly common) are incompatible.

@jantman
Copy link
Contributor

jantman commented Mar 25, 2016

I'd also vote for this.

Let's also keep in mind that the implications are vastly different for auth vs logging. I think there would still be value in adding the X-Forwarded-For to the log as a separate field...

@jantman
Copy link
Contributor

jantman commented Mar 30, 2016

I was just giving this a bit of thought... I'm not sure how much it would complicate the implementation, but it seems like the most flexible would be if this could be enabled on a per-listener basis. For those who wish to allow both proxy connections (i.e. from a load balancer, with parsing of the PROXY header) and direct connections, proper use of firewalling or access control rules would allow both options (albeit on separate listeners) without the security risk of directly-connecting clients being able to spoof the PROXY header.

@restfulhead
Copy link

Are there any updates on this? I know logging of the headers was recently released, but this doesn't solve the issue with AppRole CIDR restriction when behind a proxy...

@jefferai
Copy link
Member

@ruhkopf discussion at #1340 (comment) is most relevant.

@jantman
Copy link
Contributor

jantman commented Feb 23, 2017

(Logging of headers also doesn't solve this for the PROXY use case...)

That (comment on 1340) is a really good discussion. However, for what it's worth, one part of the "problem" really struck me:

So the second-best case scenario, IMO, would be a load-balancer operating in TCP mode and sending PROXY protocol headers up to Vault. Operators would have to specify a whitelist of IP addresses that are allowed to send PROXY headers to Vault, but (fortunately) requests would outright fail when the configuration is wrong, providing fast-feedback. In AWS, this could be implemented by putting the ELB instances in their own /28 subnets.

This concern around whitelisting IPs seems strange to me... if I was fronting Vault with a load balancer (whether an ELB in AWS, or an F5, or whatever), I would be only allowing traffic from the load balancer - i.e. my SG in AWS (or firewall, or similar tool elsewhere) would only be allowing traffic in from the ELB, I wouldn't be allowing clients to bypass the LB and hit Vault directly.

Is there really a significant portion of the user base that would want to front Vault with a load balancer and also allow direct traffic? If Vault is fronted with a LB and there's not also a requirement to support direct traffic, it seems to me that IP whitelisting and concerns around X-Forwarded-For or PROXY accuracy are moot, since traffic can only reach Vault from the LB, and the LB would be configured to strip/replace such headers...

@jefferai
Copy link
Member

I don't think it's really necessary, but I also don't think that it's a real part of the problem.

@StyleT
Copy link
Contributor

StyleT commented May 19, 2017

Hi!
After reading a lot of issues/PRs/etc that are related to this issue & trying to implement HA (<20s downtime at least, 0s in future) Vault setup I have few points to add:

  • It's really hard or I would say impossible to implement HA Vault cluster that works only with Route53 w/o ELB. Because if we have 3 instances in a cluster and one of them goes down - in the best case we'll have 10s downtime for 33% of requests because of the DNS cache. Also Route53 still have no health checks for resources inside VPC so we need to do a LOT of work, especially comparing to ELB, to notify it that one of the instances is unavailable.
  • I have a strong opinion that all talks about a chance that Vault security will be compromised because of this feature so we shouldn't implement it sounds like an absurd. It's impossible to protect software against idiots. Right now I can for example disable TLS encryption at all & launch Vault in production, PROFIT!, let's disable this feature. So please allow me to enable PROXY protocol or X-Forwarded-For support when I want it (warn me in the documentation about security risks) & let me, software user, guarantee security of the connection.
  • Right now I'm in a trap because of this issue. Solution with Route53 puts me into a risk that I'll loose some part of the requests plus adds a huge amount of additional work & solution with ELB doesn't works with cidr ranges for AppRole.
  • As @blalor said: AWS ELBs are PCI compliant, but only if you don't terminate SSL at the ELB.
  • Support X-Forwarded-For protocol when getting connection RemoteAddr #291 looks like almost ready. Should someone finish it?
  • Some of the points above are AWS specific but "ELB" can be easily replaced with "HAProxy" in non-AWS infra. Also w/o LB support it's even harder to deal with Vault when we don't have a dynamic DNS provider in non-AWS setup.
  • I really think that it's one of the most important features that needs to be done since it shouldn't require a lot of work & brings much easier Vault cluster setup option.

@jefferai @armon ^

@jefferai
Copy link
Member

I have a strong opinion that all talks about a chance that Vault security will be compromised because of this feature so we shouldn't implement it sounds like an absurd. It's impossible to protect software against idiots. Right now I can for example disable TLS encryption at all & launch Vault in production, PROFIT!, let's disable this feature

You're mixing up administratively-set options with client-set options. The latter has a long history of causing serious vulnerabilities even when administratively set options are correct. As a recent example, see https://auth0.com/blog/critical-vulnerabilities-in-json-web-token-libraries/

It is one thing for an administrator to purposefully disable security, and quite another to circumvent administratively set security by blindly accepting what a client gives you.

To @jantman 's point ("Is there really a significant portion of the user base that would want to front Vault with a load balancer and also allow direct traffic?") we see this regularly with cases where outside traffic goes through a load balancer but internal traffic goes direct. Not all cases, but it happens. However, if you have two interfaces on a machine and can firewall one off to only allow traffic from an LB that helps quite a bit.

If the PROXY protocol would have put its information within TLS encapsulation this would be easy -- simply create a listener that requires client certificate verification and the only valid certificate be on the LB. Since it doesn't, you have to find alternative methods to enhance security, such as whitelisting IPs. Given that you can have multiple Vault listeners this isn't necessarily a problem, but it's fragile.

@StyleT
Copy link
Contributor

StyleT commented May 24, 2017

@jefferai Hi! So can you clarify your opinion? Are you ok with this optional feature (considering the above drawbacks) or not? Please take into account point # 1 from the my comment above.

@jefferai
Copy link
Member

I've never been against it, but it depends on the implementation.

@innovationhub-asia
Copy link

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants