| 1 |
# SyncKit Client SDK Audit Review |
| 2 |
|
| 3 |
- Last audited: 2026-03-28 (seventh audit, Run 12 cross-project) |
| 4 |
- Previous audit: 2026-03-18 (sixth audit, Run 9 cross-project) |
| 5 |
- Crate: `synckit-client` v0.3.0 |
| 6 |
- Path: `Shared/synckit-client/` |
| 7 |
|
| 8 |
## Overall Grade: A |
| 9 |
|
| 10 |
Run 12: 297 tests (197 unit + 99 integration + 1 doctest). 0 clippy warnings. Grade stable at A. No code changes. New dep advisory: rustls-webpki (RUSTSEC-2026-0049). |
| 11 |
|
| 12 |
## Scorecard |
| 13 |
|
| 14 |
|
| 15 |
|
| 16 |
| Observability | A | |
| 17 |
| Code Quality | A | |
| 18 |
| Architecture | A | |
| 19 |
| Testing | A+ | |
| 20 |
| Security | A- | |
| 21 |
| Performance | A | |
| 22 |
| Documentation | A | |
| 23 |
| Dependencies | A | |
| 24 |
| Type Safety | A | |
| 25 |
| Concurrency | A | |
| 26 |
| Resilience | A | |
| 27 |
| API Consistency | A | |
| 28 |
| Codebase Size | A | |
| 29 |
|
| 30 |
## Module Heatmap |
| 31 |
|
| 32 |
|
| 33 |
|
| 34 |
| client | `src/client.rs` | ~750 | 85 | A | Comprehensive unit tests. Auth state, encrypt/decrypt roundtrip (incl. unicode, empty row_id), type serialization, error classification, token expiry, OAuth URL construction. Blob E2E encryption. Send+Sync compile-time assert. with_http_client constructor for custom timeouts. | |
| 35 |
| crypto | `src/crypto.rs` | ~480 | 19 | A+ | Excellent coverage. Roundtrip, wrong-key, version, truncation, uniqueness, zeroize. Binary blob encrypt/decrypt. Random salt wrapping. | |
| 36 |
| types | `src/types.rs` | ~240 | 13 | A | Serde roundtrip, Display/serde consistency, from_str_opt rejection, Copy/Hash traits, skip_serializing_if, extra field tolerance, ISO timestamp parsing. | |
| 37 |
| keystore | `src/keystore.rs` | 94 | 18 | A | Service name construction, base64 roundtrips, length validation, error handling. Platform behavior documented. | |
| 38 |
| error | `src/error.rs` | ~100 | 10 | A | Send+Sync assert, Display for all variants, Debug no-panic, source() chain for Json/Base64, source() None for leaf variants, edge cases (empty/long messages). | |
| 39 |
| lib | `src/lib.rs` | 53 | 1 (doctest) | A | Clean re-exports. Doctest validates API surface compiles. | |
| 40 |
|
| 41 |
## Cold Spots |
| 42 |
|
| 43 |
1. ~~**client.rs** (639 LOC, 0 tests, grade C)~~ -- Resolved: 81 unit tests added, grade now A-. |
| 44 |
2. ~~**keystore.rs** (0 tests, grade D)~~ -- Resolved: 18 tests added, grade now A-. |
| 45 |
3. ~~**No HTTP timeout**~~ -- Fixed: 30s request timeout + 10s connect timeout on `Client::builder()`. |
| 46 |
4. ~~**await_holding_lock in change_password**~~ -- Fixed: lock guard now dropped before `.await` point. |
| 47 |
5. **No token refresh** -- JWT tokens expire but the SDK has no detection or refresh mechanism. Consumers handle 401s ad hoc. (Token expiry detection was added, but no automatic refresh.) |
| 48 |
6. ~~**op field is raw String**~~ -- Resolved: replaced by ChangeOp enum with serde UPPERCASE. |
| 49 |
|
| 50 |
## Strengths |
| 51 |
|
| 52 |
- **Crypto module is excellent** -- 12 unit tests covering all key operations. XChaCha20-Poly1305 with 192-bit random nonces eliminates nonce collision risk. Argon2id with OWASP-minimum parameters. |
| 53 |
- **Minimal API surface** -- 8 types re-exported from lib.rs. Wire-only types are `pub(crate)`. Internal helpers are private. |
| 54 |
- **Clean key hierarchy** -- Three layers (password -> wrapping key -> master key -> per-entry encryption) are well-documented and correctly implemented. |
| 55 |
- **No consumer-specific logic** -- Fully generic. No references to GO, BB, or AF. Table names and data shapes are opaque to the SDK. |
| 56 |
- **ZeroizeOnDrop** -- In-memory keys are zeroed on drop via volatile writes. |
| 57 |
|
| 58 |
## Weaknesses |
| 59 |
|
| 60 |
- ~~**CRITICAL: Blob data NOT encrypted**~~ -- RESOLVED: encrypt_bytes/decrypt_bytes added to crypto.rs, blob_upload/blob_download now encrypt/decrypt transparently (40 bytes overhead per blob). |
| 61 |
- ~~**Deterministic Argon2 salt**~~ -- RESOLVED: wrap_master_key now generates random 32-byte salt, unwrap_master_key reads salt from envelope and derives wrapping key internally. |
| 62 |
- **No key rotation mechanism** -- No way to rotate the master key without re-encrypting all data. |
| 63 |
- ~~**13 Mutex `.unwrap()` calls**~~ -- RESOLVED: All replaced with `.lock().map_err()` returning SyncKitError::Internal. |
| 64 |
- ~~**Master key copies not zeroized**~~ -- RESOLVED: All call sites wrap in ZeroizeOnDrop. |
| 65 |
- ~~**Several public types that should be `pub(crate)`**~~ -- RESOLVED: KeyEnvelope, ZeroizeOnDrop inner field, 5 request/response types restricted. |
| 66 |
|
| 67 |
### Resolved from first audit |
| 68 |
- ~~**client.rs untested**~~ -- 66 unit tests added (S4) |
| 69 |
- ~~**No resilience**~~ -- HTTP timeouts (30s + 10s connect), retry with backoff (3 retries), token expiry detection all added (S4) |
| 70 |
- ~~**change_password concurrency bug**~~ -- Lock guard dropped before await (S4) |
| 71 |
- ~~**op field raw String**~~ -- ChangeOp enum with serde UPPERCASE (S4) |
| 72 |
|
| 73 |
## Mandatory Surprise |
| 74 |
|
| 75 |
**change_password race + deterministic salt -- Both resolved.** |
| 76 |
|
| 77 |
~~The `change_password` method holds a `std::sync::Mutex` guard across an `.await` point (`get_server_key`), which Clippy flags as `await_holding_lock`.~~ Fixed: lock guard now dropped before `.await` point. |
| 78 |
|
| 79 |
More critically, if `put_server_key` fails after the old envelope is fetched, the method may log success while the server retains the old envelope. Concurrent `change_password` calls from two devices race silently -- one wins, the other's key cache becomes stale with no notification or error. (Race condition remains a theoretical concern but is low-priority given single-device typical usage.) |
| 80 |
|
| 81 |
~~The Argon2 salt is deterministic from `(app_id, user_id)` via `SHA256(app_id || user_id)`. This means the salt never rotates on password change.~~ Fixed: wrap_master_key now generates a random 32-byte salt per operation, stored in the KeyEnvelope. unwrap_master_key reads the salt from the envelope and derives the wrapping key internally. |
| 82 |
|
| 83 |
## Metrics Over Time |
| 84 |
|
| 85 |
|
| 86 |
|
| 87 |
| 2026-03-11 | 1,416 | 6 | 13 | 9.2 | |
| 88 |
| 2026-03-13 | ~1.4K | 6 | 109 | ~77 | |
| 89 |
| 2026-03-13 (post-fix) | ~1.5K | 6 | 118 | ~79 | |
| 90 |
| 2026-03-13 (adversarial) | ~2.5K | 6 | 234 | ~94 | |
| 91 |
| 2026-03-13 (perf+resilience) | ~2.6K | 6 | 243 | ~94 | |
| 92 |
| 2026-03-13 (testing push) | ~2.8K | 6 | 297 | ~106 | |
| 93 |
| 2026-03-16 (Run 6) | 4,327 | 6 | 297 | ~69 | |
| 94 |
| 2026-03-18 (Run 9) | 4,327 | 6 | 298 | ~69 | |
| 95 |
| 2026-03-28 (Run 12) | 4,327 | 6 | 297 | ~69 | |
| 96 |
|
| 97 |
--- |
| 98 |
|
| 99 |
See [audit_history.md](./audit_history.md) for full chronological audit log. |
| 100 |
|