Merge pull request #5 from ChangemakerStudios/claude/review-certificate-update-011CUw9RFfKdbGehxfDxVmef

Implement comprehensive improvements to certificate renewal reliability

Author: Jaben

src/LettuceEncrypt/Internal/AcmeCertificateFactory.cs

@@ -9,6 +9,7 @@
 using LettuceEncrypt.Accounts;
 using LettuceEncrypt.Acme;
 using LettuceEncrypt.Internal.PfxBuilder;
+using Microsoft.AspNetCore.Hosting.Server;
 using Microsoft.Extensions.Hosting;
 using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Options;
@@ -28,6 +29,7 @@ internal class AcmeCertificateFactory
     private readonly IDnsChallengeProvider _dnsChallengeProvider;
     private readonly ICertificateAuthorityConfiguration _certificateAuthority;
     private readonly IPfxBuilderFactory _pfxBuilderFactory;
+    private readonly IServer _server;
     private readonly TaskCompletionSource<object?> _appStarted = new();
     private AcmeClient? _client;
     private IKey? _acmeAccountKey;
@@ -43,6 +45,7 @@ public AcmeCertificateFactory(
         ICertificateAuthorityConfiguration certificateAuthority,
         IDnsChallengeProvider dnsChallengeProvider,
         IPfxBuilderFactory pfxBuilderFactory,
+        IServer server,
         IAccountStore? accountRepository = null)
     {
         _acmeClientFactory = acmeClientFactory;
@@ -55,6 +58,7 @@ public AcmeCertificateFactory(
         _dnsChallengeProvider = dnsChallengeProvider;
         _certificateAuthority = certificateAuthority;
         _pfxBuilderFactory = pfxBuilderFactory;
+        _server = server;
 
         appLifetime.ApplicationStarted.Register(() => _appStarted.TrySetResult(null));
         if (appLifetime.ApplicationStarted.IsCancellationRequested)
@@ -236,23 +240,27 @@ private async Task ValidateDomainOwnershipAsync(IAuthorizationContext authorizat
         cancellationToken.ThrowIfCancellationRequested();
 
         var validators = new List<DomainOwnershipValidator>();
+        var validationTimeout = _options.Value.ValidationTimeout;
+        var validationPollInterval = _options.Value.ValidationPollInterval;
+        var enableSelfTest = _options.Value.EnableChallengeSelfTest;
+        var selfTestBaseUrl = _options.Value.ChallengeSelfTestBaseUrl;
 
         if (_tlsAlpnChallengeResponder.IsEnabled)
         {
             validators.Add(new TlsAlpn01DomainValidator(
-                _tlsAlpnChallengeResponder, _appLifetime, _client, _logger, domainName));
+                _tlsAlpnChallengeResponder, _appLifetime, _client, _logger, domainName, validationTimeout, validationPollInterval));
         }
 
         if (_options.Value.AllowedChallengeTypes.HasFlag(ChallengeType.Http01))
         {
             validators.Add(new Http01DomainValidator(
-                _challengeStore, _appLifetime, _client, _logger, domainName));
+                _challengeStore, _appLifetime, _client, _logger, domainName, validationTimeout, validationPollInterval, enableSelfTest, selfTestBaseUrl, _server));
         }
 
         if (_options.Value.AllowedChallengeTypes.HasFlag(ChallengeType.Dns01))
         {
             validators.Add(new Dns01DomainValidator(
-                _dnsChallengeProvider, _appLifetime, _client, _logger, domainName));
+                _dnsChallengeProvider, _appLifetime, _client, _logger, domainName, validationTimeout, validationPollInterval));
         }
 
         if (validators.Count == 0)
@@ -263,23 +271,67 @@ private async Task ValidateDomainOwnershipAsync(IAuthorizationContext authorizat
                 "Ensure at least one kind of these challenge types is configured: " + challengeTypes);
         }
 
+        _logger.LogInformation(
+            "Attempting domain validation for '{DomainName}' using {ValidatorCount} challenge method(s): {Validators}",
+            domainName,
+            validators.Count,
+            string.Join(", ", validators.Select(v => v.GetType().Name.Replace("DomainValidator", ""))));
+
+        var failures = new List<Exception>();
+
         foreach (var validator in validators)
         {
             cancellationToken.ThrowIfCancellationRequested();
+            var validatorName = validator.GetType().Name.Replace("DomainValidator", "");
+
             try
             {
+                _logger.LogDebug("Trying {ValidatorName} validation for domain '{DomainName}'",
+                    validatorName, domainName);
+
                 await validator.ValidateOwnershipAsync(authorizationContext, cancellationToken);
+
                 // The method above raises if validation fails. If no exception occurs, we assume validation completed successfully.
+                _logger.LogInformation(
+                    "Domain validation succeeded using {ValidatorName} for '{DomainName}'",
+                    validatorName, domainName);
                 return;
             }
             catch (Exception ex)
             {
-                _logger.LogDebug(ex, "Validation with {validatorType} failed with error: {error}",
-                    validator.GetType().Name, ex.Message);
+                // Rethrow cancellation exceptions immediately to preserve cooperative cancellation semantics
+                if (ex is OperationCanceledException || ex is TaskCanceledException)
+                {
+                    throw;
+                }
+
+                failures.Add(ex);
+                _logger.LogWarning(ex,
+                    "Validation with {ValidatorName} failed for domain '{DomainName}'. " +
+                    "Error: {ErrorMessage}. " +
+                    "{RemainingValidators} validation method(s) remaining.",
+                    validatorName,
+                    domainName,
+                    ex.Message,
+                    validators.Count - failures.Count);
             }
         }
 
