diff --git a/docs/manual/src/config-reference.md b/docs/manual/src/config-reference.md index a01521beb..863266362 100644 --- a/docs/manual/src/config-reference.md +++ b/docs/manual/src/config-reference.md @@ -23,6 +23,11 @@ track Bitcoin Core v30. - **Resolution order.** `-conf=` if given, else `/bitcoin.conf`, else `/satd.conf`. Flags override file values. +- **Comments.** As in Core, `#` starts a comment anywhere on a line: + `txindex=1 # keep it` sets `txindex` to `1`. What is left of the line is + trimmed of spaces, tabs and line endings. An `rpcpassword` line that + contains a `#` is refused at load (`using # in rpcpassword can be ambiguous + and should be avoided`), as Core refuses it. - **Key disposition.** Each config-file key gets one of four treatments: 1. **Honored.** satd implements it. This is the common case. 2. **Skipped with a warning.** A recognized Core v30 option satd does not @@ -120,7 +125,7 @@ startup error. | `logsourcelocations` | off | restart | core | Prepend source `file:line` to each log line. | | `debug` | none | hot | core | Enable debug logging for a category (repeatable; bare/`all`/`1` = everything). satd adds `stratum`, the Stratum server's per-miner lines (see [Verifying a miner](stratum.md#verifying-a-miner)). | | `debugexclude` | none | hot | core | Disable debug logging for a category `debug` would otherwise enable. | -| `loglevel` | info | hot | core | Global verbosity (`trace`/`debug`/`info`/`warn`/`error`) or a per-category override (`net:debug`). Maps onto satd's `tracing` filter: a bare level sets the default for targets without an override, and does not lower a more specific `-debug`/`RUST_LOG` directive (`-debug=net -loglevel=error` still logs `net` at debug). A `category:level` pair overrides that subsystem. | +| `loglevel` | info | hot | core | Global verbosity (`trace`/`debug`/`info`/`warn`/`error`) or a per-category override (`net:debug`). Maps onto satd's `tracing` filter: a bare level sets the default for targets without an override, and does not lower a more specific `-debug`/`RUST_LOG` directive (`-debug=net -loglevel=error` still logs `net` at debug). A `category:level` pair overrides that subsystem. The JSON-RPC and WebSocket libraries' TRACE output (`jsonrpsee*`, `tungstenite*` targets), which prints request headers and call parameters, is never logged, whatever the level; their DEBUG output is. | | `allowignoredconf` | off | restart | core | Suppress startup warnings about `includeconf` files satd had to ignore. | | `maxshutdownsecs` | 30 | hot | satd | Graceful-shutdown budget (seconds), shared by the `shutdownnotify` hook (at most half of it), the wait for the block connector to stop (at most half of what is left) and the final flush. A connector still in a block at the end of its share costs the clean-shutdown marker but not the flush; a flush that overruns the budget force-exits without the marker. | diff --git a/docs/manual/src/configuration.md b/docs/manual/src/configuration.md index 4739a0cc7..7bd0311be 100644 --- a/docs/manual/src/configuration.md +++ b/docs/manual/src/configuration.md @@ -94,7 +94,9 @@ sat-cli -datadir=/path/to/datadir getblockcount `rpccookiefile` come from the command line first, then the active chain's section (`[main]`, `[test]`, `[testnet4]`, `[signet]`, `[regtest]`), then the top level of the file; the first value in a scope wins. A top-level - `rpcport` applies on every chain, as it does for satd itself. + `rpcport` applies on every chain, as it does for satd itself. The file is + read with satd's rules: `#` starts a comment anywhere on a line, and an + `rpcpassword` line holding a `#` is an error. - **Which credentials.** A non-empty `rpcpassword` from either source means user/password authentication, with `rpcuser` looked up the same way; otherwise the cookie file, whose relative path is under the chain's data diff --git a/node/src/net/tor.rs b/node/src/net/tor.rs index 27e0b2f15..28f57ed67 100644 --- a/node/src/net/tor.rs +++ b/node/src/net/tor.rs @@ -148,20 +148,21 @@ impl TorController { // control port is trusted" model (same as Bitcoin Core): the bytes are // only fed into the HMAC, never sent or logged, and the SERVERHASH check // below fails unless the port already knows the file — i.e. it is the - // real Tor. No confidentiality leak results. - let cookie = std::fs::read(cookie_path).map_err(|e| { - format!( - "Tor SAFECOOKIE: cannot read cookie file {}: {} (run satd as a user that can read \ - it, or set CookieAuthFileGroupReadable 1 in torrc)", - cookie_path, e - ) - })?; + // real Tor. No confidentiality leak results. The read itself is + // bounded (see `read_cookie_file`), so a path such as /dev/zero or a + // FIFO cannot exhaust memory or stall startup. + let cookie = read_cookie_file(cookie_path)?; if cookie.len() != COOKIE_LEN { + // At most COOKIE_LEN + 1 bytes were read, so a longer read means + // "longer than the cookie", not its exact size. + let size = if cookie.len() > COOKIE_LEN { + format!("more than {COOKIE_LEN}") + } else { + cookie.len().to_string() + }; return Err(format!( "Tor SAFECOOKIE: cookie file {} is {} bytes, expected {}", - cookie_path, - cookie.len(), - COOKIE_LEN + cookie_path, size, COOKIE_LEN )); } @@ -357,6 +358,51 @@ impl TorController { } } +/// Open the SAFECOOKIE cookie file the control port named and read at most +/// `COOKIE_LEN + 1` bytes of it. +/// +/// Bitcoin Core reads the cookie with `ReadBinaryFile(cookiefile, +/// TOR_COOKIE_SIZE)` (`torcontrol.cpp`), which never reads more than 32 bytes. +/// The path comes from the control port, so nothing guarantees it names a +/// small regular file: a read of `/dev/zero` to end of file never ends, and +/// opening a FIFO for reading waits for a writer. The file is opened +/// non-blocking and must be a regular file. One byte past the cookie length is +/// read so that a longer file is refused by the caller's length check instead +/// of being truncated to a cookie. +fn read_cookie_file(cookie_path: &str) -> Result, String> { + let unreadable = |e: std::io::Error| { + format!( + "Tor SAFECOOKIE: cannot read cookie file {}: {} (run satd as a user that can read \ + it, or set CookieAuthFileGroupReadable 1 in torrc)", + cookie_path, e + ) + }; + let mut options = std::fs::OpenOptions::new(); + options.read(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + options.custom_flags(libc::O_NONBLOCK); + } + let file = options.open(cookie_path).map_err(unreadable)?; + let metadata = file.metadata().map_err(unreadable)?; + if !metadata.is_file() { + return Err(format!( + "Tor SAFECOOKIE: cookie file {} is not a regular file", + cookie_path + )); + } + read_cookie_bytes(file).map_err(unreadable) +} + +/// Read at most `COOKIE_LEN + 1` bytes from `reader`. +fn read_cookie_bytes(reader: impl std::io::Read) -> std::io::Result> { + use std::io::Read as _; + let mut cookie = Vec::with_capacity(COOKIE_LEN + 1); + reader.take(COOKIE_LEN as u64 + 1).read_to_end(&mut cookie)?; + Ok(cookie) +} + /// HMAC-SHA256(key, msg). fn hmac(key: &[u8], msg: &[u8]) -> Vec { let mut mac = HmacSha256::new_from_slice(key).expect("HMAC accepts any key length"); @@ -584,4 +630,180 @@ mod tests { "client leaked a proof to an unverified server" ); } + + /// A control port that offers SAFECOOKIE and names `cookie_path` as the + /// cookie file, then waits for the client's next line. Returns that line + /// (empty if the client hung up without sending one). + async fn fake_tor_naming_cookie(listener: TcpListener, cookie_path: String) -> String { + let (stream, _) = listener.accept().await.unwrap(); + let (rh, mut wh) = tokio::io::split(stream); + let mut reader = BufReader::new(rh); + let mut l = String::new(); + reader.read_line(&mut l).await.unwrap(); + assert!(l.starts_with("PROTOCOLINFO"), "got: {l}"); + wh.write_all( + format!( + "250-PROTOCOLINFO 1\r\n250-AUTH METHODS=SAFECOOKIE COOKIEFILE=\"{cookie_path}\"\r\n250 OK\r\n" + ) + .as_bytes(), + ) + .await + .unwrap(); + let mut next = String::new(); + let _ = reader.read_line(&mut next).await; + next + } + + /// Connect to a fake control port naming `cookie_path` and authenticate, + /// in a task of its own so the caller can time it out. + fn spawn_authenticate(addr: String) -> tokio::task::JoinHandle> { + tokio::spawn(async move { + let mut ctrl = TorController::connect(&addr).await?; + ctrl.authenticate(None).await + }) + } + + #[cfg(unix)] + #[test] + fn safecookie_refuses_a_fifo_cookie_path() { + // Opening a FIFO for reading waits for a writer. The cookie path comes + // from the control port, so naming a FIFO must not stall startup: the + // file has to be refused as not a regular file, promptly. + let dir = tempfile::tempdir().unwrap(); + let fifo = dir.path().join("cookie.fifo"); + let c_path = std::ffi::CString::new(fifo.to_str().unwrap()).unwrap(); + // SAFETY: `c_path` is a valid NUL-terminated path for the call. + assert_eq!(unsafe { libc::mkfifo(c_path.as_ptr(), 0o600) }, 0, "mkfifo"); + + // The client runs on a runtime of its own in its own thread: if it + // blocks in open(2), this thread's timeout still fires. + let cookie_path = fifo.to_str().unwrap().to_string(); + let (tx, rx) = std::sync::mpsc::channel(); + let client = std::thread::spawn(move || { + let rt = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .unwrap(); + let outcome = rt.block_on(async move { + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap().to_string(); + let server = tokio::spawn(fake_tor_naming_cookie(listener, cookie_path)); + let auth = async { + let mut ctrl = TorController::connect(&addr).await?; + ctrl.authenticate(None).await + } + .await; + (auth, server.await.unwrap()) + }); + let _ = tx.send(outcome); + }); + + match rx.recv_timeout(std::time::Duration::from_secs(10)) { + Ok((auth, next)) => { + client.join().unwrap(); + let err = auth.unwrap_err(); + assert!(err.contains("not a regular file"), "got: {err}"); + assert!( + !next.starts_with("AUTHCHALLENGE"), + "client went on to AUTHCHALLENGE with a refused cookie: {next}" + ); + } + Err(_) => { + // Release the reader blocked in open(2) so its thread ends, + // then fail. O_NONBLOCK: never wait here for a reader. + use std::os::unix::fs::OpenOptionsExt; + drop( + std::fs::OpenOptions::new() + .write(true) + .custom_flags(libc::O_NONBLOCK) + .open(&fifo), + ); + let _ = client.join(); + panic!("authenticate blocked on a FIFO named as the Tor cookie file"); + } + } + } + + #[cfg(unix)] + #[tokio::test] + async fn safecookie_refuses_a_device_cookie_path() { + // A character device such as /dev/zero never reaches end of file, so a + // read to EOF would not end. It is not a regular file, so it is refused + // before any read. + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap().to_string(); + let server = tokio::spawn(fake_tor_naming_cookie(listener, "/dev/zero".to_string())); + let auth = spawn_authenticate(addr); + let err = tokio::time::timeout(std::time::Duration::from_secs(10), auth) + .await + .expect("authenticate did not return for a /dev/zero cookie path") + .unwrap() + .unwrap_err(); + assert!(err.contains("not a regular file"), "got: {err}"); + let _ = tokio::time::timeout(std::time::Duration::from_secs(10), server).await; + } + + #[tokio::test] + async fn safecookie_refuses_a_cookie_file_longer_than_32_bytes() { + // Tor writes exactly 32 bytes. A longer file is refused rather than + // truncated to its first 32 bytes. + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("control.authcookie"); + std::fs::write(&path, [7u8; 64]).unwrap(); + + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap().to_string(); + let server = tokio::spawn(fake_tor_naming_cookie( + listener, + path.to_str().unwrap().to_string(), + )); + let err = tokio::time::timeout(std::time::Duration::from_secs(10), spawn_authenticate(addr)) + .await + .expect("authenticate timed out") + .unwrap() + .unwrap_err(); + assert!(err.contains("is more than 32 bytes"), "got: {err}"); + let _ = tokio::time::timeout(std::time::Duration::from_secs(10), server).await; + } + + /// A reader with `len` bytes available that counts how many it served. + struct CountingReader { + remaining: usize, + served: std::rc::Rc>, + } + + impl std::io::Read for CountingReader { + fn read(&mut self, buf: &mut [u8]) -> std::io::Result { + let n = buf.len().min(self.remaining); + buf[..n].fill(0xab); + self.remaining -= n; + self.served.set(self.served.get() + n); + Ok(n) + } + } + + #[test] + fn cookie_read_stops_one_byte_past_the_cookie_length() { + let served = std::rc::Rc::new(std::cell::Cell::new(0)); + let reader = CountingReader { + remaining: 1 << 20, + served: served.clone(), + }; + let cookie = read_cookie_bytes(reader).unwrap(); + assert_eq!(cookie.len(), COOKIE_LEN + 1); + assert!( + served.get() <= COOKIE_LEN + 1, + "read {} bytes from a 1 MiB source; the cookie read must stop at {}", + served.get(), + COOKIE_LEN + 1 + ); + + // An exact 32-byte source reads as the cookie. + let served = std::rc::Rc::new(std::cell::Cell::new(0)); + let reader = CountingReader { + remaining: COOKIE_LEN, + served, + }; + assert_eq!(read_cookie_bytes(reader).unwrap().len(), COOKIE_LEN); + } } diff --git a/sat-cli/src/conf.rs b/sat-cli/src/conf.rs index e5d5111d8..f5b6fba69 100644 --- a/sat-cli/src/conf.rs +++ b/sat-cli/src/conf.rs @@ -103,14 +103,21 @@ pub struct ConfFile { } impl ConfFile { - pub fn parse(content: &str) -> Self { + /// Parse the file as bitcoin-cli does, with Bitcoin Core's + /// GetConfigOptions (src/common/config.cpp): a `#` anywhere on a line + /// starts a comment, what is left is trimmed of spaces, tabs, CRs and + /// LFs, and an `rpcpassword` line holding a `#` is an error. + pub fn parse(content: &str) -> Result { + let trim = |s: &str| s.trim_matches([' ', '\t', '\r', '\n']).to_string(); let mut file = ConfFile::default(); let mut section: Option = None; - for line in content.lines() { - // Whole-line comments only, as the daemon parses them: a `#` - // later on a line is part of the value (a password may hold one). - let line = line.trim(); - if line.is_empty() || line.starts_with('#') { + for (idx, line) in content.lines().enumerate() { + let (line, used_hash) = match line.find('#') { + Some(pos) => (&line[..pos], true), + None => (line, false), + }; + let line = trim(line); + if line.is_empty() { continue; } if line.starts_with('[') && line.ends_with(']') { @@ -118,25 +125,34 @@ impl ConfFile { continue; } let (key, value) = match line.split_once('=') { - Some((k, v)) => (k.trim(), v.trim()), - None => (line, "1"), + Some((k, v)) => (trim(k), trim(v)), + None => (line.clone(), "1".to_string()), }; + if used_hash && key.contains("rpcpassword") { + return Err(format!( + "parse error on line {}, using # in rpcpassword can be ambiguous and \ + should be avoided", + idx + 1 + )); + } let map = match §ion { Some(s) => file.sections.entry(s.clone()).or_default(), None => &mut file.global, }; - map.entry(key.to_string()).or_default().push(value.to_string()); + map.entry(key).or_default().push(value); } - file + Ok(file) } /// Read `path`. A missing file is not an error (`Ok(None)`); an - /// unreadable one is, when the caller named it with `-conf`. - pub fn read(path: &Path) -> std::io::Result> { + /// unreadable or unparseable one is. + pub fn read(path: &Path) -> Result, String> { match std::fs::read_to_string(path) { - Ok(s) => Ok(Some(Self::parse(&s))), + Ok(s) => Self::parse(&s).map(Some).map_err(|e| { + format!("Error reading configuration file {}: {e}", path.display()) + }), Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(None), - Err(e) => Err(e), + Err(e) => Err(format!("cannot read {}: {e}", path.display())), } } @@ -344,7 +360,7 @@ mod tests { use super::*; fn resolve_str(cli: &CliConn, body: &str) -> Result { - let conf = ConfFile::parse(body); + let conf = ConfFile::parse(body)?; resolve(cli, Some(&conf), Path::new("/d"), PathBuf::from("/d/bitcoin.conf")) } @@ -489,10 +505,34 @@ mod tests { #[test] fn comments_and_bare_keys() { - let body = "# a comment\nregtest\nrpcport=9\nrpcpassword=a#b\n"; + let body = "# a comment\nregtest\nrpcport=9\nrpcpassword=ab\n"; let c = resolve_str(&CliConn::default(), body).unwrap(); assert_eq!((c.chain, c.port), (Chain::Regtest, 9)); - assert_eq!(c.creds, Creds::UserPass(String::new(), "a#b".into())); + assert_eq!(c.creds, Creds::UserPass(String::new(), "ab".into())); + } + + /// bitcoin-cli reads the file with Bitcoin Core's GetConfigOptions + /// (src/common/config.cpp): a `#` anywhere starts a comment, and the rest + /// is trimmed of spaces, tabs, CRs and LFs. + #[test] + fn an_inline_comment_is_not_part_of_the_value() { + let body = "regtest=1 # lab node\n[regtest] # this chain\nrpcport=19000\t# port\n\ + rpcuser=u #user\nrpcpassword=p\n"; + let c = resolve_str(&CliConn::default(), body).unwrap(); + assert_eq!((c.chain, c.port), (Chain::Regtest, 19000)); + assert_eq!(c.creds, Creds::UserPass("u".into(), "p".into())); + } + + /// Core refuses an `rpcpassword` line holding a `#`, as the daemon does. + #[test] + fn a_hash_on_an_rpcpassword_line_is_refused() { + for body in ["rpcpassword=a#b\n", "rpcuser=u\n[main]\nrpcpassword=p # old\n"] { + let err = resolve_str(&CliConn::default(), body).unwrap_err(); + assert!( + err.contains("using # in rpcpassword can be ambiguous and should be avoided"), + "{body:?}: {err}" + ); + } } #[test] diff --git a/sat-cli/src/main.rs b/sat-cli/src/main.rs index 03c38844c..480f45e7a 100644 --- a/sat-cli/src/main.rs +++ b/sat-cli/src/main.rs @@ -818,7 +818,7 @@ async fn main() { } Ok(None) => None, Err(e) => { - eprintln!("error: cannot read {}: {e}", conf_path.display()); + eprintln!("error: {e}"); std::process::exit(1); } }; diff --git a/satd-auth/src/operator.rs b/satd-auth/src/operator.rs index 36164a326..584b3e3e3 100644 --- a/satd-auth/src/operator.rs +++ b/satd-auth/src/operator.rs @@ -24,7 +24,7 @@ fn ct_eq(a: &[u8], b: &[u8]) -> bool { } /// The auto-generated cookie credential (`__cookie__:`). -#[derive(Debug, Clone)] +#[derive(Clone)] pub struct CookieCredential { /// Where the cookie file lives (for cleanup on shutdown). pub path: PathBuf, @@ -34,7 +34,7 @@ pub struct CookieCredential { } /// A `-rpcuser`/`-rpcpassword` pair. -#[derive(Debug, Clone)] +#[derive(Clone)] pub struct UserPassCredential { /// Username. pub username: String, @@ -45,7 +45,7 @@ pub struct UserPassCredential { /// One parsed Bitcoin-Core `-rpcauth=user:salt$hash` entry. `salt`'s ASCII bytes /// are the HMAC key (Core's `rpcauth.py` does `salt.encode('utf-8')`); `hash` is /// the expected 32-byte tag. -#[derive(Debug, Clone)] +#[derive(Clone)] pub struct RpcAuthCredential { /// Username. pub username: String, @@ -55,6 +55,34 @@ pub struct RpcAuthCredential { pub hash: Vec, } +// The credentials' `Debug` output names each credential but never shows its +// secret: the cookie token, the password, or the rpcauth salt and tag (which +// together allow offline password guessing). `OperatorCreds` and the +// `RpcAuth` handle that holds it derive `Debug` over these. +impl std::fmt::Debug for CookieCredential { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("CookieCredential") + .field("path", &self.path) + .finish_non_exhaustive() + } +} + +impl std::fmt::Debug for UserPassCredential { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("UserPassCredential") + .field("username", &self.username) + .finish_non_exhaustive() + } +} + +impl std::fmt::Debug for RpcAuthCredential { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("RpcAuthCredential") + .field("username", &self.username) + .finish_non_exhaustive() + } +} + /// Zero or more operator credentials of the three Core-compatible kinds. A /// request authenticates the operator if **any** credential matches the /// presented `user:pass` (Core's behaviour: cookie, `-rpcuser`/`-rpcpassword`, @@ -225,4 +253,33 @@ mod tests { assert_eq!(c.matching_user("alice", "tok"), None); assert_eq!(c.matching_user("bob", "secret"), None); } + + #[test] + fn debug_output_names_credentials_without_their_secrets() { + let hash = vec![0xa5u8; 32]; + let c = OperatorCreds { + cookie: Some(CookieCredential { + path: PathBuf::from("/tmp/x.cookie"), + token: "CookieTokenMarker".into(), + }), + userpass: vec![UserPassCredential { + username: "alice".into(), + password: "PasswordMarker".into(), + }], + rpcauth: vec![RpcAuthCredential { + username: "bob".into(), + salt: "SaltMarker".into(), + hash: hash.clone(), + }], + }; + for shown in [format!("{c:?}"), format!("{c:#?}")] { + for secret in ["CookieTokenMarker", "PasswordMarker", &format!("{hash:?}")] { + assert!(!shown.contains(secret), "Debug shows {secret}: {shown}"); + } + // What identifies a credential is still there. + for name in ["x.cookie", "alice", "bob"] { + assert!(shown.contains(name), "Debug lost {name}: {shown}"); + } + } + } } diff --git a/satd/src/config.rs b/satd/src/config.rs index ff36d9e19..c093fb278 100644 --- a/satd/src/config.rs +++ b/satd/src/config.rs @@ -292,7 +292,7 @@ impl std::str::FromStr for EsploraAuthMode { /// don't allow `:` in the username because Core's rpcauth.py forbids it /// too (Basic-auth header is `user:pass`, so a `:` in the user breaks /// the decode). -#[derive(Debug, Clone)] +#[derive(Clone)] pub struct RpcAuthEntry { pub username: String, /// The salt EXACTLY as written in the config line. Core's @@ -305,6 +305,16 @@ pub struct RpcAuthEntry { pub hash: Vec, } +/// The salt and the tag together let anyone holding them test password +/// guesses offline, so `Debug` shows the user only. +impl std::fmt::Debug for RpcAuthEntry { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("RpcAuthEntry") + .field("username", &self.username) + .finish_non_exhaustive() + } +} + impl RpcAuthEntry { pub fn parse(s: &str) -> Result { let raw = s.trim(); @@ -408,7 +418,10 @@ pub struct ProfileDefaults { } /// Resolved node configuration after merging CLI args, config file, and defaults. -#[derive(Debug, Clone)] +/// +/// `Debug` is written by hand (see the impl below): the struct holds RPC, Tor, +/// Esplora and webhook secrets. +#[derive(Clone)] pub struct Config { pub network: Network, pub datadir: PathBuf, @@ -1425,6 +1438,19 @@ pub struct Config { pub pending_notes: Vec, } +/// `Config` holds `rpcpassword`, `rpcauth`, `torpassword`, `esplorauserpass` +/// and `reorgwebhooksecret`, so a derived `Debug` would print them wherever a +/// config reached a `{:?}` or a panic message. This one names the network +/// and the data directory and nothing else. +impl std::fmt::Debug for Config { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("Config") + .field("network", &self.network) + .field("datadir", &self.datadir) + .finish_non_exhaustive() + } +} + /// A deferred operator-facing note emitted during config load. Its /// `level` selects the tracing macro in `main.rs`. #[derive(Debug, Clone)] @@ -8077,6 +8103,13 @@ pub fn filter_unsupported_core_cli_args( Ok((kept, warnings)) } +/// Trim a `bitcoin.conf` line, key or value the way Bitcoin Core does +/// (`TrimString(str, " \t\r\n")` in src/common/config.cpp): spaces, tabs, +/// CRs and LFs only, not every Unicode whitespace character. +fn trim_conf_whitespace(s: &str) -> &str { + s.trim_matches([' ', '\t', '\r', '\n']) +} + /// Parsed bitcoin.conf file. #[derive(Debug, Default)] pub struct ConfigFile { @@ -8106,8 +8139,15 @@ impl ConfigFile { for (idx, line) in content.lines().enumerate() { let line_no = idx + 1; - let trimmed = line.trim(); - if trimmed.is_empty() || trimmed.starts_with('#') { + // Bitcoin Core's GetConfigOptions (src/common/config.cpp): a `#` + // anywhere on a line starts a comment, and what is left is + // trimmed of spaces, tabs, CRs and LFs. + let (line, used_hash) = match line.find('#') { + Some(pos) => (&line[..pos], true), + None => (line, false), + }; + let trimmed = trim_conf_whitespace(line); + if trimmed.is_empty() { continue; } @@ -8120,13 +8160,23 @@ impl ConfigFile { // Key=value or bare key let (key, value) = if let Some(eq_pos) = trimmed.find('=') { - let k = trimmed[..eq_pos].trim().to_string(); - let v = trimmed[eq_pos + 1..].trim().to_string(); + let k = trim_conf_whitespace(&trimmed[..eq_pos]).to_string(); + let v = trim_conf_whitespace(&trimmed[eq_pos + 1..]).to_string(); (k, v) } else { (trimmed.to_string(), "1".to_string()) }; + // Core refuses a `#` on an rpcpassword line rather than guess + // whether it starts a comment or belongs to the password. Its + // check is a substring match on the key, as here. + if used_hash && key.contains("rpcpassword") { + return Err(format!( + "Error reading configuration file: parse error on line {line_no}, using # \ + in rpcpassword can be ambiguous and should be avoided" + )); + } + // Accept Bitcoin Core's hyphenated `reindex-chainstate` config // spelling — Core's own conf-file key is hyphenated, so a // migrated bitcoin.conf carries this form. satd's canonical key @@ -8899,6 +8949,39 @@ pub fn build_env_filter(config: &Config) -> tracing_subscriber::EnvFilter { env_filter } +/// Target prefixes of dependency crates whose TRACE output prints raw +/// requests. jsonrpsee-server logs each HTTP request with its headers, +/// `Authorization` included, before satd's auth layer has run +/// (jsonrpsee-server 0.26 `src/server.rs:1026`), and jsonrpsee-core logs call +/// parameters, raw request bodies and responses (`src/server/rpc_module.rs:310, +/// 356, 409`). tungstenite logs every WebSocket frame with its payload +/// (`src/protocol/frame/mod.rs:228, 257`). Bitcoin Core logs no credentials +/// and no RPC parameters at any level. +const REQUEST_DUMP_TARGET_PREFIXES: [&str; 2] = ["jsonrpsee", "tungstenite"]; + +/// Whether `meta` is TRACE output from one of [`REQUEST_DUMP_TARGET_PREFIXES`]. +fn is_request_dump(meta: &tracing::Metadata<'_>) -> bool { + *meta.level() == tracing::Level::TRACE + && REQUEST_DUMP_TARGET_PREFIXES + .iter() + .any(|prefix| meta.target().starts_with(prefix)) +} + +/// A global filter layer that drops [`is_request_dump`] spans and events. Their +/// DEBUG and higher output still follows the [`build_env_filter`] filter. +/// +/// It is a separate layer rather than an `EnvFilter` directive because +/// `EnvFilter` lets the most specific directive win: a `RUST_LOG` naming a +/// longer target, such as `jsonrpsee_core::server=trace`, would override a +/// `jsonrpsee_core=debug` cap. No `-loglevel`, `-debug`, `logging` call or +/// `RUST_LOG` value can turn this layer off. +pub fn request_dump_guard() -> impl tracing_subscriber::Layer { + tracing_subscriber::filter::FilterFn::new(|meta: &tracing::Metadata<'_>| !is_request_dump(meta)) + // Allows any level itself, so the EnvFilter's hint still sets the + // global maximum level. + .with_max_level_hint(tracing_subscriber::filter::LevelFilter::TRACE) +} + /// Normalize a Bitcoin Core `-loglevel` severity to a `tracing` level. /// Core accepts `trace|debug|info|warning|error`; `tracing` spells the /// warning level `warn`. Returns `None` for an unrecognized level. @@ -9316,6 +9399,10 @@ fn check_money_range(rate: u64, raw: &str) -> Result { Ok(rate) } +#[cfg(test)] +#[path = "config_localharden_tests.rs"] +mod localharden_tests; + #[cfg(test)] mod tests { use super::*; diff --git a/satd/src/config_localharden_tests.rs b/satd/src/config_localharden_tests.rs new file mode 100644 index 000000000..8280da9ab --- /dev/null +++ b/satd/src/config_localharden_tests.rs @@ -0,0 +1,244 @@ +//! `bitcoin.conf` comment handling and secret-free `Debug` output. + +use super::*; + +/// Parse `body` as the `--conf` file of a regtest node with its datadir in +/// `dir`, plus `extra` command-line flags. +fn config_from(dir: &tempfile::TempDir, body: &str, extra: &[&str]) -> Result { + let conf = dir.path().join("comments.conf"); + std::fs::write(&conf, body).unwrap(); + let mut args = vec![ + "satd", + "--regtest", + "--datadir", + dir.path().to_str().unwrap(), + "--conf", + conf.to_str().unwrap(), + ]; + args.extend_from_slice(extra); + Config::from_cli(CliArgs::try_parse_from(args).unwrap()) +} + +fn global(cf: &ConfigFile, key: &str) -> Vec { + cf.global.get(key).cloned().unwrap_or_default() +} + +/// Bitcoin Core's GetConfigOptions (src/common/config.cpp) drops everything +/// from the first `#` on a line, wherever it is, before it reads the line. +#[test] +fn an_inline_comment_is_not_part_of_the_value() { + let cf = ConfigFile::parse( + "txindex=1 # keep the transaction index\n\ + rpcport=8332\t# the RPC port\n\ + uacomment=abc#def\n\ + includeconf=extra.conf # more settings\n\ + # a whole-line comment\n\ + \x20 # an indented comment\n\ + [test] # the testnet3 section\n\ + rpcport=18332 #x\n", + ) + .unwrap(); + assert_eq!(global(&cf, "txindex"), ["1"]); + assert_eq!(global(&cf, "rpcport"), ["8332"]); + assert_eq!(global(&cf, "uacomment"), ["abc"]); + assert_eq!(global(&cf, "includeconf"), ["extra.conf"]); + assert_eq!( + cf.sections.get("test").and_then(|s| s.get("rpcport")).cloned(), + Some(vec!["18332".to_string()]), + "a section header followed by a comment still opens the section" + ); +} + +/// The values a node actually runs with: before, `rpcport=18555 # c` failed +/// to parse as a port and the default was used without a word. +#[test] +fn inline_comments_reach_the_running_config_as_core_reads_them() { + let dir = tempfile::tempdir().expect("tempdir"); + let cfg = config_from( + &dir, + "rpcuser=alice # the operator\nrpcpassword=pw\n[regtest] # this chain\n\ + rpcport=18555 # not the default\n", + &[], + ) + .unwrap(); + assert_eq!(cfg.rpcport, 18555); + assert_eq!(cfg.rpcuser.as_deref(), Some("alice")); +} + +/// Core refuses an `rpcpassword` line that holds a `#` at all, rather than +/// guess whether it starts a comment or belongs to the password +/// (src/common/config.cpp GetConfigOptions; the caller adds the +/// "Error reading configuration file: " prefix, src/common/init.cpp). +#[test] +fn a_hash_on_an_rpcpassword_line_is_refused_with_cores_message() { + let cases = [ + ("rpcpassword=pw # old password\n", 1), + ("rpcpassword=a#b\n", 1), + ("server=1\n[regtest]\nrpcpassword=#\n", 3), + ]; + for (body, line) in cases { + let err = ConfigFile::parse(body).expect_err(body); + assert_eq!( + err, + format!( + "Error reading configuration file: parse error on line {line}, using # in \ + rpcpassword can be ambiguous and should be avoided" + ), + "{body:?}" + ); + } + + // satd reads a bare key as `key=1`, where Core refuses any line without + // `=`; with a `#` on it, a bare `rpcpassword` is refused too. + assert!(ConfigFile::parse("rpcpassword # no value\n").is_err()); + + // A commented-out password line is only a comment, and a `#` on any + // other key is a comment, as in Core. + let cf = ConfigFile::parse("# rpcpassword=old\nrpcpassword=pw\nrpcuser=bob#x\n").unwrap(); + assert_eq!(global(&cf, "rpcpassword"), ["pw"]); + assert_eq!(global(&cf, "rpcuser"), ["bob"]); + + // The running config refuses it the same way. + let dir = tempfile::tempdir().expect("tempdir"); + let err = config_from(&dir, "rpcuser=u\nrpcpassword=pw # c\n", &[]).unwrap_err(); + assert!(err.contains("using # in rpcpassword"), "got: {err}"); +} + +/// Core trims only spaces, tabs, CRs and LFs around a line, a key and a value +/// (`TrimString(str, " \t\r\n")`), so a vertical tab or a form feed stays in +/// the value. +#[test] +fn whitespace_is_trimmed_as_core_trims_it() { + let cf = ConfigFile::parse(" \t rpcuser \t=\t alice \t\r\nuacomment=x\u{b}\nrpcbind=\u{c}y\n").unwrap(); + assert_eq!(global(&cf, "rpcuser"), ["alice"]); + assert_eq!(global(&cf, "uacomment"), ["x\u{b}"]); + assert_eq!(global(&cf, "rpcbind"), ["\u{c}y"]); +} + +/// `Config` holds the RPC, Tor, Esplora and webhook secrets. Its `Debug` +/// output must not, so a stray `{:?}` or a panic message cannot write them to +/// the log. +#[test] +fn config_debug_output_carries_no_secrets() { + let dir = tempfile::tempdir().expect("tempdir"); + let cfg = config_from( + &dir, + "", + &[ + "--rpcuser=dbguser", + "--rpcpassword=RpcPasswordMarker", + "--rpcauth=authuser:0123456789abcdef$\ + 9383f6d244049af54e59a84188e2f2b1e58ff20de019156bc0c430ff8ae4c7a3", + "--torpassword=TorPasswordMarker", + "--esplorauserpass=esplora:EsploraPasswordMarker", + "--reorg-webhook=http://127.0.0.1:9/hook", + "--reorg-webhook-secret=WebhookSecretMarker", + ], + ) + .unwrap(); + assert_eq!(cfg.rpcpassword.as_deref(), Some("RpcPasswordMarker")); + let shown = format!("{cfg:?}"); + for secret in [ + "RpcPasswordMarker", + "TorPasswordMarker", + "EsploraPasswordMarker", + "WebhookSecretMarker", + "9383f6d2", + ] { + assert!(!shown.contains(secret), "Config Debug shows {secret}: {shown}"); + } + let shown_alt = format!("{cfg:#?}"); + assert!(!shown_alt.contains("RpcPasswordMarker"), "{shown_alt}"); + assert!(shown.contains("Regtest"), "Config Debug still names the network: {shown}"); + + let entry = &cfg.rpcauth[0]; + let shown = format!("{entry:?}"); + assert!(shown.contains("authuser"), "{shown}"); + assert!(!shown.contains(&format!("{:?}", entry.hash)), "rpcauth hash shown: {shown}"); + + let hook = crate::reload::webhook_target_from(&cfg).unwrap(); + let shown = format!("{hook:?}"); + assert!(shown.contains("127.0.0.1:9/hook"), "{shown}"); + assert!(!shown.contains("WebhookSecretMarker"), "webhook secret shown: {shown}"); +} + +/// A callsite for metadata built by hand: the filters only read the metadata +/// they are handed, never the callsite's. +struct TestCallsite; + +impl tracing::callsite::Callsite for TestCallsite { + fn set_interest(&self, _: tracing::subscriber::Interest) {} + fn metadata(&self) -> &tracing::Metadata<'_> { + unreachable!("filters read the metadata they are handed") + } +} + +static TEST_CALLSITE: TestCallsite = TestCallsite; + +/// Whether satd's log stack, with `filter` as its reloadable filter and +/// [`request_dump_guard`] beside it as `main` installs them, writes an event +/// at `level` from `target`. The subscriber is asked directly, so no global +/// dispatcher or callsite cache is involved. +fn would_log(filter: tracing_subscriber::EnvFilter, target: &str, level: tracing::Level) -> bool { + use tracing::Subscriber as _; + use tracing_subscriber::layer::SubscriberExt as _; + let meta = tracing::Metadata::new( + "event", + target, + level, + None, + None, + None, + tracing::field::FieldSet::new(&[], tracing::callsite::Identifier(&TEST_CALLSITE)), + tracing::metadata::Kind::EVENT, + ); + tracing_subscriber::registry() + .with(filter) + .with(request_dump_guard()) + .enabled(&meta) +} + +/// jsonrpsee's TRACE output prints the HTTP `Authorization` header and the +/// call parameters; tungstenite's prints WebSocket payloads. Trace logging +/// must not turn either on, while their DEBUG output and satd's own TRACE +/// output still get through. +#[test] +fn trace_logging_leaves_out_the_request_dumps() { + use tracing::Level; + let dumps = [ + "jsonrpsee-server", + "jsonrpsee_server::server", + "jsonrpsee-core", + "jsonrpsee_core::server::rpc_module", + "jsonrpsee", + "tungstenite::protocol::frame", + ]; + + let cfg = Config::from_cli( + CliArgs::try_parse_from(["satd", "--regtest", "--loglevel=trace"]).unwrap(), + ) + .unwrap(); + for target in dumps { + assert!( + !would_log(build_env_filter(&cfg), target, Level::TRACE), + "{target} TRACE is logged under -loglevel=trace" + ); + assert!( + would_log(build_env_filter(&cfg), target, Level::DEBUG), + "{target} DEBUG must still be logged under -loglevel=trace" + ); + } + assert!(would_log(build_env_filter(&cfg), "node::rpc::server", Level::TRACE)); + + // Directives naming the targets, more specifically than the guard's + // prefixes, cannot turn them back on. + let explicit = "trace,jsonrpsee-server=trace,jsonrpsee_core::server::rpc_module=trace,\ + tungstenite::protocol=trace"; + for target in dumps { + assert!( + !would_log(tracing_subscriber::EnvFilter::new(explicit), target, Level::TRACE), + "{target} TRACE is logged when a directive names it" + ); + } + assert!(would_log(tracing_subscriber::EnvFilter::new(explicit), "node::net", Level::TRACE)); +} diff --git a/satd/src/main.rs b/satd/src/main.rs index 351211de6..7db0f52e1 100644 --- a/satd/src/main.rs +++ b/satd/src/main.rs @@ -585,7 +585,12 @@ async fn run() -> Option> use tracing_subscriber::Layer as _; use tracing_subscriber::layer::SubscriberExt; use tracing_subscriber::util::SubscriberInitExt; - let registry = tracing_subscriber::registry().with(filter_layer); + // `request_dump_guard` keeps the RPC and WebSocket libraries' TRACE + // output, which prints request headers and parameters, out of the + // log whatever the filter above allows. + let registry = tracing_subscriber::registry() + .with(filter_layer) + .with(config::request_dump_guard()); // Only colorize when stdout is an actual terminal and the operator // hasn't opted out via NO_COLOR (https://no-color.org). Under journald // / a pipe, `is_terminal()` is false, so we don't leak ANSI escapes diff --git a/satd/src/reload.rs b/satd/src/reload.rs index a03b65be9..adf0da920 100644 --- a/satd/src/reload.rs +++ b/satd/src/reload.rs @@ -69,12 +69,22 @@ impl LogReloadHandle { /// [`SharedWebhook`] so a SIGHUP reload can change the URL/secret — or turn the /// webhook on/off — without restarting the dispatcher task. `None` means "no /// webhook configured"; the dispatcher then drains and drops records. -#[derive(Clone, Debug)] +#[derive(Clone)] pub struct WebhookTarget { pub url: String, pub secret: Option, } +/// `secret` is the webhook's HMAC key; `Debug` says only whether one is set. +impl std::fmt::Debug for WebhookTarget { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("WebhookTarget") + .field("url", &self.url) + .field("secret", &self.secret.as_ref().map(|_| "")) + .finish() + } +} + /// Shared, reloadable reorg-webhook target. The dispatcher (spawned once at /// startup regardless of whether a webhook is configured) snapshots this per /// record; the reload path swaps its contents. diff --git a/satd/tests/local_hardening.rs b/satd/tests/local_hardening.rs new file mode 100644 index 000000000..e01ab97c7 --- /dev/null +++ b/satd/tests/local_hardening.rs @@ -0,0 +1,55 @@ +//! What satd writes to its log when trace logging is on. +//! +//! Bitcoin Core logs neither RPC credentials nor RPC parameters at any log +//! level. satd's RPC server is jsonrpsee, whose TRACE output prints each HTTP +//! request with its headers (the `Authorization` header included) and each +//! call's parameters, so `-loglevel=trace` must not let that output through. + +mod common; + +use base64::Engine as _; +use common::TestNode; +use serde_json::json; + +#[test] +fn trace_logging_never_writes_rpc_credentials_or_parameters() { + let user = "traceloguser"; + let pass = "TraceLogPasswordMarker"; + let param = "TraceLogParamMarker"; + let user_arg = format!("--rpcuser={user}"); + let pass_arg = format!("--rpcpassword={pass}"); + let mut node = TestNode::start(&["--loglevel=trace", &user_arg, &pass_arg]); + // The harness's RPC calls authenticate with `cookie` as `user:pass`. + node.cookie = format!("{user}:{pass}"); + + node.rpc_ok("getblockcount", vec![]); + node.rpc_ok("echo", vec![json!(param)]); + node.stop(); + + let log = std::fs::read_to_string(&node.stderr_log).expect("read the node's log"); + // jsonrpsee's own DEBUG lines must still be in the log, or the absence + // checks below could pass because its output was off altogether. (Before + // the TRACE cap, these same arguments put every request's headers in the + // log.) + assert!( + log.contains("Accepting new connection"), + "jsonrpsee-server's debug output is missing; only its TRACE output should be left out" + ); + + let basic = base64::engine::general_purpose::STANDARD.encode(format!("{user}:{pass}")); + for (what, secret) in [ + ("the Basic credential", basic.as_str()), + ("the password", pass), + ("an RPC parameter", param), + ] { + assert!( + !log.contains(secret), + "the trace log contains {what} ({secret}): {}", + log.lines() + .filter(|l| l.contains(secret)) + .take(3) + .collect::>() + .join("\n") + ); + } +}