From e5b0044b1f6747d06e9c808bab3ab5f5ec6c1da2 Mon Sep 17 00:00:00 2001 From: Andrea Diaz Correia Date: Thu, 13 Aug 2026 21:55:45 -0300 Subject: [PATCH 01/11] feat(config): gate the admin RPC behind a token --- src/config/constants.rs | 11 ++ src/config/secret.rs | 32 +++-- src/config/types.rs | 28 ++++ src/config/util.rs | 282 +++++++++++++++++++++++++++++++++++++++- 4 files changed, 343 insertions(+), 10 deletions(-) diff --git a/src/config/constants.rs b/src/config/constants.rs index ce5d3b5f..90b1cad2 100644 --- a/src/config/constants.rs +++ b/src/config/constants.rs @@ -30,3 +30,14 @@ pub const ENV_FILENAME: &str = ".env"; /// Environment variable name used to override the Nostr private key from the /// process environment. Shared between the wizard and the loader. pub const NSEC_ENV_VAR: &str = "MOSTRO_NSEC_PRIVKEY"; + +/// Environment variable holding the shared bearer token that authenticates +/// admin gRPC callers. Deliberately env-only (like `NSEC_ENV_VAR`): the token +/// never lives in `settings.toml`, only in the process environment or +/// `/.env`. +pub const RPC_TOKEN_ENV_VAR: &str = "MOSTRO_RPC_TOKEN"; + +/// Minimum accepted length for `MOSTRO_RPC_TOKEN`. 32 characters is the +/// shortest base64 encoding of 24 random bytes, well past the point where +/// online guessing against a single daemon is meaningful. +pub const MIN_RPC_TOKEN_LEN: usize = 32; diff --git a/src/config/secret.rs b/src/config/secret.rs index c54e4537..da7c47bc 100644 --- a/src/config/secret.rs +++ b/src/config/secret.rs @@ -1,7 +1,7 @@ //! Helpers for loading and parsing the Mostro Nostr private key with //! zeroization of transient buffers. -use crate::config::constants::NSEC_ENV_VAR; +use crate::config::constants::{NSEC_ENV_VAR, RPC_TOKEN_ENV_VAR}; use crate::config::types::NostrSettings; use mostro_core::error::MostroError::{self, *}; use mostro_core::error::ServiceError; @@ -18,20 +18,36 @@ where serializer.serialize_str(secret.expose_secret()) } -/// Read `MOSTRO_NSEC_PRIVKEY` from the process environment, trim whitespace, -/// and wrap in a [`SecretString`]. Returns `None` when unset or blank. -pub fn read_nsec_env_var() -> Option { - let mut nsec_from_env = std::env::var(NSEC_ENV_VAR).ok()?; - let trimmed = nsec_from_env.trim(); +/// Read `name` from the process environment, trim whitespace, and wrap in a +/// [`SecretString`], zeroizing the transient buffer. Returns `None` when the +/// variable is unset or blank. +fn read_secret_env_var(name: &str) -> Option { + let mut value_from_env = std::env::var(name).ok()?; + let trimmed = value_from_env.trim(); if trimmed.is_empty() { - nsec_from_env.zeroize(); + value_from_env.zeroize(); return None; } let secret = SecretString::from(trimmed.to_owned()); - nsec_from_env.zeroize(); + value_from_env.zeroize(); Some(secret) } +/// Read `MOSTRO_NSEC_PRIVKEY` from the process environment, trim whitespace, +/// and wrap in a [`SecretString`]. Returns `None` when unset or blank. +pub fn read_nsec_env_var() -> Option { + read_secret_env_var(NSEC_ENV_VAR) +} + +/// Read `MOSTRO_RPC_TOKEN` from the process environment, trim whitespace, and +/// wrap in a [`SecretString`]. Returns `None` when unset or blank. +/// +/// The admin RPC bearer token is env-only by design: `settings.toml` is the +/// file operators paste into issues when asking for help. +pub fn read_rpc_token_env_var() -> Option { + read_secret_env_var(RPC_TOKEN_ENV_VAR) +} + /// Parse a bech32 nsec into [`Keys`], exposing the secret only in this scope. pub fn parse_mostro_keys(secret: &SecretString) -> Result { let nsec = secret.expose_secret(); diff --git a/src/config/types.rs b/src/config/types.rs index 3a9e56f2..c267237b 100644 --- a/src/config/types.rs +++ b/src/config/types.rs @@ -468,6 +468,18 @@ pub struct RpcSettings { /// Duration in seconds after which inactive rate-limiter entries are evicted #[serde(default = "default_rate_limiter_stale_duration")] pub rate_limiter_stale_duration: u64, + /// Acknowledge binding the admin RPC to a non-loopback address. The daemon + /// refuses to start on a routable address unless this is set, so an + /// operator cannot publish the admin surface to a LAN by accident. + #[serde(default)] + pub allow_remote: bool, + /// Path to the PEM-encoded TLS certificate chain served by the admin RPC. + /// Must be set together with `tls_key_path`; absent means plaintext. + #[serde(default)] + pub tls_cert_path: Option, + /// Path to the PEM-encoded TLS private key matching `tls_cert_path`. + #[serde(default)] + pub tls_key_path: Option, } fn default_rate_limiter_stale_duration() -> u64 { @@ -481,6 +493,22 @@ impl Default for RpcSettings { listen_address: "127.0.0.1".to_string(), port: 50051, rate_limiter_stale_duration: default_rate_limiter_stale_duration(), + allow_remote: false, + tls_cert_path: None, + tls_key_path: None, + } + } +} + +impl RpcSettings { + /// TLS certificate/key pair when both are configured. + /// + /// `validate_rpc_settings` rejects a half-configured pair at startup, so a + /// `None` here means plaintext was chosen, never that one path was lost. + pub fn tls_paths(&self) -> Option<(&str, &str)> { + match (self.tls_cert_path.as_deref(), self.tls_key_path.as_deref()) { + (Some(cert), Some(key)) => Some((cert, key)), + _ => None, } } } diff --git a/src/config/util.rs b/src/config/util.rs index 635a5ce3..96a850eb 100644 --- a/src/config/util.rs +++ b/src/config/util.rs @@ -2,12 +2,17 @@ /// This module provides utility functions for the config module. /// It includes functions to initialize the default settings directory and create a settings file from the template if it doesn't exist. /// It also includes functions to add a trailing slash to a path if it doesn't already have one. -use crate::config::constants::{ENV_FILENAME, MAX_DEV_FEE_PERCENTAGE, MIN_DEV_FEE_PERCENTAGE}; -use crate::config::secret::read_nsec_env_var; +use crate::config::constants::{ + ENV_FILENAME, MAX_DEV_FEE_PERCENTAGE, MIN_DEV_FEE_PERCENTAGE, MIN_RPC_TOKEN_LEN, + RPC_TOKEN_ENV_VAR, +}; +use crate::config::secret::{read_nsec_env_var, read_rpc_token_env_var}; +use crate::config::types::RpcSettings; use crate::config::wizard; use crate::config::{init_mostro_settings, Settings}; use mostro_core::error::MostroError::{self, *}; use mostro_core::error::ServiceError; +use secrecy::{ExposeSecret, SecretString}; use std::fs; use std::io::IsTerminal; use std::path::PathBuf; @@ -73,6 +78,114 @@ fn validate_mostro_settings(settings: &Settings) -> Result<(), MostroError> { .is_some_and(|bond| bond.enabled), )?; + validate_rpc_settings(&settings.rpc, read_rpc_token_env_var().as_ref())?; + + Ok(()) +} + +/// True when `addr` can only be reached from the host itself. +/// +/// Accepts the `localhost` literal alongside IP literals because +/// `settings.toml` has always allowed it, and bracketed IPv6 (`[::1]`) because +/// that is how the address is written in a `host:port` pair. +fn is_loopback_address(addr: &str) -> bool { + if addr.eq_ignore_ascii_case("localhost") { + return true; + } + addr.trim_start_matches('[') + .trim_end_matches(']') + .parse::() + .is_ok_and(|ip| ip.is_loopback()) +} + +/// Validate the `[rpc]` block (finding 1.5, issue #807). +/// +/// The admin gRPC surface settles disputes, moves escrowed funds, and grants +/// permanent solver rights. Worse, every RPC is executed under the daemon's own +/// identity, which downstream authorization treats as fully privileged +/// (`db::ensure_dispute_finalize_permission`). Reaching the port therefore *is* +/// the authorization, so both guards below are startup-fatal rather than +/// warnings: +/// +/// - `enabled = true` requires `MOSTRO_RPC_TOKEN`, so the interceptor always +/// has a credential to check. A daemon that boots without one would serve an +/// admin API that nothing gates. +/// - A non-loopback `listen_address` requires an explicit `allow_remote = true`. +/// The defaults are safe, but nothing used to stop `0.0.0.0` from publishing +/// the admin API to the LAN silently. +/// +/// A half-configured TLS pair is also fatal: it reads as "TLS is on" while +/// serving plaintext. +fn validate_rpc_settings( + rpc: &RpcSettings, + token: Option<&SecretString>, +) -> Result<(), MostroError> { + if !rpc.enabled { + return Ok(()); + } + + match token { + None => { + return Err(MostroInternalErr(ServiceError::IOError(format!( + "[rpc].enabled = true but {RPC_TOKEN_ENV_VAR} is not set: the admin RPC would \ + accept every caller that can reach the port. Set {RPC_TOKEN_ENV_VAR} in the \ + environment or /.env, or set [rpc].enabled = false." + )))); + } + Some(token) if token.expose_secret().chars().count() < MIN_RPC_TOKEN_LEN => { + return Err(MostroInternalErr(ServiceError::IOError(format!( + "{RPC_TOKEN_ENV_VAR} is shorter than {MIN_RPC_TOKEN_LEN} characters: generate a \ + high-entropy token, e.g. `openssl rand -base64 32`." + )))); + } + Some(_) => {} + } + + if !is_loopback_address(&rpc.listen_address) && !rpc.allow_remote { + return Err(MostroInternalErr(ServiceError::IOError(format!( + "[rpc].listen_address ({:?}) is not a loopback address: this publishes the admin API \ + beyond this host. Set [rpc].allow_remote = true to confirm this is intended, or bind \ + 127.0.0.1.", + rpc.listen_address + )))); + } + + match (rpc.tls_cert_path.as_deref(), rpc.tls_key_path.as_deref()) { + (Some(_), None) => { + return Err(MostroInternalErr(ServiceError::IOError( + "[rpc].tls_cert_path is set without [rpc].tls_key_path: TLS needs both, and the \ + server would otherwise fall back to plaintext." + .to_string(), + ))); + } + (None, Some(_)) => { + return Err(MostroInternalErr(ServiceError::IOError( + "[rpc].tls_key_path is set without [rpc].tls_cert_path: TLS needs both, and the \ + server would otherwise fall back to plaintext." + .to_string(), + ))); + } + (Some(cert), Some(key)) => { + for (field, path) in [("tls_cert_path", cert), ("tls_key_path", key)] { + fs::metadata(path).map_err(|e| { + MostroInternalErr(ServiceError::IOError(format!( + "[rpc].{field} ({path:?}) is not readable: {e}" + ))) + })?; + } + } + (None, None) => { + if !is_loopback_address(&rpc.listen_address) { + tracing::warn!( + "[rpc] is bound to {} without TLS: admin bearer tokens and dispute data cross \ + the network in cleartext. Set [rpc].tls_cert_path and [rpc].tls_key_path, or \ + terminate TLS in a reverse proxy.", + rpc.listen_address + ); + } + } + } + Ok(()) } @@ -492,6 +605,171 @@ mod startup_validation_tests { } } +#[cfg(test)] +mod rpc_validation_tests { + use super::*; + use crate::config::types::RpcSettings; + + fn valid_token() -> SecretString { + SecretString::from("a".repeat(MIN_RPC_TOKEN_LEN)) + } + + fn enabled_rpc() -> RpcSettings { + RpcSettings { + enabled: true, + ..Default::default() + } + } + + fn temp_pem(tag: &str) -> String { + let dir = std::env::temp_dir().join(format!("mostro-rpc-tls-{}", std::process::id())); + std::fs::create_dir_all(&dir).expect("create temp dir"); + let path = dir.join(format!("{tag}.pem")); + std::fs::write(&path, b"not a real certificate").expect("write pem"); + path.to_string_lossy().into_owned() + } + + #[test] + fn disabled_rpc_needs_no_token() { + // The whole block is inert when the server never starts, including a + // deliberately unsafe bind. + let rpc = RpcSettings { + listen_address: "0.0.0.0".to_string(), + ..Default::default() + }; + assert!(validate_rpc_settings(&rpc, None).is_ok()); + } + + #[test] + fn enabled_rpc_without_a_token_is_rejected() { + let err = validate_rpc_settings(&enabled_rpc(), None) + .expect_err("an ungated admin RPC must not boot"); + assert!(err.to_string().contains(RPC_TOKEN_ENV_VAR)); + } + + #[test] + fn enabled_rpc_with_a_short_token_is_rejected() { + let short = SecretString::from("a".repeat(MIN_RPC_TOKEN_LEN - 1)); + let err = validate_rpc_settings(&enabled_rpc(), Some(&short)) + .expect_err("a guessable token must not boot"); + assert!(err.to_string().contains("shorter than")); + } + + #[test] + fn enabled_rpc_on_loopback_with_a_token_is_accepted() { + assert!(validate_rpc_settings(&enabled_rpc(), Some(&valid_token())).is_ok()); + } + + #[test] + fn loopback_is_recognised_in_every_written_form() { + for address in [ + "127.0.0.1", + "127.0.0.53", + "localhost", + "LOCALHOST", + "::1", + "[::1]", + ] { + let rpc = RpcSettings { + enabled: true, + listen_address: address.to_string(), + ..Default::default() + }; + assert!( + validate_rpc_settings(&rpc, Some(&valid_token())).is_ok(), + "{address} should be treated as loopback" + ); + } + } + + #[test] + fn non_loopback_bind_without_allow_remote_is_rejected() { + for address in ["0.0.0.0", "192.168.1.10", "::", "mostro.example.com"] { + let rpc = RpcSettings { + enabled: true, + listen_address: address.to_string(), + ..Default::default() + }; + let err = validate_rpc_settings(&rpc, Some(&valid_token())) + .expect_err("a routable bind must require allow_remote"); + assert!( + err.to_string().contains("allow_remote"), + "{address} should have been refused, got: {err}" + ); + } + } + + #[test] + fn non_loopback_bind_with_allow_remote_is_accepted() { + let rpc = RpcSettings { + enabled: true, + listen_address: "0.0.0.0".to_string(), + allow_remote: true, + ..Default::default() + }; + assert!(validate_rpc_settings(&rpc, Some(&valid_token())).is_ok()); + } + + #[test] + fn half_configured_tls_is_rejected() { + let cert_only = RpcSettings { + tls_cert_path: Some(temp_pem("cert-only")), + ..enabled_rpc() + }; + assert!(validate_rpc_settings(&cert_only, Some(&valid_token())) + .expect_err("cert without key must fail") + .to_string() + .contains("tls_key_path")); + + let key_only = RpcSettings { + tls_key_path: Some(temp_pem("key-only")), + ..enabled_rpc() + }; + assert!(validate_rpc_settings(&key_only, Some(&valid_token())) + .expect_err("key without cert must fail") + .to_string() + .contains("tls_cert_path")); + } + + #[test] + fn unreadable_tls_material_is_rejected() { + let rpc = RpcSettings { + tls_cert_path: Some("/nonexistent/mostro-rpc.pem".to_string()), + tls_key_path: Some(temp_pem("readable-key")), + ..enabled_rpc() + }; + let err = validate_rpc_settings(&rpc, Some(&valid_token())) + .expect_err("unreadable TLS material must fail"); + assert!(err.to_string().contains("not readable")); + } + + #[test] + fn readable_tls_pair_is_accepted() { + let rpc = RpcSettings { + tls_cert_path: Some(temp_pem("pair-cert")), + tls_key_path: Some(temp_pem("pair-key")), + ..enabled_rpc() + }; + assert!(validate_rpc_settings(&rpc, Some(&valid_token())).is_ok()); + } + + #[test] + fn tls_paths_helper_requires_both_halves() { + let rpc = RpcSettings { + tls_cert_path: Some("cert.pem".to_string()), + ..Default::default() + }; + assert!(rpc.tls_paths().is_none()); + + let rpc = RpcSettings { + tls_cert_path: Some("cert.pem".to_string()), + tls_key_path: Some("key.pem".to_string()), + ..Default::default() + }; + assert_eq!(rpc.tls_paths(), Some(("cert.pem", "key.pem"))); + } +} + #[cfg(test)] mod env_file_tests { use super::*; From 3610453f0b36b38deb84ffbe9f93aa810c3e0487 Mon Sep 17 00:00:00 2001 From: Andrea Diaz Correia Date: Thu, 13 Aug 2026 21:56:26 -0300 Subject: [PATCH 02/11] feat(rpc): authenticate and optionally TLS the admin API --- Cargo.lock | 1 + Cargo.toml | 2 +- src/rpc/auth.rs | 156 ++++++++++++++++++++++++++++++++++ src/rpc/mod.rs | 1 + src/rpc/server.rs | 206 ++++++++++++++++++++++++++++++++++++++++----- src/rpc/service.rs | 13 +-- 6 files changed, 353 insertions(+), 26 deletions(-) create mode 100644 src/rpc/auth.rs diff --git a/Cargo.lock b/Cargo.lock index a7744194..5d3dd106 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4319,6 +4319,7 @@ dependencies = [ "socket2 0.6.4", "sync_wrapper", "tokio", + "tokio-rustls", "tokio-stream", "tower", "tower-layer", diff --git a/Cargo.toml b/Cargo.toml index 3623faa7..63b8f85e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -84,7 +84,7 @@ dialoguer = "0.11" dirs = "6.0.0" dotenvy = "0.15.7" clearscreen = "4.0.1" -tonic = "0.14.2" +tonic = { version = "0.14.2", features = ["tls-ring"] } prost = "0.14.1" tonic-prost = "0.14.1" cdk = { version = "0.17.2", default-features = false, features = ["wallet"] } diff --git a/src/rpc/auth.rs b/src/rpc/auth.rs new file mode 100644 index 00000000..dbb722ff --- /dev/null +++ b/src/rpc/auth.rs @@ -0,0 +1,156 @@ +//! Bearer-token authentication for the admin gRPC surface. +//! +//! Every admin RPC is executed under the daemon's own Nostr identity (see +//! `crate::rpc::service`), and downstream authorization grants that identity +//! full privilege — `db::ensure_dispute_finalize_permission` short-circuits its +//! solver-category check for the daemon key. There is therefore no +//! message-level authorization left to fall back on: reaching the port is the +//! authorization, so the transport has to be the gate. +//! +//! Deliberately not rate-limited. `MIN_RPC_TOKEN_LEN` keeps the search space +//! far out of reach of online guessing, and tonic's [`Interceptor`] is +//! synchronous while [`crate::rpc::rate_limiter::RateLimiter`] is async, so +//! wiring one in would mean a second, parallel limiter for no security gain. + +use secrecy::{ExposeSecret, SecretString}; +use std::sync::Arc; +use tonic::service::Interceptor; +use tonic::{Request, Status}; +use tracing::warn; + +const AUTHORIZATION_HEADER: &str = "authorization"; +const BEARER_PREFIX: &str = "Bearer "; + +/// Rejects any request that does not carry the configured bearer token. +#[derive(Clone)] +pub struct BearerAuth { + token: Arc, +} + +impl BearerAuth { + pub fn new(token: SecretString) -> Self { + Self { + token: Arc::new(token), + } + } +} + +impl Interceptor for BearerAuth { + fn call(&mut self, request: Request<()>) -> Result, Status> { + let presented = request + .metadata() + .get(AUTHORIZATION_HEADER) + .and_then(|value| value.to_str().ok()) + .and_then(|value| value.strip_prefix(BEARER_PREFIX)); + + match presented { + Some(candidate) + if constant_time_eq( + candidate.as_bytes(), + self.token.expose_secret().as_bytes(), + ) => + { + Ok(request) + } + // One message for every failure mode: a caller learns whether the + // port is an admin RPC, never whether it guessed part of a token. + _ => { + match request.remote_addr() { + Some(addr) => warn!("Rejected unauthenticated admin RPC from {}", addr.ip()), + None => warn!("Rejected unauthenticated admin RPC from an unknown peer"), + } + Err(Status::unauthenticated("missing or invalid credentials")) + } + } + } +} + +/// Compare two byte strings without leaking how far they matched. +/// +/// Length is not a secret here (the token length is fixed by the operator's +/// config), but the contents are, so the loop always runs to the end. +fn constant_time_eq(a: &[u8], b: &[u8]) -> bool { + if a.len() != b.len() { + return false; + } + let mut difference = 0u8; + for (left, right) in a.iter().zip(b.iter()) { + difference |= left ^ right; + } + difference == 0 +} + +#[cfg(test)] +mod tests { + use super::*; + use tonic::metadata::MetadataValue; + + const TOKEN: &str = "0123456789abcdef0123456789abcdef"; + + fn interceptor() -> BearerAuth { + BearerAuth::new(SecretString::from(TOKEN)) + } + + fn request_with_authorization(value: &str) -> Request<()> { + let mut request = Request::new(()); + request.metadata_mut().insert( + AUTHORIZATION_HEADER, + MetadataValue::try_from(value).expect("header value is ASCII"), + ); + request + } + + #[test] + fn accepts_the_configured_token() { + let result = interceptor().call(request_with_authorization(&format!("Bearer {TOKEN}"))); + assert!(result.is_ok()); + } + + #[test] + fn rejects_a_missing_header() { + let status = interceptor() + .call(Request::new(())) + .expect_err("no credentials must be refused"); + assert_eq!(status.code(), tonic::Code::Unauthenticated); + } + + #[test] + fn rejects_a_token_without_the_bearer_prefix() { + let status = interceptor() + .call(request_with_authorization(TOKEN)) + .expect_err("a bare token must be refused"); + assert_eq!(status.code(), tonic::Code::Unauthenticated); + } + + #[test] + fn rejects_a_wrong_token_of_equal_length() { + let mut wrong = TOKEN.to_string(); + wrong.pop(); + wrong.push('0'); + assert_eq!(wrong.len(), TOKEN.len()); + + let status = interceptor() + .call(request_with_authorization(&format!("Bearer {wrong}"))) + .expect_err("a wrong token must be refused"); + assert_eq!(status.code(), tonic::Code::Unauthenticated); + } + + #[test] + fn rejects_a_token_that_is_a_prefix_of_the_real_one() { + let status = interceptor() + .call(request_with_authorization(&format!( + "Bearer {}", + &TOKEN[..TOKEN.len() - 1] + ))) + .expect_err("a truncated token must be refused"); + assert_eq!(status.code(), tonic::Code::Unauthenticated); + } + + #[test] + fn constant_time_eq_matches_equality() { + assert!(constant_time_eq(b"abc", b"abc")); + assert!(!constant_time_eq(b"abc", b"abd")); + assert!(!constant_time_eq(b"abc", b"ab")); + assert!(constant_time_eq(b"", b"")); + } +} diff --git a/src/rpc/mod.rs b/src/rpc/mod.rs index 60f80d72..7cfc7c59 100644 --- a/src/rpc/mod.rs +++ b/src/rpc/mod.rs @@ -4,6 +4,7 @@ //! for admin operations without going through the Nostr protocol. This is useful //! for local development and admin applications that need low-latency access. +pub mod auth; pub mod rate_limiter; pub mod server; pub mod service; diff --git a/src/rpc/server.rs b/src/rpc/server.rs index 46ff79f7..461dfe8b 100644 --- a/src/rpc/server.rs +++ b/src/rpc/server.rs @@ -1,12 +1,15 @@ //! RPC server implementation for admin operations +use crate::config::constants::RPC_TOKEN_ENV_VAR; +use crate::config::secret::read_rpc_token_env_var; use crate::config::settings::Settings; use crate::lightning::LndConnector; +use crate::rpc::auth::BearerAuth; use crate::rpc::service::AdminServiceImpl; use nostr_sdk::prelude::Keys; use sqlx::{Pool, Sqlite}; use std::sync::Arc; -use tonic::transport::Server; +use tonic::transport::{Identity, Server, ServerTlsConfig}; use tracing::{error, info}; use super::admin::admin_service_server::AdminServiceServer; @@ -15,6 +18,9 @@ use super::admin::admin_service_server::AdminServiceServer; pub struct RpcServer { listen_address: String, port: u16, + /// Certificate and key, or plaintext. Pairing them here keeps + /// "both or neither" a property of the type rather than a runtime check. + tls: Option<(String, String)>, } impl RpcServer { @@ -24,10 +30,18 @@ impl RpcServer { Self { listen_address: rpc_config.listen_address.clone(), port: rpc_config.port, + tls: rpc_config + .tls_paths() + .map(|(cert, key)| (cert.to_string(), key.to_string())), } } /// Start the RPC server + /// + /// Refuses to serve without a bearer token. `validate_rpc_settings` already + /// rejects that combination at startup, so this is the second lock on the + /// same door: a code path that reached here without a token would expose an + /// ungated admin API, and no RPC at all is the safer failure. pub async fn start( &self, my_keys: Keys, @@ -38,12 +52,31 @@ impl RpcServer { .parse() .map_err(|e| format!("Invalid address: {}", e))?; + let token = read_rpc_token_env_var().ok_or_else(|| { + format!("Refusing to start the admin RPC server: {RPC_TOKEN_ENV_VAR} is not set") + })?; + let admin_service = AdminServiceImpl::new(my_keys, pool, ln_client); - info!("Starting RPC server on {}", addr); + let mut builder = Server::builder(); + match &self.tls { + Some((cert_path, key_path)) => { + let cert = std::fs::read(cert_path) + .map_err(|e| format!("Failed to read {cert_path}: {e}"))?; + let key = std::fs::read(key_path) + .map_err(|e| format!("Failed to read {key_path}: {e}"))?; + builder = builder + .tls_config(ServerTlsConfig::new().identity(Identity::from_pem(cert, key)))?; + info!("Starting RPC server on {} (TLS)", addr); + } + None => info!("Starting RPC server on {} (plaintext)", addr), + } - let server = Server::builder() - .add_service(AdminServiceServer::new(admin_service)) + let server = builder + .add_service(AdminServiceServer::with_interceptor( + admin_service, + BearerAuth::new(token), + )) .serve(addr); if let Err(e) = server.await { @@ -82,10 +115,7 @@ mod tests { #[test] fn test_rpc_server_structure() { // Test that RpcServer can be created with explicit values - let server = RpcServer { - listen_address: "localhost".to_string(), - port: 8080, - }; + let server = server_at("localhost", 8080); assert_eq!(server.listen_address, "localhost"); assert_eq!(server.port, 8080); @@ -93,10 +123,7 @@ mod tests { #[test] fn test_address_formatting() { - let server = RpcServer { - listen_address: "127.0.0.1".to_string(), - port: 50051, - }; + let server = server_at("127.0.0.1", 50051); let expected_addr = format!("{}:{}", server.listen_address, server.port); assert_eq!(expected_addr, "127.0.0.1:50051"); @@ -111,6 +138,49 @@ mod tests { let _ = MOSTRO_CONFIG.set(test_settings()); } + /// Plaintext server bound to an explicit address, so the tests below stay + /// readable as `RpcServer` grows optional fields. + fn server_at(listen_address: &str, port: u16) -> RpcServer { + RpcServer { + listen_address: listen_address.to_string(), + port, + tls: None, + } + } + + // `MOSTRO_RPC_TOKEN` is process-wide state, so the tests that touch it run + // serially. Async-aware because the guard is held across `start().await`. + static RPC_TOKEN_LOCK: tokio::sync::Mutex<()> = tokio::sync::Mutex::const_new(()); + + /// Sets `MOSTRO_RPC_TOKEN` for the duration of a test and restores the + /// previous value on drop. + struct RpcTokenGuard { + previous: Option, + } + + impl RpcTokenGuard { + fn set(value: &str) -> Self { + let previous = std::env::var(RPC_TOKEN_ENV_VAR).ok(); + std::env::set_var(RPC_TOKEN_ENV_VAR, value); + Self { previous } + } + + fn unset() -> Self { + let previous = std::env::var(RPC_TOKEN_ENV_VAR).ok(); + std::env::remove_var(RPC_TOKEN_ENV_VAR); + Self { previous } + } + } + + impl Drop for RpcTokenGuard { + fn drop(&mut self) { + match &self.previous { + Some(value) => std::env::set_var(RPC_TOKEN_ENV_VAR, value), + None => std::env::remove_var(RPC_TOKEN_ENV_VAR), + } + } + } + /// Offline `LndConnector` (lazy connect, no network until first RPC). async fn offline_ln_client() -> Arc> { let dir = std::env::temp_dir().join(format!("mostro-rpcsrv-{}", std::process::id())); @@ -148,10 +218,9 @@ mod tests { #[tokio::test] async fn start_rejects_unparseable_address() { init_test_settings(); - let server = RpcServer { - listen_address: "not an address".to_string(), - port: 50051, - }; + let _lock = RPC_TOKEN_LOCK.lock().await; + let _token = RpcTokenGuard::set(&"t".repeat(32)); + let server = server_at("not an address", 50051); let pool = sqlx::SqlitePool::connect("sqlite::memory:").await.unwrap(); let result = server .start(Keys::generate(), Arc::new(pool), offline_ln_client().await) @@ -162,12 +231,11 @@ mod tests { #[tokio::test] async fn start_surfaces_bind_failure() { init_test_settings(); + let _lock = RPC_TOKEN_LOCK.lock().await; + let _token = RpcTokenGuard::set(&"t".repeat(32)); // 8.8.8.8 is not a local interface: the bind fails immediately, so // the server error path is exercised without serving traffic. - let server = RpcServer { - listen_address: "8.8.8.8".to_string(), - port: 1, - }; + let server = server_at("8.8.8.8", 1); let pool = sqlx::SqlitePool::connect("sqlite::memory:").await.unwrap(); let result = server .start(Keys::generate(), Arc::new(pool), offline_ln_client().await) @@ -175,6 +243,104 @@ mod tests { assert!(result.is_err()); } + #[tokio::test] + async fn start_refuses_to_serve_without_a_token() { + init_test_settings(); + let _lock = RPC_TOKEN_LOCK.lock().await; + let _token = RpcTokenGuard::unset(); + // 127.0.0.1:0 would otherwise bind successfully and serve forever, so + // reaching the error path proves the token check ran before the bind. + let server = server_at("127.0.0.1", 0); + let pool = sqlx::SqlitePool::connect("sqlite::memory:").await.unwrap(); + let error = server + .start(Keys::generate(), Arc::new(pool), offline_ln_client().await) + .await + .expect_err("an admin RPC without a token must never serve"); + assert!(error.to_string().contains(RPC_TOKEN_ENV_VAR)); + } + + /// End-to-end proof that the interceptor gates the service that is actually + /// served. The unit tests in `crate::rpc::auth` only cover the interceptor + /// in isolation, so they would stay green if a refactor registered the + /// service without it — which is precisely the regression that would + /// reopen the hole this module exists to close. + /// + /// `GetVersion` is the probe: it touches neither the database nor LND, so + /// the only thing under test is the authentication decision. + #[tokio::test] + async fn served_rpc_rejects_calls_without_the_token() { + use crate::rpc::admin::{admin_service_client::AdminServiceClient, GetVersionRequest}; + use std::time::Duration; + use tonic::metadata::MetadataValue; + use tonic::transport::Channel; + use tonic::Request; + + init_test_settings(); + let _lock = RPC_TOKEN_LOCK.lock().await; + let token = "t".repeat(32); + let _guard = RpcTokenGuard::set(&token); + + // Reserve an ephemeral port and release it: `serve` takes an address, + // not a listener, and a fixed port would collide across parallel runs. + let port = std::net::TcpListener::bind("127.0.0.1:0") + .expect("reserve an ephemeral port") + .local_addr() + .expect("reserved address") + .port(); + + let pool = sqlx::SqlitePool::connect("sqlite::memory:").await.unwrap(); + let ln_client = offline_ln_client().await; + let server = server_at("127.0.0.1", port); + let serving = tokio::spawn(async move { + let _ = server + .start(Keys::generate(), Arc::new(pool), ln_client) + .await; + }); + + let endpoint = format!("http://127.0.0.1:{port}"); + let mut channel = None; + for _ in 0..100 { + match Channel::from_shared(endpoint.clone()) + .expect("valid endpoint") + .connect() + .await + { + Ok(connected) => { + channel = Some(connected); + break; + } + Err(_) => tokio::time::sleep(Duration::from_millis(20)).await, + } + } + let channel = channel.expect("the RPC server should accept connections"); + + let status = AdminServiceClient::new(channel.clone()) + .get_version(GetVersionRequest {}) + .await + .expect_err("an anonymous call must be refused"); + assert_eq!(status.code(), tonic::Code::Unauthenticated); + + let credential: MetadataValue<_> = format!("Bearer {token}") + .parse() + .expect("token is a valid header value"); + let mut authenticated = + AdminServiceClient::with_interceptor(channel, move |mut request: Request<()>| { + request + .metadata_mut() + .insert("authorization", credential.clone()); + Ok(request) + }); + let version = authenticated + .get_version(GetVersionRequest {}) + .await + .expect("an authenticated call must go through") + .into_inner() + .version; + assert_eq!(version, env!("CARGO_PKG_VERSION")); + + serving.abort(); + } + #[test] fn test_default_rpc_settings() { let default_settings = RpcSettings::default(); diff --git a/src/rpc/service.rs b/src/rpc/service.rs index 854230a0..921e4eb0 100644 --- a/src/rpc/service.rs +++ b/src/rpc/service.rs @@ -62,11 +62,14 @@ impl AdminServiceImpl { ); // Admin RPC flows synthesize the inbound event with the node's own - // pubkey in both `identity` and `sender` slots. Authorization is then - // enforced downstream: the caller must be the assigned solver - // (`is_assigned_solver`), with `ensure_dispute_finalize_permission` - // bypassing solver category checks for the daemon key (same as - // `admin_take_dispute`). gRPC transport authenticates the operator. + // pubkey in both `identity` and `sender` slots, so every downstream + // check sees the daemon itself: `ensure_dispute_finalize_permission` + // waives the solver-category check for the daemon key (same as + // `admin_take_dispute`), and `admin_add_solver_action` accepts it + // outright. In other words the handlers below apply *no* caller + // authorization of their own — the bearer-token interceptor in + // `crate::rpc::auth` is the only thing standing between the network + // and these actions. let event = UnwrappedMessage { message: msg.clone(), signature: None, From f56d43e1015d3c794e7d8e9b930ff9d49d7fadb2 Mon Sep 17 00:00:00 2001 From: Andrea Diaz Correia Date: Thu, 13 Aug 2026 21:56:30 -0300 Subject: [PATCH 03/11] docs: document admin RPC authentication --- README.md | 23 ++++++++-- docs/ADMIN_RPC_AND_DISPUTES.md | 5 +- docs/RPC.md | 83 +++++++++++++++++++++++++++++----- docs/RPC_RATE_LIMITING.md | 4 +- docs/STARTUP_AND_CONFIG.md | 7 ++- settings.tpl.toml | 17 ++++++- 6 files changed, 117 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index b4e4e110..9094b697 100644 --- a/README.md +++ b/README.md @@ -655,6 +655,7 @@ bitcoin_price_api_url = "https://api.yadio.io" enabled = false # Set to true to enable gRPC admin interface listen_address = "127.0.0.1" port = 50051 +allow_remote = false # Required to bind a non-loopback address ``` When enabled, exposes gRPC interface on `127.0.0.1:50051` for: @@ -662,6 +663,17 @@ When enabled, exposes gRPC interface on `127.0.0.1:50051` for: - Dispute settlement - Solver management +Enabling it **requires** a bearer token in the `MOSTRO_RPC_TOKEN` environment +variable (or `~/.mostro/.env`); the daemon refuses to start without one, and +refuses a non-loopback bind unless `allow_remote = true`. Reaching this port is +equivalent to holding the operator key — see the security notes in +[docs/RPC.md](docs/RPC.md). + +```bash +# ~/.mostro/.env +MOSTRO_RPC_TOKEN= +``` + --- #### Database @@ -773,17 +785,20 @@ For client documentation, see the respective client repositories. ### Admin Operations (RPC Interface) -If RPC is enabled, use admin tools for dispute resolution: +If RPC is enabled, use admin tools for dispute resolution. Every call must carry +the bearer token; `-plaintext` is only safe over loopback: ```bash +AUTH="authorization: Bearer $MOSTRO_RPC_TOKEN" + # Cancel an order (admin override) -grpcurl -plaintext -d '{"order_id": "abc123"}' localhost:50051 mostro.admin.v1.AdminService/CancelOrder +grpcurl -plaintext -H "$AUTH" -d '{"order_id": "abc123"}' localhost:50051 mostro.admin.v1.AdminService/CancelOrder # Settle disputed order -grpcurl -plaintext -d '{"order_id": "abc123"}' localhost:50051 mostro.admin.v1.AdminService/SettleOrder +grpcurl -plaintext -H "$AUTH" -d '{"order_id": "abc123"}' localhost:50051 mostro.admin.v1.AdminService/SettleOrder # Add dispute solver -grpcurl -plaintext -d '{"solver_pubkey": "npub1..."}' localhost:50051 mostro.admin.v1.AdminService/AddSolver +grpcurl -plaintext -H "$AUTH" -d '{"solver_pubkey": "npub1..."}' localhost:50051 mostro.admin.v1.AdminService/AddSolver ``` --- diff --git a/docs/ADMIN_RPC_AND_DISPUTES.md b/docs/ADMIN_RPC_AND_DISPUTES.md index 83e292bb..e68910d5 100644 --- a/docs/ADMIN_RPC_AND_DISPUTES.md +++ b/docs/ADMIN_RPC_AND_DISPUTES.md @@ -4,8 +4,9 @@ Admin capabilities and dispute resolution paths. ## RPC Server - Source: `src/rpc/server.rs` -- Enable: `settings.toml` → `[rpc] enabled = true` +- Enable: `settings.toml` → `[rpc] enabled = true`, plus a `MOSTRO_RPC_TOKEN` bearer token in the environment (startup is fatal without it). - Binds `listen_address:port`; injects Keys, `Arc>`, `Arc>`. +- Auth: `src/rpc/auth.rs` intercepts every method and rejects any request without the token. This is the *only* caller authorization on the RPC path — the handlers run as the daemon identity, which every downstream check treats as fully privileged. - Uses `tonic`; see `docs/RPC.md` and `proto/admin.proto`. ## Dispute Lifecycle @@ -41,7 +42,7 @@ sequenceDiagram ``` ## Audit and Safety -- Require admin authentication/authorization at message level. +- Nostr admin messages are authorized at message level; the gRPC path is authorized at the transport instead (bearer token), because its synthesized events always carry the daemon identity. - Enforce solver permission levels in the daemon: `read` solvers can assist but cannot execute `admin-settle` or `admin-cancel`. - Record solver, timestamps, and decisions in DB for traceability. - Avoid leaking sensitive data in logs; scrub invoices and keys. diff --git a/docs/RPC.md b/docs/RPC.md index d18ce530..59908748 100644 --- a/docs/RPC.md +++ b/docs/RPC.md @@ -12,7 +12,7 @@ The RPC interface provides a direct communication method for admin operations, c ## Configuration -Add the following section to your `settings.toml` (keys are required; fields have Rust Default implementations but must be present): +Add the following section to your `settings.toml` (`enabled`, `listen_address` and `port` are required keys; fields have Rust Default implementations but must be present): ```toml [rpc] @@ -22,8 +22,25 @@ enabled = true listen_address = "127.0.0.1" # RPC server port (required key; default=50051) port = 50051 +# Optional: acknowledge a non-loopback bind (default=false) +allow_remote = false +# Optional: serve TLS. Both paths are required together. +# tls_cert_path = "/etc/mostro/rpc-cert.pem" +# tls_key_path = "/etc/mostro/rpc-key.pem" ``` +The bearer token is **not** configured here. It is read from the `MOSTRO_RPC_TOKEN` +environment variable, which the daemon also picks up from `/.env`: + +```bash +# ~/.mostro/.env +MOSTRO_RPC_TOKEN= +``` + +`settings.toml` is the file operators paste into bug reports, so it never holds +the credential. The daemon **refuses to start** when `enabled = true` and the +variable is unset or shorter than 32 characters. + ## Available Admin Operations The RPC interface supports the following admin operations: @@ -124,12 +141,29 @@ service AdminService { } ``` +## Authentication + +Every method, `GetVersion` included, requires an `authorization: Bearer ` +header carrying the value of `MOSTRO_RPC_TOKEN`. A missing, malformed or +incorrect token is answered with `UNAUTHENTICATED` before the handler runs, and +the token is compared in constant time so a caller learns nothing from how long +the rejection took. + +```bash +grpcurl -plaintext \ + -H "authorization: Bearer $MOSTRO_RPC_TOKEN" \ + -d '{"order_id": "550e8400-e29b-41d4-a716-446655440000"}' \ + localhost:50051 mostro.admin.v1.AdminService/CancelOrder +``` + ## Client Implementation Example Here's an example of how to create a gRPC client for the Mostro admin RPC: ```rust +use tonic::metadata::MetadataValue; use tonic::transport::Channel; +use tonic::Request; use mostro::rpc::admin::{admin_service_client::AdminServiceClient, CancelOrderRequest}; #[tokio::main] @@ -137,32 +171,59 @@ async fn main() -> Result<(), Box> { let channel = Channel::from_static("http://127.0.0.1:50051") .connect() .await?; - - let mut client = AdminServiceClient::new(channel); - + + let token: MetadataValue<_> = + format!("Bearer {}", std::env::var("MOSTRO_RPC_TOKEN")?).parse()?; + + let mut client = AdminServiceClient::with_interceptor(channel, move |mut req: Request<()>| { + req.metadata_mut().insert("authorization", token.clone()); + Ok(req) + }); + let request = tonic::Request::new(CancelOrderRequest { order_id: "550e8400-e29b-41d4-a716-446655440000".to_string(), request_id: Some("12345".to_string()), }); - + let response = client.cancel_order(request).await?; - + if response.get_ref().success { println!("Order cancelled successfully"); } else { println!("Failed to cancel order: {:?}", response.get_ref().error_message); } - + Ok(()) } ``` ## Security Considerations -- The RPC server listens on localhost by default for security -- Consider implementing authentication/authorization for production use -- The RPC interface provides the same admin capabilities as Nostr-based commands -- Only enable the RPC server in trusted environments +Treat reaching this port as equivalent to holding the Mostro operator key. + +Every RPC is executed under the daemon's own Nostr identity, and the daemon +identity is fully privileged downstream: `ensure_dispute_finalize_permission` +waives its solver-category check for that key, and `admin_add_solver_action` +accepts it outright. The handlers apply no caller authorization of their own, so +the bearer token is the only thing between the network and a settled dispute. + +- **Never expose this port beyond loopback without TLS.** The daemon refuses to + start on a non-loopback `listen_address` unless `allow_remote = true`, and + warns when such a bind runs without TLS. Over plaintext, anyone on the path + reads the bearer token and replays it. +- **The token is a credential, not a setting.** Keep it in `MOSTRO_RPC_TOKEN` + (environment or `/.env`, which the wizard writes with + owner-only permissions), rotate it by restarting with a new value, and never + commit it to `settings.toml`. +- **Container and appliance images publish ports easily.** Wrappers such as + Start9 or Umbrel map container ports to the host or LAN. Verify the mapping + before enabling the RPC; binding `0.0.0.0` inside a container whose port is + published hands the admin API to every device on the network. +- **A compromised token is a compromised node.** An attacker who holds it can + settle disputed orders to their own invoice or grant themselves permanent + solver rights over Nostr, which survives a token rotation. +- The RPC interface provides the same admin capabilities as Nostr-based + commands, without the Nostr-side key requirement. ## Debugging diff --git a/docs/RPC_RATE_LIMITING.md b/docs/RPC_RATE_LIMITING.md index d0519d28..4ff52334 100644 --- a/docs/RPC_RATE_LIMITING.md +++ b/docs/RPC_RATE_LIMITING.md @@ -68,8 +68,8 @@ How the original issue’s ideas map to the codebase today: | Per-IP rate limiting | **`check_rate_limit`** runs before the handler body. | | Exponential backoff / lockout | Implemented inside **`RateLimiter`**; **not** triggered by **`ValidateDbPassword`** (no **`record_failure`**). | | Audit logging | **tracing** in service + limiter. | -| Localhost-only | Default RPC bind **`127.0.0.1`** (see `settings.toml` / `docs/RPC.md`). | -| Strong auth | Out of scope for this stub; would need API keys or similar. | +| Localhost-only | Default RPC bind **`127.0.0.1`**, and a non-loopback bind now requires an explicit **`allow_remote = true`** (see `settings.toml` / `docs/RPC.md`). | +| Strong auth | Implemented in **`src/rpc/auth.rs`**: a **`MOSTRO_RPC_TOKEN`** bearer token, checked in constant time by a tonic interceptor on every method. The interceptor is deliberately **not** rate-limited — the token's entropy, not a counter, is what defeats guessing. | ## Testing diff --git a/docs/STARTUP_AND_CONFIG.md b/docs/STARTUP_AND_CONFIG.md index 2efad905..6bf7643a 100644 --- a/docs/STARTUP_AND_CONFIG.md +++ b/docs/STARTUP_AND_CONFIG.md @@ -148,11 +148,14 @@ Configuration is loaded from `~/.mostro/settings.toml` (template: `settings.tpl. - `picture` (Option\): URL to avatar image, recommended square max 128x128px (default: None) - `website` (Option\): Operator website URL (default: None) -**RPC** (`src/config/types.rs:55-74`): +**RPC** (`RpcSettings` in `src/config/types.rs`): - `enabled` (bool): Enable RPC server (Rust Default: false) - `listen_address` (String): Bind address (Rust Default: "127.0.0.1") - `port` (u16): Listen port (Rust Default: 50051) -- Note: These fields have a Rust Default implementation, but `settings.toml` must still include these keys. If a key is present but empty or omitted by tooling, the daemon falls back to the Rust Default value. +- `allow_remote` (bool): Acknowledge a non-loopback bind (Rust Default: false) +- `tls_cert_path` / `tls_key_path` (Option\): PEM material for TLS; required together (Rust Default: None) +- Note: `enabled`, `listen_address` and `port` have a Rust Default implementation, but `settings.toml` must still include these keys. If a key is present but empty or omitted by tooling, the daemon falls back to the Rust Default value. The remaining fields are optional. +- The bearer token lives in the `MOSTRO_RPC_TOKEN` environment variable, never in `settings.toml`. `validate_rpc_settings` (`src/config/util.rs`) makes startup fatal when `enabled = true` and the token is missing or under 32 characters, when a non-loopback address is bound without `allow_remote = true`, or when only one half of the TLS pair is configured. See `docs/RPC.md`. ## Global Variables diff --git a/settings.tpl.toml b/settings.tpl.toml index 8ad5fc4e..638a9a3d 100644 --- a/settings.tpl.toml +++ b/settings.tpl.toml @@ -126,7 +126,13 @@ fee_audit_days = 365 dm_days = 30 [rpc] -# Enable RPC server for direct admin communication +# Enable RPC server for direct admin communication. +# +# The admin RPC settles disputes, cancels orders and grants solver rights, and +# every call runs with the daemon's own privileges. Enabling it REQUIRES the +# MOSTRO_RPC_TOKEN environment variable (set it in the environment or in +# /.env, never here); the daemon refuses to start otherwise. +# Generate one with: openssl rand -base64 32 enabled = false # RPC server listen address listen_address = "127.0.0.1" @@ -134,6 +140,15 @@ listen_address = "127.0.0.1" port = 50051 # Duration in seconds after which inactive rate-limiter entries are evicted # rate_limiter_stale_duration = 3600 +# Acknowledge binding to a non-loopback address. The daemon refuses to start on +# a routable address unless this is true, so the admin API is never published to +# a LAN by accident. +# allow_remote = false +# Serve the admin RPC over TLS. Both paths are required together; without them +# the bearer token crosses the network in cleartext, so configure them (or a +# TLS-terminating reverse proxy) whenever allow_remote is true. +# tls_cert_path = "/etc/mostro/rpc-cert.pem" +# tls_key_path = "/etc/mostro/rpc-key.pem" # Multi-source price providers (see docs/PRICE_PROVIDERS.md). # Absent section ≡ legacy single-source behaviour synthesised from From 9a65173dbe94855f7bee300cbcb2ad8f12a13dee Mon Sep 17 00:00:00 2001 From: Andrea Diaz Correia Date: Thu, 13 Aug 2026 23:00:36 -0300 Subject: [PATCH 04/11] fix(rpc): reject tokens that cannot be sent as a header --- src/config/util.rs | 62 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 61 insertions(+), 1 deletion(-) diff --git a/src/config/util.rs b/src/config/util.rs index 96a850eb..740d49d1 100644 --- a/src/config/util.rs +++ b/src/config/util.rs @@ -138,6 +138,17 @@ fn validate_rpc_settings( high-entropy token, e.g. `openssl rand -base64 32`." )))); } + // The token travels verbatim inside an HTTP/2 `authorization` header. + // Anything outside printable ASCII cannot be carried there, so a daemon + // that accepted it would boot and then refuse every client — a failure + // that looks like a broken build rather than a typo in the token. + Some(token) if !token.expose_secret().chars().all(|c| c.is_ascii_graphic()) => { + return Err(MostroInternalErr(ServiceError::IOError(format!( + "{RPC_TOKEN_ENV_VAR} must contain only printable ASCII characters and no spaces: \ + it is sent as an HTTP header, so any other value can never authenticate a \ + client. `openssl rand -base64 32` produces a valid token." + )))); + } Some(_) => {} } @@ -167,11 +178,29 @@ fn validate_rpc_settings( } (Some(cert), Some(key)) => { for (field, path) in [("tls_cert_path", cert), ("tls_key_path", key)] { - fs::metadata(path).map_err(|e| { + // Open rather than stat: `fs::metadata` succeeds for a file the + // daemon has no permission to read. Opening is still not + // enough on its own — on Linux a directory opens fine and only + // fails on read — so the file type is checked through the + // handle, which is the capability `RpcServer::start` needs. + let opened = fs::File::open(path).map_err(|e| { MostroInternalErr(ServiceError::IOError(format!( "[rpc].{field} ({path:?}) is not readable: {e}" ))) })?; + let is_regular_file = opened + .metadata() + .map(|metadata| metadata.is_file()) + .map_err(|e| { + MostroInternalErr(ServiceError::IOError(format!( + "[rpc].{field} ({path:?}) could not be inspected: {e}" + ))) + })?; + if !is_regular_file { + return Err(MostroInternalErr(ServiceError::IOError(format!( + "[rpc].{field} ({path:?}) is not a regular file" + )))); + } } } (None, None) => { @@ -660,6 +689,37 @@ mod rpc_validation_tests { assert!(validate_rpc_settings(&enabled_rpc(), Some(&valid_token())).is_ok()); } + #[test] + fn a_token_that_cannot_travel_in_a_header_is_rejected() { + // Long enough to clear the length gate, but unusable as an HTTP header + // value: accepting it would boot a daemon that refuses every client. + for unusable in ["é".repeat(MIN_RPC_TOKEN_LEN), "a".repeat(31) + " b"] { + let token = SecretString::from(unusable.clone()); + let err = validate_rpc_settings(&enabled_rpc(), Some(&token)) + .expect_err("a token that cannot be sent must not boot"); + assert!( + err.to_string().contains("printable ASCII"), + "{unusable:?} should have been refused as unsendable, got: {err}" + ); + } + } + + #[test] + fn a_directory_is_not_accepted_as_tls_material() { + // Both `fs::metadata` and `File::open` succeed on a directory, so only + // the file-type check rejects this. + let dir = std::env::temp_dir().join(format!("mostro-rpc-tls-{}", std::process::id())); + std::fs::create_dir_all(&dir).expect("create temp dir"); + let rpc = RpcSettings { + tls_cert_path: Some(dir.to_string_lossy().into_owned()), + tls_key_path: Some(temp_pem("dir-case-key")), + ..enabled_rpc() + }; + let err = validate_rpc_settings(&rpc, Some(&valid_token())) + .expect_err("a directory is not a certificate"); + assert!(err.to_string().contains("not a regular file")); + } + #[test] fn loopback_is_recognised_in_every_written_form() { for address in [ From 03012914f28161c5cff6bfae70ee8bec04133b4e Mon Sep 17 00:00:00 2001 From: Andrea Diaz Correia Date: Thu, 13 Aug 2026 23:00:40 -0300 Subject: [PATCH 05/11] fix(rpc): exit when the admin API fails to start --- src/main.rs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/main.rs b/src/main.rs index 951ff5e2..178c3fa5 100644 --- a/src/main.rs +++ b/src/main.rs @@ -274,9 +274,18 @@ async fn main() -> Result<()> { let rpc_ln_client = Arc::new(tokio::sync::Mutex::new(ln_client.clone())); tokio::spawn(async move { + // `[rpc].enabled = true` is an explicit request for the admin API, + // and every other misconfiguration in that block is startup-fatal + // (see `config::util::validate_rpc_settings`). Surviving a failed + // listener would leave the operator believing an interface is up + // that nothing is serving — including when TLS material is + // unreadable or malformed, which only surfaces here. match rpc_server.start(rpc_keys, rpc_pool, rpc_ln_client).await { - Ok(_) => tracing::info!("RPC server started successfully"), - Err(e) => tracing::error!("RPC server failed to start: {}", e), + Ok(_) => tracing::warn!("RPC server stopped"), + Err(e) => { + tracing::error!("RPC server failed to start: {}", e); + exit(1); + } } }); } From 6420b2dd19d331d44ecc2397dfd106225d470405 Mon Sep 17 00:00:00 2001 From: Andrea Diaz Correia Date: Thu, 13 Aug 2026 23:00:40 -0300 Subject: [PATCH 06/11] refactor(rpc): compare credentials with subtle --- Cargo.lock | 1 + Cargo.toml | 1 + src/rpc/auth.rs | 76 +++++++++++++++++++++++++++++++++---------------- 3 files changed, 53 insertions(+), 25 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5d3dd106..89696cac 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2372,6 +2372,7 @@ dependencies = [ "serde", "serde_json", "sqlx", + "subtle", "tokio", "toml", "tonic 0.14.6", diff --git a/Cargo.toml b/Cargo.toml index 63b8f85e..f1ff048d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -90,6 +90,7 @@ tonic-prost = "0.14.1" cdk = { version = "0.17.2", default-features = false, features = ["wallet"] } secp256k1 = { version = "0.30", features = ["serde"] } secrecy = { version = "0.10", features = ["serde"] } +subtle = "2.6" zeroize = "1.8" [dev-dependencies] diff --git a/src/rpc/auth.rs b/src/rpc/auth.rs index dbb722ff..c882b2cb 100644 --- a/src/rpc/auth.rs +++ b/src/rpc/auth.rs @@ -14,12 +14,16 @@ use secrecy::{ExposeSecret, SecretString}; use std::sync::Arc; +use subtle::ConstantTimeEq; use tonic::service::Interceptor; use tonic::{Request, Status}; use tracing::warn; const AUTHORIZATION_HEADER: &str = "authorization"; -const BEARER_PREFIX: &str = "Bearer "; +/// RFC 7235 defines the auth-scheme as case-insensitive, and proxies do +/// normalize it, so the scheme is matched without regard to case. The +/// credential that follows is still compared byte for byte. +const BEARER_SCHEME: &str = "Bearer"; /// Rejects any request that does not carry the configured bearer token. #[derive(Clone)] @@ -41,15 +45,15 @@ impl Interceptor for BearerAuth { .metadata() .get(AUTHORIZATION_HEADER) .and_then(|value| value.to_str().ok()) - .and_then(|value| value.strip_prefix(BEARER_PREFIX)); + .and_then(|value| { + let (scheme, credential) = value.split_once(' ')?; + scheme + .eq_ignore_ascii_case(BEARER_SCHEME) + .then_some(credential) + }); match presented { - Some(candidate) - if constant_time_eq( - candidate.as_bytes(), - self.token.expose_secret().as_bytes(), - ) => - { + Some(candidate) if credentials_match(candidate, self.token.expose_secret()) => { Ok(request) } // One message for every failure mode: a caller learns whether the @@ -65,19 +69,20 @@ impl Interceptor for BearerAuth { } } -/// Compare two byte strings without leaking how far they matched. +/// Compare the presented credential against the configured one without leaking +/// how far the two matched. /// -/// Length is not a secret here (the token length is fixed by the operator's -/// config), but the contents are, so the loop always runs to the end. -fn constant_time_eq(a: &[u8], b: &[u8]) -> bool { - if a.len() != b.len() { - return false; - } - let mut difference = 0u8; - for (left, right) in a.iter().zip(b.iter()) { - difference |= left ^ right; - } - difference == 0 +/// `subtle` is used rather than a hand-written loop because only its +/// optimization barrier makes the constant-time property a guarantee the +/// compiler must honour. `ct_eq` on slices already answers `false` for +/// mismatched lengths, and the token's length is set by the operator's config +/// rather than being itself a secret. +fn credentials_match(presented: &str, configured: &str) -> bool { + presented + .as_bytes() + .ct_eq(configured.as_bytes()) + .unwrap_u8() + == 1 } #[cfg(test)] @@ -106,6 +111,24 @@ mod tests { assert!(result.is_ok()); } + #[test] + fn accepts_any_casing_of_the_bearer_scheme() { + // RFC 7235: the auth-scheme is case-insensitive, and proxies rewrite it. + for scheme in ["Bearer", "bearer", "BEARER", "BeArEr"] { + let result = + interceptor().call(request_with_authorization(&format!("{scheme} {TOKEN}"))); + assert!(result.is_ok(), "{scheme} should be accepted"); + } + } + + #[test] + fn rejects_another_scheme_carrying_the_right_token() { + let status = interceptor() + .call(request_with_authorization(&format!("Basic {TOKEN}"))) + .expect_err("only the Bearer scheme is accepted"); + assert_eq!(status.code(), tonic::Code::Unauthenticated); + } + #[test] fn rejects_a_missing_header() { let status = interceptor() @@ -147,10 +170,13 @@ mod tests { } #[test] - fn constant_time_eq_matches_equality() { - assert!(constant_time_eq(b"abc", b"abc")); - assert!(!constant_time_eq(b"abc", b"abd")); - assert!(!constant_time_eq(b"abc", b"ab")); - assert!(constant_time_eq(b"", b"")); + fn credentials_match_only_on_exact_equality() { + assert!(credentials_match("abc", "abc")); + assert!(!credentials_match("abc", "abd")); + assert!(!credentials_match("abc", "ab")); + assert!(!credentials_match("ab", "abc")); + // The credential itself stays case-sensitive even though the scheme + // is not. + assert!(!credentials_match("ABC", "abc")); } } From b030d94af83b4d68c78d029a002b3d641651d1ae Mon Sep 17 00:00:00 2001 From: Andrea Diaz Correia Date: Thu, 13 Aug 2026 23:00:44 -0300 Subject: [PATCH 07/11] docs: narrow the timing claim and warn about argv --- README.md | 4 ++++ docs/RPC.md | 29 +++++++++++++++++++++-------- docs/RPC_RATE_LIMITING.md | 2 +- 3 files changed, 26 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 9094b697..748624e0 100644 --- a/README.md +++ b/README.md @@ -801,6 +801,10 @@ grpcurl -plaintext -H "$AUTH" -d '{"order_id": "abc123"}' localhost:50051 mostro grpcurl -plaintext -H "$AUTH" -d '{"solver_pubkey": "npub1..."}' localhost:50051 mostro.admin.v1.AdminService/AddSolver ``` +The shell expands `$AUTH` into `grpcurl`'s arguments, so any local user can read +the token with `ps`. On a host you do not have to yourself, use the Rust client +in [docs/RPC.md](docs/RPC.md) instead, which keeps the token in the environment. + --- ### Querying Audit Events diff --git a/docs/RPC.md b/docs/RPC.md index 59908748..12922807 100644 --- a/docs/RPC.md +++ b/docs/RPC.md @@ -144,10 +144,16 @@ service AdminService { ## Authentication Every method, `GetVersion` included, requires an `authorization: Bearer ` -header carrying the value of `MOSTRO_RPC_TOKEN`. A missing, malformed or -incorrect token is answered with `UNAUTHENTICATED` before the handler runs, and -the token is compared in constant time so a caller learns nothing from how long -the rejection took. +header carrying the value of `MOSTRO_RPC_TOKEN`. The scheme is matched +case-insensitively per RFC 7235; the token itself must match exactly. A missing, +malformed or incorrect token is answered with `UNAUTHENTICATED` before the +handler runs, and the token comparison itself is constant-time +(`fn credentials_match` in `src/rpc/auth.rs`), so it does not leak how many +bytes matched. Total request latency is not claimed to be constant: header +parsing, logging and transport all contribute to it. + +The token must be printable ASCII with no spaces, since it is sent verbatim as +an HTTP header. The daemon rejects anything else at startup. ```bash grpcurl -plaintext \ @@ -156,6 +162,11 @@ grpcurl -plaintext \ localhost:50051 mostro.admin.v1.AdminService/CancelOrder ``` +> **Shared hosts:** the shell expands `$MOSTRO_RPC_TOKEN` into `grpcurl`'s +> arguments, where any local user can read it with `ps`. On a host with other +> users, drive the API from the Rust client below instead, which keeps the token +> in the process environment. + ## Client Implementation Example Here's an example of how to create a gRPC client for the Mostro admin RPC: @@ -202,10 +213,12 @@ async fn main() -> Result<(), Box> { Treat reaching this port as equivalent to holding the Mostro operator key. Every RPC is executed under the daemon's own Nostr identity, and the daemon -identity is fully privileged downstream: `ensure_dispute_finalize_permission` -waives its solver-category check for that key, and `admin_add_solver_action` -accepts it outright. The handlers apply no caller authorization of their own, so -the bearer token is the only thing between the network and a settled dispute. +identity is fully privileged downstream: `fn ensure_dispute_finalize_permission` +in `src/db.rs` waives its solver-category check for that key, and +`fn admin_add_solver_action` in `src/app/admin_add_solver.rs` accepts it +outright. The handlers apply no caller authorization of their own, so the bearer +token (`fn call` for `BearerAuth` in `src/rpc/auth.rs`) is the only thing between +the network and a settled dispute. - **Never expose this port beyond loopback without TLS.** The daemon refuses to start on a non-loopback `listen_address` unless `allow_remote = true`, and diff --git a/docs/RPC_RATE_LIMITING.md b/docs/RPC_RATE_LIMITING.md index 4ff52334..effac01c 100644 --- a/docs/RPC_RATE_LIMITING.md +++ b/docs/RPC_RATE_LIMITING.md @@ -69,7 +69,7 @@ How the original issue’s ideas map to the codebase today: | Exponential backoff / lockout | Implemented inside **`RateLimiter`**; **not** triggered by **`ValidateDbPassword`** (no **`record_failure`**). | | Audit logging | **tracing** in service + limiter. | | Localhost-only | Default RPC bind **`127.0.0.1`**, and a non-loopback bind now requires an explicit **`allow_remote = true`** (see `settings.toml` / `docs/RPC.md`). | -| Strong auth | Implemented in **`src/rpc/auth.rs`**: a **`MOSTRO_RPC_TOKEN`** bearer token, checked in constant time by a tonic interceptor on every method. The interceptor is deliberately **not** rate-limited — the token's entropy, not a counter, is what defeats guessing. | +| Strong auth | Implemented by **`fn call`** for **`BearerAuth`** in **`src/rpc/auth.rs`**: a **`MOSTRO_RPC_TOKEN`** bearer token, compared in constant time (**`fn credentials_match`**, same file) on every method. The interceptor is deliberately **not** rate-limited — the token's entropy, not a counter, is what defeats guessing. | ## Testing From 79b0b8673dfeec591ee51f269bfb85ca2d9f806e Mon Sep 17 00:00:00 2001 From: Andrea Diaz Correia Date: Thu, 13 Aug 2026 23:39:09 -0300 Subject: [PATCH 08/11] refactor(rpc): bind listener before spawning accept loop --- src/main.rs | 27 +++++--- src/rpc/server.rs | 163 ++++++++++++++++++++++++++++------------------ 2 files changed, 116 insertions(+), 74 deletions(-) diff --git a/src/main.rs b/src/main.rs index 178c3fa5..2a2308a0 100644 --- a/src/main.rs +++ b/src/main.rs @@ -273,17 +273,26 @@ async fn main() -> Result<()> { let rpc_pool = get_db_pool(); let rpc_ln_client = Arc::new(tokio::sync::Mutex::new(ln_client.clone())); + // `[rpc].enabled = true` is an explicit request for the admin API, and + // every other misconfiguration in that block is startup-fatal (see + // `config::util::validate_rpc_settings`). `bind` resolves the listener + // and the TLS material here, before any of the startup below runs, so + // the daemon never advances past this point believing an admin + // interface is up that nothing is serving. + let rpc_serving = match rpc_server.bind(rpc_keys, rpc_pool, rpc_ln_client) { + Ok(serving) => serving, + Err(e) => { + tracing::error!("RPC server failed to start: {}", e); + exit(1); + } + }; + + // Only the accept loop is detached; it is already listening. tokio::spawn(async move { - // `[rpc].enabled = true` is an explicit request for the admin API, - // and every other misconfiguration in that block is startup-fatal - // (see `config::util::validate_rpc_settings`). Surviving a failed - // listener would leave the operator believing an interface is up - // that nothing is serving — including when TLS material is - // unreadable or malformed, which only surfaces here. - match rpc_server.start(rpc_keys, rpc_pool, rpc_ln_client).await { - Ok(_) => tracing::warn!("RPC server stopped"), + match rpc_serving.await { + Ok(()) => tracing::warn!("RPC server stopped"), Err(e) => { - tracing::error!("RPC server failed to start: {}", e); + tracing::error!("RPC server error: {}", e); exit(1); } } diff --git a/src/rpc/server.rs b/src/rpc/server.rs index 461dfe8b..022f5598 100644 --- a/src/rpc/server.rs +++ b/src/rpc/server.rs @@ -9,8 +9,9 @@ use crate::rpc::service::AdminServiceImpl; use nostr_sdk::prelude::Keys; use sqlx::{Pool, Sqlite}; use std::sync::Arc; +use tonic::transport::server::TcpIncoming; use tonic::transport::{Identity, Server, ServerTlsConfig}; -use tracing::{error, info}; +use tracing::info; use super::admin::admin_service_server::AdminServiceServer; @@ -36,19 +37,29 @@ impl RpcServer { } } - /// Start the RPC server + /// Acquire the listener and return the future that serves it. /// - /// Refuses to serve without a bearer token. `validate_rpc_settings` already - /// rejects that combination at startup, so this is the second lock on the - /// same door: a code path that reached here without a token would expose an - /// ungated admin API, and no RPC at all is the safer failure. - pub async fn start( + /// Everything that can fail on the way up happens here, before the caller + /// gets anything to detach: a missing bearer token, unusable TLS material, + /// an address already in use. The returned future only accepts connections, + /// so a caller that awaits this function knows the admin API is listening + /// and gated before it lets the rest of the daemon proceed — `[rpc].enabled + /// = true` becomes an invariant rather than a hope. + /// + /// Refusing to serve without a token is deliberately redundant with + /// `config::util::validate_rpc_settings`: a code path that reached here + /// without one would expose an ungated admin API, and no RPC at all is the + /// safer failure. + pub fn bind( &self, my_keys: Keys, pool: Arc>, ln_client: Arc>, - ) -> Result<(), Box> { - let addr = format!("{}:{}", self.listen_address, self.port) + ) -> Result< + impl std::future::Future> + Send + 'static, + Box, + > { + let addr: std::net::SocketAddr = format!("{}:{}", self.listen_address, self.port) .parse() .map_err(|e| format!("Invalid address: {}", e))?; @@ -59,32 +70,33 @@ impl RpcServer { let admin_service = AdminServiceImpl::new(my_keys, pool, ln_client); let mut builder = Server::builder(); - match &self.tls { + let transport = match &self.tls { Some((cert_path, key_path)) => { let cert = std::fs::read(cert_path) .map_err(|e| format!("Failed to read {cert_path}: {e}"))?; let key = std::fs::read(key_path) .map_err(|e| format!("Failed to read {key_path}: {e}"))?; + // Malformed PEM is rejected by `tls_config`, so it surfaces + // here rather than inside the detached serving future. builder = builder .tls_config(ServerTlsConfig::new().identity(Identity::from_pem(cert, key)))?; - info!("Starting RPC server on {} (TLS)", addr); + "TLS" } - None => info!("Starting RPC server on {} (plaintext)", addr), - } + None => "plaintext", + }; + + // Binds eagerly: an occupied port is a startup error, not a surprise + // discovered later by whoever happens to read the logs. + let incoming = + TcpIncoming::bind(addr).map_err(|e| format!("Failed to bind {addr}: {e}"))?; + info!("RPC server listening on {} ({})", addr, transport); - let server = builder + Ok(builder .add_service(AdminServiceServer::with_interceptor( admin_service, BearerAuth::new(token), )) - .serve(addr); - - if let Err(e) = server.await { - error!("RPC server error: {}", e); - return Err(Box::new(e)); - } - - Ok(()) + .serve_with_incoming(incoming)) } /// Check if RPC server is enabled @@ -149,7 +161,7 @@ mod tests { } // `MOSTRO_RPC_TOKEN` is process-wide state, so the tests that touch it run - // serially. Async-aware because the guard is held across `start().await`. + // serially. Async-aware because the guard is held across awaits. static RPC_TOKEN_LOCK: tokio::sync::Mutex<()> = tokio::sync::Mutex::const_new(()); /// Sets `MOSTRO_RPC_TOKEN` for the duration of a test and restores the @@ -216,49 +228,80 @@ mod tests { } #[tokio::test] - async fn start_rejects_unparseable_address() { + async fn bind_rejects_unparseable_address() { init_test_settings(); let _lock = RPC_TOKEN_LOCK.lock().await; let _token = RpcTokenGuard::set(&"t".repeat(32)); let server = server_at("not an address", 50051); let pool = sqlx::SqlitePool::connect("sqlite::memory:").await.unwrap(); - let result = server - .start(Keys::generate(), Arc::new(pool), offline_ln_client().await) - .await; + let result = server.bind(Keys::generate(), Arc::new(pool), offline_ln_client().await); assert!(result.is_err()); } + /// The startup invariant the daemon depends on: a listener that cannot be + /// acquired is reported by `bind` itself, so `main` learns about it before + /// it detaches anything or continues booting. #[tokio::test] - async fn start_surfaces_bind_failure() { + async fn bind_surfaces_bind_failure_before_returning() { init_test_settings(); let _lock = RPC_TOKEN_LOCK.lock().await; let _token = RpcTokenGuard::set(&"t".repeat(32)); - // 8.8.8.8 is not a local interface: the bind fails immediately, so - // the server error path is exercised without serving traffic. + // 8.8.8.8 is not a local interface, so the bind fails immediately. let server = server_at("8.8.8.8", 1); let pool = sqlx::SqlitePool::connect("sqlite::memory:").await.unwrap(); - let result = server - .start(Keys::generate(), Arc::new(pool), offline_ln_client().await) - .await; - assert!(result.is_err()); + let error = server + .bind(Keys::generate(), Arc::new(pool), offline_ln_client().await) + .err() + .expect("an unavailable address must fail before serving"); + assert!(error.to_string().contains("Failed to bind")); } #[tokio::test] - async fn start_refuses_to_serve_without_a_token() { + async fn bind_refuses_to_serve_without_a_token() { init_test_settings(); let _lock = RPC_TOKEN_LOCK.lock().await; let _token = RpcTokenGuard::unset(); - // 127.0.0.1:0 would otherwise bind successfully and serve forever, so - // reaching the error path proves the token check ran before the bind. + // 127.0.0.1:0 would otherwise bind successfully, so reaching the error + // path proves the token check runs before anything starts listening. let server = server_at("127.0.0.1", 0); let pool = sqlx::SqlitePool::connect("sqlite::memory:").await.unwrap(); let error = server - .start(Keys::generate(), Arc::new(pool), offline_ln_client().await) - .await - .expect_err("an admin RPC without a token must never serve"); + .bind(Keys::generate(), Arc::new(pool), offline_ln_client().await) + .err() + .expect("an admin RPC without a token must never serve"); assert!(error.to_string().contains(RPC_TOKEN_ENV_VAR)); } + #[tokio::test] + async fn bind_rejects_malformed_tls_material() { + init_test_settings(); + let _lock = RPC_TOKEN_LOCK.lock().await; + let _token = RpcTokenGuard::set(&"t".repeat(32)); + // Readable files that are not valid PEM: config validation accepts + // them, so `bind` is the layer that has to catch this — and it must do + // so before returning, or the daemon boots without the API it was told + // to serve. + let dir = std::env::temp_dir().join(format!("mostro-rpc-badtls-{}", std::process::id())); + std::fs::create_dir_all(&dir).expect("create temp dir"); + let cert = dir.join("cert.pem"); + let key = dir.join("key.pem"); + std::fs::write(&cert, b"not a certificate").expect("write cert"); + std::fs::write(&key, b"not a key").expect("write key"); + + let server = RpcServer { + listen_address: "127.0.0.1".to_string(), + port: 0, + tls: Some(( + cert.to_string_lossy().into_owned(), + key.to_string_lossy().into_owned(), + )), + }; + let pool = sqlx::SqlitePool::connect("sqlite::memory:").await.unwrap(); + assert!(server + .bind(Keys::generate(), Arc::new(pool), offline_ln_client().await) + .is_err()); + } + /// End-to-end proof that the interceptor gates the service that is actually /// served. The unit tests in `crate::rpc::auth` only cover the interceptor /// in isolation, so they would stay green if a refactor registered the @@ -270,7 +313,6 @@ mod tests { #[tokio::test] async fn served_rpc_rejects_calls_without_the_token() { use crate::rpc::admin::{admin_service_client::AdminServiceClient, GetVersionRequest}; - use std::time::Duration; use tonic::metadata::MetadataValue; use tonic::transport::Channel; use tonic::Request; @@ -280,8 +322,8 @@ mod tests { let token = "t".repeat(32); let _guard = RpcTokenGuard::set(&token); - // Reserve an ephemeral port and release it: `serve` takes an address, - // not a listener, and a fixed port would collide across parallel runs. + // Reserve an ephemeral port and release it, so parallel runs of the + // suite cannot collide on a fixed one. let port = std::net::TcpListener::bind("127.0.0.1:0") .expect("reserve an ephemeral port") .local_addr() @@ -291,28 +333,19 @@ mod tests { let pool = sqlx::SqlitePool::connect("sqlite::memory:").await.unwrap(); let ln_client = offline_ln_client().await; let server = server_at("127.0.0.1", port); - let serving = tokio::spawn(async move { - let _ = server - .start(Keys::generate(), Arc::new(pool), ln_client) - .await; - }); - - let endpoint = format!("http://127.0.0.1:{port}"); - let mut channel = None; - for _ in 0..100 { - match Channel::from_shared(endpoint.clone()) - .expect("valid endpoint") - .connect() - .await - { - Ok(connected) => { - channel = Some(connected); - break; - } - Err(_) => tokio::time::sleep(Duration::from_millis(20)).await, - } - } - let channel = channel.expect("the RPC server should accept connections"); + let serving = server + .bind(Keys::generate(), Arc::new(pool), ln_client) + .expect("bind must succeed on a free loopback port"); + let serving = tokio::spawn(serving); + + // No retry loop: `bind` returned, so the listener already exists and + // the connection below must succeed on the first attempt. A retry here + // would hide exactly the regression this asserts against. + let channel = Channel::from_shared(format!("http://127.0.0.1:{port}")) + .expect("valid endpoint") + .connect() + .await + .expect("the listener is open as soon as bind returns"); let status = AdminServiceClient::new(channel.clone()) .get_version(GetVersionRequest {}) From 216b6e730f8e28b5f4cc4a1b129e7eba473e0482 Mon Sep 17 00:00:00 2001 From: Andrea Diaz Correia Date: Fri, 21 Aug 2026 17:01:31 -0300 Subject: [PATCH 09/11] docs(rpc): clarify that listen_address must be an IP literal --- README.md | 2 +- docs/RPC.md | 6 ++ docs/RPC_RATE_LIMITING.md | 2 +- docs/STARTUP_AND_CONFIG.md | 4 +- settings.tpl.toml | 4 +- src/config/util.rs | 68 ++++++++++----- src/main.rs | 6 +- src/rpc/auth.rs | 170 +++++++++++++++++++++++++++++++++++-- src/rpc/server.rs | 117 +++++++++++++++++++------ 9 files changed, 315 insertions(+), 64 deletions(-) diff --git a/README.md b/README.md index 748624e0..4fcbb0a4 100644 --- a/README.md +++ b/README.md @@ -653,7 +653,7 @@ bitcoin_price_api_url = "https://api.yadio.io" ```toml [rpc] enabled = false # Set to true to enable gRPC admin interface -listen_address = "127.0.0.1" +listen_address = "127.0.0.1" # IP literal, IPv6 bracketed; "localhost" is not resolved port = 50051 allow_remote = false # Required to bind a non-loopback address ``` diff --git a/docs/RPC.md b/docs/RPC.md index 12922807..0e278813 100644 --- a/docs/RPC.md +++ b/docs/RPC.md @@ -29,6 +29,12 @@ allow_remote = false # tls_key_path = "/etc/mostro/rpc-key.pem" ``` +`listen_address` must be an IP literal, with IPv6 bracketed: `127.0.0.1`, +`[::1]` or `0.0.0.0`. Hostnames are never resolved, so `localhost` and +unbracketed `::1` are refused at startup rather than accepted and then failed on +at bind time — `validate_rpc_settings` and `RpcServer::bind` parse the address +through the same function. + The bearer token is **not** configured here. It is read from the `MOSTRO_RPC_TOKEN` environment variable, which the daemon also picks up from `/.env`: diff --git a/docs/RPC_RATE_LIMITING.md b/docs/RPC_RATE_LIMITING.md index effac01c..224b94cd 100644 --- a/docs/RPC_RATE_LIMITING.md +++ b/docs/RPC_RATE_LIMITING.md @@ -69,7 +69,7 @@ How the original issue’s ideas map to the codebase today: | Exponential backoff / lockout | Implemented inside **`RateLimiter`**; **not** triggered by **`ValidateDbPassword`** (no **`record_failure`**). | | Audit logging | **tracing** in service + limiter. | | Localhost-only | Default RPC bind **`127.0.0.1`**, and a non-loopback bind now requires an explicit **`allow_remote = true`** (see `settings.toml` / `docs/RPC.md`). | -| Strong auth | Implemented by **`fn call`** for **`BearerAuth`** in **`src/rpc/auth.rs`**: a **`MOSTRO_RPC_TOKEN`** bearer token, compared in constant time (**`fn credentials_match`**, same file) on every method. The interceptor is deliberately **not** rate-limited — the token's entropy, not a counter, is what defeats guessing. | +| Strong auth | Implemented by **`fn call`** for **`BearerAuth`** in **`src/rpc/auth.rs`**: a **`MOSTRO_RPC_TOKEN`** bearer token, compared in constant time (**`fn credentials_match`**, same file) on every method. The interceptor is deliberately **not** rate-limited — the token's entropy, not a counter, is what defeats guessing. Because a rejected request never reaches the limiter, its *log line* is throttled instead (**`fn should_warn`**, same file): the first rejection from a peer within 60s is logged at `warn!` and the rest at `debug!`, so an unauthenticated flood cannot fill the disk. | ## Testing diff --git a/docs/STARTUP_AND_CONFIG.md b/docs/STARTUP_AND_CONFIG.md index 6bf7643a..29aa8a15 100644 --- a/docs/STARTUP_AND_CONFIG.md +++ b/docs/STARTUP_AND_CONFIG.md @@ -150,12 +150,12 @@ Configuration is loaded from `~/.mostro/settings.toml` (template: `settings.tpl. **RPC** (`RpcSettings` in `src/config/types.rs`): - `enabled` (bool): Enable RPC server (Rust Default: false) -- `listen_address` (String): Bind address (Rust Default: "127.0.0.1") +- `listen_address` (String): Bind address, as an IP literal with IPv6 bracketed - `127.0.0.1`, `[::1]`, `0.0.0.0`. Hostnames such as `localhost` are not resolved (Rust Default: "127.0.0.1") - `port` (u16): Listen port (Rust Default: 50051) - `allow_remote` (bool): Acknowledge a non-loopback bind (Rust Default: false) - `tls_cert_path` / `tls_key_path` (Option\): PEM material for TLS; required together (Rust Default: None) - Note: `enabled`, `listen_address` and `port` have a Rust Default implementation, but `settings.toml` must still include these keys. If a key is present but empty or omitted by tooling, the daemon falls back to the Rust Default value. The remaining fields are optional. -- The bearer token lives in the `MOSTRO_RPC_TOKEN` environment variable, never in `settings.toml`. `validate_rpc_settings` (`src/config/util.rs`) makes startup fatal when `enabled = true` and the token is missing or under 32 characters, when a non-loopback address is bound without `allow_remote = true`, or when only one half of the TLS pair is configured. See `docs/RPC.md`. +- The bearer token lives in the `MOSTRO_RPC_TOKEN` environment variable, never in `settings.toml`. `validate_rpc_settings` (`src/config/util.rs`) makes startup fatal when `enabled = true` and the token is missing or under 32 characters, when `listen_address` is not an address the server can bind, when a non-loopback address is bound without `allow_remote = true`, or when only one half of the TLS pair is configured. See `docs/RPC.md`. ## Global Variables diff --git a/settings.tpl.toml b/settings.tpl.toml index 638a9a3d..ce9fdbbf 100644 --- a/settings.tpl.toml +++ b/settings.tpl.toml @@ -134,7 +134,9 @@ dm_days = 30 # /.env, never here); the daemon refuses to start otherwise. # Generate one with: openssl rand -base64 32 enabled = false -# RPC server listen address +# RPC server listen address. Must be an IP literal, with IPv6 bracketed: +# "127.0.0.1", "[::1]" or "0.0.0.0". Hostnames such as "localhost" are not +# resolved and the daemon refuses to start on one. listen_address = "127.0.0.1" # RPC server port port = 50051 diff --git a/src/config/util.rs b/src/config/util.rs index 740d49d1..db9eed46 100644 --- a/src/config/util.rs +++ b/src/config/util.rs @@ -10,6 +10,7 @@ use crate::config::secret::{read_nsec_env_var, read_rpc_token_env_var}; use crate::config::types::RpcSettings; use crate::config::wizard; use crate::config::{init_mostro_settings, Settings}; +use crate::rpc::server::listen_socket_addr; use mostro_core::error::MostroError::{self, *}; use mostro_core::error::ServiceError; use secrecy::{ExposeSecret, SecretString}; @@ -85,17 +86,12 @@ fn validate_mostro_settings(settings: &Settings) -> Result<(), MostroError> { /// True when `addr` can only be reached from the host itself. /// -/// Accepts the `localhost` literal alongside IP literals because -/// `settings.toml` has always allowed it, and bracketed IPv6 (`[::1]`) because -/// that is how the address is written in a `host:port` pair. +/// Parses through `rpc::server::listen_socket_addr`, the same function +/// `RpcServer::bind` uses, so this can never call an address loopback that the +/// server would then refuse to bind. Callers must reject unparseable addresses +/// first — `false` here means "not loopback", not "not an address". fn is_loopback_address(addr: &str) -> bool { - if addr.eq_ignore_ascii_case("localhost") { - return true; - } - addr.trim_start_matches('[') - .trim_end_matches(']') - .parse::() - .is_ok_and(|ip| ip.is_loopback()) + listen_socket_addr(addr, 0).is_ok_and(|socket| socket.ip().is_loopback()) } /// Validate the `[rpc]` block (finding 1.5, issue #807). @@ -113,6 +109,9 @@ fn is_loopback_address(addr: &str) -> bool { /// - A non-loopback `listen_address` requires an explicit `allow_remote = true`. /// The defaults are safe, but nothing used to stop `0.0.0.0` from publishing /// the admin API to the LAN silently. +/// - `listen_address` must be an address `RpcServer::bind` can actually bind. +/// Validation and binding share `rpc::server::listen_socket_addr` so the two +/// cannot drift: a config accepted here is one the server will accept. /// /// A half-configured TLS pair is also fatal: it reads as "TLS is on" while /// serving plaintext. @@ -152,6 +151,14 @@ fn validate_rpc_settings( Some(_) => {} } + // Before the loopback check, or an unbindable address would be reported as + // a remote-exposure problem: `localhost` and bare `::1` read as loopback to + // an operator, so "set allow_remote = true" would be actively misleading + // advice for a daemon that is about to die on `Invalid address` instead. + listen_socket_addr(&rpc.listen_address, rpc.port).map_err(|e| { + MostroInternalErr(ServiceError::IOError(format!("[rpc].listen_address: {e}"))) + })?; + if !is_loopback_address(&rpc.listen_address) && !rpc.allow_remote { return Err(MostroInternalErr(ServiceError::IOError(format!( "[rpc].listen_address ({:?}) is not a loopback address: this publishes the admin API \ @@ -721,15 +728,8 @@ mod rpc_validation_tests { } #[test] - fn loopback_is_recognised_in_every_written_form() { - for address in [ - "127.0.0.1", - "127.0.0.53", - "localhost", - "LOCALHOST", - "::1", - "[::1]", - ] { + fn loopback_is_recognised_in_every_bindable_form() { + for address in ["127.0.0.1", "127.0.0.53", "[::1]"] { let rpc = RpcSettings { enabled: true, listen_address: address.to_string(), @@ -742,9 +742,37 @@ mod rpc_validation_tests { } } + /// The contract this pins: validation and `RpcServer::bind` share one + /// parser, so anything the server cannot bind is refused here with an + /// actionable message instead of at startup with `Invalid address`. + /// + /// `localhost` and bare `::1` are the cases that matter — they look like + /// valid loopback spellings, and reporting them through the `allow_remote` + /// branch would send the operator to fix the wrong setting. + #[test] + fn an_unbindable_listen_address_is_rejected() { + for address in ["localhost", "LOCALHOST", "::1", "::", "mostro.example.com"] { + let rpc = RpcSettings { + enabled: true, + listen_address: address.to_string(), + // Set so the failure cannot be attributed to the remote-bind + // guard: only the parse check can refuse these. + allow_remote: true, + ..Default::default() + }; + let err = validate_rpc_settings(&rpc, Some(&valid_token())) + .expect_err("an address the server cannot bind must not boot"); + let message = err.to_string(); + assert!( + message.contains("IP literal") && message.contains("[::1]"), + "{address} should name the accepted spellings, got: {message}" + ); + } + } + #[test] fn non_loopback_bind_without_allow_remote_is_rejected() { - for address in ["0.0.0.0", "192.168.1.10", "::", "mostro.example.com"] { + for address in ["0.0.0.0", "192.168.1.10", "[::]"] { let rpc = RpcSettings { enabled: true, listen_address: address.to_string(), diff --git a/src/main.rs b/src/main.rs index 2a2308a0..990466bc 100644 --- a/src/main.rs +++ b/src/main.rs @@ -279,8 +279,10 @@ async fn main() -> Result<()> { // and the TLS material here, before any of the startup below runs, so // the daemon never advances past this point believing an admin // interface is up that nothing is serving. - let rpc_serving = match rpc_server.bind(rpc_keys, rpc_pool, rpc_ln_client) { - Ok(serving) => serving, + // `bind` logs the address it actually acquired, so it is not repeated + // here. + let (_bound, rpc_serving) = match rpc_server.bind(rpc_keys, rpc_pool, rpc_ln_client) { + Ok(bound) => bound, Err(e) => { tracing::error!("RPC server failed to start: {}", e); exit(1); diff --git a/src/rpc/auth.rs b/src/rpc/auth.rs index c882b2cb..f7e748c1 100644 --- a/src/rpc/auth.rs +++ b/src/rpc/auth.rs @@ -7,36 +7,93 @@ //! message-level authorization left to fall back on: reaching the port is the //! authorization, so the transport has to be the gate. //! -//! Deliberately not rate-limited. `MIN_RPC_TOKEN_LEN` keeps the search space -//! far out of reach of online guessing, and tonic's [`Interceptor`] is -//! synchronous while [`crate::rpc::rate_limiter::RateLimiter`] is async, so -//! wiring one in would mean a second, parallel limiter for no security gain. +//! The authentication *decision* is deliberately not rate-limited. +//! `MIN_RPC_TOKEN_LEN` keeps the search space far out of reach of online +//! guessing, and tonic's [`Interceptor`] is synchronous while +//! [`crate::rpc::rate_limiter::RateLimiter`] is async, so wiring one in would +//! mean a second, parallel limiter for no security gain. +//! +//! The *logging* is throttled, which is a different problem. The rate limiter +//! runs inside the handlers (`check_rate_limit` in [`crate::rpc::service`]), so +//! a request rejected here never reaches it. One `warn!` per rejected request +//! is harmless on loopback, but under `[rpc].allow_remote = true` it hands any +//! host that can reach the port a free log line per request — unbounded disk +//! growth, and enough noise to bury the genuine audit lines. So the first +//! rejection from a peer within [`LOG_WINDOW`] is logged at `warn!` and the rest +//! at `debug!`. The peer table that makes this possible is itself capped, or it +//! would just move the unbounded growth from the log to memory. use secrecy::{ExposeSecret, SecretString}; -use std::sync::Arc; +use std::collections::HashMap; +use std::net::IpAddr; +use std::sync::{Arc, Mutex, PoisonError}; +use std::time::{Duration, Instant}; use subtle::ConstantTimeEq; use tonic::service::Interceptor; use tonic::{Request, Status}; -use tracing::warn; +use tracing::{debug, warn}; const AUTHORIZATION_HEADER: &str = "authorization"; /// RFC 7235 defines the auth-scheme as case-insensitive, and proxies do /// normalize it, so the scheme is matched without regard to case. The /// credential that follows is still compared byte for byte. const BEARER_SCHEME: &str = "Bearer"; +/// How long one rejected peer stays quiet after its `warn!` line. Short enough +/// that a real operator debugging a wrong token still sees a line per attempt +/// once they pause, long enough that a flood costs one line per minute. +const LOG_WINDOW: Duration = Duration::from_secs(60); +/// Ceiling on tracked peers. Without it a caller that spoofs a fresh source +/// address per request would grow this table forever, which is the same +/// resource exhaustion the throttle exists to prevent. +const MAX_TRACKED_PEERS: usize = 1024; /// Rejects any request that does not carry the configured bearer token. #[derive(Clone)] pub struct BearerAuth { token: Arc, + /// Last time each peer was logged at `warn!`. `None` keys the peers tonic + /// could not attribute to an address, so they are throttled as one bucket + /// rather than escaping the cap. + /// + /// A `std::sync::Mutex` because [`Interceptor::call`] is synchronous; the + /// critical section is a hash lookup and never awaits. `Arc` because tonic + /// clones the interceptor per connection and the table has to be shared. + warned: Arc, Instant>>>, } impl BearerAuth { pub fn new(token: SecretString) -> Self { Self { token: Arc::new(token), + warned: Arc::new(Mutex::new(HashMap::new())), } } + + /// True when this rejection deserves a `warn!` rather than a `debug!`. + /// + /// `now` is a parameter so the window can be tested without sleeping. + fn should_warn(&self, peer: Option, now: Instant) -> bool { + // A poisoned lock means another thread panicked mid-update. The table + // is a logging heuristic, so recovering the map beats propagating a + // panic out of an interceptor and killing the connection. + let mut warned = self.warned.lock().unwrap_or_else(PoisonError::into_inner); + + if let Some(last) = warned.get(&peer) { + if now.duration_since(*last) < LOG_WINDOW { + return false; + } + } else if warned.len() >= MAX_TRACKED_PEERS { + warned.retain(|_, last| now.duration_since(*last) < LOG_WINDOW); + // Still full: every slot belongs to a peer inside its window, so + // this one is part of a flood. Stay quiet rather than grow. + if warned.len() >= MAX_TRACKED_PEERS { + return false; + } + } + + warned.insert(peer, now); + true + } } impl Interceptor for BearerAuth { @@ -47,6 +104,10 @@ impl Interceptor for BearerAuth { .and_then(|value| value.to_str().ok()) .and_then(|value| { let (scheme, credential) = value.split_once(' ')?; + // RFC 7235 allows `1*SP` between the scheme and the credential, + // and token68 contains no spaces, so trimming the rest is + // lossless. + let credential = credential.trim_start_matches(' '); scheme .eq_ignore_ascii_case(BEARER_SCHEME) .then_some(credential) @@ -59,9 +120,22 @@ impl Interceptor for BearerAuth { // One message for every failure mode: a caller learns whether the // port is an admin RPC, never whether it guessed part of a token. _ => { - match request.remote_addr() { - Some(addr) => warn!("Rejected unauthenticated admin RPC from {}", addr.ip()), - None => warn!("Rejected unauthenticated admin RPC from an unknown peer"), + let peer = request.remote_addr().map(|addr| addr.ip()); + if self.should_warn(peer, Instant::now()) { + match peer { + Some(ip) => warn!("Rejected unauthenticated admin RPC from {}", ip), + None => warn!("Rejected unauthenticated admin RPC from an unknown peer"), + } + } else { + match peer { + Some(ip) => debug!( + "Rejected unauthenticated admin RPC from {} (further rejections from \ + this peer are logged at debug for up to {}s)", + ip, + LOG_WINDOW.as_secs() + ), + None => debug!("Rejected unauthenticated admin RPC from an unknown peer"), + } } Err(Status::unauthenticated("missing or invalid credentials")) } @@ -111,6 +185,14 @@ mod tests { assert!(result.is_ok()); } + #[test] + fn accepts_extra_spaces_after_the_scheme() { + // RFC 7235 auth-param grammar is `scheme 1*SP token68`, and proxies do + // rewrite the separator. + let result = interceptor().call(request_with_authorization(&format!("Bearer {TOKEN}"))); + assert!(result.is_ok()); + } + #[test] fn accepts_any_casing_of_the_bearer_scheme() { // RFC 7235: the auth-scheme is case-insensitive, and proxies rewrite it. @@ -179,4 +261,74 @@ mod tests { // is not. assert!(!credentials_match("ABC", "abc")); } + + fn peer(last_octet: u8) -> Option { + Some(IpAddr::from([192, 0, 2, last_octet])) + } + + #[test] + fn a_flooding_peer_is_warned_about_once_per_window() { + let auth = interceptor(); + let start = Instant::now(); + + assert!(auth.should_warn(peer(1), start)); + for attempt in 1..100 { + assert!( + !auth.should_warn(peer(1), start + Duration::from_millis(attempt)), + "attempt {attempt} must not produce a second warn line" + ); + } + // Once the window closes the peer is audible again, so a slow retry + // loop still leaves a trail. + assert!(auth.should_warn(peer(1), start + LOG_WINDOW)); + } + + #[test] + fn each_peer_gets_its_own_window() { + let auth = interceptor(); + let now = Instant::now(); + assert!(auth.should_warn(peer(1), now)); + assert!(auth.should_warn(peer(2), now)); + assert!(auth.should_warn(None, now)); + // ...and the second attempt from each is throttled independently. + assert!(!auth.should_warn(peer(1), now)); + assert!(!auth.should_warn(peer(2), now)); + assert!(!auth.should_warn(None, now)); + } + + #[test] + fn the_peer_table_cannot_grow_without_bound() { + // A caller spoofing a fresh source address per request must not be able + // to turn the log throttle into a memory leak. + let auth = interceptor(); + let now = Instant::now(); + for octet in 0..=u8::MAX { + for third in 0..=u8::MAX { + auth.should_warn(Some(IpAddr::from([192, 0, third, octet])), now); + } + } + let tracked = auth + .warned + .lock() + .unwrap_or_else(PoisonError::into_inner) + .len(); + assert!( + tracked <= MAX_TRACKED_PEERS, + "{tracked} peers tracked, cap is {MAX_TRACKED_PEERS}" + ); + } + + #[test] + fn expired_entries_are_reclaimed_when_the_table_fills() { + let auth = interceptor(); + let start = Instant::now(); + for third in 0..=u8::MAX { + for fourth in 0..=u8::MAX { + auth.should_warn(Some(IpAddr::from([198, 51, third, fourth])), start); + } + } + // Every tracked peer is now outside its window, so a new peer is both + // admitted and audible rather than silently dropped. + assert!(auth.should_warn(peer(7), start + LOG_WINDOW)); + } } diff --git a/src/rpc/server.rs b/src/rpc/server.rs index 022f5598..95339842 100644 --- a/src/rpc/server.rs +++ b/src/rpc/server.rs @@ -15,6 +15,32 @@ use tracing::info; use super::admin::admin_service_server::AdminServiceServer; +/// Resolve `[rpc].listen_address` and `[rpc].port` into the address the server +/// binds. +/// +/// `SocketAddr` only parses IP literals, with IPv6 bracketed. Hostnames such as +/// `localhost` and bare `::1` are therefore not bindable addresses, however +/// natural they look in a config file. +/// +/// `config::util::validate_rpc_settings` calls this too, so a `listen_address` +/// that passes validation is guaranteed to be one `bind` can use: the two must +/// never disagree, or the daemon accepts a config at startup and then dies on +/// it. +pub(crate) fn listen_socket_addr( + listen_address: &str, + port: u16, +) -> Result { + format!("{listen_address}:{port}") + .parse::() + .map_err(|e| { + format!( + "Invalid address {listen_address:?}: {e}. Expected an IP literal, with IPv6 \ + bracketed — for example 127.0.0.1, [::1] or 0.0.0.0. Hostnames such as \ + \"localhost\" are not resolved." + ) + }) +} + /// RPC server for admin operations pub struct RpcServer { listen_address: String, @@ -37,7 +63,8 @@ impl RpcServer { } } - /// Acquire the listener and return the future that serves it. + /// Acquire the listener and return the bound address with the future that + /// serves it. /// /// Everything that can fail on the way up happens here, before the caller /// gets anything to detach: a missing bearer token, unusable TLS material, @@ -46,6 +73,10 @@ impl RpcServer { /// and gated before it lets the rest of the daemon proceed — `[rpc].enabled /// = true` becomes an invariant rather than a hope. /// + /// The address comes back from the listener rather than from the config, so + /// it is the port actually in use: `port = 0` reports the ephemeral port the + /// kernel picked instead of a literal `:0`. + /// /// Refusing to serve without a token is deliberately redundant with /// `config::util::validate_rpc_settings`: a code path that reached here /// without one would expose an ungated admin API, and no RPC at all is the @@ -56,12 +87,13 @@ impl RpcServer { pool: Arc>, ln_client: Arc>, ) -> Result< - impl std::future::Future> + Send + 'static, + ( + std::net::SocketAddr, + impl std::future::Future> + Send + 'static, + ), Box, > { - let addr: std::net::SocketAddr = format!("{}:{}", self.listen_address, self.port) - .parse() - .map_err(|e| format!("Invalid address: {}", e))?; + let addr = listen_socket_addr(&self.listen_address, self.port)?; let token = read_rpc_token_env_var().ok_or_else(|| { format!("Refusing to start the admin RPC server: {RPC_TOKEN_ENV_VAR} is not set") @@ -89,14 +121,20 @@ impl RpcServer { // discovered later by whoever happens to read the logs. let incoming = TcpIncoming::bind(addr).map_err(|e| format!("Failed to bind {addr}: {e}"))?; - info!("RPC server listening on {} ({})", addr, transport); - - Ok(builder - .add_service(AdminServiceServer::with_interceptor( - admin_service, - BearerAuth::new(token), - )) - .serve_with_incoming(incoming)) + let bound = incoming + .local_addr() + .map_err(|e| format!("Failed to read the address bound to {addr}: {e}"))?; + info!("RPC server listening on {} ({})", bound, transport); + + Ok(( + bound, + builder + .add_service(AdminServiceServer::with_interceptor( + admin_service, + BearerAuth::new(token), + )) + .serve_with_incoming(incoming), + )) } /// Check if RPC server is enabled @@ -232,10 +270,37 @@ mod tests { init_test_settings(); let _lock = RPC_TOKEN_LOCK.lock().await; let _token = RpcTokenGuard::set(&"t".repeat(32)); - let server = server_at("not an address", 50051); - let pool = sqlx::SqlitePool::connect("sqlite::memory:").await.unwrap(); - let result = server.bind(Keys::generate(), Arc::new(pool), offline_ln_client().await); - assert!(result.is_err()); + // `localhost` and bare `::1` are in here on purpose: they read like + // valid loopback spellings, and `config::util` rejects them for exactly + // this reason — `SocketAddr` cannot parse either. + for address in ["not an address", "localhost", "::1"] { + let server = server_at(address, 50051); + let pool = sqlx::SqlitePool::connect("sqlite::memory:").await.unwrap(); + let error = server + .bind(Keys::generate(), Arc::new(pool), offline_ln_client().await) + .err() + .expect("an address that cannot be parsed must not serve"); + assert!( + error.to_string().contains("Invalid address"), + "{address} should have been refused, got: {error}" + ); + } + } + + #[test] + fn listen_socket_addr_accepts_only_bindable_literals() { + for address in ["127.0.0.1", "[::1]", "0.0.0.0", "[::]"] { + assert!( + listen_socket_addr(address, 50051).is_ok(), + "{address} is a bindable literal" + ); + } + for address in ["localhost", "::1", "::", "mostro.example.com", ""] { + assert!( + listen_socket_addr(address, 50051).is_err(), + "{address} is not a bindable literal" + ); + } } /// The startup invariant the daemon depends on: a listener that cannot be @@ -322,18 +387,14 @@ mod tests { let token = "t".repeat(32); let _guard = RpcTokenGuard::set(&token); - // Reserve an ephemeral port and release it, so parallel runs of the - // suite cannot collide on a fixed one. - let port = std::net::TcpListener::bind("127.0.0.1:0") - .expect("reserve an ephemeral port") - .local_addr() - .expect("reserved address") - .port(); - let pool = sqlx::SqlitePool::connect("sqlite::memory:").await.unwrap(); let ln_client = offline_ln_client().await; - let server = server_at("127.0.0.1", port); - let serving = server + // Port 0: the listener `bind` returns owns the ephemeral port for as + // long as the test needs it. Reserving a port and releasing it first + // would leave a window in which the kernel can hand it to another + // process — a flake, not a failure of what is under test. + let server = server_at("127.0.0.1", 0); + let (bound, serving) = server .bind(Keys::generate(), Arc::new(pool), ln_client) .expect("bind must succeed on a free loopback port"); let serving = tokio::spawn(serving); @@ -341,7 +402,7 @@ mod tests { // No retry loop: `bind` returned, so the listener already exists and // the connection below must succeed on the first attempt. A retry here // would hide exactly the regression this asserts against. - let channel = Channel::from_shared(format!("http://127.0.0.1:{port}")) + let channel = Channel::from_shared(format!("http://{bound}")) .expect("valid endpoint") .connect() .await From eb3849b59df69342ac98c90db7bdb58349b5a0c7 Mon Sep 17 00:00:00 2001 From: Andrea Diaz Correia Date: Sat, 22 Aug 2026 18:31:21 -0300 Subject: [PATCH 10/11] docs: clarify RPC config validation and deserialization behavior --- docs/RPC.md | 5 +++-- docs/STARTUP_AND_CONFIG.md | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/docs/RPC.md b/docs/RPC.md index 0e278813..749240f6 100644 --- a/docs/RPC.md +++ b/docs/RPC.md @@ -32,8 +32,9 @@ allow_remote = false `listen_address` must be an IP literal, with IPv6 bracketed: `127.0.0.1`, `[::1]` or `0.0.0.0`. Hostnames are never resolved, so `localhost` and unbracketed `::1` are refused at startup rather than accepted and then failed on -at bind time — `validate_rpc_settings` and `RpcServer::bind` parse the address -through the same function. +at bind time: `fn validate_rpc_settings` in `src/config/util.rs` and `fn bind` +for `RpcServer` in `src/rpc/server.rs` both resolve the address through +`fn listen_socket_addr` in `src/rpc/server.rs`. The bearer token is **not** configured here. It is read from the `MOSTRO_RPC_TOKEN` environment variable, which the daemon also picks up from `/.env`: diff --git a/docs/STARTUP_AND_CONFIG.md b/docs/STARTUP_AND_CONFIG.md index 29aa8a15..7c25fde1 100644 --- a/docs/STARTUP_AND_CONFIG.md +++ b/docs/STARTUP_AND_CONFIG.md @@ -154,7 +154,7 @@ Configuration is loaded from `~/.mostro/settings.toml` (template: `settings.tpl. - `port` (u16): Listen port (Rust Default: 50051) - `allow_remote` (bool): Acknowledge a non-loopback bind (Rust Default: false) - `tls_cert_path` / `tls_key_path` (Option\): PEM material for TLS; required together (Rust Default: None) -- Note: `enabled`, `listen_address` and `port` have a Rust Default implementation, but `settings.toml` must still include these keys. If a key is present but empty or omitted by tooling, the daemon falls back to the Rust Default value. The remaining fields are optional. +- Note: `enabled`, `listen_address` and `port` are required in `settings.toml`. `RpcSettings` has a Rust `Default` implementation, but neither those fields nor `rpc` on `Settings` carry `#[serde(default)]`, so an omitted key makes `toml::from_str` (`fn init_configuration_file` in `src/config/util.rs`) fail with a missing-field error rather than fall back, and an empty value deserializes as that empty value. The remaining fields are optional. - The bearer token lives in the `MOSTRO_RPC_TOKEN` environment variable, never in `settings.toml`. `validate_rpc_settings` (`src/config/util.rs`) makes startup fatal when `enabled = true` and the token is missing or under 32 characters, when `listen_address` is not an address the server can bind, when a non-loopback address is bound without `allow_remote = true`, or when only one half of the TLS pair is configured. See `docs/RPC.md`. ## Global Variables From 2ff1bbe33fe5544f707b8bfd7ed3c255e5ac64f2 Mon Sep 17 00:00:00 2001 From: Andrea Diaz Correia Date: Sat, 22 Aug 2026 18:51:17 -0300 Subject: [PATCH 11/11] docs: clarify deserialization behavior for bool and u16 RPC fields --- docs/STARTUP_AND_CONFIG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/STARTUP_AND_CONFIG.md b/docs/STARTUP_AND_CONFIG.md index 7c25fde1..af41c3a5 100644 --- a/docs/STARTUP_AND_CONFIG.md +++ b/docs/STARTUP_AND_CONFIG.md @@ -154,7 +154,7 @@ Configuration is loaded from `~/.mostro/settings.toml` (template: `settings.tpl. - `port` (u16): Listen port (Rust Default: 50051) - `allow_remote` (bool): Acknowledge a non-loopback bind (Rust Default: false) - `tls_cert_path` / `tls_key_path` (Option\): PEM material for TLS; required together (Rust Default: None) -- Note: `enabled`, `listen_address` and `port` are required in `settings.toml`. `RpcSettings` has a Rust `Default` implementation, but neither those fields nor `rpc` on `Settings` carry `#[serde(default)]`, so an omitted key makes `toml::from_str` (`fn init_configuration_file` in `src/config/util.rs`) fail with a missing-field error rather than fall back, and an empty value deserializes as that empty value. The remaining fields are optional. +- Note: `enabled`, `listen_address` and `port` are required in `settings.toml`. `RpcSettings` has a Rust `Default` implementation, but neither those fields nor `rpc` on `Settings` carry `#[serde(default)]`, so an omitted key makes `toml::from_str` (`fn init_configuration_file` in `src/config/util.rs`) fail with a missing-field error rather than fall back, and an empty value is preserved as-is only for the string-typed `listen_address`; an empty value for `enabled` or `port` fails deserialization against their `bool` and `u16` types. The remaining fields are optional. - The bearer token lives in the `MOSTRO_RPC_TOKEN` environment variable, never in `settings.toml`. `validate_rpc_settings` (`src/config/util.rs`) makes startup fatal when `enabled = true` and the token is missing or under 32 characters, when `listen_address` is not an address the server can bind, when a non-loopback address is bound without `allow_remote = true`, or when only one half of the TLS pair is configured. See `docs/RPC.md`. ## Global Variables