-        throw new InvalidOperationException($"Failed to validate ownership of domainName '{domainName}'");
+        // All validators failed
+        var failureDetails = string.Join("; ", failures.Select((ex, i) =>
+            $"{validators[i].GetType().Name.Replace("DomainValidator", "")}: {ex.Message}"));
+
+        _logger.LogError(
+            "All {ValidatorCount} validation methods failed for domain '{DomainName}'. Failures: {FailureDetails}",
+            validators.Count,
+            domainName,
+            failureDetails);
+
+        throw new AggregateException(
+            $"Failed to validate ownership of domainName '{domainName}' using any available challenge method. " +
+            $"Attempted: {string.Join(", ", validators.Select(v => v.GetType().Name.Replace("DomainValidator", "")))}. " +
+            $"See inner exceptions for details.",
+            failures);
     }
 
     private async Task<X509Certificate2> CompleteCertificateRequestAsync(IOrderContext order,

src/LettuceEncrypt/Internal/AcmeStates/BeginCertificateCreationState.cs

@@ -14,18 +14,24 @@ internal class BeginCertificateCreationState : AcmeState
     private readonly AcmeCertificateFactory _acmeCertificateFactory;
     private readonly CertificateSelector _selector;
     private readonly IEnumerable<ICertificateRepository> _certificateRepositories;
+    private readonly RenewalFailureTracker _failureTracker;
 
     public BeginCertificateCreationState(
-        AcmeStateMachineContext context, ILogger<ServerStartupState> logger,
-        IOptions<LettuceEncryptOptions> options, AcmeCertificateFactory acmeCertificateFactory,
-        CertificateSelector selector, IEnumerable<ICertificateRepository> certificateRepositories)
+        AcmeStateMachineContext context,
+        ILogger<ServerStartupState> logger,
+        IOptions<LettuceEncryptOptions> options,
+        AcmeCertificateFactory acmeCertificateFactory,
+        CertificateSelector selector,
+        IEnumerable<ICertificateRepository> certificateRepositories,
+        RenewalFailureTracker failureTracker)
         : base(context)
     {
         _logger = logger;
         _options = options;
         _acmeCertificateFactory = acmeCertificateFactory;
         _selector = selector;
         _certificateRepositories = certificateRepositories;
+        _failureTracker = failureTracker;
     }
 
     public override async Task<IAcmeState> MoveNextAsync(CancellationToken cancellationToken)
@@ -47,11 +53,31 @@ public override async Task<IAcmeState> MoveNextAsync(CancellationToken cancellat
                 cert.Thumbprint);
 
             await SaveCertificateAsync(cert, cancellationToken);
+
+            // Record success for all domains
+            foreach (var domain in domainNames)
+            {
+                _failureTracker.RecordSuccess(domain);
+            }
         }
         catch (Exception ex)
         {
+            // Rethrow cancellation exceptions immediately to preserve cooperative cancellation semantics
+            if (ex is OperationCanceledException || ex is TaskCanceledException)
+            {
+                throw;
+            }
+
+            // Record failure for all domains
+            foreach (var domain in domainNames)
+            {
+                _failureTracker.RecordFailure(domain, ex);
+            }
+
             _logger.LogError(0, ex, "Failed to automatically create a certificate for {hostname}", domainNames);
-            throw;
+
+            // Don't throw - return to CheckForRenewalState to implement backoff
+            // The exception has been logged and tracked
         }
 
         return MoveTo<CheckForRenewalState>();

src/LettuceEncrypt/Internal/AcmeStates/CheckForRenewalState.cs

@@ -13,18 +13,21 @@ internal class CheckForRenewalState : AcmeState
     private readonly IOptions<LettuceEncryptOptions> _options;
     private readonly CertificateSelector _selector;
     private readonly IClock _clock;
+    private readonly RenewalFailureTracker _failureTracker;
 
     public CheckForRenewalState(
         AcmeStateMachineContext context,
         ILogger<CheckForRenewalState> logger,
         IOptions<LettuceEncryptOptions> options,
         CertificateSelector selector,
-        IClock clock) : base(context)
+        IClock clock,
+        RenewalFailureTracker failureTracker) : base(context)
     {
         _logger = logger;
         _options = options;
         _selector = selector;
         _clock = clock;
+        _failureTracker = failureTracker;
     }
 
     public override async Task<IAcmeState> MoveNextAsync(CancellationToken cancellationToken)
