Optimize cert loading/rewewal checks on server startup (#205)

* fix: check if certificate needs renewal immediately on startup instead waiting 24 hours * fix: optimize loading intermediate certificate change and reduce unnecessary warnings about invalid certs
natemcmaster · Jul 9, 2021 · 5894e19 · 5894e19
1 parent e5f5089
commit 5894e19
Show file tree

Hide file tree

Showing 5 changed files with 34 additions and 13 deletions.
diff --git a/src/LettuceEncrypt/Internal/AcmeStates/CheckForRenewalState.cs b/src/LettuceEncrypt/Internal/AcmeStates/CheckForRenewalState.cs
@@ -42,8 +42,6 @@ public override async Task<IAcmeState> MoveNextAsync(CancellationToken cancellat
                     return MoveTo<TerminalState>();
                 }
 
-                await Task.Delay(checkPeriod.Value, cancellationToken);
-
                 var domainNames = _options.Value.DomainNames;
                 if (_logger.IsEnabled(LogLevel.Debug))
                 {
@@ -60,6 +58,8 @@ public override async Task<IAcmeState> MoveNextAsync(CancellationToken cancellat
                         return MoveTo<BeginCertificateCreationState>();
                     }
                 }
+
+                await Task.Delay(checkPeriod.Value, cancellationToken);
             }
 
             return MoveTo<TerminalState>();

diff --git a/src/LettuceEncrypt/Internal/CertificateSelector.cs b/src/LettuceEncrypt/Internal/CertificateSelector.cs
@@ -34,11 +34,18 @@ public CertificateSelector(IOptions<LettuceEncryptOptions> options, ILogger<Cert
 
         public virtual void Add(X509Certificate2 certificate)
         {
-            PreloadIntermediateCertificates(certificate);
-
+            var preloaded = false;
             foreach (var dnsName in X509CertificateHelpers.GetAllDnsNames(certificate))
             {
-                AddWithDomainName(_certs, dnsName, certificate);
+                var selectedCert = AddWithDomainName(_certs, dnsName, certificate);
+
+                // Call preload once per certificate, but only if the cetificate is actually selected to be used
+                // for this domain. This is a small optimization which avoids preloading on a cert that may not be used.
+                if (!preloaded && selectedCert == certificate)
+                {
+                    preloaded = true;
+                    PreloadIntermediateCertificates(selectedCert);
+                }
             }
         }
 
@@ -55,10 +62,17 @@ public void ClearChallengeCert(string domainName)
             _challengeCerts.TryRemove(domainName, out _);
         }
 
-        private void AddWithDomainName(ConcurrentDictionary<string, X509Certificate2> certs, string domainName,
+        /// <summary>
+        /// Registers the certificate for usage with domain unless there is already a newer cert for this domain.
+        /// </summary>
+        /// <param name="certs"></param>
+        /// <param name="domainName"></param>
+        /// <param name="certificate"></param>
+        /// <returns>The certificate current selected to be used for this domain</returns>
+        private X509Certificate2 AddWithDomainName(ConcurrentDictionary<string, X509Certificate2> certs, string domainName,
             X509Certificate2 certificate)
         {
-            certs.AddOrUpdate(
+            return certs.AddOrUpdate(
                 domainName,
                 certificate,
                 (_, currentCert) =>

diff --git a/src/LettuceEncrypt/Internal/StartupCertificateLoader.cs b/src/LettuceEncrypt/Internal/StartupCertificateLoader.cs
@@ -2,6 +2,8 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System.Collections.Generic;
+using System.Linq;
+using System.Security.Cryptography.X509Certificates;
 using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.Extensions.Hosting;
@@ -23,14 +25,17 @@ public StartupCertificateLoader(
 
         public async Task StartAsync(CancellationToken cancellationToken)
         {
+            var allCerts = new List<X509Certificate2>();
             foreach (var certSource in _certSources)
             {
                 var certs = await certSource.GetCertificatesAsync(cancellationToken);
+                allCerts.AddRange(certs);
+            }
 
-                foreach (var cert in certs)
-                {
-                    _selector.Add(cert);
-                }
+            // Add newer certificates first. This avoid potentially unnecessary cert validations on older certificates
+            foreach (var cert in allCerts.OrderByDescending(c => c.NotAfter))
+            {
+                _selector.Add(cert);
             }
         }
 

diff --git a/src/LettuceEncrypt/releasenotes.props b/src/LettuceEncrypt/releasenotes.props
@@ -14,8 +14,10 @@ Bug fixes:
 Other:
  * Update package to target .NET Core 3.1 as 3.0 is no longer supported by Microsoft
 
-Bug patch 1.1.1:
+Fixes in patch 1.1.1:
  * Fix infinite loop waiting for verification of domain ownership
+ * Check for certificates that new renewal immediately on server startup instead of waiting 24 hours
+ * Optimize loading intermediate certificate change and reduce unnecessary warnings about invalid certs
     </PackageReleaseNotes>
     <PackageReleaseNotes Condition="'$(VersionPrefix)' == '1.0.1'">
 * Fix bug in detecting Kestrel in .NET 5

diff --git a/test/LettuceEncrypt.UnitTests/StartupCertificateLoaderTests.cs b/test/LettuceEncrypt.UnitTests/StartupCertificateLoaderTests.cs
@@ -18,7 +18,7 @@ public class StartupCertificateLoaderTests
         [Fact]
         public async Task ItLoadsAllCertsIntoSelector()
         {
-            var testCert = new X509Certificate2();
+            var testCert = TestUtils.CreateTestCert("test1.natemcmaster.com");
             IEnumerable<X509Certificate2> certs = new[] { testCert };
 
             var selector = new Mock<CertificateSelector>(