openwrt / packages

Community maintained packages for OpenWrt. Documentation for submitting pull requests is in CONTRIBUTING.md
GNU General Public License v2.0
3.89k stars 3.4k forks source link

Nginx: support dynamic wan IP #19875

Open hgl opened 1 year ago

hgl commented 1 year ago

Maintainer: @Ansuel @@peter-stadler

Description:

Since Nginx doesn't have the ability to listen on interfaces, and WAN having dynamic IPs is fairly common in residential networks, it'd be great if the Nginx could listen on WAN and still work when its IPs change.

I think one solution could be that this package natively supports template config, where WAN IPs could be replaced whenever they change. The implementation involves using iface hotplug to gather changed WAN IPs, writing them to a standard location, and generating a procd event, and then in Nginx's init.d file, the templates are rendered and imported whenever the procd event is triggered.

Templates could just be simple shell scripts that read the written WAN IPs, and write Nginx conf to stdout.

Do you think this feature is a good idea? If so I'd be happy to implement it and send a PR.

Let me know if the description is clear.

hgl commented 1 year ago

Of course, templates are totally an addon. Only when users put scripts in certain location, say, /etc/nginx/dynamic.conf.d/site.sh, do they take effect.

Ansuel commented 1 year ago

@hgl honestly i have a similar problem and currently my solution is to disable and use a custom nginx.conf... I like the idea of adding an hotplug script and generate the site config. Ideally this should be configurable to listen for a specific interface

hgl commented 1 year ago

Cool, sounds like the feature is warranted.

There is one thing I'd like to add though.

As I read the source code of the nginx package I realized it depends on nginx-util, which I believe handles UCI and also provides SSL cert and config auto management. I have no doubt it's a useful utility to have, and am grateful to @peter-stadler for the efforts. But may I propose dropping this dependency, and instead make it an optional companion package for nginx, for the following reasons:

  1. Making nginx UCI compatible isn't that useful

    I think the major benefit of UCI is automatic LuCI app support, but given the complexity of nginx's config system, a satisfying luci-app-nginx is unlikely to happen. Having to have a mental translation between the UCI and native syntax also makes it harder to config nginx with UCI.

  2. It makes it harder to contribute to this package

    In my case, in order to contribute to this package in a deeper way, I need to understand how nginx-util works, which isn't very well documented and isn't written in a language that this package is primarily written with (shell script). This can put quite a burden on future contributors to this package, since you need to understand cpp in order to understand how nginx conf is generated. No other major packages like dnsmasq and dropbear work like this.

I'd like to propose that this package simply provides a default config with the native nginx config syntax (and nginx-mod-luci provides config for LuCI of course). And nginx-util can optionally provide UCI support (e.g., /etc/init.d/nginx favors nginx-util generated config at /var/etc/nginx.conf over /etc/nginx/nginx.conf) and SSL management.

This suggestion might seem orthogonal to the proposed PR, but it determines how the PR should be written.

Thoughts?

peter-stadler commented 1 year ago

As this seems to be the main thread, I do not spread this text over the other two parts.

For a quick background of the nginx-util:

I have no problem removing nginx-util or the UCI part if it stands in the way. But, if the default configuration comes with a server I want it with SSL.