@@ -53,6 +56,24 @@ public override async Task<IAcmeState> MoveNextAsync(CancellationToken cancellat
                     || cert == null
                     || cert.NotAfter <= _clock.Now.DateTime + daysInAdvance.Value)
                 {
+                    var certExpiration = cert?.NotAfter ?? _clock.Now.DateTime;
+
+                    // Check backoff policy before attempting renewal
+                    if (!_failureTracker.ShouldAttemptRenewal(domainName, certExpiration))
+                    {
+                        _logger.LogDebug(
+                            "Skipping renewal for '{DomainName}' due to backoff policy. {FailureInfo}",
+                            domainName,
+                            _failureTracker.GetFailureInfo(domainName));
+                        continue;
+                    }
+
+                    _logger.LogInformation(
+                        "Certificate renewal needed for '{DomainName}'. Expiration: {Expiration}, Days until expiry: {DaysUntilExpiry:F1}",
+                        domainName,
+                        certExpiration,
+                        (certExpiration - _clock.Now.DateTime).TotalDays);
+
                     return MoveTo<BeginCertificateCreationState>();
                 }
             }

src/LettuceEncrypt/Internal/Dns01DomainValidator.cs

@@ -19,8 +19,10 @@ public Dns01DomainValidator(
         IHostApplicationLifetime appLifetime,
         AcmeClient client,
         ILogger logger,
-        string domainName
-    ) : base(appLifetime, client, logger, domainName)
+        string domainName,
+        TimeSpan validationTimeout,
+        TimeSpan validationPollInterval
+    ) : base(appLifetime, client, logger, domainName, validationTimeout, validationPollInterval)
     {
         _dnsChallengeProvider = dnsChallengeProvider;
     }

src/LettuceEncrypt/Internal/DomainOwnershipValidator.cs

@@ -14,12 +14,22 @@ internal abstract class DomainOwnershipValidator
     protected readonly ILogger _logger;
     protected readonly string _domainName;
     protected readonly TaskCompletionSource<object?> _appStarted = new();
-
-    protected DomainOwnershipValidator(IHostApplicationLifetime appLifetime, AcmeClient client, ILogger logger, string domainName)
+    protected readonly TimeSpan _validationTimeout;
+    protected readonly TimeSpan _validationPollInterval;
+
+    protected DomainOwnershipValidator(
+        IHostApplicationLifetime appLifetime,
+        AcmeClient client,
+        ILogger logger,
+        string domainName,
+        TimeSpan validationTimeout,
+        TimeSpan validationPollInterval)
     {
         _client = client;
         _logger = logger;
         _domainName = domainName;
+        _validationTimeout = validationTimeout;
+        _validationPollInterval = validationPollInterval;
 
         appLifetime.ApplicationStarted.Register(() => _appStarted.TrySetResult(null));
         if (appLifetime.ApplicationStarted.IsCancellationRequested)
@@ -32,25 +42,37 @@ protected DomainOwnershipValidator(IHostApplicationLifetime appLifetime, AcmeCli
 
     protected async Task WaitForChallengeResultAsync(IAuthorizationContext authorizationContext, CancellationToken cancellationToken)
     {
-        var retries = 60;
-        var delay = TimeSpan.FromSeconds(2);
+        var startTime = DateTimeOffset.UtcNow;
+        var attempt = 0;
 
-        while (retries > 0)
+        while (true)
         {
-            retries--;
+            attempt++;
+            var elapsed = DateTimeOffset.UtcNow - startTime;
+
+            if (elapsed >= _validationTimeout)
+            {
+                throw new TimeoutException(
+                    $"Timed out after {elapsed.TotalSeconds:F1} seconds waiting for domain ownership validation of '{_domainName}'. " +
+                    $"Made {attempt} attempts. Consider increasing ValidationTimeout in LettuceEncryptOptions.");
+            }
 
             cancellationToken.ThrowIfCancellationRequested();
 
             var authorization = await _client.GetAuthorizationAsync(authorizationContext);
 
             _logger.LogAcmeAction("GetAuthorization");
+            _logger.LogTrace("Validation attempt {Attempt} for domain '{DomainName}': status = {Status}, elapsed = {Elapsed:F1}s",
+                attempt, _domainName, authorization.Status, elapsed.TotalSeconds);
 
             switch (authorization.Status)
             {
                 case AuthorizationStatus.Valid:
+                    _logger.LogInformation("Domain '{DomainName}' validated successfully after {Attempts} attempts in {Elapsed:F1}s",
+                        _domainName, attempt, elapsed.TotalSeconds);
                     return;
                 case AuthorizationStatus.Pending:
-                    await Task.Delay(delay, cancellationToken);
+                    await Task.Delay(_validationPollInterval, cancellationToken);
                     continue;
                 case AuthorizationStatus.Invalid:
                     throw InvalidAuthorizationError(authorization);
@@ -66,8 +88,6 @@ protected async Task WaitForChallengeResultAsync(IAuthorizationContext authoriza
                         "Unexpected response from server while validating domain ownership.");
             }
         }
-
-        throw new TimeoutException("Timed out waiting for domain ownership validation.");
     }
 
     private Exception InvalidAuthorizationError(Authorization authorization)