diff --git a/crates/rustingface/src/cmd/serve.rs b/crates/rustingface/src/cmd/serve.rs index d9a2edc..27e642e 100644 --- a/crates/rustingface/src/cmd/serve.rs +++ b/crates/rustingface/src/cmd/serve.rs @@ -3,7 +3,7 @@ use anyhow::{Context, Result}; use rustingface_api::auth::Tokens; use rustingface_api::{AppState, router, telemetry}; -use rustingface_entities::config::Config; +use rustingface_entities::config::{AuthMode, Config}; use tokio::net::TcpListener; use tokio::signal::unix::{SignalKind, signal}; use tracing::info; @@ -14,8 +14,16 @@ use crate::{config::read_optional_secret, wire}; pub async fn run(config: Config) -> Result<()> { let registry = wire::registry(&config)?; - let token_file = - read_optional_secret(config.auth.token_file.as_ref()).context("auth.token_file")?; + // Only read the client token file when bearer auth is on. The config + // template names the path unconditionally so an operator can see where it + // goes, and refusing to start over a credential the running configuration + // never consults would make that documentation a landmine. + let token_file = match config.auth.mode { + AuthMode::None => None, + AuthMode::Bearer => { + read_optional_secret(config.auth.token_file.as_ref()).context("auth.token_file")? + } + }; let tokens = Tokens::new(config.auth.mode, token_file.as_deref())?; let metrics = if config.observability.metrics { diff --git a/crates/rustingface/src/wire.rs b/crates/rustingface/src/wire.rs index 17647db..63f1968 100644 --- a/crates/rustingface/src/wire.rs +++ b/crates/rustingface/src/wire.rs @@ -16,10 +16,18 @@ use crate::config::read_optional_secret; /// Build the registry described by `config`. pub fn registry(config: &Config) -> Result { - let access_key_id = read_optional_secret(config.storage.access_key_id_file.as_ref()) - .context("storage.access_key_id_file")?; - let secret_access_key = read_optional_secret(config.storage.secret_access_key_file.as_ref()) - .context("storage.secret_access_key_file")?; + // A local store authenticates to nothing, so its credential paths are read + // only when there is an S3 endpoint that would use them. + let (access_key_id, secret_access_key) = if config.storage.local_path.is_some() { + (None, None) + } else { + ( + read_optional_secret(config.storage.access_key_id_file.as_ref()) + .context("storage.access_key_id_file")?, + read_optional_secret(config.storage.secret_access_key_file.as_ref()) + .context("storage.secret_access_key_file")?, + ) + }; if config.storage.local_path.is_some() { warn!( diff --git a/crates/rustingface/tests/startup.rs b/crates/rustingface/tests/startup.rs new file mode 100644 index 0000000..aa3acca --- /dev/null +++ b/crates/rustingface/tests/startup.rs @@ -0,0 +1,224 @@ +//! Startup checks: the service must not refuse to start over a credential its +//! own configuration never consults. +//! +//! The config template names every credential path unconditionally, so an +//! operator can see where each one goes. That documentation becomes a landmine +//! if the binary insists on reading a file the running configuration ignores — +//! which is exactly how the first deploy of this service failed, on +//! `auth.token_file` with `auth.mode = "none"`. + +use std::path::PathBuf; +use std::process::Command; + +/// A directory that goes away with the test. +struct Scratch(PathBuf); + +impl Scratch { + fn new(name: &str) -> Self { + let dir = std::env::temp_dir().join(format!( + "rustingface-startup-{name}-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + std::fs::create_dir_all(&dir).unwrap(); + Self(dir) + } + + fn path(&self, name: &str) -> PathBuf { + self.0.join(name) + } +} + +impl Drop for Scratch { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } +} + +/// Path to the binary under test, as cargo built it for this test run. +fn binary() -> PathBuf { + let mut path = std::env::current_exe().expect("test binary path"); + path.pop(); // the deps/ directory + if path.ends_with("deps") { + path.pop(); + } + path.join("rustingface") +} + +/// Start `serve` and report whether it was still running a moment later. +/// +/// `serve` blocks by design, so "did it start" is the absence of an early +/// exit. A configuration error surfaces immediately, well inside the window. +fn serve_survives_startup(config: &PathBuf) -> (bool, String) { + use std::io::Read; + let mut child = Command::new(binary()) + .arg("--config") + .arg(config) + .arg("serve") + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .spawn() + .expect("spawning rustingface serve"); + + for _ in 0..40 { + if let Some(status) = child.try_wait().expect("polling the child") { + let mut output = String::new(); + if let Some(mut err) = child.stderr.take() { + let _ = err.read_to_string(&mut output); + } + return (status.success(), output); + } + std::thread::sleep(std::time::Duration::from_millis(50)); + } + let _ = child.kill(); + let _ = child.wait(); + (true, String::from("still running")) +} + +/// Run a subcommand against `config` and return (success, combined output). +fn run(config: &PathBuf, args: &[&str]) -> (bool, String) { + let output = Command::new(binary()) + .arg("--config") + .arg(config) + .args(args) + .output() + .expect("running rustingface"); + let mut combined = String::from_utf8_lossy(&output.stdout).into_owned(); + combined.push_str(&String::from_utf8_lossy(&output.stderr)); + (output.status.success(), combined) +} + +/// A config over a local store, with the given extra sections appended. +fn config_with(scratch: &Scratch, extra: &str) -> PathBuf { + let bucket = scratch.path("bucket"); + let config = scratch.path("config.toml"); + std::fs::write( + &config, + format!( + r#" +[server] +listen = "127.0.0.1:0" + +[storage] +local_path = "{}" + +[upstream] +enabled = false + +[observability] +log_format = "text" +{extra} +"#, + bucket.display() + ), + ) + .unwrap(); + config +} + +#[test] +fn a_named_but_absent_client_token_file_is_ignored_when_auth_is_off() { + let scratch = Scratch::new("auth-none"); + // Exactly the shape the deployed config template produces: the path is + // documented, the file does not exist, and auth.mode is "none". + let config = config_with( + &scratch, + r#" +[auth] +mode = "none" +token_file = "/nonexistent/client-tokens" +"#, + ); + + // `serve` is the subcommand that reads it, so `serve` is what must not + // refuse. Running `list` here would pass whether or not the bug exists. + let (ok, output) = serve_survives_startup(&config); + assert!( + ok, + "a credential the configuration never consults must not block startup:\n{output}" + ); +} + +#[test] +fn bearer_auth_still_fails_loudly_when_its_token_file_is_missing() { + let scratch = Scratch::new("auth-bearer"); + let config = config_with( + &scratch, + r#" +[auth] +mode = "bearer" +token_file = "/nonexistent/client-tokens" +"#, + ); + + // `serve` consults it, and must refuse rather than come up admitting + // everyone. + let (ok, output) = serve_survives_startup(&config); + assert!(!ok, "bearer auth with no token file must refuse to start"); + assert!( + output.contains("auth.token_file") || output.contains("client-tokens"), + "the failure must name the credential it needed:\n{output}" + ); +} + +#[test] +fn a_local_store_ignores_the_s3_credential_paths() { + let scratch = Scratch::new("local-store"); + let config = config_with( + &scratch, + r#" +[auth] +mode = "none" +"#, + ); + // Append S3 credential paths that do not exist; a local store + // authenticates to nothing and must not read them. + let mut text = std::fs::read_to_string(&config).unwrap(); + text = text.replace( + "[upstream]", + "access_key_id_file = \"/nonexistent/ak\"\nsecret_access_key_file = \"/nonexistent/sk\"\n\n[upstream]", + ); + std::fs::write(&config, text).unwrap(); + + let (ok, output) = run(&config, &["list"]); + assert!( + ok, + "a local store must not require S3 credentials:\n{output}" + ); +} + +#[test] +fn a_missing_s3_credential_is_still_reported_by_name() { + let scratch = Scratch::new("s3-missing"); + let config = scratch.path("config.toml"); + std::fs::write( + &config, + r#" +[server] +listen = "127.0.0.1:0" + +[storage] +endpoint = "http://127.0.0.1:9000" +bucket = "rustingface" +access_key_id_file = "/nonexistent/ak" +secret_access_key_file = "/nonexistent/sk" + +[upstream] +enabled = false + +[observability] +log_format = "text" +"#, + ) + .unwrap(); + + let (ok, output) = run(&config, &["list"]); + assert!(!ok, "an S3 store with no credentials must fail"); + assert!( + output.contains("access_key_id_file"), + "the failure must name the credential it needed:\n{output}" + ); +}