First-class secrets: the model, control plane, and config-encryption foundations by jgraettinger · Pull Request #3423 · estuary/flow

jgraettinger · 2026-08-27T14:53:26Z

Commits 1–5 of #3372, the Phase 1
implementation plan for first-class secrets (#3366).
That issue holds the motivation, threat model, authorization rules, and the full
commit sequence; read it first. Nothing here is user-reachable yet.

models, protocols — the task secrets stanza, threaded into built specs and
Validate/Discover requests. Inert.
control-plane-api — the internal.secrets table, the ViewSecret /
EditSecret / DecryptSecret capabilities, and the setSecret / deleteSecret /
secrets GraphQL surface.
control-plane-api — the /authorize/task/decrypt-secret and
/authorize/user/decrypt-secret endpoints.
config-encryption — /secret/encrypt and /secret/decrypt, wrapping secrets
under a second keychain whose decrypt grant is held by this service alone. Its
deploy workflow is configured for that keychain in the same commit; the new
arguments are required, so the deploy must land with it.
tokens, flow-client-next, unseal — the client machinery which resolves a
secrets stanza against those routes.

A mise commit at the base of the stack declares task prerequisites and fixes the
generated-artifact checks. It is independent of the rest and predates it only
because the secrets work is what surfaced the gaps.

Remaining Phase 1 work — runtime resolution, publication validation, flowctl secret, and the E2E catalog — follows in later PRs.

Review commit-by-commit: each message carries the rationale for its layer.

bbartman

OK. So I skimmed over most of this did a lot of reading, going commit by commit as suggested in the review description. I tried to focus on the parts of the code surrounding the Axum and graphQL related stuff.

I have some general cleanup stuff around Axum that I recommend doing to improve readability, and re-usability, over all this seems fine. Nothing glaring stands out to me but you should get some additional approvals from people with more experience with other parts of the system to give it a once over, as I'm still new and still learning the system.

adrian-estuary

Added a small comment/question. Will defer to Brian for approval, though, as he has much more context on this change.

adrian-estuary · 2026-09-01T19:14:11Z

+            Ok((token, _valid_for, _revoke)) => return Ok(token),
+            Err(retry_after) => retry_after,
+        };
+        tokio::time::sleep(retry_after.to_std().unwrap_or_default()).await;


Can retry_after ever be <= 0? If so, wouldn't this loop without delay, since unwrap_or_default would return 0 in that case?

Should there be a minimum delay imposed here or a guard to break the loop if retry_after is zero / too small?

It absolutely can be == 0, and that's an intended pattern and use case within tokens crate architecture today. It basically means "immediately retry this request over the network". As an example of how this can happen: a Snapshot doesn't authorize a requested access, but the snapshot is stale, and the calculated estimate of the next-refresh time for it is ~now or even in the past. The server sends a retry_after of zero meaning "this should be ready imminently". Note there's still a natural cool-off of the network round trip time.

If you're curious, here's where we calculate that policy today for the bulk of Snapshot AuthZ requests hitting control plane APIs (including for secrets):

flow/crates/control-plane-api/src/envelope.rs

Lines 152 to 162 in f4956af

// Determine the remaining "cool off" time before the next Snapshot starts.

let cool_off = std::cmp::max(

(snapshot.taken + crate::Snapshot::MIN_REFRESH_INTERVAL) - failed,

tokens::TimeDelta::zero(),

);

// We don't know how long a Snapshot fetch will take. Currently it's ~1-5 seconds,

// but our real objective here is to smooth the herd of retries awaiting a refresh.

failed

+ cool_off

+ tokens::TimeDelta::milliseconds(rand::rng().random_range(500..10_000))

IIRC there are other use cases of tokens that use literal-zero because the request itself blocks.

Thanks for the context!

jgraettinger · 2026-09-02T14:41:19Z

Thanks for reviewing! I'll hold off on rebasing until approved, and then rebase + re-stamp approval.

