Skip to content

Support implicit credential lookup plans and shared Fastly secret operations - #444

Open
ChristianPavilonis wants to merge 10 commits into
mainfrom
feature/implicit-secret-lookup-plan
Open

ChristianPavilonis wants to merge 10 commits into
mainfrom
feature/implicit-secret-lookup-plan

Conversation

@ChristianPavilonis

@ChristianPavilonis ChristianPavilonis commented Oct 26, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Let typed applications compile active credential lookups without retaining scalar secret-reference leaves.
  • Apply the same lookups and application-native rendering in typed config validate, push and diff. Applications that do not opt in keep the existing behavior.
  • Share existing deployment resolution and secure Fastly create/replace operations with application CLIs.

Companion prerequisite for IABTechLab/trusted-server#1037. This does not implement or prove that project's runtime secret-scoping guarantee.

Changes

Crate / file Change
edgezero-core/app_config Default-compatible implicit lookup and native renderer hooks. Renderer receives effective adapter manifest defaults plus parent environment.
edgezero-cli/config Both typed walkers consume the compiled plan, validate descriptor/path/store/key metadata and declarations, deduplicate identical requests, and reject native collisions. Public effective_manifest_environment helper.
edgezero-adapter / Axum / Spin Optional exact native names retain adapter-native validation instead of validating obsolete raw labels. Existing Spin component resolver is public and reexported by edgezero-cli.
edgezero-adapter-fastly Read-only service/store resolution using existing checked selectors and complete inventories; presence-only metadata listing; explicit POST create / PATCH replace using existing stdin-only curl transport. Raw bytes are Base64-encoded in memory.
edgezero-cli/fastly_secrets Shared context_from_manifest builder and narrow host helper reexports.
tests Real typed validate/push/diff, feature-only serialization, defaults, inactive cleanup, missing metadata/stores, repeated requests, native collisions, manifest environment parity, fictional provider operations and secure subprocess transport.

No deploy descriptors, registry, raw-key fallback, release tags, merging, or production provider calls.

API

AppConfigMeta::implicit_secret_lookups(&self) -> Result<Option<Vec<ImplicitSecretLookup>>, String>

Each lookup has descriptor_id, field_path, store_id, and key. None retains the old leaf walk; Some(vec![]) means no active credentials. The application compiler owns activation and complete descriptor coverage.

AppConfigMeta::render_implicit_secret_name(&self, adapter, lookup, environment: &EnvConfig) -> Result<String, String>

The application supplies its runtime bridge's native name, not EdgeZero's generic mapping. Plans are public metadata, not resolved credentials or authorization.

Host helpers are under edgezero_cli::fastly_secrets: context_from_manifest, resolve_resources, secret_exists, write_secret, injectable _with_transport variants, comparable resource metadata, and FASTLY_SECRET_MAX_BYTES. Explicit create/replace methods preserve the reviewed action on races. Errors do not expose provider response bodies or stderr, writes are not retried, and unconfirmed writes report uncertainty.

Review follow-up

Addressed review feedback in 04d58ee4: unified test environment locking; made negative tests diagnostic-specific; corrected full-purpose lookup deduplication; added the lookup constructor and adapter namespace hook; reused deploy resource/context resolution; improved safe diagnostics and explicit HTTP rejection handling; documented opt-in and host-helper contracts.

Implicit plans continue replacing legacy CLI leaf checks. Full Fastly resource resolution still requires all declared stores. The combined plan trait redesign, derive forwarding and batch secret listing are deferred. No guessed Cloudflare identifier rule was added after primary-source inspection and a local workerd binding probe.

Test plan

Rust 1.98.1 and Node.js 24.20.0 on Linux x86_64. Two Cargo jobs, dev/test debug info disabled. All results below cover 04d58ee4. Temporary mutation probes were restored to exact verified SHA-256 file fingerprints before committing.

  • cargo test --workspace --all-targets -- --test-threads=1: 1,582 tests passed. Serial workspace execution avoids the separately reported pre-existing CLI environment race.
  • Fastly cargo test -p edgezero-adapter-fastly --features cli --lib: 10 consecutive parallel runs passed, 337 tests each.
  • CLI library tests: 237 passed, including context parity, redaction and typed preflight.
  • Disabled each of the five reviewed guards and separately omitted push preflight: all six mutations were detected by targeted failing regressions. Restored sources and reran focused tests successfully.
  • cargo clippy --workspace --all-targets --all-features -- -D warnings
  • cargo fmt --all -- --check and git diff --check
  • cargo check --workspace --all-targets --features "fastly cloudflare spin"
  • Spin wasm32-wasip2, Fastly wasm32-wasip1 and Cloudflare wasm32-unknown-unknown adapter checks with their corresponding runtime features.
  • Separate app-demo workspace/all-target tests: 33 passed.
  • Docs npm ci, format, lint and VitePress build.
  • Local Miniflare/workerd probe delivered all eight string-named bindings unchanged, including digit-leading, whitespace, punctuation and Unicode names. This is not a production secret API rehearsal.
  • Deployed native-provider rehearsals and the consuming application's runtime authorization proof.
  • Hosted CI on the updated head, pending after push.

