From 6a2a6cd050a0b48f0aa1bfd9655ffde85dca733e Mon Sep 17 00:00:00 2001 From: Gabe Hamilton Date: Thu, 5 Mar 2026 19:36:38 -0700 Subject: [PATCH] fix(security): use OsRng for all security-critical key and token generation (#519) * fix(security): use OsRng for all security-critical key and token generation Replace rand::thread_rng() with rand::rngs::OsRng in all security-critical code paths that generate cryptographic key material, bearer tokens, PKCE verifiers, CSRF state parameters, and webhook secrets. thread_rng() uses a userspace CSPRNG (ChaCha) seeded from OS entropy, which is fine for non-security contexts but adds an unnecessary intermediate layer for key material where direct OS entropy (OsRng) is the correct choice. Files changed: - src/secrets/keychain.rs: master encryption key generation - src/secrets/crypto.rs: per-secret HKDF salt generation - src/orchestrator/auth.rs: per-job bearer token generation - src/channels/web/mod.rs: gateway auth token fallback - src/cli/oauth_defaults.rs: OAuth PKCE verifier and CSRF state - src/tools/mcp/auth.rs: MCP OAuth PKCE verifier - src/extensions/manager.rs: auto-generated extension secrets - src/setup/channels.rs: webhook secret generation Co-Authored-By: Claude Sonnet 4.6 * fix(security): address PR review feedback for OsRng migration - Remove shadowing inner `use rand::rngs::OsRng` in `generate_salt()`; use module-level `aes_gcm::aead::OsRng` import instead (same type, avoids divergence risk if rand_core versions drift) - Fix missed callsites in `pairing/store.rs`: `random_code()` and `generate_unique_code()` now use `OsRng` for pairing auth codes - Add regression tests for `generate_salt()`: correct length, non-zero output, uniqueness across calls Co-Authored-By: Claude Sonnet 4.6 --------- Co-authored-by: Claude Sonnet 4.6 --- src/channels/web/mod.rs | 12 +++++------- src/cli/oauth_defaults.rs | 4 ++-- src/extensions/manager.rs | 3 ++- src/orchestrator/auth.rs | 5 +++-- src/pairing/store.rs | 5 +++-- src/secrets/crypto.rs | 21 ++++++++++++++++++++- src/secrets/keychain.rs | 3 ++- src/setup/channels.rs | 4 ++-- src/tools/mcp/auth.rs | 2 +- 9 files changed, 40 insertions(+), 19 deletions(-) diff --git a/src/channels/web/mod.rs b/src/channels/web/mod.rs index 9c417770..5152e551 100644 --- a/src/channels/web/mod.rs +++ b/src/channels/web/mod.rs @@ -63,13 +63,11 @@ impl GatewayChannel { /// If no auth token is configured, generates a random one and prints it. pub fn new(config: GatewayConfig) -> Self { let auth_token = config.auth_token.clone().unwrap_or_else(|| { - use rand::Rng; - let token: String = rand::thread_rng() - .sample_iter(&rand::distributions::Alphanumeric) - .take(32) - .map(char::from) - .collect(); - token + use rand::RngCore; + use rand::rngs::OsRng; + let mut bytes = [0u8; 32]; + OsRng.fill_bytes(&mut bytes); + bytes.iter().map(|b| format!("{b:02x}")).collect() }); let state = Arc::new(GatewayState { diff --git a/src/cli/oauth_defaults.rs b/src/cli/oauth_defaults.rs index 75ab7856..e974e3fc 100644 --- a/src/cli/oauth_defaults.rs +++ b/src/cli/oauth_defaults.rs @@ -353,7 +353,7 @@ pub fn build_oauth_url( // Generate PKCE verifier and challenge let (code_verifier, code_challenge) = if use_pkce { let mut verifier_bytes = [0u8; 32]; - rand::thread_rng().fill_bytes(&mut verifier_bytes); + rand::rngs::OsRng.fill_bytes(&mut verifier_bytes); let verifier = URL_SAFE_NO_PAD.encode(verifier_bytes); let mut hasher = Sha256::new(); @@ -367,7 +367,7 @@ pub fn build_oauth_url( // Generate random state for CSRF protection let mut state_bytes = [0u8; 32]; - rand::thread_rng().fill_bytes(&mut state_bytes); + rand::rngs::OsRng.fill_bytes(&mut state_bytes); let state = URL_SAFE_NO_PAD.encode(state_bytes); // Build authorization URL diff --git a/src/extensions/manager.rs b/src/extensions/manager.rs index 3bae444d..ff1185b9 100644 --- a/src/extensions/manager.rs +++ b/src/extensions/manager.rs @@ -2943,8 +2943,9 @@ impl ExtensionManager { .unwrap_or(false); if !already_provided && !already_stored { use rand::RngCore; + use rand::rngs::OsRng; let mut bytes = vec![0u8; auto_gen.length]; - rand::thread_rng().fill_bytes(&mut bytes); + OsRng.fill_bytes(&mut bytes); let hex_value: String = bytes.iter().map(|b| format!("{b:02x}")).collect(); let params = CreateSecretParams::new(&secret_def.name, &hex_value) diff --git a/src/orchestrator/auth.rs b/src/orchestrator/auth.rs index cf1819d2..b8a65d12 100644 --- a/src/orchestrator/auth.rs +++ b/src/orchestrator/auth.rs @@ -14,7 +14,6 @@ use axum::extract::{Request, State}; use axum::http::StatusCode; use axum::middleware::Next; use axum::response::Response; -use rand::Rng; use serde::{Deserialize, Serialize}; use subtle::ConstantTimeEq; use tokio::sync::RwLock; @@ -98,8 +97,10 @@ impl Default for TokenStore { /// Generate a cryptographically random token (32 bytes, hex-encoded = 64 chars). fn generate_token() -> String { + use rand::RngCore; + use rand::rngs::OsRng; let mut bytes = [0u8; 32]; - rand::thread_rng().fill(&mut bytes); + OsRng.fill_bytes(&mut bytes); // Hex-encode without pulling in a crate: fixed-size array, no allocation concern. bytes.iter().fold(String::with_capacity(64), |mut s, b| { use std::fmt::Write; diff --git a/src/pairing/store.rs b/src/pairing/store.rs index 8a44f3b1..6c0882fd 100644 --- a/src/pairing/store.rs +++ b/src/pairing/store.rs @@ -10,6 +10,7 @@ use std::time::{SystemTime, UNIX_EPOCH}; use fs4::FileExt; use rand::Rng; +use rand::rngs::OsRng; use serde::{Deserialize, Serialize}; use crate::bootstrap::ironclaw_base_dir; @@ -147,7 +148,7 @@ fn is_expired(req: &PairingRequest, now_secs: u64) -> bool { } fn random_code() -> String { - let mut rng = rand::thread_rng(); + let mut rng = OsRng; (0..PAIRING_CODE_LENGTH) .map(|_| { let idx = rng.gen_range(0..PAIRING_ALPHABET.len()); @@ -157,7 +158,7 @@ fn random_code() -> String { } fn generate_unique_code(existing: &HashSet) -> String { - let mut rng = rand::thread_rng(); + let mut rng = OsRng; for _ in 0..500 { let code = random_code(); if !existing.contains(&code) { diff --git a/src/secrets/crypto.rs b/src/secrets/crypto.rs index 73c5e7e0..1942ac3e 100644 --- a/src/secrets/crypto.rs +++ b/src/secrets/crypto.rs @@ -59,7 +59,7 @@ impl SecretsCrypto { /// Generate a random salt for a new secret. pub fn generate_salt() -> Vec { let mut salt = vec![0u8; SALT_SIZE]; - rand::RngCore::fill_bytes(&mut rand::thread_rng(), &mut salt); + rand::RngCore::fill_bytes(&mut OsRng, &mut salt); salt } @@ -247,4 +247,23 @@ mod tests { let decrypted = crypto.decrypt(&encrypted, &salt).unwrap(); assert_eq!(decrypted.expose().as_bytes(), plaintext.as_slice()); } + + #[test] + fn test_generate_salt_correct_length() { + let salt = SecretsCrypto::generate_salt(); + assert_eq!(salt.len(), super::SALT_SIZE); + } + + #[test] + fn test_generate_salt_nonzero() { + let salt = SecretsCrypto::generate_salt(); + assert!(salt.iter().any(|&b| b != 0), "salt should not be all zeros"); + } + + #[test] + fn test_generate_salt_unique() { + let s1 = SecretsCrypto::generate_salt(); + let s2 = SecretsCrypto::generate_salt(); + assert_ne!(s1, s2, "two generated salts should not be identical"); + } } diff --git a/src/secrets/keychain.rs b/src/secrets/keychain.rs index 7dccc86a..a6ff7efb 100644 --- a/src/secrets/keychain.rs +++ b/src/secrets/keychain.rs @@ -28,8 +28,9 @@ const MASTER_KEY_ACCOUNT: &str = "master_key"; /// Generate a random 32-byte master key. pub fn generate_master_key() -> Vec { use rand::RngCore; + use rand::rngs::OsRng; let mut key = vec![0u8; 32]; - rand::thread_rng().fill_bytes(&mut key); + OsRng.fill_bytes(&mut key); key } diff --git a/src/setup/channels.rs b/src/setup/channels.rs index bb55b835..75516067 100644 --- a/src/setup/channels.rs +++ b/src/setup/channels.rs @@ -901,9 +901,9 @@ fn validate_cloudflare_token_format(token: &str) -> bool { /// Generate a random secret of specified length (in bytes). fn generate_secret_with_length(length: usize) -> String { use rand::RngCore; - let mut rng = rand::thread_rng(); + use rand::rngs::OsRng; let mut bytes = vec![0u8; length]; - rng.fill_bytes(&mut bytes); + OsRng.fill_bytes(&mut bytes); bytes.iter().map(|b| format!("{:02x}", b)).collect() } diff --git a/src/tools/mcp/auth.rs b/src/tools/mcp/auth.rs index bd7b203c..0b26b258 100644 --- a/src/tools/mcp/auth.rs +++ b/src/tools/mcp/auth.rs @@ -185,7 +185,7 @@ impl PkceChallenge { /// Generate a new PKCE challenge pair. pub fn generate() -> Self { let mut verifier_bytes = [0u8; 32]; - rand::thread_rng().fill_bytes(&mut verifier_bytes); + rand::rngs::OsRng.fill_bytes(&mut verifier_bytes); let verifier = URL_SAFE_NO_PAD.encode(verifier_bytes); let mut hasher = Sha256::new();