Wire request signing to RuntimeServices store primitives (PR 9)

[prk-Jr]

Summary

• Replaces direct FastlyConfigStore/FastlySecretStore construction in request_signing/ with RuntimeServices platform traits, making the signing subsystem platform-agnostic
• Moves management API write operations (config item / secret CRUD) out of trusted-server-core into an adapter-only FastlyManagementApiClient, enforcing the EdgeZero layering rule that only the adapter may call Fastly-specific APIs
• Threads RuntimeServices into AuctionContext so PrebidAuctionProvider (and future providers) can call RequestSigner::from_services without a deprecated FastlyConfigStore shim

Changes

File	Change
crates/trusted-server-adapter-fastly/src/management_api.rs	New — FastlyManagementApiClient with update_config_item, delete_config_item, create_secret, delete_secret via Fastly management API
crates/trusted-server-adapter-fastly/src/platform.rs	FastlyPlatformConfigStore::put/delete and FastlyPlatformSecretStore::create/delete delegate to FastlyManagementApiClient
crates/trusted-server-adapter-fastly/src/main.rs	Wire services into /verify-signature handler; add mod management_api
crates/trusted-server-core/src/request_signing/signing.rs	Rewrite — RequestSigner::from_services(&RuntimeServices) replaces from_config(); verify_signature accepts &RuntimeServices; deprecated shim removed
crates/trusted-server-core/src/request_signing/rotation.rs	Rewrite — KeyRotationManager::new() now infallible; all methods accept services: &RuntimeServices
crates/trusted-server-core/src/request_signing/endpoints.rs	All handlers accept services: &RuntimeServices; tests use in-memory stub stores
crates/trusted-server-core/src/auction/types.rs	AuctionContext gains pub services: &'a RuntimeServices
crates/trusted-server-core/src/auction/orchestrator.rs	Pass services through derived AuctionContext construction sites
crates/trusted-server-core/src/auction/endpoints.rs	Pass services when constructing AuctionContext
crates/trusted-server-core/src/integrations/prebid.rs	Use RequestSigner::from_services(context.services) — no more #[allow(deprecated)]
crates/trusted-server-core/src/platform/test_support.rs	Add build_services_with_config_and_secret test helper
crates/trusted-server-core/src/storage/api_client.rs	Deleted — FastlyApiClient moved to adapter-only management_api.rs
crates/trusted-server-core/src/storage/mod.rs	Remove api_client module and FastlyApiClient re-export
docs/superpowers/plans/2026-03-31-pr9-wire-signing-to-store-primitives.md	Implementation plan

Closes

Closes #490

Test plan

• cargo test --workspace
• cargo clippy --workspace --all-targets --all-features -- -D warnings
• cargo fmt --all -- --check
• JS tests: cd crates/js/lib && npx vitest run
• JS format: cd crates/js/lib && npm run format
• Docs format: cd docs && npm run format
• WASM build: cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1
• Manual testing via fastly compute serve

Checklist

• Changes follow CLAUDE.md conventions
• No unwrap() in production code — use expect("should ...")
• Uses tracing macros (not println!)
• New code has tests
• No secrets or credentials committed

[ChristianPavilonis]

Overall this is a well-architected PR — the core crate is now genuinely platform-agnostic for signing, and the management API isolation to the adapter is the right layering. A few correctness and structural items below, none blocking.

---

Note: One finding could not be placed as an inline comment because the relevant line is not part of the diff:

🤔 Module doc still references Fastly-specific store semantics (crates/trusted-server-core/src/request_signing/mod.rs, line 8)

The module doc still says "Fastly stores have two identifiers" and references ConfigStore::open / SecretStore::open. Since the whole point of this PR is platform-agnostic signing, this doc should use platform-neutral language (e.g. "Platform stores have two identifiers" and reference the trait methods).

[aram356]

Summary

This PR cleanly separates the Fastly management API client from trusted-server-core into the adapter layer, replacing direct FastlyConfigStore/FastlySecretStore usage with RuntimeServices platform traits. The architecture improvement is solid. Two items need clarification before merge.

Blocking

🔧 wrench

• Secret storage format change: the new FastlyManagementApiClient::create_secret base64-encodes the secret field at the transport layer (and uses PUT), while the old FastlyApiClient::create_secret sent it as raw text (POST). Since rotation.rs::store_private_key already base64-encodes the raw key, the effective payload format has changed. Need confirmation on Fastly API contract and backward compatibility with existing keys. (management_api.rs:68)

❓ question

• Broadened status check: old code checked for specific status codes (OK, NO_CONTENT), new code uses is_success() (any 2xx). Was this intentional? (management_api.rs:90)

Non-blocking

🤔 thinking

• Per-call client construction: each write call constructs a new FastlyManagementApiClient (4x during rotate_key). SDK caching mitigates this, but worth considering caching at a higher level. (platform.rs:69)
• parse_active_kids untested directly: edge cases like ",", "", " kid-a , , kid-b " are not covered by focused unit tests. (mod.rs:56)

♻️ refactor

• signing_store_ids returns unnamed tuple: fine for two values, but a named struct would improve readability if more settings are added. (endpoints.rs:168)

⛏ nitpick

• JWKS_STORE_NAME/SIGNING_STORE_NAME visibility: pub but only used within the crate — pub(crate) would be more precise. (mod.rs:49)
• PR checklist says "tracing": actual code and CLAUDE.md use log macros.

CI Status

• integration tests: PASS
• browser integration tests: PASS
• prepare integration artifacts: PASS

[aram356]

Summary

The architectural move — extracting FastlyApiClient from trusted-server-core into trusted-server-adapter-fastly::management_api and threading RuntimeServices through signing and auction — is exactly the right direction for EdgeZero's platform-agnostic core. Local fmt/clippy/cargo test --workspace all pass, and integration checks on GitHub are green.

Four blocking items prevent merge as-is, all clustered around the rotate/deactivate lifecycle. Fixing (1) first will likely surface (2) and (3) via real tests without any further changes.

Blocking

🔧 wrench

1. Handler tests in request_signing/endpoints.rs assert nothing (crates/trusted-server-core/src/request_signing/endpoints.rs:542-658)

test_handle_rotate_key_with_empty_body, test_handle_rotate_key_with_custom_kid, test_handle_deactivate_key_request, test_handle_deactivate_key_with_delete, and test_handle_trusted_server_discovery all use this pattern:

match result {
    Ok(mut resp) => { /* log … no asserts */ }
    Err(e) => log::debug!("Expected error in test environment: {}", e),
}

They pass whether the handler succeeds or fails, so the new rotate/deactivate/discovery handler paths are effectively untested even though the PR checklist claims "New code has tests."

Fix: assert on response status and on the success/error fields of the parsed JSON body. Minimum:

let mut resp = handle_rotate_key(&settings, &noop_services(), req)
    .expect("should return a response even when stores are unavailable");
assert_eq!(resp.get_status(), StatusCode::INTERNAL_SERVER_ERROR);
let body: RotateKeyResponse = serde_json::from_str(&resp.take_body_str())
    .expect("should deserialize rotate response");
assert!(!body.success);
assert!(body.error.is_some());

2. SECRET_UPSERT_METHOD = "PUT" on the Fastly secret-store collection endpoint is not a documented API verb (crates/trusted-server-adapter-fastly/src/management_api.rs:31, 257-289)

The previous api_client.rs used POST (the documented create verb); upsert semantics in Fastly's secret-store API are expressed by posting with method: "create_or_recreate" in the JSON body, not by switching the verb.

The only guards here are secret_upsert_method_uses_put (asserts the constant against itself) and create_secret_uses_secret_store_error_for_transport_failures (uses a bogus backend). Neither touches the real API contract. As written, the second rotation of any kid — or a same-day re-rotation via generate_date_based_kid — will likely return 405/400 from the management API in production.

