Letsencrypt renewal issue since Virtualmin 7.20.0

[websmurf]

Most likely related to this change:

Add ability to check the resolvability of alternative names before issuing a Let's Encrypt certificate

I have a setup with a lot of sites that have their root record to a certain ip-address, and also aliasses for the www and mail subdomains. For example: bandhosting.be

The certificate setup in virtualmin is:

The result of a certificate renewal is, that the mail.bandhosting.be and www.bandhosting.be domains will not be requested due to the cleanup:

They all do point to the same ip though:

The result is a certificate, that is only valid for the root domain, and not the www and mail subdomains: https://bandhosting.be/ https://www.bandhosting.be/ https://mail.bandhosting.be/

[iliajie]

Hello Adam!

Thanks for the heads up!

Adam, this is indeed a bug on the Virtualmin side, as when we use check_external_dns() API, we only consider IPv4 address and not CNAME record! To work around this issue for now, you should turn off the resolvability check.

Jamie, to be clear, we need to consider CNAME record as legitimate as well, e.g.:

dig www.bandhosting.be @8.8.8.8

; <<>> DiG 9.10.6 <<>> www.bandhosting.be @8.8.8.8
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 28793
;; flags: qr rd ra ad; QUERY: 1, ANSWER: 2, AUTHORITY: 0, ADDITIONAL: 1

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 512
;; QUESTION SECTION:
;www.bandhosting.be.        IN  A

;; ANSWER SECTION:
www.bandhosting.be. 3600    IN  CNAME   bandhosting.be.
bandhosting.be.     3600    IN  A   37.97.169.184

;; Query time: 113 msec
;; SERVER: 8.8.8.8#53(8.8.8.8)
;; WHEN: Sun Jul 21 19:47:43 EEST 2024
;; MSG SIZE  rcvd: 77

[iliajie]

Adam, please check this https://github.com/virtualmin/virtualmin-gpl/commit/a287e6c3a4519c9e7051d145cf7bbb141ceef8f9 patch and see if it solves the problem for you?

[jcameron]

Thanks Ilia for the fix!

[iliajie]

Welcome!

[websmurf]

Just applied the patch and after a webmin restart:

Thanks for the swift response!

[iliajie]

I'm glad it worked!

[aqueos]

hi,

Sorry to barge in but i think this should be chnaged, the function is not future proof and will cause issues in the near future.

This function has some desing that hurts me because:

1/ it use external tools and not Net::DNS, output could be unreliable, Net::DNS seems installed on all my systems so is it in the basic install ?

webmin use net::DNS in /usr/share/webmin/bind8/bind8-lib.pl /usr/share/webmin/bind8/edit_zonedt.cgi /usr/share/webmin/bind8/cpan_modules.pl so it should be there.

2/ it does not support ipv6 or cname (well now it does for cname)

3/ it use hardcoded google DNS 8.8.8.8, this will fail on secure network that force certain DNS server with firewall rules.

Dont you think this will be an issue ?

as i dont know perl i asked our friend gpt about this, tested it and seems to be working on cname and normal domains also.

#!/usr/bin/perl

use strict;
use warnings;
use Net::DNS;

# Function to validate IPv4 addresses
sub is_valid_ipv4 {
    my ($ip) = @_;
    return $ip =~ /^(\d{1,3}\.){3}\d{1,3}$/ && !grep { $_ > 255 } split(/\./, $ip);
}

# Function to validate IPv6 addresses
sub is_valid_ipv6 {
    my ($ip) = @_;
    return $ip =~ /^([0-9a-fA-F]{1,4}:){7}[0-9a-fA-F]{1,4}$/;
}

# Function to check if a domain points correctly
# Added a depth parameter to track recursion depth
sub check_external_dns {
    my ($domain, $depth) = @_;

    # Set a recursion limit
    my $max_depth = 2;

    # Default depth to 0 if not specified
    $depth //= 0;

    # Return 1 if the recursion depth exceeds the limit
    return 1 if $depth > $max_depth;

    # Create a DNS resolver
    my $resolver = Net::DNS::Resolver->new;
    $resolver->tcp_timeout(5);  # Set TCP timeout to 5 seconds
    $resolver->udp_timeout(5);  # Set UDP timeout to 5 seconds

    # Perform a DNS search for the domain
    my $query = $resolver->search($domain);

    # Check if the query succeeded
    if ($query) {
        foreach my $rr ($query->answer) {
            if ($rr->type eq "A" && is_valid_ipv4($rr->address)) {
                return 0; # The domain points correctly to a valid IPv4 address
            }
            if ($rr->type eq "AAAA" && is_valid_ipv6($rr->address)) {
                return 0; # The domain points correctly to a valid IPv6 address
            }
            if ($rr->type eq "CNAME") {
                # Recursively check the CNAME target, incrementing the depth
                return check_domain($rr->cname, $depth + 1);
            }
        }
    }

    return 1; # The domain does not point correctly or does not exist
}

# Example usage
{
    my $domain = 'example.com';
    my $result = check_domain($domain);

    if ($result == 0) {
        print "The domain $domain points correctly.\n";
    } else {
        print "The domain $domain does not point correctly.\n";
    }
}

[iliajie]

Sorry to barge in but i think this should be chnaged, the function is not future proof and will cause issues in the near future.

This is optional and can be disabled upon request in Virtualmin.

it use external tools and not Net::DNS, output could be unreliable

We use dig with external DNS to check if the world (i.e., Let's Encrypt) can see the records. There is nothing unreliable about that.

it does not support ipv6

Well, IPv6-only configurations are still pretty rare. However, I agree that we need to consider it as well.

it use hardcoded google DNS 8.8.8.8, this will fail on secure network that force certain DNS server with firewall rules.

Alright, I would make the external DNS configurable as well.

[iliajie]

it does not support ipv6

Well, IPv6-only configurations are still pretty rare. However, I agree that we need to consider it as well.

Jamie, please check this https://github.com/virtualmin/virtualmin-gpl/commit/6bc2355d560cbc1c9a821fd64585db542007c1d1 patch and this https://github.com/virtualmin/virtualmin-gpl/commit/1e65c67f06c83d0a286308811bcc2a77f2b533d6 patch too.

[aqueos]

thanks, i am not directly concerned by those but the cname hit me on several domains.

external tools can change their output that is why i dont like using them in a program.

i saw that we can disable it but as the option is on by default even on allready configured domains i would have to check all domains of all sites on all servers to see if a cname is there (or ipv6 ) and go manualy change it one by one. :(

thanks for the quick answer.

[iliajie]

I'd like to point out that DNS resolution check is a fallback in case normal request failed.

[aqueos]

I'd like to point out that DNS resolution check is a fallback in case normal request failed.

humm from my issues i dont think so. are you sure it test dns only if the letsencrypt renew fails ?

filter_external_dns seems to test all domains on renew, i see it filter on initial only if it fail but i dont see that in renewal on /usr/share/webmin/virtual-server/letsencrypt.cgi.

[iliajie]

are you sure it test dns only if the letsencrypt renew fails ?

I have checked the code, and to my surprise, we have different logic when Let's Encrypt is initially requested (upon domain creation) versus during renewal. For renewals, we indeed always test DNS resolvability!

This is fixed in this https://github.com/virtualmin/virtualmin-gpl/commit/26256ff8625259bf5e91b6dfced19d42fafc7e8d patch.

[jcameron]

Does this only effect renewals for the default domain?

[iliajie]

All renewals.

[jcameron]

@iliajie the current code will use the letsencrypt_nodnscheck field to skip the DNS check, which can be configured in the UI. It's not set by default which causes the DNS check to be done if the dig command is installed, even on renewal. Your patch would actually change the current behavior and not do checks on renewal, which I don't think is what we want assuming that it can be done reliability.

@aqueos I actually prefer to use external commands if possible, as relying on installed Perl modules just increases the installation complexity and dependencies of Webmin

[aqueos]

i agree that this check is a good thing to prevent issues with letsencrypt. The problem is the code that is not finished and has been deployed in production without support for things like cname or ipv6 so it locks the renewal of all letsencrypt using those :(

@jcameron ok for dig , i would not, but this is a matter of personal preferences :)

thanks to all for the quick action, i think after the test it should be pushed in production as it will block a lot of people that use the basic dns configuration that is:

domain.com IN A x.X.X.X + www.domain.com IN CNAME domain.com.

and each day can lead to sites being down with no ssl :)

[iliajie]

@iliajie the current code will use the letsencrypt_nodnscheck field to skip the DNS check, which can be configured in the UI.

Yes, that's what I thought as well.

It's not set by default which causes the DNS check to be done if the dig command is installed, even on renewal.

Yes, but that is something I decided to change. In my current view, I don't think we should re-check records in renewals that were previously successful.

Your patch would actually change the current behavior and not do checks on renewal, which I don't think is what we want assuming that it can be done reliability.

Oh, alright! But why would we want to have it rechecked in renewals as well, assuming it worked in the past?

The problem is the code that is not finished and has been deployed in production without support for things like cname or ipv6 so it locks the renewal of all letsencrypt using those

Well, it is fixed now!

i think after the test it should be pushed in production as it will block a lot of people that use the basic dns configuration that is:

Yes, I agree! We will try to make a new Virtualmin 7.20.1 release as soon as we agree on everything being discussed.

[digitaldutch]

I ran in the same issue. The auto certificate renewal failed.

Checking hostnames for resolvability ..
Fatal Error!
Failed to request certificate : None of the hostnames could be resolved! 

virtual-server/letsencrypt.cgi (line 111)

The domain was named test.somedomain.com Set with a cname. The work around was switching off the checking.

[iliajie]

The domain was named test.somedomain.com Set with a cname. The work around was switching off the checking.

Sorry about that! We will fix it in the immediate Virtualmin 7.20.2 release!

[PitWenkin]

The domain was named test.somedomain.com Set with a cname. The work around was switching off the checking.

Sorry about that! We will fix it in the immediate Virtualmin 7.20.2 release!

Please make sure to update repositories as soon as possible… cf https://github.com/virtualmin/virtualmin-gpl/issues/855

[PitWenkin]

The domain was named test.somedomain.com Set with a cname. The work around was switching off the checking.

Sorry about that! We will fix it in the immediate Virtualmin 7.20.2 release!

Please make sure to update repositories as soon as possible… cf #855

7.20.2 fixes the problem and is available via repositories 👍

[aqueos]

Thanks for your work.

Just one last thing that i recall now. When you buy a domain in France a lot of time it configure a basic zone with ip4 and ipv6 pointer to a welcome page of the registrar.

This can lead to failure on letsencrypt on ipv4 host as letsencrypt prioritize ipv6 and fail. Got a lot of that on Ovh.

So my point is : if you want to add foolproof check you can add a check that if you got a ipv6 on the domain then the host should have one too because, if not, this will fail.

I guess the reverse is true but i never saw a domain with ipv4 and no ipv6 on a ipv6 host :)

best regards. Ghislain

[jcameron]

We do have that check already actually - if the domain has an IPv6 record in DNS but it's not enabled in Apache, Virtualmin won't even attempt to request the cert.