From 4a2bfb782114a294a89369da0b5cc1ff7e2127b0 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Thu, 4 Apr 2024 23:26:10 +0300 Subject: [PATCH 01/13] and_then to then remove expect move convert_rejection to utils remove signer from vc api --- beacon_node/http_api/src/lib.rs | 8 +- beacon_node/http_api/src/task_spawner.rs | 19 +- common/warp_utils/src/reject.rs | 20 +- validator_client/src/http_api/mod.rs | 263 +++++++++-------------- 4 files changed, 125 insertions(+), 185 deletions(-) diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index 9e6022dc954..b7756905142 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -97,7 +97,7 @@ use warp::hyper::Body; use warp::sse::Event; use warp::Reply; use warp::{http::Response, Filter, Rejection}; -use warp_utils::{query::multi_key_query, uor::UnifyingOrFilter}; +use warp_utils::{query::multi_key_query, reject::convert_rejection, uor::UnifyingOrFilter}; const API_PREFIX: &str = "eth"; @@ -1781,7 +1781,7 @@ pub fn serve( ) .await .map(|()| warp::reply::json(&())); - task_spawner::convert_rejection(result).await + convert_rejection(result).await }, ); @@ -3720,12 +3720,12 @@ pub fn serve( .await; if initial_result.is_err() { - return task_spawner::convert_rejection(initial_result).await; + return convert_rejection(initial_result).await; } // Await a response from the builder without blocking a // `BeaconProcessor` worker. - task_spawner::convert_rejection(rx.await.unwrap_or_else(|_| { + convert_rejection(rx.await.unwrap_or_else(|_| { Ok(warp::reply::with_status( warp::reply::json(&"No response from channel"), eth2::StatusCode::INTERNAL_SERVER_ERROR, diff --git a/beacon_node/http_api/src/task_spawner.rs b/beacon_node/http_api/src/task_spawner.rs index cfee5e01ca0..a679b294f65 100644 --- a/beacon_node/http_api/src/task_spawner.rs +++ b/beacon_node/http_api/src/task_spawner.rs @@ -4,6 +4,7 @@ use std::future::Future; use tokio::sync::{mpsc::error::TrySendError, oneshot}; use types::EthSpec; use warp::reply::{Reply, Response}; +use warp_utils::reject::convert_rejection; /// Maps a request to a queue in the `BeaconProcessor`. #[derive(Clone, Copy)] @@ -35,24 +36,6 @@ pub struct TaskSpawner { beacon_processor_send: Option>, } -/// Convert a warp `Rejection` into a `Response`. -/// -/// This function should *always* be used to convert rejections into responses. This prevents warp -/// from trying to backtrack in strange ways. See: https://github.com/sigp/lighthouse/issues/3404 -pub async fn convert_rejection(res: Result) -> Response { - match res { - Ok(response) => response.into_response(), - Err(e) => match warp_utils::reject::handle_rejection(e).await { - Ok(reply) => reply.into_response(), - Err(_) => warp::reply::with_status( - warp::reply::json(&"unhandled error"), - eth2::StatusCode::INTERNAL_SERVER_ERROR, - ) - .into_response(), - }, - } -} - impl TaskSpawner { pub fn new(beacon_processor_send: Option>) -> Self { Self { diff --git a/common/warp_utils/src/reject.rs b/common/warp_utils/src/reject.rs index b6bb5ace3d0..feedd93f9a6 100644 --- a/common/warp_utils/src/reject.rs +++ b/common/warp_utils/src/reject.rs @@ -2,7 +2,7 @@ use eth2::types::{ErrorMessage, Failure, IndexedErrorMessage}; use std::convert::Infallible; use std::error::Error; use std::fmt; -use warp::{http::StatusCode, reject::Reject}; +use warp::{http::StatusCode, reject::Reject, reply::Response, Reply}; #[derive(Debug)] pub struct ServerSentEventError(pub String); @@ -243,3 +243,21 @@ pub async fn handle_rejection(err: warp::Rejection) -> Result(res: Result) -> Response { + match res { + Ok(response) => response.into_response(), + Err(e) => match handle_rejection(e).await { + Ok(reply) => reply.into_response(), + Err(_) => warp::reply::with_status( + warp::reply::json(&"unhandled error"), + eth2::StatusCode::INTERNAL_SERVER_ERROR, + ) + .into_response(), + }, + } +} diff --git a/validator_client/src/http_api/mod.rs b/validator_client/src/http_api/mod.rs index a4480195e59..5e6cf996011 100644 --- a/validator_client/src/http_api/mod.rs +++ b/validator_client/src/http_api/mod.rs @@ -48,12 +48,13 @@ use validator_dir::Builder as ValidatorDirBuilder; use warp::{ http::{ header::{HeaderValue, CONTENT_TYPE}, - response::Response, StatusCode, }, + reply::Response, sse::Event, Filter, }; +use warp_utils::reject::convert_rejection; #[derive(Debug)] pub enum Error { @@ -176,9 +177,6 @@ pub fn serve( } }; - let signer = ctx.api_secret.signer(); - let signer = warp::any().map(move || signer.clone()); - let inner_validator_store = ctx.validator_store.clone(); let validator_store_filter = warp::any() .map(move || inner_validator_store.clone()) @@ -270,9 +268,8 @@ pub fn serve( let get_node_version = warp::path("lighthouse") .and(warp::path("version")) .and(warp::path::end()) - .and(signer.clone()) - .and_then(|signer| { - blocking_signed_json_task(signer, move || { + .then(|| { + blocking_signed_json_task(move || { Ok(api_types::GenericResponse::from(api_types::VersionData { version: version_with_platform(), })) @@ -283,9 +280,8 @@ pub fn serve( let get_lighthouse_health = warp::path("lighthouse") .and(warp::path("health")) .and(warp::path::end()) - .and(signer.clone()) - .and_then(|signer| { - blocking_signed_json_task(signer, move || { + .then(|| { + blocking_signed_json_task(move || { eth2::lighthouse::Health::observe() .map(api_types::GenericResponse::from) .map_err(warp_utils::reject::custom_bad_request) @@ -297,9 +293,8 @@ pub fn serve( .and(warp::path("spec")) .and(warp::path::end()) .and(spec_filter.clone()) - .and(signer.clone()) - .and_then(|spec: Arc<_>, signer| { - blocking_signed_json_task(signer, move || { + .then(|spec: Arc<_>| { + blocking_signed_json_task(move || { let config = ConfigAndPreset::from_chain_spec::(&spec, None); Ok(api_types::GenericResponse::from(config)) }) @@ -310,9 +305,8 @@ pub fn serve( .and(warp::path("validators")) .and(warp::path::end()) .and(validator_store_filter.clone()) - .and(signer.clone()) - .and_then(|validator_store: Arc>, signer| { - blocking_signed_json_task(signer, move || { + .then(|validator_store: Arc>| { + blocking_signed_json_task(move || { let validators = validator_store .initialized_validators() .read() @@ -335,10 +329,9 @@ pub fn serve( .and(warp::path::param::()) .and(warp::path::end()) .and(validator_store_filter.clone()) - .and(signer.clone()) - .and_then( - |validator_pubkey: PublicKey, validator_store: Arc>, signer| { - blocking_signed_json_task(signer, move || { + .then( + |validator_pubkey: PublicKey, validator_store: Arc>| { + blocking_signed_json_task(move || { let validator = validator_store .initialized_validators() .read() @@ -370,9 +363,8 @@ pub fn serve( .and(system_info_filter) .and(app_start_filter) .and(validator_dir_filter.clone()) - .and(signer.clone()) - .and_then(|sysinfo, app_start: std::time::Instant, val_dir, signer| { - blocking_signed_json_task(signer, move || { + .then(|sysinfo, app_start: std::time::Instant, val_dir| { + blocking_signed_json_task(move || { let app_uptime = app_start.elapsed().as_secs(); Ok(api_types::GenericResponse::from(observe_system_health_vc( sysinfo, val_dir, app_uptime, @@ -387,15 +379,13 @@ pub fn serve( .and(validator_store_filter.clone()) .and(graffiti_file_filter.clone()) .and(graffiti_flag_filter) - .and(signer.clone()) .and(log_filter.clone()) - .and_then( + .then( |validator_store: Arc>, graffiti_file: Option, graffiti_flag: Option, - signer, log| { - blocking_signed_json_task(signer, move || { + blocking_signed_json_task(move || { let mut result = HashMap::new(); for (key, graffiti_definition) in validator_store .initialized_validators() @@ -425,17 +415,15 @@ pub fn serve( .and(secrets_dir_filter.clone()) .and(validator_store_filter.clone()) .and(spec_filter.clone()) - .and(signer.clone()) .and(task_executor_filter.clone()) - .and_then( + .then( move |body: Vec, validator_dir: PathBuf, secrets_dir: PathBuf, validator_store: Arc>, spec: Arc, - signer, task_executor: TaskExecutor| { - blocking_signed_json_task(signer, move || { + blocking_signed_json_task(move || { let secrets_dir = store_passwords_in_secrets_dir.then_some(secrets_dir); if let Some(handle) = task_executor.handle() { let (validators, mnemonic) = @@ -472,17 +460,15 @@ pub fn serve( .and(secrets_dir_filter.clone()) .and(validator_store_filter.clone()) .and(spec_filter) - .and(signer.clone()) .and(task_executor_filter.clone()) - .and_then( + .then( move |body: api_types::CreateValidatorsMnemonicRequest, validator_dir: PathBuf, secrets_dir: PathBuf, validator_store: Arc>, spec: Arc, - signer, task_executor: TaskExecutor| { - blocking_signed_json_task(signer, move || { + blocking_signed_json_task(move || { let secrets_dir = store_passwords_in_secrets_dir.then_some(secrets_dir); if let Some(handle) = task_executor.handle() { let mnemonic = @@ -521,16 +507,14 @@ pub fn serve( .and(validator_dir_filter.clone()) .and(secrets_dir_filter.clone()) .and(validator_store_filter.clone()) - .and(signer.clone()) .and(task_executor_filter.clone()) - .and_then( + .then( move |body: api_types::KeystoreValidatorsPostRequest, validator_dir: PathBuf, secrets_dir: PathBuf, validator_store: Arc>, - signer, task_executor: TaskExecutor| { - blocking_signed_json_task(signer, move || { + blocking_signed_json_task(move || { // Check to ensure the password is correct. let keypair = body .keystore @@ -611,14 +595,12 @@ pub fn serve( .and(warp::path::end()) .and(warp::body::json()) .and(validator_store_filter.clone()) - .and(signer.clone()) .and(task_executor_filter.clone()) - .and_then( + .then( |body: Vec, validator_store: Arc>, - signer, task_executor: TaskExecutor| { - blocking_signed_json_task(signer, move || { + blocking_signed_json_task(move || { if let Some(handle) = task_executor.handle() { let web3signers: Vec = body .into_iter() @@ -666,16 +648,14 @@ pub fn serve( .and(warp::body::json()) .and(validator_store_filter.clone()) .and(graffiti_file_filter.clone()) - .and(signer.clone()) .and(task_executor_filter.clone()) - .and_then( + .then( |validator_pubkey: PublicKey, body: api_types::ValidatorPatchRequest, validator_store: Arc>, graffiti_file: Option, - signer, task_executor: TaskExecutor| { - blocking_signed_json_task(signer, move || { + blocking_signed_json_task(move || { if body.graffiti.is_some() && graffiti_file.is_some() { return Err(warp_utils::reject::custom_bad_request( "Unable to update graffiti as the \"--graffiti-file\" flag is set" @@ -784,10 +764,9 @@ pub fn serve( // GET /lighthouse/auth let get_auth = warp::path("lighthouse").and(warp::path("auth").and(warp::path::end())); let get_auth = get_auth - .and(signer.clone()) .and(api_token_path_filter) - .and_then(|signer, token_path: PathBuf| { - blocking_signed_json_task(signer, move || { + .then(|token_path: PathBuf| { + blocking_signed_json_task(move || { Ok(AuthResponse { token_path: token_path.display().to_string(), }) @@ -799,23 +778,20 @@ pub fn serve( .and(warp::path("keystores")) .and(warp::path::end()) .and(warp::body::json()) - .and(signer.clone()) .and(validator_store_filter.clone()) .and(task_executor_filter.clone()) .and(log_filter.clone()) - .and_then( - move |request, signer, validator_store, task_executor, log| { - blocking_signed_json_task(signer, move || { - if allow_keystore_export { - keystores::export(request, validator_store, task_executor, log) - } else { - Err(warp_utils::reject::custom_bad_request( - "keystore export is disabled".to_string(), - )) - } - }) - }, - ); + .then(move |request, validator_store, task_executor, log| { + blocking_signed_json_task(move || { + if allow_keystore_export { + keystores::export(request, validator_store, task_executor, log) + } else { + Err(warp_utils::reject::custom_bad_request( + "keystore export is disabled".to_string(), + )) + } + }) + }); // Standard key-manager endpoints. let eth_v1 = warp::path("eth").and(warp::path("v1")); @@ -829,10 +805,9 @@ pub fn serve( .and(warp::path("feerecipient")) .and(warp::path::end()) .and(validator_store_filter.clone()) - .and(signer.clone()) - .and_then( - |validator_pubkey: PublicKey, validator_store: Arc>, signer| { - blocking_signed_json_task(signer, move || { + .then( + |validator_pubkey: PublicKey, validator_store: Arc>| { + blocking_signed_json_task(move || { if validator_store .initialized_validators() .read() @@ -869,13 +844,11 @@ pub fn serve( .and(warp::body::json()) .and(warp::path::end()) .and(validator_store_filter.clone()) - .and(signer.clone()) - .and_then( + .then( |validator_pubkey: PublicKey, request: api_types::UpdateFeeRecipientRequest, - validator_store: Arc>, - signer| { - blocking_signed_json_task(signer, move || { + validator_store: Arc>| { + blocking_signed_json_task(move || { if validator_store .initialized_validators() .read() @@ -909,10 +882,9 @@ pub fn serve( .and(warp::path("feerecipient")) .and(warp::path::end()) .and(validator_store_filter.clone()) - .and(signer.clone()) - .and_then( - |validator_pubkey: PublicKey, validator_store: Arc>, signer| { - blocking_signed_json_task(signer, move || { + .then( + |validator_pubkey: PublicKey, validator_store: Arc>| { + blocking_signed_json_task(move || { if validator_store .initialized_validators() .read() @@ -946,10 +918,9 @@ pub fn serve( .and(warp::path("gas_limit")) .and(warp::path::end()) .and(validator_store_filter.clone()) - .and(signer.clone()) - .and_then( - |validator_pubkey: PublicKey, validator_store: Arc>, signer| { - blocking_signed_json_task(signer, move || { + .then( + |validator_pubkey: PublicKey, validator_store: Arc>| { + blocking_signed_json_task(move || { if validator_store .initialized_validators() .read() @@ -978,13 +949,11 @@ pub fn serve( .and(warp::body::json()) .and(warp::path::end()) .and(validator_store_filter.clone()) - .and(signer.clone()) - .and_then( + .then( |validator_pubkey: PublicKey, request: api_types::UpdateGasLimitRequest, - validator_store: Arc>, - signer| { - blocking_signed_json_task(signer, move || { + validator_store: Arc>| { + blocking_signed_json_task(move || { if validator_store .initialized_validators() .read() @@ -1018,10 +987,9 @@ pub fn serve( .and(warp::path("gas_limit")) .and(warp::path::end()) .and(validator_store_filter.clone()) - .and(signer.clone()) - .and_then( - |validator_pubkey: PublicKey, validator_store: Arc>, signer| { - blocking_signed_json_task(signer, move || { + .then( + |validator_pubkey: PublicKey, validator_store: Arc>| { + blocking_signed_json_task(move || { if validator_store .initialized_validators() .read() @@ -1058,17 +1026,15 @@ pub fn serve( .and(validator_store_filter.clone()) .and(slot_clock_filter) .and(log_filter.clone()) - .and(signer.clone()) .and(task_executor_filter.clone()) - .and_then( + .then( |pubkey: PublicKey, query: api_types::VoluntaryExitQuery, validator_store: Arc>, slot_clock: T, log, - signer, task_executor: TaskExecutor| { - blocking_signed_json_task(signer, move || { + blocking_signed_json_task(move || { if let Some(handle) = task_executor.handle() { let signed_voluntary_exit = handle.block_on(create_signed_voluntary_exit( @@ -1096,13 +1062,11 @@ pub fn serve( .and(warp::path::end()) .and(validator_store_filter.clone()) .and(graffiti_flag_filter) - .and(signer.clone()) - .and_then( + .then( |pubkey: PublicKey, validator_store: Arc>, - graffiti_flag: Option, - signer| { - blocking_signed_json_task(signer, move || { + graffiti_flag: Option| { + blocking_signed_json_task(move || { let graffiti = get_graffiti(pubkey.clone(), validator_store, graffiti_flag)?; Ok(GenericResponse::from(GetGraffitiResponse { pubkey: pubkey.into(), @@ -1121,14 +1085,12 @@ pub fn serve( .and(warp::path::end()) .and(validator_store_filter.clone()) .and(graffiti_file_filter.clone()) - .and(signer.clone()) - .and_then( + .then( |pubkey: PublicKey, query: SetGraffitiRequest, validator_store: Arc>, - graffiti_file: Option, - signer| { - blocking_signed_json_task(signer, move || { + graffiti_file: Option| { + blocking_signed_json_task(move || { if graffiti_file.is_some() { return Err(warp_utils::reject::invalid_auth( "Unable to update graffiti as the \"--graffiti-file\" flag is set" @@ -1149,13 +1111,11 @@ pub fn serve( .and(warp::path::end()) .and(validator_store_filter.clone()) .and(graffiti_file_filter.clone()) - .and(signer.clone()) - .and_then( + .then( |pubkey: PublicKey, validator_store: Arc>, - graffiti_file: Option, - signer| { - blocking_signed_json_task(signer, move || { + graffiti_file: Option| { + blocking_signed_json_task(move || { if graffiti_file.is_some() { return Err(warp_utils::reject::invalid_auth( "Unable to delete graffiti as the \"--graffiti-file\" flag is set" @@ -1169,32 +1129,24 @@ pub fn serve( .map(|reply| warp::reply::with_status(reply, warp::http::StatusCode::NO_CONTENT)); // GET /eth/v1/keystores - let get_std_keystores = std_keystores - .and(signer.clone()) - .and(validator_store_filter.clone()) - .and_then(|signer, validator_store: Arc>| { - blocking_signed_json_task(signer, move || Ok(keystores::list(validator_store))) - }); + let get_std_keystores = std_keystores.and(validator_store_filter.clone()).then( + |validator_store: Arc>| { + blocking_signed_json_task(move || Ok(keystores::list(validator_store))) + }, + ); // POST /eth/v1/keystores let post_std_keystores = std_keystores .and(warp::body::json()) - .and(signer.clone()) .and(validator_dir_filter) .and(secrets_dir_filter) .and(validator_store_filter.clone()) .and(task_executor_filter.clone()) .and(log_filter.clone()) - .and_then( - move |request, - signer, - validator_dir, - secrets_dir, - validator_store, - task_executor, - log| { + .then( + move |request, validator_dir, secrets_dir, validator_store, task_executor, log| { let secrets_dir = store_passwords_in_secrets_dir.then_some(secrets_dir); - blocking_signed_json_task(signer, move || { + blocking_signed_json_task(move || { keystores::import( request, validator_dir, @@ -1210,33 +1162,30 @@ pub fn serve( // DELETE /eth/v1/keystores let delete_std_keystores = std_keystores .and(warp::body::json()) - .and(signer.clone()) .and(validator_store_filter.clone()) .and(task_executor_filter.clone()) .and(log_filter.clone()) - .and_then(|request, signer, validator_store, task_executor, log| { - blocking_signed_json_task(signer, move || { + .then(|request, validator_store, task_executor, log| { + blocking_signed_json_task(move || { keystores::delete(request, validator_store, task_executor, log) }) }); // GET /eth/v1/remotekeys - let get_std_remotekeys = std_remotekeys - .and(signer.clone()) - .and(validator_store_filter.clone()) - .and_then(|signer, validator_store: Arc>| { - blocking_signed_json_task(signer, move || Ok(remotekeys::list(validator_store))) - }); + let get_std_remotekeys = std_remotekeys.and(validator_store_filter.clone()).then( + |validator_store: Arc>| { + blocking_signed_json_task(move || Ok(remotekeys::list(validator_store))) + }, + ); // POST /eth/v1/remotekeys let post_std_remotekeys = std_remotekeys .and(warp::body::json()) - .and(signer.clone()) .and(validator_store_filter.clone()) .and(task_executor_filter.clone()) .and(log_filter.clone()) - .and_then(|request, signer, validator_store, task_executor, log| { - blocking_signed_json_task(signer, move || { + .then(|request, validator_store, task_executor, log| { + blocking_signed_json_task(move || { remotekeys::import(request, validator_store, task_executor, log) }) }); @@ -1244,12 +1193,11 @@ pub fn serve( // DELETE /eth/v1/remotekeys let delete_std_remotekeys = std_remotekeys .and(warp::body::json()) - .and(signer) .and(validator_store_filter) .and(task_executor_filter) .and(log_filter.clone()) - .and_then(|request, signer, validator_store, task_executor, log| { - blocking_signed_json_task(signer, move || { + .then(|request, validator_store, task_executor, log| { + blocking_signed_json_task(move || { remotekeys::delete(request, validator_store, task_executor, log) }) }); @@ -1371,40 +1319,31 @@ pub fn serve( } /// Executes `func` in blocking tokio task (i.e., where long-running tasks are permitted). -/// JSON-encodes the return value of `func`, using the `signer` function to produce a signature of -/// those bytes. -pub async fn blocking_signed_json_task( - signer: S, - func: F, -) -> Result +/// JSON-encodes the return value of `func`. +pub async fn blocking_signed_json_task(func: F) -> Response where - S: Fn(&[u8]) -> String, F: FnOnce() -> Result + Send + 'static, T: Serialize + Send + 'static, { - warp_utils::task::blocking_task(func) + let result = warp_utils::task::blocking_task(func) .await .map(|func_output| { - let mut response = match serde_json::to_vec(&func_output) { + let response = match serde_json::to_vec(&func_output) { Ok(body) => { - let mut res = Response::new(body); + let mut res = Response::new(body.into()); res.headers_mut() .insert(CONTENT_TYPE, HeaderValue::from_static("application/json")); res } - Err(_) => Response::builder() - .status(StatusCode::INTERNAL_SERVER_ERROR) - .body(vec![]) - .expect("can produce simple response from static values"), + Err(_) => { + let res = Response::new(vec![].into()); + let (mut parts, body) = res.into_parts(); + parts.status = StatusCode::INTERNAL_SERVER_ERROR; + Response::from_parts(parts, body) + } }; - let body: &Vec = response.body(); - let signature = signer(body); - let header_value = - HeaderValue::from_str(&signature).expect("hash can be encoded as header"); - - response.headers_mut().append("Signature", header_value); - response - }) + }); + convert_rejection(result).await } From e9697698d2b4fdfee5761f07e97c48bdd910004e Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Fri, 5 Apr 2024 14:35:34 +0300 Subject: [PATCH 02/13] remove key --- common/eth2/src/lighthouse_vc/http_client.rs | 177 ++------------- lighthouse/tests/validator_manager.rs | 12 - validator_client/src/http_api/api_secret.rs | 211 ------------------ validator_client/src/http_api/mod.rs | 46 +--- validator_client/src/http_api/test_utils.rs | 59 +---- validator_client/src/http_api/tests.rs | 185 +-------------- .../src/http_api/tests/keystores.rs | 17 -- validator_client/src/lib.rs | 4 - validator_manager/src/common.rs | 12 +- validator_manager/src/import_validators.rs | 8 +- validator_manager/src/move_validators.rs | 28 +-- 11 files changed, 30 insertions(+), 729 deletions(-) delete mode 100644 validator_client/src/http_api/api_secret.rs diff --git a/common/eth2/src/lighthouse_vc/http_client.rs b/common/eth2/src/lighthouse_vc/http_client.rs index 83aeea4bfcc..5421a4a155b 100644 --- a/common/eth2/src/lighthouse_vc/http_client.rs +++ b/common/eth2/src/lighthouse_vc/http_client.rs @@ -1,18 +1,10 @@ use super::{types::*, PK_LEN, SECRET_PREFIX}; use crate::Error; -use account_utils::ZeroizeString; -use bytes::Bytes; -use libsecp256k1::{Message, PublicKey, Signature}; -use reqwest::{ - header::{HeaderMap, HeaderValue}, - IntoUrl, -}; -use ring::digest::{digest, SHA256}; +use libsecp256k1::PublicKey; +use reqwest::IntoUrl; use sensitive_url::SensitiveUrl; use serde::{de::DeserializeOwned, Serialize}; use std::fmt::{self, Display}; -use std::fs; -use std::path::Path; pub use reqwest; pub use reqwest::{Response, StatusCode, Url}; @@ -24,8 +16,6 @@ use types::graffiti::GraffitiString; pub struct ValidatorClientHttpClient { client: reqwest::Client, server: SensitiveUrl, - secret: Option, - server_pubkey: Option, authorization_header: AuthorizationHeader, } @@ -78,13 +68,11 @@ pub fn parse_pubkey(secret: &str) -> Result, Error> { } impl ValidatorClientHttpClient { - /// Create a new client pre-initialised with an API token. - pub fn new(server: SensitiveUrl, secret: String) -> Result { + /// Create a new client. + pub fn new(server: SensitiveUrl) -> Result { Ok(Self { client: reqwest::Client::new(), server, - server_pubkey: parse_pubkey(&secret)?, - secret: Some(secret.into()), authorization_header: AuthorizationHeader::Bearer, }) } @@ -96,53 +84,18 @@ impl ValidatorClientHttpClient { Ok(Self { client: reqwest::Client::new(), server, - secret: None, - server_pubkey: None, authorization_header: AuthorizationHeader::Omit, }) } - pub fn from_components( - server: SensitiveUrl, - client: reqwest::Client, - secret: String, - ) -> Result { + pub fn from_components(server: SensitiveUrl, client: reqwest::Client) -> Result { Ok(Self { client, server, - server_pubkey: parse_pubkey(&secret)?, - secret: Some(secret.into()), authorization_header: AuthorizationHeader::Bearer, }) } - /// Get a reference to this client's API token, if any. - pub fn api_token(&self) -> Option<&ZeroizeString> { - self.secret.as_ref() - } - - /// Read an API token from the specified `path`, stripping any trailing whitespace. - pub fn load_api_token_from_file(path: &Path) -> Result { - let token = fs::read_to_string(path).map_err(|e| Error::TokenReadError(path.into(), e))?; - Ok(ZeroizeString::from(token.trim_end().to_string())) - } - - /// Add an authentication token to use when making requests. - /// - /// If the token is Lighthouse-like, a pubkey derivation will be attempted. In the case - /// of failure the token will still be stored, and the client can continue to be used to - /// communicate with non-Lighthouse nodes. - pub fn add_auth_token(&mut self, token: ZeroizeString) -> Result<(), Error> { - let pubkey_res = parse_pubkey(token.as_str()); - - self.secret = Some(token); - self.authorization_header = AuthorizationHeader::Bearer; - - pubkey_res.map(|opt_pubkey| { - self.server_pubkey = opt_pubkey; - }) - } - /// Set to `false` to disable sending the `Authorization` header on requests. /// /// Failing to send the `Authorization` header will cause the VC to reject requests with a 403. @@ -155,92 +108,21 @@ impl ValidatorClientHttpClient { } } - /// Use the legacy basic auth style (bearer auth preferred by default now). - pub fn use_basic_auth(&mut self) { - self.authorization_header = AuthorizationHeader::Basic; - } - - async fn signed_body(&self, response: Response) -> Result { - let server_pubkey = self.server_pubkey.as_ref().ok_or(Error::NoServerPubkey)?; - let sig = response - .headers() - .get("Signature") - .ok_or(Error::MissingSignatureHeader)? - .to_str() - .map_err(|_| Error::InvalidSignatureHeader)? - .to_string(); - - let body = response.bytes().await.map_err(Error::from)?; - - let message = - Message::parse_slice(digest(&SHA256, &body).as_ref()).expect("sha256 is 32 bytes"); - - serde_utils::hex::decode(&sig) - .ok() - .and_then(|bytes| { - let sig = Signature::parse_der(&bytes).ok()?; - Some(libsecp256k1::verify(&message, &sig, server_pubkey)) - }) - .filter(|is_valid| *is_valid) - .ok_or(Error::InvalidSignatureHeader)?; - - Ok(body) - } - - async fn signed_json(&self, response: Response) -> Result { - let body = self.signed_body(response).await?; - serde_json::from_slice(&body).map_err(Error::InvalidJson) - } - - fn headers(&self) -> Result { - let mut headers = HeaderMap::new(); - - if self.authorization_header == AuthorizationHeader::Basic - || self.authorization_header == AuthorizationHeader::Bearer - { - let secret = self.secret.as_ref().ok_or(Error::NoToken)?; - let header_value = HeaderValue::from_str(&format!( - "{} {}", - self.authorization_header, - secret.as_str() - )) - .map_err(|e| { - Error::InvalidSecret(format!("secret is invalid as a header value: {}", e)) - })?; - - headers.insert("Authorization", header_value); - } - - Ok(headers) - } - /// Perform a HTTP GET request, returning the `Response` for further processing. async fn get_response(&self, url: U) -> Result { - let response = self - .client - .get(url) - .headers(self.headers()?) - .send() - .await - .map_err(Error::from)?; + let response = self.client.get(url).send().await.map_err(Error::from)?; ok_or_error(response).await } /// Perform a HTTP DELETE request, returning the `Response` for further processing. async fn delete_response(&self, url: U) -> Result { - let response = self - .client - .delete(url) - .headers(self.headers()?) - .send() - .await - .map_err(Error::from)?; + let response = self.client.delete(url).send().await.map_err(Error::from)?; ok_or_error(response).await } async fn get(&self, url: U) -> Result { let response = self.get_response(url).await?; - self.signed_json(response).await + Ok(response.json().await?) } async fn delete(&self, url: U) -> Result<(), Error> { @@ -252,18 +134,10 @@ impl ValidatorClientHttpClient { } } - async fn get_unsigned(&self, url: U) -> Result { - self.get_response(url) - .await? - .json() - .await - .map_err(Error::from) - } - /// Perform a HTTP GET request, returning `None` on a 404 error. async fn get_opt(&self, url: U) -> Result, Error> { match self.get_response(url).await { - Ok(resp) => self.signed_json(resp).await.map(Option::Some), + Ok(resp) => Ok(resp.json().await.map(Option::Some)?), Err(err) => { if err.status() == Some(StatusCode::NOT_FOUND) { Ok(None) @@ -283,7 +157,6 @@ impl ValidatorClientHttpClient { let response = self .client .post(url) - .headers(self.headers()?) .json(body) .send() .await @@ -295,15 +168,6 @@ impl ValidatorClientHttpClient { &self, url: U, body: &T, - ) -> Result { - let response = self.post_with_raw_response(url, body).await?; - self.signed_json(response).await - } - - async fn post_with_unsigned_response( - &self, - url: U, - body: &T, ) -> Result { let response = self.post_with_raw_response(url, body).await?; Ok(response.json().await?) @@ -314,13 +178,11 @@ impl ValidatorClientHttpClient { let response = self .client .patch(url) - .headers(self.headers()?) .json(body) .send() .await .map_err(Error::from)?; - let response = ok_or_error(response).await?; - self.signed_body(response).await?; + ok_or_error(response).await?; Ok(()) } @@ -333,7 +195,6 @@ impl ValidatorClientHttpClient { let response = self .client .delete(url) - .headers(self.headers()?) .json(body) .send() .await @@ -587,20 +448,10 @@ impl ValidatorClientHttpClient { Ok(url) } - /// `GET lighthouse/auth` - pub async fn get_auth(&self) -> Result { - let mut url = self.server.full.clone(); - url.path_segments_mut() - .map_err(|()| Error::InvalidUrl(self.server.clone()))? - .push("lighthouse") - .push("auth"); - self.get_unsigned(url).await - } - /// `GET eth/v1/keystores` pub async fn get_keystores(&self) -> Result { let url = self.make_keystores_url()?; - self.get_unsigned(url).await + self.get(url).await } /// `POST eth/v1/keystores` @@ -609,7 +460,7 @@ impl ValidatorClientHttpClient { req: &ImportKeystoresRequest, ) -> Result { let url = self.make_keystores_url()?; - self.post_with_unsigned_response(url, req).await + self.post(url, req).await } /// `DELETE eth/v1/keystores` @@ -624,7 +475,7 @@ impl ValidatorClientHttpClient { /// `GET eth/v1/remotekeys` pub async fn get_remotekeys(&self) -> Result { let url = self.make_remotekeys_url()?; - self.get_unsigned(url).await + self.get(url).await } /// `POST eth/v1/remotekeys` @@ -633,7 +484,7 @@ impl ValidatorClientHttpClient { req: &ImportRemotekeysRequest, ) -> Result { let url = self.make_remotekeys_url()?; - self.post_with_unsigned_response(url, req).await + self.post(url, req).await } /// `DELETE eth/v1/remotekeys` diff --git a/lighthouse/tests/validator_manager.rs b/lighthouse/tests/validator_manager.rs index fab1cfebf4b..ea209054e7a 100644 --- a/lighthouse/tests/validator_manager.rs +++ b/lighthouse/tests/validator_manager.rs @@ -196,7 +196,6 @@ pub fn validator_import_defaults() { let expected = ImportConfig { validators_file_path: PathBuf::from("./vals.json"), vc_url: SensitiveUrl::parse("http://localhost:5062").unwrap(), - vc_token_path: PathBuf::from("./token.json"), ignore_duplicates: false, }; assert_eq!(expected, config); @@ -213,7 +212,6 @@ pub fn validator_import_misc_flags() { let expected = ImportConfig { validators_file_path: PathBuf::from("./vals.json"), vc_url: SensitiveUrl::parse("http://localhost:5062").unwrap(), - vc_token_path: PathBuf::from("./token.json"), ignore_duplicates: true, }; assert_eq!(expected, config); @@ -245,9 +243,7 @@ pub fn validator_move_defaults() { .assert_success(|config| { let expected = MoveConfig { src_vc_url: SensitiveUrl::parse("http://localhost:1").unwrap(), - src_vc_token_path: PathBuf::from("./1.json"), dest_vc_url: SensitiveUrl::parse("http://localhost:2").unwrap(), - dest_vc_token_path: PathBuf::from("./2.json"), validators: Validators::All, builder_proposals: None, builder_boost_factor: None, @@ -280,9 +276,7 @@ pub fn validator_move_misc_flags_0() { .assert_success(|config| { let expected = MoveConfig { src_vc_url: SensitiveUrl::parse("http://localhost:1").unwrap(), - src_vc_token_path: PathBuf::from("./1.json"), dest_vc_url: SensitiveUrl::parse("http://localhost:2").unwrap(), - dest_vc_token_path: PathBuf::from("./2.json"), validators: Validators::Specific(vec![ PublicKeyBytes::from_str(EXAMPLE_PUBKEY_0).unwrap(), PublicKeyBytes::from_str(EXAMPLE_PUBKEY_1).unwrap(), @@ -311,9 +305,7 @@ pub fn validator_move_misc_flags_1() { .assert_success(|config| { let expected = MoveConfig { src_vc_url: SensitiveUrl::parse("http://localhost:1").unwrap(), - src_vc_token_path: PathBuf::from("./1.json"), dest_vc_url: SensitiveUrl::parse("http://localhost:2").unwrap(), - dest_vc_token_path: PathBuf::from("./2.json"), validators: Validators::Specific(vec![ PublicKeyBytes::from_str(EXAMPLE_PUBKEY_0).unwrap() ]), @@ -343,9 +335,7 @@ pub fn validator_move_misc_flags_2() { .assert_success(|config| { let expected = MoveConfig { src_vc_url: SensitiveUrl::parse("http://localhost:1").unwrap(), - src_vc_token_path: PathBuf::from("./1.json"), dest_vc_url: SensitiveUrl::parse("http://localhost:2").unwrap(), - dest_vc_token_path: PathBuf::from("./2.json"), validators: Validators::Specific(vec![ PublicKeyBytes::from_str(EXAMPLE_PUBKEY_0).unwrap() ]), @@ -373,9 +363,7 @@ pub fn validator_move_count() { .assert_success(|config| { let expected = MoveConfig { src_vc_url: SensitiveUrl::parse("http://localhost:1").unwrap(), - src_vc_token_path: PathBuf::from("./1.json"), dest_vc_url: SensitiveUrl::parse("http://localhost:2").unwrap(), - dest_vc_token_path: PathBuf::from("./2.json"), validators: Validators::Count(42), builder_proposals: None, builder_boost_factor: None, diff --git a/validator_client/src/http_api/api_secret.rs b/validator_client/src/http_api/api_secret.rs deleted file mode 100644 index e688792ddc1..00000000000 --- a/validator_client/src/http_api/api_secret.rs +++ /dev/null @@ -1,211 +0,0 @@ -use eth2::lighthouse_vc::{PK_LEN, SECRET_PREFIX as PK_PREFIX}; -use filesystem::create_with_600_perms; -use libsecp256k1::{Message, PublicKey, SecretKey}; -use rand::thread_rng; -use ring::digest::{digest, SHA256}; -use std::fs; -use std::path::{Path, PathBuf}; -use warp::Filter; - -/// The name of the file which stores the secret key. -/// -/// It is purposefully opaque to prevent users confusing it with the "secret" that they need to -/// share with API consumers (which is actually the public key). -pub const SK_FILENAME: &str = ".secp-sk"; - -/// Length of the raw secret key, in bytes. -pub const SK_LEN: usize = 32; - -/// The name of the file which stores the public key. -/// -/// For users, this public key is a "secret" that can be shared with API consumers to provide them -/// access to the API. We avoid calling it a "public" key to users, since they should not post this -/// value in a public forum. -pub const PK_FILENAME: &str = "api-token.txt"; - -/// Contains a `secp256k1` keypair that is saved-to/loaded-from disk on instantiation. The keypair -/// is used for authorization/authentication for requests/responses on the HTTP API. -/// -/// Provides convenience functions to ultimately provide: -/// -/// - A signature across outgoing HTTP responses, applied to the `Signature` header. -/// - Verification of proof-of-knowledge of the public key in `self` for incoming HTTP requests, -/// via the `Authorization` header. -/// -/// The aforementioned scheme was first defined here: -/// -/// https://github.com/sigp/lighthouse/issues/1269#issuecomment-649879855 -pub struct ApiSecret { - pk: PublicKey, - sk: SecretKey, - pk_path: PathBuf, -} - -impl ApiSecret { - /// If both the secret and public keys are already on-disk, parse them and ensure they're both - /// from the same keypair. - /// - /// The provided `dir` is a directory containing two files, `SK_FILENAME` and `PK_FILENAME`. - /// - /// If either the secret or public key files are missing on disk, create a new keypair and - /// write it to disk (over-writing any existing files). - pub fn create_or_open>(dir: P) -> Result { - let sk_path = dir.as_ref().join(SK_FILENAME); - let pk_path = dir.as_ref().join(PK_FILENAME); - - if !(sk_path.exists() && pk_path.exists()) { - let sk = SecretKey::random(&mut thread_rng()); - let pk = PublicKey::from_secret_key(&sk); - - // Create and write the secret key to file with appropriate permissions - create_with_600_perms( - &sk_path, - serde_utils::hex::encode(sk.serialize()).as_bytes(), - ) - .map_err(|e| { - format!( - "Unable to create file with permissions for {:?}: {:?}", - sk_path, e - ) - })?; - - // Create and write the public key to file with appropriate permissions - create_with_600_perms( - &pk_path, - format!( - "{}{}", - PK_PREFIX, - serde_utils::hex::encode(&pk.serialize_compressed()[..]) - ) - .as_bytes(), - ) - .map_err(|e| { - format!( - "Unable to create file with permissions for {:?}: {:?}", - pk_path, e - ) - })?; - } - - let sk = fs::read(&sk_path) - .map_err(|e| format!("cannot read {}: {}", SK_FILENAME, e)) - .and_then(|bytes| { - serde_utils::hex::decode(&String::from_utf8_lossy(&bytes)) - .map_err(|_| format!("{} should be 0x-prefixed hex", PK_FILENAME)) - }) - .and_then(|bytes| { - if bytes.len() == SK_LEN { - let mut array = [0; SK_LEN]; - array.copy_from_slice(&bytes); - SecretKey::parse(&array).map_err(|e| format!("invalid {}: {}", SK_FILENAME, e)) - } else { - Err(format!( - "{} expected {} bytes not {}", - SK_FILENAME, - SK_LEN, - bytes.len() - )) - } - })?; - - let pk = fs::read(&pk_path) - .map_err(|e| format!("cannot read {}: {}", PK_FILENAME, e)) - .and_then(|bytes| { - let hex = - String::from_utf8(bytes).map_err(|_| format!("{} is not utf8", SK_FILENAME))?; - if let Some(stripped) = hex.strip_prefix(PK_PREFIX) { - serde_utils::hex::decode(stripped) - .map_err(|_| format!("{} should be 0x-prefixed hex", SK_FILENAME)) - } else { - Err(format!("unable to parse {}", SK_FILENAME)) - } - }) - .and_then(|bytes| { - if bytes.len() == PK_LEN { - let mut array = [0; PK_LEN]; - array.copy_from_slice(&bytes); - PublicKey::parse_compressed(&array) - .map_err(|e| format!("invalid {}: {}", PK_FILENAME, e)) - } else { - Err(format!( - "{} expected {} bytes not {}", - PK_FILENAME, - PK_LEN, - bytes.len() - )) - } - })?; - - // Ensure that the keys loaded from disk are indeed a pair. - if PublicKey::from_secret_key(&sk) != pk { - fs::remove_file(&sk_path) - .map_err(|e| format!("unable to remove {}: {}", SK_FILENAME, e))?; - fs::remove_file(&pk_path) - .map_err(|e| format!("unable to remove {}: {}", PK_FILENAME, e))?; - return Err(format!( - "{:?} does not match {:?} and the files have been deleted. Please try again.", - sk_path, pk_path - )); - } - - Ok(Self { pk, sk, pk_path }) - } - - /// Returns the public key of `self` as a 0x-prefixed hex string. - fn pubkey_string(&self) -> String { - serde_utils::hex::encode(&self.pk.serialize_compressed()[..]) - } - - /// Returns the API token. - pub fn api_token(&self) -> String { - format!("{}{}", PK_PREFIX, self.pubkey_string()) - } - - /// Returns the path for the API token file - pub fn api_token_path(&self) -> PathBuf { - self.pk_path.clone() - } - - /// Returns the values of the `Authorization` header which indicate a valid incoming HTTP - /// request. - /// - /// For backwards-compatibility we accept the token in a basic authentication style, but this is - /// technically invalid according to RFC 7617 because the token is not a base64-encoded username - /// and password. As such, bearer authentication should be preferred. - fn auth_header_values(&self) -> Vec { - vec![ - format!("Basic {}", self.api_token()), - format!("Bearer {}", self.api_token()), - ] - } - - /// Returns a `warp` header which filters out request that have a missing or inaccurate - /// `Authorization` header. - pub fn authorization_header_filter(&self) -> warp::filters::BoxedFilter<()> { - let expected = self.auth_header_values(); - warp::any() - .map(move || expected.clone()) - .and(warp::filters::header::header("Authorization")) - .and_then(move |expected: Vec, header: String| async move { - if expected.contains(&header) { - Ok(()) - } else { - Err(warp_utils::reject::invalid_auth(header)) - } - }) - .untuple_one() - .boxed() - } - - /// Returns a closure which produces a signature over some bytes using the secret key in - /// `self`. The signature is a 32-byte hash formatted as a 0x-prefixed string. - pub fn signer(&self) -> impl Fn(&[u8]) -> String + Clone { - let sk = self.sk; - move |input: &[u8]| -> String { - let message = - Message::parse_slice(digest(&SHA256, input).as_ref()).expect("sha256 is 32 bytes"); - let (signature, _) = libsecp256k1::sign(&message, &sk); - serde_utils::hex::encode(signature.serialize_der().as_ref()) - } - } -} diff --git a/validator_client/src/http_api/mod.rs b/validator_client/src/http_api/mod.rs index 5e6cf996011..b41b2abf690 100644 --- a/validator_client/src/http_api/mod.rs +++ b/validator_client/src/http_api/mod.rs @@ -1,4 +1,3 @@ -mod api_secret; mod create_signed_voluntary_exit; mod create_validator; mod graffiti; @@ -16,12 +15,11 @@ use account_utils::{ mnemonic_from_phrase, validator_definitions::{SigningDefinition, ValidatorDefinition, Web3SignerDefinition}, }; -pub use api_secret::ApiSecret; use create_validator::{ create_validators_mnemonic, create_validators_web3signer, get_voting_password_storage, }; use eth2::lighthouse_vc::{ - std_types::{AuthResponse, GetFeeRecipientResponse, GetGasLimitResponse}, + std_types::{GetFeeRecipientResponse, GetGasLimitResponse}, types::{ self as api_types, GenericResponse, GetGraffitiResponse, Graffiti, PublicKey, PublicKeyBytes, SetGraffitiRequest, @@ -31,7 +29,7 @@ use lighthouse_version::version_with_platform; use logging::SSELoggingComponents; use parking_lot::RwLock; use serde::{Deserialize, Serialize}; -use slog::{crit, info, warn, Logger}; +use slog::{crit, info, Logger}; use slot_clock::SlotClock; use std::collections::HashMap; use std::future::Future; @@ -79,7 +77,6 @@ impl From for Error { /// The server will gracefully handle the case where any fields are `None`. pub struct Context { pub task_executor: TaskExecutor, - pub api_secret: ApiSecret, pub validator_store: Option>>, pub validator_dir: Option, pub secrets_dir: Option, @@ -162,21 +159,6 @@ pub fn serve( )); } - let authorization_header_filter = ctx.api_secret.authorization_header_filter(); - let mut api_token_path = ctx.api_secret.api_token_path(); - - // Attempt to convert the path to an absolute path, but don't error if it fails. - match api_token_path.canonicalize() { - Ok(abs_path) => api_token_path = abs_path, - Err(e) => { - warn!( - log, - "Error canonicalizing token path"; - "error" => ?e, - ); - } - }; - let inner_validator_store = ctx.validator_store.clone(); let validator_store_filter = warp::any() .map(move || inner_validator_store.clone()) @@ -228,9 +210,6 @@ pub fn serve( let inner_spec = Arc::new(ctx.spec.clone()); let spec_filter = warp::any().map(move || inner_spec.clone()); - let api_token_path_inner = api_token_path.clone(); - let api_token_path_filter = warp::any().map(move || api_token_path_inner.clone()); - // Filter for SEE Logging events let inner_components = ctx.sse_logging_components.clone(); let sse_component_filter = warp::any().map(move || inner_components.clone()); @@ -761,18 +740,6 @@ pub fn serve( }, ); - // GET /lighthouse/auth - let get_auth = warp::path("lighthouse").and(warp::path("auth").and(warp::path::end())); - let get_auth = get_auth - .and(api_token_path_filter) - .then(|token_path: PathBuf| { - blocking_signed_json_task(move || { - Ok(AuthResponse { - token_path: token_path.display().to_string(), - }) - }) - }); - // DELETE /lighthouse/keystores let delete_lighthouse_keystores = warp::path("lighthouse") .and(warp::path("keystores")) @@ -1246,11 +1213,6 @@ pub fn serve( }); let routes = warp::any() - .and(authorization_header_filter) - // Note: it is critical that the `authorization_header_filter` is applied to all routes. - // Keeping all the routes inside the following `and` is a reliable way to achieve this. - // - // When adding a route, don't forget to add it to the `routes_with_invalid_auth` tests! .and( warp::get() .and( @@ -1266,6 +1228,7 @@ pub fn serve( .or(get_graffiti) .or(get_std_keystores) .or(get_std_remotekeys) + .or(get_log_events) .recover(warp_utils::reject::handle_rejection), ) .or(warp::post().and( @@ -1293,8 +1256,6 @@ pub fn serve( .recover(warp_utils::reject::handle_rejection), )), ) - // The auth route and logs are the only routes that are allowed to be accessed without the API token. - .or(warp::get().and(get_auth.or(get_log_events.boxed()))) // Maps errors into HTTP responses. .recover(warp_utils::reject::handle_rejection) // Add a `Server` header. @@ -1312,7 +1273,6 @@ pub fn serve( log, "HTTP API started"; "listen_address" => listening_socket.to_string(), - "api_token_file" => ?api_token_path, ); Ok((listening_socket, server)) diff --git a/validator_client/src/http_api/test_utils.rs b/validator_client/src/http_api/test_utils.rs index 8bb56e87a32..f2929197e8a 100644 --- a/validator_client/src/http_api/test_utils.rs +++ b/validator_client/src/http_api/test_utils.rs @@ -1,7 +1,7 @@ use crate::doppelganger_service::DoppelgangerService; use crate::key_cache::{KeyCache, CACHE_FILENAME}; use crate::{ - http_api::{ApiSecret, Config as HttpConfig, Context}, + http_api::{Config as HttpConfig, Context}, initialized_validators::{InitializedValidators, OnDecryptFailure}, Config, ValidatorDefinitions, ValidatorStore, }; @@ -10,18 +10,13 @@ use account_utils::{ ZeroizeString, }; use deposit_contract::decode_eth1_tx_data; -use eth2::{ - lighthouse_vc::{http_client::ValidatorClientHttpClient, types::*}, - types::ErrorMessage as ApiErrorMessage, - Error as ApiError, -}; +use eth2::lighthouse_vc::{http_client::ValidatorClientHttpClient, types::*}; use eth2_keystore::KeystoreBuilder; use logging::test_logger; use parking_lot::RwLock; use sensitive_url::SensitiveUrl; use slashing_protection::{SlashingDatabase, SLASHING_PROTECTION_FILENAME}; use slot_clock::{SlotClock, TestingSlotClock}; -use std::future::Future; use std::marker::PhantomData; use std::net::{IpAddr, Ipv4Addr}; use std::sync::Arc; @@ -57,7 +52,6 @@ pub struct ApiTester { pub initialized_validators: Arc>, pub validator_store: Arc>, pub url: SensitiveUrl, - pub api_token: String, pub test_runtime: TestRuntime, pub _server_shutdown: oneshot::Sender<()>, pub validator_dir: TempDir, @@ -86,9 +80,6 @@ impl ApiTester { .await .unwrap(); - let api_secret = ApiSecret::create_or_open(validator_dir.path()).unwrap(); - let api_pubkey = api_secret.api_token(); - let config = Config { validator_dir: validator_dir.path().into(), secrets_dir: secrets_dir.path().into(), @@ -126,7 +117,6 @@ impl ApiTester { let context = Arc::new(Context { task_executor: test_runtime.task_executor.clone(), - api_secret, validator_dir: Some(validator_dir.path().into()), secrets_dir: Some(secrets_dir.path().into()), validator_store: Some(validator_store.clone()), @@ -156,14 +146,13 @@ impl ApiTester { )) .unwrap(); - let client = ValidatorClientHttpClient::new(url.clone(), api_pubkey.clone()).unwrap(); + let client = ValidatorClientHttpClient::new(url.clone()).unwrap(); Self { client, initialized_validators, validator_store, url, - api_token: api_pubkey, test_runtime, _server_shutdown: shutdown_tx, validator_dir, @@ -200,48 +189,6 @@ impl ApiTester { .expect("key cache should decypt"); } - pub fn invalid_token_client(&self) -> ValidatorClientHttpClient { - let tmp = tempdir().unwrap(); - let api_secret = ApiSecret::create_or_open(tmp.path()).unwrap(); - let invalid_pubkey = api_secret.api_token(); - ValidatorClientHttpClient::new(self.url.clone(), invalid_pubkey).unwrap() - } - - pub async fn test_with_invalid_auth(self, func: F) -> Self - where - F: Fn(ValidatorClientHttpClient) -> A, - A: Future>, - { - /* - * Test with an invalid Authorization header. - */ - match func(self.invalid_token_client()).await { - Err(ApiError::ServerMessage(ApiErrorMessage { code: 403, .. })) => (), - Err(other) => panic!("expected authorized error, got {:?}", other), - Ok(_) => panic!("expected authorized error, got Ok"), - } - - /* - * Test with a missing Authorization header. - */ - let mut missing_token_client = self.client.clone(); - missing_token_client.send_authorization_header(false); - match func(missing_token_client).await { - Err(ApiError::ServerMessage(ApiErrorMessage { - code: 401, message, .. - })) if message.contains("missing Authorization header") => (), - Err(other) => panic!("expected missing header error, got {:?}", other), - Ok(_) => panic!("expected missing header error, got Ok"), - } - - self - } - - pub fn invalidate_api_token(mut self) -> Self { - self.client = self.invalid_token_client(); - self - } - pub async fn test_get_lighthouse_version_invalid(self) -> Self { self.client.get_lighthouse_version().await.unwrap_err(); self diff --git a/validator_client/src/http_api/tests.rs b/validator_client/src/http_api/tests.rs index ce1937d4379..d1fda5b4721 100644 --- a/validator_client/src/http_api/tests.rs +++ b/validator_client/src/http_api/tests.rs @@ -5,7 +5,7 @@ mod keystores; use crate::doppelganger_service::DoppelgangerService; use crate::{ - http_api::{ApiSecret, Config as HttpConfig, Context}, + http_api::{Config as HttpConfig, Context}, initialized_validators::InitializedValidators, Config, ValidatorDefinitions, ValidatorStore, }; @@ -74,9 +74,6 @@ impl ApiTester { .await .unwrap(); - let api_secret = ApiSecret::create_or_open(validator_dir.path()).unwrap(); - let api_pubkey = api_secret.api_token(); - config.validator_dir = validator_dir.path().into(); config.secrets_dir = secrets_dir.path().into(); @@ -114,7 +111,6 @@ impl ApiTester { let context = Arc::new(Context { task_executor: test_runtime.task_executor.clone(), - api_secret, validator_dir: Some(validator_dir.path().into()), secrets_dir: Some(secrets_dir.path().into()), validator_store: Some(validator_store.clone()), @@ -147,7 +143,7 @@ impl ApiTester { )) .unwrap(); - let client = ValidatorClientHttpClient::new(url.clone(), api_pubkey).unwrap(); + let client = ValidatorClientHttpClient::new(url.clone()).unwrap(); Self { client, @@ -160,48 +156,6 @@ impl ApiTester { } } - pub fn invalid_token_client(&self) -> ValidatorClientHttpClient { - let tmp = tempdir().unwrap(); - let api_secret = ApiSecret::create_or_open(tmp.path()).unwrap(); - let invalid_pubkey = api_secret.api_token(); - ValidatorClientHttpClient::new(self.url.clone(), invalid_pubkey.clone()).unwrap() - } - - pub async fn test_with_invalid_auth(self, func: F) -> Self - where - F: Fn(ValidatorClientHttpClient) -> A, - A: Future>, - { - /* - * Test with an invalid Authorization header. - */ - match func(self.invalid_token_client()).await { - Err(ApiError::ServerMessage(ApiErrorMessage { code: 403, .. })) => (), - Err(other) => panic!("expected authorized error, got {:?}", other), - Ok(_) => panic!("expected authorized error, got Ok"), - } - - /* - * Test with a missing Authorization header. - */ - let mut missing_token_client = self.client.clone(); - missing_token_client.send_authorization_header(false); - match func(missing_token_client).await { - Err(ApiError::ServerMessage(ApiErrorMessage { - code: 401, message, .. - })) if message.contains("missing Authorization header") => (), - Err(other) => panic!("expected missing header error, got {:?}", other), - Ok(_) => panic!("expected missing header error, got Ok"), - } - - self - } - - pub fn invalidate_api_token(mut self) -> Self { - self.client = self.invalid_token_client(); - self - } - pub async fn test_get_lighthouse_version_invalid(self) -> Self { self.client.get_lighthouse_version().await.unwrap_err(); self @@ -829,141 +783,6 @@ struct Web3SignerValidatorScenario { enabled: bool, } -#[tokio::test] -async fn invalid_pubkey() { - ApiTester::new() - .await - .invalidate_api_token() - .test_get_lighthouse_version_invalid() - .await; -} - -#[tokio::test] -async fn routes_with_invalid_auth() { - ApiTester::new() - .await - .test_with_invalid_auth(|client| async move { client.get_lighthouse_version().await }) - .await - .test_with_invalid_auth(|client| async move { client.get_lighthouse_health().await }) - .await - .test_with_invalid_auth(|client| async move { - client.get_lighthouse_spec::().await - }) - .await - .test_with_invalid_auth(|client| async move { client.get_lighthouse_validators().await }) - .await - .test_with_invalid_auth(|client| async move { - client - .get_lighthouse_validators_pubkey(&PublicKeyBytes::empty()) - .await - }) - .await - .test_with_invalid_auth(|client| async move { - client - .post_lighthouse_validators(vec![ValidatorRequest { - enable: <_>::default(), - description: <_>::default(), - graffiti: <_>::default(), - suggested_fee_recipient: <_>::default(), - gas_limit: <_>::default(), - builder_proposals: <_>::default(), - deposit_gwei: <_>::default(), - builder_boost_factor: <_>::default(), - prefer_builder_proposals: <_>::default(), - }]) - .await - }) - .await - .test_with_invalid_auth(|client| async move { - client - .post_lighthouse_validators_mnemonic(&CreateValidatorsMnemonicRequest { - mnemonic: String::default().into(), - key_derivation_path_offset: <_>::default(), - validators: <_>::default(), - }) - .await - }) - .await - .test_with_invalid_auth(|client| async move { - let password = random_password(); - let keypair = Keypair::random(); - let keystore = KeystoreBuilder::new(&keypair, password.as_bytes(), String::new()) - .unwrap() - .build() - .unwrap(); - client - .post_lighthouse_validators_keystore(&KeystoreValidatorsPostRequest { - password: String::default().into(), - enable: <_>::default(), - keystore, - graffiti: <_>::default(), - suggested_fee_recipient: <_>::default(), - gas_limit: <_>::default(), - builder_proposals: <_>::default(), - builder_boost_factor: <_>::default(), - prefer_builder_proposals: <_>::default(), - }) - .await - }) - .await - .test_with_invalid_auth(|client| async move { - client - .patch_lighthouse_validators( - &PublicKeyBytes::empty(), - Some(false), - None, - None, - None, - None, - None, - ) - .await - }) - .await - .test_with_invalid_auth(|client| async move { client.get_keystores().await }) - .await - .test_with_invalid_auth(|client| async move { - let password = random_password_string(); - let keypair = Keypair::random(); - let keystore = KeystoreBuilder::new(&keypair, password.as_ref(), String::new()) - .unwrap() - .build() - .map(KeystoreJsonStr) - .unwrap(); - client - .post_keystores(&ImportKeystoresRequest { - keystores: vec![keystore], - passwords: vec![password], - slashing_protection: None, - }) - .await - }) - .await - .test_with_invalid_auth(|client| async move { - let keypair = Keypair::random(); - client - .delete_keystores(&DeleteKeystoresRequest { - pubkeys: vec![keypair.pk.compress()], - }) - .await - }) - .await - .test_with_invalid_auth(|client| async move { - client.delete_graffiti(&PublicKeyBytes::empty()).await - }) - .await - .test_with_invalid_auth(|client| async move { - client.get_graffiti(&PublicKeyBytes::empty()).await - }) - .await - .test_with_invalid_auth(|client| async move { - client - .set_graffiti(&PublicKeyBytes::empty(), GraffitiString::default()) - .await - }) - .await; -} - #[tokio::test] async fn simple_getters() { ApiTester::new() diff --git a/validator_client/src/http_api/tests/keystores.rs b/validator_client/src/http_api/tests/keystores.rs index fe58393bb8d..04b31abb294 100644 --- a/validator_client/src/http_api/tests/keystores.rs +++ b/validator_client/src/http_api/tests/keystores.rs @@ -194,23 +194,6 @@ fn check_remotekey_delete_response( } } -#[tokio::test] -async fn get_auth_no_token() { - run_test(|mut tester| async move { - let _ = &tester; - tester.client.send_authorization_header(false); - let auth_response = tester.client.get_auth().await.unwrap(); - - // Load the file from the returned path. - let token_path = Path::new(&auth_response.token_path); - let token = HttpClient::load_api_token_from_file(token_path).unwrap(); - - // The token should match the one that the client was originally initialised with. - assert!(tester.client.api_token() == Some(&token)); - }) - .await; -} - #[tokio::test] async fn get_empty_keystores() { run_test(|tester| async move { diff --git a/validator_client/src/lib.rs b/validator_client/src/lib.rs index 32a0eadbef4..63515f78665 100644 --- a/validator_client/src/lib.rs +++ b/validator_client/src/lib.rs @@ -42,7 +42,6 @@ use clap::ArgMatches; use duties_service::{sync::SyncDutiesMap, DutiesService}; use environment::RuntimeContext; use eth2::{reqwest::ClientBuilder, types::Graffiti, BeaconNodeHttpClient, StatusCode, Timeouts}; -use http_api::ApiSecret; use notifier::spawn_notifier; use parking_lot::RwLock; use preparation_service::{PreparationService, PreparationServiceBuilder}; @@ -532,12 +531,9 @@ impl ProductionValidatorClient { let (block_service_tx, block_service_rx) = mpsc::channel(channel_capacity); let log = self.context.log(); - let api_secret = ApiSecret::create_or_open(&self.config.validator_dir)?; - self.http_api_listen_addr = if self.config.http_api.enabled { let ctx = Arc::new(http_api::Context { task_executor: self.context.executor.clone(), - api_secret, validator_store: Some(self.validator_store.clone()), validator_dir: Some(self.config.validator_dir.clone()), secrets_dir: Some(self.config.secrets_dir.clone()), diff --git a/validator_manager/src/common.rs b/validator_manager/src/common.rs index 871c5362030..dc5c5949ae1 100644 --- a/validator_manager/src/common.rs +++ b/validator_manager/src/common.rs @@ -1,4 +1,4 @@ -use account_utils::{strip_off_newlines, ZeroizeString}; +use account_utils::ZeroizeString; use eth2::lighthouse_vc::std_types::{InterchangeJsonStr, KeystoreJsonStr}; use eth2::{ lighthouse_vc::{ @@ -317,16 +317,10 @@ mod bytes_4_without_0x_prefix { } } -pub async fn vc_http_client>( +pub async fn vc_http_client( url: SensitiveUrl, - token_path: P, ) -> Result<(ValidatorClientHttpClient, Vec), String> { - let token_path = token_path.as_ref(); - let token_bytes = - fs::read(token_path).map_err(|e| format!("Failed to read {:?}: {:?}", token_path, e))?; - let token_string = String::from_utf8(strip_off_newlines(token_bytes)) - .map_err(|e| format!("Failed to parse {:?} as utf8: {:?}", token_path, e))?; - let http_client = ValidatorClientHttpClient::new(url.clone(), token_string).map_err(|e| { + let http_client = ValidatorClientHttpClient::new(url.clone()).map_err(|e| { format!( "Could not instantiate HTTP client from URL and secret: {:?}", e diff --git a/validator_manager/src/import_validators.rs b/validator_manager/src/import_validators.rs index 4b924189f20..db90f8b37e2 100644 --- a/validator_manager/src/import_validators.rs +++ b/validator_manager/src/import_validators.rs @@ -71,7 +71,6 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> { pub struct ImportConfig { pub validators_file_path: PathBuf, pub vc_url: SensitiveUrl, - pub vc_token_path: PathBuf, pub ignore_duplicates: bool, } @@ -80,7 +79,6 @@ impl ImportConfig { Ok(Self { validators_file_path: clap_utils::parse_required(matches, VALIDATORS_FILE_FLAG)?, vc_url: clap_utils::parse_required(matches, VC_URL_FLAG)?, - vc_token_path: clap_utils::parse_required(matches, VC_TOKEN_FLAG)?, ignore_duplicates: matches.is_present(IGNORE_DUPLICATES_FLAG), }) } @@ -102,7 +100,6 @@ async fn run<'a>(config: ImportConfig) -> Result<(), String> { let ImportConfig { validators_file_path, vc_url, - vc_token_path, ignore_duplicates, } = config; @@ -125,7 +122,7 @@ async fn run<'a>(config: ImportConfig) -> Result<(), String> { let count = validators.len(); - let (http_client, _keystores) = vc_http_client(vc_url.clone(), &vc_token_path).await?; + let (http_client, _keystores) = vc_http_client(vc_url.clone()).await?; eprintln!( "Starting to submit {} validators to VC, each validator may take several seconds", @@ -261,15 +258,12 @@ pub mod tests { pub async fn new_with_http_config(http_config: HttpConfig) -> Self { let dir = tempdir().unwrap(); let vc = ApiTester::new_with_http_config(http_config).await; - let vc_token_path = dir.path().join(VC_TOKEN_FILE_NAME); - fs::write(&vc_token_path, &vc.api_token).unwrap(); Self { import_config: ImportConfig { // This field will be overwritten later on. validators_file_path: dir.path().into(), vc_url: vc.url.clone(), - vc_token_path, ignore_duplicates: false, }, vc, diff --git a/validator_manager/src/move_validators.rs b/validator_manager/src/move_validators.rs index 5826f2756be..c1b3eb5cf51 100644 --- a/validator_manager/src/move_validators.rs +++ b/validator_manager/src/move_validators.rs @@ -14,7 +14,6 @@ use eth2::{ }; use serde::{Deserialize, Serialize}; use std::collections::{HashMap, HashSet}; -use std::path::PathBuf; use std::str::FromStr; use std::time::Duration; use tokio::time::sleep; @@ -208,9 +207,7 @@ pub enum Validators { #[derive(Clone, PartialEq, Debug, Serialize, Deserialize)] pub struct MoveConfig { pub src_vc_url: SensitiveUrl, - pub src_vc_token_path: PathBuf, pub dest_vc_url: SensitiveUrl, - pub dest_vc_token_path: PathBuf, pub validators: Validators, pub builder_proposals: Option, pub builder_boost_factor: Option, @@ -244,9 +241,7 @@ impl MoveConfig { Ok(Self { src_vc_url: clap_utils::parse_required(matches, SRC_VC_URL_FLAG)?, - src_vc_token_path: clap_utils::parse_required(matches, SRC_VC_TOKEN_FLAG)?, dest_vc_url: clap_utils::parse_required(matches, DEST_VC_URL_FLAG)?, - dest_vc_token_path: clap_utils::parse_required(matches, DEST_VC_TOKEN_FLAG)?, validators, builder_proposals: clap_utils::parse_optional(matches, BUILDER_PROPOSALS_FLAG)?, builder_boost_factor: clap_utils::parse_optional(matches, BUILDER_BOOST_FACTOR_FLAG)?, @@ -278,9 +273,7 @@ pub async fn cli_run<'a>( async fn run<'a>(config: MoveConfig) -> Result<(), String> { let MoveConfig { src_vc_url, - src_vc_token_path, dest_vc_url, - dest_vc_token_path, validators, builder_proposals, fee_recipient, @@ -299,10 +292,8 @@ async fn run<'a>(config: MoveConfig) -> Result<(), String> { )); } - let (src_http_client, src_keystores) = - vc_http_client(src_vc_url.clone(), &src_vc_token_path).await?; - let (dest_http_client, _dest_keystores) = - vc_http_client(dest_vc_url.clone(), &dest_vc_token_path).await?; + let (src_http_client, src_keystores) = vc_http_client(src_vc_url.clone()).await?; + let (dest_http_client, _dest_keystores) = vc_http_client(dest_vc_url.clone()).await?; if src_keystores.is_empty() { return Err(NO_VALIDATORS_MSG.to_string()); @@ -767,12 +758,8 @@ mod test { where F: Fn(&[PublicKeyBytes]) -> Validators, { - let src_vc_token_path = self.dir.path().join(SRC_VC_TOKEN_FILE_NAME); - fs::write(&src_vc_token_path, &src_vc.api_token).unwrap(); let (src_vc_client, src_vc_initial_keystores) = - vc_http_client(src_vc.url.clone(), &src_vc_token_path) - .await - .unwrap(); + vc_http_client(src_vc.url.clone()).await.unwrap(); let src_vc_initial_pubkeys: Vec<_> = src_vc_initial_keystores .iter() @@ -780,19 +767,12 @@ mod test { .collect(); let validators = gen_validators_enum(&src_vc_initial_pubkeys); - let dest_vc_token_path = self.dir.path().join(DEST_VC_TOKEN_FILE_NAME); - fs::write(&dest_vc_token_path, &dest_vc.api_token).unwrap(); - let (dest_vc_client, dest_vc_initial_keystores) = - vc_http_client(dest_vc.url.clone(), &dest_vc_token_path) - .await - .unwrap(); + vc_http_client(dest_vc.url.clone()).await.unwrap(); let move_config = MoveConfig { src_vc_url: src_vc.url.clone(), - src_vc_token_path, dest_vc_url: dest_vc.url.clone(), - dest_vc_token_path: dest_vc_token_path.clone(), validators: validators.clone(), builder_proposals: None, builder_boost_factor: None, From 12a9db284ac2cd6be3fe61366bf3ba1b1d6642fb Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Fri, 5 Apr 2024 14:42:54 +0300 Subject: [PATCH 03/13] remove auth header --- common/eth2/src/lighthouse_vc/http_client.rs | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/common/eth2/src/lighthouse_vc/http_client.rs b/common/eth2/src/lighthouse_vc/http_client.rs index 5421a4a155b..632841ec96b 100644 --- a/common/eth2/src/lighthouse_vc/http_client.rs +++ b/common/eth2/src/lighthouse_vc/http_client.rs @@ -16,7 +16,6 @@ use types::graffiti::GraffitiString; pub struct ValidatorClientHttpClient { client: reqwest::Client, server: SensitiveUrl, - authorization_header: AuthorizationHeader, } #[derive(Debug, Clone, Copy, PartialEq)] @@ -73,7 +72,6 @@ impl ValidatorClientHttpClient { Ok(Self { client: reqwest::Client::new(), server, - authorization_header: AuthorizationHeader::Bearer, }) } @@ -84,7 +82,6 @@ impl ValidatorClientHttpClient { Ok(Self { client: reqwest::Client::new(), server, - authorization_header: AuthorizationHeader::Omit, }) } @@ -92,22 +89,9 @@ impl ValidatorClientHttpClient { Ok(Self { client, server, - authorization_header: AuthorizationHeader::Bearer, }) } - /// Set to `false` to disable sending the `Authorization` header on requests. - /// - /// Failing to send the `Authorization` header will cause the VC to reject requests with a 403. - /// This function is intended only for testing purposes. - pub fn send_authorization_header(&mut self, should_send: bool) { - if should_send { - self.authorization_header = AuthorizationHeader::Bearer; - } else { - self.authorization_header = AuthorizationHeader::Omit; - } - } - /// Perform a HTTP GET request, returning the `Response` for further processing. async fn get_response(&self, url: U) -> Result { let response = self.client.get(url).send().await.map_err(Error::from)?; From c271fa6666c52ca99ed0d26cafb7dfd61fb01691 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Tue, 23 Apr 2024 08:54:42 +0300 Subject: [PATCH 04/13] revert --- common/eth2/src/lighthouse_vc/http_client.rs | 193 +++++++++++++++-- validator_client/src/http_api/api_secret.rs | 211 +++++++++++++++++++ 2 files changed, 390 insertions(+), 14 deletions(-) create mode 100644 validator_client/src/http_api/api_secret.rs diff --git a/common/eth2/src/lighthouse_vc/http_client.rs b/common/eth2/src/lighthouse_vc/http_client.rs index 632841ec96b..83aeea4bfcc 100644 --- a/common/eth2/src/lighthouse_vc/http_client.rs +++ b/common/eth2/src/lighthouse_vc/http_client.rs @@ -1,10 +1,18 @@ use super::{types::*, PK_LEN, SECRET_PREFIX}; use crate::Error; -use libsecp256k1::PublicKey; -use reqwest::IntoUrl; +use account_utils::ZeroizeString; +use bytes::Bytes; +use libsecp256k1::{Message, PublicKey, Signature}; +use reqwest::{ + header::{HeaderMap, HeaderValue}, + IntoUrl, +}; +use ring::digest::{digest, SHA256}; use sensitive_url::SensitiveUrl; use serde::{de::DeserializeOwned, Serialize}; use std::fmt::{self, Display}; +use std::fs; +use std::path::Path; pub use reqwest; pub use reqwest::{Response, StatusCode, Url}; @@ -16,6 +24,9 @@ use types::graffiti::GraffitiString; pub struct ValidatorClientHttpClient { client: reqwest::Client, server: SensitiveUrl, + secret: Option, + server_pubkey: Option, + authorization_header: AuthorizationHeader, } #[derive(Debug, Clone, Copy, PartialEq)] @@ -67,11 +78,14 @@ pub fn parse_pubkey(secret: &str) -> Result, Error> { } impl ValidatorClientHttpClient { - /// Create a new client. - pub fn new(server: SensitiveUrl) -> Result { + /// Create a new client pre-initialised with an API token. + pub fn new(server: SensitiveUrl, secret: String) -> Result { Ok(Self { client: reqwest::Client::new(), server, + server_pubkey: parse_pubkey(&secret)?, + secret: Some(secret.into()), + authorization_header: AuthorizationHeader::Bearer, }) } @@ -82,31 +96,151 @@ impl ValidatorClientHttpClient { Ok(Self { client: reqwest::Client::new(), server, + secret: None, + server_pubkey: None, + authorization_header: AuthorizationHeader::Omit, }) } - pub fn from_components(server: SensitiveUrl, client: reqwest::Client) -> Result { + pub fn from_components( + server: SensitiveUrl, + client: reqwest::Client, + secret: String, + ) -> Result { Ok(Self { client, server, + server_pubkey: parse_pubkey(&secret)?, + secret: Some(secret.into()), + authorization_header: AuthorizationHeader::Bearer, }) } + /// Get a reference to this client's API token, if any. + pub fn api_token(&self) -> Option<&ZeroizeString> { + self.secret.as_ref() + } + + /// Read an API token from the specified `path`, stripping any trailing whitespace. + pub fn load_api_token_from_file(path: &Path) -> Result { + let token = fs::read_to_string(path).map_err(|e| Error::TokenReadError(path.into(), e))?; + Ok(ZeroizeString::from(token.trim_end().to_string())) + } + + /// Add an authentication token to use when making requests. + /// + /// If the token is Lighthouse-like, a pubkey derivation will be attempted. In the case + /// of failure the token will still be stored, and the client can continue to be used to + /// communicate with non-Lighthouse nodes. + pub fn add_auth_token(&mut self, token: ZeroizeString) -> Result<(), Error> { + let pubkey_res = parse_pubkey(token.as_str()); + + self.secret = Some(token); + self.authorization_header = AuthorizationHeader::Bearer; + + pubkey_res.map(|opt_pubkey| { + self.server_pubkey = opt_pubkey; + }) + } + + /// Set to `false` to disable sending the `Authorization` header on requests. + /// + /// Failing to send the `Authorization` header will cause the VC to reject requests with a 403. + /// This function is intended only for testing purposes. + pub fn send_authorization_header(&mut self, should_send: bool) { + if should_send { + self.authorization_header = AuthorizationHeader::Bearer; + } else { + self.authorization_header = AuthorizationHeader::Omit; + } + } + + /// Use the legacy basic auth style (bearer auth preferred by default now). + pub fn use_basic_auth(&mut self) { + self.authorization_header = AuthorizationHeader::Basic; + } + + async fn signed_body(&self, response: Response) -> Result { + let server_pubkey = self.server_pubkey.as_ref().ok_or(Error::NoServerPubkey)?; + let sig = response + .headers() + .get("Signature") + .ok_or(Error::MissingSignatureHeader)? + .to_str() + .map_err(|_| Error::InvalidSignatureHeader)? + .to_string(); + + let body = response.bytes().await.map_err(Error::from)?; + + let message = + Message::parse_slice(digest(&SHA256, &body).as_ref()).expect("sha256 is 32 bytes"); + + serde_utils::hex::decode(&sig) + .ok() + .and_then(|bytes| { + let sig = Signature::parse_der(&bytes).ok()?; + Some(libsecp256k1::verify(&message, &sig, server_pubkey)) + }) + .filter(|is_valid| *is_valid) + .ok_or(Error::InvalidSignatureHeader)?; + + Ok(body) + } + + async fn signed_json(&self, response: Response) -> Result { + let body = self.signed_body(response).await?; + serde_json::from_slice(&body).map_err(Error::InvalidJson) + } + + fn headers(&self) -> Result { + let mut headers = HeaderMap::new(); + + if self.authorization_header == AuthorizationHeader::Basic + || self.authorization_header == AuthorizationHeader::Bearer + { + let secret = self.secret.as_ref().ok_or(Error::NoToken)?; + let header_value = HeaderValue::from_str(&format!( + "{} {}", + self.authorization_header, + secret.as_str() + )) + .map_err(|e| { + Error::InvalidSecret(format!("secret is invalid as a header value: {}", e)) + })?; + + headers.insert("Authorization", header_value); + } + + Ok(headers) + } + /// Perform a HTTP GET request, returning the `Response` for further processing. async fn get_response(&self, url: U) -> Result { - let response = self.client.get(url).send().await.map_err(Error::from)?; + let response = self + .client + .get(url) + .headers(self.headers()?) + .send() + .await + .map_err(Error::from)?; ok_or_error(response).await } /// Perform a HTTP DELETE request, returning the `Response` for further processing. async fn delete_response(&self, url: U) -> Result { - let response = self.client.delete(url).send().await.map_err(Error::from)?; + let response = self + .client + .delete(url) + .headers(self.headers()?) + .send() + .await + .map_err(Error::from)?; ok_or_error(response).await } async fn get(&self, url: U) -> Result { let response = self.get_response(url).await?; - Ok(response.json().await?) + self.signed_json(response).await } async fn delete(&self, url: U) -> Result<(), Error> { @@ -118,10 +252,18 @@ impl ValidatorClientHttpClient { } } + async fn get_unsigned(&self, url: U) -> Result { + self.get_response(url) + .await? + .json() + .await + .map_err(Error::from) + } + /// Perform a HTTP GET request, returning `None` on a 404 error. async fn get_opt(&self, url: U) -> Result, Error> { match self.get_response(url).await { - Ok(resp) => Ok(resp.json().await.map(Option::Some)?), + Ok(resp) => self.signed_json(resp).await.map(Option::Some), Err(err) => { if err.status() == Some(StatusCode::NOT_FOUND) { Ok(None) @@ -141,6 +283,7 @@ impl ValidatorClientHttpClient { let response = self .client .post(url) + .headers(self.headers()?) .json(body) .send() .await @@ -152,6 +295,15 @@ impl ValidatorClientHttpClient { &self, url: U, body: &T, + ) -> Result { + let response = self.post_with_raw_response(url, body).await?; + self.signed_json(response).await + } + + async fn post_with_unsigned_response( + &self, + url: U, + body: &T, ) -> Result { let response = self.post_with_raw_response(url, body).await?; Ok(response.json().await?) @@ -162,11 +314,13 @@ impl ValidatorClientHttpClient { let response = self .client .patch(url) + .headers(self.headers()?) .json(body) .send() .await .map_err(Error::from)?; - ok_or_error(response).await?; + let response = ok_or_error(response).await?; + self.signed_body(response).await?; Ok(()) } @@ -179,6 +333,7 @@ impl ValidatorClientHttpClient { let response = self .client .delete(url) + .headers(self.headers()?) .json(body) .send() .await @@ -432,10 +587,20 @@ impl ValidatorClientHttpClient { Ok(url) } + /// `GET lighthouse/auth` + pub async fn get_auth(&self) -> Result { + let mut url = self.server.full.clone(); + url.path_segments_mut() + .map_err(|()| Error::InvalidUrl(self.server.clone()))? + .push("lighthouse") + .push("auth"); + self.get_unsigned(url).await + } + /// `GET eth/v1/keystores` pub async fn get_keystores(&self) -> Result { let url = self.make_keystores_url()?; - self.get(url).await + self.get_unsigned(url).await } /// `POST eth/v1/keystores` @@ -444,7 +609,7 @@ impl ValidatorClientHttpClient { req: &ImportKeystoresRequest, ) -> Result { let url = self.make_keystores_url()?; - self.post(url, req).await + self.post_with_unsigned_response(url, req).await } /// `DELETE eth/v1/keystores` @@ -459,7 +624,7 @@ impl ValidatorClientHttpClient { /// `GET eth/v1/remotekeys` pub async fn get_remotekeys(&self) -> Result { let url = self.make_remotekeys_url()?; - self.get(url).await + self.get_unsigned(url).await } /// `POST eth/v1/remotekeys` @@ -468,7 +633,7 @@ impl ValidatorClientHttpClient { req: &ImportRemotekeysRequest, ) -> Result { let url = self.make_remotekeys_url()?; - self.post(url, req).await + self.post_with_unsigned_response(url, req).await } /// `DELETE eth/v1/remotekeys` diff --git a/validator_client/src/http_api/api_secret.rs b/validator_client/src/http_api/api_secret.rs new file mode 100644 index 00000000000..e688792ddc1 --- /dev/null +++ b/validator_client/src/http_api/api_secret.rs @@ -0,0 +1,211 @@ +use eth2::lighthouse_vc::{PK_LEN, SECRET_PREFIX as PK_PREFIX}; +use filesystem::create_with_600_perms; +use libsecp256k1::{Message, PublicKey, SecretKey}; +use rand::thread_rng; +use ring::digest::{digest, SHA256}; +use std::fs; +use std::path::{Path, PathBuf}; +use warp::Filter; + +/// The name of the file which stores the secret key. +/// +/// It is purposefully opaque to prevent users confusing it with the "secret" that they need to +/// share with API consumers (which is actually the public key). +pub const SK_FILENAME: &str = ".secp-sk"; + +/// Length of the raw secret key, in bytes. +pub const SK_LEN: usize = 32; + +/// The name of the file which stores the public key. +/// +/// For users, this public key is a "secret" that can be shared with API consumers to provide them +/// access to the API. We avoid calling it a "public" key to users, since they should not post this +/// value in a public forum. +pub const PK_FILENAME: &str = "api-token.txt"; + +/// Contains a `secp256k1` keypair that is saved-to/loaded-from disk on instantiation. The keypair +/// is used for authorization/authentication for requests/responses on the HTTP API. +/// +/// Provides convenience functions to ultimately provide: +/// +/// - A signature across outgoing HTTP responses, applied to the `Signature` header. +/// - Verification of proof-of-knowledge of the public key in `self` for incoming HTTP requests, +/// via the `Authorization` header. +/// +/// The aforementioned scheme was first defined here: +/// +/// https://github.com/sigp/lighthouse/issues/1269#issuecomment-649879855 +pub struct ApiSecret { + pk: PublicKey, + sk: SecretKey, + pk_path: PathBuf, +} + +impl ApiSecret { + /// If both the secret and public keys are already on-disk, parse them and ensure they're both + /// from the same keypair. + /// + /// The provided `dir` is a directory containing two files, `SK_FILENAME` and `PK_FILENAME`. + /// + /// If either the secret or public key files are missing on disk, create a new keypair and + /// write it to disk (over-writing any existing files). + pub fn create_or_open>(dir: P) -> Result { + let sk_path = dir.as_ref().join(SK_FILENAME); + let pk_path = dir.as_ref().join(PK_FILENAME); + + if !(sk_path.exists() && pk_path.exists()) { + let sk = SecretKey::random(&mut thread_rng()); + let pk = PublicKey::from_secret_key(&sk); + + // Create and write the secret key to file with appropriate permissions + create_with_600_perms( + &sk_path, + serde_utils::hex::encode(sk.serialize()).as_bytes(), + ) + .map_err(|e| { + format!( + "Unable to create file with permissions for {:?}: {:?}", + sk_path, e + ) + })?; + + // Create and write the public key to file with appropriate permissions + create_with_600_perms( + &pk_path, + format!( + "{}{}", + PK_PREFIX, + serde_utils::hex::encode(&pk.serialize_compressed()[..]) + ) + .as_bytes(), + ) + .map_err(|e| { + format!( + "Unable to create file with permissions for {:?}: {:?}", + pk_path, e + ) + })?; + } + + let sk = fs::read(&sk_path) + .map_err(|e| format!("cannot read {}: {}", SK_FILENAME, e)) + .and_then(|bytes| { + serde_utils::hex::decode(&String::from_utf8_lossy(&bytes)) + .map_err(|_| format!("{} should be 0x-prefixed hex", PK_FILENAME)) + }) + .and_then(|bytes| { + if bytes.len() == SK_LEN { + let mut array = [0; SK_LEN]; + array.copy_from_slice(&bytes); + SecretKey::parse(&array).map_err(|e| format!("invalid {}: {}", SK_FILENAME, e)) + } else { + Err(format!( + "{} expected {} bytes not {}", + SK_FILENAME, + SK_LEN, + bytes.len() + )) + } + })?; + + let pk = fs::read(&pk_path) + .map_err(|e| format!("cannot read {}: {}", PK_FILENAME, e)) + .and_then(|bytes| { + let hex = + String::from_utf8(bytes).map_err(|_| format!("{} is not utf8", SK_FILENAME))?; + if let Some(stripped) = hex.strip_prefix(PK_PREFIX) { + serde_utils::hex::decode(stripped) + .map_err(|_| format!("{} should be 0x-prefixed hex", SK_FILENAME)) + } else { + Err(format!("unable to parse {}", SK_FILENAME)) + } + }) + .and_then(|bytes| { + if bytes.len() == PK_LEN { + let mut array = [0; PK_LEN]; + array.copy_from_slice(&bytes); + PublicKey::parse_compressed(&array) + .map_err(|e| format!("invalid {}: {}", PK_FILENAME, e)) + } else { + Err(format!( + "{} expected {} bytes not {}", + PK_FILENAME, + PK_LEN, + bytes.len() + )) + } + })?; + + // Ensure that the keys loaded from disk are indeed a pair. + if PublicKey::from_secret_key(&sk) != pk { + fs::remove_file(&sk_path) + .map_err(|e| format!("unable to remove {}: {}", SK_FILENAME, e))?; + fs::remove_file(&pk_path) + .map_err(|e| format!("unable to remove {}: {}", PK_FILENAME, e))?; + return Err(format!( + "{:?} does not match {:?} and the files have been deleted. Please try again.", + sk_path, pk_path + )); + } + + Ok(Self { pk, sk, pk_path }) + } + + /// Returns the public key of `self` as a 0x-prefixed hex string. + fn pubkey_string(&self) -> String { + serde_utils::hex::encode(&self.pk.serialize_compressed()[..]) + } + + /// Returns the API token. + pub fn api_token(&self) -> String { + format!("{}{}", PK_PREFIX, self.pubkey_string()) + } + + /// Returns the path for the API token file + pub fn api_token_path(&self) -> PathBuf { + self.pk_path.clone() + } + + /// Returns the values of the `Authorization` header which indicate a valid incoming HTTP + /// request. + /// + /// For backwards-compatibility we accept the token in a basic authentication style, but this is + /// technically invalid according to RFC 7617 because the token is not a base64-encoded username + /// and password. As such, bearer authentication should be preferred. + fn auth_header_values(&self) -> Vec { + vec![ + format!("Basic {}", self.api_token()), + format!("Bearer {}", self.api_token()), + ] + } + + /// Returns a `warp` header which filters out request that have a missing or inaccurate + /// `Authorization` header. + pub fn authorization_header_filter(&self) -> warp::filters::BoxedFilter<()> { + let expected = self.auth_header_values(); + warp::any() + .map(move || expected.clone()) + .and(warp::filters::header::header("Authorization")) + .and_then(move |expected: Vec, header: String| async move { + if expected.contains(&header) { + Ok(()) + } else { + Err(warp_utils::reject::invalid_auth(header)) + } + }) + .untuple_one() + .boxed() + } + + /// Returns a closure which produces a signature over some bytes using the secret key in + /// `self`. The signature is a 32-byte hash formatted as a 0x-prefixed string. + pub fn signer(&self) -> impl Fn(&[u8]) -> String + Clone { + let sk = self.sk; + move |input: &[u8]| -> String { + let message = + Message::parse_slice(digest(&SHA256, input).as_ref()).expect("sha256 is 32 bytes"); + let (signature, _) = libsecp256k1::sign(&message, &sk); + serde_utils::hex::encode(signature.serialize_der().as_ref()) + } + } +} From dd1bea8c390699ea1b86fa806ccc43406f55a171 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Thu, 25 Apr 2024 00:10:09 +0300 Subject: [PATCH 05/13] merge unstable --- validator_manager/src/common.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/validator_manager/src/common.rs b/validator_manager/src/common.rs index dc5c5949ae1..871c5362030 100644 --- a/validator_manager/src/common.rs +++ b/validator_manager/src/common.rs @@ -1,4 +1,4 @@ -use account_utils::ZeroizeString; +use account_utils::{strip_off_newlines, ZeroizeString}; use eth2::lighthouse_vc::std_types::{InterchangeJsonStr, KeystoreJsonStr}; use eth2::{ lighthouse_vc::{ @@ -317,10 +317,16 @@ mod bytes_4_without_0x_prefix { } } -pub async fn vc_http_client( +pub async fn vc_http_client>( url: SensitiveUrl, + token_path: P, ) -> Result<(ValidatorClientHttpClient, Vec), String> { - let http_client = ValidatorClientHttpClient::new(url.clone()).map_err(|e| { + let token_path = token_path.as_ref(); + let token_bytes = + fs::read(token_path).map_err(|e| format!("Failed to read {:?}: {:?}", token_path, e))?; + let token_string = String::from_utf8(strip_off_newlines(token_bytes)) + .map_err(|e| format!("Failed to parse {:?} as utf8: {:?}", token_path, e))?; + let http_client = ValidatorClientHttpClient::new(url.clone(), token_string).map_err(|e| { format!( "Could not instantiate HTTP client from URL and secret: {:?}", e From dad35b69220c52b9c02687fd2143323f93decd9b Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Thu, 25 Apr 2024 00:30:40 +0300 Subject: [PATCH 06/13] revert --- lighthouse/tests/validator_manager.rs | 12 ++ validator_client/src/http_api/mod.rs | 45 ++++- validator_client/src/http_api/test_utils.rs | 59 +++++- validator_client/src/http_api/tests.rs | 185 +++++++++++++++++- .../src/http_api/tests/keystores.rs | 17 ++ validator_client/src/lib.rs | 4 + validator_manager/src/import_validators.rs | 8 +- validator_manager/src/move_validators.rs | 28 ++- 8 files changed, 345 insertions(+), 13 deletions(-) diff --git a/lighthouse/tests/validator_manager.rs b/lighthouse/tests/validator_manager.rs index ea209054e7a..fab1cfebf4b 100644 --- a/lighthouse/tests/validator_manager.rs +++ b/lighthouse/tests/validator_manager.rs @@ -196,6 +196,7 @@ pub fn validator_import_defaults() { let expected = ImportConfig { validators_file_path: PathBuf::from("./vals.json"), vc_url: SensitiveUrl::parse("http://localhost:5062").unwrap(), + vc_token_path: PathBuf::from("./token.json"), ignore_duplicates: false, }; assert_eq!(expected, config); @@ -212,6 +213,7 @@ pub fn validator_import_misc_flags() { let expected = ImportConfig { validators_file_path: PathBuf::from("./vals.json"), vc_url: SensitiveUrl::parse("http://localhost:5062").unwrap(), + vc_token_path: PathBuf::from("./token.json"), ignore_duplicates: true, }; assert_eq!(expected, config); @@ -243,7 +245,9 @@ pub fn validator_move_defaults() { .assert_success(|config| { let expected = MoveConfig { src_vc_url: SensitiveUrl::parse("http://localhost:1").unwrap(), + src_vc_token_path: PathBuf::from("./1.json"), dest_vc_url: SensitiveUrl::parse("http://localhost:2").unwrap(), + dest_vc_token_path: PathBuf::from("./2.json"), validators: Validators::All, builder_proposals: None, builder_boost_factor: None, @@ -276,7 +280,9 @@ pub fn validator_move_misc_flags_0() { .assert_success(|config| { let expected = MoveConfig { src_vc_url: SensitiveUrl::parse("http://localhost:1").unwrap(), + src_vc_token_path: PathBuf::from("./1.json"), dest_vc_url: SensitiveUrl::parse("http://localhost:2").unwrap(), + dest_vc_token_path: PathBuf::from("./2.json"), validators: Validators::Specific(vec![ PublicKeyBytes::from_str(EXAMPLE_PUBKEY_0).unwrap(), PublicKeyBytes::from_str(EXAMPLE_PUBKEY_1).unwrap(), @@ -305,7 +311,9 @@ pub fn validator_move_misc_flags_1() { .assert_success(|config| { let expected = MoveConfig { src_vc_url: SensitiveUrl::parse("http://localhost:1").unwrap(), + src_vc_token_path: PathBuf::from("./1.json"), dest_vc_url: SensitiveUrl::parse("http://localhost:2").unwrap(), + dest_vc_token_path: PathBuf::from("./2.json"), validators: Validators::Specific(vec![ PublicKeyBytes::from_str(EXAMPLE_PUBKEY_0).unwrap() ]), @@ -335,7 +343,9 @@ pub fn validator_move_misc_flags_2() { .assert_success(|config| { let expected = MoveConfig { src_vc_url: SensitiveUrl::parse("http://localhost:1").unwrap(), + src_vc_token_path: PathBuf::from("./1.json"), dest_vc_url: SensitiveUrl::parse("http://localhost:2").unwrap(), + dest_vc_token_path: PathBuf::from("./2.json"), validators: Validators::Specific(vec![ PublicKeyBytes::from_str(EXAMPLE_PUBKEY_0).unwrap() ]), @@ -363,7 +373,9 @@ pub fn validator_move_count() { .assert_success(|config| { let expected = MoveConfig { src_vc_url: SensitiveUrl::parse("http://localhost:1").unwrap(), + src_vc_token_path: PathBuf::from("./1.json"), dest_vc_url: SensitiveUrl::parse("http://localhost:2").unwrap(), + dest_vc_token_path: PathBuf::from("./2.json"), validators: Validators::Count(42), builder_proposals: None, builder_boost_factor: None, diff --git a/validator_client/src/http_api/mod.rs b/validator_client/src/http_api/mod.rs index b41b2abf690..cf5bc39139f 100644 --- a/validator_client/src/http_api/mod.rs +++ b/validator_client/src/http_api/mod.rs @@ -4,6 +4,7 @@ mod graffiti; mod keystores; mod remotekeys; mod tests; +mod api_secret; pub mod test_utils; @@ -19,7 +20,7 @@ use create_validator::{ create_validators_mnemonic, create_validators_web3signer, get_voting_password_storage, }; use eth2::lighthouse_vc::{ - std_types::{GetFeeRecipientResponse, GetGasLimitResponse}, + std_types::{AuthResponse, GetFeeRecipientResponse, GetGasLimitResponse}, types::{ self as api_types, GenericResponse, GetGraffitiResponse, Graffiti, PublicKey, PublicKeyBytes, SetGraffitiRequest, @@ -29,7 +30,7 @@ use lighthouse_version::version_with_platform; use logging::SSELoggingComponents; use parking_lot::RwLock; use serde::{Deserialize, Serialize}; -use slog::{crit, info, Logger}; +use slog::{warn, crit, info, Logger}; use slot_clock::SlotClock; use std::collections::HashMap; use std::future::Future; @@ -53,6 +54,7 @@ use warp::{ Filter, }; use warp_utils::reject::convert_rejection; +pub use api_secret::ApiSecret; #[derive(Debug)] pub enum Error { @@ -77,6 +79,7 @@ impl From for Error { /// The server will gracefully handle the case where any fields are `None`. pub struct Context { pub task_executor: TaskExecutor, + pub api_secret: ApiSecret, pub validator_store: Option>>, pub validator_dir: Option, pub secrets_dir: Option, @@ -159,6 +162,21 @@ pub fn serve( )); } + let authorization_header_filter = ctx.api_secret.authorization_header_filter(); + let mut api_token_path = ctx.api_secret.api_token_path(); + + // Attempt to convert the path to an absolute path, but don't error if it fails. + match api_token_path.canonicalize() { + Ok(abs_path) => api_token_path = abs_path, + Err(e) => { + warn!( + log, + "Error canonicalizing token path"; + "error" => ?e, + ); + } + }; + let inner_validator_store = ctx.validator_store.clone(); let validator_store_filter = warp::any() .map(move || inner_validator_store.clone()) @@ -210,6 +228,9 @@ pub fn serve( let inner_spec = Arc::new(ctx.spec.clone()); let spec_filter = warp::any().map(move || inner_spec.clone()); + let api_token_path_inner = api_token_path.clone(); + let api_token_path_filter = warp::any().map(move || api_token_path_inner.clone()); + // Filter for SEE Logging events let inner_components = ctx.sse_logging_components.clone(); let sse_component_filter = warp::any().map(move || inner_components.clone()); @@ -739,6 +760,18 @@ pub fn serve( }) }, ); + + // GET /lighthouse/auth + let get_auth = warp::path("lighthouse").and(warp::path("auth").and(warp::path::end())); + let get_auth = get_auth + .and(api_token_path_filter) + .then( move|token_path: PathBuf| { + blocking_signed_json_task(move || { + Ok(AuthResponse { + token_path: token_path.display().to_string(), + }) + }) + }); // DELETE /lighthouse/keystores let delete_lighthouse_keystores = warp::path("lighthouse") @@ -1213,6 +1246,11 @@ pub fn serve( }); let routes = warp::any() + .and(authorization_header_filter) + // Note: it is critical that the `authorization_header_filter` is applied to all routes. + // Keeping all the routes inside the following `and` is a reliable way to achieve this. + // + // When adding a route, don't forget to add it to the `routes_with_invalid_auth` tests! .and( warp::get() .and( @@ -1228,7 +1266,6 @@ pub fn serve( .or(get_graffiti) .or(get_std_keystores) .or(get_std_remotekeys) - .or(get_log_events) .recover(warp_utils::reject::handle_rejection), ) .or(warp::post().and( @@ -1256,6 +1293,8 @@ pub fn serve( .recover(warp_utils::reject::handle_rejection), )), ) + // The auth route and logs are the only routes that are allowed to be accessed without the API token. + .or(warp::get().and(get_auth.or(get_log_events.boxed()))) // Maps errors into HTTP responses. .recover(warp_utils::reject::handle_rejection) // Add a `Server` header. diff --git a/validator_client/src/http_api/test_utils.rs b/validator_client/src/http_api/test_utils.rs index f2929197e8a..8bb56e87a32 100644 --- a/validator_client/src/http_api/test_utils.rs +++ b/validator_client/src/http_api/test_utils.rs @@ -1,7 +1,7 @@ use crate::doppelganger_service::DoppelgangerService; use crate::key_cache::{KeyCache, CACHE_FILENAME}; use crate::{ - http_api::{Config as HttpConfig, Context}, + http_api::{ApiSecret, Config as HttpConfig, Context}, initialized_validators::{InitializedValidators, OnDecryptFailure}, Config, ValidatorDefinitions, ValidatorStore, }; @@ -10,13 +10,18 @@ use account_utils::{ ZeroizeString, }; use deposit_contract::decode_eth1_tx_data; -use eth2::lighthouse_vc::{http_client::ValidatorClientHttpClient, types::*}; +use eth2::{ + lighthouse_vc::{http_client::ValidatorClientHttpClient, types::*}, + types::ErrorMessage as ApiErrorMessage, + Error as ApiError, +}; use eth2_keystore::KeystoreBuilder; use logging::test_logger; use parking_lot::RwLock; use sensitive_url::SensitiveUrl; use slashing_protection::{SlashingDatabase, SLASHING_PROTECTION_FILENAME}; use slot_clock::{SlotClock, TestingSlotClock}; +use std::future::Future; use std::marker::PhantomData; use std::net::{IpAddr, Ipv4Addr}; use std::sync::Arc; @@ -52,6 +57,7 @@ pub struct ApiTester { pub initialized_validators: Arc>, pub validator_store: Arc>, pub url: SensitiveUrl, + pub api_token: String, pub test_runtime: TestRuntime, pub _server_shutdown: oneshot::Sender<()>, pub validator_dir: TempDir, @@ -80,6 +86,9 @@ impl ApiTester { .await .unwrap(); + let api_secret = ApiSecret::create_or_open(validator_dir.path()).unwrap(); + let api_pubkey = api_secret.api_token(); + let config = Config { validator_dir: validator_dir.path().into(), secrets_dir: secrets_dir.path().into(), @@ -117,6 +126,7 @@ impl ApiTester { let context = Arc::new(Context { task_executor: test_runtime.task_executor.clone(), + api_secret, validator_dir: Some(validator_dir.path().into()), secrets_dir: Some(secrets_dir.path().into()), validator_store: Some(validator_store.clone()), @@ -146,13 +156,14 @@ impl ApiTester { )) .unwrap(); - let client = ValidatorClientHttpClient::new(url.clone()).unwrap(); + let client = ValidatorClientHttpClient::new(url.clone(), api_pubkey.clone()).unwrap(); Self { client, initialized_validators, validator_store, url, + api_token: api_pubkey, test_runtime, _server_shutdown: shutdown_tx, validator_dir, @@ -189,6 +200,48 @@ impl ApiTester { .expect("key cache should decypt"); } + pub fn invalid_token_client(&self) -> ValidatorClientHttpClient { + let tmp = tempdir().unwrap(); + let api_secret = ApiSecret::create_or_open(tmp.path()).unwrap(); + let invalid_pubkey = api_secret.api_token(); + ValidatorClientHttpClient::new(self.url.clone(), invalid_pubkey).unwrap() + } + + pub async fn test_with_invalid_auth(self, func: F) -> Self + where + F: Fn(ValidatorClientHttpClient) -> A, + A: Future>, + { + /* + * Test with an invalid Authorization header. + */ + match func(self.invalid_token_client()).await { + Err(ApiError::ServerMessage(ApiErrorMessage { code: 403, .. })) => (), + Err(other) => panic!("expected authorized error, got {:?}", other), + Ok(_) => panic!("expected authorized error, got Ok"), + } + + /* + * Test with a missing Authorization header. + */ + let mut missing_token_client = self.client.clone(); + missing_token_client.send_authorization_header(false); + match func(missing_token_client).await { + Err(ApiError::ServerMessage(ApiErrorMessage { + code: 401, message, .. + })) if message.contains("missing Authorization header") => (), + Err(other) => panic!("expected missing header error, got {:?}", other), + Ok(_) => panic!("expected missing header error, got Ok"), + } + + self + } + + pub fn invalidate_api_token(mut self) -> Self { + self.client = self.invalid_token_client(); + self + } + pub async fn test_get_lighthouse_version_invalid(self) -> Self { self.client.get_lighthouse_version().await.unwrap_err(); self diff --git a/validator_client/src/http_api/tests.rs b/validator_client/src/http_api/tests.rs index d1fda5b4721..ce1937d4379 100644 --- a/validator_client/src/http_api/tests.rs +++ b/validator_client/src/http_api/tests.rs @@ -5,7 +5,7 @@ mod keystores; use crate::doppelganger_service::DoppelgangerService; use crate::{ - http_api::{Config as HttpConfig, Context}, + http_api::{ApiSecret, Config as HttpConfig, Context}, initialized_validators::InitializedValidators, Config, ValidatorDefinitions, ValidatorStore, }; @@ -74,6 +74,9 @@ impl ApiTester { .await .unwrap(); + let api_secret = ApiSecret::create_or_open(validator_dir.path()).unwrap(); + let api_pubkey = api_secret.api_token(); + config.validator_dir = validator_dir.path().into(); config.secrets_dir = secrets_dir.path().into(); @@ -111,6 +114,7 @@ impl ApiTester { let context = Arc::new(Context { task_executor: test_runtime.task_executor.clone(), + api_secret, validator_dir: Some(validator_dir.path().into()), secrets_dir: Some(secrets_dir.path().into()), validator_store: Some(validator_store.clone()), @@ -143,7 +147,7 @@ impl ApiTester { )) .unwrap(); - let client = ValidatorClientHttpClient::new(url.clone()).unwrap(); + let client = ValidatorClientHttpClient::new(url.clone(), api_pubkey).unwrap(); Self { client, @@ -156,6 +160,48 @@ impl ApiTester { } } + pub fn invalid_token_client(&self) -> ValidatorClientHttpClient { + let tmp = tempdir().unwrap(); + let api_secret = ApiSecret::create_or_open(tmp.path()).unwrap(); + let invalid_pubkey = api_secret.api_token(); + ValidatorClientHttpClient::new(self.url.clone(), invalid_pubkey.clone()).unwrap() + } + + pub async fn test_with_invalid_auth(self, func: F) -> Self + where + F: Fn(ValidatorClientHttpClient) -> A, + A: Future>, + { + /* + * Test with an invalid Authorization header. + */ + match func(self.invalid_token_client()).await { + Err(ApiError::ServerMessage(ApiErrorMessage { code: 403, .. })) => (), + Err(other) => panic!("expected authorized error, got {:?}", other), + Ok(_) => panic!("expected authorized error, got Ok"), + } + + /* + * Test with a missing Authorization header. + */ + let mut missing_token_client = self.client.clone(); + missing_token_client.send_authorization_header(false); + match func(missing_token_client).await { + Err(ApiError::ServerMessage(ApiErrorMessage { + code: 401, message, .. + })) if message.contains("missing Authorization header") => (), + Err(other) => panic!("expected missing header error, got {:?}", other), + Ok(_) => panic!("expected missing header error, got Ok"), + } + + self + } + + pub fn invalidate_api_token(mut self) -> Self { + self.client = self.invalid_token_client(); + self + } + pub async fn test_get_lighthouse_version_invalid(self) -> Self { self.client.get_lighthouse_version().await.unwrap_err(); self @@ -783,6 +829,141 @@ struct Web3SignerValidatorScenario { enabled: bool, } +#[tokio::test] +async fn invalid_pubkey() { + ApiTester::new() + .await + .invalidate_api_token() + .test_get_lighthouse_version_invalid() + .await; +} + +#[tokio::test] +async fn routes_with_invalid_auth() { + ApiTester::new() + .await + .test_with_invalid_auth(|client| async move { client.get_lighthouse_version().await }) + .await + .test_with_invalid_auth(|client| async move { client.get_lighthouse_health().await }) + .await + .test_with_invalid_auth(|client| async move { + client.get_lighthouse_spec::().await + }) + .await + .test_with_invalid_auth(|client| async move { client.get_lighthouse_validators().await }) + .await + .test_with_invalid_auth(|client| async move { + client + .get_lighthouse_validators_pubkey(&PublicKeyBytes::empty()) + .await + }) + .await + .test_with_invalid_auth(|client| async move { + client + .post_lighthouse_validators(vec![ValidatorRequest { + enable: <_>::default(), + description: <_>::default(), + graffiti: <_>::default(), + suggested_fee_recipient: <_>::default(), + gas_limit: <_>::default(), + builder_proposals: <_>::default(), + deposit_gwei: <_>::default(), + builder_boost_factor: <_>::default(), + prefer_builder_proposals: <_>::default(), + }]) + .await + }) + .await + .test_with_invalid_auth(|client| async move { + client + .post_lighthouse_validators_mnemonic(&CreateValidatorsMnemonicRequest { + mnemonic: String::default().into(), + key_derivation_path_offset: <_>::default(), + validators: <_>::default(), + }) + .await + }) + .await + .test_with_invalid_auth(|client| async move { + let password = random_password(); + let keypair = Keypair::random(); + let keystore = KeystoreBuilder::new(&keypair, password.as_bytes(), String::new()) + .unwrap() + .build() + .unwrap(); + client + .post_lighthouse_validators_keystore(&KeystoreValidatorsPostRequest { + password: String::default().into(), + enable: <_>::default(), + keystore, + graffiti: <_>::default(), + suggested_fee_recipient: <_>::default(), + gas_limit: <_>::default(), + builder_proposals: <_>::default(), + builder_boost_factor: <_>::default(), + prefer_builder_proposals: <_>::default(), + }) + .await + }) + .await + .test_with_invalid_auth(|client| async move { + client + .patch_lighthouse_validators( + &PublicKeyBytes::empty(), + Some(false), + None, + None, + None, + None, + None, + ) + .await + }) + .await + .test_with_invalid_auth(|client| async move { client.get_keystores().await }) + .await + .test_with_invalid_auth(|client| async move { + let password = random_password_string(); + let keypair = Keypair::random(); + let keystore = KeystoreBuilder::new(&keypair, password.as_ref(), String::new()) + .unwrap() + .build() + .map(KeystoreJsonStr) + .unwrap(); + client + .post_keystores(&ImportKeystoresRequest { + keystores: vec![keystore], + passwords: vec![password], + slashing_protection: None, + }) + .await + }) + .await + .test_with_invalid_auth(|client| async move { + let keypair = Keypair::random(); + client + .delete_keystores(&DeleteKeystoresRequest { + pubkeys: vec![keypair.pk.compress()], + }) + .await + }) + .await + .test_with_invalid_auth(|client| async move { + client.delete_graffiti(&PublicKeyBytes::empty()).await + }) + .await + .test_with_invalid_auth(|client| async move { + client.get_graffiti(&PublicKeyBytes::empty()).await + }) + .await + .test_with_invalid_auth(|client| async move { + client + .set_graffiti(&PublicKeyBytes::empty(), GraffitiString::default()) + .await + }) + .await; +} + #[tokio::test] async fn simple_getters() { ApiTester::new() diff --git a/validator_client/src/http_api/tests/keystores.rs b/validator_client/src/http_api/tests/keystores.rs index 04b31abb294..fe58393bb8d 100644 --- a/validator_client/src/http_api/tests/keystores.rs +++ b/validator_client/src/http_api/tests/keystores.rs @@ -194,6 +194,23 @@ fn check_remotekey_delete_response( } } +#[tokio::test] +async fn get_auth_no_token() { + run_test(|mut tester| async move { + let _ = &tester; + tester.client.send_authorization_header(false); + let auth_response = tester.client.get_auth().await.unwrap(); + + // Load the file from the returned path. + let token_path = Path::new(&auth_response.token_path); + let token = HttpClient::load_api_token_from_file(token_path).unwrap(); + + // The token should match the one that the client was originally initialised with. + assert!(tester.client.api_token() == Some(&token)); + }) + .await; +} + #[tokio::test] async fn get_empty_keystores() { run_test(|tester| async move { diff --git a/validator_client/src/lib.rs b/validator_client/src/lib.rs index 4c467e7afd4..268c25cdf7d 100644 --- a/validator_client/src/lib.rs +++ b/validator_client/src/lib.rs @@ -42,6 +42,7 @@ use clap::ArgMatches; use duties_service::{sync::SyncDutiesMap, DutiesService}; use environment::RuntimeContext; use eth2::{reqwest::ClientBuilder, types::Graffiti, BeaconNodeHttpClient, StatusCode, Timeouts}; +use http_api::ApiSecret; use notifier::spawn_notifier; use parking_lot::RwLock; use preparation_service::{PreparationService, PreparationServiceBuilder}; @@ -531,9 +532,12 @@ impl ProductionValidatorClient { let (block_service_tx, block_service_rx) = mpsc::channel(channel_capacity); let log = self.context.log(); + let api_secret = ApiSecret::create_or_open(&self.config.validator_dir)?; + self.http_api_listen_addr = if self.config.http_api.enabled { let ctx = Arc::new(http_api::Context { task_executor: self.context.executor.clone(), + api_secret, validator_store: Some(self.validator_store.clone()), validator_dir: Some(self.config.validator_dir.clone()), secrets_dir: Some(self.config.secrets_dir.clone()), diff --git a/validator_manager/src/import_validators.rs b/validator_manager/src/import_validators.rs index db90f8b37e2..4b924189f20 100644 --- a/validator_manager/src/import_validators.rs +++ b/validator_manager/src/import_validators.rs @@ -71,6 +71,7 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> { pub struct ImportConfig { pub validators_file_path: PathBuf, pub vc_url: SensitiveUrl, + pub vc_token_path: PathBuf, pub ignore_duplicates: bool, } @@ -79,6 +80,7 @@ impl ImportConfig { Ok(Self { validators_file_path: clap_utils::parse_required(matches, VALIDATORS_FILE_FLAG)?, vc_url: clap_utils::parse_required(matches, VC_URL_FLAG)?, + vc_token_path: clap_utils::parse_required(matches, VC_TOKEN_FLAG)?, ignore_duplicates: matches.is_present(IGNORE_DUPLICATES_FLAG), }) } @@ -100,6 +102,7 @@ async fn run<'a>(config: ImportConfig) -> Result<(), String> { let ImportConfig { validators_file_path, vc_url, + vc_token_path, ignore_duplicates, } = config; @@ -122,7 +125,7 @@ async fn run<'a>(config: ImportConfig) -> Result<(), String> { let count = validators.len(); - let (http_client, _keystores) = vc_http_client(vc_url.clone()).await?; + let (http_client, _keystores) = vc_http_client(vc_url.clone(), &vc_token_path).await?; eprintln!( "Starting to submit {} validators to VC, each validator may take several seconds", @@ -258,12 +261,15 @@ pub mod tests { pub async fn new_with_http_config(http_config: HttpConfig) -> Self { let dir = tempdir().unwrap(); let vc = ApiTester::new_with_http_config(http_config).await; + let vc_token_path = dir.path().join(VC_TOKEN_FILE_NAME); + fs::write(&vc_token_path, &vc.api_token).unwrap(); Self { import_config: ImportConfig { // This field will be overwritten later on. validators_file_path: dir.path().into(), vc_url: vc.url.clone(), + vc_token_path, ignore_duplicates: false, }, vc, diff --git a/validator_manager/src/move_validators.rs b/validator_manager/src/move_validators.rs index c1b3eb5cf51..5826f2756be 100644 --- a/validator_manager/src/move_validators.rs +++ b/validator_manager/src/move_validators.rs @@ -14,6 +14,7 @@ use eth2::{ }; use serde::{Deserialize, Serialize}; use std::collections::{HashMap, HashSet}; +use std::path::PathBuf; use std::str::FromStr; use std::time::Duration; use tokio::time::sleep; @@ -207,7 +208,9 @@ pub enum Validators { #[derive(Clone, PartialEq, Debug, Serialize, Deserialize)] pub struct MoveConfig { pub src_vc_url: SensitiveUrl, + pub src_vc_token_path: PathBuf, pub dest_vc_url: SensitiveUrl, + pub dest_vc_token_path: PathBuf, pub validators: Validators, pub builder_proposals: Option, pub builder_boost_factor: Option, @@ -241,7 +244,9 @@ impl MoveConfig { Ok(Self { src_vc_url: clap_utils::parse_required(matches, SRC_VC_URL_FLAG)?, + src_vc_token_path: clap_utils::parse_required(matches, SRC_VC_TOKEN_FLAG)?, dest_vc_url: clap_utils::parse_required(matches, DEST_VC_URL_FLAG)?, + dest_vc_token_path: clap_utils::parse_required(matches, DEST_VC_TOKEN_FLAG)?, validators, builder_proposals: clap_utils::parse_optional(matches, BUILDER_PROPOSALS_FLAG)?, builder_boost_factor: clap_utils::parse_optional(matches, BUILDER_BOOST_FACTOR_FLAG)?, @@ -273,7 +278,9 @@ pub async fn cli_run<'a>( async fn run<'a>(config: MoveConfig) -> Result<(), String> { let MoveConfig { src_vc_url, + src_vc_token_path, dest_vc_url, + dest_vc_token_path, validators, builder_proposals, fee_recipient, @@ -292,8 +299,10 @@ async fn run<'a>(config: MoveConfig) -> Result<(), String> { )); } - let (src_http_client, src_keystores) = vc_http_client(src_vc_url.clone()).await?; - let (dest_http_client, _dest_keystores) = vc_http_client(dest_vc_url.clone()).await?; + let (src_http_client, src_keystores) = + vc_http_client(src_vc_url.clone(), &src_vc_token_path).await?; + let (dest_http_client, _dest_keystores) = + vc_http_client(dest_vc_url.clone(), &dest_vc_token_path).await?; if src_keystores.is_empty() { return Err(NO_VALIDATORS_MSG.to_string()); @@ -758,8 +767,12 @@ mod test { where F: Fn(&[PublicKeyBytes]) -> Validators, { + let src_vc_token_path = self.dir.path().join(SRC_VC_TOKEN_FILE_NAME); + fs::write(&src_vc_token_path, &src_vc.api_token).unwrap(); let (src_vc_client, src_vc_initial_keystores) = - vc_http_client(src_vc.url.clone()).await.unwrap(); + vc_http_client(src_vc.url.clone(), &src_vc_token_path) + .await + .unwrap(); let src_vc_initial_pubkeys: Vec<_> = src_vc_initial_keystores .iter() @@ -767,12 +780,19 @@ mod test { .collect(); let validators = gen_validators_enum(&src_vc_initial_pubkeys); + let dest_vc_token_path = self.dir.path().join(DEST_VC_TOKEN_FILE_NAME); + fs::write(&dest_vc_token_path, &dest_vc.api_token).unwrap(); + let (dest_vc_client, dest_vc_initial_keystores) = - vc_http_client(dest_vc.url.clone()).await.unwrap(); + vc_http_client(dest_vc.url.clone(), &dest_vc_token_path) + .await + .unwrap(); let move_config = MoveConfig { src_vc_url: src_vc.url.clone(), + src_vc_token_path, dest_vc_url: dest_vc.url.clone(), + dest_vc_token_path: dest_vc_token_path.clone(), validators: validators.clone(), builder_proposals: None, builder_boost_factor: None, From ea091abb16cb3daf074ff39774a68da6f629dcee Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Sat, 27 Apr 2024 14:28:19 +0300 Subject: [PATCH 07/13] refactor blocking json task --- common/warp_utils/src/task.rs | 9 +- validator_client/src/http_api/api_secret.rs | 89 ++------------- validator_client/src/http_api/mod.rs | 114 ++++++++++---------- validator_client/src/lib.rs | 1 + 4 files changed, 75 insertions(+), 138 deletions(-) diff --git a/common/warp_utils/src/task.rs b/common/warp_utils/src/task.rs index 001231f2c6b..c7ecda63289 100644 --- a/common/warp_utils/src/task.rs +++ b/common/warp_utils/src/task.rs @@ -1,5 +1,6 @@ use serde::Serialize; use warp::reply::{Reply, Response}; +use crate::reject::convert_rejection; /// A convenience wrapper around `blocking_task`. pub async fn blocking_task(func: F) -> Result @@ -24,14 +25,16 @@ where } /// A convenience wrapper around `blocking_task` for use with `warp` JSON responses. -pub async fn blocking_json_task(func: F) -> Result +pub async fn blocking_json_task(func: F) -> Response where F: FnOnce() -> Result + Send + 'static, T: Serialize + Send + 'static, { - blocking_response_task(|| { + let result = blocking_response_task(|| { let response = func()?; Ok(warp::reply::json(&response)) }) - .await + .await; + + convert_rejection(result).await } diff --git a/validator_client/src/http_api/api_secret.rs b/validator_client/src/http_api/api_secret.rs index e688792ddc1..ea1a22b6940 100644 --- a/validator_client/src/http_api/api_secret.rs +++ b/validator_client/src/http_api/api_secret.rs @@ -7,15 +7,6 @@ use std::fs; use std::path::{Path, PathBuf}; use warp::Filter; -/// The name of the file which stores the secret key. -/// -/// It is purposefully opaque to prevent users confusing it with the "secret" that they need to -/// share with API consumers (which is actually the public key). -pub const SK_FILENAME: &str = ".secp-sk"; - -/// Length of the raw secret key, in bytes. -pub const SK_LEN: usize = 32; - /// The name of the file which stores the public key. /// /// For users, this public key is a "secret" that can be shared with API consumers to provide them @@ -28,47 +19,34 @@ pub const PK_FILENAME: &str = "api-token.txt"; /// /// Provides convenience functions to ultimately provide: /// -/// - A signature across outgoing HTTP responses, applied to the `Signature` header. /// - Verification of proof-of-knowledge of the public key in `self` for incoming HTTP requests, /// via the `Authorization` header. /// /// The aforementioned scheme was first defined here: /// /// https://github.com/sigp/lighthouse/issues/1269#issuecomment-649879855 +/// +/// This scheme has since been tweaked to remove VC response signing +/// https://github.com/sigp/lighthouse/issues/5423 pub struct ApiSecret { pk: PublicKey, - sk: SecretKey, pk_path: PathBuf, } impl ApiSecret { - /// If both the secret and public keys are already on-disk, parse them and ensure they're both - /// from the same keypair. + /// If the public key is already on-disk, use it. /// - /// The provided `dir` is a directory containing two files, `SK_FILENAME` and `PK_FILENAME`. + /// The provided `dir` is a directory containing `PK_FILENAME`. /// - /// If either the secret or public key files are missing on disk, create a new keypair and + /// If the public key file is missing on disk, create a new key and /// write it to disk (over-writing any existing files). pub fn create_or_open>(dir: P) -> Result { - let sk_path = dir.as_ref().join(SK_FILENAME); let pk_path = dir.as_ref().join(PK_FILENAME); - if !(sk_path.exists() && pk_path.exists()) { + if !pk_path.exists() { let sk = SecretKey::random(&mut thread_rng()); let pk = PublicKey::from_secret_key(&sk); - // Create and write the secret key to file with appropriate permissions - create_with_600_perms( - &sk_path, - serde_utils::hex::encode(sk.serialize()).as_bytes(), - ) - .map_err(|e| { - format!( - "Unable to create file with permissions for {:?}: {:?}", - sk_path, e - ) - })?; - // Create and write the public key to file with appropriate permissions create_with_600_perms( &pk_path, @@ -87,37 +65,16 @@ impl ApiSecret { })?; } - let sk = fs::read(&sk_path) - .map_err(|e| format!("cannot read {}: {}", SK_FILENAME, e)) - .and_then(|bytes| { - serde_utils::hex::decode(&String::from_utf8_lossy(&bytes)) - .map_err(|_| format!("{} should be 0x-prefixed hex", PK_FILENAME)) - }) - .and_then(|bytes| { - if bytes.len() == SK_LEN { - let mut array = [0; SK_LEN]; - array.copy_from_slice(&bytes); - SecretKey::parse(&array).map_err(|e| format!("invalid {}: {}", SK_FILENAME, e)) - } else { - Err(format!( - "{} expected {} bytes not {}", - SK_FILENAME, - SK_LEN, - bytes.len() - )) - } - })?; - let pk = fs::read(&pk_path) .map_err(|e| format!("cannot read {}: {}", PK_FILENAME, e)) .and_then(|bytes| { let hex = - String::from_utf8(bytes).map_err(|_| format!("{} is not utf8", SK_FILENAME))?; + String::from_utf8(bytes).map_err(|_| format!("{} is not utf8", PK_FILENAME))?; if let Some(stripped) = hex.strip_prefix(PK_PREFIX) { serde_utils::hex::decode(stripped) - .map_err(|_| format!("{} should be 0x-prefixed hex", SK_FILENAME)) + .map_err(|_| format!("{} should be 0x-prefixed hex", PK_FILENAME)) } else { - Err(format!("unable to parse {}", SK_FILENAME)) + Err(format!("unable to parse {}", PK_FILENAME)) } }) .and_then(|bytes| { @@ -136,19 +93,7 @@ impl ApiSecret { } })?; - // Ensure that the keys loaded from disk are indeed a pair. - if PublicKey::from_secret_key(&sk) != pk { - fs::remove_file(&sk_path) - .map_err(|e| format!("unable to remove {}: {}", SK_FILENAME, e))?; - fs::remove_file(&pk_path) - .map_err(|e| format!("unable to remove {}: {}", PK_FILENAME, e))?; - return Err(format!( - "{:?} does not match {:?} and the files have been deleted. Please try again.", - sk_path, pk_path - )); - } - - Ok(Self { pk, sk, pk_path }) + Ok(Self { pk, pk_path }) } /// Returns the public key of `self` as a 0x-prefixed hex string. @@ -196,16 +141,4 @@ impl ApiSecret { .untuple_one() .boxed() } - - /// Returns a closure which produces a signature over some bytes using the secret key in - /// `self`. The signature is a 32-byte hash formatted as a 0x-prefixed string. - pub fn signer(&self) -> impl Fn(&[u8]) -> String + Clone { - let sk = self.sk; - move |input: &[u8]| -> String { - let message = - Message::parse_slice(digest(&SHA256, input).as_ref()).expect("sha256 is 32 bytes"); - let (signature, _) = libsecp256k1::sign(&message, &sk); - serde_utils::hex::encode(signature.serialize_der().as_ref()) - } - } } diff --git a/validator_client/src/http_api/mod.rs b/validator_client/src/http_api/mod.rs index cf5bc39139f..f6689a515be 100644 --- a/validator_client/src/http_api/mod.rs +++ b/validator_client/src/http_api/mod.rs @@ -269,7 +269,7 @@ pub fn serve( .and(warp::path("version")) .and(warp::path::end()) .then(|| { - blocking_signed_json_task(move || { + blocking_json_task(move || { Ok(api_types::GenericResponse::from(api_types::VersionData { version: version_with_platform(), })) @@ -281,7 +281,7 @@ pub fn serve( .and(warp::path("health")) .and(warp::path::end()) .then(|| { - blocking_signed_json_task(move || { + blocking_json_task(move || { eth2::lighthouse::Health::observe() .map(api_types::GenericResponse::from) .map_err(warp_utils::reject::custom_bad_request) @@ -294,7 +294,7 @@ pub fn serve( .and(warp::path::end()) .and(spec_filter.clone()) .then(|spec: Arc<_>| { - blocking_signed_json_task(move || { + blocking_json_task(move || { let config = ConfigAndPreset::from_chain_spec::(&spec, None); Ok(api_types::GenericResponse::from(config)) }) @@ -306,7 +306,7 @@ pub fn serve( .and(warp::path::end()) .and(validator_store_filter.clone()) .then(|validator_store: Arc>| { - blocking_signed_json_task(move || { + blocking_json_task(move || { let validators = validator_store .initialized_validators() .read() @@ -331,7 +331,7 @@ pub fn serve( .and(validator_store_filter.clone()) .then( |validator_pubkey: PublicKey, validator_store: Arc>| { - blocking_signed_json_task(move || { + blocking_json_task(move || { let validator = validator_store .initialized_validators() .read() @@ -364,7 +364,7 @@ pub fn serve( .and(app_start_filter) .and(validator_dir_filter.clone()) .then(|sysinfo, app_start: std::time::Instant, val_dir| { - blocking_signed_json_task(move || { + blocking_json_task(move || { let app_uptime = app_start.elapsed().as_secs(); Ok(api_types::GenericResponse::from(observe_system_health_vc( sysinfo, val_dir, app_uptime, @@ -385,7 +385,7 @@ pub fn serve( graffiti_file: Option, graffiti_flag: Option, log| { - blocking_signed_json_task(move || { + blocking_json_task(move || { let mut result = HashMap::new(); for (key, graffiti_definition) in validator_store .initialized_validators() @@ -423,7 +423,7 @@ pub fn serve( validator_store: Arc>, spec: Arc, task_executor: TaskExecutor| { - blocking_signed_json_task(move || { + blocking_json_task(move || { let secrets_dir = store_passwords_in_secrets_dir.then_some(secrets_dir); if let Some(handle) = task_executor.handle() { let (validators, mnemonic) = @@ -468,7 +468,7 @@ pub fn serve( validator_store: Arc>, spec: Arc, task_executor: TaskExecutor| { - blocking_signed_json_task(move || { + blocking_json_task(move || { let secrets_dir = store_passwords_in_secrets_dir.then_some(secrets_dir); if let Some(handle) = task_executor.handle() { let mnemonic = @@ -514,7 +514,7 @@ pub fn serve( secrets_dir: PathBuf, validator_store: Arc>, task_executor: TaskExecutor| { - blocking_signed_json_task(move || { + blocking_json_task(move || { // Check to ensure the password is correct. let keypair = body .keystore @@ -600,7 +600,7 @@ pub fn serve( |body: Vec, validator_store: Arc>, task_executor: TaskExecutor| { - blocking_signed_json_task(move || { + blocking_json_task(move || { if let Some(handle) = task_executor.handle() { let web3signers: Vec = body .into_iter() @@ -655,7 +655,7 @@ pub fn serve( validator_store: Arc>, graffiti_file: Option, task_executor: TaskExecutor| { - blocking_signed_json_task(move || { + blocking_json_task(move || { if body.graffiti.is_some() && graffiti_file.is_some() { return Err(warp_utils::reject::custom_bad_request( "Unable to update graffiti as the \"--graffiti-file\" flag is set" @@ -766,7 +766,7 @@ pub fn serve( let get_auth = get_auth .and(api_token_path_filter) .then( move|token_path: PathBuf| { - blocking_signed_json_task(move || { + blocking_json_task(move || { Ok(AuthResponse { token_path: token_path.display().to_string(), }) @@ -782,7 +782,7 @@ pub fn serve( .and(task_executor_filter.clone()) .and(log_filter.clone()) .then(move |request, validator_store, task_executor, log| { - blocking_signed_json_task(move || { + blocking_json_task(move || { if allow_keystore_export { keystores::export(request, validator_store, task_executor, log) } else { @@ -807,7 +807,7 @@ pub fn serve( .and(validator_store_filter.clone()) .then( |validator_pubkey: PublicKey, validator_store: Arc>| { - blocking_signed_json_task(move || { + blocking_json_task(move || { if validator_store .initialized_validators() .read() @@ -848,7 +848,7 @@ pub fn serve( |validator_pubkey: PublicKey, request: api_types::UpdateFeeRecipientRequest, validator_store: Arc>| { - blocking_signed_json_task(move || { + blocking_json_task(move || { if validator_store .initialized_validators() .read() @@ -884,7 +884,7 @@ pub fn serve( .and(validator_store_filter.clone()) .then( |validator_pubkey: PublicKey, validator_store: Arc>| { - blocking_signed_json_task(move || { + blocking_json_task(move || { if validator_store .initialized_validators() .read() @@ -920,7 +920,7 @@ pub fn serve( .and(validator_store_filter.clone()) .then( |validator_pubkey: PublicKey, validator_store: Arc>| { - blocking_signed_json_task(move || { + blocking_json_task(move || { if validator_store .initialized_validators() .read() @@ -953,7 +953,7 @@ pub fn serve( |validator_pubkey: PublicKey, request: api_types::UpdateGasLimitRequest, validator_store: Arc>| { - blocking_signed_json_task(move || { + blocking_json_task(move || { if validator_store .initialized_validators() .read() @@ -989,7 +989,7 @@ pub fn serve( .and(validator_store_filter.clone()) .then( |validator_pubkey: PublicKey, validator_store: Arc>| { - blocking_signed_json_task(move || { + blocking_json_task(move || { if validator_store .initialized_validators() .read() @@ -1034,7 +1034,7 @@ pub fn serve( slot_clock: T, log, task_executor: TaskExecutor| { - blocking_signed_json_task(move || { + blocking_json_task(move || { if let Some(handle) = task_executor.handle() { let signed_voluntary_exit = handle.block_on(create_signed_voluntary_exit( @@ -1066,7 +1066,7 @@ pub fn serve( |pubkey: PublicKey, validator_store: Arc>, graffiti_flag: Option| { - blocking_signed_json_task(move || { + blocking_json_task(move || { let graffiti = get_graffiti(pubkey.clone(), validator_store, graffiti_flag)?; Ok(GenericResponse::from(GetGraffitiResponse { pubkey: pubkey.into(), @@ -1090,7 +1090,7 @@ pub fn serve( query: SetGraffitiRequest, validator_store: Arc>, graffiti_file: Option| { - blocking_signed_json_task(move || { + blocking_json_task(move || { if graffiti_file.is_some() { return Err(warp_utils::reject::invalid_auth( "Unable to update graffiti as the \"--graffiti-file\" flag is set" @@ -1115,7 +1115,7 @@ pub fn serve( |pubkey: PublicKey, validator_store: Arc>, graffiti_file: Option| { - blocking_signed_json_task(move || { + blocking_json_task(move || { if graffiti_file.is_some() { return Err(warp_utils::reject::invalid_auth( "Unable to delete graffiti as the \"--graffiti-file\" flag is set" @@ -1131,7 +1131,7 @@ pub fn serve( // GET /eth/v1/keystores let get_std_keystores = std_keystores.and(validator_store_filter.clone()).then( |validator_store: Arc>| { - blocking_signed_json_task(move || Ok(keystores::list(validator_store))) + blocking_json_task(move || Ok(keystores::list(validator_store))) }, ); @@ -1146,7 +1146,7 @@ pub fn serve( .then( move |request, validator_dir, secrets_dir, validator_store, task_executor, log| { let secrets_dir = store_passwords_in_secrets_dir.then_some(secrets_dir); - blocking_signed_json_task(move || { + blocking_json_task(move || { keystores::import( request, validator_dir, @@ -1166,7 +1166,7 @@ pub fn serve( .and(task_executor_filter.clone()) .and(log_filter.clone()) .then(|request, validator_store, task_executor, log| { - blocking_signed_json_task(move || { + blocking_json_task(move || { keystores::delete(request, validator_store, task_executor, log) }) }); @@ -1174,7 +1174,7 @@ pub fn serve( // GET /eth/v1/remotekeys let get_std_remotekeys = std_remotekeys.and(validator_store_filter.clone()).then( |validator_store: Arc>| { - blocking_signed_json_task(move || Ok(remotekeys::list(validator_store))) + blocking_json_task(move || Ok(remotekeys::list(validator_store))) }, ); @@ -1185,7 +1185,7 @@ pub fn serve( .and(task_executor_filter.clone()) .and(log_filter.clone()) .then(|request, validator_store, task_executor, log| { - blocking_signed_json_task(move || { + blocking_json_task(move || { remotekeys::import(request, validator_store, task_executor, log) }) }); @@ -1197,7 +1197,7 @@ pub fn serve( .and(task_executor_filter) .and(log_filter.clone()) .then(|request, validator_store, task_executor, log| { - blocking_signed_json_task(move || { + blocking_json_task(move || { remotekeys::delete(request, validator_store, task_executor, log) }) }); @@ -1319,30 +1319,30 @@ pub fn serve( /// Executes `func` in blocking tokio task (i.e., where long-running tasks are permitted). /// JSON-encodes the return value of `func`. -pub async fn blocking_signed_json_task(func: F) -> Response -where - F: FnOnce() -> Result + Send + 'static, - T: Serialize + Send + 'static, -{ - let result = warp_utils::task::blocking_task(func) - .await - .map(|func_output| { - let response = match serde_json::to_vec(&func_output) { - Ok(body) => { - let mut res = Response::new(body.into()); - res.headers_mut() - .insert(CONTENT_TYPE, HeaderValue::from_static("application/json")); - res - } - Err(_) => { - let res = Response::new(vec![].into()); - let (mut parts, body) = res.into_parts(); - parts.status = StatusCode::INTERNAL_SERVER_ERROR; - Response::from_parts(parts, body) - } - }; - - response - }); - convert_rejection(result).await -} +// pub async fn blocking_json_task(func: F) -> Response +// where +// F: FnOnce() -> Result + Send + 'static, +// T: Serialize + Send + 'static, +// { +// let result = warp_utils::task::blocking_task(func) +// .await +// .map(|func_output| { +// let response = match serde_json::to_vec(&func_output) { +// Ok(body) => { +// let mut res = Response::new(body.into()); +// res.headers_mut() +// .insert(CONTENT_TYPE, HeaderValue::from_static("application/json")); +// res +// } +// Err(_) => { +// let res = Response::new(vec![].into()); +// let (mut parts, body) = res.into_parts(); +// parts.status = StatusCode::INTERNAL_SERVER_ERROR; +// Response::from_parts(parts, body) +// } +// }; + +// response +// }); +// convert_rejection(result).await +// } diff --git a/validator_client/src/lib.rs b/validator_client/src/lib.rs index 268c25cdf7d..6fc01c2eac3 100644 --- a/validator_client/src/lib.rs +++ b/validator_client/src/lib.rs @@ -64,6 +64,7 @@ use tokio::{ }; use types::{EthSpec, Hash256, PublicKeyBytes}; use validator_store::ValidatorStore; +use http_api::ApiSecret; /// The interval between attempts to contact the beacon node during startup. const RETRY_DELAY: Duration = Duration::from_secs(2); From 437dd1389f0745c906a666be25867afd77748afc Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Sat, 27 Apr 2024 14:45:34 +0300 Subject: [PATCH 08/13] linting --- common/warp_utils/src/task.rs | 2 +- validator_client/src/http_api/api_secret.rs | 3 +- validator_client/src/http_api/mod.rs | 72 +++++---------------- validator_client/src/lib.rs | 1 - 4 files changed, 19 insertions(+), 59 deletions(-) diff --git a/common/warp_utils/src/task.rs b/common/warp_utils/src/task.rs index c7ecda63289..e2fa4ebc368 100644 --- a/common/warp_utils/src/task.rs +++ b/common/warp_utils/src/task.rs @@ -1,6 +1,6 @@ +use crate::reject::convert_rejection; use serde::Serialize; use warp::reply::{Reply, Response}; -use crate::reject::convert_rejection; /// A convenience wrapper around `blocking_task`. pub async fn blocking_task(func: F) -> Result diff --git a/validator_client/src/http_api/api_secret.rs b/validator_client/src/http_api/api_secret.rs index ea1a22b6940..655e7a1c327 100644 --- a/validator_client/src/http_api/api_secret.rs +++ b/validator_client/src/http_api/api_secret.rs @@ -1,8 +1,7 @@ use eth2::lighthouse_vc::{PK_LEN, SECRET_PREFIX as PK_PREFIX}; use filesystem::create_with_600_perms; -use libsecp256k1::{Message, PublicKey, SecretKey}; +use libsecp256k1::{PublicKey, SecretKey}; use rand::thread_rng; -use ring::digest::{digest, SHA256}; use std::fs; use std::path::{Path, PathBuf}; use warp::Filter; diff --git a/validator_client/src/http_api/mod.rs b/validator_client/src/http_api/mod.rs index f6689a515be..fe536010823 100644 --- a/validator_client/src/http_api/mod.rs +++ b/validator_client/src/http_api/mod.rs @@ -1,10 +1,10 @@ +mod api_secret; mod create_signed_voluntary_exit; mod create_validator; mod graffiti; mod keystores; mod remotekeys; mod tests; -mod api_secret; pub mod test_utils; @@ -16,6 +16,7 @@ use account_utils::{ mnemonic_from_phrase, validator_definitions::{SigningDefinition, ValidatorDefinition, Web3SignerDefinition}, }; +pub use api_secret::ApiSecret; use create_validator::{ create_validators_mnemonic, create_validators_web3signer, get_voting_password_storage, }; @@ -30,7 +31,7 @@ use lighthouse_version::version_with_platform; use logging::SSELoggingComponents; use parking_lot::RwLock; use serde::{Deserialize, Serialize}; -use slog::{warn, crit, info, Logger}; +use slog::{crit, info, warn, Logger}; use slot_clock::SlotClock; use std::collections::HashMap; use std::future::Future; @@ -44,17 +45,8 @@ use task_executor::TaskExecutor; use tokio_stream::{wrappers::BroadcastStream, StreamExt}; use types::{ChainSpec, ConfigAndPreset, EthSpec}; use validator_dir::Builder as ValidatorDirBuilder; -use warp::{ - http::{ - header::{HeaderValue, CONTENT_TYPE}, - StatusCode, - }, - reply::Response, - sse::Event, - Filter, -}; -use warp_utils::reject::convert_rejection; -pub use api_secret::ApiSecret; +use warp::{sse::Event, Filter}; +use warp_utils::task::blocking_json_task; #[derive(Debug)] pub enum Error { @@ -760,18 +752,18 @@ pub fn serve( }) }, ); - - // GET /lighthouse/auth - let get_auth = warp::path("lighthouse").and(warp::path("auth").and(warp::path::end())); - let get_auth = get_auth - .and(api_token_path_filter) - .then( move|token_path: PathBuf| { - blocking_json_task(move || { - Ok(AuthResponse { - token_path: token_path.display().to_string(), - }) - }) - }); + + // GET /lighthouse/auth + let get_auth = warp::path("lighthouse").and(warp::path("auth").and(warp::path::end())); + let get_auth = get_auth + .and(api_token_path_filter) + .then(move |token_path: PathBuf| { + blocking_json_task(move || { + Ok(AuthResponse { + token_path: token_path.display().to_string(), + }) + }) + }); // DELETE /lighthouse/keystores let delete_lighthouse_keystores = warp::path("lighthouse") @@ -1316,33 +1308,3 @@ pub fn serve( Ok((listening_socket, server)) } - -/// Executes `func` in blocking tokio task (i.e., where long-running tasks are permitted). -/// JSON-encodes the return value of `func`. -// pub async fn blocking_json_task(func: F) -> Response -// where -// F: FnOnce() -> Result + Send + 'static, -// T: Serialize + Send + 'static, -// { -// let result = warp_utils::task::blocking_task(func) -// .await -// .map(|func_output| { -// let response = match serde_json::to_vec(&func_output) { -// Ok(body) => { -// let mut res = Response::new(body.into()); -// res.headers_mut() -// .insert(CONTENT_TYPE, HeaderValue::from_static("application/json")); -// res -// } -// Err(_) => { -// let res = Response::new(vec![].into()); -// let (mut parts, body) = res.into_parts(); -// parts.status = StatusCode::INTERNAL_SERVER_ERROR; -// Response::from_parts(parts, body) -// } -// }; - -// response -// }); -// convert_rejection(result).await -// } diff --git a/validator_client/src/lib.rs b/validator_client/src/lib.rs index 6fc01c2eac3..268c25cdf7d 100644 --- a/validator_client/src/lib.rs +++ b/validator_client/src/lib.rs @@ -64,7 +64,6 @@ use tokio::{ }; use types::{EthSpec, Hash256, PublicKeyBytes}; use validator_store::ValidatorStore; -use http_api::ApiSecret; /// The interval between attempts to contact the beacon node during startup. const RETRY_DELAY: Duration = Duration::from_secs(2); From afccdf84b1b9067ea35acc4f4cededf769574c7f Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Sat, 27 Apr 2024 14:51:10 +0300 Subject: [PATCH 09/13] revert logging --- validator_client/src/http_api/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/validator_client/src/http_api/mod.rs b/validator_client/src/http_api/mod.rs index fe536010823..3d7cab8e5e0 100644 --- a/validator_client/src/http_api/mod.rs +++ b/validator_client/src/http_api/mod.rs @@ -1304,6 +1304,7 @@ pub fn serve( log, "HTTP API started"; "listen_address" => listening_socket.to_string(), + "api_token_file" => ?api_token_path, ); Ok((listening_socket, server)) From 89e92900c9445a51840bbef3fddd0c123e66323d Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Sat, 27 Apr 2024 15:17:36 +0300 Subject: [PATCH 10/13] remove response signing checks in validtor http_api client --- common/eth2/src/lighthouse_vc/http_client.rs | 50 ++++---------------- 1 file changed, 10 insertions(+), 40 deletions(-) diff --git a/common/eth2/src/lighthouse_vc/http_client.rs b/common/eth2/src/lighthouse_vc/http_client.rs index 83aeea4bfcc..bb16a356baa 100644 --- a/common/eth2/src/lighthouse_vc/http_client.rs +++ b/common/eth2/src/lighthouse_vc/http_client.rs @@ -1,13 +1,11 @@ use super::{types::*, PK_LEN, SECRET_PREFIX}; use crate::Error; use account_utils::ZeroizeString; -use bytes::Bytes; -use libsecp256k1::{Message, PublicKey, Signature}; +use libsecp256k1::PublicKey; use reqwest::{ header::{HeaderMap, HeaderValue}, IntoUrl, }; -use ring::digest::{digest, SHA256}; use sensitive_url::SensitiveUrl; use serde::{de::DeserializeOwned, Serialize}; use std::fmt::{self, Display}; @@ -160,38 +158,6 @@ impl ValidatorClientHttpClient { self.authorization_header = AuthorizationHeader::Basic; } - async fn signed_body(&self, response: Response) -> Result { - let server_pubkey = self.server_pubkey.as_ref().ok_or(Error::NoServerPubkey)?; - let sig = response - .headers() - .get("Signature") - .ok_or(Error::MissingSignatureHeader)? - .to_str() - .map_err(|_| Error::InvalidSignatureHeader)? - .to_string(); - - let body = response.bytes().await.map_err(Error::from)?; - - let message = - Message::parse_slice(digest(&SHA256, &body).as_ref()).expect("sha256 is 32 bytes"); - - serde_utils::hex::decode(&sig) - .ok() - .and_then(|bytes| { - let sig = Signature::parse_der(&bytes).ok()?; - Some(libsecp256k1::verify(&message, &sig, server_pubkey)) - }) - .filter(|is_valid| *is_valid) - .ok_or(Error::InvalidSignatureHeader)?; - - Ok(body) - } - - async fn signed_json(&self, response: Response) -> Result { - let body = self.signed_body(response).await?; - serde_json::from_slice(&body).map_err(Error::InvalidJson) - } - fn headers(&self) -> Result { let mut headers = HeaderMap::new(); @@ -240,7 +206,8 @@ impl ValidatorClientHttpClient { async fn get(&self, url: U) -> Result { let response = self.get_response(url).await?; - self.signed_json(response).await + let body = response.bytes().await.map_err(Error::from)?; + serde_json::from_slice(&body).map_err(Error::InvalidJson) } async fn delete(&self, url: U) -> Result<(), Error> { @@ -263,7 +230,10 @@ impl ValidatorClientHttpClient { /// Perform a HTTP GET request, returning `None` on a 404 error. async fn get_opt(&self, url: U) -> Result, Error> { match self.get_response(url).await { - Ok(resp) => self.signed_json(resp).await.map(Option::Some), + Ok(resp) => { + let body = resp.bytes().await.map_err(Error::from)?; + serde_json::from_slice(&body).map_err(Error::InvalidJson) + } Err(err) => { if err.status() == Some(StatusCode::NOT_FOUND) { Ok(None) @@ -297,7 +267,8 @@ impl ValidatorClientHttpClient { body: &T, ) -> Result { let response = self.post_with_raw_response(url, body).await?; - self.signed_json(response).await + let body = response.bytes().await.map_err(Error::from)?; + serde_json::from_slice(&body).map_err(Error::InvalidJson) } async fn post_with_unsigned_response( @@ -319,8 +290,7 @@ impl ValidatorClientHttpClient { .send() .await .map_err(Error::from)?; - let response = ok_or_error(response).await?; - self.signed_body(response).await?; + ok_or_error(response).await?; Ok(()) } From f270e02d874a36cfcbcbb7bfc79ef27e1bc90f91 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Tue, 30 Apr 2024 17:06:12 +0300 Subject: [PATCH 11/13] remove notion of public key, prefixes, and simplify token generation --- common/eth2/src/lighthouse_vc/http_client.rs | 74 +++++--------------- common/eth2/src/lighthouse_vc/mod.rs | 7 -- validator_client/src/http_api/api_secret.rs | 70 +++++------------- 3 files changed, 37 insertions(+), 114 deletions(-) diff --git a/common/eth2/src/lighthouse_vc/http_client.rs b/common/eth2/src/lighthouse_vc/http_client.rs index bb16a356baa..f4bd07ee90f 100644 --- a/common/eth2/src/lighthouse_vc/http_client.rs +++ b/common/eth2/src/lighthouse_vc/http_client.rs @@ -1,7 +1,6 @@ -use super::{types::*, PK_LEN, SECRET_PREFIX}; +use super::types::*; use crate::Error; use account_utils::ZeroizeString; -use libsecp256k1::PublicKey; use reqwest::{ header::{HeaderMap, HeaderValue}, IntoUrl, @@ -22,8 +21,7 @@ use types::graffiti::GraffitiString; pub struct ValidatorClientHttpClient { client: reqwest::Client, server: SensitiveUrl, - secret: Option, - server_pubkey: Option, + api_token: Option, authorization_header: AuthorizationHeader, } @@ -44,45 +42,13 @@ impl Display for AuthorizationHeader { } } -/// Parse an API token and return a secp256k1 public key. -/// -/// If the token does not start with the Lighthouse token prefix then `Ok(None)` will be returned. -/// An error will be returned if the token looks like a Lighthouse token but doesn't correspond to a -/// valid public key. -pub fn parse_pubkey(secret: &str) -> Result, Error> { - let secret = if !secret.starts_with(SECRET_PREFIX) { - return Ok(None); - } else { - &secret[SECRET_PREFIX.len()..] - }; - - serde_utils::hex::decode(secret) - .map_err(|e| Error::InvalidSecret(format!("invalid hex: {:?}", e))) - .and_then(|bytes| { - if bytes.len() != PK_LEN { - return Err(Error::InvalidSecret(format!( - "expected {} bytes not {}", - PK_LEN, - bytes.len() - ))); - } - - let mut arr = [0; PK_LEN]; - arr.copy_from_slice(&bytes); - PublicKey::parse_compressed(&arr) - .map_err(|e| Error::InvalidSecret(format!("invalid secp256k1 pubkey: {:?}", e))) - }) - .map(Some) -} - impl ValidatorClientHttpClient { /// Create a new client pre-initialised with an API token. pub fn new(server: SensitiveUrl, secret: String) -> Result { Ok(Self { client: reqwest::Client::new(), server, - server_pubkey: parse_pubkey(&secret)?, - secret: Some(secret.into()), + api_token: Some(secret.into()), authorization_header: AuthorizationHeader::Bearer, }) } @@ -94,8 +60,7 @@ impl ValidatorClientHttpClient { Ok(Self { client: reqwest::Client::new(), server, - secret: None, - server_pubkey: None, + api_token: None, authorization_header: AuthorizationHeader::Omit, }) } @@ -108,15 +73,14 @@ impl ValidatorClientHttpClient { Ok(Self { client, server, - server_pubkey: parse_pubkey(&secret)?, - secret: Some(secret.into()), + api_token: Some(secret.into()), authorization_header: AuthorizationHeader::Bearer, }) } /// Get a reference to this client's API token, if any. pub fn api_token(&self) -> Option<&ZeroizeString> { - self.secret.as_ref() + self.api_token.as_ref() } /// Read an API token from the specified `path`, stripping any trailing whitespace. @@ -126,19 +90,11 @@ impl ValidatorClientHttpClient { } /// Add an authentication token to use when making requests. - /// - /// If the token is Lighthouse-like, a pubkey derivation will be attempted. In the case - /// of failure the token will still be stored, and the client can continue to be used to - /// communicate with non-Lighthouse nodes. pub fn add_auth_token(&mut self, token: ZeroizeString) -> Result<(), Error> { - let pubkey_res = parse_pubkey(token.as_str()); - - self.secret = Some(token); + self.api_token = Some(token); self.authorization_header = AuthorizationHeader::Bearer; - pubkey_res.map(|opt_pubkey| { - self.server_pubkey = opt_pubkey; - }) + Ok(()) } /// Set to `false` to disable sending the `Authorization` header on requests. @@ -164,11 +120,13 @@ impl ValidatorClientHttpClient { if self.authorization_header == AuthorizationHeader::Basic || self.authorization_header == AuthorizationHeader::Bearer { - let secret = self.secret.as_ref().ok_or(Error::NoToken)?; + let auth_header_token = self + .api_token() + .ok_or(Error::NoToken)?; let header_value = HeaderValue::from_str(&format!( "{} {}", self.authorization_header, - secret.as_str() + auth_header_token.as_str() )) .map_err(|e| { Error::InvalidSecret(format!("secret is invalid as a header value: {}", e)) @@ -231,8 +189,12 @@ impl ValidatorClientHttpClient { async fn get_opt(&self, url: U) -> Result, Error> { match self.get_response(url).await { Ok(resp) => { - let body = resp.bytes().await.map_err(Error::from)?; - serde_json::from_slice(&body).map_err(Error::InvalidJson) + let body = resp.bytes().await.map(Option::Some)?; + if let Some(body) = body { + serde_json::from_slice(&body).map_err(Error::InvalidJson) + } else { + Ok(None) + } } Err(err) => { if err.status() == Some(StatusCode::NOT_FOUND) { diff --git a/common/eth2/src/lighthouse_vc/mod.rs b/common/eth2/src/lighthouse_vc/mod.rs index 81b4fca283a..038726c829a 100644 --- a/common/eth2/src/lighthouse_vc/mod.rs +++ b/common/eth2/src/lighthouse_vc/mod.rs @@ -1,10 +1,3 @@ pub mod http_client; pub mod std_types; pub mod types; - -/// The number of bytes in the secp256k1 public key used as the authorization token for the VC API. -pub const PK_LEN: usize = 33; - -/// The prefix for the secp256k1 public key when it is used as the authorization token for the VC -/// API. -pub const SECRET_PREFIX: &str = "api-token-"; diff --git a/validator_client/src/http_api/api_secret.rs b/validator_client/src/http_api/api_secret.rs index 655e7a1c327..67bc27de5b8 100644 --- a/validator_client/src/http_api/api_secret.rs +++ b/validator_client/src/http_api/api_secret.rs @@ -1,7 +1,6 @@ -use eth2::lighthouse_vc::{PK_LEN, SECRET_PREFIX as PK_PREFIX}; use filesystem::create_with_600_perms; -use libsecp256k1::{PublicKey, SecretKey}; -use rand::thread_rng; +use rand::distributions::Alphanumeric; +use rand::{thread_rng, Rng}; use std::fs; use std::path::{Path, PathBuf}; use warp::Filter; @@ -13,8 +12,9 @@ use warp::Filter; /// value in a public forum. pub const PK_FILENAME: &str = "api-token.txt"; -/// Contains a `secp256k1` keypair that is saved-to/loaded-from disk on instantiation. The keypair -/// is used for authorization/authentication for requests/responses on the HTTP API. +pub const PK_LEN: usize = 33; + +/// Contains a randomly generated string which is used for authorization of requests to the HTTP API. /// /// Provides convenience functions to ultimately provide: /// @@ -25,10 +25,10 @@ pub const PK_FILENAME: &str = "api-token.txt"; /// /// https://github.com/sigp/lighthouse/issues/1269#issuecomment-649879855 /// -/// This scheme has since been tweaked to remove VC response signing +/// This scheme has since been tweaked to remove VC response signing and secp256k1 key generation. /// https://github.com/sigp/lighthouse/issues/5423 pub struct ApiSecret { - pk: PublicKey, + pk: String, pk_path: PathBuf, } @@ -43,20 +43,15 @@ impl ApiSecret { let pk_path = dir.as_ref().join(PK_FILENAME); if !pk_path.exists() { - let sk = SecretKey::random(&mut thread_rng()); - let pk = PublicKey::from_secret_key(&sk); + let length = PK_LEN; + let pk: String = thread_rng() + .sample_iter(&Alphanumeric) + .take(length) + .map(char::from) + .collect(); // Create and write the public key to file with appropriate permissions - create_with_600_perms( - &pk_path, - format!( - "{}{}", - PK_PREFIX, - serde_utils::hex::encode(&pk.serialize_compressed()[..]) - ) - .as_bytes(), - ) - .map_err(|e| { + create_with_600_perms(&pk_path, pk.to_string().as_bytes()).map_err(|e| { format!( "Unable to create file with permissions for {:?}: {:?}", pk_path, e @@ -65,44 +60,17 @@ impl ApiSecret { } let pk = fs::read(&pk_path) - .map_err(|e| format!("cannot read {}: {}", PK_FILENAME, e)) - .and_then(|bytes| { - let hex = - String::from_utf8(bytes).map_err(|_| format!("{} is not utf8", PK_FILENAME))?; - if let Some(stripped) = hex.strip_prefix(PK_PREFIX) { - serde_utils::hex::decode(stripped) - .map_err(|_| format!("{} should be 0x-prefixed hex", PK_FILENAME)) - } else { - Err(format!("unable to parse {}", PK_FILENAME)) - } - }) - .and_then(|bytes| { - if bytes.len() == PK_LEN { - let mut array = [0; PK_LEN]; - array.copy_from_slice(&bytes); - PublicKey::parse_compressed(&array) - .map_err(|e| format!("invalid {}: {}", PK_FILENAME, e)) - } else { - Err(format!( - "{} expected {} bytes not {}", - PK_FILENAME, - PK_LEN, - bytes.len() - )) - } - })?; + .map_err(|e| format!("cannot read {}: {}", PK_FILENAME, e))? + .iter() + .map(|&c| char::from(c)) + .collect(); Ok(Self { pk, pk_path }) } - /// Returns the public key of `self` as a 0x-prefixed hex string. - fn pubkey_string(&self) -> String { - serde_utils::hex::encode(&self.pk.serialize_compressed()[..]) - } - /// Returns the API token. pub fn api_token(&self) -> String { - format!("{}{}", PK_PREFIX, self.pubkey_string()) + self.pk.clone() } /// Returns the path for the API token file From dbfd05cee5dd72d0bdcccbd252afc880fd3135cc Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Tue, 30 Apr 2024 17:40:35 +0300 Subject: [PATCH 12/13] fmt --- common/eth2/src/lighthouse_vc/http_client.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/common/eth2/src/lighthouse_vc/http_client.rs b/common/eth2/src/lighthouse_vc/http_client.rs index f4bd07ee90f..67fe77a3157 100644 --- a/common/eth2/src/lighthouse_vc/http_client.rs +++ b/common/eth2/src/lighthouse_vc/http_client.rs @@ -120,9 +120,7 @@ impl ValidatorClientHttpClient { if self.authorization_header == AuthorizationHeader::Basic || self.authorization_header == AuthorizationHeader::Bearer { - let auth_header_token = self - .api_token() - .ok_or(Error::NoToken)?; + let auth_header_token = self.api_token().ok_or(Error::NoToken)?; let header_value = HeaderValue::from_str(&format!( "{} {}", self.authorization_header, From b408697df73078f38f6781719086a7181d0a5bda Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Tue, 16 Jul 2024 17:13:06 +1000 Subject: [PATCH 13/13] Remove outdated comment on public key --- validator_client/src/http_api/api_secret.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/validator_client/src/http_api/api_secret.rs b/validator_client/src/http_api/api_secret.rs index 67bc27de5b8..32035caf473 100644 --- a/validator_client/src/http_api/api_secret.rs +++ b/validator_client/src/http_api/api_secret.rs @@ -5,11 +5,7 @@ use std::fs; use std::path::{Path, PathBuf}; use warp::Filter; -/// The name of the file which stores the public key. -/// -/// For users, this public key is a "secret" that can be shared with API consumers to provide them -/// access to the API. We avoid calling it a "public" key to users, since they should not post this -/// value in a public forum. +/// The name of the file which stores the API token. pub const PK_FILENAME: &str = "api-token.txt"; pub const PK_LEN: usize = 33;