Repository navigation
feat(control-plane): CITrust reconciler - Keycloak v2 external-internal token-exchange (#819) - #821
Conversation
…al token-exchange (#819) Replace the stubbed CITrust reconciler (which made zero Keycloak calls) with a real implementation against the Keycloak 26.x external-to-internal token-exchange v2 mechanism (TOKEN_EXCHANGE_EXTERNAL_INTERNAL_V2). The v1 token-exchange permission API (.../management/permissions) returns 501 on the production fleet Keycloak and is never called (PLAT-CITR-08). New keycloak.Client CI-federation surface (internal/keycloak/ci_federation.go), idempotent and driven over the v2 admin REST API: - (a) per-trust scoped claim-to-workspace IdP mapper on the adopted CIProvider IdP: the repository / repository_owner (plus tighter) claim match is the trust boundary; a non-matching token lands in no workspace group and is rejected default-deny (PLAT-CITR-04). - (b) token-exchange authorization opting the gateway client in as the exchange audience for the public, keyless federation client. - (c) gateway RBAC binding of the role onto the derived workspace group. Delete removes the per-trust subject mapper (authoritative revocation, PLAT-CITR-05.02); the shared workspace group / role binding / exchange opt-in are preserved for sibling trusts. Reconciler (internal/reconciler/ci_trust_reconciler.go): resolve CIProvider (forge + adopted idp_alias) and gateway (Keycloak client + owning cluster), derive the forge-namespaced workspace, realize the artifacts, and write status (exchange_endpoint, workspace, workflow_hint). Cluster-scoped like the RoleBinding reconciler; a gone CIProvider on delete is a no-op (PLAT-CITR-05.01). Reconcile is idempotent and re-entrant (PLAT-CITR-09). Tests: table-driven httptest-mocked admin API for the keycloak methods (create/idempotency/delete/missing-IdP/missing-client/subject-claim shaping), and reconciler upsert/delete against a faked keycloak client and faked gRPC services. Code-comment references to spec IDs PLAT-CITR-08/09/05.02 (added by the approved spec PR #820, not yet on main) are written as prose so this PR is CI-green off main; the IDs are retained here in the commit/PR body for traceability. Refs #819 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the
 |
HyperShell environment destroyedThis ephemeral OpenShift environment has been destroyed. Comment |
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #821 +/- ##
==========================================
+ Coverage 43.88% 44.31% +0.43%
==========================================
Files 383 384 +1
Lines 29514 29822 +308
==========================================
+ Hits 12951 13215 +264
- Misses 14923 14924 +1
- Partials 1640 1683 +43
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
markturansky
left a comment
markturansky
left a comment
There was a problem hiding this comment.
Code review verdict: agent/review-code-rejected. Blocking findings are on the trust boundary (findings 1 and 2). Full rationale is on issue #819. Inline comments mark each finding.
| if _, err := c.doRequest(ctx, http.MethodPost, path, body); err != nil { | ||
| return fmt.Errorf("bind role %q to workspace group %q on gateway client %q: %w", | ||
| spec.Role, spec.Workspace, spec.GatewayClientID, err) | ||
| } |
There was a problem hiding this comment.
BLOCKER (finding 1): cross-repo / cross-gateway privilege escalation. The workspace group is per-owner (gh-), but each trust POSTs its own role onto this one shared group, and POST is additive. Two repos under one owner with different roles both land in gh-, so the lower-privileged repo inherits the sibling's higher role. The per-repo subject mapper gates entry, but the role grant is aggregated per-owner. Bind the role to a per-repo (per-trust) group, or otherwise scope the grant to the subject that the mapper matched.
| } | ||
| if len(subjectClaims(s.Forge, s.Subject)) == 0 { | ||
| return fmt.Errorf("CI federation requires at least one subject claim constraint (the trust boundary); refusing to authorize an unconstrained exchange") | ||
| } |
There was a problem hiding this comment.
BLOCKER (finding 2): validate() only requires that subjectClaims is non-empty. A github/forgejo trust with only subject_ref or subject_environment (no subject_repository) passes, and the mapper then admits ANY repository building that ref. The comment above says repository/owner is 'the real gate'; enforce it. Require the identity claim (repository/repository_owner for github/forgejo, project_path for gitlab), not just any claim.
| case "gitlab": | ||
| add("project_path", s.ProjectPath) | ||
| add("ref", s.Ref) | ||
| add("environment", s.Environment) |
There was a problem hiding this comment.
Finding 3: the gitlab case omits ref_protected, which the github/forgejo case honors below. A GitLab trust with RefProtected=true silently federates unprotected refs.
| func (c *Client) DeleteCIFederation(ctx context.Context, spec CIFederationSpec) error { | ||
| if spec.IdpAlias == "" || spec.TrustID == "" { | ||
| return fmt.Errorf("CI federation delete requires idp_alias and trust id") | ||
| } |
There was a problem hiding this comment.
Finding 4: when a provider resolves but status.idp_alias is empty, no mapper was ever created, yet this returns an error instead of a no-op. Combined with the WatchCITrusts no-resync limitation, a delete can stay failing. Treat an empty idp_alias on delete as nothing-to-revoke.
| } | ||
| if roleUUID == "" { | ||
| return fmt.Errorf("role %q not found on gateway client %q", spec.Role, spec.GatewayClientID) | ||
| } |
There was a problem hiding this comment.
Finding 5: getClientRoleUUID -> doRequest turns any status >= 400 (including 404) into a generic error, so this roleUUID == "" branch is dead code. A provisioning race (client exists, roles not yet created) surfaces as a terminal 'reconcile keycloak federation' error instead of the retryable ClientNotFoundError path used for the client lookup. Map a 404 on the role to the not-ready retry path.
… the trust boundary Address the code-review findings on #819 / PR #821. 1. BLOCKER - cross-repo / cross-gateway privilege escalation. The role grant was aggregated onto a shared per-owner group (gh-<owner>), so a low-privilege repo inherited a sibling repo's higher role and roles leaked across gateways. Both the IdP entry mapper and the gateway role binding now land on a per-trust group keyed on the CITrust id (one repository, one gateway, one role). The gh-<owner> workspace stays an informational status/quota label only. 2. BLOCKER - validate() now requires the repository/owner identity claim for github/forgejo (project_path for gitlab); a ref/environment-only trust is rejected, since the identity claim is the real trust boundary. 3. The gitlab subject predicate now includes ref_protected. 4. DeleteCIFederation is a tolerant no-op on an empty idp_alias / already-gone mapper, so a delete cannot wedge given WatchCITrusts does not resync. 5. A missing gateway role now surfaces the retryable ClientNotFoundError path: the role lookup inspects the raw 404 (doRequest collapsed every >=400 into a generic error, leaving the retry branch dead). Tests lock each fix, including same-owner-different-repo non-inheritance and cross-gateway isolation at both the keycloak and reconciler layers. The spec's PLAT-CITR-02 realization is clarified to mandate per-trust RBAC isolation and the identity-claim trust boundary, with scenario PLAT-CITR-02.02. LIVE-VERIFY markers retained. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Review findings addressed (
|
Re-review (iteration 2) - APPROVEBoth iteration-1 BLOCKING findings are genuinely fixed and locked by non-vacuous tests. Verified against the code, not just the commit message. Blocker 1 - cross-repo / cross-gateway privilege escalation: FIXEDThe role grant and the IdP entry mapper now both target the same per-trust group, keyed on the CITrust id:
No remaining code path grants a trust's role onto a shared per-owner group. Tests prove it against real fake-Keycloak state (distinct group ids, recorded role mappings), not string comparison:
Blocker 2 - validate() must require an identity claim: FIXED
Also confirmed: gitlab CICode-gating checks green: Go - control plane, Unit / Go - control plane, Repository policy, Konflux control-plane, codecov, both CI Gates. Non-blocking follow-ups (do not gate this PR)
Verdict: APPROVE. The two blockers are closed and proven; remaining items are hardening follow-ups. |
Summary
Implements the
CITrustreconciler (issue #819, PLAT-DATA-25), replacing thestub that made zero Keycloak calls. Realizes keyless CI → gateway federation
through the Keycloak 26.x external-to-internal token-exchange v2 mechanism
(
TOKEN_EXCHANGE_EXTERNAL_INTERNAL_V2). The v1 token-exchange permission API(
.../management/permissions) returns 501 on the production fleet Keycloak andis never called.
This is the code reconciliation of the approved spec PR #820
(
specs/platform/ci-trust-ext-api.spec.md, prefixPLAT-CITR).Refs #819(non-closing).What changed
New Keycloak CI-federation surface (
components/control-plane/internal/keycloak/ci_federation.go),idempotent, driven over the v2 admin REST API. A
CITrustis realized as three artifacts:mapper (
oidc-advanced-group-idp-mapper) on the adopted CIProvider IdP. Therepository/repository_owner(plusproject_path/environment/ref/ref_protectedwhen set) claim match is the trust boundary (PLAT-CITR-04): atoken carrying the pinned audience but a non-matching repository lands in no
workspace group, holds no role, and is rejected default-deny.
audience (
standard.token.exchange.enabled) for the shared public, keylessfederation client (
ci-federation).roleto the derived workspacegroup on the gateway client.
Delete removes the per-trust subject mapper (the authoritative revocation: the
deleted subject can no longer reach the workspace group). The shared workspace
group, its gateway role binding, and the gateway exchange opt-in are preserved
for sibling trusts (another repo under the same owner, or another trust on the
same gateway), and grant nothing without a matching subject mapper.
Reconciler (
components/control-plane/internal/reconciler/ci_trust_reconciler.go):resolves CIProvider (forge + adopted
status.idp_alias) and gateway (Keycloakclient
{name}-{id}+ owning cluster), derives the forge-namespaced workspace(
gh-<owner>/gl-<namespace>), realizes the artifacts, and writesstatus.exchange_endpoint/status.workspace/status.workflow_hint.Cluster-scoped like the RoleBinding reconciler (only the control plane whose
cluster hosts the gateway acts). Idempotent and re-entrant; a gone CIProvider on
delete is a no-op.
NewCITrustReconcilernow takesclusterID.Tests
ci_federation_test.go): table-driven, httptest-mockedadmin API — create-all-artifacts, idempotency (create once / update in place /
no duplicate / no rewrite when already enabled), missing-IdP fails closed,
missing gateway client is retryable (
ClientNotFoundError), unconstrainedsubject rejected, GitLab
project_pathclaim, tighter claims, delete removesthe subject mapper while preserving the shared binding, delete of an absent
mapper is a no-op.
ci_trust_reconciler_test.go): upsert realizes federation +writes status, foreign-cluster skip, gateway-not-found retryable, keycloak
client-not-ready retryable, delete revokes, cascade-deleted gateway still
revokes, missing provider on delete is a no-op, nil-keycloak skip, workspace
derivation.
Per issue #819 Open Questions, the exact Keycloak 26.6 v2 admin-REST shape for
parts of (a) and (b) cannot be confirmed without a live Keycloak (none available
here). These calls are implemented against the documented v2 admin REST API,
each idempotent and unit-tested against a mock, and marked in code with
LIVE-VERIFY:group mapper. Whether the predicate is best enforced as this IdP mapper vs. an
external-internal exchange authorization policy (Open Question Add OpenShell gateway provisioning specs from ACP #2) needs
empirical confirmation on 26.6.
standard.token.exchange.enabledattribute; the precise per-requester authorization for external-internal v2
with a public requester (Open Question Restructure: move API server to components/api-server, add specs and skills #1) needs live confirmation.
No client secret is required anywhere (keyless). The subject-predicate trust
boundary and default-deny behavior do not depend on the uncertain endpoints.
Known limitation (pre-existing, not introduced here)
WatchCITrustslogs handler errors without a resync/requeue (unlikeWatchRoleBindings). A reconcile error (including a failed revoke) is retriedonly on the next watch reseed (reconnect / idle timeout), not on a periodic
resync. Wiring a resync for CITrust/CIProvider watches is a follow-up.
Spec ID note
Code-comment references to
PLAT-CITR-08/-09/-05.02(added by theapproved spec PR #820, not yet on
main) are written as prose so this PR isCI-green off
main; the IDs are retained in the commit message and here fortraceability. Once #820 merges they can be tightened.
🤖 Generated with Claude Code