diff --git a/CLAUDE.md b/CLAUDE.md index d08ee404f..11881ad02 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -849,7 +849,19 @@ transitions are in `ethlambda-types`, per the section above. Nothing above keeps EIP-3076 minimal-watermark slashing protection in `--validators-dir/slashing_protection.json` (`crate::slashing_protection`), durable before every block and attestation signature; PTC votes have an - in-memory `(validator, slot)` dedup only, and envelopes are unguarded. + in-memory `(validator, slot)` dedup only, and envelopes are unguarded. A key can live + in a Web3Signer instead of the process (`SigningMethod::Web3Signer`, + `keys/web3signer.rs`): every `SigningContext::sign_*` is async, resolves the + signer under a read guard scoped to the lookup and signs after releasing it, + and sends the remote signer the message in full so its own slashing + protection applies on top of this client's record, which a remote key is + registered in and checked against like a local one; the signature the + signer returns is verified against the pubkey and root before it is used. + Each duty signs its batch at once through + `signing::sign_all`, every signature bounded by `SIGNING_BUDGET`, so one + hung signer costs only its own keys. Remote keys are `type: web3signer` + entries in `validator_definitions.yml` or `POST /eth/v1/remotekeys` (default signer + `--web3signer-url`); `/keystores` and `/remotekeys` partition the store. `get_head_node` is one bottom-up walk for every fork, with the current slot's fork selecting the payload rules and the boost gate, so a follower on any network that schedules gloas (Sepolia does) pays work diff --git a/Cargo.lock b/Cargo.lock index 0bec6c9e0..61e826be2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2272,6 +2272,7 @@ dependencies = [ "ctr", "ethlambda-metrics", "ethlambda-types", + "futures", "hex", "hmac", "http-body-util", diff --git a/bin/ethlambda/src/validator.rs b/bin/ethlambda/src/validator.rs index 968244963..28f65d141 100644 --- a/bin/ethlambda/src/validator.rs +++ b/bin/ethlambda/src/validator.rs @@ -79,6 +79,14 @@ pub(crate) struct ValidatorOptions { #[arg(long, default_value = "false")] pub(crate) enable_keymanager: bool, + /// Base URL of a Web3Signer remote signer (e.g. https://signer:9000), + /// the default for remote keys imported through the keymanager API's + /// `POST /eth/v1/remotekeys` without a `url` of their own. + /// + /// Optional: a remote key may name its own signer in that request or in + /// `validator_definitions.yml`, and local keystores never need one. + #[arg(long)] + pub(crate) web3signer_url: Option, /// Register every loaded key that has no slashing-protection record, /// instead of refusing to start. /// @@ -167,6 +175,7 @@ pub(crate) async fn run(options: ValidatorOptions) -> eyre::Result<()> { keymanager: options .enable_keymanager .then(|| std::net::SocketAddr::new(options.http_address, options.keymanager_port)), + web3signer_url: options.web3signer_url, init_slashing_protection: options.init_slashing_protection, }) .await @@ -188,6 +197,7 @@ mod tests { graffiti: graffiti.to_string(), suggested_fee_recipient: None, enable_keymanager: false, + web3signer_url: None, init_slashing_protection: false, } } diff --git a/crates/net/rpc/src/beacon/validator_client_tests.rs b/crates/net/rpc/src/beacon/validator_client_tests.rs index b62b61cdc..5c20a075d 100644 --- a/crates/net/rpc/src/beacon/validator_client_tests.rs +++ b/crates/net/rpc/src/beacon/validator_client_tests.rs @@ -865,10 +865,7 @@ async fn serve_fulu_pinned( .spec() .await .expect("the client reads this node's spec"); - let context = Arc::new(SigningContext { - config: spec, - genesis_validators_root: genesis.genesis_validators_root, - }); + let context = Arc::new(SigningContext::new(spec, genesis.genesis_validators_root)); (Arc::new(client), state, network, context) } @@ -1165,10 +1162,7 @@ mod gloas { .spec() .await .expect("the client reads this node's spec"); - let context = Arc::new(SigningContext { - config: spec, - genesis_validators_root: genesis.genesis_validators_root, - }); + let context = Arc::new(SigningContext::new(spec, genesis.genesis_validators_root)); Served { client, state, diff --git a/crates/validator/Cargo.toml b/crates/validator/Cargo.toml index b34e72488..e129410b6 100644 --- a/crates/validator/Cargo.toml +++ b/crates/validator/Cargo.toml @@ -32,6 +32,9 @@ tracing.workspace = true # so this crate still builds if it is ever compiled on its own. tokio = { workspace = true, features = ["time", "macros", "rt"] } reqwest = { workspace = true, features = ["json"] } +# `join_all`, so a slot's attestations are signed concurrently: each one is a +# round trip when a remote signer holds the key. +futures = "0.3" # BLS12-381. Same version and backend the state transition verifies with, so a # signature produced here and one checked there come from one implementation. diff --git a/crates/validator/src/aggregation.rs b/crates/validator/src/aggregation.rs index 3a0a32dcd..2d83393b2 100644 --- a/crates/validator/src/aggregation.rs +++ b/crates/validator/src/aggregation.rs @@ -39,7 +39,7 @@ use ethlambda_types::beacon::containers::electra; use ethlambda_types::beacon::containers::gloas; use ethlambda_types::beacon::containers::shared::AttestationData; use ethlambda_types::beacon::fork::ForkName; -use ethlambda_types::beacon::primitives::{HashTreeRoot as _, Slot}; +use ethlambda_types::beacon::primitives::{BlsPubkey, BlsSignature, HashTreeRoot as _, Slot}; use tokio::sync::RwLock; use tracing::{info, warn}; @@ -48,7 +48,7 @@ use crate::beacon_node::dto::{AttesterDutyDto, parse_pubkey}; use crate::beacon_node::{AggregateKind, BeaconNodeApi, SignedAggregates}; use crate::error::{Error, Result}; use crate::keys::ValidatorStore; -use crate::signing::SigningContext; +use crate::signing::{AggregateAndProofRef, SigningContext, sign_all}; /// An `AggregateAndProof` of either fork, held between building it and /// signing it. @@ -91,30 +91,42 @@ impl AggregationService { } // Which of this client's validators aggregates, and with what proof. - // - // The read guard covers the whole selection pass and no await, the way - // the attestation path's does: `selection_for` signs, and the lock is - // write-preferring, so holding this across the fetches below would put - // every later duty behind any keymanager writer that queued meanwhile. - let selected: Vec<(AttesterDutyDto, _)> = { - let store = store.read().await; - duties + // Asked for all at once, each within the signing budget: with only + // the last third of the slot left, a round trip per duty in series + // could run the whole duty out. Usually no request is made at all, + // since the proof was already produced for this epoch's subscription + // and `sign_selection_proof` remembers it. + let mut keyed = Vec::with_capacity(duties.len()); + for duty in duties { + match parse_pubkey(&duty.pubkey) { + Ok(pubkey) => keyed.push((duty, pubkey)), + Err(err) => { + warn!(%slot, validator = duty.validator_index, %err, "Duty carried an unreadable pubkey") + } + } + } + let selections = sign_all( + keyed .iter() - .filter_map(|duty| match selection_for(&self.context, &store, duty) { - Ok(Some(proof)) => Some((duty.clone(), proof)), - Ok(None) => None, - Err(err) => { - warn!( - %slot, - validator = duty.validator_index, - %err, - "Could not compute aggregator selection; skipping this duty" - ); - None - } - }) - .collect() - }; + .map(|(duty, pubkey)| (*pubkey, selection_for(&self.context, store, duty))), + ) + .await; + let mut selected: Vec<(&AttesterDutyDto, BlsPubkey, BlsSignature)> = Vec::new(); + for ((duty, pubkey), selection) in keyed.into_iter().zip(selections) { + match selection { + Ok(Some(proof)) => selected.push((duty, pubkey, proof)), + Ok(None) => {} + Err(err) => { + warn!( + %slot, + validator = duty.validator_index, + %err, + "Could not compute aggregator selection; skipping this duty" + ); + crate::metrics::inc_signing_failures(); + } + } + } if selected.is_empty() { return Ok(0); @@ -126,7 +138,7 @@ impl AggregationService { // twice for it would be the same answer at twice the cost. let data_root = data.hash_tree_root(); let mut aggregates = BTreeMap::new(); - for (duty, _) in &selected { + for (duty, _, _) in &selected { if aggregates.contains_key(&duty.committee_index) { continue; } @@ -161,54 +173,59 @@ impl AggregationService { // fork boundary sits. let fork = self.batch_fork(&aggregates, slot)?; + // Every wrapper is built first and all of them are signed at once, + // each within the signing budget, for the reason the selections are. + let mut wrappers = Vec::with_capacity(selected.len()); + for (duty, pubkey, selection_proof) in selected { + let Some(aggregate) = aggregates.get(&duty.committee_index) else { + continue; + }; + // The wrapper is the fork's own container, and the signing path + // takes its root from it: gloas's `Attestation` hashes + // differently from electra's, so a root computed over the wrong + // one would sign a message no node can verify. + let wrapper = match &aggregate.attestation { + AggregateKind::Electra(attestation) => { + Wrapper::Electra(electra::AggregateAndProof { + aggregator_index: duty.validator_index, + aggregate: attestation.clone(), + selection_proof, + }) + } + AggregateKind::Gloas(attestation) => Wrapper::Gloas(gloas::AggregateAndProof { + aggregator_index: duty.validator_index, + aggregate: attestation.clone(), + selection_proof, + }), + }; + wrappers.push((duty, pubkey, wrapper)); + } + + // Signed over the whole wrapper, not over the aggregate: that is what + // binds this validator's index and its selection proof to the votes + // it is republishing. + let signatures = sign_all(wrappers.iter().map(|(_, pubkey, wrapper)| { + let message = match wrapper { + Wrapper::Electra(message) => AggregateAndProofRef::Electra(message), + Wrapper::Gloas(message) => AggregateAndProofRef::Gloas(message), + }; + ( + *pubkey, + self.context + .sign_aggregate_and_proof(store, pubkey, message), + ) + })) + .await; + let signed = { - let store = store.read().await; let mut electra_signed = Vec::new(); let mut gloas_signed = Vec::new(); - for (duty, selection_proof) in &selected { - let Some(aggregate) = aggregates.get(&duty.committee_index) else { - continue; - }; - let pubkey = match parse_pubkey(&duty.pubkey) { - Ok(pubkey) => pubkey, - Err(err) => { - warn!(%slot, validator = duty.validator_index, %err, "Duty carried an unreadable pubkey"); - continue; - } - }; - - // The wrapper is the fork's own container, and its root is taken - // from it: gloas's `Attestation` hashes differently from - // electra's, so a root computed over the wrong one would sign a - // message no node can verify. - let (root, wrapper) = match &aggregate.attestation { - AggregateKind::Electra(attestation) => { - let message = electra::AggregateAndProof { - aggregator_index: duty.validator_index, - aggregate: attestation.clone(), - selection_proof: *selection_proof, - }; - (message.hash_tree_root(), Wrapper::Electra(message)) - } - AggregateKind::Gloas(attestation) => { - let message = gloas::AggregateAndProof { - aggregator_index: duty.validator_index, - aggregate: attestation.clone(), - selection_proof: *selection_proof, - }; - (message.hash_tree_root(), Wrapper::Gloas(message)) - } - }; - // Signed over the whole wrapper, not over the aggregate: that - // is what binds this validator's index and its selection proof - // to the votes it is republishing. - let signature = match self - .context - .sign_aggregate_and_proof(&store, &pubkey, root, slot) - { + for ((duty, _, wrapper), signature) in wrappers.into_iter().zip(signatures) { + let signature = match signature { Ok(signature) => signature, Err(err) => { warn!(%slot, validator = duty.validator_index, %err, "Failed to sign aggregate"); + crate::metrics::inc_signing_failures(); continue; } }; @@ -285,10 +302,7 @@ mod tests { } fn context() -> Arc { - Arc::new(SigningContext { - config: Config::mainnet(), - genesis_validators_root: Root::ZERO, - }) + Arc::new(SigningContext::new(Config::mainnet(), Root::ZERO)) } fn store() -> (RwLock, BlsPubkey) { @@ -333,13 +347,14 @@ mod tests { /// A slot this key is selected to aggregate at, and one it is not. Which /// slots those are is a property of SHA-256, so they are found rather than /// written down. - fn slots(pubkey: &BlsPubkey, store: &ValidatorStore) -> (Slot, Slot) { + async fn slots(pubkey: &BlsPubkey, store: &RwLock) -> (Slot, Slot) { let context = context(); let mut selected = None; let mut rejected = None; for slot in 3_200..3_400 { let proof = context .sign_selection_proof(store, pubkey, slot) + .await .expect("signs"); if is_aggregator(128, &proof) { selected.get_or_insert(slot); @@ -358,10 +373,7 @@ mod tests { async fn fixture() -> (RwLock, BlsPubkey, Slot, Slot) { let (store, pubkey) = store(); - let (selected, rejected) = { - let guard = store.read().await; - slots(&pubkey, &guard) - }; + let (selected, rejected) = slots(&pubkey, &store).await; (store, pubkey, selected, rejected) } @@ -489,9 +501,9 @@ mod tests { let (_, list) = node.published_aggregates().remove(0); - let guard = store.read().await; let expected = context() - .sign_selection_proof(&guard, &pubkey, slot) + .sign_selection_proof(&store, &pubkey, slot) + .await .expect("signs"); assert_eq!(list[0].message.selection_proof, expected); assert!( @@ -531,24 +543,26 @@ mod tests { } fn gloas_context() -> Arc { - Arc::new(SigningContext { - config: Config::mainnet().with_fork_epoch(ForkName::Gloas, 100), - genesis_validators_root: Root::ZERO, - }) + Arc::new(SigningContext::new( + Config::mainnet().with_fork_epoch(ForkName::Gloas, 100), + Root::ZERO, + )) } /// A slot in the gloas era at which this key is selected under the gloas /// fork version, found rather than written down for the reason `slots` is. - fn gloas_selected_slot(pubkey: &BlsPubkey, store: &ValidatorStore) -> Slot { + async fn gloas_selected_slot(pubkey: &BlsPubkey, store: &RwLock) -> Slot { let context = gloas_context(); - (3_200..3_600) - .find(|slot| { - let proof = context - .sign_selection_proof(store, pubkey, *slot) - .expect("signs"); - is_aggregator(128, &proof) - }) - .expect("some slot selects this key") + for slot in 3_200..3_600 { + let proof = context + .sign_selection_proof(store, pubkey, slot) + .await + .expect("signs"); + if is_aggregator(128, &proof) { + return slot; + } + } + panic!("some slot selects this key") } /// At a gloas slot the aggregate is gloas's container, and the wrapper is @@ -560,10 +574,7 @@ mod tests { use blst::min_pk::{PublicKey, Signature}; let (store, pubkey) = store(); - let slot = { - let guard = store.read().await; - gloas_selected_slot(&pubkey, &guard) - }; + let slot = gloas_selected_slot(&pubkey, &store).await; let node = Arc::new(MockBeaconNode::new().with_gloas_aggregate(data(slot))); let published = AggregationService::new(node.clone(), gloas_context()) diff --git a/crates/validator/src/aggregation_selection.rs b/crates/validator/src/aggregation_selection.rs index d29aa159d..4aec8323b 100644 --- a/crates/validator/src/aggregation_selection.rs +++ b/crates/validator/src/aggregation_selection.rs @@ -34,6 +34,7 @@ use ethlambda_types::beacon::primitives::BlsSignature; use sha2::{Digest, Sha256}; +use tokio::sync::RwLock; use crate::beacon_node::dto::{AttesterDutyDto, parse_pubkey}; use crate::error::Result; @@ -88,13 +89,15 @@ pub fn is_aggregator(committee_length: u64, selection_proof: &BlsSignature) -> b /// silent `None`. Both mean the duty cannot be served at all, and reporting /// them as "not an aggregator" would hide a misconfiguration behind a role /// this validator was never going to fill anyway. -pub fn selection_for( +pub async fn selection_for( context: &SigningContext, - store: &ValidatorStore, + store: &RwLock, duty: &AttesterDutyDto, ) -> Result> { let pubkey = parse_pubkey(&duty.pubkey)?; - let proof = context.sign_selection_proof(store, &pubkey, duty.slot)?; + let proof = context + .sign_selection_proof(store, &pubkey, duty.slot) + .await?; Ok(is_aggregator(duty.committee_length, &proof).then_some(proof)) } diff --git a/crates/validator/src/attestation.rs b/crates/validator/src/attestation.rs index 5372b8470..564434f0b 100644 --- a/crates/validator/src/attestation.rs +++ b/crates/validator/src/attestation.rs @@ -15,7 +15,7 @@ use crate::beacon_node::dto::{ use crate::beacon_node::{BeaconNodeApi, validate_attestation_data}; use crate::error::Result; use crate::keys::ValidatorStore; -use crate::signing::SigningContext; +use crate::signing::{SigningContext, sign_all}; use crate::slashing_protection::SharedProtection; /// What one slot's attestation duty produced. @@ -195,8 +195,8 @@ impl AttestationService { // Check and durably record the whole batch before any signature exists: // a signature that exists is a signature that can escape, whatever the // code after it intended. One write for the slot, off the executor, - // and before the store's read lock below is taken, so the disk flush - // never holds up a keymanager writer queued behind it. + // and with no guard on the store held, so the disk flush never holds + // up a keymanager writer queued behind it. let pubkeys: Vec = candidates.iter().map(|(_, pubkey)| *pubkey).collect(); let verdicts = crate::slashing_protection::blocking(&self.protection, move |protection| { protection.check_and_record_attestations(&pubkeys, &data) @@ -219,48 +219,64 @@ impl AttestationService { } }; - // The read guard is scoped to this block alone and must never be widened to cover an - // await. `tokio::sync::RwLock` is write-preferring, so a keymanager - // import or delete queuing for the write lock blocks every read behind - // it; keeping this guard narrow bounds how long that wait can be. The - // import holds its own write lock for the inserts alone, doing the slow - // EIP-2335 derivation outside it (see `http_api::keystores::import`), - // so this is the duty path keeping its half of the same bargain. - let mut attestations = Vec::with_capacity(candidates.len()); + // Every duty the record let through is signed together below. + let mut to_sign = Vec::with_capacity(candidates.len()); let mut refused = 0; - { - let _timing = crate::metrics::time_signing(); - let store = store.read().await; - for ((duty, pubkey), verdict) in candidates.into_iter().zip(verdicts) { - if let Err(refusal) = verdict { - warn!( - %slot, - validator = duty.validator_index, - %refusal, - "Refusing to sign: slashing protection judged this attestation unsafe" - ); - crate::metrics::inc_attestations_refused(); - refused += 1; + for ((duty, pubkey), verdict) in candidates.into_iter().zip(verdicts) { + if let Err(refusal) = verdict { + warn!( + %slot, + validator = duty.validator_index, + %refusal, + "Refusing to sign: slashing protection judged this attestation unsafe" + ); + crate::metrics::inc_attestations_refused(); + refused += 1; + continue; + } + to_sign.push((duty, pubkey)); + } + + // Signed concurrently rather than one after another. A local key + // signs in microseconds either way; a remote signer costs a round + // trip per signature, and a hundred validators behind one signer + // must not pay a hundred of them in series inside one slot. Each + // signature takes the store's read guard for its own lookup only + // (see `SigningContext::sign`), so nothing here holds it across an + // await: `tokio::sync::RwLock` is write-preferring, and a keymanager + // import queuing for the write lock would otherwise stall every read + // behind it for as long as the guard was held. + // + // Each signature gets `SIGNING_BUDGET` and no more, so a hung signer + // costs only its own keys: the batch below goes out with every + // signature that arrived, rather than the slot's deadline dropping + // the whole batch, local keys included, while it waits. + let signatures = + { + let _timing = crate::metrics::time_signing(); + sign_all(to_sign.iter().map(|(_, pubkey)| { + (*pubkey, self.context.sign_attestation(store, pubkey, &data)) + })) + .await + }; + + let mut attestations = Vec::with_capacity(to_sign.len()); + for ((duty, _), signature) in to_sign.iter().zip(signatures) { + // One validator failing must not cost the others their attestation. + let signature = match signature { + Ok(signature) => signature, + Err(err) => { + error!(%slot, validator = duty.validator_index, %err, "Failed to sign attestation"); + crate::metrics::inc_signing_failures(); continue; } - - // One validator failing must not cost the others their attestation. - let signature = match self.context.sign_attestation(&store, &pubkey, &data) { - Ok(signature) => signature, - Err(err) => { - error!(%slot, validator = duty.validator_index, %err, "Failed to sign attestation"); - crate::metrics::inc_signing_failures(); - continue; - } - }; - - attestations.push(SingleAttestationDto { - committee_index: duty.committee_index, - attester_index: duty.validator_index, - data: data_dto.clone(), - signature: encode_hex(&signature.0), - }); - } + }; + attestations.push(SingleAttestationDto { + committee_index: duty.committee_index, + attester_index: duty.validator_index, + data: data_dto.clone(), + signature: encode_hex(&signature.0), + }); } if attestations.is_empty() { @@ -331,10 +347,7 @@ mod tests { } fn context() -> Arc { - Arc::new(SigningContext { - config: Config::mainnet(), - genesis_validators_root: Root::ZERO, - }) + Arc::new(SigningContext::new(Config::mainnet(), Root::ZERO)) } fn duty( @@ -820,11 +833,10 @@ mod tests { } fn gloas_context() -> Arc { - Arc::new(SigningContext { - config: Config::mainnet() - .with_fork_epoch(ethlambda_types::beacon::fork::ForkName::Gloas, 10), - genesis_validators_root: Root::ZERO, - }) + Arc::new(SigningContext::new( + Config::mainnet().with_fork_epoch(ethlambda_types::beacon::fork::ForkName::Gloas, 10), + Root::ZERO, + )) } /// From gloas `data.index` is the payload signal, so a node answering 1 diff --git a/crates/validator/src/beacon_node/block_contents.rs b/crates/validator/src/beacon_node/block_contents.rs index c4d2806ca..77a837cd3 100644 --- a/crates/validator/src/beacon_node/block_contents.rs +++ b/crates/validator/src/beacon_node/block_contents.rs @@ -390,6 +390,33 @@ impl ProducedBlock { } } + /// The block's header: the block with its body replaced by the body's + /// root. It merkleizes to [`Self::block_root`], which is what lets the + /// signing path take the header (a remote signer reads it) and sign the + /// block's root all the same. + pub fn header(&self) -> ethlambda_types::beacon::containers::shared::BeaconBlockHeader { + use ethlambda_types::beacon::containers::shared::BeaconBlockHeader; + use ethlambda_types::beacon::primitives::HashTreeRoot as _; + match &self.contents { + Contents::WithBlobProofs { block, .. } | Contents::WithCellProofs { block, .. } => { + BeaconBlockHeader { + slot: block.slot, + proposer_index: block.proposer_index, + parent_root: block.parent_root, + state_root: block.state_root, + body_root: block.body.hash_tree_root(), + } + } + Contents::Gloas { block, .. } => BeaconBlockHeader { + slot: block.slot, + proposer_index: block.proposer_index, + parent_root: block.parent_root, + state_root: block.state_root, + body_root: block.body.hash_tree_root(), + }, + } + } + /// Where the block pays its execution-layer rewards: the payload's fee /// recipient before gloas, the bid's from it on. pub fn fee_recipient(&self) -> ethlambda_types::beacon::primitives::ExecutionAddress { diff --git a/crates/validator/src/error.rs b/crates/validator/src/error.rs index ae65aee92..2239fbbc2 100644 --- a/crates/validator/src/error.rs +++ b/crates/validator/src/error.rs @@ -97,12 +97,14 @@ pub enum Error { #[error("no signing key for validator {0:?}")] UnknownValidator(BlsPubkey), - /// Not constructed today: the only `SigningMethod` is `LocalKeystore`, - /// whose signing call cannot fail once a key has been resolved. Kept for - /// the remote-signer variant `keys::store`'s module doc already commits - /// to supporting, whose call to an external signer can fail in ways a - /// local scalar multiplication cannot; remove this only if that plan is - /// dropped. + /// A remote signer did not produce the signature: it refused the message + /// (its slashing protection), does not hold the key, answered badly, or + /// could not be reached. A local key cannot raise this; its signing call + /// cannot fail once the key has been resolved. + /// + /// Not retryable: it costs one signature, the duty that needed it logs + /// and counts it, and none of its causes is something trying again + /// within the slot would change. #[error("signing failed for {pubkey:?}: {reason}")] Signing { pubkey: BlsPubkey, reason: String }, diff --git a/crates/validator/src/http_api/keystores.rs b/crates/validator/src/http_api/keystores.rs index c8356decb..11d55beb8 100644 --- a/crates/validator/src/http_api/keystores.rs +++ b/crates/validator/src/http_api/keystores.rs @@ -37,6 +37,7 @@ //! [`crate::slashing_protection`]), so an export lists, per validator, its //! highest slot and its highest source and target rather than every message. +use std::collections::HashSet; use std::path::{Path, PathBuf}; use std::sync::Arc; @@ -51,7 +52,7 @@ use crate::Error; use crate::beacon_node::dto::{encode_hex, parse_pubkey}; use crate::http_api::KeymanagerContext; use crate::keys::keystore::Keystore; -use crate::keys::{ValidatorDefinition, ValidatorDefinitions, ValidatorStore}; +use crate::keys::{SigningDefinition, ValidatorDefinition, ValidatorDefinitions, ValidatorStore}; use crate::secure_fs; /// The most keystores accepted in a single import request. @@ -121,12 +122,20 @@ pub struct KeyStatus { } impl KeyStatus { - fn error(message: impl std::fmt::Display) -> Self { + pub(super) fn error(message: impl std::fmt::Display) -> Self { Self { status: "error".to_string(), message: Some(message.to_string()), } } + + /// The key is already known to this client, through either route. + pub(super) fn duplicate() -> Self { + Self { + status: "duplicate".to_string(), + message: None, + } + } } #[derive(Debug, Deserialize)] @@ -142,11 +151,13 @@ pub struct DeleteResponse { pub slashing_protection: String, } +/// The keys this process holds itself. Remote ones are `/eth/v1/remotekeys`'s +/// to list; the two routes partition the store. pub async fn list(State(context): State) -> Json { let store = context.store.read().await; Json(ListResponse { data: store - .pubkeys() + .local_pubkeys() .into_iter() .map(|pubkey| ListedKeystore { validating_pubkey: encode_hex(&pubkey.0), @@ -301,10 +312,29 @@ pub async fn import( // due for as long as the whole batch takes, rather than just for the // instant the inserts below actually need. `definitions_lock` above has // no such reader/writer to starve: only import and delete ever take it. + // + // The keys a remote signer holds in memory are read once, under a guard + // released at once. `definitions_lock` is held until the inserts below, + // and every keymanager writer takes it first, so the store cannot change + // between this read and those inserts. + let held_remotely: HashSet = context + .store + .read() + .await + .remote_entries() + .into_iter() + .map(|(pubkey, _)| pubkey) + .collect(); let mut prepared = Vec::with_capacity(request.keystores.len()); for (position, json) in request.keystores.iter().enumerate() { let password = request.passwords.get(position).map(String::as_str); - prepared.push(prepare_import(&context, &mut definitions, json, password)); + prepared.push(prepare_import( + &context, + &held_remotely, + &mut definitions, + json, + password, + )); } // Every persisted key is registered in one durable write, before any of @@ -503,7 +533,7 @@ fn put_back(path: &Path, previous: Option<&[u8]>) -> crate::Result<()> { /// Save the definitions file, keeping `definitions_unflushed` true from an /// after-rename flush failure until a later save flushes the directory, which /// makes every earlier rename in it durable too. -fn save_definitions( +pub(super) fn save_definitions( context: &KeymanagerContext, definitions: &ValidatorDefinitions, ) -> crate::Result<()> { @@ -545,6 +575,7 @@ fn remove_if_present(path: &Path) -> crate::Result<()> { /// before it needs the store's write lock and its slashing-protection record. fn prepare_import( context: &KeymanagerContext, + held_remotely: &HashSet, definitions: &mut ValidatorDefinitions, json: &str, password: Option<&str>, @@ -557,6 +588,22 @@ fn prepare_import( let secret = keystore.decrypt(password).map_err(KeyStatus::error)?; let pubkey = ValidatorStore::derive_pubkey(&secret).map_err(KeyStatus::error)?; + // A key a remote signer holds, by the file or by the store, is + // `duplicate` here, the mirror of what `/eth/v1/remotekeys` answers for a + // local one: neither route silently replaces the other's entry. Judged + // before anything is written, so a `duplicate` never leaves files behind. + // Re-importing a key this route already holds is still `imported`, since + // that is how a password is rotated. + let remote_on_disk = definitions.0.iter().any(|definition| { + definition.signing.is_remote() + && parse_pubkey(&definition.voting_public_key) + .map(|parsed| parsed == pubkey) + .unwrap_or(false) + }); + if remote_on_disk || held_remotely.contains(&pubkey) { + return Err(KeyStatus::duplicate()); + } + // Persisted before the key is reachable through the store: a validator // this process is willing to sign with must already be durable, or a // crash right after "imported" is reported silently loses it, with @@ -599,7 +646,9 @@ fn regular_file_bytes(path: &Path) -> crate::Result>> { /// Add an already-decrypted, already-persisted key to the live store. The /// only step of an import that needs the write lock: a cheap, synchronous -/// scalar validation and hashmap insert, never an await. +/// scalar validation and hashmap insert, never an await. Nothing is judged +/// again here: `prepare_import` judged the file and the store under +/// `definitions_lock`, which is still held. fn finish_import(store: &mut ValidatorStore, secret: &[u8; 32]) -> KeyStatus { match store.insert_secret("keymanager import", secret) { Ok(pubkey) => { @@ -680,8 +729,10 @@ fn persist_import( let definition = ValidatorDefinition { enabled: true, voting_public_key: base.clone(), - voting_keystore_path: keystore_path.clone(), - voting_keystore_password_path: password_path.clone(), + signing: SigningDefinition::LocalKeystore { + voting_keystore_path: keystore_path.clone(), + voting_keystore_password_path: password_path.clone(), + }, }; // How to put `definitions` back if the save below fails. // @@ -774,16 +825,29 @@ pub async fn delete( .pubkeys .iter() .map(|text| match parse_pubkey(text) { - Ok(pubkey) => (Some(pubkey), delete_one(&context, &mut store, &pubkey)), + Ok(pubkey) => match delete_one(&context, &mut store, &pubkey) { + Some(status) => (Some(pubkey), status), + // The other route's key: `not_found` here, and carried + // without its pubkey, so nothing below exports its record or + // reports it `not_active` while a remote signer still signs + // with it. + None => ( + None, + KeyStatus { + status: "not_found".to_string(), + message: None, + }, + ), + }, Err(err) => (None, KeyStatus::error(err)), }) .collect(); drop(store); // Exported only now, with the keys out of the store: a signature with one - // of them either finished before the write lock above was granted, and is - // in the record, or can no longer happen. That holds because the duty - // services record before they take the store's read lock to sign (see + // of them was either asked for before the write lock above was granted, + // and is in the record, or can no longer happen. That holds because the + // duty services record before they look a key up to sign with it (see // `attestation.rs` and `proposal.rs`), so no signature exists without its // record already on disk. Only for the keys that are `deleted` or may // become `not_active`, as the specification requires: a key whose delete @@ -860,23 +924,45 @@ pub async fn delete( }) } +/// Delete one of this route's keys. `None` is a key `/eth/v1/remotekeys` +/// holds, which this route neither removes nor says anything about. fn delete_one( context: &KeymanagerContext, store: &mut ValidatorStore, pubkey: &BlsPubkey, -) -> KeyStatus { +) -> Option { // Whether this key is on disk, read before the in-memory removal makes the // store an unreliable witness. `not_found` has to mean "this client has no // definitions entry for it", not "it is not in memory right now": a key // removed from memory by an earlier failed attempt is still very much // present on disk, and reporting `not_found` for it tells an operator the // key is gone when a restart will bring it back signing. - let on_disk = match definitions_contain(context, pubkey) { - Ok(present) => present, + let entry = match definitions_entry(context, pubkey) { + Ok(entry) => entry, // Cannot tell. Report the failure rather than guessing either way: a // wrong `not_found` here is the dangerous direction. - Err(err) => return KeyStatus::error(err), + Err(err) => return Some(KeyStatus::error(err)), }; + let on_disk = entry.as_ref().is_some_and(|signing| !signing.is_remote()); + // A remote key is the other route's whatever its state: one the store + // holds, or one only the file lists, which the next start would load. It + // is `/eth/v1/remotekeys`'s to delete, and this route must not reach into + // the other's half of the store. + let remote_on_disk = entry.is_some_and(|signing| signing.is_remote()); + if store.remote_url(pubkey).is_some() || (remote_on_disk && !store.contains(pubkey)) { + return None; + } + Some(delete_local(context, store, pubkey, on_disk)) +} + +/// Remove one of this route's keys: from the store first, then from the +/// definitions file. `on_disk` is whether the file lists it as a local key. +fn delete_local( + context: &KeymanagerContext, + store: &mut ValidatorStore, + pubkey: &BlsPubkey, + on_disk: bool, +) -> KeyStatus { let in_memory = store.remove(pubkey); // Whatever becomes of the definitions file below, this process no longer // signs with the key, so its overrides have nothing left to apply to. A key @@ -884,24 +970,8 @@ fn delete_one( context.settings.forget(pubkey); if !on_disk && !in_memory { - // A removal whose directory flush failed is on disk but may not - // survive a power loss, and a retry lands here. Answering `not_found` - // then would tell the operator the key is gone for good; flush first, - // by saving the file again, and keep the error until that works. - if context - .definitions_unflushed - .load(std::sync::atomic::Ordering::Acquire) - { - let definitions = match ValidatorDefinitions::open(&context.validators_dir) { - Ok(definitions) => definitions, - Err(err) => return KeyStatus::error(err), - }; - if let Err(err) = save_definitions(context, &definitions) { - return KeyStatus::error(format!( - "an earlier removal is on disk but the directory could not be flushed: \ - {err}; the key may come back after a power loss" - )); - } + if let Err(status) = flush_earlier_removal(context) { + return status; } return KeyStatus { status: "not_found".to_string(), @@ -921,7 +991,7 @@ fn delete_one( // a `not_found` that reads as "already gone": acting on that, by importing // the key elsewhere, is what turns a failed delete into two hosts signing // for one validator. - match persist_delete(context, pubkey) { + match persist_delete(context, pubkey, Origin::Local) { Ok(()) => {} // The removal is on disk but may not survive a power loss, and the // keystore stays, so the key could come back enabled, with its @@ -943,38 +1013,98 @@ fn delete_one( } } -/// Whether the definitions file currently holds an entry for `pubkey`. +/// Make an earlier removal durable before a key is reported `not_found`. +/// +/// A removal whose directory flush failed is on disk but may not survive a +/// power loss, and a retry of that delete finds nothing left to remove. +/// Answering `not_found` then would tell the operator the key is gone for +/// good; this flushes first, by saving the file again, and `Err` is the +/// status to answer with until that works. Shared by both routes' deletes. +pub(super) fn flush_earlier_removal( + context: &KeymanagerContext, +) -> std::result::Result<(), KeyStatus> { + if !context + .definitions_unflushed + .load(std::sync::atomic::Ordering::Acquire) + { + return Ok(()); + } + let definitions = + ValidatorDefinitions::open(&context.validators_dir).map_err(KeyStatus::error)?; + save_definitions(context, &definitions).map_err(|err| { + KeyStatus::error(format!( + "an earlier removal is on disk but the directory could not be flushed: {err}; the \ + key may come back after a power loss" + )) + }) +} + +/// How the definitions file says `pubkey` signs, or `None` when it holds no +/// entry for it. /// /// Read from disk rather than from the in-memory store, because the two can /// disagree and it is the file that decides what the next restart loads. A -/// disabled entry counts as present: it is still there, `delete` is still what -/// removes it, and reporting `not_found` for one would leave the operator -/// believing a key is gone while its entry waits to be re-enabled. +/// disabled entry counts as present: it is still there, a delete is still +/// what removes it, and reporting `not_found` for one would leave the +/// operator believing a key is gone while its entry waits to be re-enabled. /// /// An unparseable `voting_public_key` cannot match, matching the filter in /// [`persist_delete`]: if that entry will not be removed, this must not claim /// it was found, or a delete would report success having removed nothing. -fn definitions_contain(context: &KeymanagerContext, pubkey: &BlsPubkey) -> crate::Result { +pub(super) fn definitions_entry( + context: &KeymanagerContext, + pubkey: &BlsPubkey, +) -> crate::Result> { let definitions = ValidatorDefinitions::open(&context.validators_dir)?; - Ok(definitions.0.iter().any(|definition| { - parse_pubkey(&definition.voting_public_key) - .map(|parsed| parsed == *pubkey) - .unwrap_or(false) - })) + Ok(definitions + .0 + .into_iter() + .find(|definition| { + parse_pubkey(&definition.voting_public_key) + .map(|parsed| parsed == *pubkey) + .unwrap_or(false) + }) + .map(|definition| definition.signing)) +} + +/// Which route a definitions entry belongs to: the keystores route's local +/// keys, or the remotekeys route's remote ones. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(super) enum Origin { + Local, + Remote, +} + +impl Origin { + fn owns(self, signing: &SigningDefinition) -> bool { + signing.is_remote() == (self == Self::Remote) + } } -/// Drop a deleted key's entry from the definitions file. +/// Drop a deleted key's entry from the definitions file, if it is an entry of +/// `origin`'s kind. +/// +/// Only that kind: the routes partition the store, and a delete through one +/// must not erase the other's entry for the same key when memory and the file +/// disagree about which kind it is. The file is saved either way, as it was +/// before the routes were two: a save is also what flushes an earlier removal +/// whose directory flush failed (see `save_definitions`). /// /// Deliberately leaves the keystore and password files on disk: `deleted` /// means this client has stopped using the key, not that it destroys operator /// key material on an HTTP call. A definitions file with no entry for them is /// enough to keep them from being loaded again. -fn persist_delete(context: &KeymanagerContext, pubkey: &BlsPubkey) -> crate::Result<()> { +pub(super) fn persist_delete( + context: &KeymanagerContext, + pubkey: &BlsPubkey, + origin: Origin, +) -> crate::Result<()> { let mut definitions = ValidatorDefinitions::open(&context.validators_dir)?; definitions.0.retain(|definition| { - parse_pubkey(&definition.voting_public_key) - .map(|parsed| parsed != *pubkey) - .unwrap_or(true) + let names_key = parse_pubkey(&definition.voting_public_key) + .map(|parsed| parsed == *pubkey) + .unwrap_or(false); + !(names_key && origin.owns(&definition.signing)) }); save_definitions(context, &definitions) } @@ -982,10 +1112,15 @@ fn persist_delete(context: &KeymanagerContext, pubkey: &BlsPubkey) -> crate::Res #[cfg(test)] mod tests { use super::*; + use crate::http_api::router; + use axum::body::Body; + use axum::http::{Request, StatusCode}; + use http_body_util::BodyExt as _; + use tower::ServiceExt as _; const TOKEN: &str = "test-token"; @@ -1028,6 +1163,7 @@ mod tests { definitions_lock: Arc::new(tokio::sync::Mutex::new(())), definitions_unflushed: Arc::new(std::sync::atomic::AtomicBool::new(false)), protection: crate::slashing_protection::SlashingProtection::in_memory(), + default_remote_signer: None, } } @@ -1180,8 +1316,10 @@ mod tests { let existing = ValidatorDefinition { enabled: false, voting_public_key: encode_hex(&pubkey.0), - voting_keystore_path: PathBuf::from("old.json"), - voting_keystore_password_path: PathBuf::from("old.txt"), + signing: SigningDefinition::LocalKeystore { + voting_keystore_path: PathBuf::from("old.json"), + voting_keystore_password_path: PathBuf::from("old.txt"), + }, }; let mut definitions = ValidatorDefinitions(vec![existing.clone()]); @@ -1364,6 +1502,131 @@ mod tests { ); } + /// The two routes must not replace each other's entries. A keystore for + /// a key a remote signer already holds is `duplicate`, and the remote + /// entry survives in memory and on disk; and a remote import of a key + /// the file already lists (here, a disabled local entry) is `duplicate` + /// too, rather than turning the disabled key into a signing one. + #[tokio::test] + async fn neither_route_replaces_the_others_entry() { + let dir = tempfile::tempdir().expect("temp dir"); + let store = Arc::new(RwLock::new(ValidatorStore::new())); + let app = router( + context_with_dirs(store.clone(), dir.path()), + TOKEN.to_string(), + ); + let pubkey = "0x9612d7a727c9d0a22e185a1c768478dfe919cada9266988cb32359c11f2b7b27f4ae4040902382ae2910c15e2b420d07"; + let signer = "https://signer.example:9000"; + + let (_, body) = send( + app.clone(), + authed( + "POST", + "/eth/v1/remotekeys", + serde_json::json!({ "remote_keys": [{ "pubkey": pubkey, "url": signer }] }), + ), + ) + .await; + assert_eq!(body["data"][0]["status"], "imported"); + + let (_, body) = send( + app.clone(), + authed( + "POST", + "/eth/v1/keystores", + serde_json::json!({ "keystores": [keystore_json()], "passwords": [PASSWORD] }), + ), + ) + .await; + assert_eq!(body["data"][0]["status"], "duplicate"); + let parsed = parse_pubkey(pubkey).expect("pubkey"); + assert_eq!( + store.read().await.remote_url(&parsed).as_deref(), + Some(signer), + "the remote entry must survive in memory" + ); + let reloaded = ValidatorStore::load(dir.path()).expect("loads"); + assert_eq!(reloaded.remote_url(&parsed).as_deref(), Some(signer)); + + // The other direction: a disabled local entry on disk, nothing in + // memory, and a remote import for the same key. + let other = tempfile::tempdir().expect("temp dir"); + let mut definitions = ValidatorDefinitions::default(); + definitions.0.push(ValidatorDefinition { + enabled: false, + voting_public_key: pubkey.to_string(), + signing: SigningDefinition::LocalKeystore { + voting_keystore_path: PathBuf::from("keystore.json"), + voting_keystore_password_path: PathBuf::from("password.txt"), + }, + }); + definitions.save(other.path()).expect("saves"); + let app = router( + context_with_dirs(Arc::new(RwLock::new(ValidatorStore::new())), other.path()), + TOKEN.to_string(), + ); + let (_, body) = send( + app, + authed( + "POST", + "/eth/v1/remotekeys", + serde_json::json!({ "remote_keys": [{ "pubkey": pubkey, "url": signer }] }), + ), + ) + .await; + assert_eq!(body["data"][0]["status"], "duplicate"); + let on_disk = ValidatorDefinitions::open(other.path()).expect("opens"); + assert!( + !on_disk.0[0].enabled && !on_disk.0[0].signing.is_remote(), + "the disabled local entry must be untouched" + ); + } + + /// Memory holding a local key while the file holds a remote entry for + /// it: a delete through `/keystores` stops the local key signing, and + /// must leave the other route's file entry alone. + #[tokio::test] + async fn a_delete_never_erases_the_other_routes_file_entry() { + let dir = tempfile::tempdir().expect("temp dir"); + let mut store = ValidatorStore::new(); + let secret: [u8; 32] = + hex::decode("000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f") + .expect("valid hex") + .try_into() + .expect("32 bytes"); + let pubkey = store.insert_secret("test", &secret).expect("inserts"); + let hex = encode_hex(&pubkey.0); + ValidatorDefinitions(vec![ValidatorDefinition { + enabled: true, + voting_public_key: hex.clone(), + signing: SigningDefinition::Web3Signer { + url: "https://signer.example:9000".to_string(), + }, + }]) + .save(dir.path()) + .expect("saves"); + let store = Arc::new(RwLock::new(store)); + let app = router( + context_with_dirs(store.clone(), dir.path()), + TOKEN.to_string(), + ); + + let (_, body) = send( + app, + authed( + "DELETE", + "/eth/v1/keystores", + serde_json::json!({ "pubkeys": [hex] }), + ), + ) + .await; + assert_eq!(body["data"][0]["status"], "deleted"); + assert!(store.read().await.is_empty(), "the local key stops signing"); + let on_disk = ValidatorDefinitions::open(dir.path()).expect("opens"); + assert_eq!(on_disk.0.len(), 1, "the remote entry is not this route's"); + assert!(on_disk.0[0].signing.is_remote()); + } + /// An imported key is registered in the slashing-protection record before /// it is loaded, so the next start finds a record for it instead of /// refusing it. @@ -1485,8 +1748,10 @@ mod tests { let previous = ValidatorDefinition { enabled: false, voting_public_key: format!("0x{}", keystore["pubkey"].as_str().expect("pubkey")), - voting_keystore_path: dir.path().join("old.json"), - voting_keystore_password_path: dir.path().join("old"), + signing: SigningDefinition::LocalKeystore { + voting_keystore_path: dir.path().join("old.json"), + voting_keystore_password_path: dir.path().join("old"), + }, }; ValidatorDefinitions(vec![previous.clone()]) .save(dir.path()) @@ -1554,8 +1819,10 @@ mod tests { let previous = ValidatorDefinition { enabled: true, voting_public_key: base.clone(), - voting_keystore_path: keystore_path.clone(), - voting_keystore_password_path: password_path.clone(), + signing: SigningDefinition::LocalKeystore { + voting_keystore_path: keystore_path.clone(), + voting_keystore_password_path: password_path.clone(), + }, }; ValidatorDefinitions(vec![previous.clone()]) .save(dir.path()) @@ -1608,8 +1875,10 @@ mod tests { let previous = ValidatorDefinition { enabled: true, voting_public_key: base.clone(), - voting_keystore_path: keystore_path.clone(), - voting_keystore_password_path: dir.path().join("old"), + signing: SigningDefinition::LocalKeystore { + voting_keystore_path: keystore_path.clone(), + voting_keystore_password_path: dir.path().join("old"), + }, }; ValidatorDefinitions(vec![previous.clone()]) .save(dir.path()) diff --git a/crates/validator/src/http_api/mod.rs b/crates/validator/src/http_api/mod.rs index 189e5ac7a..4b8500f25 100644 --- a/crates/validator/src/http_api/mod.rs +++ b/crates/validator/src/http_api/mod.rs @@ -24,6 +24,7 @@ use crate::secure_fs; pub mod keystores; pub mod proposer; +pub mod remotekeys; pub use keystores::SharedStore; @@ -52,12 +53,22 @@ pub struct KeymanagerContext { /// each handler across its entire read-modify-write cycle (not just /// `store`), is what actually orders those cycles. /// - /// Guards the file only, never `store`: holding it across the slow - /// EIP-2335 key derivation an import performs must not stall the duty - /// loop, which only ever waits on `store`. Where a handler needs both, - /// it takes this one first and `store` second, consistently, so the two - /// never deadlock against each other. + /// Every keymanager handler that changes the store or the file holds it + /// until both changes are done, so the two are always judged and changed + /// together: a handler that released it between writing the file and + /// inserting into the store would let a concurrent delete land in the + /// gap and leave memory and disk disagreeing. + /// + /// The duty loop never takes it, so holding it across the slow EIP-2335 + /// key derivation an import performs stalls only other keymanager + /// requests. Where a handler needs both, it takes this one first and + /// `store` second, consistently, so the two never deadlock against each + /// other, and it holds `store` only for in-memory work, never across file + /// I/O or the derivation. pub definitions_lock: Arc>, + /// `--web3signer-url`: the signer a remote key imported without a `url` + /// of its own is assigned to. `None` makes such an import an error. + pub default_remote_signer: Option, /// Whether the last save of the definitions file replaced it without /// flushing the directory, so its last change may not survive a power /// loss. Set and cleared by `keystores::save_definitions`; read where a @@ -142,6 +153,12 @@ pub fn router(context: KeymanagerContext, token: String) -> Router { .post(keystores::import) .delete(keystores::delete), ) + .route( + "/eth/v1/remotekeys", + get(remotekeys::list) + .post(remotekeys::import) + .delete(remotekeys::delete), + ) .route( "/eth/v1/validator/{pubkey}/feerecipient", get(proposer::get_fee_recipient) diff --git a/crates/validator/src/http_api/proposer.rs b/crates/validator/src/http_api/proposer.rs index 91c5ab5e4..0d63984dd 100644 --- a/crates/validator/src/http_api/proposer.rs +++ b/crates/validator/src/http_api/proposer.rs @@ -367,6 +367,7 @@ mod tests { validators_dir: dir.path().to_path_buf(), secrets_dir: dir.path().to_path_buf(), definitions_lock: Arc::new(tokio::sync::Mutex::new(())), + default_remote_signer: None, definitions_unflushed: Arc::new(std::sync::atomic::AtomicBool::new(false)), protection: crate::slashing_protection::SlashingProtection::in_memory(), }; diff --git a/crates/validator/src/http_api/remotekeys.rs b/crates/validator/src/http_api/remotekeys.rs new file mode 100644 index 000000000..470bcc410 --- /dev/null +++ b/crates/validator/src/http_api/remotekeys.rs @@ -0,0 +1,893 @@ +//! `GET`, `POST` and `DELETE /eth/v1/remotekeys`: the validators a Web3Signer +//! signs for. +//! +//! The counterpart of `/eth/v1/keystores`, and shaped like it, with the key +//! material replaced by a URL. The two routes partition the store: this one +//! lists and deletes remote keys only, that one local keys only, and a key +//! asked of the wrong route is `not_found`. An import here of a key the +//! other route holds is `duplicate`, since the specification's word for "the +//! pubkey is already known to the validator client" covers both. +//! +//! Nothing is asked of the signer at import. The signer may start after this +//! client, or hold the key only later, and a key it turns out not to have +//! fails at its first signature, where it is logged and counted, the same way +//! an unreachable signer is. +//! +//! `readonly` is always `false`: this client has no keys it cannot delete. +//! +//! # Slashing protection +//! +//! A remote key is registered in this client's slashing-protection record +//! before it is loaded, as a local one is, and is checked against it before +//! every block and attestation: the signer keeps a record of its own, and +//! this one is kept as well, so the key is covered whichever of the two a +//! message meets first. Unlike `/eth/v1/keystores`, these routes carry no +//! history in or out, since the specification gives them no +//! `slashing_protection` field. A key imported with no record here starts as +//! never having signed, with a warning, and `DELETE` leaves the record where +//! it is, so a key imported again continues from it. + +use std::collections::{HashMap, HashSet}; +use std::sync::Arc; + +use axum::{Json, extract::State}; +use ethlambda_types::beacon::primitives::BlsPubkey; +use serde::{Deserialize, Serialize}; +use tracing::{info, warn}; + +use crate::Error; +use crate::beacon_node::dto::{encode_hex, parse_pubkey}; +use crate::http_api::KeymanagerContext; +use crate::http_api::keystores::{ + DeleteRequest, KeyStatus, Origin, StatusResponse, definitions_entry, flush_earlier_removal, + persist_delete, save_definitions, +}; +use crate::keys::{ + SigningDefinition, ValidatorDefinition, ValidatorDefinitions, ValidatorStore, Web3Signer, +}; + +#[derive(Debug, Serialize)] +pub struct ListResponse { + pub data: Vec, +} + +#[derive(Debug, Serialize)] +pub struct RemoteKey { + pub pubkey: String, + pub url: String, + pub readonly: bool, +} + +#[derive(Debug, Deserialize)] +pub struct ImportRequest { + pub remote_keys: Vec, +} + +#[derive(Debug, Deserialize)] +pub struct ImportRemoteKey { + pub pubkey: String, + /// The signer's base URL. Absent means `--web3signer-url`, and an import + /// with neither is an error for that key. + #[serde(default)] + pub url: Option, +} + +pub async fn list(State(context): State) -> Json { + let store = context.store.read().await; + let mut data: Vec = store + .remote_entries() + .into_iter() + .map(|(pubkey, url)| RemoteKey { + pubkey: encode_hex(&pubkey.0), + url, + readonly: false, + }) + .collect(); + // The store is a hash map; a stable order makes the answer diffable. + data.sort_by(|a, b| a.pubkey.cmp(&b.pubkey)); + Json(ListResponse { data }) +} + +/// The most keys one `POST /eth/v1/remotekeys` may carry. Larger than the +/// keystores route's cap, since a remote import does no key derivation, but +/// still bounded: the whole batch is judged and persisted while +/// `definitions_lock` is held, and a migration of thousands of keys is a few +/// requests rather than one unbounded one. +pub const MAX_REMOTE_KEYS_PER_IMPORT: usize = 1000; + +/// One key of an import, judged: the key and its signer's client to insert, +/// `None` for a duplicate, or the status to answer with instead. +type Prepared = std::result::Result)>, KeyStatus>; + +pub async fn import( + State(context): State, + Json(request): Json, +) -> Json { + let fail_all = |message: &str| { + let data = request + .remote_keys + .iter() + .map(|_| KeyStatus::error(message)) + .collect(); + Json(StatusResponse { data }) + }; + + if request.remote_keys.len() > MAX_REMOTE_KEYS_PER_IMPORT { + warn!( + count = request.remote_keys.len(), + "Rejecting oversized remote key import request" + ); + return fail_all(&format!( + "request carries {} remote keys, more than the limit of \ + {MAX_REMOTE_KEYS_PER_IMPORT}", + request.remote_keys.len() + )); + } + + // Held until the keys are in the store, not just until they are on disk. + // Every keymanager writer takes this first, so while it is held nothing + // else can change the file or the store: what is judged below against + // the two is still true at the insert, and no re-check is needed there. + // See `KeymanagerContext::definitions_lock` for the lock order. + let _definitions_guard = context.definitions_lock.lock().await; + + let mut definitions = match ValidatorDefinitions::open(&context.validators_dir) { + Ok(definitions) => definitions, + Err(err) => return fail_all(&err.to_string()), + }; + + // What the store holds, read under a guard released at once: nothing + // below holds the store across file I/O or client construction, so a + // duty's per-signature read never waits on this request. + let (known, mut signers) = { + let store = context.store.read().await; + let known: HashSet = store.pubkeys().into_iter().collect(); + (known, store.remote_signers()) + }; + + let mut prepared: Vec = request + .remote_keys + .iter() + .map(|key| prepare_import(&context, &known, &mut signers, &mut definitions, key)) + .collect(); + + // One save for the whole batch rather than one per key. A failure fails + // every key that would have been added, and none is put in the store: + // a key this process signs with must already be durable. + let staged: Vec = prepared + .iter() + .filter_map(|result| match result { + Ok(Some((pubkey, _))) => Some(*pubkey), + _ => None, + }) + .collect(); + let mut failure = None; + if !staged.is_empty() { + match save_definitions(&context, &definitions) { + Ok(()) => {} + // The entries are on disk and every reader sees them; only their + // survival of a power loss is in doubt, and an entry lost that way + // only removes a key. Counted as persisted, as a local import is. + Err(Error::Unflushed { path, source }) => warn!( + %path, + %source, + "Validator definitions written, but the directory could not be flushed" + ), + Err(err) => failure = Some(err.to_string()), + } + } + + // Every key is registered in the slashing-protection record, in one + // durable write, before any of them is loaded: the duty path must never + // meet a key with no record. After the save rather than before, for the + // reason `keystores::import` gives: a key whose save failed leaves no + // record behind to let a hand-written entry past the startup check later. + if failure.is_none() && !staged.is_empty() { + let registering = staged.clone(); + let registration = + crate::slashing_protection::blocking(&context.protection, move |protection| { + protection.absorb(None, ®istering) + }) + .await; + match registration { + Ok(absorbed) => { + if absorbed.registered > 0 { + warn!( + registered = absorbed.registered, + "Imported remote validator keys with no slashing-protection record \ + here; each starts as never having signed" + ); + } + } + // On disk and in memory, with only a power loss in doubt, and a + // registration lost that way is refused at the next start. So the + // keys are loaded, as `keystores::import` loads its own. + Err(Error::Unflushed { path, source }) => warn!( + %path, + %source, + "Slashing-protection records written, but the directory could not be flushed; \ + the imported keys are registered" + ), + // The write is known to have failed, so the entries this request + // added come off the file again: left there, the next start would + // refuse the whole client for a key with no record. + Err(err) => { + let added: HashSet = staged.iter().copied().collect(); + definitions.0.retain(|definition| { + parse_pubkey(&definition.voting_public_key) + .map(|parsed| !added.contains(&parsed)) + .unwrap_or(true) + }); + failure = Some(match save_definitions(&context, &definitions) { + Ok(()) | Err(Error::Unflushed { .. }) => format!( + "not imported: its slashing-protection record could not be written: {err}" + ), + Err(undo) => { + tracing::error!( + %undo, + "Could not undo a failed remote import; the next start refuses its \ + keys until they have a slashing-protection record" + ); + format!( + "saved, but not loaded: its slashing-protection record could not be \ + written: {err}" + ) + } + }); + } + } + } + if let Some(message) = &failure { + for result in &mut prepared { + if matches!(result, Ok(Some(_))) { + *result = Err(KeyStatus::error(message)); + } + } + } + + // The write lock covers map inserts and nothing else: every client was + // built above. + let mut store = context.store.write().await; + let data = prepared + .into_iter() + .map(|result| match result { + Ok(Some((pubkey, signer))) => { + info!(pubkey = %encode_hex(&pubkey.0), url = signer.url(), "Imported remote validator key"); + store.insert_remote_signer(pubkey, signer); + KeyStatus { + status: "imported".to_string(), + message: None, + } + } + Ok(None) => KeyStatus::duplicate(), + Err(status) => status, + }) + .collect(); + drop(store); + + Json(StatusResponse { data }) +} + +/// Judge one key of an import and stage its definition: resolve the URL, +/// refuse a key already known, build (or reuse) the signer's client, and +/// append the entry to `definitions`, which the caller saves once for the +/// whole batch. +/// +/// `Ok(None)` is a duplicate. `known` is the store's keys; `definitions` +/// already holds the entries staged for earlier keys in the batch, so a key +/// named twice in one request is a duplicate the second time. +fn prepare_import( + context: &KeymanagerContext, + known: &HashSet, + signers: &mut HashMap>, + definitions: &mut ValidatorDefinitions, + key: &ImportRemoteKey, +) -> Prepared { + let pubkey = parse_pubkey(&key.pubkey).map_err(KeyStatus::error)?; + let url = match key + .url + .as_deref() + .or(context.default_remote_signer.as_deref()) + { + Some(url) => url.trim_end_matches('/').to_string(), + None => { + return Err(KeyStatus::error( + "no url supplied for this key and no --web3signer-url to default to", + )); + } + }; + + // Known to the store, or to the file: a disabled local entry, or one + // left behind by a failed delete, is still "already known", and + // overwriting it would turn a key the operator disabled into a signing + // one. Judged before anything is built or staged. + let on_disk = definitions.0.iter().any(|definition| { + parse_pubkey(&definition.voting_public_key) + .map(|parsed| parsed == pubkey) + .unwrap_or(false) + }); + if on_disk || known.contains(&pubkey) { + return Ok(None); + } + + // Built here, outside the store's lock, and once per URL: the client is + // the one the store will keep, so the URL's shape is checked by building + // the thing that will use it, and an unusable URL is refused before it + // is persisted and refuses the next startup. + let signer = match signers.get(&url) { + Some(signer) => signer.clone(), + None => { + let signer = Arc::new(Web3Signer::new(&url).map_err(KeyStatus::error)?); + signers.insert(url.clone(), signer.clone()); + signer + } + }; + + definitions.0.push(ValidatorDefinition { + enabled: true, + voting_public_key: encode_hex(&pubkey.0), + signing: SigningDefinition::Web3Signer { url }, + }); + Ok(Some((pubkey, signer))) +} + +pub async fn delete( + State(context): State, + Json(request): Json, +) -> Json { + let _definitions_guard = context.definitions_lock.lock().await; + + let mut store = context.store.write().await; + let data = request + .pubkeys + .iter() + .map(|text| match parse_pubkey(text) { + Ok(pubkey) => delete_one(&context, &mut store, &pubkey), + Err(err) => KeyStatus::error(err), + }) + .collect(); + drop(store); + + Json(StatusResponse { data }) +} + +/// The keystores route's delete, gated the other way: only a remote key is +/// this route's to remove, and a local one is `not_found` here whatever its +/// state. The on-disk check comes first and decides `not_found`, for the +/// reason spelled out on `keystores::delete_one`. +fn delete_one( + context: &KeymanagerContext, + store: &mut ValidatorStore, + pubkey: ðlambda_types::beacon::primitives::BlsPubkey, +) -> KeyStatus { + let on_disk = match definitions_entry(context, pubkey) { + Ok(entry) => entry.is_some_and(|signing| signing.is_remote()), + Err(err) => return KeyStatus::error(err), + }; + let in_memory = store.remote_url(pubkey).is_some(); + if !on_disk && !in_memory { + // An earlier removal whose directory flush failed must reach the disk + // for good before this says the key is gone; see + // `keystores::flush_earlier_removal`. + if let Err(status) = flush_earlier_removal(context) { + return status; + } + return KeyStatus { + status: "not_found".to_string(), + message: None, + }; + } + + // The in-memory removal first, so this process stops signing with the + // key whatever becomes of the file; a failed persist is reported, not + // swallowed, so a retry gets the same error rather than a `not_found` + // that reads as "already gone". + if in_memory { + store.remove(pubkey); + } + context.settings.forget(pubkey); + match persist_delete(context, pubkey, Origin::Remote) { + Ok(()) => {} + // On disk, but it may not survive a power loss, so the key could come + // back enabled. An error, so no caller takes the key for gone; this + // process does not sign with it. + Err(Error::Unflushed { path, source }) => { + return KeyStatus::error(format!( + "removed from {path}, but the directory could not be flushed: {source}; the key \ + may come back after a power loss" + )); + } + Err(err) => return KeyStatus::error(err), + } + + info!(pubkey = %encode_hex(&pubkey.0), "Deleted remote validator key"); + KeyStatus { + status: "deleted".to_string(), + message: None, + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::http_api::{SharedStore, router}; + use axum::body::Body; + use axum::http::{Request, StatusCode}; + use http_body_util::BodyExt as _; + use std::sync::Arc; + use tokio::sync::RwLock; + use tower::ServiceExt as _; + + const TOKEN: &str = "test-token"; + const SIGNER: &str = "https://signer.example:9000"; + + fn pubkey_hex(byte: u8) -> String { + format!("0x{}", hex::encode([byte; 48])) + } + + fn context_with_dirs( + store: SharedStore, + dir: &std::path::Path, + default_remote_signer: Option<&str>, + ) -> KeymanagerContext { + KeymanagerContext { + store, + settings: Arc::new(crate::proposer_settings::ProposerSettings::new( + ethlambda_types::beacon::primitives::Bytes32::ZERO, + None, + )), + validators_dir: dir.to_path_buf(), + secrets_dir: dir.to_path_buf(), + definitions_lock: Arc::new(tokio::sync::Mutex::new(())), + definitions_unflushed: Arc::new(std::sync::atomic::AtomicBool::new(false)), + protection: crate::slashing_protection::SlashingProtection::in_memory(), + default_remote_signer: default_remote_signer.map(str::to_string), + } + } + + async fn send(app: axum::Router, request: Request) -> (StatusCode, serde_json::Value) { + let response = app.oneshot(request).await.expect("responds"); + let status = response.status(); + let bytes = response + .into_body() + .collect() + .await + .expect("body") + .to_bytes(); + let json = serde_json::from_slice(&bytes).unwrap_or(serde_json::Value::Null); + (status, json) + } + + fn authed(method: &str, uri: &str, body: serde_json::Value) -> Request { + Request::builder() + .method(method) + .uri(uri) + .header("authorization", format!("Bearer {TOKEN}")) + .header("content-type", "application/json") + .body(Body::from(body.to_string())) + .expect("request") + } + + fn statuses(body: &serde_json::Value) -> Vec { + body["data"] + .as_array() + .expect("data array") + .iter() + .map(|entry| entry["status"].as_str().expect("status").to_string()) + .collect() + } + + #[tokio::test] + async fn the_route_is_behind_the_bearer_token() { + let dir = tempfile::tempdir().expect("temp dir"); + let app = router( + context_with_dirs( + Arc::new(RwLock::new(ValidatorStore::new())), + dir.path(), + None, + ), + TOKEN.to_string(), + ); + let request = Request::builder() + .uri("/eth/v1/remotekeys") + .body(Body::empty()) + .expect("request"); + let (status, _) = send(app, request).await; + assert_eq!(status, StatusCode::UNAUTHORIZED); + } + + #[tokio::test] + async fn import_list_delete_round_trip() { + let dir = tempfile::tempdir().expect("temp dir"); + let store = Arc::new(RwLock::new(ValidatorStore::new())); + let app = router( + context_with_dirs(store.clone(), dir.path(), None), + TOKEN.to_string(), + ); + + let (status, body) = send( + app.clone(), + authed( + "POST", + "/eth/v1/remotekeys", + serde_json::json!({ "remote_keys": [ + { "pubkey": pubkey_hex(1), "url": SIGNER }, + { "pubkey": pubkey_hex(2), "url": format!("{SIGNER}/") }, + ] }), + ), + ) + .await; + assert_eq!(status, StatusCode::OK); + assert_eq!(statuses(&body), ["imported", "imported"]); + + let (status, body) = send( + app.clone(), + authed("GET", "/eth/v1/remotekeys", serde_json::json!({})), + ) + .await; + assert_eq!(status, StatusCode::OK); + let listed = body["data"].as_array().expect("data"); + assert_eq!(listed.len(), 2); + assert_eq!(listed[0]["pubkey"], pubkey_hex(1)); + assert_eq!(listed[0]["url"], SIGNER, "the trailing slash is normalized"); + assert_eq!(listed[1]["url"], SIGNER); + assert_eq!(listed[0]["readonly"], false); + + // Persisted: a fresh load from the directory holds both as remote. + let reloaded = ValidatorStore::load(dir.path()).expect("loads"); + assert_eq!(reloaded.remote_entries().len(), 2); + assert!(reloaded.local_pubkeys().is_empty()); + + // The keystores route does not list them. + let (_, body) = send( + app.clone(), + authed("GET", "/eth/v1/keystores", serde_json::json!({})), + ) + .await; + assert!(body["data"].as_array().expect("data").is_empty()); + + let (status, body) = send( + app.clone(), + authed( + "DELETE", + "/eth/v1/remotekeys", + serde_json::json!({ "pubkeys": [pubkey_hex(1), pubkey_hex(3)] }), + ), + ) + .await; + assert_eq!(status, StatusCode::OK); + assert_eq!(statuses(&body), ["deleted", "not_found"]); + assert!( + body.get("slashing_protection").is_none(), + "the remote route has no interchange to report" + ); + + assert_eq!(store.read().await.len(), 1); + let reloaded = ValidatorStore::load(dir.path()).expect("loads"); + assert_eq!(reloaded.len(), 1, "the delete is persisted"); + } + + #[tokio::test] + async fn a_missing_url_falls_back_to_the_configured_signer_or_errors() { + let dir = tempfile::tempdir().expect("temp dir"); + let with_default = router( + context_with_dirs( + Arc::new(RwLock::new(ValidatorStore::new())), + dir.path(), + Some(SIGNER), + ), + TOKEN.to_string(), + ); + let (_, body) = send( + with_default.clone(), + authed( + "POST", + "/eth/v1/remotekeys", + serde_json::json!({ "remote_keys": [{ "pubkey": pubkey_hex(1) }] }), + ), + ) + .await; + assert_eq!(statuses(&body), ["imported"]); + let (_, body) = send( + with_default, + authed("GET", "/eth/v1/remotekeys", serde_json::json!({})), + ) + .await; + assert_eq!(body["data"][0]["url"], SIGNER); + + let other = tempfile::tempdir().expect("temp dir"); + let without_default = router( + context_with_dirs( + Arc::new(RwLock::new(ValidatorStore::new())), + other.path(), + None, + ), + TOKEN.to_string(), + ); + let (_, body) = send( + without_default, + authed( + "POST", + "/eth/v1/remotekeys", + serde_json::json!({ "remote_keys": [{ "pubkey": pubkey_hex(1) }] }), + ), + ) + .await; + assert_eq!(statuses(&body), ["error"]); + assert!( + body["data"][0]["message"] + .as_str() + .expect("message") + .contains("--web3signer-url") + ); + assert!( + ValidatorStore::load(other.path()) + .expect("loads") + .is_empty(), + "an errored import persists nothing" + ); + } + + #[tokio::test] + async fn an_already_held_key_is_a_duplicate_whichever_route_holds_it() { + let dir = tempfile::tempdir().expect("temp dir"); + let mut store = ValidatorStore::new(); + let local = store + .insert_secret( + "test", + &hex::decode("000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f") + .expect("hex") + .try_into() + .expect("32 bytes"), + ) + .expect("inserts"); + let store = Arc::new(RwLock::new(store)); + let app = router( + context_with_dirs(store.clone(), dir.path(), None), + TOKEN.to_string(), + ); + + let import = |pubkey: String| { + authed( + "POST", + "/eth/v1/remotekeys", + serde_json::json!({ "remote_keys": [{ "pubkey": pubkey, "url": SIGNER }] }), + ) + }; + let (_, body) = send(app.clone(), import(pubkey_hex(1))).await; + assert_eq!(statuses(&body), ["imported"]); + let (_, body) = send(app.clone(), import(pubkey_hex(1))).await; + assert_eq!(statuses(&body), ["duplicate"]); + let (_, body) = send(app.clone(), import(encode_hex(&local.0))).await; + assert_eq!(statuses(&body), ["duplicate"]); + + assert!( + store.read().await.remote_url(&local).is_none(), + "a duplicate must not replace the local key with a remote one" + ); + } + + #[tokio::test] + async fn a_bad_pubkey_or_url_is_an_error_for_that_key_alone() { + let dir = tempfile::tempdir().expect("temp dir"); + let app = router( + context_with_dirs( + Arc::new(RwLock::new(ValidatorStore::new())), + dir.path(), + None, + ), + TOKEN.to_string(), + ); + let (_, body) = send( + app, + authed( + "POST", + "/eth/v1/remotekeys", + serde_json::json!({ "remote_keys": [ + { "pubkey": "0xnope", "url": SIGNER }, + { "pubkey": pubkey_hex(1), "url": "ftp://signer" }, + { "pubkey": pubkey_hex(2), "url": SIGNER }, + ] }), + ), + ) + .await; + assert_eq!(statuses(&body), ["error", "error", "imported"]); + } + + /// The two routes partition the store: a key each one holds is + /// `not_found` to the other's delete, and only its own route removes it. + #[tokio::test] + async fn each_route_deletes_only_its_own_keys() { + let dir = tempfile::tempdir().expect("temp dir"); + let store = Arc::new(RwLock::new(ValidatorStore::new())); + let app = router( + context_with_dirs(store.clone(), dir.path(), None), + TOKEN.to_string(), + ); + let (_, body) = send( + app.clone(), + authed( + "POST", + "/eth/v1/remotekeys", + serde_json::json!({ "remote_keys": [{ "pubkey": pubkey_hex(1), "url": SIGNER }] }), + ), + ) + .await; + assert_eq!(statuses(&body), ["imported"]); + + let (_, body) = send( + app.clone(), + authed( + "DELETE", + "/eth/v1/keystores", + serde_json::json!({ "pubkeys": [pubkey_hex(1)] }), + ), + ) + .await; + // `not_found`, not `not_active`: the key has a slashing-protection + // record, but it is held, by the other route, and still signing. Its + // record is not handed over as though the key were gone. + assert_eq!(statuses(&body), ["not_found"]); + let exported: serde_json::Value = + serde_json::from_str(body["slashing_protection"].as_str().expect("a string")) + .expect("an interchange"); + assert_eq!(exported["data"], serde_json::json!([]), "got {exported}"); + assert_eq!( + store.read().await.len(), + 1, + "the keystores route left it alone" + ); + assert_eq!(ValidatorStore::load(dir.path()).expect("loads").len(), 1); + + let (_, body) = send( + app, + authed( + "DELETE", + "/eth/v1/remotekeys", + serde_json::json!({ "pubkeys": [pubkey_hex(1)] }), + ), + ) + .await; + assert_eq!(statuses(&body), ["deleted"]); + assert!(store.read().await.is_empty()); + } + + /// A context over `dir` whose slashing protection is a real record there, + /// for the chain `0x00..00`, and the handle to inspect it with. + fn context_with_record( + store: SharedStore, + dir: &std::path::Path, + ) -> ( + KeymanagerContext, + crate::slashing_protection::SharedProtection, + ) { + let protection = crate::slashing_protection::SlashingProtection::open( + dir, + ethlambda_types::beacon::primitives::Root::ZERO, + &[], + false, + ) + .expect("opens with no keys"); + let mut context = context_with_dirs(store, dir, None); + context.protection = Arc::new(std::sync::Mutex::new(protection)); + let protection = context.protection.clone(); + (context, protection) + } + + /// A remote key is registered in this client's slashing-protection record + /// before it is loaded, as a local one is: the duty path checks every key + /// against the record, and the next start refuses one that has none. + #[tokio::test] + async fn an_imported_remote_key_is_registered_for_slashing_protection() { + let dir = tempfile::tempdir().expect("temp dir"); + let store = Arc::new(RwLock::new(ValidatorStore::new())); + let (context, protection) = context_with_record(store.clone(), dir.path()); + + let (_, body) = send( + router(context, TOKEN.to_string()), + authed( + "POST", + "/eth/v1/remotekeys", + serde_json::json!({ "remote_keys": [{ "pubkey": pubkey_hex(1), "url": SIGNER }] }), + ), + ) + .await; + assert_eq!(statuses(&body), ["imported"]); + + let pubkey = parse_pubkey(&pubkey_hex(1)).expect("valid"); + assert!(store.read().await.contains(&pubkey), "loaded"); + assert!( + protection.lock().expect("lock").contains(&pubkey), + "and registered" + ); + } + + /// A registration that fails costs the request only: the entries it added + /// come off the file again and no key is loaded. Left on the file, a key + /// with no record would have the next start refuse the whole client. + #[tokio::test] + async fn a_failed_registration_rolls_the_remote_import_back() { + let dir = tempfile::tempdir().expect("temp dir"); + let store = Arc::new(RwLock::new(ValidatorStore::new())); + let (context, protection) = context_with_record(store.clone(), dir.path()); + // A directory in the record's place makes the registration's rename + // fail, whoever runs the test. + let record = protection + .lock() + .expect("lock") + .path() + .expect("persisted") + .to_path_buf(); + std::fs::remove_file(&record).expect("removes the record"); + std::fs::create_dir(&record).expect("a directory in its place"); + + let (_, body) = send( + router(context, TOKEN.to_string()), + authed( + "POST", + "/eth/v1/remotekeys", + serde_json::json!({ "remote_keys": [ + { "pubkey": pubkey_hex(1), "url": SIGNER }, + { "pubkey": pubkey_hex(2), "url": SIGNER }, + ] }), + ), + ) + .await; + assert_eq!(statuses(&body), ["error", "error"], "got {body}"); + assert!( + body["data"][0]["message"] + .as_str() + .unwrap_or_default() + .starts_with("not imported"), + "got {body}" + ); + assert!(store.read().await.is_empty(), "not loaded"); + assert!( + ValidatorDefinitions::open(dir.path()) + .expect("opens") + .0 + .is_empty(), + "the next start finds no key to refuse" + ); + } + + /// A remote entry still on disk after a failed delete must not be + /// `not_found`, for the reason the keystores route's is not: an operator + /// who reads `not_found` moves the key elsewhere, and the next restart + /// brings it back here too. + #[tokio::test] + async fn a_remote_key_still_on_disk_is_not_reported_not_found() { + let dir = tempfile::tempdir().expect("temp dir"); + let store = Arc::new(RwLock::new(ValidatorStore::new())); + let app = router( + context_with_dirs(store.clone(), dir.path(), None), + TOKEN.to_string(), + ); + let (_, body) = send( + app.clone(), + authed( + "POST", + "/eth/v1/remotekeys", + serde_json::json!({ "remote_keys": [{ "pubkey": pubkey_hex(1), "url": SIGNER }] }), + ), + ) + .await; + assert_eq!(statuses(&body), ["imported"]); + + // Gone from memory, still on disk. + let pubkey = parse_pubkey(&pubkey_hex(1)).expect("pubkey"); + assert!(store.write().await.remove(&pubkey)); + + let (_, body) = send( + app, + authed( + "DELETE", + "/eth/v1/remotekeys", + serde_json::json!({ "pubkeys": [pubkey_hex(1)] }), + ), + ) + .await; + assert_eq!(statuses(&body), ["deleted"]); + assert!(ValidatorStore::load(dir.path()).expect("loads").is_empty()); + } +} diff --git a/crates/validator/src/keys/definitions.rs b/crates/validator/src/keys/definitions.rs index 3b091585a..a34617024 100644 --- a/crates/validator/src/keys/definitions.rs +++ b/crates/validator/src/keys/definitions.rs @@ -4,8 +4,10 @@ //! keystores themselves. It is what the keymanager API mutates, so a key //! imported at runtime is still there after a restart. +use std::collections::BTreeMap; use std::path::{Path, PathBuf}; +use serde::de::IgnoredAny; use serde::{Deserialize, Serialize}; use crate::error::{Error, Result}; @@ -15,16 +17,190 @@ use crate::secure_fs; pub const DEFINITIONS_FILE: &str = "validator_definitions.yml"; #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(try_from = "RawDefinition", into = "RawDefinition")] pub struct ValidatorDefinition { /// Whether this validator should perform duties. A disabled entry keeps its /// keystore on disk and is simply not loaded. pub enabled: bool, /// The validator's BLS public key, `0x`-prefixed hex. pub voting_public_key: String, - /// Path to the EIP-2335 keystore. - pub voting_keystore_path: PathBuf, - /// Path to the file holding that keystore's password. - pub voting_keystore_password_path: PathBuf, + /// Where the key is and how it signs. Kept at the top level of the entry + /// in the file, beside a `type` naming it; see [`RawDefinition`]. + pub signing: SigningDefinition, +} + +/// How one definition's key signs. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum SigningDefinition { + /// An EIP-2335 keystore on disk, decrypted into this process at startup. + LocalKeystore { + /// Path to the EIP-2335 keystore. + voting_keystore_path: PathBuf, + /// Path to the file holding that keystore's password. + voting_keystore_password_path: PathBuf, + }, + /// A Web3Signer holding the key; `url` is the signer's base URL. + Web3Signer { url: String }, +} + +/// The `type` of an entry, under the names Lighthouse's definitions file uses. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +enum SigningKind { + LocalKeystore, + Web3signer, +} + +/// One entry exactly as the file holds it. +/// +/// Every entry is written with a `type`, and one that has a `type` must carry +/// that method's fields and no other's. An entry without one, which is every +/// entry written before remote signers existed, is read by its fields, and is +/// refused when it names both a keystore and a `url`: loading that as either +/// would silently ignore half of what the operator wrote, and loading it as +/// local would bypass the remote signer and its slashing protection. +/// +/// A field this client does not read is refused on a web3signer entry and +/// skipped on a local one. On a remote entry it is most likely one of +/// Lighthouse's signer settings (`root_certificate_path`, say), and an +/// operator who set it expects it to take effect: skipped, the key would sign +/// through a connection configured differently from the one they wrote down. +/// A local entry has no such field, so a file Lighthouse wrote, with its own +/// per-validator settings on each entry, still loads. +#[derive(Serialize, Deserialize)] +struct RawDefinition { + enabled: bool, + voting_public_key: String, + #[serde(rename = "type", default, skip_serializing_if = "Option::is_none")] + kind: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + voting_keystore_path: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + voting_keystore_password_path: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + url: Option, + /// Every other field the entry carries, by name: what a web3signer entry + /// is refused for. Read only, so a rewritten entry holds the fields above + /// and nothing else. + #[serde(flatten, skip_serializing)] + unread: BTreeMap, +} + +/// The one unread field a web3signer entry may carry. Lighthouse writes +/// `description` on every entry, an empty one included, so refusing it would +/// refuse every remote entry Lighthouse wrote; it is a label, and changes +/// nothing about how the key signs. +const DESCRIPTION_FIELD: &str = "description"; + +impl TryFrom for ValidatorDefinition { + type Error = String; + + fn try_from(raw: RawDefinition) -> std::result::Result { + let names_keystore = + raw.voting_keystore_path.is_some() || raw.voting_keystore_password_path.is_some(); + let names_url = raw.url.is_some(); + let kind = match raw.kind { + Some(SigningKind::LocalKeystore) if names_url => { + return Err(format!( + "{}: a local_keystore entry must not carry a url", + raw.voting_public_key + )); + } + Some(SigningKind::Web3signer) if names_keystore => { + return Err(format!( + "{}: a web3signer entry must not carry keystore paths", + raw.voting_public_key + )); + } + Some(kind) => kind, + None if names_keystore && names_url => { + return Err(format!( + "{}: the entry names both a keystore and a url; remove one, or set \ + `type: local_keystore` or `type: web3signer`", + raw.voting_public_key + )); + } + None if names_url => SigningKind::Web3signer, + None if names_keystore => SigningKind::LocalKeystore, + None => { + return Err(format!( + "{}: the entry names no way to sign: it needs keystore paths or a url", + raw.voting_public_key + )); + } + }; + if kind == SigningKind::Web3signer { + let unread: Vec<&str> = raw + .unread + .keys() + .map(String::as_str) + .filter(|field| *field != DESCRIPTION_FIELD) + .collect(); + if !unread.is_empty() { + return Err(format!( + "{}: a web3signer entry carries fields this client does not implement: {}", + raw.voting_public_key, + unread.join(", ") + )); + } + } + let missing = |field: &str| format!("{}: missing field `{field}`", raw.voting_public_key); + let signing = match kind { + SigningKind::LocalKeystore => SigningDefinition::LocalKeystore { + voting_keystore_path: raw + .voting_keystore_path + .clone() + .ok_or_else(|| missing("voting_keystore_path"))?, + voting_keystore_password_path: raw + .voting_keystore_password_path + .clone() + .ok_or_else(|| missing("voting_keystore_password_path"))?, + }, + SigningKind::Web3signer => SigningDefinition::Web3Signer { + url: raw.url.clone().ok_or_else(|| missing("url"))?, + }, + }; + Ok(Self { + enabled: raw.enabled, + voting_public_key: raw.voting_public_key, + signing, + }) + } +} + +impl From for RawDefinition { + fn from(definition: ValidatorDefinition) -> Self { + let (kind, voting_keystore_path, voting_keystore_password_path, url) = + match definition.signing { + SigningDefinition::LocalKeystore { + voting_keystore_path, + voting_keystore_password_path, + } => ( + SigningKind::LocalKeystore, + Some(voting_keystore_path), + Some(voting_keystore_password_path), + None, + ), + SigningDefinition::Web3Signer { url } => { + (SigningKind::Web3signer, None, None, Some(url)) + } + }; + Self { + enabled: definition.enabled, + voting_public_key: definition.voting_public_key, + kind: Some(kind), + voting_keystore_path, + voting_keystore_password_path, + url, + unread: BTreeMap::new(), + } + } +} + +impl SigningDefinition { + pub fn is_remote(&self) -> bool { + matches!(self, Self::Web3Signer { .. }) + } } #[derive(Debug, Clone, Default, Serialize, Deserialize)] @@ -130,9 +306,11 @@ impl ValidatorDefinitions { let pubkey = format!("0x{}", pubkey.trim_start_matches("0x")); definitions.push(ValidatorDefinition { enabled: true, - voting_keystore_password_path: secrets_dir.join(&pubkey), + signing: SigningDefinition::LocalKeystore { + voting_keystore_path: keystore_path, + voting_keystore_password_path: secrets_dir.join(&pubkey), + }, voting_public_key: pubkey, - voting_keystore_path: keystore_path, }); } // Directory order is not stable across filesystems; sorting makes the @@ -186,8 +364,11 @@ mod tests { let definitions = ValidatorDefinitions::open(validators.path()).unwrap(); assert_eq!(definitions.0[0].voting_public_key, "0xaa"); assert_eq!( - definitions.0[0].voting_keystore_password_path, - secrets.path().join("0xaa") + definitions.0[0].signing, + SigningDefinition::LocalKeystore { + voting_keystore_path: validators.path().join("0xaa").join("voting-keystore.json"), + voting_keystore_password_path: secrets.path().join("0xaa"), + } ); assert!(definitions.0.iter().all(|definition| definition.enabled)); } @@ -216,11 +397,213 @@ mod tests { ValidatorDefinition { enabled, voting_public_key: pubkey.to_string(), - voting_keystore_path: PathBuf::from("keystore.json"), - voting_keystore_password_path: PathBuf::from("password.txt"), + signing: SigningDefinition::LocalKeystore { + voting_keystore_path: PathBuf::from("keystore.json"), + voting_keystore_password_path: PathBuf::from("password.txt"), + }, + } + } + + fn remote_definition(pubkey: &str, url: &str) -> ValidatorDefinition { + ValidatorDefinition { + enabled: true, + voting_public_key: pubkey.to_string(), + signing: SigningDefinition::Web3Signer { + url: url.to_string(), + }, } } + /// A file written before remote signers existed has no `url` and no tag, + /// and must read back as the local entry it always was. + #[test] + fn a_pre_remote_signer_file_still_parses() { + let dir = tempfile::tempdir().expect("temp dir"); + let legacy = "- enabled: true\n voting_public_key: '0xaa'\n \ + voting_keystore_path: keystore.json\n \ + voting_keystore_password_path: password.txt\n"; + std::fs::write(dir.path().join(DEFINITIONS_FILE), legacy).expect("writes"); + + let read = ValidatorDefinitions::open(dir.path()).expect("opens"); + assert_eq!(read.0, vec![definition("0xaa", true)]); + } + + #[test] + fn remote_and_local_entries_round_trip_together() { + let dir = tempfile::tempdir().expect("temp dir"); + let written = ValidatorDefinitions(vec![ + definition("0xaa", true), + remote_definition("0xbb", "https://signer:9000"), + ]); + written.save(dir.path()).expect("saves"); + + let text = std::fs::read_to_string(dir.path().join(DEFINITIONS_FILE)).expect("reads"); + assert!(text.contains("url: https://signer:9000"), "{text}"); + assert!(text.contains("type: web3signer"), "{text}"); + assert!(text.contains("type: local_keystore"), "{text}"); + + let read = ValidatorDefinitions::open(dir.path()).expect("opens"); + assert_eq!(read.0, written.0); + assert!(read.0[1].signing.is_remote()); + assert!(!read.0[0].signing.is_remote()); + } + + /// An entry with neither a keystore nor a URL describes no way to sign, + /// and must be refused rather than read as one of the variants. + #[test] + fn an_entry_with_no_signing_method_is_refused() { + let dir = tempfile::tempdir().expect("temp dir"); + std::fs::write( + dir.path().join(DEFINITIONS_FILE), + "- enabled: true\n voting_public_key: '0xaa'\n", + ) + .expect("writes"); + assert!(ValidatorDefinitions::open(dir.path()).is_err()); + } + + /// Open a definitions file holding `yaml`, rendering a refusal as text. + fn open_text(yaml: &str) -> std::result::Result { + let dir = tempfile::tempdir().expect("temp dir"); + std::fs::write(dir.path().join(DEFINITIONS_FILE), yaml).expect("writes"); + ValidatorDefinitions::open(dir.path()).map_err(|err| err.to_string()) + } + + /// An entry naming both a keystore and a url must not load as either: + /// read as local, it would sign in this process and bypass the remote + /// signer's slashing protection without a word. + #[test] + fn an_untyped_entry_with_two_signing_methods_is_refused() { + let err = open_text( + "- enabled: true\n voting_public_key: '0xaa'\n \ + voting_keystore_path: keystore.json\n \ + voting_keystore_password_path: password.txt\n \ + url: https://signer:9000\n", + ) + .expect_err("ambiguous entry"); + assert!(err.contains("both a keystore and a url"), "{err}"); + } + + #[test] + fn a_typed_entry_must_carry_only_its_own_fields() { + let local_with_url = open_text( + "- enabled: true\n voting_public_key: '0xaa'\n type: local_keystore\n \ + voting_keystore_path: keystore.json\n \ + voting_keystore_password_path: password.txt\n \ + url: https://signer:9000\n", + ); + assert!(local_with_url.is_err()); + let remote_with_keystore = open_text( + "- enabled: true\n voting_public_key: '0xaa'\n type: web3signer\n \ + url: https://signer:9000\n voting_keystore_path: keystore.json\n", + ); + assert!(remote_with_keystore.is_err()); + let remote = open_text( + "- enabled: true\n voting_public_key: '0xaa'\n type: web3signer\n \ + url: https://signer:9000\n", + ) + .expect("a well-formed remote entry"); + assert_eq!( + remote.0, + vec![remote_definition("0xaa", "https://signer:9000")] + ); + } + + /// The refusal names what is missing, rather than serde's generic "did + /// not match any variant". + #[test] + fn a_local_entry_missing_its_password_path_names_the_field() { + let err = open_text( + "- enabled: true\n voting_public_key: '0xaa'\n \ + voting_keystore_path: keystore.json\n", + ) + .expect_err("incomplete entry"); + assert!(err.contains("voting_keystore_password_path"), "{err}"); + } + + /// A field this client does not implement is reported on a remote entry, + /// not ignored: an operator who set `root_certificate_path` expects it to + /// take effect. Every such field is named, whether the entry says its + /// `type` or is read as remote by its `url`. + #[test] + fn an_unimplemented_field_on_a_remote_entry_is_refused() { + let typed = open_text( + "- enabled: true\n voting_public_key: '0xaa'\n type: web3signer\n \ + url: https://signer:9000\n root_certificate_path: ca.pem\n \ + request_timeout_ms: 500\n", + ) + .expect_err("unimplemented fields"); + assert!( + typed.contains("request_timeout_ms, root_certificate_path"), + "{typed}" + ); + + let untyped = open_text( + "- enabled: true\n voting_public_key: '0xaa'\n \ + url: https://signer:9000\n suggested_fee_recipient: '0x00'\n", + ) + .expect_err("unimplemented field"); + assert!(untyped.contains("suggested_fee_recipient"), "{untyped}"); + } + + /// Lighthouse writes `description` on every entry, so a remote entry it + /// wrote, with no signer settings of its own, has to load. + #[test] + fn a_remote_entry_may_carry_a_description() { + let read = open_text( + "- enabled: true\n voting_public_key: '0xaa'\n description: ''\n \ + type: web3signer\n url: https://signer:9000\n", + ) + .expect("a remote entry as Lighthouse writes it"); + assert_eq!( + read.0, + vec![remote_definition("0xaa", "https://signer:9000")] + ); + } + + /// A local entry is read past the fields this client does not have, typed + /// or not, so a file Lighthouse wrote loads as it did before entries had + /// a `type`. + #[test] + fn a_field_this_client_does_not_read_is_skipped_on_a_local_entry() { + let read = open_text( + "- enabled: true\n voting_public_key: '0xaa'\n \ + graffiti: mine\n builder_proposals: true\n description: ''\n \ + type: local_keystore\n voting_keystore_path: keystore.json\n \ + voting_keystore_password_path: password.txt\n\ + - enabled: false\n voting_public_key: '0xbb'\n gas_limit: 30000000\n \ + voting_keystore_path: keystore.json\n \ + voting_keystore_password_path: password.txt\n", + ) + .expect("local entries with Lighthouse's own fields"); + assert_eq!( + read.0, + vec![definition("0xaa", true), definition("0xbb", false)] + ); + } + + /// The fields read past on the way in are not written back: an entry is + /// saved as the fields this client has, as it always was. + #[test] + fn a_skipped_field_is_not_written_back() { + let dir = tempfile::tempdir().expect("temp dir"); + std::fs::write( + dir.path().join(DEFINITIONS_FILE), + "- enabled: true\n voting_public_key: '0xaa'\n graffiti: mine\n \ + voting_keystore_path: keystore.json\n \ + voting_keystore_password_path: password.txt\n", + ) + .expect("writes"); + + let read = ValidatorDefinitions::open(dir.path()).expect("opens"); + read.save(dir.path()).expect("saves"); + let text = std::fs::read_to_string(dir.path().join(DEFINITIONS_FILE)).expect("reads"); + assert!(!text.contains("graffiti"), "{text}"); + assert_eq!( + ValidatorDefinitions::open(dir.path()).expect("opens").0, + read.0 + ); + } + #[test] fn a_missing_file_is_an_empty_set_not_an_error() { let dir = tempfile::tempdir().expect("temp dir"); diff --git a/crates/validator/src/keys/mod.rs b/crates/validator/src/keys/mod.rs index ac89a776a..b4947d41c 100644 --- a/crates/validator/src/keys/mod.rs +++ b/crates/validator/src/keys/mod.rs @@ -3,6 +3,8 @@ pub mod definitions; pub mod keystore; pub mod store; +pub mod web3signer; -pub use definitions::{ValidatorDefinition, ValidatorDefinitions}; +pub use definitions::{SigningDefinition, ValidatorDefinition, ValidatorDefinitions}; pub use store::{SigningMethod, ValidatorStore}; +pub use web3signer::Web3Signer; diff --git a/crates/validator/src/keys/store.rs b/crates/validator/src/keys/store.rs index 216855253..10f85f14d 100644 --- a/crates/validator/src/keys/store.rs +++ b/crates/validator/src/keys/store.rs @@ -2,6 +2,7 @@ use std::collections::HashMap; use std::path::Path; +use std::sync::{Arc, Weak}; use blst::min_pk::SecretKey; use ethlambda_types::beacon::primitives::{BLS_PUBKEY_SIZE, BlsPubkey}; @@ -9,13 +10,15 @@ use tracing::info; use crate::beacon_node::dto::{encode_hex, parse_pubkey}; use crate::error::{Error, Result}; -use crate::keys::definitions::ValidatorDefinitions; +use crate::keys::definitions::{SigningDefinition, ValidatorDefinitions}; use crate::keys::keystore::Keystore; +use crate::keys::web3signer::Web3Signer; /// How one validator produces a signature. /// -/// One variant today. It is an enum from the start so that adding a remote -/// signer later is a new arm rather than a reshaping of every call site. +/// Cheap to clone: both arms are an `Arc`, so a signing path can take a copy +/// out of the store under a short read guard and sign after releasing it, +/// which matters for the remote arm, whose signature is an HTTP round trip. /// /// **Never derive `Debug` on this type, or on anything holding it.** /// `blst::min_pk::SecretKey` zeroizes on drop but still derives a plain @@ -23,20 +26,39 @@ use crate::keys::keystore::Keystore; /// chain would put a validator's signing key into a log line. `Zeroizing` /// is no protection either: it forwards `Debug` to the type it wraps, so it /// controls the key's lifetime in memory, not whether it can be printed. +#[derive(Clone)] pub enum SigningMethod { /// The secret key is held in this process, decrypted from a local keystore. - LocalKeystore { secret_key: Box }, + LocalKeystore { secret_key: Arc }, + /// The key is held by a Web3Signer, which signs on this client's behalf. + Web3Signer { signer: Arc }, +} + +impl SigningMethod { + pub fn is_remote(&self) -> bool { + matches!(self, Self::Web3Signer { .. }) + } } /// Every validator this client signs for, keyed by public key. pub struct ValidatorStore { validators: HashMap, + /// One client per remote signer URL, shared by every validator on it, so + /// a hundred keys behind one signer are one connection pool rather than a + /// hundred. + /// + /// Weak: the validators using a client hold it, and so does any signature + /// in flight through it. A client stops being reachable here once the + /// last of those lets go, with no pruning pass having to guess whether an + /// in-flight request still counts as a user. + signers: HashMap>, } impl ValidatorStore { pub fn new() -> Self { Self { validators: HashMap::new(), + signers: HashMap::new(), } } @@ -45,14 +67,35 @@ impl ValidatorStore { /// A keystore that fails to decrypt aborts startup rather than being /// skipped: a validator that silently does not attest looks exactly like a /// healthy one from inside this process, and the operator finds out from - /// missed-attestation penalties days later. + /// missed-attestation penalties days later. A remote entry is registered + /// without contacting its signer: the signer may well start after this + /// process does, and a key it turns out not to hold fails at its first + /// signature, where it is logged and counted, rather than at startup. pub fn load(validators_dir: &Path) -> Result { let definitions = ValidatorDefinitions::open(validators_dir)?; let mut store = Self::new(); for definition in definitions.enabled() { - let keystore_path = resolve(validators_dir, &definition.voting_keystore_path); - let password_path = resolve(validators_dir, &definition.voting_keystore_password_path); + let (voting_keystore_path, voting_keystore_password_path) = match &definition.signing { + SigningDefinition::Web3Signer { url } => { + let pubkey = parse_pubkey(&definition.voting_public_key).map_err(|err| { + Error::Keystore { + path: url.clone(), + reason: format!( + "the definitions entry's voting_public_key is unreadable: {err}" + ), + } + })?; + store.insert_remote(pubkey, url)?; + continue; + } + SigningDefinition::LocalKeystore { + voting_keystore_path, + voting_keystore_password_path, + } => (voting_keystore_path, voting_keystore_password_path), + }; + let keystore_path = resolve(validators_dir, voting_keystore_path); + let password_path = resolve(validators_dir, voting_keystore_password_path); let json = std::fs::read_to_string(&keystore_path).map_err(|source| Error::Io { path: keystore_path.display().to_string(), @@ -125,7 +168,11 @@ impl ValidatorStore { } } - info!(count = store.len(), "Loaded validator keys"); + info!( + count = store.len(), + remote = store.remote_entries().len(), + "Loaded validator keys" + ); Ok(store) } @@ -142,12 +189,65 @@ impl ValidatorStore { self.validators.insert( pubkey, SigningMethod::LocalKeystore { - secret_key: Box::new(secret_key), + secret_key: Arc::new(secret_key), }, ); Ok(pubkey) } + /// Add a validator whose key a Web3Signer at `url` holds, replacing any + /// method the store had for it, building the signer's client if no other + /// validator already shares one. + /// + /// Nothing is asked of the signer here; see [`Self::load`] for why. The + /// URL is checked for shape, which is the one failure worth refusing up + /// front: an entry nobody could ever reach. + /// + /// For startup only. Building a client is not cheap, so a caller holding + /// the store behind a lock uses [`Self::signer_for_url`] and + /// [`Self::insert_remote_signer`] instead, and builds any missing client + /// before taking the write lock. + pub fn insert_remote(&mut self, pubkey: BlsPubkey, url: &str) -> Result<()> { + let signer = match self.signer_for_url(url) { + Some(signer) => signer, + None => Arc::new(Web3Signer::new(url)?), + }; + self.insert_remote_signer(pubkey, signer); + Ok(()) + } + + /// The client already serving `url`, if any validator still uses one. + /// Keyed by the URL as the client normalizes it, so `.../` and `...` name + /// one signer rather than two. + pub fn signer_for_url(&self, url: &str) -> Option> { + self.signers + .get(url.trim_end_matches('/')) + .and_then(Weak::upgrade) + } + + /// Every client some validator still uses, by URL. Lets a caller that + /// builds clients outside the store's lock reuse these instead. + pub fn remote_signers(&self) -> HashMap> { + self.signers + .iter() + .filter_map(|(url, signer)| Some((url.clone(), signer.upgrade()?))) + .collect() + } + + /// Add a validator signing through an already-built `signer`, replacing + /// any method the store had for it. A map insert and nothing more, so it + /// is cheap enough to run under the store's write lock. + pub fn insert_remote_signer(&mut self, pubkey: BlsPubkey, signer: Arc) { + // Entries whose client nobody holds any more are dropped here rather + // than on every mutation: they are a URL and a dead pointer, and a + // lookup already treats one as absent. + self.signers.retain(|_, signer| signer.strong_count() > 0); + self.signers + .insert(signer.url().to_string(), Arc::downgrade(&signer)); + self.validators + .insert(pubkey, SigningMethod::Web3Signer { signer }); + } + /// Derive a validator's public key from its raw secret, without adding it /// to the store. /// @@ -169,14 +269,46 @@ impl ValidatorStore { self.validators.contains_key(pubkey) } - pub fn get(&self, pubkey: &BlsPubkey) -> Option<&SigningMethod> { - self.validators.get(pubkey) + /// How `pubkey` signs, as a copy the caller can hold after releasing + /// whatever lock guards this store. `None` for a validator it does not + /// hold. + pub fn signer(&self, pubkey: &BlsPubkey) -> Option { + self.validators.get(pubkey).cloned() + } + + /// The remote signer's URL for `pubkey`, or `None` when the key is local + /// or unknown. + pub fn remote_url(&self, pubkey: &BlsPubkey) -> Option { + match self.validators.get(pubkey)? { + SigningMethod::Web3Signer { signer } => Some(signer.url().to_string()), + SigningMethod::LocalKeystore { .. } => None, + } } pub fn pubkeys(&self) -> Vec { self.validators.keys().copied().collect() } + /// The validators whose key is in this process. + pub fn local_pubkeys(&self) -> Vec { + self.validators + .iter() + .filter(|(_, method)| !method.is_remote()) + .map(|(pubkey, _)| *pubkey) + .collect() + } + + /// The validators a remote signer signs for, each with its signer's URL. + pub fn remote_entries(&self) -> Vec<(BlsPubkey, String)> { + self.validators + .iter() + .filter_map(|(pubkey, method)| match method { + SigningMethod::Web3Signer { signer } => Some((*pubkey, signer.url().to_string())), + SigningMethod::LocalKeystore { .. } => None, + }) + .collect() + } + pub fn len(&self) -> usize { self.validators.len() } @@ -220,7 +352,7 @@ mod tests { use std::path::PathBuf; use super::*; - use crate::keys::definitions::{ValidatorDefinition, ValidatorDefinitions}; + use crate::keys::definitions::{SigningDefinition, ValidatorDefinition, ValidatorDefinitions}; /// The secret from the EIP-2335 test vectors. fn test_secret() -> [u8; 32] { @@ -278,8 +410,10 @@ mod tests { let definitions = ValidatorDefinitions(vec![ValidatorDefinition { enabled: true, voting_public_key: "0x9612d7a727c9d0a22e185a1c768478dfe919cada9266988cb32359c11f2b7b27f4ae4040902382ae2910c15e2b420d07".to_string(), - voting_keystore_path: PathBuf::from("keystore.json"), - voting_keystore_password_path: PathBuf::from("password.txt"), + signing: SigningDefinition::LocalKeystore { + voting_keystore_path: PathBuf::from("keystore.json"), + voting_keystore_password_path: PathBuf::from("password.txt"), + }, }]); definitions.save(dir.path()).expect("saves definitions"); @@ -304,13 +438,118 @@ mod tests { ValidatorDefinitions(vec![ValidatorDefinition { enabled: true, voting_public_key: voting_public_key.to_string(), - voting_keystore_path: PathBuf::from("keystore.json"), - voting_keystore_password_path: PathBuf::from("password.txt"), + signing: SigningDefinition::LocalKeystore { + voting_keystore_path: PathBuf::from("keystore.json"), + voting_keystore_password_path: PathBuf::from("password.txt"), + }, }]) .save(dir) .expect("saves definitions"); } + #[test] + fn a_remote_definition_loads_without_contacting_its_signer() { + let dir = tempfile::tempdir().expect("temp dir"); + // A URL nothing listens on: loading must not care. + let url = "http://127.0.0.1:9"; + ValidatorDefinitions(vec![ValidatorDefinition { + enabled: true, + voting_public_key: VECTOR_PUBKEY.to_string(), + signing: SigningDefinition::Web3Signer { + url: url.to_string(), + }, + }]) + .save(dir.path()) + .expect("saves definitions"); + + let store = ValidatorStore::load(dir.path()).expect("loads"); + let pubkey = parse_pubkey(VECTOR_PUBKEY).expect("pubkey"); + assert_eq!(store.len(), 1); + assert_eq!(store.remote_url(&pubkey).as_deref(), Some(url)); + assert!(store.signer(&pubkey).expect("held").is_remote()); + assert!(store.local_pubkeys().is_empty()); + assert_eq!(store.remote_entries(), vec![(pubkey, url.to_string())]); + } + + #[test] + fn a_remote_definition_with_a_bad_url_is_refused() { + let dir = tempfile::tempdir().expect("temp dir"); + ValidatorDefinitions(vec![ValidatorDefinition { + enabled: true, + voting_public_key: VECTOR_PUBKEY.to_string(), + signing: SigningDefinition::Web3Signer { + url: "not a url".to_string(), + }, + }]) + .save(dir.path()) + .expect("saves definitions"); + + assert!(ValidatorStore::load(dir.path()).is_err()); + } + + /// How many validators' worth of clients the store can still hand out. + fn live_signers(store: &ValidatorStore) -> usize { + store + .signers + .values() + .filter(|signer| signer.strong_count() > 0) + .count() + } + + #[test] + fn remote_validators_on_one_url_share_a_client_and_release_it_together() { + let mut store = ValidatorStore::new(); + let first = BlsPubkey([1; BLS_PUBKEY_SIZE]); + let second = BlsPubkey([2; BLS_PUBKEY_SIZE]); + store + .insert_remote(first, "http://signer:9000") + .expect("inserts"); + store + .insert_remote(second, "http://signer:9000/") + .expect("inserts"); + assert_eq!(live_signers(&store), 1, "one URL, one client"); + + assert!(store.remove(&first)); + assert_eq!(live_signers(&store), 1, "still in use by the second key"); + assert!(store.remove(&second)); + assert_eq!(live_signers(&store), 0, "nobody uses it any more"); + assert!(store.signer_for_url("http://signer:9000").is_none()); + } + + /// A signature in flight holds the client, and must not keep it alive in + /// the store after it finishes: removal and the request's end can come in + /// either order. + #[test] + fn a_client_held_by_an_in_flight_signature_is_released_when_it_ends() { + let mut store = ValidatorStore::new(); + let pubkey = BlsPubkey([1; BLS_PUBKEY_SIZE]); + store + .insert_remote(pubkey, "http://signer:9000") + .expect("inserts"); + let in_flight = store.signer(&pubkey).expect("held"); + + assert!(store.remove(&pubkey)); + assert_eq!(live_signers(&store), 1, "the request still holds it"); + drop(in_flight); + assert_eq!(live_signers(&store), 0, "released with the request"); + } + + #[test] + fn inserting_a_local_key_over_a_remote_one_replaces_it() { + let mut store = ValidatorStore::new(); + let pubkey = parse_pubkey(VECTOR_PUBKEY).expect("pubkey"); + store + .insert_remote(pubkey, "http://signer:9000") + .expect("inserts"); + let derived = store + .insert_secret("test", &test_secret()) + .expect("inserts"); + assert_eq!(derived, pubkey); + assert_eq!(store.len(), 1); + assert!(store.remote_url(&pubkey).is_none()); + assert_eq!(live_signers(&store), 0); + } + /// A definitions entry declaring a key its keystore does not hold must not /// load. /// @@ -388,6 +627,7 @@ mod tests { #[test] fn an_unknown_pubkey_has_no_signing_method() { let store = ValidatorStore::new(); - assert!(store.get(&BlsPubkey([7; BLS_PUBKEY_SIZE])).is_none()); + assert!(store.signer(&BlsPubkey([7; BLS_PUBKEY_SIZE])).is_none()); + assert!(store.remote_url(&BlsPubkey([7; BLS_PUBKEY_SIZE])).is_none()); } } diff --git a/crates/validator/src/keys/web3signer.rs b/crates/validator/src/keys/web3signer.rs new file mode 100644 index 000000000..9f037a9bf --- /dev/null +++ b/crates/validator/src/keys/web3signer.rs @@ -0,0 +1,647 @@ +//! Signing through a Web3Signer remote signer. +//! +//! The key never enters this process: each signature is a `POST +//! /api/v1/eth2/sign/{pubkey}` carrying the message, the fork it is signed +//! under and the signing root, and the signer answers with the signature. The +//! message travels in full rather than as its root alone because the signer +//! keeps a slashing-protection record of its own, beside this client's (see +//! [`crate::slashing_protection`]), and it can only refuse a double vote it +//! can read. The root is sent as well so the signer can check the two agree. +//! +//! Request shapes follow Web3Signer's own OpenAPI +//! (`core/src/main/resources/openapi-specs/eth2/signing/schemas.yaml`): +//! quoted integers, `0x`-prefixed hex, and a fork-named `version` on the +//! containers that changed shape across forks. + +use std::time::Duration; + +use ethlambda_types::beacon::containers::altair::{ + ContributionAndProof, SyncAggregatorSelectionData, +}; +use ethlambda_types::beacon::containers::gloas::{ + ExecutionPayloadEnvelope, PayloadAttestationData, +}; +use ethlambda_types::beacon::containers::shared::{AttestationData, BeaconBlockHeader, Fork}; +use ethlambda_types::beacon::containers::{electra, gloas}; +use ethlambda_types::beacon::primitives::{BLS_SIGNATURE_SIZE, BlsPubkey, BlsSignature, Root}; +use serde::Serialize; + +use crate::beacon_node::dto::{encode_hex, parse_signature}; +use crate::error::{Error, Result}; + +/// How long one signing request may take. The same bound the beacon node +/// client uses; every duty is cut off by its own slot deadline anyway, and a +/// signer slower than this is one whose signatures arrive too late to count. +const REQUEST_TIMEOUT: Duration = Duration::from_secs(8); + +/// Web3Signer's answer when its slashing protection refused the message. +const SLASHING_PROTECTION_REFUSED: u16 = 412; + +/// One remote signer, shared by every validator whose key it holds. +pub struct Web3Signer { + base_url: String, + client: reqwest::Client, +} + +impl Web3Signer { + /// Build a client for the signer at `url`, which is the server's base + /// (`https://signer:9000`), not the signing path. + /// + /// Building one sets up a TLS configuration and a connection pool, so it + /// is not free: callers that hold a lock build it before taking the lock + /// (see [`crate::keys::ValidatorStore::insert_remote_signer`]). + pub fn new(url: &str) -> Result { + let base_url = url.trim_end_matches('/').to_string(); + // Every error below names the URL as `redact` renders it, never as + // given: these errors are logged, at startup and on import, and a URL + // refused for its shape may still carry a `user:password@` that the + // credentials check further down never got to reject. + let invalid = |reason: &str| Error::Keystore { + path: redact(&base_url), + reason: reason.to_string(), + }; + let parsed = reqwest::Url::parse(&base_url) + .map_err(|err| invalid(&format!("not a valid remote signer URL: {err}")))?; + if parsed.scheme() != "http" && parsed.scheme() != "https" { + return Err(invalid("a remote signer URL must be http or https")); + } + // The URL is named in every error this client raises about the + // signer, and those are logged. Credentials embedded in it would be + // logged with them, so they are refused rather than redacted: the + // signer's own authentication is TLS, not basic auth. + if !parsed.username().is_empty() || parsed.password().is_some() { + return Err(invalid("a remote signer URL must not carry credentials")); + } + // The signing path is appended to the base, so a query or fragment + // would swallow it (`https://signer?token=abc` would request `/`), and + // a token in the query would be logged with every signing error. + if parsed.query().is_some() || parsed.fragment().is_some() { + return Err(invalid( + "a remote signer URL must not carry a query string or fragment", + )); + } + let client = reqwest::Client::builder() + .timeout(REQUEST_TIMEOUT) + .build() + .map_err(|err| invalid(&format!("could not build an HTTP client: {err}")))?; + Ok(Self { base_url, client }) + } + + /// The signer's base URL, with any trailing slash already stripped. + pub fn url(&self) -> &str { + &self.base_url + } + + /// Ask the signer for `pubkey`'s signature over `request`. + /// + /// Every failure is an [`Error::Signing`]: a refusal by the signer's + /// slashing protection, a key the signer does not hold, and a transport + /// failure all cost this one signature and nothing else, and none of them + /// is something retrying within the slot would change. + pub async fn sign( + &self, + pubkey: &BlsPubkey, + request: &SigningRequest<'_>, + ) -> Result { + let pubkey_hex = encode_hex(&pubkey.0); + let url = format!("{}/api/v1/eth2/sign/{pubkey_hex}", self.base_url); + let failed = |reason: String| Error::Signing { + pubkey: *pubkey, + reason, + }; + + let response = self + .client + .post(&url) + .header(reqwest::header::ACCEPT, "application/json") + .json(request) + .send() + .await + .map_err(|err| failed(format!("remote signer {}: {err}", self.base_url)))?; + + let status = response.status(); + let body = response + .text() + .await + .map_err(|err| failed(format!("remote signer {}: {err}", self.base_url)))?; + + if status.as_u16() == SLASHING_PROTECTION_REFUSED { + return Err(failed(format!( + "remote signer {} refused to sign: its slashing protection rejected the message", + self.base_url + ))); + } + if status == reqwest::StatusCode::NOT_FOUND { + return Err(failed(format!( + "remote signer {} does not hold this key", + self.base_url + ))); + } + if !status.is_success() { + return Err(failed(format!( + "remote signer {} answered {status}: {}", + self.base_url, + truncate(body.trim()) + ))); + } + + parse_signature_body(&body) + .map_err(|err| failed(format!("remote signer {}: {err}", self.base_url))) + } +} + +/// The most of a failed response's body that goes into an error, and so into +/// a log line: enough to say why, not a whole error page. +const MAX_LOGGED_BODY: usize = 200; + +fn truncate(body: &str) -> String { + match body.char_indices().nth(MAX_LOGGED_BODY) { + Some((cut, _)) => format!("{}...", &body[..cut]), + None => body.to_string(), + } +} + +/// `url` with anything that could be a credential cut out, for an error +/// message: the userinfo before an `@` in the authority, and any query string +/// or fragment. Works on text rather than a parsed URL, since it is what +/// reports a URL that does not parse. +fn redact(url: &str) -> String { + let (scheme, rest) = match url.split_once("://") { + Some((scheme, rest)) => (Some(scheme), rest), + None => (None, url), + }; + let rest = rest.split(['?', '#']).next().unwrap_or_default(); + let (authority, path) = match rest.find('/') { + Some(cut) => rest.split_at(cut), + None => (rest, ""), + }; + let host = match authority.rsplit_once('@') { + Some((_, host)) => host, + None => authority, + }; + match scheme { + Some(scheme) => format!("{scheme}://{host}{path}"), + None => format!("{host}{path}"), + } +} + +/// The signature out of a `200` body: `{"signature": "0x..."}` when the +/// signer honoured `Accept: application/json`, or the bare hex string an +/// older signer answers with regardless. +fn parse_signature_body(body: &str) -> Result { + #[derive(serde::Deserialize)] + struct SignatureResponse { + signature: String, + } + let hex = match serde_json::from_str::(body) { + Ok(response) => response.signature, + Err(_) => body.trim().trim_matches('"').to_string(), + }; + let signature = parse_signature(&hex).map_err(|err| { + Error::Decode(format!( + "the signature in the response is unreadable: {err}" + )) + })?; + debug_assert_eq!(signature.0.len(), BLS_SIGNATURE_SIZE); + Ok(signature) +} + +/// The fork a message is signed under, as the signer wants it told. +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +pub struct ForkInfo { + pub fork: Fork, + pub genesis_validators_root: Root, +} + +/// One signing request: what kind of message, under which fork, over which +/// root, and the message itself. +#[derive(Serialize)] +pub struct SigningRequest<'a> { + #[serde(rename = "type")] + kind: &'static str, + pub fork_info: ForkInfo, + #[serde(rename = "signingRoot")] + pub signing_root: Root, + #[serde(flatten)] + pub body: SigningBody<'a>, +} + +impl<'a> SigningRequest<'a> { + pub fn new(fork_info: ForkInfo, signing_root: Root, body: SigningBody<'a>) -> Self { + Self { + kind: body.kind(), + fork_info, + signing_root, + body, + } + } +} + +/// The message half of a request, one field per Web3Signer request type. +/// +/// Untagged on purpose: the `type` discriminator is a sibling of these fields +/// in the request, not a tag around them, so the variant contributes its one +/// field and [`SigningRequest`] writes the `type` beside it. +#[derive(Serialize)] +#[serde(untagged)] +pub enum SigningBody<'a> { + Attestation { + attestation: &'a AttestationData, + }, + RandaoReveal { + randao_reveal: RandaoReveal, + }, + Block { + beacon_block: VersionedHeader, + }, + AggregationSlot { + aggregation_slot: AggregationSlot, + }, + AggregateAndProof { + aggregate_and_proof: Versioned>, + }, + SyncCommitteeMessage { + sync_committee_message: SyncCommitteeMessage, + }, + SyncCommitteeSelectionProof { + sync_aggregator_selection_data: SyncAggregatorSelectionData, + }, + SyncCommitteeContributionAndProof { + contribution_and_proof: &'a ContributionAndProof, + }, + ExecutionPayloadEnvelope { + execution_payload_envelope: Versioned<&'a ExecutionPayloadEnvelope>, + }, + PayloadAttestationMessage { + payload_attestation_message: Versioned<&'a PayloadAttestationData>, + }, +} + +impl SigningBody<'_> { + /// Web3Signer's `type` for this message. + fn kind(&self) -> &'static str { + match self { + Self::Attestation { .. } => "ATTESTATION", + Self::RandaoReveal { .. } => "RANDAO_REVEAL", + Self::Block { .. } => "BLOCK_V2", + Self::AggregationSlot { .. } => "AGGREGATION_SLOT", + Self::AggregateAndProof { .. } => "AGGREGATE_AND_PROOF_V2", + Self::SyncCommitteeMessage { .. } => "SYNC_COMMITTEE_MESSAGE", + Self::SyncCommitteeSelectionProof { .. } => "SYNC_COMMITTEE_SELECTION_PROOF", + Self::SyncCommitteeContributionAndProof { .. } => { + "SYNC_COMMITTEE_CONTRIBUTION_AND_PROOF" + } + Self::ExecutionPayloadEnvelope { .. } => "EXECUTION_PAYLOAD_ENVELOPE", + Self::PayloadAttestationMessage { .. } => "PAYLOAD_ATTESTATION_MESSAGE", + } + } +} + +#[derive(Serialize)] +pub struct RandaoReveal { + #[serde(with = "ethlambda_types::beacon::serde_helpers::quoted_or_bare")] + pub epoch: u64, +} + +#[derive(Serialize)] +pub struct AggregationSlot { + #[serde(with = "ethlambda_types::beacon::serde_helpers::quoted_or_bare")] + pub slot: u64, +} + +#[derive(Serialize)] +pub struct SyncCommitteeMessage { + pub beacon_block_root: Root, + #[serde(with = "ethlambda_types::beacon::serde_helpers::quoted_or_bare")] + pub slot: u64, +} + +/// A block, as the signer wants it: the fork's name and the header alone. +/// The body never travels; its root in the header is what the signature +/// covers. +#[derive(Serialize)] +pub struct VersionedHeader { + pub version: String, + pub block_header: BeaconBlockHeader, +} + +/// A container whose JSON shape the signer selects by fork name. +#[derive(Serialize)] +pub struct Versioned { + pub version: String, + pub data: T, +} + +/// An `AggregateAndProof` of either fork this client publishes, by reference. +#[derive(Clone, Copy, Serialize)] +#[serde(untagged)] +pub enum AggregateAndProofRef<'a> { + Electra(&'a electra::AggregateAndProof), + Gloas(&'a gloas::AggregateAndProof), +} + +impl AggregateAndProofRef<'_> { + /// The slot of the votes being republished, which is the slot the + /// wrapper's signature is domained on. + pub fn slot(&self) -> u64 { + match self { + Self::Electra(message) => message.aggregate.data.slot, + Self::Gloas(message) => message.aggregate.data.slot, + } + } + + pub fn hash_tree_root(&self) -> Root { + use ethlambda_types::beacon::primitives::HashTreeRoot as _; + match self { + Self::Electra(message) => message.hash_tree_root(), + Self::Gloas(message) => message.hash_tree_root(), + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use ethlambda_types::beacon::containers::shared::Checkpoint; + use serde_json::json; + + fn fork_info() -> ForkInfo { + ForkInfo { + fork: Fork { + previous_version: [0, 0, 0, 0], + current_version: [1, 0, 0, 0], + epoch: 74_240, + }, + genesis_validators_root: Root::repeat_byte(0xaa), + } + } + + fn attestation_data() -> AttestationData { + AttestationData { + slot: 3200, + index: 2, + beacon_block_root: Root::repeat_byte(1), + source: Checkpoint { + epoch: 99, + root: Root::repeat_byte(2), + }, + target: Checkpoint { + epoch: 100, + root: Root::repeat_byte(3), + }, + } + } + + fn encode(request: &SigningRequest<'_>) -> serde_json::Value { + serde_json::to_value(request).expect("serializes") + } + + /// The exact wire shape, pinned against Web3Signer's `AttestationSigning` + /// schema: `type` beside `fork_info`, `signingRoot` camel-cased, every + /// integer quoted, every hash `0x`-prefixed. + #[test] + fn an_attestation_request_has_web3signers_shape() { + let data = attestation_data(); + let request = SigningRequest::new( + fork_info(), + Root::repeat_byte(9), + SigningBody::Attestation { attestation: &data }, + ); + assert_eq!( + encode(&request), + json!({ + "type": "ATTESTATION", + "fork_info": { + "fork": { + "previous_version": "0x00000000", + "current_version": "0x01000000", + "epoch": "74240" + }, + "genesis_validators_root": format!("0x{}", "aa".repeat(32)) + }, + "signingRoot": format!("0x{}", "09".repeat(32)), + "attestation": { + "slot": "3200", + "index": "2", + "beacon_block_root": format!("0x{}", "01".repeat(32)), + "source": { "epoch": "99", "root": format!("0x{}", "02".repeat(32)) }, + "target": { "epoch": "100", "root": format!("0x{}", "03".repeat(32)) } + } + }) + ); + } + + #[test] + fn every_body_names_its_type_and_its_one_field() { + let data = attestation_data(); + let header = BeaconBlockHeader { + slot: 3200, + proposer_index: 7, + parent_root: Root::repeat_byte(1), + state_root: Root::repeat_byte(2), + body_root: Root::repeat_byte(3), + }; + let cases: Vec<(SigningBody<'_>, &str, &str)> = vec![ + ( + SigningBody::Attestation { attestation: &data }, + "ATTESTATION", + "attestation", + ), + ( + SigningBody::RandaoReveal { + randao_reveal: RandaoReveal { epoch: 100 }, + }, + "RANDAO_REVEAL", + "randao_reveal", + ), + ( + SigningBody::Block { + beacon_block: VersionedHeader { + version: "ELECTRA".to_string(), + block_header: header, + }, + }, + "BLOCK_V2", + "beacon_block", + ), + ( + SigningBody::AggregationSlot { + aggregation_slot: AggregationSlot { slot: 3200 }, + }, + "AGGREGATION_SLOT", + "aggregation_slot", + ), + ( + SigningBody::SyncCommitteeMessage { + sync_committee_message: SyncCommitteeMessage { + beacon_block_root: Root::ZERO, + slot: 3200, + }, + }, + "SYNC_COMMITTEE_MESSAGE", + "sync_committee_message", + ), + ( + SigningBody::SyncCommitteeSelectionProof { + sync_aggregator_selection_data: SyncAggregatorSelectionData { + slot: 3200, + subcommittee_index: 1, + }, + }, + "SYNC_COMMITTEE_SELECTION_PROOF", + "sync_aggregator_selection_data", + ), + ]; + for (body, kind, field) in cases { + let encoded = encode(&SigningRequest::new(fork_info(), Root::ZERO, body)); + assert_eq!(encoded["type"], kind); + assert!( + encoded.get(field).is_some(), + "{kind} must carry its message under `{field}`: {encoded}" + ); + assert_eq!( + encoded.as_object().expect("object").len(), + 4, + "{kind}: type, fork_info, signingRoot and the message, nothing else" + ); + } + } + + #[test] + fn a_block_request_carries_the_fork_name_and_the_header_only() { + let header = BeaconBlockHeader { + slot: 3200, + proposer_index: 7, + parent_root: Root::repeat_byte(1), + state_root: Root::repeat_byte(2), + body_root: Root::repeat_byte(3), + }; + let request = SigningRequest::new( + fork_info(), + Root::ZERO, + SigningBody::Block { + beacon_block: VersionedHeader { + version: "FULU".to_string(), + block_header: header, + }, + }, + ); + assert_eq!( + encode(&request)["beacon_block"], + json!({ + "version": "FULU", + "block_header": { + "slot": "3200", + "proposer_index": "7", + "parent_root": format!("0x{}", "01".repeat(32)), + "state_root": format!("0x{}", "02".repeat(32)), + "body_root": format!("0x{}", "03".repeat(32)) + } + }) + ); + } + + #[test] + fn a_payload_attestation_request_is_versioned_gloas() { + let data = PayloadAttestationData { + beacon_block_root: Root::repeat_byte(5), + slot: 3200, + payload_present: true, + blob_data_available: false, + }; + let request = SigningRequest::new( + fork_info(), + Root::ZERO, + SigningBody::PayloadAttestationMessage { + payload_attestation_message: Versioned { + version: "GLOAS".to_string(), + data: &data, + }, + }, + ); + assert_eq!( + encode(&request)["payload_attestation_message"], + json!({ + "version": "GLOAS", + "data": { + "beacon_block_root": format!("0x{}", "05".repeat(32)), + "slot": "3200", + "payload_present": true, + "blob_data_available": false + } + }) + ); + } + + #[test] + fn a_failed_response_body_is_cut_short_before_it_is_reported() { + let long = "x".repeat(1_000); + assert_eq!(truncate(&long).len(), MAX_LOGGED_BODY + 3); + assert_eq!(truncate("short"), "short"); + } + + #[test] + fn the_signature_is_read_from_json_or_from_a_bare_string() { + let hex = format!("0x{}", "ab".repeat(BLS_SIGNATURE_SIZE)); + let from_json = + parse_signature_body(&format!("{{\"signature\":\"{hex}\"}}")).expect("json"); + let from_text = parse_signature_body(&format!("{hex}\n")).expect("text"); + assert_eq!(from_json, from_text); + assert_eq!(from_json.0, [0xab; BLS_SIGNATURE_SIZE]); + assert!(parse_signature_body("0x1234").is_err()); + } + + #[test] + fn the_url_must_be_http_or_https() { + assert!(Web3Signer::new("https://signer:9000/").is_ok()); + assert_eq!( + Web3Signer::new("https://signer:9000/") + .expect("valid") + .url(), + "https://signer:9000" + ); + assert!(Web3Signer::new("ftp://signer:9000").is_err()); + let Err(err) = Web3Signer::new("https://user:secret@signer:9000") else { + panic!("a URL carrying credentials must be refused"); + }; + assert!(!err.to_string().contains("secret"), "{err}"); + assert!(Web3Signer::new("signer:9000").is_err()); + assert!(Web3Signer::new("not a url").is_err()); + } + + /// A URL refused for its shape must not put its credentials into the + /// error either: the scheme and parse checks run before the credentials + /// one, and every one of these errors is logged. + #[test] + fn no_refusal_echoes_the_urls_credentials() { + for url in [ + "ftp://admin:s3cret@signer:9000", + "https://admin:s3cret@signer:notaport", + "https://admin:s3cret@signer:9000", + "https://signer:9000?token=s3cret", + "https://signer:9000/#s3cret", + ] { + let Err(err) = Web3Signer::new(url) else { + panic!("{url} must be refused"); + }; + assert!(!err.to_string().contains("s3cret"), "{url}: {err}"); + } + } + + #[test] + fn a_url_with_a_query_or_fragment_is_refused() { + assert!(Web3Signer::new("https://signer:9000?token=abc").is_err()); + assert!(Web3Signer::new("https://signer:9000/#frag").is_err()); + assert!(Web3Signer::new("https://signer:9000/prefix").is_ok()); + } + + #[test] + fn redaction_keeps_the_host_and_path_only() { + assert_eq!( + redact("https://user:pw@signer:9000/base?x=1#y"), + "https://signer:9000/base" + ); + assert_eq!(redact("signer:9000"), "signer:9000"); + assert_eq!(redact("a@b"), "b"); + } +} diff --git a/crates/validator/src/lib.rs b/crates/validator/src/lib.rs index 24fc99dfd..e75f9e559 100644 --- a/crates/validator/src/lib.rs +++ b/crates/validator/src/lib.rs @@ -89,6 +89,10 @@ pub struct ValidatorConfig { /// Bind address for the keymanager API. `None` when `--enable-keymanager` /// was not passed, in which case it is never spawned. pub keymanager: Option, + /// `--web3signer-url`: the remote signer a key imported through + /// `POST /eth/v1/remotekeys` without a `url` of its own is assigned to. + /// Keys the definitions file already names a signer for need no default. + pub web3signer_url: Option, /// Register keys that have no slashing-protection record as never having /// signed, instead of refusing to start. See /// [`slashing_protection::SlashingProtection::open`]. @@ -133,6 +137,12 @@ pub async fn run(config: ValidatorConfig) -> Result<()> { directory" ); } + // Checked for shape at startup rather than at the first import that + // leans on it, which could be days later. + if let Some(url) = &config.web3signer_url { + keys::Web3Signer::new(url)?; + } + let store = ValidatorStore::load(&config.validators_dir)?; if store.is_empty() { warn!("No validators loaded; the client will idle"); @@ -182,7 +192,9 @@ pub async fn run(config: ValidatorConfig) -> Result<()> { // After the beacon node names the chain, which the record must belong to, // and before any service that signs exists. Every loaded key must already - // have a record, unless the operator said otherwise. + // have a record, unless the operator said otherwise. That includes a key + // a remote signer holds: the signer keeps its own record, and this one is + // kept as well, so a key is covered whichever of the two it meets first. let keys = store.read().await.pubkeys(); let protection = slashing_protection::SlashingProtection::open_locked( record_lock, @@ -198,10 +210,7 @@ pub async fn run(config: ValidatorConfig) -> Result<()> { ); let protection = Arc::new(std::sync::Mutex::new(protection)); - let context = Arc::new(SigningContext { - config: spec, - genesis_validators_root: genesis.genesis_validators_root, - }); + let context = Arc::new(SigningContext::new(spec, genesis.genesis_validators_root)); let mut duties = DutiesService::new(beacon_node.clone(), Vec::new()); let attestation = @@ -224,6 +233,7 @@ pub async fn run(config: ValidatorConfig) -> Result<()> { validators_dir: config.validators_dir.clone(), secrets_dir: config.secrets_dir.clone(), definitions_lock: Arc::new(Mutex::new(())), + default_remote_signer: config.web3signer_url.clone(), definitions_unflushed: Arc::new(std::sync::atomic::AtomicBool::new(false)), protection: protection.clone(), }; @@ -879,10 +889,10 @@ mod tests { } fn context() -> SigningContext { - SigningContext { - config: ethlambda_types::beacon::config::Config::mainnet(), - genesis_validators_root: Root::ZERO, - } + SigningContext::new( + ethlambda_types::beacon::config::Config::mainnet(), + Root::ZERO, + ) } fn empty_store() -> RwLock { @@ -1076,11 +1086,11 @@ mod tests { // ---- gloas ---------------------------------------------------------- fn gloas_context(epoch: u64) -> SigningContext { - SigningContext { - config: ethlambda_types::beacon::config::Config::mainnet() + SigningContext::new( + ethlambda_types::beacon::config::Config::mainnet() .with_fork_epoch(ethlambda_types::beacon::fork::ForkName::Gloas, epoch), - genesis_validators_root: Root::ZERO, - } + Root::ZERO, + ) } fn ptc_duty(index: u64, slot: u64) -> crate::beacon_node::dto::PtcDutyDto { @@ -1176,10 +1186,7 @@ mod tests { let mut config = ethlambda_types::beacon::config::Config::mainnet() .with_fork_epoch(ethlambda_types::beacon::fork::ForkName::Gloas, 0); config.slot_duration_ms = 1_000; - let context = Arc::new(SigningContext { - config: config.clone(), - genesis_validators_root: Root::ZERO, - }); + let context = Arc::new(SigningContext::new(config.clone(), Root::ZERO)); let now_secs = SystemTime::now() .duration_since(std::time::UNIX_EPOCH) .expect("after the epoch") @@ -1233,11 +1240,11 @@ mod tests { } fn altair_context() -> SigningContext { - SigningContext { - config: ethlambda_types::beacon::config::Config::mainnet() + SigningContext::new( + ethlambda_types::beacon::config::Config::mainnet() .with_fork_epoch(ethlambda_types::beacon::fork::ForkName::Altair, 0), - genesis_validators_root: Root::ZERO, - } + Root::ZERO, + ) } fn sync_duty(index: u64, seats: Vec) -> crate::beacon_node::dto::SyncDutyDto { @@ -1325,10 +1332,7 @@ mod tests { let mut config = ethlambda_types::beacon::config::Config::mainnet() .with_fork_epoch(ethlambda_types::beacon::fork::ForkName::Altair, 0); config.slot_duration_ms = 1_000; - let context = Arc::new(SigningContext { - config: config.clone(), - genesis_validators_root: Root::ZERO, - }); + let context = Arc::new(SigningContext::new(config.clone(), Root::ZERO)); let now_secs = SystemTime::now() .duration_since(std::time::UNIX_EPOCH) .expect("after the epoch") @@ -1384,10 +1388,7 @@ mod tests { use crate::beacon_node::mock::MockBeaconNode; let config = ethlambda_types::beacon::config::Config::mainnet(); - let context = Arc::new(SigningContext { - config: config.clone(), - genesis_validators_root: Root::ZERO, - }); + let context = Arc::new(SigningContext::new(config.clone(), Root::ZERO)); let clock = SlotClock::from_config(0, &config); let node = Arc::new(MockBeaconNode::new()); let store = empty_store(); diff --git a/crates/validator/src/payload_attestation.rs b/crates/validator/src/payload_attestation.rs index 0870c038a..fbfa39d67 100644 --- a/crates/validator/src/payload_attestation.rs +++ b/crates/validator/src/payload_attestation.rs @@ -43,7 +43,7 @@ use crate::beacon_node::BeaconNodeApi; use crate::beacon_node::dto::{PtcDutyDto, parse_pubkey}; use crate::error::Result; use crate::keys::ValidatorStore; -use crate::signing::SigningContext; +use crate::signing::{SigningContext, sign_all}; /// How many slots of dedup history are kept. Far more than the one slot a duty /// is ever owed for; the margin only absorbs a clock that steps back a little. @@ -117,38 +117,43 @@ impl PayloadAttestationService { ))); } - let mut messages = Vec::with_capacity(pending.len()); - { - let _timing = crate::metrics::time_signing(); - let store = store.read().await; - for duty in pending { - let pubkey = match parse_pubkey(&duty.pubkey) { - Ok(pubkey) => pubkey, - Err(err) => { - error!(%slot, validator = duty.validator_index, %err, "Duty carried an unreadable pubkey"); - continue; - } - }; - - // Recorded before signing, so a signature that exists is one - // the record already knows about. - if !self.record(duty.validator_index, slot) { + // Recorded one by one before anything is signed, so a signature that + // exists is one the record already knows about; then signed all at + // once, each within the signing budget. + let mut to_sign = Vec::with_capacity(pending.len()); + for duty in pending { + let pubkey = match parse_pubkey(&duty.pubkey) { + Ok(pubkey) => pubkey, + Err(err) => { + error!(%slot, validator = duty.validator_index, %err, "Duty carried an unreadable pubkey"); continue; } - - match self - .context - .sign_payload_attestation(&store, &pubkey, &data) - { - Ok(signature) => messages.push(PayloadAttestationMessage { - validator_index: duty.validator_index, - data, - signature, - }), - Err(err) => { - error!(%slot, validator = duty.validator_index, %err, "Failed to sign payload attestation"); - crate::metrics::inc_signing_failures(); - } + }; + if self.record(duty.validator_index, slot) { + to_sign.push((duty.validator_index, pubkey)); + } + } + let signatures = { + let _timing = crate::metrics::time_signing(); + sign_all(to_sign.iter().map(|(_, pubkey)| { + ( + *pubkey, + self.context.sign_payload_attestation(store, pubkey, &data), + ) + })) + .await + }; + let mut messages = Vec::with_capacity(to_sign.len()); + for ((validator_index, _), signature) in to_sign.into_iter().zip(signatures) { + match signature { + Ok(signature) => messages.push(PayloadAttestationMessage { + validator_index, + data, + signature, + }), + Err(err) => { + error!(%slot, validator = validator_index, %err, "Failed to sign payload attestation"); + crate::metrics::inc_signing_failures(); } } } @@ -225,10 +230,10 @@ mod tests { } fn context() -> Arc { - Arc::new(SigningContext { - config: Config::mainnet().with_fork_epoch(ForkName::Gloas, GLOAS_EPOCH), - genesis_validators_root: Root::ZERO, - }) + Arc::new(SigningContext::new( + Config::mainnet().with_fork_epoch(ForkName::Gloas, GLOAS_EPOCH), + Root::ZERO, + )) } fn store() -> (RwLock, BlsPubkey) { diff --git a/crates/validator/src/proposal.rs b/crates/validator/src/proposal.rs index c16319f07..8b84d5dcc 100644 --- a/crates/validator/src/proposal.rs +++ b/crates/validator/src/proposal.rs @@ -55,7 +55,9 @@ use std::sync::Arc; use ethlambda_types::beacon::constants::BUILDER_INDEX_SELF_BUILD; use ethlambda_types::beacon::fork::ForkName; -use ethlambda_types::beacon::primitives::{BlsPubkey, ExecutionAddress, Root, Slot}; +use ethlambda_types::beacon::primitives::{ + BlsPubkey, ExecutionAddress, HashTreeRoot as _, Root, Slot, +}; use ethlambda_types::beacon::signing::compute_epoch_at_slot; use tokio::sync::RwLock; use tracing::{error, info, warn}; @@ -172,15 +174,7 @@ impl ProposalService { .await; refuse_unless_safe(slot, duty.validator_index, "consult", checked)?; - // Scoped away from every await, the way the attestation path's read - // guard is, and for the reason spelled out there: a write-preferring - // lock lets one queued keymanager writer block every reader behind it, - // so a guard held across the block production round trip below would - // put the whole duty path behind it. - let randao_reveal = { - let store = store.read().await; - self.context.sign_randao(&store, &pubkey, epoch)? - }; + let randao_reveal = self.context.sign_randao(store, &pubkey, epoch).await?; // Both read once, before the block is asked for, so the check below // compares against the address in force when the request went out. @@ -240,26 +234,29 @@ impl ProposalService { "Block produced; signing" ); - // The root is taken from the decoded container, never from anything - // reassembled here, and it is taken once: the same value is what the + // The header is taken from the decoded container, never from anything + // reassembled here, and it is taken once: its root is what the // recorded slot belongs to and what the signature covers. - let block_root = produced.block_root(); + // + // The block's root is the header's, so it is taken from the header: + // that rehashes five fields, where `produced.block_root()` would hash + // the whole body, transactions included, a second time inside the + // proposal's budget. + let header = produced.header(); + let block_root = header.hash_tree_root(); // The check that matters, and the one that records, durably, before // the signature exists. Everything after this point may be abandoned // without risk, because the record already counts this slot as - // proposed. Off the executor, and before the store's read lock below, - // so the disk flush never holds up a keymanager writer. + // proposed. Off the executor, and with no guard on the store held, so + // the disk flush never holds up a keymanager writer. let recorded = crate::slashing_protection::blocking(&self.protection, move |protection| { protection.check_and_record_block(&pubkey, slot) }) .await; refuse_unless_safe(slot, duty.validator_index, "record", recorded)?; - let signature = { - let store = store.read().await; - self.context.sign_block(&store, &pubkey, block_root, slot)? - }; + let signature = self.context.sign_block(store, &pubkey, &header).await?; // Taken out before the block is signed into its body: the envelope is // published on its own, after the block, and the bare gloas block body @@ -350,11 +347,10 @@ impl ProposalService { ))); } - let signature = { - let store = store.read().await; - self.context - .sign_execution_payload_envelope(&store, pubkey, &envelope, slot)? - }; + let signature = self + .context + .sign_execution_payload_envelope(store, pubkey, &envelope, slot) + .await?; let body = match payload { Some(payload) => payload.into_signed_contents_ssz(signature), None => signed_envelope_ssz(envelope, signature), @@ -443,7 +439,6 @@ mod tests { use crate::beacon_node::mock::MockBeaconNode; use crate::slashing_protection::SlashingProtection; use ethlambda_types::beacon::config::Config; - use ethlambda_types::beacon::primitives::HashTreeRoot as _; use ethlambda_types::beacon::primitives::{BlsPubkey, Bytes32, H160, Root}; use libssz::SszDecode as _; @@ -455,10 +450,7 @@ mod tests { } fn context() -> Arc { - Arc::new(SigningContext { - config: Config::mainnet(), - genesis_validators_root: Root::ZERO, - }) + Arc::new(SigningContext::new(Config::mainnet(), Root::ZERO)) } /// A store holding one key, and the pubkey it resolves to. @@ -860,7 +852,7 @@ mod gloas_tests { use ethlambda_types::beacon::config::Config; use ethlambda_types::beacon::containers::gloas; use ethlambda_types::beacon::preset::SLOTS_PER_EPOCH; - use ethlambda_types::beacon::primitives::{BlsPubkey, HashTreeRoot as _}; + use ethlambda_types::beacon::primitives::BlsPubkey; use libssz::SszDecode as _; const GLOAS_EPOCH: u64 = 10; @@ -874,10 +866,10 @@ mod gloas_tests { } fn context() -> Arc { - Arc::new(SigningContext { - config: Config::mainnet().with_fork_epoch(ForkName::Gloas, GLOAS_EPOCH), - genesis_validators_root: Root::ZERO, - }) + Arc::new(SigningContext::new( + Config::mainnet().with_fork_epoch(ForkName::Gloas, GLOAS_EPOCH), + Root::ZERO, + )) } fn store() -> (RwLock, BlsPubkey) { diff --git a/crates/validator/src/signing.rs b/crates/validator/src/signing.rs index 9317ab341..ccb72cc8e 100644 --- a/crates/validator/src/signing.rs +++ b/crates/validator/src/signing.rs @@ -17,7 +17,8 @@ use ethlambda_types::beacon::containers::altair::{ use ethlambda_types::beacon::containers::gloas::{ ExecutionPayloadEnvelope, PayloadAttestationData, }; -use ethlambda_types::beacon::containers::shared::AttestationData; +use ethlambda_types::beacon::containers::shared::{AttestationData, BeaconBlockHeader, Fork}; +use ethlambda_types::beacon::fork::ForkName; use ethlambda_types::beacon::primitives::{ BLS_SIGNATURE_SIZE, BlsPubkey, BlsSignature, Domain, DomainType, Epoch, HashTreeRoot as _, Root, Slot, @@ -28,14 +29,42 @@ use ethlambda_types::beacon::signing::{ compute_domain, compute_epoch_at_slot, compute_signing_root, }; +use std::collections::HashMap; +use std::sync::{Mutex, MutexGuard, PoisonError}; +use std::time::Duration; + +use ethlambda_types::beacon::preset::SLOTS_PER_EPOCH; +use tokio::sync::RwLock; + use crate::error::{Error, Result}; +use crate::keys::web3signer::{ + AggregationSlot, ForkInfo, RandaoReveal, SigningBody, SigningRequest, SyncCommitteeMessage, + Versioned, VersionedHeader, +}; use crate::keys::{SigningMethod, ValidatorStore}; +pub use crate::keys::web3signer::AggregateAndProofRef; + /// The ciphersuite the consensus layer pins BLS signatures to. It must match /// `ethlambda_state_transition::beacon::bls`'s own constant, or nothing this /// client signs will ever verify. const DST: &[u8] = b"BLS_SIG_BLS12381G2_XMD:SHA-256_SSWU_RO_POP_"; +/// Whether `signature` is `pubkey`'s signature over `root`. A pubkey or a +/// signature that is not a point on the curve verifies nothing. +fn verifies(pubkey: &BlsPubkey, root: Root, signature: &BlsSignature) -> bool { + use blst::min_pk::{PublicKey, Signature}; + + let (Ok(public_key), Ok(signature)) = ( + PublicKey::from_bytes(&pubkey.0), + Signature::from_bytes(&signature.0), + ) else { + return false; + }; + signature.verify(true, root.as_slice(), DST, &[], &public_key, true) + == blst::BLST_ERROR::BLST_SUCCESS +} + /// The chain-wide half of a signing domain, fixed for the life of the process. /// /// The fork version deliberately does not live here. It is a function of the @@ -49,9 +78,64 @@ pub struct SigningContext { /// signature from one chain never verifies on another running the same /// fork schedule. pub genesis_validators_root: Root, + /// Selection proofs already produced; see [`Self::sign_selection_proof`]. + selection_proofs: Mutex>, +} + +/// How long a selection proof is remembered past its slot: the duty +/// schedule holds the current epoch and the next, so a proof computed for a +/// subscription at the start of an epoch is still wanted by the aggregation +/// up to two epochs later. +const SELECTION_PROOF_RETENTION_SLOTS: Slot = 3 * SLOTS_PER_EPOCH; + +/// How long one duty's batch of signatures may take, from when it is asked +/// for. +/// +/// Well inside every duty's own budget (the rest of its slot, at least a +/// third of one), so a signer that hangs costs the signatures it owes and +/// nothing else: the batch is published with whatever arrived in time, local +/// keys and other signers' keys included. A healthy Web3Signer answers in +/// milliseconds; one slower than this is one whose signatures would land too +/// late to be worth much anyway. +pub const SIGNING_BUDGET: Duration = Duration::from_secs(2); + +/// Ask for every signature in `requests` at once, and give each until +/// [`SIGNING_BUDGET`] from now. +/// +/// One result per request, in order. A request still outstanding at the +/// deadline is dropped and reported as an [`Error::Signing`] for its key, so +/// one unresponsive signer delays the batch by the budget at most and never +/// takes the signatures that did arrive down with it. +pub async fn sign_all(requests: impl IntoIterator) -> Vec> +where + F: Future>, +{ + let deadline = tokio::time::Instant::now() + SIGNING_BUDGET; + futures::future::join_all(requests.into_iter().map(|(pubkey, request)| async move { + tokio::time::timeout_at(deadline, request) + .await + .unwrap_or_else(|_| { + Err(Error::Signing { + pubkey, + reason: format!( + "no signature within the {}s signing budget", + SIGNING_BUDGET.as_secs() + ), + }) + }) + })) + .await } impl SigningContext { + pub fn new(config: Config, genesis_validators_root: Root) -> Self { + Self { + config, + genesis_validators_root, + selection_proofs: Mutex::new(HashMap::new()), + } + } + /// The domain for `domain_type` as of `epoch`. pub fn domain(&self, domain_type: DomainType, epoch: Epoch) -> Domain { let fork = self.config.fork_at_epoch(epoch); @@ -192,38 +276,124 @@ impl SigningContext { compute_signing_root(message.hash_tree_root(), domain) } + /// The fork a message domained at `epoch` is signed under, told the way a + /// remote signer wants it: the fork active at that epoch as `current`, + /// the one before it as `previous`, and the epoch the current one took + /// effect. Phase0 has no predecessor and names itself twice, as the + /// genesis state's own `Fork` does. + pub fn fork_info(&self, epoch: Epoch) -> ForkInfo { + let fork = self.config.fork_at_epoch(epoch); + let current_version = self.config.fork_version(fork); + let previous_version = fork + .previous() + .map(|previous| self.config.fork_version(previous)) + .unwrap_or(current_version); + ForkInfo { + fork: Fork { + previous_version, + current_version, + epoch: self.config.fork_epoch(fork), + }, + genesis_validators_root: self.genesis_validators_root, + } + } + + /// Web3Signer's name for the fork at `epoch`: the specification's + /// lowercase name in capitals (`ELECTRA`, `GLOAS`). + fn fork_label(&self, epoch: Epoch) -> String { + self.config.fork_at_epoch(epoch).as_str().to_uppercase() + } + /// Sign an already-computed signing root on behalf of `pubkey`. /// /// Every public signing method funnels through here, so there is one place - /// a signature is actually produced and one place the remote-signer - /// variant will need to be added. Deliberately private: a caller that can - /// hand in an arbitrary root can make this client sign anything at all, - /// and the domain separation that keeps an attestation from being read as - /// a block lives in the callers above. - fn sign_root( + /// a signature is actually produced. Deliberately private: a caller that + /// can hand in an arbitrary root can make this client sign anything at + /// all, and the domain separation that keeps an attestation from being + /// read as a block lives in the callers above. + /// + /// `epoch` is the epoch the root's domain was taken at, and `body` the + /// message itself; a local key needs neither, a remote signer needs both. + /// + /// The store's read guard is held for the lookup alone and released before + /// anything is signed. `tokio::sync::RwLock` is write-preferring, so a + /// guard held across the remote signer's round trip would let one queued + /// keymanager writer stall every other duty behind it for that long. + async fn sign( &self, - store: &ValidatorStore, + store: &RwLock, pubkey: &BlsPubkey, root: Root, + epoch: Epoch, + body: SigningBody<'_>, ) -> Result { - let method = store.get(pubkey).ok_or(Error::UnknownValidator(*pubkey))?; + let method = store + .read() + .await + .signer(pubkey) + .ok_or(Error::UnknownValidator(*pubkey))?; match method { SigningMethod::LocalKeystore { secret_key } => { let signature = secret_key.sign(root.as_slice(), DST, &[]); let bytes: [u8; BLS_SIGNATURE_SIZE] = signature.to_bytes(); Ok(BlsSignature(bytes)) } + SigningMethod::Web3Signer { signer } => { + // Web3Signer's `BLOCK_V2` takes the full block before + // bellatrix and the header only from bellatrix on, and this + // client only ever sends the header. Refused here rather than + // sent in a shape the signer answers with a 400; the proposal + // path refuses those forks long before this anyway. + if matches!(body, SigningBody::Block { .. }) + && self.config.fork_at_epoch(epoch) < ForkName::Bellatrix + { + return Err(Error::Signing { + pubkey: *pubkey, + reason: "a remote signer is only asked for blocks from bellatrix on" + .to_string(), + }); + } + let request = SigningRequest::new(self.fork_info(epoch), root, body); + let signature = signer.sign(pubkey, &request).await?; + // Checked before it leaves this function. The signer is + // another process on the far side of a network: one holding a + // different key under this pubkey, computing another root + // for a fork it does not know, or answering through a proxy + // that rewrote the body would otherwise have this client + // publish a signature no peer accepts, and decide + // `is_aggregator` on bytes that are not the selection proof. + // A local key needs no such check: its signature is computed + // here, over this root. + if !verifies(pubkey, root, &signature) { + return Err(Error::Signing { + pubkey: *pubkey, + reason: format!( + "remote signer {} answered with a signature that does not verify \ + for this key and message", + signer.url() + ), + }); + } + Ok(signature) + } } } /// Sign `data` on behalf of `pubkey`. - pub fn sign_attestation( + pub async fn sign_attestation( &self, - store: &ValidatorStore, + store: &RwLock, pubkey: &BlsPubkey, data: &AttestationData, ) -> Result { - self.sign_root(store, pubkey, self.attestation_signing_root(data)) + self.sign( + store, + pubkey, + self.attestation_signing_root(data), + data.target.epoch, + SigningBody::Attestation { attestation: data }, + ) + .await } /// Sign the RANDAO reveal for `epoch` on behalf of `pubkey`. @@ -232,13 +402,22 @@ impl SigningContext { /// it commits to an epoch, not to a chain position, and producing one for /// an epoch a validator is not proposing in reveals nothing and risks /// nothing. It is therefore not guarded, and does not need to be. - pub fn sign_randao( + pub async fn sign_randao( &self, - store: &ValidatorStore, + store: &RwLock, pubkey: &BlsPubkey, epoch: Epoch, ) -> Result { - self.sign_root(store, pubkey, self.randao_signing_root(epoch)) + self.sign( + store, + pubkey, + self.randao_signing_root(epoch), + epoch, + SigningBody::RandaoReveal { + randao_reveal: RandaoReveal { epoch }, + }, + ) + .await } /// Sign the selection proof for `slot` on behalf of `pubkey`. @@ -247,17 +426,48 @@ impl SigningContext { /// to a chain position, so producing one twice is producing the same bytes /// twice: BLS signatures are deterministic, which is the property the /// selection rule rests on. - pub fn sign_selection_proof( + /// + /// That determinism is also why the answer is remembered. The same proof + /// is wanted once when the epoch's subscriptions are sent and again by the + /// aggregation tick, and for a remote key each ask is a round trip; a + /// remembered proof is the very signature the signer would return. Only + /// a proof that was produced is remembered, so a failure is asked again. + pub async fn sign_selection_proof( &self, - store: &ValidatorStore, + store: &RwLock, pubkey: &BlsPubkey, slot: Slot, ) -> Result { - self.sign_root(store, pubkey, self.selection_proof_signing_root(slot)) + if let Some(proof) = self.selection_proofs().get(&(*pubkey, slot)) { + return Ok(*proof); + } + let proof = self + .sign( + store, + pubkey, + self.selection_proof_signing_root(slot), + compute_epoch_at_slot(slot), + SigningBody::AggregationSlot { + aggregation_slot: AggregationSlot { slot }, + }, + ) + .await?; + let mut proofs = self.selection_proofs(); + proofs.retain(|(_, held), _| held + SELECTION_PROOF_RETENTION_SLOTS >= slot); + proofs.insert((*pubkey, slot), proof); + Ok(proof) } - /// Sign the aggregate whose `AggregateAndProof` root is `root`, for `slot`, - /// on behalf of `pubkey`. + /// The remembered selection proofs. A poisoned map is still a map of + /// valid proofs, since an entry is inserted whole or not at all. + fn selection_proofs(&self) -> MutexGuard<'_, HashMap<(BlsPubkey, Slot), BlsSignature>> { + self.selection_proofs + .lock() + .unwrap_or_else(PoisonError::into_inner) + } + + /// Sign `message`, an aggregator's `AggregateAndProof`, on behalf of + /// `pubkey`. /// /// Not slashable either, which is worth stating because it is the only /// signature here that covers an attestation and is not. The slashing @@ -265,18 +475,34 @@ impl SigningContext { /// republishing other validators' votes with a wrapper saying who /// collected them, and publishing two different aggregates for one slot is /// wasteful rather than punishable. - pub fn sign_aggregate_and_proof( + /// + /// Takes the whole wrapper rather than its root: a remote signer wants to + /// read what it signs, and the wrapper's own fork decides its JSON shape. + pub async fn sign_aggregate_and_proof( &self, - store: &ValidatorStore, + store: &RwLock, pubkey: &BlsPubkey, - root: Root, - slot: Slot, + message: AggregateAndProofRef<'_>, ) -> Result { - self.sign_root( + let slot = message.slot(); + let epoch = compute_epoch_at_slot(slot); + let version = match message { + AggregateAndProofRef::Gloas(_) => ForkName::Gloas.as_str().to_uppercase(), + AggregateAndProofRef::Electra(_) => self.fork_label(epoch), + }; + self.sign( store, pubkey, - self.aggregate_and_proof_signing_root(root, slot), + self.aggregate_and_proof_signing_root(message.hash_tree_root(), slot), + epoch, + SigningBody::AggregateAndProof { + aggregate_and_proof: Versioned { + version, + data: message, + }, + }, ) + .await } /// Sign the envelope revealing the payload of the block proposed for `slot` @@ -287,14 +513,26 @@ impl SigningContext { /// condition to an envelope: signing two for one slot is at worst a /// payload the network ignores one of. The block it belongs to is the /// guarded signature. - pub fn sign_execution_payload_envelope( + pub async fn sign_execution_payload_envelope( &self, - store: &ValidatorStore, + store: &RwLock, pubkey: &BlsPubkey, envelope: &ExecutionPayloadEnvelope, slot: Slot, ) -> Result { - self.sign_root(store, pubkey, self.envelope_signing_root(envelope, slot)) + self.sign( + store, + pubkey, + self.envelope_signing_root(envelope, slot), + compute_epoch_at_slot(slot), + SigningBody::ExecutionPayloadEnvelope { + execution_payload_envelope: Versioned { + version: ForkName::Gloas.as_str().to_uppercase(), + data: envelope, + }, + }, + ) + .await } /// Sign a payload timeliness committee vote on behalf of `pubkey`. @@ -302,13 +540,25 @@ impl SigningContext { /// Not slashable by the specification, and deduplicated per validator and /// slot by [`crate::payload_attestation`] only to avoid publishing the same /// vote twice. - pub fn sign_payload_attestation( + pub async fn sign_payload_attestation( &self, - store: &ValidatorStore, + store: &RwLock, pubkey: &BlsPubkey, data: &PayloadAttestationData, ) -> Result { - self.sign_root(store, pubkey, self.payload_attestation_signing_root(data)) + self.sign( + store, + pubkey, + self.payload_attestation_signing_root(data), + compute_epoch_at_slot(data.slot), + SigningBody::PayloadAttestationMessage { + payload_attestation_message: Versioned { + version: ForkName::Gloas.as_str().to_uppercase(), + data, + }, + }, + ) + .await } /// Sign a sync committee message for `slot` over `beacon_block_root`. @@ -316,53 +566,79 @@ impl SigningContext { /// Not slashable, and deduplicated per validator and slot by /// [`crate::sync_committee`] only to avoid publishing the same message /// twice. - pub fn sign_sync_committee_message( + pub async fn sign_sync_committee_message( &self, - store: &ValidatorStore, + store: &RwLock, pubkey: &BlsPubkey, slot: Slot, beacon_block_root: Root, ) -> Result { - self.sign_root( + self.sign( store, pubkey, self.sync_committee_message_signing_root(slot, beacon_block_root), + compute_epoch_at_slot(slot), + SigningBody::SyncCommitteeMessage { + sync_committee_message: SyncCommitteeMessage { + beacon_block_root, + slot, + }, + }, ) + .await } /// Sign the sync committee selection proof for `slot` and one /// subcommittee. Deterministic, so not guarded, for the reason /// [`Self::sign_selection_proof`] is not. - pub fn sign_sync_selection_proof( + pub async fn sign_sync_selection_proof( &self, - store: &ValidatorStore, + store: &RwLock, pubkey: &BlsPubkey, slot: Slot, subcommittee_index: u64, ) -> Result { - self.sign_root( + self.sign( store, pubkey, self.sync_selection_proof_signing_root(slot, subcommittee_index), + compute_epoch_at_slot(slot), + SigningBody::SyncCommitteeSelectionProof { + sync_aggregator_selection_data: SyncAggregatorSelectionData { + slot, + subcommittee_index, + }, + }, ) + .await } /// Sign a `ContributionAndProof` on behalf of its aggregator `pubkey`. - pub fn sign_contribution_and_proof( + pub async fn sign_contribution_and_proof( &self, - store: &ValidatorStore, + store: &RwLock, pubkey: &BlsPubkey, message: &ContributionAndProof, ) -> Result { - self.sign_root( + self.sign( store, pubkey, self.contribution_and_proof_signing_root(message), + compute_epoch_at_slot(message.contribution.slot), + SigningBody::SyncCommitteeContributionAndProof { + contribution_and_proof: message, + }, ) + .await } - /// Sign the block whose root is `block_root`, proposed for `slot`, on - /// behalf of `pubkey`. + /// Sign the block whose header is `header` on behalf of `pubkey`. + /// + /// The root signed is the header's, which is the block's: a header is the + /// block with its body replaced by that body's root, and the two + /// merkleize to the same value. The header rather than the root travels + /// because a remote signer reads it to keep its slashing-protection + /// record. /// /// This one *is* slashable, and is the most expensive signature this /// client can get wrong: two distinct blocks for one slot need no second @@ -370,14 +646,26 @@ impl SigningContext { /// evidence. Callers must have it allowed and recorded by /// [`crate::slashing_protection::SlashingProtection::check_and_record_block`] /// first. - pub fn sign_block( + pub async fn sign_block( &self, - store: &ValidatorStore, + store: &RwLock, pubkey: &BlsPubkey, - block_root: Root, - slot: Slot, + header: &BeaconBlockHeader, ) -> Result { - self.sign_root(store, pubkey, self.block_signing_root(block_root, slot)) + let epoch = compute_epoch_at_slot(header.slot); + self.sign( + store, + pubkey, + self.block_signing_root(header.hash_tree_root(), header.slot), + epoch, + SigningBody::Block { + beacon_block: VersionedHeader { + version: self.fork_label(epoch), + block_header: header.clone(), + }, + }, + ) + .await } } @@ -395,10 +683,7 @@ mod tests { } fn context() -> SigningContext { - SigningContext { - config: Config::mainnet(), - genesis_validators_root: Root::ZERO, - } + SigningContext::new(Config::mainnet(), Root::ZERO) } fn attestation_data(target_epoch: Epoch) -> AttestationData { @@ -417,17 +702,17 @@ mod tests { } } - #[test] - fn a_signature_verifies_under_the_signing_root() { + #[tokio::test] + async fn a_signature_verifies_under_the_signing_root() { use blst::min_pk::{PublicKey, Signature}; - let mut store = ValidatorStore::new(); - let pubkey = store.insert_secret("test", &secret()).expect("inserts"); + let (store, pubkey) = store_with_key(); let context = context(); let data = attestation_data(100); let signature = context .sign_attestation(&store, &pubkey, &data) + .await .expect("signs"); let root = context.attestation_signing_root(&data); @@ -439,6 +724,54 @@ mod tests { ); } + /// One request that never answers costs its own signature, after the + /// budget, and nothing else: the others come back as they were. + #[tokio::test(start_paused = true)] + async fn a_signature_past_the_budget_does_not_hold_back_the_rest() { + use futures::FutureExt as _; + + let started = tokio::time::Instant::now(); + let answered = BlsPubkey([1; 48]); + let hung = BlsPubkey([2; 48]); + let results = sign_all([ + (answered, async { Ok(7u8) }.boxed()), + (hung, std::future::pending().boxed()), + ]) + .await; + + assert_eq!(started.elapsed(), SIGNING_BUDGET); + assert!(matches!(results[0], Ok(7))); + assert!( + matches!(&results[1], Err(Error::Signing { pubkey, .. }) if *pubkey == hung), + "the late one is a signing failure for its own key" + ); + } + + #[tokio::test] + async fn a_selection_proof_is_remembered_once_produced() { + let (store, pubkey) = store_with_key(); + let context = context(); + let first = context + .sign_selection_proof(&store, &pubkey, 3200) + .await + .expect("signs"); + + // With the key gone, only the remembered proof can answer. + assert!(store.write().await.remove(&pubkey)); + let again = context + .sign_selection_proof(&store, &pubkey, 3200) + .await + .expect("remembered"); + assert_eq!(first, again); + assert!( + context + .sign_selection_proof(&store, &pubkey, 3201) + .await + .is_err(), + "another slot is another proof" + ); + } + #[test] fn the_domain_changes_with_the_epoch() { let context = context(); @@ -480,11 +813,12 @@ mod tests { assert_eq!(context.attestation_signing_root(&data), from_target); } - #[test] - fn signing_for_an_unknown_validator_is_an_error() { - let store = ValidatorStore::new(); + #[tokio::test] + async fn signing_for_an_unknown_validator_is_an_error() { + let store = RwLock::new(ValidatorStore::new()); let err = context() .sign_attestation(&store, &BlsPubkey::default(), &attestation_data(1)) + .await .expect_err("must fail"); assert!(matches!(err, Error::UnknownValidator(_)), "got {err:?}"); } @@ -501,18 +835,31 @@ mod tests { sig.verify(true, root.as_slice(), DST, &[], &pk, true) == blst::BLST_ERROR::BLST_SUCCESS } - fn store_with_key() -> (ValidatorStore, BlsPubkey) { + fn store_with_key() -> (RwLock, BlsPubkey) { let mut store = ValidatorStore::new(); let pubkey = store.insert_secret("test", &secret()).expect("inserts"); - (store, pubkey) + (RwLock::new(store), pubkey) } - #[test] - fn a_randao_reveal_verifies_under_the_randao_signing_root() { + fn header(slot: Slot) -> BeaconBlockHeader { + BeaconBlockHeader { + slot, + proposer_index: 7, + parent_root: Root::repeat_byte(1), + state_root: Root::repeat_byte(2), + body_root: Root::repeat_byte(3), + } + } + + #[tokio::test] + async fn a_randao_reveal_verifies_under_the_randao_signing_root() { let (store, pubkey) = store_with_key(); let context = context(); - let signature = context.sign_randao(&store, &pubkey, 100).expect("signs"); + let signature = context + .sign_randao(&store, &pubkey, 100) + .await + .expect("signs"); assert!(verify( &pubkey, &signature, @@ -520,19 +867,20 @@ mod tests { )); } - #[test] - fn a_block_signature_verifies_under_the_block_signing_root() { + #[tokio::test] + async fn a_block_signature_verifies_under_the_block_signing_root() { let (store, pubkey) = store_with_key(); let context = context(); - let block_root = Root::repeat_byte(7); + let header = header(3200); let signature = context - .sign_block(&store, &pubkey, block_root, 3200) + .sign_block(&store, &pubkey, &header) + .await .expect("signs"); assert!(verify( &pubkey, &signature, - context.block_signing_root(block_root, 3200) + context.block_signing_root(header.hash_tree_root(), 3200) )); } @@ -635,22 +983,24 @@ mod tests { ); } - #[test] - fn signing_a_block_for_an_unknown_validator_is_an_error() { - let store = ValidatorStore::new(); + #[tokio::test] + async fn signing_a_block_for_an_unknown_validator_is_an_error() { + let store = RwLock::new(ValidatorStore::new()); let err = context() - .sign_block(&store, &BlsPubkey::default(), Root::ZERO, 1) + .sign_block(&store, &BlsPubkey::default(), &header(1)) + .await .expect_err("must fail"); assert!(matches!(err, Error::UnknownValidator(_)), "got {err:?}"); } - #[test] - fn a_selection_proof_verifies_under_its_own_root() { + #[tokio::test] + async fn a_selection_proof_verifies_under_its_own_root() { let (store, pubkey) = store_with_key(); let context = context(); let signature = context .sign_selection_proof(&store, &pubkey, 3200) + .await .expect("signs"); assert!(verify( &pubkey, @@ -659,30 +1009,43 @@ mod tests { )); } - #[test] - fn an_aggregate_signature_verifies_under_its_own_root() { + #[tokio::test] + async fn an_aggregate_signature_verifies_under_its_own_root() { + use ethlambda_types::beacon::containers::electra; + let (store, pubkey) = store_with_key(); let context = context(); - let root = Root::repeat_byte(4); + let message = electra::AggregateAndProof { + aggregator_index: 9, + aggregate: electra::Attestation { + aggregation_bits: Default::default(), + data: attestation_data(100), + signature: BlsSignature([2; 96]), + committee_bits: Default::default(), + }, + selection_proof: BlsSignature([3; 96]), + }; let signature = context - .sign_aggregate_and_proof(&store, &pubkey, root, 3200) + .sign_aggregate_and_proof(&store, &pubkey, AggregateAndProofRef::Electra(&message)) + .await .expect("signs"); assert!(verify( &pubkey, &signature, - context.aggregate_and_proof_signing_root(root, 3200) + context.aggregate_and_proof_signing_root(message.hash_tree_root(), 3200) )); } - #[test] - fn a_sync_committee_message_verifies_under_its_own_root() { + #[tokio::test] + async fn a_sync_committee_message_verifies_under_its_own_root() { let (store, pubkey) = store_with_key(); let context = context(); let root = Root::repeat_byte(5); let signature = context .sign_sync_committee_message(&store, &pubkey, 3200, root) + .await .expect("signs"); assert!(verify( &pubkey, @@ -707,13 +1070,14 @@ mod tests { ); } - #[test] - fn a_sync_selection_proof_verifies_and_names_the_subcommittee() { + #[tokio::test] + async fn a_sync_selection_proof_verifies_and_names_the_subcommittee() { let (store, pubkey) = store_with_key(); let context = context(); let signature = context .sign_sync_selection_proof(&store, &pubkey, 3200, 2) + .await .expect("signs"); assert!(verify( &pubkey, @@ -732,8 +1096,8 @@ mod tests { ); } - #[test] - fn a_contribution_and_proof_verifies_under_its_own_root() { + #[tokio::test] + async fn a_contribution_and_proof_verifies_under_its_own_root() { use ethlambda_types::beacon::containers::altair::SyncCommitteeContribution; let (store, pubkey) = store_with_key(); @@ -752,6 +1116,7 @@ mod tests { let signature = context .sign_contribution_and_proof(&store, &pubkey, &message) + .await .expect("signs"); assert!(verify( &pubkey, @@ -760,12 +1125,327 @@ mod tests { )); } - #[test] - fn signing_a_randao_reveal_for_an_unknown_validator_is_an_error() { - let store = ValidatorStore::new(); + #[tokio::test] + async fn signing_a_randao_reveal_for_an_unknown_validator_is_an_error() { + let store = RwLock::new(ValidatorStore::new()); let err = context() .sign_randao(&store, &BlsPubkey::default(), 1) + .await .expect_err("must fail"); assert!(matches!(err, Error::UnknownValidator(_)), "got {err:?}"); } + + /// `fork_info` must describe the fork at the epoch the domain was taken + /// at, since the remote signer recomputes the domain from it. Mainnet's + /// altair boundary pins all three fields on either side. + #[test] + fn fork_info_names_the_fork_at_the_epoch_and_its_predecessor() { + let context = context(); + let altair = context.config.altair_fork_epoch; + + let before = context.fork_info(altair - 1); + assert_eq!( + before.fork.current_version, + context.config.genesis_fork_version + ); + assert_eq!( + before.fork.previous_version, before.fork.current_version, + "phase0 has no predecessor and names itself twice" + ); + assert_eq!(before.fork.epoch, 0); + + let after = context.fork_info(altair); + assert_eq!( + after.fork.current_version, + context.config.altair_fork_version + ); + assert_eq!( + after.fork.previous_version, + context.config.genesis_fork_version + ); + assert_eq!(after.fork.epoch, altair); + assert_eq!( + after.genesis_validators_root, + context.genesis_validators_root + ); + } + + /// A remote signer, stood in for by a local HTTP server: the request it + /// receives is the one Web3Signer documents, and the signature it returns + /// is what the caller gets back. + mod remote { + use super::*; + use axum::Router; + use axum::extract::{Path, State}; + use axum::http::StatusCode; + use axum::routing::post; + use std::sync::{Arc, Mutex}; + + /// What the fake signer does: records every request and answers with + /// a signature, or with a fixed status. + #[derive(Clone)] + struct Fake { + requests: Arc>>, + answer: Arc>, + } + + #[derive(Clone, Copy)] + enum Answer { + /// The test key's signature over the request's `signingRoot`, as + /// a signer holding that key would give. + Signature, + /// A well-formed signature over the request's `signingRoot`, by a + /// key other than the one asked for. + AnotherKeysSignature, + /// The test key's signature, over a root other than the request's. + AnotherRootsSignature, + /// 96 bytes that are not a signature at all. + Garbage, + Status(u16), + } + + /// `secret`'s signature over `root`, as hex. + fn signature_hex(secret: &[u8; 32], root: &[u8]) -> String { + let secret_key = blst::min_pk::SecretKey::from_bytes(secret).expect("a valid scalar"); + hex::encode(secret_key.sign(root, DST, &[]).to_bytes()) + } + + /// A second key, for a signer that answers with the wrong one. + fn other_secret() -> [u8; 32] { + let mut bytes = secret(); + bytes[31] ^= 0x01; + bytes + } + + async fn handle( + State(fake): State, + Path(pubkey): Path, + body: String, + ) -> (StatusCode, String) { + let json: serde_json::Value = serde_json::from_str(&body).expect("json body"); + let root = hex::decode( + json["signingRoot"] + .as_str() + .expect("a signing root") + .trim_start_matches("0x"), + ) + .expect("hex"); + fake.requests.lock().expect("lock").push((pubkey, json)); + let signed = |signature: String| { + ( + StatusCode::OK, + format!("{{\"signature\":\"0x{signature}\"}}"), + ) + }; + match *fake.answer.lock().expect("lock") { + Answer::Signature => signed(signature_hex(&secret(), &root)), + Answer::AnotherKeysSignature => signed(signature_hex(&other_secret(), &root)), + Answer::AnotherRootsSignature => signed(signature_hex(&secret(), &[0x11; 32])), + Answer::Garbage => signed("ab".repeat(96)), + Answer::Status(code) => ( + StatusCode::from_u16(code).expect("status"), + "refused".to_string(), + ), + } + } + + /// Start the fake on an ephemeral port and return it with its URL. + async fn serve(answer: Answer) -> (Fake, String) { + let fake = Fake { + requests: Arc::new(Mutex::new(Vec::new())), + answer: Arc::new(Mutex::new(answer)), + }; + let app = Router::new() + .route("/api/v1/eth2/sign/{pubkey}", post(handle)) + .with_state(fake.clone()); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0") + .await + .expect("binds"); + let address = listener.local_addr().expect("address"); + tokio::spawn(async move { + axum::serve(listener, app).await.expect("serves"); + }); + (fake, format!("http://{address}")) + } + + /// A store whose one key, the test key's, is held by the signer at + /// `url` and not by this process. + fn remote_store(url: &str) -> (RwLock, BlsPubkey) { + let pubkey = ValidatorStore::derive_pubkey(&secret()).expect("a valid key"); + let mut store = ValidatorStore::new(); + store.insert_remote(pubkey, url).expect("inserts"); + (RwLock::new(store), pubkey) + } + + #[tokio::test] + async fn an_attestation_is_signed_by_the_remote_signer() { + let (fake, url) = serve(Answer::Signature).await; + let (store, pubkey) = remote_store(&url); + let context = context(); + let data = attestation_data(context.config.altair_fork_epoch + 1); + + let signature = context + .sign_attestation(&store, &pubkey, &data) + .await + .expect("signs"); + assert!( + verify(&pubkey, &signature, context.attestation_signing_root(&data)), + "the signer's signature is the one returned" + ); + + let requests = fake.requests.lock().expect("lock"); + assert_eq!(requests.len(), 1); + let (path_pubkey, body) = &requests[0]; + assert_eq!(path_pubkey, &format!("0x{}", hex::encode(pubkey.0))); + assert_eq!(body["type"], "ATTESTATION"); + assert_eq!( + body["signingRoot"], + serde_json::to_value(context.attestation_signing_root(&data)).expect("root") + ); + assert_eq!( + body["attestation"]["target"]["epoch"], + data.target.epoch.to_string() + ); + assert_eq!( + body["fork_info"], + serde_json::to_value(context.fork_info(data.target.epoch)).expect("fork info"), + "the fork is the target epoch's, where the domain came from" + ); + } + + #[tokio::test] + async fn a_block_request_names_the_fork_and_carries_the_header() { + let (fake, url) = serve(Answer::Signature).await; + let (store, pubkey) = remote_store(&url); + let context = context(); + let slot = context.config.electra_fork_epoch * preset::SLOTS_PER_EPOCH; + let header = header(slot); + + context + .sign_block(&store, &pubkey, &header) + .await + .expect("signs"); + + let requests = fake.requests.lock().expect("lock"); + let body = &requests[0].1; + assert_eq!(body["type"], "BLOCK_V2"); + assert_eq!(body["beacon_block"]["version"], "ELECTRA"); + assert_eq!( + body["beacon_block"]["block_header"]["slot"], + slot.to_string() + ); + assert_eq!( + body["signingRoot"], + serde_json::to_value(context.block_signing_root(header.hash_tree_root(), slot)) + .expect("root") + ); + } + + /// Before bellatrix the signer wants the full block, which this + /// client never sends, so the request is refused rather than made. + #[tokio::test] + async fn a_pre_bellatrix_block_is_not_sent_to_the_signer() { + let (fake, url) = serve(Answer::Signature).await; + let (store, pubkey) = remote_store(&url); + let context = context(); + let slot = context.config.altair_fork_epoch * preset::SLOTS_PER_EPOCH; + + let err = context + .sign_block(&store, &pubkey, &header(slot)) + .await + .expect_err("refused"); + assert!(matches!(err, Error::Signing { .. }), "got {err:?}"); + assert!(fake.requests.lock().expect("lock").is_empty()); + } + + /// What the signer answers is checked before it is used: a signature + /// by another key, over another root, or that is no signature at all + /// is a signing failure for that key, never something to publish. + #[tokio::test] + async fn a_signature_that_does_not_verify_is_a_signing_error() { + for answer in [ + Answer::AnotherKeysSignature, + Answer::AnotherRootsSignature, + Answer::Garbage, + ] { + let (fake, url) = serve(answer).await; + let (store, pubkey) = remote_store(&url); + + let err = context() + .sign_attestation(&store, &pubkey, &attestation_data(100)) + .await + .expect_err("must fail"); + assert!(matches!(err, Error::Signing { .. }), "got {err:?}"); + assert!(err.to_string().contains("does not verify"), "{err}"); + assert!(!err.is_retryable()); + assert_eq!(fake.requests.lock().expect("lock").len(), 1, "asked once"); + } + } + + /// A selection proof that does not verify is not remembered: the + /// next ask goes to the signer again rather than being answered from + /// bytes that were never the proof. + #[tokio::test] + async fn an_unverified_selection_proof_is_not_remembered() { + let (fake, url) = serve(Answer::AnotherKeysSignature).await; + let (store, pubkey) = remote_store(&url); + let context = context(); + + context + .sign_selection_proof(&store, &pubkey, 100) + .await + .expect_err("must fail"); + *fake.answer.lock().expect("lock") = Answer::Signature; + let proof = context + .sign_selection_proof(&store, &pubkey, 100) + .await + .expect("signs"); + assert!(verify( + &pubkey, + &proof, + context.selection_proof_signing_root(100) + )); + assert_eq!(fake.requests.lock().expect("lock").len(), 2); + } + + #[tokio::test] + async fn a_slashing_protection_refusal_is_a_signing_error() { + let (_fake, url) = serve(Answer::Status(412)).await; + let (store, pubkey) = remote_store(&url); + + let err = context() + .sign_attestation(&store, &pubkey, &attestation_data(100)) + .await + .expect_err("must fail"); + assert!(matches!(err, Error::Signing { .. }), "got {err:?}"); + assert!(err.to_string().contains("slashing protection"), "{err}"); + assert!(!err.is_retryable()); + } + + #[tokio::test] + async fn a_key_the_signer_does_not_hold_is_a_signing_error() { + let (_fake, url) = serve(Answer::Status(404)).await; + let (store, pubkey) = remote_store(&url); + + let err = context() + .sign_randao(&store, &pubkey, 1) + .await + .expect_err("must fail"); + assert!(matches!(err, Error::Signing { .. }), "got {err:?}"); + assert!(err.to_string().contains("does not hold"), "{err}"); + } + + #[tokio::test] + async fn an_unreachable_signer_is_a_signing_error() { + // Nothing listens on port 9 (discard), and a connection refused + // comes back at once rather than after the request timeout. + let (store, pubkey) = remote_store("http://127.0.0.1:9"); + + let err = context() + .sign_randao(&store, &pubkey, 1) + .await + .expect_err("must fail"); + assert!(matches!(err, Error::Signing { .. }), "got {err:?}"); + } + } } diff --git a/crates/validator/src/subscriptions.rs b/crates/validator/src/subscriptions.rs index 5de975781..37acf95f8 100644 --- a/crates/validator/src/subscriptions.rs +++ b/crates/validator/src/subscriptions.rs @@ -17,10 +17,11 @@ use ethlambda_types::beacon::primitives::Epoch; use crate::beacon_node::dto::{ AttesterDutyDto, CommitteeSubscriptionDto, SyncCommitteeSubscriptionDto, SyncDutyDto, + parse_pubkey, }; use crate::error::Result; use crate::keys::ValidatorStore; -use crate::signing::SigningContext; +use crate::signing::{SigningContext, sign_all}; /// Send one subscription per held duty, claiming the aggregator role for the /// duties this client was selected for. @@ -47,37 +48,59 @@ pub async fn subscribe( return Ok(()); } - // The read guard is scoped to this block and never held across the await - // below, for the reason spelled out in `crate::attestation`: the lock is - // write-preferring, so one queued keymanager writer blocks every reader - // behind it. - let subscriptions: Vec = { - let store = store.read().await; - duties + // Every selection proof is asked for at once, each within the signing + // budget. This runs on the duty loop at an epoch boundary, ahead of that + // slot's proposal, so one request per duty in series would let a slow or + // unreachable signer hold up every validator's duties, local keys + // included, for as many round trips as there are duties. Bounded this + // way it costs `SIGNING_BUDGET` at most, and the proofs that did arrive + // are remembered for the aggregation tick (see + // `SigningContext::sign_selection_proof`). + let mut is_aggregator = vec![false; duties.len()]; + let mut keyed = Vec::with_capacity(duties.len()); + for (position, duty) in duties.iter().enumerate() { + match parse_pubkey(&duty.pubkey) { + Ok(pubkey) => keyed.push((position, duty, pubkey)), + Err(err) => warn!( + slot = duty.slot, + validator = duty.validator_index, + %err, + "Duty carried an unreadable pubkey; subscribing without the role" + ), + } + } + let selections = sign_all( + keyed .iter() - .map(|duty| { - let is_aggregator = match selection_for(context, &store, duty) { - Ok(selection) => selection.is_some(), - Err(err) => { - warn!( - slot = duty.slot, - validator = duty.validator_index, - %err, - "Could not compute aggregator selection; subscribing without the role" - ); - false - } - }; - CommitteeSubscriptionDto { - validator_index: duty.validator_index, - committee_index: duty.committee_index, - committees_at_slot: duty.committees_at_slot, - slot: duty.slot, - is_aggregator, - } - }) - .collect() - }; + .map(|(_, duty, pubkey)| (*pubkey, selection_for(context, store, duty))), + ) + .await; + for ((position, duty, _), selection) in keyed.into_iter().zip(selections) { + match selection { + Ok(selection) => is_aggregator[position] = selection.is_some(), + Err(err) => { + warn!( + slot = duty.slot, + validator = duty.validator_index, + %err, + "Could not compute aggregator selection; subscribing without the role" + ); + crate::metrics::inc_signing_failures(); + } + } + } + + let subscriptions: Vec = duties + .iter() + .zip(is_aggregator) + .map(|(duty, is_aggregator)| CommitteeSubscriptionDto { + validator_index: duty.validator_index, + committee_index: duty.committee_index, + committees_at_slot: duty.committees_at_slot, + slot: duty.slot, + is_aggregator, + }) + .collect(); let aggregating = subscriptions .iter() @@ -134,10 +157,7 @@ mod tests { } fn context() -> SigningContext { - SigningContext { - config: Config::mainnet(), - genesis_validators_root: Root::ZERO, - } + SigningContext::new(Config::mainnet(), Root::ZERO) } fn store() -> (RwLock, BlsPubkey) { @@ -236,10 +256,10 @@ mod tests { .await .expect("subscribes"); - let guard = store.read().await; for (duty, sent) in duties.iter().zip(node.subscriptions()) { let proof = context - .sign_selection_proof(&guard, &pubkey, duty.slot) + .sign_selection_proof(&store, &pubkey, duty.slot) + .await .expect("signs"); assert_eq!( sent.is_aggregator, diff --git a/crates/validator/src/sync_committee.rs b/crates/validator/src/sync_committee.rs index 7501895e6..d898c92a9 100644 --- a/crates/validator/src/sync_committee.rs +++ b/crates/validator/src/sync_committee.rs @@ -46,7 +46,7 @@ use crate::beacon_node::dto::{SyncDutyDto, parse_pubkey}; use crate::beacon_node::{BeaconNodeApi, validate_sync_contribution}; use crate::error::{Error, Result}; use crate::keys::ValidatorStore; -use crate::signing::SigningContext; +use crate::signing::{SigningContext, sign_all}; /// How many slots of dedup history are kept. const HISTORY_SLOTS: u64 = 64; @@ -129,35 +129,45 @@ impl SyncCommitteeService { Err(err) => return Err(err), }; - let mut messages = Vec::with_capacity(pending.len()); - { - let _timing = crate::metrics::time_signing(); - let store = store.read().await; - for duty in pending { - let pubkey = match parse_pubkey(&duty.pubkey) { - Ok(pubkey) => pubkey, - Err(err) => { - error!(%slot, validator = duty.validator_index, %err, "Duty carried an unreadable pubkey"); - continue; - } - }; - if !self.record(duty.validator_index, slot) { + // Recorded one by one, then signed all at once, each within the + // signing budget: a hung remote signer costs only its own keys' + // messages, not the whole batch's. + let mut to_sign = Vec::with_capacity(pending.len()); + for duty in pending { + let pubkey = match parse_pubkey(&duty.pubkey) { + Ok(pubkey) => pubkey, + Err(err) => { + error!(%slot, validator = duty.validator_index, %err, "Duty carried an unreadable pubkey"); continue; } - match self - .context - .sign_sync_committee_message(&store, &pubkey, slot, root) - { - Ok(signature) => messages.push(SyncCommitteeMessage { - slot, - beacon_block_root: root, - validator_index: duty.validator_index, - signature, - }), - Err(err) => { - error!(%slot, validator = duty.validator_index, %err, "Failed to sign sync committee message"); - crate::metrics::inc_signing_failures(); - } + }; + if self.record(duty.validator_index, slot) { + to_sign.push((duty.validator_index, pubkey)); + } + } + let signatures = { + let _timing = crate::metrics::time_signing(); + sign_all(to_sign.iter().map(|(_, pubkey)| { + ( + *pubkey, + self.context + .sign_sync_committee_message(store, pubkey, slot, root), + ) + })) + .await + }; + let mut messages = Vec::with_capacity(to_sign.len()); + for ((validator_index, _), signature) in to_sign.into_iter().zip(signatures) { + match signature { + Ok(signature) => messages.push(SyncCommitteeMessage { + slot, + beacon_block_root: root, + validator_index, + signature, + }), + Err(err) => { + error!(%slot, validator = validator_index, %err, "Failed to sign sync committee message"); + crate::metrics::inc_signing_failures(); } } } @@ -191,32 +201,40 @@ impl SyncCommitteeService { duties: &[SyncDutyDto], store: &RwLock, ) -> Result { + // Every (validator, subnet) pair's selection proof, asked for at once + // and each within the signing budget. + let mut seats = Vec::new(); + for duty in duties { + let pubkey = match parse_pubkey(&duty.pubkey) { + Ok(pubkey) => pubkey, + Err(err) => { + error!(%slot, validator = duty.validator_index, %err, "Duty carried an unreadable pubkey"); + continue; + } + }; + for subnet in subnets_of(duty) { + seats.push((duty.validator_index, pubkey, subnet)); + } + } + let proofs = sign_all(seats.iter().map(|(_, pubkey, subnet)| { + ( + *pubkey, + self.context + .sign_sync_selection_proof(store, pubkey, slot, *subnet), + ) + })) + .await; // (validator, pubkey, subnet, selection proof) for each selected pair. let mut selected: Vec<(ValidatorIndex, BlsPubkey, u64, BlsSignature)> = Vec::new(); - { - let store = store.read().await; - for duty in duties { - let pubkey = match parse_pubkey(&duty.pubkey) { - Ok(pubkey) => pubkey, - Err(err) => { - error!(%slot, validator = duty.validator_index, %err, "Duty carried an unreadable pubkey"); - continue; - } - }; - for subnet in subnets_of(duty) { - match self - .context - .sign_sync_selection_proof(&store, &pubkey, slot, subnet) - { - Ok(proof) if is_sync_committee_aggregator(&proof) => { - selected.push((duty.validator_index, pubkey, subnet, proof)); - } - Ok(_) => {} - Err(err) => { - error!(%slot, validator = duty.validator_index, %err, "Failed to sign sync selection proof"); - crate::metrics::inc_signing_failures(); - } - } + for ((validator, pubkey, subnet), proof) in seats.into_iter().zip(proofs) { + match proof { + Ok(proof) if is_sync_committee_aggregator(&proof) => { + selected.push((validator, pubkey, subnet, proof)); + } + Ok(_) => {} + Err(err) => { + error!(%slot, validator, %err, "Failed to sign sync selection proof"); + crate::metrics::inc_signing_failures(); } } } @@ -258,27 +276,33 @@ impl SyncCommitteeService { fetched.insert(*subnet, answer); } - let mut signed = Vec::new(); - { - let store = store.read().await; - for (validator, pubkey, subnet, proof) in selected { - let Some(Some(contribution)) = fetched.get(&subnet) else { - continue; - }; - let message = ContributionAndProof { - aggregator_index: validator, - contribution: contribution.clone(), - selection_proof: proof, - }; - match self - .context - .sign_contribution_and_proof(&store, &pubkey, &message) - { - Ok(signature) => signed.push(SignedContributionAndProof { message, signature }), - Err(err) => { - error!(%slot, validator, %err, "Failed to sign contribution and proof"); - crate::metrics::inc_signing_failures(); - } + let mut to_sign = Vec::new(); + for (validator, pubkey, subnet, proof) in selected { + let Some(Some(contribution)) = fetched.get(&subnet) else { + continue; + }; + let message = ContributionAndProof { + aggregator_index: validator, + contribution: contribution.clone(), + selection_proof: proof, + }; + to_sign.push((pubkey, message)); + } + let signatures = sign_all(to_sign.iter().map(|(pubkey, message)| { + ( + *pubkey, + self.context + .sign_contribution_and_proof(store, pubkey, message), + ) + })) + .await; + let mut signed = Vec::with_capacity(to_sign.len()); + for ((_, message), signature) in to_sign.into_iter().zip(signatures) { + match signature { + Ok(signature) => signed.push(SignedContributionAndProof { message, signature }), + Err(err) => { + error!(%slot, validator = message.aggregator_index, %err, "Failed to sign contribution and proof"); + crate::metrics::inc_signing_failures(); } } } @@ -327,10 +351,7 @@ mod tests { } fn context() -> Arc { - Arc::new(SigningContext { - config: Config::mainnet(), - genesis_validators_root: Root::ZERO, - }) + Arc::new(SigningContext::new(Config::mainnet(), Root::ZERO)) } fn store() -> (RwLock, BlsPubkey) { @@ -452,16 +473,13 @@ mod tests { /// Duty with a seat in every subnet, on the first slot where the selection /// is a mix, so "only selected pairs fetch" is not satisfied by all or none. - fn mixed_slot(context: &SigningContext, store: &ValidatorStore, pubkey: &BlsPubkey) -> Slot { + async fn mixed_slot( + context: &SigningContext, + store: &RwLock, + pubkey: &BlsPubkey, + ) -> Slot { for slot in 3200..4200 { - let picks = (0..4) - .filter(|subnet| { - let proof = context - .sign_sync_selection_proof(store, pubkey, slot, *subnet) - .expect("signs"); - is_sync_committee_aggregator(&proof) - }) - .count(); + let picks = picked_subnets(context, store, pubkey, slot).await.len(); if picks > 0 && picks < 4 { return slot; } @@ -469,6 +487,26 @@ mod tests { panic!("no mixed slot found"); } + /// The subnets whose selection proof selects this key at `slot`. + async fn picked_subnets( + context: &SigningContext, + store: &RwLock, + pubkey: &BlsPubkey, + slot: Slot, + ) -> Vec { + let mut picked = Vec::new(); + for subnet in 0..4 { + let proof = context + .sign_sync_selection_proof(store, pubkey, slot, subnet) + .await + .expect("signs"); + if is_sync_committee_aggregator(&proof) { + picked.push(subnet); + } + } + picked + } + fn all_subnet_duty(pubkey: &BlsPubkey) -> SyncDutyDto { let size = SYNC_SUBCOMMITTEE_SIZE as u64; duty(pubkey, 9, vec![0, size, 2 * size, 3 * size]) @@ -478,10 +516,7 @@ mod tests { async fn only_selected_pairs_fetch_and_a_published_contribution_verifies() { let (store, pubkey) = store(); let context = context(); - let slot = { - let guard = store.read().await; - mixed_slot(&context, &guard, &pubkey) - }; + let slot = mixed_slot(&context, &store, &pubkey).await; let root = Root::repeat_byte(4); let mut node = MockBeaconNode::new(); for subnet in 0..4 { @@ -495,15 +530,7 @@ mod tests { .await .expect("aggregates"); - let guard = store.read().await; - let expected: Vec = (0..4) - .filter(|subnet| { - let proof = context - .sign_sync_selection_proof(&guard, &pubkey, slot, *subnet) - .expect("signs"); - is_sync_committee_aggregator(&proof) - }) - .collect(); + let expected = picked_subnets(&context, &store, &pubkey, slot).await; let requested: Vec = node .contribution_requests() .iter() @@ -535,10 +562,7 @@ mod tests { async fn a_mismatched_contribution_is_not_published() { let (store, pubkey) = store(); let context = context(); - let slot = { - let guard = store.read().await; - mixed_slot(&context, &guard, &pubkey) - }; + let slot = mixed_slot(&context, &store, &pubkey).await; let root = Root::repeat_byte(4); let mut node = MockBeaconNode::new(); for subnet in 0..4 { @@ -561,22 +585,15 @@ mod tests { let (store, pubkey) = store(); let context = context(); // A slot where at least two subnets are selected. - let (slot, picked) = { - let guard = store.read().await; - (3200..6000) - .find_map(|slot| { - let picked: Vec = (0..4) - .filter(|subnet| { - let proof = context - .sign_sync_selection_proof(&guard, &pubkey, slot, *subnet) - .expect("signs"); - is_sync_committee_aggregator(&proof) - }) - .collect(); - (picked.len() >= 2).then_some((slot, picked)) - }) - .expect("a slot selecting two subnets") - }; + let mut found = None; + for slot in 3200..6000 { + let picked = picked_subnets(&context, &store, &pubkey, slot).await; + if picked.len() >= 2 { + found = Some((slot, picked)); + break; + } + } + let (slot, picked) = found.expect("a slot selecting two subnets"); let root = Root::repeat_byte(4); // Only the last picked subnet has a contribution; the first answers 404. let kept = *picked.last().expect("nonempty"); diff --git a/docs/cli.md b/docs/cli.md index e77aa426a..83df863b3 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -286,14 +286,15 @@ implementation's, so none of the [common flags](#common-flags) apply to it. | Flag | Default | Meaning | |---|---|---| | `--beacon-nodes` | required | Base URLs of the beacon nodes to use, comma-separated or repeated. Tried in list order; the first that answers serves the request, so the order is a preference, not load balancing | -| `--validators-dir` | required | Directory holding the EIP-2335 keystores and `validator_definitions.yml`. Without that file, keystores in the Lighthouse layout (`<0xpubkey>/voting-keystore.json`, password in `--secrets-dir/<0xpubkey>`) are discovered and the file is written | +| `--validators-dir` | required | Directory holding the EIP-2335 keystores and `validator_definitions.yml`, which also lists the keys a remote signer holds (see [Remote signing](#remote-signing-web3signer)). Without that file, keystores in the Lighthouse layout (`<0xpubkey>/voting-keystore.json`, password in `--secrets-dir/<0xpubkey>`) are discovered and the file is written | | `--secrets-dir` | required | Directory holding one password file per keystore, named after the validator's public key | | `--http-address` | `127.0.0.1` | Bind address for the metrics and keymanager servers | | `--metrics-port` | `5064` | Prometheus metrics port | | `--keymanager-port` | `5062` | Keymanager API port. Only bound with `--enable-keymanager` | | `--suggested-fee-recipient` | none | Execution address to receive block rewards, `0x`-prefixed, for every validator the keymanager API gives no address of its own. Optional, and startup warns when it is absent: without it the beacon node picks an address, and it will not be yours | | `--graffiti` | empty | Text for the graffiti field of proposed blocks, for every validator the keymanager API gives no graffiti of its own. At most 32 bytes as UTF-8, right-padded with zeros. Refused rather than truncated if longer. An ethlambda beacon node appends both clients' versions to it (see [`rpc.md`](rpc.md#validator-endpoints)) | -| `--enable-keymanager` | off | Serve the keymanager API: keystores, plus each validator's own fee recipient, graffiti and gas limit. Off by default because it mutates key material | +| `--enable-keymanager` | off | Serve the keymanager API: keystores, remote keys, plus each validator's own fee recipient, graffiti and gas limit. Off by default because it mutates key material | +| `--web3signer-url` | none | Base URL of a Web3Signer (`https://signer:9000`): the signer a remote key imported through `POST /eth/v1/remotekeys` without a `url` of its own is assigned to. Checked for shape at startup. Keys whose definitions entry names a signer need no default | | `--init-slashing-protection` | off | Register every loaded key with no slashing-protection record as never having signed, instead of refusing to start. Dangerous: only for keys that have signed nothing anywhere for at least two epochs, and even then it rules out a double vote only, not a surround of an older vote; a key with a history should bring it instead. Never lowers an existing record; remove it once the keys are registered, and startup warns while it is set for nothing | ### What it does today @@ -407,6 +408,73 @@ attester duties untouched can still move a proposer between slots. This client fetches proposer duties once per epoch, at the boundary, so a reorg inside the epoch is not picked up until the next one. +### Remote signing (Web3Signer) + +A validator's key can stay in a [Web3Signer](https://docs.web3signer.consensys.io/) +instead of this process. Each signature is then one `POST +/api/v1/eth2/sign/{pubkey}` carrying the message in full (the attestation +data, the block header, the aggregate, and so on), the fork it is signed under +and the signing root, and the signer answers with the signature. Every duty +this client performs signs this way when the key is remote, the gloas envelope +and payload attestation included; a slot's attestations are requested +concurrently, so a hundred keys behind one signer cost one round trip rather +than a hundred in series. + +Because the message travels in full, the signer's own slashing protection +applies to it, on top of this client's: a remote key has a record in +[`slashing_protection.json`](#slashing-protection) like a local one, and a +block or attestation is checked against it and recorded before the signer is +asked. So a remote key written into the definitions file by hand is refused at +startup until it has a record, the same as a local keystore; one imported +through the keymanager API is registered as it is imported. The two records +are separate (see +[Spec Deviations](./spec_deviations.md#the-validator-client-keeps-eip-3076s-minimal-watermarks-not-full-history)): +a key moved to or from a signer carries its history through the signer's own +tools. A signer refusal (`412`) is logged, counted as a +signing failure and never retried; so is a key the signer does not hold, a +signer that cannot be reached, and an answer that does not verify: every +signature a signer returns is checked against the validator's public key and +the signing root before it is used, so a signer holding another key under that +pubkey, or computing another root, costs the signature rather than publishing +one no peer accepts. Each duty asks for its signatures all at once +and gives each one 2 seconds: a signer that has not answered by then costs the +signatures it owes and nothing else, so the duty publishes whatever arrived, +local keys and other signers' keys included. Aggregator selection proofs are +remembered once produced, so the one the epoch's subnet subscription asked for +is not asked for again by the aggregation. Nothing is asked of the signer at startup or at +import: a signer may start after this client does. + +A remote key is declared in `validator_definitions.yml` with `type: +web3signer` and a `url` where a local entry has `type: local_keystore` and its +two keystore paths: + +```yaml +- enabled: true + voting_public_key: "0x9612…0d07" + type: web3signer + url: https://signer:9000 +``` + +or imported at runtime through `POST /eth/v1/remotekeys` (below). A `url` +left out of that request means `--web3signer-url`. An entry written without a +`type` (every entry from before remote signing) is read by its fields; one +naming both keystore paths and a `url` is refused at startup rather than read +as either. So is a `web3signer` entry carrying a field this client does not +implement, such as Lighthouse's `root_certificate_path`: skipping it would sign +through a connection the operator did not describe. `description` is the one +exception, since Lighthouse writes it on every entry. A local entry is read +past such fields, so a file Lighthouse wrote for local keystores loads as it +is; the fields are not kept when the file is next rewritten. The `url` must not +carry credentials, a query string or a fragment. + +The signer is reached over HTTP or HTTPS. HTTPS is verified against the Mozilla +root store bundled into the binary (`webpki-roots`), **not** the operating +system's trust store, and no other CA can be added: a signer behind an internal +or self-signed CA fails every TLS handshake, and every signature for its keys +is then counted as a signing failure. Client certificates are not supported +either. Put such a signer behind a certificate from a public CA, or reach it +over plain HTTP on a private network. + ### The keymanager API Off unless `--enable-keymanager` is passed. It serves `GET`, `POST` and @@ -415,6 +483,19 @@ Off unless `--enable-keymanager` is passed. It serves `GET`, `POST` and if absent. The token file is written `0600` and a file too short to be a real token is refused rather than accepted. +It serves `GET`, `POST` and `DELETE /eth/v1/remotekeys` for the keys a +Web3Signer holds. The two routes partition the store: `/keystores` lists and +deletes local keys only, `/remotekeys` remote keys only, and a key asked of the +wrong route is `not_found`. An import of a key either route already holds is +`duplicate`. `POST /remotekeys` persists each key to the definitions file and +registers it in the slashing-protection record before it starts signing, the +same order the keystores import uses, and `readonly` is always `false`: this +client has no key it cannot delete. These routes carry no slashing-protection +history, since the specification gives them no field for it: a remote key +imported with no record starts as never having signed, and `DELETE +/remotekeys` leaves the record in place. `DELETE /keystores` exports nothing +for a key a remote signer holds. + It also serves `GET`, `POST` and `DELETE` on `/eth/v1/validator/{pubkey}/feerecipient`, `/graffiti` and `/gas_limit`, which give one validator its own value in place of the command line's default: diff --git a/docs/spec_deviations.md b/docs/spec_deviations.md index 49ee15c03..263e0c7ed 100644 --- a/docs/spec_deviations.md +++ b/docs/spec_deviations.md @@ -114,6 +114,14 @@ anything that does not advance past them. slot is equivocation that peers penalize in gossip, not a slashing. - **What is not covered:** the same keys running in another client on another machine. No local record can see that. +- **A key held by a remote signer:** a key in a Web3Signer (see + [Remote signing](./cli.md#remote-signing-web3signer)) is registered in this + record and checked against it like any other, and is then signed by the + signer, which receives every message in full and keeps a record of its own; + a message it refuses is dropped here and never retried. The two records are + separate: the signer's is neither imported into this one nor exported from + it, so a key moved to or from a signer carries its history through the + signer's own tools. - **Interchange:** an import (`ethlambda slashing-protection import`, or the keymanager API's `POST /eth/v1/keystores`) reduces each validator to its highest slot, source and target and merges them with the record by maximum,