Several test and build workflows depended on setup steps that lived only in `ci:platform-test`'s ordering (and in the matching GitHub Actions job), so running one task on its own failed in ways that read as real breakage: a missing submodule looks like hundreds of JSON-Schema validation bugs, and an unbuilt `libbindings.a` looks like a Go linker problem. Encode those as task dependencies, so that `mise run ci:<task>` brings up what it needs. `ci:nextest-run` gains musl-dev (flow-connector-init on PATH), build:gazette (e2e-support spawns a real broker), supabase, and bigtable. `ci:nextest-build` and `ci:nextest-run` also gain `bootstrap:submodules`, a new task covering crates/json/tests/official, which CI gets from actions/checkout but a plain clone or `git worktree add` does not. Supabase is a dependency of everything compiling sqlx's macros: `cargo sqlx prepare` runs a plain `cargo check`, so test targets are absent from the .sqlx cache and need a live database. `build:flowctl-go` now builds the bindings staticlib it links, rather than requiring that `ci:gnu-dev` was run first; the --release path, which links a release libbindings.a produced by `ci:gnu-opt`, says so when it is missing. `ci:nextest-run` put ${HOME}/cargo-target/<arch>-unknown-linux-musl/debug on PATH, which no longer exists: per-stack target directories place musl output under ${CARGO_TARGET_DIR}, as ci:gotest and ci:catalog-test already spell it. `ci:flow-schema-check` and `ci:graphql-schema-check` asked `git diff` whether the committed artifact matches HEAD, which conflates "not regenerated" with "not committed" and so fails for anyone holding an uncommitted models change alongside its equally uncommitted regenerated artifact. Snapshot the working-tree copy and diff against that instead: it asks whether the checked out artifact matches what the checked out sources produce, and is equivalent on CI's clean tree. pgTAP tests pass only against a freshly reset database -- they read and blanket-delete shared tables, so rows left behind by the Rust suites (which clear the tables they need at test init, not teardown) or by a running local stack surface as unrelated failures. CI is unaffected, running against a fresh database and ahead of the Rust suites. Say so in AGENTS.md and in the task description.

