proxyprotocol: use github.com/pires/go-proxyproto

[mohammed90]

Further enhancements can be added later, e.g. advanced allow/denylist policies.

I'll test it out soon, unless someone beats me to the test.

[francislavoie]

/cc @Embraser01 @KorvinSzanto if you'd like to test this

[francislavoie]

The code looks good to me. Glad we could retain the same options as-is!

[WeidiDeng]

A few nitpicks only, LGTM.

[WeidiDeng]

Why store this in a slice when there can be one func at once? proxyproto takes a slice of IP address and return a function. There is no need to store it in a slice.

[mohammed90]

It's my attempt at future-proofing. proxyproto has a selection of policies. We can also build more as long as they conform to the PolicyFunc type. My thinking is we could expand the implementation later to unionize policies.

[WeidiDeng]

Just use a field of type PolicyFunc and assign it here.

[KorvinSzanto]

This does seem to resolve #5863 for me.

[Embraser01]

🎉 Can confirm it works as well as the one we made over at https://github.com/caddyserver/ingress/tree/master/pkg/proxy.

I created a PR to replace it by this one once merged and released

[mholt]

Great, thanks for testing it out.

This is an elegant change. Once Weidi's comments are considered (or replied to) I'm good with merging this. Thank you @mohammed90 !!

[mohammed90]

🎉 Can confirm it works as well as the one we made over at https://github.com/caddyserver/ingress/tree/master/pkg/proxy.

I created a PR to replace it by this one once merged and released

@Embraser01, I see your implementation uses the REQUIRE policy, while this PR goes with USE. Should I expose this configuration option?

[Embraser01]

🎉 Can confirm it works as well as the one we made over at caddyserver/ingress@master/pkg/proxy.

I created a PR to replace it by this one once merged and released

@Embraser01, I see your implementation uses the REQUIRE policy, while this PR goes with USE. Should I expose this configuration option?

It could be nice to expose this option as some user may want to force proxy protocol. Currently the ingress controller doesn't let choose between those modes but I think it should probably be USE by default (don't remember why I set it up to REQUIRE)

[WeidiDeng]

Nothing major except for the possible unix socket bug. I prefer to use the new netip types if possible.

[WeidiDeng]

LGTM, .

[TimWolla]

I don't do Caddy and I only do the bare minimum of Go. This came to my attention via haproxy/haproxy#2450 and if I understand the code correctly, the USE policy is a footgun will result in security vulnerabilities, as per my comment on the HAProxy issue. My understanding is that it uses the PROXY header if one is sent and does not use it, if it is missing, allowing a malicious client to add the PROXY header itself if a gateway in front of Caddy is trusted, but does not add the PROXY header for whatever reason.

[TimWolla]

I also find REJECT somewhat questionable, because it effectively is SKIP, but worse: It would reject a connection if the first few bytes of the connection happen to match a valid PROXY header by chance, instead of just passing it to the underlying protocol handler.

[francislavoie]

If the user knows their Caddy instance is accessible on public networks, they should configure allow to limit PROXY protocol to only those IP ranges.

[francislavoie]

It would reject a connection if the first few bytes of the connection happen to match a valid PROXY header by chance, instead of just passing it to the underlying protocol handler.

We're only concerned with HTTP and TLS in this case, so that's not a concern. We have a different implementation for https://github.com/mholt/caddy-l4 which allows any TCP/UDP traffic.

[TimWolla]

If the user knows their Caddy instance is accessible on public networks, they should configure allow to limit PROXY protocol to only those IP ranges.

My understanding is that the allow configuration option sets the USE policy for a given client as per

https://github.com/caddyserver/caddy/pull/5915/files#diff-39cd21a1e121836c0740aa16ed05549fb146c932b9c0f93f5409a6ccb902c0a9R92-R97

Now consider the following:

1. I configure allow 10.10.10.10 to preserve IP addresses from my trusted gateway running at 10.10.10.10.
2. This configures the USE policy, which to my understanding takes the client information from the PROXY header if present and accepts the connection without doing anything special if the PROXY header is not present.
3. My trusted gateway at 10.10.10.10 does not add the PROXY header for whatever reason (bug, configuration mistake, ...).
4. Now a malicious client could connect to my trusted gateway at 10.10.10.10 and prepend a valid PROXY header to their request. This PROXY header would be forwarded as-is to Caddy and then accepted.

