From 5a663915fafc38c25deb03e7f34ed8aeba115752 Mon Sep 17 00:00:00 2001 From: John Schaeffer Date: Wed, 6 Sep 2023 17:50:41 -0400 Subject: [PATCH] Role permissions checks (#170) * Refactor logic for getting current subject and checking permissions Some functions, like currentSubject and checkAction, have logic that is either duplicated or could be reused elsewhere for things like checking access to actions in permissions-api itself. This commit moves some of that logic around so it is easier for other handlers (such as the handlers for roles) to use. Signed-off-by: John Schaeffer * Add permissions checks for role creation This commit adds permissions checks for role creation, as well as the action and bindings to the example policy. Signed-off-by: John Schaeffer * Add create-role command to bootstrap permissions-api This commit adds a command to create roles directly in SpiceDB, bypassing permissions checks. The intent of this command is to bootstrap a new permissions-api deployment with enough access to start provisioning roles using some subject. Signed-off-by: John Schaeffer * Add permissions checks for other role operations This commit adds permissions checks for getting, listing, updating, and deleting roles. Signed-off-by: John Schaeffer * Fix linting whitespace issue in checkActionWithResponse Signed-off-by: John Schaeffer * Check role permissions based on the resource, not the role One of the quirks of our current role model is that roles don't belong to a resource outright - instead, their binding to a resource is inferred by the actions that can be performed. This means that we can't use the role itself to make authorization decisions. This commit updates permissions checks for roles to use the role's resource rather than the role itself for checking permissions. Signed-off-by: John Schaeffer --------- Signed-off-by: John Schaeffer --- cmd/createrole.go | 111 +++++++++++++++++++++++++++++++++++ internal/api/permissions.go | 58 ++++++++++-------- internal/api/roles.go | 71 ++++++++++++++++++++++ internal/api/router.go | 18 +++++- permissions-api.example.yaml | 3 +- policy.example.yaml | 40 +++++++++++++ 6 files changed, 274 insertions(+), 27 deletions(-) create mode 100644 cmd/createrole.go diff --git a/cmd/createrole.go b/cmd/createrole.go new file mode 100644 index 00000000..8dc22ed8 --- /dev/null +++ b/cmd/createrole.go @@ -0,0 +1,111 @@ +package cmd + +import ( + "context" + + "github.com/spf13/cobra" + "github.com/spf13/viper" + "go.infratographer.com/permissions-api/internal/config" + "go.infratographer.com/permissions-api/internal/iapl" + "go.infratographer.com/permissions-api/internal/query" + "go.infratographer.com/permissions-api/internal/spicedbx" + "go.infratographer.com/x/gidx" + "go.infratographer.com/x/viperx" +) + +const ( + createRoleFlagSubject = "subject" + createRoleFlagResource = "resource" + createRoleFlagActions = "actions" +) + +var ( + createRoleCmd = &cobra.Command{ + Use: "create-role", + Short: "create role in SpiceDB directly", + Run: func(cmd *cobra.Command, args []string) { + createRole(cmd.Context(), globalCfg) + }, + } +) + +func init() { + rootCmd.AddCommand(createRoleCmd) + + flags := createRoleCmd.Flags() + flags.String(createRoleFlagSubject, "", "subject to assign to created role") + flags.StringSlice(createRoleFlagActions, []string{}, "actions to assign to created role") + flags.String(createRoleFlagResource, "", "resource to bind to created role") + + v := viper.GetViper() + + viperx.MustBindFlag(v, createRoleFlagSubject, flags.Lookup(createRoleFlagSubject)) + viperx.MustBindFlag(v, createRoleFlagActions, flags.Lookup(createRoleFlagActions)) + viperx.MustBindFlag(v, createRoleFlagResource, flags.Lookup(createRoleFlagResource)) +} + +func createRole(ctx context.Context, cfg *config.AppConfig) { + subjectIDStr := viper.GetString(createRoleFlagSubject) + actions := viper.GetStringSlice(createRoleFlagActions) + resourceIDStr := viper.GetString(createRoleFlagResource) + + if subjectIDStr == "" || len(actions) == 0 || resourceIDStr == "" { + logger.Fatal("invalid config") + } + + spiceClient, err := spicedbx.NewClient(cfg.SpiceDB, cfg.Tracing.Enabled) + if err != nil { + logger.Fatalw("unable to initialize spicedb client", "error", err) + } + + var policy iapl.Policy + + if cfg.SpiceDB.PolicyFile != "" { + policy, err = iapl.NewPolicyFromFile(cfg.SpiceDB.PolicyFile) + if err != nil { + logger.Fatalw("unable to load new policy from schema file", "policy_file", cfg.SpiceDB.PolicyFile, "error", err) + } + } else { + logger.Warn("no spicedb policy file defined, using default policy") + + policy = iapl.DefaultPolicy() + } + + if err = policy.Validate(); err != nil { + logger.Fatalw("invalid spicedb policy", "error", err) + } + + resourceID, err := gidx.Parse(resourceIDStr) + if err != nil { + logger.Fatalw("error parsing resource ID", "error", err) + } + + subjectID, err := gidx.Parse(subjectIDStr) + if err != nil { + logger.Fatalw("error parsing subject ID", "error", err) + } + + engine := query.NewEngine("infratographer", spiceClient, query.WithPolicy(policy), query.WithLogger(logger)) + + resource, err := engine.NewResourceFromID(resourceID) + if err != nil { + logger.Fatalw("error creating resource", "error", err) + } + + subjectResource, err := engine.NewResourceFromID(subjectID) + if err != nil { + logger.Fatalw("error creating subject resource", "error", err) + } + + role, _, err := engine.CreateRole(ctx, resource, actions) + if err != nil { + logger.Fatalw("error creating role", "error", err) + } + + _, err = engine.AssignSubjectRole(ctx, subjectResource, role) + if err != nil { + logger.Fatalw("error creating role", "error", err) + } + + logger.Infow("role successfully created", "role_id", role.ID) +} diff --git a/internal/api/permissions.go b/internal/api/permissions.go index 7aa9aa99..c2fc4d81 100644 --- a/internal/api/permissions.go +++ b/internal/api/permissions.go @@ -67,28 +67,37 @@ func (r *Router) checkAction(c echo.Context) error { } // Subject validation - subject, err := currentSubject(c) + subjectResource, err := r.currentSubject(c) if err != nil { - return echo.NewHTTPError(http.StatusBadRequest, "failed to get the subject").SetInternal(err) + return err } - subjectResource, err := r.engine.NewResourceFromID(subject) - if err != nil { - return echo.NewHTTPError(http.StatusBadRequest, "error processing subject ID").SetInternal(err) + // Check the permissions + if err := r.checkActionWithResponse(ctx, subjectResource, action, resource); err != nil { + return err } - // Check the permissions - err = r.engine.SubjectHasPermission(ctx, subjectResource, action, resource) - if err != nil && errors.Is(err, query.ErrActionNotAssigned) { - msg := fmt.Sprintf("subject '%s' does not have permission to perform action '%s' on resource '%s'", - subject, action, resourceIDStr) + return c.JSON(http.StatusOK, echo.Map{}) +} + +func (r *Router) checkActionWithResponse(ctx context.Context, subjectResource types.Resource, action string, resource types.Resource) error { + err := r.engine.SubjectHasPermission(ctx, subjectResource, action, resource) + + switch { + case errors.Is(err, query.ErrActionNotAssigned): + msg := fmt.Sprintf( + "subject '%s' does not have permission to perform action '%s' on resource '%s'", + subjectResource.ID.String(), + action, + resource.ID.String(), + ) return echo.NewHTTPError(http.StatusForbidden, msg).SetInternal(err) - } else if err != nil { + case err != nil: return echo.NewHTTPError(http.StatusInternalServerError, "an error occurred checking permissions").SetInternal(err) + default: + return nil } - - return c.JSON(http.StatusOK, echo.Map{}) } type checkPermissionsRequest struct { @@ -124,14 +133,9 @@ func (r *Router) checkAllActions(c echo.Context) error { defer span.End() // Subject validation - subject, err := currentSubject(c) - if err != nil { - return echo.NewHTTPError(http.StatusBadRequest, "failed to get the subject").SetInternal(err) - } - - subjectResource, err := r.engine.NewResourceFromID(subject) + subjectResource, err := r.currentSubject(c) if err != nil { - return echo.NewHTTPError(http.StatusBadRequest, "error processing subject ID").SetInternal(err) + return err } var reqBody checkPermissionsRequest @@ -223,8 +227,13 @@ func (r *Router) checkAllActions(c echo.Context) error { case result := <-resultsCh: if result.Error != nil { if errors.Is(result.Error, query.ErrActionNotAssigned) { - err := fmt.Errorf("%w: subject '%s' does not have permission to perform action '%s' on resource '%s'", - ErrAccessDenied, subject, result.Request.Action, result.Request.Resource.ID.String()) + err := fmt.Errorf( + "%w: subject '%s' does not have permission to perform action '%s' on resource '%s'", + ErrAccessDenied, + subjectResource.ID, + result.Request.Action, + result.Request.Resource.ID, + ) unauthorizedErrors++ @@ -254,7 +263,10 @@ func (r *Router) checkAllActions(c echo.Context) error { } if unauthorizedErrors != 0 { - msg := fmt.Sprintf("subject '%s' does not have permission to the requested resource actions", subject) + msg := fmt.Sprintf( + "subject '%s' does not have permission to the requested resource actions", + subjectResource.ID, + ) return echo.NewHTTPError(http.StatusForbidden, msg).SetInternal(multierr.Combine(allErrors...)) } diff --git a/internal/api/roles.go b/internal/api/roles.go index 9893a43a..7c323b25 100644 --- a/internal/api/roles.go +++ b/internal/api/roles.go @@ -9,6 +9,14 @@ import ( "go.opentelemetry.io/otel/trace" ) +const ( + actionRoleCreate = "role_create" + actionRoleGet = "role_get" + actionRoleList = "role_list" + actionRoleUpdate = "role_update" + actionRoleDelete = "role_delete" +) + func (r *Router) roleCreate(c echo.Context) error { resourceIDStr := c.Param("id") @@ -27,11 +35,20 @@ func (r *Router) roleCreate(c echo.Context) error { return echo.NewHTTPError(http.StatusBadRequest, "error parsing request body").SetInternal(err) } + subjectResource, err := r.currentSubject(c) + if err != nil { + return err + } + resource, err := r.engine.NewResourceFromID(resourceID) if err != nil { return echo.NewHTTPError(http.StatusBadRequest, "error creating resource").SetInternal(err) } + if err := r.checkActionWithResponse(ctx, subjectResource, actionRoleCreate, resource); err != nil { + return err + } + role, _, err := r.engine.CreateRole(ctx, resource, reqBody.Actions) if err != nil { return echo.NewHTTPError(http.StatusInternalServerError, "error creating resource").SetInternal(err) @@ -56,11 +73,29 @@ func (r *Router) roleGet(c echo.Context) error { return echo.NewHTTPError(http.StatusBadRequest, "error getting resource").SetInternal(err) } + subjectResource, err := r.currentSubject(c) + if err != nil { + return err + } + roleResource, err := r.engine.NewResourceFromID(roleResourceID) if err != nil { return echo.NewHTTPError(http.StatusBadRequest, "error getting resource").SetInternal(err) } + // Roles belong to resources by way of the actions they can perform; do the permissions + // check on the role resource. + resource, err := r.engine.GetRoleResource(ctx, roleResource, "") + if err != nil { + return echo.NewHTTPError(http.StatusBadRequest, "error getting resource").SetInternal(err) + } + + // TODO: This shows an error for the role's resource, not the role. Determine if that + // matters. + if err := r.checkActionWithResponse(ctx, subjectResource, actionRoleGet, resource); err != nil { + return err + } + role, err := r.engine.GetRole(ctx, roleResource, "") if err != nil { return echo.NewHTTPError(http.StatusInternalServerError, "error getting resource").SetInternal(err) @@ -85,11 +120,20 @@ func (r *Router) rolesList(c echo.Context) error { return echo.NewHTTPError(http.StatusBadRequest, "error parsing resource ID").SetInternal(err) } + subjectResource, err := r.currentSubject(c) + if err != nil { + return err + } + resource, err := r.engine.NewResourceFromID(resourceID) if err != nil { return echo.NewHTTPError(http.StatusBadRequest, "error creating resource").SetInternal(err) } + if err := r.checkActionWithResponse(ctx, subjectResource, actionRoleList, resource); err != nil { + return err + } + roles, err := r.engine.ListRoles(ctx, resource, "") if err != nil { return echo.NewHTTPError(http.StatusInternalServerError, "error getting role").SetInternal(err) @@ -122,11 +166,27 @@ func (r *Router) roleDelete(c echo.Context) error { return echo.NewHTTPError(http.StatusBadRequest, "error deleting resource").SetInternal(err) } + subjectResource, err := r.currentSubject(c) + if err != nil { + return err + } + roleResource, err := r.engine.NewResourceFromID(roleResourceID) if err != nil { return echo.NewHTTPError(http.StatusBadRequest, "error deleting resource").SetInternal(err) } + // Roles belong to resources by way of the actions they can perform; do the permissions + // check on the role resource. + resource, err := r.engine.GetRoleResource(ctx, roleResource, "") + if err != nil { + return echo.NewHTTPError(http.StatusBadRequest, "error getting resource").SetInternal(err) + } + + if err := r.checkActionWithResponse(ctx, subjectResource, actionRoleDelete, resource); err != nil { + return err + } + _, err = r.engine.DeleteRole(ctx, roleResource, "") if err != nil { return echo.NewHTTPError(http.StatusInternalServerError, "error deleting resource").SetInternal(err) @@ -150,16 +210,27 @@ func (r *Router) roleGetResource(c echo.Context) error { return echo.NewHTTPError(http.StatusBadRequest, "error getting resource").SetInternal(err) } + subjectResource, err := r.currentSubject(c) + if err != nil { + return err + } + roleResource, err := r.engine.NewResourceFromID(roleResourceID) if err != nil { return echo.NewHTTPError(http.StatusBadRequest, "error getting resource").SetInternal(err) } + // There's a little irony here in that getting a role's resource here is required to actually + // do the permissions check. resource, err := r.engine.GetRoleResource(ctx, roleResource, "") if err != nil { return echo.NewHTTPError(http.StatusInternalServerError, "error getting resource").SetInternal(err) } + if err := r.checkActionWithResponse(ctx, subjectResource, actionRoleGet, resource); err != nil { + return err + } + resp := resourceResponse{ ID: resource.ID, } diff --git a/internal/api/router.go b/internal/api/router.go index bd3d1c29..f2e2cbcf 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -2,9 +2,11 @@ package api import ( "context" + "net/http" "github.com/labstack/echo/v4" "go.infratographer.com/permissions-api/internal/query" + "go.infratographer.com/permissions-api/internal/types" "go.infratographer.com/x/echojwtx" "go.infratographer.com/x/gidx" "go.opentelemetry.io/otel" @@ -97,8 +99,18 @@ func WithCheckConcurrency(count int) Option { } } -func currentSubject(c echo.Context) (gidx.PrefixedID, error) { - subject := echojwtx.Actor(c) +func (r *Router) currentSubject(c echo.Context) (types.Resource, error) { + subjectStr := echojwtx.Actor(c) - return gidx.Parse(subject) + subject, err := gidx.Parse(subjectStr) + if err != nil { + return types.Resource{}, echo.NewHTTPError(http.StatusBadRequest, "failed to get the subject").SetInternal(err) + } + + subjectResource, err := r.engine.NewResourceFromID(subject) + if err != nil { + return types.Resource{}, echo.NewHTTPError(http.StatusBadRequest, "error processing subject ID").SetInternal(err) + } + + return subjectResource, nil } diff --git a/permissions-api.example.yaml b/permissions-api.example.yaml index 46eb2281..3e74ca68 100644 --- a/permissions-api.example.yaml +++ b/permissions-api.example.yaml @@ -1,3 +1,4 @@ oidc: issuer: http://mock-oauth2-server:8081/default - +spicedb: + policyFile: /workspace/policy.example.yaml diff --git a/policy.example.yaml b/policy.example.yaml index 4a6e46f2..80af7c78 100644 --- a/policy.example.yaml +++ b/policy.example.yaml @@ -30,12 +30,52 @@ unions: resourcetypenames: - tenant actions: + - name: role_create + - name: role_get + - name: role_list + - name: role_update + - name: role_delete - name: loadbalancer_create - name: loadbalancer_get - name: loadbalancer_list - name: loadbalancer_update - name: loadbalancer_delete actionbindings: + - actionname: role_create + typename: resourceowner + conditions: + - rolebinding: {} + - relationshipaction: + relation: parent + actionname: role_create + - actionname: role_get + typename: resourceowner + conditions: + - rolebinding: {} + - relationshipaction: + relation: parent + actionname: role_get + - actionname: role_list + typename: resourceowner + conditions: + - rolebinding: {} + - relationshipaction: + relation: parent + actionname: role_list + - actionname: role_update + typename: resourceowner + conditions: + - rolebinding: {} + - relationshipaction: + relation: parent + actionname: role_update + - actionname: role_delete + typename: resourceowner + conditions: + - rolebinding: {} + - relationshipaction: + relation: parent + actionname: role_delete - actionname: loadbalancer_create typename: resourceowner conditions: