hive-priv: stop run_forge_admin's bail! from reproducing --password
The comment above the bail! named the gap itself: stderr went through
redact_secret_line, but args.join(" ") did not. hive-c0re passes a
live --password value as an argument on user create and
change-password, so any non-zero forgejo admin exit put the plaintext
password in the error string, and from there into hive-c0re's warn!
log (journal + VictoriaLogs) and hivectl's returned error.
Add describe_forge_admin, a pure function that names the invocation by
its leading verb path and stops at the first flag, same approach as
hive-c0re's own describe_forge_admin (forge/mod.rs, from #2936) and
for the same reason: the verbs are a closed set this crate chooses,
argument values never are, so an allowlist over shape excludes any
future secret-bearing flag by construction instead of by someone
remembering to redact its value.
Unit tests cover: a --password value dropped, the --password=value
form also dropped (it starts with '-', so the take_while excludes the
whole argument), and a control that the verb path still appears.
Closes #4670.
This commit is contained in:
parent
9986da4c7f
commit
fdb847cd87
1 changed files with 83 additions and 10 deletions
|
|
@ -2351,6 +2351,32 @@ fn contains_secret_shaped_run(line: &str) -> bool {
|
|||
false
|
||||
}
|
||||
|
||||
/// Name a `forgejo admin` invocation for an error message without
|
||||
/// reproducing its arguments: keep the leading verb path, stop at the
|
||||
/// first flag. `["user", "change-password", "--username", "iris",
|
||||
/// "--password", "…"]` becomes `forgejo admin user change-password`.
|
||||
///
|
||||
/// Same allowlist as hive-c0re's `describe_forge_admin`
|
||||
/// (`hive-c0re/src/forge/mod.rs`), for the same reason: the verbs are a
|
||||
/// closed set this crate chooses itself, but hive-c0re passes a live
|
||||
/// `--password` value as an argument on some calls, and argument values
|
||||
/// are never safe to assume closed. Redacting the value after
|
||||
/// `--password` instead would repeat the bug `redact_secret_line`'s own
|
||||
/// doc comment describes — a denylist of one flag name fails open the
|
||||
/// moment a differently named secret-bearing flag is added.
|
||||
fn describe_forge_admin(args: &[String]) -> String {
|
||||
let verbs: Vec<&str> = args
|
||||
.iter()
|
||||
.take_while(|a| !a.starts_with('-'))
|
||||
.map(String::as_str)
|
||||
.collect();
|
||||
if verbs.is_empty() {
|
||||
"forgejo admin".to_owned()
|
||||
} else {
|
||||
format!("forgejo admin {}", verbs.join(" "))
|
||||
}
|
||||
}
|
||||
|
||||
/// Run `forgejo admin <args>` inside the `hive-forge` container as the
|
||||
/// `forgejo` unix user. Requires root (for nsenter into the container's
|
||||
/// namespaces). Returns `(stdout, stderr)`.
|
||||
|
|
@ -2392,17 +2418,20 @@ async fn run_forge_admin(args: &[String]) -> Result<(String, String)> {
|
|||
if !out.status.success() {
|
||||
// Redact here too. The error string is propagated to the caller and
|
||||
// ends up logged; a partial-failure stderr can carry the same
|
||||
// material stdout would have. Redacting the log but not the error
|
||||
// is the same "two of three sites" gap that makes these leaks
|
||||
// survive a fix.
|
||||
// material stdout would have. And name the invocation via
|
||||
// `describe_forge_admin` rather than `args.join(" ")`: some callers
|
||||
// pass a live `--password` value as an argument, so reproducing the
|
||||
// raw vector here is the same "two of three sites" gap that makes
|
||||
// these leaks survive a fix — the log was redacted, but the error
|
||||
// was not.
|
||||
let safe_stderr: String = stderr
|
||||
.lines()
|
||||
.map(|l| redact_secret_line(l).into_owned())
|
||||
.collect::<Vec<_>>()
|
||||
.join("; ");
|
||||
bail!(
|
||||
"forgejo admin {} failed ({}): {}",
|
||||
args.join(" "),
|
||||
"{} failed ({}): {}",
|
||||
describe_forge_admin(args),
|
||||
out.status,
|
||||
safe_stderr.trim()
|
||||
);
|
||||
|
|
@ -3243,11 +3272,11 @@ mod tests {
|
|||
use super::{
|
||||
AgentTmpfilesEntry, BindMount, OwnedFd, PAUSED_MARKER_FILE, PrivRequest,
|
||||
agent_tmpfiles_content, check_fd_agreement, clear_runner_credentials,
|
||||
contains_secret_shaped_run, ensure_plain_filename, git_overlay_flags, limits_dropin_body,
|
||||
matrix_token_filename, open_export_dest, partial_name, redact_secret_line,
|
||||
remove_marker_in, single_output_path, toplevel_attr, validate_account_name,
|
||||
validate_credential_name, validate_snapshot_name, write_agent_dir_file,
|
||||
write_bridge_dns_marker_in, write_state_file_nofollow,
|
||||
contains_secret_shaped_run, describe_forge_admin, ensure_plain_filename, git_overlay_flags,
|
||||
limits_dropin_body, matrix_token_filename, open_export_dest, partial_name,
|
||||
redact_secret_line, remove_marker_in, single_output_path, toplevel_attr,
|
||||
validate_account_name, validate_credential_name, validate_snapshot_name,
|
||||
write_agent_dir_file, write_bridge_dns_marker_in, write_state_file_nofollow,
|
||||
};
|
||||
use std::path::PathBuf;
|
||||
use std::sync::atomic::{AtomicU32, Ordering};
|
||||
|
|
@ -3527,6 +3556,50 @@ mod tests {
|
|||
);
|
||||
}
|
||||
|
||||
/// `run_forge_admin`'s `bail!` names the invocation through this
|
||||
/// function instead of `args.join(" ")` — the regression this test
|
||||
/// guards. A `--password` value never survives to the error string.
|
||||
#[test]
|
||||
fn describe_forge_admin_does_not_reproduce_a_password_argument() {
|
||||
let described = describe_forge_admin(&[
|
||||
"user".to_owned(),
|
||||
"create".to_owned(),
|
||||
"--username".to_owned(),
|
||||
"x".to_owned(),
|
||||
"--password".to_owned(),
|
||||
"S3cret".to_owned(),
|
||||
]);
|
||||
assert_eq!(described, "forgejo admin user create");
|
||||
assert!(!described.contains("S3cret"), "{described}");
|
||||
}
|
||||
|
||||
/// The `--password=value` single-argument form is also excluded: it
|
||||
/// starts with `-`, so it never enters the kept verb path.
|
||||
#[test]
|
||||
fn describe_forge_admin_redacts_the_equals_sign_form_too() {
|
||||
let described = describe_forge_admin(&[
|
||||
"user".to_owned(),
|
||||
"create".to_owned(),
|
||||
"--password=S3cret".to_owned(),
|
||||
]);
|
||||
assert!(!described.contains("S3cret"), "{described}");
|
||||
}
|
||||
|
||||
/// Control: a non-secret argument (the verb path) still appears, so
|
||||
/// the message stays useful for diagnosing which call failed.
|
||||
#[test]
|
||||
fn describe_forge_admin_keeps_the_verb_path() {
|
||||
assert_eq!(
|
||||
describe_forge_admin(&["user".to_owned(), "change-password".to_owned()]),
|
||||
"forgejo admin user change-password"
|
||||
);
|
||||
assert_eq!(
|
||||
describe_forge_admin(&["--help".to_owned()]),
|
||||
"forgejo admin"
|
||||
);
|
||||
assert_eq!(describe_forge_admin(&[]), "forgejo admin");
|
||||
}
|
||||
|
||||
/// The regression this function exists for. The keyword rule passes
|
||||
/// this line straight through — it says nothing about a password — so
|
||||
/// only the shape rule catches it.
|
||||
|
|
|
|||
Loading…
Reference in a new issue