Skip to main content

max / makenotwork

synckit-client: bound server-supplied multipart geometry + fix red test suite Deepaudit 07-21 SERIOUS: stream_blob_parts allocated from server-supplied part_count/part_size with only a zero-check, so a hostile part_count aborts the client on an over-large reservation and a lying part_count/part_size defeats the one-part-in-memory bound. Cap both against sane ceilings (1 GiB part, 10k parts) and require the plan to match the known ciphertext geometry (the server derives it from the same size_bytes). Regression test added. Also fixes the pre-existing 38-test-red suite: reqwest is built rustls-no-provider, so the tests (which have no consumer app to install a provider) now install ring themselves -- unit tests via a cfg(test) Once in new(), integration tests via a helper. Adds rustls as a dev-dependency. Minor doc/hygiene from the same audit: zeroize the plaintext scratch buffer; correct the module header (content address is the plaintext, not ciphertext, hash); fix architecture.md drift (keyring v4 not v3; part URLs are pulled one per part, not 100 at a time). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-07-24 20:13 UTC
Commit: 05f79c078840583aa07e0906870813873760413f
Parent: fc59ca8
6 files changed, +118 insertions, -7 deletions
M shared/synckit-client/Cargo.lock +2
@@ -1826,6 +1826,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
1826 1826 checksum = "758025cb5fccfd3bc2fd74708fd4682be41d99e5dff73c377c0646c6012c73a4"
1827 1827 dependencies = [
1828 1828 "once_cell",
1829 + "ring",
1829 1830 "rustls-pki-types",
1830 1831 "rustls-webpki",
1831 1832 "subtle",
@@ -2187,6 +2188,7 @@ dependencies = [
2187 2188 "rand",
2188 2189 "reqwest",
2189 2190 "rusqlite",
2191 + "rustls",
2190 2192 "serde",
2191 2193 "serde_json",
2192 2194 "sha2 0.11.0",
M shared/synckit-client/Cargo.toml +4
@@ -73,6 +73,10 @@ rusqlite = { version = "0.39", features = ["bundled", "functions"], optional = t
73 73
74 74 [dev-dependencies]
75 75 wiremock = "0.6"
76 + # reqwest is built `rustls-no-provider`, so real consumers install a rustls crypto
77 + # provider at startup (audiofiles installs ring). The test suite has no such app, so
78 + # it installs ring itself before building any client. Dev-only: never in a consumer build.
79 + rustls = { version = "0.23", default-features = false, features = ["ring"] }
76 80 # Self dev-dependency: turns on the `testing` feature for this crate's own
77 81 # unit/integration test builds without leaking it into a consumer's default set.
78 82 # `default-features = false` so it only ADDS `testing` to whatever the test run
M shared/synckit-client/docs/architecture.md +4 -3
@@ -269,8 +269,9 @@ path)` replaces both with a multipart session and holds one part at a time:
269 269 returns the part geometry both sides then derive identically.
270 270 2. It seals 1 MiB v3 chunks straight out of the file, staging them until a full
271 271 part is ready, and PUTs each part to a presigned URL signed for that exact
272 - `Content-Length`. Presigned part URLs are pulled 100 at a time as the upload
273 - progresses.
272 + `Content-Length`. Each presigned part URL is requested on its own, one per
273 + part, since a part's checksum only exists once it has been sealed and only one
274 + part is ever held in memory.
274 275 3. It recomputes the SHA-256 as it reads, so a file that changed since the
275 276 caller hashed it fails rather than landing under a stale content address.
276 277 4. Any failure past `start` aborts the session, since uploaded parts bill until
@@ -282,7 +283,7 @@ is not persisted.
282 283
283 284 ## Keychain integration
284 285
285 - The `keychain` feature (default on) uses the `keyring` crate (v3) to store
286 + The `keychain` feature (default on) uses the `keyring` crate (v4) to store
286 287 the master key in the OS credential store:
287 288
288 289 - **macOS**: Keychain Services
M shared/synckit-client/src/client/blob.rs +38 -4
@@ -1,9 +1,10 @@
1 1 //! Content-addressed blob upload and download.
2 2 //!
3 3 //! Blobs are encrypted client-side and stored under the SHA-256 of their
4 - //! ciphertext. Uploads go through a presigned URL after a size/quota check;
5 - //! downloads are re-hashed on arrival and discarded on any mismatch, so a
6 - //! server that swaps or rolls back a blob cannot slip it past the client.
4 + //! plaintext. Uploads go through a presigned URL after a size/quota check;
5 + //! downloads are decrypted and re-hashed on arrival and discarded on any
6 + //! mismatch, so a server that swaps or rolls back a blob cannot slip it past
7 + //! the client.
7 8
8 9 use std::path::Path;
9 10
@@ -282,13 +283,46 @@ impl SyncKitClient {
282 283 ));
283 284 }
284 285
286 + // The part geometry is server-supplied, so bound it before it drives any
287 + // allocation. Without this a hostile part_count aborts the client on an
288 + // over-large `Vec`/`BytesMut` reservation, and a part_count/part_size that
289 + // does not match the blob defeats the one-part-plus-one-chunk memory bound.
290 + // The plan must describe exactly the ciphertext we are about to upload
291 + // (the server derives it from this same `size_bytes`), so recompute and
292 + // compare rather than trust it.
293 + const MAX_PART_SIZE: usize = 1 << 30; // 1 GiB; real plan parts are well under this
294 + const MAX_PART_COUNT: u32 = 10_000; // S3's hard multipart-part ceiling
295 + if part_size > MAX_PART_SIZE {
296 + return Err(SyncKitError::Internal(format!(
297 + "server part_size {part_size} exceeds the {MAX_PART_SIZE}-byte ceiling"
298 + )));
299 + }
300 + if start.part_count > MAX_PART_COUNT {
301 + return Err(SyncKitError::Internal(format!(
302 + "server part_count {} exceeds the {MAX_PART_COUNT}-part ceiling",
303 + start.part_count
304 + )));
305 + }
306 + let total = usize::try_from(size_bytes).map_err(|_| {
307 + SyncKitError::Internal(format!("blob size {size_bytes} is not a valid length"))
308 + })?;
309 + let expected_parts = total.div_ceil(part_size);
310 + if start.part_count as usize != expected_parts {
311 + return Err(SyncKitError::Internal(format!(
312 + "server multipart plan (part_count {}, part_size {part_size}) does not match the \
313 + {total}-byte blob (expected {expected_parts} parts)",
314 + start.part_count
315 + )));
316 + }
317 +
285 318 let mut file = tokio::fs::File::open(path).await.map_err(|e| {
286 319 SyncKitError::Internal(format!("open blob file {}: {e}", path.display()))
287 320 })?;
288 321
289 322 let chunk_count = crypto::blob_chunk_count_for(plaintext_len);
290 323 let mut hasher = Sha256::new();
291 - let mut plain = vec![0u8; crypto::BLOB_CHUNK_SIZE];
324 + // Plaintext scratch: zeroized on drop so a decrypted chunk does not linger.
325 + let mut plain = zeroize::Zeroizing::new(vec![0u8; crypto::BLOB_CHUNK_SIZE]);
292 326 let mut staged = BytesMut::with_capacity(part_size + crypto::BLOB_CHUNK_SIZE);
293 327 staged.extend_from_slice(&crypto::blob_header_bytes(plaintext_len));
294 328
M shared/synckit-client/src/client/mod.rs +12
@@ -327,6 +327,18 @@ pub struct SyncKitClient {
327 327 impl SyncKitClient {
328 328 /// Create a new client with the given configuration.
329 329 pub fn new(config: SyncKitConfig) -> Self {
330 + // reqwest is built with `rustls-no-provider`; a real consumer app installs the
331 + // process-wide crypto provider (audiofiles installs ring) before it ever builds
332 + // a client. Unit tests have no such app, so install ring here once, or the
333 + // client build below would panic. Never compiled into a consumer build.
334 + #[cfg(test)]
335 + {
336 + static PROVIDER: std::sync::Once = std::sync::Once::new();
337 + PROVIDER.call_once(|| {
338 + let _ = rustls::crypto::ring::default_provider().install_default();
339 + });
340 + }
341 +
330 342 let https_only = requires_https(&config.server_url);
331 343 let http = Client::builder()
332 344 .timeout(Duration::from_secs(30))
M shared/synckit-client/tests/integration.rs +58
@@ -46,7 +46,18 @@ fn test_ids() -> (UserId, AppId) {
46 46 )
47 47 }
48 48
49 + /// Install the rustls crypto provider once. reqwest is built `rustls-no-provider`,
50 + /// so a real consumer app installs one at startup (audiofiles installs ring); these
51 + /// tests have no such app, so they install ring themselves before building a client.
52 + fn ensure_crypto_provider() {
53 + static PROVIDER: std::sync::Once = std::sync::Once::new();
54 + PROVIDER.call_once(|| {
55 + let _ = rustls::crypto::ring::default_provider().install_default();
56 + });
57 + }
58 +
49 59 fn client_for(server: &MockServer) -> SyncKitClient {
60 + ensure_crypto_provider();
50 61 SyncKitClient::new(SyncKitConfig {
51 62 server_url: server.uri(),
52 63 api_key: "test-api-key".to_string(),
@@ -2849,6 +2860,7 @@ async fn concurrent_push_100_entries_each() {
2849 2860 // ── Timeout tests ──
2850 2861
2851 2862 fn short_timeout_client(server: &MockServer) -> SyncKitClient {
2863 + ensure_crypto_provider();
2852 2864 let http = reqwest::Client::builder()
2853 2865 .timeout(Duration::from_millis(100))
2854 2866 .connect_timeout(Duration::from_millis(100))
@@ -3430,6 +3442,52 @@ mod blob_multipart {
3430 3442 }
3431 3443
3432 3444 #[tokio::test]
3445 + async fn streaming_upload_rejects_a_server_part_plan_that_lies_about_geometry() {
3446 + let server = MockServer::start().await;
3447 + let client = authed_client(&server);
3448 + let key = synckit_client::crypto::generate_master_key();
3449 + client.set_master_key_raw(key);
3450 +
3451 + // A blob that genuinely spans several 1 MiB parts.
3452 + let plaintext: Vec<u8> = (0..(synckit_client::crypto::BLOB_CHUNK_SIZE * 3 + 7))
3453 + .map(|i| i as u8)
3454 + .collect();
3455 + let hash = hex::encode(sha2::Sha256::digest(&plaintext));
3456 + let file = temp_blob("liar.bin", &plaintext);
3457 +
3458 + // Hostile server: claims the whole multi-part blob fits in ONE part.
3459 + // Trusting it would defeat the one-part-in-memory bound, so the client
3460 + // must refuse before minting or PUTting anything.
3461 + Mock::given(method("POST"))
3462 + .and(path(START_PATH))
3463 + .respond_with(ResponseTemplate::new(200).set_body_json(json!({
3464 + "upload_id": "test-upload-id",
3465 + "part_size": 1024 * 1024,
3466 + "part_count": 1,
3467 + "already_exists": false,
3468 + })))
3469 + .mount(&server)
3470 + .await;
3471 +
3472 + let err = client
3473 + .blob_upload_streaming(&hash, &file)
3474 + .await
3475 + .unwrap_err();
3476 + assert!(
3477 + matches!(err, SyncKitError::Internal(ref m) if m.contains("does not match")),
3478 + "expected a geometry-mismatch rejection, got {err:?}"
3479 + );
3480 + let reqs = server.received_requests().await.unwrap();
3481 + assert_eq!(
3482 + hits(&reqs, PART_PUT_PATH),
3483 + 0,
3484 + "no part may be uploaded once the plan is rejected"
3485 + );
3486 +
3487 + std::fs::remove_file(&file).ok();
3488 + }
3489 +
3490 + #[tokio::test]
3433 3491 async fn streaming_upload_handles_an_empty_file() {
3434 3492 let server = MockServer::start().await;
3435 3493 let client = authed_client(&server);