Unverified and blocked

The provider API documentation specifies POST create, PATCH replacement and a raw value limit written as "64KB". The exact 64,000 versus 65,536-byte provider boundary remains unverified. Host tests verify request construction and subprocess argv/stdin handling with fictional inputs, not real provider acceptance, replacement propagation, read-through-runtime byte preservation, native declaration limits, or config-writer/secret-writer IAM separation. No real credentials or provider mutations were used.

The consuming application's actual renderer/CLI integration and four-adapter runtime proof belong to the companion change. Successful storage never implies a deploy, resource link, activation, retained-state refresh or immediate fleet-wide rotation.

Checklist

  • No Tokio dependencies added
  • Existing default behavior preserved and exercised
  • New behavior has executable host tests
  • No credentials committed
  • No merging or releases

@ChristianPavilonis
ChristianPavilonis marked this pull request as ready for review November 2, 2026 20:03

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review

Summary

Adds application-compiled implicit secret lookup plans, with native-name rendering and collision checks across typed config validate / push / diff, plus shared host-side Fastly secret operations. I found no blocking issues. Two behaviours are worth confirming (inline), plus a few nits.

😃 Praise

  • The credential handling is careful. The token and secret value reach curl only through the stdin config. Provider bodies and stderr are kept out of errors. Writes use explicit POST (create) or PATCH (replace) and are never retried. A per-key 404 isn't taken as proof of absence, and repeated pagination cursors are caught. The fake-curl subprocess test shows the token stays off argv and leaked stderr is stripped from errors.

Findings

Non-blocking

  • 🤔 Shared store/key across descriptors is always rejected, with misleading messages (crates/edgezero-cli/src/config.rs:1991, :2006). Inline.
  • 🤔 Opting in skips all legacy #[secret] leaf checks (crates/edgezero-cli/src/config.rs:1846). Inline.
  • 🌱 No derive-level opt-in. #[derive(AppConfig)] writes the whole AppConfigMeta impl, so opting in means hand-writing the root impl and losing the derive's compile-time rejections (e.g. rename_all). A derive attribute that forwards to app-supplied functions could come later.
  • ⛏ Stray \' in an untouched doc comment (crates/edgezero-core/src/app_config.rs:141). Inline.
  • ⛏ Read-only Fastly calls report "mutation outcome may be unknown" (crates/edgezero-adapter-fastly/src/secret_operations.rs:246). Inline.
  • ⛏ #[path = "secret_operations.rs"] pub mod secrets name mismatch (crates/edgezero-adapter-fastly/src/cli.rs:7). Inline.
  • 📝 The test-plan section reports gates at 5ad74df2 / 4f645b1e. The head now has two more commits (c24cd994, 47784581). Hosted CI and the local run below cover the current head, so this only needs a description refresh.

CI Status (reviewed head 47784581, local Rust 1.98.1 per .tool-versions)

  • fmt: PASS
  • clippy (--workspace --all-targets --all-features -D warnings): PASS
  • tests (--workspace --all-targets): PASS (1,565 tests)
  • check (--features "fastly cloudflare spin"): PASS
  • Hosted CI: all 28 checks pass, including the WASM jobs.

Not covered here: whether Fastly really accepts these API calls, and the companion application integration (both noted as unverified in the PR).

Comment thread crates/edgezero-cli/src/config.rs Resolved
Comment thread crates/edgezero-cli/src/config.rs Resolved
Comment thread crates/edgezero-core/src/app_config.rs Resolved

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review

Summary

Adds application-compiled implicit secret lookup plans, applied by typed config validate / push / diff, plus shared host-side Fastly secret operations. Requesting changes for two items. The new Fastly transport test races the rest of the crate's env-mutating tests and can run the real fastly CLI. Five of the new guards can be deleted with every test still passing.

