Feature/dns01 digitalocean - #5
Merged
Merged
Conversation
…, log-forging Full-review triage on PR #1: - FindZoneForRecordAsync used the pagination link's PathAndQuery instead of the absolute URL, doubling the /v2 segment on page 2+ and breaking zone resolution for any account with more than 200 domains. - DeleteRecordAsync matched by name+type only, so cleanup for one authorization could delete a different pending authorization's TXT record when two SANs (e.g. apex + wildcard) share the same _acme-challenge name. StageValidation now tracks staged values per key so CleanupValidation can match by value too. - CancellationToken accepted by StageValidation/CleanupValidation was never forwarded to the provider or its HTTP calls. - Record names reached log/exception messages with no control-character stripping, allowing embedded CRLF to forge log lines (CWE-117).
… pagination cap Round 2 full-review findings on the round-1 fix: - CleanupValidation popped the staged value from the FIFO queue before confirming the delete succeeded, permanently losing it on a failed/retried cleanup and corrupting disambiguation for later calls on the same key. Now peeks the value and only dequeues it after DeleteRecordAsync confirms success. - StripControlCharacters was only applied inside DigitalOceanProvider on a local copy of the record name; DigitalOceanDomainValidator's own log calls and DomainValidationResult.ErrorMessage still embedded the raw, unsanitized key (same CWE-117 gap one layer up). Added a SafeForLog wrapper at every validator call site that logs or returns the key. - FindZoneForRecordAsync's pagination loop had no bound; added a page cap so a malformed/cyclic `next` link can't hang validation indefinitely. - DeleteRecordAsync's success log now includes the record ID and whether the match was by value or by name, so an audit trail can distinguish a precise disambiguated delete from a name-only fallback. Residual, accepted risk: cleanup's best-effort FIFO match (oldest staged value) can pick the wrong record if two SANs sharing a challenge name complete out of staging order, since CleanupValidation's interface never receives the challenge value. Kept as the least-bad option (discussed and confirmed with the requester) and now logs a warning whenever more than one value is outstanding for a key, so the residual ambiguity is operationally visible.
…r response bodies Round 3 full-review findings on the round-2 fix commit: - The per-key lock introduced in round 2 only covered the queue peek and the final conditional dequeue, not the DeleteRecordAsync call between them. Two concurrent CleanupValidation calls for the same key could both peek the same staged value before either dequeued it, letting one call's delete mask the loss of the other's distinct, still-existing record while both reported success. Now serializes ALL Stage/Cleanup calls for a given key end-to-end (including the network round-trip) via a per-key SemaphoreSlim; different keys still run fully in parallel. - CreateRecordAsync never logged the newly created record's ID (the response type for it existed but was unused), so the audit trail couldn't correlate which DigitalOcean record came from which staging call when disambiguating a later best-effort cleanup match. Now deserializes and logs it. - HTTP response bodies read from DigitalOcean (domain list, records list, create/delete failure bodies) were embedded into log messages and exception text without the same control-character stripping applied to plugin-derived values, reopening the CWE-117 log-forging gap for server-supplied content. Now sanitized immediately after read.
Round 4 full-review finding on the round-3 fix commit: - keyLock.WaitAsync(cancellationToken) was called before the try block in both StageValidation and CleanupValidation. If the token fired while a caller was queued behind another same-key operation, the resulting OperationCanceledException propagated straight out of the method, bypassing the catch block and violating this class's documented contract that no exception may escape into the gateway. Moved the wait inside the try, guarded by a lockAcquired flag so the finally still only releases a lock that was actually taken. - Documented the pre-existing (accepted, low-severity) unbounded growth of _keyLocks/_stagedValues as a known characteristic rather than adding eviction logic under time pressure. - Fixed FakeHttpMessageHandler to run its responder via Task.Run instead of Task.FromResult: a synchronously-completed task let a "fire without awaiting" call in a test run fully inline on the calling thread, so two calls issued without an intervening await never actually overlapped -- masking whether concurrency-related fixes were exercised at all. Added a regression test proving cancellation while queued on the per-key lock now returns Success=false instead of throwing.
…P timeout Round 5 full-review findings (a convergence round): - GetKeyLock(key) was called before the try block in both StageValidation and CleanupValidation. A null key makes the underlying dictionary lookup throw ArgumentNullException, which would escape uncaught -- the same class of contract violation fixed for cancellation in round 4, one line earlier. Moved inside the try alongside the lock wait. - _stagedValues and _keyLocks used ordinal (case-sensitive) string keys while DigitalOceanProvider already matches DNS record names case-insensitively. A Stage/Cleanup pair for the same domain differing only in casing would silently fail to correlate, falling back to the pre-fix name-only match this branch's disambiguation work was built to avoid. Both dictionaries now use StringComparer.OrdinalIgnoreCase. - HttpClient never set an explicit Timeout, so under round 3's per-key lock (now held across the full network round-trip) a single stalled DigitalOcean connection could block an unrelated, legitimate operation for the same key for several minutes (the .NET default of 100s, times up to 3 sequential requests per Create/Delete call). Bounded to 30 seconds. Correctness and security lenses independently re-verified the round 1-4 fixes end-to-end this round with no further findings beyond these three; compliance lens converged clean.
Round 6 full-review finding (a convergence round): the domains list in
FindZoneForRecordAsync was hardened with pagination across all five prior
rounds, but the structurally identical domains/{zone}/records listing used
by DeleteRecordAsync had none -- RecordsResponse didn't even declare a
Links field to read a cursor from. A zone with more than one page of TXT
records (other TXT records, concurrent SAN challenges, or previously
stranded records) would have a target record on page 2+ silently treated
as "not found", reporting cleanup as complete without deleting it and
leaving it in DNS indefinitely -- no adversary required, just a normal
zone with enough TXT records.
Extracted the page cap (now MaxPaginationPages, shared with the domains
listing) and added the same paging loop to the records fetch.
Correctness and compliance lenses converged clean this round with no other
findings.
Triage: full-review findings on PR #1 (DigitalOcean DNS-01 validator)
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
No description provided.