feat(secrets): wire the secretspec provider hard-delete (RIG-3436) - #1066
Open
rigel-mintaka wants to merge 1 commit into
Open
feat(secrets): wire the secretspec provider hard-delete (RIG-3436)#1066rigel-mintaka wants to merge 1 commit into
rigel-mintaka wants to merge 1 commit into
Conversation
|
❌ This pull request could not start testing because there was a merge conflict. See more details here.
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
Compass engineering docs preview: https://compass-server-rig-3436-prov.compass-eng-docs.pages.dev Deployed from |
mattwilkinsonn
approved these changes
Sep 11, 2026
mattwilkinsonn
approved these changes
Sep 11, 2026
`SpecResolver.Delete` was a validate-only no-op, documented as an upstream gap. The `secretspec delete` verb has existed since 0.18 and the pin is 0.20.0, so an operator delete left the provider value in the backing store — a data-retention gap, not a correctness bug, since an undeclared name never resolves. Delete now shells the real verb, mirroring the Set path: a manifest declaring just this name, argv from a pure `deleteArgs` builder, stderr captured, and an error wrapping name/cli/stderr but never a value. The audit `reason` is threaded through as Set already threads it. Arming that method arms it for BOTH callers, so both doors needed work. The F1 guard now covers each delete door. `HasServerSecretPrefix` was enforced only at `store.DeclareSecret`, which `SetSecret` reaches via its declare step — but neither delete path declares. On the user door a reserved-prefix name reached the resolver unguarded, so a user deleting `GATEWAY_CREDENTIALS_MASTER_KEY` would have destroyed the master key and stranded every encrypted credential row. On the admin door the inverse held: no prefix check and no declaration check, so an admin could delete ANY name from the shared provider keyspace — including an unprefixed one the user keyspace owns, leaving that user's required=true declaration pointing at nothing and poisoning every live session's resolve. Both doors now check before any store or provider call. The reject side is case-INSENSITIVE via a new `ShadowsServerSecretPrefix`. The name grammar admits lowercase, and a provider keyspace's case sensitivity is provider-dependent, so on a case-insensitive provider `gateway_credentials_master_key` reaches the real master key's value. The byte-exact `HasServerSecretPrefix` stays the ADMIT check, because the `server_secrets` CHECK behind it is itself case-sensitive — admitting a variant would pass the Go door and fail in Postgres. Reject wide, admit narrow. Both doors now delete the declaration FIRST. The declaration is what Resolve reads: on the user path an orphan poisons every live session's FetchSecrets, and on the server path it fails the forge lanes at request time and hard-fails the next boot at `validateForgeSecret`. A provider failure after the row is gone is CodeUnavailable and deliberately not rolled back — re-creating the row would point a required=true entry at a value that may already be gone. The user-facing error now names the recovery, since re-running the delete returns NotFound and never re-attempts the provider. Red-green verified against the actual prior logic, not a hypothetical. Reverting the admin guard reds both new admin tests, one with `server resolver deleted [SERVER_NEVER_DECLARED] for a never-declared name` — the destructive call on an undeclared name. Reverting the case-fold reds all three case variants. Reverting the user guard or the ordering reds their own tests. Refs RIG-3436 Co-authored-by: Matt Wilkinson <matt@rigel.build>
rigel-mintaka
force-pushed
the
compass-server/rig-3436-provider-hard-delete
branch
from
September 11, 2026 02:27
f1eb56f to
0a1fefe
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SpecResolver.Deletewas a validate-only no-op, documented as an upstreamgap. The
secretspec deleteverb has existed since 0.18 and the pin is0.20.0, so an operator delete left the provider value in the backing
store — a data-retention gap, not a correctness bug, since an undeclared
name never resolves.
Delete now shells the real verb, mirroring the Set path: a manifest
declaring just this name, argv from a pure
deleteArgsbuilder, stderrcaptured, and an error wrapping name/cli/stderr but never a value. The
audit
reasonis threaded through as Set already threads it.Arming that method arms it for BOTH callers, so both doors needed work.
The F1 guard now covers each delete door.
HasServerSecretPrefixwasenforced only at
store.DeclareSecret, whichSetSecretreaches via itsdeclare step — but neither delete path declares. On the user door a
reserved-prefix name reached the resolver unguarded, so a user deleting
GATEWAY_CREDENTIALS_MASTER_KEYwould have destroyed the master key andstranded every encrypted credential row. On the admin door the inverse
held: no prefix check and no declaration check, so an admin could delete
ANY name from the shared provider keyspace — including an unprefixed one
the user keyspace owns, leaving that user's required=true declaration
pointing at nothing and poisoning every live session's resolve. Both
doors now check before any store or provider call.
The reject side is case-INSENSITIVE via a new
ShadowsServerSecretPrefix.The name grammar admits lowercase, and a provider keyspace's case
sensitivity is provider-dependent, so on a case-insensitive provider
gateway_credentials_master_keyreaches the real master key's value. Thebyte-exact
HasServerSecretPrefixstays the ADMIT check, because theserver_secretsCHECK behind it is itself case-sensitive — admitting avariant would pass the Go door and fail in Postgres. Reject wide, admit
narrow.
Both doors now delete the declaration FIRST. The declaration is what
Resolve reads: on the user path an orphan poisons every live session's
FetchSecrets, and on the server path it fails the forge lanes at request
time and hard-fails the next boot at
validateForgeSecret. A providerfailure after the row is gone is CodeUnavailable and deliberately not
rolled back — re-creating the row would point a required=true entry at a
value that may already be gone. The user-facing error now names the
recovery, since re-running the delete returns NotFound and never
re-attempts the provider.
Red-green verified against the actual prior logic, not a hypothetical.
Reverting the admin guard reds both new admin tests, one with
server resolver deleted [SERVER_NEVER_DECLARED] for a never-declared name— thedestructive call on an undeclared name. Reverting the case-fold reds all
three case variants. Reverting the user guard or the ordering reds their
own tests.
Refs RIG-3436
Co-authored-by: Matt Wilkinson matt@rigel.build