-
Notifications
You must be signed in to change notification settings - Fork 100
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(rpc): Refactor the cookie-based RPC authentication #8940
base: auth-rpc-endpoint
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -6,43 +6,47 @@ 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<()> { | ||||||||||
File::create(dir.join(FILE))?.write_all(format!("__cookie__:{}", cookie.0).as_bytes())?; | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
|
||||||||||
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(()) | ||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
} | ||
Comment on lines
+50
to
+54
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks great, good idea! Optional: This could be useful elsewhere in Zebra as well, such as in the |
||
|
||
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)) | ||
}) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't seem too important to remove the cookie file at shutdown, it should still work as expected when Zebra is restarted, so it seems better to try logging a warning here instead, at least for release builds.
We could also always skip removing the cookie and overwrite it at startup instead.