From 41489a1579c97282414d20aa161b16db87ba025d Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Thu, 5 Nov 2026 03:45:04 +0000 Subject: [PATCH 1/3] fix(cli): warn when manage gets the password as an argument extenddb manage takes the password from --password or, when the flag is absent, from the EXTENDDB_PASSWORD environment variable. A password given with the flag appears in process listings, which other users of the host can read, and in the history of an interactive shell. The command now prints one line on stderr when the password came from the flag: Warning: --password is visible in process listings and shell history. Omit it and set EXTENDDB_PASSWORD instead. Nothing else changes. The flag still takes precedence when both are set, a password from the variable prints nothing, and the output on stdout is the same. The choice of source is a pure function, choose_password, with unit tests for the flag, the variable, both and neither. resolve_password moves from manage_http.rs to its only caller, cmd_manage.rs, so that manage_http.rs stays under 500 lines. The --password help text now names the exposure and the variable, and the S-2 entry in docs/technical-debt.md records the warning. The management API takes passwords and secret access keys in request bodies, and Basic auth puts the password in the Authorization header of every request. The server has no HTTP trace layer and no request logging middleware, and no log call in the management or console code formats a body, a header map or a form. To check whether any log level writes these values, a SQLite build of the server ran with logging.level = "trace" and the log_level and sqlx_log_level settings at trace. It served create-admin, change-admin-password, create-user with a console password, change-user-password and import-access-key, a wrong Basic auth password for an admin and for an IAM user, and a failed and a successful console login, each with a marker secret. The whole log held 4,888 lines, 1,929 of them at TRACE. Each marker, the admin password, each base64 Basic credential, "Authorization" and "Basic " appeared 0 times. At trace level, axum also logs the parse error of a refused JSON body. For the bodies that carry a secret, that error quotes a value in two cases: a field that must hold a string holds a number or a boolean, for example a password sent as a JSON number, or the whole body is a bare JSON string, number or boolean. Neither the CLI nor the console sends such a body. Signed-off-by: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> --- crates/app/src/cmd_manage.rs | 75 +++++++++++++++++++++++++++++++++- crates/app/src/manage_http.rs | 13 ------ crates/app/src/manage_types.rs | 3 +- docs/technical-debt.md | 2 +- 4 files changed, 77 insertions(+), 16 deletions(-) diff --git a/crates/app/src/cmd_manage.rs b/crates/app/src/cmd_manage.rs index af84db359..a6d9c6fab 100755 --- a/crates/app/src/cmd_manage.rs +++ b/crates/app/src/cmd_manage.rs @@ -9,9 +9,43 @@ //! 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 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. The second element is +/// the warning to print on stderr: `Some` when the password came from the +/// flag, `None` when it came from the variable. +fn choose_password( + flag: Option, + env: Option, +) -> anyhow::Result<(String, Option<&'static str>)> { + match (flag, env) { + (Some(password), _) => Ok((password, Some(PASSWORD_FLAG_WARNING))), + (None, Some(password)) => Ok((password, None)), + (None, None) => Err(anyhow::anyhow!( + "No password provided. Use --password VALUE or set EXTENDDB_PASSWORD." + )), + } +} + +/// Resolve the password from the `--password` flag or the `EXTENDDB_PASSWORD` +/// env var, and warn on stderr when it came from the flag. +/// +/// Never reads from stdin. Errors if no password source is available. +fn resolve_password(cli_password: Option) -> anyhow::Result { + let (password, warning) = + choose_password(cli_password, std::env::var("EXTENDDB_PASSWORD").ok())?; + if let Some(warning) = warning { + eprintln!("{warning}"); + } + Ok(password) +} + // Caller (`run_interactive`) requires a Future; removing `async` would need a // manual `impl Future` wrapper for no benefit. #[allow(clippy::unused_async)] @@ -42,3 +76,42 @@ pub async fn run(args: ManageArgs) -> anyhow::Result<()> { anyhow::bail!("HTTP {status}: {body}") } } + +#[cfg(test)] +mod tests { + use super::{PASSWORD_FLAG_WARNING, choose_password}; + + fn value(s: &str) -> Option { + Some(s.to_owned()) + } + + #[test] + fn a_password_from_the_flag_is_used_with_the_warning() { + let (password, warning) = choose_password(value("from-flag"), None).unwrap(); + assert_eq!(password, "from-flag"); + assert_eq!(warning, Some(PASSWORD_FLAG_WARNING)); + } + + #[test] + fn a_password_from_the_variable_is_used_without_a_warning() { + let (password, warning) = choose_password(None, value("from-env")).unwrap(); + assert_eq!(password, "from-env"); + assert_eq!(warning, None); + } + + #[test] + fn the_flag_takes_precedence_over_the_variable_and_warns() { + let (password, warning) = choose_password(value("from-flag"), value("from-env")).unwrap(); + assert_eq!(password, "from-flag"); + assert_eq!(warning, Some(PASSWORD_FLAG_WARNING)); + } + + #[test] + fn no_flag_and_no_variable_is_an_error() { + let err = choose_password(None, None).unwrap_err(); + assert_eq!( + err.to_string(), + "No password provided. Use --password VALUE or set EXTENDDB_PASSWORD." + ); + } +} 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/technical-debt.md b/docs/technical-debt.md index 44d79f8e8..09ce9cb6e 100755 --- a/docs/technical-debt.md +++ b/docs/technical-debt.md @@ -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 | `app/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 From 7e642159e609690259b013b8874aaebaf53cc1b2 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Thu, 5 Nov 2026 04:47:10 +0000 Subject: [PATCH 2/3] fix(cli): choose the --password warning without the password CodeQL reported cleartext logging at the eprintln that prints the --password warning. choose_password returned the password and the warning in one tuple, so the analysis traced the printed warning to the password. The printed text was always the fixed warning. choose_password now returns the password only. A new function, flag_warning, returns the warning when the command line has --password and None otherwise, and resolve_password calls it with cli_password.is_some(). The flag still takes precedence, so the warning appears exactly when the password came from the flag, and the output does not change. The four choose_password tests stay without their warning checks, and a fifth test covers flag_warning. Signed-off-by: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> --- crates/app/src/cmd_manage.rs | 66 ++++++++++++++++++++---------------- 1 file changed, 37 insertions(+), 29 deletions(-) diff --git a/crates/app/src/cmd_manage.rs b/crates/app/src/cmd_manage.rs index a6d9c6fab..4c41fac3e 100755 --- a/crates/app/src/cmd_manage.rs +++ b/crates/app/src/cmd_manage.rs @@ -17,20 +17,19 @@ const PASSWORD_FLAG_WARNING: &str = "Warning: --password is visible in process l 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. The second element is -/// the warning to print on stderr: `Some` when the password came from the -/// flag, `None` when it came from the variable. -fn choose_password( - flag: Option, - env: Option, -) -> anyhow::Result<(String, Option<&'static str>)> { - match (flag, env) { - (Some(password), _) => Ok((password, Some(PASSWORD_FLAG_WARNING))), - (None, Some(password)) => Ok((password, None)), - (None, None) => Err(anyhow::anyhow!( - "No password provided. Use --password VALUE or set 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. +fn flag_warning(flag_given: bool) -> Option<&'static str> { + flag_given.then_some(PASSWORD_FLAG_WARNING) } /// Resolve the password from the `--password` flag or the `EXTENDDB_PASSWORD` @@ -38,8 +37,8 @@ fn choose_password( /// /// Never reads from stdin. Errors if no password source is available. fn resolve_password(cli_password: Option) -> anyhow::Result { - let (password, warning) = - choose_password(cli_password, std::env::var("EXTENDDB_PASSWORD").ok())?; + let warning = flag_warning(cli_password.is_some()); + let password = choose_password(cli_password, std::env::var("EXTENDDB_PASSWORD").ok())?; if let Some(warning) = warning { eprintln!("{warning}"); } @@ -79,31 +78,40 @@ pub async fn run(args: ManageArgs) -> anyhow::Result<()> { #[cfg(test)] mod tests { - use super::{PASSWORD_FLAG_WARNING, choose_password}; + use super::{PASSWORD_FLAG_WARNING, choose_password, flag_warning}; fn value(s: &str) -> Option { Some(s.to_owned()) } #[test] - fn a_password_from_the_flag_is_used_with_the_warning() { - let (password, warning) = choose_password(value("from-flag"), None).unwrap(); - assert_eq!(password, "from-flag"); - assert_eq!(warning, Some(PASSWORD_FLAG_WARNING)); + fn a_password_from_the_flag_is_used() { + assert_eq!( + choose_password(value("from-flag"), None).unwrap(), + "from-flag" + ); } #[test] - fn a_password_from_the_variable_is_used_without_a_warning() { - let (password, warning) = choose_password(None, value("from-env")).unwrap(); - assert_eq!(password, "from-env"); - assert_eq!(warning, None); + fn a_password_from_the_variable_is_used() { + assert_eq!( + choose_password(None, value("from-env")).unwrap(), + "from-env" + ); + } + + #[test] + fn the_flag_takes_precedence_over_the_variable() { + assert_eq!( + choose_password(value("from-flag"), value("from-env")).unwrap(), + "from-flag" + ); } #[test] - fn the_flag_takes_precedence_over_the_variable_and_warns() { - let (password, warning) = choose_password(value("from-flag"), value("from-env")).unwrap(); - assert_eq!(password, "from-flag"); - assert_eq!(warning, Some(PASSWORD_FLAG_WARNING)); + fn only_the_flag_gets_the_warning() { + assert_eq!(flag_warning(true), Some(PASSWORD_FLAG_WARNING)); + assert_eq!(flag_warning(false), None); } #[test] From 6c1b3aa416d52f407abf48eb563e8426ca943f88 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Mon, 9 Nov 2026 23:05:41 +0000 Subject: [PATCH 3/3] fix(cli): test the --password warning where it is printed The tests called choose_password and flag_warning, but not the function that prints the warning, so a change that removed the warning still passed them. resolve_password_with now takes the EXTENDDB_PASSWORD value and a writer for the warning, and resolve_password passes it the variable and stderr. The tests check what each password source writes. The flag writes the warning once, without the password. The variable writes nothing, and so does the error when neither is set. A failed write of the warning no longer stops the command, where eprintln! panicked. flag_warning gets a comment on why it exists. CodeQL's rust/cleartext-logging query traces printed text back to the password when both come from one function result. Two notes in docs/getting-started.md and one line of docs/manuals/10-security-model.md named EXTENDDB_ADMIN_PASSWORD for manage. Only init and the SQLite bootstrap read that variable. All three now name EXTENDDB_PASSWORD, the variable that manage reads. The S-2 row of docs/technical-debt.md gets its file path and a link to #916, and the file gets a new date. Signed-off-by: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> --- crates/app/src/cmd_manage.rs | 79 +++++++++++++++++++------------ docs/getting-started.md | 4 +- docs/manuals/10-security-model.md | 2 +- docs/technical-debt.md | 4 +- 4 files changed, 55 insertions(+), 34 deletions(-) diff --git a/crates/app/src/cmd_manage.rs b/crates/app/src/cmd_manage.rs index 4c41fac3e..271b688b4 100755 --- a/crates/app/src/cmd_manage.rs +++ b/crates/app/src/cmd_manage.rs @@ -9,6 +9,8 @@ //! Types are in `manage_types.rs`; HTTP transport and dispatch are in //! `manage_http.rs`. +use std::io::Write; + use crate::manage_http::{basic_auth, dispatch, resolve_endpoint}; pub use crate::manage_types::ManageArgs; @@ -28,23 +30,42 @@ fn choose_password(flag: Option, env: Option) -> anyhow::Result< /// 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` flag or the `EXTENDDB_PASSWORD` -/// env var, and warn on stderr when it came from the flag. +/// 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. -fn resolve_password(cli_password: Option) -> anyhow::Result { +/// 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, std::env::var("EXTENDDB_PASSWORD").ok())?; + let password = choose_password(cli_password, env_password)?; if let Some(warning) = warning { - eprintln!("{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)] @@ -78,48 +99,48 @@ pub async fn run(args: ManageArgs) -> anyhow::Result<()> { #[cfg(test)] mod tests { - use super::{PASSWORD_FLAG_WARNING, choose_password, flag_warning}; + use super::{PASSWORD_FLAG_WARNING, resolve_password_with}; fn value(s: &str) -> Option { Some(s.to_owned()) } - #[test] - fn a_password_from_the_flag_is_used() { - assert_eq!( - choose_password(value("from-flag"), None).unwrap(), - "from-flag" - ); + /// 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_variable_is_used() { - assert_eq!( - choose_password(None, value("from-env")).unwrap(), - "from-env" - ); + 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 the_flag_takes_precedence_over_the_variable() { - assert_eq!( - choose_password(value("from-flag"), value("from-env")).unwrap(), - "from-flag" - ); + 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 only_the_flag_gets_the_warning() { - assert_eq!(flag_warning(true), Some(PASSWORD_FLAG_WARNING)); - assert_eq!(flag_warning(false), None); + 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() { - let err = choose_password(None, None).unwrap_err(); + fn no_flag_and_no_variable_is_an_error_with_no_warning() { + let (password, warnings) = resolve(None, None); assert_eq!( - err.to_string(), + password.unwrap_err().to_string(), "No password provided. Use --password VALUE or set EXTENDDB_PASSWORD." ); + assert_eq!(warnings, ""); } } diff --git a/docs/getting-started.md b/docs/getting-started.md index 712f34cfe..d3ed527e0 100755 --- a/docs/getting-started.md +++ b/docs/getting-started.md @@ -796,7 +796,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`). For sensitive environments, use the `EXTENDDB_ADMIN_PASSWORD` environment variable instead. +**Note:** The `--password` flag is visible in process listings (`ps aux`) and shell history, and `extenddb manage` prints a warning when it gets the flag. Omit the flag and set the `EXTENDDB_PASSWORD` environment variable instead. ### List admin users @@ -928,7 +928,7 @@ Both `--secret-access-key` and `--yes` are required: --yes ``` -> **Security note:** `--secret-access-key` and `--password` are visible in `ps` output and shell history. For sensitive environments, use the `EXTENDDB_ADMIN_PASSWORD` environment variable for the password. +> **Security note:** `--secret-access-key` and `--password` are visible in `ps` output and shell history. For the password, omit `--password` and set the `EXTENDDB_PASSWORD` environment variable. Credential import is gated by the `allow_credential_import` runtime setting (default: `true`). To disable: diff --git a/docs/manuals/10-security-model.md b/docs/manuals/10-security-model.md index 8558c0681..46dedb8a3 100755 --- a/docs/manuals/10-security-model.md +++ b/docs/manuals/10-security-model.md @@ -309,7 +309,7 @@ Import/export file paths are validated: ### Credential Handling - `extenddb init` prints admin credentials once; they are not stored in plaintext -- The `--password` flag accepts passwords via environment variable (`EXTENDDB_ADMIN_PASSWORD`) to avoid process listing exposure +- `extenddb manage` reads the password from the `EXTENDDB_PASSWORD` environment variable when `--password` is omitted, which keeps it out of process listings, and it prints a warning on stderr when it gets the flag - 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 09ce9cb6e..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`) 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 | `app/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