You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
ee site ssl-verify can go down the first-request path in executeFirstRequest() while the stored ACME order for that domain set is already finalized. EE then stored a new domain key and DN and called finalizeOrder(). For a valid order acmephp skips the CSR and LE returns the existing certificate, so nothing was issued, but EE deployed that certificate next to the new key.
The proxy then held a mismatched key/cert pair: nginx -t failed with key values mismatch, every later proxy reload was skipped for all sites, and a proxy restart would have taken them all down. The command still printed "Success: SSL verification completed." and exited 0.
The same function also replaced the stored key before checking that the order exists, and a 403 from LE at finalize ended in an uncaught PHP fatal with the key already replaced.
Fix
The order is looked up before the stored key is touched.
The previous key pair and DN are restored if the request fails, including a failure while storing the new key or DN, with a warning. The warning says the current certificate is kept only when the domain has one, so a failed first issuance doesn't claim it.
The certificate LE returns must match the new key, otherwise it is treated as a failure.
moveCertsToNginxProxy() refuses to deploy a certificate that doesn't match its key.
A 403 from LE at finalize now gives a warning with LE's reason instead of a PHP fatal.
Exception logs in ee.log no longer contain key material, on both the first-request and the renewal path.
Testing
Already-finalized order: warning "the returned certificate does not match the new domain key", the stored key, DN and proxy files are unchanged, nginx -t passes, and the served certificate is the same. Without the fix the same setup breaks the proxy.
403 at finalize: warning with LE's reason, no fatal, key and DN unchanged.
A normal Let's Encrypt site create still issues and deploys a matching pair.
executeFirstRequest() stored a new domain key pair and DN before it looked up the order and finalized it. A missing order ("has not yet been authorized"), a refused finalize (403, uncaught) or an order that LE had already finalized left acme-conf with a key that no longer matched the served certificate. In the last case acmephp's finalizeOrder() skips the CSR and returns the order's existing certificate, so the new key and the old certificate were deployed together and nginx -t failed for the whole proxy.
Look up the order first, keep the previous key pair and DN, check that the returned certificate matches the new key, and on any failure restore both, warn and return false. moveCertsToNginxProxy() also refuses to deploy a mismatched pair.
The new key and DN were stored before the try block, so a failure while storing them (for example a full disk) skipped the restore and left acme-conf with a key that doesn't match the served certificate. Start the try before the key is generated, and log the error before restoring, so a restore that fails too doesn't hide it.
A failed first request on a new site (site create, or update --ssl=le on a site without SSL) also printed "The current certificate is kept.", although there was no certificate. Add that sentence only when a certificate is stored for the domain.
…fails
print_r() of the exception also prints its trace arguments when zend.exception_ignore_args is off (the PHP default, and always on PHP 7.2/7.3). The finalizeOrder() frame holds the CSR with the new private key, so the key was written to ee.log, which every EE::debug() reaches. Log the exception as a string instead: message, location and a trace without argument contents.
executeRenewal() logged the caught exception with print_r(). When zend.exception_ignore_args is off (the PHP default, and the easyengine/php* images), that prints the trace arguments, and the finalizeOrder() frame holds the CSR with the live domain private key, so the key went to ee.log through EE::debug(). Walking every object in the trace can also exhaust memory. Log the exception as a string instead, as executeFirstRequest() already does.
Certificate persistence is outside rollback transaction
src/helper/Site_Letsencrypt.php:630
The rollback scope ends before storeDomainCertificate(). That method writes several files sequentially; if it fails before replacing the full-chain file, the new key remains stored beside the old certificate and the exception escapes, recreating the inconsistent repository state this change is intended to prevent. Certificate persistence needs to participate in the same transaction/rollback, including restoring any partially replaced certificate files.
On the finding in the last review overview (certificate persistence outside the rollback, Site_Letsencrypt.php:630): this needs a file write to fail inside storeDomainCertificate(), and even then the served pair is safe. moveCertsToNginxProxy() isn't reached, and it refuses any key/certificate pair that doesn't match, so the proxy keeps its current files and nginx -t keeps passing. The next renewal signs a CSR with the stored key, so the ACME state becomes consistent again. Making the certificate files part of the rollback means backing up and restoring each of them, which is out of scope here; it's tracked as a follow-up.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Merge after #505.
Problem
ee site ssl-verifycan go down the first-request path inexecuteFirstRequest()while the stored ACME order for that domain set is already finalized. EE then stored a new domain key and DN and calledfinalizeOrder(). For a valid order acmephp skips the CSR and LE returns the existing certificate, so nothing was issued, but EE deployed that certificate next to the new key.The proxy then held a mismatched key/cert pair:
nginx -tfailed withkey values mismatch, every later proxy reload was skipped for all sites, and a proxy restart would have taken them all down. The command still printed "Success: SSL verification completed." and exited 0.The same function also replaced the stored key before checking that the order exists, and a 403 from LE at finalize ended in an uncaught PHP fatal with the key already replaced.
Fix
moveCertsToNginxProxy()refuses to deploy a certificate that doesn't match its key.ee.logno longer contain key material, on both the first-request and the renewal path.Testing
nginx -tpasses, and the served certificate is the same. Without the fix the same setup breaks the proxy.php -lpasses on PHP 7.4 and 8.5.