Remove VC response signing and fix HTTP error handling (#5529)

eserilev · web-flow · commit bf2f0b02b84d · 2024-07-16T07:57:58.000Z
* and_then to then remove expect move convert_rejection to utils remove signer from vc api * remove key * remove auth header * revert * Merge branch 'unstable' of https://github.com/sigp/lighthouse into vc-api-fix * merge unstable * revert * Merge branch 'unstable' of https://github.com/sigp/lighthouse into vc-api-fix * Merge branch 'unstable' of https://github.com/sigp/lighthouse into vc-api-fix * refactor blocking json task * linting * revert logging * remove response signing checks in validtor http_api client * remove notion of public key, prefixes, and simplify token generation * fmt * Remove outdated comment on public key
diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs
@@ -97,7 +97,7 @@ use warp::hyper::Body;
 use warp::sse::Event;
 use warp::Reply;
 use warp::{http::Response, Filter, Rejection};
-use warp_utils::{query::multi_key_query, uor::UnifyingOrFilter};
+use warp_utils::{query::multi_key_query, reject::convert_rejection, uor::UnifyingOrFilter};
 
 const API_PREFIX: &str = "eth";
 
@@ -1802,7 +1802,7 @@ pub fn serve<T: BeaconChainTypes>(
                 )
                 .await
                 .map(|()| warp::reply::json(&()));
-                task_spawner::convert_rejection(result).await
+                convert_rejection(result).await
             },
         );
 
