Implement comprehensive improvements to certificate renewal reliability #5
Conversation
This commit addresses network unreliability and improves the robustness of
the ACME certificate lifecycle with the following enhancements:
**1. Challenge Cleanup and Memory Management**
- Add explicit cleanup of HTTP challenge responses after validation
- Implement automatic cleanup of orphaned challenges (1-hour TTL)
- Add IDisposable to InMemoryHttpChallengeResponseStore with background timer
- Prevent memory leaks during long-running processes
**2. Challenge Endpoint Self-Test**
- Add self-test of HTTP challenge endpoint before requesting Let's Encrypt validation
- Verify challenge endpoint is reachable and returns correct response
- Catch configuration issues (firewall, reverse proxy, port binding) early
- Configurable via LettuceEncryptOptions.EnableChallengeSelfTest (default: true)
- Reduces rate limiting risk from failed validation attempts
**3. Configurable Validation Timeouts and Polling**
- Replace hardcoded 60 retries × 2s with configurable timeout/interval
- Add LettuceEncryptOptions.ValidationTimeout (default: 5 minutes)
- Add LettuceEncryptOptions.ValidationPollInterval (default: 2 seconds)
- Improved for environments with slow DNS propagation or network latency
- Enhanced timeout error messages with actionable guidance
**4. Exponential Backoff for Renewal Failures**
- Implement RenewalFailureTracker to track failures per domain
- Exponential backoff: 1h, 2h, 4h, 8h, capped at 24h
- Override backoff when certificate expires in < 7 days (aggressive retry)
- Reset failure count on successful renewal
- Prevent cascading failures and rate limit exhaustion
- BeginCertificateCreationState no longer throws on failure (returns to CheckForRenewalState)
**5. Enhanced Logging and Diagnostics**
- Add structured logging throughout certificate lifecycle
- Log validation attempt counts, timing, and detailed error context
- Track which challenge methods are attempted and why they fail
- Log renewal decisions with certificate expiry information
- Add startup certificate validation logging with counts
**6. Graceful Degradation on Validation Failures**
- Improve error handling in ValidateDomainOwnershipAsync
- Log each validation method attempt with detailed failure reasons
- Collect all failures before throwing AggregateException
- Better error messages showing which methods were tried and why they failed
- Show remaining validators count during attempts
**7. Startup Certificate Validation**
- Validate certificates on startup (private key, expiry, validity period)
- Skip expired, corrupted, or invalid certificates
- Warn about certificates expiring within 30 days
- Prevent loading broken certificates into the selector
- Comprehensive logging of validation results
**Breaking Changes:**
None - all changes are backward compatible with sensible defaults.
**Configuration Examples:**
```csharp
services.AddLettuceEncrypt(options => {
options.ValidationTimeout = TimeSpan.FromMinutes(10); // For slow DNS
options.ValidationPollInterval = TimeSpan.FromSeconds(5); // Less aggressive polling
options.EnableChallengeSelfTest = true; // Default, catches config issues early
});
```
Fixes issues with:
- Network unreliability causing certificate renewal failures
- Memory leaks from orphaned challenge responses
- Configuration errors not being caught until Let's Encrypt validation
- Lack of backoff after failures leading to rate limiting
- Poor diagnostics when renewals fail
- Loading of invalid/corrupted certificates
Related to the timing fix in commit efe145b that ensured HTTP server
readiness before ACME challenge validation.
|
Warning Rate limit exceeded@Jaben has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 7 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds per-domain renewal backoff tracking, configurable validation timing and optional HTTP-01 self-tests; replaces fixed retry loops with timeout-driven polling; enriches HTTP challenge store with TTL cleanup and removal; threads new options through validators and records per-domain renewal outcomes. Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler as Renewal Scheduler
participant CRS as CheckForRenewalState
participant Tracker as RenewalFailureTracker
participant BCS as BeginCertificateCreationState
participant Factory as AcmeCertificateFactory
participant Validators as Domain Validators
Scheduler->>CRS: MoveNextAsync(domain)
CRS->>Tracker: ShouldAttemptRenewal(domain, certExpiration)
alt Backoff active (skip)
Tracker-->>CRS: false
CRS->>Scheduler: log skip
else Proceed
Tracker-->>CRS: true
CRS->>BCS: Begin creation
BCS->>Factory: CreateCertificateAsync(domains)
Factory->>Validators: Run validators (with timeout/poll/self-test)
par Validator attempts
Validators-->>Factory: success / failure per validator
end
alt Any success
Factory->>BCS: return certificate (early exit)
BCS->>Tracker: RecordSuccess(domain)
else All fail
Factory->>BCS: throw AggregateException
BCS->>Tracker: RecordFailure(domain, exception)
end
end
sequenceDiagram
participant Validator as Http01DomainValidator
participant Store as InMemoryHttpChallengeResponseStore
participant SelfTest as Self-Test Endpoint
participant ACME as ACME Server
Validator->>Store: AddChallengeResponse(token, response)
alt Self-test enabled
Validator->>SelfTest: GET /.well-known/acme-challenge/{token}
alt 200 + expected body
SelfTest-->>Validator: success
else error/timeout
SelfTest-->>Validator: failure (log)
end
end
Validator->>ACME: WaitForChallengeResultAsync(timeout, pollInterval)
ACME-->>Validator: authorization status updates
Validator->>Store: RemoveChallenge(token) // finally
Store-->>Store: delete entry
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/LettuceEncrypt/Internal/InMemoryHttpChallengeStore.cs (1)
23-28: Consider starting cleanup timer sooner.The cleanup timer is configured to run the first cleanup after 15 minutes. While this is acceptable since explicit cleanup via
RemoveChallengeis the primary mechanism, orphaned challenges could remain for up to 1 hour 15 minutes in the worst case.Consider starting the first cleanup sooner for more aggressive memory management:
-_cleanupTimer = new Timer(CleanupExpiredChallenges, null, TimeSpan.FromMinutes(15), TimeSpan.FromMinutes(15)); +_cleanupTimer = new Timer(CleanupExpiredChallenges, null, TimeSpan.FromMinutes(1), TimeSpan.FromMinutes(15));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/LettuceEncrypt/Internal/AcmeCertificateFactory.cs(2 hunks)src/LettuceEncrypt/Internal/AcmeStates/BeginCertificateCreationState.cs(2 hunks)src/LettuceEncrypt/Internal/AcmeStates/CheckForRenewalState.cs(2 hunks)src/LettuceEncrypt/Internal/Dns01DomainValidator.cs(1 hunks)src/LettuceEncrypt/Internal/DomainOwnershipValidator.cs(2 hunks)src/LettuceEncrypt/Internal/Http01DomainValidator.cs(2 hunks)src/LettuceEncrypt/Internal/IHttpChallengeResponseStore.cs(1 hunks)src/LettuceEncrypt/Internal/InMemoryHttpChallengeStore.cs(1 hunks)src/LettuceEncrypt/Internal/RenewalFailureTracker.cs(1 hunks)src/LettuceEncrypt/Internal/StartupCertificateLoader.cs(1 hunks)src/LettuceEncrypt/Internal/TlsAlpn01DomainValidator.cs(1 hunks)src/LettuceEncrypt/LettuceEncryptOptions.cs(1 hunks)src/LettuceEncrypt/LettuceEncryptServiceCollectionExtensions.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
src/LettuceEncrypt/Internal/IHttpChallengeResponseStore.cs (1)
src/LettuceEncrypt/Internal/InMemoryHttpChallengeStore.cs (1)
RemoveChallenge(57-63)
src/LettuceEncrypt/Internal/Http01DomainValidator.cs (6)
src/LettuceEncrypt/Internal/AcmeCertificateFactory.cs (6)
Task(68-84)Task(86-112)Task(114-154)Task(156-205)Task(216-324)Task(326-351)src/LettuceEncrypt/Internal/Dns01DomainValidator.cs (1)
Task(30-46)src/LettuceEncrypt/Internal/DomainOwnershipValidator.cs (2)
Task(41-41)Task(43-91)src/LettuceEncrypt/Internal/TlsAlpn01DomainValidator.cs (2)
Task(28-40)Task(42-58)src/LettuceEncrypt/Internal/IHttpChallengeResponseStore.cs (1)
RemoveChallenge(12-12)src/LettuceEncrypt/Internal/InMemoryHttpChallengeStore.cs (1)
RemoveChallenge(57-63)
src/LettuceEncrypt/Internal/AcmeStates/BeginCertificateCreationState.cs (1)
src/LettuceEncrypt/Internal/RenewalFailureTracker.cs (4)
RenewalFailureTracker(12-151)RenewalFailureTracker(25-28)RecordSuccess(72-81)RecordFailure(33-67)
src/LettuceEncrypt/Internal/AcmeCertificateFactory.cs (4)
src/LettuceEncrypt/Internal/TlsAlpn01DomainValidator.cs (2)
TlsAlpn01DomainValidator(11-59)TlsAlpn01DomainValidator(15-26)src/LettuceEncrypt/Internal/Http01DomainValidator.cs (2)
Http01DomainValidator(11-134)Http01DomainValidator(17-30)src/LettuceEncrypt/Internal/Dns01DomainValidator.cs (2)
Dns01DomainValidator(13-75)Dns01DomainValidator(17-28)src/LettuceEncrypt/Internal/DomainOwnershipValidator.cs (1)
Exception(93-112)
src/LettuceEncrypt/LettuceEncryptOptions.cs (1)
src/LettuceEncrypt/Internal/RenewalFailureTracker.cs (1)
TimeSpan(127-132)
src/LettuceEncrypt/Internal/TlsAlpn01DomainValidator.cs (2)
src/LettuceEncrypt/Internal/TlsAlpnChallengeResponder.cs (2)
TlsAlpnChallengeResponder(22-123)TlsAlpnChallengeResponder(34-44)src/LettuceEncrypt/Internal/AcmeClient.cs (2)
AcmeClient(12-131)AcmeClient(20-27)
src/LettuceEncrypt/Internal/AcmeStates/CheckForRenewalState.cs (1)
src/LettuceEncrypt/Internal/RenewalFailureTracker.cs (4)
RenewalFailureTracker(12-151)RenewalFailureTracker(25-28)ShouldAttemptRenewal(86-121)GetFailureInfo(137-150)
src/LettuceEncrypt/Internal/DomainOwnershipValidator.cs (5)
src/LettuceEncrypt/Internal/AcmeClient.cs (2)
AcmeClient(12-131)AcmeClient(20-27)src/LettuceEncrypt/Internal/LoggerExtensions.cs (2)
LogAcmeAction(11-19)LogAcmeAction(21-29)src/LettuceEncrypt/Internal/AcmeCertificateFactory.cs (6)
Task(68-84)Task(86-112)Task(114-154)Task(156-205)Task(216-324)Task(326-351)src/LettuceEncrypt/Internal/Http01DomainValidator.cs (3)
Task(32-48)Task(50-83)Task(85-133)src/LettuceEncrypt/Internal/TlsAlpn01DomainValidator.cs (2)
Task(28-40)Task(42-58)
src/LettuceEncrypt/Internal/InMemoryHttpChallengeStore.cs (1)
src/LettuceEncrypt/Internal/IHttpChallengeResponseStore.cs (3)
AddChallengeResponse(8-8)TryGetResponse(10-10)RemoveChallenge(12-12)
src/LettuceEncrypt/LettuceEncryptServiceCollectionExtensions.cs (1)
src/LettuceEncrypt/Internal/RenewalFailureTracker.cs (2)
RenewalFailureTracker(12-151)RenewalFailureTracker(25-28)
src/LettuceEncrypt/Internal/StartupCertificateLoader.cs (3)
src/LettuceEncrypt/Internal/X509CertificateHelpers.cs (1)
IEnumerable(21-28)src/LettuceEncrypt/Internal/CertificateSelector.cs (5)
CertificateSelector(14-174)CertificateSelector(25-29)X509Certificate2(70-85)X509Certificate2(100-122)Add(33-48)test/LettuceEncrypt.UnitTests/TestUtils.cs (2)
X509Certificate2(11-14)X509Certificate2(16-64)
🪛 GitHub Actions: CI
src/LettuceEncrypt/LettuceEncryptOptions.cs
[error] 117-117: RS0016: Symbol 'ValidationTimeout.get' is not part of the declared public API.
🪛 GitHub Check: build (macos-latest)
src/LettuceEncrypt/LettuceEncryptOptions.cs
[failure] 123-123:
Symbol 'ValidationPollInterval.set' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 123-123:
Symbol 'ValidationPollInterval.get' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 117-117:
Symbol 'ValidationTimeout.set' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 117-117:
Symbol 'ValidationTimeout.get' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 129-129:
Symbol 'EnableChallengeSelfTest.set' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 129-129:
Symbol 'EnableChallengeSelfTest.get' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 123-123:
Symbol 'ValidationPollInterval.set' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 123-123:
Symbol 'ValidationPollInterval.get' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 117-117:
Symbol 'ValidationTimeout.set' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 117-117:
Symbol 'ValidationTimeout.get' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
🪛 GitHub Check: build (ubuntu-latest)
src/LettuceEncrypt/LettuceEncryptOptions.cs
[failure] 123-123:
Symbol 'ValidationPollInterval.set' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 123-123:
Symbol 'ValidationPollInterval.get' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 117-117:
Symbol 'ValidationTimeout.set' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 117-117:
Symbol 'ValidationTimeout.get' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 129-129:
Symbol 'EnableChallengeSelfTest.set' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 129-129:
Symbol 'EnableChallengeSelfTest.get' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 123-123:
Symbol 'ValidationPollInterval.set' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 123-123:
Symbol 'ValidationPollInterval.get' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 117-117:
Symbol 'ValidationTimeout.set' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 117-117:
Symbol 'ValidationTimeout.get' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
🪛 GitHub Check: build (windows-latest)
src/LettuceEncrypt/LettuceEncryptOptions.cs
[failure] 123-123:
Symbol 'ValidationPollInterval.set' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 123-123:
Symbol 'ValidationPollInterval.get' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 117-117:
Symbol 'ValidationTimeout.get' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 117-117:
Symbol 'ValidationTimeout.set' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 129-129:
Symbol 'EnableChallengeSelfTest.set' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 129-129:
Symbol 'EnableChallengeSelfTest.get' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 123-123:
Symbol 'ValidationPollInterval.set' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 123-123:
Symbol 'ValidationPollInterval.get' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 117-117:
Symbol 'ValidationTimeout.get' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 117-117:
Symbol 'ValidationTimeout.set' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (windows-latest)
🔇 Additional comments (9)
src/LettuceEncrypt/Internal/TlsAlpn01DomainValidator.cs (1)
15-26: LGTM! Constructor updated for configurable validation timing.The constructor signature cleanly passes through the new timeout and polling parameters to the base class, aligning with the PR's goal of making validation timing configurable.
src/LettuceEncrypt/Internal/Http01DomainValidator.cs (3)
14-15: Good use of static HttpClient.Using a static
HttpClientinstance is the correct pattern here to avoid socket exhaustion. The 10-second timeout is appropriate for the localhost self-test scenario.
17-30: LGTM! Constructor properly initialized.The constructor correctly accepts the new parameters and forwards the validation timing configuration to the base class while initializing the self-test flag.
85-133: Well-implemented self-test with comprehensive error handling.The self-test implementation is thorough with clear error messages, proper exception handling for network failures and timeouts, and safe string operations using
Math.Minto avoid bounds issues.src/LettuceEncrypt/Internal/InMemoryHttpChallengeStore.cs (5)
9-21: LGTM! Well-structured class with proper lifecycle management.The class correctly implements
IDisposableand uses a privateChallengeEntrytype to track creation timestamps for TTL-based cleanup.
30-41: LGTM! Thread-safe implementation with helpful logging.The method correctly creates timestamped entries and uses
AddOrUpdatefor thread-safe insertion. The logging includes a token prefix for debugging without exposing sensitive data.
43-63: LGTM! Clean implementation of retrieval and removal.Both
TryGetResponseandRemoveChallengecorrectly use thread-safeConcurrentDictionaryoperations with appropriate trace and debug logging.
65-95: LGTM! Robust cleanup implementation with proper error handling.The cleanup logic correctly:
- Guards against execution after disposal
- Uses LINQ to snapshot expired entries (safe with
ConcurrentDictionary)- Handles removal safely with
TryRemove- Includes comprehensive error handling and logging
97-106: Dispose implementation is correct.The disposal pattern properly prevents re-entrance and disposes the timer. Note that public methods (
AddChallengeResponse,TryGetResponse,RemoveChallenge) don't check the_disposedflag, but this is likely acceptable since the store typically has application lifetime and disposal only occurs at shutdown.
| public TimeSpan ValidationTimeout { get; set; } = TimeSpan.FromMinutes(5); | ||
|
|
||
| /// <summary> | ||
| /// Interval between polling attempts when waiting for domain validation. | ||
| /// Default is 2 seconds. Reduce for faster feedback in testing environments. | ||
| /// </summary> | ||
| public TimeSpan ValidationPollInterval { get; set; } = TimeSpan.FromSeconds(2); | ||
|
|
||
| /// <summary> | ||
| /// Enable self-test of HTTP challenge endpoint before requesting validation from Let's Encrypt. | ||
| /// Default is true. Helps catch configuration issues early. | ||
| /// </summary> | ||
| public bool EnableChallengeSelfTest { get; set; } = true; |
There was a problem hiding this comment.
Fix missing public API declarations for new options
The Roslyn PublicAPI analyzer is failing the build (RS0016) because these new public properties aren’t listed in the public API baseline. Please add them to the appropriate PublicAPI.Unshipped.txt (or equivalent) so CI passes.
+P:LettuceEncrypt.LettuceEncryptOptions.ValidationTimeout
+P:LettuceEncrypt.LettuceEncryptOptions.ValidationPollInterval
+P:LettuceEncrypt.LettuceEncryptOptions.EnableChallengeSelfTestCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: CI
[error] 117-117: RS0016: Symbol 'ValidationTimeout.get' is not part of the declared public API.
🪛 GitHub Check: build (macos-latest)
[failure] 123-123:
Symbol 'ValidationPollInterval.set' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 123-123:
Symbol 'ValidationPollInterval.get' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 117-117:
Symbol 'ValidationTimeout.set' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 117-117:
Symbol 'ValidationTimeout.get' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 129-129:
Symbol 'EnableChallengeSelfTest.set' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 129-129:
Symbol 'EnableChallengeSelfTest.get' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 123-123:
Symbol 'ValidationPollInterval.set' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 123-123:
Symbol 'ValidationPollInterval.get' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 117-117:
Symbol 'ValidationTimeout.set' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 117-117:
Symbol 'ValidationTimeout.get' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
🪛 GitHub Check: build (ubuntu-latest)
[failure] 123-123:
Symbol 'ValidationPollInterval.set' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 123-123:
Symbol 'ValidationPollInterval.get' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 117-117:
Symbol 'ValidationTimeout.set' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 117-117:
Symbol 'ValidationTimeout.get' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 129-129:
Symbol 'EnableChallengeSelfTest.set' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 129-129:
Symbol 'EnableChallengeSelfTest.get' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 123-123:
Symbol 'ValidationPollInterval.set' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 123-123:
Symbol 'ValidationPollInterval.get' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 117-117:
Symbol 'ValidationTimeout.set' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 117-117:
Symbol 'ValidationTimeout.get' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
🪛 GitHub Check: build (windows-latest)
[failure] 123-123:
Symbol 'ValidationPollInterval.set' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 123-123:
Symbol 'ValidationPollInterval.get' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 117-117:
Symbol 'ValidationTimeout.get' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 117-117:
Symbol 'ValidationTimeout.set' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 129-129:
Symbol 'EnableChallengeSelfTest.set' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 129-129:
Symbol 'EnableChallengeSelfTest.get' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 123-123:
Symbol 'ValidationPollInterval.set' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 123-123:
Symbol 'ValidationPollInterval.get' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 117-117:
Symbol 'ValidationTimeout.get' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
[failure] 117-117:
Symbol 'ValidationTimeout.set' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)
🤖 Prompt for AI Agents
In src/LettuceEncrypt/LettuceEncryptOptions.cs around lines 117-129, three new
public properties were added but not recorded in the public API baseline; add
entries for them to PublicAPI.Unshipped.txt so the Roslyn PublicAPI analyzer
passes. Add these three lines (one per property) to the Unshipped file:
P:LettuceEncrypt.LettuceEncryptOptions.ValidationTimeout
P:LettuceEncrypt.LettuceEncryptOptions.ValidationPollInterval
P:LettuceEncrypt.LettuceEncryptOptions.EnableChallengeSelfTest, then commit the
updated PublicAPI.Unshipped.txt (or use your publicapi tool to add them) and
re-run the build/CI.
Add the three new configuration properties to the public API surface: - ValidationTimeout - ValidationPollInterval - EnableChallengeSelfTest This fixes the PublicApiAnalyzer errors in the build.
This script automatically detects new public APIs and adds them to PublicAPI.Unshipped.txt, making it easier to maintain the public API surface. Usage: ./update-public-api.sh
**1. Fix challenge token leak in Http01DomainValidator** - Challenge was stored but not cleaned up if PrepareHttpChallengeResponseAsync threw after AddChallengeResponse but before returning token - Wrapped challenge preparation in try-catch to remove from store on failure - Ensures cleanup happens even if self-test or validation request fails **2. Fix cancellation handling in BeginCertificateCreationState** - Catch-all was swallowing OperationCanceledException/TaskCanceledException - Now rethrows cancellation exceptions immediately before recording failures - Preserves cooperative cancellation semantics for host shutdown **3. Fix cancellation handling in AcmeCertificateFactory** - ValidateDomainOwnershipAsync catch-all was swallowing cancellations - Now rethrows cancellation exceptions immediately before adding to failures - Ensures proper cancellation propagation during domain validation These fixes ensure: - No challenge response memory leaks on validation failures - Proper cancellation token propagation throughout the system - Only non-cancellation exceptions are tracked as renewal failures
Update test to pass logger parameter to StartupCertificateLoader constructor. This was required after adding certificate validation logging.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
update-public-api.sh (1)
8-29: Harden pipeline error handlingPlease add
set -o pipefail(preferably alongsideset -e) so failures inside theecho | grep | whilepipeline don’t get masked. That keeps the script honest ifgrepencounters an I/O error.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/LettuceEncrypt/Internal/AcmeCertificateFactory.cs(2 hunks)src/LettuceEncrypt/Internal/AcmeStates/BeginCertificateCreationState.cs(2 hunks)src/LettuceEncrypt/Internal/Http01DomainValidator.cs(2 hunks)src/LettuceEncrypt/PublicAPI.Unshipped.txt(1 hunks)test/LettuceEncrypt.UnitTests/StartupCertificateLoaderTests.cs(1 hunks)update-public-api.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
test/LettuceEncrypt.UnitTests/StartupCertificateLoaderTests.cs (1)
src/LettuceEncrypt/Internal/StartupCertificateLoader.cs (1)
StartupCertificateLoader(10-159)
src/LettuceEncrypt/Internal/Http01DomainValidator.cs (7)
src/LettuceEncrypt/Internal/AcmeClient.cs (2)
AcmeClient(12-131)AcmeClient(20-27)src/LettuceEncrypt/Internal/AcmeCertificateFactory.cs (6)
Task(68-84)Task(86-112)Task(114-154)Task(156-205)Task(216-330)Task(332-357)src/LettuceEncrypt/Internal/DomainOwnershipValidator.cs (2)
Task(41-41)Task(43-91)src/LettuceEncrypt/Internal/Dns01DomainValidator.cs (1)
Task(30-46)src/LettuceEncrypt/Internal/TlsAlpn01DomainValidator.cs (2)
Task(28-40)Task(42-58)src/LettuceEncrypt/Internal/IHttpChallengeResponseStore.cs (2)
RemoveChallenge(12-12)AddChallengeResponse(8-8)src/LettuceEncrypt/Internal/InMemoryHttpChallengeStore.cs (2)
RemoveChallenge(57-63)AddChallengeResponse(30-41)
src/LettuceEncrypt/Internal/AcmeCertificateFactory.cs (4)
src/LettuceEncrypt/Internal/TlsAlpn01DomainValidator.cs (2)
TlsAlpn01DomainValidator(11-59)TlsAlpn01DomainValidator(15-26)src/LettuceEncrypt/Internal/Http01DomainValidator.cs (2)
Http01DomainValidator(11-145)Http01DomainValidator(17-30)src/LettuceEncrypt/Internal/Dns01DomainValidator.cs (2)
Dns01DomainValidator(13-75)Dns01DomainValidator(17-28)src/LettuceEncrypt/Internal/DomainOwnershipValidator.cs (1)
Exception(93-112)
src/LettuceEncrypt/Internal/AcmeStates/BeginCertificateCreationState.cs (1)
src/LettuceEncrypt/Internal/RenewalFailureTracker.cs (4)
RenewalFailureTracker(12-151)RenewalFailureTracker(25-28)RecordSuccess(72-81)RecordFailure(33-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (windows-latest)
- GitHub Check: build (windows-latest)
🔇 Additional comments (6)
test/LettuceEncrypt.UnitTests/StartupCertificateLoaderTests.cs (1)
32-35: Constructor call updated correctlyPassing
NullLogger<StartupCertificateLoader>.Instancekeeps the test lightweight while satisfying the new dependency. Looks good.src/LettuceEncrypt/PublicAPI.Unshipped.txt (1)
2-7: Public API list looks accurateThe added getters/setters align with the new option properties. No further changes needed.
src/LettuceEncrypt/Internal/AcmeCertificateFactory.cs (1)
238-329: Validator configuration wiring looks solidThreading
ValidationTimeout,ValidationPollInterval, andEnableChallengeSelfTestthrough to the individual validators makes the behavior configurable end-to-end. No concerns here.src/LettuceEncrypt/Internal/AcmeStates/BeginCertificateCreationState.cs (3)
17-17: RenewalFailureTracker integration looks good.The dependency injection of
RenewalFailureTrackeris properly implemented with readonly field declaration, constructor parameter, and assignment following the established pattern.Also applies to: 19-26, 34-34
57-61: Success tracking correctly placed after certificate persistence.Recording success for all domains after
SaveCertificateAsynccompletes ensures the operation is fully successful before clearing failure state. The per-domain iteration is appropriate for the backoff strategy.
65-80: Exception handling and backoff flow implemented correctly.The cancellation exception handling properly preserves cooperative cancellation semantics (addressing the past review comment). Non-cancellation exceptions are recorded per-domain for backoff tracking, logged with full context, and the state machine transitions to
CheckForRenewalStateto implement exponential backoff rather than propagating the failure.
…ings **Problem:** The self-test hardcoded 'http://localhost' which fails in: - Containerized apps binding to 0.0.0.0 or specific ports - Apps behind reverse proxies - Apps with non-standard ports - Multi-binding scenarios **Solution:** 1. Add ChallengeSelfTestBaseUrl option to LettuceEncryptOptions - Allows explicit configuration for containers/proxies - Examples: 'http://localhost:5000', 'http://0.0.0.0:8080' 2. Implement intelligent URL detection in Http01DomainValidator: - Prefer explicitly configured base URL (ChallengeSelfTestBaseUrl) - Auto-detect from IServerAddressesFeature when not configured - Convert wildcard bindings (0.0.0.0, +, *) to localhost - Match domain name when multiple HTTP bindings exist - Warn if only HTTPS bindings found - Fall back to localhost only as last resort 3. Enhanced error messages: - Show resolved challenge URL in all messages - Suggest setting ChallengeSelfTestBaseUrl for containers - Log detected server bindings for troubleshooting **Examples:** **Changes:** - Add ChallengeSelfTestBaseUrl to LettuceEncryptOptions - Inject IServer into AcmeCertificateFactory for binding detection - Pass server and base URL to Http01DomainValidator - Implement ResolveChallengeSelfTestBaseUrl() with fallback logic - Update PublicAPI.Unshipped.txt for new public property
Add missing IServer parameter to AcmeCertificateFactory test constructor. This was required after adding server binding detection for self-test URL.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/LettuceEncrypt/LettuceEncryptOptions.cs (1)
113-123: Consider adding validation for timing configuration.The properties lack constraints on their values, which could lead to runtime issues:
- Negative or zero durations could cause unexpected behavior in polling loops
- A
ValidationPollIntervalexceedingValidationTimeoutcreates a logical inconsistencyConsider adding property setters with validation:
- public TimeSpan ValidationTimeout { get; set; } = TimeSpan.FromMinutes(5); + private TimeSpan _validationTimeout = TimeSpan.FromMinutes(5); + public TimeSpan ValidationTimeout + { + get => _validationTimeout; + set => _validationTimeout = value > TimeSpan.Zero + ? value + : throw new ArgumentOutOfRangeException(nameof(value), "ValidationTimeout must be positive"); + }- public TimeSpan ValidationPollInterval { get; set; } = TimeSpan.FromSeconds(2); + private TimeSpan _validationPollInterval = TimeSpan.FromSeconds(2); + public TimeSpan ValidationPollInterval + { + get => _validationPollInterval; + set => _validationPollInterval = value > TimeSpan.Zero + ? value + : throw new ArgumentOutOfRangeException(nameof(value), "ValidationPollInterval must be positive"); + }src/LettuceEncrypt/Internal/AcmeCertificateFactory.cs (1)
274-334: Consider extracting validator display name to a property.The string manipulation
Replace("DomainValidator", "")at lines 285, 322, and 332 is repeated and fragile. If validator class names change, these will silently break.Consider adding a virtual property to
DomainOwnershipValidator:// In DomainOwnershipValidator base class public virtual string DisplayName => GetType().Name.Replace("DomainValidator", "");Then use
validator.DisplayNameinstead of repeating the replacement logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/LettuceEncrypt/Internal/AcmeCertificateFactory.cs(6 hunks)src/LettuceEncrypt/Internal/Http01DomainValidator.cs(3 hunks)src/LettuceEncrypt/LettuceEncryptOptions.cs(1 hunks)src/LettuceEncrypt/PublicAPI.Unshipped.txt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/LettuceEncrypt/Internal/Http01DomainValidator.cs (5)
src/LettuceEncrypt/Internal/AcmeClient.cs (2)
AcmeClient(12-131)AcmeClient(20-27)src/LettuceEncrypt/Internal/AcmeCertificateFactory.cs (6)
Task(72-88)Task(90-116)Task(118-158)Task(160-209)Task(220-335)Task(337-362)src/LettuceEncrypt/Internal/DomainOwnershipValidator.cs (2)
Task(41-41)Task(43-91)src/LettuceEncrypt/Internal/IHttpChallengeResponseStore.cs (2)
RemoveChallenge(12-12)AddChallengeResponse(8-8)src/LettuceEncrypt/Internal/InMemoryHttpChallengeStore.cs (2)
RemoveChallenge(57-63)AddChallengeResponse(30-41)
src/LettuceEncrypt/Internal/AcmeCertificateFactory.cs (4)
src/LettuceEncrypt/Internal/TlsAlpn01DomainValidator.cs (2)
TlsAlpn01DomainValidator(11-59)TlsAlpn01DomainValidator(15-26)src/LettuceEncrypt/Internal/Http01DomainValidator.cs (2)
Http01DomainValidator(13-220)Http01DomainValidator(21-38)src/LettuceEncrypt/Internal/Dns01DomainValidator.cs (2)
Dns01DomainValidator(13-75)Dns01DomainValidator(17-28)src/LettuceEncrypt/Internal/DomainOwnershipValidator.cs (1)
Exception(93-112)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (windows-latest)
- GitHub Check: build (windows-latest)
🔇 Additional comments (10)
src/LettuceEncrypt/LettuceEncryptOptions.cs (1)
125-137: LGTM!The self-test feature with default-on behavior provides valuable early detection of configuration issues. The opt-out via
EnableChallengeSelfTestand the explicitChallengeSelfTestBaseUrlconfiguration give users adequate control for non-standard environments.src/LettuceEncrypt/PublicAPI.Unshipped.txt (1)
1-9: LGTM! Public API declarations correctly address analyzer requirements.All four new properties from
LettuceEncryptOptionsare properly declared with their getters and setters. This resolves the RS0016 analyzer failures flagged in previous reviews.src/LettuceEncrypt/Internal/AcmeCertificateFactory.cs (3)
12-12: LGTM! Clean IServer integration.The
IServerdependency injection follows the established pattern and is required for HTTP challenge self-test URL detection inHttp01DomainValidator.Also applies to: 32-32, 48-48, 61-61
243-263: LGTM! Configuration correctly threaded to validators.The options are cleanly extracted and passed to the appropriate validators. Each validator receives the timing configuration, and the HTTP-01 validator additionally receives self-test parameters and server access.
300-317: LGTM! Cancellation semantics properly preserved.The exception handling correctly rethrows cancellation exceptions (lines 302-306) before collecting other failures, addressing the concern from the previous review. This ensures cooperative cancellation works as expected while still aggregating validation failures.
src/LettuceEncrypt/Internal/Http01DomainValidator.cs (5)
16-19: LGTM! Fields appropriately structured for self-test functionality.The static
HttpClientwith a 10-second timeout is the correct pattern for reusable HTTP calls, avoiding socket exhaustion while providing reasonable self-test latency bounds.
40-56: LGTM! Resource cleanup correctly implemented.The try-finally pattern properly addresses the resource leak concern from previous reviews. If
PrepareHttpChallengeResponseAsyncthrows before returning the token (e.g., self-test failure), its internal catch block cleans up. If it succeeds, the finally block here ensures cleanup after validation completes or fails.
58-102: LGTM! Challenge preparation with robust cleanup.The try-catch structure ensures that if any step after adding the challenge to the store fails (self-test, validation request), the challenge is removed before rethrowing. The token is returned only on complete success, properly coordinating with the outer finally block.
104-155: LGTM! Comprehensive self-test implementation with excellent diagnostics.The self-test thoroughly validates the HTTP challenge endpoint with detailed error messages for each failure mode (status code, content mismatch, network errors, timeouts). The error guidance correctly points users to common issues (middleware registration, server configuration, containerized deployments).
157-219: LGTM! Sophisticated base URL resolution addresses past review concern.The three-tier fallback strategy (explicit config → server bindings → localhost) effectively addresses the previous concern about hard-coded localhost. The server binding detection handles wildcard addresses (0.0.0.0, +, *) and provides informative warnings when falling back. This should work well across standard, containerized, and non-standard deployments.
This commit addresses network unreliability and improves the robustness of the ACME certificate lifecycle with the following enhancements:
1. Challenge Cleanup and Memory Management
2. Challenge Endpoint Self-Test
3. Configurable Validation Timeouts and Polling
4. Exponential Backoff for Renewal Failures
5. Enhanced Logging and Diagnostics
6. Graceful Degradation on Validation Failures
7. Startup Certificate Validation
Breaking Changes:
None - all changes are backward compatible with sensible defaults.
Configuration Examples:
Fixes issues with:
Related to the timing fix in commit efe145b that ensured HTTP server readiness before ACME challenge validation.
Summary by CodeRabbit
New Features
Improvements