feat(client): unified OAuth Client registry (machine & agent identity foundation)#648
Merged
Conversation
…, and M2M constants Introduces the two new storage entities for the workload identity program: - ServiceAccount: machine/workload OAuth2 client (client_id = UUID, client_secret = bcrypt hash). Intentionally omits all human-user fields. - TrustedIssuer: external JWT issuer registry for K8s SA tokens, SPIFFE JWT-SVIDs, and generic OIDC workload tokens. Carries all fields needed for Phases 1-6: KeySourceType, ExpectedAud, EnableTokenReview (Phase 4), SpiffeRefreshHintSeconds (Phase 5), AuthMethod + proxy fields (Phase 6). Storage changes: - Added ServiceAccount and TrustedIssuer to CollectionList and Collections - Extended storage.Provider interface with 5 ServiceAccount + 6 TrustedIssuer methods - SQL provider: full GORM CRUD implementation + AutoMigrate registration - NoSQL providers (mongodb, arangodb, cassandradb, dynamodb, couchbase): stubs returning not-implemented errors, to be replaced in PR 3 Constants: - grant_types.go: RFC 6749/8693 grant types, RFC 7521/7523 client assertion types, RFC 8693 token type URNs, and TrustedIssuer key-source/issuer-type/ auth-method identifiers - audit_event.go: service_account and trusted_issuer audit events + resource types Spec: docs/specs/WORKLOAD_IDENTITY_PROGRAM.md
- Add gorm:"index" on TrustedIssuer.ServiceAccountID (was missing; SQL ListTrustedIssuers filters on this column and would full-table-scan) - Fix Count error swallowed in ListServiceAccounts and ListTrustedIssuers; now checks countRes.Error consistent with webhook.go pattern - Capture time.Now().Unix() once per Add call so CreatedAt == UpdatedAt - Guard UpdateServiceAccount/UpdateTrustedIssuer against partial-struct callers: returns error if CreatedAt == 0 (load-then-mutate enforced) - Cascade-delete TrustedIssuers inside DeleteServiceAccount, matching the webhook/webhooklog pattern; prevents orphaned issuers post-delete - AllowedScopes comment: deny-all on empty, trim+drop-empty requirement - IsActive gorm default:true comment: documents the zero-value footgun - Add AuditServiceAccountDeactivated/Activated and AuditTrustedIssuerTokenReviewChanged events for queryable IR signals
…providers
Replace the placeholder stubs in mongodb, arangodb, cassandradb, dynamodb,
and couchbase with real implementations matching the SQL reference semantics.
- UUID id generation and Key=ID parity on create; unix CreatedAt/UpdatedAt
- Update rejects a partial struct (CreatedAt==0 guard) per provider
- DeleteServiceAccount cascades to its TrustedIssuer rows
- ListTrustedIssuers optionally filters by serviceAccountID; pagination
returns {Limit, Page, Offset, Total}
- GetTrustedIssuerByIssuerURL is an indexed/keyed lookup (client_assertion
hot path): mongo/arango/cassandra/couchbase secondary index, dynamo GSI
- Per-provider collection/table setup added where the provider requires it
(mongo CreateCollection+indexes, arango CollectionExists+EnsureHashIndex,
cassandra CREATE TABLE + secondary indexes, dynamo ensureTables entries);
couchbase collections+primary indexes auto-create via reflection, only
secondary indexes added
Prevents accidental exposure through structured logging, webhook payloads, or error dumps. Matches the existing User.Password convention.
github.com/openfga/openfga@v1.17.1 skips OIDC audience validation when --authn-oidc-audience is unset. We embed the FGA server in-process via server.WithDatastore and never wire its OIDC authn middleware, so the vulnerable path isn't reachable at runtime, but govulncheck flags it via static call-graph reachability and fails CI on every branch.
github.com/openfga/openfga@v1.17.1 skips OIDC audience validation when --authn-oidc-audience is unset. We embed the FGA server in-process via server.WithDatastore and never wire its OIDC authn middleware, so the vulnerable path isn't reachable at runtime, but govulncheck flags it via static call-graph reachability and fails CI on every branch.
…se1-schema feat(m2m): ServiceAccount + TrustedIssuer schemas, storage interface, M2M constants [Phase 1 — PR 1/5]
…se1-nosql-providers feat(m2m): NoSQL provider implementations for ServiceAccount + TrustedIssuer [Phase 1 — PR 2/5]
Wire the Phase 1 workload-identity service layer and admin GraphQL surface on top of the existing storage CRUD. - service layer: CreateServiceAccount/Update/Delete/RotateSecret/get/list and AddTrustedIssuer/Update/Delete/get/list, each gated by requireSuperAdmin and audited via the existing admin.* audit constants - secrets: 32-byte crypto/rand plaintext, bcrypt cost 12, hash-only storage, one-time reveal via CreateServiceAccountResponse; reads never surface it - allowed_scopes normalized (trim, drop-empty, dedupe) and rejected when empty on both create and update - IsActive set explicitly on create (never rely on the GORM default) - GraphQL schema types/inputs/queries/mutations + regenerated gqlgen output - AsAPIServiceAccount/AsAPITrustedIssuer converters (no client_secret field) - integration tests: one-time secret reveal, rotation invalidates old secret, service-account delete cascades to trusted issuers, empty-scope rejection, partial-update preservation, super-admin enforcement
UpdateTrustedIssuer let a caller blank expected_aud even though AddTrustedIssuer already rejects it empty. expected_aud backs the audience-binding invariant the token endpoint will enforce later, so the update path shouldn't be the hole that lets it go empty.
…se1-service-graphql feat(m2m): service layer + admin GraphQL API for ServiceAccount + TrustedIssuer [Phase 1 — PR 3/5]
- 11 admin RPCs (create/update/delete/rotate/get/list service accounts, add/update/delete/get/list trusted issuers) over gRPC + REST gateway - client secret surfaced only via CreateServiceAccountResponse; the ServiceAccount proto message has no secret field so get/list/update cannot leak it - thin handlers delegate to the existing AdminProvider; projections mirror the webhook pattern
…se1-grpc feat(m2m): gRPC admin RPCs for ServiceAccount + TrustedIssuer [Phase 1 — PR 4/5]
Implements the RFC 6749 §4.4 client_credentials grant for service account (machine/workload) identities — the final PR of Workload Identity Phase 1. - Extend AuthTokenConfig with ServiceAccountID; CreateAccessToken branches to a machine token (sub=service account id, scope only, no roles/allowed_roles, no custom-user script). Human token path is unchanged. New CreateMachineAuthToken issues an access token only — no id_token, no refresh_token, no session token. - Machine tokens are stateful like all access tokens: registered in the memory store under a namespaced session key (service_account:<id>). login_method=service_account makes ValidateAccessToken derive the same key with zero changes to the validation path. - Token endpoint: dual-path client auth (Basic or form body), bcrypt secret verification, and a cost-12 dummy-hash timing mitigation so unknown vs inactive vs wrong-secret are timing/response indistinguishable. - Scope enforcement: requested scopes must be a subset of AllowedScopes (invalid_scope otherwise, no silent downgrade); omitted scope grants the full authorized set. Empty AllowedScopes is deny-all. - Audit + metrics on issuance, consistent with existing grants. Adds ParsedAllowedScopes() as the single source of truth for the stored scope string. Integration tests cover happy paths (Basic + form body), wrong/unknown/inactive client, scope over-request, omitted scope, and a full ValidateAccessToken round-trip.
Flagged in review so the global-ClientID audience choice reads as a scoped decision, not an oversight.
…se1-client-credentials feat(m2m): client_credentials grant at /oauth/token [Phase 1 — PR 5/5]
Couchbase (de)serializes whole schema structs via encoding/json (gocb default transcoder), which honors json:"-". User.Password is tagged json:"-" for API safety, so it was silently dropped on both write and read — password auth never worked on Couchbase, and no migration existed. - add structToDocument / decodeDocument helpers that re-add json:"-" fields under their bson key; route all Insert/Upsert through the write helper and User reads through the read helper - assert password round-trips in the cross-DB storage provider test (would have caught this) plus unit tests for the helpers
CompareHashAndPassword dereferenced user.Password unconditionally. A basic_auth user with no persisted hash (e.g. a pre-fix Couchbase record, since that provider silently dropped json:"-" fields on write) would nil-pointer-panic instead of failing the login cleanly. Treats a nil hash the same as a wrong password, using the existing dummy-bcrypt timing-equalisation helper.
log.Fatal calls os.Exit(1), so one malformed row would crash the whole server on an admin list call. Flagged independently by both review passes on this PR since it now also covers decodeDocument failures.
Cassandra derived INSERT/UPDATE column lists from json.Marshal, which honors json:"-" and silently dropped the two secret-bearing fields: - User.Password was never written at signup (AddUser) — basic_auth login has never worked on Cassandra/ScyllaDB for any user. - ServiceAccount.ClientSecret rotation no-op'd (UpdateServiceAccount) — the old hash stayed active forever while rotation reported success. json.Marshal is the wrong source of truth for what to persist. Derive columns from the `cql` tag instead — it is never set to "-" for API-safety, so every persisted field is included. - add buildCQLColumnMap: reflects over cql tags, preserving the existing null/omitempty semantics the json path had - use it in AddUser, UpdateUser, UpdateServiceAccount - add DB-agnostic test proving AsAPIUser/AsAPIServiceAccount never leak the secret back through the API model
- token.go: emit an audit event when a resolved ServiceAccount fails client_credentials auth (wrong secret / inactive) — mirrors login.go's bad_password/account_revoked branches, which likewise skip auditing the unknown-client_id case since there's no actor to attribute it to. - token_client_credentials_test.go: add a true end-to-end test that creates a ServiceAccount via the real admin GraphQL API, takes the one-time client_secret from the response, and authenticates with it at /oauth/token — every other subtest fabricates the ServiceAccount + hash directly via storage, so this is the only test proving the real admin-creates-then-machine-authenticates flow actually works.
…tion Every schema's ID is json:"_id", so after Add/Get it holds the full "collection/key" document handle, while cross-entity foreign-key-style string fields (WebhookLog.WebhookID, Session.UserID) get whatever value the caller passed — always the full handle, since that's what Add/Get return. Filtering a cascade-delete on the wrong (bare-key) form matches zero documents, silently. - DeleteWebhook: cascade to WebhookLog bound webhook.Key; WebhookLog.WebhookID is stored as webhook.ID. Every webhook log survived deletion. Bind webhook.ID. - DeleteUser: cascade to Session bound user.Key; Session.UserID is stored as user.ID. This is the only session-cleanup path on user deletion, so every session survived. Bind user.ID. - AddSession never generated an ID and only set Key in the empty-ID branch, leaving both blank. Match the SQL provider: generate a uuid when empty, then always mirror ID into Key. - Correct UpdateServiceAccount/UpdateTrustedIssuer doc comments: UpdateDocument is a partial PATCH, not a full replace. Regression tests in provider_test.go, verified against live ArangoDB with negative-proof (each test fails on the old code, passes on the fix).
- AddTrustedIssuer: no duplicate-issuer_url guard existed. issuer_url must be unique per instance (GetTrustedIssuerByIssuerURL is a client_assertion hot path expecting one deterministic match); add a check-then-insert guard mirroring AddEmailTemplate's event_name pattern. - Convert AddAuthenticator/UpdateAuthenticator, UpdateWebhook, UpdateEmailTemplate, UpdateTrustedIssuer from the json.Marshal->map column-building pattern to buildCQLColumnMap (sources columns from the cql tag, which is never "-"), so a future json:"-" tag on any of these entities can't silently drop a column the way it did for User.Password / ServiceAccount.ClientSecret. Authenticator (Secret, RecoveryCodes) was the highest-risk instance of this footgun. - UpdateUser: add the CreatedAt == 0 partial-struct guard already present on UpdateServiceAccount / UpdateTrustedIssuer. - Correct misleading IF NOT EXISTS comments: it only guards the UUID partition key, not email/phone/user+method uniqueness — that's the racy check-then-insert above, an accepted limitation given Cassandra has no cross-attribute unique constraint. New buildCQLColumnMap unit test (proves a json:"-" field survives), UpdateUser guard test, and a cross-DB duplicate-issuer_url regression test (scoped to uniqueness-enforcing backends). Verified against live ScyllaDB.
- ListWebhook, GetWebhookByEventName, ListWebhookLogs, ListTrustedIssuers, ListEmailTemplate, ListVerificationRequests: return row-decode errors instead of log.Fatal, which crashed the entire server process on one bad row (a sibling log.Fatal in ListUsers was already fixed this way). - ListVerificationRequests: SELECT the real columns (token/identifier/ email/nonce/redirect_uri/...) instead of a copy-pasted, non-existent `env` column that left every listed record's fields empty. - GetAuthenticatorDetailsByUserId: same copy-paste class of bug — SELECTed `recovery_code` (singular, nonexistent) instead of `recovery_codes`, so RecoveryCodes never survived a read. Caught by the new cross-DB "unrelated fields survive an update" regression test. - GetSetFields: bind Go nil for null values so the N1QL driver writes a real NULL, not the 4-character literal string "null". - AddUser / AddAuthenticator: document the non-atomic check-then-insert uniqueness race (shared by the other NoSQL providers, no code change). - DeleteAllSessionTokensByUserID: investigated and kept the CONTAINS (substring) match — user_id is stored as "<method>:<user.ID>" and the delete is called with the bare id, so exact/prefix matching would silently break "log out all my sessions". Documented why. - shared.go: refresh the json:"-" persistence note to name both secret fields (User.Password, ServiceAccount.ClientSecret) and record that TrustedIssuer is deliberately unconverted (no such field today). Regression tests for each fix; verified against live Couchbase.
- DeleteServiceAccount: delete TrustedIssuer children before the parent and return query/delete errors instead of swallowing them. The old order (delete parent, then swallow a child-query error) could leave orphaned TrustedIssuers that still authenticate client_assertion JWTs after their ServiceAccount was "deleted". A cascade failure now leaves parent+children both present — safe and retryable — mirroring SQL's ordering. - UpdateUser: REMOVE every nullable pointer field when nil, not just the 3 timestamp fields. A nil pointer is omitted from an UpdateItem SET expression, so clearing e.g. Picture left the stale URL forever; DynamoDB now matches SQL NULL / Mongo null semantics. - Fix UpdateServiceAccount/UpdateTrustedIssuer doc comments to describe the UpdateItem partial SET/REMOVE merge, not a full PutItem replace. - admin_trusted_issuers.go (service layer): reject a duplicate issuer_url before AddTrustedIssuer, since GetTrustedIssuerByIssuerURL expects one deterministic match on every client_assertion validation. This is a service-layer guard, so it protects every storage backend uniformly. Regression tests: cross-DB service-account cascade + user null-clear in the storage suite, duplicate issuer_url rejection in integration tests. Verified against live DynamoDB Local.
- Add the CreatedAt == 0 partial-struct guard (matching the existing
ServiceAccount/TrustedIssuer guard) to UpdateUser, UpdateWebhook,
UpdateEmailTemplate, UpdateAuthenticator — each does a full-document
$set, so a partial struct would otherwise silently blank every field
the caller didn't populate.
- Make the (user_id, method) authenticator index UNIQUE, closing the
check-then-insert race in AddAuthenticator that could create duplicate
MFA enrollments with divergent secrets. Root cause found while fixing
this: the driver silently rejects a multi-key bson.M for index keys
("multi-key map passed in for ordered parameter keys") — the index was
never actually being created until switched to ordered bson.D. The
pre-existing sessionToken/mfaSession compound indexes have the same
bson.M bug and are very likely also silently never created (not fixed
here — out of scope, flagged for follow-up).
Regression tests per fixed method (field-preservation + guard-rejection)
plus a live-Mongo unique-index-violation test. Verified against live
MongoDB.
- AddUser: check email and phone uniqueness independently. An else-if skipped the email check whenever a phone was also supplied, letting duplicate emails persist. - session tokens: anchor DeleteAllSessionTokensByUserID to an exact or ":<id>" suffix match and escape LIKE metacharacters, so deleting "42" no longer sweeps up "142". Root cause: SessionToken.UserID is stored as "<login_method>:<user.ID>" but every caller passes the bare id, so a naive exact-match fix would have broken deletion entirely — this keeps cross-login-method deletion working while closing the substring over-match. Escape the namespace prefix in DeleteSessionTokensByNamespace likewise (it was already correctly prefix-anchored, just unescaped). - UpdateUser: reject partial structs (CreatedAt == 0) before Save, which would otherwise blank Password/Roles and other unset columns. - authenticators: add a (user_id, method) unique index and target it via OnConflict, closing the enrollment check-then-insert race that could leave duplicate MFA rows with divergent secrets. Adding this index can fail AutoMigrate on any deployment that already accumulated duplicate rows from the race — flagged for the maintainer, no dedup migration written here. - otp: document the same check-then-create race (short-lived, low-impact, no unique index to route through without a schema change). - docs: correct UpdateUsers' doc comment (empty ids performs no update and returns an error — global updates are disabled — not a silent global update) and note that Delete methods are idempotent. Regression tests per fix; verified against live Postgres and SQLite.
…fixes Threads dbType through testUserOperations, testWebhookOperations, testEmailTemplateOperations, testAuthenticatorOperations, and testTrustedIssuerOperations so DB-specific assertions (Mongo's partial-struct guard, Cassandra/SQL's duplicate-issuer_url rejection) can gate cleanly alongside the portable "unrelated fields survive an update" checks every provider must satisfy. Adds: Picture nil-clear round-trip, ServiceAccount/TrustedIssuer cascade delete + orphan check, a distinct-user session-token survives DeleteAllSessionTokensByUserID, ListVerificationRequests real-field assertions, and the SQL-only CRUD correctness subtest (email uniqueness, anchored session-token delete, partial-struct guard).
Phased spec (A: client registry, B: secretless client auth, C: orgs/SSO/SCIM, D: delegation) with per-feature db/api/ui/test detail, and a manual test plan covering regression + new-feature edge cases per phase. Follows the Rev. 5.1 design doc. No code yet.
…t-identity # Conflicts: # internal/storage/db/couchbase/shared.go # internal/storage/db/couchbase/shared_test.go # internal/storage/provider_test.go
…stry with kind discriminator
… dependency graph)
This was referenced Jul 8, 2026
This was referenced Jul 21, 2026
Closed
lakhansamani
added a commit
that referenced
this pull request
Jul 22, 2026
…CP entries Add exhaustive coverage of all PRs missed in the prior pass: **Machine Identity (M2M) — PRs #648, #641-647, #652, #651, #654, #659, #655:** - Unified OAuth Client registry with `kind` discriminator (interactive/service_account) - client_credentials grant for service accounts (RFC 6749 §4.4) - Secretless workload identity: RFC 7523 client_assertion, SPIFFE JWT-SVID, K8s TokenReview - Registry-authoritative client authentication (introspect/revoke/discovery) - Interactive client registry columns + reserved-client seed - Shared client-auth resolver + grant-matrix hardening **Agent Delegation (A2A) — PR #658:** - RFC 8693 token-exchange for agentic on-behalf-of with nested act chains - Scope attenuation (monotonic non-widening intersection) - Hard depth cap on nesting; reserved claims prevent forging **Multi-Tenant SSO/SCIM Foundations — PRs #653, #657, #660, #656:** - Organization + user-org membership entities (CR1) - Per-org OIDC SSO federation (Okta/Entra/Google as upstream IdPs) - Per-org SAML 2.0 Service Provider SSO (XML-DSIG, replay detection, JIT) - Per-org SCIM 2.0 user provisioning (+ 3 pre-existing storage bugs fixed) **Authorization & Transports — PRs #625, #620:** - OpenFGA ReBAC engine (replaces non-released bespoke FGA) - Multi-protocol API: GraphQL + gRPC + REST + MCP - MCP: authorizer mcp CLI subcommand for claude mcp add - Single proto source of truth with buf STANDARD enforcement **Dashboard & UI — PRs #662, #663, #605:** - Admin pages: /identity/clients (with secret rotation), /identity/trusted-issuers - Admin pages: /identity/organizations (with Members, SSO, SCIM tabs) - UI migration: Chakra UI → shadcn/ui + Tailwind CSS (full type safety) **Security & Compliance — PRs #604, #603, #606:** - OIDC/OAuth2 spec compliance (RFC 6749 errors, auth_time, TTL, discovery) - PKCE RFC-compliance (base64url padding tolerance, secret bypass prevention) - Introspection auth bypass fix + backchannel SSRF hardening **Bugfix — PR #664:** - Expose public client_id in Client API type (distinct from surrogate id) **README enhancements:** - M2M: Add "Machine-to-machine (service-to-service) authentication" bullet - A2A: Add "Agent-to-agent (A2A) delegation" bullet - OIDC/OIDC: Clarify Authorizer is both IdP, RP/broker, and both for multi-tenant SSO - Transports: Add MCP to "GraphQL, REST, gRPC" list; clarify admin API transports Verification: All 32 PRs now have explicit CHANGELOG entries (consolidated where appropriate).
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
1 participant
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.
Summary
Introduces the unified OAuth Client registry — the foundation of the machine & agent identity program. This supersedes the never-merged
ServiceAccountdesign (previously reviewed as #641–#646 into the workload-identity branch, never landed onmain): instead of a separateServiceAccountentity, machine identity is akind = service_accountrow in oneauthorizer_clientstable, matching how Keycloak/Auth0 model it.What reviewers should focus on
This diff is large by line count but most of it is not hand-written:
gqlgen, proto.pb.go, models)The ~4,000 hand-written lines are the code originally reviewed as #641–#646; the net-new change on top is the
ServiceAccount→Clientrename + akinddiscriminator +authorizer_clientstable name.What's in this PR
Clientschema (internal/storage/schemas/client.go) with akinddiscriminator (interactive|service_account),ClientSecretprotected byjson:"-".Collectionswiring + per-provider indexes/migrations.internal/service/admin_clients.go): secret generation (crypto/rand 32B → bcrypt cost 12), one-time reveal, scope-subset enforcement._client_*ops + gRPC admin surface (kind-aware projection — the hash never serializes out).client_credentialsgrant in the token handler (constant-time / dummy-hash timing guard, scope-subset, audit events).TrustedIssuerentity + storage scaffolding (its client-assertion behavior lands in a follow-up)._keyfiltering, Couchbasejson:"-"persistence, Cassandracql-tag UPDATE map, etc.).What's deferred to stacked follow-up PRs
The client-auth resolver + backward-compat invariants, secretless auth (
client_assertion/SPIFFE/K8s), organizations, SSO (SAML/OIDC), SCIM, and delegation (RFC 8693) each land as their own small, individually-reviewed PR on top of this one. Seespecs/machine-agent-identity/for the plan, manual test plan, and PR pipeline.Testing
make test-all-db— green across all 7 databases (postgres, sqlite, mongodb, arangodb, scylladb, dynamodb, couchbase);internal/storage128s ok,integration_tests59s ok, 0 failures.go build ./...,go vet ./...,make lint-goclean;make generate-graphql/make proto-genproduce zero drift.ClientSecretpersists and never serializes out;client_credentialse2e + admin GraphQL/gRPC suites pass.Design reference
specs/machine-agent-identity/IMPLEMENTATION_PLAN.md,MANUAL_TEST_PLAN.md,SUBAGENT_GUIDE.md.