Skip to content

Added support for ddns lookups for addresses in access lists #3364

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

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

vari
Copy link

@vari vari commented Dec 3, 2023

Added support for ddns:somedomain.whateverddns.com address format in the client access lists.

This allows users to specify domain names instead of IP addresses for allow/deny lists, thereby allowing dynamic allow/deny lists.

This is useful if users have a service exposed to the public internet via a ddns domain, and they want to limit it so that only users from the local network can access the service on the ddns domain.

In theory, this is probably already possible by using custom DNS server to prevent any local network request to the domain name from going outside to the internet (and then setting allow list to local subnet in proxy manager), but not everyone can (or will) use a custom DNS server for their setup.

The new ddns support makes it trivially easy for anyone to limit to local network if they are using ddns without having to mess with custom DNS servers or network configuration. Also, if users want to expose their service to a fixed number of external users, then the ddns lookup can be used with the allow list provided the external users are using a ddns service. E.g. if I want to share a service on my network to 2 friends, and each friend uses a ddns that points to their public IP (friend1.domain.com, friend2.domain.com), then I can just add ddns:friend1.domain.com and ddns:friend2.domain.com to my allow list in proxy manager and they will continue to have access even if their public IP changes. I won't have to manually go an update the access list every time the IP changes.

This should address #1708 and #2240 .

Compared to some of the existing solutions mentioned in the above issues, this implementation should be the simplest with minimal overhead and no other dependencies (e.g. no cron, env vars, etc needed). Can directly specify the domains in the normal allow list UI.

Usage:

  • Prefix the dynamic domain/host you want to use for the access list with ddns:
    e.g. If you want to add the dynamic hosts yourdomain.ddns.com and yourdomain2.ddns.com to your allow list, do the following:
    image
  • Upon saving the access list, any associated hosts will automatically be updated to use the resolved IP of the dynamic domain/host in the access list (and this will trigger a nginx reload so that changes take effect).
  • The proxy manager will also periodically poll the dynamic domains, and update any proxy hosts that are using those domains if there is an IP address update. Default interval is 1 hour, can be configured by setting the DDNS_UPDATE_INTERVAL env var to the desired number of seconds (minimum 60).
    On start up, all used domains will be resolved and any associated hosts will be updated in nginx about 10s after the proxy manager starts (10s buffer ensures server has enough time to finish loading).

Disclaimers:

  • I haven't used js in almost 6 years, do not be surprised if the code is inefficient / not following best practices. Suggestions for cleaning up and refactoring the code to make it more efficient/readable are welcome!
  • I'm using getent hosts <hostname> to look up the IP of the user defined domains - if there is a better way, please let me know and I can update the PR. I've tried to make it safe by using spawn instead of exec to prevent issues with unsanitized user inputs, however I'm not doing any custom sanitization.

@vari
Copy link
Author

vari commented Dec 3, 2023

Update the CI build is working. The easiest way to test/use the changes is to use jc21/nginx-proxy-manager:github-pr-3364 as the image for your nginx proxy manager container.

E.g. in your docker compose file, replace:
image: 'jc21/nginx-proxy-manager:latest'
with
image: 'jc21/nginx-proxy-manager:github-pr-3364'


Manual instructions (no longer needed, but kept in case the above image does not work). You can test the changes in this PR by doing the following:
  • Save the following files to whatever directory you use for /data with nginx proxy manager:
  • Change the permissions on the downloaded files and directories so that they are readable (chmod o+r on each file/directory)
  • Update the volumes section in your compose file so that it includes the following paths:
    your-data-path is the path to your data folder on the host
volumes:
  # Add these
  - your-data-path/index.js:/app/index.js
  - your-data-path/logger.js:/app/logger.js
  - your-data-path/nginx.js:/app/internal/nginx.js
  - your-data-path/access-lists.json:/app/schema/endpoints/access-lists.json
  - your-data-path/utils.js:/app/lib/utils.js
  - your-data-path/ddns_resolver/ddns_resolver.js:/app/lib/ddns_resolver/ddns_resolver.js
  - your-data-path/ddns_resolver/ddns_updater.js:/app/lib/ddns_resolver/ddns_updater.js
  • Redeploy the compose file (docker compose down && docker compose up -d)

