feat(secrets): cluster secrets page on the hdb_secret store#1402
Conversation
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces client-side envelope encryption (enc:v1) using the Web Crypto API to securely manage cluster-level deployment secrets, including UI components for listing, adding, and editing secrets. Feedback focuses on utilizing the query cache for the public key, adding a deletion confirmation prompt, handling non-secure contexts where the Web Crypto API is unavailable, ensuring type safety when closing the edit modal, and optimizing ArrayBuffer slicing to avoid unnecessary memory allocations.
f3a8577 to
a2827d7
Compare
dawsontoth
changed the title
f7aafe5 to
d47f200
Compare
448fef2 to
775ba23
Compare
dawsontoth
changed the title
Config > Secrets manages the replicated system.hdb_secret table (harper#1554; Pro key custody in harper-pro#512, tracked by harper-pro#166): named, envelope-encrypted rows on the cluster itself, edited through the per-instance operations API and scoped to applications via grants. - list_secrets / set_secret / delete_secret / grant_secret / revoke_secret / get_secrets_public_key hooks on the instance operations client. - Values are encrypted in the browser (enc:v1 via lib/crypto/envSecret.ts, which harper#1554 ported into core as its envelope codec) and can never be read back. Rotation-aware: the public key is cached minutes not forever, and a kid-mismatch rejection drops the cached key, re-encrypts, and retries once. - Grants editor in the edit dialog (only granted applications receive a secret at load time) and per-row warnings for rows whose kid no longer matches the cluster's custody key. - Degrades gracefully without key custody: the list stays browsable, a banner explains why nothing can be encrypted, and add/edit is disabled. - Nav entry is version-gated only (>= 5.2.0 placeholder, matching harper#1554's upgrade directive) — the store ships in core, so it is not restricted to Fabric-managed clusters. Builds on the shared SecretsManager / SecretModals components from the .env editor PR underneath this one. Hook coverage runs against real WebCrypto RSA keys, including the rotation retry re-encrypting under the newly served key. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ager central-manager#409 makes CM the custodian for hosted clusters: it mints the per-cluster RSA keypair on first use (Studio's key fetch is what triggers the mint), stamps the fingerprint onto instances, and delivers the private key to host-managers — so the public key exists and is authoritative at CM even before any node has custody registered. Fabric-managed clusters now fetch the public key via POST /ClusterSecrets (get_secrets_public_key + clusterId, camelCase response); self-hosted and local instances keep the node operation (snake_case response). Both are normalized to one shape, and the kid-mismatch retry now also heals CM's documented first-mint race (the losing key's envelopes fail with a kid mismatch; re-encrypting against the stored winner fixes them). Values are unchanged: still encrypted in the browser and submitted to the cluster's own set_secret — they never pass through or live in CM. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
73d8841 to
c481285
Compare
…on gate Harper ships the hdb_secret store in the 5.2.0 line (harper#1554); dev builds carry prerelease tags like 5.2.0-alpha.1 / 5.2.0-beta.1. Floor the Secrets nav gate at 5.2.0-alpha.1 (the earliest 5.2 prerelease) so those builds pass it — a plain '5.2.0' floor would exclude every prerelease (SemVer ranks prereleases below the release), same convention as the existing 4.7.0-beta.7 / 4.7.0-alpha.1 gates. Also type the processEnv field that list_secrets rows carry in the merged harper#1554. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
c481285 to
41cf167
Compare
Cluster secrets (hdb_secret) can be delivered two mutually-exclusive
ways (harper#1554): materialized onto process.env for every component
(global), or scoped to granted apps and read through the `secrets`
accessor. Expose that choice in the add/edit dialogs and, based on the
pick, show an inline, copy-paste-correct code example of how to read the
secret — `process.env.NAME` vs `const { NAME } = secrets` (bracket
notation for names that aren't valid JS identifiers).
- accessExample.ts: pure, name-aware example builder (+ unit tests).
- SecretDeliveryPicker / SecretAccessExample / PendingGrantsInput: the
tier radio, the copyable example, and an API-free grants collector for
the add flow (the live grant_secret editor can't run pre-creation).
- SecretModals / SecretsManager: opt-in `delivery` prop threads the tier
through onSet as { processEnv, grants }; the .env panel is unchanged.
- set_secret now sends processEnv/grants (omitted to preserve the stored
tier on a plain value rotation).
Scoped grants stay live (grant_secret/revoke_secret) in edit; env-var
rows hide the grants editor since a global secret can't be scoped.
Verified with a jsdom test driving the real dialog tree end-to-end.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
This can be ready-for-review now, since the surface we expose in Harper shouldn't change meaningfully, and no one will see this until they're rocking 5.2.0 with us. We can make more little adjustments as we see fit, too. 🥳 |
kriszyp
left a comment
kriszyp
left a comment
There was a problem hiding this comment.
Reviewed with a focus on the client-side secret handling, and the security fundamentals hold up well — nicely done, and unusually well-tested for a crypto path. No blockers.
Security core (the part that matters):
- Plaintext never leaves the browser: values are AES-256-GCM encrypted with an RSA-OAEP-wrapped key before submission,
set_secretcarriesenvelopeand nevervalue, and that's asserted directly (setBody.valueundefined,JSON.stringify(setBody)doesn't contain the plaintext). Nothing plaintext in the DOM, URL (route param is the name only), or error messages (kid mismatch surfaces a fingerprint, not the value). - Crypto correctness is verified against the real backend shape — fresh 32-byte key + 12-byte IV per call (nonce-freshness asserted), correct GCM tag split,
fingerprintOf= SHA-256(DER SPKI), round-tripped through a NodeprivateDecrypt/GCM stand-in across unicode/multiline/PEM/empty inputs.requireSubtle()fails closed outside a secure context. Solid. - Rotation is fail-safe: a kid mismatch drops the cached key and re-encrypts exactly once (bounded by
isRetry), second mismatch surfaces.
Two UX items I'd suggest tightening (recoverable, no data loss, but confusing traps):
Significant — managed-cluster banner-flash + wrong-target key fetch (config/secrets/index.tsx:32). While the cluster query is pending, cluster === undefined ⇒ isSelfManaged = true ⇒ keySource targets the node, so publicKeyQuery fires a node get_secrets_public_key that fails on a CM-custody cluster and flashes the amber read-only banner until the cluster query resolves. Gating the public-key query on the cluster query settling (enabled: clusterId === undefined || !clusterPending) would avoid it.
Significant — live grants editor renders on an unsaved tier switch (SecretModals.tsx:342). Editing a processEnv secret and flipping the radio to "Scoped" (not yet saved) still renders the live SecretGrantsEditor; clicking Grant fires grant_secret immediately against a secret still processEnv server-side → rejected. Gating the live editor behind tierChanged (e.g. "Save first to manage grants") would close the trap.
Minor suggestions: pending grant text typed but not "Add"-ed is silently dropped on submit (PendingGrantsInput.tsx:24 — onBlur={add} or block submit while non-empty); the rotation auto-heal couples to the backend's error wording via message.includes('does not match') (secrets.ts:158) — a structured error code from harper#1554 would be more robust; and a mutation.reset() after success would shorten the window the plaintext value lingers in useMutation variables (in-memory only, low risk, just hygiene).
Two Gemini bot findings (warning icon has no title; delete double-fires) are rebutted by the actual head code — the icon has <title>/aria-label and the confirm button is disabled={busy} where busy includes isDeleting.
A couple of questions for you:
- Should the Secrets nav entry gate on
useInstanceManagePermission()like the other Config entries, or is browsable-to-all (metadata-only;list_secretsreturns no envelopes/values) the intended design? If intended, a one-line comment at the gate would keep it from being "fixed" later by mistake — a read-only user reaching the route will see secret names. - The write path can't be integration-tested until its backends land (harper#1554, harper-pro#512, central-manager#409), and the PR self-describes as a draft stacked on #1409 but isn't marked draft here — what's the intended merge posture?
Approve-worthy on the security core; the UX items are your call on timing relative to the backends.
—
🤖 Reviewed by KrAIs (Claude Opus 4.8) on Kris's behalf.
…saved tier switch Review feedback from Kris on #1402: - Managed-cluster banner flash: while the cluster lookup was pending, `cluster === undefined` read as self-managed, so the public-key query targeted the node and failed on a CM-custody cluster, flashing the read-only banner. Gate the key fetch on the cluster query settling (or no clusterId, i.e. local Studio, where the node is correct). - Live grants editor on an unsaved tier switch: flipping a processEnv secret to Scoped (not yet saved) rendered the live SecretGrantsEditor, whose Grant fires grant_secret against a still-processEnv secret → rejected. Gate it behind an unsaved change: show a "save as scoped first" hint until the scoped save lands. Minor items: - PendingGrantsInput commits a typed-but-not-Added grant on blur, so submitting the Add form no longer silently drops it. - Clear the set_secret mutation after each call so the plaintext value doesn't linger in useMutation variables. - Comment the Secrets nav gate: manage-gated like its siblings, and the secret ops are super_user-only server-side. Covers fix #2 with two jsdom tests (grants editor hidden on the unsaved switch, shown when the stored tier is already scoped). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks Kris — addressed the two significant items and most minors in
Not changed:
Left for Dawson:
🤖 Addressed by Claude Code |
… cluster lookup Follow-up to Barber AI review on #1402: - Key-source routing conflated "cluster info loading OR failed" with self-managed. getClusterInfoQuery has retry:false, so on error `data` stays undefined permanently — the prior !isPending gate re-enabled the key fetch but still routed a managed cluster to the node, a non-self-healing mis-route (set_secret would encrypt against the wrong key and fail a kid mismatch the retry can't heal). Gate the fetch on the cluster lookup actually succeeding (clusterTierKnown = !clusterId || clusterQuery.isSuccess), and surface a failed lookup as its own read-only banner instead of a misleading "no secrets key" one. - Centralize the kid-mismatch rotation sentinel (`does not match`) in one documented KID_MISMATCH_ERROR_PHRASE / isKidMismatchError, flagged as a cross-repo contract with harper#1554 to swap for a stable error code once one is exposed. Covered by the existing rotation-retry test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
IMO this can merge when we're ready, we can always make adjustments if we have a big shift in the API surface before 5.2.0 lands. It's properly gated, so no one will see this until we're ready to show it to them (or they opt in to seeing it, in which case, that's also fantastic for us). |
kriszyp
left a comment
kriszyp
left a comment
There was a problem hiding this comment.
Re-reviewed at c2fac6a — both significant items from my last pass are fixed correctly: the managed-cluster banner-flash / wrong-target key fetch, and the live grants editor firing against a still-processEnv secret on an unsaved tier switch. The client-side crypto core (plaintext never leaves the browser, envelope correctness, bounded rotation retry) re-verified intact, and the write-path backends have since merged so the earlier can't-integration-test caveat is resolved. Clean security work — approving.
— KrAIs (Claude Opus 4.8), reviewing on Kris's behalf
3 participants
What
Config → Secrets manages the replicated
system.hdb_secretstore: named, envelope-encrypted rows on the cluster itself, edited through the per-instance operations API and scoped to applications via grants. Built on the sharedSecretsManager/SecretModalscomponents from #1409.integrations/api/instance/secrets/secrets.ts— hooks forlist_secrets/set_secret/delete_secret/grant_secret/revoke_secret, plus public-key resolution:POST /ClusterSecrets, per central-manager#409) — CM is the custodian there, and Studio's key fetch is what triggers the first-use mint. Requires cluster-update rights on first use (the mint stamps fingerprints onto instances). The fetch waits for the cluster lookup to succeed before choosing node-vs-CM, so a managed cluster is never mis-routed to the node key (even on a failed lookup).get_secrets_public_keyoperation (file-tier custody mints its own key).lib/crypto/envSecret.ts, theenc:v1codec harper#1554 ported into core): values are encrypted in the browser and submitted as envelopes to the cluster's ownset_secret— plaintext never reaches the operations API, the operation log, disk, or CM, and can never be read back.secretsaccessor —const { NAME } = secrets— by the apps you grant) or a global environment variable (process.env.NAME, every component and its child processes). The add/edit dialogs let you pick the tier and show a copy-paste-correct, name-aware code example for reading it; the two tiers are mutually exclusive server-side (a global secret can't be scoped).grant_secretnever fires. Per-row warning icons flag rows whosekidno longer matches the cluster's custody key (or storedunverified) — they may fail to decrypt until re-saved..mjsfiles #409 CM) the list stays browsable, a banner explains why, and add/edit is disabled. A failed cluster lookup gets its own distinct read-only banner rather than the misleading "no key" one.>= 5.2.0-alpha.1, so 5.2 beta/prerelease builds pass too) and manage-gated like every other Config entry; the secret operations are super_user-only server-side. The store ships in core, so it isn't restricted to Fabric-managed clusters.Verification
tsc,oxlint,dprintclean; full suite green (1289 tests). Secrets-hook tests run real WebCrypto RSA keys, asserting plaintext never appears in a request body, the CM-vs-node key routing, and the rotation retry re-encrypting under the newly served key; jsdom tests drive the delivery-tier dialog end-to-end (theprocess.envvssecretsexample swap, the submitted{ processEnv, grants }, and the grant-scoping guard on an unsaved tier switch)..mjsfiles #409): the version gate correctly hides the nav entry, and direct navigation shows the degraded state (banner, read-only, empty list) with the key fetch correctly routed to CM's/ClusterSecretsfor the managed cluster..mjsfiles #409 — covered by the hook tests until then.Notes / open items
5.2.0-alpha.1(matching harper#1554's upgrade directive, tagged 5.2.0) so alpha/beta prerelease builds pass too; retag if it ships in a different release./ClusterSecretsisn't in central-manager's generated OpenAPI spec (the generator only emits method-level@pathJSDoc, and these resources carry class-level docs), so regenerating the SDK won't type it — the plain-Axios view is the standing approach, not a stopgap.does not match), centralized in one documented constant as a cross-repo contract; swap for a stable errorcodeonce the backend exposes one.get_componentswould be a nice follow-up.ClusterSecretsstore from central-manager#405 (control-plane values + host-manager value injection). That model was superseded by the two-key custody plan (harper-pro#166 / central-manager#409: CM keeps the key, the cluster keeps the ciphertext), and this page was retargeted accordingly.🤖 Generated with Claude Code
Related PRs — secrets management
One
enc:v1:client contract everywhere (public key + envelope; values encrypted before they leave the client):.envprotection in the operations API — merged (consumed by feat(secrets): version-adaptive .env editor in the application editor #1409)enc:v1:decrypt hook + envelope contract — mergedhdb_secretstore with grant-scoped secret operations +get_secrets_public_key— mergedfile(self-hosted) +injected(Fabric) providers; supersedes harper-pro#505 — merged.envpanel — merged