encryption: sealed channel (X448 + HKDF-SHA384), and neutralize dalek's SIMD by cfg - #258
Conversation
… the wrong substrate Second half of "dalek komplett raus". #257 reverted the x25519-dalek + hkdf additions from #256; this removes the pre-existing ed25519-dalek too: sign.rs, the `sign` module + re-export, the dependency, and the four Ed25519 wasm bindings (generate_signing_seed / public_key_of / sign_message / verify_signature). sha384 and the envelope bindings stay. Two reasons: 1. Nothing uses it. The `sign` surface was re-exported through ogar-encryption -> ogar-auth but never called — ogar-auth's real crypto is argon2 (password) and TOTP. There are no stored signatures, so removal changes no behaviour. (I earlier claimed removing it would break licence signatures and laid it out as a hard operator choice; that was an invented cost — checked now, there are zero real callers.) 2. dalek is the wrong substrate for this stack's SIMD policy. Its AVX2 backend is not separable: field.rs reaches around packed_simd.rs to raw intrinsics at 35 sites, so the ndarray::simd matryoshka cannot swap one backend module the way it does for chacha20/sha2/poly1305, where AVX2 is its own file. And RustCrypto has no non-dalek Ed25519, so "keep Ed25519, drop dalek" is not satisfiable — with no consumer, dropping it outright is the clean answer. The forward suite is now argon2 + XChaCha20-Poly1305 + SHA-384 — all RustCrypto crates with separable AVX2, the next step being the fork switch. 25 tests green; wasm-bindings still compiles. DOWNSTREAM (out of this repo's push scope, needs a matching drop): OGAR's crates/ogar-encryption/src/lib.rs and crates/ogar-auth/src/lib.rs both `pub use ...{sign}` — those re-exports must drop `sign` or ogar-encryption fails to build against this master. Generated by [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mwq1QKpw4zRd6oaGRoJhF2
📝 WalkthroughWalkthroughThe encryption crate adds a self-contained HKDF-SHA384 implementation and an X448-based sealed channel with authenticated, replay-resistant records. It also updates x86_64 crypto backend selection, dependencies, and public module exports. ChangesEncryption crate transition
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant SealedChannel
Client->>Server: client_handshake(server_public)
Server->>SealedChannel: server_handshake(identity, client_ephemeral)
Client->>SealedChannel: seal(plaintext)
SealedChannel->>Server: encrypted record
Server->>SealedChannel: open(record)
SealedChannel->>Server: authenticated plaintext
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_bd215b36-00db-4735-9401-277d885575fa) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3694a3c1c9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -69,7 +68,6 @@ pub mod aead; | |||
| pub mod envelope; | |||
| pub mod hash; | |||
| pub mod kdf; | |||
There was a problem hiding this comment.
For the downstream OGAR crates identified in the commit message, deleting this module causes their existing pub use encryption::sign imports to fail at compile time even if no signing function is ever called. Because those crates track this repository's master branch, merging this commit before their re-exports are removed breaks every affected build; retain a compatibility module or coordinate the downstream change before landing this deletion.
Useful? React with 👍 / 👎.
| rust-version = "1.95" | ||
| license = "MIT OR Apache-2.0" | ||
| description = "Zero-knowledge envelope crypto for the Ada stack: Argon2id KDF, XChaCha20-Poly1305 AEAD, Ed25519 signatures, SHA-384 hashing. One codebase for native servers and wasm32 browsers." | ||
| description = "Zero-knowledge envelope crypto for the Ada stack: Argon2id KDF, XChaCha20-Poly1305 AEAD, SHA-384 hashing. One codebase for native servers and wasm32 browsers." |
There was a problem hiding this comment.
After this description stops advertising Ed25519 and the commit deletes the corresponding module and wasm bindings, the crate-level documentation still says at src/lib.rs:39-40 that the crate exists to provide Ed25519 and at src/lib.rs:59-62 describes Ed25519 as one of its chosen primitives. Consumers reading the generated documentation are therefore still told that the removed capability is part of this suite, so those passages should be updated as part of the removal.
Useful? React with 👍 / 👎.
…a sealed record
This is the POC the session was for and had not delivered. What existed was
`envelope`, which seals a RECORD under a password. The job was a CONNECTION
between the browser and the server, above TLS, so that the proxy / TLS
terminator / remote-desktop layer in front of the server sees ciphertext only.
`channel.rs` is that, end to end:
handshake: client -> server ephemeral X448 public key (56 bytes)
record: counter (8, BE) | ciphertext | Poly1305 tag (16)
nonce = direction (1) | zeros (15) | counter (8)
aad = direction (1) | counter (8)
Server authentication comes from the key agreement, NOT from a signature: the
client holds the server's static X448 public key (pinned in the client build),
both sides compute X448(ephemeral, static), and only the holder of the static
private key derives the same value. A man in the middle can substitute its own
key and complete a handshake — and then nothing it forwards opens, in either
direction. That is a test, not a claim. The whole Ed25519/Ed448 detour earlier
in this session was unnecessary: the DH IS the authentication (Noise NK shape).
Directions are separated by key AND nonce, so a record cannot be reflected back
at its sender. The per-direction counter is strictly increasing and only
committed after the tag verifies, so replays and rewinds are refused and a
forged record cannot advance the window to lock out real traffic.
x448 comes from the AdaWorldAPI fork of RustCrypto/elliptic-curves — no dalek,
and crypto-bigint underneath means no foreign AVX2 intrinsics for the polyfill
to have to displace.
hkdf_sha384.rs is HMAC-SHA384 + HKDF written directly on the crate's own
sha384, deliberately: the `hmac`/`hkdf` crates sit on the digest 0.11 trait
generation while this crate's sha2 is 0.10, and dragging a second generation in
for forty lines is the tail wagging the dog. It is pinned by RFC 4231 vectors
(cases 1, 2 and 6 — case 6 covers the oversized-key branch), not by trust.
Measured, release, this box:
handshake (both sides) 1.55 ms
record 64 B seal+open 1.17 us 52 MiB/s
record 4096 B seal+open 8.26 us 473 MiB/s
record 65536 B seal+open 122.20 us 512 MiB/s
So the per-session cost is one 1.5 ms handshake and the steady state runs at
~500 MiB/s. X448 is the slow part; that is the price of the non-dalek curve.
15 new tests (9 channel, 6 kdf), 40 green in the crate, clippy clean.
Not in scope here, and stated rather than hidden: x448's own README says the
code has not been audited. And the server process still sees plaintext — this
is not zero-knowledge against the operator, and the module doc says so where
someone reading the code will find it.
Generated by [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Mwq1QKpw4zRd6oaGRoJhF2
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_e27cfbd1-dff1-4b82-b5e5-b3c53b849ddd) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
crates/encryption/src/channel.rs (1)
276-441: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider a negative test for
ChannelError::BadPublicKey.The suite thoroughly covers replay, reorder, tamper, truncation, reflection, and MITM-with-a-different-key scenarios, but none exercise a degenerate/low-order public key input to confirm the rejection path actually fires.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/encryption/src/channel.rs` around lines 276 - 441, Add a negative test in the tests module that supplies a degenerate or low-order public key to the client/server handshake entry point and asserts it returns ChannelError::BadPublicKey. Use the existing handshake helpers and preserve the established Result-based error assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/encryption/Cargo.toml`:
- Around line 19-22: The X448 dependency must not track the mutable master
branch. In crates/encryption/Cargo.toml lines 19-22, replace the branch
reference with an immutable commit revision from the trusted fork, and verify
that revision preserves RFC 7748 clamping and low-order public-key rejection for
the authentication flow. The sibling usage in crates/encryption/src/channel.rs
lines 108-109 requires no direct change.
In `@crates/encryption/src/channel.rs`:
- Around line 92-274: Add concise runnable `///` examples to the public APIs in
crates/encryption/src/channel.rs (lines 92-274): ServerIdentity::generate,
ServerIdentity::from_secret, ServerIdentity::public_key, client_handshake,
server_handshake, SealedChannel::seal, and SealedChannel::open. Also add
examples to hmac_sha384, Prk::as_bytes, extract, and expand in
crates/encryption/src/hkdf_sha384.rs (lines 22-106), using existing types and
valid inputs so documentation tests compile.
- Around line 152-170: Update client_handshake and server_handshake to
explicitly zeroize ephemeral_secret and shared before they are dropped,
including cleanup on error paths where applicable. Reuse the crate’s existing
zeroization mechanism and preserve the current handshake derivation and error
behavior.
In `@crates/encryption/src/hkdf_sha384.rs`:
- Around line 33-36: Update the loop initializing inner and outer in the HKDF
setup to use iterators, zipping mutable iterators for inner and outer with
padded.iter() instead of indexing over 0..BLOCK_LEN. Preserve the existing XOR
values and initialization behavior.
---
Nitpick comments:
In `@crates/encryption/src/channel.rs`:
- Around line 276-441: Add a negative test in the tests module that supplies a
degenerate or low-order public key to the client/server handshake entry point
and asserts it returns ChannelError::BadPublicKey. Use the existing handshake
helpers and preserve the established Result-based error assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 30d4653b-c8c0-4ce8-9070-814694952568
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
crates/encryption/Cargo.tomlcrates/encryption/src/channel.rscrates/encryption/src/hkdf_sha384.rscrates/encryption/src/lib.rscrates/encryption/src/sign.rscrates/encryption/src/wasm.rs
💤 Files with no reviewable changes (2)
- crates/encryption/src/sign.rs
- crates/encryption/src/wasm.rs
|
Closing without merging — not a revert, since nothing here ever landed on This PR's move to X448 was a workaround for a SIMD-isolation concern about Given that, and given the explicit requirement for Signal-grade trust (Signal Protocol, WireGuard, TLS 1.3 all run on X25519/Ed25519 — X448 has essentially no comparable real-world deployment or audit depth), we're going back to X25519/Ed25519 sourced from the AdaWorldAPI dalek forks (not crates.io — the actual #256 fix) instead of moving to X448. Keeping Ed25519 also dissolves this PR's own open item-1 review thread (downstream OGAR's The HKDF-SHA384 module and the channel-framing/replay-protection/zeroization design here are curve-agnostic and worth keeping as reference for the follow-up PR that rebuilds the same shape on X25519. Generated by Claude Code |
Re-scopes this branch to its additive half. The removal commit (3694a3c) is reverted: `sign.rs`, the wasm Ed25519 bindings, and the `ed25519-dalek` dependency come back. The removal rested on a premise that does not survive checking. dalek's vector backend is real — 57 `_mm*` intrinsic calls under 52 `unsafe` occurrences across `avx2/field.rs` and `packed_simd.rs` — but neutralizing it was never a porting problem. The whole module is gated at `backend/mod.rs:42` by `#[cfg(curve25519_dalek_backend = "simd")]`, and curve25519-dalek's build.rs reads that cfg from the environment. One line in `.cargo/config.toml` compiles the entire second SIMD surface out, which is what the matryoshka pattern actually asks for: all SIMD lives once, audited, inside `ndarray::simd`. Nothing we use pays for `serial`. X25519's Montgomery ladder never references the vector backend, and the vector path serves only Edwards multi-scalar / variable-base multiplication — batch verification and Ristretto protocols, neither of which this crate performs. Verified after `cargo clean -p curve25519-dalek`: cargo build -p encryption -v | grep curve25519_dalek_backend -> curve25519_dalek_backend="serial" (only) Also in this commit, from review: - Zeroize `ephemeral_secret` and `shared` in both handshakes. The DH output was outliving the fold into the directional keys. - Add `low_order_peer_keys_are_refused_and_honest_ones_are_not`. Both halves are asserted, per the falsifiability rule: the all-zero point and u=1 must be refused on client and server sides, and an honest basepoint-derived key must go through. A guard that fires on everything carries as much information as one that never fires. This is also the tripwire for any future X25519 port — `x448()` returns `Option` and rejects low-order points, while `x25519_dalek::x25519()` returns a bare `[u8; 32]`, so a mechanical port would silently drop the RFC 7748 contributory check and this test would catch it. - Pin the `x448` git dependency to a rev instead of tracking `master`. - Replace the `needless_range_loop` in HMAC's ipad/opad derivation. - Doc examples on all 11 new public items. They assert behaviour rather than illustrate syntax: the `client_handshake` example substitutes an impostor key and shows the failure surfacing only when a record fails to open; the `open` example shows the replay window refusing an out-of-order record. 46 unit tests + 13 doc tests pass; clippy and rustdoc clean.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_b6f3fbb2-6971-4829-89c8-e07b0015b3b6) |
AdaWorldAPI
changed the title
board: record the PR #258 arc — neutralize-by-cfg beats remove
2 participants
What this is now
Re-scoped. This branch originally removed Ed25519/dalek; it no longer does. Ed25519 stays, and the second SIMD surface it drags in is compiled out by one cfg line.
Two things land here:
crates/encryption/src/channel.rs— a sealed channel between browser and server. X448 key agreement (NoiseNKshape), HKDF-SHA384 key schedule, ChaCha20-Poly1305 records with per-direction counters..cargo/config.toml—curve25519_dalek_backend="serial", which compiles dalek's vector backend out entirely.Correcting this PR's original premise
The first version of this branch removed dalek on the grounds that it "can't take the matryoshka" — that
field.rsreaches aroundpacked_simd.rsto raw intrinsics at 35 sites, so the surface could not be neutralized without porting it. Checking that:The counts are right — 57 intrinsic calls in the AVX2 path, under 52
unsafeoccurrences in those two files. What was wrong is the conclusion drawn from them. This is a reachability question, not a porting one:The whole module sits behind one cfg, and
build.rsreads it fromCARGO_CFG_CURVE25519_DALEK_BACKEND. One line in.cargo/config.tomlremoves all 57 sites from the build. Removing the dependency was solving a problem that a flag already solved.Verified after
cargo clean -p curve25519-dalek:Only
serial. No second SIMD surface in the binary — which is what the matryoshka pattern asks for (cf.vendor/chacha20: "no raw intrinsics and nounsafehere — all of that lives once, audited, insidendarray::simd").serialcosts nothing we use. X25519's Montgomery ladder (montgomery.rs) never references the vector backend. The vector path serves only Edwards multi-scalar and variable-base multiplication — batch signature verification and Ristretto protocols — neither of which this crate performs.The channel
TLS terminates at whatever proxy or load balancer sits in front of the server; everything from there inward reads plaintext. This sits above TLS, anchored on the server's static X448 key pinned into the client build, so nothing between the browser and the server process can read or forge a record.
Authentication is the key agreement itself — no signatures, no certificates, no PKI. A man in the middle can substitute its own key, but then the two sides derive different keys and the first record fails to open.
What it does not protect against, stated in the module docs so no downstream document can overclaim: the server process sees plaintext. This is not zero-knowledge against the operator. Nor does it help a client already running attacker code — it removes the exportable bearer credential, not the compromised machine.
hkdf_sha384.rsimplements HMAC (RFC 2104) and HKDF (RFC 5869) directly onhash::sha384rather than pulling thehmac/hkdfcrates, which sit ondigest 0.11while this crate'ssha2is on0.10— a second trait generation in the graph for two twenty-line functions. Both are pinned by published vectors (RFC 4231 cases 1, 2, and 6) rather than by trust; case 6 covers the hash-the-oversized-key branch a short-key-only test leaves unproven.Review items addressed
ephemeral_secretandsharedin both handshakes. The DH output was outliving its fold into the directional keys.x448to a rev instead of trackingmaster.needless_range_loopin HMAC's ipad/opad derivation.client_handshakeexample substitutes an impostor key and shows the failure surfacing only when a record fails to open; theopenexample shows the replay window refusing an out-of-order record.A note for whoever ports this to X25519
The low-order test is the tripwire.
x448::x448()returnsOptionand rejects low-order points;x25519_dalek::x25519()returns a bare[u8; 32]. RFC 7748's contributory-behaviour check is optional, so a mechanical port drops it silently. That test fails if it does.Verification
cargo clippy -p encryption --all-targets— cleancargo fmt -p encryption -- --check— cleancargo doc -p encryption --no-deps— no warningsserialafter a clean rebuild of curve25519-dalekSummary by CodeRabbit