From 7bf02ae2cd3aeae122060f496f00e6c5c9794a3f Mon Sep 17 00:00:00 2001 From: John Schaeffer Date: Wed, 6 Sep 2023 14:14:30 +0000 Subject: [PATCH 1/6] 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 --- internal/api/permissions.go | 57 ++++++++++++++++++++++--------------- internal/api/router.go | 18 ++++++++++-- 2 files changed, 49 insertions(+), 26 deletions(-) diff --git a/internal/api/permissions.go b/internal/api/permissions.go index 7aa9aa99..e69c6ac1 100644 --- a/internal/api/permissions.go +++ b/internal/api/permissions.go @@ -67,28 +67,36 @@ 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 +132,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 +226,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 +262,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/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 } From 6f6bb28ef013e3be4e46713fee507d4e76d59940 Mon Sep 17 00:00:00 2001 From: John Schaeffer Date: Wed, 6 Sep 2023 17:33:10 +0000 Subject: [PATCH 2/6] 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 --- internal/api/roles.go | 13 +++++++++++++ permissions-api.example.yaml | 3 ++- policy.example.yaml | 8 ++++++++ 3 files changed, 23 insertions(+), 1 deletion(-) diff --git a/internal/api/roles.go b/internal/api/roles.go index 9893a43a..924a4e3a 100644 --- a/internal/api/roles.go +++ b/internal/api/roles.go @@ -9,6 +9,10 @@ import ( "go.opentelemetry.io/otel/trace" ) +const ( + actionRoleCreate = "role_create" +) + func (r *Router) roleCreate(c echo.Context) error { resourceIDStr := c.Param("id") @@ -27,11 +31,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) 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..dac96979 100644 --- a/policy.example.yaml +++ b/policy.example.yaml @@ -30,12 +30,20 @@ unions: resourcetypenames: - tenant actions: + - name: role_create - 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: loadbalancer_create typename: resourceowner conditions: From d1352a4f2697de55ebd69ec2f57584559b25ad68 Mon Sep 17 00:00:00 2001 From: John Schaeffer Date: Wed, 6 Sep 2023 17:40:13 +0000 Subject: [PATCH 3/6] 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 --- cmd/createrole.go | 111 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 111 insertions(+) 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) +} From 43a9d0b6b11be28c1eeaecf5a04b99b29a4a8c3d Mon Sep 17 00:00:00 2001 From: John Schaeffer Date: Wed, 6 Sep 2023 19:00:55 +0000 Subject: [PATCH 4/6] Add permissions checks for other role operations This commit adds permissions checks for getting, listing, updating, and deleting roles. Signed-off-by: John Schaeffer --- internal/api/roles.go | 40 ++++++++++++++++++++++++++++++++++++++++ policy.example.yaml | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+) diff --git a/internal/api/roles.go b/internal/api/roles.go index 924a4e3a..e020909c 100644 --- a/internal/api/roles.go +++ b/internal/api/roles.go @@ -11,6 +11,10 @@ import ( const ( actionRoleCreate = "role_create" + actionRoleGet = "role_get" + actionRoleList = "role_list" + actionRoleUpdate = "role_update" + actionRoleDelete = "role_delete" ) func (r *Router) roleCreate(c echo.Context) error { @@ -69,11 +73,20 @@ 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) } + if err := r.checkActionWithResponse(ctx, subjectResource, actionRoleGet, roleResource); err != nil { + return err + } + role, err := r.engine.GetRole(ctx, roleResource, "") if err != nil { return echo.NewHTTPError(http.StatusInternalServerError, "error getting resource").SetInternal(err) @@ -98,11 +111,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) @@ -135,11 +157,20 @@ 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) } + if err := r.checkActionWithResponse(ctx, subjectResource, actionRoleDelete, roleResource); err != nil { + return err + } + _, err = r.engine.DeleteRole(ctx, roleResource, "") if err != nil { return echo.NewHTTPError(http.StatusInternalServerError, "error deleting resource").SetInternal(err) @@ -163,11 +194,20 @@ 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) } + if err := r.checkActionWithResponse(ctx, subjectResource, actionRoleGet, roleResource); err != nil { + return err + } + resource, err := r.engine.GetRoleResource(ctx, roleResource, "") if err != nil { return echo.NewHTTPError(http.StatusInternalServerError, "error getting resource").SetInternal(err) diff --git a/policy.example.yaml b/policy.example.yaml index dac96979..80af7c78 100644 --- a/policy.example.yaml +++ b/policy.example.yaml @@ -31,6 +31,10 @@ unions: - 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 @@ -44,6 +48,34 @@ actionbindings: - 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: From a1071c8ec9c58510cf73ba75d14f86d7295fa921 Mon Sep 17 00:00:00 2001 From: John Schaeffer Date: Wed, 6 Sep 2023 19:02:18 +0000 Subject: [PATCH 5/6] Fix linting whitespace issue in checkActionWithResponse Signed-off-by: John Schaeffer --- internal/api/permissions.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/api/permissions.go b/internal/api/permissions.go index e69c6ac1..c2fc4d81 100644 --- a/internal/api/permissions.go +++ b/internal/api/permissions.go @@ -82,6 +82,7 @@ func (r *Router) checkAction(c echo.Context) error { 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( From 5baa9489dcdaf8fa69b7af790f8e2d46108e3ced Mon Sep 17 00:00:00 2001 From: John Schaeffer Date: Wed, 6 Sep 2023 21:17:26 +0000 Subject: [PATCH 6/6] 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 --- internal/api/roles.go | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/internal/api/roles.go b/internal/api/roles.go index e020909c..7c323b25 100644 --- a/internal/api/roles.go +++ b/internal/api/roles.go @@ -83,7 +83,16 @@ func (r *Router) roleGet(c echo.Context) error { return echo.NewHTTPError(http.StatusBadRequest, "error getting resource").SetInternal(err) } - if err := r.checkActionWithResponse(ctx, subjectResource, actionRoleGet, roleResource); err != nil { + // 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 } @@ -167,7 +176,14 @@ func (r *Router) roleDelete(c echo.Context) error { return echo.NewHTTPError(http.StatusBadRequest, "error deleting resource").SetInternal(err) } - if err := r.checkActionWithResponse(ctx, subjectResource, actionRoleDelete, roleResource); err != nil { + // 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 } @@ -204,15 +220,17 @@ func (r *Router) roleGetResource(c echo.Context) error { return echo.NewHTTPError(http.StatusBadRequest, "error getting resource").SetInternal(err) } - if err := r.checkActionWithResponse(ctx, subjectResource, actionRoleGet, roleResource); err != nil { - return 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, }