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

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 11 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
4 changes: 2 additions & 2 deletions contrib/auth/acl/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -895,8 +895,8 @@ 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)
perm := &auth.NeededPermissions{}
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, Im confused what are you doing with this variable?

allowed := auth.CheckPermissions(ctx, req.RequiredPermissions, req.Username, policies, perm)

if allowed != auth.CheckAllow {
return &auth.AuthorizationResponse{
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
151 changes: 151 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,155 @@ 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 from:\nfs:DeleteRepository",
}, /////////////////////////////////////////////////////////////////
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Please avoid adding filler comments. They are pleasant for different people at different widths, they are never consistent, and they end up not appearing everywhere.

Suggested change
}, /////////////////////////////////////////////////////////////////
{
}, {

name: "deny multiple actions",
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 from:\nfs:DeleteRepository\nfs:CreateRepository",
}, /////////////////////////////////////////////////////////////////
{
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: "lacking permissions for:\nfs:ReadRepository",
}, /////////////////////////////////////////////////////////////////
{
name: "node and no 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",
expected: "lacking permissions for:\nfs:CreateRepository",
Copy link
Contributor

Choose a reason for hiding this comment

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

I would expect it to report both missing permissions. Reporting only one is fine, but then the message is surprising - it says "permissions" when it only ever means a single permissions.

},
{
name: "node and one 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: "lacking permissions for:\nfs:AttachStorageNamespace",
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
perm := &auth.NeededPermissions{}
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
48 changes: 40 additions & 8 deletions pkg/auth/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,11 @@ type AuthorizationResponse struct {
Error error
}

type NeededPermissions struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

rename, MissingPermissions or PermissionsAudit or DenyHistory w/e would be more appropriate?

Denied []model.Statement
Unauthorized []permissions.Node
Copy link
Contributor

Choose a reason for hiding this comment

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

These are a bit too informative IMO. Let's just keep the action name here? Otherwise I learn details of the policy including denied paths that I might not have known existed!

Suggested change
Denied []model.Statement
Unauthorized []permissions.Node
// 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

Expand Down Expand Up @@ -918,13 +923,13 @@ func (a *APIAuthService) Authorize(ctx context.Context, req *AuthorizationReques
if err != nil {
return nil, err
}

allowed := CheckPermissions(ctx, req.RequiredPermissions, req.Username, policies)
perm := &NeededPermissions{}
allowed := CheckPermissions(ctx, req.RequiredPermissions, req.Username, policies, perm)

if allowed != CheckAllow {
return &AuthorizationResponse{
Allowed: false,
Error: ErrInsufficientPermissions,
Error: fmt.Errorf("%w\n%s", ErrInsufficientPermissions, perm.String()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the results in the tests, I prefer without the newline, error messages are rarely >1 line.

Suggested change
Error: fmt.Errorf("%w\n%s", ErrInsufficientPermissions, perm.String()),
Error: fmt.Errorf("%w: %s", ErrInsufficientPermissions, perm.String()),

}, nil
}

Expand Down Expand Up @@ -1147,10 +1152,32 @@ func NewAPIAuthServiceWithClient(client ClientWithResponsesInterface, externalPr
}, nil
}

func CheckPermissions(ctx context.Context, node permissions.Node, username string, policies []*model.Policy) CheckResult {
func (n *NeededPermissions) String() string {
w := &strings.Builder{}
if len(n.Denied) != 0 {
fmt.Fprintln(w, "denied from:")
for _, statement := range n.Denied {
for _, action := range statement.Action {
fmt.Fprintln(w, action)
}
}
return strings.TrimSpace(w.String())
Copy link
Contributor

Choose a reason for hiding this comment

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

It is quite rare to need to TrimSpace on a string you created.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you don't even need a strings.Builder here:

Suggested change
fmt.Fprintln(w, "denied from:")
for _, statement := range n.Denied {
for _, action := range statement.Action {
fmt.Fprintln(w, action)
}
}
return strings.TrimSpace(w.String())
return fmt.Sprintf("denied permission to %s", strings.Join(n.Denied, ", "))

The same of course in the next section.

}
if len(n.Unauthorized) != 0 {
fmt.Fprintln(w, "lacking permissions for:")
for _, node := range n.Unauthorized {
fmt.Fprintln(w, node.Permission.Action)
}
return strings.TrimSpace(w.String())
}
return strings.TrimSpace(w.String())
Copy link
Contributor

Choose a reason for hiding this comment

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

This returns "". If that is not helpful, please return some constant string like "not allowed".

}

func CheckPermissions(ctx context.Context, node permissions.Node, username string, policies []*model.Policy, perm *NeededPermissions) CheckResult {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: variable name perm is too generic.
Can you use something more explicit?
for example: audit, denyAudit, permAudit, history etc...
So that in the future when extending, debugging or just reading the code we can immediately understand what it is used for.

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 +1192,26 @@ func CheckPermissions(ctx context.Context, node permissions.Node, username strin

if stmt.Effect == model.StatementEffectDeny {
// this is a "Deny" and it takes precedence
perm.Denied = append(perm.Denied, stmt)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is confusing: I take perm as an input, now I modify it and return it. So I've changed my input, but also returned it as an output.

I think either make perm into an object, pass it by pointer, and keep state on it, or explicitly handle the lists.

Copy link
Contributor

Choose a reason for hiding this comment

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

Was thinking the same, agree with @arielshaqed 100%

return CheckDeny
} else {
hasPermission = true
allowed = CheckAllow
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
} else {
hasPermission = true
allowed = CheckAllow
hasPermission = true
allowed = CheckAllow

is closer to the previous version.

We prefer to use an "early return" style: errors return immediately - like l. 1188 does; successes keep going with no "else" and no need to indent.

}

allowed = CheckAllow
}
}
}
if !hasPermission {
perm.Unauthorized = append(perm.Unauthorized, node)
}

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, perm)
if result == CheckDeny {
return CheckDeny
}
Expand All @@ -1194,7 +1226,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, perm)
if result == CheckNeutral || result == CheckDeny {
return result
}
Expand Down
Loading