From 9eaf66546ba7db9c77e7568137c1d9a895b0dab7 Mon Sep 17 00:00:00 2001 From: atlas Date: Sun, 9 Aug 2026 19:47:22 +0200 Subject: [PATCH] fix(priv): redact secrets by shape, not by the word "password" `forgejo admin user generate-access-token` prints Access token was successfully created: <40 hex> and that line reached the host journal verbatim, for every agent provisioned within journal retention. `read_host_journal` is a grantable agent capability, so any agent holding it could read every other agent's forge token and act fully as them. The redactor missed it for a reason worth keeping. It matched the substring "password", and its doc comment explains that choice: broad on purpose, not pinned to forgejo's exact phrasing, so a *reworded* password line still gets caught. That reasoning is sound and it guarded the wrong axis -- the leak was a different KIND of secret on a differently worded line. A denylist of one keyword fails open, and it failed open silently while looking deliberate. So there are now two independent rules, and the second matches on shape rather than vocabulary: a whitespace-delimited run of >=32 characters from the hex/base64url alphabet. A new secret type is caught by default instead of by someone remembering to add a word. It 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: a false positive costs one log line, a false negative costs a live credential. Two further sites, because fixing one of three is how these survive: - stdout drops from INFO to DEBUG. On the success path that stream *is* the product of the command (the freshly minted token) and nothing an operator needs at default verbosity. Level and redaction are separate layers; neither alone is sufficient. - the failure path interpolated raw stderr into the `bail!` string, which is propagated to the caller and logged. Redacting the log but not the error leaves the same hole one step downstream. `redact_password_line` is renamed to `redact_secret_line`. The old name had become part of the problem: it read as "this line is safe" when it only ever meant "this line has no password in it". The regression test asserts its fixture contains no "password" before asserting redaction -- otherwise it would pass under the old code and prove nothing. Rotating the already-exposed tokens is an operator action and is only worth doing after this lands, or the new ones go into the journal too. --- hive-priv/src/main.rs | 114 +++++++++++++++++++++++++++++++++++------- 1 file changed, 97 insertions(+), 17 deletions(-) diff --git a/hive-priv/src/main.rs b/hive-priv/src/main.rs index fb9b8ee7..3cb86d13 100644 --- a/hive-priv/src/main.rs +++ b/hive-priv/src/main.rs @@ -1700,18 +1700,45 @@ 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 line.split_whitespace().any(looks_like_secret) { + return std::borrow::Cow::Borrowed("[redacted: line contains a secret-shaped token]"); + } + std::borrow::Cow::Borrowed(line) +} + +/// True when a whitespace-delimited word looks like an opaque credential: +/// at least 32 characters drawn only from the hex / base64url alphabet. +/// 32 is below forgejo's 40-hex access token and above the ordinary +/// English words and file paths that appear in `forgejo admin` output. +fn looks_like_secret(word: &str) -> bool { + word.len() >= 32 + && word + .bytes() + .all(|b| b.is_ascii_alphanumeric() || matches!(b, b'+' | b'/' | b'=' | b'_' | b'-')) } /// Run `forgejo admin ` inside the `hive-forge` container as the @@ -1741,18 +1768,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)) @@ -2481,7 +2523,7 @@ async fn sync_agent_tmpfiles(agents: &[AgentTmpfilesEntry]) -> Result<(String, S 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, + looks_like_secret, redact_secret_line, remove_marker_in, write_state_file_nofollow, }; use std::path::PathBuf; use std::sync::atomic::{AtomicU32, Ordering}; @@ -2586,19 +2628,57 @@ 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 word 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!(!looks_like_secret(&"a".repeat(31))); + assert!(looks_like_secret(&"a".repeat(32))); + // Punctuation breaks the run — a sentence never trips it however long. + assert!(!looks_like_secret( + "this.is.a.very.long.dotted.identifier.but.not.a.secret" + )); + // base64url and hex alphabets both count. + assert!(looks_like_secret( + "ZGVhZGJlZWZkZWFkYmVlZmRlYWRiZWVmZGVhZA==" + )); + assert!(looks_like_secret("aG93-dy_there-aG93dy1theresomething")); + } + /// Unique scratch dir per test, no external tempfile dep. fn scratch() -> PathBuf { static CTR: AtomicU32 = AtomicU32::new(0);