Skip to content

Feat: Add token-broker service for OAuth session and token management#449

Open
davidhadas wants to merge 1 commit into
kagenti:mainfrom
davidhadas:token-broker-service
Open

Feat: Add token-broker service for OAuth session and token management#449
davidhadas wants to merge 1 commit into
kagenti:mainfrom
davidhadas:token-broker-service

Conversation

@davidhadas

@davidhadas davidhadas commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Summary

The token-broker service provides centralized OAuth token acquisition for AI agents via PKCE-based authorization flows, token caching with JWT expiry parsing, and session management with long-polling coordination.

Fixes: #455

Related issue(s)

kagenti/kagenti#1435

@davidhadas davidhadas requested a review from a team as a code owner June 18, 2026 20:12
Comment thread token-broker/internal/oauth/discovery.go Fixed
@davidhadas davidhadas force-pushed the token-broker-service branch 4 times, most recently from 0efc5f4 to cd98d27 Compare June 18, 2026 20:56
@rhuss

rhuss commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Thanks for the work on this, David. I don't want to block your research progress, but a few observations from the productization side might be still ok:

Completeness: The token-broker cannot complete an OAuth flow end-to-end without the Backend service (Phase 3) that handles the user-facing browser interaction. JWT signature validation is also explicitly skipped. As it stands, this is a Phase 1 foundation, not a shippable component.

Keycloak overlap: The core functionality (authorization code + PKCE, token caching, JWT expiry parsing, session management) overlaps significantly with what Keycloak provides natively through identity brokering. The implementation also reimplements standard Go library functionality (golang.org/x/oauth2, golang-jwt/jwt) rather than using them, which increases the maintenance and CVE surface without clear benefit. The legitimate gap (bridging to external providers not federated with Keycloak) can potentially be addressed through Keycloak identity brokering configuration or CIBA, both of which are production-hardened paths that don't require a new service.

Repo integration: The token-broker is architecturally independent (separate binary, separate Deployment, zero imports from operator code), but it shares the operator's go.mod. This means downstream consumers who don't ship the token-broker still inherit its dependencies in SBOM/vulnerability scans and need to carry deletion patches that require go mod tidy after every upstream sync. Since the code is fully decoupled, moving it to its own Go module (separate go.mod, either as a subdirectory module or a separate repo) would make it cleanly excludable without per-sync patching.

