Bound the 429 retry loop against a hostile Retry-After - #396
Merged
Conversation
ben-duo
approved these changes
Jul 31, 2026
Make loop termination independent of a server-supplied Retry-After header, and validate that header consistently across both of its forms. lib/duo.c: - Cap 429 retries at six (matching the no-header backoff schedule 1,2,4,8,16,32) so _duo_https_exchange() always returns and the caller can apply failmode. - Map a 429 that survives the retry cap to DUO_SERVER_ERROR so failmode is applied rather than a terminal abort. - Compute the header-derived delay in time_t and clamp it into the backoff band before narrowing to int, so an out-of-range deadline cannot be masked by the conversion. - Only an absent header enables exponential backoff; a present-but- unusable header is terminal. lib/https.c / lib/https.h: - Factor the delay->deadline conversion into _retry_after_deadline(), which takes `now` as a parameter so the range and overflow guards are unit-testable. Reject out-of-range (ERANGE), negative, and implausibly large values, and guard the time_t addition against overflow near the time_t maximum (32-bit 2038 case). - Parse the HTTP-date form with a literal "GMT" instead of %Z (glibc's %Z accepts any token, or none), require full consumption, reject past or unrepresentable dates, and route the result through the same validator as the numeric form. - Distinguish "header absent" from "header present but unusable" with two named sentinels (DUO_RETRY_AFTER_NONE / DUO_RETRY_AFTER_INVALID) so an unusable header does not enable backoff. - Snapshot errno immediately after strtol() rather than reading it as a sibling argument to time(). Tests: - parse_retry_after_test covers both branches: valid/garbage/negative/ overflow/over-ceiling/trailing-junk numeric parses; valid-future, past, non-GMT zone, absent zone, trailing junk (glued and separated), and far-future dates; the empty-header edge case; and the near-time_t headroom guard compared as time_t directly so a (long) truncation on 64-bit-time_t/32-bit-long targets cannot void the assertion. - PreauthStates integration tests: a server that replies 429 on every request terminates and applies failmode in bounded time under both failmode=safe (allow) and failmode=secure (deny, exit 1); an unusable header is terminal rather than a full backoff. - Forward **kwargs through the PreauthStates call_binary wrappers so tests can set a per-call timeout; mock date fixture emits literal GMT. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
AaronAtDuo
force-pushed
the
bound-429-retry-loop
branch
from
July 31, 2026 18:45
3113626 to
8656510
Compare
jeffreyparker
approved these changes
Jul 31, 2026
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
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.
Summary of the change
Make loop termination independent of a server-supplied Retry-After header, and validate that header consistently across both of its accepted forms (delta-seconds and HTTP-date).
Fixes:
_duo_https_exchange()always returns and the caller can apply failmode; map a 429 that survives the retry cap toDUO_SERVER_ERRORso failmode applies rather than a terminal abort; compute the header-derived delay intime_tand clamp it into the backoff band before narrowing toint, so an out-of-range deadline cannot be masked by the conversion. Only an absent header enables exponential backoff._retry_after_deadline(), which takesnowas a parameter so the range and overflow guards are unit-testable; reject out-of-range (ERANGE), negative, and implausibly large values and guard thetime_taddition against overflow near thetime_tmaximum. Parse the HTTP-date form with a literalGMTinstead of%Z(glibc's%Zaccepts any token, or none), require full consumption, reject past or unrepresentable dates, and route the result through the same validator as the numeric form. Distinguish "header absent" from "header present but unusable" with two named sentinels (DUO_RETRY_AFTER_NONE/DUO_RETRY_AFTER_INVALID) so an unusable header does not enable backoff. Snapshoterrnoimmediately afterstrtol()rather than reading it as a sibling argument totime().Tests:
parse_retry_after_testcovers both branches: numeric (valid / garbage / negative / overflow / over-ceiling / trailing-junk) and HTTP-date (valid-future, past, non-GMT zone, absent zone, trailing junk both glued and whitespace-separated, far-future), plus the empty-header edge case. The near-time_theadroom assertions comparetime_tvalues directly so a(long)truncation on 64-bit-time_t/32-bit-longtargets cannot void them.failmode=safe(allow) andfailmode=secure(deny, exit 1); an unusable header is terminal rather than a full backoff.**kwargsthrough the PreauthStatescall_binarywrappers so tests can set a per-call timeout; the mock date fixture emits a literal GMT zone.Review resolution
Addresses the required items from the security review: hardens the HTTP-date branch (item 1); adds HTTP-date and empty-header unit tests (item 2); replaces the overloaded sentinel so an invalid/oversized header is terminal rather than triggering backoff, and corrects the accompanying comment (item 3); covers the fail-secure 429 path in the integration tests (item 4); and compares
time_tdirectly so the 2038 assertions are not voided by narrowing (item 5). The remediation approach follows the iteration-cap option; the exponential-doubling alternative was declined so the header-less schedule stays unchanged.Test Plan
New and existing tests pass. Verified in a clean build: full unity suite (19
parse_retry_aftercases + 17 others) green, fullPreauthStatesintegration suite green under both failmodes, and cppcheck clean on the changed C files. Each guarded regression was mutation-tested — reverting the date hardening, the terminal-invalid behavior, or the 429→DUO_SERVER_ERRORmapping each fails only the corresponding test.This PR description update was generated with AI assistance (Claude).