Skip to content

R2: re-sign every retry, bound every attempt, and give the signer a test bed - #822

Merged
anupsv merged 1 commit into
mainfrom
fix/r2-retry-resign-timeout-and-key-charset
Jul 31, 2026
Merged

R2: re-sign every retry, bound every attempt, and give the signer a test bed#822
anupsv merged 1 commit into
mainfrom
fix/r2-retry-resign-timeout-and-key-charset

Conversation

@anupsv

@anupsv anupsv commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Lands the SigV4 audit's remaining findings against
.github/scripts/download-r2-object.sh and .github/scripts/upload-r2-object.sh.

Base note. The task said to branch off fix/r2-sigv4-canonical-request so as
not to revert #818's fix. #818 landed on main as cfc644b before this work
started, so this branches off main (3e9f072), which already contains it — the
canonical-request construction is carried forward unchanged (it just moved inside
sign_request()).


F2 — retries reused the signature, and nothing bounded an attempt

amz_date was computed once and curl owned the retry (--retry 5 --retry-all-errors). curl replays the argv it was handed, so all six attempts
carried the same x-amz-date and the same Authorization: a retry that
cannot re-sign. R2 only accepts a signature inside its ~15 minute clock-skew
window, so a run that spends long enough retrying converts a transient 503 into a
permanent-looking 403 RequestTimeTooSkewed.

With no --max-time/--connect-timeout, a peer that accepts the connection and
then sends nothing triggered neither a retry nor a timeout: the transfer
"succeeded" with an empty body, or the job hung to its own 30-minute limit with no
diagnosis.

The transfer is now a bash loop:

  • every attempt calls sign_request(), which recomputes amz_date,
    date_stamp, the canonical request, the string-to-sign and the signature;
  • every attempt carries --connect-timeout 30 --max-time 600;
  • six attempts as before — what changed is who retries, not how many times.
    R2_RETRY_DELAY_SECONDS (default 2, validated as a non-negative integer) lets
    a test exercise the loop's shape without its wall clock;
  • the --output file is truncated at the top of each iteration. curl opens it
    lazily, so an attempt that dies at connect time never truncates it and a 503
    <Error> document could otherwise sit in front of the object the next attempt
    writes. The caller verifies a pinned sha256 and would report a golden mismatch
    for a transport artefact.

The status-gated error-body reporting from #810 is unchanged.

F4 — the upload payload hash broke on exotic paths

shasum -a 256 "${input_path}" escapes the filename and prefixes the digest
with a backslash
when the path contains a backslash or a newline. Measured:

$ shasum -a 256 'back\slash.json' | awk '{print $1}'
\b7367c22dfc669fdf6f9fcdb91112e6aee109312a2fe68a1508b00dba48cc9cb    (65 chars)
$ shasum -a 256 < 'back\slash.json' | awk '{print $1}'
b7367c22dfc669fdf6f9fcdb91112e6aee109312a2fe68a1508b00dba48cc9cb     (64 chars)

That 65-character non-hex value went into both the signed canonical request
and the x-amz-content-sha256 header — self-consistent, and still rejected,
because R2 compares it against the body it received. A path starting with - was
parsed as options (Unknown option: d). Now shasum -a 256 < "${input_path}",
which has no filename in its output at all.

F5 — --location removed from both scripts

Verified against real curl 8.7.1 (macOS system curl, what the runners use) with
two local servers:

hop authorization x-amz-date x-amz-content-sha256
cross-host, before redirect PRESENT PRESENT PRESENT
cross-host, after 302 absent PRESENT PRESENT
same-host, after 302 to a different path PRESENT (bound to the OLD path) PRESENT PRESENT

So a cross-host redirect delivers an unsigned request that still carries the
signing headers, and a same-host redirect delivers a signature for the wrong
path. R2 path-style issues no legitimate redirects, so following one could only
convert a loud failure into a confusing one.

F3 — the object key charset is now an enforced invariant

Nothing percent-encodes anywhere: the same raw bytes are signed into the canonical
request and handed to curl. For the keys actually in use ([A-Za-z0-9._/-])
identity encoding is exactly right. Outside that set it is not, and every way it
breaks is quiet: a space is curl exit 3, # truncates the URL at the fragment,
? turns the tail into a query string that the canonical request signs as empty,
and non-ASCII is percent-encoded on the wire after being signed raw.

