feat: end-to-end Keycloak authentication, RBAC, and Python SDK integration#367
feat: end-to-end Keycloak authentication, RBAC, and Python SDK integration#367mahil-2040 wants to merge 18 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a Keycloak deployment, service, and realm configuration to the Helm chart for external user authentication. The reviewer identified several critical security and configuration issues: wildcard redirect URIs and web origins on the confidential SDK client should be configurable; the admin password should be sourced from a Kubernetes Secret instead of plaintext; production mode lacks support for external databases and proxy/HTTPS settings; SSL requirements should be dynamically enabled; and global image pull secrets need to be propagated to the Keycloak pod template.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds an optional Keycloak deployment to the Helm base chart to support external user authentication, including default values, a Deployment/Service, and a realm-import ConfigMap.
Changes:
- Introduces
keycloakconfiguration block in chart values. - Adds Keycloak Deployment + Service template gated by
keycloak.enabled. - Adds a realm JSON import ConfigMap template for bootstrapping clients/roles.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| manifests/charts/base/values.yaml | Adds configurable values for enabling and running a Keycloak instance |
| manifests/charts/base/templates/keycloak/keycloak.yaml | Adds Deployment/Service templates for Keycloak |
| manifests/charts/base/templates/keycloak/keycloak-realm.yaml | Adds ConfigMap for importing an initial realm configuration |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #367 +/- ##
===========================================
+ Coverage 47.57% 58.60% +11.03%
===========================================
Files 30 37 +7
Lines 2819 3491 +672
===========================================
+ Hits 1341 2046 +705
+ Misses 1338 1235 -103
- Partials 140 210 +70
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
hzxuzhonghu
left a comment
hzxuzhonghu
left a comment
There was a problem hiding this comment.
before adding a bunch of yamls, can you first add a docs exmplain about the proposal and the workflow
@hzxuzhonghu @acsoto |
|
Either is ok |
mahil-2040
force-pushed
the
feat/keycloak-deployment-realm-setup
branch
from
5f5d385 to
9f8f5df
Compare
Okay i will continue in this one only for this time. |
|
@mahil-2040 I donot see your design |
mahil-2040
changed the title
@hzxuzhonghu @acsoto My plan is to implement the entire Keycloak authentication flow within this PR. Please review the desing doc and tell me if any changes are required. Once the proposed approach is reviewed and approved, I'll move forward with the implementation in this same PR. Please let me know your feedback on the design when you have a chance to review it. Thanks! |
hzxuzhonghu
changed the title
|
|
||
| Confidential client secrets can be provided securely via an existing Kubernetes Secret (`keycloak.clients.existingSecret`) to prevent leaking them in Helm values. They are injected into the Keycloak pod as environment variables and securely substituted into the realm JSON during import. The `agentcube-sdk` client is a **public client** (no secret) that uses authorization code with PKCE for interactive flows, following RFC 8252 (OAuth 2.0 for Native Apps). The confidential clients are used only where a secret can be stored securely, and each client ID is separated by trust boundary so service, internal Router, and admin credentials can be scoped and rotated independently. | ||
|
|
||
| Both clients include a **hardcoded audience protocol mapper** (`oidc-audience-mapper`) that injects `agentcube-api` into the access token's `aud` claim. The Router validates `aud = "agentcube-api"`. This follows OAuth2 convention — the audience identifies the resource server (the Router API), not the client that requested the token. |
There was a problem hiding this comment.
Both clients means what. I can guess, but not clear
|
|
||
| type Claims struct { | ||
| Subject string // from standard "sub" claim | ||
| Email string // from standard "email" claim |
There was a problem hiding this comment.
How can user add email claim when using keycloak? Can you point where it is documented
There was a problem hiding this comment.
The email claim is a standard OIDC claim, no special setup is needed. Keycloak includes it automatically via the built-in email client scope, which is a default scope for all clients. The user just needs to have an email address set in their Keycloak profile.
For service accounts (client_credentials grant), there is no real user, so email will be empty. The Router handles this gracefully, it only forwards the email to sandboxes when present (oidc.go:115, handlers.go:291). It is not a required claim.
See Keycloak docs: Server Administration - Client Scopes
There was a problem hiding this comment.
Other than a static config, can we still use keycloak api to dynamically register other roles?
There was a problem hiding this comment.
Yes. The static realm config is only used for initial bootstrap on first startup. After that, roles, clients, users, scopes, etc. can be managed dynamically through:
- Keycloak Admin REST API. : e.g., create a new role:
POST /admin/realms/agentcube/roles
{"name": "custom:role"}
- Keycloak Admin Console, web UI at
/adminfor point-and-click management.
The Router does not need any changes when new roles are added, it only checks that the configured requiredRole exists in the token's role claim. Any additional roles can be used for application-level authorization downstream.
YOu can check here: Keycloak Admin REST API - Roles
hzxuzhonghu
left a comment
hzxuzhonghu
left a comment
There was a problem hiding this comment.
Reviewed the Keycloak auth / RLAC changes. Overall the OIDC validation, RBAC middleware, and SDK auth look solid. Main concern: the sandbox CRD is never actually tagged with the owner label/annotation, even though the PR description and the builder code suggest it should be. RLAC still works because the owner is persisted in the store, but the kubectl-level owner labels won't be present. Left a few inline notes.
|
|
||
| // Set ownership from the Router-signed identity JWT | ||
| if ownerID := extractOwnerID(c.Request); ownerID != "" { | ||
| sandboxEntry.OwnerID = ownerID |
There was a problem hiding this comment.
The owner is set on sandboxEntry here, but the sandbox CR object was already built above (lines 108/110) before we know the owner. So buildSandboxParams.ownerID stays empty and the agentcube.io/owner annotation / agentcube.io/owner-hash label are never applied to the actual CRD. RLAC still works via the store's OwnerID, but the CR labeling described in the PR doesn't happen. Consider extracting ownerID before building, then passing it into the builder.
There was a problem hiding this comment.
I moved the extractOwnerID call to the beginning of the handler and passed the extracted ownerID into buildSandboxByAgentRuntime and buildSandboxByCodeInterpreter before building the sandbox objects. This ensures that the owner metadata is successfully populated on both the Sandbox CRD and the store entry.
|
|
||
| // Ownership metadata for RLAC | ||
| if params.ownerID != "" { | ||
| sandbox.ObjectMeta.Annotations["agentcube.io/owner"] = params.ownerID |
There was a problem hiding this comment.
This block is effectively dead code today: params.ownerID is never assigned anywhere, so this if is always false and the owner annotation/label is never written. The owner only ends up on the sandboxEntry (store), not on the CR. Wiring ownerID into buildSandboxParams/buildSandboxClaimParams would make this work as intended.
| sub, err := verifyIdentityJWT(publicKey, rawToken) | ||
| if err != nil { | ||
| klog.V(2).Infof("Identity JWT verification failed: %v", err) | ||
| return "" |
There was a problem hiding this comment.
extractOwnerID fails open here: if the public key isn't cached yet or verification fails, it returns an empty owner and the sandbox is created with no owner. On the Router side, RLAC fails closed (no owner = access denied), so such a sandbox becomes permanently unreachable by its creator. Worth deciding on one consistent behavior, or at least logging at a higher level so this is easy to spot.
There was a problem hiding this comment.
I've updated the log level from V(2) to klog.Warning for both failure paths (key not cached + verification failed), so this is now easy to spot in production.
| if expiry == 0 { | ||
| return nil, fmt.Errorf("token missing required exp claim") | ||
| } | ||
| if time.Now().After(time.Unix(int64(expiry), 0)) { |
There was a problem hiding this comment.
nit: exp/nbf are compared against time.Now() with no clock-skew leeway. A small allowance (e.g. 30s) avoids spurious rejections when the IdP and Router clocks drift slightly.
There was a problem hiding this comment.
Fixed. Added a clockSkewLeeway = 30s constant and applied it to both exp and nbf comparisons. exp is extended by 30s, nbf is shifted back by 30s. This handles minor clock drift between the IdP and Router without accepting truly expired tokens.
| body = resp.json() | ||
| self._token = body["access_token"] | ||
| expires_in = int(body.get("expires_in", 3600)) | ||
| self._expires_at = time.monotonic() + expires_in - _REFRESH_BUFFER_SECONDS |
There was a problem hiding this comment.
nit: if the IdP returns expires_in <= 30, _expires_at lands in the past and every get_token() call will re-fetch a token. Clamping the buffer (e.g. max(expires_in - buffer, expires_in / 2)) would avoid hammering the token endpoint for short-lived tokens.
There was a problem hiding this comment.
Fixed. Clamped the effective cache lifetime to max(expires_in - buffer, expires_in // 2), so for short-lived tokens the refresh window is at least half the token lifetime instead of going negative and hammering the token endpoint.
- Declarative Realm Import: Created a ConfigMap with the realm JSON defining clients (agentcube-sdk, agentcube-router, workloadmanager, agentcube-admin) and role hierarchy (sandbox:invoke -> sandbox:manage -> admin). - Deployment & Service: Added Keycloak Deployment and Service manifests with management-port health probes and a 300s startupProbe window. - Helm Integration: Added keycloak configuration values to values.yaml, gated behind 'keycloak.enabled: false' to ensure zero impact by default. Signed-off-by: Mahil Patel <mahilpatel0808@gmail.com>
- Parameterized SDK redirect URIs with secure defaults, added support for secretKeyRef for admin/database credentials, and made sslRequired enforcement dynamic based on devMode. - Added configuration blocks for external databases (PostgreSQL/MySQL) and reverse proxy settings (headers, hostname). - Propagated global imagePullSecrets, decoupled Service targetPort by using the named 'http' port, and dynamically derived the realm import filename from Helm values. Signed-off-by: Mahil Patel <mahilpatel0808@gmail.com>
- Added fail-fast validation for database and proxy settings in production mode - Decoupled credentials into chart-managed Secrets with required-value checks - Added restricted securityContext for Keycloak pod and container - Migrated realm configuration to Keycloak 26 default roles and client secrets Signed-off-by: Mahil Patel <mahilpatel0808@gmail.com>
- Require explicit Keycloak admin and client credentials - Enforced production Secret and external DB validation - Fixed Secret key references and quote realm import filename Signed-off-by: Mahil Patel <mahilpatel0808@gmail.com>
Signed-off-by: Mahil Patel <mahilpatel0808@gmail.com>
- Injected client secrets securely via env vars instead of helm values - Templated Keycloak container port dynamically to match service port - Updated proposal to remove old IGNORE_EXISTING terminology Signed-off-by: Mahil Patel <mahilpatel0808@gmail.com>
…sign - Extracted Keycloak into a standalone addon chart (manifests/charts/addons/keycloak) to ensure the core AgentCube chart remains identity-provider agnostic. - Added HA replica support and wildcard URI security validation to the Keycloak addon. - Replaced keycloak.enabled in the base chart with generic OIDC configuration (router.oidc.issuerUrl, audience, and a required rolesClaim for dynamic role extraction). Signed-off-by: Mahil Patel <mahilpatel0808@gmail.com>
- Quoted OIDC flags in router deployment to prevent YAML parsing errors - Fixed inaccurate existingSecret comment in values.yaml - Upgraded Keycloak addon Chart.yaml to apiVersion v2 (Helm 3) - Added validation to block scaling replicas when using embedded H2 database (devMode) Signed-off-by: Mahil Patel <mahilpatel0808@gmail.com>
…values Signed-off-by: Mahil Patel <mahilpatel0808@gmail.com>
- Implemented provider-agnostic OIDC authentication with JWKS caching and strict JWT algorithm validation. - Added RBAC middleware to enforce configurable role-based access control. - Enhanced SessionManager to securely forward verified external user identities to WorkloadManager. Signed-off-by: Mahil Patel <mahilpatel0808@gmail.com>
…dentity header Signed-off-by: Mahil Patel <mahilpatel0808@gmail.com>
…nforcement - Enforced resource-level access control (RLAC) in backend and reject router JWTs missing sub claim. - Added Python SDK pluggable auth providers and routed data plane traffic via Router proxy. - Wired Keycloak realm imports, client secrets, and service account roles dynamically. - Added OIDC E2E tests Signed-off-by: Mahil Patel <mahilpatel0808@gmail.com>
Signed-off-by: Mahil Patel <mahilpatel0808@gmail.com>
…d fixed python lint issue Signed-off-by: Mahil Patel <mahilpatel0808@gmail.com>
… and updated related documentation Signed-off-by: Mahil Patel <mahilpatel0808@gmail.com>
…oved token verification - Renamed OAuth2 client IDs : agentcube-service -> agentcube-app and agentcube-sdk -> agentcube-cli. - Fixed ownership metadata propagation by extracting ownerID before constructing Sandbox CRD objects. - Added 30s clock-skew leeway to OIDC validation and bumped token-error log levels to warnings. - Clamped Python SDK token refresh buffer to prevent infinite fetching loops on short-lived tokens. Signed-off-by: Mahil Patel <mahilpatel0808@gmail.com>
mahil-2040
force-pushed
the
feat/keycloak-deployment-realm-setup
branch
from
fd7110f to
99465bc
Compare
…API schema - Renamed all remaining Helm values, secrets, and E2E configs to use app and cli prefixes. - Failed sandbox creation with 401/503 on identity verification failures instead of failing open. - Supported custom namespaced claims in OIDC validation by checking flat keys before splitting on . - Aligned REST API schema by renaming the ownerID JSON tag to ownerId for consistency. - Tightened E2E tests to strictly assert that admin bypass succeeds with 200 OK. Signed-off-by: Mahil Patel <mahilpatel0808@gmail.com>
Signed-off-by: Mahil Patel <mahilpatel0808@gmail.com>
| // Initialize OIDC validator when issuer URL is configured. | ||
| if config.JWTIssuerURL != "" { | ||
| if config.JWTRoleClaim == "" { | ||
| return nil, fmt.Errorf("--jwt-role-claim is required when --jwt-issuer-url is set") | ||
| } | ||
| if config.JWTRequiredRole == "" { | ||
| return nil, fmt.Errorf("--jwt-required-role is required when --jwt-issuer-url is set") |
| // Ownership metadata for RLAC | ||
| if params.ownerID != "" { | ||
| sandbox.ObjectMeta.Annotations["agentcube.io/owner"] = params.ownerID | ||
| sandbox.ObjectMeta.Labels["agentcube.io/owner-hash"] = sha256Short(params.ownerID) | ||
| } |
| // setIdentityHeader signs and attaches the caller's identity as a JWT. | ||
| func (m *manager) setIdentityHeader(ctx context.Context, req *http.Request) { | ||
| if m.jwtManager == nil { | ||
| return | ||
| } | ||
| claims, ok := ctx.Value(contextKeyOIDCClaims).(*Claims) | ||
| if !ok || claims == nil { | ||
| return | ||
| } | ||
| identityClaims := map[string]interface{}{ | ||
| "sub": claims.Subject, | ||
| "aud": "workloadmanager", | ||
| } | ||
| if claims.Email != "" { | ||
| identityClaims["email"] = claims.Email | ||
| } | ||
| identityToken, err := m.jwtManager.GenerateToken(identityClaims) | ||
| if err != nil { | ||
| klog.Warningf("Failed to sign identity JWT for WM: %v", err) | ||
| return | ||
| } | ||
| req.Header.Set("X-AgentCube-User-Identity", identityToken) | ||
| } |
| // Validate audience | ||
| aud, _ := claims["aud"].(string) | ||
| if aud != "workloadmanager" { | ||
| return "", fmt.Errorf("invalid audience: %q", aud) | ||
| } | ||
|
|
||
| sub, _ := claims["sub"].(string) | ||
| if sub == "" { | ||
| return "", fmt.Errorf("missing sub claim") | ||
| } | ||
|
|
||
| return sub, nil |
| def _request(self, method: str, endpoint: str, body: Optional[bytes] = None, **kwargs) -> requests.Response: | ||
| """Make a request to the Data Plane via Router. | ||
|
|
||
| Note: Router handles JWT authentication, so we don't add Authorization header here. | ||
| """ | ||
| url = urljoin(self.base_url, endpoint) | ||
|
|
||
| headers = {} | ||
| if self._auth: | ||
| headers["Authorization"] = f"Bearer {self._auth.get_token()}" | ||
| if body: | ||
| headers["Content-Type"] = "application/json" | ||
|
|
| body = resp.json() | ||
| self._token = body["access_token"] | ||
| expires_in = int(body.get("expires_in", 3600)) | ||
| effective = max(expires_in - _REFRESH_BUFFER_SECONDS, expires_in // 2) | ||
| self._expires_at = time.monotonic() + effective | ||
| return self._token or "" |
| KEYCLOAK_TOKEN=$(curl -s -X POST \ | ||
| -H "Host: keycloak.${AGENTCUBE_NAMESPACE}.svc.cluster.local:8080" \ | ||
| "http://localhost:8082/realms/agentcube/protocol/openid-connect/token" \ | ||
| -d "grant_type=client_credentials" \ | ||
| -d "client_id=agentcube-app" \ | ||
| -d "client_secret=e2e-app-secret" | jq -r '.access_token') | ||
|
|
||
| ADMIN_TOKEN=$(curl -s -X POST \ | ||
| -H "Host: keycloak.${AGENTCUBE_NAMESPACE}.svc.cluster.local:8080" \ | ||
| "http://localhost:8082/realms/agentcube/protocol/openid-connect/token" \ | ||
| -d "grant_type=client_credentials" \ | ||
| -d "client_id=agentcube-admin" \ | ||
| -d "client_secret=e2e-admin-secret" | jq -r '.access_token') | ||
|
|
||
| # Override the K8s SA token with the Keycloak token | ||
| export API_TOKEN="${KEYCLOAK_TOKEN}" | ||
| export ADMIN_TOKEN="${ADMIN_TOKEN}" | ||
| export OIDC_ENABLED="true" | ||
| export KEYCLOAK_TOKEN_URL="http://localhost:8082/realms/agentcube/protocol/openid-connect/token" | ||
| echo "Keycloak tokens acquired" |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR introduces end-to-end external user authentication and authorization using Keycloak as the identity provider. It expands the initial scope to include the complete flow in a single PR: Keycloak deployment, Router integration, Resource-Level Access Control (RLAC), Python SDK support, and the formal design proposal.
1. Design Proposal
docs/design/keycloak-proposal.mddetailing the architecture, request flow, and implementation decisions for the external authentication layer.2. Keycloak Deployment
agentcuberealm, role hierarchy (sandbox:invoke→sandbox:manage→admin), and OAuth2 clients.3. Router OIDC Integration & RBAC
go-oidcwith automatic JWKS key caching.sandbox:invokerealm role on all/v1/*endpoints.--enable-external-auth(default: off, zero impact).4. Downstream Identity Forwarding & RLAC
sub) in internal PicoD JWT claims.X-AgentCube-User-Idheader to WorkloadManager.agentcube.io/owner: <user_id>label.OwnerIDbefore proxying requests.5. Python SDK Auth Integration
AuthProviderprotocol).ServiceAccountAuthforclient_credentialsflow with automatic token refresh.TokenAuthfor static tokens.6. Testing
Which issue(s) this PR fixes:
Fixes Part of #243
Special notes for your reviewer:
keycloak.enabled=true. Existing deployments are unaffected.github.com/coreos/go-oidc/v3(used by Kubernetes itself).Does this PR introduce a user-facing change?:
yes