From 9eaf66546ba7db9c77e7568137c1d9a895b0dab7 Mon Sep 17 00:00:00 2001 From: atlas Date: Sun, 9 Aug 2026 19:47:22 +0200 Subject: [PATCH 1/2] 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); From 462f353c107298850402e31cd9f5adcd20b88f42 Mon Sep 17 00:00:00 2001 From: atlas Date: Sun, 9 Aug 2026 19:53:56 +0200 Subject: [PATCH 2/2] fix(priv): scan for a secret-shaped RUN, not a whitespace-delimited word argus caught a false negative in the shape rule, and it sits exactly in the property the change is sold on -- "a new secret type is caught by default". `split_whitespace()` yields `","` for a token with punctuation glued to it, and the comma fails the alphabet check for the whole word, so the line passes unredacted with the credential in it. `[]`, `""`, `()` and a no-whitespace-at-all blob all defeat it the same way. Whitespace is not what delimits a secret; the alphabet is. So scan the line for a maximal run of >=32 alphabet characters and let punctuation reset the counter. Simpler than the version it replaces, and it closes the gap by construction rather than by enumerating the delimiters someone might glue on next. The existing tests all passed against the broken version because I wrote them from the same mental model that produced the bug -- every fixture had a space before the token. The new test carries the six shapes that used to slip through. --- hive-priv/src/main.rs | 79 +++++++++++++++++++++++++++++++++---------- 1 file changed, 61 insertions(+), 18 deletions(-) diff --git a/hive-priv/src/main.rs b/hive-priv/src/main.rs index 3cb86d13..2b819c7c 100644 --- a/hive-priv/src/main.rs +++ b/hive-priv/src/main.rs @@ -1724,21 +1724,37 @@ fn redact_secret_line(line: &str) -> std::borrow::Cow<'_, str> { if line.to_ascii_lowercase().contains("password") { return std::borrow::Cow::Borrowed("[redacted: line mentions a password]"); } - if line.split_whitespace().any(looks_like_secret) { + 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 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'-')) +/// 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 @@ -2522,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, - looks_like_secret, redact_secret_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}; @@ -2660,23 +2676,50 @@ mod tests { #[test] fn secret_shape_boundaries() { - // Ordinary forgejo-admin output survives: no word is long enough. + // 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!(!looks_like_secret(&"a".repeat(31))); - assert!(looks_like_secret(&"a".repeat(32))); + 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!(!looks_like_secret( + assert!(!contains_secret_shaped_run( "this.is.a.very.long.dotted.identifier.but.not.a.secret" )); // base64url and hex alphabets both count. - assert!(looks_like_secret( + assert!(contains_secret_shaped_run( "ZGVhZGJlZWZkZWFkYmVlZmRlYWRiZWVmZGVhZA==" )); - assert!(looks_like_secret("aG93-dy_there-aG93dy1theresomething")); + 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.