Deliberately not an RFC 3986 encoder — the aws-cli branch encodes by
botocore's rules, and a second encoder here that disagreed with it would be a
fresh divergence between the two branches for the same key. object_path is
validated against ^[A-Za-z0-9._/-]+$ next to the existing empty/absolute/dot-
segment/control-character checks, so an unsupported key is a clear refusal before
any request instead of a 403 that reads like a credentials fault.

A companion test reads the literal *_R2_PATH keys out of benchmark.yml and
dflash-benchmark.yml and runs each one through the script, so a future key the
charset would reject fails in CI rather than on the runner.

F6 — the signed path now has an execution environment

download_with_aws_cli is silently preferred, so the bash signer only ever
ran on boxes without aws. That is exactly how two signing bugs shipped
undetected, and why M5-C's first ever execution of this signer was a production
hidden-golden fetch. R2_FORCE_SIGNED=1 skips the fallback.

A comment at each call site records the other half of the problem: if aws is
present but fails, the script falls through to the signed path silently — so
which implementation performed a given transfer varies run to run, and the only
record is the banner line each branch prints.


Tests

Tests/MLXFastTests/R2RequestExecutionTests.swift (new, 12 tests) executes the
real scripts
, unmodified, with stub curl/date/aws first on M5-C's minimal
PATH (/usr/bin:/bin:/usr/sbin:/sbin), and asserts on the argv curl was actually
handed, attempt by attempt. The stub curl honours --retry in-process, replaying
its argv, so the pre-fix script still produces six attempts here — with identical
headers. The stub date advances 7 minutes per SigV4 clock read, so two attempts
straddle R2's skew window.

R2SignatureTests gains the two gaps the audit named:

  • nothing asserted the string_to_sign CONSTRUCTION. The canonical-request
    test stops at the CR hash and the HMAC test starts from a hand-written
    string-to-sign whose last line is the literal deadbeef, so the line that
    assembles them could lose a newline, swap the date and the scope, or
    interpolate canonical_request where it means canonical_request_hash, and
    every existing assertion still passed. Pinned against the AWS SigV4
    documentation's worked example — its published CanonicalRequest, StringToSign
    and Signature, reproduced byte for byte.
  • nothing composed the chain. CR → hash → STS → signature are each pinned
    separately, so a mismatch between links passes all of them. One pinned
    end-to-end signature per script now runs the shipped chain start to finish over
    R2-shaped inputs.

Both new tests splice the script's own assignment lines and hmac_hex()
definition into a shell, so they track the shipped construction rather than a
restatement of it. Every pinned value was recomputed independently with Python's
hmac/hashlib before being trusted.

swift test: 549 tests / 25 suites pass (535 / 24 on the parent commit).

Attack

Each fix was reverted in turn, with the tests left alone:

reintroduced defect tests that fired
retries reuse one signature (signing hoisted out of the loop) everyRetryIsSignedAfresh, everyUploadRetryIsSignedAfresh
curl owns the retry again (--retry 5 --retry-all-errors) + noAttemptDelegatesRetryToCurl, theOutputFileIsTruncatedBetweenAttempts
no --connect-timeout / --max-time everyAttemptBoundsConnectAndTotalTime, aStalledAttemptTimesOutAndIsRetriedRatherThanSucceedingEmpty
--output not truncated between attempts theOutputFileIsTruncatedBetweenAttempts, everyRetryIsSignedAfresh
shasum -a 256 "${path}" restored thePayloadHashIsTheContentDigestForExoticInputPaths
--location restored noAttemptFollowsRedirects
charset guard removed unsignableObjectKeysAreRefusedBeforeAnyRequest
R2_FORCE_SIGNED removed forceSignedBypassesAWorkingAWSCLI
string_to_sign swaps date and scope theStringToSignMatchesTheAWSDocumentationWorkedExample, theWholeSigningChainReproducesOnePinnedEndToEndSignature
string_to_sign signs the request, not its hash same two
credential scope region ≠ the region keyed into k_region theWholeSigningChainReproducesOnePinnedEndToEndSignature, theEmittedRequestMatchesWhatWasSigned

