diff --git a/docs/GITHUB.md b/docs/GITHUB.md index 9957f38..c0b5c0d 100644 --- a/docs/GITHUB.md +++ b/docs/GITHUB.md @@ -23,9 +23,10 @@ openpages login # wraps `gh auth login` with the right scopes ``` Everything after that reads the token with `gh auth token`, per operation. The -token is never written to the openpages database. It is passed to a build only -as part of a single `git clone` URL, and every log line is redacted against it -before being stored or shown. +token is never written to the openpages database or to disk. git receives it +as an `http.extraheader` for github.com in its environment, so it never lands in +`.git/config` inside the workspace that the build container mounts, and every +log line is redacted against it before being stored or shown. To switch accounts: @@ -91,6 +92,6 @@ anything that could remove code. | List repositories | `gh repo list --source --json …` | | Default branch, branch head | `gh api repos/{repo}` | | Add / remove a push webhook | `gh api repos/{repo}/hooks` | -| Clone for a build | `git clone` with a token in the URL | +| Clone for a build | `git fetch` with the token as an HTTP header (environment, not URL) | | Open a repair pull request | `gh pr create` | | Check for an openpages release | `gh api repos/{repo}/releases/latest` | diff --git a/docs/NODES.md b/docs/NODES.md index 72e7379..4d6401f 100644 --- a/docs/NODES.md +++ b/docs/NODES.md @@ -43,8 +43,12 @@ x-openpages-target: static:blog/412 x-openpages-target: container:38211 ``` -The node has no database, so it is told rather than asked to look up. A request -to a node's edge without that header is refused — it is not a general proxy. +The node has no database, so it is told rather than asked to look up. Each +request also carries `x-openpages-node-key`, the SHA-256 of the node's secret +that the panel stored at enrolment; the node recomputes it from its own token. +A request without both headers is refused — it is not a general proxy. The +panel never forwards a visitor's own copies of these headers. Panel and node +must be upgraded together: a node with this check refuses an older panel. Give `--edge-url` an address the panel can reach: a LAN address, a tailnet address, a private VPC address. If the node is genuinely unreachable, keep its diff --git a/src/api.rs b/src/api.rs index 6345b0b..7f7e812 100644 --- a/src/api.rs +++ b/src/api.rs @@ -872,7 +872,12 @@ async fn webhook( let push: PushEvent = serde_json::from_str(&body).map_err(|e| bad(format!("unreadable push event: {e}")))?; - let branch = push.git_ref.rsplit('/').next().unwrap_or("").to_string(); + // Only branch pushes deploy. `rsplit('/')` used to turn `refs/heads/fix/main` + // into `main` (a production deploy of a feature branch) and tags into + // branches. + let Some(branch) = push.git_ref.strip_prefix("refs/heads/").map(str::to_string) else { + return Ok(Json(json!({ "ok": true, "ignored": push.git_ref }))); + }; let mut started = Vec::new(); for site in db::site_by_repo(&state.db, &push.repository.full_name).await? { @@ -1004,7 +1009,11 @@ async fn node_logs( headers: HeaderMap, Json(b): Json, ) -> ApiResult> { - node_auth(&state, &headers).await?; + let node = node_auth(&state, &headers).await?; + let d = db::deployment(&state.db, b.deployment_id) + .await? + .ok_or_else(|| missing("deployment"))?; + claimed_by(&d, &node)?; db::append_logs(&state.db, b.deployment_id, &b.lines).await?; Ok(Json(json!({ "ok": true }))) } @@ -1014,10 +1023,17 @@ async fn node_result( headers: HeaderMap, Json(out): Json, ) -> ApiResult> { - node_auth(&state, &headers).await?; + let node = node_auth(&state, &headers).await?; let d = db::deployment(&state.db, out.deployment_id) .await? .ok_or_else(|| missing("deployment"))?; + claimed_by(&d, &node)?; + if d.state != "building" { + return Err(Fail( + StatusCode::CONFLICT, + "that deployment is not building".into(), + )); + } let site = db::site(&state.db, d.site_id) .await? .ok_or_else(|| missing("site"))?; @@ -1051,6 +1067,16 @@ async fn node_result( Ok(Json(json!({ "ok": true }))) } +/// A node may only report on a deployment that was queued for it. Without +/// this, any node token could mark a local deployment ready with an +/// `artifact_path` of `/`, or a `container_port` of any port on the panel. +fn claimed_by(d: &crate::models::Deployment, node: &crate::models::Node) -> ApiResult<()> { + if d.node_id.as_deref().unwrap_or("local") != node.id { + return Err(missing("deployment")); + } + Ok(()) +} + // ── the panel itself ──────────────────────────────────────────────────────── #[derive(rust_embed::RustEmbed)] diff --git a/src/buildpipe.rs b/src/buildpipe.rs index be11689..4c06f80 100644 --- a/src/buildpipe.rs +++ b/src/buildpipe.rs @@ -34,7 +34,13 @@ pub struct BuildEnv { pub type LogSink = mpsc::Sender<(String, String)>; pub async fn run(job: &BuildJob, env: &BuildEnv, sink: LogSink) -> BuildOutcome { - let secrets: Vec = job.clone_token.iter().cloned().collect(); + let mut secrets: Vec = job.clone_token.iter().cloned().collect(); + // The header form of the token, in case git ever echoes it. + if let Some(t) = &job.clone_token { + use base64::Engine; + secrets + .push(base64::engine::general_purpose::STANDARD.encode(format!("x-access-token:{t}"))); + } let recorder = Recorder::new(sink, secrets.clone()); match run_inner(job, env, &recorder).await { @@ -106,10 +112,18 @@ async fn run_inner(job: &BuildJob, env: &BuildEnv, rec: &Recorder) -> Result Result<()> { - let url = gh::clone_url(&job.repo, job.clone_token.as_deref()); - git(work, &["init", "--quiet"], rec).await?; - git(work, &["remote", "add", "origin", &url], rec).await?; + let url = gh::clone_url(&job.repo, None); + let auth = job.clone_token.as_deref().map(gh::auth_env); + let auth = auth.as_deref(); + git(work, &["init", "--quiet"], None, rec).await?; + git(work, &["remote", "add", "origin", &url], None, rec).await?; git( work, &[ @@ -120,6 +134,7 @@ async fn clone(job: &BuildJob, work: &Path, rec: &Recorder) -> Result<()> { "origin", &job.commit_sha, ], + auth, rec, ) .await @@ -133,6 +148,7 @@ async fn clone(job: &BuildJob, work: &Path, rec: &Recorder) -> Result<()> { git( work, &["checkout", "--quiet", "--detach", "FETCH_HEAD"], + None, rec, ) .await?; @@ -150,6 +166,7 @@ async fn clone(job: &BuildJob, work: &Path, rec: &Recorder) -> Result<()> { "1", "--recursive", ], + auth, rec, ) .await?; @@ -157,13 +174,22 @@ async fn clone(job: &BuildJob, work: &Path, rec: &Recorder) -> Result<()> { Ok(()) } -async fn git(dir: &Path, args: &[&str], rec: &Recorder) -> Result<()> { - let out = Command::new("git") - .args(args) +async fn git( + dir: &Path, + args: &[&str], + auth: Option<&[(String, String)]>, + rec: &Recorder, +) -> Result<()> { + let mut cmd = Command::new("git"); + cmd.args(args) .current_dir(dir) .env("GIT_TERMINAL_PROMPT", "0") .env("GIT_CONFIG_NOSYSTEM", "1") - .stdin(Stdio::null()) + .stdin(Stdio::null()); + for (k, v) in auth.unwrap_or_default() { + cmd.env(k, v); + } + let out = cmd .output() .await .context("running git — is it installed on this node?")?; @@ -248,6 +274,11 @@ async fn static_build( })?; let out_dir = src.join(rel.trim_matches('/')); + contained( + &env.work_dir.join(format!("d{}", job.deployment_id)), + &out_dir, + &rel, + )?; verify_static(&out_dir, &rel, env.max_artifact_mb)?; let dest = env @@ -278,6 +309,26 @@ async fn static_build( }) } +/// The output directory must be a real directory inside this build's workspace. +/// The repository and every install script control what is in there, so a +/// symlinked `dist` (or root directory) would otherwise make `place` move or +/// publish a directory from anywhere on the host. +fn contained(work: &Path, out_dir: &Path, rel: &str) -> Result<()> { + if std::fs::symlink_metadata(out_dir) + .map(|m| m.file_type().is_symlink()) + .unwrap_or(false) + { + bail!("output directory `{rel}` is a symlink; it must be a real directory"); + } + let (Ok(work), Ok(out)) = (std::fs::canonicalize(work), std::fs::canonicalize(out_dir)) else { + return Ok(()); // does not exist: verify_static says so + }; + if !out.starts_with(&work) { + bail!("output directory `{rel}` resolves outside the build workspace"); + } + Ok(()) +} + /// The checks that catch the mistake everybody makes: a build that "succeeded" /// into the wrong directory. Publishing that produces a live site serving /// nothing, with a green tick next to it. @@ -586,6 +637,25 @@ mod tests { d } + #[test] + fn a_symlinked_or_escaping_output_directory_is_refused() { + let outside = dir_with(&[("index.html", "

