From c4e119d6459512d9395dc157f89eb52c15431cfd Mon Sep 17 00:00:00 2001 From: Riddhesh Sanghvi Date: Tue, 29 Sep 2026 11:02:19 +0530 Subject: [PATCH 1/5] fix(ssl): keep the served key when a first certificate request fails 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. --- src/helper/Site_Letsencrypt.php | 51 +++++++++++++++++++++++++-------- 1 file changed, 39 insertions(+), 12 deletions(-) diff --git a/src/helper/Site_Letsencrypt.php b/src/helper/Site_Letsencrypt.php index 7285f4fd..8ff291da 100644 --- a/src/helper/Site_Letsencrypt.php +++ b/src/helper/Site_Letsencrypt.php @@ -580,6 +580,18 @@ public function request( $domain, $altNames = [], $email, $force = false ) { private function executeFirstRequest( $domain, array $alternativeNames, $email ) { \EE::log( 'Executing first request.' ); + // Order first, so a missing order can't replace the key of the certificate that is still served. + $domains = array_merge( [ $domain ], $alternativeNames ); + \EE::debug( sprintf( 'Loading the order related to the domains %s .', implode( ', ', $domains ) ) ); + if ( ! $this->repository->hasCertificateOrder( $domains ) ) { + \EE::error( "$domain has not yet been authorized." ); + } + $order = $this->repository->loadCertificateOrder( $domains ); + + // Restored if no certificate is stored below: they belong to the certificate that is still served. + $previous_key_pair = $this->repository->hasDomainKeyPair( $domain ) ? $this->repository->loadDomainKeyPair( $domain ) : null; + $previous_dn = $this->repository->hasDomainDistinguishedName( $domain ) ? $this->repository->loadDomainDistinguishedName( $domain ) : null; + // Generate domain key pair $keygen = new KeyPairGenerator(); $domainKeyPair = $keygen->generateKeyPair(); @@ -591,19 +603,29 @@ private function executeFirstRequest( $domain, array $alternativeNames, $email ) // TODO: ask them ;) \EE::debug( 'Distinguished name informations have been stored locally for this domain (they won\'t be asked on renewal).' ); - // Order - $domains = array_merge( [ $domain ], $alternativeNames ); - \EE::debug( sprintf( 'Loading the order related to the domains %s .', implode( ', ', $domains ) ) ); - if ( ! $this->repository->hasCertificateOrder( $domains ) ) { - \EE::error( "$domain has not yet been authorized." ); - } - $order = $this->repository->loadCertificateOrder( $domains ); + try { + // Request + \EE::log( sprintf( 'Requesting first certificate for domain %s.', $domain ) ); + $csr = new CertificateRequest( $distinguishedName, $domainKeyPair ); + $response = $this->client->finalizeOrder( $order, $csr ); + \EE::log( 'Certificate received' ); + + // finalizeOrder() skips the CSR for an already-finalized order and returns that order's certificate, issued for another key. + if ( ! openssl_x509_check_private_key( $response->getCertificate()->getPEM(), $domainKeyPair->getPrivateKey()->getPEM() ) ) { + throw new \Exception( 'the returned certificate does not match the new domain key (the stored order was already finalized)' ); + } + } catch ( \Throwable $e ) { + if ( $previous_key_pair ) { + $this->repository->storeDomainKeyPair( $domain, $previous_key_pair ); + } + if ( $previous_dn ) { + $this->repository->storeDomainDistinguishedName( $domain, $previous_dn ); + } + \EE::debug( print_r( $e, true ) ); + \EE::warning( sprintf( 'Certificate request for %s failed: %s. The current certificate is kept.', $domain, $e->getMessage() ) ); - // Request - \EE::log( sprintf( 'Requesting first certificate for domain %s.', $domain ) ); - $csr = new CertificateRequest( $distinguishedName, $domainKeyPair ); - $response = $this->client->finalizeOrder( $order, $csr ); - \EE::log( 'Certificate received' ); + return false; + } // Store $this->repository->storeDomainCertificate( $domain, $response->getCertificate() ); @@ -625,6 +647,11 @@ private function moveCertsToNginxProxy( string $domain ) { $crt_dest_file = EE_ROOT_DIR . '/services/nginx-proxy/certs/' . $domain . '.crt'; $chain_dest_file = EE_ROOT_DIR . '/services/nginx-proxy/certs/' . $domain . '.chain.pem'; + // A mismatched pair fails nginx -t, which blocks every later reload of the shared proxy. + if ( is_readable( $crt_source_file ) && is_readable( $key_source_file ) && ! openssl_x509_check_private_key( file_get_contents( $crt_source_file ), file_get_contents( $key_source_file ) ) ) { + throw new \Exception( sprintf( 'Certificate %s does not match its private key; not deploying it.', $crt_source_file ) ); + } + // Stage temps in the destination dir and rename() them in, so a failed copy never leaves a half-written live key/cert. // Each rename is atomic, the set is not; an already-renamed file is not rolled back. $copy_map = [ From 1061262de09b0b47cd2e0c57d90bf1e16fefde86 Mon Sep 17 00:00:00 2001 From: Riddhesh Sanghvi Date: Tue, 29 Sep 2026 11:29:38 +0530 Subject: [PATCH 2/5] fix(ssl): restore the previous key when storing the new key or DN fails 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. --- src/helper/Site_Letsencrypt.php | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/helper/Site_Letsencrypt.php b/src/helper/Site_Letsencrypt.php index 8ff291da..6ab1d730 100644 --- a/src/helper/Site_Letsencrypt.php +++ b/src/helper/Site_Letsencrypt.php @@ -592,18 +592,18 @@ private function executeFirstRequest( $domain, array $alternativeNames, $email ) $previous_key_pair = $this->repository->hasDomainKeyPair( $domain ) ? $this->repository->loadDomainKeyPair( $domain ) : null; $previous_dn = $this->repository->hasDomainDistinguishedName( $domain ) ? $this->repository->loadDomainDistinguishedName( $domain ) : null; - // Generate domain key pair - $keygen = new KeyPairGenerator(); - $domainKeyPair = $keygen->generateKeyPair(); - $this->repository->storeDomainKeyPair( $domain, $domainKeyPair ); + try { + // Generate domain key pair + $keygen = new KeyPairGenerator(); + $domainKeyPair = $keygen->generateKeyPair(); + $this->repository->storeDomainKeyPair( $domain, $domainKeyPair ); - \EE::debug( "$domain Domain key pair generated and stored" ); + \EE::debug( "$domain Domain key pair generated and stored" ); - $distinguishedName = $this->getOrCreateDistinguishedName( $domain, $alternativeNames, $email ); - // TODO: ask them ;) - \EE::debug( 'Distinguished name informations have been stored locally for this domain (they won\'t be asked on renewal).' ); + $distinguishedName = $this->getOrCreateDistinguishedName( $domain, $alternativeNames, $email ); + // TODO: ask them ;) + \EE::debug( 'Distinguished name informations have been stored locally for this domain (they won\'t be asked on renewal).' ); - try { // Request \EE::log( sprintf( 'Requesting first certificate for domain %s.', $domain ) ); $csr = new CertificateRequest( $distinguishedName, $domainKeyPair ); @@ -615,13 +615,14 @@ private function executeFirstRequest( $domain, array $alternativeNames, $email ) throw new \Exception( 'the returned certificate does not match the new domain key (the stored order was already finalized)' ); } } catch ( \Throwable $e ) { + // Logged first, so a restore that fails too doesn't hide the reason. + \EE::debug( print_r( $e, true ) ); if ( $previous_key_pair ) { $this->repository->storeDomainKeyPair( $domain, $previous_key_pair ); } if ( $previous_dn ) { $this->repository->storeDomainDistinguishedName( $domain, $previous_dn ); } - \EE::debug( print_r( $e, true ) ); \EE::warning( sprintf( 'Certificate request for %s failed: %s. The current certificate is kept.', $domain, $e->getMessage() ) ); return false; From e9e9da117603058bf961ead57f9a152aa27207d1 Mon Sep 17 00:00:00 2001 From: Riddhesh Sanghvi Date: Tue, 29 Sep 2026 11:29:46 +0530 Subject: [PATCH 3/5] fix(ssl): only say the current certificate is kept when there is one 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. --- src/helper/Site_Letsencrypt.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/helper/Site_Letsencrypt.php b/src/helper/Site_Letsencrypt.php index 6ab1d730..626eb5e6 100644 --- a/src/helper/Site_Letsencrypt.php +++ b/src/helper/Site_Letsencrypt.php @@ -588,7 +588,7 @@ private function executeFirstRequest( $domain, array $alternativeNames, $email ) } $order = $this->repository->loadCertificateOrder( $domains ); - // Restored if no certificate is stored below: they belong to the certificate that is still served. + // Restored if no certificate is stored below: they belong to the stored certificate, if there is one. $previous_key_pair = $this->repository->hasDomainKeyPair( $domain ) ? $this->repository->loadDomainKeyPair( $domain ) : null; $previous_dn = $this->repository->hasDomainDistinguishedName( $domain ) ? $this->repository->loadDomainDistinguishedName( $domain ) : null; @@ -623,7 +623,8 @@ private function executeFirstRequest( $domain, array $alternativeNames, $email ) if ( $previous_dn ) { $this->repository->storeDomainDistinguishedName( $domain, $previous_dn ); } - \EE::warning( sprintf( 'Certificate request for %s failed: %s. The current certificate is kept.', $domain, $e->getMessage() ) ); + $kept = $this->repository->hasDomainCertificate( $domain ) ? ' The current certificate is kept.' : ''; + \EE::warning( sprintf( 'Certificate request for %s failed: %s.%s', $domain, $e->getMessage(), $kept ) ); return false; } From 0dfc92de07665750e2c9251b5ea8ac5d021e6f92 Mon Sep 17 00:00:00 2001 From: Riddhesh Sanghvi Date: Tue, 29 Sep 2026 11:29:55 +0530 Subject: [PATCH 4/5] fix(ssl): keep the new domain key out of ee.log when a first request 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. --- src/helper/Site_Letsencrypt.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/helper/Site_Letsencrypt.php b/src/helper/Site_Letsencrypt.php index 626eb5e6..1dd7ad46 100644 --- a/src/helper/Site_Letsencrypt.php +++ b/src/helper/Site_Letsencrypt.php @@ -615,8 +615,8 @@ private function executeFirstRequest( $domain, array $alternativeNames, $email ) throw new \Exception( 'the returned certificate does not match the new domain key (the stored order was already finalized)' ); } } catch ( \Throwable $e ) { - // Logged first, so a restore that fails too doesn't hide the reason. - \EE::debug( print_r( $e, true ) ); + // Logged first, so a restore that fails too doesn't hide the reason. Not print_r(): its trace args hold the new private key. + \EE::debug( (string) $e ); if ( $previous_key_pair ) { $this->repository->storeDomainKeyPair( $domain, $previous_key_pair ); } From fd7df734e903abec6feacc53cc7e0837da5b5c82 Mon Sep 17 00:00:00 2001 From: Riddhesh Sanghvi Date: Tue, 29 Sep 2026 11:38:15 +0530 Subject: [PATCH 5/5] fix(ssl): don't log certificate keys when a renewal fails 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. --- src/helper/Site_Letsencrypt.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/helper/Site_Letsencrypt.php b/src/helper/Site_Letsencrypt.php index 1dd7ad46..6086c01d 100644 --- a/src/helper/Site_Letsencrypt.php +++ b/src/helper/Site_Letsencrypt.php @@ -837,7 +837,7 @@ private function executeRenewal( $domain, array $alternativeNames, $email, $forc } catch ( \Exception $e ) { \EE::warning( 'A critical error occurred during certificate renewal: ' . $e->getMessage() ); - \EE::debug( print_r( $e, true ) ); + \EE::debug( (string) $e ); // A rate limit is not a misconfigured-domain failure; point the user to the LE rate-limit docs. if ( $this->is_rate_limit_exception( $e ) ) { \EE::warning( 'Let\'s Encrypt rate limit hit for: ' . $domain . '. Please wait before retrying. Ref: https://letsencrypt.org/docs/rate-limits/' ); @@ -847,7 +847,7 @@ private function executeRenewal( $domain, array $alternativeNames, $email, $forc return false; } catch ( \Throwable $e ) { \EE::warning( 'A critical error occurred during certificate renewal: ' . $e->getMessage() ); - \EE::debug( print_r( $e, true ) ); + \EE::debug( (string) $e ); // A rate limit is not a misconfigured-domain failure; point the user to the LE rate-limit docs. if ( $this->is_rate_limit_exception( $e ) ) { \EE::warning( 'Let\'s Encrypt rate limit hit for: ' . $domain . '. Please wait before retrying. Ref: https://letsencrypt.org/docs/rate-limits/' );