Findings

Blocking

  • 🔧 New fake-curl test uses a different env mutex than the rest of the Fastly test binary: reproduced 2 of 18 failing runs (0 of 12 with the test skipped), including the real fastly compute deploy running from a unit test (crates/edgezero-adapter-fastly/src/secret_operations.rs:626)
  • 🔧 Five new guards are untested, and the push arm of the all-consumers test can't fail: verified by deleting each guard (crates/edgezero-cli/src/config.rs:5708)

Non-blocking

  • ♻️ Fastly special case in the adapter-agnostic CLI: move it to an adapter hook (crates/edgezero-cli/src/config.rs:1983)
  • 🤔 Cloudflare native names are never validated (crates/edgezero-cli/src/config.rs:2016)
  • ♻️ Resource resolution re-implements deploy and skips from_inventories (crates/edgezero-adapter-fastly/src/secret_operations.rs:155)
  • 🤔 Secret operations require all declared Config/KV stores to exist (crates/edgezero-adapter-fastly/src/secret_operations.rs:168)
  • 🤔 Errors drop store identifiers that aren't secret (crates/edgezero-adapter-fastly/src/secret_operations.rs:193)
  • 🤔 A definite 4xx rejection is reported as an unknown outcome (crates/edgezero-adapter-fastly/src/secret_operations.rs:371)
  • ♻️ ImplicitSecretLookup API shape: #[non_exhaustive], three-state return (crates/edgezero-core/src/app_config.rs:94)
  • ♻️ context_from_manifest duplicates run_deploy's context builder (crates/edgezero-cli/src/fastly_secrets.rs:26)
  • 🌱 secret_exists lists the whole store per key (crates/edgezero-adapter-fastly/src/secret_operations.rs:266)
  • ⛏ Glob re-export widens the public API (crates/edgezero-cli/src/fastly_secrets.rs:4)
  • ⛏ Axum native names may start with a digit (crates/edgezero-adapter-axum/src/cli.rs:366)
  • ⛏ base64 dependency ordering (Cargo.toml:46)
  • 🌱 Docs: docs/guide/configuration.md documents #[secret] and hand-written AppConfigMeta, but not the opt-in hooks (implicit_secret_lookups / render_implicit_secret_name) or the edgezero_cli::fastly_secrets helpers. A short section would help the companion's reviewers, and later adopters, find the contract.