@vari vari force-pushed the access-list-client-ddns-support branch 2 times, most recently from 51f7715 to dd8ded7 Compare December 4, 2023 00:56
@sgelineau17
Copy link

Hello,

Is it possible for the NPM build to be updated to the latest version?

Is your change expected to be added to the final version of NPM?

Is it possible to add a block by country?

Is it possible to add the use of FQDN for list access and not necessarily DDNS.

Thanks for you job :)

@sgelineau17
Copy link

an update ?

I just detected a bug, we can use an fqdn after "ddns:" it should work, the problem is that for a ddns entry or whatever we can't have a "-" in the name, like "ddns:example-ddns.com" doesn't work

Thanks for your job !

vari and others added 8 commits April 28, 2024 17:58
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
@vari vari force-pushed the access-list-client-ddns-support branch from bae32ee to e317900 Compare April 29, 2024 01:02
@vari
Copy link
Author

vari commented Apr 29, 2024

an update ?

I just detected a bug, we can use an fqdn after "ddns:" it should work, the problem is that for a ddns entry or whatever we can't have a "-" in the name, like "ddns:example-ddns.com" doesn't work

Thanks for your job !

I no longer use this but the fix to support - in the the ddns field is pretty straight forward. I've updated this PR with the required changes (and pulled in the latest changes) but I will not be able to test it. Once the build is completed, you can try out the updated image and let me know if you run into issues.

@nginxproxymanagerci
Copy link

Docker Image for build 8 is available on DockerHub as jc21/nginx-proxy-manager:github-pr-3364

Note: ensure you backup your NPM instance before testing this PR image! Especially if this PR contains database changes.

@LBenotsch
Copy link

Is this ready to merge? I'd love to use this feature. Thanks.

@Cloudineer
Copy link

Hi All,

just found this PR - exactly what I was looking for so I am testing it now. I can reboot my router to get a new IP address out of working hours to test the change of IP address. Are there any logs/ checks that would be useful when I run the test?

Thanks for the code. I did look at doing something myself, but js is not my thing, so happy to be a tester,

@Cloudineer
Copy link

It worked in the described way. Log shows:

[7/20/2024] [7:42:30 AM] [DDNS     ] › ℹ  info      Checking for DDNS updates...
[7/20/2024] [7:42:30 AM] [DDNS     ] › ℹ  info      Found 1 address(es) in use.
[7/20/2024] [7:42:30 AM] [DDNS     ] › ℹ  info      1 DDNS IP(s) updated.
[7/20/2024] [7:42:30 AM] [DDNS     ] › ℹ  info      Updating 7 proxy host(s) affected by DDNS changes
[7/20/2024] [7:42:30 AM] [Global   ] › ⬤  debug     CMD: /usr/sbin/nginx -t -g "error_log off;"
[7/20/2024] [7:42:30 AM] [Nginx    ] › ℹ  info      Reloading Nginx
[7/20/2024] [7:42:30 AM] [Global   ] › ⬤  debug     CMD: /usr/sbin/nginx -s reload
[7/20/2024] [7:42:30 AM] [DDNS     ] › ℹ  info      Finished checking for DDNS updates

The hosts configured using the access list which is dynamic IP address only, are now accessible. Those hosts did appear as 403 forbidden between the times of the IP address change and the processing above. My DDNS hostname does not have a hyphen. For the record I used image 'jc21/nginx-proxy-manager:github-pr-3364', downloaded 19th July 2024.

The DDNS update runs every hour. I would prefer a quicker response; can I suggest checking every 15 minutes.

Other than that, it all works great, so I think it is ready to merge into the next release.

Thanks again to those who have worked on this PR.

@Cloudineer
Copy link