host

")]); + let work = dir_with(&[("real/index.html", "

ok

")]); + std::os::unix::fs::symlink(outside.path(), work.path().join("dist")).unwrap(); + std::os::unix::fs::symlink(outside.path(), work.path().join("root")).unwrap(); + + assert!(contained(work.path(), &work.path().join("real"), "real").is_ok()); + let err = contained(work.path(), &work.path().join("dist"), "dist") + .unwrap_err() + .to_string(); + assert!(err.contains("symlink"), "{err}"); + // A symlinked root directory with a real-looking output under it. + let err = contained(work.path(), &work.path().join("root/."), "root/.") + .unwrap_err() + .to_string(); + assert!(err.contains("outside"), "{err}"); + } + #[test] fn verification_rejects_a_missing_output_directory() { let d = tempfile::tempdir().unwrap(); diff --git a/src/edge.rs b/src/edge.rs index 2d18b83..84fa509 100644 --- a/src/edge.rs +++ b/src/edge.rs @@ -33,11 +33,16 @@ pub enum Target { Remote { base: String, spec: String, + /// The node's stored token hash, sent so the node can tell the panel + /// from anyone else who can reach its edge port. + key: Option, }, } /// The contract between the panel's edge and a node's edge. pub const TARGET_HEADER: &str = "x-openpages-target"; +/// SHA-256 of the node's secret. The node recomputes it from its own token. +pub const NODE_KEY_HEADER: &str = "x-openpages-node-key"; #[derive(Clone, Debug)] pub struct Route { @@ -137,7 +142,11 @@ async fn resolve_uncached(state: &Shared, host: &str) -> Option { return Some(Route { site_id: site.id, deployment_id: deployment.id, - target: Target::Remote { base, spec }, + target: Target::Remote { + base, + spec, + key: node.token_hash.clone(), + }, }); } } @@ -174,8 +183,10 @@ pub async fn handle(State(state): State, req: Request) -> Response { let mut response = match route.target.clone() { Target::Static { dir, rules } => serve_static(&dir, &rules, req).await, - Target::Container { port } => proxy(&format!("http://127.0.0.1:{port}"), req, None).await, - Target::Remote { base, spec } => proxy(&base, req, Some(&spec)).await, + Target::Container { port } => { + proxy(&format!("http://127.0.0.1:{port}"), req, None, None).await + } + Target::Remote { base, spec, key } => proxy(&base, req, Some(&spec), key.as_deref()).await, }; // Which deployment answered. The difference between "the swap did not @@ -229,8 +240,7 @@ pub async fn serve_static(dir: &Path, rules: &Rules, req: Request) -> Response { None => {} } - let custom = dir.join("404.html"); - if custom.is_file() { + if let Some((custom, _)) = pick_file(dir, "404.html").await { return send_file(&custom, 404, &path, rules, req.headers()).await; } ( @@ -244,6 +254,16 @@ pub async fn serve_static(dir: &Path, rules: &Rules, req: Request) -> Response { /// `/`, `/about`, `/about/` and `/about.html` should all find the same page — /// static site generators disagree about which one they emit. async fn pick_file(dir: &Path, rel: &str) -> Option<(PathBuf, u16)> { + let (file, status) = pick_file_unchecked(dir, rel).await?; + // A symlink in the build output must not reach outside the deployment: + // `ln -s /etc dist/x`, from the repo or from any install script, would + // otherwise publish the host's filesystem. + let root = tokio::fs::canonicalize(dir).await.ok()?; + let real = tokio::fs::canonicalize(&file).await.ok()?; + real.starts_with(&root).then_some((real, status)) +} + +async fn pick_file_unchecked(dir: &Path, rel: &str) -> Option<(PathBuf, u16)> { let base = dir.join(rel); if base.is_file() { return Some((base, 200)); @@ -532,7 +552,12 @@ const HOP_BY_HOP: [&str; 8] = [ "upgrade", ]; -pub async fn proxy(base: &str, req: Request, spec: Option<&str>) -> Response { +pub async fn proxy( + base: &str, + req: Request, + spec: Option<&str>, + node_key: Option<&str>, +) -> Response { let (parts, body) = req.into_parts(); let path_q = parts .uri @@ -552,6 +577,13 @@ pub async fn proxy(base: &str, req: Request, spec: Option<&str>) -> Response { if HOP_BY_HOP.contains(&name.as_str()) || name == "host" { continue; } + // Never forward a visitor's own routing headers. reqwest appends rather + // than replaces, and the node reads the first value, so a visitor + // sending `x-openpages-target: container:6379` used to choose which + // local port on the node the request went to. + if name == TARGET_HEADER || name == NODE_KEY_HEADER { + continue; + } out = out.header(k, v); } out = out.header("x-forwarded-proto", "https"); @@ -561,6 +593,9 @@ pub async fn proxy(base: &str, req: Request, spec: Option<&str>) -> Response { if let Some(spec) = spec { out = out.header(TARGET_HEADER, spec); } + if let Some(key) = node_key { + out = out.header(NODE_KEY_HEADER, key); + } let resp = match out.body(bytes).send().await { Ok(r) => r, @@ -601,6 +636,80 @@ pub fn header_str(headers: &HeaderMap, name: header::HeaderName) -> Option Request { + axum::http::Request::builder() + .uri(path) + .body(Body::empty()) + .unwrap() + } + + #[tokio::test] + async fn symlinks_out_of_the_deployment_are_not_served() { + let outside = tempfile::tempdir().unwrap(); + std::fs::write(outside.path().join("secret.txt"), "host file").unwrap(); + let site = tempfile::tempdir().unwrap(); + std::fs::write(site.path().join("index.html"), "

