Feat: IdP-agnostic token exchange plugin interface#521
Conversation
|
Warning Review limit reached
More reviews will be available in 44 minutes and 50 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe token-exchange plugin is generalized from Keycloak-only behavior to a provider-driven, multi-IdP model. A new ChangesMulti-IdP Token-Exchange Plugin
Sequence Diagram(s)sequenceDiagram
participant Configure
participant applyDefaults
participant LookupProvider
participant keycloakProvider
participant buildClientAuth
Configure->>applyDefaults: tokenExchangeConfig
applyDefaults->>applyDefaults: migrate keycloak_url/keycloak_realm → provider_url/provider_realm
applyDefaults->>LookupProvider: LookupProvider("keycloak")
LookupProvider->>keycloakProvider: (lookup in registry)
keycloakProvider->>keycloakProvider: TokenEndpoint(providerURL, providerRealm)
keycloakProvider-->>LookupProvider: token_url
LookupProvider-->>applyDefaults: IdPProvider instance
applyDefaults->>applyDefaults: default identity.assertion_type from provider.DefaultAssertionType()
applyDefaults-->>Configure: resolved config with token_url, provider, assertion_type
Configure->>buildClientAuth: provider, identity (with assertion_type), jwtSrc
buildClientAuth->>LookupProvider: LookupProvider(provider)
alt Provider registered
buildClientAuth->>keycloakProvider: BuildClientAuth(identity, jwtSrc)
keycloakProvider->>keycloakProvider: map assertion_type → AssertionTypeURN
keycloakProvider-->>buildClientAuth: ClientAuth
else Fallback
buildClientAuth->>buildClientAuth: map assertion_type → AssertionTypeURN, default jwt-spiffe
buildClientAuth-->>buildClientAuth: ClientAuth
end
buildClientAuth-->>Configure: ClientAuth ready
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Possibly related issues
Poem
🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@authbridge/authlib/plugins/tokenexchange/plugin.go`:
- Around line 143-152: The backward-compatibility migration logic handles the
deprecated keycloak_url field by defaulting the Provider to "keycloak" when it's
empty, but the keycloak_realm migration block does not perform the same check.
To fix the mixed legacy/new config path issue, add a check in the KeycloakRealm
block that sets c.Provider to "keycloak" if it is empty, similar to the logic
already present in the KeycloakURL block. This ensures that the Provider field
is always populated when any deprecated Keycloak field is present, allowing
proper token URL derivation and config validation.
- Around line 471-474: The code currently silently coerces any unknown
assertionType value to the default "jwt-spiffe" by checking only if urn is
empty. Instead, distinguish between empty input (which should default to
"jwt-spiffe") and invalid non-empty input (which should return an error). Modify
the conditional logic that sets urn to assertionTypeURN["jwt-spiffe"] to only
apply when assertionType is empty or when it doesn't exist in the
assertionTypeURN map, otherwise return an error indicating the unsupported
assertion_type value to prevent config errors from being silently hidden.
In `@authbridge/docs/idp-plugin-contract.md`:
- Around line 209-212: The assertion type support table incorrectly indicates
that Keycloak supports the jwt-bearer assertion type. Update the table row for
the jwt-bearer assertion type to change Keycloak's support status from a
checkmark to an X mark, as Keycloak only supports the jwt-spiffe assertion type.
This aligns the documentation with the actual code behavior referenced in
plugin.go where jwt-bearer is noted as being for Okta compatibility only, and
there are no verified tests demonstrating Keycloak's jwt-bearer support.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d04e6ef4-f0bc-4a70-8bc4-f151cf2347c7
📒 Files selected for processing (6)
authbridge/authlib/plugins/tokenexchange/plugin.goauthbridge/authlib/plugins/tokenexchange/plugin_test.goauthbridge/authlib/plugins/tokenexchange/provider.goauthbridge/authlib/plugins/tokenexchange/provider_keycloak.goauthbridge/authlib/plugins/tokenexchange/provider_test.goauthbridge/docs/idp-plugin-contract.md
huang195
left a comment
There was a problem hiding this comment.
Solid, well-tested Phase-1 config generalization with a clean, backward-compatible deprecation path (keycloak_url/keycloak_realm → provider_*). The direction — isolate IdP-specifics, keep the RFC 8693 core generic — is right.
My main concern is that the abstraction draws its boundary around the least divergent axis (endpoint URL templating) while the genuinely IdP-specific axis (client authentication) stays as loose, uncoordinated config flags outside the IdPProvider interface. Entra ID needs certificate auth (private_key_jwt/x5t) and Okta needs jwt-bearer — per this PR's own Phase 2/3 plan, both require touching exchange.ClientAuth + buildClientAuthFrom. So the headline "add one provider_<name>.go, no core changes" promise holds only for URLs, not for the auth those IdPs actually require. Widening IdPProvider to own auth-method selection + defaults + validation (see inline comments) would make it deliver on that promise and reject invalid combinations at config time instead of at the IdP.
One must-fix: the committed design doc (idp-plugin-contract.md) describes a different design (a switch/resolveEndpoints()) than the shipped code (interface + registry), and its Phase-1 checklist marks resolveEndpoints() as done — it doesn't exist. That will mislead the next contributor; please reconcile it with the as-built code.
The Keycloak path itself, backward compat, and test coverage all look good — this is about getting the extension shape right before more IdPs are built on it.
Author: akram (MEMBER — maintainer) · Areas reviewed: Go, Docs · Agent/IDE config (.claude/.vscode): none · Commits: 1, signed-off: yes · CI: passing
Assisted-By: Claude Code
|
|
||
| ### Interface (Go) | ||
|
|
||
| No new Go interface is needed for URL derivation — it is a pure |
There was a problem hiding this comment.
must-fix — This contradicts the shipped code. The section says "No new Go interface is needed for URL derivation… Implemented as a switch in applyDefaults()" and gives a resolveEndpoints() function (line 124); the Phase-1 checklist (line 246) lists "Implement resolveEndpoints()" as done. But the PR actually ships an IdPProvider interface + registry, and resolveEndpoints() does not exist. A contributor following this doc would build the wrong thing. Please reconcile the contract with the as-built design (the IdPProvider interface) before merge.
| // JWKSEndpoint derives the JWKS endpoint URL from the provider | ||
| // base URL and realm/tenant. Returns "" if the inputs are | ||
| // insufficient (caller must supply explicit jwks_url). | ||
| JWKSEndpoint(providerURL, providerRealm string) string |
There was a problem hiding this comment.
suggestion — JWKSEndpoint() has no consumer on the outbound exchange path: applyDefaults() only calls TokenEndpoint(). JWKS is for validating inbound tokens (the jwtvalidation plugin's job), not the exchange client. As-is it's interface surface every future provider must implement for no current effect. Recommend dropping it from this interface (and its test) until something consumes it.
| // which leaves TokenURL empty — validate() will catch it. | ||
| if c.TokenURL == "" && c.Provider != "" { | ||
| if p := LookupProvider(c.Provider); p != nil { | ||
| c.TokenURL = p.TokenEndpoint(c.ProviderURL, c.ProviderRealm) |
There was a problem hiding this comment.
suggestion — Pair the derivation with provider-driven defaults + validation. After deriving TokenURL, set c.Identity.AssertionType = p.DefaultAssertionType() when unset (so Okta gets jwt-bearer automatically without the operator knowing the URN), and reject an identity.type that isn't in p.SupportedAuthMethods(). Today nothing ties provider / identity.type / assertion_type together — provider: okta + jwt-spiffe is accepted here even though your own matrix (doc line 723) says Okta rejects it, and it only fails later at the IdP rather than at Configure().
| } | ||
| urn := assertionTypeURN[assertionType] | ||
| if urn == "" { | ||
| urn = assertionTypeURN["jwt-spiffe"] // default |
There was a problem hiding this comment.
suggestion — This hardcoded jwt-spiffe fallback is what silently makes every IdP default to Keycloak's assertion URN. If the provider supplies the default (see comment on applyDefaults), this should source from p.DefaultAssertionType() rather than being pinned to jwt-spiffe here — otherwise an Okta config that omits assertion_type silently gets the wrong (Keycloak) URN.
| // When empty and keycloak_url is set, defaults to "keycloak" for | ||
| // backward compatibility. When "generic" or empty, token_url must | ||
| // be supplied explicitly. | ||
| Provider string `json:"provider" description:"IdP provider for endpoint derivation: keycloak, entra-id, okta, or generic." enum:"keycloak,entra-id,okta,generic"` |
There was a problem hiding this comment.
suggestion — The enum advertises entra-id and okta, but only keycloak is registered in this PR. Selecting them makes LookupProvider return nil → TokenURL stays empty → the user hits a confusing "token_url required" error instead of "provider not implemented." Recommend trimming the enum to the implemented providers (keycloak, generic) until #481 Phases 2/3 land, then re-adding each as it ships.
|
Thanks for the review @huang195 , I will address the comments. |
Per review feedback on PR rossoctl#521, the IdPProvider interface now covers the full IdP surface (endpoint derivation + client authentication), not just URL templating. Changes: - IdPProvider interface: add DefaultAssertionType(), SupportedIdentityTypes(), BuildClientAuth() — remove JWKSEndpoint() (no consumer on the exchange path, belongs to jwt-validation) - Keycloak provider: implements full interface including auth building - Remove buildClientAuthFrom() from core — delegated to provider - Validate provider↔identity compatibility at Configure() time (e.g. provider:keycloak + identity.type:certificate → rejected) - Reject unknown assertion_type values (was silently defaulting) - Reject unknown provider names at validation - Trim provider enum to implemented providers only (keycloak, generic) - Fix mixed back-compat path (keycloak_realm without keycloak_url) - IdentityConfig exported struct for provider contract Tests: - 5 new Keycloak provider tests (DefaultAssertionType, SupportedIdentityTypes, BuildClientAuth for client-secret/spiffe, unsupported type rejection) - 3 new validation tests (invalid assertion_type, provider↔identity incompatibility, unknown provider rejection) - All 40 plugin tests pass, full suite zero regression Ref: RHAIENG-5681 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Akram <akram.benaissi@gmail.com>
Per review feedback on PR rossoctl#521, the IdPProvider interface now covers the full IdP surface (endpoint derivation + client authentication), not just URL templating. Changes: - IdPProvider interface: add DefaultAssertionType(), SupportedIdentityTypes(), BuildClientAuth() — remove JWKSEndpoint() (no consumer on the exchange path, belongs to jwt-validation) - Keycloak provider: implements full interface including auth building - Remove buildClientAuthFrom() from core — delegated to provider - Validate provider↔identity compatibility at Configure() time (e.g. provider:keycloak + identity.type:certificate → rejected) - Reject unknown assertion_type values (was silently defaulting) - Reject unknown provider names at validation - Trim provider enum to implemented providers only (keycloak, generic) - Fix mixed back-compat path (keycloak_realm without keycloak_url) - IdentityConfig exported struct for provider contract Tests: - 5 new Keycloak provider tests (DefaultAssertionType, SupportedIdentityTypes, BuildClientAuth for client-secret/spiffe, unsupported type rejection) - 3 new validation tests (invalid assertion_type, provider↔identity incompatibility, unknown provider rejection) - All 40 plugin tests pass, full suite zero regression Ref: RHAIENG-5681 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Akram <akram.benaissi@gmail.com>
Per review feedback on PR rossoctl#521, the IdPProvider interface now covers the full IdP surface (endpoint derivation + client authentication), not just URL templating. Changes: - IdPProvider interface: add DefaultAssertionType(), SupportedIdentityTypes(), BuildClientAuth() — remove JWKSEndpoint() (no consumer on the exchange path, belongs to jwt-validation) - Keycloak provider: implements full interface including auth building - Remove buildClientAuthFrom() from core — delegated to provider - Validate provider↔identity compatibility at Configure() time (e.g. provider:keycloak + identity.type:certificate → rejected) - Reject unknown assertion_type values (was silently defaulting) - Reject unknown provider names at validation - Trim provider enum to implemented providers only (keycloak, generic) - Fix mixed back-compat path (keycloak_realm without keycloak_url) - IdentityConfig exported struct for provider contract Tests: - 5 new Keycloak provider tests (DefaultAssertionType, SupportedIdentityTypes, BuildClientAuth for client-secret/spiffe, unsupported type rejection) - 3 new validation tests (invalid assertion_type, provider↔identity incompatibility, unknown provider rejection) - All 40 plugin tests pass, full suite zero regression Ref: RHAIENG-5681 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Akram <akram.benaissi@gmail.com>
Per review feedback on PR rossoctl#521, the IdPProvider interface now covers the full IdP surface (endpoint derivation + client authentication), not just URL templating. Changes: - IdPProvider interface: add DefaultAssertionType(), SupportedIdentityTypes(), BuildClientAuth() — remove JWKSEndpoint() (no consumer on the exchange path, belongs to jwt-validation) - Keycloak provider: implements full interface including auth building - Remove buildClientAuthFrom() from core — delegated to provider - Validate provider↔identity compatibility at Configure() time (e.g. provider:keycloak + identity.type:certificate → rejected) - Reject unknown assertion_type values (was silently defaulting) - Reject unknown provider names at validation - Trim provider enum to implemented providers only (keycloak, generic) - Fix mixed back-compat path (keycloak_realm without keycloak_url) - IdentityConfig exported struct for provider contract Tests: - 5 new Keycloak provider tests (DefaultAssertionType, SupportedIdentityTypes, BuildClientAuth for client-secret/spiffe, unsupported type rejection) - 3 new validation tests (invalid assertion_type, provider↔identity incompatibility, unknown provider rejection) - All 40 plugin tests pass, full suite zero regression Ref: RHAIENG-5681 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Akram <akram.benaissi@gmail.com>
Per review feedback on PR rossoctl#521, the IdPProvider interface now covers the full IdP surface (endpoint derivation + client authentication), not just URL templating. Changes: - IdPProvider interface: add DefaultAssertionType(), SupportedIdentityTypes(), BuildClientAuth() — remove JWKSEndpoint() (no consumer on the exchange path, belongs to jwt-validation) - Keycloak provider: implements full interface including auth building - Remove buildClientAuthFrom() from core — delegated to provider - Validate provider↔identity compatibility at Configure() time (e.g. provider:keycloak + identity.type:certificate → rejected) - Reject unknown assertion_type values (was silently defaulting) - Reject unknown provider names at validation - Trim provider enum to implemented providers only (keycloak, generic) - Fix mixed back-compat path (keycloak_realm without keycloak_url) - IdentityConfig exported struct for provider contract Tests: - 5 new Keycloak provider tests (DefaultAssertionType, SupportedIdentityTypes, BuildClientAuth for client-secret/spiffe, unsupported type rejection) - 3 new validation tests (invalid assertion_type, provider↔identity incompatibility, unknown provider rejection) - All 40 plugin tests pass, full suite zero regression Ref: RHAIENG-5681 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Akram <akram.benaissi@gmail.com>
Introduce an IdPProvider interface and provider registry so new IdP backends (Entra ID, Okta, etc.) can be added without modifying core plugin code. Each provider implements TokenEndpoint() and JWKSEndpoint() for its URL conventions and registers via init(). Changes: - IdPProvider interface with TokenEndpoint/JWKSEndpoint contract - Provider registry (RegisterProvider/LookupProvider) in provider.go - Keycloak provider as built-in reference implementation - Config fields: provider, provider_url, provider_realm for IdP selection - Configurable assertion_type on spiffe identity (jwt-spiffe/jwt-bearer) - Backward compat: keycloak_url/keycloak_realm auto-migrate with warning - Contract documented in docs/idp-plugin-contract.md To add a new IdP (e.g. Entra ID, Okta): 1. Implement IdPProvider (Name, TokenEndpoint, JWKSEndpoint) 2. Call RegisterProvider() in an init() function 3. No changes to plugin.go needed Ref: RHAIENG-5681 Upstream: kagenti-extensions#481 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Akram <akram.benaissi@gmail.com>
Per review feedback on PR rossoctl#521, the IdPProvider interface now covers the full IdP surface (endpoint derivation + client authentication), not just URL templating. Changes: - IdPProvider interface: add DefaultAssertionType(), SupportedIdentityTypes(), BuildClientAuth() — remove JWKSEndpoint() (no consumer on the exchange path, belongs to jwt-validation) - Keycloak provider: implements full interface including auth building - Remove buildClientAuthFrom() from core — delegated to provider - Validate provider↔identity compatibility at Configure() time (e.g. provider:keycloak + identity.type:certificate → rejected) - Reject unknown assertion_type values (was silently defaulting) - Reject unknown provider names at validation - Trim provider enum to implemented providers only (keycloak, generic) - Fix mixed back-compat path (keycloak_realm without keycloak_url) - IdentityConfig exported struct for provider contract Tests: - 5 new Keycloak provider tests (DefaultAssertionType, SupportedIdentityTypes, BuildClientAuth for client-secret/spiffe, unsupported type rejection) - 3 new validation tests (invalid assertion_type, provider↔identity incompatibility, unknown provider rejection) - All 40 plugin tests pass, full suite zero regression Ref: RHAIENG-5681 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Akram <akram.benaissi@gmail.com>
f40ce79 to
05c0f7a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
authbridge/docs/idp-plugin-contract.md (2)
209-212:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the
jwt-bearersupport row.Keycloak is still marked as supporting
jwt-bearer, but the current implementation/docs only verifyjwt-spiffefor Keycloak and routejwt-bearerto Okta. Please change this row so the matrix reflects what is actually supported.Based on learnings: this mismatch was already called out in a previous review comment.
🤖 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 `@authbridge/docs/idp-plugin-contract.md` around lines 209 - 212, The support matrix in the idp-plugin-contract.md documentation incorrectly shows Keycloak as supporting the jwt-bearer assertion type with a checkmark, but the actual implementation only verifies jwt-spiffe for Keycloak and routes jwt-bearer assertions to Okta. Update the jwt-bearer row to change Keycloak's status from ✅ to ❌ so the matrix accurately reflects the current implementation behavior.
116-147:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReconcile this section with the registry-based design.
This still describes a switch-based
resolveEndpoints()helper, but the PR ships anIdPProviderinterface plusRegisterProvider()/LookupProvider(). Please rewrite this section and the Phase 1 checklist so the contract matches the actual implementation.Based on learnings: the PR summary says endpoint derivation is implemented through a provider registry.
Also applies to: 243-249
🤖 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 `@authbridge/docs/idp-plugin-contract.md` around lines 116 - 147, The documentation section describes endpoint derivation using a switch-based resolveEndpoints() function, but the actual PR implementation uses a registry-based design with an IdPProvider interface and RegisterProvider()/LookupProvider() functions. Rewrite this section to accurately describe the registry pattern: explain how providers are registered using the IdPProvider interface, how endpoint URLs are derived through the registry lookup mechanism, and how the provider registry enables extensibility. Update the Phase 1 checklist and the section at lines 243-249 to reflect this registry-based approach rather than the switch-based implementation shown in the code example.
🤖 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 `@authbridge/authlib/plugins/tokenexchange/plugin_test.go`:
- Around line 399-405: In the TestKeycloakProvider_TokenEndpoint_TrailingSlash
function and similar test functions mentioned (lines 408-421, 423-465), add an
explicit nil check immediately after calling LookupProvider("keycloak") on the
variable p before attempting to call methods on it. If p is nil, use t.Fatal or
t.Fatalf to fail the test with a clear error message rather than allowing the
test to panic. Apply this same nil guard pattern to all test functions that call
LookupProvider and dereference its result.
- Around line 415-420: The test TestKeycloakProvider_SupportedIdentityTypes is
checking the supported identity types at hard-coded slice indices (supported[0]
and supported[1]), which is brittle and will fail if the order changes even
though the provider still supports the correct types. Modify the assertion to be
order-independent by checking that the returned slice contains both
"client-secret" and "spiffe" values regardless of their position in the slice,
such as by checking membership in the slice without relying on specific indices.
In `@authbridge/docs/idp-plugin-contract.md`:
- Around line 15-17: The fenced code blocks in the documentation are missing
language specifiers after the opening triple backticks, which violates MD040
markdown linting rules. Add a language label such as `text` or `plaintext`
immediately after each opening ``` fence throughout the document. This applies
to the diagram block showing the request flow, the component descriptions block,
and any other bare fenced blocks to ensure consistent rendering and maintain
markdown lint compliance.
---
Duplicate comments:
In `@authbridge/docs/idp-plugin-contract.md`:
- Around line 209-212: The support matrix in the idp-plugin-contract.md
documentation incorrectly shows Keycloak as supporting the jwt-bearer assertion
type with a checkmark, but the actual implementation only verifies jwt-spiffe
for Keycloak and routes jwt-bearer assertions to Okta. Update the jwt-bearer row
to change Keycloak's status from ✅ to ❌ so the matrix accurately reflects the
current implementation behavior.
- Around line 116-147: The documentation section describes endpoint derivation
using a switch-based resolveEndpoints() function, but the actual PR
implementation uses a registry-based design with an IdPProvider interface and
RegisterProvider()/LookupProvider() functions. Rewrite this section to
accurately describe the registry pattern: explain how providers are registered
using the IdPProvider interface, how endpoint URLs are derived through the
registry lookup mechanism, and how the provider registry enables extensibility.
Update the Phase 1 checklist and the section at lines 243-249 to reflect this
registry-based approach rather than the switch-based implementation shown in the
code example.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d48a3bef-5234-464b-8696-a70f177e6c8f
📒 Files selected for processing (6)
authbridge/authlib/plugins/tokenexchange/plugin.goauthbridge/authlib/plugins/tokenexchange/plugin_test.goauthbridge/authlib/plugins/tokenexchange/provider.goauthbridge/authlib/plugins/tokenexchange/provider_keycloak.goauthbridge/authlib/plugins/tokenexchange/provider_test.goauthbridge/docs/idp-plugin-contract.md
🚧 Files skipped from review as they are similar to previous changes (2)
- authbridge/authlib/plugins/tokenexchange/provider_test.go
- authbridge/authlib/plugins/tokenexchange/plugin.go
Per review feedback on PR rossoctl#521, the IdPProvider interface now covers the full IdP surface (endpoint derivation + client authentication), not just URL templating. Changes: - IdPProvider interface: add DefaultAssertionType(), SupportedIdentityTypes(), BuildClientAuth() — remove JWKSEndpoint() (no consumer on the exchange path, belongs to jwt-validation) - Keycloak provider: implements full interface including auth building - Remove buildClientAuthFrom() from core — delegated to provider - Validate provider↔identity compatibility at Configure() time (e.g. provider:keycloak + identity.type:certificate → rejected) - Reject unknown assertion_type values (was silently defaulting) - Reject unknown provider names at validation - Trim provider enum to implemented providers only (keycloak, generic) - Fix mixed back-compat path (keycloak_realm without keycloak_url) - IdentityConfig exported struct for provider contract Tests: - 5 new Keycloak provider tests (DefaultAssertionType, SupportedIdentityTypes, BuildClientAuth for client-secret/spiffe, unsupported type rejection) - 3 new validation tests (invalid assertion_type, provider↔identity incompatibility, unknown provider rejection) - All 40 plugin tests pass, full suite zero regression Ref: RHAIENG-5681 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Akram <akram.benaissi@gmail.com>
05c0f7a to
317bd14
Compare
Per review feedback on PR rossoctl#521, the IdPProvider interface now covers the full IdP surface (endpoint derivation + client authentication), not just URL templating. Changes: - IdPProvider interface: add DefaultAssertionType(), SupportedIdentityTypes(), BuildClientAuth() — remove JWKSEndpoint() (no consumer on the exchange path, belongs to jwt-validation) - Keycloak provider: implements full interface including auth building - Remove buildClientAuthFrom() from core — delegated to provider - Validate provider↔identity compatibility at Configure() time (e.g. provider:keycloak + identity.type:certificate → rejected) - Reject unknown assertion_type values (was silently defaulting) - Reject unknown provider names at validation - Trim provider enum to implemented providers only (keycloak, generic) - Fix mixed back-compat path (keycloak_realm without keycloak_url) - IdentityConfig exported struct for provider contract Tests: - 5 new Keycloak provider tests (DefaultAssertionType, SupportedIdentityTypes, BuildClientAuth for client-secret/spiffe, unsupported type rejection) - 3 new validation tests (invalid assertion_type, provider↔identity incompatibility, unknown provider rejection) - All 40 plugin tests pass, full suite zero regression Ref: RHAIENG-5681 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Akram <akram.benaissi@gmail.com>
317bd14 to
5468fde
Compare
Per review feedback on PR rossoctl#521, the IdPProvider interface now covers the full IdP surface (endpoint derivation + client authentication), not just URL templating. Changes: - IdPProvider interface: add DefaultAssertionType(), SupportedIdentityTypes(), BuildClientAuth() — remove JWKSEndpoint() (no consumer on the exchange path, belongs to jwt-validation) - Keycloak provider: implements full interface including auth building - Remove buildClientAuthFrom() from core — delegated to provider - Validate provider↔identity compatibility at Configure() time (e.g. provider:keycloak + identity.type:certificate → rejected) - Reject unknown assertion_type values (was silently defaulting) - Reject unknown provider names at validation - Trim provider enum to implemented providers only (keycloak, generic) - Fix mixed back-compat path (keycloak_realm without keycloak_url) - IdentityConfig exported struct for provider contract Tests: - 5 new Keycloak provider tests (DefaultAssertionType, SupportedIdentityTypes, BuildClientAuth for client-secret/spiffe, unsupported type rejection) - 3 new validation tests (invalid assertion_type, provider↔identity incompatibility, unknown provider rejection) - All 40 plugin tests pass, full suite zero regression Ref: RHAIENG-5681 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Akram <akram.benaissi@gmail.com>
5468fde to
f904c2f
Compare
huang195
left a comment
There was a problem hiding this comment.
Excellent turnaround — the updated commit (f904c2f) resolves all six items from the prior review, including the must-fix doc/code mismatch.
- Doc reconciled —
idp-plugin-contract.mdnow documents the actual 5-methodIdPProviderinterface; the contradictoryswitch/resolveEndpoints()section is gone and status is "Implemented." - Interface widened —
IdPProvidernow owns the full per-IdP surface:TokenEndpoint,DefaultAssertionType,SupportedIdentityTypes, andBuildClientAuth. This is the right boundary: adding Okta is a genuine one-file drop-in, and Entra ID is one provider file plus a sharedCertificateAuth. JWKSEndpointremoved from the interface.- Config-time validation —
validate()now rejects unknown providers, invalidassertion_type, and provider↔identity-type incompatibilities (e.g.keycloak+certificate), instead of failing later at the IdP. - Default assertion type is now provider-driven (
applyDefaultspullsDefaultAssertionType()); the hardcodedjwt-spiffeURN is gone. - Enum trimmed to
keycloak,generic.
New tests track each behavior (BuildClientAuth_*, Validate_InvalidAssertionType, Validate_ProviderIdentityIncompatibility, Validate_UnknownProviderRejected), and pollCredentials correctly threads provider through on credential reload. CI is green.
One non-blocking nit inline. Optional future cleanup (not needed here): generic is handled as an inline fallback in buildClientAuth that duplicates the Keycloak spiffe/client-secret logic — registering a genericProvider would collapse it to a single LookupProvider(name).BuildClientAuth(...) path and drop the generic special-casing in validate().
Author: akram (MEMBER — maintainer) · Areas reviewed: Go, Docs · Agent/IDE config (.claude/.vscode): none · Commits: 2, signed-off: yes · CI: passing
Assisted-By: Claude Code
| TokenURL string `json:"token_url" description:"OAuth token endpoint URL. Required unless provider + provider_url (or keycloak_url + keycloak_realm) are set for automatic derivation."` | ||
|
|
||
| // Provider selects the IdP for automatic endpoint derivation. | ||
| // Supported: keycloak, entra-id, okta, generic. |
There was a problem hiding this comment.
nit — This comment still says "Supported: keycloak, entra-id, okta, generic" (and the provider_url/provider_realm comments below at lines 46-53 describe entra-id/okta interpretations), but the enum is now correctly keycloak,generic. Minor staleness — either trim the comment to match the enum, or mark the not-yet-registered ones as "(future)" so it doesn't read as currently supported.
Summary
Introduces an
IdPProviderinterface and provider registry so new IdP backends (Entra ID, Okta, etc.) can be added without modifying core plugin code. This is Phase 1 of the IdP-agnostic token exchange work (kagenti-extensions#481 / RHAIENG-5681).Problem
The token-exchange plugin hardcodes Keycloak-specific assumptions:
/realms/{realm}/protocol/openid-connect/tokenkeycloak_url,keycloak_realmurn:ietf:params:oauth:client-assertion-type:jwt-spiffeThis prevents direct support for Entra ID, Okta, or other RFC 8693-compliant IdPs.
Changes
IdPProvider interface (
provider.go)Providers self-register via
init()— adding a new IdP requires only:provider_<name>.goIdPProviderRegisterProvider()ininit()Keycloak provider (
provider_keycloak.go)Reference implementation as a standalone file, same pattern any contributor follows.
Config generalization (
plugin.go)provider,provider_url,provider_realmfor IdP selectionassertion_typeon spiffe identity (jwt-spiffedefault,jwt-bearerfor Okta)keycloak_url/keycloak_realmauto-migrate with deprecation warningapplyDefaults()usesLookupProvider()instead of hardcoded switchRegistration guard test (
provider_test.go)CI test scans
provider_*.gofiles and verifies each hasRegisterProvider()ininit()— catches missing registration at build time.Contract documentation (
docs/idp-plugin-contract.md)Full specification for IdP plugin contributors: extension points, config patterns, RFC 8693 compliance analysis, per-IdP auth method matrix, implementation phases.
What does NOT change
For Entra ID / Okta contributors
To add a new provider, create one file:
Test plan
go test ./...) — zero regressionContext
Assisted-By: Claude (Anthropic AI) noreply@anthropic.com
Summary by CodeRabbit
assertion_typeto control client authentication.provider,provider_url,provider_realm(withkeycloak_url/keycloak_realmkept as backward-compatible aliases).