The last row is the one worth reading. It is a defect between links, and under
it all three pre-existing tests still pass:

Pre-existing coverage (all of it, from #817/#818) under this attack:
   PASS noOpensslDigestParsesTheTextOutputFormat
   PASS theCanonicalRequestMatchesAnIndependentSigV4Implementation
   PASS theScriptSigningChainReproducesThePinnedSignature

Only the tests this PR adds catch it.

Honest scope note. One assertion in this PR is text-based rather than
executed: R2SignatureTests.noOpensslDigestParsesTheTextOutputFormat (pre-
existing, unchanged). Everything else runs the real scripts or splices their real
lines into a shell. noAttemptFollowsRedirects asserts on the argv the real
script handed curl — the consequence of --location was measured separately
with the two-server probe above, not asserted in CI, because reproducing a
cross-host redirect inside the test suite would need two listening hosts.

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith with what you need. Autofix is disabled.

…est bed

The SigV4 audit's remaining findings against .github/scripts/{download,upload}-r2-object.sh.

F2 -- retries reused the signature, and nothing bounded an attempt.
`amz_date` was computed once and curl owned the retry (`--retry 5
--retry-all-errors`). curl replays the argv it was handed, so all six attempts
carried the SAME x-amz-date and the SAME Authorization: a retry that cannot
re-sign. R2 only accepts a signature inside its ~15 minute clock-skew window,
so a run that spends long enough retrying turns a transient 503 into a
permanent-looking 403 RequestTimeTooSkewed. With no --max-time/--connect-timeout
a peer that accepts the connection and then sends nothing triggered neither a
retry nor a timeout: the transfer "succeeded" with an empty body, or the job
hung to its own 30-minute limit with no diagnosis.

The transfer is now a bash loop. Every attempt calls sign_request(), which
recomputes amz_date, date_stamp, the canonical request, the string-to-sign and
the signature, and every attempt carries --connect-timeout 30 --max-time 600.
Six attempts, as before -- what changed is WHO retries. The --output file is
truncated at the top of each iteration, because curl opens it lazily and a
connect-time failure never truncates it, so a 503 <Error> document could
otherwise sit in front of the object the next attempt writes. The
status-gated error-body reporting from #810 is unchanged.

F4 -- the upload payload hash broke on exotic paths.
`shasum -a 256 "${input_path}"` escapes the filename and prefixes the digest
with a backslash when the path contains a backslash or a newline:

    $ shasum -a 256 'back\slash.json' | awk '{print $1}'
    \b7367c22dfc669fdf6f9fcdb91112e6aee109312a2fe68a1508b00dba48cc9cb   (65 chars)

That 65-character non-hex value went into BOTH the signed canonical request
and the x-amz-content-sha256 header -- self-consistent and still rejected. A
path starting with '-' was parsed as options. Now `shasum -a 256 < "${path}"`,
which has no filename in its output at all.

F5 -- --location removed from both scripts.
Measured against curl 8.7.1 with two local servers: on a cross-host redirect
curl DROPS the custom Authorization header but FORWARDS x-amz-date and
x-amz-content-sha256, so the follow-up arrives unsigned with the signing
headers still attached; on a same-host redirect it re-sends a signature bound
to the OLD path. R2 path-style issues no legitimate redirects, so following
one could only convert a loud failure into a confusing one.

F3 -- the object key charset is now an enforced invariant.
Nothing percent-encodes: the same raw bytes are signed and handed to curl. For
the keys in use ([A-Za-z0-9._/-]) identity encoding is correct; outside it a
space is curl exit 3, '#' truncates at the fragment, '?' becomes a query
string the canonical request signs as empty, and non-ASCII is percent-encoded
on the wire after being signed raw. Deliberately NOT an RFC 3986 encoder --
the aws-cli branch encodes by botocore's rules and a second, disagreeing
encoder would be a new divergence. object_path is validated against
^[A-Za-z0-9._/-]+$ next to the existing checks instead.

F6 -- the signed path now has an execution environment.
download_with_aws_cli() is silently PREFERRED, so the bash signer only ran on
boxes without `aws` -- which is how two signing bugs shipped undetected, and
why M5-C's first ever execution of it was a production hidden-golden fetch.
R2_FORCE_SIGNED=1 skips the fallback. A comment at each call site records the
other half of the problem: if `aws` is present but FAILS the script falls
through to the signed path silently, so which implementation performed a given
transfer varies run to run and only the banner line records it.

Tests
-----
Tests/MLXFastTests/R2RequestExecutionTests.swift (new, 12 tests) executes the
real scripts with stub curl/date/aws in front of M5-C's minimal PATH and
asserts on the argv curl was actually handed, attempt by attempt. The stub
curl honours --retry in-process, replaying its argv, so the pre-fix script
still produces six attempts here -- with identical headers.

R2SignatureTests gains the two gaps the audit named: nothing asserted the
string_to_sign CONSTRUCTION (the CR test stops at the hash, the HMAC test
starts from a hand-written string-to-sign ending in `deadbeef`), and nothing
composed CR -> hash -> STS -> signature into one pinned end-to-end signature,
so a mismatch BETWEEN links passed every per-link pin. The string-to-sign test
is pinned against the AWS documentation's worked example (its published
CanonicalRequest, StringToSign and Signature, reproduced byte for byte); the
end-to-end test is pinned against botocore's canonical-request hashes and
signatures independently recomputed with Python's hmac.

