Skip to content

feat: add exact encrypted backup read receipts - #31

Open
ben-kaufman wants to merge 2 commits into
masterfrom
feat/backup-object-receipts
Open

ben-kaufman wants to merge 2 commits into
masterfrom
feat/backup-object-receipts

Conversation

@ben-kaufman

@ben-kaufman ben-kaufman commented Nov 4, 2026 •

Copy link
Copy Markdown
Collaborator

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:

  • Full Rust suite: 37 passed, 0 failed/ignored. Release build, documentation, changed-file formatting and diff checks passed. All-target Clippy retains two warnings in unchanged base test modules; whole-repo rustfmt retains unchanged ldk_client.rs formatting.
  • Both iOS slices and all four Android ABIs rebuilt in a separate 0.5.27-dev.2 validation copy. Generated Swift/header/modulemap bytes exactly match the committed baseline. Device/simulator links and Kotlin compile verify the existing vssGet API; no receipt FFI symbol is present.
  • Android lint XML: 0 errors, 3 unchanged dependency-version warnings, 0 introduced findings. ZIP/AAR CRCs and hashes, manifest checksum, all 862 object platform records per iOS slice, Android 16 KiB alignment, and matching debug/release build IDs were inspected. The dev Maven output is isolated; no public SDK publication occurred.
  • Two independent risk-scoped source reviews completed. The packaging correction received an affected integration recheck; unchanged transport, parsing and test coverage retains its prior independent review.

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.

  • 1. Exact encrypted-envelope hashing, including unknown protobuf fields; original-key AAD and valid legacy envelope compatibility.
  • 2. Typed absence versus empty/malformed/auth responses, wire integer overflow and duplicate fields; wrong owner, key, version, metadata and ciphertext rejection.
  • 3. Declared/chunked response bounds, credential/stalled-body deadlines, and original-client completion across singleton replacement.
  • 4. Full Rust gates, regenerated mobile interface compatibility, iOS17 existing-API links, Kotlin compile, structured lint and artifact inspection as above.

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

  • Planning/scoping: gpt-6-astra (reasoning: xhigh)
  • Implementation: gpt-6-astra (reasoning: xhigh)
  • Review: gpt-6.1-sol (reasoning: high), two independent contexts; actual execution metadata verified.

@ben-kaufman
ben-kaufman requested review from dzdidi and ovitrif November 4, 2026 02:22

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 talosmachina left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 32daa7c the new #[uniffi::export] and uniffi::Record failed the "Check committed iOS interface files" step. 03035b8 drops 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 shape is_missing_key_response maps to None; other 404 bodies, duplicate or overflowing codes and packed encodings error.
  • version <= 0 rejecting real objects: stored versions start at INITIAL_RECORD_VERSION = 1 and 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) or expected_version + 1 (put_conditional, which equals the server's new version); the pre-encryption writers from 2025 stored plaintext, which deconstruct already rejects on the existing read path.
  • Unbounded body: the declared Content-Length is 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::timeout wraps get_headers as well as the send and body, and deadline_includes_credentials_and_held_storage_response covers both.

Merge confidence: 4/5, the PR was not built here and the bindgen jobs had not finished at 03035b8.

@ovitrif ovitrif left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utAck

@dzdidi dzdidi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.

4 participants