First commit of first-class secrets (#3372, phase 1 of #3366). This lands the catalog model and protocol surface for task secrets. Nothing reads the new fields yet. A `secrets` stanza maps a JSON pointer within a task's connector configuration to the catalog name of a secret. The runtime resolves each name and applies it at that location as an RFC 7396 merge patch before handing the configuration to the connector, so a configuration using secrets stays plaintext and carries no ciphertext of its own. Pointers are the map keys because locations must be unique, while one secret may serve several of them. `BTreeMap` gives the lexicographic pointer ordering that the merge contract requires, under which a deeper pointer wins wherever two entries overlap. The stanza is carried by explicit typed proto fields rather than smuggled inside `config_json`. Built specs cover the runtime Open path. Validate and Discover carry it separately because both are built from the model, before any built spec exists, and are proxied into the data plane. `TaskServiceConfig` gains `config_encryption_url`, which the runtime will use to reach the decryption service.

… surface Second commit of first-class secrets (#3372, phase 1 of #3366). This lands where a secret lives, who may touch it, and the GraphQL surface for managing one. Nothing resolves a secret yet. `internal.secrets` holds the sops-wrapped document keyed by catalog name. The document is `json` rather than `jsonb` because sops verifies its MAC by traversing the document in order, and `jsonb` would normalize key order and break verification. It is opaque to the control plane throughout: transported as a `models::RawValue`, stored via a cast from text, and parsed only into a throwaway side copy to check structure. A round trip through `serde_json::Value` would alphabetize keys, so the GraphQL scalar routes through `async_graphql::Value`, whose objects preserve order — a test posts raw request bodies with deliberately non-alphabetical keys to pin the property. All access flows through control-plane-api, hence `internal` and no RLS. `id` is the secret's lifecycle identity in the spirit of flowids elsewhere: each distinct wrapped document is a distinct entity, and flowids are time-ordered so `id` also answers which of two observations is newer. There is no `updated_at` and no version counter. `setSecret` takes an already-wrapped document, because wrapping and setting are separable steps — which is what makes a GitOps re-apply of a stored document expressible. Its write semantics are one atomic statement: a CTE locks the current row, and a conditional insert/update follows. An identical document is the same entity and keeps its id, so re-applying is idempotent. Identity is normalized-text equality with key order significant and formatting not: the GraphQL layer hands the scalar an already-parsed value, so the literal request bytes are never in reach, and sops is indifferent to formatting anyway. A document whose embedded `sops.lastmodified` predates the stored one is rejected, which guards against a stale re-apply; ties pass, because the timestamp has second granularity. The check is best-effort by construction — only config-encryption can verify the MAC that makes the timestamp tamper-evident. Because Postgres re-parses the stored timestamp text on each later set, the few timestamps chrono accepts but Postgres cannot parse (year zero, offsets beyond ±15:59) are rejected up front rather than stored as poison values. `deleteSecret` sweeps recursively only through an explicit `prefix` argument, and rejects the empty prefix outright rather than leaning on the invariant that no grant is broad enough to cover it. The `secrets` query returns names and lifecycle ids only, never documents, and follows the prefix-scoped list pattern of its sibling modules: results are limited to prefixes where the caller holds `ViewSecret`, and `filter.catalogName` narrows within that scope by subtree (`startsWith`) or exact set (`in`). Sharing `filtered_authorized_prefixes` with those siblings keeps one owner for the invariant that a filter can only remove authorized prefixes, never add them. `ViewSecret`, `EditSecret`, and `DecryptSecret` all land in the `Editor` bundle, so legacy Admin grants inherit them. Publish authority is already disclosure authority: someone who can publish a task at a prefix can point a connector at any sibling secret and read its plaintext, so withholding decrypt from an editor buys nothing. The lifecycle test is a table of steps driven through one transcript and snapshotted. Secret ids are symbolized by order of first appearance, so the snapshot states entity identity directly: a recurring id is what proves an identical re-apply was a no-op on the same entity rather than a fresh write.

Third commit of first-class secrets (#3372, phase 1 of #3366). Add `GET /authorize/user/decrypt-secret` and `POST /authorize/task/decrypt-secret`, which authorize a user or task subject to decrypt a named secret and disclose its sops-wrapped document to config-encryption, the intended caller and holder of the KMS decrypt grant. The user route requires `DecryptSecret` covering the secret name, and requires a `started` parameter so that retries are answered as 200-with-`retryMillis` (the route is proxied, so a 307 redirect is not actionable). The task route verifies a data-plane-signed token whose `sub` is a real or synthetic Shard ID and whose `sel` names the secret under the new `estuary.dev/secret-name` label. It enforces the sibling rule (dirname(secret) == dirname(task)) and residency of Snapshot-known tasks. Tasks absent from the Snapshot (Discover / Validate of unpublished tasks) fall back to a fresh DB read of storage mappings: the longest covering mapping alone decides whether the issuing plane is admitted, mirroring publication's `lookup_mapping`, and admissibility is settled before existence so a denied caller doesn't learn which secrets exist. Also correct fixture shard template IDs to use the real `materialize/` prefix (as `assemble::shard_id_prefix` emits), which the synthetic Shard ID parsing relies upon, and add a legacy no-generation shard fixture.

Fourth commit of first-class secrets (#3372, phase 1 of #3366). Add `POST /secret/encrypt` and `POST /secret/decrypt`, wrapping and unwrapping the sops document of a named secret under a second keychain whose decrypt grant is held by this service alone. The legacy keychain and route are untouched, and the two are separate Routers with separate state so neither can reach the other's key. The document is `{name, value, sops}`, with only `value` encrypted, via `--encrypted-regex '^value$'` -- which covers an entire object subtree, so a secret value may be arbitrary JSON. `name` stays plaintext but is MAC-covered, because sops MACs unencrypted values too. That is what binds a document to its secret: renaming one in place breaks its MAC, and storing one verbatim under another name is caught by comparing the name we asked for against the name we decrypted. Both are pinned by tests through the real sops binary, as is the tamper-evidence of `sops.lastmodified` on which the control-plane's monotonicity guardrail rests. `/secret/encrypt` is unauthenticated: wrapping a value the caller already holds discloses nothing, and authority is enforced where the secret is set. Keeping the two separable keeps this service small. Its body is a JSON value under a modest size limit -- a secret is a credential, not a place to stash bulk data. `/secret/decrypt` does not interpret its bearer token beyond sniffing which authorize route can verify it: a data-plane token, carrying the gazette `cap` and `sel` claims, is POSTed to `/authorize/task/decrypt-secret`, and anything else is forwarded as a Bearer to `/authorize/user/decrypt-secret` along with the `name` and `started` which that route cannot recover from the token itself. Misclassification is harmless, since the route we pick verifies the token it's given. A control-plane retry response passes through as our own response body: this service never sleeps and never retries, because retry policy belongs to the client, which alone knows the deadline of the overall operation. The local stack's config-encryption env gains the second Age keypair now, rather than in the commit which adds the E2E catalog, so that a stack keeps starting across the commits in between. Its private half lives in that env file and nowhere else -- reactors hold the legacy key, but never this one. The deploy workflow configures `SECRETS_KMS_TYPE`, `SECRETS_KMS_KEY`, and `CONTROL_PLANE_URL`, which this commit makes required arguments: without them the next deploy after merge would exit at startup. `SECRETS_KMS_KEY` is the new `platform-secrets` key in `estuary-control`, kept deliberately distinct from the legacy `KMS_KEY`: data-plane reactors hold decrypt on the legacy key, because a sops-wrapped endpoint config travels inside a built spec, and must never hold it on this one.

…nery Fifth commit of first-class secrets (#3372, phase 1 of #3366). - `tokens::fetch_once` one-shot driver: retries as the Source directs, holding `started` constant, bounded by a caller-side timeout. It never retains what it fetches, which is why a Watch must not be used here. - `TaskSecretDecrypt` and `UserSecretDecrypt` workflows against config-encryption `/secret/decrypt`, with the `new_signed_source` sibling that pins `iat` to `started` and refreshes `exp` per attempt. - `unseal::secrets`: the `sops` sniff and `resolve()`, which fetches each distinct secret once through a generic async decrypt callback and applies entries as RFC 7396 merge patches in lexicographic pointer order. Pointer tokens are always object property names.

jgraettinger · 2026-09-02T19:10:53Z

PTAL (just rebased, no other changes)

jgraettinger requested a review from a team August 27, 2026 14:54

jgraettinger force-pushed the johnny/issue-3372-commit-1-d3927d branch 2 times, most recently from 8acdb56 to 5886cce Compare September 1, 2026 01:58

bbartman reviewed Sep 2, 2026

View reviewed changes

adrian-estuary reviewed Sep 2, 2026

View reviewed changes

jgraettinger added 6 commits September 2, 2026 13:42

jgraettinger force-pushed the johnny/issue-3372-commit-1-d3927d branch from 5886cce to 661f246 Compare September 2, 2026 18:49

Navigation Menu

Uh oh!

First-class secrets: the model, control plane, and config-encryption foundations - #3423

First-class secrets: the model, control plane, and config-encryption foundations#3423
jgraettinger wants to merge 6 commits into
masterfrom
johnny/issue-3372-commit-1-d3927d

jgraettinger commented Aug 27, 2026

Uh oh!

bbartman left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adrian-estuary left a comment

Uh oh!

adrian-estuary Sep 1, 2026

Uh oh!

jgraettinger Sep 2, 2026

Uh oh!

jgraettinger Sep 2, 2026

Uh oh!

adrian-estuary Sep 2, 2026

Uh oh!

jgraettinger commented Sep 2, 2026

Uh oh!

jgraettinger commented Sep 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	// Determine the remaining "cool off" time before the next Snapshot starts.
	let cool_off = std::cmp::max(
	(snapshot.taken + crate::Snapshot::MIN_REFRESH_INTERVAL) - failed,
	tokens::TimeDelta::zero(),
	);

	// We don't know how long a Snapshot fetch will take. Currently it's ~1-5 seconds,
	// but our real objective here is to smooth the herd of retries awaiting a refresh.
	failed
	+ cool_off
	+ tokens::TimeDelta::milliseconds(rand::rng().random_range(500..10_000))

Uh oh!

Conversation

jgraettinger commented Aug 27, 2026

Uh oh!

bbartman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adrian-estuary left a comment

Choose a reason for hiding this comment

Uh oh!

adrian-estuary Sep 1, 2026

Choose a reason for hiding this comment

Uh oh!

jgraettinger Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

jgraettinger Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

adrian-estuary Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

jgraettinger commented Sep 2, 2026

Uh oh!

jgraettinger commented Sep 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants