From e916e4499dbf6b93dae0de9802439c371c258f91 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 07:30:22 +0000 Subject: [PATCH 1/2] Reach the object store with the deployment's AWS_* contract body_bake_v4 demanded BODY_BAKE_V4_ copies of the endpoint, bucket, key id and secret. Every deploy already sets AWS_ENDPOINT_URL, AWS_S3_BUCKET_NAME, AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY -- the sandbox and Railway identically -- so that asked an operator to duplicate four secrets to serve one artifact, and /helix2 could not work anywhere without them. Isolation from the map is about PATHS, not credentials: the cache directory stays v4's own and RAILWAY_VOL is still not consulted, but read-only credentials shared with every other consumer of this bucket cannot move anything. Setting a bake is now two variables, BODY_BAKE_V4_TAG and BODY_BAKE_V4_ASSET, neither with a default. .with_bucket_name stays and is load-bearing: AmazonS3Builder::from_env drops any AWS_* whose lowercased name is not one of its config keys, and its bucket key accepts only aws_bucket, aws_bucket_name, bucket_name and bucket (object_store-0.13.2 src/aws/builder.rs:497) -- AWS_S3_BUCKET_NAME is discarded with no warning. The endpoint, credentials and default region are read. Same call shape as osm_slab_hydrate, which is proven against this bucket. Env reads now strip surrounding quotes, which some variables in these containers carry and which otherwise fails as a bad credential. The replaced guard test asserted that a list of literals started with the prefix they were written with -- true by construction, unfalsifiable. It now checks the required set carries no BODY_BAKE_V4_ credential name and does carry AWS_S3_BUCKET_NAME. Verified end to end against the real bucket with only ambient AWS_* set: boot hydrated and checksum-verified, GET /api/bake/v4 returned 200 with the artifact streamed from disk. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012wrzeZAdwGYTCKoxamwQht --- crates/cockpit-server/src/body_bake_v4.rs | 138 +++++++++++++--------- 1 file changed, 85 insertions(+), 53 deletions(-) diff --git a/crates/cockpit-server/src/body_bake_v4.rs b/crates/cockpit-server/src/body_bake_v4.rs index a0d1ed2f3..a86ed6566 100644 --- a/crates/cockpit-server/src/body_bake_v4.rs +++ b/crates/cockpit-server/src/body_bake_v4.rs @@ -19,13 +19,17 @@ //! runs a baker, never reads a baker's inputs, and never writes into a //! directory a baker writes to. Bake sources and bake outputs live on the //! producer side; this side only ever consumes a published artifact. -//! - **It shares no variable NAME with anything.** Every setting is -//! `BODY_BAKE_V4_*`, including its own object-store credentials — it does -//! NOT read the ambient `AWS_*` or `RAILWAY_VOL` that the map path uses. -//! Paste the same values in if you like; the point is that changing this -//! deploy's v4 settings cannot move anything else, and vice versa. This is -//! also what makes the module liftable into its own crate later: it has no -//! configuration entanglement to unpick. +//! - **It invents no credential variable.** The object store is reached with +//! the deployment's existing `AWS_*` contract — `AWS_ENDPOINT_URL`, +//! `AWS_S3_BUCKET_NAME`, `AWS_ACCESS_KEY_ID`, `AWS_SECRET_ACCESS_KEY`, +//! `AWS_DEFAULT_REGION` — the same five every other consumer of this bucket +//! reads. An earlier version of this module demanded `BODY_BAKE_V4_`-prefixed +//! COPIES of all of them, which meant four duplicate secrets per deploy to +//! serve one artifact; that is a configuration burden, not an isolation win. +//! Shared read-only CREDENTIALS cannot move anything. Shared PATHS can, which +//! is why the cache directory below is still v4's own and why `RAILWAY_VOL` +//! (which steers the map's cache) is deliberately not consulted. That is the +//! real boundary: credentials shared, paths never. //! - **There is no v3 slot.** Not an oversight: a v3 slot here would be a //! second way to serve the shipped body, adjacent to the v4 one, and the //! whole reason `/helix2` exists is that the two must be separable. v3 is @@ -35,7 +39,7 @@ //! //! ```text //! S3 (durable source of truth, published by the baker — a different system) -//! │ $BODY_BAKE_V4_ENDPOINT/$BODY_BAKE_V4_BUCKET +//! │ $AWS_ENDPOINT_URL/$AWS_S3_BUCKET_NAME //! │ /q2/bakes/$BODY_BAKE_V4_TAG/{$BODY_BAKE_V4_ASSET, SHA256SUMS} //! ▼ //! volume ($BODY_BAKE_V4_DIR, /volume01/body-v4, else temp) @@ -46,7 +50,7 @@ //! //! # No defaults, ever //! -//! Neither variable has a default. A default artifact name is a bake this +//! Neither v4 coordinate has a default. A default artifact name is a bake this //! deploy did not choose, and the only artifact that exists to default TO is //! the v3 one — which is how a first version of this module came to point the //! v4 route at the v3 bake. Unconfigured means unconfigured: the route says so @@ -88,7 +92,14 @@ pub fn verified_path() -> Option<&'static Path> { /// exactly like an unset one rather than attempt a doomed call with an empty /// bucket name. fn env_var_nonempty(key: &str) -> Option { - std::env::var(key).ok().filter(|v| !v.trim().is_empty()) + // The surrounding-quote strip is defensive, not cosmetic: some variables in + // these containers arrive wrapped in literal `"` (documented for the token + // vars in `medcare-rs`'s CLAUDE.md), and an unstripped value fails auth in a + // way that reads as a bad credential rather than a quoting artifact. + std::env::var(key) + .ok() + .map(|v| v.trim().trim_matches('"').trim_matches('\'').to_string()) + .filter(|v| !v.is_empty()) } /// A name interpolated into BOTH an S3 key and a filesystem path. @@ -124,28 +135,25 @@ fn coordinates() -> Option<(String, String)> { Some((checked("BODY_BAKE_V4_TAG")?, checked("BODY_BAKE_V4_ASSET")?)) } -/// The v4 object store, built from v4-only variables. +/// The object store, from the deployment's existing `AWS_*` contract. /// -/// `AmazonS3Builder::from_env()` is deliberately NOT used: it reads the ambient -/// `AWS_*` that the map path also reads, which would make one deploy's v4 -/// credentials and another subsystem's the same knob. Every field is named -/// here so the two can never be the same setting — set them to the same values -/// if that is what you want. +/// **`.with_bucket_name` is load-bearing, not redundant.** +/// `AmazonS3Builder::from_env()` walks every `AWS_*` variable and silently drops +/// any whose lowercased name does not parse as one of its config keys. Its +/// bucket key accepts only `aws_bucket`, `aws_bucket_name`, `bucket_name` and +/// `bucket` (`object_store-0.13.2` `src/aws/builder.rs:497`) — so +/// `AWS_S3_BUCKET_NAME`, this workspace's name for it, is discarded with no +/// warning and the build then fails as if the bucket were never configured. +/// The endpoint, key id, secret and default region ARE read by `from_env` +/// (`:492-497`, `aws_endpoint_url` among the accepted endpoint spellings), so +/// the bucket is the only one this has to re-apply. +/// +/// Same call shape as [`crate::osm_slab_hydrate`]'s, deliberately: that path is +/// proven against this bucket, and a second spelling of the same handshake is a +/// second thing to get wrong. fn build_store() -> Option { - let endpoint = env_var_nonempty("BODY_BAKE_V4_ENDPOINT")?; - let bucket = env_var_nonempty("BODY_BAKE_V4_BUCKET")?; - let key_id = env_var_nonempty("BODY_BAKE_V4_ACCESS_KEY_ID")?; - let secret = env_var_nonempty("BODY_BAKE_V4_SECRET_ACCESS_KEY")?; - let region = env_var_nonempty("BODY_BAKE_V4_REGION").unwrap_or_else(|| "auto".to_string()); - - match AmazonS3Builder::new() - .with_endpoint(endpoint) - .with_bucket_name(bucket) - .with_access_key_id(key_id) - .with_secret_access_key(secret) - .with_region(region) - .build() - { + let bucket = env_var_nonempty("AWS_S3_BUCKET_NAME")?; + match AmazonS3Builder::from_env().with_bucket_name(bucket).build() { Ok(s) => Some(s), Err(e) => { tracing::error!(error = %e, "{LABEL}: S3 client build failed"); @@ -156,17 +164,25 @@ fn build_store() -> Option { /// Which required v4 variables are absent, so one boot line settles it. fn missing_vars() -> Vec<&'static str> { + required_vars() + .into_iter() + .filter(|k| env_var_nonempty(k).is_none()) + .collect() +} + +/// Every input this module requires, set or not. +fn required_vars() -> [&'static str; 6] { [ + // v4's own coordinates — the only names this module adds. "BODY_BAKE_V4_TAG", "BODY_BAKE_V4_ASSET", - "BODY_BAKE_V4_ENDPOINT", - "BODY_BAKE_V4_BUCKET", - "BODY_BAKE_V4_ACCESS_KEY_ID", - "BODY_BAKE_V4_SECRET_ACCESS_KEY", + // The deployment's existing object-store contract, shared with every + // other consumer of this bucket. Not duplicated under a v4 prefix. + "AWS_ENDPOINT_URL", + "AWS_S3_BUCKET_NAME", + "AWS_ACCESS_KEY_ID", + "AWS_SECRET_ACCESS_KEY", ] - .into_iter() - .filter(|k| env_var_nonempty(k).is_none()) - .collect() } /// The v4 cache directory. Its own leaf — `body-v4` — so nothing this module @@ -452,26 +468,42 @@ mod tests { unsafe { std::env::remove_var("RAILWAY_VOL") }; } + fn missing_vars_all() -> Vec<&'static str> { + required_vars().to_vec() + } + #[test] - fn no_v4_setting_shares_a_name_with_the_map_or_the_ambient_aws_config() { - // The crossed-wire guard: every name this module reads must be - // v4-scoped, so changing a v4 setting cannot move anything else. - for key in [ - "BODY_BAKE_V4_TAG", - "BODY_BAKE_V4_ASSET", - "BODY_BAKE_V4_ENDPOINT", - "BODY_BAKE_V4_BUCKET", - "BODY_BAKE_V4_ACCESS_KEY_ID", - "BODY_BAKE_V4_SECRET_ACCESS_KEY", - "BODY_BAKE_V4_REGION", - "BODY_BAKE_V4_DIR", - ] { - assert!(key.starts_with("BODY_BAKE_V4_"), "{key} is not v4-scoped"); + fn the_module_invents_no_credential_variable() { + // The defect this pins: an earlier version demanded BODY_BAKE_V4_ + // copies of the endpoint, bucket, key id and secret, so serving one + // artifact cost four duplicate secrets on a deploy that already had + // them. The object-store contract must be the deployment's existing + // AWS_* names; the ONLY names this module adds are v4's own + // coordinates and its cache directory. + let required: Vec<&str> = missing_vars_all(); + for k in &required { + assert!( + !(k.starts_with("BODY_BAKE_V4_") + && (k.contains("ENDPOINT") + || k.contains("BUCKET") + || k.contains("ACCESS_KEY") + || k.contains("SECRET") + || k.contains("REGION"))), + "{k} duplicates a credential the deployment already sets as AWS_*" + ); } + assert!( + required.contains(&"AWS_S3_BUCKET_NAME"), + "the bucket must come from the deployment's AWS_S3_BUCKET_NAME" + ); + assert!( + required.contains(&"BODY_BAKE_V4_TAG") && required.contains(&"BODY_BAKE_V4_ASSET"), + "the artifact coordinates must stay v4's own" + ); assert_eq!( - missing_vars().len(), + required.len(), 6, - "with nothing set, every required v4 variable must be reported missing" + "required inputs: 2 v4 coordinates + 4 AWS" ); } From a9cadc5013e849adf27cfea8a41f64c132b05a63 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 09:12:47 +0000 Subject: [PATCH 2/2] Normalise every object-store value before the client sees it CodeRabbit on #153: `env_var_nonempty` stripped quotes on the PRESENCE check while `AmazonS3Builder::from_env()` read the raw environment separately, so a quoted AWS_ENDPOINT_URL or credential passed missing_vars() and still reached the client with its quotes attached -- failing later as a bad endpoint or bad credential. The strip was decorative for exactly the values that matter. `s3_env` now resolves all five through one normalising read and build_store re-applies them after from_env(), which keeps anything else the deployment sets (a session token) while overriding what from_env parsed from the raw strings. The process environment is never mutated. AWS_REGION is accepted as an alias for AWS_DEFAULT_REGION, "auto" as the fallback -- the region is SigV4 credential scope, so it must be some value. Writing a test that could fail then exposed an ordering bug in the strip itself: trim ran BEFORE the quotes came off, so a value written `" x "` kept its padding once they were removed. Trim, unquote, trim. Both new tests fail with the strip disabled -- verified, not assumed. Re-verified end to end against the real bucket with only ambient AWS_*: hydrated + checksum-verified, GET /api/bake/v4 -> 200 streamed. The throwaway probe was deleted; q2/bakes/ is back to its original 7 entries. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012wrzeZAdwGYTCKoxamwQht --- crates/cockpit-server/src/body_bake_v4.rs | 154 +++++++++++++++++++++- 1 file changed, 151 insertions(+), 3 deletions(-) diff --git a/crates/cockpit-server/src/body_bake_v4.rs b/crates/cockpit-server/src/body_bake_v4.rs index a86ed6566..1d5a1f773 100644 --- a/crates/cockpit-server/src/body_bake_v4.rs +++ b/crates/cockpit-server/src/body_bake_v4.rs @@ -96,9 +96,20 @@ fn env_var_nonempty(key: &str) -> Option { // these containers arrive wrapped in literal `"` (documented for the token // vars in `medcare-rs`'s CLAUDE.md), and an unstripped value fails auth in a // way that reads as a bad credential rather than a quoting artifact. + // Trim, THEN unquote, THEN trim again. The second trim is not redundant: + // a value written as `" x "` has its padding INSIDE the quotes, so a + // single leading trim sees only the quote characters and leaves the spaces + // behind once they are removed. Caught by + // `every_object_store_value_is_unquoted_before_it_reaches_the_client`. std::env::var(key) .ok() - .map(|v| v.trim().trim_matches('"').trim_matches('\'').to_string()) + .map(|v| { + v.trim() + .trim_matches('"') + .trim_matches('\'') + .trim() + .to_string() + }) .filter(|v| !v.is_empty()) } @@ -152,8 +163,26 @@ fn coordinates() -> Option<(String, String)> { /// proven against this bucket, and a second spelling of the same handshake is a /// second thing to get wrong. fn build_store() -> Option { - let bucket = env_var_nonempty("AWS_S3_BUCKET_NAME")?; - match AmazonS3Builder::from_env().with_bucket_name(bucket).build() { + let cfg = s3_env()?; + // `from_env()` FIRST so anything else the deployment sets (a session token, + // say) is still picked up, then the values that matter re-applied from + // `s3_env` — a later setter overrides what `from_env` parsed. + // + // Re-applying them is not belt-and-braces: `from_env()` hands the RAW + // environment string to the builder, so a quoted `AWS_ENDPOINT_URL` or + // credential would pass [`missing_vars`] (which reads through + // `env_var_nonempty`) and still reach the client with its quotes attached, + // failing later as a bad endpoint or bad credential. Normalising in one + // place and one place only is what makes the strip real rather than + // decorative. The process environment itself is never mutated. + let builder = AmazonS3Builder::from_env() + .with_bucket_name(cfg.bucket) + .with_endpoint(cfg.endpoint) + .with_access_key_id(cfg.key_id) + .with_secret_access_key(cfg.secret) + .with_region(cfg.region); + + match builder.build() { Ok(s) => Some(s), Err(e) => { tracing::error!(error = %e, "{LABEL}: S3 client build failed"); @@ -162,6 +191,39 @@ fn build_store() -> Option { } } +/// The object-store settings, read once and normalised. +/// +/// Split out so the normalisation is testable without constructing a client: +/// the quoting defect this guards is invisible from the outside of +/// [`build_store`]. +struct S3Env { + endpoint: String, + bucket: String, + key_id: String, + secret: String, + region: String, +} + +/// Resolve every object-store value through [`env_var_nonempty`], so each one +/// is trimmed and unquoted. `None` when any required one is absent — the same +/// partial-config-is-no-config rule [`missing_vars`] reports on. +/// +/// `AWS_DEFAULT_REGION` is the documented name here; `AWS_REGION` is accepted +/// as the AWS-standard alias, and `"auto"` is the fallback these providers +/// expect — the region is part of the SigV4 credential scope, not an +/// addressing input, so it must be *some* value. +fn s3_env() -> Option { + Some(S3Env { + endpoint: env_var_nonempty("AWS_ENDPOINT_URL")?, + bucket: env_var_nonempty("AWS_S3_BUCKET_NAME")?, + key_id: env_var_nonempty("AWS_ACCESS_KEY_ID")?, + secret: env_var_nonempty("AWS_SECRET_ACCESS_KEY")?, + region: env_var_nonempty("AWS_DEFAULT_REGION") + .or_else(|| env_var_nonempty("AWS_REGION")) + .unwrap_or_else(|| "auto".to_string()), + }) +} + /// Which required v4 variables are absent, so one boot line settles it. fn missing_vars() -> Vec<&'static str> { required_vars() @@ -507,6 +569,92 @@ mod tests { ); } + #[test] + fn every_object_store_value_is_unquoted_before_it_reaches_the_client() { + // The defect this pins (CodeRabbit, #153): `env_var_nonempty` stripped + // quotes on the PRESENCE check while `AmazonS3Builder::from_env()` read + // the raw environment separately — so a quoted endpoint or credential + // passed `missing_vars()` and still reached the client quoted, failing + // later as a bad endpoint or bad credential. Remove the strip in + // `env_var_nonempty` and this fails on all five fields. + // + // SAFETY: single-threaded test; every variable is restored below. + let saved: Vec<(&str, Option)> = [ + "AWS_ENDPOINT_URL", + "AWS_S3_BUCKET_NAME", + "AWS_ACCESS_KEY_ID", + "AWS_SECRET_ACCESS_KEY", + "AWS_DEFAULT_REGION", + ] + .iter() + .map(|k| (*k, std::env::var(k).ok())) + .collect(); + + unsafe { + std::env::set_var("AWS_ENDPOINT_URL", "\"https://example.invalid\""); + std::env::set_var("AWS_S3_BUCKET_NAME", "\"a-bucket\""); + std::env::set_var("AWS_ACCESS_KEY_ID", "'a-key'"); + std::env::set_var("AWS_SECRET_ACCESS_KEY", "\" a-secret \""); + std::env::set_var("AWS_DEFAULT_REGION", "\"eu-central-1\""); + } + let cfg = s3_env().expect("all five are set"); + assert_eq!(cfg.endpoint, "https://example.invalid"); + assert_eq!(cfg.bucket, "a-bucket"); + assert_eq!(cfg.key_id, "a-key"); + assert_eq!(cfg.secret, "a-secret"); + assert_eq!(cfg.region, "eu-central-1"); + + // A missing required value is "no S3", never a half-built client. + unsafe { std::env::remove_var("AWS_SECRET_ACCESS_KEY") }; + assert!(s3_env().is_none(), "partial config must resolve to None"); + + for (k, v) in saved { + // SAFETY: same single-threaded restore. + unsafe { + match v { + Some(v) => std::env::set_var(k, v), + None => std::env::remove_var(k), + } + } + } + } + + #[test] + fn the_region_falls_back_without_inventing_an_endpoint() { + // SAFETY: single-threaded test; restored below. + let saved = ( + std::env::var("AWS_DEFAULT_REGION").ok(), + std::env::var("AWS_REGION").ok(), + ); + unsafe { + std::env::remove_var("AWS_DEFAULT_REGION"); + std::env::set_var("AWS_REGION", "\"us-east-1\""); + } + // The alias is honoured... + assert_eq!( + env_var_nonempty("AWS_DEFAULT_REGION") + .or_else(|| env_var_nonempty("AWS_REGION")) + .unwrap_or_else(|| "auto".to_string()), + "us-east-1" + ); + unsafe { std::env::remove_var("AWS_REGION") }; + // ...and with neither set, the scope still gets a value. + assert_eq!( + env_var_nonempty("AWS_DEFAULT_REGION") + .or_else(|| env_var_nonempty("AWS_REGION")) + .unwrap_or_else(|| "auto".to_string()), + "auto" + ); + unsafe { + if let Some(v) = saved.0 { + std::env::set_var("AWS_DEFAULT_REGION", v); + } + if let Some(v) = saved.1 { + std::env::set_var("AWS_REGION", v); + } + } + } + #[test] fn nothing_is_served_before_a_verified_hydrate() { assert!(verified_path().is_none());