hi

").unwrap(); + std::fs::write(site.path().join("a.txt"), "inside").unwrap(); + std::os::unix::fs::symlink( + outside.path().join("secret.txt"), + site.path().join("leak.txt"), + ) + .unwrap(); + std::os::unix::fs::symlink(outside.path(), site.path().join("dir")).unwrap(); + std::os::unix::fs::symlink(site.path().join("a.txt"), site.path().join("alias.txt")) + .unwrap(); + let rules = Rules::default(); + + let ok = serve_static(site.path(), &rules, get("/index.html")).await; + assert_eq!(ok.status(), StatusCode::OK); + // A symlink that stays inside the deployment is fine. + let alias = serve_static(site.path(), &rules, get("/alias.txt")).await; + assert_eq!(alias.status(), StatusCode::OK); + for p in ["/leak.txt", "/dir/secret.txt"] { + let r = serve_static(site.path(), &rules, get(p)).await; + assert_eq!(r.status(), StatusCode::NOT_FOUND, "{p}"); + } + } + + #[tokio::test] + async fn a_visitor_cannot_choose_the_node_target() { + // A stand-in node edge that reports every target and key it was sent. + async fn echo(req: Request) -> String { + let all = |n: &str| { + req.headers() + .get_all(n) + .iter() + .map(|v| v.to_str().unwrap().to_string()) + .collect::>() + .join(",") + }; + format!("{}|{}", all(TARGET_HEADER), all(NODE_KEY_HEADER)) + } + let l = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = l.local_addr().unwrap(); + tokio::spawn(async move { + axum::serve(l, axum::Router::new().fallback(echo)) + .await + .unwrap(); + }); + + let req = axum::http::Request::builder() + .uri("/") + .header(TARGET_HEADER, "container:6379") + .header(NODE_KEY_HEADER, "forged") + .body(Body::empty()) + .unwrap(); + let resp = proxy( + &format!("http://{addr}"), + req, + Some("static:blog/1"), + Some("k"), + ) + .await; + let body = axum::body::to_bytes(resp.into_body(), 1024).await.unwrap(); + assert_eq!(std::str::from_utf8(&body).unwrap(), "static:blog/1|k"); + } + #[test] fn the_panel_host_beats_a_site_with_the_same_name() { assert!(matches!( diff --git a/src/gh.rs b/src/gh.rs index a43987b..6361ed7 100644 --- a/src/gh.rs +++ b/src/gh.rs @@ -342,6 +342,24 @@ pub fn clone_url(repo: &str, token: Option<&str>) -> String { } } +/// Environment that hands git the token as an HTTP header for github.com only, +/// without putting it in argv or in any file. +pub fn auth_env(token: &str) -> Vec<(String, String)> { + use base64::Engine; + let basic = base64::engine::general_purpose::STANDARD.encode(format!("x-access-token:{token}")); + vec![ + ("GIT_CONFIG_COUNT".into(), "1".into()), + ( + "GIT_CONFIG_KEY_0".into(), + "http.https://github.com/.extraheader".into(), + ), + ( + "GIT_CONFIG_VALUE_0".into(), + format!("AUTHORIZATION: basic {basic}"), + ), + ] +} + pub fn first_line(s: &str) -> &str { s.lines().next().unwrap_or("").trim() } @@ -356,6 +374,17 @@ mod tests { assert!(clone_url("a/b", Some("ghs_x")).contains("x-access-token:ghs_x@")); } + #[test] + fn auth_env_carries_the_token_as_a_github_scoped_header() { + let env = auth_env("ghs_x"); + assert!(env + .iter() + .any(|(k, v)| k == "GIT_CONFIG_KEY_0" && v == "http.https://github.com/.extraheader")); + // base64("x-access-token:ghs_x") + assert!(env.iter().any(|(k, v)| k == "GIT_CONFIG_VALUE_0" + && v == "AUTHORIZATION: basic eC1hY2Nlc3MtdG9rZW46Z2hzX3g=")); + } + #[test] fn first_line_trims_multiline_errors() { assert_eq!(first_line("boom\ndetails\nmore"), "boom"); diff --git a/src/heal.rs b/src/heal.rs index 54a1996..280fa3f 100644 --- a/src/heal.rs +++ b/src/heal.rs @@ -224,8 +224,11 @@ async fn run(state: &Shared, t: &Trigger, mode: &'static str, heal_id: i64) -> R let _ = tokio::fs::remove_dir_all(&dir).await; tokio::fs::create_dir_all(&dir).await?; + // The token goes to git through the environment, never into the remote + // URL: `.git/config` is readable by the agent working in this checkout. let token = state.gh.token().await.ok(); - let url = gh::clone_url(&t.repo, token.as_deref()); + let auth = token.as_deref().map(gh::auth_env).unwrap_or_default(); + let url = gh::clone_url(&t.repo, None); // Shallow but not depth-1: a fix often needs to see what recently changed, // and `git log` on a single commit tells you nothing. @@ -233,22 +236,22 @@ async fn run(state: &Shared, t: &Trigger, mode: &'static str, heal_id: i64) -> R &dir, "git", &["clone", "--depth", "30", "--branch", &t.branch, &url, "."], - None, + &auth, ) .await .context("cloning the repository to work in")?; if let Some(sha) = &t.commit_sha { - let _ = sh(&dir, "git", &["checkout", "--quiet", sha], None).await; + let _ = sh(&dir, "git", &["checkout", "--quiet", sha], &[]).await; } - let before = sh(&dir, "git", &["rev-parse", "HEAD"], None) + let before = sh(&dir, "git", &["rev-parse", "HEAD"], &[]) .await? .trim() .to_string(); let note = ask_claude(state, &dir, &t.incident).await?; - let dirty = sh(&dir, "git", &["status", "--porcelain"], None).await?; + let dirty = sh(&dir, "git", &["status", "--porcelain"], &[]).await?; if dirty.trim().is_empty() { let _ = tokio::fs::remove_dir_all(&dir).await; return Ok(Outcome::NoFix { note }); @@ -261,8 +264,8 @@ async fn run(state: &Shared, t: &Trigger, mode: &'static str, heal_id: i64) -> R crate::incident::first_sentence(&t.incident.summary) ); - sh(&dir, "git", &["checkout", "-b", &branch], None).await?; - sh(&dir, "git", &["add", "-A"], None).await?; + sh(&dir, "git", &["checkout", "-b", &branch], &[]).await?; + sh(&dir, "git", &["add", "-A"], &[]).await?; sh( &dir, "git", @@ -275,7 +278,7 @@ async fn run(state: &Shared, t: &Trigger, mode: &'static str, heal_id: i64) -> R "-m", &format!("{title}\n\n{note}"), ], - None, + &[], ) .await?; @@ -289,7 +292,7 @@ async fn run(state: &Shared, t: &Trigger, mode: &'static str, heal_id: i64) -> R &dir, "git", &["push", "--force-with-lease", "origin", &refspec], - None, + &auth, ) .await .context("pushing the fix")?; @@ -401,7 +404,7 @@ pub fn parse_reply(stdout: &str) -> String { .to_string() } -async fn sh(dir: &PathBuf, bin: &str, args: &[&str], env: Option<(&str, &str)>) -> Result { +async fn sh(dir: &PathBuf, bin: &str, args: &[&str], env: &[(String, String)]) -> Result { let mut cmd = Command::new(bin); cmd.args(args) .current_dir(dir) @@ -409,7 +412,7 @@ async fn sh(dir: &PathBuf, bin: &str, args: &[&str], env: Option<(&str, &str)>) .stdin(Stdio::null()) .stdout(Stdio::piped()) .stderr(Stdio::piped()); - if let Some((k, v)) = env { + for (k, v) in env { cmd.env(k, v); } let out = cmd diff --git a/src/node.rs b/src/node.rs index 31f3396..cebf3ee 100644 --- a/src/node.rs +++ b/src/node.rs @@ -242,6 +242,23 @@ impl Agent { // ── the node's edge ───────────────────────────────────────────────────────── +fn not_from_panel() -> Response { + ( + StatusCode::BAD_REQUEST, + "this is an openpages node edge; it only answers requests proxied by a panel\n", + ) + .into_response() +} + +/// The panel stores `sha256(secret)` for a token `.` and sends it +/// with every proxied request. +fn key_matches(token: &str, presented: &str) -> bool { + let Some((_, secret)) = token.split_once('.') else { + return false; + }; + !presented.is_empty() && crate::util::secret_eq(&crate::util::sha256_hex(secret), presented) +} + async fn serve_edge(agent: Agent, listen: &str) -> Result<()> { let app = Router::new() .fallback(get(serve).post(serve).put(serve).patch(serve).delete(serve)) @@ -259,16 +276,22 @@ async fn serve(State(agent): State, req: Request) -> Response { req.headers(), axum::http::HeaderName::from_static(edge::TARGET_HEADER), ) else { - return ( - StatusCode::BAD_REQUEST, - "this is an openpages node edge; it only answers requests proxied by a panel\n", - ) - .into_response(); + return not_from_panel(); }; + // Only the panel knows the hash of this node's secret. Without the check, + // anyone who could reach this port picked any local port to proxy to. + let presented = edge::header_str( + req.headers(), + axum::http::HeaderName::from_static(edge::NODE_KEY_HEADER), + ) + .unwrap_or_default(); + if !key_matches(&agent.token, &presented) { + return not_from_panel(); + } match spec.split_once(':') { Some(("container", port)) => match port.parse::() { - Ok(p) => edge::proxy(&format!("http://127.0.0.1:{p}"), req, None).await, + Ok(p) => edge::proxy(&format!("http://127.0.0.1:{p}"), req, None, None).await, Err(_) => (StatusCode::BAD_REQUEST, "bad container target").into_response(), }, Some(("static", rel)) => { @@ -286,3 +309,18 @@ async fn serve(State(agent): State, req: Request) -> Response { _ => (StatusCode::BAD_REQUEST, "unrecognised target").into_response(), } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn only_the_hash_of_our_own_secret_opens_the_edge() { + let token = "n_ab12cd.s3cret"; + let key = crate::util::sha256_hex("s3cret"); + assert!(key_matches(token, &key)); + assert!(!key_matches(token, "")); + assert!(!key_matches(token, &crate::util::sha256_hex("other"))); + assert!(!key_matches("no-dot", &key)); + } +}