diff --git a/hive-priv/src/main.rs b/hive-priv/src/main.rs index 2b819c7c..fb9b8ee7 100644 --- a/hive-priv/src/main.rs +++ b/hive-priv/src/main.rs @@ -1700,61 +1700,18 @@ fn validate_forge_admin_arg(arg: &str) -> Result<()> { } /// Redact a line before it hits the (root-readable, but still -/// 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> { +/// 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> { if line.to_ascii_lowercase().contains("password") { - return std::borrow::Cow::Borrowed("[redacted: line mentions a password]"); + std::borrow::Cow::Borrowed("[redacted: line mentions a password]") + } else { + std::borrow::Cow::Borrowed(line) } - 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 @@ -1784,33 +1741,18 @@ 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::debug!(target: "forgejo-admin", "{}", redact_secret_line(line)); + tracing::info!(target: "forgejo-admin", "{}", redact_password_line(line)); } for line in stderr.lines() { - tracing::warn!(target: "forgejo-admin", "{}", redact_secret_line(line)); + tracing::warn!(target: "forgejo-admin", "{}", redact_password_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, - safe_stderr.trim() + stderr.trim() ); } Ok((stdout, stderr)) @@ -2538,8 +2480,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, contains_secret_shaped_run, - limits_dropin_body, redact_secret_line, remove_marker_in, write_state_file_nofollow, + OwnedFd, PAUSED_MARKER_FILE, PrivRequest, check_fd_agreement, limits_dropin_body, + redact_password_line, remove_marker_in, write_state_file_nofollow, }; use std::path::PathBuf; use std::sync::atomic::{AtomicU32, Ordering}; @@ -2644,84 +2586,19 @@ mod tests { #[test] fn redacts_lines_mentioning_password_case_insensitively() { assert_eq!( - redact_secret_line("New password: hunter2"), + redact_password_line("New password: hunter2"), "[redacted: line mentions a password]" ); assert_eq!( - redact_secret_line("PASSWORD=hunter2"), + redact_password_line("PASSWORD=hunter2"), "[redacted: line mentions a password]" ); assert_eq!( - redact_secret_line("User \"foo\" was successfully created."), + redact_password_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);