max / makenotwork
5 files changed,
+148 insertions,
-75 deletions
| @@ -78,9 +78,24 @@ | |||
| 78 | 78 | staging_root = "/srv/sando/staging" | |
| 79 | 79 | url = "http://100.103.89.95:7766" | |
| 80 | 80 | sando_app = "pom" | |
| 81 | - | token_file = "sando/api-token" | |
| 81 | + | token_env = "BENTO_SANDO_TOKEN" | |
| 82 | 82 | ``` | |
| 83 | 83 | ||
| 84 | + | `token_env` names the environment variable holding sandod's bearer token; it | |
| 85 | + | never holds the token itself and never points at a file. Set the variable in | |
| 86 | + | bentod's `EnvironmentFile` (`~/.config/bento/bento.env`, already there for | |
| 87 | + | `BENTO_API_TOKEN`) with the value of `SANDO_API_TOKEN` from | |
| 88 | + | `/etc/sando/sando.env` on the Sando host. Those two have to match, and the | |
| 89 | + | daemon says so by name when the variable is unset. | |
| 90 | + | ||
| 91 | + | This field used to be `token_file`, a path under `secrets_root`. That put a | |
| 92 | + | bearer token on disk in plaintext inside `_private`, which is under git now, and | |
| 93 | + | a signing key and a bearer token are not the same kind of secret: a key is a | |
| 94 | + | file because a tool opens it by path, while a token is one opaque string held | |
| 95 | + | for the life of the process. The old key is refused rather than ignored, so a | |
| 96 | + | config that was never migrated fails at startup instead of sending no header and | |
| 97 | + | collecting a 401. | |
| 98 | + | ||
| 84 | 99 | Per target, the collect directory is rsynced to | |
| 85 | 100 | `<staging_root>/<app>-<version>-<target>/` and sandod is asked to take it in. | |
| 86 | 101 | Sando proves the bundle against its record before anything else happens, so a |
| @@ -114,7 +114,12 @@ | |||
| 114 | 114 | /// record copied in among the artifacts would change the digest it names and | |
| 115 | 115 | /// Sando would refuse the bundle — correctly. The record travels in the request | |
| 116 | 116 | /// body instead. | |
| 117 | + | /// | |
| 118 | + | /// Unknown fields are refused. The table is five keys an operator writes by | |
| 119 | + | /// hand, and the one migration it has had — `token_file` to `token_env` — is | |
| 120 | + | /// exactly the case a silently ignored key turns into a 401 nobody can read. | |
| 117 | 121 | #[derive(Debug, Clone, Deserialize)] | |
| 122 | + | #[serde(deny_unknown_fields)] | |
| 118 | 123 | pub struct Handoff { | |
| 119 | 124 | /// SSH destination of the host sandod runs on (a tailnet alias like `fw13`, | |
| 120 | 125 | /// or `sando@fw13`); `local` stages on this daemon's own filesystem. | |
| @@ -137,13 +142,20 @@ | |||
| 137 | 142 | /// present routes to `/apps/<id>/intake`. | |
| 138 | 143 | #[serde(default)] | |
| 139 | 144 | pub sando_app: Option<String>, | |
| 140 | - | /// File holding sandod's bearer token, as a relative path under | |
| 141 | - | /// `secrets_root` — the same private layer the `secret()` host function | |
| 142 | - | /// reads, so the token is not a second secrets mechanism. Absent sends no | |
| 143 | - | /// `Authorization` header, which only works against a loopback sandod that | |
| 144 | - | /// configured none. | |
| 145 | + | /// Name of the environment variable holding sandod's bearer token. | |
| 146 | + | /// | |
| 147 | + | /// The token is named, never inlined and never a path into `secrets_root`: | |
| 148 | + | /// this file describes topology and has every reason to be readable, while | |
| 149 | + | /// both daemons already take their own tokens from the environment | |
| 150 | + | /// (`SANDO_API_TOKEN`, `BENTO_API_TOKEN`). Pointing at a file put the | |
| 151 | + | /// credential on disk in plaintext inside a tree that is now under git, | |
| 152 | + | /// where the only thing standing between it and a mirror was a pre-commit | |
| 153 | + | /// hook. Same convention as magicmirror's `[[source]] token_env`. | |
| 154 | + | /// | |
| 155 | + | /// Absent sends no `Authorization` header, which only works against a | |
| 156 | + | /// loopback sandod that configured none. | |
| 145 | 157 | #[serde(default)] | |
| 146 | - | pub token_file: Option<PathBuf>, | |
| 158 | + | pub token_env: Option<String>, | |
| 147 | 159 | } | |
| 148 | 160 | ||
| 149 | 161 | fn default_true() -> bool { | |
| @@ -213,16 +225,19 @@ | |||
| 213 | 225 | "[handoff.{app}] url `{}` must be an http(s) URL for sandod", | |
| 214 | 226 | h.url | |
| 215 | 227 | ); | |
| 216 | - | // The token is read relative to `secrets_root`, so the same | |
| 217 | - | // traversal guard the `secret()` host function applies belongs here: | |
| 218 | - | // a config that could name `../../etc/shadow` would turn a path into | |
| 219 | - | // a read primitive. | |
| 220 | - | if let Some(t) = &h.token_file { | |
| 228 | + | // A variable name, not a value and not a path. Catching the two | |
| 229 | + | // wrong shapes at startup matters more than it looks: a name with a | |
| 230 | + | // `/` in it is an operator who wrote the old `token_file` spelling | |
| 231 | + | // under the new key, and a name that parses as one but holds the | |
| 232 | + | // secret itself is the paste this field exists to prevent. Both | |
| 233 | + | // would otherwise surface as an unset variable at the first release. | |
| 234 | + | if let Some(t) = &h.token_env { | |
| 221 | 235 | anyhow::ensure!( | |
| 222 | - | t.is_relative() | |
| 223 | - | && !t.components().any(|c| c == std::path::Component::ParentDir), | |
| 224 | - | "[handoff.{app}] token_file `{}` must be a relative path under secrets_root", | |
| 225 | - | t.display() | |
| 236 | + | !t.trim().is_empty() | |
| 237 | + | && t.chars().all(|c| c.is_ascii_alphanumeric() || c == '_') | |
| 238 | + | && !t.starts_with(|c: char| c.is_ascii_digit()), | |
| 239 | + | "[handoff.{app}] token_env `{t}` must name an environment variable \ | |
| 240 | + | (letters, digits and underscores), not hold the token or a path" | |
| 226 | 241 | ); | |
| 227 | 242 | } | |
| 228 | 243 | } | |
| @@ -329,7 +344,7 @@ | |||
| 329 | 344 | // Absent means Sando's default product and no bearer header, which is | |
| 330 | 345 | // the shape a single-product sandod on loopback wants. | |
| 331 | 346 | assert!(h.sando_app.is_none()); | |
| 332 | - | assert!(h.token_file.is_none()); | |
| 347 | + | assert!(h.token_env.is_none()); | |
| 333 | 348 | } | |
| 334 | 349 | ||
| 335 | 350 | /// The destination is rsynced with `--delete`. A relative path lands in the | |
| @@ -347,19 +362,39 @@ | |||
| 347 | 362 | assert!(parse(&with("/srv/sando/staging")).is_ok()); | |
| 348 | 363 | } | |
| 349 | 364 | ||
| 350 | - | /// `token_file` is resolved under `secrets_root`. Left unguarded it would be | |
| 351 | - | /// a read primitive for any file the daemon user can open. | |
| 365 | + | /// `token_env` names a variable. The two ways to get it wrong both read as | |
| 366 | + | /// plausible TOML and both fail at the first release rather than at | |
| 367 | + | /// startup: a path is the retired `token_file` spelling moved to the new | |
| 368 | + | /// key, and a value is the paste the field exists to prevent. | |
| 352 | 369 | #[test] | |
| 353 | - | fn a_token_file_that_escapes_secrets_root_is_rejected() { | |
| 370 | + | fn a_token_env_that_is_a_path_or_a_pasted_secret_is_rejected() { | |
| 354 | 371 | let with = |token: &str| { | |
| 355 | 372 | format!( | |
| 356 | 373 | "[handoff.pom]\nhost = \"fw13\"\nstaging_root = \"/srv/sando/staging\"\n\ | |
| 357 | - | url = \"http://x:1\"\ntoken_file = \"{token}\"\n" | |
| 374 | + | url = \"http://x:1\"\ntoken_env = \"{token}\"\n" | |
| 358 | 375 | ) | |
| 359 | 376 | }; | |
| 377 | + | assert!(parse(&with("sando/api-token")).is_err()); | |
| 360 | 378 | assert!(parse(&with("../../etc/shadow")).is_err()); | |
| 361 | - | assert!(parse(&with("/etc/shadow")).is_err()); | |
| 362 | - | assert!(parse(&with("sando/api-token")).is_ok()); | |
| 379 | + | assert!(parse(&with("")).is_err()); | |
| 380 | + | // A hex token pasted where its name belongs: the dashes and slashes are | |
| 381 | + | // gone, but so is any reason a variable would be named this. | |
| 382 | + | assert!(parse(&with("2f9c4e1a-77bd-4f0e-9a3e-1c8d5b6e0f21")).is_err()); | |
| 383 | + | assert!(parse(&with("BENTO_SANDO_TOKEN")).is_ok()); | |
| 384 | + | } | |
| 385 | + | ||
| 386 | + | /// The old key is gone rather than silently ignored. `serde` accepts an | |
| 387 | + | /// unknown field by default, so a config still carrying `token_file` would | |
| 388 | + | /// load, send no `Authorization` header, and fail at sandod as a 401 that | |
| 389 | + | /// reads as a wrong token rather than as a config that was never migrated. | |
| 390 | + | #[test] | |
| 391 | + | fn the_retired_token_file_key_is_refused() { | |
| 392 | + | let err = parse( | |
| 393 | + | "[handoff.pom]\nhost = \"fw13\"\nstaging_root = \"/srv/sando/staging\"\n\ | |
| 394 | + | url = \"http://x:1\"\ntoken_file = \"sando/api-token\"\n", | |
| 395 | + | ) | |
| 396 | + | .expect_err("token_file was replaced by token_env"); | |
| 397 | + | assert!(format!("{err:#}").contains("token_file"), "{err:#}"); | |
| 363 | 398 | } | |
| 364 | 399 | ||
| 365 | 400 | /// The url is interpolated into a request. A bare host:port would be sent |
| @@ -81,25 +81,42 @@ | |||
| 81 | 81 | } | |
| 82 | 82 | } | |
| 83 | 83 | ||
| 84 | - | /// Read the bearer token for this handoff out of the private layer. | |
| 84 | + | /// Read the bearer token for this handoff out of the environment. | |
| 85 | 85 | /// | |
| 86 | - | /// Resolved under `secrets_root` the same way the `secret()` host function | |
| 87 | - | /// resolves a recipe's credentials, so there is one place secrets live rather | |
| 88 | - | /// than two. Never logged, and the trailing newline a file written by `echo` | |
| 89 | - | /// carries is trimmed — it would otherwise become part of the header value. | |
| 90 | - | async fn token(cfg: &Config, handoff: &Handoff) -> anyhow::Result<Option<String>> { | |
| 91 | - | let Some(rel) = &handoff.token_file else { | |
| 86 | + | /// From the environment and not from `secrets_root`, which is where this used | |
| 87 | + | /// to read: a bearer token is not a signing key. The recipe secrets under the | |
| 88 | + | /// private layer are files because they are files — a keystore, a notary | |
| 89 | + | /// credential, things a tool opens by path — while this is one opaque string | |
| 90 | + | /// held for the life of the process, and writing it to disk to read it back | |
| 91 | + | /// bought nothing but a plaintext credential in a tree that is now under git. | |
| 92 | + | /// Both daemons already take their own tokens this way (`SANDO_API_TOKEN`, | |
| 93 | + | /// `BENTO_API_TOKEN`), so this is the existing mechanism rather than a third. | |
| 94 | + | /// | |
| 95 | + | /// Never logged, and the value is trimmed: an `EnvironmentFile` line pasted | |
| 96 | + | /// with a trailing space would otherwise put it inside the header value, which | |
| 97 | + | /// fails as a 401 with nothing to see in it. | |
| 98 | + | fn token(handoff: &Handoff) -> anyhow::Result<Option<String>> { | |
| 99 | + | let Some(name) = &handoff.token_env else { | |
| 92 | 100 | return Ok(None); | |
| 93 | 101 | }; | |
| 94 | - | let path = cfg.secrets_root.join(rel); | |
| 95 | - | let raw = tokio::fs::read_to_string(&path) | |
| 96 | - | .await | |
| 97 | - | .map_err(|e| anyhow::anyhow!("reading the sando token at {}: {e}", path.display()))?; | |
| 98 | - | let trimmed = raw.trim().to_string(); | |
| 102 | + | token_from(name, std::env::var(name).ok()) | |
| 103 | + | } | |
| 104 | + | ||
| 105 | + | /// The value half of `token`, split out so the rules can be tested without | |
| 106 | + | /// touching the process environment. `set_var` is global and unsynchronized, | |
| 107 | + | /// and this binary has already been bitten once by a test that set one (see | |
| 108 | + | /// `engine::tests::expand_tilde_handles_home`). | |
| 109 | + | fn token_from(name: &str, raw: Option<String>) -> anyhow::Result<Option<String>> { | |
| 110 | + | // Unset and empty are one case on purpose. `EnvironmentFile` turns a line | |
| 111 | + | // whose value was never filled in into an empty variable rather than no | |
| 112 | + | // variable, so treating empty as "no header" would make a half-finished | |
| 113 | + | // bootstrap look like a deliberately unauthenticated handoff. | |
| 114 | + | let trimmed = raw.unwrap_or_default().trim().to_string(); | |
| 99 | 115 | anyhow::ensure!( | |
| 100 | 116 | !trimmed.is_empty(), | |
| 101 | - | "the sando token at {} is empty", | |
| 102 | - | path.display() | |
| 117 | + | "the sando bearer token is missing: `{name}` is unset or empty. Set it in \ | |
| 118 | + | bentod's EnvironmentFile (~/.config/bento/bento.env) to match \ | |
| 119 | + | SANDO_API_TOKEN in /etc/sando/sando.env on the sando host." | |
| 103 | 120 | ); | |
| 104 | 121 | Ok(Some(trimmed)) | |
| 105 | 122 | } | |
| @@ -161,7 +178,7 @@ | |||
| 161 | 178 | "staged": dest.to_string_lossy(), | |
| 162 | 179 | "record": record, | |
| 163 | 180 | })); | |
| 164 | - | if let Some(t) = token(cfg, handoff).await? { | |
| 181 | + | if let Some(t) = token(handoff)? { | |
| 165 | 182 | req = req.bearer_auth(t); | |
| 166 | 183 | } | |
| 167 | 184 | let resp = req | |
| @@ -201,7 +218,7 @@ | |||
| 201 | 218 | staging_root: root.join("staging"), | |
| 202 | 219 | url: "http://127.0.0.1:1".into(), | |
| 203 | 220 | sando_app: None, | |
| 204 | - | token_file: None, | |
| 221 | + | token_env: None, | |
| 205 | 222 | } | |
| 206 | 223 | } | |
| 207 | 224 | ||
| @@ -350,20 +367,41 @@ | |||
| 350 | 367 | assert!(!staged.exists(), "nothing should have been staged"); | |
| 351 | 368 | } | |
| 352 | 369 | ||
| 353 | - | /// An empty token file is a bootstrap that half-ran. Sending the header | |
| 354 | - | /// anyway would fail at sandod as a plain 401, which reads as a wrong token | |
| 355 | - | /// rather than a missing one. | |
| 356 | - | #[tokio::test] | |
| 357 | - | async fn an_empty_token_file_is_an_error_not_an_empty_header() { | |
| 358 | - | let tmp = tempfile::tempdir().unwrap(); | |
| 359 | - | let cfg = Config::for_tests(tmp.path()); | |
| 360 | - | std::fs::create_dir_all(&cfg.secrets_root).unwrap(); | |
| 361 | - | std::fs::write(cfg.secrets_root.join("sando-token"), "\n").unwrap(); | |
| 370 | + | /// An unset or empty variable is a bootstrap that half-ran, and both are | |
| 371 | + | /// the same mistake: `EnvironmentFile` turns `BENTO_SANDO_TOKEN=` into an | |
| 372 | + | /// empty variable, not a missing one. Sending the header anyway would fail | |
| 373 | + | /// at sandod as a plain 401, which reads as a wrong token rather than a | |
| 374 | + | /// missing one, and the error names the two files that have to agree. | |
| 375 | + | #[test] | |
| 376 | + | fn an_unset_or_empty_token_variable_is_an_error_not_an_empty_header() { | |
| 377 | + | for raw in [None, Some(String::new()), Some("\n".into())] { | |
| 378 | + | let err = token_from("BENTO_SANDO_TOKEN", raw) | |
| 379 | + | .expect_err("nothing in the variable is not a token"); | |
| 380 | + | let msg = format!("{err:#}"); | |
| 381 | + | assert!(msg.contains("BENTO_SANDO_TOKEN"), "{msg}"); | |
| 382 | + | assert!(msg.contains("SANDO_API_TOKEN"), "{msg}"); | |
| 383 | + | } | |
| 384 | + | } | |
| 362 | 385 | ||
| 363 | - | let mut h = handoff(tmp.path()); | |
| 364 | - | h.token_file = Some(PathBuf::from("sando-token")); | |
| 365 | - | let err = token(&cfg, &h).await.expect_err("empty is not a token"); | |
| 366 | - | assert!(format!("{err:#}").contains("empty"), "{err:#}"); | |
| 386 | + | /// The value is trimmed. An `EnvironmentFile` line pasted with a trailing | |
| 387 | + | /// space puts that space inside the header value, which fails as a 401 with | |
| 388 | + | /// nothing visible in it to explain why. | |
| 389 | + | #[test] | |
| 390 | + | fn a_token_is_read_trimmed() { | |
| 391 | + | assert_eq!( | |
| 392 | + | token_from("BENTO_SANDO_TOKEN", Some(" s3cr3t \n".into())) | |
| 393 | + | .unwrap() | |
| 394 | + | .as_deref(), | |
| 395 | + | Some("s3cr3t") | |
| 396 | + | ); | |
| 397 | + | } | |
| 398 | + | ||
| 399 | + | /// No variable named means no header, not an empty one — the shape a | |
| 400 | + | /// single-product sandod on loopback wants. | |
| 401 | + | #[test] | |
| 402 | + | fn no_token_env_means_no_header() { | |
| 403 | + | let tmp = tempfile::tempdir().unwrap(); | |
| 404 | + | assert_eq!(token(&handoff(tmp.path())).unwrap(), None); | |
| 367 | 405 | } | |
| 368 | 406 | ||
| 369 | 407 | /// The whole motion against a sandod-shaped listener: the bytes are staged, | |
| @@ -402,12 +440,15 @@ | |||
| 402 | 440 | ||
| 403 | 441 | let tmp = tempfile::tempdir().unwrap(); | |
| 404 | 442 | let mut cfg = Config::for_tests(tmp.path()); | |
| 405 | - | std::fs::create_dir_all(&cfg.secrets_root).unwrap(); | |
| 406 | - | std::fs::write(cfg.secrets_root.join("sando-token"), "s3cr3t\n").unwrap(); | |
| 443 | + | // The one place this binary sets a variable. Distinct from the HOME | |
| 444 | + | // lesson in `engine::tests`: the name is this test's own, so no other | |
| 445 | + | // test reads it, and it is never removed — the value it holds is the | |
| 446 | + | // whole point of the assertion at the bottom. | |
| 447 | + | unsafe { std::env::set_var("BENTO_HANDOFF_WIRE_TEST_TOKEN", "s3cr3t\n") }; | |
| 407 | 448 | let mut h = handoff(tmp.path()); | |
| 408 | 449 | h.url = format!("http://{addr}"); | |
| 409 | 450 | h.sando_app = Some("pom".into()); | |
| 410 | - | h.token_file = Some(PathBuf::from("sando-token")); | |
| 451 | + | h.token_env = Some("BENTO_HANDOFF_WIRE_TEST_TOKEN".into()); | |
| 411 | 452 | cfg.handoff.insert("pom".into(), h); | |
| 412 | 453 | ||
| 413 | 454 | let collected = tmp.path().join("collected"); | |
| @@ -479,22 +520,4 @@ | |||
| 479 | 520 | let msg = format!("{err:#}"); | |
| 480 | 521 | assert!(msg.contains("`pom` differs"), "{msg}"); | |
| 481 | 522 | } | |
| 482 | - | ||
| 483 | - | /// The token is trimmed: a file written with `echo` ends in a newline, and a | |
| 484 | - | /// newline inside a header value is not a credential problem an operator | |
| 485 | - | /// would ever guess at. | |
| 486 | - | #[tokio::test] | |
| 487 | - | async fn a_token_is_read_trimmed() { | |
| 488 | - | let tmp = tempfile::tempdir().unwrap(); | |
| 489 | - | let cfg = Config::for_tests(tmp.path()); | |
| 490 | - | std::fs::create_dir_all(&cfg.secrets_root).unwrap(); | |
| 491 | - | std::fs::write(cfg.secrets_root.join("sando-token"), "s3cr3t\n").unwrap(); | |
| 492 | - | ||
| 493 | - | let mut h = handoff(tmp.path()); | |
| 494 | - | h.token_file = Some(PathBuf::from("sando-token")); | |
| 495 | - | assert_eq!(token(&cfg, &h).await.unwrap().as_deref(), Some("s3cr3t")); | |
| 496 | - | // No file named means no header, not an empty one. | |
| 497 | - | h.token_file = None; | |
| 498 | - | assert_eq!(token(&cfg, &h).await.unwrap(), None); | |
| 499 | - | } | |
| 500 | 523 | } |
| @@ -298,7 +298,7 @@ | |||
| 298 | 298 | // bounds: adding MNW widens the gate by MNW, not by ~/Code. | |
| 299 | 299 | let err = sync | |
| 300 | 300 | .pull_file( | |
| 301 | - | std::path::Path::new("/home/max/Code/_private/sando/api-token"), | |
| 301 | + | std::path::Path::new("/home/max/Code/_private/apple/notary.p8"), | |
| 302 | 302 | std::path::Path::new("/tmp/out"), | |
| 303 | 303 | &SyncOpts::default(), | |
| 304 | 304 | ) |
| @@ -921,7 +921,7 @@ | |||
| 921 | 921 | staging_root: "/srv/sando/staging".into(), | |
| 922 | 922 | url: "http://127.0.0.1:7766".into(), | |
| 923 | 923 | sando_app: None, | |
| 924 | - | token_file: None, | |
| 924 | + | token_env: None, | |
| 925 | 925 | }, | |
| 926 | 926 | ); | |
| 927 | 927 | handing_off.validate_delivery(&cfg).unwrap(); |