And I want to emphasize the following use case: When one installs an app (e.g. luci-nginx) it can be made available via nginx in an automated way, such that it does not interfere with other apps. For this we need some convention: I suggest keeping /etc/nginx/conf.d or something similar and a main .conf with include /etc/nginx/conf.d/*.conf ...

For your problem(s): Can you explain a bit more what you want to achieve?

PS: a previous version of the nginx-util restricted the access to the LAN by listening on local addresses (there was a procd_add_reload_interface_trigger lan in the init script of nginx and the nginx-util then collected the IPs from ubus). Now the restriction is done by allow/deny directives.

hgl commented 1 year ago

@peter-stadler Thanks for the detailed background info.

adds SSL directives ... And it can revert this again.

This functionality can be generalized by adding self-signed or otherwise managed SSL certificates

and check_ssl for renewing the created certificates when needed (added as cron job)

Again, to implement all these features you must have put in lots of efforts, and I want to thank you for that. It made me realize that I missed the requirement of supporting both luci-nginx and luci-ssl-nginx.

On the other hand (and IMHO), I don't really think if all these feature are necessary. For example, adding/deleting options to/from user config is always flaky. It's much more reliable to simply detect a missing certificate from user config and just generate it, which is how uhttpd works I believe. And a certificate with far-future expiration date should cover 99% of the use cases. If a user wants to ensure browsers won't complain even after one or two years, she should set a up PKI or ACME.

One of the main reasons I proposed dropping nginx-util was that I was not sure if the proposed dynamic config feature would break nginx-util, and to be sure of that, I had to understand its ins and outs. It being written in a different language than what the nginx package is mostly written in, and the design of using Line to breakup nginx config syntax for example, don't help with that. And given I didn't think the complexity was warranted, I hope it's understandable that I didn't feel like spending more time with it.


To answer your questions:

Do you want to expose the server only over the WAN interface not over the LAN or ...?

Yes, for my case I simply want the ability to serve only over the WAN. @Ansuel also seems to have this use case.

If the IP changes, how do you get the actual one for accessing the server (do you have a FQDN)?

I do have a FQDN, and I use DDNS to make sure it points to the new IPs.

Or is there another problem in creating a /etc/nginx/conf.d/wan.conf server that listens on all IPs and has a server_name FQDN?

There are a few cases where listening on all IPs won't work:


I also have a few thoughts and questions regarding the background info.

Now it is separated into /etc/nginx/uci.conf

If we don't write to user config and simply detect missing certificate, I think providing a default /etc/nginx/nginx.conf should be enough.

When one installs an app (e.g. luci-nginx) it can be made available via nginx in an automated way, such that it does not interfere with other apps.

I don't think I understand. What do you mean by "other apps"? If the default config contains include conf.d/*.conf, and luci config is at conf.d/luci.conf, wouldn't luci already be automatically available? And it shouldn't interfere with other servers living in conf.d/*.conf at least?

Now the restriction is done by allow/deny directives.

I wonder why is this necessary? Shouldn't the firewall rejects INPUT packets to WAN by default?

peter-stadler commented 1 year ago

I forgot to write it before: I am appreciating your thank you very much and want to say that I like that you are putting effort into this.

Back than, I took a look at the changes of acme and although I did not investigate it deeply, I think it was a really good idea to introduce an interface there ...


If not explicitly requested, nginx-util is not touching user configs (though, on installation it will be called on the included _lan server, which I would call a finalization of a provided config). As far as I understand, the proposed dynamic config and nginx-util should work side by side.

But, I think we should put the focus on the use cases; it is good for me, if we do not need nginx-util anymore (there is no nginx/luci-nginx without ssl anymore).

Now it is separated into /etc/nginx/uci.conf

If we don't write to user config and simply detect missing certificate, I think providing a default /etc/nginx/nginx.conf should be enough.

I think I formulated it unclear: I meant the separation between

I am supposing you want to have a include conf.d/*.conf in the main config, too. Then I think we want something similar for the separation. And for the certificates I am open to changes.

When one installs an app (e.g. luci-nginx) it can be made available via nginx in an automated way, such that it does not interfere with other apps.

I don't think I understand. What do you mean by "other apps"? If the default config contains include conf.d/*.conf, and luci config is at conf.d/luci.conf, wouldn't luci already be automatically available? And it shouldn't interfere with other servers living in conf.d/*.conf at least?

My use case is to install luci-nginx and etebase (another possibility would be ariang) and both should be accessible from LAN by default (luci from https://127.0.0.1/, etebase from https://127.0.0.1/etebase).

Right now, this is done by the default HTTPS server _lan that is installed with nginx(-util), which includes all conf.d/*.locations.

Then luci-nginx and etebase just install their /etc/nginx/conf.d/luci.locations resp. /etc/nginx/conf.d/etebase.locations. Here I must take care as packager to not interfere with other apps (luci should be the only app at the root location /).

Additionally, I setup the firewall, ddns and another nginx server with the FQDN for exposing etebase (but not luci) to the WAN: it includes only conf.d/etebase.locations.

Now the restriction is done by allow/deny directives.

I wonder why is this necessary? Shouldn't the firewall rejects INPUT packets to WAN by default?

This prevents exposing the _lan server with all *.locations to the WAN (while the firewall is open for the other server).

The other possibility is to listen on all LAN IPs, but this is difficult for link local addresses and in the end I found it more straight forward to limit the access. The drawback is to listen on all addresses.


There are a few cases where listening on all IPs won't work:

  • Serving different contents on WAN and LAN.

This should work when using something like the following (note that the first part is just the provided server):

server {
       listen [::]:443 ssl default_server;
       listen 443 ssl default_server;
       server_name _lan; # or another invalid name
       ...
}
server {
       listen [::]:443 ssl;
       listen 443 ssl;
       server_name FQDN;
       ...
}

See https://nginx.org/en/docs/http/request_processing.html

- Listening on a WAN port that either you don't feel like occupying or not available on LAN.

For this case, I see only the same solution as you. And since there seems to be a need for it, I'm looking forward to your implementation of the dynamic config (Maybe it helps if you know it: Back than, I had rare timing issues).