How to change DDNS update interval

After a little digging in the code, I saw a comment that the ddnsUpdater._updateIntervalMs can be overridden using the env var DDNS_UPDATE_INTERVAL. So I added DDNS_UPDATE_INTERVAL: 900 to the environment section for the app in my docker-compose.yml (see below) and it now checks for DDNS IP address change every 15 minutes (900 seconds = 15 * 60). The only slight gotcha is that DDNS_UPDATE_INTERVAL is in seconds, whereas the _updateIntervalMs variable is in ms (which makes perfect sense, but I got it wrong the first time).

So please ignore my comment above, it is configurable in docker. It just ran after 15 minutes, so this override is working.

To help others change the interval in the future, see the line starting DDNS_UPDATE_INTERVAL in my docker-compose.yml below (passwords obscured):

services:
  app:
    image: 'jc21/nginx-proxy-manager:github-pr-3364'
    restart: unless-stopped
    ports:
      - '80:80' # Public HTTP Port
      - '443:443' # Public HTTPS Port
      - '81:81' # Admin Web Port
    environment:
      DB_MYSQL_HOST: "db"
      DB_MYSQL_PORT: 3306
      DB_MYSQL_USER: "npm"
      DB_MYSQL_PASSWORD: "my secret"
      DB_MYSQL_NAME: "npm"
      DISABLE_IPV6: 'true'
      DDNS_UPDATE_INTERVAL: 900
    volumes:
      - ./data:/data
      - ./letsencrypt:/etc/letsencrypt
    depends_on:
      - db

  db:
    image: 'jc21/mariadb-aria:latest'
    restart: unless-stopped
    environment:
      MYSQL_ROOT_PASSWORD: 'not telling you'
      MYSQL_DATABASE: 'npm'
      MYSQL_USER: 'npm'
      MYSQL_PASSWORD: 'my secret'
    volumes:
      - ./mysql:/var/lib/mysql

@vari
Copy link
Author

vari commented Jul 26, 2024

Glad to see that this is helping folks. In terms of merging it into the main project, I'd love to but whether or not this gets merged is up to the maintainers. As you can, see there are currently 50+ open PRs (many of which are older than this one) so I wouldn't hold out hope of this being merged anytime soon.

The best option in the meantime is to continue to use the PR branch image (jc21/nginx-proxy-manager:github-pr-3364). If I get some time I'll try and rebase the changes in this PR to the latest develop branch (no guarantees as I no longer use this and don't have much free time these days). If anyone wants to take over or attempt rebasing these changes, feel free to do so!

@sgelineau17
Copy link

sgelineau17 commented Aug 29, 2024

an update ?
I just detected a bug, we can use an fqdn after "ddns:" it should work, the problem is that for a ddns entry or whatever we can't have a "-" in the name, like "ddns:example-ddns.com" doesn't work
Thanks for your job !

I no longer use this but the fix to support - in the the ddns field is pretty straight forward. I've updated this PR with the required changes (and pulled in the latest changes) but I will not be able to test it. Once the build is completed, you can try out the updated image and let me know if you run into issues.

Hello,

Thanks for your support.

I just tried, we no longer have an error when we configure a ddns entry with one or several - in the host name but after several cross tests, the access does not work behind. Are your changes expected to be integrated into NPM? Can you update your merge with the last NPM release ?

edit : I have try to pull the last docker images and now the ddns wasn't working :/

Thanks again !

@sylphrena0
Copy link

I made #4386 address merge conflicts, if you'd like to use these changes, you can use the build from that PR nginxproxymanager/nginx-proxy-manager-dev:pr-4386. It seems they moved the location of the PR builds.

@GER-PAL
Copy link

GER-PAL commented Mar 26, 2025

Hello,
Any chance that this feature gets implemented into the main branch?
What can I do for this to happen?

@virtualdj
Copy link

What can I do for this to happen?

We should convince the maintainer @jc21 to do that

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

Successfully merging this pull request may close these issues.

7 participants