Virtualmin 7.20.0 pre-release possible bugs

[iliajie]

Hey Jamie!

I have made a real life tests, starting with a clean install with Virtualmin 7.20.0 tagged code and discovered a few possible issues.

I usually don't use Contabo, but a few users reported some issues with it, so I decided to give it a try myself.

❗ Keep in mind that the DNS for the domain is hosted on Cloudflare, but Virtualmin is not configured to handle it. I did this intentionally to create a test case where Virtualmin doesn't have control over DNS!

1. The issue that I have fixed was related to how errors were incorrectly printed when validation on domain creation time is enabled (default) but errors are enabled to be shown (non default). The patch for this is here https://github.com/virtualmin/virtualmin-gpl/commit/d673898c58d828ea593a58f7c074c081bd26f966.

2. This issue hasn't been addressed on my side. The problem is that when we create the initial SSL certificate, Virtualmin discards domains that cannot be resolved using the new feature. However, when visiting the SSL Certificate page afterwards, Virtualmin still shows the full set of SANs, for example:

   Domain creation time:

   Setting up initial SSL certificate ..
   .. successfully requested Let's Encrypt certificate for contabo-dom3.virtualmin.dev, www.contabo-dom3.virtualmin.dev

   Later on, when visiting the SSL Certificate page, Virtualmin still shows the full set of associated domains:

   Domains associated with this server 
       contabo-dom3.virtualmin.dev
       www.contabo-dom3.virtualmin.dev
       mail.contabo-dom3.virtualmin.dev
       admin.contabo-dom3.virtualmin.dev
       webmail.contabo-dom3.virtualmin.dev

   🪲 I believe it shouldn't and should display only the domains from the last successful request.

   Question: Will this new feature work for the renewal requests made in the background? What I mean is, will Virtualmin remove SANs that aren't resolvable when renewing a domain in the background, during a cron job?

3. If domain validation isn't disabled during SSL certificate creation at domain creation time in the Virtualmin config (default), the Virtualmin validation process still fails with this error:

   Setting up initial SSL certificate ..
   .. domain validation failed : Apache website : An IPv6 DNS record www.contabo-dom3.virtualmin.dev with address 2a06:98c1:3120::3 exists, but this virtual server does not have IPv6 enabled

   The problem traces back to this initial patch https://github.com/virtualmin/virtualmin-gpl/commit/d7306b995b5adc6de7d07c1ac160974f426ef1c3 .. however, I don't believe that this code is the issue on its own.

   🪲 I don't think we can reliably validate IPv6 addresses in this configuration scenario with CF proxy enabled. My suggestion is to disable IPv6 validation completely when an SSL certificate is requested during the initial domain creation, where the user doesn't have the option to disable validation.

[iliajie]

4. Another issue is with CGI scripts support being disabled, when it should actually be enabled. In the previous release, we didn't enable CGI mode correctly by default, cause we forgot to add this new option to the config, and therefore left this feature disabled for all systems, which may be a problem. The current code here still won't enable CGI on systems where it should be enabled. Right now, all those configs have cgimode set to '' (i.e., an empty string).

   Question: Shouldn't the linked line above be if (defined($tmpl->{'web_cgimode'}) && instead? I realize that we don't set none for the CGI mode disabled state and can actually override a user's wish to have CGI disabled. However, it's a trade-off, as we currently have all CGI scripts disabled, which may be a problem for features like mail autodiscover and AWStats, at the very least!

   🪲 My suggestion for a simple fix is to check for defined($tmpl->{'web_cgimode'}) in 7.20.0 upcoming release. In the 7.30.0 release, we can revert it and check again for an empty string.

[jcameron]

I cleaned up your commit in https://github.com/virtualmin/virtualmin-gpl/commit/f54f4b7368eafddd83e4cb73e3e452897ad8b75f

[jcameron]

1. Thanks for that fix!

2. The "Domains associated with this server" are those that we'd ideally like to include in the cert. The "Current certificate" tab shows hostnames actually in the cert. And yes, during renewal it will continue to remove unresolvable hostnames from the request.

3. That is a real issue we're detecting. If there is an IPv6 DNS record and apache isn't accepting IPv6 connections, Let's Encrypt will try to use a v6 address and fail.

4. Because cgimode wasn't set at all in the config in older versions, when 7.20 is installed the cgimode=suexec line should get copied from the default config file into /etc/webmin/virtual-server/config since it's not there yet.