The two existing 🤔 threads from prk-Jr (descriptors sharing a credential; opting in skips the legacy #[secret] checks) are still open, and I agree with both.

📌 Out of Scope

  • push_typed_honours_key_override (crates/edgezero-cli/src/config.rs:4721) runs a push without manifest_guard(), while guarded tests such as config.rs:2601 set EDGEZERO__STORES__CONFIG__APP_CONFIG__NAME="". This already exists on main and isn't touched by this PR, but it failed 3 times during these runs. Worth a separate issue.

CI Status (reviewed head 47784581, local Rust 1.98.1)

  • fmt: PASS
  • clippy (--workspace --all-targets --all-features -D warnings): PASS
  • tests (--workspace --all-targets): PASS (1,565). The edgezero-adapter-fastly lib suite fails intermittently (2 of 18 runs) because of the first blocking finding.
  • check (--features "fastly cloudflare spin"): PASS
  • WASM checks (spin wasm32-wasip2, fastly wasm32-wasip1, core and cloudflare wasm32-unknown-unknown): PASS
  • Hosted CI: 28/28 checks pass

Comment thread crates/edgezero-cli/src/config.rs Resolved
Comment thread crates/edgezero-cli/src/config.rs Resolved
Comment thread crates/edgezero-cli/src/config.rs Resolved
Comment thread crates/edgezero-adapter-axum/src/cli.rs Resolved
Comment thread Cargo.toml Resolved
@ChristianPavilonis

Copy link
Copy Markdown
Collaborator Author

Addressed the approved review recommendations in 04d58ee and refreshed the PR description with checks for that head. Each inline request has its own detailed reply.

Summary-only points:

  • The configuration guide now documents both opt-in hooks, constructor metadata, complete compiler coverage, replacement of legacy CLI checks, native rendering/collisions, handwritten-impl limitations, and shared Fastly host helpers.
  • Derive forwarding remains deferred to keep this prerequisite focused. The larger combined plan/renderer trait and batch secret-name API are also deferred, with the reasons in the relevant replies.
  • The stale validation record is replaced. Workspace tests passed 1,582; the Fastly library passed ten parallel repetitions of 337 tests; six targeted guard/preflight mutations were detected. All-feature Clippy, formatting, feature compilation, all three adapter WASM checks, separate demo tests and docs checks passed.
  • The reported unguarded CLI test on main is not changed here. Workspace tests were serial; Fastly locking was independently exercised in parallel.
  • No production provider mutations or credentials were used. Production acceptance/propagation/IAM and companion runtime authorization remain unverified. Local workerd string-binding evidence does not establish production secret API acceptance, and the exact provider meaning of 64KB remains unresolved.

Re-requesting review from both reviewers after resolving the handled threads. No merge or release.

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review

Summary

Follow-up review of 217983c0. The changes in 04d58ee4 address both blocking findings from the previous round and the non-blocking threads, either with a fix or with a documented, reasoned decision. The merge from main (217983c0) only touches CI/scripts. I found no blocking issues, so approving. One nitpick and one seed are inline.

😃 Praise

  • The credential handling survived the refactor intact. The token and secret value reach curl only through the stdin config. Path inputs are validated or percent-encoded, and redirects and curlrc are disabled. Provider bodies and stderr never reach errors, and writes are never retried. Explicit POST/PATCH keeps create vs replace safe against races.

Confirmed fixes

  • Shared env lock: path_mutation_guard() now returns env_lock(), with a pointer-identity test. The Fastly CLI lib suite passed 5 consecutive parallel runs (337 tests each).
  • Guard coverage: the push arm now sets yes = true and every consumer asserts the specific diagnostic. As a spot check, I disabled the per-store namespace branch in check_implicit_adapter, and implicit_fastly_detects_physical_store_alias_collisions and implicit_namespace_policy_is_adapter_owned both failed.
  • Adapter-owned secret_names_scoped_per_store(). Resolution now reuses deploy's from_inventories / desired_resource_links. The deploy-context builder is shared. ImplicitSecretLookup is #[non_exhaustive] with a constructor. 4xx rejection vs uncertain outcome is now distinguished. Re-exports are explicit. Axum leading-digit check, docs section and nits all done.
  • The deferred or declined items (combined trait, secret-only resolver, batch listing, Cloudflare name rule, shared physical credential) have reasoned explanations and are documented in the guide. That's fine for this prerequisite.

Findings

Non-blocking

  • ⛏ Spin legacy-path messages now mention native names (crates/edgezero-adapter-spin/src/cli.rs:546, :552). Inline.
  • 🌱 No length cap on Fastly native names (crates/edgezero-adapter-fastly/src/cli/secrets.rs:229). Inline.

CI Status (reviewed head 217983c0, local Rust 1.98.1 per .tool-versions)

  • fmt: PASS
  • clippy (--workspace --all-targets --all-features -D warnings): PASS
  • tests (--workspace --all-targets): PASS (1,582)
  • check (--features "fastly cloudflare spin"): PASS
  • Spin wasm32-wasip2 check: PASS
  • Hosted CI: all checks pass

Not covered here: whether Fastly really accepts these API calls (including the exact 64KB boundary), and the companion application's runtime integration. The PR also lists both as unverified.

let reason = spin_key_rule_violation(&spin_var);
return Err(format!(
"`#[secret]` field `{field}` does not reference a valid Spin variable name. {reason}. Pick a secret reference that conforms; its value is redacted.",
"secret lookup at `{field}` does not reference a valid Spin variable name. {reason}. Use a conforming native name; the name is redacted.",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⛏ validate_typed_secrets also serves legacy #[secret] apps (where native_name is None), but both messages now only describe the opt-in path. A legacy app whose secret reference isn't a valid Spin variable is told to "Use a conforming native name", which it can't set. The collision message at line 552 also lost the "after lowercasing" hint, which was the only clue to why two distinct references like Token and token collide. Only the wording is affected; the checks themselves are unchanged.

Suggested wording that covers both paths:

  • Line 546: ... Use a conforming secret reference or native name; the name is redacted.
  • Line 552: ... reference the same Spin variable (secret references are lowercased); Spin's flat variable namespace cannot disambiguate them. Use distinct secret references or native names; the names are redacted.

Ok(())
}

pub(super) fn validate_key(key: &str) -> Result<(), String> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌱 validate_key now also backs config validate's native-name check for Fastly, but it only checks the alphabet, not the length. If Fastly caps secret-name length, an over-long rendered name would pass preflight and only fail at write time. I haven't confirmed the provider's limit, so this is just something to consider later.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants