From 8d474ee70478836c0638d9f0f4ea07d2a888c269 Mon Sep 17 00:00:00 2001 From: Bailin He <15058035+bailinhe@users.noreply.github.com> Date: Mon, 1 Apr 2024 18:35:30 -0400 Subject: [PATCH] Apply suggestions from code review Co-authored-by: John Schaeffer Signed-off-by: Bailin He <15058035+bailinhe@users.noreply.github.com> --- .gitignore | 13 +++ go.mod | 4 +- internal/iapl/policy.go | 153 ++++++++++++++++++++++------------- internal/iapl/policy_test.go | 8 +- internal/iapl/rbac.go | 16 ++-- 5 files changed, 123 insertions(+), 71 deletions(-) diff --git a/.gitignore b/.gitignore index f5fcb062a..9dff30142 100644 --- a/.gitignore +++ b/.gitignore @@ -17,6 +17,19 @@ # Emacs stuff *~ +# vscode stuff +.vscode/* +!.vscode/settings.json +!.vscode/tasks.json +!.vscode/extensions.json +!.vscode/*.code-snippets + +# Local History for Visual Studio Code +.history/ + +# Built Visual Studio Code Extensions +*.vsix + # .tools dir .tools/ diff --git a/go.mod b/go.mod index 126571069..3817dfc10 100644 --- a/go.mod +++ b/go.mod @@ -7,8 +7,6 @@ require ( github.com/authzed/grpcutil v0.0.0-20240123194739-2ea1e3d2d98b github.com/cockroachdb/cockroach-go/v2 v2.3.6 github.com/go-jose/go-jose/v4 v4.0.1 - github.com/golang-jwt/jwt v3.2.2+incompatible - github.com/labstack/echo-jwt/v4 v4.2.0 github.com/labstack/echo/v4 v4.11.4 github.com/lib/pq v1.10.9 github.com/nats-io/nats.go v1.31.0 @@ -44,6 +42,7 @@ require ( github.com/go-logr/logr v1.2.4 // indirect github.com/go-logr/stdr v1.2.2 // indirect github.com/gofrs/flock v0.8.1 // indirect + github.com/golang-jwt/jwt v3.2.2+incompatible // indirect github.com/golang-jwt/jwt/v5 v5.0.0 // indirect github.com/golang/protobuf v1.5.3 // indirect github.com/grpc-ecosystem/go-grpc-middleware v1.4.0 // indirect @@ -63,6 +62,7 @@ require ( github.com/jzelinskie/stringz v0.0.2 // indirect github.com/klauspost/compress v1.17.2 // indirect github.com/labstack/echo-contrib v0.15.0 // indirect + github.com/labstack/echo-jwt/v4 v4.2.0 // indirect github.com/labstack/gommon v0.4.2 // indirect github.com/magiconair/properties v1.8.7 // indirect github.com/mattn/go-colorable v0.1.13 // indirect diff --git a/internal/iapl/policy.go b/internal/iapl/policy.go index c54f6559e..249a2cdea 100644 --- a/internal/iapl/policy.go +++ b/internal/iapl/policy.go @@ -167,15 +167,6 @@ func (v *policy) validateUnions() error { func (v *policy) validateResourceTypes() error { for _, resourceType := range v.rt { for _, rel := range resourceType.Relationships { - // two kinds of relationships target types to be validated - // 1. simple target type names - for _, tt := range rel.TargetTypes { - if _, ok := v.rt[tt.Name]; !ok { - return fmt.Errorf("%s: relationships: %s: %w", resourceType.Name, tt.Name, ErrorUnknownType) - } - } - - // 2. target types with optional subject relation or subject identifier for _, tt := range rel.TargetTypes { if _, ok := v.rt[tt.Name]; !ok { return fmt.Errorf("%s: relationships: %s: %w", resourceType.Name, tt.Name, ErrorUnknownType) @@ -210,6 +201,17 @@ func (v *policy) validateConditionRelationshipAction(rt ResourceType, c Conditio return fmt.Errorf("%s: %w", c.Relation, ErrorUnknownRelation) } + // if there's a relationship action defined with only the relation, + // e.g., + // ```yaml + // actionname: someaction + // typename: someresource + // conditions: + // - relationshipaction: + // relation: some_relation + // ``` + // the above logics ensure that `some_relation` exists, thus can safely + // return without error if c.ActionName == "" { return nil } @@ -271,62 +273,20 @@ func (v *policy) validateActionBindings() error { return nil } -// validateMemberRoleRelationship ensures that there's a valid `member_role` -// relationship exists in a given resource type -func (v *policy) validateMemberRoleRelationship(res ResourceType) error { - relationshipExists := false - targetOk := false - - for _, rel := range res.Relationships { - if rel.Relation != RoleOwnerMemberRoleRelation { - continue - } - - relationshipExists = true - - for i := 0; i < len(rel.TargetTypes) && !targetOk; i++ { - if rel.TargetTypes[i].Name == v.p.RBAC.RoleResource.Name { - targetOk = true - } - } - - if !targetOk { - break - } - } - - if !relationshipExists || !targetOk { - return fmt.Errorf( - "%w: role owner %s must have %s relation to %s", - ErrorMissingRelationship, res.Name, RoleOwnerMemberRoleRelation, - v.p.RBAC.RoleResource, - ) - } - - return nil -} - // validateRoles validates V2 role resource types to ensure that: -// 1. role resource type has a valid owner relationship -// 2. role-owner resource types have a valid member-role relationship points -// to the role resource +// - role resource type has a valid owner relationship func (v *policy) validateRoles() error { if v.p.RBAC == nil { return nil } for _, roleOwnerName := range v.p.RBAC.RoleOwners { - owner, ok := v.rt[roleOwnerName] + _, ok := v.rt[roleOwnerName] // check if role owner exists if !ok { return fmt.Errorf("%w: role owner %s does not exist", ErrorUnknownType, roleOwnerName) } - - // role owner must have `member_role` relationship to `role` resource - if err := v.validateMemberRoleRelationship(owner); err != nil { - return err - } } return nil @@ -364,6 +324,18 @@ func (v *policy) expandActionBindings() { // createV2RoleResourceType creates a v2 role resource type contains a list of relationships // representing all the actions, as well as relationships and permissions for // the management of the roles themselves. +// This is equivalent to including a resource that looks like: +// +// name: rolev2 +// idprefix: permrv2 +// relationships: +// - relation: owner +// targettypes: +// - name: tenant +// - relation: foo_resource_get_rel +// targettypes: +// - name: user +// subjectidentifier: "*" func (v *policy) createV2RoleResourceType() { role := ResourceType{ Name: v.p.RBAC.RoleResource.Name, @@ -408,7 +380,37 @@ func (v *policy) createV2RoleResourceType() { // createRoleBindingResourceType creates a role-binding resource type contains // a list of all the actions. // The role-binding resources will be used to create a 3-way relationship -// between a resource, a subject and a role +// between a resource, a subject and a role. +// +// this function effectively creates: +// 1. a resource type like this: +// +// name: rolebinding +// idprefix: permrbn +// relationships: +// - relation: rolev2 +// targettypes: +// - name: rolev2 +// - relation: subject +// targettypes: +// - name: user +// subjectidentifier: "*" +// +// 2. a list of action-bindings representing permissions for all the actions in the policy +// +// actionbindings: +// - actionname: foo_resource_get +// typename: rolebinding +// conditionsets: +// - conditions: +// - relationshipaction: +// relation: rolev2 +// actionname: foo_resource_get_rel +// - conditions: +// - relationshipaction: +// relation: subject +// # ... other action bindings on the rolebinding resource for each +// # action defined in the policy func (v *policy) createRoleBindingResourceType() { rolebinding := ResourceType{ Name: v.p.RBAC.RoleBindingResource.Name, @@ -469,7 +471,44 @@ func (v *policy) createRoleBindingResourceType() { // types that has `ResourceRoleBindingV2` defined. // Relationships like member_roles, available_roles, are created to support // role inheritance, e.g., an org should be able to use roles defined by its -// parents +// parents. +// This function augments the resource types and effectively creating +// resource types like this: +// +// 1. for resource owners, `member_role` relationship is added +// +// ```diff +// resourcetypes: +// name: tenant +// idprefix: tnntten +// rolebindingv2: +// *rolesFromParent +// relationships: +// - relation: parent +// targettypes: +// - name: tenant_parent +// # ... other existing relationships +// + - relation: member_role +// + targettypes: +// + - name: rolev2 +// +// ``` +// +// 2. for resources that inherit permissions from other resources, `available_roles` +// action is added +// +// ```diff +// actionbindings: +// # ... other existing action bindings +// + - actionname: available_roles +// + typename: tenant +// + conditions: +// + - relationshipaction: +// + relation: member_role +// + - relationshipaction: +// + relation: parent +// + actionname: available_roles +// ``` func (v *policy) expandRBACV2Relationships() { for name, resourceType := range v.rt { // not all roles are available for all resources, available roles are @@ -503,7 +542,7 @@ func (v *policy) expandRBACV2Relationships() { availableRoles = append(availableRoles, Condition{ RelationshipAction: &ConditionRelationshipAction{ Relation: from, - ActionName: AvailableRoleRelation, + ActionName: AvailableRolesList, }, }) } @@ -512,7 +551,7 @@ func (v *policy) expandRBACV2Relationships() { // create available role permission if len(availableRoles) > 0 { action := ActionBinding{ - ActionName: AvailableRoleRelation, + ActionName: AvailableRolesList, TypeName: resourceType.Name, Conditions: availableRoles, } diff --git a/internal/iapl/policy_test.go b/internal/iapl/policy_test.go index a309b4c77..99718f94e 100644 --- a/internal/iapl/policy_test.go +++ b/internal/iapl/policy_test.go @@ -420,8 +420,8 @@ func TestPolicy(t *testing.T) { Name: "RBAC_OK", Input: PolicyDocument{ RBAC: &RBAC{ - RoleResource: rbacResourceDefinition{"rolev2", "permrv2"}, - RoleBindingResource: rbacResourceDefinition{"role_binding", "permrbn"}, + RoleResource: RBACResourceDefinition{"rolev2", "permrv2"}, + RoleBindingResource: RBACResourceDefinition{"role_binding", "permrbn"}, RoleSubjectTypes: []string{"user"}, RoleOwners: []string{"tenant"}, RoleBindingSubjects: []types.TargetType{{Name: "user"}}, @@ -458,8 +458,8 @@ func TestPolicy(t *testing.T) { func defaultRBAC() RBAC { return RBAC{ - RoleResource: rbacResourceDefinition{"rolev2", "permrv2"}, - RoleBindingResource: rbacResourceDefinition{"role_binding", "permrbn"}, + RoleResource: RBACResourceDefinition{"rolev2", "permrv2"}, + RoleBindingResource: RBACResourceDefinition{"role_binding", "permrbn"}, RoleSubjectTypes: []string{"user", "client"}, RoleOwners: []string{"tenant"}, RoleBindingSubjects: []types.TargetType{{Name: "user"}, {Name: "client"}, {Name: "group", SubjectRelation: "member"}}, diff --git a/internal/iapl/rbac.go b/internal/iapl/rbac.go index 56aa98a41..81b9d21b3 100644 --- a/internal/iapl/rbac.go +++ b/internal/iapl/rbac.go @@ -10,9 +10,9 @@ const ( // RoleOwnerMemberRoleRelation is the name of the relationship that connects a resource // to a role that it owns RoleOwnerMemberRoleRelation = "member_role" - // AvailableRoleRelation is the name of the relationship that list all roles - // that are available for a given resource - AvailableRoleRelation = "avail_role" + // AvailableRolesList is the name of the action in a resource that returns a list + // of roles that are available for the resource + AvailableRolesList = "avail_role" // RolebindingRoleRelation is the name of the relationship that connects a role binding to a role. RolebindingRoleRelation = "role" // RolebindingSubjectRelation is the name of the relationship that connects a role binding to a subject. @@ -106,9 +106,9 @@ in IAPL policy terms: */ type RBAC struct { // RoleResource is the name of the resource type that represents a role. - RoleResource rbacResourceDefinition + RoleResource RBACResourceDefinition // RoleBindingResource is the name of the resource type that represents a role binding. - RoleBindingResource rbacResourceDefinition + RoleBindingResource RBACResourceDefinition // RoleSubjectTypes is a list of subject types that the relationships in a // role resource will contain, see the example above. RoleSubjectTypes []string @@ -128,9 +128,9 @@ type RBAC struct { roleownersset map[string]struct{} } -// rbacResourceDefinition is a struct to define a resource type for a role +// RBACResourceDefinition is a struct to define a resource type for a role // and role-bindings -type rbacResourceDefinition struct { +type RBACResourceDefinition struct { Name string IDPrefix string } @@ -204,7 +204,7 @@ func (r *RBAC) RoleBindingActions() []Action { actions = append(actions, Action{Name: string(action)}) } - actions = append(actions, Action{Name: AvailableRoleRelation}) + actions = append(actions, Action{Name: AvailableRolesList}) return actions }