If instead the REQUIRE policy would be configured, I would notice at step (3), because all connections from my trusted gateway would be rejected as the PROXY header is missing, thus alerting my monitoring.

[francislavoie]

Hmm. So the concern is that the "trusted" gateway somehow becomes not trusted? Sounds like it's already game-over at that point. It would be very wrong for a gateway to pass through PROXY protocol bytes as-is without validation/stripping, if it is itself configured to add PROXY protocol.

The problem with REQUIRE is that it's a common for internal apps (from private IP ranges) to need to connect to the server directly (e.g. a cron job which uses the HTTP server to publish an event of somekind, or a metrics scraper pulling data), and those apps may not support adding PROXY protocol; and for those usecases the connecting IP is not relevant anyway.

[polarathene]

@TimWolla Apologies that this turned out so verbose, but I wanted to be thorough 😓

• I've provided a reproduction environment that you are welcome to show an exploit for due to go-proxyproto policy feature allowing to accept the PROXY protocol header to replace the Client IP only when the host is trusted, but otherwise allowing connections through including from the trusted host.
• Traefik without any handling of PROXY protocol or trust can route non-TLS connections via TCP router to Caddy, which can trust Traefik as a host IP for PROXY protocol. This is the only situation where the client could forge their IP via PROXY protocol.
• I have written a program to proxy a TCP connection and inject a PROXY protocol header with fake IP to try exploit, but no success beyond the above generic TCP connection router unaware of PROXY protocol.

---

as per my comment on the HAProxy issue

allowing a malicious client to add the PROXY header itself if a gateway in front of Caddy is trusted, but does not add the PROXY header for whatever reason.

For others in this discussion to be aware of, I have provided a compose.yaml environment that can be used to easily setup Caddy with a trusted host (Traefik).

While @TimWolla may not be familiar with Caddy, hopefully that is sufficient for them to use for demonstrating how a client could exploit the trust policy feature go-proxyproto has.

The flexibility of trust via policy by not only host IP, but also on the port (by making PROXY header optional for a trusted host) is quite nice to have vs requiring separate ports just to allow trusted hosts to preserve a client IP via PROXY protocol.

The current PROXY protocol specification also disallows normal connections to a port by untrusted IPs (which Caddy 2.7 allowed to configure for). go-proxyproto expands on that with the PROXY header being optional. Personally I don't see a concern with either of these features, so I'd love to see it exploited to clear up any misunderstanding I have.

---

Now a malicious client could connect to my trusted gateway at 10.10.10.10 and prepend a valid PROXY header to their request. This PROXY header would be forwarded as-is to Caddy and then accepted.

This makes no sense to me. How is the gateway trusted if a malicious client can connect to it with their own PROXY header added?

The information in the PROXY header is limited:

From the PROXY protocol spec in question:

• 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.

If you trust any client connection from 10.10.10.10 to provide PROXY protocol, then anyone can abuse that. No one disputes that, but has no relevance to optionally allowing connections to not use PROXY protocol to the same receiver port?

The only security concern is that when you receive PROXY protocol header, that it only be trusted if the connecting client is trusted. What's changed here with your example of a malicious client?

Is there something different to this scenario you've described compared to the compose.yaml example I've linked?:

Client => <PROXY protocol not accepted> => Traefik => <Optional PROXY protocol> => Caddy

Only Traefik can add the PROXY protocol header for those connections. I think your concern is where Traefik (or any other service) would unconditionally forward that PROXY header through to Caddy instead of managing that itself as the trusted host.

If that's the case, Traefik is not configured by any means as a PROXY protocol receiver, only Caddy (which only trusts Traefik). The fact that PROXY protocol is optional for connections to Caddy at the same port doesn't change that outcome?

Previous findings (ignore)

I have confirmed by having Caddy make a connection back to Traefik (via the reverse_proxy directive):

• If Caddy performs that request with PROXY protocol then Traefik rejects it (unless explicitly configuring trust for incoming connections via entrypoints using PROXY protocol).
• It could also be tested with curl --haproxy-protocol.

For traffic routed through Traefik (via the TCP TLS router without PROXY protocol) to Caddy, no forgery occurs.

