Repository navigation
Support implicit credential lookup plans and shared Fastly secret operations - #444
ChristianPavilonis wants to merge 8 commits into
Conversation
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
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
curlonly 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-curlsubprocess 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 wholeAppConfigMetaimpl, 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 secretsname 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).
| String::new() | ||
| }; | ||
| let identity = (&lookup.descriptor_id, &lookup.store_id, &lookup.key); | ||
| if let Some(previous) = native_owners.insert((namespace, name.clone()), identity) |
There was a problem hiding this comment.
🤔 Two descriptors can't share one stored credential, and the error calls it a collision
If two lookups with different descriptor_ids (say fictional.pull and fictional.push) both target store default, key ts1_shared, they refer to the same physical secret, but this function always rejects them:
- Same rendered name: this
native_ownerscheck sees different identities and fails withnative secret identity collision. I confirmed this with a throwaway test callingcheck_implicit_adapterfor bothfastlyandaxum. - Different rendered names: the dedup check at line 2006 matches on
store_id+keyonly (not the descriptor), so it fails withidentical secret request has inconsistent native rendering, although the comment says that check is for repeated identical requests. (From reading the code; not run.)
If one credential per descriptor is the intended rule, could it be checked up front with a message that says so (e.g. "store X key Y is requested by more than one descriptor")? If sharing should be allowed, the dedup check should ignore the descriptor when store and key match.
| where | ||
| C: AppConfigMeta, | ||
| { | ||
| if let Some(lookups) = typed.implicit_secret_lookups()? { |
There was a problem hiding this comment.
🤔 Opting in skips every #[secret] leaf check
When implicit_secret_lookups() returns Some, this returns before check_legacy_typed_secrets runs. A hand-written impl that opts in but still has #[secret] fields therefore loses three checks: empty values, whether [stores.secrets] is declared, and whether a store_ref points at a declared id. Runtime code still reads those fields, so the mistake would surface at runtime instead of at config validate. (From reading the code; not run.)
If skipping is deliberate (the PR description says "without retaining scalar secret-reference leaves"), please say so in the implicit_secret_lookups doc. Otherwise, consider still running the legacy checks when C::secret_fields() is non-empty.
| /// Every `#[secret]` / `#[secret(store_ref)]` leaf on the struct, | ||
| /// including those reached through `#[app_config(nested)]` children, | ||
| /// each carrying its full path from this struct's root. | ||
| /// each carrying its full path from this struct\'s root. |
There was a problem hiding this comment.
⛏ This line isn't part of the feature, but it gained a stray backslash (struct\'s). Please revert it to struct's.
| path: &str, | ||
| body: Option<&str>, | ||
| ) -> Result<FastlySecretResponse, String> { | ||
| transport.request(method, path, body).map_err(|_error| { |
There was a problem hiding this comment.
⛏ Every caller shares this helper, so read-only calls (secret_exists, resolve_resources) also report "mutation outcome may be unknown" on transport failure, although nothing was written. A neutral message for GETs, or a per-call message, would avoid alarming operators.
| )] | ||
|
|
||
| /// Narrow host operations for reviewed secret provisioning. | ||
| #[path = "secret_operations.rs"] |
There was a problem hiding this comment.
⛏ The secrets module loads secret_operations.rs via #[path]. Matching the module and file names (e.g. src/cli/secrets.rs, or renaming the module) would make it easier to find.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
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 deployrunning 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) - ♻️
ImplicitSecretLookupAPI shape:#[non_exhaustive], three-state return (crates/edgezero-core/src/app_config.rs:94) - ♻️
context_from_manifestduplicatesrun_deploy's context builder (crates/edgezero-cli/src/fastly_secrets.rs:26) - 🌱
secret_existslists 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) - ⛏
base64dependency ordering (Cargo.toml:46) - 🌱 Docs:
docs/guide/configuration.mddocuments#[secret]and hand-writtenAppConfigMeta, but not the opt-in hooks (implicit_secret_lookups/render_implicit_secret_name) or theedgezero_cli::fastly_secretshelpers. 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 withoutmanifest_guard(), while guarded tests such asconfig.rs:2601setEDGEZERO__STORES__CONFIG__APP_CONFIG__NAME="". This already exists onmainand 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). Theedgezero-adapter-fastlylib suite fails intermittently (2 of 18 runs) because of the first blocking finding. - check (
--features "fastly cloudflare spin"): PASS - WASM checks (spin
wasm32-wasip2, fastlywasm32-wasip1, core and cloudflarewasm32-unknown-unknown): PASS - Hosted CI: 28/28 checks pass
| use edgezero_core::test_env::{EnvOverride, PathPrepend, env_lock}; | ||
| use std::fs::Permissions; | ||
| use std::os::unix::fs::PermissionsExt as _; | ||
| let _lock = env_lock().lock().expect("env lock"); |
There was a problem hiding this comment.
🔧 This test takes a different env mutex than every other env-mutating test in this binary
This test changes PATH and FASTLY_API_TOKEN while holding edgezero_core::test_env::env_lock(). The other 68 env-mutating tests in the edgezero-adapter-fastly lib test binary hold cli::tests::path_mutation_guard() (cli.rs:10362), which is a separate OnceLock<Mutex<()>>. They also set PATH and FASTLY_API_TOKEN (e.g. cli.rs:7133, :7166). The two locks don't exclude each other, so EnvOverride's unsafe { set_var } safety argument ("the caller holds the env lock") does not hold.
Reproduced on this head (cargo test -p edgezero-adapter-fastly --features cli --lib, macOS):
- 2 of 18 runs failed, each with 62 failures: one root failure plus 61
PoisonErrorcascades from the poisonedpath_mutation_guard. - With
--skip production_transport_keeps_credentials_off_argv_and_sanitizes_stderr: 0 of 12. - Root failure both times:
deploy_honours_threaded_manifest_path_and_strips_it_from_the_fastly_argv→"fastly compute deploy failed with status exit status: 1". That test's fakefastlyalways exits 0. This test'sPathPrependdrop restoredPATHfrom under it, so the developer's real/opt/homebrew/bin/fastly compute deploy --service-id SVC1 --non-interactiveran. The opposite interleaving sends this test's write through the realcurltoapi.fastly.com.
Fix (verified: clippy/fmt clean, 0 of 10 failing runs): give the binary one lock by delegating the existing guard in cli.rs:
use edgezero_core::test_env::{EnvOverride, PathPrepend, env_lock};
fn path_mutation_guard() -> &'static Mutex<()> {
env_lock()
}| let (dir, manifest, _) = setup_project(PUSH_MANIFEST, &config); | ||
| assert!(run_config_validate_typed::<ImplicitConfig>(&args_for(&manifest)).is_err()); | ||
| assert!( | ||
| run_config_push_typed::<ImplicitConfig>(&push_args(&manifest, "axum")).is_err() |
There was a problem hiding this comment.
🔧 Five new guards can be deleted with the suite still green, and this push assertion can't fail
This push call doesn't set yes = true, so it fails at the consent gate whether or not the implicit preflight rejects the plan. Deleting typed_secret_checks(...) from run_config_push_typed entirely still passes this test. None of the three arms assert on the error text either, so each could pass for an unrelated reason.
I disabled each new guard in turn and ran the affected suites. All of these survived:
| Guard | Location |
|---|---|
| Axum native-name charset check | edgezero-adapter-axum/src/cli.rs:366 |
Conflicting (descriptor, path) owner metadata |
config.rs:1949 |
| "identical secret request has inconsistent native rendering" | config.rs:2004 |
| Duplicate logical Fastly store declaration | secret_operations.rs:212 |
| Selected Fastly store missing from inventory | secret_operations.rs:193 |
Fix: set push.yes = true (or local + dry_run, as the Fastly test does), assert the expected substring for each bad case, and add one focused test per guard above. The PR checklist says new behavior has executable host tests; right now these branches don't.
| // Fastly stores have independent namespaces; the other built-in secret | ||
| // bridges flatten their names. Never merge different requests merely | ||
| // because their rendered names happen to match. | ||
| let namespace = if adapter.name() == "fastly" { |
There was a problem hiding this comment.
♻️ Fastly-specific rule in the adapter-agnostic CLI
The header of this section (config.rs:1596-1598) says: "defer per-adapter rules to each adapter crate's Adapter trait impl. No if adapter == "spin" branches here." This is that kind of branch. It also means a future adapter with store-scoped secret namespaces would silently get flat-namespace collision semantics.
Suggest a registry hook that Fastly overrides, so the rule (and its test) lives with the adapter:
// edgezero-adapter registry
/// Whether secret names are scoped per physical secret store
/// (true) or share one flat namespace (false).
fn secret_names_scoped_per_store(&self) -> bool {
false
}let namespace = if adapter.secret_names_scoped_per_store() {
environment.store_name_checked("secrets", &lookup.store_id)...
} else {
String::new()
};| .with_native_name(name), | ||
| ); | ||
| } | ||
| adapter.validate_typed_secrets(&entries) |
There was a problem hiding this comment.
🤔 Cloudflare native names are never validated
Axum, Spin and Fastly now check native_name, but CloudflareCliAdapter still inherits the no-op validate_typed_secrets (edgezero-adapter-cloudflare/src/cli.rs:134). An implicit plan rendered for cloudflare passes config validate / push / diff with /, spaces or a newline in the name. The PR describes a four-adapter companion, so this path will be used.
Suggest a Cloudflare override, or at least a common floor in check_implicit_adapter (non-empty, no whitespace or control characters) that applies to every adapter.
Related: Spin's rejection message still reads "#[secret] field ... Pick a secret reference that conforms" when the entry came from an implicit lookup that has no #[secret] field.
| }; | ||
| let environment = effective_deploy_environment(context)?; | ||
| let mut stores = Vec::new(); | ||
| for (kind, ids, endpoint) in [ |
There was a problem hiding this comment.
♻️ Reuse deploy's resolution rather than re-implementing it
The PR says it shares existing deployment resolution, but this loop duplicates the deploy path (cli.rs:801-815) and drops one of its checks:
- Deploy builds
ResourceInventories::from_inventories(config, kv, secret), which rejects a resource ID that appears under two kinds. That check isn't run here. - Deploy resolves via
desired_resource_links(&stores, &environment, &inventories), which also rejects duplicate logical IDs and names the store in its errors. Here that's re-done by hand with a post-sortwindows(2)scan.
Building ResourceInventories and mapping desired_resource_links's output into FastlyStoreResource would keep secret operations and deploy from drifting apart. It would also cover the two branches here that currently have no test (missing store, duplicate logical ID).
| })?; | ||
| crate::ensure_adapter_defined("fastly", Some(&loader))?; | ||
| let stores = &loader.manifest().stores; | ||
| Ok(AdapterDeployContext { |
There was a problem hiding this comment.
♻️ Second copy of the deploy-context builder
run_deploy (lib.rs:194-228) builds the same DeployStoreIds, variable_defaults, adapter-manifest path and application-manifest path inline. The two copies already differ: deploy's loaded_application_manifest_path rejects a path that isn't a regular file, and this one only canonicalizes. Suggest one helper both call, e.g. fn deploy_context_from_loader(loader: &ManifestLoader, manifest_path: &Path, adapter: &str) -> Result<AdapterDeployContext, String>, so secret operations always target what deploy targets.
| /// # Errors | ||
| /// Returns an operational error distinct from confirmed absence. | ||
| #[inline] | ||
| pub fn secret_exists(resource_id: &str, key: &str) -> Result<bool, String> { |
There was a problem hiding this comment.
🌱 One full store listing per key
Each secret_exists call pages through the whole Secret Store (limit=100) and keeps paging after the key is found. A caller checking N lookups makes N x ceil(secrets/100) GETs. A list_secret_names(resource_id) -> Result<BTreeSet<String>, String> helper (with the same completeness and cursor checks) would let the caller check every key from one listing. Fastly also allows limit=200.
| //! Host helpers shared with application-specific secret commands. | ||
|
|
||
| pub use edgezero_adapter::registry::{AdapterDeployContext, DeployStoreIds}; | ||
| pub use edgezero_adapter_fastly::cli::secrets::*; |
There was a problem hiding this comment.
⛏ The PR describes these as narrow host helper re-exports, but a glob makes any future pub item in edgezero_adapter_fastly::cli::secrets part of edgezero-cli's public API automatically. Listing the items (resolve_resources, secret_exists, write_secret, the _with_transport variants, FastlySecretTransport, FastlySecretResponse, FastlySecretResources, FastlyStoreResource, SecretWriteAction, FASTLY_SECRET_MAX_BYTES) keeps that surface deliberate.
| let var = secret_env_var(entry.key_value) | ||
| .map_err(|err| format!("`#[secret]` field `{}`: {err}", entry.field_name))?; | ||
| let var = if let Some(native_name) = &entry.native_name { | ||
| if native_name.is_empty() |
There was a problem hiding this comment.
⛏ This accepts a leading digit (1TOKEN), which shells and most env-file loaders can't set, so the secret would silently be unreachable at runtime. Suggest also requiring the first byte to be A-Z or _. This check currently has no test either (see the coverage comment on config.rs:5708).
| # wasm in the demo Spin build). We only use `Decompressor` and | ||
| # `CompressorWriter`, unchanged across the two majors. | ||
| brotli = "8" | ||
| base64 = "0.22" |
There was a problem hiding this comment.
⛏ base64 is out of alphabetical order here (after brotli) and in crates/edgezero-adapter-fastly/Cargo.toml (listed before anyhow).
Summary
Companion prerequisite for IABTechLab/trusted-server#1037. This does not implement or prove that project's runtime secret-scoping guarantee.
Changes
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, andkey.Noneretains 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_transportvariants, comparable resource metadata, andFASTLY_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.Test plan
Rust 1.95.0 on Linux x86_64. Independent target directory, two Cargo jobs, dev/test debug info disabled.
Workspace gates below passed at
5ad74df2. The final4f645b1echanges only the implicit Fastly physical-store collision check and its regression. The affected CLI all-target tests passed again, including 227 library tests, and its all-features/all-target clippy passed. Other tested inputs remain unchanged.cargo test --workspace --all-targets -- --test-threads=1cargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkcargo check --workspace --all-targets --features "fastly cloudflare spin"Unverified and blocked
The provider API documentation specifies POST create, PATCH replacement and 64 KiB raw value limits. 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