diff --git a/common/eth2/src/lighthouse_vc/http_client.rs b/common/eth2/src/lighthouse_vc/http_client.rs index 07b9567a3a..c9082e9cc1 100644 --- a/common/eth2/src/lighthouse_vc/http_client.rs +++ b/common/eth2/src/lighthouse_vc/http_client.rs @@ -437,7 +437,7 @@ impl ValidatorClientHttpClient { pub async fn post_lighthouse_validators_web3signer( &self, request: &[Web3SignerValidatorRequest], - ) -> Result, Error> { + ) -> Result<(), Error> { let mut path = self.server.full.clone(); path.path_segments_mut() diff --git a/validator_client/src/http_api/keystores.rs b/validator_client/src/http_api/keystores.rs index 50cbde2084..9682028c63 100644 --- a/validator_client/src/http_api/keystores.rs +++ b/validator_client/src/http_api/keystores.rs @@ -27,24 +27,24 @@ pub fn list( let keystores = initialized_validators .validator_definitions() .iter() - .filter(|def| def.enabled && def.signing_definition.is_local_keystore()) + .filter(|def| def.enabled) .map(|def| { let validating_pubkey = def.voting_public_key.compress(); - let derivation_path = initialized_validators + let (derivation_path, readonly) = initialized_validators .signing_method(&validating_pubkey) - .and_then(|signing_method| match *signing_method { + .map_or((None, None), |signing_method| match *signing_method { SigningMethod::LocalKeystore { ref voting_keystore, .. - } => voting_keystore.path(), - SigningMethod::Web3Signer { .. } => None, + } => (voting_keystore.path(), None), + SigningMethod::Web3Signer { .. } => (None, Some(true)), }); SingleKeystoreResponse { validating_pubkey, derivation_path, - readonly: None, + readonly, } }) .collect::>(); @@ -59,6 +59,15 @@ pub fn import( runtime: Weak, log: Logger, ) -> Result { + // Check request validity. This is the only cases in which we should return a 4xx code. + if request.keystores.len() != request.passwords.len() { + return Err(custom_bad_request(format!( + "mismatched numbers of keystores ({}) and passwords ({})", + request.keystores.len(), + request.passwords.len(), + ))); + } + info!( log, "Importing keystores via standard HTTP API"; @@ -98,7 +107,6 @@ pub fn import( // Import each keystore. Some keystores may fail to be imported, so we record a status for each. let mut statuses = Vec::with_capacity(request.keystores.len()); - // FIXME(sproul): check and test different length keystores vs passwords for (keystore, password) in request .keystores .into_iter() @@ -157,6 +165,15 @@ fn import_single_keystore( return Ok(ImportKeystoreStatus::Duplicate); } + // Check that the password is correct. + // In future we should re-structure to avoid the double decryption here. It's not as simple + // as removing this check because `add_validator_keystore` will break if provided with an + // invalid validator definition (`update_validators` will get stuck trying to decrypt with the + // wrong password indefinitely). + keystore + .decrypt_keypair(password.as_ref()) + .map_err(|e| format!("incorrect password: {:?}", e))?; + let validator_dir = ValidatorDirBuilder::new(validator_dir_path) .voting_keystore(keystore, password.as_ref()) .store_withdrawal_keystore(false) diff --git a/validator_client/src/http_api/tests.rs b/validator_client/src/http_api/tests.rs index 97f8cf88d3..dcb4ac2d85 100644 --- a/validator_client/src/http_api/tests.rs +++ b/validator_client/src/http_api/tests.rs @@ -460,7 +460,7 @@ impl ApiTester { self.client .post_lighthouse_validators_web3signer(&request) .await - .unwrap_err(); + .unwrap(); assert_eq!(self.vals_total(), initial_vals + s.count); if s.enabled { diff --git a/validator_client/src/http_api/tests/keystores.rs b/validator_client/src/http_api/tests/keystores.rs index 7d2c74a815..6a707901d6 100644 --- a/validator_client/src/http_api/tests/keystores.rs +++ b/validator_client/src/http_api/tests/keystores.rs @@ -1,6 +1,9 @@ use super::*; use account_utils::random_password_string; -use eth2::lighthouse_vc::{http_client::ValidatorClientHttpClient as HttpClient, std_types::*}; +use eth2::lighthouse_vc::{ + http_client::ValidatorClientHttpClient as HttpClient, std_types::*, + types::Web3SignerValidatorRequest, +}; use eth2_keystore::Keystore; use itertools::Itertools; use rand::{rngs::SmallRng, Rng, SeedableRng}; @@ -15,6 +18,27 @@ fn new_keystore(password: ZeroizeString) -> Keystore { .unwrap() } +fn web3_signer_url() -> String { + "http://localhost:1/this-url-hopefully-doesnt-exist".into() +} + +fn new_web3signer_validator() -> (Keypair, Web3SignerValidatorRequest) { + let keypair = Keypair::random(); + let pk = keypair.pk.clone(); + ( + keypair, + Web3SignerValidatorRequest { + enable: true, + description: "".into(), + graffiti: None, + voting_public_key: pk, + url: web3_signer_url(), + root_certificate_path: None, + request_timeout_ms: None, + }, + ) +} + fn run_test(f: F) where F: FnOnce(ApiTester) -> V, @@ -62,13 +86,13 @@ fn check_get_response<'a>( } } -fn check_import_response<'a>( +fn check_import_response( response: &ImportKeystoresResponse, expected_statuses: impl IntoIterator, ) { for (status, expected_status) in response.data.iter().zip_eq(expected_statuses) { assert_eq!( - status.status, expected_status, + expected_status, status.status, "message: {:?}", status.message ); @@ -216,13 +240,159 @@ fn import_some_duplicate_keystores() { }) } -// FIXME(sproul): finish these test stubs -// FIXME(sproul): add test involving web3signer validators #[test] -fn import_keystores_wrong_password_all() {} +fn import_wrong_number_of_passwords() { + run_test(|tester| async move { + let password = random_password_string(); + let keystores = (0..3) + .map(|_| new_keystore(password.clone())) + .collect::>(); + + let err = tester + .client + .post_keystores(&ImportKeystoresRequest { + keystores: keystores.clone(), + passwords: vec![password.clone()], + slashing_protection: None, + }) + .await + .unwrap_err(); + assert_eq!(err.status().unwrap(), 400); + }) +} #[test] -fn import_keystores_wrong_password_some() {} +fn get_web3_signer_keystores() { + run_test(|tester| async move { + let num_local = 3; + let num_remote = 2; + + // Add some local validators. + let password = random_password_string(); + let keystores = (0..num_local) + .map(|_| new_keystore(password.clone())) + .collect::>(); + + let import_res = tester + .client + .post_keystores(&ImportKeystoresRequest { + keystores: keystores.clone(), + passwords: vec![password.clone(); keystores.len()], + slashing_protection: None, + }) + .await + .unwrap(); + + // All keystores should be imported. + check_import_response(&import_res, all_imported(&keystores)); + + // Add some web3signer validators. + let remote_vals = (0..num_remote) + .map(|_| new_web3signer_validator().1) + .collect::>(); + + tester + .client + .post_lighthouse_validators_web3signer(&remote_vals) + .await + .unwrap(); + + // Check that both local and remote validators are returned. + let get_res = tester.client.get_keystores().await.unwrap(); + + let expected_responses = keystores + .iter() + .map(|local_keystore| SingleKeystoreResponse { + validating_pubkey: keystore_pubkey(local_keystore), + derivation_path: local_keystore.path(), + readonly: None, + }) + .chain(remote_vals.iter().map(|remote_val| SingleKeystoreResponse { + validating_pubkey: remote_val.voting_public_key.compress(), + derivation_path: None, + readonly: Some(true), + })) + .collect::>(); + + for response in expected_responses { + assert!(get_res.data.contains(&response), "{:?}", response); + } + }) +} + +#[test] +fn import_keystores_wrong_password() { + run_test(|tester| async move { + let num_keystores = 4; + let (keystores, correct_passwords): (Vec<_>, Vec<_>) = (0..num_keystores) + .map(|_| { + let password = random_password_string(); + (new_keystore(password.clone()), password) + }) + .unzip(); + + // First import with some incorrect passwords. + let incorrect_passwords = (0..num_keystores) + .map(|i| { + if i % 2 == 0 { + random_password_string() + } else { + correct_passwords[i].clone() + } + }) + .collect::>(); + + let import_res = tester + .client + .post_keystores(&ImportKeystoresRequest { + keystores: keystores.clone(), + passwords: incorrect_passwords.clone(), + slashing_protection: None, + }) + .await + .unwrap(); + + let expected_statuses = (0..num_keystores).map(|i| { + if i % 2 == 0 { + ImportKeystoreStatus::Error + } else { + ImportKeystoreStatus::Imported + } + }); + check_import_response(&import_res, expected_statuses); + + // Import again with the correct passwords and check that the statuses are as expected. + let correct_import_req = ImportKeystoresRequest { + keystores: keystores.clone(), + passwords: correct_passwords.clone(), + slashing_protection: None, + }; + let import_res = tester + .client + .post_keystores(&correct_import_req) + .await + .unwrap(); + let expected_statuses = (0..num_keystores).map(|i| { + if i % 2 == 0 { + ImportKeystoreStatus::Imported + } else { + ImportKeystoreStatus::Duplicate + } + }); + check_import_response(&import_res, expected_statuses); + + // Import one final time, at which point all keys should be duplicates. + let import_res = tester + .client + .post_keystores(&correct_import_req) + .await + .unwrap(); + check_import_response( + &import_res, + (0..num_keystores).map(|_| ImportKeystoreStatus::Duplicate), + ); + }); +} #[test] fn import_keystores_full_slashing_protection() {} @@ -237,10 +407,7 @@ fn delete_some_keystores() {} fn delete_all_keystores() {} #[test] -fn delete_same_keystores_twice() {} - -#[test] -fn delete_some_keystores_twice() { +fn delete_keystores_twice() { run_test(|tester| async move { let password = random_password_string(); let keystores = (0..2) @@ -286,9 +453,6 @@ fn delete_nonexistent_keystores() { }) } -#[test] -fn delete_some_nonexistent_keystores() {} - fn make_attestation(source_epoch: u64, target_epoch: u64) -> Attestation { Attestation { aggregation_bits: BitList::with_capacity( @@ -360,19 +524,10 @@ fn delete_concurrent_with_signing() { let handle = runtime.spawn(async move { for j in 0..num_attestations { let mut att = make_attestation(j, j + 1); - for (validator_id, public_key) in thread_pubkeys.iter().enumerate() { - let res = validator_store + for (_validator_id, public_key) in thread_pubkeys.iter().enumerate() { + let _ = validator_store .sign_attestation(*public_key, 0, &mut att, Epoch::new(j + 1)) .await; - - println!( - "attestation {}->{} for validator t{}/v{}: {:?}", - j, - j + 1, - thread_index, - validator_id, - res - ); } } });