|
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 |
+ |
| Dimension | Grade |
|
|
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 |
+ |
| Module | File | LOC | Tests | Grade | Notes |
|
|
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 |
+ |
| Date | LOC | Files | Tests | Tests/KLOC |
|
|
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 |
+ |
## Changes Since Last Audit
|
|
98 |
+ |
|
|
99 |
+ |
### Seventh audit (2026-03-28, Run 12 cross-project)
|
|
100 |
+ |
- **Test count:** 297 (197 unit + 99 integration + 1 doctest). 0 clippy warnings. 0 failures.
|
|
101 |
+ |
- **Grade:** A (maintained). v0.3.0.
|
|
102 |
+ |
- **No code changes since Run 9.**
|
|
103 |
+ |
- **New dependency advisory:** rustls-webpki 0.103.9 (RUSTSEC-2026-0049) — upgrade to 0.103.10 via `cargo update -p rustls-webpki`.
|
|
104 |
+ |
- **Mandatory surprise:** None new. Previous surprise (fresh random Argon2 salt per wrap) still valid and impressive.
|
|
105 |
+ |
- **No new findings.** All previous items remain resolved.
|
|
106 |
+ |
|
|
107 |
+ |
### Rust Patterns Audit (2026-03-21)
|
|
108 |
+ |
- `SessionInfo.token` changed from `String` to `Arc<String>` -- `Arc::clone` instead of String clone
|
|
109 |
+ |
- Auth request structs already use `&'a str` -- confirmed optimal, no change needed
|
|
110 |
+ |
|
|
111 |
+ |
### Sixth audit (2026-03-18, Run 9 cross-project)
|
|
112 |
+ |
- **Test count:** 298 (197 unit + 99 integration + 1 doctest). 0 clippy warnings.
|
|
113 |
+ |
- **Grade:** A (maintained). v0.3.0.
|
|
114 |
+ |
- **No new findings.** All previous items remain resolved.
|
|
115 |
+ |
- **Crypto audit notes:** XChaCha20-Poly1305, Argon2id with OWASP minimums, ZeroizeOnDrop keys, NFC normalization. 100+ crypto-specific tests.
|
|
116 |
+ |
- **No sensitive data in logs:** Confirmed — tracing calls log events (e.g., "Master key generated") without leaking key material or passwords.
|
|
117 |
+ |
- **Mandatory surprise:** Argon2 salt uniqueness — every wrap generates fresh random salt (crypto.rs:130-131). Verified by `two_wraps_use_different_salts` test. Correct design, uncommon rigor.
|
|
118 |
+ |
|
|
119 |
+ |
### Concurrency Upgrade (2026-03-13)
|
|
120 |
+ |
- **Concurrency:** B+ -> A-
|
|
121 |
+ |
- Replaced std::sync::RwLock with parking_lot::RwLock. Removed 16 poison-handling .map_err() sites. All 234 tests pass.
|
|
122 |
+ |
|
|
123 |
+ |
### Second audit (2026-03-13, pre-launch skeptical lens)
|
|
124 |
+ |
- **Grade:** B+ (maintained). S4 fixes resolved 4/6 first-audit issues. New critical finding: blob data not encrypted.
|
|
125 |
+ |
- **Test count:** 13 -> 109 (+96 tests, mostly from S4 remediation)
|
|
126 |
+ |
- **S4 fixes:** await_holding_lock, HTTP timeouts, retry with backoff, token expiry detection, client.rs tests (66), keystore.rs tests (18), ChangeOp enum
|
|
127 |
+ |
- **New findings:** Blob encryption gap (CRITICAL), no key rotation, Mutex unwraps, master key copies not zeroized, public types that should be pub(crate)
|
|
128 |
+ |
- **Deterministic Argon2 salt:** Persists from first audit (tracked in MNW todo as "consider random salt")
|
|
129 |
+ |
|
|
130 |
+ |
### Post-audit remediation (2026-03-13)
|
|
131 |
+ |
- **Grade:** B+ -> A-. 5 of 6 new findings from second audit resolved. Only key rotation deferred.
|
|
132 |
+ |
- **Test count:** 109 -> 118 (+9 tests: 7 blob encrypt/decrypt, 2 salt tests)
|
|
133 |
+ |
- **Blob encryption:** encrypt_bytes/decrypt_bytes in crypto.rs. blob_upload/blob_download encrypt/decrypt transparently using master key. 40-byte overhead (24 nonce + 16 tag).
|
|
134 |
+ |
- **Random Argon2 salt:** wrap_master_key generates random 32-byte salt per operation. unwrap_master_key reads salt from envelope. Eliminates deterministic salt precomputation risk.
|
|
135 |
+ |
- **Previous S4 fixes verified:** Mutex unwraps, ZeroizeOnDrop, pub(crate) restrictions -- all still in place.
|
|
136 |
+ |
- **Key rotation:** Deferred post-beta. Requires server-side re-encryption of all sync_log entries.
|
|
137 |
+ |
- Documentation upgraded to A: Device/SyncStatus/ChangeEntry/BlobUploadUrlResponse field docs added. All 12 error variants documented with when-they-occur. Keystore platform behavior documented (macOS/Linux/Windows backends). Client helpers documented (require_token, require_session_ids, etc). SessionInfo field docs. client.rs module doc expanded to 50 lines. architecture.md created (217 lines), README created (78 lines).
|
|
138 |
+ |
|
|
139 |
+ |
### Observability Upgrade (2026-03-13)
|
|
140 |
+ |
- Added Observability dimension to scorecard (grade A)
|
|
141 |
+ |
- Added 16 `#[instrument]` annotations to all pub async methods in client.rs with appropriate skip params
|
|
142 |
+ |
- Sensitive params skipped: password, old_password, new_password, email, code, code_verifier, presigned_url, data
|
|
143 |
+ |
- `use tracing::instrument;` import added to client.rs
|
|
144 |
+ |
- `cargo check` passes clean
|
|
145 |
+ |
|
|
146 |
+ |
### Performance Upgrade (2026-03-13)
|
|
147 |
+ |
- **Performance:** B -> A-
|
|
148 |
+ |
- Cached JWT `exp` claim in Session struct — `require_token()` and `is_token_expired()` no longer re-parse the JWT on every call
|
|
149 |
+ |
- Retry request bodies use `bytes::Bytes` instead of `Vec<u8>` — clone in retry closures is O(1) refcount bump, not O(n) copy (10 sites)
|
|
150 |
+ |
- Batch encrypt/decrypt in `push()`/`pull()` extracts master key once before the loop instead of per-entry lock acquisition
|
|
151 |
+ |
|
|
152 |
+ |
### Resilience Upgrade (2026-03-13)
|
|
153 |
+ |
- **Resilience:** B- -> A-
|
|
154 |
+ |
- Added 9 integration tests for encryption setup flows: `setup_encryption_new` (happy path, no-auth, server retry), `setup_encryption_existing` (happy path, wrong password, no-auth, server retry, missing key 404), cross-device encryption roundtrip
|
|
155 |
+ |
- Cross-device test proves full two-device flow: device 1 creates encryption → pushes data → device 2 recovers key → pulls and decrypts successfully
|
|
156 |
+ |
- **Test count:** 234 -> 243 (+9 integration tests). 170 unit + 72 integration + 1 doctest.
|
|
157 |
+ |
|
|
158 |
+ |
### Adversarial Test Audit (2026-03-13)
|
|
159 |
+ |
- **Grade:** A- -> A-. Testing grade upgraded from B- to A.
|
|
160 |
+ |
- **Test count:** 150 -> 234 (+84 tests: 52 unit, 32 integration). Test density ~94 tests/KLOC.
|
|
161 |
+ |
- **CRITICAL fix: change_password bypass** -- Old password verification skipped when master key was cached in memory. Attacker with session access (stolen device, malware) could change encryption password without knowing the old one. Fixed: always verify old password against server envelope regardless of cache state. Added 8 tests covering cache hit/miss, wrong old password, concurrent password changes.
|
|
162 |
+ |
- **HIGH fix: Unicode password normalization** -- NFC vs NFD normalization inconsistency across operating systems could derive different keys from "same" password (e.g., é as single codepoint vs e+combining-acute). Added `unicode-normalization` crate, NFC normalization before all key derivation (wrap_master_key, unwrap_master_key, change_password). 4 tests covering NFC/NFD/mixed inputs.
|
|
163 |
+ |
- **Empty password rejection** -- wrap_master_key, unwrap_master_key, change_password now return error on empty password. 3 tests.
|
|
164 |
+ |
- **Password length limits** -- 1024-byte max after UTF-8 encoding. Prevents resource exhaustion on Argon2 (linear memory cost with input length). 2 tests.
|
|
165 |
+ |
- **Comprehensive crypto tests** -- Tamper detection (flip bits in nonce/ciphertext/tag), envelope validation (version mismatch, truncated fields), key rotation simulation (decrypt with wrong key), concurrent encryption (nonce uniqueness under load), large payload handling (1MB encrypt/decrypt). 28 new crypto unit tests.
|
|
166 |
+ |
- **Integration tests** -- Error mapping for all 4xx/5xx codes (400/401/403/404/409/413/429/500/502/503), retry behavior (transient vs permanent), auth enforcement (missing token, invalid token, expired token), blob roundtrips (upload -> download, tamper detection, decrypt failure), malformed response handling (invalid JSON, missing fields). 32 new integration tests.
|
|
167 |
+ |
- **Concurrency tests** -- Parallel encrypt operations (nonce uniqueness), concurrent password changes (last-write-wins, cache invalidation), device registration race (409 conflict), push/pull interleaving (optimistic locking). 9 tests across unit and integration.
|
|
168 |
+ |
- **Resolved findings:** All 2 critical vulnerabilities from adversarial audit fixed. No new security issues discovered.
|
|
169 |
+ |
|
|
170 |
+ |
### Third audit (2026-03-16, Run 6 cross-project)
|
|
171 |
+ |
- **Test count:** 297 (unchanged)
|
|
172 |
+ |
- **Grade:** A (maintained).
|
|
173 |
+ |
- **Source LOC:** 4,327 src + 2,749 test
|
|
174 |
+ |
- **New finding (MEDIUM):** Wrapping key in `crypto.rs:99` (`derive_wrapping_key`) is computed on the stack but not wrapped in `ZeroizeOnDrop`. Intermediate key material sits in memory after function returns. Other keys properly use ZeroizeOnDrop.
|
|
175 |
+ |
- **New finding (LOW):** Unused `sha2` dependency in Cargo.toml.
|
|
176 |
+ |
- **Mandatory surprise:** Wrapping key not zeroized — Genuine issue (MEDIUM).
|
|
177 |
+ |
- **Previous items verified:** All previous remediated items confirmed intact. Key rotation still deferred (post-beta).
|
|
178 |
+ |
|
|
179 |
+ |
### Testing Push (2026-03-13)
|
|
180 |
+ |
- **Grade:** A- -> A. Testing A -> A+. Code Quality, Type Safety, Concurrency, Resilience all upgraded to A.
|
|
181 |
+ |
- **Test count:** 243 -> 297 (+54 tests). 197 unit + 99 integration + 1 doctest.
|
|
182 |
+ |
- **types.rs:** 13 unit tests added. Serde roundtrip, Display/serde consistency, from_str_opt edge cases, Copy/Hash trait verification, skip_serializing_if, extra unknown fields tolerance, ISO timestamp deserialization.
|
|
183 |
+ |
- **error.rs:** 10 unit tests added. Send+Sync compile-time assert, Display for all 8 variants, Debug no-panic, source() chain verification (Json, Base64 have source; leaf variants do not), empty/very-long server messages.
|
|
184 |
+ |
- **client.rs:** 4 unit tests added. SyncKitClient Send+Sync compile-time assert, with_http_client constructor, unicode table name encrypt/decrypt roundtrip, empty row_id roundtrip.
|
|
185 |
+ |
- **Integration tests:** 27 new. Retry count verification (exhaustion at 4 requests, 404 not retried, 3rd-attempt success). Malformed responses (HTML body, empty body, missing has_more, wrong cursor type, missing app_id, missing already_exists, 413 error, extra fields ignored). Session edge cases (double authenticate, clear then re-auth, expired token on restore). Encryption setup overwrite. Blob edge cases (confirm retry, download retry, 1MB upload overhead). Device edge cases (empty name, unicode name, empty list). Concurrency stress (50 concurrent session_info reads, 50 has_master_key reads, 20 status checks, 4x100-entry pushes). Timeout tests (slow server timeout, retry after timeout).
|
|
186 |
+ |
- **New constructor:** `with_http_client(config, client)` enables custom timeout testing without modifying production defaults.
|
|
187 |
+ |
|
|
188 |
+ |
### Performance Upgrade (2026-03-13)
|
|
189 |
+ |
- **Performance:** A- -> A
|
|
190 |
+ |
- Pre-built endpoint URLs: new `Endpoints` struct computes all 10 API endpoint URLs once at client construction, eliminating per-request `format!()` string allocations
|
|
191 |
+ |
- `Arc<String>` session token: `require_token()` returns `Arc<String>` instead of `String`, making per-request token extraction O(1) refcount bump instead of O(n) string clone (~300-500 byte JWT)
|
|
192 |
+ |
- `key_url_and_token()` returns `(&str, Arc<String>)` instead of `(String, String)`, zero allocations per call
|
|
193 |
+ |
- All 297 tests pass unchanged (2 test assertions updated for Arc deref)
|