fix(rpc): Refactor the cookie-based RPC authentication (#8940)

* Refactor the cookie-based RPC authentication

* Rephrase docs

* Apply suggestions from code review

Co-authored-by: Arya <[email protected]>

---------

Co-authored-by: Arya <[email protected]>

zebra-rpc/src/server.rs

@@ -9,6 +9,8 @@
 
 use std::{fmt, panic, thread::available_parallelism};
 
+use cookie::Cookie;
+use http_request_compatibility::With;
 use jsonrpc_core::{Compatibility, MetaIoHandler};
 use jsonrpc_http_server::{CloseHandle, ServerBuilder};
 use tokio::task::JoinHandle;
@@ -25,7 +27,7 @@ use crate::{
     config::Config,
     methods::{Rpc, RpcImpl},
     server::{
-        http_request_compatibility::FixHttpRequestMiddleware,
+        http_request_compatibility::HttpRequestMiddleware,
         rpc_call_compatibility::FixRpcResponseMiddleware,
     },
 };
@@ -194,24 +196,28 @@ impl RpcServer {
                 parallel_cpu_threads = available_parallelism().map(usize::from).unwrap_or(1);
             }
 
-            // Generate a RPC authentication cookie
-            if config.enable_cookie_auth {
-                let _ = cookie::generate(config.cookie_dir.clone());
-            }
-
             // The server is a blocking task, which blocks on executor shutdown.
             // So we need to start it in a std::thread.
             // (Otherwise tokio panics on RPC port conflict, which shuts down the RPC server.)
             let span = Span::current();
             let start_server = move || {
                 span.in_scope(|| {
+                    let middleware = if config.enable_cookie_auth {
+                        let cookie = Cookie::default();
+                        cookie::write_to_disk(&cookie, &config.cookie_dir)
+                            .expect("Zebra must be able to write the auth cookie to the disk");
+                        HttpRequestMiddleware::default().with(cookie)
+                    } else {
+                        HttpRequestMiddleware::default()
+                    };
+
                     // Use a different tokio executor from the rest of Zebra,
                     // so that large RPCs and any task handling bugs don't impact Zebra.
                     let server_instance = ServerBuilder::new(io)
                         .threads(parallel_cpu_threads)
                         // TODO: disable this security check if we see errors from lightwalletd
                         //.allowed_hosts(DomainsValidation::Disabled)
-                        .request_middleware(FixHttpRequestMiddleware(config.clone()))
+                        .request_middleware(middleware)
                         .start_http(&listen_addr)
                         .expect("Unable to start RPC server");
 
@@ -280,32 +286,41 @@ impl RpcServer {
     /// This method can be called from within a tokio executor without panicking.
     /// But it is blocking, so `shutdown()` should be used instead.
     pub fn shutdown_blocking(&self) {
-        Self::shutdown_blocking_inner(self.close_handle.clone())
+        Self::shutdown_blocking_inner(self.close_handle.clone(), self.config.clone())
     }
 
     /// Shut down this RPC server asynchronously.
     /// Returns a task that completes when the server is shut down.
     pub fn shutdown(&self) -> JoinHandle<()> {
         let close_handle = self.close_handle.clone();
-
+        let config = self.config.clone();
         let span = Span::current();
+
         tokio::task::spawn_blocking(move || {
-            span.in_scope(|| Self::shutdown_blocking_inner(close_handle))
+            span.in_scope(|| Self::shutdown_blocking_inner(close_handle, config))
         })
     }
 
     /// Shuts down this RPC server using its `close_handle`.
     ///
     /// See `shutdown_blocking()` for details.
-    fn shutdown_blocking_inner(close_handle: CloseHandle) {
+    fn shutdown_blocking_inner(close_handle: CloseHandle, config: Config) {
         // The server is a blocking task, so it can't run inside a tokio thread.
         // See the note at wait_on_server.
         let span = Span::current();
         let wait_on_shutdown = move || {
             span.in_scope(|| {
+                if config.enable_cookie_auth {
+                    if let Err(err) = cookie::remove_from_disk(&config.cookie_dir) {
+                        warn!(
+                            ?err,
+                            "unexpectedly could not remove the rpc auth cookie from the disk"
+                        )
+                    }
+                }
+
                 info!("Stopping RPC server");
                 close_handle.clone().close();
-                let _ = cookie::delete(); // delete the auth cookie if exists
                 debug!("Stopped RPC server");
             })
         };

zebra-rpc/src/server/cookie.rs

@@ -6,43 +6,49 @@ use rand::RngCore;
 
 use std::{
     fs::{remove_file, File},
-    io::{Read, Write},
-    path::PathBuf,
+    io::Write,
+    path::Path,
 };
 
-/// The user field in the cookie (arbitrary, only for recognizability in debugging/logging purposes)
-pub const COOKIEAUTH_USER: &str = "__cookie__";
-/// Default name for auth cookie file */
-pub const COOKIEAUTH_FILE: &str = ".cookie";
+/// The name of the cookie file on the disk
+const FILE: &str = ".cookie";
 
-/// Generate a new auth cookie and store it in the given `cookie_dir`.
-pub fn generate(cookie_dir: PathBuf) -> Result<()> {
-    let mut data = [0u8; 32];
-    rand::thread_rng().fill_bytes(&mut data);
-    let encoded_password = URL_SAFE.encode(data);
-    let cookie_content = format!("{}:{}", COOKIEAUTH_USER, encoded_password);
+/// If the RPC authentication is enabled, all requests must contain this cookie.
+#[derive(Clone, Debug)]
+pub struct Cookie(String);
 
-    let mut file = File::create(cookie_dir.join(COOKIEAUTH_FILE))?;
-    file.write_all(cookie_content.as_bytes())?;
+impl Cookie {
+    /// Checks if the given passwd matches the contents of the cookie.
+    pub fn authenticate(&self, passwd: String) -> bool {
+        *passwd == self.0
+    }
+}
 
-    tracing::info!("RPC auth cookie generated successfully");
+impl Default for Cookie {
+    fn default() -> Self {
+        let mut bytes = [0u8; 32];
+        rand::thread_rng().fill_bytes(&mut bytes);
 
-    Ok(())
+        Self(URL_SAFE.encode(bytes))
+    }
 }
 
-/// Get the encoded password from the auth cookie.
-pub fn get(cookie_dir: PathBuf) -> Result<String> {
-    let mut file = File::open(cookie_dir.join(COOKIEAUTH_FILE))?;
-    let mut contents = String::new();
-    file.read_to_string(&mut contents)?;
+/// Writes the given cookie to the given dir.
+pub fn write_to_disk(cookie: &Cookie, dir: &Path) -> Result<()> {
+    // Create the directory if needed.
+    std::fs::create_dir_all(&dir)?;
+    File::create(dir.join(FILE))?.write_all(format!("__cookie__:{}", cookie.0).as_bytes())?;
+
+    tracing::info!("RPC auth cookie written to disk");
 
-    let parts: Vec<&str> = contents.split(":").collect();
-    Ok(parts[1].to_string())
+    Ok(())
 }
 
-/// Delete the auth cookie.
-pub fn delete() -> Result<()> {
-    remove_file(COOKIEAUTH_FILE)?;
-    tracing::info!("RPC auth cookie deleted successfully");
+/// Removes a cookie from the given dir.
+pub fn remove_from_disk(dir: &Path) -> Result<()> {
+    remove_file(dir.join(FILE))?;
+
+    tracing::info!("RPC auth cookie removed from disk");
+
     Ok(())
 }

zebra-rpc/src/server/http_request_compatibility.rs

@@ -9,38 +9,58 @@ use jsonrpc_http_server::{
     RequestMiddleware, RequestMiddlewareAction,
 };
 
-use crate::server::cookie;
+use super::cookie::Cookie;
 
 /// HTTP [`RequestMiddleware`] with compatibility workarounds.
 ///
 /// This middleware makes the following changes to HTTP requests:
 ///
-/// ## Remove `jsonrpc` field in JSON RPC 1.0
+/// ### Remove `jsonrpc` field in JSON RPC 1.0
 ///
 /// Removes "jsonrpc: 1.0" fields from requests,
 /// because the "jsonrpc" field was only added in JSON-RPC 2.0.
 ///
 /// <http://www.simple-is-better.org/rpc/#differences-between-1-0-and-2-0>
 ///
-/// ## Add missing `content-type` HTTP header
+/// ### Add missing `content-type` HTTP header
 ///
 /// Some RPC clients don't include a `content-type` HTTP header.
 /// But unlike web browsers, [`jsonrpc_http_server`] does not do content sniffing.
 ///
 /// If there is no `content-type` header, we assume the content is JSON,
 /// and let the parser error if we are incorrect.
 ///
+/// ### Authenticate incoming requests
+///
+/// If the cookie-based RPC authentication is enabled, check that the incoming request contains the
+/// authentication cookie.
+///
 /// This enables compatibility with `zcash-cli`.
 ///
 /// ## Security
 ///
 /// Any user-specified data in RPC requests is hex or base58check encoded.
 /// We assume lightwalletd validates data encodings before sending it on to Zebra.
 /// So any fixes Zebra performs won't change user-specified data.
-#[derive(Clone, Debug)]
-pub struct FixHttpRequestMiddleware(pub crate::config::Config);
+#[derive(Clone, Debug, Default)]
+pub struct HttpRequestMiddleware {
+    cookie: Option<Cookie>,
+}
+
+/// A trait for updating an object, consuming it and returning the updated version.
+pub trait With<T> {
+    /// Updates `self` with an instance of type `T` and returns the updated version of `self`.
+    fn with(self, _: T) -> Self;
+}
 
-impl RequestMiddleware for FixHttpRequestMiddleware {
+impl With<Cookie> for HttpRequestMiddleware {
+    fn with(mut self, cookie: Cookie) -> Self {
+        self.cookie = Some(cookie);
+        self
+    }
+}
+
+impl RequestMiddleware for HttpRequestMiddleware {
     fn on_request(&self, mut request: Request<Body>) -> RequestMiddlewareAction {
         tracing::trace!(?request, "original HTTP request");
 
@@ -62,7 +82,7 @@ impl RequestMiddleware for FixHttpRequestMiddleware {
         }
 
         // Fix the request headers if needed and we can do so.
-        FixHttpRequestMiddleware::insert_or_replace_content_type_header(request.headers_mut());
+        HttpRequestMiddleware::insert_or_replace_content_type_header(request.headers_mut());
 
         // Fix the request body
         let request = request.map(|body| {
@@ -100,7 +120,7 @@ impl RequestMiddleware for FixHttpRequestMiddleware {
     }
 }
 
-impl FixHttpRequestMiddleware {
+impl HttpRequestMiddleware {
     /// Remove any "jsonrpc: 1.0" fields in `data`, and return the resulting string.
     pub fn remove_json_1_fields(data: String) -> String {
         // Replace "jsonrpc = 1.0":
@@ -164,27 +184,15 @@ impl FixHttpRequestMiddleware {
 
     /// Check if the request is authenticated.
     pub fn check_credentials(&self, headers: &header::HeaderMap) -> bool {
-        if !self.0.enable_cookie_auth {
-            return true;
-        }
-        headers
-            .get(header::AUTHORIZATION)
-            .and_then(|auth_header| auth_header.to_str().ok())
-            .and_then(|auth| auth.split_whitespace().nth(1))
-            .and_then(|token| URL_SAFE.decode(token).ok())
-            .and_then(|decoded| String::from_utf8(decoded).ok())
-            .and_then(|decoded_str| {
-                decoded_str
-                    .split(':')
-                    .nth(1)
-                    .map(|password| password.to_string())
-            })
-            .map_or(false, |password| {
-                if let Ok(cookie_password) = cookie::get(self.0.cookie_dir.clone()) {
-                    cookie_password == password
-                } else {
-                    false
-                }
-            })
+        self.cookie.as_ref().map_or(true, |internal_cookie| {
+            headers
+                .get(header::AUTHORIZATION)
+                .and_then(|auth_header| auth_header.to_str().ok())
+                .and_then(|auth_header| auth_header.split_whitespace().nth(1))
+                .and_then(|encoded| URL_SAFE.decode(encoded).ok())
+                .and_then(|decoded| String::from_utf8(decoded).ok())
+                .and_then(|request_cookie| request_cookie.split(':').nth(1).map(String::from))
+                .map_or(false, |passwd| internal_cookie.authenticate(passwd))
+        })
     }
 }