From 3d0db3081411d1afb54fd5317f51a1dd6f44180e Mon Sep 17 00:00:00 2001 From: ItamarYuran <95186982+ItamarYuran@users.noreply.github.com> Date: Tue, 12 Nov 2024 18:40:13 +0200 Subject: [PATCH] 8224 report required permissions when authorization fails (#8314) In case of unauthorized request, an error including denied/missing permissions will be raised --- contrib/auth/acl/service.go | 6 +- esti/auth_test.go | 9 ++- pkg/api/controller_test.go | 148 ++++++++++++++++++++++++++++++++++++ pkg/auth/service.go | 41 +++++++--- 4 files changed, 188 insertions(+), 16 deletions(-) diff --git a/contrib/auth/acl/service.go b/contrib/auth/acl/service.go index 5de25352da3..cc83c528847 100644 --- a/contrib/auth/acl/service.go +++ b/contrib/auth/acl/service.go @@ -895,13 +895,13 @@ func (s *AuthService) Authorize(ctx context.Context, req *auth.AuthorizationRequ if err != nil { return nil, err } - - allowed := auth.CheckPermissions(ctx, req.RequiredPermissions, req.Username, policies) + permAudit := &auth.MissingPermissions{} + allowed := auth.CheckPermissions(ctx, req.RequiredPermissions, req.Username, policies, permAudit) if allowed != auth.CheckAllow { return &auth.AuthorizationResponse{ Allowed: false, - Error: auth.ErrInsufficientPermissions, + Error: fmt.Errorf("%w: %s", auth.ErrInsufficientPermissions, permAudit.String()), }, nil } diff --git a/esti/auth_test.go b/esti/auth_test.go index dcbc7bdccca..9cc40421f9e 100644 --- a/esti/auth_test.go +++ b/esti/auth_test.go @@ -4,6 +4,7 @@ import ( "context" "net/http" "slices" + "strings" "testing" "time" @@ -234,7 +235,7 @@ func TestCreateRepo_Unauthorized(t *testing.T) { }) require.NoError(t, err) require.NotNil(t, resp.JSON401) - if resp.JSON401.Message != auth.ErrInsufficientPermissions.Error() { + if !strings.Contains(resp.JSON401.Message, auth.ErrInsufficientPermissions.Error()) { t.Fatalf("expected error message %q, got %q", auth.ErrInsufficientPermissions.Error(), resp.JSON401.Message) } } @@ -255,7 +256,7 @@ func TestRepoMetadata_Unauthorized(t *testing.T) { }) require.NoError(t, err) require.NotNil(t, resp.JSON401) - if resp.JSON401.Message != auth.ErrInsufficientPermissions.Error() { + if !strings.Contains(resp.JSON401.Message, auth.ErrInsufficientPermissions.Error()) { t.Errorf("expected error message %q, got %q", auth.ErrInsufficientPermissions.Error(), resp.JSON401.Message) } }) @@ -263,7 +264,7 @@ func TestRepoMetadata_Unauthorized(t *testing.T) { resp, err := clt.DeleteRepositoryMetadataWithResponse(ctx, repo, apigen.DeleteRepositoryMetadataJSONRequestBody{Keys: []string{"foo"}}) require.NoError(t, err) require.NotNil(t, resp.JSON401) - if resp.JSON401.Message != auth.ErrInsufficientPermissions.Error() { + if !strings.Contains(resp.JSON401.Message, auth.ErrInsufficientPermissions.Error()) { t.Errorf("expected error message %q, got %q", auth.ErrInsufficientPermissions.Error(), resp.JSON401.Message) } }) @@ -272,7 +273,7 @@ func TestRepoMetadata_Unauthorized(t *testing.T) { resp, err := clt.GetRepositoryMetadataWithResponse(ctx, repo) require.NoError(t, err) require.NotNil(t, resp.JSON401) - if resp.JSON401.Message != auth.ErrInsufficientPermissions.Error() { + if !strings.Contains(resp.JSON401.Message, auth.ErrInsufficientPermissions.Error()) { t.Errorf("expected error message %q, got %q", auth.ErrInsufficientPermissions.Error(), resp.JSON401.Message) } }) diff --git a/pkg/api/controller_test.go b/pkg/api/controller_test.go index cca384c877c..4ef5ccf8d65 100644 --- a/pkg/api/controller_test.go +++ b/pkg/api/controller_test.go @@ -34,11 +34,13 @@ import ( "github.com/treeverse/lakefs/pkg/api/apigen" "github.com/treeverse/lakefs/pkg/api/apiutil" "github.com/treeverse/lakefs/pkg/auth" + "github.com/treeverse/lakefs/pkg/auth/model" "github.com/treeverse/lakefs/pkg/block" "github.com/treeverse/lakefs/pkg/catalog" "github.com/treeverse/lakefs/pkg/config" "github.com/treeverse/lakefs/pkg/graveler" "github.com/treeverse/lakefs/pkg/httputil" + "github.com/treeverse/lakefs/pkg/permissions" "github.com/treeverse/lakefs/pkg/stats" "github.com/treeverse/lakefs/pkg/testutil" "github.com/treeverse/lakefs/pkg/upload" @@ -5355,6 +5357,152 @@ func TestController_CreateCommitRecord(t *testing.T) { }) } +func TestCheckPermissions_UnpermittedRequests(t *testing.T) { + ctx := context.Background() + testCases := []struct { + name string + node permissions.Node + username string + policies []*model.Policy + expected string + }{ + { + name: "deny single action", + node: permissions.Node{ + Type: permissions.NodeTypeNode, + Permission: permissions.Permission{ + Action: "fs:DeleteRepository", + Resource: "arn:lakefs:fs:::repository/repo1", + }, + }, + username: "user1", + policies: []*model.Policy{ + { + Statement: []model.Statement{ + { + Action: []string{"fs:DeleteRepository"}, + Resource: "arn:lakefs:fs:::repository/repo1", + Effect: model.StatementEffectDeny, + }, + }, + }, + }, + expected: "denied permission to fs:DeleteRepository", + }, + { + name: "deny multiple actions, one concerning the request", + node: permissions.Node{ + Type: permissions.NodeTypeNode, + Permission: permissions.Permission{ + Action: "fs:DeleteRepository", + Resource: "arn:lakefs:fs:::repository/repo1", + }, + }, + username: "user1", + policies: []*model.Policy{ + { + Statement: []model.Statement{ + { + Action: []string{"fs:DeleteRepository", "fs:CreateRepository"}, + Resource: "arn:lakefs:fs:::repository/repo1", + Effect: model.StatementEffectDeny, + }, + }, + }, + }, + expected: "denied permission to fs:DeleteRepository", + }, { + name: "neutral action", + node: permissions.Node{ + Type: permissions.NodeTypeNode, + Permission: permissions.Permission{ + Action: "fs:ReadRepository", + Resource: "arn:lakefs:fs:::repository/repo1", + }, + }, + username: "user1", + policies: []*model.Policy{ + { + Statement: []model.Statement{ + { + Action: []string{"fs:DeleteRepository"}, + Resource: "arn:lakefs:fs:::repository/repo1", + Effect: model.StatementEffectDeny, + }, + }, + }, + }, + expected: "not allowed to fs:ReadRepository", + }, { + name: "nodeAnd no policy, returns first missing one", + node: permissions.Node{ + Type: permissions.NodeTypeAnd, + Nodes: []permissions.Node{ + { + Type: permissions.NodeTypeNode, + Permission: permissions.Permission{ + Action: "fs:CreateRepository", + Resource: "*", + }, + }, + { + Type: permissions.NodeTypeNode, + Permission: permissions.Permission{ + Action: "fs:AttachStorageNamespace", + Resource: "*", + }, + }, + }, + }, + username: "user1", + expected: "not allowed to fs:CreateRepository", + }, { + name: "nodeAnd one policy, returns first missing policy", + node: permissions.Node{ + Type: permissions.NodeTypeAnd, + Nodes: []permissions.Node{ + { + Type: permissions.NodeTypeNode, + Permission: permissions.Permission{ + Action: "fs:CreateRepository", + Resource: "*", + }, + }, + { + Type: permissions.NodeTypeNode, + Permission: permissions.Permission{ + Action: "fs:AttachStorageNamespace", + Resource: "*", + }, + }, + }, + }, + username: "user1", + policies: []*model.Policy{ + { + Statement: []model.Statement{ + { + Action: []string{"fs:CreateRepository"}, + Resource: "*", + Effect: model.StatementEffectAllow, + }, + }, + }, + }, + expected: "not allowed to fs:AttachStorageNamespace", + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + perm := &auth.MissingPermissions{} + result := auth.CheckPermissions(ctx, tc.node, tc.username, tc.policies, perm) + fmt.Println("expected:\n" + tc.expected) + fmt.Println("got:\n" + perm.String()) + fmt.Println(result) + require.Equal(t, tc.expected, perm.String()) + }) + } +} func TestController_CreatePullRequest(t *testing.T) { clt, deps := setupClientWithAdmin(t) ctx := context.Background() diff --git a/pkg/auth/service.go b/pkg/auth/service.go index 8ae9ea5742a..96873a2b714 100644 --- a/pkg/auth/service.go +++ b/pkg/auth/service.go @@ -44,12 +44,20 @@ type AuthorizationResponse struct { Error error } +type MissingPermissions struct { + // Denied is a list of actions the user was denied for the attempt. + Denied []string + // Unauthorized is a list of actions the user did not have for the attempt. + Unauthorized []string +} + // CheckResult - the final result for the authorization is accepted only if it's CheckAllow type CheckResult int const ( - InvalidUserID = "" - MaxPage = 1000 + UserNotAllowed = "not allowed" + InvalidUserID = "" + MaxPage = 1000 // CheckAllow Permission allowed CheckAllow CheckResult = iota // CheckNeutral Permission neither allowed nor denied @@ -918,13 +926,13 @@ func (a *APIAuthService) Authorize(ctx context.Context, req *AuthorizationReques if err != nil { return nil, err } - - allowed := CheckPermissions(ctx, req.RequiredPermissions, req.Username, policies) + permAudit := &MissingPermissions{} + allowed := CheckPermissions(ctx, req.RequiredPermissions, req.Username, policies, permAudit) if allowed != CheckAllow { return &AuthorizationResponse{ Allowed: false, - Error: ErrInsufficientPermissions, + Error: fmt.Errorf("%w: %s", ErrInsufficientPermissions, permAudit.String()), }, nil } @@ -1147,10 +1155,21 @@ func NewAPIAuthServiceWithClient(client ClientWithResponsesInterface, externalPr }, nil } -func CheckPermissions(ctx context.Context, node permissions.Node, username string, policies []*model.Policy) CheckResult { +func (n *MissingPermissions) String() string { + if len(n.Denied) != 0 { + return fmt.Sprintf("denied permission to %s", strings.Join(n.Denied, ",")) + } + if len(n.Unauthorized) != 0 { + return fmt.Sprintf("not allowed to %s", strings.Join(n.Unauthorized, ",")) + } + return UserNotAllowed +} + +func CheckPermissions(ctx context.Context, node permissions.Node, username string, policies []*model.Policy, permAudit *MissingPermissions) CheckResult { allowed := CheckNeutral switch node.Type { case permissions.NodeTypeNode: + hasPermission := false // check whether the permission is allowed, denied or natural (not allowed and not denied) for _, policy := range policies { for _, stmt := range policy.Statement { @@ -1165,13 +1184,17 @@ func CheckPermissions(ctx context.Context, node permissions.Node, username strin if stmt.Effect == model.StatementEffectDeny { // this is a "Deny" and it takes precedence + permAudit.Denied = append(permAudit.Denied, action) return CheckDeny } - + hasPermission = true allowed = CheckAllow } } } + if !hasPermission { + permAudit.Unauthorized = append(permAudit.Unauthorized, node.Permission.Action) + } case permissions.NodeTypeOr: // returns: @@ -1179,7 +1202,7 @@ func CheckPermissions(ctx context.Context, node permissions.Node, username strin // Denied - one of the permissions is Deny // Natural - otherwise for _, node := range node.Nodes { - result := CheckPermissions(ctx, node, username, policies) + result := CheckPermissions(ctx, node, username, policies, permAudit) if result == CheckDeny { return CheckDeny } @@ -1194,7 +1217,7 @@ func CheckPermissions(ctx context.Context, node permissions.Node, username strin // Denied - one of the permissions is Deny // Natural - otherwise for _, node := range node.Nodes { - result := CheckPermissions(ctx, node, username, policies) + result := CheckPermissions(ctx, node, username, policies, permAudit) if result == CheckNeutral || result == CheckDeny { return result }