For a long term solution, maybe consider extracting the token-broker into its own module/repo and evaluating whether Keycloak identity brokering or CIBA could address the external-provider token acquisition gap before building a custom service. Happy to discuss the Keycloak/CIBA angle further (see also the discussion on #1435).

@davidhadas davidhadas force-pushed the token-broker-service branch from cd98d27 to 0246162 Compare June 19, 2026 08:25
@davidhadas

davidhadas commented Jun 19, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review.
A Backend service - see kagenti/kagenti#1583
JWT signature validation - committed

As it stands, this is a Phase 1 foundation, not a shippable component.

Agreed. We are laying down the HITL foundation. More work is definitely needed, but we are at a phase where we can finally see the landscape and discuss the design decisions, controls, configs, and user experience.

Does this mean we should hold off on merging this PR once it passes review? Or can we merge it post-review with the explicit note that we still have work before it is production-ready?

Repo integration: Understood. So, keep it in this repo but ensure it uses its own isolated go.mod.

Relationship with Keycloak: Let’s keep that specific discussion over to the Epic issue kagenti/kagenti#1435 as it is part of the big picture we need to discuss.

It should be noted here though that token broker service should have a later PR to include integration with KeyCloak supporting use cases in which Kagneti is deployed with KeyCloak and need to offer OAuth 2 against federated systems.

@davidhadas davidhadas force-pushed the token-broker-service branch 2 times, most recently from 8495d4a to 1e49387 Compare June 19, 2026 09:24
@rhuss

rhuss commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Thanks David. To be clear: please don't feel blocked by me on merging this. My comments were architectural observations, not merge blockers.

I've come to better appreciate that kagenti's upstream is designed for research incubation and rapid iteration, without stability guarantees that would slow contributors down. That's a valid and valuable mode of operation. The productization constraints I've been raising come from a different set of goals, and it's not fair to apply those as a gate on upstream research work.

Go ahead and merge when it passes review. The foundation you're laying here is solid exploration of the HITL problem space. Separate go.mod would still be appreciated for cleaner downstream consumption, but that's a housekeeping ask, not a blocker.

@davidhadas davidhadas force-pushed the token-broker-service branch 2 times, most recently from b942b2b to 88af9aa Compare June 20, 2026 19:45
Comment thread token-broker/internal/oauth/discovery.go Dismissed
@davidhadas

Copy link
Copy Markdown
Contributor Author

@rhuss ,
The token broker service was moved to its own Go module.

The token-broker service provides centralized OAuth token acquisition for AI
agents via PKCE-based authorization flows, token caching with JWT
expiry parsing, and session management with long-polling coordination.

Signed-off-by: David Hadas <david.hadas@gmail.com>

// CleanupExpiredTokens removes all expired tokens from the cache.
// This can be called periodically to free memory.
func (tc *TokenCache) CleanupExpiredTokens() int {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CleanupExpiredTokens() is defined but never wired to a ticker in main.go. For long-running instances, expired tokens will accumulate in memory. Suggestion: add a periodic cleanup
goroutine, or clean up lazily on SetToken when the session map exceeds a threshold.


// OAuthDiscoverer performs OAuth discovery against a resource server.
// Updated to support the new architecture where Token Broker is the OAuth client.
type OAuthDiscoverer interface {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the OAuthDiscoverer interface is tightly coupled to the OAuth authorization code + PKCE flow.
If we want to support alternative token acquisition mechanisms (e.g., Keycloak identity brokering, CIBA, or custom IdP flows), it might be worth defining a higher-level TokenAcquirer
interface that abstracts the grant type entirely, with PKCE as one implementation.

}

// writeBackendError writes a standardized error response (nested format for Backend API).
func writeBackendError(w http.ResponseWriter, status int, code, message string) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API uses two different error response formats: writeError (flat, for AuthBridge) and writeBackendError (nested, for Backend). This makes client integration harder — consumers need
to know which endpoint returns which format. Consider standardizing on a single error envelope.

labels:
app: token-broker
spec:
replicas: 1 # In-memory sessions/token cache: do NOT scale >1 without shared state.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already discussed in the Slack thread — the replicas: 1 constraint with in-memory state is a known limitation. Just noting that the security posture is good (non-root, read-only
filesystem, capabilities dropped).

@akram

akram commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Keycloak overlap question

Roland's observation about the overlap with Keycloak identity brokering is worth exploring further. The core of what the token-broker implements — authorization code + PKCE, token caching, JWT expiry parsing, session management — maps closely to what Keycloak provides natively. CIBA (Client-Initiated Backchannel Authentication) is also an interesting alternative for the HITL use case: the backend initiates the auth request, the user consents via a separate channel, no browser redirect needed.

Provider-agnostic token brokering interface

That said, tying the HITL flow exclusively to Keycloak would create a hard dependency on a specific IdP. Since we're actively working on decoupling from Keycloak on the AuthBridge side, it raises a broader design question: could we define a provider-agnostic interface for token brokering, where Keycloak is one possible backend but not the only one?

Concretely, the current OAuthDiscoverer interface in interfaces.go is tightly coupled to the authorization code + PKCE flow. A higher-level TokenAcquirer abstraction could support multiple grant types — PKCE, CIBA, identity brokering, or custom IdP flows — so the HITL mechanism works regardless of the underlying IdP (Keycloak, external OIDC provider, or something else entirely).

@davidhadas

Copy link
Copy Markdown
Contributor Author

@akram - all good points. Thank you for the review. I will address one by one and present for additional review.

Let record the overall solution directions discussions in the epic kagenti/kagenti#1435 - I think some of your comments related to Keycloak overlap question were already discussed there and maybe you can comment when it was not.

Related to Provider-agnostic token brokering interface, I would like to consider the proposed higher-level TokenAcquirer abstraction and come back with thoughts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feature: Token Broker Service

4 participants