[iliajie]

I cleaned up your commit in https://github.com/virtualmin/virtualmin-gpl/commit/f54f4b7368eafddd83e4cb73e3e452897ad8b75f

1. Thanks for that fix!

You're welcome. However, your clean-up isn't correct from my perspective. The whole point of my fix was to handle different types of data, because @err can contain a plain list or a list of hash references. Your clean-up changes the logic and essentially brings back the original issue, but from the other way around. Please revert it.

2. The "Domains associated with this server" are those that we'd ideally like to include in the cert. The "Current certificate" tab shows hostnames actually in the cert.

Alright!

And yes, during renewal it will continue to remove unresolvable hostnames from the request.

If you're sure about it, then we're safe in this case!

3. That is a real issue we're detecting. If there is an IPv6 DNS record and apache isn't accepting IPv6 connections, Let's Encrypt will try to use a v6 address and fail.

Yes, though I don't think we can address this easily. In my example, www.contabo-dom3.virtualmin.dev hosted in Cloudflare returns an IPv6 record when dig AAAA www.contabo-dom3.virtualmin.dev, but the DNS zone for virtualmin.dev in Cloudflare does not contain any IPv6 records at all.

I think, in the case of unattended requests, we shouldn't check or validate IPv6 at all, as it has been causing a lot of issues

4. Because cgimode wasn't set at all in the config in older versions, when 7.20 is installed the cgimode=suexec line should get copied from the default config file into /etc/webmin/virtual-server/config since it's not there yet.

Yes, I understand that.

However, my concern is that some users might have already saved a template.

[jcameron]

You're welcome. However, your clean-up isn't correct from my perspective. The whole point of my fix was to handle different types of data, because @err can contain a plain list or a list of hash references. Your clean-up changes the logic and essentially brings back the original issue, but from the other way around. Please revert it.

How can @err contain anything other than a list of hash refs? That's all that validate_letsencrypt_config returns..

I think, in the case of unattended requests, we shouldn't check or validate IPv6 at all, as it has been causing a lot of issues

I mean we could turn that off for this case, but in other cases it will lead to failed LE requests that are hard for users to understand.

However, my concern is that some users might have already saved a template.

Maybe we need to check if the first version installed was below 7.10.0, and if so set web_cgimode to the first supported mode if it's not currently set. Also, I think we need to introduce a none option rather than just leaving it empty.

[iliajie]

How can @err contain anything other than a list of hash refs? That's all that validate_letsencrypt_config returns..

Hmm, I think I incorrectly assumed that the @err could also be a list. Since validate_letsencrypt_config always returns a list of hash refs, we're good then! Sorry for the false alarm here.

I mean we could turn that off for this case, but in other cases it will lead to failed LE requests that are hard for users to understand.

I agree. But how do we deal with this then? Perhaps we don't check for IPv6 if the domain's dns_cloud isn't configured and if DNS isn't locally hosted? This might work, right?

Maybe we need to check if the first version installed was below 7.10.0, and if so set web_cgimode to the first supported mode if it's not currently set.

Cool, that could work! Clever idea!

Also, I think we need to introduce a none option rather than just leaving it empty.

Well, it may break existing installs. I think we can just handle it the way you just suggested.

[jcameron]

I agree. But how do we deal with this then? Perhaps we don't check for IPv6 if the domain's dns_cloud isn't configured and if DNS isn't locally hosted? This might work, right?

How can Virtualmin tell if the domain isn't hosted locally though? It does skip that IPv6 check already if dns_cloud is enabled.

[iliajie]

How can Virtualmin tell if the domain isn't hosted locally though?

It's not easy to do exact, I don't think.

It does skip that IPv6 check already if dns_cloud is enabled.

Yeah, I know. But couldn't we add more checks to avoid false-positive results for such case scenarios?

[jcameron]

Not sure how we can do that without failing to detect true positives!

[iliajie]

I think we should handle this for specific DNS providers, particularly when Virtualmin doesn't manage the DNS. For example, with Cloudflare, if the Virtualmin system doesn't have control over DNS (i.e. dns_cloud not set) but the domain's name servers point to Cloudflare's name servers, it's nearly always a false positive regarding missing IPv6.

We could use a dedicated function for this and add other DNS services later if needed.

[jcameron]

Maaaybe .. but that's going to be hard to do reliability, because it depends on knowing that DNS is not just hosted elsewhere, but also at a provider like Cloudflare that does proxying.