Add native secret-store config resolution - #1036
Conversation
ChristianPavilonis
changed the title
|
@ChristianPavilonis to test it before merging into #1019 |
Unify Tinybird, DataDome, and S3 static credentials under the logical default secret store, resolve them during typed config loading, and remove request-time static secret reads. Honor Fastly logical-to-physical store mappings, preserve deserialize-only selector compatibility, redact runtime values, and document provisioning and migration behavior.
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Moves static app-config credentials from plaintext blob values to secret-store key
references resolved after envelope verification, and fixes the logical-to-physical
store mapping that broke the Fastly deployment. The core design is sound: integrity
verification genuinely precedes resolution, resolution is atomic (the blob is left
untouched on failure), the deploy/load validation split keeps value checks on the load
path where PartnerRegistry::from_config still fails closed, and the two end-to-end
payload tests cover both the all-credentials-resolve and inactive-feature-skip arms.
Four blocking items: resolution discards the one diagnostic that would explain a
mis-mapped store, the documented migration order opens a total outage window, the
Fastly Hooks::routes() path reads the store mapping from the wrong source, and the
EdgeZero dependency is pinned to an unmerged upstream commit.
3 of the inline comments below carry a one-click GitHub
suggestion— use
Commit suggestion (or Add suggestion to batch) to apply them as commits on
the PR branch. The remaining comments describe the fix in prose because the change
spans multiple files, needs a new import, or adds code outside the diff. No
suggestion in this review was scratch-verified — local runs were skipped for this
pass, so please re-run the matching checks after applying.
Blocking
🔧 wrench
- Secret-store resolution throws away every adapter's diagnostic — see inline at
crates/trusted-server-core/src/secret_resolution.rs:164 - Documented migration order opens a full outage window — see Cross-cutting below
Hooks::routes()reads the wrong source for the store mapping — see inline atcrates/trusted-server-adapter-fastly/src/app.rs:1261
❓ question
- EdgeZero pinned to an unmerged upstream PR — see Cross-cutting below
Non-blocking
♻️ refactor / 🤔 thinking / ⛏ nitpick / 🌱 seedling
- Required S3 secret references still have serde defaults — see inline at
crates/trusted-server-core/src/settings.rs:767 - Feature-enablement logic duplicated in three places — see inline at
crates/trusted-server-core/src/config_payload.rs:63 EchoSecretStoremakes resolution untestable — see inline atcrates/trusted-server-core/src/config_payload.rs:145expect()on the Tinybird token traps the Wasm guest — see inline atcrates/trusted-server-adapter-fastly/src/tinybird.rs:57- Deploy validation misses duplicate partner key names — see inline at
crates/trusted-server-core/src/ec/registry.rs:74 - New docs bullets lost their markdown hard breaks — see inline at
docs/guide/configuration.md:1620 partners = []is redundant and a footgun — see inline attrusted-server.example.toml:17- Two overlapping ways to express leaf optionality — see inline at
crates/trusted-server-core/src/secret_resolution.rs:64 - Spin's five declared secret variables read as a contract — see inline at
crates/trusted-server-adapter-spin/spin.toml:28
Cross-cutting / body-level findings
-
🔧 Documented migration order opens a full outage window —
docs/guide/configuration.md:60-72gives the order: populate store, replace values with key names,ts config validate+ts config push, then "restart/redeploy instances as needed."Step 3 lands the reference-bearing blob while the old binary is still serving. On Fastly each request reads the config store fresh, so from that instant every request runs
Ec::validate_passphrase— which requires at least 32 bytes onmaintoday (MIN_PASSPHRASE_LENGTH = 32,crates/trusted-server-core/src/settings.rs) — againstpassphrase = "ec_passphrase"(13 bytes). That yieldsshort_passphrase, config load fails, and the service returns its startup-error response for all traffic until the redeploy finishes.The reverse mismatch fails too: a new binary reading a plaintext blob resolves each plaintext secret as a key name. There is no safe intermediate state — the binary and the blob have to flip together, and the doc currently puts the break in the middle. Please correct the ordering and add an explicit warning that a mismatched binary/blob pair fails config load outright. The staged Fastly deployment cited in the PR description would not surface this, since no old binary is in play there.
-
❓ EdgeZero pinned to an unmerged upstream PR —
Cargo.toml:57-62moves all six edgezero crates from stable tagv0.0.4to git rev0d6ebf9b0250efa5f7031a93ec7b7f09f2c9bf34, a commit on the still-openstackpop/edgezero#344. Merging this putsmainon a branch commit of an unmerged PR: if #344 is rebased or force-pushed before it merges, that commit can become unreachable andmainstops building.You disclosed this in the PR description, and the issue comment suggests this lands in #1019 first, so this may already be handled. It still needs an explicit answer because it constrains
main: hold this PR until #344 merges and re-pin to a tag, or is a rev pin onmainacceptable here? -
📝 CI coverage gap on the reviewed head — only
Analyze (javascript-typescript)ran on1315cdb1. The full gate suite (cargo fmt/test/clippy, all four adapters, cross-adapter parity, vitest, format-docs, integration and browser tests) last ran green on the merge commit598f7100, three commits earlier. That leaves070397f1 Resolve static credentials through typed config,b1e967e3, and1315cdb1without Rust, adapter, or lint coverage. Worth re-triggering the suite on the current head before merge, independent of the findings above. -
👍
validation_error_summaryis a real leak fix —crates/trusted-server-core/src/settings.rs:2387-2424walksValidationErrorsemitting onlypath: code, nevervalidator'sparams, which hold the offending value. The previous code formattedValidationErrorswholesale into a config error message. -
👍 Deleting
S3_CREDENTIALS_CACHEremoves a genuinely bad structure —crates/trusted-server-core/src/proxy.rspreviously kept a process-globalHashMapkeyed on the plaintext secret access key, with unbounded growth and a poisoning-proneMutex. Startup-resolved values are strictly better. -
👍
IntegrationSettings's customDebugcloses the DataDome-key leak that the flattenedJsonValuemap would otherwise print. -
👍 The two payload resolution tests are the right pair —
resolves_all_static_credentials_from_the_mapped_default_storeproves every path arm resolves through a mapped physical store, andinactive_optional_features_do_not_resolve_stale_secret_referencesproves disabled features do not demand stale references. Also good: droppinginclude_str!("trusted-server.example.toml")from the Spin and Cloudflare startup paths in favour of a hard error.
CI Status
- Analyze (javascript-typescript): PASS
- cargo fmt: not run on this head (PASS on
598f7100) - cargo test: not run on this head (PASS on
598f7100) - cargo test (axum native): not run on this head (PASS on
598f7100) - cargo test (cross-adapter parity): not run on this head (PASS on
598f7100) - cargo test (ts CLI, native): not run on this head (PASS on
598f7100) - cargo check (cloudflare native + wasm32-unknown-unknown): not run on this head (PASS on
598f7100) - cargo check/build/test (spin native + wasm32-wasip1): not run on this head (PASS on
598f7100) - integration tests: not run on this head (PASS on
598f7100) - integration tests (Fastly EC lifecycle): not run on this head (PASS on
598f7100) - browser integration tests: not run on this head (PASS on
598f7100) - prepare integration artifacts: not run on this head (PASS on
598f7100) - vitest: not run on this head (PASS on
598f7100) - format-typescript: not run on this head (PASS on
598f7100) - format-docs: not run on this head (PASS on
598f7100) - Analyze (rust): not run on this head (PASS on
598f7100) - Analyze (actions): not run on this head (PASS on
598f7100) - CodeQL: not run on this head (PASS on
598f7100)
No check reported a fail or cancel bucket. Branch protection reported no required checks for this PR.
| let resolved = secret_store | ||
| .get_string(default_store_name, &key_name) | ||
| .map_err(|_| { | ||
| configuration_error(format!( | ||
| "failed to resolve secret reference at `{leaf_path}`" | ||
| )) | ||
| })?; |
There was a problem hiding this comment.
🔧 wrench — Resolution discards the underlying PlatformError, so the one diagnostic that would explain a mis-mapped store is destroyed.
.map_err(|_| ...) drops the Report<PlatformError> entirely. Every adapter attaches something useful to it — Axum attaches env var 'TRUSTED_SERVER_SECRET_..._X' not set — export it to supply this secret value, and the Fastly, Cloudflare, and Spin adapters attach their own equivalents — and all of it is thrown away here. The message also never names the store that was actually opened.
That is exactly the gap that made this PR's motivating failure hard to diagnose: a valid secret existed in Fastly, but the logical store name was opened instead of the mapped physical one. With this code, that scenario produces failed to resolve secret reference at 'publisher.proxy_secret' and nothing else — no store name, no platform cause. It compounds with the Hooks::routes() finding, and with the pinned env_config_from_runtime_dictionary, which only warns and returns an empty EnvConfig when edgezero_runtime_env is absent, silently falling back to the logical store name.
Store names and key names are already stored in plaintext in the config blob, so surfacing both is not a disclosure.
| let resolved = secret_store | |
| .get_string(default_store_name, &key_name) | |
| .map_err(|_| { | |
| configuration_error(format!( | |
| "failed to resolve secret reference at `{leaf_path}`" | |
| )) | |
| })?; | |
| let resolved = secret_store | |
| .get_string(default_store_name, &key_name) | |
| .map_err(|error| { | |
| configuration_error(format!( | |
| "failed to resolve secret reference at `{leaf_path}` from secret store `{}`", | |
| default_store_name.as_ref() | |
| )) | |
| .attach(error.to_string()) | |
| })?; |
(not scratch-verified — local test/lint runs were skipped for this pass; please re-run the matching cargo / docs checks after applying)
| fn routes() -> RouterService { | ||
| Self::router_with_state().0 | ||
| let stores = RuntimeStoreConfig::from_env(&EnvConfig::from_env()); | ||
| Self::router_with_state(&stores).0 | ||
| } |
There was a problem hiding this comment.
🔧 wrench — This reads the store mapping from a source that is empty on deployed Fastly Compute, reintroducing the bug this PR fixes.
main.rs:89 correctly uses env_config_from_runtime_dictionary(TrustedServerApp::stores()). This path uses EnvConfig::from_env() instead. On deployed Fastly Compute std::env holds only build-time variables; the logical-to-physical mapping lives solely in the edgezero_runtime_env config store. So any path that reaches routes() resolves secrets to the logical trusted_server_secrets — precisely the deployment failure this PR set out to fix.
This is latent today, because main.rs builds the App itself via build_app_with_state and never calls routes(). But it is a trap for the next edgezero bump: nothing in the code or a test pins the assumption that routes() is dead on the Fastly path, and the failure mode is silent (a warn log plus a resolution error that, per the secret_resolution.rs finding, does not name the store).
Proposed fix (apply manually — it needs a new use at the top of the file, so it spans two hunks and cannot be expressed as a single suggestion):
// add near the other edgezero_adapter_fastly imports:
use edgezero_adapter_fastly::env_config_from_runtime_dictionary;
fn routes() -> RouterService {
let runtime_env = env_config_from_runtime_dictionary(Self::stores());
let stores = RuntimeStoreConfig::from_env(&runtime_env);
Self::router_with_state(&stores).0
}If routes() is genuinely unreachable on this adapter, the alternative is to make that explicit rather than leave a working-but-wrong implementation in place.
| /// Secret reference containing the `AWS` access key ID. | ||
| #[serde(default = "default_s3_access_key_id")] | ||
| pub access_key_id: String, | ||
| /// Secret name containing the `AWS` secret access key. | ||
| pub access_key_id: Redacted<String>, | ||
| /// Secret reference containing the `AWS` secret access key. | ||
| #[serde(default = "default_s3_secret_access_key")] | ||
| pub secret_access_key: String, | ||
| /// Optional secret name containing an `AWS` session token. | ||
| pub secret_access_key: Redacted<String>, |
There was a problem hiding this comment.
♻️ refactor — These two fields are marked required in secret_fields() but still carry serde defaults, so omitting them silently references the wrong store keys instead of failing.
AppConfigMeta::secret_fields() lists proxy.asset_routes[*].auth.access_key_id and secret_access_key with optional: false, yet both keep #[serde(default = ...)] returning the literal strings "access_key_id" and "secret_access_key". Two consequences:
- An operator who defines an S3 asset route and omits the credential keys gets a silent reference to store keys literally named
access_key_id/secret_access_keyrather than a config error. If keys with those names exist in the shared store for any reason, the route signs requests with the wrong credentials. - The
optional: falsemetadata can never fire its missing-field path, because serde always fills a value before resolution runs.
Before this PR the same defaults were scoped to a per-route secret_store ("s3-auth"), so a name collision was contained to one route. Now every route resolves from one logical store, so the collision surface is global. The docs were also updated to s3_access_key_id / s3_secret_access_key in both examples, which now contradicts the defaults still shipped in code.
Proposed fix (apply manually — spans three files, so it cannot be a single-file suggestion):
/// Secret reference containing the `AWS` access key ID.
pub access_key_id: Redacted<String>,
/// Secret reference containing the `AWS` secret access key.
pub secret_access_key: Redacted<String>,Then delete the now-unused default_s3_access_key_id and default_s3_secret_access_key functions (around settings.rs:665-671), and set Required=Yes with no default for both rows in the docs/guide/configuration.md S3 auth table and the docs/guide/asset-routes.md "Secret store values" table.
| fn remove_inactive_secret_references(data: &mut serde_json::Value) { | ||
| if data | ||
| .pointer("/tinybird/enabled") | ||
| .and_then(serde_json::Value::as_bool) | ||
| != Some(true) | ||
| && let Some(tinybird) = data | ||
| .get_mut("tinybird") | ||
| .and_then(serde_json::Value::as_object_mut) | ||
| { | ||
| tinybird.remove("auction_token_secret"); | ||
| tinybird.remove("access_token_secret"); | ||
| } | ||
|
|
||
| let Some(datadome) = data | ||
| .pointer_mut("/integrations/datadome") | ||
| .and_then(serde_json::Value::as_object_mut) | ||
| else { | ||
| return; | ||
| }; | ||
| let integration_enabled = | ||
| datadome.get("enabled").and_then(serde_json::Value::as_bool) != Some(false); | ||
| let protection_enabled = integration_enabled | ||
| && datadome | ||
| .get("enable_protection") | ||
| .and_then(serde_json::Value::as_bool) | ||
| == Some(true); | ||
| if !protection_enabled { | ||
| datadome.remove("server_side_key_secret_name"); | ||
| } | ||
|
|
||
| let bypass_enabled = protection_enabled | ||
| && datadome | ||
| .get("protection_test_bypass") | ||
| .and_then(serde_json::Value::as_object) | ||
| .and_then(|bypass| bypass.get("enabled")) | ||
| .and_then(serde_json::Value::as_bool) | ||
| == Some(true); | ||
| if !bypass_enabled | ||
| && let Some(bypass) = datadome | ||
| .get_mut("protection_test_bypass") | ||
| .and_then(serde_json::Value::as_object_mut) | ||
| { | ||
| bypass.remove("credential_secret_name"); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
♻️ refactor — Per-feature enablement knowledge is now encoded in three places that must stay in agreement.
remove_inactive_secret_references hardcodes which features are active (tinybird, datadome, and the nested protection bypass). That same knowledge already exists declaratively in secret_fields()'s optional flag and again in validate_secret_key_references's conditional branches. A future conditional secret that gets a secret_fields() entry but no matching removal rule here will fail startup for every operator who has that feature disabled — the reference is present in their blob, so resolution demands a secret they never provisioned.
Two signs of drift already present:
- It strips
tinybird.access_token_secret, whichsecret_fields()never lists, so that removal is dead code. - The datadome polarity is
get("enabled") != Some(false)(absent means enabled) while the tinybird polarity three lines earlier ispointer("/tinybird/enabled") != Some(true)(absent means disabled). Both happen to be correct for their respective defaults, but the inversion invites a mistake in the next addition.
Best fix is to carry the condition in the metadata so there is one source of truth. Failing that, a test asserting that every conditionally-required entry in secret_fields() has a corresponding removal rule would catch the drift at build time rather than in an operator's startup logs.
| struct EchoSecretStore; | ||
|
|
||
| impl PlatformSecretStore for EchoSecretStore { | ||
| fn get_bytes( | ||
| &self, | ||
| _store_name: &StoreName, | ||
| key: &str, | ||
| ) -> Result<Vec<u8>, Report<PlatformError>> { | ||
| let value = match key { | ||
| "placeholder_proxy" => "change-me-proxy-secret", | ||
| "unit-test-proxy-secret" => "unit-test-proxy-secret-32-bytes-ok", | ||
| _ => key, | ||
| }; | ||
| Ok(value.as_bytes().to_vec()) |
There was a problem hiding this comment.
♻️ refactor — This test double makes secret resolution unverifiable, because it returns each key as its own value.
get_bytes falls through to _ => key, so a resolved value is byte-identical to the unresolved key name for every key except the two special-cased ones. "Resolution ran" is therefore indistinguishable from "resolution was skipped." Concretely: delete the resolve_secret_references call from settings_from_config_blob and every test routed through load_settings still passes — only resolves_all_static_credentials_from_the_mapped_default_store, which uses UnifiedSecretStore, catches the regression. The same identity-map pattern is in crates/trusted-server-core/src/settings_data.rs around line 226.
Proposed fix (apply manually — the change is a one-line default plus new assertions in several tests, which reach outside this hunk):
let value = match key {
"placeholder_proxy" => "change-me-proxy-secret".to_owned(),
"unit-test-proxy-secret" => "unit-test-proxy-secret-32-bytes-ok".to_owned(),
other => format!("resolved-{other}"),
};
Ok(value.into_bytes())Then assert on the resolved- prefix in the round-trip tests so a skipped resolution fails loudly.
| pub fn validate_config_for_deploy( | ||
| partners: &[EcPartner], | ||
| ) -> Result<(), Report<TrustedServerError>> { | ||
| let mut source_domains = HashMap::with_capacity(partners.len()); | ||
|
|
||
| for partner in partners { | ||
| let normalized_source = normalize_partner_source_domain(&partner.source_domain) | ||
| .map_err(|msg| { | ||
| Report::new(TrustedServerError::Configuration { | ||
| message: format!("ec.partners: {msg}"), | ||
| }) | ||
| })?; | ||
|
|
||
| if source_domains | ||
| .insert(normalized_source.clone(), ()) | ||
| .is_some() | ||
| { | ||
| return Err(Report::new(TrustedServerError::Configuration { | ||
| message: format!("ec.partners: duplicate source_domain '{normalized_source}'"), | ||
| })); | ||
| } | ||
|
|
||
| validate_rate_limits_values(partner.batch_rate_limit, partner.pull_sync_rate_limit) | ||
| .map_err(|error| { | ||
| Report::new(TrustedServerError::Configuration { | ||
| message: format!( | ||
| "ec.partners: invalid rate limits for '{normalized_source}': {error}" | ||
| ), | ||
| }) | ||
| })?; | ||
|
|
||
| if partner.pull_sync_enabled { | ||
| validate_pull_sync_fields( | ||
| partner.pull_sync_url.as_deref(), | ||
| &partner.pull_sync_allowed_domains, | ||
| partner | ||
| .ts_pull_token | ||
| .as_ref() | ||
| .map(|token| token.expose().as_str()), | ||
| false, | ||
| ) | ||
| .change_context(TrustedServerError::Configuration { | ||
| message: format!( | ||
| "ec.partners: pull sync config invalid for '{normalized_source}'" | ||
| ), | ||
| })?; | ||
| } | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
♻️ refactor — Splitting partner validation correctly dropped the token-hash collision check from the deploy path, but nothing replaced it with a key-name collision check.
Dropping the hash comparison here is right: at push time api_token holds a key name, so hashing it and comparing is meaningless. But the deploy path now has no check at all for two partners referencing the same secret key name. Those two partners resolve to identical tokens at startup, PartnerRegistry::from_config's by_api_key_hash check then fires, and config load fails — so the operator learns about a typo at deploy time instead of during ts config validate.
The same applies to ts_pull_token. Adding the check restores push-time feedback without inspecting any value:
let mut api_token_keys = HashMap::with_capacity(partners.len());
// ... inside the existing `for partner in partners` loop, after the
// duplicate source_domain check:
if api_token_keys
.insert(partner.api_token.expose().clone(), ())
.is_some()
{
return Err(Report::new(TrustedServerError::Configuration {
message: format!(
"ec.partners: source_domain '{normalized_source}' reuses an api_token secret key referenced by another partner"
),
}));
}Apply manually — this adds a binding above the loop as well as the check inside it, so it does not fit one contiguous suggestion range.
| ✅ Store values in the platform secret store | ||
| ✅ Rotate values deliberately and restart/redeploy instances | ||
| ✅ Generate values locally without printing them to logs | ||
| ✅ Use different values per environment when appropriate | ||
| ✅ Keep stable key names for rotation | ||
|
|
||
| **Don't**: | ||
| ❌ Commit secrets to version control | ||
| ❌ Commit secret values to version control | ||
| ❌ Put secret values in environment overlays | ||
| ❌ Put secret values in config diff output or app-config blobs | ||
| ❌ Treat missing secret-store keys as inline values |
There was a problem hiding this comment.
⛏ nitpick — These new bullets dropped the trailing two-space hard breaks, so both lists will render as run-together paragraphs.
Lines 1620-1624 and 1627-1630 end with no trailing whitespace, while the surviving pre-existing bullets at 1631-1633 still carry the two-space hard break. In Markdown that means the ✅ block renders as one paragraph, and the ❌ block runs the four new bullets together before finally breaking at line 1631. Prettier with proseWrap: preserve keeps hard breaks as-is and will not add the missing ones, which is consistent with the note in the PR description that cd docs && npm run format was not fully run.
The two trailing spaces at the end of each line below are the entire change — please confirm they survive when you commit the suggestion, since some editors strip trailing whitespace on save.
| ✅ Store values in the platform secret store | |
| ✅ Rotate values deliberately and restart/redeploy instances | |
| ✅ Generate values locally without printing them to logs | |
| ✅ Use different values per environment when appropriate | |
| ✅ Keep stable key names for rotation | |
| **Don't**: | |
| ❌ Commit secrets to version control | |
| ❌ Commit secret values to version control | |
| ❌ Put secret values in environment overlays | |
| ❌ Put secret values in config diff output or app-config blobs | |
| ❌ Treat missing secret-store keys as inline values | |
| ✅ Store values in the platform secret store | |
| ✅ Rotate values deliberately and restart/redeploy instances | |
| ✅ Generate values locally without printing them to logs | |
| ✅ Use different values per environment when appropriate | |
| ✅ Keep stable key names for rotation | |
| **Don't**: | |
| ❌ Commit secret values to version control | |
| ❌ Put secret values in environment overlays | |
| ❌ Put secret values in config diff output or app-config blobs | |
| ❌ Treat missing secret-store keys as inline values |
(not scratch-verified — local test/lint runs were skipped for this pass; please re-run the matching cargo / docs checks after applying)
| pull_sync_concurrency = 3 | ||
| # Keep this empty when no partners are configured. Replace this line with | ||
| # `[[ec.partners]]` entries when adding partners. | ||
| partners = [] |
There was a problem hiding this comment.
⛏ nitpick — partners = [] looks redundant, and the comment above it makes the line a footgun.
Ec::partners is declared #[serde(default, deserialize_with = "vec_from_seq_or_map")] with no skip_serializing_if, so serializing Settings always emits the array. The pushed blob therefore always carries ec.partners, and resolution's required Field("partners") segment cannot miss it — which appears to make this line unnecessary.
The comment is also self-defeating: TOML forbids defining both partners = [] and [[ec.partners]] in the same document, so anyone following the commented example below has to delete this line first, exactly as the comment warns them to.
Was this working around a failure you actually hit? If resolution did reject a blob for a missing ec.partners, the real fix is SecretPathSegment::OptionalField("partners") in secret_fields() rather than a required line in the starter config — and the same argument applies to handlers.
The suggestion below covers lines 17-20 (my triage note said 18-20; the replacement bytes are unchanged, but they only produce the intended result when the range starts at pull_sync_concurrency, otherwise line 17 would be duplicated):
| pull_sync_concurrency = 3 | |
| # Keep this empty when no partners are configured. Replace this line with | |
| # `[[ec.partners]]` entries when adding partners. | |
| partners = [] | |
| pull_sync_concurrency = 3 |
(not scratch-verified — local test/lint runs were skipped for this pass; please re-run the matching cargo / docs checks after applying)
| Some((SecretPathSegment::OptionalField(name), [])) => { | ||
| if matches!(node.get(name.as_ref()), None | Some(Value::Null)) { | ||
| return Ok(()); | ||
| } | ||
| resolve_leaf( | ||
| node, | ||
| field, | ||
| name.as_ref(), | ||
| rendered_path, | ||
| secret_store, | ||
| default_store_name, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🌱 seedling — Leaf optionality is now expressible two different ways, and only one of them is used.
This arm handles OptionalField as the final path segment, but every leaf in TrustedServerAppConfig::secret_fields() is a plain Field; optionality at the leaf is carried by SecretField.optional and handled in resolve_leaf. So this arm is unreachable for the real config today.
Two mechanisms for the same property invites a future contributor to set OptionalField at the leaf and optional: false, or the reverse, and get precedence that reads as a bug. Worth either removing the arm or adding a short comment stating that leaf optionality is expressed through SecretField.optional and that OptionalField is for intermediate segments only.
| # Trusted Server app-config secret references. Replace the empty defaults with | ||
| # values supplied by the deployment's secret provider; never commit values here. | ||
| v_trusted_x5fserver_x5fsecrets_v_publisher_x5fproxy_x5fsecret = { default = "", secret = true } | ||
| v_trusted_x5fserver_x5fsecrets_v_ec_x5fpassphrase = { default = "", secret = true } | ||
| v_trusted_x5fserver_x5fsecrets_v_partner_x5fapi_x5ftoken = { default = "", secret = true } |
There was a problem hiding this comment.
🌱 seedling — These five variable names read as a fixed contract, but they are really just the example config's key names.
Spin cannot declare variables dynamically, so the set committed here works only for an operator whose config uses exactly these key names. Anyone who picks different names, configures a second partner, or configures a second password-protected handler has to hand-edit this file and re-derive the _x5f encoding. The configuration.md migration section does point at the encoder documented here, which is the right pointer.
Worth stating explicitly in the comment that these five correspond to the key names in trusted-server.example.toml and must be regenerated per deployment, rather than leaving them to read as the complete set. Related: docs/guide/configuration.md documents handler passwords under admin_password and api_handler_password, which do not match the single handler_password declared here — a reader comparing the two files gets contradictory guidance.
1315cdb to
3e2b3d2
Compare
3 participants
Summary
trusted_server_secretsas the logical store name while allowing adapters to map it to a physical store such as Fastly'sts_secrets. Missing or invalid secrets fail configuration loading without exposing their values.secret_storeselectors for one release, warn that they are ignored, and omit them when serializing configuration.This fixes the deployment failure where a valid secret existed in Fastly but Trusted Server opened the logical store name instead of the mapped physical store.
Changes
.env.dev.env.exampleCargo.tomlCargo.lockcrates/trusted-server-adapter-axum/src/app.rscrates/trusted-server-adapter-cloudflare/src/app.rscrates/trusted-server-adapter-cloudflare/src/lib.rscrates/trusted-server-adapter-cloudflare/src/platform.rscrates/trusted-server-adapter-cloudflare/wrangler.ci.tomlcrates/trusted-server-adapter-cloudflare/wrangler.tomlcrates/trusted-server-adapter-fastly/src/app.rscrates/trusted-server-adapter-fastly/src/main.rscrates/trusted-server-adapter-fastly/src/tinybird.rscrates/trusted-server-adapter-spin/spin.tomlcrates/trusted-server-adapter-spin/src/app.rscrates/trusted-server-adapter-spin/src/platform.rscrates/trusted-server-core/src/config.rscrates/trusted-server-core/src/config_payload.rscrates/trusted-server-core/src/ec/registry.rscrates/trusted-server-core/src/integrations/datadome.rscrates/trusted-server-core/src/integrations/datadome/protection.rscrates/trusted-server-core/src/lib.rscrates/trusted-server-core/src/proxy.rscrates/trusted-server-core/src/publisher.rscrates/trusted-server-core/src/secret_resolution.rscrates/trusted-server-core/src/settings.rscrates/trusted-server-core/src/settings_data.rscrates/trusted-server-integration-tests/Cargo.tomlcrates/trusted-server-integration-tests/fixtures/configs/trusted-server.integration.tomlcrates/trusted-server-integration-tests/fixtures/configs/viceroy-template.tomlcrates/trusted-server-integration-tests/src/bin/generate-viceroy-config.rscrates/trusted-server-integration-tests/tests/common/config.rscrates/trusted-server-integration-tests/tests/environments/axum.rsdocs/guide/asset-routes.mddocs/guide/configuration.mddocs/guide/fastly.mdtrusted_server_secretsto physicalts_secretsmapping and provisioning requirements.docs/guide/getting-started.mddocs/guide/integrations/datadome.mdfastly.tomltrusted-server.example.tomlScope
This PR touches the core schema, each adapter startup path, integration fixtures, and operator documentation because secret references must behave the same on Fastly, Axum, Cloudflare, and Spin. The request-signing key collection, rotation stores, and Fastly management credentials remain outside this change because those stores are managed at runtime rather than loaded as static application configuration.
EdgeZero dependency
This PR depends on stackpop/edgezero#344, "Support optional typed secret paths and Fastly store mappings." That PR adds optional intermediate path handling and persists validated logical-to-physical store mappings during Fastly provisioning and staged deployment. Trusted Server pins its tested commit,
0d6ebf9b0250efa5f7031a93ec7b7f09f2c9bf34. All checks on the EdgeZero PR pass.Closes
Closes #684
Test plan
cargo test-fastly && cargo test-axumcargo clippy-fastly && cargo clippy-axumcargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest run, no JS source changedcd crates/trusted-server-js/lib && npm run format, no JS source changedcd docs && npm run format, changed documentation passes targeted formatting; the full command has unrelated existing failuresfastly compute servecargo test-cloudflare,cargo test-spin, adapter parity tests, CLI tests, Cloudflare and Spin WASM checks, all adapter-specific Clippy targets, andgit diff --check/health; a settings-load probe found no secret-resolution or application-state errorsChecklist
unwrap()in production code, useexpect("should ...")