feat(api): add dynamic OIDC issuer registration#2839
Open
janekbaraniewski wants to merge 4 commits into
Open
Conversation
…ration Add the persistent Issuer entity backing dynamic OIDC issuer registration: an issuer-to-org trust binding with full claim-mapping parity to the static config (claimMappings, audiences, scopes, jwksTimeout, allowDuplicateStaticOrgNames). IssuerSQLDAO provides Create/GetByID/GetByIssuerURL/GetAll/Update/Delete with soft delete. A partial unique index on issuer_url (WHERE deleted IS NULL) enforces one active binding per issuer URL while allowing re-registration after an issuer is offboarded. Signed-off-by: Jan Baraniewski <dev@baraniewski.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (9)
📒 Files selected for processing (16)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (12)
Summary by CodeRabbit
WalkthroughAdds dynamic custom OIDC issuer registration end to end: persisted issuer storage, shared reserved-org state, config hot-apply and boot seeding, REST CRUD endpoints, API models, and OpenAPI updates. ChangesDynamic OIDC issuer registration
Sequence Diagram(s)sequenceDiagram
participant InitAPIServer
participant SeedIssuersFromDB
participant IssuerDAO
participant JWTOriginConfig
InitAPIServer->>SeedIssuersFromDB: SeedIssuersFromDB(ctx, dbSession)
SeedIssuersFromDB->>IssuerDAO: GetAll()
IssuerDAO-->>SeedIssuersFromDB: []Issuer
loop each DB issuer
SeedIssuersFromDB->>JWTOriginConfig: AddJwksConfig
SeedIssuersFromDB->>JWTOriginConfig: UpdateJWKS
alt JWKS reachable
SeedIssuersFromDB->>IssuerDAO: Update status=Ready
else JWKS unreachable
SeedIssuersFromDB->>IssuerDAO: Update status=Pending
end
end
SeedIssuersFromDB->>JWTOriginConfig: ReplaceReservedOrgs
SeedIssuersFromDB-->>InitAPIServer: nil or error
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
rest-api/api/internal/config/issuer.go (1)
39-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit assignment from condition in the new control flow.
These new branches use combined assign-and-condition forms; the REST Go guidelines prefer separate assignment and
ifstatements for cleaner error handling consistency.As per coding guidelines, “Split assign-and-condition into two statements.”
Also applies to: 99-101, 175-183
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/api/internal/config/issuer.go` around lines 39 - 42, The new conditional branches in issuer.go use combined assign-and-condition form, which should be split into separate assignment and if statements for consistency with the REST Go guidelines. Update the JWKSTimeout parsing logic in the config issuer flow, and apply the same pattern to the other affected branches in the same file, using the relevant control-flow blocks around JWKSTimeout and the other listed sections.Source: Coding guidelines
rest-api/auth/pkg/config/jwks.go (1)
185-190: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCopy the replacement map before publishing it.
ReservedOrgSetis only concurrency-safe for access through its methods, butReplacestores the caller’s map by reference. If any caller reuses or mutates that map afterReplace,Hascan race and panic with concurrent map access.Suggested change
func (s *ReservedOrgSet) Replace(m map[string]bool) { s.mu.Lock() defer s.mu.Unlock() - s.m = m + next := make(map[string]bool, len(m)) + for org, reserved := range m { + next[org] = reserved + } + s.m = next }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/auth/pkg/config/jwks.go` around lines 185 - 190, ReservedOrgSet.Replace currently publishes the caller’s map directly, which can lead to races if the input map is later reused or mutated while Has is reading it. Update Replace in ReservedOrgSet to make its own copy of the provided map before assigning to s.m, so the internal state is only owned by the set and remains safe for concurrent access through its methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rest-api/api/internal/config/issuer.go`:
- Around line 73-88: ValidateRegisteredIssuer is still doing a pre-write GetAll
lookup, which leaves a TOCTOU gap for duplicate static orgName mappings. Move
the duplicate-check logic in ValidateRegisteredIssuer to run inside the same
transaction used by the create/update flow, after acquiring the advisory lock,
and have it read from that locked transaction snapshot instead of an earlier
session read. Make sure the call sites in the issuer create/update handlers use
the transactional session when invoking the helper so concurrent registrations
cannot both pass validation and commit.
In `@rest-api/api/internal/server/server.go`:
- Around line 263-266: The startup seeding in server.go’s JWT origin setup
currently logs and continues after SeedIssuersFromDB fails, which can leave
jwtOriginConfig missing persisted issuers. Update the server startup flow around
cfg.SeedIssuersFromDB to treat this as fatal or perform a bounded
retry/reconciliation before any authenticated routes are served, and make sure
the failure path clearly stops initialization instead of proceeding with partial
issuer state.
In `@rest-api/api/pkg/api/handler/issuer.go`:
- Around line 157-160: Move issuer validation into the advisory-locked
transaction so the cross-row checks happen against locked state, not a stale
snapshot. Update the issuer handler paths that currently call
ValidateRegisteredIssuer() before WithTxResult so they re-run that validation
after acquiring the advisory lock and before any writes. Use the existing
handler flow around ValidateRegisteredIssuer, WithTxResult, and the issuer
create/update logic to keep the validation and persistence inside the same
transaction.
- Around line 42-49: The issuer status reconciliation in the handler is
returning a locally overwritten status even when the database update fails,
which can make the create/update response disagree with what was actually
persisted. Update the logic in the issuer handler around cdb.WithTxResult and
issDAO.Update so that on failure you either propagate the reconciliation error
or leave iss.Status aligned with the last persisted value; do not set iss.Status
to the new status when the follow-up update fails. Keep the response path
consistent with the returned updated issuer value and the existing logger.Error
call in issuer.go.
- Around line 415-423: The issuer PATCH handler currently binds into
APIIssuerUpdateRequest and silently ignores unknown JSON keys, so requests that
include immutable fields like issuerUrl or origin can still succeed. Update the
issuer update flow in the issuer.go handler to explicitly detect those keys in
the incoming body before or during binding, and reject the request with a 400
via cutil.NewAPIErrorResponse if either immutable field is present. Keep the
existing validation path for APIIssuerUpdateRequest, but ensure the handler
enforces the immutable-field contract instead of allowing Echo’s binder to drop
them.
In `@rest-api/openapi/spec.yaml`:
- Around line 13350-13353: The top-level serviceAccount schema currently
documents that it must be false for dynamic custom issuer registrations, but it
still permits true. Tighten the OpenAPI definitions for serviceAccount in the
relevant spec entries so the schema enforces the contract directly, using the
existing serviceAccount property definitions as the place to locate and update
the constraint while preserving request/response compatibility.
- Around line 13287-13320: The IssuerClaimMapping schema currently allows
invalid combinations like empty objects, both org sources, or missing role
sources. Tighten the schema in IssuerClaimMapping by adding oneOf constraints
that enforce exactly one organization source (`orgName` vs `orgAttribute`) and
at least one valid role source (`roles`, `rolesAttribute`, or
`isServiceAccount`), using the existing property names so generated clients and
validators align with the REST contract.
---
Nitpick comments:
In `@rest-api/api/internal/config/issuer.go`:
- Around line 39-42: The new conditional branches in issuer.go use combined
assign-and-condition form, which should be split into separate assignment and if
statements for consistency with the REST Go guidelines. Update the JWKSTimeout
parsing logic in the config issuer flow, and apply the same pattern to the other
affected branches in the same file, using the relevant control-flow blocks
around JWKSTimeout and the other listed sections.
In `@rest-api/auth/pkg/config/jwks.go`:
- Around line 185-190: ReservedOrgSet.Replace currently publishes the caller’s
map directly, which can lead to races if the input map is later reused or
mutated while Has is reading it. Update Replace in ReservedOrgSet to make its
own copy of the provided map before assigning to s.m, so the internal state is
only owned by the set and remains safe for concurrent access through its
methods.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 651d1c78-f5be-425b-a548-4f34f2f4a610
⛔ Files ignored due to path filters (9)
rest-api/sdk/standard/api_issuer.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/client.gois excluded by!rest-api/sdk/standard/client.gorest-api/sdk/standard/model_bmc_credential.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_deletion_accepted_response.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_issuer.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_issuer_claim_mapping.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_issuer_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_issuer_status.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_issuer_update_request.gois excluded by!rest-api/sdk/standard/model_*.go
📒 Files selected for processing (16)
rest-api/api/internal/config/config.gorest-api/api/internal/config/config_test.gorest-api/api/internal/config/issuer.gorest-api/api/internal/config/issuer_test.gorest-api/api/internal/server/server.gorest-api/api/pkg/api/handler/issuer.gorest-api/api/pkg/api/handler/issuer_test.gorest-api/api/pkg/api/model/error.gorest-api/api/pkg/api/model/issuer.gorest-api/api/pkg/api/routes.gorest-api/auth/pkg/config/jwks.gorest-api/auth/pkg/config/jwtOrigin.gorest-api/db/pkg/db/model/issuer.gorest-api/db/pkg/db/model/issuer_test.gorest-api/db/pkg/migrations/20260617120000_issuer.gorest-api/openapi/spec.yaml
janekbaraniewski
force-pushed
the
feat/dynamic-oidc-issuer-registration
branch
from
e63aee6 to
6bc97a3
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rest-api/api/internal/config/issuer.go`:
- Around line 177-185: SeedIssuersFromDB currently calls jwksCfg.UpdateJWKS()
without using the provided context, so issuer bootstrapping can overrun the
startup deadline and then fail later on status persistence. Update the issuer
refresh path in SeedIssuersFromDB to derive a per-issuer context from the
remaining deadline before calling UpdateJWKS(), or otherwise move the JWKS probe
into a bounded non-fatal reconciliation step. Keep the status update logic in
sync with the refresh result, and use the existing symbols SeedIssuersFromDB,
UpdateJWKS, and cdb.WithTxResult to locate the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9a3f98d7-affb-4e0b-bd5c-9a35993df027
⛔ Files ignored due to path filters (9)
rest-api/sdk/standard/api_issuer.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/client.gois excluded by!rest-api/sdk/standard/client.gorest-api/sdk/standard/model_bmc_credential.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_deletion_accepted_response.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_issuer.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_issuer_claim_mapping.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_issuer_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_issuer_status.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_issuer_update_request.gois excluded by!rest-api/sdk/standard/model_*.go
📒 Files selected for processing (13)
rest-api/api/internal/config/config.gorest-api/api/internal/config/config_test.gorest-api/api/internal/config/issuer.gorest-api/api/internal/config/issuer_test.gorest-api/api/internal/server/server.gorest-api/api/pkg/api/handler/issuer.gorest-api/api/pkg/api/handler/issuer_test.gorest-api/api/pkg/api/model/error.gorest-api/api/pkg/api/model/issuer.gorest-api/api/pkg/api/routes.gorest-api/auth/pkg/config/jwks.gorest-api/auth/pkg/config/jwtOrigin.gorest-api/openapi/spec.yaml
✅ Files skipped from review due to trivial changes (1)
- rest-api/api/pkg/api/model/error.go
🚧 Files skipped from review as they are similar to previous changes (10)
- rest-api/api/pkg/api/routes.go
- rest-api/auth/pkg/config/jwks.go
- rest-api/auth/pkg/config/jwtOrigin.go
- rest-api/api/internal/config/config_test.go
- rest-api/api/internal/config/issuer_test.go
- rest-api/api/pkg/api/model/issuer.go
- rest-api/api/internal/config/config.go
- rest-api/openapi/spec.yaml
- rest-api/api/pkg/api/handler/issuer_test.go
- rest-api/api/pkg/api/handler/issuer.go
Wire DB-registered issuers into the live JWT origin map so they verify identically to statically-configured issuers. - jwksConfigForIssuer builds a JwksConfig from a DB issuer; ApplyIssuer fetches the JWKS first and installs the config only on success, so a fetch failure never clobbers an existing live config. - ValidateRegisteredIssuer validates a candidate against the static config and the registered DB issuers using the existing issuer-config rules. - SeedIssuersFromDB installs all registered issuers at startup (best-effort JWKS, skipping any URL that is statically configured). - reservedOrgNames becomes a thread-safe set wired by reference into every JwksConfig at insertion time (consulted only for dynamic claim mappings) and recomputed as issuers are registered or removed. Signed-off-by: Jan Baraniewski <dev@baraniewski.com>
Expose CRUD for OIDC issuer bindings under /org/{org}/nico/issuer, restricted to
Provider Admins. Create and Update validate the candidate against the combined
static and registered issuer set before persisting, hot-apply the binding into
the live JWT origin map, settle its status from the JWKS fetch, and recompute
the reserved-org set; Delete removes it from the live map. A statically
configured issuer URL cannot be registered, and a duplicate active issuer URL
returns 409.
Signed-off-by: Jan Baraniewski <dev@baraniewski.com>
Cover the hot-apply verify seam end to end: a token signed by a freshly registered issuer's key is accepted for its bound org, rejected for another org, and rejected after deregistration. Also cover boot seeding: statically configured issuer URLs are skipped, a JWKS fetch failure is non-fatal, and the reserved-org set is the union of the config static orgs and the DB issuers' static orgs. Signed-off-by: Jan Baraniewski <dev@baraniewski.com>
janekbaraniewski
force-pushed
the
feat/dynamic-oidc-issuer-registration
branch
from
6bc97a3 to
d3925de
Compare
thossain-nv
reviewed
Jun 25, 2026
thossain-nv
left a comment
thossain-nv
left a comment
Contributor
There was a problem hiding this comment.
Runtime reloading of auth/issuer configuration is very expensive as it requires any number of parallel HTTP requests to share same RWMutex.
Any configuration used by all request handled by the API are usually applied by restarting the deployment/statefulset.
@janekbaraniewski let's discuss this a bit more in the issue so I can understand the intentions.
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
2 participants
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
Resolves #2108.
Adds runtime registration for external/custom OIDC issuers so operators can add, update, and remove auth issuer bindings without restarting infra-controller-rest pods.
This implements the hot-reload use case through Provider Admin APIs backed by persistent issuer rows, rather than by periodically rereading the deployment ConfigMap. Static deployment-managed issuers such as KAS, SSA, and Keycloak remain config-driven.
What Changed
Issuermodel, DAO, and migration for dynamic OIDC issuer registrations.customexternal IdPs only.Behavior Notes
Pendingstatus and authentication fails closed until JWKS refresh succeeds.issuerUrlandoriginare immutable after creation.