fix(bin): don't refuse to start over a credential the config never reads
The deployed config template names every credential path unconditionally so an operator can see where each one goes. `serve` then read auth.token_file whether or not bearer auth was on, so a deployment with auth.mode = "none" -- the shipped default -- failed at startup with "/etc/rustingface/client-tokens does not exist". The documentation was a landmine. Reads a credential only when the running configuration actually consults it: the client token file only under bearer auth, and the object-store keys only when there is an S3 endpoint rather than a local path. A credential that *is* needed and missing still fails loudly and by name; that is the behaviour worth keeping. Adds tests/startup.rs, which drives the real binary. The first case spawns `serve` rather than a subcommand that never touches the file -- running `list` there would have passed either way. Verified the test catches the bug: reverting the fix reproduces the exact production error. Observed on deploy run 8, which got the whole pipeline through sysusers, rsync, firewalld and restart before the service exited on this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XZG2i4AmfSqE97EJGBVb64
This commit is contained in:
@@ -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 {
|
||||
|
||||
@@ -16,10 +16,18 @@ use crate::config::read_optional_secret;
|
||||
|
||||
/// Build the registry described by `config`.
|
||||
pub fn registry(config: &Config) -> Result<Registry> {
|
||||
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!(
|
||||
|
||||
@@ -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}"
|
||||
);
|
||||
}
|
||||
Reference in New Issue
Block a user