Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

8224 report required permissions when authorization fails #8314

Merged
merged 18 commits into from
Nov 12, 2024
Merged

8224 report required permissions when authorization fails #8314

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
f1c6393
deny works
ItamarYuran Oct 28, 2024
88175ae
reporting missing permissions
ItamarYuran Oct 28, 2024
2242218
works
ItamarYuran Oct 28, 2024
b1f9e3a
fixed errors
ItamarYuran Oct 28, 2024
11aa10a
fixed esti
ItamarYuran Oct 28, 2024
ee2986a
enhanced, very enhanced
ItamarYuran Oct 29, 2024
d04f675
crossing fingaz
ItamarYuran Oct 29, 2024
e883b3c
using string.contains
ItamarYuran Oct 29, 2024
5da4f66
with changes no tests
ItamarYuran Oct 31, 2024
54dc6aa
service
ItamarYuran Nov 3, 2024
743f818
yesh tests
ItamarYuran Nov 3, 2024
4a054c6
basic changes
ItamarYuran Nov 4, 2024
803d4a3
minor changes
ItamarYuran Nov 4, 2024
758d910
simpler, returning first missing
ItamarYuran Nov 5, 2024
7b304cc
simpler, no mock
ItamarYuran Nov 5, 2024
7992c4b
final tunings
ItamarYuran Nov 10, 2024
060083f
no empty lines
ItamarYuran Nov 10, 2024
2c50f4a
now using parm
ItamarYuran Nov 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions contrib/auth/acl/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
9 changes: 5 additions & 4 deletions esti/auth_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"net/http"
"slices"
"strings"
"testing"
"time"

Expand Down Expand Up @@ -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()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we test here (and below) that we got an expected value? Obviously doing so makes sense only after we finalize the message contents. So it is fine to open an issue to improve these checks in this test file.

t.Fatalf("expected error message %q, got %q", auth.ErrInsufficientPermissions.Error(), resp.JSON401.Message)
}
}
Expand All @@ -255,15 +256,15 @@ 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)
}
})
t.Run("delete", func(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)
}
})
Expand All @@ -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)
}
})
Expand Down
148 changes: 148 additions & 0 deletions pkg/api/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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()
Expand Down
41 changes: 32 additions & 9 deletions pkg/auth/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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 {
Expand All @@ -1165,21 +1184,25 @@ 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:
// Allowed - at least one of the permissions is allowed and no one is denied
// 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
}
Expand All @@ -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
}
Expand Down
Loading