encryption: remove Ed25519/dalek entirely (no consumer, wrong SIMD substrate)#258
Open
AdaWorldAPI wants to merge 2 commits into
Open
encryption: remove Ed25519/dalek entirely (no consumer, wrong SIMD substrate)#258AdaWorldAPI wants to merge 2 commits into
AdaWorldAPI wants to merge 2 commits into
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
Contributor
📝 WalkthroughWalkthroughThe encryption crate replaces Ed25519 support with X448 sealed channels and HKDF-SHA384 utilities. It adds authenticated directional record handling and removes the Rust and wasm signing APIs. ChangesEncryption crate transition
Estimated code review effort: 4 (Complex) | ~60 minutes 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}
Contributor
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
Comment on lines
19
to
+22
| sha2 = { version = "0.10", default-features = false } | ||
| # X448 key agreement — AdaWorldAPI fork of RustCrypto/elliptic-curves. No dalek, | ||
| # and crypto-bigint underneath means no foreign AVX2 for the polyfill to fix. | ||
| x448 = { git = "https://github.com/AdaWorldAPI/elliptic-curves", branch = "master", default-features = false } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files relevant to encryption =="
git ls-files | rg '(^|/)crates/encryption/(Cargo\.toml|src/channel\.rs|Cargo\.lock)$|Cargo\.lock$' || true
echo
echo "== encryption Cargo.toml =="
if [ -f crates/encryption/Cargo.toml ]; then
cat -n crates/encryption/Cargo.toml
fi
echo
echo "== Cargo.lock x448/elliptic-curves entries =="
if [ -f Cargo.lock ]; then
rg -n -C 4 'name = "x448"|name = "elliptic-curves"|branch = "master"|git = "https://github.com/AdaWorldAPI/elliptic-curves"' Cargo.lock || true
fi
echo
echo "== channel.rs outline and relevant sections =="
if [ -f crates/encryption/src/channel.rs ]; then
wc -l crates/encryption/src/channel.rs
ast-grep outline crates/encryption/src/channel.rs || true
sed -n '1,240p' crates/encryption/src/channel.rs | cat -n
fiRepository: AdaWorldAPI/ndarray
Length of output: 15279
Fix the X448 fork pin instead of relying on its head branch. The channel’s authentication flow trusts this dependency for key agreement and low-order public-key rejection, but crates/encryption/Cargo.toml#L20-L22 pins x448 to branch = "master" from the private fork; lock the dependency to an immutable commit and verify the fork keeps the intended RFC 7748 clamping/rejection behavior before accepting this auth path.
📍 Affects 2 files
crates/encryption/Cargo.toml#L19-L22(this comment)crates/encryption/src/channel.rs#L108-L109
🤖 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/Cargo.toml` around lines 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.
Comment on lines
+92
to
+274
| /// The server's long-term identity. Its public half is pinned into the client | ||
| /// build; its private half never leaves the server. | ||
| pub struct ServerIdentity { | ||
| secret: [u8; KEY_LEN], | ||
| public: [u8; KEY_LEN], | ||
| } | ||
|
|
||
| impl ServerIdentity { | ||
| /// Generate a fresh server identity from the platform CSPRNG. | ||
| pub fn generate() -> Result<Self, ChannelError> { | ||
| let mut secret = [0u8; KEY_LEN]; | ||
| crate::fill_random(&mut secret).map_err(|_| ChannelError::Rng)?; | ||
| Self::from_secret(secret) | ||
| } | ||
|
|
||
| /// Rebuild an identity from stored secret bytes. | ||
| pub fn from_secret(secret: [u8; KEY_LEN]) -> Result<Self, ChannelError> { | ||
| let public = x448::x448(secret, x448::X448_BASEPOINT_BYTES).ok_or(ChannelError::BadPublicKey)?; | ||
| Ok(Self { secret, public }) | ||
| } | ||
|
|
||
| /// The public key to pin in the client build. | ||
| #[must_use] | ||
| pub fn public_key(&self) -> [u8; KEY_LEN] { | ||
| self.public | ||
| } | ||
| } | ||
|
|
||
| impl Drop for ServerIdentity { | ||
| fn drop(&mut self) { | ||
| use zeroize::Zeroize; | ||
| self.secret.zeroize(); | ||
| } | ||
| } | ||
|
|
||
| /// An established channel: two directional keys and the counters that keep | ||
| /// every nonce unique. | ||
| pub struct SealedChannel { | ||
| send_key: [u8; 32], | ||
| recv_key: [u8; 32], | ||
| send_dir: u8, | ||
| recv_dir: u8, | ||
| send_counter: u64, | ||
| highest_seen: u64, | ||
| } | ||
|
|
||
| impl Drop for SealedChannel { | ||
| fn drop(&mut self) { | ||
| use zeroize::Zeroize; | ||
| self.send_key.zeroize(); | ||
| self.recv_key.zeroize(); | ||
| } | ||
| } | ||
|
|
||
| /// Client side of the handshake. | ||
| /// | ||
| /// Returns the bytes to send (the ephemeral public key) and the established | ||
| /// channel. Note what is NOT returned: any indication of whether the server is | ||
| /// genuine. That is only learned when a record from the server opens — which | ||
| /// is the point, because a man in the middle cannot make one open. | ||
| pub fn client_handshake(server_public: &[u8; KEY_LEN]) -> Result<([u8; KEY_LEN], SealedChannel), ChannelError> { | ||
| let mut ephemeral_secret = [0u8; KEY_LEN]; | ||
| crate::fill_random(&mut ephemeral_secret).map_err(|_| ChannelError::Rng)?; | ||
|
|
||
| let ephemeral_public = | ||
| x448::x448(ephemeral_secret, x448::X448_BASEPOINT_BYTES).ok_or(ChannelError::BadPublicKey)?; | ||
| let shared = x448::x448(ephemeral_secret, *server_public).ok_or(ChannelError::BadPublicKey)?; | ||
|
|
||
| let channel = derive(&shared, &ephemeral_public, server_public, true); | ||
| Ok((ephemeral_public, channel)) | ||
| } | ||
|
|
||
| /// Server side of the handshake, given the client's ephemeral public key. | ||
| pub fn server_handshake( | ||
| identity: &ServerIdentity, client_ephemeral: &[u8; KEY_LEN], | ||
| ) -> Result<SealedChannel, ChannelError> { | ||
| let shared = x448::x448(identity.secret, *client_ephemeral).ok_or(ChannelError::BadPublicKey)?; | ||
| Ok(derive(&shared, client_ephemeral, &identity.public, false)) | ||
| } | ||
|
|
||
| /// Both sides run exactly this, over exactly these inputs — if either the | ||
| /// transcript or the shared secret differs by one bit, the keys differ entirely | ||
| /// and nothing opens. | ||
| fn derive( | ||
| shared: &[u8; KEY_LEN], client_ephemeral: &[u8; KEY_LEN], server_public: &[u8; KEY_LEN], is_client: bool, | ||
| ) -> SealedChannel { | ||
| let mut transcript_input = Vec::with_capacity(PROTOCOL.len() + 2 * KEY_LEN); | ||
| transcript_input.extend_from_slice(PROTOCOL); | ||
| transcript_input.extend_from_slice(client_ephemeral); | ||
| transcript_input.extend_from_slice(server_public); | ||
| let transcript = sha384(&transcript_input); | ||
|
|
||
| let prk = extract(&transcript, shared); | ||
| let mut c2s = [0u8; 32]; | ||
| let mut s2c = [0u8; 32]; | ||
| // Unwrap: 32 bytes is far below HKDF's 255*48 ceiling. | ||
| expand(&prk, b"c2s", &mut c2s).expect("32 bytes is within HKDF's ceiling"); | ||
| expand(&prk, b"s2c", &mut s2c).expect("32 bytes is within HKDF's ceiling"); | ||
|
|
||
| if is_client { | ||
| SealedChannel { | ||
| send_key: c2s, | ||
| recv_key: s2c, | ||
| send_dir: DIR_C2S, | ||
| recv_dir: DIR_S2C, | ||
| send_counter: 0, | ||
| highest_seen: 0, | ||
| } | ||
| } else { | ||
| SealedChannel { | ||
| send_key: s2c, | ||
| recv_key: c2s, | ||
| send_dir: DIR_S2C, | ||
| recv_dir: DIR_C2S, | ||
| send_counter: 0, | ||
| highest_seen: 0, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn nonce_for(dir: u8, counter: u64) -> [u8; NONCE_LEN] { | ||
| let mut nonce = [0u8; NONCE_LEN]; | ||
| nonce[0] = dir; | ||
| nonce[NONCE_LEN - COUNTER_LEN..].copy_from_slice(&counter.to_be_bytes()); | ||
| nonce | ||
| } | ||
|
|
||
| fn aad_for(dir: u8, counter: u64) -> [u8; 1 + COUNTER_LEN] { | ||
| let mut aad = [0u8; 1 + COUNTER_LEN]; | ||
| aad[0] = dir; | ||
| aad[1..].copy_from_slice(&counter.to_be_bytes()); | ||
| aad | ||
| } | ||
|
|
||
| impl SealedChannel { | ||
| /// Seal one record for the peer. | ||
| /// | ||
| /// The counter advances on every call and is never reused, which is what | ||
| /// keeps the nonce unique — the one failure this construction does not | ||
| /// survive. | ||
| pub fn seal(&mut self, plaintext: &[u8]) -> Result<Vec<u8>, ChannelError> { | ||
| let counter = self | ||
| .send_counter | ||
| .checked_add(1) | ||
| .ok_or(ChannelError::BadRecord)?; | ||
| let nonce = nonce_for(self.send_dir, counter); | ||
| let aad = aad_for(self.send_dir, counter); | ||
| let ciphertext = | ||
| aead::seal_with_key(&self.send_key, &nonce, &aad, plaintext).map_err(|_| ChannelError::Encrypt)?; | ||
|
|
||
| self.send_counter = counter; | ||
| let mut record = Vec::with_capacity(COUNTER_LEN + ciphertext.len()); | ||
| record.extend_from_slice(&counter.to_be_bytes()); | ||
| record.extend_from_slice(&ciphertext); | ||
| Ok(record) | ||
| } | ||
|
|
||
| /// Open a record from the peer. | ||
| /// | ||
| /// Rejects any counter that is not strictly greater than the highest | ||
| /// already accepted: a captured record replayed later is refused, and so is | ||
| /// a reordered one. The counter is only committed after the tag verifies, | ||
| /// so a forged record cannot advance the window and lock out real traffic. | ||
| pub fn open(&mut self, record: &[u8]) -> Result<Vec<u8>, ChannelError> { | ||
| if record.len() < COUNTER_LEN + TAG_LEN { | ||
| return Err(ChannelError::BadRecord); | ||
| } | ||
| let mut counter_bytes = [0u8; COUNTER_LEN]; | ||
| counter_bytes.copy_from_slice(&record[..COUNTER_LEN]); | ||
| let counter = u64::from_be_bytes(counter_bytes); | ||
| if counter <= self.highest_seen { | ||
| return Err(ChannelError::BadRecord); | ||
| } | ||
|
|
||
| let nonce = nonce_for(self.recv_dir, counter); | ||
| let aad = aad_for(self.recv_dir, counter); | ||
| let plaintext = aead::open_with_key(&self.recv_key, &nonce, &aad, &record[COUNTER_LEN..]) | ||
| .map_err(|_| ChannelError::Decrypt)?; | ||
|
|
||
| self.highest_seen = counter; | ||
| Ok(plaintext) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Newly added public APIs are missing required doc examples. As per coding guidelines, "All public APIs (public functions and methods) must have /// doc comments with examples," but none of the public functions introduced by this cohort include one.
crates/encryption/src/channel.rs#L92-L274: add///examples toServerIdentity::generate,ServerIdentity::from_secret,ServerIdentity::public_key,client_handshake,server_handshake,SealedChannel::seal, andSealedChannel::open.crates/encryption/src/hkdf_sha384.rs#L22-L106: add///examples tohmac_sha384,Prk::as_bytes,extract, andexpand.
📍 Affects 2 files
crates/encryption/src/channel.rs#L92-L274(this comment)crates/encryption/src/hkdf_sha384.rs#L22-L106
🤖 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 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.
Source: Coding guidelines
Comment on lines
+152
to
+170
| pub fn client_handshake(server_public: &[u8; KEY_LEN]) -> Result<([u8; KEY_LEN], SealedChannel), ChannelError> { | ||
| let mut ephemeral_secret = [0u8; KEY_LEN]; | ||
| crate::fill_random(&mut ephemeral_secret).map_err(|_| ChannelError::Rng)?; | ||
|
|
||
| let ephemeral_public = | ||
| x448::x448(ephemeral_secret, x448::X448_BASEPOINT_BYTES).ok_or(ChannelError::BadPublicKey)?; | ||
| let shared = x448::x448(ephemeral_secret, *server_public).ok_or(ChannelError::BadPublicKey)?; | ||
|
|
||
| let channel = derive(&shared, &ephemeral_public, server_public, true); | ||
| Ok((ephemeral_public, channel)) | ||
| } | ||
|
|
||
| /// Server side of the handshake, given the client's ephemeral public key. | ||
| pub fn server_handshake( | ||
| identity: &ServerIdentity, client_ephemeral: &[u8; KEY_LEN], | ||
| ) -> Result<SealedChannel, ChannelError> { | ||
| let shared = x448::x448(identity.secret, *client_ephemeral).ok_or(ChannelError::BadPublicKey)?; | ||
| Ok(derive(&shared, client_ephemeral, &identity.public, false)) | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Ephemeral secret and ECDH shared secret aren't zeroized before drop.
ephemeral_secret (client_handshake) and shared (both client_handshake and server_handshake) are raw [u8; 56] key material left as plain stack locals — unlike every other secret in this crate (ServerIdentity.secret, SealedChannel keys, Prk, HMAC's scratch buffers in hkdf_sha384.rs), which are all explicitly zeroized. This is an inconsistency in the crate's own memory-hygiene guarantee for the two values most directly derived from the private key material.
🔒️ Suggested fix
+use zeroize::Zeroize;
+
pub fn client_handshake(server_public: &[u8; KEY_LEN]) -> Result<([u8; KEY_LEN], SealedChannel), ChannelError> {
let mut ephemeral_secret = [0u8; KEY_LEN];
crate::fill_random(&mut ephemeral_secret).map_err(|_| ChannelError::Rng)?;
let ephemeral_public =
x448::x448(ephemeral_secret, x448::X448_BASEPOINT_BYTES).ok_or(ChannelError::BadPublicKey)?;
- let shared = x448::x448(ephemeral_secret, *server_public).ok_or(ChannelError::BadPublicKey)?;
+ let mut shared = x448::x448(ephemeral_secret, *server_public).ok_or(ChannelError::BadPublicKey)?;
+ ephemeral_secret.zeroize();
let channel = derive(&shared, &ephemeral_public, server_public, true);
+ shared.zeroize();
Ok((ephemeral_public, channel))
}
pub fn server_handshake(
identity: &ServerIdentity, client_ephemeral: &[u8; KEY_LEN],
) -> Result<SealedChannel, ChannelError> {
- let shared = x448::x448(identity.secret, *client_ephemeral).ok_or(ChannelError::BadPublicKey)?;
- Ok(derive(&shared, client_ephemeral, &identity.public, false))
+ let mut shared = x448::x448(identity.secret, *client_ephemeral).ok_or(ChannelError::BadPublicKey)?;
+ let channel = derive(&shared, client_ephemeral, &identity.public, false);
+ shared.zeroize();
+ Ok(channel)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub fn client_handshake(server_public: &[u8; KEY_LEN]) -> Result<([u8; KEY_LEN], SealedChannel), ChannelError> { | |
| let mut ephemeral_secret = [0u8; KEY_LEN]; | |
| crate::fill_random(&mut ephemeral_secret).map_err(|_| ChannelError::Rng)?; | |
| let ephemeral_public = | |
| x448::x448(ephemeral_secret, x448::X448_BASEPOINT_BYTES).ok_or(ChannelError::BadPublicKey)?; | |
| let shared = x448::x448(ephemeral_secret, *server_public).ok_or(ChannelError::BadPublicKey)?; | |
| let channel = derive(&shared, &ephemeral_public, server_public, true); | |
| Ok((ephemeral_public, channel)) | |
| } | |
| /// Server side of the handshake, given the client's ephemeral public key. | |
| pub fn server_handshake( | |
| identity: &ServerIdentity, client_ephemeral: &[u8; KEY_LEN], | |
| ) -> Result<SealedChannel, ChannelError> { | |
| let shared = x448::x448(identity.secret, *client_ephemeral).ok_or(ChannelError::BadPublicKey)?; | |
| Ok(derive(&shared, client_ephemeral, &identity.public, false)) | |
| } | |
| use zeroize::Zeroize; | |
| pub fn client_handshake(server_public: &[u8; KEY_LEN]) -> Result<([u8; KEY_LEN], SealedChannel), ChannelError> { | |
| let mut ephemeral_secret = [0u8; KEY_LEN]; | |
| crate::fill_random(&mut ephemeral_secret).map_err(|_| ChannelError::Rng)?; | |
| let ephemeral_public = | |
| x448::x448(ephemeral_secret, x448::X448_BASEPOINT_BYTES).ok_or(ChannelError::BadPublicKey)?; | |
| let mut shared = x448::x448(ephemeral_secret, *server_public).ok_or(ChannelError::BadPublicKey)?; | |
| ephemeral_secret.zeroize(); | |
| let channel = derive(&shared, &ephemeral_public, server_public, true); | |
| shared.zeroize(); | |
| Ok((ephemeral_public, channel)) | |
| } | |
| /// Server side of the handshake, given the client's ephemeral public key. | |
| pub fn server_handshake( | |
| identity: &ServerIdentity, client_ephemeral: &[u8; KEY_LEN], | |
| ) -> Result<SealedChannel, ChannelError> { | |
| let mut shared = x448::x448(identity.secret, *client_ephemeral).ok_or(ChannelError::BadPublicKey)?; | |
| let channel = derive(&shared, client_ephemeral, &identity.public, false); | |
| shared.zeroize(); | |
| Ok(channel) | |
| } |
🤖 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 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.
Comment on lines
+33
to
+36
| for i in 0..BLOCK_LEN { | ||
| inner[i] = padded[i] ^ 0x36; | ||
| outer[i] = padded[i] ^ 0x5c; | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate hkdf file =="
fd -a 'hkdf_sha384\.rs$' . || true
echo "== file excerpt =="
FILE="$(fd 'hkdf_sha384\.rs$' . | head -n1 || true)"
if [ -n "${FILE:-}" ]; then
wc -l "$FILE"
sed -n '1,120p' "$FILE" | cat -n
fi
echo "== Rust/toolchain hints =="
for f in Cargo.toml rustfmt.toml clippy.toml; do
if [ -f "$f" ]; then echo "--- $f"; sed -n '1,160p' "$f"; fi
done
echo "== deterministic lints config/search =="
rg -n "needless_range_loop|clippy::|rust-version|cargo-clippy|clippy.toml|deny\(warnings\)" -S Cargo.toml .clippy.toml .cargo 2>/dev/null || trueRepository: AdaWorldAPI/ndarray
Length of output: 9211
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== try clippy on target crate if available =="
if command -v cargo >/dev/null 2>&1 && cargo --version >/dev/null 2>&1; then
echo "cargo version: $(cargo --version)"
if [ -f crates/encryption/Cargo.toml ]; then
cargo build --quiet 2>/tmp/clippy_build.out || { cat /tmp/clippy_build.out >&2; false; }
cargo clippy --package encryption -- -D warnings 2>/tmp/clippy.out || { sed -n '1,220p' /tmp/clippy.out; false; }
echo "clippy passed"
else
echo "crates/encryption/Cargo.toml missing"
fi
else
echo "cargo unavailable"
fi
echo "== minimal standalone clippy lint probe if rustc available =="
python3 - <<'PY'
from pathlib import Path
import subprocess, sys
for p in [Path("/usr/local/cargo/bin/clippy-driver"), Path("/usr/bin/clippy-driver"), Path("clippy-driver").resolve()]:
if p.exists():
print("clippy-driver at", p)
PYRepository: AdaWorldAPI/ndarray
Length of output: 149
🌐 Web query:
Rust Clippy needless_range_loop lint pattern for loop i in 0.. indexing array[i]
💡 Result:
The needless_range_loop lint in Rust Clippy detects when you iterate over a range (e.g., 0..len) specifically to access elements of a collection by index [1]. This pattern is considered non-idiomatic in Rust because it is more verbose and can be less performant due to the compiler needing to perform unnecessary bounds checks [1]. To resolve this lint, you should iterate over the collection directly or use iterator methods [1]. For simple iteration over elements: // Instead of this: for i in 0..vec.len { println!("{}", vec[i]); } // Do this: for item in &vec { println!("{}", item); } When you need both the index and the item, use the.iter.enumerate method [2]: for (index, item) in vec.iter.enumerate { println!("{}: {}", index, item); } If you are performing operations like copying between slices, idiomatic alternatives include using slice methods such as copy_from_slice or iterators like zip [2]. Using iterators is generally preferred because it enables the compiler to optimize the code more effectively by eliminating bounds checks [1].
Citations:
- 1: https://help.klocwork.com/current/en-us/reference/rs.clippy.needless_range_loop.htm
- 2: https://stackoverflow.com/questions/44564772/how-do-i-fix-clippys-needless-range-loop-for-loops-that-copy-between-slices-wit
Use iterators to avoid the range-indexing lint.
Indexing inner[i] and outer[i] from padded[i] over 0..BLOCK_LEN triggers needless_range_loop under -D warnings; replace it with a direct zip/iterator form.
🤖 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/hkdf_sha384.rs` around lines 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.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Second half of "dalek komplett raus". #257 reverted #256's
x25519-dalek+hkdfadditions; this removes the pre-existinged25519-dalekas well.Gone:
sign.rs, thesignmodule + re-export, theed25519-dalekdependency, and the four Ed25519 wasm bindings.sha384and the envelope bindings stay. The forward suite is now argon2 + XChaCha20-Poly1305 + SHA-384 — all RustCrypto crates whose AVX2 backend is a separable module, which is the point.Why remove rather than keep on a serial backend:
No consumer.
signwas re-exported throughogar-encryption→ogar-authbut never called; ogar-auth's real crypto is argon2 + TOTP. No signing, no stored signatures, so removal changes no behaviour. (I earlier framed this as a hard choice that would break licence signatures — that was an invented cost; there are zero real callers.)dalek can't take the matryoshka.
field.rsreaches aroundpacked_simd.rsto raw intrinsics at 35 sites, so the polyfill can't swap one backend module the way it does for chacha20/sha2/poly1305. RustCrypto has no non-dalek Ed25519, so "keep Ed25519, drop dalek" isn't satisfiable — with no consumer, dropping it is the clean resolution.25 tests green;
--features wasm-bindingsstill compiles.OGAR's
crates/ogar-encryption/src/lib.rsandcrates/ogar-auth/src/lib.rsbothpub use …{sign}. Those re-exports must dropsignin the same coordination, orogar-encryptionfails to build against this master (it depsencryptionatbranch = "master"). That edit is on the OGAR side, which I can't push to.🤖 Generated with Claude Code
https://claude.ai/code/session_01Mwq1QKpw4zRd6oaGRoJhF2
Generated by Claude Code
Summary by CodeRabbit