Add native HPKE encryption for nsec backups - #7849
Conversation
Signed-off-by: jm <jm@squareup.com> Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: jm <jm@squareup.com>
Signed-off-by: jm <jm@squareup.com> Co-authored-by: Codex <noreply@openai.com>
🔐 Codex Security Review
|
Signed-off-by: Jordan Mecom <jm@squareup.com>
Signed-off-by: Jordan Mecom <jm@squareup.com>
Signed-off-by: Jordan Mecom <jm@squareup.com>
|
@buzz-security-review 95f3f5c |
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 thanks for putting this together. The crypto looks right to me: fixed suite, a fresh context per seal, length-framed and domain-separated AAD, canonical decoding, and keeping it off the Tauri command surface. I also ran the sealing module against an independent Python/OpenSSL recipient. It passed the RFC 9180 vector first, then about 440 cases covering edge scalars, malformed and off-curve recipient keys, noncanonical envelopes, and tampering, and everything behaved. A few things inline. The main one is the plaintext copy inside the rustls seal path.
| ciphertext: String::new(), | ||
| }; | ||
| let aad = envelope.associated_data()?; | ||
| let plaintext = Zeroizing::new(secret_key.to_secret_bytes()); |
There was a problem hiding this comment.
Zeroizing here only covers our copy. rustls' aws-lc-rs Sealer::seal does Vec::from(plaintext), which allocates exactly 32 bytes. aws-lc-rs seal_in_place_append_tag then extends that buffer by the 16-byte tag before encrypting. So the realloc can move the buffer and free the original 32 bytes with the raw secret still in them, and the same thing happens on the error path. Nothing on our side can wipe that copy.
For the hidden copy, I see two options. One is to fix it upstream by pre-sizing that buffer to plaintext.len() + tag_len in the rustls sealer. The other is to use an HPKE implementation that seals in place into a caller-owned buffer. Either way, I'd pass secret_key.as_secret_bytes() straight through here. SecretKey already erases its own storage on drop, so the extra to_secret_bytes() + Zeroizing copy isn't buying anything. I'd also reword the doc comment above: "copies only its raw 32-byte secret representation into a zeroizing plaintext buffer" reads like the secret never lands anywhere unwiped.
| .is_err()); | ||
|
|
||
| let left = | ||
| HpkeBackupEnrollment::new("a|b", "c", "owner", test_backup_id(), &public_key.0).unwrap(); |
There was a problem hiding this comment.
This pair doesn't actually depend on the framing. a|bc vs ab|c differ even with bare concatenation, so the test still passes if push_framed becomes extend_from_slice. The pair that collides without length prefixes is ("ab", "c") vs ("a", "bc"). The fixture's aad_hex does pin the framing, so it's covered, just not by the test named for it.
| let encrypted_nsec_marker = ["ncrypt", "sec1"].concat(); | ||
| assert!(!json.contains(&encrypted_nsec_marker)); |
There was a problem hiding this comment.
The ["ncrypt", "sec1"].concat() looks like it's there to get past the NIP-49 allowlist scan in egress_guard_tests.rs. This module never touches the NIP-49 codec, so I'd just drop these two lines instead of routing around the guard.
| } | ||
| let decoded = URL_SAFE_NO_PAD | ||
| .decode(value) | ||
| .map_err(|_| HpkeBackupError::InvalidEnvelope("invalid base64url payload"))?; |
There was a problem hiding this comment.
nit: every other failure in decode_canonical_base64 returns InvalidField { field, .. }, but malformed base64 goes through InvalidEnvelope and drops the field name. InvalidField { field, reason: "invalid base64url" } would be consistent. Relatedly, the InvalidField doc says "trusted-enrollment field", but the variant is also used for enc and ciphertext.
| mod egress_guard; | ||
| mod event_sync; | ||
| mod events; | ||
| pub mod hpke_key_backup; |
There was a problem hiding this comment.
nit: I think the pub here is just keeping dead_code quiet until there's a caller. The in-tree pattern for that is #[cfg_attr(not(test), allow(dead_code))] mod ... (see terminal_transport further down), which keeps this off buzz_lib's public surface.
Signed-off-by: Jordan Mecom <jm@squareup.com>
Signed-off-by: Jordan Mecom <jm@squareup.com>
wpfleger96
left a comment
There was a problem hiding this comment.
I re-reviewed 2cd08b1b7bd82ce43d4f41664216d5211f525237. The framing test now covers actual concatenation collisions at both field boundaries, and the comments accurately describe the zeroization limitation. No new blocking findings.
We’re explicitly accepting the documented provider-memory risk for this unintegrated part-1 PR. The hidden plaintext copy is not fixed; this approval records that risk acceptance rather than treating the comment as a fix. The remaining cleanup suggestions are non-blocking.
This follow-up was source review only; I did not rerun tests on this revision.
Signed-off-by: Jordan Mecom <jm@squareup.com>
There was a problem hiding this comment.
🤖 Reviewed exact head 90c01a13f51939ff22375b2885f9a6252b93f677 for cryptographic correctness, wire canonicalization/interoperability, secret handling, and API misuse hazards. No blocking findings. The fixed RFC 9180 suite IDs and big-endian encoding are correct; metadata is domain-separated and length-framed in AAD; recipient keys are structurally constrained with curve validation delegated to the HPKE provider; envelope decoding enforces canonical UUID/pubkey/base64url forms; Base mode sender-auth limitations and the provider plaintext-copy limitation are documented; and the API remains native-only rather than renderer-callable.
Non-blocking follow-ups: before treating v1 as a frozen multi-platform protocol, add a reciprocal external test proving the intended Kotlin/service receiver opens a Rust-produced vector; when this dormant primitive is integrated, add tests enforcing identity-mutation serialization, live-key acquisition, and authenticated enrollment/pubkey binding.
Independent exact-head validation reported: git diff --check origin/main...HEAD passed; all 13 hpke_key_backup::tests passed; and a final full just desktop-tauri-test rerun passed. CI has no pending or failing checks. GitHub still reports BLOCKED / REVIEW_REQUIRED, and the exact-head Codex security-review comment says a new review is required for the current range.
Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com> * origin/main: Add native HPKE encryption for nsec backups (#7849) test(desktop): scope video menu e2e probes to emitted messages (#7953) Add owner deletion admission control plane (#7818) Signed-off-by: Hayt <211b96e6a2b7f45fd4047988976c7bbbeeda0c15f3ae7b32eec20834b5a55118@buzz.block.builderlab.xyz>
…in-ui * origin/main: test(desktop): wait for channel head refresh before paging thread summary test (#7955) 🤖 perf: bound long-thread aux reads and make query deadlines terminal (#7854) Add native HPKE encryption for nsec backups (#7849) test(desktop): scope video menu e2e probes to emitted messages (#7953) Add owner deletion admission control plane (#7818) fix(sidebar): converge stale-at-open state across devices (sections/sort/stars/mutes) (#7805) feat(nip-fi): harden Blossom kind-24242 verifier to NIP-FI spec (#7288) Make relay readiness process-local (#7341) 🤖 docs(nip-fi): remove implementation references from the spec (#7912) Signed-off-by: Sol <49aa1f65411fd096d2e2ec144f1e7aa36fdc76d1b907cfdf7be000c66f9d3b8e@buzz.block.builderlab.xyz>
Summary
Add a Rust API that encrypts an nsec into a backup envelope using RFC 9180 Base mode: DHKEM(P-256, HKDF-SHA256), HKDF-SHA256, and AES-256-GCM.