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

PROXY Protocol #2651

Closed
Roguelazer opened this issue Jun 30, 2021 · 5 comments · Fixed by #2654
Closed

PROXY Protocol #2651

Roguelazer opened this issue Jun 30, 2021 · 5 comments · Fixed by #2654
Labels

Comments

@Roguelazer
Copy link
Contributor

Roguelazer commented Jun 30, 2021

Is your feature request related to a problem? Please describe.
Many HTTP servers speak the PROXY Protocol to easily forward remote connection information below layer 7. It does not appear that Puma has such support.

Describe the solution you'd like
It would be great if puma supported the PROXY protocol as another option of the set_remote_address DSL. When this is set, Puma could attempt to read the PROXY protocol (versions 1 and/or 2) from the socket prior to running the HTTP parser, and, if it successfully read a PROXY header, would use that to set the remote_addr before calling into the application.

Describe alternatives you've considered
Current main alternatives:

  • Use X-Forwarded-For and have Rack handle it in the request middleware
  • Use X-Remote-IP or somesuch and the remote_address_header option
  • Use a different HTTP server (Passenger + Apache, uWSGI)
@nateberkopec
Copy link
Member

From the protocol description:

Then comes a new class of products which we'll call "dumb proxies", not because
they don't do anything, but because they're processing protocol-agnostic data.

Since we removed tcp-mode in Puma 5, I no longer think of Puma as something which fits this description. Puma is an HTTP and Ruby HTTP-application server, not a protocol-agnostic TCP server.

Can you help me to understand the use-case where Puma is a proxy for something else in your infrastructure?

@Roguelazer
Copy link
Contributor Author

It's fairly common to use PROXY not just through proxies, but also to end servers (including being supported by nginx, apache (with mod_remoteip), uwsgi, Percona MySQL, and a large number of other tools).

In no small part, this is because of how poorly X-Forwarded-For is handled. For example, Puma honors the first value in X-Forwarded-For as authoritative if you use the set_remote_address header: "X-Forwarded-For" config declaration, but if you use Rack's built-in X-Forwarded-For handling, it uses the last value (FWIW, haproxy expects downstream servers to use the last comma-separated value, but it's this ambiguity that causes no end to problems). In my experience, almost no HTTP servers correctly handle normalization if the header is provided multiple times. It's a lot easier to have a layer 5-ish protocol for sending over remote IP information.

@nateberkopec
Copy link
Member

it's this ambiguity that causes no end to problems

Totally, I feel like this comes up once every few years on every single HTTP-related project I've been a part of!

Is the real problem here that Puma is using a different value from X-Forwarded-For than everyone else?

@Roguelazer
Copy link
Contributor Author

Roguelazer commented Jul 2, 2021

I would says that that's also a problem (and mdn at least agrees with the way Rack interprets it, as the right-most string representing the nominal next-hop and the only trusted value), but support for PROXY is pretty much expected in HTTP servers these days, so it seems reasonable to also have such support (as #2654 provides). In my use case, all of the other HTTP servers used in every other platform I support internally use PROXY, so having to have a single application using X-Forwarded-For is kind of a bummer.

@nateberkopec
Copy link
Member

Had another think and I agree that this makes sense for inclusion in Puma. Needs a proper review (I need to understand PROXY first) but PR looks broadly fine.

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

Successfully merging a pull request may close this issue.

2 participants