PROXY Protocol support (rd 2)

[adrum]

This is essentially https://github.com/NginxProxyManager/nginx-proxy-manager/pull/1882 with the merge conflicts resolved.

I first tried submitting a PR to the source branch, but it's not yet been merged. https://github.com/SBado/nginx-proxy-manager/pull/1

I thought I might have better luck here 😄

[adrum]

cc @Iaotle

[adrum]

Seems like this was a timing issue with the CI during the apt update

[code-a-cola]

Having the same issue on my pull request.

[beatles1]

What's the difference between this and #4105 ? This functionality would be really useful for me and just wondering which would be better to help testing for?

[adrum]

@beatles1 the only difference between this one and the one you linked is this MR layers in a commit on top that resolves the merge conflicts with upstream. Merging this MR should also mark the other one merged.

[beatles1]

Awesome, hopefully the CI can be re-run and then I'll test the Docker image

[Iaotle]

@adrum I feel kinda bad for rushing you to re-make the PR, you put in effort fixing merge conflicts and submitting a PR just to end up in limbo </3

I admire the effort though!

[AlperShal]

@jc21 Really sorry for tagging you but looks like this is failing because of a CI issue. And the bot didn't post the details about why CI failed after the build bump. Can you please investigate this?

[NikeLaosClericus]

I've just started testing this in my homelab (I create pr #4660 just to get a working image).

I can report that I get the same errors that @Coolicky was getting in pr #4105 when enabling PP for Streams specifically when setting the Proxy IP. When enabling it for Streams without a proxy IP it works fine. I think this is because the proxy ip lines that are added to _listen.conf:

set_real_ip_from {{ load_balancer_ip }};
real_ip_header proxy_protocol;

Aren't being handled for Stream confs at all (or incorrectly?)

[NikeLaosClericus]

My next comment is on design choice.

I dislike that fact that enabling PP disables (replaces) the standard web hosting ports altogether. Yes, PP only works with proxy servers that are using PROXY, but they can live side-by-side without issue in a single server conf on the downstream server. This provides for using both PROXY and non-PROXY connections at the same time.

Mine, manually modified, for example:

server {
...
  listen 80;
  listen 88 proxy_protocol;
  listen 443 ssl;
  listen 444 ssl proxy_protocol;
  server_name example.com;
...
  set_real_ip_from xxx.xxx.xxx.xxx;
  real_ip_header proxy_protocol;
...

And as far as I can tell, the inclusion of the real_ip lines does nothing to harm port 80/443 standard implementations, while still working correctly for 88/444.

This is necessary for my implementation as my upstream proxy server is an nginx Stream configuration using SNI for domain based routing of https/443 connections (without termination). But nginx does not allow the use of proxy_protocol on; in the http configuration (nor is it required in my case, as I don't need SNI and standard proxying works fine for http connections).

So I need my server configurations in NPM to simultaneously accept 444 PROXY and 80 non-PROXY connections. Also accepting 443 is great for internally routed connections that don't need to go through my stream proxy, and 88 is good if I ever start using HAProxy (which I understand uses PROXY for both).

[NikeLaosClericus]

All-in-all, this is absolutely a necessary inclusion for NPM that I'm grateful it's being worked on and is close, but can the implementation be tweaked so that it doesn't 'turn of' standard hosting when enabled, or provide an additional switch to add both if desired?

Unrelated, but it occurred to me that I could have circumvented this by adding an additional proxy host to NPM for the same domains, if NPM allowed per port/protocol configurations. That way, I could have made the https host PROXY enabled, and the http host non-PROXY. But seeing as these can live side-by-side in a single server without issue, I think that's probably the best way to do it.

[NikeLaosClericus]

How I modified /app/templates/_listen.conf, enabling both simultaneously:

{% if enable_proxy_protocol == 1 or enable_proxy_protocol == true%}
  listen 88 proxy_protocol;
  listen 80;
{% if ipv6 -%}
  listen [::]:88 proxy_protocol;
  listen [::]:80;
{% endif %}
{% else -%}
  listen 80;
{% if ipv6 -%}
  listen [::]:80;
{% endif %}
{% endif %}
{% if certificate -%}
{% if enable_proxy_protocol == 1 or enable_proxy_protocol == true%}
  listen 444 ssl proxy_protocol;
  listen 443 ssl;
{% if ipv6 -%}
  listen [::]:444 ssl proxy_protocol;
  listen [::]:443 ssl;
{% endif %}
{% else -%}
  listen 443 ssl;
{% if ipv6 -%}
  listen [::]:443 ssl;
{% endif %}
{% endif %}
{% else %}
  #listen [::]:443;
{% endif %}

[adrum]

I'll get this PR updated with all this feedback today! Thanks for all the feedback, everyone.

[adrum]

@NikeLaosClericus I pulled in your changes from #4660 if you want to close it.

[NikeLaosClericus]

Closed my pr. I want to be clear that I made no changes in that pr, it was Iaotle's fix on top of your pr. I was just too impatient to wait.

[nginxproxymanagerci[bot]]

Docker Image for build 4 is available on DockerHub:

nginxproxymanager/nginx-proxy-manager-dev:pr-4505

[!NOTE] Ensure you backup your NPM instance before testing this image! Especially if there are database changes. This is a different docker image namespace than the official image.

[!WARNING] Changes and additions to DNS Providers require verification by at least 2 members of the community!

[NikeLaosClericus]

I have a better idea about the stream conf issues (https://github.com/NginxProxyManager/nginx-proxy-manager/pull/4105#issuecomment-2590999018):

I do not believe @SBado (et al) intended to add this feature to stream configs at all with their original PR. None of the code changes in the PR seem to (intentionally) cover streams. (I list some code files below that I think would have included changes if so...)

An example is that lines added to frontend/js/models/proxy-host.js:

enable_proxy_protocol:   false,
load_balancer_ip:        '',

have not been added to frontend/js/models/stream.js in this PR.

The same is true for the rest of the code changes being almost entirely specific to proxy_host files. I'm not exactly sure what would need to be changed for streams to work, but here are some other files that may need to be updated: (because there are equivalent changes in the proxy host files in this PR)

frontend/js/app/nginx/stream/form.(e)js frontend/js/i18n/messages.json (under the streams section) backend/templates/stream.conf backend/schema/paths/nginx/streams/post.json backend/schema/paths/nginx/streams/get.json backend/schema/paths/nginx/streams/streamID/get.json backend/schema/paths/nginx/streams/streamID/put.json backend/schema/components/stream-object.json backend/models/stream.js backend/migrations/???_stream_ssl.js (similar to backend/migrations/20220209144645_proxy_protocol.js)

And accepting that stream support wasn't intended... Is it possible that however NPM is generating the web page form for proxy host configuration, it's also handling the form for stream configurations and showing options that aren't really configured for streams? (thus the errors)

Given the possible scope of work increase this would represent, maybe it's better just to figure out why the options are being errantly shown on stream confs, and remove them, rather than expand to fit both.

I'm not saying I don't want stream support for PP, but this pr could be quickly accepted as enabling Proxy Protocol support on Proxy Hosts alone, and another PR could handle adding in support for streams later.