Fix:

const SECRET_UPSERT_METHOD: &str = "POST";

fn build_secret_payload(secret_name: &str, secret_value: &str) -> String {
    serde_json::json!({
        "name": secret_name,
        "secret": general_purpose::STANDARD.encode(secret_value.as_bytes()),
        "method": "create_or_recreate",
    })
    .to_string()
}

This is distinct from the earlier base64 finding — it's about the verb itself.

3. KeyRotationManager::rotate_key is non-atomic across four management-API writes with no rollback (crates/trusted-server-core/src/request_signing/rotation.rs:61-97)

Sequence:

1. store_private_key (secret store)
2. store_public_jwk (config store)
3. update_current_kid (config store)
4. update_active_kids (config store)

Failure modes that leave the store inconsistent:

• Fail at (2): private key lives in the secret store with no corresponding JWK — orphaned key material.
• Fail at (4): current-kid advances to a kid that isn't in active-kids. The edge starts signing with a kid whose JWK is not advertised in the JWKS discovery document, so downstream verifiers that refresh via active-kids will reject every signed request until the store is manually repaired. This is a signing outage.

Fix: on failure after (1) or (2), attempt best-effort deletion of the freshly written artifacts and surface the rollback status in the returned Report. Ideally, reorder so current-kid only flips after both artifacts AND active-kids are confirmed written:

store_private_key → store_public_jwk → update_active_kids → update_current_kid

That ordering ensures any partial failure leaves the old kid still active and the new kid visible in JWKS but unused — a recoverable state.

4. delete_key has a symmetric partial-failure hole that leaks private key material (crates/trusted-server-core/src/request_signing/rotation.rs:205-227)

If deactivate_key and the JWK delete both succeed but secret_store.delete fails, the private key remains in the secret store while its JWK is gone. A retry then fails at the JWK delete step because check_response rejects 404 (see ❓ below), so the operator can't converge by re-running the command.

Fix: order the secret-store delete before the JWK delete so a partial failure leaves a verifiable-but-not-signable orphan JWK (safer than orphaned key material), and treat 404 from both deletes as success so retries converge.

❓ question

5. Are management-API deletes meant to be idempotent? (crates/trusted-server-adapter-fastly/src/management_api.rs:224-322)

check_response rejects any non-2xx, so a 404 on delete_config_item/delete_secret propagates as an error. For key-lifecycle use, idempotent deletes are usually desirable so a retried delete_key converges after a partial failure. Intentional? A minimal change:

if response.get_status() == StatusCode::NOT_FOUND {
    return Ok(());
}

This interacts directly with finding (4).

6. Status check broadened from specific codes to is_success() (crates/trusted-server-adapter-fastly/src/management_api.rs:90)

Old api_client.rs checked specific status codes (OK/NO_CONTENT); now any 2xx passes. Usually a good thing, but worth confirming it's deliberate.

Non-blocking

🤔 thinking

• TOCTOU in rotate_key (request_signing/rotation.rs:70-89): reads current-kid and active-kids, then writes unconditionally. Two concurrent rotations can lose one side's update. Rotation is admin-gated and rare, so probably acceptable — but a read-before-write invariant check would be cheap insurance.
• parse_ed25519_signing_key length heuristic is fragile (request_signing/signing.rs:38-56): len > 32 → base64, else raw. The production path always writes base64 via rotation.rs::store_private_key, so a raw value >32 bytes in production means data corruption — and this code hides it behind a misleading "failed to decode base64" error. Consider requiring base64 unconditionally or gating the raw branch behind #[cfg(test)].
• Per-call client construction (adapter-fastly/src/platform.rs:68-76, 129-142): every put/delete constructs a fresh FastlyManagementApiClient, which re-opens api-keys and re-registers the management backend. Doc acknowledges it, but for a single rotate_key call that's four back-to-back opens to read the same entry. Follow-up could cache an Arc<FastlyManagementApiClient> on the stores behind a OnceLock.

