diff --git a/pkg/tbtc/signer/Cargo.toml b/pkg/tbtc/signer/Cargo.toml index 85ae957305..29e9ff10e2 100644 --- a/pkg/tbtc/signer/Cargo.toml +++ b/pkg/tbtc/signer/Cargo.toml @@ -27,7 +27,7 @@ frost-core = { version = "=3.0.0", default-features = false } chacha20poly1305 = "0.10" rand_chacha = "0.3" libc = "0.2" -zeroize = { version = "1.8", default-features = false, features = ["serde"] } +zeroize = { version = "1.8", default-features = false, features = ["alloc", "serde"] } bitcoin = "0.32" [dev-dependencies] diff --git a/pkg/tbtc/signer/src/api.rs b/pkg/tbtc/signer/src/api.rs index 5ee7856f10..19c11863b5 100644 --- a/pkg/tbtc/signer/src/api.rs +++ b/pkg/tbtc/signer/src/api.rs @@ -1,4 +1,46 @@ +use std::fmt; + use serde::{Deserialize, Serialize}; +use zeroize::{Zeroize, ZeroizeOnDrop, Zeroizing}; + +/// A hex-encoded secret whose owned Rust allocation is wiped on drop and whose +/// `Debug` representation never exposes its contents. Serde remains transparent +/// so the C-ABI JSON contract continues to carry an ordinary string. +#[derive(Clone, Default, Deserialize, Eq, PartialEq, Serialize)] +#[serde(transparent)] +pub struct SecretHex(Zeroizing); + +impl SecretHex { + pub fn new(value: String) -> Self { + Self(Zeroizing::new(value)) + } + + /// Borrows the secret for the narrow decode/serialization boundary without + /// creating another unmanaged `String` allocation. + pub fn expose_secret(&self) -> &str { + self.0.as_str() + } +} + +impl From for SecretHex { + fn from(value: String) -> Self { + Self::new(value) + } +} + +impl fmt::Debug for SecretHex { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str("") + } +} + +impl Zeroize for SecretHex { + fn zeroize(&mut self) { + self.0.zeroize(); + } +} + +impl ZeroizeOnDrop for SecretHex {} #[derive(Clone, Debug, Deserialize, PartialEq, Eq, Serialize)] pub struct DkgResult { @@ -20,7 +62,7 @@ pub struct DkgRound2Package { pub identifier: String, #[serde(default, skip_serializing_if = "Option::is_none")] pub sender_identifier: Option, - pub package_hex: String, + pub package_hex: SecretHex, } #[derive(Clone, Debug, Deserialize, PartialEq, Eq, Serialize)] @@ -32,26 +74,26 @@ pub struct DkgPart1Request { #[derive(Clone, Debug, Deserialize, PartialEq, Eq, Serialize)] pub struct DkgPart1Result { - pub secret_package_hex: String, + pub secret_package_hex: SecretHex, pub package: DkgRound1Package, } #[derive(Clone, Debug, Deserialize, PartialEq, Eq, Serialize)] pub struct DkgPart2Request { - pub secret_package_hex: String, + pub secret_package_hex: SecretHex, pub round1_packages: Vec, } #[derive(Clone, Debug, Deserialize, PartialEq, Eq, Serialize)] pub struct DkgPart2Result { - pub secret_package_hex: String, + pub secret_package_hex: SecretHex, pub packages: Vec, } #[derive(Clone, Debug, Deserialize, PartialEq, Eq, Serialize)] pub struct NativeFrostKeyPackage { pub identifier: String, - pub data_hex: String, + pub data_hex: SecretHex, } #[derive(Clone, Debug, Deserialize, PartialEq, Eq, Serialize)] @@ -62,7 +104,7 @@ pub struct NativeFrostPublicKeyPackage { #[derive(Clone, Debug, Deserialize, PartialEq, Eq, Serialize)] pub struct DkgPart3Request { - pub secret_package_hex: String, + pub secret_package_hex: SecretHex, pub round1_packages: Vec, pub round2_packages: Vec, } @@ -848,3 +890,172 @@ pub struct InitSignerConfigResult { pub config_fingerprint: String, pub configured_key_count: u32, } + +#[cfg(test)] +mod tests { + use super::*; + + const SECRET_SENTINEL: &str = + "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef"; + + fn secret_hex() -> SecretHex { + SecretHex::new(SECRET_SENTINEL.to_string()) + } + + fn round1_package() -> DkgRound1Package { + DkgRound1Package { + identifier: "round1-identifier".to_string(), + package_hex: "public-round1-package".to_string(), + } + } + + fn round2_package() -> DkgRound2Package { + DkgRound2Package { + identifier: "round2-recipient".to_string(), + sender_identifier: Some("round2-sender".to_string()), + package_hex: secret_hex(), + } + } + + fn public_key_package() -> NativeFrostPublicKeyPackage { + NativeFrostPublicKeyPackage { + verifying_shares: std::collections::BTreeMap::new(), + verifying_key: "public-verifying-key".to_string(), + } + } + + fn key_package() -> NativeFrostKeyPackage { + NativeFrostKeyPackage { + identifier: "key-package-identifier".to_string(), + data_hex: secret_hex(), + } + } + + #[test] + fn dkg_secret_hex_zeroizes_and_is_zeroize_on_drop() { + fn assert_zeroize_on_drop() {} + + assert_zeroize_on_drop::(); + + let mut secret = secret_hex(); + secret.zeroize(); + assert!(secret.expose_secret().is_empty()); + } + + #[test] + fn dkg_secret_fields_preserve_json_string_wire_shape() { + let encoded_holder = + serde_json::to_string(&secret_hex()).expect("secret holder serializes"); + assert_eq!(encoded_holder, format!("\"{SECRET_SENTINEL}\"")); + let decoded_holder: SecretHex = + serde_json::from_str(&encoded_holder).expect("secret holder deserializes"); + assert_eq!(decoded_holder.expose_secret(), SECRET_SENTINEL); + + let serialized_fields = [ + serde_json::to_value(DkgRound2Package { + identifier: "recipient".to_string(), + sender_identifier: Some("sender".to_string()), + package_hex: secret_hex(), + }) + .expect("round2 package serializes")["package_hex"] + .clone(), + serde_json::to_value(DkgPart1Result { + secret_package_hex: secret_hex(), + package: round1_package(), + }) + .expect("part1 result serializes")["secret_package_hex"] + .clone(), + serde_json::to_value(DkgPart2Request { + secret_package_hex: secret_hex(), + round1_packages: vec![round1_package()], + }) + .expect("part2 request serializes")["secret_package_hex"] + .clone(), + serde_json::to_value(DkgPart2Result { + secret_package_hex: secret_hex(), + packages: vec![round2_package()], + }) + .expect("part2 result serializes")["secret_package_hex"] + .clone(), + serde_json::to_value(DkgPart3Request { + secret_package_hex: secret_hex(), + round1_packages: vec![round1_package()], + round2_packages: vec![round2_package()], + }) + .expect("part3 request serializes")["secret_package_hex"] + .clone(), + serde_json::to_value(key_package()).expect("key package serializes")["data_hex"] + .clone(), + ]; + + for serialized_field in serialized_fields { + assert_eq!(serialized_field, serde_json::json!(SECRET_SENTINEL)); + } + } + + #[test] + fn dkg_secret_fields_redact_direct_and_nested_debug_output() { + let rendered = [ + format!("{:?}", round2_package()), + format!( + "{:?}", + DkgPart1Result { + secret_package_hex: secret_hex(), + package: round1_package(), + } + ), + format!( + "{:?}", + DkgPart2Request { + secret_package_hex: secret_hex(), + round1_packages: vec![round1_package()], + } + ), + format!( + "{:?}", + DkgPart2Result { + secret_package_hex: secret_hex(), + packages: vec![round2_package()], + } + ), + format!("{:?}", key_package()), + format!( + "{:?}", + DkgPart3Request { + secret_package_hex: secret_hex(), + round1_packages: vec![round1_package()], + round2_packages: vec![round2_package()], + } + ), + format!( + "{:?}", + DkgPart3Result { + key_package: key_package(), + public_key_package: public_key_package(), + } + ), + format!( + "{:?}", + PersistDistributedDkgKeyPackageRequest { + session_id: "debug-redaction-session".to_string(), + participant_identifier: 1, + threshold: 2, + participant_count: 3, + key_package: key_package(), + public_key_package: public_key_package(), + } + ), + ]; + + for rendered_value in rendered { + assert!( + !rendered_value.contains(SECRET_SENTINEL), + "Debug output leaked DKG secret material: {rendered_value}" + ); + assert!( + rendered_value.contains(""), + "Debug output did not mark DKG secret material as redacted: {rendered_value}" + ); + } + } +} diff --git a/pkg/tbtc/signer/src/engine/codec.rs b/pkg/tbtc/signer/src/engine/codec.rs index 9010a76db8..c1cddba959 100644 --- a/pkg/tbtc/signer/src/engine/codec.rs +++ b/pkg/tbtc/signer/src/engine/codec.rs @@ -229,7 +229,7 @@ pub(crate) fn decode_round2_package_map( let mut package_bytes = decode_hex_field( operation, &format!("round2_packages[{index}].package_hex"), - &package.package_hex, + package.package_hex.expose_secret(), )?; let round2_package_result = frost::keys::dkg::round2::Package::deserialize(&package_bytes); package_bytes.zeroize(); diff --git a/pkg/tbtc/signer/src/engine/dkg.rs b/pkg/tbtc/signer/src/engine/dkg.rs index d2d8e2e0ff..5479767bb8 100644 --- a/pkg/tbtc/signer/src/engine/dkg.rs +++ b/pkg/tbtc/signer/src/engine/dkg.rs @@ -16,10 +16,10 @@ pub fn persist_distributed_dkg_key_package( mut request: PersistDistributedDkgKeyPackageRequest, ) -> Result { const OP: &str = "persist_distributed_dkg_key_package"; - // data_hex is the serialized SECRET signing share. Move it into a zeroizing holder - // BEFORE any fallible check (validation, admission, quarantine can all return first), - // so serde's owned String is wiped on EVERY return path rather than dropped un-wiped. - let data_hex = Zeroizing::new(std::mem::take(&mut request.key_package.data_hex)); + // data_hex is the serialized SECRET signing share. Move its redacting, + // zeroizing holder out BEFORE any fallible check so it is wiped on every + // return path and the rest of the request no longer retains it. + let data_hex = std::mem::take(&mut request.key_package.data_hex); validate_session_id(&request.session_id)?; // Gate BEFORE decoding or persisting any key material: this op writes signing // material to durable state that interactive signing trusts after restart, so @@ -102,7 +102,11 @@ pub fn persist_distributed_dkg_key_package( auto_quarantine_config.as_ref(), )?; - let key_package = decode_key_package(OP, &request.key_package.identifier, &data_hex)?; + let key_package = decode_key_package( + OP, + &request.key_package.identifier, + data_hex.expose_secret(), + )?; // The key package must belong to this participant AND be consistent with the // group public key package: matching identifier, embedded threshold, group diff --git a/pkg/tbtc/signer/src/engine/frost_ops.rs b/pkg/tbtc/signer/src/engine/frost_ops.rs index b1f0c95f61..0099db7297 100644 --- a/pkg/tbtc/signer/src/engine/frost_ops.rs +++ b/pkg/tbtc/signer/src/engine/frost_ops.rs @@ -46,7 +46,7 @@ pub fn dkg_part1(request: DkgPart1Request) -> Result Result Result Result Result Result> = OnceLock::new(); +// Loading can rewrite a legacy or stale encrypted envelope in place. A +// OnceLock serializes only the final in-memory installation, so it does not by +// itself prevent concurrent first callers from racing those fallible storage +// reads and migrations. Keep that entire path behind a process-local mutex +// that is deliberately separate from STATE_FILE_LOCK: the loader resolves the +// active path through STATE_FILE_LOCK and would deadlock if its slot guard were +// held here. +static ENGINE_STATE_INITIALIZATION_LOCK: Mutex<()> = Mutex::new(()); + pub(crate) static STATE_FILE_LOCK: OnceLock>> = OnceLock::new(); pub(crate) static STATE_PATH_OVERRIDE_WARNED: OnceLock<()> = OnceLock::new(); @@ -327,12 +336,50 @@ pub(crate) fn state() -> Result<&'static Mutex, EngineError> { ensure_state_file_lock()?; warn_disabled_policy_gates(); - if let Some(state) = ENGINE_STATE.get() { + initialize_engine_state_with_loader( + &ENGINE_STATE, + &ENGINE_STATE_INITIALIZATION_LOCK, + || {}, + load_engine_state_from_storage, + ) +} + +/// Installs the first engine state while serializing the complete fallible load +/// path, not just the final OnceLock write. +/// +/// `after_initial_miss` is a no-op in production and lets concurrency tests +/// deterministically place multiple callers past the optimistic fast path. +/// The loader runs under `initialization_lock`; a failed load leaves +/// `engine_state` unset so a later call can retry. +pub(crate) fn initialize_engine_state_with_loader<'state, AfterInitialMiss, Load>( + engine_state: &'state OnceLock>, + initialization_lock: &Mutex<()>, + after_initial_miss: AfterInitialMiss, + load: Load, +) -> Result<&'state Mutex, EngineError> +where + AfterInitialMiss: FnOnce(), + Load: FnOnce() -> Result, +{ + if let Some(state) = engine_state.get() { + return Ok(state); + } + + after_initial_miss(); + + // The mutex protects no data of its own. Recovering its guard after a + // panic is safe, and the second OnceLock check determines whether the + // previous caller completed installation before panicking. + let _initialization_guard = initialization_lock + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + + if let Some(state) = engine_state.get() { return Ok(state); } - let loaded_state = load_engine_state_from_storage()?; - Ok(ENGINE_STATE.get_or_init(|| Mutex::new(loaded_state))) + let loaded_state = load()?; + Ok(engine_state.get_or_init(|| Mutex::new(loaded_state))) } pub(crate) fn state_file_path() -> Result { diff --git a/pkg/tbtc/signer/src/engine/tests.rs b/pkg/tbtc/signer/src/engine/tests.rs index e1b2a78db8..a81fd77218 100644 --- a/pkg/tbtc/signer/src/engine/tests.rs +++ b/pkg/tbtc/signer/src/engine/tests.rs @@ -30,7 +30,7 @@ use std::{ #[derive(Clone, Debug)] struct GenerateNoncesAndCommitmentsRequest { key_package_identifier: String, - key_package_hex: String, + key_package_hex: SecretHex, } #[derive(Clone, Debug)] @@ -44,7 +44,7 @@ struct SignShareRequest { signing_package_hex: String, nonces_hex: String, key_package_identifier: String, - key_package_hex: String, + key_package_hex: SecretHex, } #[derive(Clone, Debug)] @@ -67,13 +67,13 @@ struct AggregateResult { fn generate_nonces_and_commitments( mut request: GenerateNoncesAndCommitmentsRequest, ) -> Result { - let key_package_hex = Zeroizing::new(std::mem::take(&mut request.key_package_hex)); + let key_package_hex = std::mem::take(&mut request.key_package_hex); enforce_provenance_gate()?; let key_package = decode_key_package( "GenerateNoncesAndCommitments", &request.key_package_identifier, - &key_package_hex, + key_package_hex.expose_secret(), )?; let mut rng = zeroizing_rng_from_os(); let (mut nonces, commitments) = frost::round1::commit(key_package.signing_share(), &mut rng); @@ -105,7 +105,7 @@ fn generate_nonces_and_commitments( fn sign_share(mut request: SignShareRequest) -> Result { let nonces_hex = Zeroizing::new(std::mem::take(&mut request.nonces_hex)); - let key_package_hex = Zeroizing::new(std::mem::take(&mut request.key_package_hex)); + let key_package_hex = std::mem::take(&mut request.key_package_hex); enforce_provenance_gate()?; let signing_package_bytes = decode_hex_field( @@ -125,7 +125,7 @@ fn sign_share(mut request: SignShareRequest) -> Result key_package, @@ -437,7 +437,7 @@ fn deterministic_interactive_dkg_fixture(seed: u8) -> InteractiveDkgFixture { sender_identifier: Some(frost_identifier_to_go_string( participant_identifiers[&sender_id], )), - package_hex: hex::encode(package.serialize().expect("round2 package")), + package_hex: hex::encode(package.serialize().expect("round2 package")).into(), }); } } @@ -468,7 +468,7 @@ fn deterministic_interactive_dkg_fixture(seed: u8) -> InteractiveDkgFixture { part3_requests.insert( id, DkgPart3Request { - secret_package_hex: hex::encode(secret_package_bytes), + secret_package_hex: hex::encode(secret_package_bytes).into(), round1_packages: round1_packages_for(id), round2_packages: round2_packages_by_recipient .get(&id) @@ -1125,7 +1125,8 @@ fn sample_distributed_dkg_native_material( member, crate::api::NativeFrostKeyPackage { identifier: frost_identifier_to_go_string(*key_package.identifier()), - data_hex: hex::encode(key_package.serialize().expect("serialize key package")), + data_hex: hex::encode(key_package.serialize().expect("serialize key package")) + .into(), }, ); } @@ -1663,7 +1664,7 @@ fn persist_distributed_dkg_key_package_rejects_signing_share_not_deriving_to_pub participant_count: 3, key_package: crate::api::NativeFrostKeyPackage { identifier: frost_identifier_to_go_string(*key_package_1.identifier()), - data_hex: hex::encode(corrupt_data), + data_hex: hex::encode(corrupt_data).into(), }, public_key_package: native_public, }) @@ -5356,6 +5357,115 @@ fn restart_reload_recovers_persisted_state_across_operation_types() { clear_state_storage_policy_overrides(); } +#[test] +fn first_engine_state_load_is_serialized_across_callers() { + let engine_state = std::sync::Arc::new(OnceLock::>::new()); + let initialization_lock = std::sync::Arc::new(Mutex::new(())); + let initial_miss_barrier = std::sync::Arc::new(std::sync::Barrier::new(2)); + let load_count = std::sync::Arc::new(std::sync::atomic::AtomicUsize::new(0)); + + let callers = (0..2) + .map(|_| { + let engine_state = std::sync::Arc::clone(&engine_state); + let initialization_lock = std::sync::Arc::clone(&initialization_lock); + let initial_miss_barrier = std::sync::Arc::clone(&initial_miss_barrier); + let load_count = std::sync::Arc::clone(&load_count); + + std::thread::spawn(move || { + let initialized = initialize_engine_state_with_loader( + &engine_state, + &initialization_lock, + || { + // Both callers must pass the optimistic OnceLock check + // before either may enter the serialized loader path. + initial_miss_barrier.wait(); + }, + || { + let invocation = + load_count.fetch_add(1, std::sync::atomic::Ordering::SeqCst); + Ok(EngineState { + refresh_epoch_counter: invocation as u64 + 41, + ..EngineState::default() + }) + }, + ) + .expect("concurrent initialization"); + + let state_pointer = std::ptr::from_ref(initialized) as usize; + let refresh_epoch_counter = initialized + .lock() + .expect("initialized engine state lock") + .refresh_epoch_counter; + (state_pointer, refresh_epoch_counter) + }) + }) + .collect::>(); + + let results = callers + .into_iter() + .map(|caller| caller.join().expect("initialization caller")) + .collect::>(); + + assert_eq!( + load_count.load(std::sync::atomic::Ordering::SeqCst), + 1, + "only one first caller may load or migrate persistent state" + ); + assert_eq!(results[0].0, results[1].0); + assert_eq!(results[0].1, 41); + assert_eq!(results[1].1, 41); +} + +#[test] +fn failed_engine_state_load_remains_retryable() { + let engine_state = OnceLock::>::new(); + let initialization_lock = Mutex::new(()); + let load_count = std::sync::atomic::AtomicUsize::new(0); + + let first_error = match initialize_engine_state_with_loader( + &engine_state, + &initialization_lock, + || {}, + || { + load_count.fetch_add(1, std::sync::atomic::Ordering::SeqCst); + Err(EngineError::Internal( + "intentional first-load failure".to_string(), + )) + }, + ) { + Ok(_) => panic!("failed loader must not initialize engine state"), + Err(error) => error, + }; + expect_internal_error_contains(first_error, "intentional first-load failure"); + assert!( + engine_state.get().is_none(), + "a fallible load must leave the OnceLock unset" + ); + + let initialized = initialize_engine_state_with_loader( + &engine_state, + &initialization_lock, + || {}, + || { + load_count.fetch_add(1, std::sync::atomic::Ordering::SeqCst); + Ok(EngineState { + refresh_epoch_counter: 73, + ..EngineState::default() + }) + }, + ) + .expect("retry after failed state load"); + + assert_eq!(load_count.load(std::sync::atomic::Ordering::SeqCst), 2); + assert_eq!( + initialized + .lock() + .expect("initialized engine state lock") + .refresh_epoch_counter, + 73 + ); +} + #[test] #[cfg(unix)] fn state_lock_rejects_multi_process_contention() { @@ -6749,7 +6859,8 @@ fn ensure_interactive_dkg_session( let mut frost_key_packages = BTreeMap::new(); for (id, key_package) in &native { let deserialized = frost::keys::KeyPackage::deserialize( - &hex::decode(&key_package.data_hex).expect("fixture key package hex decodes"), + &hex::decode(key_package.data_hex.expose_secret()) + .expect("fixture key package hex decodes"), ) .expect("fixture key package deserializes"); frost_key_packages.insert(*id, deserialized); @@ -13220,7 +13331,7 @@ fn verify_signature_share_tweaked_root_matches_aggregate() { .expect("member 1 tweaked Round2 share") .signature_share_hex; let share2 = sign_tweaked( - &key_packages[&2].data_hex, + key_packages[&2].data_hex.expose_secret(), &member2.nonces_hex, &signing_package_hex, ); @@ -13273,7 +13384,7 @@ fn verify_signature_share_tweaked_root_matches_aggregate() { ], ); let bogus_share2 = sign_tweaked( - &key_packages[&2].data_hex, + key_packages[&2].data_hex.expose_secret(), &bogus_member2.nonces_hex, &other_package_hex, ); diff --git a/pkg/tbtc/signer/src/lib.rs b/pkg/tbtc/signer/src/lib.rs index fdb7ded39d..cc90f231c3 100644 --- a/pkg/tbtc/signer/src/lib.rs +++ b/pkg/tbtc/signer/src/lib.rs @@ -647,7 +647,7 @@ mod tests { let result: DkgPart1Result = serde_json::from_slice(&payload).expect("part1 response decode"); assert_eq!(result.package.identifier, participant_identifiers[&id]); - assert!(!result.secret_package_hex.is_empty()); + assert!(!result.secret_package_hex.expose_secret().is_empty()); assert!(!result.package.package_hex.is_empty()); part1_results.insert(id, result); }