security/acme-client: calculated renewal date from 'lastUpdate' can differ from the certificate

[Hell-o-Admin]

Important notices

[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 and I'm convinced that mine is new.

[x] The title contains the plugin to which this issue belongs

Describe the bug

As of my understanding the acme-client plugin calculates the renewal date from the config setting lastUpdate instead of the actual validity of the given certificate. After re-importing a certificate, this can lead to the plugin not renewing the certificate, even it is almost or already invalid. There might be other cases where the current calculation bases on lastUpdate will be wrong, e.g when the issuing is started but fails for some reason, but i did not check further.

With my limited knowledge of php (and OPNsense) i beleave to have found the reason for the the behavior.
When renewing all certificates via cron LeCertificate.php will eventually be run at this line:

plugins/security/acme-client/src/opnsense/mvc/app/library/OPNsense/AcmeClient/LeCertificate.php

Line 418 in df1b283

$last_update = !empty((string)$this->config->lastUpdate) ? (string)$this->config->lastUpdate : 0;

The variable config->lastUpdate is (at least) set during re-import in LeCertificate.php at this line and to the current time:

plugins/security/acme-client/src/opnsense/mvc/app/library/OPNsense/AcmeClient/LeCertificate.php

Line 295 in df1b283

$this->config->lastUpdate = time();

To Reproduce
Steps to reproduce the behavior:

1. Go to Services>ACME Client>Certificates
2. Click on 'Re-Import certificate'
3. See how 'Issue/Renewal Date' is set to the current time, but the certificate has not (and should not) been renewed
4. 'Issue/Renewal Date' shows the config->lastUpdate variable of the certificate, which is used to calculate the renewal date as explained above

Expected behavior
From my point of view, it would be the best solution to let LeCertificate.php check the actual validity dates from the certificate it self when renewing (e.g. when run from cron). As an alternative, it should be checked when importing a certificate and stored in lastUpdate, because it is not likely enough to assume, that it has been issued at the same day.

As a side note:
OPNsense can read the validity of a certificate under System>Trust with a function in certs.inc here:
https://github.com/opnsense/core/blob/14f3cb5214f891365fb5e6d8d3bebdab897ddeef/src/etc/inc/certs.inc#L477

I tried to implement or use this function from within LeCertificate.php but failed, most likely because i don't really know what i am doing. At some point certs.inc is also called legacy, so i assume it might be removed in the future.

I am not sure, if this something that should be fixed or more like a feature request, but still wanted to let you know about it.

[fraenki]

Thanks for reporting this. This bug is the result of ae69739 and is fixed in 8350155.

The following patches should fix this:

opnsense-patch -c plugins 9b444c34917d1ab9e57e5dfcd2969b7b195831ed 8350155d1152bc38e6c860fa3545c92d80dde3b6

(Only the latter patch addresses this bug, but I think both are required for the patch to apply cleanly.)

Please let me know if it fixes the issue for you. I plan to release it in os-acme-client 3.8.

[Hell-o-Admin]

hey fraenki,

thank you for fixing this so quickly. i applied your patches and the cron job did renew all my invalid certificates this night. i dont have any valid but due certificates right now, but will report, if this should fail in the future.
for some reason my syslogd was not running, so i dont have all the log data, but the result looks good.

thanks again for your great support!