♻️ refactor

• Inconsistent admin-endpoint response semantics (request_signing/endpoints.rs:237, :345): handle_verify_signature returns 200 with opaque verified: false on internal errors, while handle_rotate_key/handle_deactivate_key return 500 with error: Some(format!("{}", e)) stringifying the full error-stack report. These are admin endpoints and presumably auth-gated upstream, but either the error detail should be opt-in behind a debug flag or the contract should be documented.
• KeyRotationManager::new takes impl Into<String> (request_signing/rotation.rs:49): CLAUDE.md's "prefer From over Into" guidance suggests &str would be clearer. Minor.

🏕 camp site

• SpySecretStore::create unconditionally returns Ok (request_signing/rotation.rs:329-342): a failing-create variant would exercise the rollback path requested in (3). A minimal SpySecretStore::with_create_failure_after(n) would unlock real tests for partial-failure recovery.

⛏ nitpick

• Cargo.toml dep ordering (crates/trusted-server-adapter-fastly/Cargo.toml): trusted-server-js now sits between urlencoding and the alphabetical dependencies.
• api_key: String (management_api.rs:109): no zeroize-on-drop. Client is short-lived so low-risk, but the previous Vec<u8> is gone. Flagging for awareness only.

👍 praise

• Extracting FastlyApiClient into the adapter layer is the right layering move — trusted-server-core is now genuinely platform-agnostic for request signing.
• parse_active_kids now has focused unit tests covering trimming, empty segments, and whitespace-only segments (request_signing/mod.rs:86-128) — directly addresses a prior review comment.
• SigningPayload using structured JSON instead of delimiter-based formats is a real security improvement (prevents signature confusion attacks), and the round-trip tests in signing.rs validate it end-to-end.

CI Status

• cargo fmt --all -- --check — PASS (local)
• cargo clippy --workspace --all-targets --all-features -- -D warnings — PASS (local)
• cargo test --workspace — PASS (local)
• integration tests — PASS (GitHub)
• browser integration tests — PASS (GitHub)
• prepare integration artifacts — PASS (GitHub)

[aram356]

Follow-up with inline comments properly pinned to diff lines. Two corrections to the previous review:

1. Retraction on the "handler tests assert nothing" 🔧: those weak match { Ok => log / Err => log } tests are pre-existing in feature/edgezero-pr8-content-rewriting; this PR only touched them mechanically to pass services through. Not blocking for PR 9. Reclassifying as a 🏕 for a follow-up: while you're in here, please replace them with real assertions — noop_services() returns Err deterministically, so an assertion like assert_eq!(resp.get_status(), StatusCode::INTERNAL_SERVER_ERROR) on the Ok(resp) branch would pin the contract.

2. Remaining blockers stand (see inline):

   • SECRET_UPSERT_METHOD = "PUT" is not a documented Fastly verb — second same-kid rotation will hit the wire and likely 405.
   • rotate_key / delete_key are non-atomic across four management-API writes with no rollback — partial failures cause signing outages or orphaned key material.

The architectural direction remains the right one; these are lifecycle-correctness holes sitting on top of a clean refactor.

[aram356]

Summary

Prior review findings on rotation atomicity, POST-based secret upsert, 404-idempotent deletes, base64 contract hardening, and endpoint-test assertions are all resolved cleanly. Blocking concerns on this round are merge-conflict leftovers rather than signing-subsystem bugs.

Blocking

🔧 wrench

• Out-of-scope proxy.rs regression: per-request allowed_domains is silently ignored — redirect_is_permitted now reads from settings.proxy.allowed_domains at both call sites, and the now-dead ProxyRequestHeaders::allowed_domains field is hidden behind #[allow(dead_code)]. Doc and behavior disagree; integration proxies that defaulted to open mode are now bounded by the global allowlist. (crates/trusted-server-core/src/proxy.rs:440, :604, :731)
• Unused trusted-server-js dep re-introduced: PR #615 removed it from the adapter as unused; this branch's merge kept it back. base64/serde/serde_json/urlencoding are justified by management_api.rs, but trusted-server-js is not referenced anywhere under crates/trusted-server-adapter-fastly/. (crates/trusted-server-adapter-fastly/Cargo.toml:24)

