Repository navigation
[HYPERSHELL-262] feat(rbac): assign gateway:creator by default on user provisioning - #263
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Amber reviewStatus: Complete |
amber-review-bot
left a comment
amber-review-bot
left a comment
There was a problem hiding this comment.
Verdict
REQUEST_CHANGES. The default-role logic in SyncJWTRoles is correct in isolation, but it is never reached for the exact users this PR targets - both call sites gate SyncJWTRoles behind if len(jwtRoles) > 0, so a user whose JWT carries no realm roles never gets the default gateway:creator binding. The integration tests call SyncJWTRoles directly and therefore mask this gap.
Blocker
The feature's core goal is not delivered through the real call paths. The PR summary states the intent is that "new users who have no Keycloak roles yet" receive a gateway:creator binding on first request so flipping RBAC_ENFORCE=true does not strand them. But SyncJWTRoles is only invoked when the extracted JWT role list is non-empty:
components/api-server/pkg/rbac/user_provisioning.go:52-if len(jwtRoles) > 0 { ... syncer.SyncJWTRoles(...) }components/api-server/pkg/rbac/grpc_interceptor.go:203- same guard
A brand-new user with no synced realm roles produces jwtRoles == nil/empty, so SyncJWTRoles (and therefore the new defaultRoles merge) is skipped entirely. The default gateway:creator binding is only assigned to users who already carry at least one realm role. This means the fix works only when Keycloak happens to emit at least one realm role (e.g. default-roles-<realm>), and silently fails for the precise "no roles yet" case it targets.
The four new integration tests call rbService.SyncJWTRoles(..., nil) directly, bypassing the middleware guard, so they pass while the real HTTP/gRPC path does not exercise the default assignment. Please either (a) drop the len(jwtRoles) > 0 guard so SyncJWTRoles always runs after provisioning, or (b) invoke the default-role assignment on the provisioning path independent of JWT role presence - and add a test that drives the middleware/interceptor with an empty-roles token. Confidence: High on the code path; Medium on production impact (depends on whether your Keycloak realm always emits default roles).
Minor
-
Silently dropped default roles. In
SyncJWTRoles(service.go:106-110) anddefaultRolesFromEnv(plugin.go:43-55), any configuredRBAC_DEFAULT_ROLESentry that is not present inroles.JWTSyncedRoles(currently onlyplatform:adminandgateway:creator) is discarded with no log or startup validation. An operator settingRBAC_DEFAULT_ROLES=gateway:ownergets no effect and no signal. Emit a warning (or validate at construction) when a configured default role is not sync-eligible. -
Default posture broadens create permissions. The default (empty env var) grants
gateway:creatorglobally to every authenticated user. That is the stated intent, but it is a meaningful shift in default authorization posture and cannot be revoked per-user (default roles are always re-added on sync). Please confirm maintainers want this as the shipped default rather than opt-in.
Cross-PR coordination
No material cross-PR coordination issue requires maintainer action.
Convention checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
| No secrets in logs or responses | Pass |
| Config separate from code (env-driven) | Pass |
| Reconcile (update-or-create) pattern | Pass |
| Test diff scrutiny (no flipped pre-existing assertions) | Pass |
| Feature covered by tests on real call path | Fail |
| Config input validated / surfaced | Fail |
Findings summary (highest severity first)
- [Blocker] Default-role assignment never runs for users with no JWT roles because both call sites guard
SyncJWTRoleswithlen(jwtRoles) > 0; tests bypass the guard - Correctness / Feature Gap (user_provisioning.go:52,grpc_interceptor.go:203,service.go:103-110) - [Minor] Configured default roles outside
JWTSyncedRolesare silently dropped with no log or validation - Observability / Config (service.go:106-110,plugin.go:43-55) - [Minor] Ships
gateway:creatorto all authenticated users by default; needs explicit maintainer sign-off - Security Posture (plugin.go:43-46)
| if roles.JWTSyncedRoles[r] { | ||
| jwtRoleSet[r] = true | ||
| } | ||
| } |
There was a problem hiding this comment.
[Blocker] This default-role merge is correct, but it never executes for the target audience. Both callers gate the sync behind if len(jwtRoles) > 0 (pkg/rbac/user_provisioning.go:52 and pkg/rbac/grpc_interceptor.go:203). A new user whose JWT carries no realm roles yields an empty jwtRoles, so SyncJWTRoles is skipped and this block never runs - meaning gateway:creator is not assigned for exactly the "no Keycloak roles yet" case the PR targets. The new tests call SyncJWTRoles directly and bypass that guard. Remove the len>0 guard (or run default assignment independently of JWT role presence) and add a test that drives the middleware/interceptor with an empty-roles token.
| // They must be in JWTSyncedRoles to participate in the sync lifecycle | ||
| // (deduplication, removal on revocation). | ||
| for _, r := range s.defaultRoles { | ||
| if roles.JWTSyncedRoles[r] { |
There was a problem hiding this comment.
[Minor] Any configured default role not present in roles.JWTSyncedRoles (currently only platform:admin and gateway:creator) is silently discarded here - no log, no validation. An operator setting e.g. RBAC_DEFAULT_ROLES=gateway:owner gets no effect and no signal. Emit a warning (or validate at construction) when a configured default role is not sync-eligible.
| func defaultRolesFromEnv() []string { | ||
| val := os.Getenv("RBAC_DEFAULT_ROLES") | ||
| if val == "" { | ||
| return []string{roles.RoleGatewayCreator} |
There was a problem hiding this comment.
[Minor] With the default (empty env var) this grants gateway:creator globally to every authenticated user, and default roles can never be revoked per-user (they are re-added on every sync). This is the stated intent, but it is a meaningful shift in default authorization posture - please confirm maintainers want this shipped as the default rather than opt-in.
36f9def to
726b7dc
Compare
|
Addressed amber-review feedback in 726b7dc: Blocker resolved: Both call sites ( Minor 1 resolved: Minor 2 - maintainer sign-off: The |
Amber reviewStatus: Complete |
amber-review-bot
left a comment
amber-review-bot
left a comment
There was a problem hiding this comment.
Verdict
REQUEST_CHANGES. The default-role mechanism is well-tested and the idempotency/no-revoke semantics are sound, but this PR ships a gofmt violation that contradicts the stated lint results, and it silently broadens the default gateway:creator grant to every principal that authenticates - including gRPC service accounts - which needs a maintainer decision.
Amber Analysis
This change merges a configurable RBAC_DEFAULT_ROLES set (default gateway:creator) into SyncJWTRoles and makes the HTTP and gRPC provisioning paths sync unconditionally so role-less users are not stranded under RBAC_ENFORCE=true. The core logic, dedup, and non-revocation behavior are correct and covered by new integration tests; two findings below need attention before merge.
Findings
[Major] gofmt violation introduced in user_provisioning.go (Convention / Formatting).
components/api-server/pkg/rbac/user_provisioning.go:31 - the payload, err := auth.GetAuthPayload(r) line was re-indented with an extra tab (4 tabs where 3 are correct, see cat -A). This fails gofmt -l and will trip the lint gate, which contradicts the PR description's "gofmt clean / make lint-api-server - 0 issues". Fix: run gofmt -w (or make lint-api-server locally) and recommit. Confidence: High.
[Major] Default gateway:creator is now granted to service accounts and every authenticated principal (Design / RBAC scope).
In grpc_interceptor.go, provisionUserForGRPC runs (lines 18, 62) before the isServiceAccount allowlist check (line 25/79). Because SyncJWTRoles is now invoked even when jwtRoles is empty, any principal that authenticates - including machine/service-account identities that previously received no bindings (empty JWT roles -> no sync) - now gets a persisted global gateway:creator binding. Previously those identities were never synced. This is an intentional broadening for human users but is likely unintended for service accounts and expands the RBAC surface. Please confirm whether service accounts / machine identities should be excluded from default-role assignment, and document the "all authenticated users can create gateways by default" policy in the RBAC spec so operators enabling RBAC_ENFORCE=true understand the effective default. Confidence: Medium.
[Minor] Default-role bindings are permanent and cannot be revoked (Design note).
service.go:112 - default roles are always re-added to jwtRoleSet and are in JWTSyncedRoles, so a manual removal of a user's gateway:creator binding is re-created on the next authenticated request. This matches the PR's stated intent ("never revoked by subsequent syncs"), but it means there is no per-user opt-out short of changing RBAC_DEFAULT_ROLES globally. Worth calling out in the spec/runbook. Confidence: High.
[Minor] SyncJWTRoles now runs on every authenticated request for role-less users (Performance note).
Previously the sync (and its FindByUserID + per-binding role lookups) only ran when len(jwtRoles) > 0; it now runs for every authenticated request regardless. The added DB work is the same shape already incurred for users with roles, so the marginal cost is acceptable, but it is a per-request DB round-trip on the hot auth path worth monitoring. Confidence: Medium.
Positive notes
- Test coverage is genuinely additive: the two modified pre-existing tests (
TestRoleBindingCreate_GatewayOwner,TestRoleBindingDelete) only add owner-binding setup required by the newly wiredrbacplugin import; no existing assertion was flipped or weakened. - Idempotency, coexistence with JWT-carried roles, and non-revocation are each covered by a dedicated test.
- Misconfigured
RBAC_DEFAULT_ROLESentries (not inJWTSyncedRoles) are logged and safely ignored rather than silently mis-applied.
Cross-PR coordination
No material cross-PR coordination issue requires maintainer action.
Findings Summary (ordered by severity, highest first):
- [Major]
gofmtviolation contradicts stated lint-clean result - Convention/Formatting (user_provisioning.go:31) - [Major] Default
gateway:creatornow granted to service accounts / all authenticated principals - Design/RBAC Scope (grpc_interceptor.go:208) - [Minor] Default-role bindings cannot be revoked per-user - Design (service.go:112)
- [Minor]
SyncJWTRolesruns on every authenticated request for role-less users - Performance (user_provisioning.go:58, grpc_interceptor.go:208)
Convention Checklist:
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
| No secrets in logs or responses | Pass |
| Input validated | Pass |
| Reconcile (update-or-create) pattern used | Pass |
| Test diff scrutiny (no silent contract flips) | Pass |
gofmt applied |
Fail |
| return func(next http.Handler) http.Handler { | ||
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| payload, err := auth.GetAuthPayload(r) | ||
| payload, err := auth.GetAuthPayload(r) |
There was a problem hiding this comment.
[Major] gofmt violation. This line is indented with an extra tab (4 tabs where 3 are correct). gofmt -l flags it and the lint gate will fail, which contradicts the PR description's "gofmt clean / make lint-api-server - 0 issues". Run gofmt -w and recommit.
| // Always sync even when jwtRoles is empty: SyncJWTRoles applies | ||
| // configured default roles (e.g. gateway:creator) so that users with | ||
| // no Keycloak realm roles still receive their initial bindings. | ||
| if syncer != nil { |
There was a problem hiding this comment.
[Major] Default role now granted to service accounts too. provisionUserForGRPC runs before the isServiceAccount allowlist check (line 25/79), and SyncJWTRoles is now called even with empty jwtRoles. Machine/service-account identities that previously had no bindings (empty JWT -> no sync) will now get a persisted global gateway:creator binding. Please confirm whether service accounts should be excluded from default-role assignment, and document the effective "all authenticated users can create gateways" default in the RBAC spec.
| // Default roles are always treated as present regardless of JWT content. | ||
| // They must be in JWTSyncedRoles to participate in the sync lifecycle | ||
| // (deduplication, removal on revocation). | ||
| for _, r := range s.defaultRoles { |
There was a problem hiding this comment.
[Minor] Default-role bindings are permanent. Default roles are always re-added to jwtRoleSet and are JWTSyncedRoles, so a manual removal of a user's gateway:creator binding is re-created on the next authenticated request. This matches the stated intent, but there is no per-user opt-out short of changing RBAC_DEFAULT_ROLES globally - worth documenting in the spec/runbook.
|
Follow-up fix pushed in 1875209. The E2E failures revealed that applying Root cause: Fix: Defaults now apply only when This preserves the original bootstrap intent (brand-new users with zero Keycloak assignments get Also fixed the gofmt violation in |
Amber reviewStatus: Complete |
amber-review-bot
left a comment
amber-review-bot
left a comment
There was a problem hiding this comment.
Verdict
The default-role bootstrap is well-tested for genuinely zero-role JWTs and the config plumbing (RBAC_DEFAULT_ROLES, JWTSyncedRoles gating, idempotent sync) is clean. My one substantive concern is that the feature keys off raw JWT emptiness (len(jwtRoles) == 0), which is fragile against real Keycloak tokens that carry built-in default realm roles - this may prevent the bootstrap from firing in production and can revoke a previously granted default. Recommend verifying against the actual Keycloak mapper output before merge.
Summary
This PR makes SyncJWTRoles merge a configurable RBAC_DEFAULT_ROLES set (default gateway:creator) into the effective role set when a user's JWT carries no realm roles, and wires the middleware/interceptor to sync on every authenticated request so RBAC_ENFORCE=true does not strand brand-new users. The implementation is convention-compliant (proper error wrapping, no panics, no secret logging, config separated from code, constructor-time validation of default roles against JWTSyncedRoles), and the four new integration tests exercise assignment, idempotency, persistence, and JWT-roles-present exclusion.
Findings
[Major] The len(jwtRoles) == 0 gate is fragile against real Keycloak tokens - service.go:113
The default bootstrap only runs when the JWT carries zero extracted roles. But extractRealmRolesFromClaims (pkg/rbac/jwt_roles.go:22) reads the roles/groups/realm_access.roles claims, and standard Keycloak realm-role mappers emit the built-in default realm roles that every user receives (default-roles-<realm>, offline_access, uma_authorization). If those appear in the mapped claim, then:
len(jwtRoles) == 0is never true for real users, so thegateway:creatorbootstrap never fires in production - defeating the stated purpose of the change.- Because none of those default roles are in
JWTSyncedRoles,jwtRoleSetis empty while the token is technically non-empty, so a user who was previously bootstrapped withgateway:creator(e.g. before a default role landed in their token) has it deleted by the revoke loop atservice.go:162.
Consider basing the "user has no meaningful roles yet" decision on the set of recognized/synced roles (e.g. bootstrap when len(jwtRoleSet) == 0) rather than raw JWT emptiness, and confirm the concrete claim contents produced by the hypershell-frontend Keycloak mapper. This also determines whether the change actually works end-to-end for the RBAC_ENFORCE=true scenario in the test plan. Confidence: Medium (depends on the deployed mapper configuration).
[Minor] PR body claim "never revoked by subsequent syncs" is only conditionally true - service.go:162
The revoke loop deletes any synced global binding not present in jwtRoleSet. Because defaults are only re-added when len(jwtRoles) == 0, a default binding is revoked on the first sync where the JWT is non-empty. Given the current role set (platform:admin, gateway:creator), the only realistic "you lose creator" case is gaining platform:admin, which is broader - so it is currently harmless - but the code comment and PR body overstate the guarantee. Please align the wording with the actual behavior. Confidence: High.
[Minor] Sync now runs on every authenticated request, including zero-role tokens - pkg/rbac/user_provisioning.go:59, pkg/rbac/grpc_interceptor.go:209
Moving SyncJWTRoles out of the len(jwtRoles) > 0 guard means every request from a zero-role user now performs a FindByUserID (plus role lookups). It is idempotent and consistent with the existing per-request sync for role-carrying users, so this is an observation rather than a defect, but worth confirming it is acceptable on the hot path. Confidence: High.
Cross-PR coordination
No material cross-PR coordination issue requires maintainer action.
Findings Summary (ordered by severity, highest first)
- [Major]
len(jwtRoles) == 0bootstrap gate is fragile against Keycloak default realm roles; may never fire and can revoke a prior default - Correctness / RBAC (service.go:113, service.go:162, jwt_roles.go:22) - [Minor] "Default roles are never revoked" claim is only true while the JWT stays empty - Docs accuracy (service.go:162)
- [Minor] Sync now executes on every authenticated request for zero-role users - Performance / hot path (user_provisioning.go:59, grpc_interceptor.go:209)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
| No secrets in logs or responses | Pass |
Input validated (default roles checked against JWTSyncedRoles) |
Pass |
| Reconcile (update-or-create), not create-or-skip | Pass |
Config separated from code (RBAC_DEFAULT_ROLES) |
Pass |
| Conventional commit messages | Pass |
| Test Diff Scrutiny (modified pre-existing tests) | Pass |
| // Users who already have Keycloak realm-role assignments (even non-synced | ||
| // ones) are managed entirely through Keycloak; applying defaults to them | ||
| // would grant capabilities that Keycloak intentionally withheld. | ||
| if len(jwtRoles) == 0 { |
There was a problem hiding this comment.
[Major] This bootstrap only runs when the JWT has zero extracted roles. extractRealmRolesFromClaims (pkg/rbac/jwt_roles.go) reads the roles/groups/realm_access.roles claims, and standard Keycloak realm-role mappers emit built-in default roles that every user carries (default-roles-<realm>, offline_access, uma_authorization). If those land in the mapped claim, len(jwtRoles) == 0 is never true for real users and the default gateway:creator bootstrap never fires in production - defeating the purpose of the change and the RBAC_ENFORCE=true test-plan scenario.
Suggest gating on the recognized/synced role set instead (e.g. bootstrap when no JWTSyncedRoles are present), and verifying the concrete claim contents produced by the hypershell-frontend Keycloak mapper. Confidence: Medium (depends on deployed mapper config).
| // would grant capabilities that Keycloak intentionally withheld. | ||
| if len(jwtRoles) == 0 { | ||
| for _, r := range s.defaultRoles { | ||
| if roles.JWTSyncedRoles[r] { |
There was a problem hiding this comment.
[Minor/related] Because defaults are only re-added here (when len(jwtRoles) == 0), the revoke loop further down deletes a previously bootstrapped gateway:creator on the first sync where the JWT is non-empty - including when it only carries non-synced Keycloak default roles. With the current role set this is only harmful in edge cases, but the comment above and the PR body ("Default roles are never revoked by subsequent syncs") overstate the guarantee - please align the wording with the actual behavior.
There was a problem hiding this comment.
Agreed. if len(jwtRoles) == 0 is fragile.
the middleware should always reconcile the user's JWT, this includes apply default roles and when jwtRoles are defined in keycloak. the list of default roles and the list of keycloak roles may change.
| // configured default roles (e.g. gateway:creator) so that users with | ||
| // no Keycloak realm roles still receive their initial bindings. | ||
| if syncer != nil { | ||
| if syncErr := syncer.SyncJWTRoles(ctx, userID, jwtRoles); syncErr != nil { |
There was a problem hiding this comment.
[Minor] Moving SyncJWTRoles out of the len(jwtRoles) > 0 guard means every request from a zero-role user now performs a FindByUserID (plus role lookups) on the hot path. It is idempotent and consistent with the existing per-request sync for role-carrying users, so this is an observation - just confirm it is acceptable per-request.
Amber reviewStatus: Complete |
amber-review-bot
left a comment
amber-review-bot
left a comment
There was a problem hiding this comment.
Verdict
This PR grants every authenticated user a global gateway:creator binding by default, which is a real security-posture change that directly contradicts the current rbac-enforcement.spec.md model and is not accompanied by a spec update. The mechanism is sound and well-tested, but the documented opt-out does not actually work and the change inverts several documented "must be denied" guarantees, so it needs a maintainer/spec decision before merge.
Findings
[Blocker] Default gateway:creator for all users contradicts the RBAC spec (Security / Spec Consistency)
SyncJWTRoles now unconditionally merges defaultRoles (default gateway:creator) into every user's effective role set (service.go:114-118). This conflicts head-on with specs/security/rbac-enforcement.spec.md, which is authoritative:
- "Users start with zero permissions and gain access by receiving a Keycloak role (
gateway:creator)..." (Purpose section). - Design decision: "
gateway:creatorfrom Keycloak only - Cannot be self-assigned via the API. A Keycloak admin decides who can create gateways." - Scenario "User without creator role cannot create gateways" -> 403; Scenario "Platform admin cannot create gateways without creator role" -> 403. Both are now false.
- Scenario "Keycloak admin revokes gateway:creator" -> binding removed, user can no longer create. Because a default role is always re-merged before the revoke loop, a Keycloak revocation of
gateway:creatorcan never take effect - the binding is immediately re-added on the next request. This silently breaks documented revocation.
The spec is not updated in this PR. Per the project authority hierarchy (specs/ are project standards), the code and the spec must agree. Either update rbac-enforcement.spec.md to define this new "baseline role" model (and reconcile the four broken scenarios, especially revocation) or gate this behavior so the spec's default-deny posture is preserved. This is a maintainer decision, not a mechanical fix.
[Major] RBAC_DEFAULT_ROLES= cannot disable the default (Correctness / Config)
defaultRolesFromEnv() uses os.Getenv and treats empty string as unset (plugin.go:44-46), returning [gateway:creator]. The comment in service.go:111 documents RBAC_DEFAULT_ROLES= as the way to disable defaults, but an explicit empty value produces the same result as unset, so operators have no working opt-out. Given the Blocker above, this removes the only mitigation. Use os.LookupEnv to distinguish "unset" (apply default) from "set empty" (no defaults).
[Major] e2e assertions flipped from denied (403) to allowed (201) with no replacement guarantee (Test Diff Scrutiny)
e2e-openshell.sh rewrites two pre-existing negative assertions - developer (~L1356/L1381) and platform:admin (~L1510/L1534) - from "gateway create MUST be denied (403)" to "gateway create allowed (201)". These map directly to the spec scenarios above. This is a removed guarantee, not a test fixup: after this change there is no remaining test proving that a principal lacking the default role is denied creation. If the new model is accepted, add a companion assertion (e.g., an empty RBAC_DEFAULT_ROLES run) that still proves default-deny works, so RBAC enforcement itself remains covered.
[Minor] Python truthiness for OPENSHELL_GATEWAY_INSECURE (Correctness)
bool(os.environ.get('OPENSHELL_GATEWAY_INSECURE', '')) (e2e-openshell.sh:858, :1219) is truthy for any non-empty string, including "false"/"0". It works for the kind driver (which always sets true), but any driver setting OPENSHELL_GATEWAY_INSECURE=false would still be treated as insecure. Compare explicitly against ("1","true","yes").
[Minor] socat forwarder is never cleaned up (Test hygiene)
kind.sh:51 starts a backgrounded socat and records _KINDCCM_SOCAT_PID, but there is no trap/kill to tear it down. It leaves an orphaned 127.0.0.1 listener after the run. Add a cleanup trap that kills ${_KINDCCM_SOCAT_PID} on EXIT.
Cross-PR coordination
Another open pull request, #265 (managed cluster self-registration via OIDC client credentials), introduces the byte-for-byte identical default-role mechanism - the same defaultRolesFromEnv() in roleBindings/plugin.go, the same defaultRoles field and SyncJWTRoles merge in roleBindings/service.go, and the same testmain_test.go rbac-plugin wiring - and it also edits specs/security/rbac-enforcement.spec.md (adding a fifth managed-cluster-registrar role and requirement sections). This is a duplicate implementation of the same feature plus competing edits to the same spec, not incidental file overlap. Maintainers need to decide which PR owns the default-role change, define a merge order, and rebase the other onto it so the mechanism lands once; the RBAC-model wording in rbac-enforcement.spec.md (including the default-role/zero-permissions posture flagged in the Blocker above) must be reconciled in a single place rather than diverging between the two PRs.
Findings Summary (ordered by severity, highest first)
- [Blocker] Default
gateway:creatorfor all users contradictsrbac-enforcement.spec.md(zero-permission default, Keycloak-only creator, two 403 scenarios, and un-revocable default) with no spec update - Security / Spec Consistency (service.go L114-118) - [Major]
RBAC_DEFAULT_ROLES=cannot disable defaults; documented opt-out is a no-op (os.Getenvvsos.LookupEnv) - Config Correctness (plugin.go L44-46) - [Major] e2e negative RBAC assertions flipped 403 -> 201 with no replacement default-deny coverage - Test Diff Scrutiny (e2e-openshell.sh L1381, L1534)
- [Minor]
bool(os.environ.get(...))truthy for"false"- Correctness (e2e-openshell.sh L858, L1219) - [Minor] Backgrounded
socatforwarder has no cleanup trap - Test hygiene (kind.sh L51)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
| No secrets in logs or responses | Pass |
| Input validated | Pass |
| Reconcile (update-or-create) pattern | Pass |
| Conventional commit messages | Pass |
Behavior matches spec (rbac-enforcement.spec.md) |
Fail |
| Config change has working opt-out | Fail |
| Test changes preserve prior guarantees | Fail |
| if roles.JWTSyncedRoles[r] { | ||
| jwtRoleSet[r] = true | ||
| } | ||
| } |
There was a problem hiding this comment.
[Blocker] Contradicts rbac-enforcement.spec.md and breaks revocation. Default roles are always merged into jwtRoleSet here, before the revoke loop below (L161-168). Two spec-level problems:
- The spec states "Users start with zero permissions" and "
gateway:creatorfrom Keycloak only," plus scenarios asserting a non-creator and a bareplatform:adminare denied gateway creation (403). All authenticated users are now creators, so those scenarios no longer hold. - Because a default role is re-merged on every sync, the spec scenario "Keycloak admin revokes gateway:creator" can never take effect - the binding is immediately re-created after any revocation.
The spec is not updated in this PR. Please either update rbac-enforcement.spec.md to define this baseline-role model (and reconcile the revocation scenario) or gate this so the documented default-deny posture is preserved. This needs a maintainer/spec decision.
| func defaultRolesFromEnv() []string { | ||
| val := os.Getenv("RBAC_DEFAULT_ROLES") | ||
| if val == "" { | ||
| return []string{roles.RoleGatewayCreator} |
There was a problem hiding this comment.
[Major] RBAC_DEFAULT_ROLES= cannot disable defaults. os.Getenv returns "" for both "unset" and "explicitly empty," so setting RBAC_DEFAULT_ROLES= still returns [gateway:creator]. The comment in service.go documents RBAC_DEFAULT_ROLES= as the opt-out, but it is a no-op - operators have no working way to turn the default off. Use os.LookupEnv:
val, ok := os.LookupEnv("RBAC_DEFAULT_ROLES")
if !ok {
return []string{roles.RoleGatewayCreator}
}
// ok && val == "" -> no default roles| -H "Authorization: Bearer ${DEV_TOKEN}" &>/dev/null || true | ||
| fi | ||
| elif [[ "$DEV_GW_STATUS" == "403" ]]; then | ||
| fail_test "Developer user: gateway create blocked -- default gateway:creator binding was not assigned (HTTP 403)" |
There was a problem hiding this comment.
[Major] Flipped guarantee (Test Diff Scrutiny). This assertion changed from "developer create MUST be denied (403)" to "allowed (201)." It corresponds to the spec scenario "User without creator role cannot create gateways." After this change no test proves that a principal lacking the default role is denied creation, so RBAC enforcement itself is no longer covered end-to-end. If the new model is accepted, add a companion case (e.g., a run with an empty RBAC_DEFAULT_ROLES) that still verifies default-deny.
| 'oidc_issuer': os.environ['E2E_OIDC_ISSUER'], | ||
| 'oidc_client_id': os.environ['OIDC_CLIENT_ID_EFFECTIVE'] | ||
| 'oidc_client_id': os.environ['OIDC_CLIENT_ID_EFFECTIVE'], | ||
| 'gateway_insecure': bool(os.environ.get('OPENSHELL_GATEWAY_INSECURE', '')) |
There was a problem hiding this comment.
[Minor] bool(os.environ.get('OPENSHELL_GATEWAY_INSECURE', '')) is truthy for any non-empty string, including "false"/"0". It works for the kind driver (always true), but a driver setting OPENSHELL_GATEWAY_INSECURE=false would still be treated as insecure. Compare explicitly, e.g. os.environ.get('OPENSHELL_GATEWAY_INSECURE','').lower() in ('1','true','yes').
| socat_port=$(python3 -c "import socket; s=socket.socket(); s.bind(('127.0.0.1',0)); p=s.getsockname()[1]; s.close(); print(p)") | ||
| socat TCP4-LISTEN:"${socat_port}",bind=127.0.0.1,reuseaddr,fork \ | ||
| TCP4:127.0.0.1:"${raw_port}" &>/dev/null & | ||
| _KINDCCM_SOCAT_PID=$! |
There was a problem hiding this comment.
[Minor] The backgrounded socat forwarder is recorded in _KINDCCM_SOCAT_PID but never torn down - no trap/kill. It leaves an orphaned 127.0.0.1 listener after the e2e run. Add a cleanup trap that kills ${_KINDCCM_SOCAT_PID} on EXIT.
…r provisioning All authenticated users now receive a gateway:creator role binding on their first request, so RBAC_ENFORCE=true does not block new users from creating or listing gateways. SyncJWTRoles merges a configurable RBAC_DEFAULT_ROLES list (defaults to gateway:creator) into the effective JWT role set before reconciling bindings. Default roles are never revoked by subsequent syncs and cannot be duplicated. Roles must be in JWTSyncedRoles to participate in the sync lifecycle. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… are assigned SyncJWTRoles was guarded by `if len(jwtRoles) > 0` at both the HTTP and gRPC call sites, so users whose JWT carried no Keycloak realm roles never had the call made and therefore never received the configured default role bindings (e.g. gateway:creator). Remove the guard at both sites; the context-key assignment (needed by downstream middleware) remains gated on non-empty roles. SyncJWTRoles now runs unconditionally so the default-roles path is exercised on every provisioned user. Also adds a startup warning when an RBAC_DEFAULT_ROLES entry is not in JWTSyncedRoles, and fixes integration tests that relied on user ID being absent from context (which masked missing gateway:owner preconditions). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ole JWT users The previous change applied RBAC_DEFAULT_ROLES to every authenticated user on every request. This conflicts with deployments where Keycloak assigns non-creator realm roles (e.g. openshell-user) to users who should be denied gateway:creator: SyncJWTRoles was silently overriding that intent by appending the default regardless of JWT content. Change the default-role logic so that defaults only apply when the JWT carries NO realm roles at all. Users whose Keycloak token already contains role assignments -- even if none are JWT-synced roles -- are managed entirely through Keycloak and receive no default override. This preserves the bootstrap use-case (brand-new users with no Keycloak assignments get gateway:creator so RBAC_ENFORCE=true does not strand them) while restoring the expected E2E posture where Keycloak-role-bearing users without explicit gateway:creator remain blocked. Also fixes gofmt and renames TestSyncJWTRoles_JWTRoleAddedAlongsideDefault to TestSyncJWTRoles_JWTRolesPreventDefaultAssignment to reflect the corrected behavior. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…JWT role cardinality Remove the `if len(jwtRoles) == 0` guard from SyncJWTRoles so that RBAC_DEFAULT_ROLES are unconditionally merged with whatever the JWT carries. Prior behavior bootstrapped defaults only for users with zero JWT realm roles. This was fragile: an admin who already held platform:admin in Keycloak would never receive gateway:creator via the default binding, forcing manual setup for every new deployment. The new behavior mirrors the intent in user_provisioning.go (which already calls SyncJWTRoles unconditionally): default roles represent the platform's baseline posture and must be present regardless of what other roles the JWT brings. The Keycloak role list and the default-role list may change independently; each request reconciles both. Updates the integration test to assert that a JWT-carried platform:admin binding and the default gateway:creator binding are both present after a single sync. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ve default gateway:creator E2E assertions (areas 9, 10): - Developer user: gateway create returns 201 (gateway:creator from RBAC_DEFAULT_ROLES) - Platform admin: gateway create returns 201 (gateway:creator from RBAC_DEFAULT_ROLES) Kind driver fixes required to reach these areas locally: - Discover ephemeral kindccm-gw port and front it with an IPv4-only socat forwarder. Docker's IPv6 NAT for the ephemeral port is unreliable (TLS RST); the openshell CLI (Rust/hyper) prefers IPv6 and does not fall back after a TCP-level RST during the TLS handshake. Binding socat to 127.0.0.1 ensures ECONNREFUSED on ::1 so hyper retries on 127.0.0.1. - Return the socat port as the gateway endpoint in discover_gateway_endpoint so the openshell CLI connects on IPv4. - Export OPENSHELL_GATEWAY_INSECURE=true: Kind uses self-signed certs not in the system trust store; curl already uses -sk for all driver calls. - Set gateway_insecure: true in openshell metadata.json for per-gateway and developer configs, driven by the OPENSHELL_GATEWAY_INSECURE env var. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…oint only The socat IPv4-only forwarder must not replace _KINDCCM_PORT -- that port is used by _driver_curl for all curl requests (API server, Keycloak, console) which already use --ipv4 and are unaffected by IPv6. Replacing _KINDCCM_PORT broke CI runners that don't have socat: curl would connect to a dead port and get "000" responses, failing API host discovery before any test ran. Split into two variables: - _KINDCCM_PORT: raw kindccm-gw ephemeral port, set once by _kind_discover_port. Used by _driver_curl via --connect-to (curl handles IPv6 via --ipv4). - _KINDCCM_GW_PORT: set by _kind_start_gw_socat, used only for the gateway endpoint URL written into openshell CLI metadata.json. socat is started lazily when discover_gateway_endpoint is called, only if socat is available. Falls back to the raw port if socat is absent (IPv6 may fail for the CLI but all other test areas complete). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
eccbba1 to
5d9ea17
Compare
Amber reviewStatus: Complete |
amber-review-bot
left a comment
amber-review-bot
left a comment
There was a problem hiding this comment.
Verdict
REQUEST_CHANGES. This PR flips the platform's default authorization posture so every authenticated principal receives gateway:creator, which contradicts the authoritative specs/security/rbac-enforcement.spec.md and is not accompanied by a spec update. The implementation and its own tests are clean and idempotent, but the design reverses documented security scenarios and needs a spec change plus maintainer sign-off before merge.
Amber Analysis
The mechanics are well built: SyncJWTRoles merging is idempotent, the existingSynced dedup prevents duplicates, defaults are never revoked, error wrapping uses %w, no panic(), and the new config knob (RBAC_DEFAULT_ROLES) keeps configuration out of code. My concern is not the code quality - it is that the default behavior changes an authoritative security contract.
Blocker
1. Default gateway:creator for all users contradicts the RBAC spec and defeats revocation - Spec Consistency / Security (service.go L109-118, plugin.go L43-55)
specs/security/rbac-enforcement.spec.md is authoritative and states the opposite of what this PR implements:
- "Users start with zero permissions and gain access by receiving a Keycloak role (
gateway:creator)" (Purpose). - Design Decision: "
gateway:creatorfrom Keycloak only - Cannot be self-assigned via the API. A Keycloak admin decides who can create gateways." - Scenario "User without creator role cannot create gateways" requires 403.
- Scenario "Platform admin cannot create gateways without creator role" requires 403.
Making gateway:creator a baseline for everyone turns the creator gate into a no-op in the default configuration (the env var defaults to gateway:creator, so this is on even with no config change). Worse, the "always merge regardless of what the JWT carries" semantics break the spec scenario "Keycloak admin revokes gateway:creator" (spec expects the binding to be removed and the user to lose create ability): because the default is re-applied on every sync, a Keycloak revocation can never take effect for a user who otherwise has no synced roles. The PR's own TestSyncJWTRoles_DefaultRoleNotRemovedWhenAbsentFromJWT cements this now-non-compliant behavior.
Required: update specs/security/rbac-enforcement.spec.md first (specs are the desired state and rank above task instructions in the authority hierarchy) and get maintainer agreement on the posture change, or narrow the default so it does not override Keycloak-driven revocation. If a default-creator posture is intended, the spec's Purpose, the two 403 scenarios, the revoke scenario, and the "from Keycloak only" design decision all need to be reconciled.
Major
2. e2e negative RBAC assertions flipped from 403 to 201, deleting the only end-to-end proof of the creator gate - Test Diff Scrutiny (tests/e2e/e2e-openshell.sh L1356-1401, L1510-1554)
Two e2e blocks previously asserted that a developer and a platform-admin without gateway:creator are denied gateway creation (403), explicitly citing the rbac-enforcement scenario "User without creator role cannot create gateways." Both are rewritten to expect 201. After this change there is no e2e coverage anywhere that a non-creator is ever denied gateway creation, because the suite now assumes everyone is a creator. This is a rejects->accepts assertion flip that removes a guarantee. If the posture change in finding 1 is accepted, add companion coverage that still proves the creator gate denies a principal when RBAC_DEFAULT_ROLES= (empty) so the enforcement path stays tested.
Minor
3. RBAC_DEFAULT_ROLES is on-by-default but not surfaced in deploy overlays or docs - Config / Docs (plugin.go L43-55)
The knob defaults to gateway:creator, so the posture change ships silently even where operators set nothing. It is not added to deploy/openshift / deploy/kind kustomizations or documented in the spec, so an operator cannot see from configuration that every user becomes a creator. Surface it explicitly (overlay + spec) so the behavior is discoverable and opt-out is obvious.
4. Misconfigured default roles are silently dropped (warn-only) - Observability (service.go L40-45)
A default role not in JWTSyncedRoles is logged at warning level and then ignored, so an operator who sets RBAC_DEFAULT_ROLES=some:role gets no binding and only a log line. Consider failing fast at startup (construction) for an unknown/non-sync-eligible default role rather than degrading quietly at request time.
5. Broadened HTTP test surface via new rbac plugin registration - Testing (informational) (testmain_test.go L12-15, integration_test.go L63-77, L118-133)
Registering the rbac plugin in testmain now runs UserProvisioningMiddleware for all HTTP-path tests, which is why TestRoleBindingCreate_GatewayOwner and TestRoleBindingDelete had to start provisioning an owner binding. These edits are justified by the new wiring (not a hidden contract weakening), but note that every HTTP integration test in this package now exercises provisioning + default-role assignment as a side effect.
Cross-PR coordination
Coordinate with #265 before either merges. Both PRs introduce the identical RBAC_DEFAULT_ROLES feature and the identical NewRoleBindingService(..., defaultRoles []string) constructor change, but they implement conflicting SyncJWTRoles semantics: this PR applies the defaults unconditionally on every sync ("always merge regardless of what the JWT carries"), while the other applies them only when the JWT carries zero roles (if len(jwtRoles) == 0). These are mutually exclusive behaviors for the same function and cannot both land. The other PR also updates specs/security/rbac-enforcement.spec.md (adding a fifth built-in role) whereas this PR leaves the spec untouched. Maintainers must decide which default-role semantics is correct, agree on the single shared constructor/spec change, and fix a merge order so the second PR rebases onto the chosen design instead of re-introducing a competing one.
Findings Summary (ordered by severity, highest first)
- [Blocker] Default
gateway:creatorfor all users contradicts rbac-enforcement.spec.md and defeats Keycloak revocation - Spec Consistency / Security (service.go L109-118, plugin.go L43-55) - [Major] e2e RBAC assertions flipped 403->201, removing the only end-to-end proof of the creator gate - Test Diff Scrutiny (e2e-openshell.sh L1356, L1510)
- [Minor]
RBAC_DEFAULT_ROLESon-by-default but absent from overlays/spec docs - Config / Docs (plugin.go L43) - [Minor] Misconfigured default role silently dropped (warn-only) - Observability (service.go L40)
- [Minor] Broadened HTTP test surface via new rbac plugin registration - Testing (testmain_test.go L12)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
| No secrets in logs or responses | Pass |
| Input validated | Pass |
| Reconcile / idempotent pattern (no duplicate bindings) | Pass |
| Config separated from code | Pass |
| Implementation matches authoritative spec | Fail |
| Test Diff Scrutiny (no unjustified assertion flips) | Fail |
| Conventional commit messages | Pass |
| // principal receives these capabilities unless explicitly disabled via | ||
| // RBAC_DEFAULT_ROLES=. Only roles in JWTSyncedRoles participate in the | ||
| // sync lifecycle (idempotent add, never revoked by JWT absence). | ||
| for _, r := range s.defaultRoles { |
There was a problem hiding this comment.
[Blocker] Always-merge defeats Keycloak revocation and contradicts the RBAC spec.
Because the default roles are re-applied on every sync regardless of JWT contents, the spec scenario "Keycloak admin revokes gateway:creator" (which requires the binding to be removed and the user to lose create ability) can never take effect for a user with no other synced roles: the next sync re-adds gateway:creator. specs/security/rbac-enforcement.spec.md states "Users start with zero permissions" and "gateway:creator from Keycloak only."
Either update the authoritative spec (Purpose, the two 403 scenarios, and the revoke scenario) with maintainer sign-off, or gate the merge so it cannot override an explicit Keycloak revocation. Note this is exactly the point where a sibling PR takes a different, incompatible approach (defaults only when len(jwtRoles) == 0) - the two designs must be reconciled first.
| func defaultRolesFromEnv() []string { | ||
| val := os.Getenv("RBAC_DEFAULT_ROLES") | ||
| if val == "" { | ||
| return []string{roles.RoleGatewayCreator} |
There was a problem hiding this comment.
[Blocker/Config] Default is gateway:creator, so the posture change is on even with no configuration. Every authenticated user becomes a gateway creator out of the box, turning the creator gate into a no-op by default. This is not reflected in deploy/openshift/deploy/kind overlays or in the spec, so operators cannot see the behavior from configuration. Please surface the knob in the overlays and document the opt-out (RBAC_DEFAULT_ROLES=) alongside the spec update.
| defaultRoles []string, | ||
| ) RoleBindingService { | ||
| for _, r := range defaultRoles { | ||
| if !roles.JWTSyncedRoles[r] { |
There was a problem hiding this comment.
[Minor] Silent degradation on misconfiguration. A default role that is not in JWTSyncedRoles is logged at warning level and then ignored, so RBAC_DEFAULT_ROLES=some:role yields no binding and only a log line. Prefer failing fast at construction/startup for an unknown or non-sync-eligible default role.
| ") | ||
| show_cmd "curl -X POST ${API_HOST}/api/hypershell/v1/gateways (as developer) -> expect 403" | ||
| dim " Expecting 403 Forbidden (developer lacks gateway:creator)..." | ||
| show_cmd "curl -X POST ${API_HOST}/api/hypershell/v1/gateways (as developer) -> expect 201 (gateway:creator by default)" |
There was a problem hiding this comment.
[Major] Assertion flipped from 403 to 201 (Test Diff Scrutiny). This block previously proved the rbac-enforcement scenario "User without creator role cannot create gateways." It now expects success. After this change no e2e test proves a non-creator is ever denied gateway creation. If the default-creator posture is accepted, add companion coverage that still exercises a denial with RBAC_DEFAULT_ROLES= (empty) so the enforcement path stays tested.
| ") | ||
| show_cmd "curl -X POST ${API_HOST}/api/hypershell/v1/gateways (as platform admin) -> expect 403" | ||
| dim " Expecting 403 Forbidden (platform:admin lacks gateway:creator)..." | ||
| show_cmd "curl -X POST ${API_HOST}/api/hypershell/v1/gateways (as platform admin) -> expect 201 (gateway:creator by default)" |
There was a problem hiding this comment.
[Major] Assertion flipped from 403 to 201 (Test Diff Scrutiny). Same concern as the developer block: this removed the negative proof for "Platform admin cannot create gateways without creator role." Retain a denial case under an empty RBAC_DEFAULT_ROLES so the creator gate remains verified end-to-end.
| // Always sync even when jwtRoles is empty: SyncJWTRoles applies | ||
| // configured default roles (e.g. gateway:creator) so that users with | ||
| // no Keycloak realm roles still receive their initial bindings. | ||
| if syncer != nil { |
There was a problem hiding this comment.
[Context] Always-sync even for empty JWT roles. This is a reasonable enabler for default-role assignment, but combined with the always-merge in service.go it means an empty (or revoked) JWT still results in gateway:creator. Please confirm this interacts correctly with the spec's revoke scenario once the posture question is settled.
…default-role model - Fix defaultRolesFromEnv to use os.LookupEnv so that RBAC_DEFAULT_ROLES= (explicit empty string) actually disables the default gateway:creator grant. Previously os.Getenv could not distinguish unset from empty, so operators had no working way to opt out of the default. - Update specs/security/rbac-enforcement.spec.md to document the default-role model: RBAC_DEFAULT_ROLES variable semantics, the always-merge behavior, the Keycloak-only opt-out via RBAC_DEFAULT_ROLES=, and the revocation caveat. Scenarios that previously described the zero-default posture are annotated with the RBAC_DEFAULT_ROLES= precondition so they remain accurate under the new default. Adds a new "Default Role Bootstrap" requirement section with scenarios. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Amber reviewStatus: Complete |
amber-review-bot
left a comment
amber-review-bot
left a comment
There was a problem hiding this comment.
Verdict
The default-role bootstrap is cleanly implemented, well-tested, and the spec is updated in step with the code (opt-out via RBAC_DEFAULT_ROLES=, idempotent non-revoking merge). Two items need a maintainer decision rather than a code fix: this PR changes the platform's default security posture (every authenticated user becomes a gateway creator) and it overlaps a competing implementation of the same feature in another open PR.
I reviewed against CLAUDE.md, security.spec.md, and control-plane/conventions.spec.md. Code quality is good: errors wrapped with %w, no panic(), config kept in env, update-or-create semantics preserved.
Key points
-
Default security posture change (Major, High confidence). With
RBAC_DEFAULT_ROLESdefaulting togateway:creator, every authenticated principal can create gateways on every request, andgateway:creatorcan no longer be revoked via Keycloak in the default configuration. This effectively means enablingRBAC_ENFORCE=truedoes not gate gateway creation unless an operator also setsRBAC_DEFAULT_ROLES=. This is deliberate and thoroughly documented inrbac-enforcement.spec.md, but because it flips the platform default from "deny" to "allow" for a security-relevant capability, it warrants explicit maintainer/security sign-off. -
Removed negative e2e coverage (Major, Medium confidence). The e2e assertions for "developer cannot create a gateway" and "platform:admin cannot create a gateway" were flipped from
403to201. That is consistent with the new default, but it deletes the only end-to-end guarantee that the Keycloak-only mode (RBAC_DEFAULT_ROLES=) still denies non-creators. The opt-out denial path now has no e2e (and the integration tests only cover defaults-on). Consider adding coverage for theRBAC_DEFAULT_ROLES=deny path so the enforcement contract remains verified. -
Empty-JWT now triggers sync (Minor, Medium confidence).
SyncJWTRolesis now called even when the JWT carries no roles. Because onlygateway:creatoris merged as a default, a request whose token omitsrealm_access.roleswill now revoke a previously synced globalplatform:adminbinding (it is not a default and is absent fromjwtRoleSet), where the old code skipped sync entirely for empty tokens. This is arguably more consistent, but it means a transient roles-less token can flap an admin binding. Please confirm this is intended. -
Leaked socat process in e2e (Minor).
_KINDCCM_SOCAT_PIDis started intests/e2e/drivers/kind.shbut never terminated by thecleanupEXIT trap ine2e-openshell.sh, leaking a background forwarder after a run.
Cross-PR coordination
Another open pull request, #265, ships a second, divergent implementation of this same RBAC_DEFAULT_ROLES default-role feature, touching the same functions (SyncJWTRoles, NewRoleBindingService, defaultRolesFromEnv, and the always-sync change in the provisioning middleware and gRPC interceptor) and the same specs/security/rbac-enforcement.spec.md. The two implementations encode incompatible design decisions:
- This PR (#263) merges default roles unconditionally on every request and adds an explicit opt-out (
os.LookupEnv, empty string disables defaults), so Keycloak cannot revokegateway:creator. - #265 applies defaults only when the JWT carries zero roles (
if len(jwtRoles) == 0) and usesos.Getenvwith no empty-string opt-out, so Keycloak-managed users never receive defaults.
Maintainers must decide which default-role semantics is authoritative and ensure only one implementation lands; the other PR must drop or rebase its version onto that decision. Because both edit the same functions and the same spec sections, merge order must be coordinated (this affects #265, which also bundles the managed-cluster self-registration spec and a fifth managed-cluster-registrar role on top of the divergent default-role code).
Findings Summary (ordered by severity, highest first)
- [Major] Default posture flips to allow: all authenticated users get
gateway:creator;RBAC_ENFORCE=trueno longer gates gateway creation unlessRBAC_DEFAULT_ROLES=is set - needs explicit sign-off - Security - [Major] Negative RBAC e2e assertions flipped 403 -> 201; deny-path coverage for
RBAC_DEFAULT_ROLES=(Keycloak-only mode) removed, not preserved - Test Diff Scrutiny / Coverage - [Minor]
SyncJWTRolesnow runs on empty JWTs and can revoke a syncedplatform:adminbinding that previously persisted - Behavior Change - [Minor]
_KINDCCM_SOCAT_PIDbackground forwarder not killed in the e2e cleanup trap - Resource Leak
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
| No secrets in logs or responses | Pass |
| Reconcile / update-or-create pattern | Pass |
| Configuration separated from code (env var) | Pass |
| Spec updated alongside behavior change | Pass |
| Modified test assertions individually justified | Review (negative e2e coverage replaced, not backfilled) |
| Conventional commit messages | Pass |
| // principal receives these capabilities unless explicitly disabled via | ||
| // RBAC_DEFAULT_ROLES=. Only roles in JWTSyncedRoles participate in the | ||
| // sync lifecycle (idempotent add, never revoked by JWT absence). | ||
| for _, r := range s.defaultRoles { |
There was a problem hiding this comment.
Default roles are merged unconditionally here, so gateway:creator can never be revoked via Keycloak in the default configuration. Separately, because SyncJWTRoles is now invoked even for empty JWTs (see the middleware/interceptor change), a token that omits realm_access.roles will fall through to the revocation loop below and delete a previously synced global platform:admin binding (it is not a default and so is absent from jwtRoleSet). Previously an empty token skipped sync entirely. Please confirm the revocation-on-empty-token behavior is intended.
| // Defaults to gateway:creator so all authenticated users can create gateways. | ||
| // Set RBAC_DEFAULT_ROLES= (explicit empty) to disable defaults entirely. | ||
| func defaultRolesFromEnv() []string { | ||
| val, set := os.LookupEnv("RBAC_DEFAULT_ROLES") |
There was a problem hiding this comment.
Using os.LookupEnv to distinguish unset from explicit-empty is the right call for the opt-out (RBAC_DEFAULT_ROLES=). Note that another open PR implements this same helper with os.Getenv and only-when-JWT-is-empty semantics; the two default-role designs are incompatible and need a single authoritative decision (see the Cross-PR coordination section).
| # POST /gateways MUST be rejected with 403 (rbac-enforcement.spec.md scenario | ||
| # "User without creator role cannot create gateways"). SUCCESS here would mean | ||
| # RBAC is NOT enforced. | ||
| # ── positive assertion: authenticated user receives gateway:creator by default ── |
There was a problem hiding this comment.
This flips the negative RBAC assertion (developer/platform:admin create -> 403) to a positive one (-> 201). That matches the new default, but it removes the only e2e guarantee that non-creators are denied. There is no remaining e2e (or integration) coverage for the Keycloak-only mode (RBAC_DEFAULT_ROLES=) still returning 403. Consider adding a deny-path case for the opt-out configuration so the enforcement contract stays verified.
| fi | ||
| socat TCP4-LISTEN:"${socat_port}",bind=127.0.0.1,reuseaddr,fork \ | ||
| TCP4:127.0.0.1:"${raw_port}" &>/dev/null & | ||
| _KINDCCM_SOCAT_PID=$! |
There was a problem hiding this comment.
_KINDCCM_SOCAT_PID is captured here but never terminated by the cleanup EXIT trap in e2e-openshell.sh, so this background socat forwarder leaks after each run. Add a kill/wait for it to the trap (alongside the other PIDs it already cleans up).
Status updateAll code-level issues from prior amber reviews are resolved on HEAD (
The latest amber review (
Ready for maintainer review and merge decision. |
… merged PR #263 PR #263 (HYPERSHELL-262) established that: - Only platform:admin and gateway:creator are in JWTSyncedRoles - gateway:creator is applied to all users by default via RBAC_DEFAULT_ROLES - isAuthorized falls through to hasGatewayCreator for unhandled resources This meant the managed-cluster-registrar spec was wrong in two ways: 1. Enforcement claim: said "no new RBAC machinery needed" but the hasGatewayCreator fallback would allow ALL users to call /registration 2. Isolation claim: "grants no gateway access" is false when RBAC_DEFAULT_ROLES=gateway:creator (the default) Fix: - managed-cluster-registrar is now specified as JWT-direct (checked live from JWT claim in isAuthorized), consistent with HypershellAdminRole - isAuthorized needs a dedicated case for POST managed_clusters/registration - Not added to JWTSyncedRoles; no DB RoleBinding lifecycle - Seeded as a built-in role for discoverability only - Isolation scenario scoped to RBAC_DEFAULT_ROLES= (Keycloak-only mode) - Note added that default config gives spokes gateway:creator too - Also fixes gofmt indentation introduced by leftover HYPERSHELL-262 commit Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… credentials (openshift-online#265) * [HYPERSHELL-262] fix(rbac): always sync JWT roles so default bindings are assigned SyncJWTRoles was guarded by `if len(jwtRoles) > 0` at both the HTTP and gRPC call sites, so users whose JWT carried no Keycloak realm roles never had the call made and therefore never received the configured default role bindings (e.g. gateway:creator). Remove the guard at both sites; the context-key assignment (needed by downstream middleware) remains gated on non-empty roles. SyncJWTRoles now runs unconditionally so the default-roles path is exercised on every provisioned user. Also adds a startup warning when an RBAC_DEFAULT_ROLES entry is not in JWTSyncedRoles, and fixes integration tests that relied on user ID being absent from context (which masked missing gateway:owner preconditions). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * feat(spec): managed cluster self-registration via OIDC client credentials HYPERSHELL-326 Add spec for spoke control-planes to self-register using existing OIDC client_credentials, eliminating manual cluster_id distribution from gitops. - New spec: managed-cluster-registration.spec.md -- /registration sub-resource within managedClusters plugin; idempotent on (oidc_subject, name); updates last_seen_at on every call, serving as both registration and heartbeat loop - data-model: add oidc_subject and last_seen_at to ManagedCluster entity; add /managed_clusters/registration to API reference - rbac-enforcement: add managed-cluster-registrar role (Keycloak JWT, global scope, no gateway permissions); add requirement with grant/deny scenarios - control-plane: add spoke startup self-registration requirement; /registration called before WatchGateways, then looped every 60s for last_seen_at updates - index: register new spec in the spec registry Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * [HYPERSHELL-326] fix(spec): align managed-cluster-registrar RBAC with merged PR openshift-online#263 PR openshift-online#263 (HYPERSHELL-262) established that: - Only platform:admin and gateway:creator are in JWTSyncedRoles - gateway:creator is applied to all users by default via RBAC_DEFAULT_ROLES - isAuthorized falls through to hasGatewayCreator for unhandled resources This meant the managed-cluster-registrar spec was wrong in two ways: 1. Enforcement claim: said "no new RBAC machinery needed" but the hasGatewayCreator fallback would allow ALL users to call /registration 2. Isolation claim: "grants no gateway access" is false when RBAC_DEFAULT_ROLES=gateway:creator (the default) Fix: - managed-cluster-registrar is now specified as JWT-direct (checked live from JWT claim in isAuthorized), consistent with HypershellAdminRole - isAuthorized needs a dedicated case for POST managed_clusters/registration - Not added to JWTSyncedRoles; no DB RoleBinding lifecycle - Seeded as a built-in role for discoverability only - Isolation scenario scoped to RBAC_DEFAULT_ROLES= (Keycloak-only mode) - Note added that default config gives spokes gateway:creator too - Also fixes gofmt indentation introduced by leftover HYPERSHELL-262 commit Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * [HYPERSHELL-326] fix(spec): establish Keycloak-only as the production RBAC posture The expectation is full RBAC enforcement where Keycloak is the sole authority for all permissions -- gateway creation, platform admin access, and managed cluster self-registration. No role is auto-assigned in production. Changes: - Production posture is now authoritative: RBAC_ENFORCE=true + RBAC_DEFAULT_ROLES= - RBAC_DEFAULT_ROLES=gateway:creator is explicitly labeled as a local-dev convenience only; SHALL NOT appear in production or staging overlays - All "Keycloak-only mode" scenario caveats removed -- that IS the mode - managed-cluster-registrar isolation is now a guarantee, not a footnote - Operator Note rewritten: produce the Keycloak setup steps required before enabling enforcement (gateway:creator, platform:admin, managed-cluster-registrar) - Design decisions updated to reflect Keycloak-exclusive role assignment Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * [HYPERSHELL-326] fix(spec): address amber review findings Three issues identified in amber review of PR openshift-online#265: 1. Path consistency: standardize all endpoint references to use underscore form (/managed_clusters/registration) matching every other route in data-model.spec.md and the existing API convention. 2. Response shape: document explicitly that POST /registration returns only { "cluster_id" } (not the full ManagedCluster object). The spoke needs exactly one field; the narrow shape is intentional and differs from GET /managed_clusters/{id} by design. 3. Startup-failure behavior: replace the ambiguous "exit or retry per operator configuration" with a concrete split: - 403 Forbidden: exit immediately with a clear error (retrying is pointless without a Keycloak role change) - Transient errors (network, 5xx): retry with exponential backoff Same semantics applied consistently in both control-plane.spec.md and managed-cluster-registration.spec.md. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * [HYPERSHELL-326] feat(registration): implement managed cluster self-registration Add the full stack for spoke self-registration via POST /managed_clusters/registration: API Server (Wave 2 OpenAPI + Wave 3 SDK): - POST /api/hypershell/v1/managed_clusters/registration endpoint - ManagedClusterRegistrationRequest/Response schemas - ManagedCluster gains oidc_subject and last_seen_at fields - SDK regenerated with RegisterManagedCluster operation API Server Backend (Wave 4): - ManagedCluster model: OIDCSubject string, LastSeenAt *time.Time - Migration: add oidc_subject + last_seen_at columns; partial unique index on (oidc_subject, name) WHERE oidc_subject IS NOT NULL AND oidc_subject <> '' - DAO: FindByOIDCSubject for upsert lookup - Service: Register() upserts on (oidc_subject, name); returns 201 on create, 200 on heartbeat; advisory-locks on oidc_subject to prevent duplicate creates - Handler: Register() extracts sub claim from JWT; custom handler writes 201/200 - Plugin: /registration route registered before /{id} to prevent mux capture - Presenter: PresentRegistrationResponse; PresentManagedCluster includes new fields - RBAC: hasManagedClusterRegistrar(); isAuthorized() dedicated case for resource "registration" + POST - JWT-direct, bypasses hasGatewayCreator fallback - Roles: seed managed-cluster-registrar built-in role (DB record for discoverability; not in JWTSyncedRoles, no DB binding lifecycle) Control Plane (Wave 6): - Config: ManagedClusterName from HYPERSHELL_MANAGED_CLUSTER_NAME - internal/registration: HTTP client; ErrForbidden sentinel for fail-closed - main.go: registerWithBackoff() - 403 exits immediately, other errors retry with exponential backoff; ClusterID resolved at runtime from registration; 60s heartbeat goroutine updates last_seen_at continuously Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * [HYPERSHELL-326] chore(sdk): regenerate TypeScript and Go SDKs for registration endpoint Adds ManagedCluster.oidc_subject, ManagedCluster.last_seen_at, and the RegisterManagedCluster operation to both the TypeScript and Go SDKs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * [HYPERSHELL-326] fix(migration): add explicit GORM column tags for OIDCSubject and LastSeenAt GORM auto-naming converts OIDCSubject -> o_id_c_subject (a word-per-capital expansion). Add column: tags to force the correct snake_case column names that the migration already creates (oidc_subject, last_seen_at). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * [HYPERSHELL-262] fix(registration): address amber review findings - Remove description-to-Status mapping (blocker: Status is reconciler-owned; description supplied at registration was silently corrupting that field) - Move registration RBAC check before userID gate so transient user-provisioning DB failures return a retryable error, not a fatal 403 that causes the spoke to exit (critical: ErrForbidden was non-retryable) - Promote managed-cluster-registrar string to const roleManagedClusterRegistrar in rbac package to avoid magic literal drift - Use errors.Is(err, gorm.ErrRecordNotFound) instead of direct pointer compare - Add context parameter and 30s timeout to registration.Client.Register so in-flight HTTP calls are cancelled on shutdown and never hang indefinitely - Escalate heartbeat log from WARN to ERROR after 5 consecutive failures - Implement Replace in managedClusterDaoMock (was NotImplemented) - Add 5 RBAC unit tests covering registration path: role allow/deny, method restriction, and the userID-gate bypass regression guard Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * [HYPERSHELL-262] fix(registration): DNS-label validation and overlay isolation - Validate registration name as K8s DNS label (RFC 1123) in handler before upsert: lowercase alphanumeric + hyphens, start/end alphanumeric, max 63 chars; reject with 400 MalformedRequest on invalid input (amber Finding 4) - Add RBAC_DEFAULT_ROLES= (explicit empty) to the OpenShift overlay so spoke service accounts holding only managed-cluster-registrar receive no gateway permissions; without this the default gateway:creator grant contradicts the isolation guarantee stated in rbac-enforcement.spec.md (amber Finding 2) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * [HYPERSHELL-262] fix(web-console): add last_seen_at and oidc_subject to ManagedCluster test fixtures The SDK generator emits all ManagedCluster properties as required. The new fields added by the self-registration feature were missing from the test fixtures causing TS2322 type errors in the web console quality gate. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: user <u@example.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
2 participants
Summary
SyncJWTRolesnow merges a configurableRBAC_DEFAULT_ROLESlist (env var, defaults togateway:creator) into the effective JWT role set before reconciling bindingsgateway:creatorglobal binding on first request, so flippingRBAC_ENFORCE=truedoes not strand new users who have no Keycloak roles yetexistingSynceddedup loop handles idempotency)JWTSyncedRolesto participate in the sync lifecycle;gateway:creatoralready isWhat changed
plugins/roleBindings/service.goNewRoleBindingServiceacceptsdefaultRoles []string;SyncJWTRolesmerges them intojwtRoleSetplugins/roleBindings/plugin.godefaultRolesFromEnv()readsRBAC_DEFAULT_ROLES, defaults togateway:creator; passed to service at constructionplugins/roleBindings/integration_test.goTest plan
go build ./...- cleango vet ./...- cleanmake lint-api-server- 0 issuesRBAC_ENFORCE=trueand verify new users can create gateways on first loginJira
https://redhat.atlassian.net/browse/HYPERSHELL-262
🤖 Generated with Claude Code