Repository navigation
feat: add exact encrypted backup read receipts - #31
ben-kaufman wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Verdict: ✅ Approve
Findings: none.
QA for 32daa7c:
Full Rust test suite at the head ✅ All 37 tests pass, including the 10 new receipt tests.
cargo test at 32daa7c: 37 passed, 0 failed, 0 ignored. Log: RUN/media/cargo-test-32daa7c.log
Check: Clippy and formatting of the changed files ✅ No new warnings, and the new files are formatted.
cargo clippy --all-targets reports only the two existing module_inception warnings in src/tests.rs and src/ffi_tests.rs. rustfmt --check is clean on src/receipt.rs, src/receipt/tests.rs and src/types.rs.
Check: Missing-key and version handling match vss-server ✅ Real server responses for missing and stored objects are handled correctly.
Checked by reading vss-server at main. A missing key returns HTTP 404 with ErrorCode::NoSuchKeyException (4) (server/src/vss_service.rs), and stored versions start at INITIAL_RECORD_VERSION = 1. So the typed-404 absence rule and the version > 0 check in src/receipt.rs accept what the server sends. Every write path in this repo's history embeds either -1 (store) or expected_version + 1 (put_conditional), and the receipt accepts both.
No app run: this is a source-only API that no released binding or app calls yet, and it has no user-visible behaviour. The PR's QA notes list library checks only.
Review covered the full range eba41ef..32daa7c: protobuf parsing, response cap, deadline, auth and status mapping, AAD and key obfuscation compared with the existing get, version checks, and the singleton capture in vss_get_with_receipt. No case ran against a live VSS server.
Reviewed by claude-opus-5-5-medium
There was a problem hiding this comment.
Verdict: ✅ Approve
Findings: none.
QA for 03035b8:
Full Rust test suite at the head ✅ All 37 tests pass, including the receipt tests.
cargo test at 03035b8: 37 passed, 0 failed, 0 ignored. The receipt tests still call get_with_receipt and crate::vss_get_with_receipt directly, so they don't depend on the UniFFI export that this push removed. Log: RUN/media/cargo-test-03035b8.log
Check: Clippy, formatting and release build after removing the exports ✅ No new warnings.
cargo clippy --all-targets reports only the two existing module_inception warnings in src/tests.rs and src/ffi_tests.rs. rustfmt --check is clean on src/types.rs and src/receipt.rs, and git diff --check is clean. cargo build --release gives 0 warnings, so removing uniffi::Record and #[uniffi::export] leaves no dead-code warning. Logs: RUN/media/clippy-03035b8.log, RUN/media/cargo-build-release-03035b8.log
Check: Released bindings still match the exported API ✅ The generated interfaces match the source again.
Checked by grepping the head and listing the release dylib's symbols. bindings/ and Package.swift are unchanged by the PR, and neither mentions vss_get_with_receipt or VssReadReceipt. At 32daa7c the source exported the function through UniFFI, but the checked-in interfaces didn't include it. At 03035b8 the source and the checked-in interfaces agree. The release dylib exports 18 uniffi_vss_rust_client_ffi_fn_func_* symbols and none for the receipt. Log: RUN/media/exports-03035b8.log
No app run: the API isn't exported to Swift or Kotlin, so no app can call it and nothing changes for users. The PR's QA notes list library checks only.
Note: crate-type is ["cdylib", "staticlib"], so no Rust crate can depend on this crate either. Until a release adds the exports, vss_get_with_receipt can only be reached from this crate's own tests. The README says this.
Review covered the delta 32daa7c..03035b8 (README, src/lib.rs, src/types.rs). Our earlier approval at 32daa7c covered the full range and left nothing open. No case ran against a live VSS server.
Reviewed by claude-opus-5-5-medium
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Adds VssClient::get_with_receipt, a single-attempt bounded getObject read that hashes the exact stored envelope before decoding, now kept as a Rust source API with no UniFFI export. Reviewed 03035b8, full tier, static: talos-pr-run could not confirm the author's permission and failed closed, so nothing was built here; this rests on CI and the code.
What I checked, and 6 candidates I ruled out
Read in full: src/receipt.rs, src/receipt/tests.rs, the diffs to src/lib.rs, src/types.rs, src/implementation.rs, README.md, and 32daa7c..03035b8
Traced: try_get_raw (existing read path), StorableBuilder::deconstruct in vss-client-ng 0.4.0, vss-server's get and the put paths in postgres_store.rs, .github/workflows/ios-bindgen.yml
CI: Tests, Clippy, SwiftPM manifest and the manifest checks green at 03035b8; iOS and Android Bindgen still running when this was posted
Ruled out
- Stale committed iOS interface files: at
32daa7cthe new#[uniffi::export]anduniffi::Recordfailed the "Check committed iOS interface files" step.03035b8drops both, so the generated Swift and header match the committed ones again. - Absence on a missing key: vss-server answers a missing key with HTTP 404 and
NoSuchKeyException, which is the only shapeis_missing_key_responsemaps toNone; other 404 bodies, duplicate or overflowing codes and packed encodings error. version <= 0rejecting real objects: stored versions start atINITIAL_RECORD_VERSION = 1and only grow, so a real row is always positive.- Embedded-version check rejecting existing backups: every writer in this repo's history embeds
-1(store,put_with_key_prefix) orexpected_version + 1(put_conditional, which equals the server's new version); the pre-encryption writers from 2025 stored plaintext, whichdeconstructalready rejects on the existing read path. - Unbounded body: the declared
Content-Lengthis checked before reading, and each chunk is checked against the remaining budget before it is appended, so a chunked response cannot pass the limit. - Credential hang: the outer
tokio::time::timeoutwrapsget_headersas well as the send and body, anddeadline_includes_credentials_and_held_storage_responsecovers both.
Merge confidence: 4/5, the PR was not built here and the bindgen jobs had not finished at 03035b8.
dzdidi
left a comment
There was a problem hiding this comment.
Reviewed exact head 03035b8865f6837d54e632af43c2cde6aedc5a38 against base eba41ef62b7314aa75827a5a3e71d2b23930b981. No blocking correctness, security, persistence, data-loss, API-contract, test, or documentation findings. Exact-head Tests, Clippy, scripts/manifests, SwiftPM, iOS Bindgen, and Android Bindgen checks succeeded. Recommended verdict: approve.
Description
Add a Rust source API,
vss_get_with_receipt, to observe the exact encrypted app-backup object before protobuf decoding. It returns authenticated plaintext, logical/opaque keys, configured store ID, outer server version, and the SHA-256 of the original encrypted bytes, including unknown protobuf fields. This supplies a bounded read primitive for the complete-WALLET archive work in synonymdev/bitkit-ios#1121 and synonymdev/bitkit-android#1686.The storage request runs once without redirects, under a ten-second deadline and caller-selected response cap. Only an explicit typed HTTP 404 missing-key response becomes absence. Empty success, malformed/overflowing or duplicate error codes, unauthorized data, incorrect keys/versions, and invalid encrypted envelopes fail closed. Existing encryption and key derivations are unchanged.
The new operation and record are not exported through UniFFI. The regenerated Swift interfaces match the committed released interfaces byte for byte, and the released binary tag/checksum stays matched. Swift/Kotlin access requires a future coordinated export, binding and binary release. The crate currently emits static/dynamic libraries, not an rlib for use as a Rust dependency. This PR does not activate migration or independent writers, establish principal/freshness authorization, or resolve the terminal complete-WALLET finding. No homeserver change or deployment is included.
Validation
Final local inputs at
03035b8865f6837d54e632af43c2cde6aedc5a38:ldk_client.rsformatting.0.5.27-dev.2validation copy. Generated Swift/header/modulemap bytes exactly match the committed baseline. Device/simulator links and Kotlin compile verify the existingvssGetAPI; no receipt FFI symbol is present.The initial hosted iOS bindgen job failed because the premature new FFI exports produced stale committed interfaces. The additive correction removes only those new exports and documents the Rust-only scope; CI remains unchanged. All six final-head hosted jobs passed on attempt 1: Rust, manifest checks, iOS Bindgen, and Android Bindgen. The synthetic checkout tree exactly matches this published head. All 7,146 log lines, annotations and both uploaded artifacts were audited: 37 tests passed; iOS interface bytes match the base, both slices retain all 93 declared FFI symbols, and all four Android ABIs have matching debug/release build IDs, correct alignment and the same existing FFI exports. GitHub artifact hashes, nested archive CRCs and Kotlin parity pass. The receipt remains absent from FFI. The initial Android job was superseded by this correction and is recorded as cancelled, not a pass.
Preserved earlier failures: the sandbox prevented 16 loopback fixtures from binding, followed by the successful identical-input authorized run above; an auxiliary dev1 host-dylib link failed with
mis-aligned LINKEDIT string pool, cause unproven. Old host static-link deployment warnings and generation notices remain in the evidence. Final native builds used NDK30.0.14904198 with matching NDK environment variables. Python escape, SDK XML and missing optional ktlint notices are retained.QA Notes
Source/library checks are not authenticated app journeys. Existing apps and released bindings do not call this new operation.
Future migration still requires original-owner admission, verified principal mapping, coherent capture, legacy reader/writer exclusion and complete restore acceptance. The receipt is an observation, not a signed server receipt. Never log it or its plaintext. The storage byte limit does not cap the inherited credential provider's separate responses; cancellation does not guarantee prompt preemption or prevent late completion.
Models used
gpt-6-astra(reasoning:xhigh)gpt-6-astra(reasoning:xhigh)gpt-6.1-sol(reasoning:high), two independent contexts; actual execution metadata verified.