Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions components/api-server/pkg/rbac/grpc_interceptor.go
Original file line number Diff line number Diff line change
Expand Up @@ -198,13 +198,16 @@ func provisionUserForGRPC(ctx context.Context, provisioner UserProvisioner, sync

ctx = context.WithValue(ctx, ContextUserIDKey, userID)

jwtRoles := extractJWTRolesFromContext(ctx)
if len(jwtRoles) > 0 {
ctx = context.WithValue(ctx, ContextJWTRolesKey, jwtRoles)
}
// 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 {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[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.

jwtRoles := extractJWTRolesFromContext(ctx)
if len(jwtRoles) > 0 {
ctx = context.WithValue(ctx, ContextJWTRolesKey, jwtRoles)
if syncErr := syncer.SyncJWTRoles(ctx, userID, jwtRoles); syncErr != nil {
glog.Warningf("gRPC JWT role sync failed for %q: %v", username, syncErr)
}
if syncErr := syncer.SyncJWTRoles(ctx, userID, jwtRoles); syncErr != nil {
glog.Warningf("gRPC JWT role sync failed for %q: %v", username, syncErr)
}
}

Expand Down
11 changes: 7 additions & 4 deletions components/api-server/pkg/rbac/user_provisioning.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,13 @@ func UserProvisioningMiddleware(provisioner UserProvisioner, syncer JWTRoleSynce
jwtRoles := extractJWTRoles(r)
if len(jwtRoles) > 0 {
ctx = context.WithValue(ctx, ContextJWTRolesKey, jwtRoles)
if syncer != nil {
if syncErr := syncer.SyncJWTRoles(ctx, userID, jwtRoles); syncErr != nil {
glog.Warningf("JWT role sync failed for %q: %v", payload.Username, syncErr)
}
}
// 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 {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[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.

if syncErr := syncer.SyncJWTRoles(ctx, userID, jwtRoles); syncErr != nil {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[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.

glog.Warningf("JWT role sync failed for %q: %v", payload.Username, syncErr)
}
}

Expand Down
167 changes: 165 additions & 2 deletions components/api-server/plugins/roleBindings/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package roleBindings_test
import (
"context"
"net/http"
"strings"
"testing"

. "github.com/onsi/gomega"
Expand Down Expand Up @@ -59,12 +60,20 @@ func TestRoleBindingCreate_GatewayOwner(t *testing.T) {
account := h.NewRandAccount()
ctx := h.NewAuthenticatedContext(account)

userService := users.Service(&environments.Environment().Services)
rbService := roleBindings.Service(&environments.Environment().Services)
roleService := roles.Service(&environments.Environment().Services)

ownerRole, svcErr := roleService.GetByName(context.Background(), roles.RoleGatewayOwner)
Expect(svcErr).NotTo(HaveOccurred())

gatewayID := "gw-test-create"

// Provision the account as gateway:owner so the HTTP create passes validation.
callerID, userErr := userService.UpsertByUsername(context.Background(), strings.ToLower(account.Username), nil, nil)
Expect(userErr).NotTo(HaveOccurred())
Expect(rbService.CreateGatewayOwnerBinding(context.Background(), callerID, gatewayID)).To(Succeed())

rbInput := openapi.RoleBinding{
RoleId: ownerRole.ID,
Scope: "gateway",
Expand Down Expand Up @@ -105,13 +114,23 @@ func TestRoleBindingDelete(t *testing.T) {
account := h.NewRandAccount()
ctx := h.NewAuthenticatedContext(account)

userService := users.Service(&environments.Environment().Services)
rbService := roleBindings.Service(&environments.Environment().Services)
roleService := roles.Service(&environments.Environment().Services)

viewerRole, svcErr := roleService.GetByName(context.Background(), roles.RoleGatewayViewer)
Expect(svcErr).NotTo(HaveOccurred())

gatewayID := "gw-test-delete"
rbService := roleBindings.Service(&environments.Environment().Services)
rb, createErr := rbService.Create(context.Background(), &roleBindings.RoleBinding{

// Provision the account as gateway:owner so the HTTP delete passes validation.
callerID, userErr := userService.UpsertByUsername(context.Background(), strings.ToLower(account.Username), nil, nil)
Expect(userErr).NotTo(HaveOccurred())
Expect(rbService.CreateGatewayOwnerBinding(context.Background(), callerID, gatewayID)).To(Succeed())

// Create a viewer binding to delete (bypassing HTTP so no owner check needed for setup).
ownerCtx := context.WithValue(context.Background(), rbac.ContextUserIDKey, callerID)
rb, createErr := rbService.Create(ownerCtx, &roleBindings.RoleBinding{
RoleID: viewerRole.ID,
Scope: roleBindings.ScopeGateway,
GatewayID: &gatewayID,
Expand Down Expand Up @@ -316,3 +335,147 @@ func TestGrantValidation_CrossGatewayEscalation(t *testing.T) {
Expect(crossGWErr).To(HaveOccurred())
Expect(crossGWErr.HttpCode).To(Equal(http.StatusForbidden))
}

// TestUserProvisioningMiddleware_DefaultRoleAssignedWithNoJWTRoles drives the
// real HTTP middleware path with a token that carries no realm_access roles and
// asserts that the user receives a gateway:creator binding. This is the exact
// scenario RBAC_ENFORCE=true must handle: a brand-new user with no Keycloak roles.
func TestUserProvisioningMiddleware_DefaultRoleAssignedWithNoJWTRoles(t *testing.T) {
h, client := test.RegisterIntegration(t)

// NewRandAccount creates a JWT with no realm_access claims.
account := h.NewRandAccount()
ctx := h.NewAuthenticatedContext(account)

// GET /roles is RBAC-exempt so it succeeds regardless of bindings,
// but it still runs UserProvisioningMiddleware on the apiV1Router.
_, resp, err := client.DefaultAPI.ListRoles(ctx).Execute()
Expect(err).NotTo(HaveOccurred())
Expect(resp.StatusCode).To(Equal(http.StatusOK))

// The JWT helper lowercases the username; UpsertByUsername is idempotent.
userService := users.Service(&environments.Environment().Services)
rbService := roleBindings.Service(&environments.Environment().Services)
userID, userErr := userService.UpsertByUsername(context.Background(), strings.ToLower(account.Username), nil, nil)
Expect(userErr).NotTo(HaveOccurred())

bindings, findErr := rbService.FindBindingsByUserID(context.Background(), userID)
Expect(findErr).NotTo(HaveOccurred())

found := false
for _, b := range bindings {
if b.RoleName == roles.RoleGatewayCreator && b.Scope == roleBindings.ScopeGlobal {
found = true
}
}
Expect(found).To(BeTrue(), "expected gateway:creator binding after first authenticated request with no JWT roles")
}

// TestSyncJWTRoles_DefaultRoleAssigned verifies that a newly provisioned user
// receives a gateway:creator binding even when the JWT carries no roles.
func TestSyncJWTRoles_DefaultRoleAssigned(t *testing.T) {
test.RegisterIntegration(t)

rbService := roleBindings.Service(&environments.Environment().Services)
userService := users.Service(&environments.Environment().Services)

userID, userErr := userService.UpsertByUsername(context.Background(), "sync-default-new", nil, nil)
Expect(userErr).NotTo(HaveOccurred())

syncErr := rbService.SyncJWTRoles(context.Background(), userID, nil)
Expect(syncErr).NotTo(HaveOccurred())

bindings, findErr := rbService.FindBindingsByUserID(context.Background(), userID)
Expect(findErr).NotTo(HaveOccurred())

found := false
for _, b := range bindings {
if b.RoleName == roles.RoleGatewayCreator && b.Scope == roleBindings.ScopeGlobal {
found = true
}
}
Expect(found).To(BeTrue(), "expected gateway:creator global binding after sync with empty JWT")
}

// TestSyncJWTRoles_DefaultRoleIsIdempotent verifies that calling SyncJWTRoles
// twice does not create duplicate gateway:creator bindings.
func TestSyncJWTRoles_DefaultRoleIsIdempotent(t *testing.T) {
test.RegisterIntegration(t)

rbService := roleBindings.Service(&environments.Environment().Services)
userService := users.Service(&environments.Environment().Services)

userID, userErr := userService.UpsertByUsername(context.Background(), "sync-default-idempotent", nil, nil)
Expect(userErr).NotTo(HaveOccurred())

Expect(rbService.SyncJWTRoles(context.Background(), userID, nil)).To(Succeed())
Expect(rbService.SyncJWTRoles(context.Background(), userID, nil)).To(Succeed())

bindings, findErr := rbService.FindBindingsByUserID(context.Background(), userID)
Expect(findErr).NotTo(HaveOccurred())

creatorCount := 0
for _, b := range bindings {
if b.RoleName == roles.RoleGatewayCreator && b.Scope == roleBindings.ScopeGlobal {
creatorCount++
}
}
Expect(creatorCount).To(Equal(1), "expected exactly one gateway:creator binding after two syncs")
}

// TestSyncJWTRoles_DefaultRoleNotRemovedWhenAbsentFromJWT verifies that a user's
// gateway:creator binding is retained across syncs even when the JWT never carries it.
func TestSyncJWTRoles_DefaultRoleNotRemovedWhenAbsentFromJWT(t *testing.T) {
test.RegisterIntegration(t)

rbService := roleBindings.Service(&environments.Environment().Services)
userService := users.Service(&environments.Environment().Services)

userID, userErr := userService.UpsertByUsername(context.Background(), "sync-default-persist", nil, nil)
Expect(userErr).NotTo(HaveOccurred())

// First sync assigns the default.
Expect(rbService.SyncJWTRoles(context.Background(), userID, nil)).To(Succeed())

// Second sync with still-empty JWT must not revoke it.
Expect(rbService.SyncJWTRoles(context.Background(), userID, nil)).To(Succeed())

bindings, findErr := rbService.FindBindingsByUserID(context.Background(), userID)
Expect(findErr).NotTo(HaveOccurred())

found := false
for _, b := range bindings {
if b.RoleName == roles.RoleGatewayCreator && b.Scope == roleBindings.ScopeGlobal {
found = true
}
}
Expect(found).To(BeTrue(), "gateway:creator binding must survive a sync with an empty JWT")
}

// TestSyncJWTRoles_DefaultAndJWTRolesBothApplied verifies that a JWT-carried role
// (platform:admin) is synced in addition to the default gateway:creator.
// Default roles are always applied regardless of what the JWT carries.
func TestSyncJWTRoles_DefaultAndJWTRolesBothApplied(t *testing.T) {
test.RegisterIntegration(t)

rbService := roleBindings.Service(&environments.Environment().Services)
userService := users.Service(&environments.Environment().Services)

userID, userErr := userService.UpsertByUsername(context.Background(), "sync-default-and-jwt", nil, nil)
Expect(userErr).NotTo(HaveOccurred())

syncErr := rbService.SyncJWTRoles(context.Background(), userID, []string{roles.RolePlatformAdmin})
Expect(syncErr).NotTo(HaveOccurred())

bindings, findErr := rbService.FindBindingsByUserID(context.Background(), userID)
Expect(findErr).NotTo(HaveOccurred())

roleNames := make(map[string]bool)
for _, b := range bindings {
if b.Scope == roleBindings.ScopeGlobal {
roleNames[b.RoleName] = true
}
}
Expect(roleNames[roles.RolePlatformAdmin]).To(BeTrue(), "expected JWT-carried platform:admin binding")
Expect(roleNames[roles.RoleGatewayCreator]).To(BeTrue(), "expected default gateway:creator binding alongside JWT roles")
}
24 changes: 24 additions & 0 deletions components/api-server/plugins/roleBindings/plugin.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package roleBindings

import (
"net/http"
"os"
"strings"

"github.com/gorilla/mux"
"google.golang.org/grpc"
Expand All @@ -24,16 +26,38 @@ import (
type ServiceLocator func() RoleBindingService

func NewServiceLocator(env *environments.Env) ServiceLocator {
defaultRoles := defaultRolesFromEnv()
return func() RoleBindingService {
return NewRoleBindingService(
db.NewAdvisoryLockFactory(env.Database.SessionFactory),
NewRoleBindingDao(&env.Database.SessionFactory),
roles.NewRoleDao(&env.Database.SessionFactory),
events.Service(&env.Services),
defaultRoles,
)
}
}

// defaultRolesFromEnv reads RBAC_DEFAULT_ROLES (comma-separated role names).
// 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")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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).

if !set {
return []string{roles.RoleGatewayCreator}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[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.

}
if val == "" {
return nil
}
var result []string
for _, r := range strings.Split(val, ",") {
if trimmed := strings.TrimSpace(r); trimmed != "" {
result = append(result, trimmed)
}
}
return result
}

func Service(s *environments.Services) RoleBindingService {
if s == nil {
return nil
Expand Down
35 changes: 27 additions & 8 deletions components/api-server/plugins/roleBindings/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"fmt"

"github.com/golang/glog"
"github.com/openshift-online/hypershell/components/api-server/pkg/rbac"
"github.com/openshift-online/hypershell/components/api-server/plugins/roles"
"github.com/openshift-online/rh-trex-ai/pkg/api"
Expand Down Expand Up @@ -35,22 +36,30 @@ func NewRoleBindingService(
rbDao RoleBindingDao,
roleDao roles.RoleDao,
events services.EventService,
defaultRoles []string,
) RoleBindingService {
for _, r := range defaultRoles {
if !roles.JWTSyncedRoles[r] {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[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.

glog.Warningf("RBAC_DEFAULT_ROLES: role %q is not in JWTSyncedRoles and will be ignored; add it to JWTSyncedRoles to make it sync-eligible", r)
}
}
return &sqlRoleBindingService{
lockFactory: lockFactory,
rbDao: rbDao,
roleDao: roleDao,
events: events,
lockFactory: lockFactory,
rbDao: rbDao,
roleDao: roleDao,
events: events,
defaultRoles: defaultRoles,
}
}

var _ RoleBindingService = &sqlRoleBindingService{}

type sqlRoleBindingService struct {
lockFactory db.LockFactory
rbDao RoleBindingDao
roleDao roles.RoleDao
events services.EventService
lockFactory db.LockFactory
rbDao RoleBindingDao
roleDao roles.RoleDao
events services.EventService
defaultRoles []string
}

func (s *sqlRoleBindingService) CreateGatewayOwnerBinding(ctx context.Context, userID string, gatewayID string) error {
Expand Down Expand Up @@ -97,6 +106,16 @@ func (s *sqlRoleBindingService) SyncJWTRoles(ctx context.Context, userID string,
jwtRoleSet[r] = true
}
}
// Default roles are always merged regardless of what the JWT carries.
// They represent the platform's baseline posture: every authenticated
// 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 {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

if roles.JWTSyncedRoles[r] {
jwtRoleSet[r] = true
}
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[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:

  1. The spec states "Users start with zero permissions" and "gateway:creator from Keycloak only," plus scenarios asserting a non-creator and a bare platform:admin are denied gateway creation (403). All authenticated users are now creators, so those scenarios no longer hold.
  2. 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.


existing, err := s.rbDao.FindByUserID(ctx, userID)
if err != nil {
Expand Down
4 changes: 4 additions & 0 deletions components/api-server/plugins/roleBindings/testmain_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ import (
"github.com/golang/glog"

"github.com/openshift-online/hypershell/components/api-server/test"

// Register the rbac plugin so that UserProvisioningMiddleware is wired onto
// apiV1Router for HTTP-path integration tests.
_ "github.com/openshift-online/hypershell/components/api-server/plugins/rbac"
)

func TestMain(m *testing.M) {
Expand Down
Loading
Loading