diff --git a/hive-priv/src/main.rs b/hive-priv/src/main.rs index fb9b8ee7..2b819c7c 100644 --- a/hive-priv/src/main.rs +++ b/hive-priv/src/main.rs @@ -1700,18 +1700,61 @@ fn validate_forge_admin_arg(arg: &str) -> Result<()> { } /// Redact a line before it hits the (root-readable, but still -/// unnecessarily exposed) host journal. `forgejo admin user create -/// --random-password` prints the generated password straight to stdout — -/// case-insensitive substring match on "password" is deliberately broad -/// (not pinned to forgejo's exact wording, which can change across -/// versions) so any password-bearing line gets caught rather than relying -/// on a phrase that could silently drift out of sync. -fn redact_password_line(line: &str) -> std::borrow::Cow<'_, str> { +/// unnecessarily exposed) host journal. +/// +/// Two independent rules, because the previous single rule failed open. +/// It matched only the substring "password", chosen to be robust against +/// forgejo *rewording* its password line — and the leak arrived from the +/// other axis entirely: a **different kind of secret** on a differently +/// worded line. `forgejo admin user generate-access-token` prints +/// `Access token was successfully created: <40 hex>`, which contains no +/// "password" and went to the journal verbatim for every agent ever +/// provisioned. +/// +/// So the second rule matches on **shape, not vocabulary**: a long +/// unbroken run of secret-alphabet characters. A new secret type is then +/// caught by default rather than by someone remembering to add a keyword. +/// +/// ⚠️ This deliberately over-matches. A nix store hash is also a long +/// opaque run and will redact its line. That is the correct direction to +/// be wrong in: the cost of a false positive is one less log line, and +/// the cost of a false negative is a live credential in a journal that +/// any `read_host_journal` holder can read. +fn redact_secret_line(line: &str) -> std::borrow::Cow<'_, str> { if line.to_ascii_lowercase().contains("password") { - std::borrow::Cow::Borrowed("[redacted: line mentions a password]") - } else { - std::borrow::Cow::Borrowed(line) + return std::borrow::Cow::Borrowed("[redacted: line mentions a password]"); } + if contains_secret_shaped_run(line) { + return std::borrow::Cow::Borrowed("[redacted: line contains a secret-shaped token]"); + } + std::borrow::Cow::Borrowed(line) +} + +/// True when the line contains an unbroken run of at least 32 characters +/// from the hex / base64url alphabet. 32 sits below forgejo's 40-hex +/// access token and above the ordinary words and path segments that +/// appear in `forgejo admin` output. +/// +/// ⚠️ Scans for a RUN, not for a whitespace-delimited word. An earlier +/// version split on whitespace and required the whole word to match, +/// which a secret with punctuation glued to it defeats: `","` and +/// `"[]"` both fail an all-chars check on the word while still +/// containing the credential in full. Whitespace is not what delimits a +/// secret — the alphabet is (thanks @argus for catching it). +fn contains_secret_shaped_run(line: &str) -> bool { + const MIN: usize = 32; + let mut run = 0usize; + for b in line.bytes() { + if b.is_ascii_alphanumeric() || matches!(b, b'+' | b'/' | b'=' | b'_' | b'-') { + run += 1; + if run >= MIN { + return true; + } + } else { + run = 0; + } + } + false } /// Run `forgejo admin ` inside the `hive-forge` container as the @@ -1741,18 +1784,33 @@ async fn run_forge_admin(args: &[String]) -> Result<(String, String)> { .context("invoke nixos-container run hive-forge -- forgejo admin")?; let stdout = String::from_utf8_lossy(&out.stdout).into_owned(); let stderr = String::from_utf8_lossy(&out.stderr).into_owned(); + // stdout at DEBUG, not INFO: on the success path this stream carries + // the *product* of the command (the freshly minted token, the created + // user's details) and nothing an operator needs at default verbosity. + // Redaction stays on as the second layer — the level decides who sees + // it, the redactor decides what it says, and neither alone is enough. for line in stdout.lines() { - tracing::info!(target: "forgejo-admin", "{}", redact_password_line(line)); + tracing::debug!(target: "forgejo-admin", "{}", redact_secret_line(line)); } for line in stderr.lines() { - tracing::warn!(target: "forgejo-admin", "{}", redact_password_line(line)); + tracing::warn!(target: "forgejo-admin", "{}", redact_secret_line(line)); } 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. + let safe_stderr: String = stderr + .lines() + .map(|l| redact_secret_line(l).into_owned()) + .collect::>() + .join("; "); bail!( "forgejo admin {} failed ({}): {}", args.join(" "), out.status, - stderr.trim() + safe_stderr.trim() ); } Ok((stdout, stderr)) @@ -2480,8 +2538,8 @@ async fn sync_agent_tmpfiles(agents: &[AgentTmpfilesEntry]) -> Result<(String, S #[cfg(test)] mod tests { use super::{ - OwnedFd, PAUSED_MARKER_FILE, PrivRequest, check_fd_agreement, limits_dropin_body, - redact_password_line, remove_marker_in, write_state_file_nofollow, + OwnedFd, PAUSED_MARKER_FILE, PrivRequest, check_fd_agreement, contains_secret_shaped_run, + limits_dropin_body, redact_secret_line, remove_marker_in, write_state_file_nofollow, }; use std::path::PathBuf; use std::sync::atomic::{AtomicU32, Ordering}; @@ -2586,19 +2644,84 @@ mod tests { #[test] fn redacts_lines_mentioning_password_case_insensitively() { assert_eq!( - redact_password_line("New password: hunter2"), + redact_secret_line("New password: hunter2"), "[redacted: line mentions a password]" ); assert_eq!( - redact_password_line("PASSWORD=hunter2"), + redact_secret_line("PASSWORD=hunter2"), "[redacted: line mentions a password]" ); assert_eq!( - redact_password_line("User \"foo\" was successfully created."), + redact_secret_line("User \"foo\" was successfully created."), "User \"foo\" was successfully created." ); } + /// 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. + #[test] + fn redacts_access_token_line_which_mentions_no_password() { + let line = + "Access token was successfully created: 0123456789abcdef0123456789abcdef01234567"; + assert!( + !line.to_ascii_lowercase().contains("password"), + "fixture must not contain the keyword, or it proves nothing" + ); + assert_eq!( + redact_secret_line(line), + "[redacted: line contains a secret-shaped token]" + ); + } + + #[test] + fn secret_shape_boundaries() { + // Ordinary forgejo-admin output survives: no run is long enough. + assert_eq!( + redact_secret_line("Command 'user' 'create' finished with no errors."), + "Command 'user' 'create' finished with no errors." + ); + // 31 chars is below the floor, 32 is at it. + assert!(!contains_secret_shaped_run(&"a".repeat(31))); + assert!(contains_secret_shaped_run(&"a".repeat(32))); + // Punctuation breaks the run — a sentence never trips it however long. + assert!(!contains_secret_shaped_run( + "this.is.a.very.long.dotted.identifier.but.not.a.secret" + )); + // base64url and hex alphabets both count. + assert!(contains_secret_shaped_run( + "ZGVhZGJlZWZkZWFkYmVlZmRlYWRiZWVmZGVhZA==" + )); + assert!(contains_secret_shaped_run( + "aG93-dy_there-aG93dy1theresomething" + )); + } + + /// argus on the review: a whitespace-delimited check is defeated by + /// punctuation glued to the secret — the punctuation joins the "word" + /// and fails the alphabet test for the whole run, while the credential + /// sits there in full. Scanning for a RUN rather than a WORD closes it. + /// These are the shapes that used to slip through. + #[test] + fn secret_is_caught_with_punctuation_glued_to_it() { + const TOK: &str = "0123456789abcdef0123456789abcdef01234567"; + for line in [ + format!("token: {TOK},"), + format!("token: {TOK}."), + format!("using [{TOK}] now"), + format!("value=\"{TOK}\""), + format!("(created {TOK})"), + // no whitespace anywhere -- one glued blob + format!("Bearer:{TOK};next"), + ] { + assert_eq!( + redact_secret_line(&line), + "[redacted: line contains a secret-shaped token]", + "leaked through: {line}" + ); + } + } + /// Unique scratch dir per test, no external tempfile dep. fn scratch() -> PathBuf { static CTR: AtomicU32 = AtomicU32::new(0);