Add multi-source entropy mixing for defense-in-depth randomness #129
Conversation
WalkthroughAdds a new public entropy module that mixes OS randomness, optional x86_64 RDRAND, timing jitter, and process context via Blake2b to produce variable-length random bytes. The crypto module now sources randomness from this entropy API instead of direct RNG usage. Changes
Sequence DiagramsequenceDiagram
actor Caller
participant Crypto as Crypto Module
participant Entropy as Entropy Module
participant Sources as Entropy Sources
Caller->>Crypto: SecretKey::generate() / encrypt nonce request
Crypto->>Entropy: random_bytes<32>() / random_bytes<N>()
Entropy->>Sources: gather OS randomness
Entropy->>Sources: probe RDRAND (if available)
Entropy->>Sources: capture timing jitter
Entropy->>Sources: collect process context
Sources-->>Entropy: raw entropy pieces
Entropy->>Entropy: Blake2b-512 each source, truncate/combine
Entropy-->>Crypto: composed random bytes
Crypto-->>Caller: generated SecretKey / nonce
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (15)
✏️ Tip: You can disable this entire section by setting 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@keep-core/src/entropy.rs`:
- Around line 34-44: The fill_rdrand function uses a single call to the
_rdrand64_step intrinsic and returns false on the first transient failure;
change fill_rdrand to retry the _rdrand64_step call (e.g., up to 10 attempts)
for each 64-bit chunk before giving up so transient failures don't abort filling
the pool; keep the existing behavior of writing val.to_le_bytes() into each
chunk when successful and only return false if all retry attempts for a chunk
fail.
- Around line 247-252: The test test_random_bytes_large_buffer incorrectly
compares the first 32 bytes to a single byte (bytes[32]) instead of comparing
blocks for independence; update the assertion to compare slices of equal length
(e.g., compare &bytes[..32] with &bytes[32..64] or compare corresponding bytes
across blocks) so the test verifies that the first 32-byte block is not
identical to the next 32-byte block returned by random_bytes().
- Around line 18-32: The is_rdrand_available function should not unconditionally
return true when compiled with target_feature="rdrand"; always perform the
runtime CPUID check (using core::arch::x86_64::__cpuid) and cache the result
(e.g., with the existing static AVAILABLE OnceLock) so that fill_rdrand() can
safely rely on is_rdrand_available() at runtime even if the compile-time flag
and actual CPU capabilities differ; remove the #[cfg(target_feature = "rdrand")]
early-true branch and ensure the CPUID bit test ((result.ecx & (1 << 30)) != 0)
always runs.
🧹 Nitpick comments (4)
keep-core/src/entropy.rs (4)
56-73: Timing jitter source provides minimal entropy on virtualized or high-performance systems.The
spin_loop()hint is a no-op on many architectures and may produce near-zero or constant jitter values, especially in VMs, containers, or systems with consistent clock resolution. While this is mixed with other sources (defense-in-depth), the loop only runs 8 iterations with a trivial spin, which may not accumulate meaningful entropy variance.Consider adding more iterations or actual work within the loop to increase timing variance. However, since this is one of multiple sources and OS entropy is the primary source, this is acceptable for defense-in-depth.
75-87: Process context entropy is largely static within a single process lifetime.PID, thread ID, and memory addresses are deterministic values that remain constant or predictable within a process. An attacker with access to the process can trivially determine these values. While hashed together with other sources, this provides minimal additional entropy beyond the initial call.
This is acceptable for defense-in-depth since OS entropy is the primary source, but the comment should clarify these are anti-prediction measures rather than true entropy sources.
138-153: Unnecessary re-gathering of entropy for each 32-byte block when N > 32.For requests larger than 32 bytes,
random_bytes_mixed()is called for every 32-byte block, which re-gathers entropy from all sources (OS, RDRAND, timing, context) each iteration. This is computationally expensive and unnecessary—the standard approach is to gather entropy once and use counter-mode expansion via the hash.♻️ Suggested improvement for large buffer generation
} else { + let base_block = random_bytes_mixed(); let mut offset = 0; let mut counter: u64 = 0; while offset < N { - let block = random_bytes_mixed(); let mut hasher = Blake2b512::new(); - hasher.update(block); + hasher.update(base_block); hasher.update(counter.to_le_bytes()); let result = hasher.finalize(); let remaining = N - offset; let copy_len = remaining.min(32); output[offset..offset + copy_len].copy_from_slice(&result[..copy_len]); offset += copy_len; counter += 1; } }
175-186: Health check is probabilistically weak with only 3 samples.The uniqueness check
samples[0] != samples[1] && samples[1] != samples[2] && samples[0] != samples[2]only verifies 3 samples are distinct. A broken RNG that cycles through a small set of values could pass this check. Consider increasing the sample count or adding additional statistical checks (e.g., hamming distance, bit distribution).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
keep-core/src/crypto.rskeep-core/src/entropy.rskeep-core/src/lib.rs
🧰 Additional context used
🧬 Code graph analysis (2)
keep-core/src/entropy.rs (1)
keep-core/src/crypto.rs (1)
random_bytes(363-365)
keep-core/src/crypto.rs (1)
keep-core/src/entropy.rs (1)
random_bytes(132-157)
🔇 Additional comments (7)
keep-core/src/entropy.rs (2)
89-100: Good domain separation in entropy mixing.The
mix_entropyfunction properly uses a domain separator ("keep-entropy-v1"), source index, and length prefixing before each source. This prevents length extension attacks and ensures different source combinations produce distinct outputs.
111-126: Solid multi-source entropy mixing implementation.The
random_bytes_mixed()function correctly gathers from multiple sources and conditionally includes RDRAND when available. The design ensures that even if one source is compromised, the output remains secure as long as at least one source provides good entropy.keep-core/src/lib.rs (1)
10-10: Verify intentional public exposure of entropy module.Making the entropy module
pubexposesrandom_bytes_mixed(),random_bytes(),check_entropy_health(), andhas_rdrand()as part of the crate's public API. If this is intended only for internal use within the crate, considerpub(crate) mod entropy;instead.keep-core/src/crypto.rs (4)
12-12: Clean import of the new entropy module.The import correctly references the crate-local entropy module.
227-230: Key generation correctly uses mixed entropy.
SecretKey::generate()now uses the multi-source entropy for cryptographic key generation, improving defense-in-depth for this security-critical operation.
331-332: Nonce generation correctly uses mixed entropy.The 24-byte XChaCha20-Poly1305 nonce is now generated from mixed entropy sources, which is appropriate for this security-critical value.
363-365: Publicrandom_bytesfunction is now a thin wrapper.This maintains backward compatibility while delegating to the new entropy module. The wrapper in
crypto.rsand the implementation inentropy.rs(shown in relevant snippets) are consistent.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Summary
Test plan
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.