opnsense / plugins

OPNsense plugin collection
https://opnsense.org/
BSD 2-Clause "Simplified" License
810 stars 593 forks source link

www/caddy: Add error message when OPNsense WebGUI settings conflict with Caddy #3999

Closed Monviech closed 1 month ago

Monviech commented 1 month ago

Caddy can not start when port 80 and 443 are in use. A conflict with other plugins can cause this, yet the majority of cases is a conflict with the WebGUI due to default settings.

This error message in General Settings helps users to fix the most common error why Caddy refuses to start (without emitting usable logs in the Log Viewer).

I have stumbled upon this multiple times myself, and it's confusing every time, especially in new installations. I want to eradicate this nonsense once and for all.

Fixes: https://forum.opnsense.org/index.php?topic=38714.msg199411#msg199411 (Thats only one example, I wonder how many confused users simply give up and remove the plugin, not everybody reads the docs)

AdSchellevis commented 1 month ago

Wouldn't it be better to move port validations to the model? (although throwing a usable error to the log would likely also fix the problem)

At a first glance it often looks like a good idea to signal specific cases, but in the long run you risk ending up with a lot of glue which has the tendency to grow with countless exceptions.

Monviech commented 1 month ago

This is the one specific case that causes the most grief with users. I know its not generalized programatically, and I feel bad about it, but preemtively throwing this error message will definitely help.

I don't per se validate anything here, it's just a very useful hint. If the glue is too much, I can hard code it as information in the general volt file statically.

Edit:

If I would do it in the model, how can I access the legacy configuration? Does System Webgui have a model? Where would I throw the validation error? At the enabled key?

AdSchellevis commented 1 month ago

This is the one specific case that causes the most grief with users. I know its not generalized programatically, and I feel bad about it, but preemtively throwing this error message will definitely help.

I don't per se validate anything here, it's just a very useful hint. If the glue is too much, I can hard code it as information in the general volt file statically.

If you want it in, I don't mind that much, just mentioning my concerns.

If I would do it in the model, how can I access the legacy configuration? Does System Webgui have a model? Where would I >throw the validation error? At the enabled key?

When you rather validate if the port overlaps, I can help you there, no problem. You can access the config object from the model and use legacy components as well. The safest option for validating ports is to only validate when the gui is not bound to specific interfaces (recommended option).

Just let me know if you need some help here, I'll try to find an example for you.

Monviech commented 1 month ago

I take your concerns seriously since you have more experience than me.

I need to validate two things, the webgui port, and the disable gui redirect rule.

The problem with the redirect rule is that it can be null when not configured (aka the xml key totally vanishes from the config)

I would really like examples if you can help me. :)

AdSchellevis commented 1 month ago

I'll try to offer an example later this week.

Monviech commented 1 month ago

Oops, I actually got it working. What you said earlier gave me to think. Also, this way it was much easier... oh well.

Monviech commented 1 month ago

I will tether the disablehttpredirect check (port 80) to the state of autohttps on, since that creates port 80 when caddy starts.

The port 443 default webgui check will be tethered to "FromPort" of a reverse proxy domain or subdomain.

This way there is the most flexibility and its unlikely current setups are impacted with the new validation.

Monviech commented 1 month ago

@AdSchellevis

I thought a lot about it, and the current refactor strikes the right balance between code simplicity, low impact to current configurations, and guiding users into the right direction on first setup.

Tethering it to AutoHTTPS makes the most sense, since that setting spawns the ports for the Lets Encrypt HTTP-01 (80) and TLS-ALPN-01 (443) challenge. Turning it off, allows users to use Caddy with customized certificates without being validated.

AdSchellevis commented 1 month ago

@Monviech want me to review the current state or are you planning additional changes?

Monviech commented 1 month ago

I would like to have this reviewed when you have time. I don't plan to change it much anymore. This already used a lot more time than I expected. xD

Monviech commented 1 month ago

Thank you a lot for the extensive feedback. I made sure the code is well readable without many comments by also taking extra care with the variable names.

AdSchellevis commented 1 month ago

@Monviech thanks!