Non-blocking

🤔 thinking

• FastlyManagementApiClient reconstructed on every write — a single rotate_key pays four opens of api-keys + four backend re-registrations. (platform.rs:68-76, :129-142)
• TOCTOU in rotate_key's active-kids RMW — admin-gated so concurrency is unlikely, but worth a cheap check-and-set. (rotation.rs:94-102)
• Write-cost doc leads with the wrong concern — handle-open caching is a footnote; the outbound HTTPS round-trip to api.fastly.com is the headline. (platform.rs:37-45)

♻️ refactor

• make_request dispatches HTTP method via string literals — typo-prone; taking fastly::http::Method eliminates the _ => unreachable branch. (management_api.rs:156-165)
• signing_store_ids returns an unnamed (&str, &str) — flagged previously; a named struct would be clearer at both call sites. (endpoints.rs:168-184)

⛏ nitpick

• check_response reads response body on success paths — move read_to_string into the error branch. (management_api.rs:86-94)
• SECRET_UPSERT_METHOD test is self-referential — asserts a const against its own literal; inline the verb or drop the test. (management_api.rs:366-372)

📌 out of scope

• Plan doc bundled in the diff (+1835 lines) — docs/superpowers/plans/2026-03-31-pr9-wire-signing-to-store-primitives.md doubles the reviewer surface area. Plans belong in their own PR or outside review scope.

CI Status

• fmt: PASS
• clippy: PASS
• cargo test: PASS
• vitest: PASS
• format-docs / format-typescript: PASS
• integration tests / browser integration tests: PASS
• CodeQL: PASS

[aram356]

Summary

The architectural rewrite is clean — RequestSigner::from_services(&RuntimeServices), KeyRotationManager is infallible, management API writes are adapter-only, and test stubs exercise real crypto. Prior review rounds have addressed the HTTP status handling and Box::leak test-helper issues.

This pass surfaces four correctness/operational concerns around key rotation and the admin surface, plus a test-coverage gap for the rollback paths. None are data-loss bugs, but each can wedge request signing for operators using the admin endpoints.

Blocking

🔧 wrench

• Same-day re-rotation silently invalidates prior signatures (rotation.rs:66) — inline.
• No validation on operator-supplied kid (endpoints.rs:146, applies to both rotate and deactivate handlers) — inline.
• Rollback paths are documented but untested (rotation.rs:80-129) — inline.
• Deactivating or deleting the current-kid bricks signing (rotation.rs:227-275) — inline. Pre-existing, but this PR finalizes the admin surface.

❓ question

• Rotation resurrects a previously deactivated previous_kid (rotation.rs:95-99) — inline.

Non-blocking

🌱 seedling

• PlatformSecretStore::create taking &str forces double-base64 on binary keys (crates/trusted-server-core/src/platform/traits.rs:79). The signing key is base64-encoded in rotation::store_private_key, then base64-encoded again by management_api::build_secret_payload when serialized to the Fastly management API JSON payload. Documented and correct, but the root cause is value: &str in the trait signature — there's no way to pass opaque bytes. Not for this PR, but worth considering value: &[u8] when the trait is next revved. The Fastly adapter would base64-encode internally, other adapters can choose their encoding, and callers stop double-encoding.

⛏ nitpick

• Transport-failure test reaches into FastlyManagementApiClient struct fields (management_api.rs:387-402) — inline.

CI Status

• fmt: PASS
• clippy: PASS
• rust tests: PASS
• js tests: PASS
• browser integration: PASS
• format-typescript / format-docs: PASS

[aram356]

👍 Looks good