@@ -3817,12 +3817,12 @@ pub fn serve<T: BeaconChainTypes>(
                     .await;
 
                 if initial_result.is_err() {
-                    return task_spawner::convert_rejection(initial_result).await;
+                    return convert_rejection(initial_result).await;
                 }
 
                 // Await a response from the builder without blocking a
                 // `BeaconProcessor` worker.
-                task_spawner::convert_rejection(rx.await.unwrap_or_else(|_| {
+                convert_rejection(rx.await.unwrap_or_else(|_| {
                     Ok(warp::reply::with_status(
                         warp::reply::json(&"No response from channel"),
                         eth2::StatusCode::INTERNAL_SERVER_ERROR,
diff --git a/beacon_node/http_api/src/task_spawner.rs b/beacon_node/http_api/src/task_spawner.rs
@@ -4,6 +4,7 @@ use std::future::Future;
 use tokio::sync::{mpsc::error::TrySendError, oneshot};
 use types::EthSpec;
 use warp::reply::{Reply, Response};
+use warp_utils::reject::convert_rejection;
 
 /// Maps a request to a queue in the `BeaconProcessor`.
 #[derive(Clone, Copy)]
@@ -35,24 +36,6 @@ pub struct TaskSpawner<E: EthSpec> {
     beacon_processor_send: Option<BeaconProcessorSend<E>>,
 }
 
-/// Convert a warp `Rejection` into a `Response`.
-///
-/// This function should *always* be used to convert rejections into responses. This prevents warp
-/// from trying to backtrack in strange ways. See: https://github.com/sigp/lighthouse/issues/3404
-pub async fn convert_rejection<T: Reply>(res: Result<T, warp::Rejection>) -> Response {
-    match res {
-        Ok(response) => response.into_response(),
-        Err(e) => match warp_utils::reject::handle_rejection(e).await {
-            Ok(reply) => reply.into_response(),
-            Err(_) => warp::reply::with_status(
-                warp::reply::json(&"unhandled error"),
-                eth2::StatusCode::INTERNAL_SERVER_ERROR,
-            )
-            .into_response(),
-        },
-    }
-}
-
 impl<E: EthSpec> TaskSpawner<E> {
     pub fn new(beacon_processor_send: Option<BeaconProcessorSend<E>>) -> Self {
         Self {
diff --git a/common/eth2/src/lighthouse_vc/http_client.rs b/common/eth2/src/lighthouse_vc/http_client.rs
@@ -1,13 +1,10 @@
-use super::{types::*, PK_LEN, SECRET_PREFIX};
+use super::types::*;
 use crate::Error;
 use account_utils::ZeroizeString;
-use bytes::Bytes;
-use libsecp256k1::{Message, PublicKey, Signature};
 use reqwest::{
     header::{HeaderMap, HeaderValue},
     IntoUrl,
 };
-use ring::digest::{digest, SHA256};
 use sensitive_url::SensitiveUrl;
 use serde::{de::DeserializeOwned, Serialize};
 use std::fmt::{self, Display};
@@ -24,8 +21,7 @@ use types::graffiti::GraffitiString;
 pub struct ValidatorClientHttpClient {
     client: reqwest::Client,
     server: SensitiveUrl,
-    secret: Option<ZeroizeString>,
-    server_pubkey: Option<PublicKey>,
+    api_token: Option<ZeroizeString>,
     authorization_header: AuthorizationHeader,
 }
 
@@ -46,45 +42,13 @@ impl Display for AuthorizationHeader {
     }
 }
 
-/// Parse an API token and return a secp256k1 public key.
-///
-/// If the token does not start with the Lighthouse token prefix then `Ok(None)` will be returned.
-/// An error will be returned if the token looks like a Lighthouse token but doesn't correspond to a
-/// valid public key.
-pub fn parse_pubkey(secret: &str) -> Result<Option<PublicKey>, Error> {
-    let secret = if !secret.starts_with(SECRET_PREFIX) {
-        return Ok(None);
-    } else {
-        &secret[SECRET_PREFIX.len()..]
-    };
-
-    serde_utils::hex::decode(secret)
-        .map_err(|e| Error::InvalidSecret(format!("invalid hex: {:?}", e)))
-        .and_then(|bytes| {
-            if bytes.len() != PK_LEN {
-                return Err(Error::InvalidSecret(format!(
-                    "expected {} bytes not {}",
-                    PK_LEN,
-                    bytes.len()
-                )));
-            }
-
-            let mut arr = [0; PK_LEN];
-            arr.copy_from_slice(&bytes);
-            PublicKey::parse_compressed(&arr)
-                .map_err(|e| Error::InvalidSecret(format!("invalid secp256k1 pubkey: {:?}", e)))
-        })
-        .map(Some)
-}
-
 impl ValidatorClientHttpClient {
     /// Create a new client pre-initialised with an API token.
     pub fn new(server: SensitiveUrl, secret: String) -> Result<Self, Error> {
         Ok(Self {
             client: reqwest::Client::new(),
             server,
-            server_pubkey: parse_pubkey(&secret)?,
-            secret: Some(secret.into()),
+            api_token: Some(secret.into()),
             authorization_header: AuthorizationHeader::Bearer,
         })
     }
@@ -96,8 +60,7 @@ impl ValidatorClientHttpClient {
         Ok(Self {
             client: reqwest::Client::new(),
             server,
-            secret: None,
-            server_pubkey: None,
+            api_token: None,
             authorization_header: AuthorizationHeader::Omit,
         })
     }
@@ -110,15 +73,14 @@ impl ValidatorClientHttpClient {
         Ok(Self {
             client,
             server,
-            server_pubkey: parse_pubkey(&secret)?,
-            secret: Some(secret.into()),
+            api_token: Some(secret.into()),
             authorization_header: AuthorizationHeader::Bearer,
         })
     }
 
     /// Get a reference to this client's API token, if any.
     pub fn api_token(&self) -> Option<&ZeroizeString> {
-        self.secret.as_ref()
+        self.api_token.as_ref()
     }
 
     /// Read an API token from the specified `path`, stripping any trailing whitespace.
@@ -128,19 +90,11 @@ impl ValidatorClientHttpClient {
     }
 
     /// Add an authentication token to use when making requests.
-    ///
-    /// If the token is Lighthouse-like, a pubkey derivation will be attempted. In the case
-    /// of failure the token will still be stored, and the client can continue to be used to
-    /// communicate with non-Lighthouse nodes.
     pub fn add_auth_token(&mut self, token: ZeroizeString) -> Result<(), Error> {
-        let pubkey_res = parse_pubkey(token.as_str());
-
-        self.secret = Some(token);
+        self.api_token = Some(token);
         self.authorization_header = AuthorizationHeader::Bearer;
 
-        pubkey_res.map(|opt_pubkey| {
-            self.server_pubkey = opt_pubkey;
-        })
+        Ok(())
     }
 
     /// Set to `false` to disable sending the `Authorization` header on requests.
@@ -160,49 +114,17 @@ impl ValidatorClientHttpClient {
         self.authorization_header = AuthorizationHeader::Basic;
     }
 
-    async fn signed_body(&self, response: Response) -> Result<Bytes, Error> {
-        let server_pubkey = self.server_pubkey.as_ref().ok_or(Error::NoServerPubkey)?;
-        let sig = response
-            .headers()
-            .get("Signature")
-            .ok_or(Error::MissingSignatureHeader)?
-            .to_str()
-            .map_err(|_| Error::InvalidSignatureHeader)?
-            .to_string();
-
-        let body = response.bytes().await.map_err(Error::from)?;
-
-        let message =
-            Message::parse_slice(digest(&SHA256, &body).as_ref()).expect("sha256 is 32 bytes");
-
-        serde_utils::hex::decode(&sig)
-            .ok()
-            .and_then(|bytes| {
-                let sig = Signature::parse_der(&bytes).ok()?;
-                Some(libsecp256k1::verify(&message, &sig, server_pubkey))
-            })
-            .filter(|is_valid| *is_valid)
-            .ok_or(Error::InvalidSignatureHeader)?;
-
-        Ok(body)
-    }
-
-    async fn signed_json<T: DeserializeOwned>(&self, response: Response) -> Result<T, Error> {
-        let body = self.signed_body(response).await?;
-        serde_json::from_slice(&body).map_err(Error::InvalidJson)
-    }
-
     fn headers(&self) -> Result<HeaderMap, Error> {
         let mut headers = HeaderMap::new();
 
         if self.authorization_header == AuthorizationHeader::Basic
             || self.authorization_header == AuthorizationHeader::Bearer
         {
-            let secret = self.secret.as_ref().ok_or(Error::NoToken)?;
+            let auth_header_token = self.api_token().ok_or(Error::NoToken)?;
             let header_value = HeaderValue::from_str(&format!(
                 "{} {}",
                 self.authorization_header,
-                secret.as_str()
+                auth_header_token.as_str()
             ))
             .map_err(|e| {
                 Error::InvalidSecret(format!("secret is invalid as a header value: {}", e))
@@ -240,7 +162,8 @@ impl ValidatorClientHttpClient {
 
     async fn get<T: DeserializeOwned, U: IntoUrl>(&self, url: U) -> Result<T, Error> {
         let response = self.get_response(url).await?;
-        self.signed_json(response).await
+        let body = response.bytes().await.map_err(Error::from)?;
+        serde_json::from_slice(&body).map_err(Error::InvalidJson)
     }
 
     async fn delete<U: IntoUrl>(&self, url: U) -> Result<(), Error> {
@@ -263,7 +186,14 @@ impl ValidatorClientHttpClient {
     /// Perform a HTTP GET request, returning `None` on a 404 error.
     async fn get_opt<T: DeserializeOwned, U: IntoUrl>(&self, url: U) -> Result<Option<T>, Error> {
         match self.get_response(url).await {
-            Ok(resp) => self.signed_json(resp).await.map(Option::Some),
+            Ok(resp) => {
+                let body = resp.bytes().await.map(Option::Some)?;
+                if let Some(body) = body {
+                    serde_json::from_slice(&body).map_err(Error::InvalidJson)
+                } else {
+                    Ok(None)
+                }
+            }
             Err(err) => {
                 if err.status() == Some(StatusCode::NOT_FOUND) {
                     Ok(None)
@@ -297,7 +227,8 @@ impl ValidatorClientHttpClient {
         body: &T,
     ) -> Result<V, Error> {
         let response = self.post_with_raw_response(url, body).await?;
-        self.signed_json(response).await
+        let body = response.bytes().await.map_err(Error::from)?;
+        serde_json::from_slice(&body).map_err(Error::InvalidJson)
     }
 
     async fn post_with_unsigned_response<T: Serialize, U: IntoUrl, V: DeserializeOwned>(
@@ -319,8 +250,7 @@ impl ValidatorClientHttpClient {
             .send()
             .await
             .map_err(Error::from)?;
-        let response = ok_or_error(response).await?;
-        self.signed_body(response).await?;
+        ok_or_error(response).await?;
         Ok(())
     }
 
diff --git a/common/eth2/src/lighthouse_vc/mod.rs b/common/eth2/src/lighthouse_vc/mod.rs
@@ -1,10 +1,3 @@
 pub mod http_client;
 pub mod std_types;
 pub mod types;
-
-/// The number of bytes in the secp256k1 public key used as the authorization token for the VC API.
-pub const PK_LEN: usize = 33;
-
-/// The prefix for the secp256k1 public key when it is used as the authorization token for the VC
-/// API.
-pub const SECRET_PREFIX: &str = "api-token-";
diff --git a/common/warp_utils/src/reject.rs b/common/warp_utils/src/reject.rs
@@ -2,7 +2,7 @@ use eth2::types::{ErrorMessage, Failure, IndexedErrorMessage};
 use std::convert::Infallible;
 use std::error::Error;
 use std::fmt;
-use warp::{http::StatusCode, reject::Reject};
+use warp::{http::StatusCode, reject::Reject, reply::Response, Reply};
 
 #[derive(Debug)]
 pub struct ServerSentEventError(pub String);
@@ -255,3 +255,21 @@ pub async fn handle_rejection(err: warp::Rejection) -> Result<impl warp::Reply,
 
     Ok(warp::reply::with_status(json, code))
 }
+
+/// Convert a warp `Rejection` into a `Response`.
+///
+/// This function should *always* be used to convert rejections into responses. This prevents warp
+/// from trying to backtrack in strange ways. See: https://github.com/sigp/lighthouse/issues/3404
+pub async fn convert_rejection<T: Reply>(res: Result<T, warp::Rejection>) -> Response {
+    match res {
+        Ok(response) => response.into_response(),
+        Err(e) => match handle_rejection(e).await {
+            Ok(reply) => reply.into_response(),
+            Err(_) => warp::reply::with_status(
+                warp::reply::json(&"unhandled error"),
+                eth2::StatusCode::INTERNAL_SERVER_ERROR,
+            )
+            .into_response(),
+        },
+    }
+}
diff --git a/common/warp_utils/src/task.rs b/common/warp_utils/src/task.rs
@@ -1,3 +1,4 @@
+use crate::reject::convert_rejection;
 use serde::Serialize;
 use warp::reply::{Reply, Response};
 
@@ -24,14 +25,16 @@ where
 }
 
 /// A convenience wrapper around `blocking_task` for use with `warp` JSON responses.
-pub async fn blocking_json_task<F, T>(func: F) -> Result<Response, warp::Rejection>
+pub async fn blocking_json_task<F, T>(func: F) -> Response
 where
     F: FnOnce() -> Result<T, warp::Rejection> + Send + 'static,
     T: Serialize + Send + 'static,
 {
-    blocking_response_task(|| {
+    let result = blocking_response_task(|| {
         let response = func()?;
         Ok(warp::reply::json(&response))
     })
-    .await
+    .await;
+
+    convert_rejection(result).await
 }
diff --git a/validator_client/src/http_api/api_secret.rs b/validator_client/src/http_api/api_secret.rs
diff --git a/validator_client/src/http_api/mod.rs b/validator_client/src/http_api/mod.rs
