You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
feat(broker): store provider client secrets in GCP Secret Manager - #113
First external secret backend for #63. With SECRET_BACKEND=gcp-secret-manager, the Broker stores each OAuth2 provider's client_secret as its own Secret Manager secret and leaves provider_profiles.client_secret NULL, so application credentials no longer sit next to metadata or in database backups. The default internal backend is unchanged.
Design deviations from the issue, posted there for discussion: only client_secret moves (not client_id), secrets are keyed by provider UUID (not name), and GCP ships first (not HashiCorp Vault).
Type of Change
Bug fix
New feature
Breaking change
Documentation update
Refactor
Changes Made
nexus-broker/pkg/vault/tokens: existing AES-GCM token encryption, moved from pkg/vault (first commit, no behaviour change).
nexus-broker/pkg/vault/providers: Backend interface, ParseKind, in-memory backend for tests.
nexus-broker/pkg/vault/providers/gcp: Secret Manager implementation. One secret per provider, automatic replication, managed-by=nexus-broker label; Set adds a version and destroys earlier enabled versions; Ping runs Set twice, Get and Delete on a random probe key under the prefix at startup, so every permission a registration needs (secret create/delete, version add/access/list/destroy) is verified before the Broker serves traffic; the probe secret lives for under a second and is removed on every exit path.
nexus-broker/pkg/provider/store.go: NewStore(db, opts...) with WithSecretBackend. Register writes the row then the secret and removes the row if the secret write fails. Patch and update rotate. Delete removes the secret. Reads resolve from the backend only when the column is NULL, so pre-existing rows keep working.
nexus-broker/pkg/config/broker.go: SECRET_BACKEND, GCP_PROJECT_ID, GCP_SECRET_PREFIX, validated at boot. main.go pings the backend and refuses to start on failure.
deploy/terraform/gcp/secret-backend: custom role with secretmanager.secrets.create plus roles/secretmanager.admin conditioned on resource.name.startsWith(".../secrets/<prefix>-").
Review hardening (later commits): typed backend and validation errors mapped to 503/502/400, missing-secret state that refuses to send an empty secret, concurrent-safe rotation and delete-by-name, bounded startup probe, Go 1.26 toolchain for the broker.
VERSION 0.5.0 (minor: new feature), stamped.
How to Test
cd nexus-broker && go test ./... — unit tests cover register/rollback, resolve, missing-secret, non-OAuth skip, patch, update, delete and delete-by-name against the in-memory backend; the internal path is asserted unchanged.
NEXUS_TEST_GCP_PROJECT=<project> go test ./pkg/vault/providers/gcp/ -run TestBackend_RoundTrip -v with ADC that can write secrets — create, rotate, read, delete. Skips in CI.
Run the Broker with SECRET_BACKEND=gcp-secret-manager GCP_PROJECT_ID=<project> against local Postgres and Redis, POST /providers with a client secret: the row's client_secret is NULL, the secret nexus-providers-<id> exists, GET /providers/by-name/... is still redacted, PATCH with a new client_secret leaves one enabled version, DELETE removes the secret. Done against nexus-cloud-dae3 on 2026-10-10.
terraform validate in deploy/terraform/gcp/secret-backend.
Migration / Breaking Changes
Database migration required
No migration required
Opt-in via SECRET_BACKEND. Existing deployments are unaffected. Existing rows are not migrated into the backend; that is a follow-up.
Checklist
Code follows the Go styleguide (gofmt applied)
Commit messages use present tense, imperative mood, ≤72 chars
pkg/vault becomes the umbrella for the broker's two secret concerns: the
AES-GCM token vault (now pkg/vault/tokens) and the provider client-secret
backends that follow in the next commit. Call sites import the package as
tokenvault. No behaviour change.
Refs #63
Add SECRET_BACKEND with two implementations: internal (the existing
provider_profiles.client_secret column, still the default) and
gcp-secret-manager, which keeps one Secret Manager secret per provider
named <GCP_SECRET_PREFIX>-<provider id> and leaves the column NULL.
The provider store takes the backend as an option. Register writes the
row then the secret and removes the row again if the secret write fails.
Patch rotates the secret and destroys earlier versions. Delete removes
the secret. Reads resolve the secret from the backend only when the
column is NULL, so rows written before the switch keep working. Secrets
are keyed by provider id, not name, so renames never orphan them. Only
client_secret moves; client_id is not a secret.
The broker pings the backend at startup and refuses to boot if it cannot
reach Secret Manager under the prefix.
deploy/terraform/gcp/secret-backend grants the broker service account
secretmanager.secrets.create on the project (create cannot be scoped by
name) plus Secret Manager admin conditioned on the secret-name prefix,
so infrastructure secrets in the same project stay unreadable to it.
Unit tests cover the store against an in-memory backend. The GCP
round-trip test runs only when NEXUS_TEST_GCP_PROJECT is set.
Closes#63 for GCP; Vault and AWS backends follow on the same interface.
- Set reports a failure to retire earlier secret versions instead of
returning success; the new version is live either way, and the caller
can retry since Set is idempotent.
- GCP_SECRET_PREFIX is validated only when the GCP backend is selected,
so a stale value cannot stop an internal deployment from booting.
- A registration whose secret write fails removes the external secret
as well as the row, since the write may have failed part way.
The Terraform module also accepts a prefix longer than 218 characters, even though appending -<provider UUID> then exceeds Secret Manager's 255-character secret ID limit. Mirror the broker's generated-name length constraint so invalid deployments fail during validation.
Prefix validation allows Secret Manager IDs over 255 characters
nexus-broker/pkg/config/broker.go:182
The charset check accepts prefixes that can never form a valid Secret Manager ID. Secret IDs are limited to 255 characters, and the generated prefix + "-" + UUID adds 37 characters, so prefixes longer than 218 pass startup validation but every registration fails. Enforce the generated-ID length here.
Missing secrets are treated as successful reads
nexus-broker/pkg/provider/store.go:75
Treating a missing external secret as a successful read causes OAuth flows to send an empty client_secret. It also makes the health worker classify the provider's typical invalid_client 400/401 response as healthy, contrary to the stated missing-secret behavior. Preserve a typed missing-secret state so execution and health paths fail clearly while metadata-only reads can remain available.
This issue also appears in the following locations of the same file:
line 392
line 397
line 435
Soft delete leaves secrets behind after backend failures
nexus-broker/pkg/provider/store.go:556
Once the soft delete succeeds, a backend failure returns immediately and leaves this and all later secrets behind. Retrying by name cannot clean them up because the ID query only selects non-deleted rows. Include historical IDs for idempotent cleanup and attempt every deletion before returning the joined errors.
- Rotation retires only versions numerically older than the one just
added, so two concurrent rotations cannot destroy each other's value.
- GCP_SECRET_PREFIX is capped at 218 characters in the broker and in the
Terraform module, so <prefix>-<uuid> always fits Secret Manager's
255-character id limit.
- A provider whose secret is absent from the backend is flagged
ClientSecretMissing: metadata reads still work, but exchange, refresh
and revocation refuse with provider_secret_missing and the health
probe reports unhealthy, instead of sending an empty secret that the
provider would reject in a way that looked healthy.
- DeleteProfileByName drops the secret of every row with that name,
including rows soft-deleted earlier, attempts every drop, and joins
the errors, so a retry after a backend failure cleans up.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Legacy PUT handling can lose secrets, missing providers can create orphaned secrets, and the Go 1.26 requirement conflicts with existing CI and Docker toolchains.
PATCH creates orphaned secrets for inactive providers
nexus-broker/pkg/provider/store.go:466
A PATCH containing client_secret creates the external secret before confirming that this UUID identifies an active provider. For a missing or soft-deleted ID, the later UPDATE affects zero rows without error, so the request succeeds and leaves an orphaned secret. Verify the row first or check RowsAffected and clean up the secret on zero rows.
…1.26
- UpdateProfile with an external backend clears the column only when a
new secret was written; a PUT that omits client_secret leaves a
legacy row's column value in place instead of nulling it.
- PATCH and PUT that wrote a secret verify the UPDATE matched an active
row; on zero rows the secret is removed again and the caller gets
not-found, so an unknown or soft-deleted id cannot orphan a secret.
- The Secret Manager client and the x/* modules it pulls in require Go
1.26, so the broker Dockerfile builds with golang:1.26-alpine and CI
installs the toolchain from nexus-broker/go.mod, the highest directive
in the repo, rather than relying on toolchain auto-download.
Moving these exported helpers removes vault.Encrypt and vault.Decrypt, so consumers of the public pkg/vault package no longer compile despite this being described as a no-behavior-change refactor. Keep deprecated forwarding wrappers in pkg/vault while introducing the new package, or explicitly mark this as a breaking API change.
Bound startup probe with a timeout
nexus-broker/cmd/nexus-broker/main.go:102
Bound the startup probe with a deadline. context.Background() has no cancellation, so an unreachable Secret Manager endpoint or prolonged client retries can hang startup indefinitely instead of failing fast as documented.
- PUT and PATCH with an external backend refuse an empty client_secret;
clearing a secret is done by deleting the provider.
- pkg/vault keeps Encrypt and Decrypt as deprecated wrappers around
pkg/vault/tokens, so the move does not break importers.
- The startup backend probe runs under a 15s deadline so an unreachable
Secret Manager fails fast instead of hanging in client retries.
Missing-secret status is checked after premature discovery failures
nexus-broker/pkg/provider/health.go:126
The missing-secret check runs after discovery and the empty-token-URL return. Consequently, a profile flagged ClientSecretMissing can be reported as a discovery failure or unknown instead of the promised missing-secret unhealthy result. Check this flag immediately after identifying an OAuth provider, before either early return.
Backend read failures are misclassified as missing providers
nexus-broker/pkg/provider/store.go:80
Backend read failures now share the same GetProfile error channel as a missing database row. Existing callers classify any such error as provider_not_found/404 (and GetByName returns the raw error), so a Secret Manager timeout is falsely reported as a missing provider and may expose cloud error details. Return a distinguishable backend-unavailable error and map it to a sanitized 5xx response.
Registration maps backend failures to 400 and exposes internal errors
nexus-broker/pkg/provider/store.go:285
A putSecret or cleanup failure is operational, but ProvidersHandler.Register maps every RegisterProfile error to HTTP 400 and returns its text. A transient GCP outage will therefore look like a non-retryable client error and can expose backend details. Use a typed backend error and have registration return a sanitized 5xx response.
This startup probe only exercises secretmanager.secrets.get. The Terraform module deliberately grants secretmanager.secrets.create through a separate project-level custom role, so a deployment missing that binding will pass Ping and then fail on its first registration. Either verify every permission needed by Set/Delete (for example via IAM permission checks or a create/delete probe), or narrow the documented fail-fast guarantee.
…artup
- Store errors from the secret backend wrap provider.ErrSecretBackend.
Handlers map it to a sanitized 503 instead of a 400 or 404 carrying
the cloud error; the exchange and refresh services map it to 502
instead of provider_not_found.
- The health prober checks ClientSecretMissing before discovery and the
token-URL check, so the missing-secret verdict is never masked.
- The startup probe creates, reads and deletes a probe secret under the
prefix (nil UUID, never versioned, never billed), so a missing create
binding fails at boot rather than on the first registration.
The backend-unavailable mapping is only applied to token exchange. CreateConsentSpec also calls GetProfile, which can now return ErrSecretBackend, but its existing branch at connection.go:149-152 reports that outage as provider_not_found. Apply the same secret_backend_unavailable mapping there so starting an OAuth flow does not return a false 404 during a Secret Manager outage.
This startup check does not prove the permissions required by Set and Get: it never adds, accesses, or destroys a secret version. An IAM setup with create/get/delete but without version permissions passes Ping and then fails on the first registration or OAuth exchange, contrary to the fail-fast contract. Exercise a temporary version through add/access/destroy before deleting the probe secret.
The startup probe now runs the backend's own Set, Get and Delete on a
random key, which exercises every permission a registration needs,
including version add, access, list and destroy. A per-call key means
replicas starting together cannot delete each other's probe, and the
probe is removed on every exit path.
CreateConsentSpec maps a backend outage to secret_backend_unavailable
like the exchange path, instead of a false provider_not_found.
The new client_secret validation errors from UpdateProfile and PatchProfile fall through this helper using the callers' 500 status. Thus an empty or non-string client secret is reported as an internal failure rather than invalid input. Introduce a typed/sentinel validation error and map it to HTTP 400 here.
Non-atomic ID lookup can orphan concurrently registered secrets
nexus-broker/pkg/provider/store.go:614
The ID lookup and soft-delete are separate statements. On a retry where only historical rows exist, a concurrent registration can insert the same name after this SELECT; the later UPDATE deletes that new row, but its ID is absent from ids, leaving its backend secret orphaned. Obtain active IDs from UPDATE ... RETURNING id atomically, while retaining a separate retry path for already-deleted rows.
Probe does not verify DestroySecretVersion permission
This probe creates only one version, so Set lists the new version but never calls DestroySecretVersion. Startup can therefore succeed without the destroy permission and the first real rotation will fail. Write the probe twice so the second Set must retire the first version.
A successful backend read is treated as a usable secret even when the backend returns an empty or whitespace-only payload. That leaves ClientSecretMissing false, so RequireClientSecret and the health probe send the blank value instead of failing closed (the Backend contract does not prohibit such values, and secrets can be changed out of band). Mark blank values as missing before assigning ClientSecret.
- A rotation through PUT or PATCH writes the backend and the row inside
one transaction holding pg_advisory_xact_lock on the provider id, so
two concurrent updates cannot pair one request's metadata with the
other's secret, across replicas.
- A blank value read from the backend is treated as missing.
- A rotation that matches no active row returns ErrProviderNotFound and
the handler answers 404 instead of 500.
Verified end to end against a real Postgres with the GCP backend:
register, PATCH and PUT rotation, PUT without secret preserving it,
empty secret rejected with 400, unknown id leaving no orphan, delete.
Cleanup failures are discarded even though this probe is specifically meant to detect incomplete permissions. For example, if the project-level create grant exists but the prefixed admin grant is missing, the first Set can create a secret container, fail adding its version, and this deferred delete also fails with permission denied; every restart then leaks another random probe resource. Verify delete access before creating the probe and surface cleanup failures, or use a probe naming/cleanup strategy that can recover leftovers.
- With an external backend, registration chooses the id, inserts the row
inside a transaction, writes the secret, and commits only then. No
reader sees a row without its secret, a crash in between leaves no
row, and a failed commit removes the secret it had written.
- Handlers check backend failures before not-found, so a rotation that
missed its row and also failed to clean up reads as retryable.
- The startup probe reports a cleanup failure with the probe secret id
instead of discarding it.
Verified against a real Postgres: register, read, delete.
Document secret rotation or deletion before re-registering
docs/guides/provider-secret-backends.md:33
An active missing-secret provider cannot be re-registered directly because RegisterProfile rejects duplicate names. Document the actual recovery path: rotate the secret with PATCH/PUT, or delete the existing provider before registering it again.
Document supported recovery instead of re-registering active providers
nexus-broker/pkg/provider/health.go:101
The suggested re-register action fails while this active row exists because registration rejects duplicate provider names. Point users to the supported in-place recovery (PATCH a new client_secret) or explicitly tell them to delete before re-registering.
A commit error may mean the row was committed and only the
acknowledgement was lost. The secret is dropped only after the database
confirms the row is absent; if it exists or cannot be checked, it is
kept. Health and docs now name the supported recovery for a missing
secret: PATCH a new client_secret, or delete and register again.
Reject whitespace-only client secrets during registration
nexus-broker/pkg/provider/store.go:362
Registration rejects only the exact empty string before this branch, while reads treat any whitespace-only backend value as missing. A client_secret such as " " therefore returns success, commits a NULL column, and becomes unusable on the next read. Reject the same trimmed-empty values here that rotation already rejects.
Prevent inconsistent reads during credential rotation
nexus-broker/pkg/provider/store.go:528
The advisory lock serializes writers, but GetProfile/GetAllProfiles do not acquire it. Since putSecret publishes the new latest version before this transaction updates fields such as client_id and auth_header, an OAuth request can read the new secret together with the old metadata during a PUT/PATCH. Use an immutable secret version/key referenced atomically by the database row, or make reads participate in the same lock across row loading and secret resolution; otherwise credential rotation is not read-consistent.
Review status after fourteen Copilot passes: every thread is resolved with a code change and verified against a real Postgres plus the live project. One summary note is deliberately not addressed here: read consistency during a rotation (new secret visible with previous row metadata for the milliseconds between the backend write and the row update). That needs an immutable version reference on the row and a migration, and is tracked as #114.
The core Secret Manager lifecycle is not exercised by an always-run test: the unit test only covers version-name comparison, while TestBackend_RoundTrip skips whenever cloud credentials are absent. Regressions in create/AlreadyExists handling, rotation, partial failures, and version retirement can therefore reach production unnoticed. Wrap the client calls behind a small interface/fake and add deterministic unit coverage for these paths.
This recovery guidance omits that PATCH under the internal backend only repopulates PostgreSQL; it cannot delete the existing GCP secret. Deleting the provider while internal is active also leaves that GCP secret behind. Document the required Secret Manager cleanup (or a switch-back procedure performed while the GCP backend is still configured) so operators do not retain a duplicate credential indefinitely.
An existence check right after a failed commit can run while the commit
is still completing and see no row. The secret is now kept on any commit
error and the error names it, so an operator can remove it if the row
turns out to be absent. Document Secret Manager cleanup when recovering
under the internal backend.
Avoid secret resolution for metadata-only provider operations
nexus-broker/pkg/provider/store.go:389
Resolving the secret in every GetProfile call makes metadata-only operations depend on Secret Manager. ProvidersHandler.Get immediately redacts this value, and CreateConsentSpec uses only authorization metadata/client ID, yet either operation now fails during a backend outage and performs an unnecessary secret access. Split metadata loading from credential resolution (or resolve explicitly only in exchange, refresh, revocation, and health paths).
This issue also appears on line 446 of the same file.
Test fail-closed health behavior when client secret is missing
nexus-broker/pkg/provider/health.go:102
The new fail-closed branch is not exercised by health_test.go. Add a test with ClientSecretMissing: true that asserts an unhealthy result and verifies the token endpoint is never contacted, so a regression cannot silently send an empty secret or report the provider healthy.
The Terraform module binds a custom role with only get, delete and
version add/access/list/destroy under the prefix condition, instead of
roles/secretmanager.admin, which also carried IAM-policy and metadata
administration. A health test asserts a provider with a missing secret
is unhealthy and the token endpoint is never contacted.
This new operational commit failure is untyped, so ProvidersHandler.Register falls through to its default HTTP 400 response. A database commit with an unknown outcome is a server failure, not an invalid registration; returning 400 can prevent retries and leaves clients unable to distinguish the possible committed row/orphaned secret. Add a typed database/commit error and map it to a sanitized 5xx response.
PUT ignores missing provider when client_secret is omitted
nexus-broker/pkg/provider/store.go:510
When client_secret is omitted, this external-backend PUT never checks RowsAffected, so an unknown or soft-deleted provider returns success (and the handler emits an update audit event). The same PUT with a secret returns ErrProviderNotFound, making existence semantics depend on whether a secret was supplied. Check the result and return ErrProviderNotFound when no active row matched.
PATCH ignores missing provider without secret rotation
nexus-broker/pkg/provider/store.go:659
A PATCH that does not rotate the secret also ignores RowsAffected, so PATCHing an unknown or deleted provider returns 200 and produces a misleading audit event, while a secret-bearing PATCH correctly returns 404. Check the update result and return ErrProviderNotFound when no active row matched.
One secret resolution error prevents all health checks
nexus-broker/pkg/provider/store.go:786
resolveSecrets joins operational errors from every provider, and this all-or-nothing return makes HealthWorker.runChecks exit before updating any status. Thus one per-secret permission problem or transient access failure leaves every provider's previous health status stale. Preserve per-profile resolution errors so the worker can mark only affected providers unhealthy and continue checking the rest.
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
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.
Pull Request
Description
First external secret backend for #63. With
SECRET_BACKEND=gcp-secret-manager, the Broker stores each OAuth2 provider'sclient_secretas its own Secret Manager secret and leavesprovider_profiles.client_secretNULL, so application credentials no longer sit next to metadata or in database backups. The defaultinternalbackend is unchanged.Design deviations from the issue, posted there for discussion: only
client_secretmoves (notclient_id), secrets are keyed by provider UUID (not name), and GCP ships first (not HashiCorp Vault).Type of Change
Changes Made
nexus-broker/pkg/vault/tokens: existing AES-GCM token encryption, moved frompkg/vault(first commit, no behaviour change).nexus-broker/pkg/vault/providers:Backendinterface,ParseKind, in-memory backend for tests.nexus-broker/pkg/vault/providers/gcp: Secret Manager implementation. One secret per provider, automatic replication,managed-by=nexus-brokerlabel;Setadds a version and destroys earlier enabled versions;PingrunsSettwice,GetandDeleteon a random probe key under the prefix at startup, so every permission a registration needs (secret create/delete, version add/access/list/destroy) is verified before the Broker serves traffic; the probe secret lives for under a second and is removed on every exit path.nexus-broker/pkg/provider/store.go:NewStore(db, opts...)withWithSecretBackend. Register writes the row then the secret and removes the row if the secret write fails. Patch and update rotate. Delete removes the secret. Reads resolve from the backend only when the column is NULL, so pre-existing rows keep working.nexus-broker/pkg/config/broker.go:SECRET_BACKEND,GCP_PROJECT_ID,GCP_SECRET_PREFIX, validated at boot.main.gopings the backend and refuses to start on failure.deploy/terraform/gcp/secret-backend: custom role withsecretmanager.secrets.createplusroles/secretmanager.adminconditioned onresource.name.startsWith(".../secrets/<prefix>-").docs/guides/provider-secret-backends.md, config table rows, deploy page note, broker README, changelog 0.5.0.docker-compose.ymland.env.examplewireSECRET_BACKEND,GCP_PROJECT_ID,GCP_SECRET_PREFIX.VERSION0.5.0 (minor: new feature), stamped.How to Test
cd nexus-broker && go test ./...— unit tests cover register/rollback, resolve, missing-secret, non-OAuth skip, patch, update, delete and delete-by-name against the in-memory backend; the internal path is asserted unchanged.NEXUS_TEST_GCP_PROJECT=<project> go test ./pkg/vault/providers/gcp/ -run TestBackend_RoundTrip -vwith ADC that can write secrets — create, rotate, read, delete. Skips in CI.SECRET_BACKEND=gcp-secret-manager GCP_PROJECT_ID=<project>against local Postgres and Redis,POST /providerswith a client secret: the row'sclient_secretis NULL, the secretnexus-providers-<id>exists,GET /providers/by-name/...is still redacted,PATCHwith a newclient_secretleaves one enabled version,DELETEremoves the secret. Done againstnexus-cloud-dae3on 2026-10-10.terraform validateindeploy/terraform/gcp/secret-backend.Migration / Breaking Changes
Opt-in via
SECRET_BACKEND. Existing deployments are unaffected. Existing rows are not migrated into the backend; that is a follow-up.Checklist
gofmtapplied)