• If Traefik permits a client connecting to an entrypoint to use PROXY protocol, the client IP is resolved at the entrypoint, the PROXY header is not retained to carry over to Caddy (even with TLS passthrough or a non-TLS TCP router that just routes the TCP traffic).

• If I enable PROXY protocol again between Traefik => Caddy, and have both curl and Caddy perform requests using PROXY protocol, the final response is the client IP belonging to Caddy. That is (with PROXY protocol used between each request =>):

  Client (curl) => Traefik => Caddy (reverse_proxy request) => Traefik => Caddy (response to reverse_proxy request)
  

UPDATE: While exploring this I realized it wasn't sufficient. I had to learn how to write a TCP proxy that injects a PROXY header to my curl requests (tried avoiding curl but at least with rust that complicated getting the response back with my inexperience).

I ran the program outside of the Docker subnet and connected to the exposed ports, where I'm aware of some security gotchas:

• From the Docker host, accessing the docker subnet will route the traffic with source IP as the network gateway IP. If the trusted hosts happens to be that entire subnet, that's an easy way in through trust.
• External clients over IPv6 will also be able to route through to a Docker container with published ports that only have an IPv4 network via this same gateway IP.
• Both are due to Dockers default network management with iptables, they can be avoided by opting out of things like userland-proxy.
• Those issues provide a simpler way to do such a test with curl --haproxy-protocol --connect-to ::localhost:80 http://example.com to provide the client IP 127.0.0.1 instead of the rewritten client IP Docker introduced before it reaches Traefik. I had forgotten about this until after I wrote the rust program 😓

Ignoring that, when no trust is configured at the Traefik entrypoint, the client can do request forgery.

• A non-TLS TCP router will just pass the incoming traffic through to the proxied service. This allows the curl client to provide it's own PROXY header and for Caddy to accept it since Traefik IP is trusted.
• This won't effect HTTP routers that expect HTTP / TLS traffic, likewise a TCP router with TLS (passthrough or not) as the unexpected PROXY protocol header interferes.
• It also fails when Traefik configures PROXY protocol for a route to the service, as this would add a 2nd PROXY header, causing the service like Caddy which is expecting HTTP or TLS connections (with optional PROXY header) to fail as the 2nd PROXY header remains, causing issues like described for Traefik.
• If Traefik did configure an explicit set of trusted hosts (even if it was an IP that'd never connect to Traefik), untrusted clients trying to sneak in their own PROXY header would have the header removed and treat it like a normal connection without it (IGNORE policy).
• For any explicitly trusted hosts providing a PROXY header, Traefik will acknowledge the client IP from the PROXY header (USE policy) and if the TCP routed service enabled PROXY protocol, that will also become the client IP (regardless of the actual IP of the connecting trusted host to Traefik) given to the proxied service, Caddy.

Caddy in 2.7 by default (without configuring any trusted hosts via allow) would have accepted any PROXY protocol header. While with the upcoming 2.8 release with go-proxyproto has the same behaviour I observed with Traefik not having configured trusted hosts. They both default to IGNORE policy (EDIT: Traefik doesn't process the header by default, so IGNORE is not exactly correct).

---

I also find REJECT somewhat questionable, because it effectively is SKIP, but worse: It would reject a connection if the first few bytes of the connection happen to match a valid PROXY header by chance, instead of just passing it to the underlying protocol handler.

Given findings above, how likely is that to be a practical concern?

In this PR, REJECT is only used for errors prior to having the host IP parsed, then it is checked in the allow and deny lists:

• If it's in deny list REJECT refuses the connection if it has the PROXY header 👍
• If it's in the allow list USE does the opposite and trusts the PROXY header for client IP 👍 (REQUIRE adds nothing here, since the expectation is already that the PROXY header is present, optionally allowing connections through without one)
• If the IP is not in either allow or deny, IGNORE 👍 (discard the header as if it weren't provided, allowing the connection)

Traefik has a very similar policy, but they only have an allow list for USE, otherwise using IGNORE but only if an allow list exists.

If I was proxying connections via Traefik TCP router without TLS, and I knew the proxied service (Caddy) was trusting Traefik for PROXY header, I'd probably want REJECT as a policy for any untrusted client going through that route instead of IGNORE. Since a valid PROXY header by chance would be a known security issue in that scenario?

If instead there was no trusted PROXY protocol hosts to be concerned about, and the service is doing something completely unrelated, then sure REJECT would be bad for that.

REQUIRE only makes sense to enforce if you only ever expect connections to that service port to be traffic that requires the client IP to be corrected. If you're concerned about misconfiguration by the PROXY header not being present, then you could monitor logs for the client IP being the trusted host IP, except when that is worse than service failure from rejecting connections, then prefer REQUIRE. The policy approach is just more granular than IP or necessitating a separate port configuration to support internal + external traffic to a service.

REJECT rejects a connection missing the PROXY header while SKIP allows a connection through without a PROXY header, not sure how that's effectively the same? (see usage in go-proxyproto here), it was implemented for k8s to have local/internal traffic not need to go through a proxy just to get the PROXY header added for the receiving service to accept the connection (where the direct connection provides expected client IP without needing PROXY header like external traffic through a reverse proxy does).

[TimWolla]

@francislavoie

It would be very wrong for a gateway to pass through PROXY protocol bytes as-is without validation/stripping, if it is itself configured to add PROXY protocol.

No, it would be very wrong for a gateway to touch the PROXY header - unless its client is also expected to send a proxy header. The gateway should just add the PROXY header and not touch anything the client sends, otherwise it would modify the data sent by the client. The target service would then parse (and strip) the (single) PROXY header added by the gateway and attempt to interpret the fake PROXY header by the client as whatever protocol the backend speaks.

That's why the REJECT policy is non-sense.

The problem with REQUIRE is that it's a common for internal apps (from private IP ranges) to need to connect to the server directly (e.g. a cron job which uses the HTTP server to publish an event of somekind, or a metrics scraper pulling data), and those apps may not support adding PROXY protocol; and for those usecases the connecting IP is not relevant anyway.

Then the policy for those internal apps that do not send a PROXY header must be configured as SKIP. Everything else is semantically incorrect and/or a security vulnerability, because something that is not a PROXY header might be interpreted as one.

See also my explanation over in HAProxy's issue tracker: haproxy/haproxy#2450 (comment)

[TimWolla]

@polarathene

For folks following along, I've provided an extensive response over in HAProxy's tracker: haproxy/haproxy#2450 (comment)

The current PROXY protocol specification also disallows normal connections to a port by untrusted IPs (which Caddy 2.7 allowed to configure for). go-proxyproto expands on that with the PROXY header being optional. Personally I don't see a concern with either of these features, so I'd love to see it exploited to clear up any misunderstanding I have.

I believe the important part is the distinction between “optional” (i.e. USE; unsafe) and “no special handling” (i.e. SKIP; safe). Quoting from my response in HAProxy's tracker:

Conditionally handling which hosts can connect without PROXY procotol is fine, if the decision is made without looking into the contents.
[…]
A trust policy on the same port is fine, if it's a decision between “client MUST send a PROXY header” and “no special handling is performed”. Making the PROXY header optional is not fine. Note the important distinction between “no special handling” and ”optional”.

How is the gateway trusted if a malicious client can connect to it with their own PROXY header added?

see my response in HAProxy's tracker (keyword: dumb TCP forwarder).

A non-TLS TCP router will just pass the incoming traffic through to the proxied service. This allows the curl client to provide it's own PROXY header and for Caddy to accept it since Traefik IP is trusted.

This is the security vulnerability of the USE policy. With the REQUIRE policy, those cases would easily be detected, because the vast majority of traffic is rejected, due to the lack of the PROXY header.

Given findings above, how likely is that to be a practical concern? […] If it's in deny list REJECT refuses the connection if it has the PROXY header 👍

re practical concern: Depends on your type of traffic. For HTTP it is likely not a concern, because a PROXY header is not valid HTTP and thus the request would be rejected no-matter-what.

If it's in the allow list USE does the opposite and trusts the PROXY header for client IP 👍 (REQUIRE adds nothing here, since the expectation is already that the PROXY header is present, optionally allowing connections through without one)

At the risk of repeating myself: USE is unsafe. Only REQUIRE is safe.

If the IP is not in either allow or deny, IGNORE 👍 (discard the header as if it weren't provided, allowing the connection)

See REJECT.