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.
This commit is contained in:
parent
22f8805a77
commit
9eaf66546b
1 changed files with 97 additions and 17 deletions
|
|
@ -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 <args>` 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::<Vec<_>>()
|
||||
.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);
|
||||
|
|
|
|||
Loading…
Reference in a new issue