From 30691384759c809040c7a39ab9d90a3446276bd1 Mon Sep 17 00:00:00 2001
From: Luke Parker <lukeparker5132@gmail.com>
Date: Thu, 26 Oct 2023 12:45:39 -0400
Subject: [PATCH] Fix handling of Monero daemon connections when using an
 authenticated RPC

The lack of locking the connection when making an authenticated request, which
is actually two sequential requests, risked another caller making a request in
between, invalidating the state.

Now, only unauthenticated connections share a connection object.
---
 coins/monero/src/rpc/http.rs                  | 40 ++++++++++++++-----
 .../coins/monero/scripts/entry-dev.sh         |  6 +--
 processor/src/tests/literal/mod.rs            |  3 +-
 3 files changed, 35 insertions(+), 14 deletions(-)

diff --git a/coins/monero/src/rpc/http.rs b/coins/monero/src/rpc/http.rs
index f2eb1328..8c24762a 100644
--- a/coins/monero/src/rpc/http.rs
+++ b/coins/monero/src/rpc/http.rs
@@ -5,10 +5,21 @@ use reqwest::Client;
 
 use crate::rpc::{RpcError, RpcConnection, Rpc};
 
+#[derive(Clone, Debug)]
+enum Authentication {
+  // If unauthenticated, reuse a single client
+  Unauthenticated(Client),
+  // If authenticated, don't reuse clients so that each connection makes its own connection
+  // This ensures that if a nonce is requested, another caller doesn't make a request invalidating
+  // it
+  // We could acquire a mutex over the client, yet creating a new client is preferred for the
+  // possibility of parallelism
+  Authenticated(String, String),
+}
+
 #[derive(Clone, Debug)]
 pub struct HttpRpc {
-  client: Client,
-  userpass: Option<(String, String)>,
+  authentication: Authentication,
   url: String,
 }
 
@@ -18,8 +29,8 @@ impl HttpRpc {
   /// A daemon requiring authentication can be used via including the username and password in the
   /// URL.
   pub fn new(mut url: String) -> Result<Rpc<HttpRpc>, RpcError> {
-    // Parse out the username and password
-    let userpass = if url.contains('@') {
+    let authentication = if url.contains('@') {
+      // Parse out the username and password
       let url_clone = url;
       let split_url = url_clone.split('@').collect::<Vec<_>>();
       if split_url.len() != 2 {
@@ -42,22 +53,31 @@ impl HttpRpc {
       if split_userpass.len() != 2 {
         Err(RpcError::InvalidNode)?;
       }
-      Some((split_userpass[0].to_string(), split_userpass[1].to_string()))
+      Authentication::Authenticated(split_userpass[0].to_string(), split_userpass[1].to_string())
     } else {
-      None
+      Authentication::Unauthenticated(Client::new())
     };
 
-    Ok(Rpc(HttpRpc { client: Client::new(), userpass, url }))
+    Ok(Rpc(HttpRpc { authentication, url }))
   }
 }
 
 #[async_trait]
 impl RpcConnection for HttpRpc {
   async fn post(&self, route: &str, body: Vec<u8>) -> Result<Vec<u8>, RpcError> {
-    let mut builder = self.client.post(self.url.clone() + "/" + route).body(body);
+    #[allow(unused_assignments)] // False positive
+    let mut client_storage = None;
+    let client = match &self.authentication {
+      Authentication::Unauthenticated(client) => client,
+      Authentication::Authenticated(_, _) => {
+        client_storage = Some(Client::new());
+        client_storage.as_ref().unwrap()
+      }
+    };
 
-    if let Some((user, pass)) = &self.userpass {
-      let req = self.client.post(&self.url).send().await.map_err(|_| RpcError::InvalidNode)?;
+    let mut builder = client.post(self.url.clone() + "/" + route).body(body);
+    if let Authentication::Authenticated(user, pass) = &self.authentication {
+      let req = client.post(&self.url).send().await.map_err(|_| RpcError::InvalidNode)?;
       // Only provide authentication if this daemon actually expects it
       if let Some(header) = req.headers().get("www-authenticate") {
         builder = builder.header(
diff --git a/orchestration/coins/monero/scripts/entry-dev.sh b/orchestration/coins/monero/scripts/entry-dev.sh
index 77df5b33..b5367f0c 100755
--- a/orchestration/coins/monero/scripts/entry-dev.sh
+++ b/orchestration/coins/monero/scripts/entry-dev.sh
@@ -5,6 +5,6 @@ RPC_PASS="${RPC_PASS:=seraidex}"
 
 # Run Monero
 # TODO: Restore Auth
-monerod --regtest --offline --fixed-difficulty=1 \
-  --rpc-bind-ip=0.0.0.0 --rpc-access-control-origins * --confirm-external-bind \
-  --non-interactive
+monerod --non-interactive --regtest --offline --fixed-difficulty=1 \
+  --no-zmq --rpc-bind-ip=0.0.0.0 --confirm-external-bind \
+  --rpc-access-control-origins * --disable-rpc-ban
diff --git a/processor/src/tests/literal/mod.rs b/processor/src/tests/literal/mod.rs
index 142d1492..fb47bc9b 100644
--- a/processor/src/tests/literal/mod.rs
+++ b/processor/src/tests/literal/mod.rs
@@ -89,6 +89,7 @@ mod monero {
       "--no-zmq".to_string(),
       "--disable-rpc-ban".to_string(),
       "--rpc-bind-ip=0.0.0.0".to_string(),
+      "--rpc-login=serai:seraidex".to_string(),
       "--rpc-access-control-origins=*".to_string(),
       "--confirm-external-bind".to_string(),
       "--non-interactive".to_string(),
@@ -110,7 +111,7 @@ mod monero {
     let handle = ops.handle("serai-dev-monero").host_port(18081).unwrap();
     // TODO: Replace with a check if the node has booted
     tokio::time::sleep(core::time::Duration::from_secs(20)).await;
-    let monero = Monero::new(format!("http://{}:{}", handle.0, handle.1));
+    let monero = Monero::new(format!("http://serai:seraidex@{}:{}", handle.0, handle.1));
     while monero.get_latest_block_number().await.unwrap() < 150 {
       monero.mine_block().await;
     }