fix(cloud-backups): checksum Azure uploads server-side (CRC64), at every payload size - #11
Merged
Merged
Conversation
…ery payload size The S3 upload path opts into server-side integrity verification, so a completed upload means the stored bytes are the bytes we sent. The Azure path asked for nothing, so Azure received NO checksum at all and a "successful" upload proved only that bytes arrived -- while audit-rotate DROPs the source table once an upload verifies, and the default gate is existence + size, which a length-preserving corruption passes. That gap is why the docs told Azure users to run --verify-restore (a full re-download + decrypt + pg_restore -l) on every weekly run: several GB of billed egress and ~10 extra minutes per run to compensate for a missing header. Set TransactionalValidation on the upload so the SDK computes a CRC64 and Azure validates it server-side, rejecting the payload on mismatch. That alone is NOT sufficient, and the difference is easy to miss: azblob's UploadStream short-circuits to a single-shot Put Blob whenever the whole payload fit in block 0 and is smaller than BlockSize (blockblob/chunkwriting.go commitBlocks), and that path silently discards the option, because UploadStreamOptions.getUploadOptions does not copy TransactionalValidation. Every .sha256 sidecar (65 bytes) and every archive under the 10 MiB block size would therefore have uploaded unchecksummed while the docs claimed otherwise -- worse than the status quo, since it would also have removed the --verify-restore recommendation that was the only mitigation for exactly those payloads. So UploadStream now buffers at most one block to see which path applies and sends undersized payloads through blockblob.Upload, which does honour the option. Verified on the wire, not just structurally. A test asserts every payload request carries x-ms-content-crc64 across four sizes (65 B, BlockSize-1, BlockSize, BlockSize+1KiB) using a non-seekable pipe, like the real pg_dump | age source. Without the size split it fails on the two undersized cases (comp="" Put Blob, no checksum header) while an options-struct assertion stays green -- which is precisely why the test checks the wire. Memory is bounded and flat in archive size: ComputeCRC64 is applied per BLOCK, so a 4.5 GB archive costs the same as a 100 MB one. It is not free -- ReadAll's append growth re-buffers each block at roughly 2x, measuring ~+100-200 MiB RSS at BlockSize 10 MiB x Concurrency 3, well inside the cronjob's 1Gi limit but above a naive BlockSize*Concurrency estimate. The comment records the measured figure rather than the naive one. Also corrects the README: uploads are now checksum-verified on both backends at every size, so --verify-restore is about proving RESTORABILITY (a valid, decryptable dump, and that the passphrase in the secret actually works) rather than catching corruption in transit. A self-hosted S3-compatible endpoint still receives the SDK's default CRC32 but enforcement is up to that server, so the recommendation stands there. ReARM-Agent: 1420896f-adf5-4843-896f-d863cfcc6528 ReARM-Agentic-Session: 4477ca5a-8dfc-435b-804c-952d39345110
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.
Problem
The S3 upload path opts into server-side integrity verification, so a completed upload means the stored bytes are the bytes we sent. The Azure path asked for nothing — Azure received no checksum at all, and a "successful" upload proved only that bytes arrived.
That matters here because
audit-rotateDROPs the source table once an upload verifies, and the default gate is existence + size — which a length-preserving corruption passes. It's also why the docs told Azure users to run--verify-restore(full re-download + decrypt +pg_restore -l) on every weekly run: several GB of billed egress and ~10 extra minutes per run, to compensate for a missing header.Change
Set
TransactionalValidationso the SDK computes a CRC64 and Azure validates it server-side, rejecting the payload on mismatch.That alone is not sufficient, and this is the part worth reviewing carefully.
azblob'sUploadStreamshort-circuits to a single-shot Put Blob whenever the whole payload fit in block 0 and is smaller thanBlockSize:…and
UploadStreamOptions.getUploadOptions()(blockblob/models.go) copies Tags/Metadata/Tier/HTTPHeaders/CPK/AccessConditions — notTransactionalValidation. So every.sha256sidecar (65 bytes) and every archive under 10 MiB would have uploaded unchecksummed while the docs claimed otherwise — worse than the status quo, because it would also have removed the--verify-restorerecommendation that was the only mitigation for exactly those payloads.blockblob.Uploaddoes honour the option, soUploadStreamnow buffers at most one block to decide which path applies and routes undersized payloads through it.Evidence
Asserted on the wire, not structurally.
TestAzureUploadStreamChecksumsEverySizedrives the real provider against anhttptestserver through a non-seekable pipe (like the realpg_dump | agesource) and requiresx-ms-content-crc64on every payload request:BlockSize - 1BlockSizeBlockSize + 1 KiBNegative control — with the size split reverted, the test fails on exactly the two undersized cases while the options-struct assertion stays green:
That's why the test checks the wire rather than the options struct.
Memory
ComputeCRC64doesio.ReadAll, which is only safe because it's applied per block — peak memory is flat in archive size (a 4.5 GB archive costs the same as a 100 MB one; every staged request body is exactly 10 MiB regardless of payload). Measured peak RSS, fresh process per case:The delta is ~+100–200 MiB, not the naive
BlockSize * Concurrency= 30 MiB, becauseReadAll's append growth re-buffers each block at roughly 2x. Comfortably inside the cronjob'slimits.memory: 1Gi; the code comment records the measured figure rather than the naive one. Possible follow-up inrearm-saas:requests.memory: 256Miis now marginal alongside thepg_dumpsubprocess in the same cgroup — worth ~384Mi, but that's a different repo and not needed for correctness.Verified clean (SDK source + empirical)
StageBlockaNopCloser(bytes.NewReader(...))over a buffer it had to materialize anyway, andApplyreturns a fresh reader at position 0 of identical length, soContent-Lengthstays correct.400 Md5Mismatchon a staged block surfaces as a hard error, so the archive is never dropped.ClientSecretCredential— orthogonal; the bearer-challenge policy replays the same seekable body with the header attached.Before merge
I could not exercise real Azure from here — the provider builds its endpoint as
https://<account>.blob.core.windows.netwith AAD auth, so Azurite isn't reachable through it, and my kubectl only reaches the rhythm sandbox. Recommend one live smoke upload against a real storage account, with both a >10 MiB and a small object, since if anything about the header were wrong the failure mode is fail-closed but total for the Azure backup path.Informational, no action:
blockblob/client.goStageBlockdoesif err != nil { return StageBlockResponse{}, nil }afterApply— it swallows that error. Only reachable now thatTransactionalValidationis non-nil; in practiceApplyreads an in-memorybytes.Readerand can't fail, and a missing block would be rejected atCommitBlockListanyway.Notes
Independent of #10 (which narrows the S3 IAM surface) and reviewable separately, per request. Both touch adjacent
README.mdlines, so whichever merges second may need a trivialgit merge origin/mainon its branch — happy to do that.CI: branch runs in this repo are red for unrelated reasons (10 jobs for other images fail in the legacy
reliza-docker-actiongetversionstep againstapp.relizahub.com; every branch run here fails that way while main is green).build-cloud-backups— the only job building anything this PR touches — passes.🤖 Generated with Claude Code