From d51c89d775b17ad12d3063f3f589f02a45126082 Mon Sep 17 00:00:00 2001 From: Mark Mackey Date: Tue, 8 Sep 2026 13:31:52 -0500 Subject: [PATCH] Gloas builder API review follow-ups: bid hash check for direct bids, no-redirect block forwarding Two fixes from review of the Gloas builder API stack, both against code merged in #9805: - The block_hash != parent_block_hash check (#9970, consensus-specs#5594) only covered gossip bids: it sat in the gossip-only helper while the direct (builder-API) path never applied it, so a builder bid violating the consensus assert could win selection and fail block production. Extract it as verify_bid_block_hash_not_parent, called from both the gossip helper (unchanged behavior) and verify_bid_consistency (covering verify_direct_bid), with a direct-path regression test. Audited the remaining process_execution_payload_bid asserts against verify_direct_bid: this was the only one not front-run. - forward_signed_block sent the signed block to the Eth-Builder-Url target through a reqwest client that follows redirects; beacon-APIs publishBlock requires that this SSRF-prone request MUST NOT follow redirects. Give BuilderHttpClient a dedicated no-redirect client used only for submitSignedBeaconBlock; bid requests and preferences keep the default client, and the pre-Gloas builder client is unchanged. Change-Id: Ic6aead799857a9e1465cf4ee13764292dee79111 --- .../direct_verified_bid.rs | 35 +++++++++++++++++++ .../gossip_verified_bid.rs | 21 +++++++++-- .../builder_client/src/builder_http_client.rs | 18 ++++++++-- 3 files changed, 70 insertions(+), 4 deletions(-) diff --git a/beacon_node/beacon_chain/src/payload_bid_verification/direct_verified_bid.rs b/beacon_node/beacon_chain/src/payload_bid_verification/direct_verified_bid.rs index 9983df131d8..a421140e1ea 100644 --- a/beacon_node/beacon_chain/src/payload_bid_verification/direct_verified_bid.rs +++ b/beacon_node/beacon_chain/src/payload_bid_verification/direct_verified_bid.rs @@ -267,6 +267,38 @@ mod tests { )); } + #[test] + fn rejects_block_hash_equal_to_parent_block_hash() { + let (state, spec) = state_and_spec(); + // Passes every earlier check (slot, ancestor hash, parent root, RANDAO, gas limit), then + // claims a `block_hash` equal to its `parent_block_hash` — the consensus assert from + // `process_execution_payload_bid` that must be front-run before selection. + let executed_ancestor = ExecutionBlockHash::repeat_byte(7); + let mut bid = signed_bid( + Slot::new(1), + executed_ancestor, + Hash256::ZERO, + Hash256::ZERO, + ); + bid.message.block_hash = executed_ancestor; + bid.message.gas_limit = EXECUTED_ANCESTOR_GAS_LIMIT; + let result = verify_direct_bid( + &bid, + Slot::new(1), + executed_ancestor, + Hash256::ZERO, + EXECUTED_ANCESTOR_GAS_LIMIT, + &BuilderPubkeys::default(), + &preferences(), + &state, + &spec, + ); + assert!(matches!( + result, + Err(PayloadBidError::BlockHashEqualsParentBlockHash { .. }) + )); + } + #[test] fn rejects_gas_limit_incompatible_with_parent() { let (state, spec) = state_and_spec(); @@ -305,6 +337,9 @@ mod tests { Hash256::ZERO, ); bid.message.gas_limit = EXECUTED_ANCESTOR_GAS_LIMIT; + // A default (zero) `block_hash` would equal the zero parent hash and trip the + // block-hash-equals-parent rejection before the checks this test targets. + bid.message.block_hash = ExecutionBlockHash::repeat_byte(1); let result = verify_direct_bid( &bid, Slot::new(1), diff --git a/beacon_node/beacon_chain/src/payload_bid_verification/gossip_verified_bid.rs b/beacon_node/beacon_chain/src/payload_bid_verification/gossip_verified_bid.rs index 86411f21fff..9ff809558b9 100644 --- a/beacon_node/beacon_chain/src/payload_bid_verification/gossip_verified_bid.rs +++ b/beacon_node/beacon_chain/src/payload_bid_verification/gossip_verified_bid.rs @@ -42,14 +42,26 @@ fn verify_bid_payment_and_blobs( }); } + verify_bid_block_hash_not_parent(bid)?; + + verify_bid_blobs(bid, spec) +} + +/// Reject a bid whose `block_hash` equals its `parent_block_hash`. +/// +/// `process_execution_payload_bid` enforces this in `per_block_processing`, so every bid intake — +/// gossip *and* direct (builder-API) — must front-run it: a bid that fails only at block +/// processing has already won selection and costs the proposer the slot. +pub(crate) fn verify_bid_block_hash_not_parent( + bid: &ExecutionPayloadBid, +) -> Result<(), PayloadBidError> { if bid.block_hash == bid.parent_block_hash { return Err(PayloadBidError::BlockHashEqualsParentBlockHash { slot: bid.slot, block_hash: bid.block_hash, }); } - - verify_bid_blobs(bid, spec) + Ok(()) } fn verify_bid_blobs( @@ -87,6 +99,11 @@ pub(crate) fn verify_bid_consistency( return Err(PayloadBidError::InvalidFeeRecipient); } + // Mirrors the consensus assert in `process_execution_payload_bid`. The gossip path applies + // this earlier (via `verify_bid_payment_and_blobs`); repeating it here keeps the direct path + // covered without depending on the gossip caller's composition. + verify_bid_block_hash_not_parent(bid)?; + verify_bid_blobs(bid, spec)?; verify_bid_state_conditions(bid, head_state, spec) diff --git a/beacon_node/builder_client/src/builder_http_client.rs b/beacon_node/builder_client/src/builder_http_client.rs index a857d13226c..12241036b3f 100644 --- a/beacon_node/builder_client/src/builder_http_client.rs +++ b/beacon_node/builder_client/src/builder_http_client.rs @@ -41,6 +41,11 @@ const DATE_MILLISECONDS: HeaderName = HeaderName::from_static("date-milliseconds #[derive(Clone)] pub struct BuilderHttpClient { client: reqwest::Client, + /// Client for `submitSignedBeaconBlock` only. The target URL arrives over the wire (the + /// `Eth-Builder-Url` request header echoed by the VC) and is an SSRF risk, so beacon-APIs + /// `publishBlock` requires that the forwarding request "MUST NOT follow redirects" — reqwest's + /// redirect policy is client-wide, hence a dedicated client with redirects disabled. + no_redirect_client: reqwest::Client, user_agent: String, /// Only use json for all request/response types. disable_ssz: bool, @@ -50,8 +55,13 @@ impl BuilderHttpClient { pub fn new(user_agent: Option, disable_ssz: bool) -> Result { let user_agent = user_agent.unwrap_or_else(|| DEFAULT_USER_AGENT.to_string()); let client = reqwest::Client::builder().user_agent(&user_agent).build()?; + let no_redirect_client = reqwest::Client::builder() + .user_agent(&user_agent) + .redirect(reqwest::redirect::Policy::none()) + .build()?; Ok(Self { client, + no_redirect_client, user_agent, disable_ssz, }) @@ -234,6 +244,10 @@ impl BuilderHttpClient { /// /// `ssz_request` selects the request-body encoding: SSZ when `true` and the client has SSZ /// enabled, otherwise JSON. + /// + /// Sent via [`Self::no_redirect_client`]: `builder_url` is wire input (`Eth-Builder-Url`), and + /// the spec forbids following redirects on this request. A redirect response surfaces as + /// [`Error::StatusCode`] like any other non-202. pub async fn submit_signed_beacon_block( &self, builder_url: &SensitiveUrl, @@ -263,7 +277,7 @@ impl BuilderHttpClient { HeaderValue::from_str(SSZ_CONTENT_TYPE_HEADER) .map_err(|e| Error::InvalidHeaders(format!("{}", e)))?, ); - self.client + self.no_redirect_client .post(path) .timeout(timeout) .headers(headers) @@ -274,7 +288,7 @@ impl BuilderHttpClient { HeaderValue::from_str(JSON_CONTENT_TYPE_HEADER) .map_err(|e| Error::InvalidHeaders(format!("{}", e)))?, ); - self.client + self.no_redirect_client .post(path) .timeout(timeout) .headers(headers)