From 39b69c29717642c6bb285cf5e3fe7d91c6f7e79f Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Wed, 4 Nov 2026 18:23:34 -0300 Subject: [PATCH 1/5] Add Web3Signer remote signing and the keymanager's /eth/v1/remotekeys route to the validator client. A validator's key can now stay in a Web3Signer: each signature is a POST /api/v1/eth2/sign/{pubkey} carrying the message in full, the fork it is signed under and the signing root, so the signer's own slashing protection applies to it. Every signature the client produces is covered, the gloas envelope and payload attestation included. Every SigningContext::sign_* is async over the shared store lock: it resolves the signer under a read guard scoped to the lookup and signs after releasing it, because a remote signature is an HTTP round trip and the lock is write-preferring. A slot's attestations are signed concurrently so many keys behind one signer cost one round trip. Block signing takes the header and aggregate signing the AggregateAndProof, since the signer reads them. validator_definitions.yml stays backward compatible: a remote entry carries `url` where a local one has its keystore paths. /eth/v1/keystores and /eth/v1/remotekeys partition the store, each listing and deleting its own kind. --web3signer-url is the default signer for a remote key imported without a url of its own. A remote key is covered by this client's EIP-3076 record like a local one, beside the signer's own: the record is opened over every loaded key, a block or attestation is checked and recorded before the signer is asked, and POST /eth/v1/remotekeys registers its keys in one durable write before any is loaded, taking its entries off the file again when that write fails. DELETE /eth/v1/keystores answers `not_found` for a key the remote route holds and exports nothing for it. --- CLAUDE.md | 10 +- Cargo.lock | 1 + bin/ethlambda/src/validator.rs | 10 + crates/validator/Cargo.toml | 3 + crates/validator/src/aggregation.rs | 112 ++- crates/validator/src/aggregation_selection.rs | 9 +- crates/validator/src/attestation.rs | 92 ++- .../src/beacon_node/block_contents.rs | 27 + crates/validator/src/error.rs | 14 +- crates/validator/src/http_api/keystores.rs | 162 +++- crates/validator/src/http_api/mod.rs | 10 + crates/validator/src/http_api/proposer.rs | 1 + crates/validator/src/http_api/remotekeys.rs | 709 ++++++++++++++++++ crates/validator/src/keys/definitions.rs | 114 ++- crates/validator/src/keys/mod.rs | 4 +- crates/validator/src/keys/store.rs | 226 +++++- crates/validator/src/keys/web3signer.rs | 546 ++++++++++++++ crates/validator/src/lib.rs | 11 + crates/validator/src/payload_attestation.rs | 4 +- crates/validator/src/proposal.rs | 45 +- crates/validator/src/signing.rs | 567 +++++++++++--- crates/validator/src/subscriptions.rs | 57 +- crates/validator/src/sync_committee.rs | 92 ++- docs/cli.md | 54 +- docs/spec_deviations.md | 8 + 25 files changed, 2517 insertions(+), 371 deletions(-) create mode 100644 crates/validator/src/http_api/remotekeys.rs create mode 100644 crates/validator/src/keys/web3signer.rs diff --git a/CLAUDE.md b/CLAUDE.md index d08ee404f..33a1b4dd3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -849,7 +849,15 @@ 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. Remote keys are `url` + 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..297c9eb9e 100644 --- a/bin/ethlambda/src/validator.rs +++ b/bin/ethlambda/src/validator.rs @@ -91,6 +91,14 @@ pub(crate) struct ValidatorOptions { /// one; remove it once the keys are registered. #[arg(long, default_value = "false")] pub(crate) init_slashing_protection: 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, } impl ValidatorOptions { @@ -168,6 +176,7 @@ pub(crate) async fn run(options: ValidatorOptions) -> eyre::Result<()> { .enable_keymanager .then(|| std::net::SocketAddr::new(options.http_address, options.keymanager_port)), init_slashing_protection: options.init_slashing_protection, + web3signer_url: options.web3signer_url, }) .await .map_err(Into::into) @@ -189,6 +198,7 @@ mod tests { suggested_fee_recipient: None, enable_keymanager: false, init_slashing_protection: false, + web3signer_url: None, } } 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..2cd9931b3 100644 --- a/crates/validator/src/aggregation.rs +++ b/crates/validator/src/aggregation.rs @@ -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}; /// An `AggregateAndProof` of either fork, held between building it and /// signing it. @@ -91,30 +91,19 @@ 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 - .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() - }; + let mut selected: Vec<(AttesterDutyDto, _)> = Vec::new(); + for duty in duties { + match selection_for(&self.context, store, duty).await { + Ok(Some(proof)) => selected.push((duty.clone(), proof)), + Ok(None) => {} + Err(err) => warn!( + %slot, + validator = duty.validator_index, + %err, + "Could not compute aggregator selection; skipping this duty" + ), + } + } if selected.is_empty() { return Ok(0); @@ -162,7 +151,6 @@ impl AggregationService { let fork = self.batch_fork(&aggregates, slot)?; 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 { @@ -177,34 +165,35 @@ impl AggregationService { } }; - // 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 { + // 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) => { - 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 { + Wrapper::Electra(electra::AggregateAndProof { aggregator_index: duty.validator_index, aggregate: attestation.clone(), selection_proof: *selection_proof, - }; - (message.hash_tree_root(), Wrapper::Gloas(message)) + }) } + AggregateKind::Gloas(attestation) => Wrapper::Gloas(gloas::AggregateAndProof { + aggregator_index: duty.validator_index, + aggregate: attestation.clone(), + selection_proof: *selection_proof, + }), }; // 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 message = match &wrapper { + Wrapper::Electra(message) => AggregateAndProofRef::Electra(message), + Wrapper::Gloas(message) => AggregateAndProofRef::Gloas(message), + }; let signature = match self .context - .sign_aggregate_and_proof(&store, &pubkey, root, slot) + .sign_aggregate_and_proof(store, &pubkey, message) + .await { Ok(signature) => signature, Err(err) => { @@ -333,13 +322,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 +348,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 +476,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!( @@ -539,16 +526,18 @@ mod tests { /// 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 +549,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..47938d1db 100644 --- a/crates/validator/src/attestation.rs +++ b/crates/validator/src/attestation.rs @@ -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,60 @@ 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; - { + 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. + let signatures = { 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; + futures::future::join_all( + to_sign + .iter() + .map(|(_, 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() { 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..02991c162 100644 --- a/crates/validator/src/http_api/keystores.rs +++ b/crates/validator/src/http_api/keystores.rs @@ -51,7 +51,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,7 +121,7 @@ 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()), @@ -142,11 +142,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), @@ -503,7 +505,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<()> { @@ -680,8 +682,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. // @@ -700,10 +704,6 @@ fn persist_import( }, } - // Re-importing an already-known key updates its entry in place instead - // of appending a duplicate: `voting_public_key` is this file's natural - // key, and a duplicate would make `ValidatorStore::load` process the - // same keystore twice on the next restart. let undo = match definitions .0 .iter() @@ -751,6 +751,48 @@ fn persist_import( } } +/// Record `definition` in the definitions file, replacing any entry for the +/// same key, and save it. For the remote import, which writes no key files +/// and so has none to put back. +/// +/// A failed save puts `definitions` back as it was, for the reason +/// `persist_import` gives: one vector is shared by every key in the batch, +/// and a failed key's entry left in it would be written by the next key's +/// save. A save whose directory flush failed counts as persisted, as it does +/// there. +pub(super) fn upsert_definition( + context: &KeymanagerContext, + definitions: &mut ValidatorDefinitions, + definition: ValidatorDefinition, +) -> crate::Result<()> { + let position = definitions + .0 + .iter() + .position(|existing| existing.voting_public_key == definition.voting_public_key); + let previous = match position { + Some(index) => Some(( + index, + std::mem::replace(&mut definitions.0[index], definition), + )), + None => { + definitions.0.push(definition); + None + } + }; + match save_definitions(context, definitions) { + Ok(()) | Err(Error::Unflushed { .. }) => Ok(()), + Err(err) => { + match previous { + Some((index, previous)) => definitions.0[index] = previous, + None => { + definitions.0.pop(); + } + } + Err(err) + } + } +} + fn absolute(path: PathBuf) -> crate::Result { std::path::absolute(&path).map_err(|source| Error::Io { path: path.display().to_string(), @@ -774,7 +816,20 @@ 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(); @@ -860,23 +915,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 @@ -943,24 +1020,32 @@ fn delete_one( } } -/// Whether the definitions file currently holds an entry for `pubkey`. +/// 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)) } /// Drop a deleted key's entry from the definitions file. @@ -969,7 +1054,7 @@ fn definitions_contain(context: &KeymanagerContext, pubkey: &BlsPubkey) -> crate /// 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) -> crate::Result<()> { let mut definitions = ValidatorDefinitions::open(&context.validators_dir)?; definitions.0.retain(|definition| { parse_pubkey(&definition.voting_public_key) @@ -1028,6 +1113,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 +1266,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()]); @@ -1485,8 +1573,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 +1644,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 +1700,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..1c66ddb15 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; @@ -66,6 +67,9 @@ pub struct KeymanagerContext { /// The record every key must be registered in before it is loaded, so the /// duty path never meets a key it has no record for. pub protection: crate::slashing_protection::SharedProtection, + /// `--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, } /// The file the generated token is written to, inside the validators directory. @@ -142,6 +146,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..856e392c5 100644 --- a/crates/validator/src/http_api/proposer.rs +++ b/crates/validator/src/http_api/proposer.rs @@ -369,6 +369,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, }; Harness { app: router(context, TOKEN.to_string()), diff --git a/crates/validator/src/http_api/remotekeys.rs b/crates/validator/src/http_api/remotekeys.rs new file mode 100644 index 000000000..e20a1e026 --- /dev/null +++ b/crates/validator/src/http_api/remotekeys.rs @@ -0,0 +1,709 @@ +//! `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. + +use axum::{Json, extract::State}; +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, StatusResponse, definitions_entry, persist_delete, save_definitions, + upsert_definition, +}; +use crate::keys::{SigningDefinition, ValidatorDefinition, ValidatorDefinitions, ValidatorStore}; + +#[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 }) +} + +pub async fn import( + State(context): State, + Json(request): Json, +) -> Json { + // Held across the whole open-mutate-save cycle over the definitions file, + // and taken before `store`'s write lock, never the other way around: the + // same order every keymanager handler uses. See + // `KeymanagerContext::definitions_lock`. + let _definitions_guard = context.definitions_lock.lock().await; + + let mut definitions = match ValidatorDefinitions::open(&context.validators_dir) { + Ok(definitions) => definitions, + Err(err) => { + let message = err.to_string(); + let data = request + .remote_keys + .iter() + .map(|_| KeyStatus::error(&message)) + .collect(); + return Json(StatusResponse { data }); + } + }; + + // Everything that can be decided and persisted before the store's write + // lock is needed. Duplicates are judged against a read of the store: + // nothing between here and the insert below can add a key, since this + // handler and the keystores import both hold `definitions_lock`. + let mut prepared = Vec::with_capacity(request.remote_keys.len()); + { + let store = context.store.read().await; + for key in &request.remote_keys { + prepared.push(prepare_import(&context, &store, &mut definitions, key)); + } + } + + // Every persisted 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 persisting rather than before, + // for the reason `keystores::import` gives. A remote signer keeps a + // record of its own; this one is kept as well. + let persisted: Vec = prepared + .iter() + .filter_map(|result| match result { + Ok(Some((pubkey, _))) => Some(*pubkey), + _ => None, + }) + .collect(); + let mut failure = None; + if !persisted.is_empty() { + let registering = persisted.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) => { + definitions.0.retain(|definition| { + parse_pubkey(&definition.voting_public_key) + .map(|parsed| !persisted.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}" + ) + } + }); + } + } + } + drop(_definitions_guard); + + let mut store = context.store.write().await; + let data = prepared + .into_iter() + .map(|result| match (result, &failure) { + (Ok(Some(_)), Some(message)) => KeyStatus::error(message), + (result, _) => match result { + Ok(Some((pubkey, url))) => finish_import(&mut store, pubkey, &url), + Ok(None) => KeyStatus { + status: "duplicate".to_string(), + message: None, + }, + Err(status) => status, + }, + }) + .collect(); + drop(store); + + Json(StatusResponse { data }) +} + +/// Everything one import does before it needs the store's write lock: +/// resolve the URL, refuse a key already held, and persist the definition. +/// +/// `Ok(None)` is a duplicate. +fn prepare_import( + context: &KeymanagerContext, + store: &ValidatorStore, + definitions: &mut ValidatorDefinitions, + key: &ImportRemoteKey, +) -> std::result::Result, KeyStatus> +{ + 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", + )); + } + }; + // Refused for shape before anything is written: an unusable URL would + // otherwise be persisted and refuse the next startup. + crate::keys::Web3Signer::new(&url).map_err(KeyStatus::error)?; + + if store.contains(&pubkey) { + return Ok(None); + } + + // Persisted before the key is reachable through the store, for the reason + // the keystores import does it in this order: a validator this process + // is willing to sign with must already be durable. + let definition = ValidatorDefinition { + enabled: true, + voting_public_key: encode_hex(&pubkey.0), + signing: SigningDefinition::Web3Signer { url: url.clone() }, + }; + upsert_definition(context, definitions, definition).map_err(KeyStatus::error)?; + + Ok(Some((pubkey, url))) +} + +/// The one step of an import that needs the write lock: a map insert. +/// +/// The duplicate check runs again here, under the write lock this time. The +/// read-side check in `prepare_import` ran under `definitions_lock`, but that +/// lock was released before this one was taken, and a keystores import can +/// land in between; replacing the local key it just put in the store with a +/// remote entry would be the one outcome neither request asked for. +fn finish_import( + store: &mut ValidatorStore, + pubkey: ethlambda_types::beacon::primitives::BlsPubkey, + url: &str, +) -> KeyStatus { + if store.contains(&pubkey) { + return KeyStatus { + status: "duplicate".to_string(), + message: None, + }; + } + match store.insert_remote(pubkey, url) { + Ok(()) => { + info!(pubkey = %encode_hex(&pubkey.0), url, "Imported remote validator key"); + KeyStatus { + status: "imported".to_string(), + message: None, + } + } + Err(err) => KeyStatus::error(err), + } +} + +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 { + 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); + if let Err(err) = persist_delete(context, pubkey) { + 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; + assert_eq!(statuses(&body), ["not_found"]); + 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 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..8418a4650 100644 --- a/crates/validator/src/keys/definitions.rs +++ b/crates/validator/src/keys/definitions.rs @@ -21,10 +21,38 @@ pub struct ValidatorDefinition { 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. Flattened, so a local entry keeps + /// the two path fields it always had at the top level and a remote one + /// has `url` there instead. + #[serde(flatten)] + pub signing: SigningDefinition, +} + +/// How one definition's key signs. +/// +/// Untagged rather than carrying a `type` field: the file predates remote +/// signers, and every existing entry has the two keystore paths and nothing +/// else, so the fields themselves are the tag. Deserialization tries the +/// variants in order, and the field sets are disjoint, so an entry cannot +/// match both. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(untagged)] +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 }, +} + +impl SigningDefinition { + pub fn is_remote(&self) -> bool { + matches!(self, Self::Web3Signer { .. }) + } } #[derive(Debug, Clone, Default, Serialize, Deserialize)] @@ -130,9 +158,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 +216,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 +249,72 @@ 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:"), + "the variant is told by its fields, not a tag: {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()); + } + #[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..4c2f38763 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; 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,34 @@ 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. + signers: HashMap>, } impl ValidatorStore { pub fn new() -> Self { Self { validators: HashMap::new(), + signers: HashMap::new(), } } @@ -45,14 +62,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 +163,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 +184,46 @@ impl ValidatorStore { self.validators.insert( pubkey, SigningMethod::LocalKeystore { - secret_key: Box::new(secret_key), + secret_key: Arc::new(secret_key), }, ); + self.drop_unused_signers(); Ok(pubkey) } + /// Add a validator whose key a Web3Signer at `url` holds, replacing any + /// method the store had for it. + /// + /// 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. + pub fn insert_remote(&mut self, pubkey: BlsPubkey, url: &str) -> Result<()> { + // Keyed by the URL as the client normalizes it, so `.../` and `...` + // name one signer rather than two. + let key = url.trim_end_matches('/'); + let signer = match self.signers.get(key) { + Some(signer) => signer.clone(), + None => { + let signer = Arc::new(Web3Signer::new(url)?); + self.signers.insert(key.to_string(), signer.clone()); + signer + } + }; + self.validators + .insert(pubkey, SigningMethod::Web3Signer { signer }); + self.drop_unused_signers(); + Ok(()) + } + + /// Forget any signer client no validator uses any more. Called after + /// every mutation, since each can orphan one: an insert that replaces a + /// remote entry with a local one, a remove, a remote insert on a new URL + /// for a key that was the last on its old one. + fn drop_unused_signers(&mut self) { + self.signers + .retain(|_, signer| Arc::strong_count(signer) > 1); + } + /// Derive a validator's public key from its raw secret, without adding it /// to the store. /// @@ -162,21 +238,55 @@ impl ValidatorStore { } pub fn remove(&mut self, pubkey: &BlsPubkey) -> bool { - self.validators.remove(pubkey).is_some() + let removed = self.validators.remove(pubkey).is_some(); + self.drop_unused_signers(); + removed } pub fn contains(&self, pubkey: &BlsPubkey) -> bool { 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 +330,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 +388,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 +416,90 @@ 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()); + } + + #[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!(store.signers.len(), 1, "one URL, one client"); + + assert!(store.remove(&first)); + assert_eq!(store.signers.len(), 1, "still in use by the second key"); + assert!(store.remove(&second)); + assert!(store.signers.is_empty(), "nobody uses it any more"); + } + + #[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!(store.signers.is_empty()); + } + /// A definitions entry declaring a key its keystore does not hold must not /// load. /// @@ -388,6 +577,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..abbc718ee --- /dev/null +++ b/crates/validator/src/keys/web3signer.rs @@ -0,0 +1,546 @@ +//! 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. + pub fn new(url: &str) -> Result { + let base_url = url.trim_end_matches('/').to_string(); + let parsed = reqwest::Url::parse(&base_url).map_err(|err| Error::Keystore { + path: base_url.clone(), + reason: format!("not a valid remote signer URL: {err}"), + })?; + if parsed.scheme() != "http" && parsed.scheme() != "https" { + return Err(Error::Keystore { + path: base_url, + reason: "a remote signer URL must be http or https".to_string(), + }); + } + let client = reqwest::Client::builder() + .timeout(REQUEST_TIMEOUT) + .build() + .map_err(|err| Error::Keystore { + path: base_url.clone(), + reason: 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, + body.trim() + ))); + } + + parse_signature_body(&body) + .map_err(|err| failed(format!("remote signer {}: {err}", self.base_url))) + } +} + +/// 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 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()); + assert!(Web3Signer::new("signer:9000").is_err()); + assert!(Web3Signer::new("not a url").is_err()); + } +} diff --git a/crates/validator/src/lib.rs b/crates/validator/src/lib.rs index 24fc99dfd..b198a031b 100644 --- a/crates/validator/src/lib.rs +++ b/crates/validator/src/lib.rs @@ -93,6 +93,10 @@ pub struct ValidatorConfig { /// signed, instead of refusing to start. See /// [`slashing_protection::SlashingProtection::open`]. pub init_slashing_protection: bool, + /// `--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, } /// Run the validator client until the process is stopped. @@ -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"); @@ -226,6 +236,7 @@ pub async fn run(config: ValidatorConfig) -> Result<()> { definitions_lock: Arc::new(Mutex::new(())), definitions_unflushed: Arc::new(std::sync::atomic::AtomicBool::new(false)), protection: protection.clone(), + default_remote_signer: config.web3signer_url.clone(), }; let router = http_api::router(context, token); let listener = tokio::net::TcpListener::bind(address) diff --git a/crates/validator/src/payload_attestation.rs b/crates/validator/src/payload_attestation.rs index 0870c038a..f08295c83 100644 --- a/crates/validator/src/payload_attestation.rs +++ b/crates/validator/src/payload_attestation.rs @@ -120,7 +120,6 @@ 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, @@ -138,7 +137,8 @@ impl PayloadAttestationService { match self .context - .sign_payload_attestation(&store, &pubkey, &data) + .sign_payload_attestation(store, &pubkey, &data) + .await { Ok(signature) => messages.push(PayloadAttestationMessage { validator_index: duty.validator_index, diff --git a/crates/validator/src/proposal.rs b/crates/validator/src/proposal.rs index c16319f07..ded84cb0f 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 header = produced.header(); let block_root = produced.block_root(); + debug_assert_eq!( + header.hash_tree_root(), + block_root, + "a header merkleizes to its block's 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 _; @@ -860,7 +855,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; diff --git a/crates/validator/src/signing.rs b/crates/validator/src/signing.rs index 9317ab341..b968d6536 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,9 +29,17 @@ use ethlambda_types::beacon::signing::{ compute_domain, compute_epoch_at_slot, compute_signing_root, }; +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. @@ -192,38 +201,90 @@ 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 } => { + let request = SigningRequest::new(self.fork_info(epoch), root, body); + signer.sign(pubkey, &request).await + } } } /// 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 +293,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 +317,26 @@ 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( + 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)) + self.sign( + store, + pubkey, + self.selection_proof_signing_root(slot), + compute_epoch_at_slot(slot), + SigningBody::AggregationSlot { + aggregation_slot: AggregationSlot { slot }, + }, + ) + .await } - /// Sign the aggregate whose `AggregateAndProof` root is `root`, for `slot`, - /// on behalf of `pubkey`. + /// 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 +344,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 +382,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 +409,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 +435,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 +515,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 } } @@ -417,17 +574,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); @@ -480,11 +637,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 +659,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 +691,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 +807,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 +833,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 +894,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 +920,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 +940,7 @@ mod tests { let signature = context .sign_contribution_and_proof(&store, &pubkey, &message) + .await .expect("signs"); assert!(verify( &pubkey, @@ -760,12 +949,220 @@ 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 fixed signature, or with a fixed status. + #[derive(Clone)] + struct Fake { + requests: Arc>>, + answer: Arc>, + } + + #[derive(Clone, Copy)] + enum Answer { + Signature, + Status(u16), + } + + 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"); + fake.requests.lock().expect("lock").push((pubkey, json)); + match *fake.answer.lock().expect("lock") { + Answer::Signature => ( + StatusCode::OK, + format!("{{\"signature\":\"0x{}\"}}", "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}")) + } + + fn remote_store(url: &str) -> (RwLock, BlsPubkey) { + let pubkey = BlsPubkey([0x42; 48]); + 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_eq!(signature.0, [0xab; 96]); + + let requests = fake.requests.lock().expect("lock"); + assert_eq!(requests.len(), 1); + let (path_pubkey, body) = &requests[0]; + assert_eq!(path_pubkey, &format!("0x{}", "42".repeat(48))); + 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.altair_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"], "ALTAIR"); + 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") + ); + } + + #[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..374d78bf1 100644 --- a/crates/validator/src/subscriptions.rs +++ b/crates/validator/src/subscriptions.rs @@ -47,37 +47,28 @@ 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 - .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() - }; + let mut subscriptions = Vec::with_capacity(duties.len()); + for duty in duties { + let is_aggregator = match selection_for(context, store, duty).await { + 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 + } + }; + subscriptions.push(CommitteeSubscriptionDto { + validator_index: duty.validator_index, + committee_index: duty.committee_index, + committees_at_slot: duty.committees_at_slot, + slot: duty.slot, + is_aggregator, + }); + } let aggregating = subscriptions .iter() @@ -236,10 +227,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..3b2149555 100644 --- a/crates/validator/src/sync_committee.rs +++ b/crates/validator/src/sync_committee.rs @@ -132,7 +132,6 @@ impl SyncCommitteeService { 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, @@ -146,7 +145,8 @@ impl SyncCommitteeService { } match self .context - .sign_sync_committee_message(&store, &pubkey, slot, root) + .sign_sync_committee_message(store, &pubkey, slot, root) + .await { Ok(signature) => messages.push(SyncCommitteeMessage { slot, @@ -194,7 +194,6 @@ impl SyncCommitteeService { // (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, @@ -206,7 +205,8 @@ impl SyncCommitteeService { for subnet in subnets_of(duty) { match self .context - .sign_sync_selection_proof(&store, &pubkey, slot, subnet) + .sign_sync_selection_proof(store, &pubkey, slot, subnet) + .await { Ok(proof) if is_sync_committee_aggregator(&proof) => { selected.push((duty.validator_index, pubkey, subnet, proof)); @@ -260,7 +260,6 @@ impl SyncCommitteeService { 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; @@ -272,7 +271,8 @@ impl SyncCommitteeService { }; match self .context - .sign_contribution_and_proof(&store, &pubkey, &message) + .sign_contribution_and_proof(store, &pubkey, &message) + .await { Ok(signature) => signed.push(SignedContributionAndProof { message, signature }), Err(err) => { @@ -452,16 +452,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 +466,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 +495,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 +509,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 +541,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 +564,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..4a801f96d 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,47 @@ 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, and a +signer that cannot be reached. 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 a `url` where a +local entry has its two keystore paths: + +```yaml +- enabled: true + voting_public_key: "0x9612…0d07" + url: https://signer:9000 +``` + +or imported at runtime through `POST /eth/v1/remotekeys` (below). A `url` +left out of that request means `--web3signer-url`. The signer is reached over +plain HTTPS with the system's trust roots; client certificates are not +supported. + ### The keymanager API Off unless `--enable-keymanager` is passed. It serves `GET`, `POST` and @@ -415,6 +457,14 @@ 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 +before it starts signing, the same order the keystores import uses, and +`readonly` is always `false`: this client has no key it cannot delete. + 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, From 647a8cad9a01fb4238324fcc10aed7eb6c1b3316 Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Wed, 4 Nov 2026 19:02:29 -0300 Subject: [PATCH 2/5] Keep the keystores and remotekeys routes from replacing each other's entries, and keep signer URLs and response bodies out of the logs. A keystore import for a key a remote signer holds, and a remote import for a key the definitions file already lists, now answer `duplicate` instead of overwriting the other route's entry; both are judged on the file under the definitions lock and again on the store under the write lock. A remote signer URL that carries credentials is refused, since the URL is named in every signing error, and a failed response's body is cut to 200 characters before it is reported. --- crates/validator/src/http_api/keystores.rs | 114 ++++++++++++++++++++ crates/validator/src/http_api/remotekeys.rs | 21 ++-- crates/validator/src/keys/web3signer.rs | 34 +++++- 3 files changed, 159 insertions(+), 10 deletions(-) diff --git a/crates/validator/src/http_api/keystores.rs b/crates/validator/src/http_api/keystores.rs index 02991c162..219c79beb 100644 --- a/crates/validator/src/http_api/keystores.rs +++ b/crates/validator/src/http_api/keystores.rs @@ -127,6 +127,14 @@ impl KeyStatus { 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)] @@ -559,6 +567,20 @@ 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 is `duplicate` here, the mirror of what + // `/eth/v1/remotekeys` answers for a local one: neither route silently + // replaces the other's entry. Re-importing a key this route already + // holds is still `imported`, since that is how a password is rotated. + let held_remotely = definitions.0.iter().any(|definition| { + definition.signing.is_remote() + && parse_pubkey(&definition.voting_public_key) + .map(|parsed| parsed == pubkey) + .unwrap_or(false) + }); + if held_remotely { + 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 @@ -603,6 +625,13 @@ fn regular_file_bytes(path: &Path) -> crate::Result>> { /// only step of an import that needs the write lock: a cheap, synchronous /// scalar validation and hashmap insert, never an await. fn finish_import(store: &mut ValidatorStore, secret: &[u8; 32]) -> KeyStatus { + // Checked again under the write lock: `prepare_import` judged the file, + // and a remote import can land between that and this insert. + match ValidatorStore::derive_pubkey(secret) { + Ok(pubkey) if store.remote_url(&pubkey).is_some() => return KeyStatus::duplicate(), + Ok(_) => {} + Err(err) => return KeyStatus::error(err), + } match store.insert_secret("keymanager import", secret) { Ok(pubkey) => { info!(pubkey = %encode_hex(&pubkey.0), "Imported validator key"); @@ -1067,10 +1096,15 @@ pub(super) fn persist_delete(context: &KeymanagerContext, pubkey: &BlsPubkey) -> #[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"; @@ -1452,6 +1486,86 @@ 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" + ); + } + /// 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. diff --git a/crates/validator/src/http_api/remotekeys.rs b/crates/validator/src/http_api/remotekeys.rs index e20a1e026..eebc7fce0 100644 --- a/crates/validator/src/http_api/remotekeys.rs +++ b/crates/validator/src/http_api/remotekeys.rs @@ -181,10 +181,7 @@ pub async fn import( (Ok(Some(_)), Some(message)) => KeyStatus::error(message), (result, _) => match result { Ok(Some((pubkey, url))) => finish_import(&mut store, pubkey, &url), - Ok(None) => KeyStatus { - status: "duplicate".to_string(), - message: None, - }, + Ok(None) => KeyStatus::duplicate(), Err(status) => status, }, }) @@ -222,7 +219,16 @@ fn prepare_import( // otherwise be persisted and refuse the next startup. crate::keys::Web3Signer::new(&url).map_err(KeyStatus::error)?; - if store.contains(&pubkey) { + // 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. + let on_disk = definitions.0.iter().any(|definition| { + parse_pubkey(&definition.voting_public_key) + .map(|parsed| parsed == pubkey) + .unwrap_or(false) + }); + if on_disk || store.contains(&pubkey) { return Ok(None); } @@ -252,10 +258,7 @@ fn finish_import( url: &str, ) -> KeyStatus { if store.contains(&pubkey) { - return KeyStatus { - status: "duplicate".to_string(), - message: None, - }; + return KeyStatus::duplicate(); } match store.insert_remote(pubkey, url) { Ok(()) => { diff --git a/crates/validator/src/keys/web3signer.rs b/crates/validator/src/keys/web3signer.rs index abbc718ee..f744deb63 100644 --- a/crates/validator/src/keys/web3signer.rs +++ b/crates/validator/src/keys/web3signer.rs @@ -58,6 +58,16 @@ impl Web3Signer { reason: "a remote signer URL must be http or https".to_string(), }); } + // 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(Error::Keystore { + path: parsed.host_str().unwrap_or("").to_string(), + reason: "a remote signer URL must not carry credentials".to_string(), + }); + } let client = reqwest::Client::builder() .timeout(REQUEST_TIMEOUT) .build() @@ -122,7 +132,7 @@ impl Web3Signer { return Err(failed(format!( "remote signer {} answered {status}: {}", self.base_url, - body.trim() + truncate(body.trim()) ))); } @@ -131,6 +141,17 @@ impl Web3Signer { } } +/// 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(), + } +} + /// 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. @@ -519,6 +540,13 @@ mod tests { ); } + #[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)); @@ -540,6 +568,10 @@ mod tests { "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()); } From 1b72c391e199cf203a8d50f2ebc8e1a3ad6cfc8d Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Fri, 6 Nov 2026 19:28:21 -0300 Subject: [PATCH 3/5] Address review of the Web3Signer PR: bound every duty's signing by a per-signature budget, and make the keymanager routes judge and change the file and the store together. Each duty now asks for its signatures at once through `signing::sign_all`, each given `SIGNING_BUDGET` (2s), so a hung signer costs only its own keys and the batch goes out with whatever arrived. Selection proofs are remembered in `SigningContext` once produced, so the aggregation tick reuses the subscription's. Selection, aggregate and subscription failures are now counted as signing failures. Both imports hold `definitions_lock` until the store insert and judge duplicates before writing anything; `persist_delete` only removes its own route's kind of entry. A remote import is capped at 1000 keys, saves the definitions once, builds signer clients outside the store lock, and the store holds those clients weakly. Definitions entries carry `type: local_keystore` / `type: web3signer`; an untyped entry naming both a keystore and a url, and any unknown field, is refused. Signer URL errors no longer echo credentials, and a URL with a query or fragment is refused. Remote block signing is refused before bellatrix (BLOCK_V2 wants the full block there). A proposal's block root is taken from its header. The docs now say TLS uses the bundled webpki roots, not the system's. Both deletes share `flush_earlier_removal`, so a retry after a removal whose directory flush failed is not told the key is gone before the removal is durable. --- CLAUDE.md | 5 +- bin/ethlambda/src/validator.rs | 20 +- .../rpc/src/beacon/validator_client_tests.rs | 10 +- crates/validator/src/aggregation.rs | 149 ++++--- crates/validator/src/attestation.rs | 38 +- crates/validator/src/http_api/keystores.rs | 229 +++++++---- crates/validator/src/http_api/mod.rs | 23 +- crates/validator/src/http_api/proposer.rs | 2 +- crates/validator/src/http_api/remotekeys.rs | 365 +++++++++++++----- crates/validator/src/keys/definitions.rs | 217 ++++++++++- crates/validator/src/keys/store.rs | 110 ++++-- crates/validator/src/keys/web3signer.rs | 101 ++++- crates/validator/src/lib.rs | 58 ++- crates/validator/src/payload_attestation.rs | 75 ++-- crates/validator/src/proposal.rs | 25 +- crates/validator/src/signing.rs | 190 ++++++++- crates/validator/src/subscriptions.rs | 57 ++- crates/validator/src/sync_committee.rs | 175 +++++---- docs/cli.md | 41 +- 19 files changed, 1344 insertions(+), 546 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 33a1b4dd3..43fbf3b4d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -855,7 +855,10 @@ transitions are in `ethlambda-types`, per the section above. Nothing above 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. Remote keys are `url` + registered in and checked against like a local one. 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 diff --git a/bin/ethlambda/src/validator.rs b/bin/ethlambda/src/validator.rs index 297c9eb9e..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. /// @@ -91,14 +99,6 @@ pub(crate) struct ValidatorOptions { /// one; remove it once the keys are registered. #[arg(long, default_value = "false")] pub(crate) init_slashing_protection: 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, } impl ValidatorOptions { @@ -175,8 +175,8 @@ pub(crate) async fn run(options: ValidatorOptions) -> eyre::Result<()> { keymanager: options .enable_keymanager .then(|| std::net::SocketAddr::new(options.http_address, options.keymanager_port)), - init_slashing_protection: options.init_slashing_protection, web3signer_url: options.web3signer_url, + init_slashing_protection: options.init_slashing_protection, }) .await .map_err(Into::into) @@ -197,8 +197,8 @@ mod tests { graffiti: graffiti.to_string(), suggested_fee_recipient: None, enable_keymanager: false, - init_slashing_protection: 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/src/aggregation.rs b/crates/validator/src/aggregation.rs index 2cd9931b3..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::{AggregateAndProofRef, SigningContext}; +use crate::signing::{AggregateAndProofRef, SigningContext, sign_all}; /// An `AggregateAndProof` of either fork, held between building it and /// signing it. @@ -91,17 +91,40 @@ impl AggregationService { } // Which of this client's validators aggregates, and with what proof. - let mut selected: Vec<(AttesterDutyDto, _)> = Vec::new(); + // 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 selection_for(&self.context, store, duty).await { - Ok(Some(proof)) => selected.push((duty.clone(), proof)), + 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() + .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" - ), + Err(err) => { + warn!( + %slot, + validator = duty.validator_index, + %err, + "Could not compute aggregator selection; skipping this duty" + ); + crate::metrics::inc_signing_failures(); + } } } @@ -115,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; } @@ -150,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 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 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: *selection_proof, - }) - } - AggregateKind::Gloas(attestation) => Wrapper::Gloas(gloas::AggregateAndProof { - aggregator_index: duty.validator_index, - aggregate: attestation.clone(), - selection_proof: *selection_proof, - }), - }; - // 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 message = match &wrapper { - Wrapper::Electra(message) => AggregateAndProofRef::Electra(message), - Wrapper::Gloas(message) => AggregateAndProofRef::Gloas(message), - }; - let signature = match self - .context - .sign_aggregate_and_proof(store, &pubkey, message) - .await - { + 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; } }; @@ -274,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) { @@ -518,10 +543,10 @@ 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 diff --git a/crates/validator/src/attestation.rs b/crates/validator/src/attestation.rs index 47938d1db..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. @@ -246,15 +246,19 @@ impl AttestationService { // 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. - let signatures = { - let _timing = crate::metrics::time_signing(); - futures::future::join_all( - to_sign - .iter() - .map(|(_, pubkey)| self.context.sign_attestation(store, pubkey, &data)), - ) - .await - }; + // + // 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) { @@ -343,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( @@ -832,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/http_api/keystores.rs b/crates/validator/src/http_api/keystores.rs index 219c79beb..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; @@ -311,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 @@ -555,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>, @@ -567,17 +588,19 @@ 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 is `duplicate` here, the mirror of what - // `/eth/v1/remotekeys` answers for a local one: neither route silently - // replaces the other's entry. Re-importing a key this route already - // holds is still `imported`, since that is how a password is rotated. - let held_remotely = definitions.0.iter().any(|definition| { + // 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 held_remotely { + if remote_on_disk || held_remotely.contains(&pubkey) { return Err(KeyStatus::duplicate()); } @@ -623,15 +646,10 @@ 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 { - // Checked again under the write lock: `prepare_import` judged the file, - // and a remote import can land between that and this insert. - match ValidatorStore::derive_pubkey(secret) { - Ok(pubkey) if store.remote_url(&pubkey).is_some() => return KeyStatus::duplicate(), - Ok(_) => {} - Err(err) => return KeyStatus::error(err), - } match store.insert_secret("keymanager import", secret) { Ok(pubkey) => { info!(pubkey = %encode_hex(&pubkey.0), "Imported validator key"); @@ -733,6 +751,10 @@ fn persist_import( }, } + // Re-importing an already-known key updates its entry in place instead + // of appending a duplicate: `voting_public_key` is this file's natural + // key, and a duplicate would make `ValidatorStore::load` process the + // same keystore twice on the next restart. let undo = match definitions .0 .iter() @@ -780,48 +802,6 @@ fn persist_import( } } -/// Record `definition` in the definitions file, replacing any entry for the -/// same key, and save it. For the remote import, which writes no key files -/// and so has none to put back. -/// -/// A failed save puts `definitions` back as it was, for the reason -/// `persist_import` gives: one vector is shared by every key in the batch, -/// and a failed key's entry left in it would be written by the next key's -/// save. A save whose directory flush failed counts as persisted, as it does -/// there. -pub(super) fn upsert_definition( - context: &KeymanagerContext, - definitions: &mut ValidatorDefinitions, - definition: ValidatorDefinition, -) -> crate::Result<()> { - let position = definitions - .0 - .iter() - .position(|existing| existing.voting_public_key == definition.voting_public_key); - let previous = match position { - Some(index) => Some(( - index, - std::mem::replace(&mut definitions.0[index], definition), - )), - None => { - definitions.0.push(definition); - None - } - }; - match save_definitions(context, definitions) { - Ok(()) | Err(Error::Unflushed { .. }) => Ok(()), - Err(err) => { - match previous { - Some((index, previous)) => definitions.0[index] = previous, - None => { - definitions.0.pop(); - } - } - Err(err) - } - } -} - fn absolute(path: PathBuf) -> crate::Result { std::path::absolute(&path).map_err(|source| Error::Io { path: path.display().to_string(), @@ -865,9 +845,9 @@ pub async fn delete( 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 @@ -990,24 +970,8 @@ fn delete_local( 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(), @@ -1027,7 +991,7 @@ fn delete_local( // 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 @@ -1049,6 +1013,32 @@ fn delete_local( } } +/// 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. /// @@ -1077,18 +1067,44 @@ pub(super) fn definitions_entry( .map(|definition| definition.signing)) } -/// Drop a deleted key's entry from the definitions file. +/// 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, 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. -pub(super) 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) } @@ -1566,6 +1582,51 @@ mod tests { ); } + /// 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. diff --git a/crates/validator/src/http_api/mod.rs b/crates/validator/src/http_api/mod.rs index 1c66ddb15..4b8500f25 100644 --- a/crates/validator/src/http_api/mod.rs +++ b/crates/validator/src/http_api/mod.rs @@ -53,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 @@ -67,9 +77,6 @@ pub struct KeymanagerContext { /// The record every key must be registered in before it is loaded, so the /// duty path never meets a key it has no record for. pub protection: crate::slashing_protection::SharedProtection, - /// `--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, } /// The file the generated token is written to, inside the validators directory. diff --git a/crates/validator/src/http_api/proposer.rs b/crates/validator/src/http_api/proposer.rs index 856e392c5..0d63984dd 100644 --- a/crates/validator/src/http_api/proposer.rs +++ b/crates/validator/src/http_api/proposer.rs @@ -367,9 +367,9 @@ 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(), - default_remote_signer: None, }; Harness { app: router(context, TOKEN.to_string()), diff --git a/crates/validator/src/http_api/remotekeys.rs b/crates/validator/src/http_api/remotekeys.rs index eebc7fce0..470bcc410 100644 --- a/crates/validator/src/http_api/remotekeys.rs +++ b/crates/validator/src/http_api/remotekeys.rs @@ -14,8 +14,24 @@ //! 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}; @@ -23,10 +39,12 @@ use crate::Error; use crate::beacon_node::dto::{encode_hex, parse_pubkey}; use crate::http_api::KeymanagerContext; use crate::http_api::keystores::{ - DeleteRequest, KeyStatus, StatusResponse, definitions_entry, persist_delete, save_definitions, - upsert_definition, + DeleteRequest, KeyStatus, Origin, StatusResponse, definitions_entry, flush_earlier_removal, + persist_delete, save_definitions, +}; +use crate::keys::{ + SigningDefinition, ValidatorDefinition, ValidatorDefinitions, ValidatorStore, Web3Signer, }; -use crate::keys::{SigningDefinition, ValidatorDefinition, ValidatorDefinitions, ValidatorStore}; #[derive(Debug, Serialize)] pub struct ListResponse { @@ -70,47 +88,73 @@ pub async fn list(State(context): State) -> Json)>, KeyStatus>; + pub async fn import( State(context): State, Json(request): Json, ) -> Json { - // Held across the whole open-mutate-save cycle over the definitions file, - // and taken before `store`'s write lock, never the other way around: the - // same order every keymanager handler uses. See - // `KeymanagerContext::definitions_lock`. + 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) => { - let message = err.to_string(); - let data = request - .remote_keys - .iter() - .map(|_| KeyStatus::error(&message)) - .collect(); - return Json(StatusResponse { data }); - } + Err(err) => return fail_all(&err.to_string()), }; - // Everything that can be decided and persisted before the store's write - // lock is needed. Duplicates are judged against a read of the store: - // nothing between here and the insert below can add a key, since this - // handler and the keystores import both hold `definitions_lock`. - let mut prepared = Vec::with_capacity(request.remote_keys.len()); - { + // 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; - for key in &request.remote_keys { - prepared.push(prepare_import(&context, &store, &mut definitions, key)); - } - } + 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(); - // Every persisted 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 persisting rather than before, - // for the reason `keystores::import` gives. A remote signer keeps a - // record of its own; this one is kept as well. - let persisted: Vec = prepared + // 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), @@ -118,8 +162,28 @@ pub async fn import( }) .collect(); let mut failure = None; - if !persisted.is_empty() { - let registering = persisted.clone(); + 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) @@ -148,9 +212,10 @@ pub async fn import( // 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| !persisted.contains(&parsed)) + .map(|parsed| !added.contains(&parsed)) .unwrap_or(true) }); failure = Some(match save_definitions(&context, &definitions) { @@ -172,18 +237,30 @@ pub async fn import( } } } - drop(_definitions_guard); + 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, &failure) { - (Ok(Some(_)), Some(message)) => KeyStatus::error(message), - (result, _) => match result { - Ok(Some((pubkey, url))) => finish_import(&mut store, pubkey, &url), - Ok(None) => KeyStatus::duplicate(), - Err(status) => status, - }, + .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); @@ -191,17 +268,21 @@ pub async fn import( Json(StatusResponse { data }) } -/// Everything one import does before it needs the store's write lock: -/// resolve the URL, refuse a key already held, and persist the definition. +/// 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. +/// `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, - store: &ValidatorStore, + known: &HashSet, + signers: &mut HashMap>, definitions: &mut ValidatorDefinitions, key: &ImportRemoteKey, -) -> std::result::Result, KeyStatus> -{ +) -> Prepared { let pubkey = parse_pubkey(&key.pubkey).map_err(KeyStatus::error)?; let url = match key .url @@ -215,61 +296,39 @@ fn prepare_import( )); } }; - // Refused for shape before anything is written: an unusable URL would - // otherwise be persisted and refuse the next startup. - crate::keys::Web3Signer::new(&url).map_err(KeyStatus::error)?; // 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. + // 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 || store.contains(&pubkey) { + if on_disk || known.contains(&pubkey) { return Ok(None); } - // Persisted before the key is reachable through the store, for the reason - // the keystores import does it in this order: a validator this process - // is willing to sign with must already be durable. - let definition = ValidatorDefinition { - enabled: true, - voting_public_key: encode_hex(&pubkey.0), - signing: SigningDefinition::Web3Signer { url: url.clone() }, + // 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 + } }; - upsert_definition(context, definitions, definition).map_err(KeyStatus::error)?; - - Ok(Some((pubkey, url))) -} -/// The one step of an import that needs the write lock: a map insert. -/// -/// The duplicate check runs again here, under the write lock this time. The -/// read-side check in `prepare_import` ran under `definitions_lock`, but that -/// lock was released before this one was taken, and a keystores import can -/// land in between; replacing the local key it just put in the store with a -/// remote entry would be the one outcome neither request asked for. -fn finish_import( - store: &mut ValidatorStore, - pubkey: ethlambda_types::beacon::primitives::BlsPubkey, - url: &str, -) -> KeyStatus { - if store.contains(&pubkey) { - return KeyStatus::duplicate(); - } - match store.insert_remote(pubkey, url) { - Ok(()) => { - info!(pubkey = %encode_hex(&pubkey.0), url, "Imported remote validator key"); - KeyStatus { - status: "imported".to_string(), - message: None, - } - } - Err(err) => KeyStatus::error(err), - } + 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( @@ -307,6 +366,12 @@ fn delete_one( }; 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, @@ -321,8 +386,18 @@ fn delete_one( store.remove(pubkey); } context.settings.forget(pubkey); - if let Err(err) = persist_delete(context, pubkey) { - return KeyStatus::error(err); + 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"); @@ -649,7 +724,14 @@ mod tests { ), ) .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, @@ -670,6 +752,105 @@ mod tests { 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 diff --git a/crates/validator/src/keys/definitions.rs b/crates/validator/src/keys/definitions.rs index 8418a4650..633d218ed 100644 --- a/crates/validator/src/keys/definitions.rs +++ b/crates/validator/src/keys/definitions.rs @@ -15,28 +15,20 @@ 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, - /// Where the key is and how it signs. Flattened, so a local entry keeps - /// the two path fields it always had at the top level and a remote one - /// has `url` there instead. - #[serde(flatten)] + /// 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. -/// -/// Untagged rather than carrying a `type` field: the file predates remote -/// signers, and every existing entry has the two keystore paths and nothing -/// else, so the fields themselves are the tag. Deserialization tries the -/// variants in order, and the field sets are disjoint, so an entry cannot -/// match both. -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] -#[serde(untagged)] +#[derive(Debug, Clone, PartialEq, Eq)] pub enum SigningDefinition { /// An EIP-2335 keystore on disk, decrypted into this process at startup. LocalKeystore { @@ -49,6 +41,130 @@ pub enum SigningDefinition { 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. +/// +/// Unknown fields are refused rather than skipped, so a Lighthouse field this +/// client does not implement (`root_certificate_path`, say) is reported at +/// startup instead of quietly having no effect. +#[derive(Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +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, +} + +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 + )); + } + }; + 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, + } + } +} + impl SigningDefinition { pub fn is_remote(&self) -> bool { matches!(self, Self::Web3Signer { .. }) @@ -291,10 +407,8 @@ mod tests { 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:"), - "the variant is told by its fields, not a tag: {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); @@ -315,6 +429,77 @@ mod tests { 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, not ignored: an + /// operator who set `root_certificate_path` expects it to take effect. + #[test] + fn an_unknown_field_is_refused() { + let err = open_text( + "- enabled: true\n voting_public_key: '0xaa'\n type: web3signer\n \ + url: https://signer:9000\n root_certificate_path: ca.pem\n", + ) + .expect_err("unknown field"); + assert!(err.contains("root_certificate_path"), "{err}"); + } + #[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/store.rs b/crates/validator/src/keys/store.rs index 4c2f38763..10f85f14d 100644 --- a/crates/validator/src/keys/store.rs +++ b/crates/validator/src/keys/store.rs @@ -2,7 +2,7 @@ use std::collections::HashMap; use std::path::Path; -use std::sync::Arc; +use std::sync::{Arc, Weak}; use blst::min_pk::SecretKey; use ethlambda_types::beacon::primitives::{BLS_PUBKEY_SIZE, BlsPubkey}; @@ -46,7 +46,12 @@ pub struct ValidatorStore { /// 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. - signers: HashMap>, + /// + /// 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 { @@ -187,41 +192,60 @@ impl ValidatorStore { secret_key: Arc::new(secret_key), }, ); - self.drop_unused_signers(); Ok(pubkey) } /// Add a validator whose key a Web3Signer at `url` holds, replacing any - /// method the store had for it. + /// 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<()> { - // Keyed by the URL as the client normalizes it, so `.../` and `...` - // name one signer rather than two. - let key = url.trim_end_matches('/'); - let signer = match self.signers.get(key) { - Some(signer) => signer.clone(), - None => { - let signer = Arc::new(Web3Signer::new(url)?); - self.signers.insert(key.to_string(), signer.clone()); - signer - } + let signer = match self.signer_for_url(url) { + Some(signer) => signer, + None => Arc::new(Web3Signer::new(url)?), }; - self.validators - .insert(pubkey, SigningMethod::Web3Signer { signer }); - self.drop_unused_signers(); + self.insert_remote_signer(pubkey, signer); Ok(()) } - /// Forget any signer client no validator uses any more. Called after - /// every mutation, since each can orphan one: an insert that replaces a - /// remote entry with a local one, a remove, a remote insert on a new URL - /// for a key that was the last on its old one. - fn drop_unused_signers(&mut self) { + /// 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 - .retain(|_, signer| Arc::strong_count(signer) > 1); + .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 @@ -238,9 +262,7 @@ impl ValidatorStore { } pub fn remove(&mut self, pubkey: &BlsPubkey) -> bool { - let removed = self.validators.remove(pubkey).is_some(); - self.drop_unused_signers(); - removed + self.validators.remove(pubkey).is_some() } pub fn contains(&self, pubkey: &BlsPubkey) -> bool { @@ -465,6 +487,15 @@ mod tests { 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(); @@ -476,12 +507,31 @@ mod tests { store .insert_remote(second, "http://signer:9000/") .expect("inserts"); - assert_eq!(store.signers.len(), 1, "one URL, one client"); + assert_eq!(live_signers(&store), 1, "one URL, one client"); assert!(store.remove(&first)); - assert_eq!(store.signers.len(), 1, "still in use by the second key"); + assert_eq!(live_signers(&store), 1, "still in use by the second key"); assert!(store.remove(&second)); - assert!(store.signers.is_empty(), "nobody uses it any more"); + 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] @@ -497,7 +547,7 @@ mod tests { assert_eq!(derived, pubkey); assert_eq!(store.len(), 1); assert!(store.remote_url(&pubkey).is_none()); - assert!(store.signers.is_empty()); + assert_eq!(live_signers(&store), 0); } /// A definitions entry declaring a key its keystore does not hold must not diff --git a/crates/validator/src/keys/web3signer.rs b/crates/validator/src/keys/web3signer.rs index f744deb63..9f037a9bf 100644 --- a/crates/validator/src/keys/web3signer.rs +++ b/crates/validator/src/keys/web3signer.rs @@ -46,35 +46,44 @@ pub struct Web3Signer { 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(); - let parsed = reqwest::Url::parse(&base_url).map_err(|err| Error::Keystore { - path: base_url.clone(), - reason: format!("not a valid remote signer URL: {err}"), - })?; + // 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(Error::Keystore { - path: base_url, - reason: "a remote signer URL must be http or https".to_string(), - }); + 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(Error::Keystore { - path: parsed.host_str().unwrap_or("").to_string(), - reason: "a remote signer URL must not carry credentials".to_string(), - }); + 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| Error::Keystore { - path: base_url.clone(), - reason: format!("could not build an HTTP client: {err}"), - })?; + .map_err(|err| invalid(&format!("could not build an HTTP client: {err}")))?; Ok(Self { base_url, client }) } @@ -152,6 +161,30 @@ fn truncate(body: &str) -> 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. @@ -575,4 +608,40 @@ mod tests { 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 b198a031b..e75f9e559 100644 --- a/crates/validator/src/lib.rs +++ b/crates/validator/src/lib.rs @@ -89,14 +89,14 @@ 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, - /// Register keys that have no slashing-protection record as never having - /// signed, instead of refusing to start. See - /// [`slashing_protection::SlashingProtection::open`]. - pub init_slashing_protection: bool, /// `--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`]. + pub init_slashing_protection: bool, } /// Run the validator client until the process is stopped. @@ -192,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, @@ -208,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 = @@ -234,9 +233,9 @@ 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(), - default_remote_signer: config.web3signer_url.clone(), }; let router = http_api::router(context, token); let listener = tokio::net::TcpListener::bind(address) @@ -890,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 { @@ -1087,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 { @@ -1187,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") @@ -1244,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 { @@ -1336,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") @@ -1395,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 f08295c83..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(); - 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) - .await - { - 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 ded84cb0f..8b84d5dcc 100644 --- a/crates/validator/src/proposal.rs +++ b/crates/validator/src/proposal.rs @@ -237,13 +237,13 @@ impl ProposalService { // 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. + // + // 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 = produced.block_root(); - debug_assert_eq!( - header.hash_tree_root(), - block_root, - "a header merkleizes to its block's root" - ); + 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 @@ -450,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. @@ -869,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 b968d6536..338bdc6e6 100644 --- a/crates/validator/src/signing.rs +++ b/crates/validator/src/signing.rs @@ -29,6 +29,11 @@ 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}; @@ -58,9 +63,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); @@ -264,6 +324,20 @@ impl SigningContext { 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); signer.sign(pubkey, &request).await } @@ -317,22 +391,44 @@ 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. + /// + /// 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: &RwLock, pubkey: &BlsPubkey, slot: Slot, ) -> Result { - self.sign( - store, - pubkey, - self.selection_proof_signing_root(slot), - compute_epoch_at_slot(slot), - SigningBody::AggregationSlot { - aggregation_slot: AggregationSlot { slot }, - }, - ) - .await + 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) + } + + /// 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 @@ -552,10 +648,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 { @@ -596,6 +689,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(); @@ -1102,7 +1243,7 @@ mod tests { 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 slot = context.config.electra_fork_epoch * preset::SLOTS_PER_EPOCH; let header = header(slot); context @@ -1113,7 +1254,7 @@ mod tests { let requests = fake.requests.lock().expect("lock"); let body = &requests[0].1; assert_eq!(body["type"], "BLOCK_V2"); - assert_eq!(body["beacon_block"]["version"], "ALTAIR"); + assert_eq!(body["beacon_block"]["version"], "ELECTRA"); assert_eq!( body["beacon_block"]["block_header"]["slot"], slot.to_string() @@ -1125,6 +1266,23 @@ mod tests { ); } + /// 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()); + } + #[tokio::test] async fn a_slashing_protection_refusal_is_a_signing_error() { let (_fake, url) = serve(Answer::Status(412)).await; diff --git a/crates/validator/src/subscriptions.rs b/crates/validator/src/subscriptions.rs index 374d78bf1..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,10 +48,36 @@ pub async fn subscribe( return Ok(()); } - let mut subscriptions = Vec::with_capacity(duties.len()); - for duty in duties { - let is_aggregator = match selection_for(context, store, duty).await { - Ok(selection) => selection.is_some(), + // 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, 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, @@ -58,17 +85,22 @@ pub async fn subscribe( %err, "Could not compute aggregator selection; subscribing without the role" ); - false + crate::metrics::inc_signing_failures(); } - }; - subscriptions.push(CommitteeSubscriptionDto { + } + } + + 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() @@ -125,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) { diff --git a/crates/validator/src/sync_committee.rs b/crates/validator/src/sync_committee.rs index 3b2149555..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(); - 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) - .await - { - 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(); - { - 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) - .await - { - 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(); - { - 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) - .await - { - 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) { diff --git a/docs/cli.md b/docs/cli.md index 4a801f96d..88fa70d9c 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -432,22 +432,40 @@ are separate (see 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, and a -signer that cannot be reached. Nothing is asked of the signer at startup or at +signer that cannot be reached. 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 a `url` where a -local entry has its two keystore paths: +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`. The signer is reached over -plain HTTPS with the system's trust roots; client certificates are not -supported. +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, and so is any field this client does not implement, such as +Lighthouse's `root_certificate_path`. 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 @@ -461,9 +479,14 @@ 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 -before it starts signing, the same order the keystores import uses, and -`readonly` is always `false`: this client has no key it cannot delete. +`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 From 4a197b2244876dafeeea475e4141bd34ea8fe409 Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Wed, 11 Nov 2026 17:10:54 -0300 Subject: [PATCH 4/5] Refuse a field this client does not implement only on a web3signer entry of the definitions file, and read past it on a local one. Refusing every unknown field stopped the client on any file Lighthouse wrote, since Lighthouse puts its own per-validator settings on each entry. The hazard the refusal was for is a remote entry: there an unread field is most likely one of Lighthouse's signer settings (`root_certificate_path`, `request_timeout_ms`), and skipping it would sign through a connection the operator did not describe. So a web3signer entry carrying one is refused at startup, naming every such field, whether the entry states its `type` or is read as remote by its `url`. `description` is the one field a remote entry may carry unread: Lighthouse writes it on every entry, an empty one included, and it changes nothing about how the key signs. A local entry is read past any field this client does not have, as it was before entries had a `type`; those fields are not written back. --- crates/validator/src/keys/definitions.rs | 126 +++++++++++++++++++++-- docs/cli.md | 10 +- 2 files changed, 122 insertions(+), 14 deletions(-) diff --git a/crates/validator/src/keys/definitions.rs b/crates/validator/src/keys/definitions.rs index 633d218ed..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}; @@ -58,11 +60,14 @@ enum SigningKind { /// would silently ignore half of what the operator wrote, and loading it as /// local would bypass the remote signer and its slashing protection. /// -/// Unknown fields are refused rather than skipped, so a Lighthouse field this -/// client does not implement (`root_certificate_path`, say) is reported at -/// startup instead of quietly having no effect. +/// 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)] -#[serde(deny_unknown_fields)] struct RawDefinition { enabled: bool, voting_public_key: String, @@ -74,8 +79,19 @@ struct RawDefinition { 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; @@ -113,6 +129,21 @@ impl TryFrom for ValidatorDefinition { )); } }; + 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 { @@ -161,6 +192,7 @@ impl From for RawDefinition { voting_keystore_path, voting_keystore_password_path, url, + unread: BTreeMap::new(), } } } @@ -488,16 +520,88 @@ mod tests { assert!(err.contains("voting_keystore_password_path"), "{err}"); } - /// A field this client does not implement is reported, not ignored: an - /// operator who set `root_certificate_path` expects it to take effect. + /// 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_unknown_field_is_refused() { - let err = open_text( + 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", + 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_err("unknown field"); - assert!(err.contains("root_certificate_path"), "{err}"); + .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] diff --git a/docs/cli.md b/docs/cli.md index 88fa70d9c..2720d4572 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -455,9 +455,13 @@ 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, and so is any field this client does not implement, such as -Lighthouse's `root_certificate_path`. The `url` must not carry credentials, a -query string or a fragment. +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 From d39e249c2cb76b4fa778b29eede69769a7ac21de Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Wed, 11 Nov 2026 17:31:07 -0300 Subject: [PATCH 5/5] Verify a remote signer's signature against the validator's public key and the signing root before it is used. Whatever 96 bytes the signer answered were returned as the signature. A signer holding another key under the pubkey, one computing a different root for a fork it does not know, or a proxy rewriting the body would have this client publish a signature no peer accepts, and decide `is_aggregator` on bytes that are not the selection proof. `SigningContext::sign` now checks the answer and reports one that does not verify as a signing failure for that key: logged, counted, never retried, and never remembered as a selection proof. The fake signer in the tests signs the request's `signingRoot` with the test key, so the signing path is exercised with real signatures, and can answer with another key's signature, another root's, or bytes that are no signature. --- CLAUDE.md | 5 +- crates/validator/src/signing.rs | 143 ++++++++++++++++++++++++++++++-- docs/cli.md | 8 +- 3 files changed, 143 insertions(+), 13 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 43fbf3b4d..11881ad02 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -855,8 +855,9 @@ transitions are in `ethlambda-types`, per the section above. Nothing above 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. Each duty signs its - batch at once through + 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 diff --git a/crates/validator/src/signing.rs b/crates/validator/src/signing.rs index 338bdc6e6..ccb72cc8e 100644 --- a/crates/validator/src/signing.rs +++ b/crates/validator/src/signing.rs @@ -50,6 +50,21 @@ pub use crate::keys::web3signer::AggregateAndProofRef; /// 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 @@ -339,7 +354,27 @@ impl SigningContext { }); } let request = SigningRequest::new(self.fork_info(epoch), root, body); - signer.sign(pubkey, &request).await + 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) } } } @@ -1147,7 +1182,7 @@ mod tests { use std::sync::{Arc, Mutex}; /// What the fake signer does: records every request and answers with - /// a fixed signature, or with a fixed status. + /// a signature, or with a fixed status. #[derive(Clone)] struct Fake { requests: Arc>>, @@ -1156,22 +1191,57 @@ mod tests { #[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)); - match *fake.answer.lock().expect("lock") { - Answer::Signature => ( + let signed = |signature: String| { + ( StatusCode::OK, - format!("{{\"signature\":\"0x{}\"}}", "ab".repeat(96)), - ), + 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(), @@ -1198,8 +1268,10 @@ mod tests { (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 = BlsPubkey([0x42; 48]); + 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) @@ -1216,12 +1288,15 @@ mod tests { .sign_attestation(&store, &pubkey, &data) .await .expect("signs"); - assert_eq!(signature.0, [0xab; 96]); + 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{}", "42".repeat(48))); + assert_eq!(path_pubkey, &format!("0x{}", hex::encode(pubkey.0))); assert_eq!(body["type"], "ATTESTATION"); assert_eq!( body["signingRoot"], @@ -1283,6 +1358,56 @@ mod tests { 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; diff --git a/docs/cli.md b/docs/cli.md index 2720d4572..83df863b3 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -431,8 +431,12 @@ 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, and a -signer that cannot be reached. Each duty asks for its signatures all at once +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