From 72dddd85d4d853a0d51fd6ff1c819ea2ee49593e Mon Sep 17 00:00:00 2001 From: Simon Murray Date: Thu, 4 Jul 2024 17:11:44 +0100 Subject: [PATCH] Fix Role RBAC So bloody simple when you think about it! When reading roles, only return the ones you already have all the permissions for at the same or greated scope. Basically reuses the existing functions like magic! --- charts/identity/Chart.yaml | 4 +- .../identity.unikorn-cloud.org_roles.yaml | 5 --- pkg/apis/unikorn/v1alpha1/types.go | 3 -- pkg/handler/handler.go | 2 +- pkg/handler/roles/client.go | 20 ++++----- pkg/rbac/handler.go | 42 ++++++++++++++++--- 6 files changed, 49 insertions(+), 27 deletions(-) diff --git a/charts/identity/Chart.yaml b/charts/identity/Chart.yaml index 1df62048..3c319f08 100644 --- a/charts/identity/Chart.yaml +++ b/charts/identity/Chart.yaml @@ -4,8 +4,8 @@ description: A Helm chart for deploying Unikorn's IdP type: application -version: v0.2.20 -appVersion: v0.2.20 +version: v0.2.21 +appVersion: v0.2.21 icon: https://raw.githubusercontent.com/unikorn-cloud/assets/main/images/logos/dark-on-light/icon.png diff --git a/charts/identity/crds/identity.unikorn-cloud.org_roles.yaml b/charts/identity/crds/identity.unikorn-cloud.org_roles.yaml index 658f42d7..d745b6de 100644 --- a/charts/identity/crds/identity.unikorn-cloud.org_roles.yaml +++ b/charts/identity/crds/identity.unikorn-cloud.org_roles.yaml @@ -56,11 +56,6 @@ spec: spec: description: RoleSpec defines the role's requested state. properties: - protected: - description: |- - Protected means the role will only be shown to users if they already - have it in the scope of the organization. - type: boolean scopes: description: Scopes are a list of uniquely named scopes for the role. properties: diff --git a/pkg/apis/unikorn/v1alpha1/types.go b/pkg/apis/unikorn/v1alpha1/types.go index aba318d0..2d617fa3 100644 --- a/pkg/apis/unikorn/v1alpha1/types.go +++ b/pkg/apis/unikorn/v1alpha1/types.go @@ -140,9 +140,6 @@ type Role struct { // RoleSpec defines the role's requested state. type RoleSpec struct { - // Protected means the role will only be shown to users if they already - // have it in the scope of the organization. - Protected bool `json:"protected,omitempty"` // Scopes are a list of uniquely named scopes for the role. Scopes RoleScopes `json:"scopes,omitempty"` } diff --git a/pkg/handler/handler.go b/pkg/handler/handler.go index 9e7e3ef6..64e5f3be 100644 --- a/pkg/handler/handler.go +++ b/pkg/handler/handler.go @@ -187,7 +187,7 @@ func (h *Handler) GetApiV1OrganizationsOrganizationIDRoles(w http.ResponseWriter return } - result, err := roles.New(h.client, h.namespace).List(r.Context()) + result, err := roles.New(h.client, h.namespace).List(r.Context(), organizationID) if err != nil { errors.HandleError(w, r, err) return diff --git a/pkg/handler/roles/client.go b/pkg/handler/roles/client.go index 455790b4..507fe487 100644 --- a/pkg/handler/roles/client.go +++ b/pkg/handler/roles/client.go @@ -25,6 +25,7 @@ import ( "github.com/unikorn-cloud/core/pkg/server/conversion" unikornv1 "github.com/unikorn-cloud/identity/pkg/apis/unikorn/v1alpha1" "github.com/unikorn-cloud/identity/pkg/openapi" + "github.com/unikorn-cloud/identity/pkg/rbac" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -41,9 +42,9 @@ func New(client client.Client, namespace string) *Client { } } -func convert(in *unikornv1.Role) openapi.RoleRead { +func convert(in unikornv1.Role) openapi.RoleRead { out := openapi.RoleRead{ - Metadata: conversion.ResourceReadMetadata(in, coreapi.ResourceProvisioningStatusProvisioned), + Metadata: conversion.ResourceReadMetadata(&in, coreapi.ResourceProvisioningStatusProvisioned), } return out @@ -52,14 +53,7 @@ func convert(in *unikornv1.Role) openapi.RoleRead { func convertList(in unikornv1.RoleList) openapi.Roles { var out openapi.Roles - for i := range in.Items { - resource := &in.Items[i] - - // We need to only display these if we have them in scope. - if resource.Spec.Protected { - continue - } - + for _, resource := range in.Items { out = append(out, convert(resource)) } @@ -70,12 +64,16 @@ func convertList(in unikornv1.RoleList) openapi.Roles { return out } -func (c *Client) List(ctx context.Context) (openapi.Roles, error) { +func (c *Client) List(ctx context.Context, organizationID string) (openapi.Roles, error) { var result unikornv1.RoleList if err := c.client.List(ctx, &result, &client.ListOptions{Namespace: c.namespace}); err != nil { return nil, err } + result.Items = slices.DeleteFunc(result.Items, func(role unikornv1.Role) bool { + return rbac.AllowRole(ctx, &role, organizationID) != nil + }) + return convertList(result), nil } diff --git a/pkg/rbac/handler.go b/pkg/rbac/handler.go index 4d86e14e..83596d7c 100644 --- a/pkg/rbac/handler.go +++ b/pkg/rbac/handler.go @@ -21,12 +21,13 @@ import ( "slices" "github.com/unikorn-cloud/core/pkg/server/errors" + unikornv1 "github.com/unikorn-cloud/identity/pkg/apis/unikorn/v1alpha1" "github.com/unikorn-cloud/identity/pkg/openapi" ) -// allowEndpoints iterates through all endpoints and tries to match the required name and +// operationAllowedByEndpoints iterates through all endpoints and tries to match the required name and // operation. -func allowEndpoints(endpoints openapi.AclEndpoints, endpoint string, operation openapi.AclOperation) error { +func operationAllowedByEndpoints(endpoints openapi.AclEndpoints, endpoint string, operation openapi.AclOperation) error { for _, e := range endpoints { if e.Name != endpoint { continue @@ -50,7 +51,7 @@ func AllowGlobalScope(ctx context.Context, endpoint string, operation openapi.Ac return errors.HTTPForbidden("operation is not allowed by rbac (no global endpoints)") } - return allowEndpoints(*acl.Global, endpoint, operation) + return operationAllowedByEndpoints(*acl.Global, endpoint, operation) } // AllowOrganizationScope tries to allow the requested operation at the global scope, then @@ -66,7 +67,7 @@ func AllowOrganizationScope(ctx context.Context, endpoint string, operation open return errors.HTTPForbidden("operation is not allowed by rbac (no matching organization endpoints)") } - return allowEndpoints(acl.Organization.Endpoints, endpoint, operation) + return operationAllowedByEndpoints(acl.Organization.Endpoints, endpoint, operation) } // AllowProjectScope tries to allow the requested operation at the global scope, then @@ -87,10 +88,41 @@ func AllowProjectScope(ctx context.Context, endpoint string, operation openapi.A continue } - if err := allowEndpoints(project.Endpoints, endpoint, operation); err == nil { + if err := operationAllowedByEndpoints(project.Endpoints, endpoint, operation); err == nil { return nil } } return errors.HTTPForbidden("operation is not allowed by rbac (no matching project endpoints)") } + +// AllowRole determines whether your ACL contains the same or higher privileges than +// the role, which is then used to determine role visibility and limit privilege +// escalation. +func AllowRole(ctx context.Context, role *unikornv1.Role, organizationID string) error { + for _, endpoint := range role.Spec.Scopes.Global { + for _, operation := range endpoint.Operations { + if err := AllowGlobalScope(ctx, endpoint.Name, convertOperation(operation)); err != nil { + return err + } + } + } + + for _, endpoint := range role.Spec.Scopes.Organization { + for _, operation := range endpoint.Operations { + if err := AllowOrganizationScope(ctx, endpoint.Name, convertOperation(operation), organizationID); err != nil { + return err + } + } + } + + for _, endpoint := range role.Spec.Scopes.Project { + for _, operation := range endpoint.Operations { + if err := AllowOrganizationScope(ctx, endpoint.Name, convertOperation(operation), organizationID); err != nil { + return err + } + } + } + + return nil +}