Added support for ddns lookups for addresses in access lists (resolved merge conflicts from #3364) #4386
Conversation
This is a first pass attempt at adding support for using ddns (really any resolvable domain name) as the address in access list clients. This helps make it possible to restrict access to hosts using a dynamic public IP (e.g. allow access to a proxied host from your local network only via ddns address). Current approach is hacky since it was developed by manually replacing files in an existing npm docker container. Future commits will integrate this better and avoid needing to patch/intercept existing APIs. See associated PR for more details.
Refactored ddns resolver so that no patching is done. nginx.js will automatically resolve ddns addresses if needed. Added dedicated logger scope for ddns resovler.
Other changes: - Fixed null property read error on clients (when switching to public access) - Use separate `resolvedAddress` field for resolved IP instead of overwriting address - Reduced ddns log verbosity
…port' into develop
|
I've just given this a try and it doesn't seem to work for me? I tried adding a dynamic domain to one of my existing access lists prefixed with |
Yep, I ran into the same issue. I'm guessing there have been other changes since the previous PR, I'm going to actually dig into the code instead of doing a simple merge and hoping the previous implementation holds up. |
I've just done a tiny bit of testing and discovered that it recognises the DDNS IP(s) if I restart the proxy after adding one. However there's also another issue, the IP that is being resolved for me is IPv6 not IPv4 which results in me still not having access 😂 |
Yeah, I'm also not sure why they used the ddns prefix instead of RegEx to match valid domains/subdomains, since no other access hosts would accept a domain. I'm looking into it, but do let me know if you find any solutions! |
|
I pushed a change that should resolve addresses to ipv4 and changed the syntax of ddns entries to remove that prefix. I'm still investigating why entries only work after a restart |
…ns/subdomains prefixed with ddns, update resolution to return ipv4 addresses
Co-authored-by: Jenson R <[email protected]>
|
If the IP fails to resolve and fallbacks to the domain, it currently still gets added to the nginx config which causes which initially is given as a warning in the logs (the website just shows a popup saying "Internal Error") however if you then restart the proxy, nginx fails to start |
Good catch. The previous author added a check for this in the |
|
Docker Image for build 16 is available on Note: ensure you backup your NPM instance before testing this image! Especially if there are database changes |
|
Yea this works as intended, but when applying changes in ACL tab I get a "400 internal error" that shows no message, but actually after hitting F5 the changes are saved and ends up working. |
If you get an 400 internal error then that likely means the nginx config failed the test and will fail to start whenever you next restart the nginx-proxy-manager (at least that's what happened for me anyway). Could be something else erroring though ofc |
|
Nono its actually working, weird tho. |
Well yeah like I said it worked fine for me until I restarted the proxy manager in which nginx failed to start and I had to manually go into the nginx configs and remove the lines that were causing it to break. That was apparently fixed though so might be a different issue you are having |
|
It looks like the last run failed and the helpful bot didn't post the reason, so some of those fixes might not be in the latest pr image. I will probably take a look this weekend, but y'all are welcome to poke around The error is likely an attempt to call a nonexistent method to update the DNS records on creation/update of ddns records, so it would make sense that it would work on restart. This is fixed in the last commit, the build that is failing. maybe I'll see if I can properly setup their dev environment so I don't need to wait for failed ci runs that I can't inspect to test lol |
Resolves conflicts from @vari's #3364 with a merge commit.