www/caddy: Add option to compile only the needed DNS Provider

Monviech commented 7 months ago

Important notices Before you add a new report, we ask you kindly to acknowledge the following:

[x] I have read the contributing guide lines at https://github.com/opnsense/plugins/blob/master/CONTRIBUTING.md
[x] I have searched the existing issues, open and closed, and I'm convinced that mine is new.
[x] When the request is meant for an existing plugin, I've added its name to the title.

Is your feature request related to a problem? Please describe.

The problem is, that the current caddy build is unmaintainable, because the DNS Provider modules are creating incompatibilities between each other. This can happen at any time, because of the dependency structure of golang. Also, this kind of build is not recommended by the developers, only the DNS module that is used, should also be compiled in. https://caddy.community/t/having-trouble-with-reproducable-builds-with-caddy-dns-providers/23238

Describe the solution you'd like

The caddy-custom binary build in opnsense ports will exclude all DNS Provider modules, and only add the NTLM and Dynamic DNS non standard modules.
There will be a button in the GUI called "Compile DNS Provider", which resides in the Dynamic DNS Tab, with an explanation.
When pressed, it will trigger a REST API call, and an action, which will trigger a php script.
Inside this script, there is a new function that will read the currently selected dns provider from the model, then use "caddy add-package MODULENAME", which can compile a new module in on the fly by downloading a new caddy binary from the caddy repo and replacing the current one with the new one. (https://caddyserver.com/docs/command-line#caddy-add-package)
To validate this, another function will call "caddy list-modules --skip-standard" and parse the non standard modules. https://caddyserver.com/docs/command-line#caddy-remove-package
This can be rolled back with https://caddyserver.com/docs/command-line#caddy-remove-package - which can be used to recompile excluding the DNS Provider module.

Describe alternatives you've considered

The alternative would be, that this kind of setup would be possible in the CLI, and there is information in the GUI for the user which cli action is necessary for their chosen DNS Provider to start working. I have seen other Plugins support this CLI only approach, most recent example is Crowdsec with their .yaml parsers, or using the crowdsec hub for additional modules.
Another alternative would be, to totally remove the current DNS Provider, DnyDns and DNS-01 challenge features. Though that would definitely cripple the usecases of the plugin a lot.

Additional context

This kind of solution undermines the packet manager. The binary of the new compiled version is a different one than the installed one. Their SHA hashes differ, don't know if the paket manager will remove or replace this custom binary in the future.
It's not update persistent. As soon as a new packet is pushed from the OPNsense ports, it will overwrite the custom one of the user. That means the user will have to customize it again afterwards.

All in all, this is the biggest issue my plugin is facing right now, and whichever solution is chosen will have ups and downs. There is no perfect solution right now.

Short term solution

Offering customized help text in the GUI that's context sensitive to the currently chosen DNS-Provider with the shell commands needed to install it, would be the best short term solution. The features used are marked "Experimental" in Caddy v.2.7.6, so there is no indication how this will play out in future versions. Spending lots of effort in a GUI implementation without a feature being marked as "stable" is not really in my interest.
The current input validation already prevents mistakes, because it will give feedback when a DNS Provider is missing and won't start Caddy.

Monviech commented 7 months ago

Example of a successful module addition:

root@OPNsense:/usr/local/bin # caddy add-package github.com/caddy-dns/cloudflare
2024/03/24 07:27:54.625 INFO    this executable will be replaced        {"path": "/usr/local/bin/caddy"}
2024/03/24 07:27:54.625 INFO    requesting build        {"os": "freebsd", "arch": "amd64", "packages": ["github.com/caddy-dns/cloudflare"]}
2024/03/24 07:27:55.178 INFO    build acquired; backing up current executable   {"current_path": "/usr/local/bin/caddy", "backup_path": "/usr/local/bin/caddy.tmp"}
2024/03/24 07:27:55.178 INFO    downloading binary      {"destination": "/usr/local/bin/caddy"}
2024/03/24 07:27:57.807 INFO    download successful; displaying new binary details      {"location": "/usr/local/bin/caddy"}

Module versions:

dns.providers.cloudflare v0.0.0-20231220181002-8789126791ed

  Non-standard modules: 1

  Unknown modules: 0

Version:
v2.7.6 h1:w0NymbG2m9PcvKWsrXO6EEkY9Ru4FJK8uQbYcev1p3A=

2024/03/24 07:27:57.894 INFO    upgrade successful; please restart any running Caddy instances  {"executable": "/usr/local/bin/caddy"}

Example of a successful module removal:

root@OPNsense:/usr/local/bin # caddy remove-package github.com/caddy-dns/cloudflare
2024/03/24 07:29:27.523 INFO    this executable will be replaced        {"path": "/usr/local/bin/caddy"}
2024/03/24 07:29:27.523 INFO    requesting build        {"os": "freebsd", "arch": "amd64", "packages": []}
2024/03/24 07:29:28.150 INFO    build acquired; backing up current executable   {"current_path": "/usr/local/bin/caddy", "backup_path": "/usr/local/bin/caddy.tmp"}
2024/03/24 07:29:28.150 INFO    downloading binary      {"destination": "/usr/local/bin/caddy"}
2024/03/24 07:29:30.674 INFO    download successful; displaying new binary details      {"location": "/usr/local/bin/caddy"}

Module versions:

  Non-standard modules: 0

  Unknown modules: 0

Version:
v2.7.6 h1:w0NymbG2m9PcvKWsrXO6EEkY9Ru4FJK8uQbYcev1p3A=

2024/03/24 07:29:30.755 INFO    upgrade successful; please restart any running Caddy instances  {"executable": "/usr/local/bin/caddy"}

fichtner commented 7 months ago

To be frank I'd advise against making a barely manageable dyndns subsystem more complex and I believe that loading binary files from a remote location is a security risk we do not want to be introducing in the public plugins repo. Let alone the work for code and the risk for regressions that cannot be traced.

Cheers, Franco

Monviech commented 7 months ago

@fichtner If that's the case I have to remove the whole DNS Provider feature. I haven't expected this to turn out like this when I implemented it.

Current state of the DNS Provider feature:

It is very hard to maintain
Introduces regressions randomly
Is a pain to compile reproducible
Precompiling all of them is not supported in this way by the caddy developers.
Support of it can potentially become a huge time sink with little benefit. (It already is... it feels like cancer in an otherwise sleek working plugin)

The way forward:

Since this plugin supports custom certificates, people who need the DNS-01 challenge could use the acme client plugin to get a wildcard certificate for the subdomain feature.
And people who need DynDNS could use the ddclient plugin.
This will create a clear separation of concerns.

All in all, I leave this decision up to you, and I will follow what you suggest as the best way forward.

fichtner commented 7 months ago

Most of these problems appear to be inherent to DynDNS as we have seen over the years (individual hobby projects glued together not supported by enterprises at all) and the way caddy outsources them to avoid trouble in their own project.

My recommendation is to leave the current state as it is, only improve with actual user reports and confirmations and/or consider removal of actually broken integrations later on.

What do you think?

Monviech commented 7 months ago

We already can't leave it as it is anymore, since the libdns version of cloudflare was bumped, and that makes it collide with most other already compiled dns providers.

Next time you try to build caddy-custom it will fail. (Already checked that with opnsense tools)

If this current feature set should be retained, we have to fork the currently DNS Provider Plugins, and use the forked ones as build reference.

Or... -I have to check that-, specify the exact git commit hashes when compiling from their repos, so that it doesn't pull latest with all plugins, but the state we know that works.

But since those plugins are all community maintained (except cloudflare, which the caddy developers seem to have an eye on), this will break more and more in the future when version go even more out of sync between different plugins.

This seems to be supported by go, so I could freeze the current working build by including commit hashes to all additional modules: github.com/someone/some_module@af044c0995fe

EDIT: Example, currently testing.

https://github.com/Monviech/opnsense-tools/blob/f27010f538da62fa2c80560e40075097dbba3d8e/config/24.1/make.conf#L97

Monviech commented 7 months ago

@fichtner Yup, specifying the commit hash makes the build go through.

2024/03/24 09:56:13 [INFO] Build complete: ./caddy
2024/03/24 09:56:13 [INFO] Cleaning up temporary folder: /tmp/buildenv_2024-03-24-0954.3907689736
[20240324095613] ===> Staging for caddy-custom-2.7.6.3.0.3.5.3_15
[20240324095613] ===> Generating temporary packing list
install  -s -m 555 /usr/obj/usr/ports/www/caddy-custom/work/caddy-custom-2.7.6.3.0.3.5.3/caddy /usr/obj/usr/ports/www/caddy-custom/work/stage/usr/local/bin
[20240324095613] ====> Compressing man pages (compress-man)
[20240324095613] ===> Staging rc.d startup script(s)
[20240324095613] ===> Installing for caddy-custom-2.7.6.3.0.3.5.3_15
[20240324095613] ===> Checking if caddy-custom is already installed
[20240324095613] ===> Registering installation for caddy-custom-2.7.6.3.0.3.5.3_15
Installing caddy-custom-2.7.6.3.0.3.5.3_15...
Creating package for bash-5.2.26_1
bash-5.2.26_1 already packaged, skipping...
Creating package for caddy-custom-2.7.6.3.0.3.5.3_15

So I will put the commit hashes behind each of the plugins I use. And if a plugin breaks in the future we will remove it.

fichtner commented 7 months ago

Sounds like a plan. I haven’t noticed it breaking yet but I can confirm that nightly didn’t build caddy-custom after checking the log.

Monviech commented 7 months ago

https://github.com/opnsense/ports/pull/190 https://github.com/opnsense/tools/pull/400

Ran the build a few times, always works, always takes the same sources. I think it might be fixed for a while now.

Monviech commented 7 months ago

The current build with the commit hashes is successful. I will keep an eye out for the state of these plugins. The current issues seem mitigated for now.

A DNS provider plugin will be removed if:

they're unmaintained for too long (like years of no updates)
they make build problems
users report errors with the functionality

A DNS Provider plugin will be added if:

it's explicitely requested
it doesn't conflict with the build
it is in the caddy-dns repository and doesn't look unmaintained

fichtner commented 7 months ago

@Monviech yep, good policy

Monviech commented 7 months ago

Adding a list about the current state of DNS Plugins:

https://github.com/opnsense/plugins/issues/3872

opnsense / plugins

www/caddy: Add option to compile only the needed DNS Provider #3867