diff --git a/lean_client/Cargo.lock b/lean_client/Cargo.lock index 077f2b77..a87d63d4 100644 --- a/lean_client/Cargo.lock +++ b/lean_client/Cargo.lock @@ -6118,6 +6118,8 @@ dependencies = [ "fork_choice", "futures", "metrics", + "rand 0.10.1", + "rand_chacha 0.10.0", "rayon", "serde", "serde_yaml", diff --git a/lean_client/src/main.rs b/lean_client/src/main.rs index 4055511e..a240f090 100644 --- a/lean_client/src/main.rs +++ b/lean_client/src/main.rs @@ -723,7 +723,7 @@ async fn main() -> Result<()> { if keys_path.exists() { match ValidatorService::new_with_keys_and_aggregator( config.clone(), - num_validators, + &anchor_state.validators, keys_path, cpu_normal_executor.clone(), cpu_verify_executor.clone(), @@ -741,33 +741,18 @@ async fn main() -> Result<()> { Some(service) } Err(e) => { - warn!( - "Failed to load XMSS keys: {}, falling back to zero signatures", - e - ); - Some(ValidatorService::new_with_aggregator( - config, - num_validators, - cpu_normal_executor.clone(), - cpu_verify_executor.clone(), - cpu_snark_executor.clone(), - args.is_aggregator, - )) + error!("Failed to load XMSS keys: {}. Refusing to start.", e); + return Err(anyhow::anyhow!("failed to load XMSS keys: {e}")); } } } else { - warn!( - "Hash-sig key directory not found: {:?}, using zero signatures", + error!( + "Hash-sig key directory not found: {:?}. Refusing to start.", keys_path ); - Some(ValidatorService::new_with_aggregator( - config, - num_validators, - cpu_normal_executor.clone(), - cpu_verify_executor.clone(), - cpu_snark_executor.clone(), - args.is_aggregator, - )) + return Err(anyhow::anyhow!( + "hash-sig key directory not found: {keys_path:?}" + )); } } else { info!( diff --git a/lean_client/validator/Cargo.toml b/lean_client/validator/Cargo.toml index 01471cbf..3f0a660d 100644 --- a/lean_client/validator/Cargo.toml +++ b/lean_client/validator/Cargo.toml @@ -20,3 +20,7 @@ typenum = { workspace = true } xmss = { workspace = true } zeroize = { workspace = true } rayon = { workspace = true } + +[dev-dependencies] +rand = { workspace = true } +rand_chacha = { workspace = true } diff --git a/lean_client/validator/src/keys.rs b/lean_client/validator/src/keys.rs index 299c052c..904f69b3 100644 --- a/lean_client/validator/src/keys.rs +++ b/lean_client/validator/src/keys.rs @@ -5,6 +5,7 @@ use tracing::info; use anyhow::anyhow; use anyhow::{Context, Result, ensure}; +use containers::Validator; use xmss::{SecretKey, Signature}; /// Manages XMSS secret keys for validators (attestation + proposal keys per validator) @@ -99,6 +100,39 @@ impl KeyManager { Ok(()) } + /// Check a validator's loaded keys against its registry entry. + /// + /// The attestation and proposal keys must differ: a validator signs a proposal and an + /// attestation in the same slot, and XMSS is a stateful one-time signature, so one key + /// shared by both roles would be reused (leanSpec `ValidatorRegistry`). Each key must + /// also equal the registry's public key, or every signature fails verification at peers. + pub fn check_keys(&self, validator_index: u64, validator: &Validator) -> Result<()> { + let attestation_key = self + .attestation_keys + .get(&validator_index) + .ok_or_else(|| anyhow!("No attestation key loaded for validator {validator_index}"))? + .public_key(); + let proposal_key = self + .proposal_keys + .get(&validator_index) + .ok_or_else(|| anyhow!("No proposal key loaded for validator {validator_index}"))? + .public_key(); + + ensure!( + attestation_key != proposal_key, + "Attestation and proposal keys must differ for validator {validator_index}, but the same key is loaded for both" + ); + ensure!( + attestation_key == validator.attestation_pubkey, + "Attestation key for validator {validator_index} does not match the validator registry" + ); + ensure!( + proposal_key == validator.proposal_pubkey, + "Proposal key for validator {validator_index} does not match the validator registry" + ); + Ok(()) + } + /// Sign an attestation message with the validator's attestation secret key. pub fn sign_attestation( &self, @@ -140,6 +174,10 @@ impl KeyManager { #[cfg(test)] mod tests { + use rand::SeedableRng; + use rand_chacha::ChaCha8Rng; + use xmss::PublicKey; + use super::*; #[test] @@ -148,4 +186,68 @@ mod tests { let result = KeyManager::new("/nonexistent/path"); assert!(result.is_err()); } + + fn key_manager(attestation: SecretKey, proposal: SecretKey) -> KeyManager { + KeyManager { + attestation_keys: HashMap::from([(0, attestation)]), + proposal_keys: HashMap::from([(0, proposal)]), + keys_dir: PathBuf::new(), + } + } + + fn validator(attestation_pubkey: &PublicKey, proposal_pubkey: &PublicKey) -> Validator { + Validator { + attestation_pubkey: attestation_pubkey.clone(), + proposal_pubkey: proposal_pubkey.clone(), + index: 0, + } + } + + #[test] + fn check_keys_matches_registry_and_distinct_roles() { + let mut rng = ChaCha8Rng::seed_from_u64(7); + let (attestation_pk, attestation_sk) = SecretKey::generate_key_pair(&mut rng, 0, 4); + let (proposal_pk, proposal_sk) = SecretKey::generate_key_pair(&mut rng, 0, 4); + let keys = key_manager(attestation_sk, proposal_sk); + + keys.check_keys(0, &validator(&attestation_pk, &proposal_pk)) + .expect("matching keys"); + + let cases = [ + ( + "swapped roles", + validator(&proposal_pk, &attestation_pk), + "Attestation key for validator 0 does not match", + ), + ( + "proposal key not in registry", + validator(&attestation_pk, &attestation_pk), + "Proposal key for validator 0 does not match", + ), + ]; + for (name, registry_entry, want) in cases { + let err = keys + .check_keys(0, ®istry_entry) + .expect_err(name) + .to_string(); + assert!( + err.contains(want), + "{name}: error {err:?} does not contain {want:?}" + ); + } + } + + #[test] + fn check_keys_rejects_one_key_for_both_roles() { + let mut rng = ChaCha8Rng::seed_from_u64(7); + let (public_key, secret_key) = SecretKey::generate_key_pair(&mut rng, 0, 4); + let mut rng = ChaCha8Rng::seed_from_u64(7); + let (_, same_secret_key) = SecretKey::generate_key_pair(&mut rng, 0, 4); + + let err = key_manager(secret_key, same_secret_key) + .check_keys(0, &validator(&public_key, &public_key)) + .expect_err("one key for both roles") + .to_string(); + assert!(err.contains("must differ"), "error {err:?}"); + } } diff --git a/lean_client/validator/src/lib.rs b/lean_client/validator/src/lib.rs index 332fee8f..cbb8882b 100644 --- a/lean_client/validator/src/lib.rs +++ b/lean_client/validator/src/lib.rs @@ -181,8 +181,6 @@ pub fn snapshot_aggregation_inputs(store: &Store) -> Option /// Entry in an annotated_validators.yaml file. /// Each validator index has two entries: one for the attester key and one for the proposer key, /// distinguished by the filename containing "attester" or "proposer". -/// Single-key format (e.g. validator_N_sk.ssz with neither keyword) is also accepted and -/// uses the same file for both attestation and proposal. #[derive(Debug, Clone, Deserialize)] pub struct AnnotatedValidatorEntry { pub index: u64, @@ -219,9 +217,12 @@ impl ValidatorConfig { } else if entry.privkey_file.contains("proposer") { slot.1 = Some(entry.privkey_file.clone()); } else { - // Single-key format: same file for both keys. - slot.0 = Some(entry.privkey_file.clone()); - slot.1 = Some(entry.privkey_file.clone()); + // One key for both roles would reuse XMSS one-time state; leanSpec rejects it. + bail!( + "privkey_file {} for validator {} names neither an attester nor a proposer key", + entry.privkey_file, + entry.index + ); } } @@ -372,7 +373,7 @@ impl ValidatorService { pub fn new_with_keys( config: ValidatorConfig, - num_validators: u64, + validators: &Validators, keys_dir: impl AsRef, cpu_normal_executor: Arc, cpu_verify_executor: Arc, @@ -380,7 +381,7 @@ impl ValidatorService { ) -> Result> { Self::new_with_keys_and_aggregator( config, - num_validators, + validators, keys_dir, cpu_normal_executor, cpu_verify_executor, @@ -389,9 +390,11 @@ impl ValidatorService { ) } + /// Loads each assigned validator's keys and checks them against `validators`, the + /// anchor state's registry; see `KeyManager::check_keys`. pub fn new_with_keys_and_aggregator( config: ValidatorConfig, - num_validators: u64, + validators: &Validators, keys_dir: impl AsRef, cpu_normal_executor: Arc, cpu_verify_executor: Arc, @@ -411,7 +414,15 @@ impl ValidatorService { } else { key_manager.load_keys(idx)?; } + let validator = validators.get(idx).map_err(|_| { + format!( + "Validator {idx} is not in the registry of {} validators", + validators.len_u64() + ) + })?; + key_manager.check_keys(idx, validator)?; } + let num_validators = validators.len_u64(); info!( node_id = %config.node_id, diff --git a/lean_client/validator/tests/unit_tests.rs b/lean_client/validator/tests/unit_tests.rs index 4bef9827..4e29bebc 100644 --- a/lean_client/validator/tests/unit_tests.rs +++ b/lean_client/validator/tests/unit_tests.rs @@ -43,3 +43,24 @@ fn test_is_assigned() { assert!(!config.is_assigned(1)); assert!(!config.is_assigned(3)); } + +#[test] +fn test_load_from_file_rejects_single_key_entry() { + let path = std::env::temp_dir().join(format!( + "annotated_validators_single_key_{}.yaml", + std::process::id() + )); + std::fs::write( + &path, + "test_0:\n - index: 0\n pubkey_hex: \"0x00\"\n privkey_file: validator_0_sk.ssz\n", + ) + .unwrap(); + + let result = ValidatorConfig::load_from_file(&path, "test_0"); + std::fs::remove_file(&path).unwrap(); + let err = result.expect_err("single-key entry").to_string(); + assert!( + err.contains("names neither an attester nor a proposer key"), + "error {err:?}" + ); +} diff --git a/lean_client/xmss/src/secret_key.rs b/lean_client/xmss/src/secret_key.rs index a26b9dc9..96b25ab8 100644 --- a/lean_client/xmss/src/secret_key.rs +++ b/lean_client/xmss/src/secret_key.rs @@ -15,6 +15,11 @@ use crate::{PublicKey, Signature}; pub struct SecretKey(XmssSecretKey); impl SecretKey { + /// Returns the public key this secret key signs for. + pub fn public_key(&self) -> PublicKey { + PublicKey::from_lean(self.0.public_key()) + } + pub fn sign(&self, message: H256, epoch: u32) -> Result { if !self.0.activation_slots().contains(&epoch) { return Err(anyhow!("epoch {epoch} outside key activation window"));