tests: cover the three security boundaries that had none (#3950)
- hive-c0re::webhook_secret::verify_signature — the HMAC comparison
verify_hmac (the only gate on the public webhook endpoint) delegates
to. Correct-signature and mismatched-signature (wrong secret, tampered
body) cases.
- hive-forge credential-helper get (host= check) — subprocess
integration tests since the check is inlined in run(), which reads
real stdin/env and prints real stdout. Host mismatch (error, token
withheld), host match (credentials printed), and no host= line
(backwards compat) cases.
- hive-priv::{validate_credential_name, validate_snapshot_name,
ensure_plain_filename} — the only gate on the root-privileged socket.
Empty/charset/dot/slash rejection, hive- prefix requirement, and
./../slash rejection respectively.
This commit is contained in:
parent
549156e55f
commit
e0673b6192
3 changed files with 195 additions and 3 deletions
|
|
@ -97,3 +97,41 @@ fn hex_decode(s: &str) -> Option<Vec<u8>> {
|
||||||
}
|
}
|
||||||
Some(out)
|
Some(out)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[cfg(test)]
|
||||||
|
mod tests {
|
||||||
|
use super::{hex_encode, verify_signature};
|
||||||
|
|
||||||
|
/// Compute the `sha256=<hex>` header Forgejo would send for `secret` +
|
||||||
|
/// `body`, so the "matches" test below isn't asserting against a
|
||||||
|
/// hand-picked string that happens to look like a signature.
|
||||||
|
fn sign(secret: &str, body: &[u8]) -> String {
|
||||||
|
use hmac::{Hmac, KeyInit, Mac};
|
||||||
|
use sha2::Sha256;
|
||||||
|
let mut mac = Hmac::<Sha256>::new_from_slice(secret.as_bytes()).unwrap();
|
||||||
|
mac.update(body);
|
||||||
|
format!("sha256={}", hex_encode(&mac.finalize().into_bytes()))
|
||||||
|
}
|
||||||
|
|
||||||
|
/// `verify_hmac` (`dashboard/webhook.rs`) delegates the actual HMAC
|
||||||
|
/// comparison here — this is the only gate on an inbound Forgejo
|
||||||
|
/// webhook reachable via the public gateway. A signature computed
|
||||||
|
/// with the right secret over the right body must verify.
|
||||||
|
#[test]
|
||||||
|
fn verify_signature_accepts_the_correct_hmac() {
|
||||||
|
let header = sign("s3cr3t", b"payload");
|
||||||
|
assert!(verify_signature("s3cr3t", b"payload", &header).is_ok());
|
||||||
|
}
|
||||||
|
|
||||||
|
/// If `mac.verify_slice`'s result were ever inverted (accept on
|
||||||
|
/// mismatch), an unauthenticated actor could queue an operator
|
||||||
|
/// approval through the public webhook endpoint. Cover both ways a
|
||||||
|
/// signature can stop matching: wrong secret, and a body that no
|
||||||
|
/// longer matches the one that was signed.
|
||||||
|
#[test]
|
||||||
|
fn verify_signature_rejects_a_mismatched_hmac() {
|
||||||
|
let header = sign("s3cr3t", b"payload");
|
||||||
|
assert!(verify_signature("different-secret", b"payload", &header).is_err());
|
||||||
|
assert!(verify_signature("s3cr3t", b"tampered-payload", &header).is_err());
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
|
||||||
113
hive-forge/tests/credential_helper.rs
Normal file
113
hive-forge/tests/credential_helper.rs
Normal file
|
|
@ -0,0 +1,113 @@
|
||||||
|
//! Integration coverage for `hive-forge credential-helper get` — the
|
||||||
|
//! `host=` check in `verbs::credential_helper::run` exists to close a
|
||||||
|
//! reported credential leak, and git invokes the helper fresh over
|
||||||
|
//! stdin/env on every fetch/push. The check is inlined in `run()`
|
||||||
|
//! (real stdin, real env, real stdout), so it can only be driven the way
|
||||||
|
//! git actually drives it: run the compiled binary.
|
||||||
|
|
||||||
|
use std::io::Write as _;
|
||||||
|
use std::path::{Path, PathBuf};
|
||||||
|
use std::process::{Command, Stdio};
|
||||||
|
|
||||||
|
const TOKEN: &str = "test-token-value";
|
||||||
|
|
||||||
|
/// A unique scratch `HYPERHIVE_STATE_DIR`, seeded with a `forge-token`
|
||||||
|
/// file. Matches the manual-tmpdir pattern `hive-c0re`'s
|
||||||
|
/// `dashboard::state_files` tests use — this crate carries no `tempfile`
|
||||||
|
/// dependency to reach for instead.
|
||||||
|
fn scratch_state_dir(tag: &str) -> PathBuf {
|
||||||
|
let ts = std::time::SystemTime::now()
|
||||||
|
.duration_since(std::time::UNIX_EPOCH)
|
||||||
|
.map_or(0, |d| d.as_nanos());
|
||||||
|
let dir = std::env::temp_dir().join(format!("hive-forge-cred-test-{tag}-{ts}"));
|
||||||
|
std::fs::create_dir_all(&dir).expect("create scratch state dir");
|
||||||
|
std::fs::write(dir.join("forge-token"), format!("{TOKEN}\n")).expect("seed forge-token");
|
||||||
|
dir
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Run `hive-forge credential-helper get` against `base_url` +
|
||||||
|
/// `state_dir`, feeding `request` as the git credential-protocol
|
||||||
|
/// request body on stdin. Returns `(exit success, stdout, stderr)`.
|
||||||
|
fn run_get(base_url: &str, state_dir: &Path, request: &str) -> (bool, String, String) {
|
||||||
|
let mut child = Command::new(env!("CARGO_BIN_EXE_hive-forge"))
|
||||||
|
.arg("credential-helper")
|
||||||
|
.arg("get")
|
||||||
|
.env("HIVE_FORGE_URL", base_url)
|
||||||
|
.env("HYPERHIVE_STATE_DIR", state_dir)
|
||||||
|
.env_remove("HIVE_LABEL")
|
||||||
|
.stdin(Stdio::piped())
|
||||||
|
.stdout(Stdio::piped())
|
||||||
|
.stderr(Stdio::piped())
|
||||||
|
.spawn()
|
||||||
|
.expect("spawn hive-forge credential-helper get");
|
||||||
|
child
|
||||||
|
.stdin
|
||||||
|
.take()
|
||||||
|
.expect("piped stdin")
|
||||||
|
.write_all(request.as_bytes())
|
||||||
|
.expect("write credential request to stdin");
|
||||||
|
let out = child.wait_with_output().expect("wait for hive-forge");
|
||||||
|
(
|
||||||
|
out.status.success(),
|
||||||
|
String::from_utf8_lossy(&out.stdout).into_owned(),
|
||||||
|
String::from_utf8_lossy(&out.stderr).into_owned(),
|
||||||
|
)
|
||||||
|
}
|
||||||
|
|
||||||
|
/// This is the check the module doc says exists to close a reported
|
||||||
|
/// credential leak: refusing to hand the forge token to a host git
|
||||||
|
/// didn't ask this invocation to serve. If the host comparison were ever
|
||||||
|
/// inverted, a mismatched host would get the token instead of being
|
||||||
|
/// refused.
|
||||||
|
#[test]
|
||||||
|
fn credential_helper_get_refuses_a_mismatched_host_and_withholds_the_token() {
|
||||||
|
let dir = scratch_state_dir("mismatch");
|
||||||
|
let (ok, stdout, stderr) = run_get(
|
||||||
|
"http://forge.internal.example",
|
||||||
|
&dir,
|
||||||
|
"protocol=https\nhost=not-the-forge.example\n",
|
||||||
|
);
|
||||||
|
assert!(!ok, "a mismatched host= must fail, stderr={stderr}");
|
||||||
|
assert!(
|
||||||
|
!stdout.contains(TOKEN),
|
||||||
|
"the token must never reach stdout on a host mismatch: stdout={stdout}"
|
||||||
|
);
|
||||||
|
let _ = std::fs::remove_dir_all(&dir);
|
||||||
|
}
|
||||||
|
|
||||||
|
/// The other side of the same check: a `host=` that matches the
|
||||||
|
/// configured forge must still print the credentials, or every real git
|
||||||
|
/// fetch/push would break.
|
||||||
|
#[test]
|
||||||
|
fn credential_helper_get_answers_when_host_matches() {
|
||||||
|
let dir = scratch_state_dir("match");
|
||||||
|
let (ok, stdout, stderr) = run_get(
|
||||||
|
"http://forge.internal.example",
|
||||||
|
&dir,
|
||||||
|
"protocol=https\nhost=forge.internal.example\n",
|
||||||
|
);
|
||||||
|
assert!(ok, "a matching host= must succeed, stderr={stderr}");
|
||||||
|
assert!(stdout.contains("username="), "stdout={stdout}");
|
||||||
|
assert!(
|
||||||
|
stdout.contains(&format!("password={TOKEN}")),
|
||||||
|
"stdout={stdout}"
|
||||||
|
);
|
||||||
|
let _ = std::fs::remove_dir_all(&dir);
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Backwards compatibility: the request shape git sent before this check
|
||||||
|
/// existed — no `host=` line at all — must still work.
|
||||||
|
#[test]
|
||||||
|
fn credential_helper_get_still_works_with_no_host_line() {
|
||||||
|
let dir = scratch_state_dir("nohost");
|
||||||
|
let (ok, stdout, stderr) = run_get("http://forge.internal.example", &dir, "protocol=https\n");
|
||||||
|
assert!(
|
||||||
|
ok,
|
||||||
|
"a request with no host= line must not be treated as a failure, stderr={stderr}"
|
||||||
|
);
|
||||||
|
assert!(
|
||||||
|
stdout.contains(&format!("password={TOKEN}")),
|
||||||
|
"stdout={stdout}"
|
||||||
|
);
|
||||||
|
let _ = std::fs::remove_dir_all(&dir);
|
||||||
|
}
|
||||||
|
|
@ -3114,9 +3114,10 @@ mod tests {
|
||||||
use super::{
|
use super::{
|
||||||
AgentTmpfilesEntry, BindMount, OwnedFd, PAUSED_MARKER_FILE, PrivRequest,
|
AgentTmpfilesEntry, BindMount, OwnedFd, PAUSED_MARKER_FILE, PrivRequest,
|
||||||
agent_tmpfiles_content, check_fd_agreement, clear_runner_credentials,
|
agent_tmpfiles_content, check_fd_agreement, clear_runner_credentials,
|
||||||
contains_secret_shaped_run, git_overlay_flags, limits_dropin_body, matrix_token_filename,
|
contains_secret_shaped_run, ensure_plain_filename, git_overlay_flags, limits_dropin_body,
|
||||||
partial_name, redact_secret_line, remove_marker_in, single_output_path, toplevel_attr,
|
matrix_token_filename, partial_name, redact_secret_line, remove_marker_in,
|
||||||
validate_account_name, write_agent_dir_file, write_state_file_nofollow,
|
single_output_path, toplevel_attr, validate_account_name, validate_credential_name,
|
||||||
|
validate_snapshot_name, write_agent_dir_file, write_state_file_nofollow,
|
||||||
};
|
};
|
||||||
use std::path::PathBuf;
|
use std::path::PathBuf;
|
||||||
use std::sync::atomic::{AtomicU32, Ordering};
|
use std::sync::atomic::{AtomicU32, Ordering};
|
||||||
|
|
@ -3735,4 +3736,44 @@ mod tests {
|
||||||
);
|
);
|
||||||
std::fs::remove_dir_all(&dir).ok();
|
std::fs::remove_dir_all(&dir).ok();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// The only gate on the root-privileged socket for a `--load-credential`
|
||||||
|
/// name. The doc comment is explicit that `.` is excluded on purpose
|
||||||
|
/// ("this name gets interpolated into filesystem paths") — a future
|
||||||
|
/// "let's allow dots for version numbers" loosening must fail this,
|
||||||
|
/// not just the `/` case below.
|
||||||
|
#[test]
|
||||||
|
fn validate_credential_name_rejects_dot_slash_and_empty_but_allows_the_charset() {
|
||||||
|
assert!(
|
||||||
|
validate_credential_name("").is_err(),
|
||||||
|
"empty must be rejected"
|
||||||
|
);
|
||||||
|
assert!(validate_credential_name("valid-name_123").is_ok());
|
||||||
|
assert!(
|
||||||
|
validate_credential_name("bad.name").is_err(),
|
||||||
|
"dot must be rejected — see the fn's doc comment"
|
||||||
|
);
|
||||||
|
assert!(validate_credential_name("bad/name").is_err());
|
||||||
|
}
|
||||||
|
|
||||||
|
/// A snapshot label must additionally carry the `hive-` prefix (it
|
||||||
|
/// doubles as the allow-list gating the btrfs snapshot/delete
|
||||||
|
/// shellouts) on top of [`validate_credential_name`]'s charset rule.
|
||||||
|
#[test]
|
||||||
|
fn validate_snapshot_name_requires_hive_prefix() {
|
||||||
|
assert!(validate_snapshot_name("not-hive-prefixed").is_err());
|
||||||
|
assert!(validate_snapshot_name("hive-valid").is_ok());
|
||||||
|
}
|
||||||
|
|
||||||
|
/// `ensure_plain_filename` is the gate between an agent-writable
|
||||||
|
/// directory and a root-privileged file write. `.`/`..` would resolve
|
||||||
|
/// to the directory itself or its parent; any `/` climbs into a path
|
||||||
|
/// component the caller never named.
|
||||||
|
#[test]
|
||||||
|
fn ensure_plain_filename_rejects_dot_dotdot_and_any_slash() {
|
||||||
|
assert!(ensure_plain_filename("test", ".").is_err());
|
||||||
|
assert!(ensure_plain_filename("test", "..").is_err());
|
||||||
|
assert!(ensure_plain_filename("test", "sub/dir").is_err());
|
||||||
|
assert!(ensure_plain_filename("test", "matrix-token").is_ok());
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue