From 5d7ced9f37d326b2918551c079f39b1847929628 Mon Sep 17 00:00:00 2001 From: anupsv <6407789+anupsv@users.noreply.github.com> Date: Thu, 30 Jul 2026 17:24:06 -0700 Subject: [PATCH] R2: sign the canonical request SigV4 actually specifies The DFlash gates-only dry run dies at "Prepare hidden correctness golden" with HTTP 403 SignatureDoesNotMatch. #817 fixed a real blank-signature 400 underneath this and uncovered the next bug: the signature is well-formed, and R2 is rejecting it because we sign a MALFORMED canonical request. SigV4 is METHOD \n URI \n QUERY \n CanonicalHeaders \n SignedHeaders \n PayloadHash and CanonicalHeaders is itself "name:value\n" per header, so a BLANK LINE separates the last header from SignedHeaders. Both scripts spelled that terminating newline inside canonical_headers' own printf: canonical_headers="$(printf 'host:%s\n...\nx-amz-date:%s\n' ...)" canonical_request="$(printf 'GET\n%s\n\n%s\n%s\n%s' ...)" Command substitution strips every trailing newline, so canonical_headers arrived without its terminator and the canonical request went on the wire one line short -- 8 lines instead of 9. R2 hashes the 9-line form for the same request, the hashes disagree, and it answers 403. That reads like a bucket-permission fault and is not one: R2 resolved the access key, the bucket and the key, and only disagreed about the signature. Move both newlines into the canonical_request format string, where nothing can strip them: one terminates the last header line, one is the blank separator. Same defect and same fix in the upload twin. Why this survived: it has never run anywhere. download_with_aws_cli() short-circuits the signer whenever `aws` is on the runner PATH, and the serial box has it -- every successful hidden-golden fetch in either repo announced "using AWS CLI S3 path-style download", never "using signed HTTPS download". M5-C's runner PATH is /usr/bin:/bin:/usr/sbin:/sbin, so it is the first box to execute this code at all, and it has now surfaced two latent bugs in a row from the same unexercised path. So also correct #817's comment claiming "the serial box worked only because OpenSSL 3 was first on its PATH." That is false -- the serial box never runs openssl in this script -- and believing it sends the next debugger to audit PATH ordering on a box that does not run the code. The existing guard could not catch this. It signs a string-to-sign ending in the literal `deadbeef`, a stand-in for the canonical-request hash, so it proves the HMAC chain and says nothing about the canonical request being hashed -- which is how #817 shipped a correct signer over a malformed input. Add theCanonicalRequestMatchesAnIndependentSigV4Implementation: it extracts the real canonical_headers/canonical_request assignments out of each shipped script, evaluates them under pinned inputs, and asserts the request is 9 lines, that line 7 is the blank separator, and that its sha256 equals botocore's own CanonicalRequest hash for the identical request. Verified: reverting either script to the pre-fix construction fails the new test with lines.count -> 8 and the exact hashes 5a4af0b9 (GET) / 6a643959 (PUT) against the expected 18ec091e / 48c8f7da. With the fix, the full Authorization header produced by the real script's own signing block byte-matches an independent Python SigV4 implementation. swift test: 531 tests in 23 suites pass. shellcheck clean. Co-Authored-By: Claude Opus 5 (1M context) --- .github/scripts/download-r2-object.sh | 32 ++++- .github/scripts/upload-r2-object.sh | 28 +++- Tests/MLXFastTests/R2SignatureTests.swift | 164 +++++++++++++++++++++- 3 files changed, 214 insertions(+), 10 deletions(-) diff --git a/.github/scripts/download-r2-object.sh b/.github/scripts/download-r2-object.sh index c7e4d7fb..4bf11b24 100755 --- a/.github/scripts/download-r2-object.sh +++ b/.github/scripts/download-r2-object.sh @@ -117,8 +117,24 @@ amz_date="$(date -u +%Y%m%dT%H%M%SZ)" date_stamp="${amz_date:0:8}" payload_hash="$(printf '' | shasum -a 256 | awk '{print $1}')" signed_headers="host;x-amz-content-sha256;x-amz-date" -canonical_headers="$(printf 'host:%s\nx-amz-content-sha256:%s\nx-amz-date:%s\n' "${host}" "${payload_hash}" "${amz_date}")" -canonical_request="$(printf 'GET\n%s\n\n%s\n%s\n%s' "${request_path}" "${canonical_headers}" "${signed_headers}" "${payload_hash}")" +# SigV4 CanonicalRequest is +# METHOD \n URI \n QUERY \n CanonicalHeaders \n SignedHeaders \n PayloadHash +# and CanonicalHeaders is itself "name:value\n" per header -- so a BLANK LINE +# separates the last header from SignedHeaders. This used to spell that +# terminating newline inside canonical_headers' own printf, where command +# substitution ATE it: `$(...)` strips every trailing newline, so the canonical +# request went on the wire one line short, hashed differently from what R2 +# computed for the same request, and R2 answered HTTP 403 SignatureDoesNotMatch +# with a well-formed signature. Never reached on the serial box because its +# runner PATH has the aws CLI, which takes download_with_aws_cli() instead; +# M5-C's runner PATH is /usr/bin:/bin:/usr/sbin:/sbin, so it is the first box to +# execute this signer at all (2026-07-30). +# +# Both newlines are therefore spelled in the canonical_request format string +# ("...%s\n\n%s..."), where nothing can strip them: one terminates the last +# header line, one is the blank separator. +canonical_headers="$(printf 'host:%s\nx-amz-content-sha256:%s\nx-amz-date:%s' "${host}" "${payload_hash}" "${amz_date}")" +canonical_request="$(printf 'GET\n%s\n\n%s\n\n%s\n%s' "${request_path}" "${canonical_headers}" "${signed_headers}" "${payload_hash}")" credential_scope="${date_stamp}/${region}/${service}/aws4_request" canonical_request_hash="$(printf '%s' "${canonical_request}" | shasum -a 256 | awk '{print $1}')" string_to_sign="$(printf 'AWS4-HMAC-SHA256\n%s\n%s\n%s' "${amz_date}" "${credential_scope}" "${canonical_request_hash}")" @@ -147,8 +163,16 @@ k_signing="$(hmac_hex "hexkey:${k_service}" "aws4_request")" # OpenSSL ahead of it the signature came out EMPTY and R2 answered # "InvalidArgument: Signature element value should not be blank" -- a 400 that # looks like a credentials problem and is not one. Diagnosed on M5-C -# (LibreSSL 3.3.6) 2026-07-30; the serial box worked only because OpenSSL 3 was -# first on its PATH. +# (LibreSSL 3.3.6) 2026-07-30. +# +# That diagnosis originally added "the serial box worked only because OpenSSL 3 +# was first on its PATH." That is FALSE, and believing it sends the next +# debugger to audit PATH ordering on a box that never runs this code. The +# serial box has the aws CLI on its runner PATH, so download_with_aws_cli() +# below returns 0 and the signer is never reached: every successful hidden- +# golden fetch in either repo announced "using AWS CLI S3 path-style download", +# never "using signed HTTPS download". Whichever openssl it ships is +# irrelevant. Treat this signed path as covered ONLY by the box that lacks aws. # # -binary sidesteps the text format entirely, so there is no field to index. signature="$(hmac_hex "hexkey:${k_signing}" "${string_to_sign}")" diff --git a/.github/scripts/upload-r2-object.sh b/.github/scripts/upload-r2-object.sh index 5f05430b..aec728ad 100755 --- a/.github/scripts/upload-r2-object.sh +++ b/.github/scripts/upload-r2-object.sh @@ -122,8 +122,22 @@ amz_date="$(date -u +%Y%m%dT%H%M%SZ)" date_stamp="${amz_date:0:8}" payload_hash="$(shasum -a 256 "${input_path}" | awk '{print $1}')" signed_headers="host;x-amz-content-sha256;x-amz-date" -canonical_headers="$(printf 'host:%s\nx-amz-content-sha256:%s\nx-amz-date:%s\n' "${host}" "${payload_hash}" "${amz_date}")" -canonical_request="$(printf 'PUT\n%s\n\n%s\n%s\n%s' "${request_path}" "${canonical_headers}" "${signed_headers}" "${payload_hash}")" +# SigV4 CanonicalRequest is +# METHOD \n URI \n QUERY \n CanonicalHeaders \n SignedHeaders \n PayloadHash +# and CanonicalHeaders is itself "name:value\n" per header -- so a BLANK LINE +# separates the last header from SignedHeaders. This used to spell that +# terminating newline inside canonical_headers' own printf, where command +# substitution ATE it (`$(...)` strips every trailing newline), producing a +# canonical request one line short of the one R2 computes and a 403 +# SignatureDoesNotMatch with a well-formed signature. See the twin comment in +# download-r2-object.sh; that script is where the fault was observed, and this +# one had the identical construction. +# +# Both newlines are therefore spelled in the canonical_request format string +# ("...%s\n\n%s..."), where nothing can strip them: one terminates the last +# header line, one is the blank separator. +canonical_headers="$(printf 'host:%s\nx-amz-content-sha256:%s\nx-amz-date:%s' "${host}" "${payload_hash}" "${amz_date}")" +canonical_request="$(printf 'PUT\n%s\n\n%s\n\n%s\n%s' "${request_path}" "${canonical_headers}" "${signed_headers}" "${payload_hash}")" credential_scope="${date_stamp}/${region}/${service}/aws4_request" canonical_request_hash="$(printf '%s' "${canonical_request}" | shasum -a 256 | awk '{print $1}')" string_to_sign="$(printf 'AWS4-HMAC-SHA256\n%s\n%s\n%s' "${amz_date}" "${credential_scope}" "${canonical_request_hash}")" @@ -152,8 +166,14 @@ k_signing="$(hmac_hex "hexkey:${k_service}" "aws4_request")" # OpenSSL ahead of it the signature came out EMPTY and R2 answered # "InvalidArgument: Signature element value should not be blank" -- a 400 that # looks like a credentials problem and is not one. Diagnosed on M5-C -# (LibreSSL 3.3.6) 2026-07-30; the serial box worked only because OpenSSL 3 was -# first on its PATH. +# (LibreSSL 3.3.6) 2026-07-30. +# +# That diagnosis originally added "the serial box worked only because OpenSSL 3 +# was first on its PATH." That is FALSE, and believing it sends the next +# debugger to audit PATH ordering on a box that never runs this code. The +# serial box has the aws CLI on its runner PATH, so upload_with_aws_cli() below +# returns 0 and the signer is never reached. Whichever openssl it ships is +# irrelevant. Treat this signed path as covered ONLY by the box that lacks aws. # # -binary sidesteps the text format entirely, so there is no field to index. signature="$(hmac_hex "hexkey:${k_signing}" "${string_to_sign}")" diff --git a/Tests/MLXFastTests/R2SignatureTests.swift b/Tests/MLXFastTests/R2SignatureTests.swift index 84adf14b..af90b3b7 100644 --- a/Tests/MLXFastTests/R2SignatureTests.swift +++ b/Tests/MLXFastTests/R2SignatureTests.swift @@ -15,8 +15,13 @@ import Testing /// macOS ships LibreSSL as `/usr/bin/openssl`. On a box without Homebrew /// OpenSSL ahead of it the signature came out empty, and R2 answered HTTP 400 /// `InvalidArgument: Signature element value should not be blank` — which reads -/// like a credentials fault and is not one. Measured on M5-C (LibreSSL 3.3.6); -/// the serial box only worked because OpenSSL 3 was first on its PATH. +/// like a credentials fault and is not one. Measured on M5-C (LibreSSL 3.3.6). +/// +/// Not because the serial box had a better openssl — the serial box has the aws +/// CLI on its runner PATH, so `download_with_aws_cli()` short-circuits and the +/// signer never runs there at all. Every successful hidden-golden fetch in +/// either repo announced "using AWS CLI S3 path-style download". These tests +/// are the only coverage this signing path has. /// /// Two guards, because either alone is weak: a structural one (no `openssl /// dgst` may parse the text form) and a behavioural known-answer test (the @@ -151,4 +156,159 @@ struct R2SignatureTests { ) } } + + /// What the two tests above do NOT cover, and the gap that cost a second + /// failed run. + /// + /// `theScriptSigningChainReproducesThePinnedSignature` signs a string-to-sign + /// ending in the literal `deadbeef` — a stand-in for the canonical-request + /// hash. So it proves the HMAC chain is right while saying nothing about the + /// canonical request being hashed, and both scripts built a MALFORMED one: + /// + /// canonical_headers="$(printf 'host:%s\n...\nx-amz-date:%s\n' ...)" + /// canonical_request="$(printf 'GET\n%s\n\n%s\n%s\n%s' ...)" + /// + /// SigV4 is `METHOD \n URI \n QUERY \n CanonicalHeaders \n SignedHeaders \n + /// PayloadHash`, and CanonicalHeaders is `name:value\n` per header — so a + /// BLANK LINE must separate the last header from SignedHeaders. The scripts + /// spelled that terminating newline inside `canonical_headers`' own printf, + /// where `$(...)` ATE it (command substitution strips every trailing + /// newline). The canonical request went out one line short, hashed + /// differently from the one R2 computed for the same bytes, and R2 answered + /// HTTP 403 SignatureDoesNotMatch — with a perfectly well-formed signature, + /// which reads like a bucket-permission fault and is not one. + /// + /// It survived because it was never executed: `download_with_aws_cli()` + /// short-circuits the signer whenever `aws` is on PATH, and the serial + /// ranked box has it. M5-C's runner PATH is `/usr/bin:/bin:/usr/sbin:/sbin`, + /// so it became the first box to run this code at all. The aws CLI fallback + /// must not be what makes R2 work. + /// + /// Reference hashes are botocore's own `CanonicalRequest` for the identical + /// request (aws-cli 2.35.21, `--debug`, path-style endpoint), so this pins + /// against an independent SigV4 implementation rather than against a + /// restatement of the script. + @Test + func theCanonicalRequestMatchesAnIndependentSigV4Implementation() throws { + // (script, HTTP method, request path, payload hash, x-amz-date, + // botocore's canonical-request hash for exactly those inputs) + let cases: + [( + path: String, method: String, requestPath: String, payloadHash: String, + amzDate: String, expected: String + )] = [ + ( + ".github/scripts/download-r2-object.sh", + "GET", + "/mybucket/correctness_prompts/x.json", + "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", + "20260731T000829Z", + "18ec091efbf67625090e1e4b4faa2909228244578b5802a1b8222ad60a7096ad" + ), + ( + ".github/scripts/upload-r2-object.sh", + "PUT", + "/mybucket/up/x.json", + "e38af85860a1452206b018e69c01595e89ce0626bd0068d69ea1b270e993cd41", + "20260731T001036Z", + "48c8f7da8097d5493dcb5140f1f66b1fd9c609d67a70c68637127dc60ab6651a" + ), + ] + + for testCase in cases { + let text = try String(contentsOfFile: testCase.path, encoding: .utf8) + + // Pull the REAL assignments out of the script so this test tracks + // the shipped construction instead of a copy of it. + let assignments = text + .split(separator: "\n", omittingEmptySubsequences: false) + .filter { + $0.hasPrefix("canonical_headers=") || $0.hasPrefix("canonical_request=") + } + .joined(separator: "\n") + #expect( + assignments.contains("canonical_headers=") + && assignments.contains("canonical_request="), + """ + \(testCase.path) no longer assigns canonical_headers/canonical_request \ + at top level; if the canonical request moved, retarget this test rather \ + than deleting it. + """ + ) + + let program = """ + set -euo pipefail + host='acct.r2.cloudflarestorage.com' + request_path='\(testCase.requestPath)' + payload_hash='\(testCase.payloadHash)' + amz_date='\(testCase.amzDate)' + signed_headers='host;x-amz-content-sha256;x-amz-date' + \(assignments) + printf '%s' "${canonical_request}" | shasum -a 256 | awk '{print $1}' + printf '%s' "${canonical_request}" >&2 + """ + + let process = Process() + process.executableURL = URL(fileURLWithPath: "/bin/bash") + process.arguments = ["-c", program] + let out = Pipe() + let err = Pipe() + process.standardOutput = out + process.standardError = err + try process.run() + let hash = String( + decoding: out.fileHandleForReading.readDataToEndOfFile(), + as: UTF8.self + ).trimmingCharacters(in: .whitespacesAndNewlines) + let rendered = String( + decoding: err.fileHandleForReading.readDataToEndOfFile(), + as: UTF8.self + ) + process.waitUntilExit() + + #expect( + process.terminationStatus == 0, + "\(testCase.path) canonical request failed to build: \(rendered)" + ) + + // The blank separator is the byte that was missing, so assert it + // directly too: a hash mismatch alone does not say which line moved. + let lines = rendered.split(separator: "\n", omittingEmptySubsequences: false) + #expect( + lines.count == 9, + """ + \(testCase.path) canonical request has \(lines.count) lines, expected 9 \ + (method, uri, empty query, 3 headers, BLANK separator, signed headers, \ + payload hash). Rendered: + \(rendered) + """ + ) + if lines.count == 9 { + #expect( + lines[2].isEmpty, + "\(testCase.path) line 3 must be the empty canonical query string" + ) + #expect( + lines[6].isEmpty, + """ + \(testCase.path) line 7 must be the BLANK line that terminates \ + CanonicalHeaders and separates it from SignedHeaders. Omitting it \ + yields HTTP 403 SignatureDoesNotMatch with a well-formed signature. \ + Do not spell that newline inside canonical_headers' printf -- \ + command substitution strips trailing newlines. + """ + ) + } + + #expect( + hash == testCase.expected, + """ + \(testCase.path) canonical-request hash \(hash), expected \ + \(testCase.expected) (botocore's own CanonicalRequest for the same \ + \(testCase.method) request). Rendered: + \(rendered) + """ + ) + } + } }