Attacked: reintroducing each defect fires the intended test and nothing else
silently passes. The between-link case (credential_scope advertising a region
that is not the one keyed into k_region) passes ALL pre-existing coverage and
is caught only by the two tests this commit adds.

swift test: 549 tests / 25 suites pass (535 / 24 on the parent commit).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@anupsv
anupsv requested a review from a team July 31, 2026 04:14
@anupsv
anupsv merged commit 975736d into main Jul 31, 2026
2 checks passed
anupsv added a commit that referenced this pull request Jul 31, 2026
I had stripped the leading "gautham-experiments/" segment on the assumption it
was the bucket name already carried by R2_BUCKET_ENDPOINT, because the serial
keys are written without a bucket segment.  That was me second-guessing the
operator, who said where the objects are.  Use the path as given:

  gautham-experiments/correctness_prompts/laguna-xs-2.1-dflash/dflash_correctness_golden_hidden.json
  gautham-experiments/correctness_prompts/laguna-xs-2.1-dflash/dflash_benchmark_golden_hidden.json

Both keys pass the object-key charset guard added in #822 (^[A-Za-z0-9._/-]+$),
asserted at pin time rather than discovered on the box: a key outside that set
is refused before any request is signed, and a leading segment does not change
the canonical path construction.

If this key is wrong the failure is unambiguous rather than mysterious: the
download helper reports the HTTP status and the R2 error code separately, so
404 NoSuchKey means the key is wrong and 403 means the bucket or the
credentials' scope is.

swift test: 564 tests, 26 suites, green.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
anupsv added a commit that referenced this pull request Jul 31, 2026
The operator uploaded to
gautham-experiments/correctness_prompts/laguna-xs-2.1-dflash/ and the pinned
keys omit the leading segment, so the fetch would fail 404 NoSuchKey after a
full dispatch.

This is the second time the prefix has been lost.  First I stripped it myself,
assuming "gautham-experiments" was the bucket already carried by
R2_BUCKET_ENDPOINT -- the serial keys are written with no bucket segment, which
made the assumption feel safe, and it was still me overriding what the operator
had told me.  Then the commit that corrected it (18ccd58) missed the merge of
#824, which went in from the preceding commit, so main kept the wrong key.
#825 did not touch paths.

Fixed in both places -- the correctness step's env and the timed_prompt_pool
entry -- and pinned by DFlashGoldenKeyTests so a third loss fails in CI rather
than 30-40 minutes into a ranked dispatch.  The test also asserts each key
stays inside the signer's charset guard (^[A-Za-z0-9._/-]+$ from #822), because
that guard refuses a key BEFORE signing and would otherwise turn a typo into a
confusing dispatch-time abort.

If the objects genuinely move, change that test in the same commit that moves
them.

swift test: 582 tests, 28 suites, green.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant