diff --git a/crates/app/src/cmd_manage.rs b/crates/app/src/cmd_manage.rs index af84db359..271b688b4 100755 --- a/crates/app/src/cmd_manage.rs +++ b/crates/app/src/cmd_manage.rs @@ -9,9 +9,63 @@ //! Types are in `manage_types.rs`; HTTP transport and dispatch are in //! `manage_http.rs`. -use crate::manage_http::{basic_auth, dispatch, resolve_endpoint, resolve_password}; +use std::io::Write; + +use crate::manage_http::{basic_auth, dispatch, resolve_endpoint}; pub use crate::manage_types::ManageArgs; +/// Printed on stderr when the password comes from `--password`. +const PASSWORD_FLAG_WARNING: &str = "Warning: --password is visible in process listings and \ + shell history. Omit it and set EXTENDDB_PASSWORD instead."; + +/// Choose the password from the `--password` value and the `EXTENDDB_PASSWORD` +/// value. The flag takes precedence when both are set. +fn choose_password(flag: Option, env: Option) -> anyhow::Result { + flag.or(env).ok_or_else(|| { + anyhow::anyhow!("No password provided. Use --password VALUE or set EXTENDDB_PASSWORD.") + }) +} + +/// The warning to print on stderr: `Some` when `--password` was given, `None` +/// when it was not. The flag takes precedence, so `Some` means the password +/// came from the flag. The result depends only on `flag_given`, so the printed +/// text never holds the password. +/// +/// CodeQL's `rust/cleartext-logging` query traces a printed value back to the +/// password when both come from one function result. So the warning comes from +/// this function, which takes only a `bool`, and not from `choose_password`. +fn flag_warning(flag_given: bool) -> Option<&'static str> { + flag_given.then_some(PASSWORD_FLAG_WARNING) +} + +/// Resolve the password from the `--password` value or the `EXTENDDB_PASSWORD` +/// value, and write the warning to `warnings` when it came from the flag. +/// +/// Never reads from stdin. Errors if no password source is available. A failed +/// write of the warning does not stop the command. +fn resolve_password_with( + cli_password: Option, + env_password: Option, + warnings: &mut impl Write, +) -> anyhow::Result { + let warning = flag_warning(cli_password.is_some()); + let password = choose_password(cli_password, env_password)?; + if let Some(warning) = warning { + let _ = writeln!(warnings, "{warning}"); + } + Ok(password) +} + +/// [`resolve_password_with`] for the command line: reads `EXTENDDB_PASSWORD` +/// and writes the warning to stderr. +fn resolve_password(cli_password: Option) -> anyhow::Result { + resolve_password_with( + cli_password, + std::env::var("EXTENDDB_PASSWORD").ok(), + &mut std::io::stderr(), + ) +} + // Caller (`run_interactive`) requires a Future; removing `async` would need a // manual `impl Future` wrapper for no benefit. #[allow(clippy::unused_async)] @@ -42,3 +96,51 @@ pub async fn run(args: ManageArgs) -> anyhow::Result<()> { anyhow::bail!("HTTP {status}: {body}") } } + +#[cfg(test)] +mod tests { + use super::{PASSWORD_FLAG_WARNING, resolve_password_with}; + + fn value(s: &str) -> Option { + Some(s.to_owned()) + } + + /// Runs `resolve_password_with` and returns its result with what it wrote. + fn resolve(flag: Option, env: Option) -> (anyhow::Result, String) { + let mut warnings = Vec::new(); + let result = resolve_password_with(flag, env, &mut warnings); + (result, String::from_utf8(warnings).unwrap()) + } + + #[test] + fn a_password_from_the_flag_is_used_with_the_warning() { + let (password, warnings) = resolve(value("from-flag"), None); + assert_eq!(password.unwrap(), "from-flag"); + assert_eq!(warnings, format!("{PASSWORD_FLAG_WARNING}\n")); + assert!(!warnings.contains("from-flag")); + } + + #[test] + fn a_password_from_the_variable_is_used_with_no_warning() { + let (password, warnings) = resolve(None, value("from-env")); + assert_eq!(password.unwrap(), "from-env"); + assert_eq!(warnings, ""); + } + + #[test] + fn the_flag_takes_precedence_over_the_variable_and_warns() { + let (password, warnings) = resolve(value("from-flag"), value("from-env")); + assert_eq!(password.unwrap(), "from-flag"); + assert_eq!(warnings, format!("{PASSWORD_FLAG_WARNING}\n")); + } + + #[test] + fn no_flag_and_no_variable_is_an_error_with_no_warning() { + let (password, warnings) = resolve(None, None); + assert_eq!( + password.unwrap_err().to_string(), + "No password provided. Use --password VALUE or set EXTENDDB_PASSWORD." + ); + assert_eq!(warnings, ""); + } +} diff --git a/crates/app/src/manage_http.rs b/crates/app/src/manage_http.rs index fadc794c2..747c73b6b 100755 --- a/crates/app/src/manage_http.rs +++ b/crates/app/src/manage_http.rs @@ -68,19 +68,6 @@ pub fn resolve_endpoint( Ok((format!("{addr}:{}", app_config.server.port), use_tls, cert)) } -/// Resolve the password from: CLI arg or `EXTENDDB_PASSWORD` env var. -/// -/// Never reads from stdin. Errors if no password source is available. -pub fn resolve_password(cli_password: Option) -> anyhow::Result { - if let Some(pw) = cli_password { - return Ok(pw); - } - if let Ok(pw) = std::env::var("EXTENDDB_PASSWORD") { - return Ok(pw); - } - anyhow::bail!("No password provided. Use --password VALUE or set EXTENDDB_PASSWORD.") -} - /// Build a Basic auth header value. pub fn basic_auth(user: &str, password: &str) -> String { let encoded = base64::engine::general_purpose::STANDARD.encode(format!("{user}:{password}")); diff --git a/crates/app/src/manage_types.rs b/crates/app/src/manage_types.rs index c3d3ec60e..0076f13a5 100755 --- a/crates/app/src/manage_types.rs +++ b/crates/app/src/manage_types.rs @@ -13,7 +13,8 @@ pub struct ManageArgs { /// Credentials: admin username, or account_id/user_name for IAM users #[arg(long)] pub user: String, - /// Password (reads from EXTENDDB_PASSWORD env var if not provided) + /// Password (visible in process listings and shell history; when omitted, + /// read from the EXTENDDB_PASSWORD env var) #[arg(long)] pub password: Option, /// Server endpoint (default: from config file) diff --git a/docs/getting-started.md b/docs/getting-started.md index efbcd46d2..1978740ba 100755 --- a/docs/getting-started.md +++ b/docs/getting-started.md @@ -800,7 +800,7 @@ The management API is available at `/management/*` on the running extenddb serve Admin commands require admin credentials (the username and password printed during `extenddb init`). IAM user self-service commands accept `account_id/user_name` as the `--user` value. -**Note:** The `--password` flag is visible in process listings (`ps aux`) and shell history. For sensitive environments, leave out `--password` and set the `EXTENDDB_PASSWORD` environment variable instead, with `read -rs EXTENDDB_PASSWORD && export EXTENDDB_PASSWORD`, which keeps the password out of shell history. `extenddb manage` reads the password from `EXTENDDB_PASSWORD`. `--admin-password`, `--new-password`, `--user-password` and `--secret-access-key` have no environment variable. +**Note:** The `--password` flag is visible in process listings (`ps aux`) and shell history. For sensitive environments, leave out `--password` and set the `EXTENDDB_PASSWORD` environment variable instead, with `read -rs EXTENDDB_PASSWORD && export EXTENDDB_PASSWORD`, which keeps the password out of shell history. `extenddb manage` reads the password from `EXTENDDB_PASSWORD`, and it prints a warning on stderr when it gets `--password`. `--admin-password`, `--new-password`, `--user-password` and `--secret-access-key` have no environment variable. ### List admin users diff --git a/docs/manuals/10-security-model.md b/docs/manuals/10-security-model.md index 62b409a38..775f65a73 100755 --- a/docs/manuals/10-security-model.md +++ b/docs/manuals/10-security-model.md @@ -314,6 +314,7 @@ Import/export file paths are validated: - `extenddb init` prints admin credentials once; they are not stored in plaintext - `extenddb manage` reads the password from the `EXTENDDB_PASSWORD` environment variable when `--password` is not given, so the password does not appear in process listings +- `extenddb manage` prints a warning on stderr when it gets `--password` - Access key secrets are shown once on creation and cannot be retrieved afterward ### Logging diff --git a/docs/technical-debt.md b/docs/technical-debt.md index 44d79f8e8..fc125ff08 100755 --- a/docs/technical-debt.md +++ b/docs/technical-debt.md @@ -1,6 +1,6 @@ # Technical Debt Tracker -Last updated: 2026-09-16 +Last updated: 2026-11-09 ## Categories @@ -55,7 +55,7 @@ Last updated: 2026-09-16 | # | Item | Location | Priority | Origin | |---|------|----------|----------|--------| | S-1 | Multi-instance safety: no lease table prevents multiple extenddb instances sharing one PostgreSQL database | — | High | P25 review | -| S-2 | `--password` flag visible in process listings (`ps aux`) | `bin/cmd_manage.rs` | Low | P12b | +| S-2 | `--password` flag visible in process listings (`ps aux`) and shell history. `extenddb manage` prints a warning on stderr when the flag is used. With the flag omitted, it reads `EXTENDDB_PASSWORD`. The subcommand flags `--admin-password`, `--new-password`, `--user-password` and `--secret-access-key` have the same exposure, no environment variable and no warning (#916) | `crates/app/src/cmd_manage.rs` | Low | P12b | | S-3 | Release tarballs not GPG-signed — blocked on key ownership, public key distribution, CI secrets management | `devtools/build-release` | Medium | P28 | ## Testing