From f1c6393a0322ec995946e5e5e0ecd1b075931523 Mon Sep 17 00:00:00 2001 From: Itamar Yuran Date: Mon, 28 Oct 2024 13:35:21 +0200 Subject: [PATCH 01/15] deny works --- contrib/auth/acl/service.go | 4 +-- pkg/auth/service.go | 54 ++++++++++++++++++++++++++++--------- 2 files changed, 44 insertions(+), 14 deletions(-) diff --git a/contrib/auth/acl/service.go b/contrib/auth/acl/service.go index 5de25352da3..bb7c5e4abf6 100644 --- a/contrib/auth/acl/service.go +++ b/contrib/auth/acl/service.go @@ -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{} + allowed, _ := auth.CheckPermissions(ctx, req.RequiredPermissions, req.Username, policies, perm) if allowed != auth.CheckAllow { return &auth.AuthorizationResponse{ diff --git a/pkg/auth/service.go b/pkg/auth/service.go index 8ae9ea5742a..17873b4cc5b 100644 --- a/pkg/auth/service.go +++ b/pkg/auth/service.go @@ -12,6 +12,7 @@ package auth import ( "context" + "errors" "fmt" "net/http" "strconv" @@ -918,13 +919,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, permissions := CheckPermissions(ctx, req.RequiredPermissions, req.Username, policies, perm) if allowed != CheckAllow { return &AuthorizationResponse{ Allowed: false, - Error: ErrInsufficientPermissions, + Error: errors.New(permissions.String()), }, nil } @@ -1147,7 +1148,30 @@ func NewAPIAuthServiceWithClient(client ClientWithResponsesInterface, externalPr }, nil } -func CheckPermissions(ctx context.Context, node permissions.Node, username string, policies []*model.Policy) CheckResult { +type NeededPermissions struct { + Denied []model.Statement + Unauthorized []model.Statement +} + +func (n *NeededPermissions) String() string { + if len(n.Denied) != 0 { + deniedStr := "denied from:\n" + for _, statement := range n.Denied { + deniedStr += strings.Join(statement.Action, "\n") + } + return deniedStr + } + if len(n.Unauthorized) != 0 { + permStr := "lacking permissions for:\n" + for _, statement := range n.Unauthorized { + permStr += strings.Join(statement.Action, "\n") + } + return permStr + } + return "--____" +} + +func CheckPermissions(ctx context.Context, node permissions.Node, username string, policies []*model.Policy, perm *NeededPermissions) (CheckResult, *NeededPermissions) { allowed := CheckNeutral switch node.Type { case permissions.NodeTypeNode: @@ -1161,11 +1185,17 @@ func CheckPermissions(ctx context.Context, node permissions.Node, username strin for _, action := range stmt.Action { if !wildcard.Match(action, node.Permission.Action) { continue // not a matching action + } else { + perm.Unauthorized = append(perm.Unauthorized, stmt) + + perm.Denied = append(perm.Denied, stmt) + fmt.Println("hihiihi") } if stmt.Effect == model.StatementEffectDeny { // this is a "Deny" and it takes precedence - return CheckDeny + perm.Denied = append(perm.Denied, stmt) + return CheckDeny, perm } allowed = CheckAllow @@ -1179,9 +1209,9 @@ 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, perm := CheckPermissions(ctx, node, username, policies, perm) if result == CheckDeny { - return CheckDeny + return CheckDeny, perm } if allowed != CheckAllow { allowed = result @@ -1194,18 +1224,18 @@ 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, perm := CheckPermissions(ctx, node, username, policies, perm) if result == CheckNeutral || result == CheckDeny { - return result + return result, perm } } - return CheckAllow + return CheckAllow, perm default: logging.FromContext(ctx).Error("unknown permission node type") - return CheckDeny + return CheckDeny, perm } - return allowed + return allowed, perm } func interpolateUser(resource string, username string) string { From 88175ae8af7129e2c194b13020adb78ffd5134af Mon Sep 17 00:00:00 2001 From: Itamar Yuran Date: Mon, 28 Oct 2024 14:06:00 +0200 Subject: [PATCH 02/15] reporting missing permissions --- pkg/auth/service.go | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/pkg/auth/service.go b/pkg/auth/service.go index 17873b4cc5b..bc69130675e 100644 --- a/pkg/auth/service.go +++ b/pkg/auth/service.go @@ -45,6 +45,11 @@ type AuthorizationResponse struct { Error error } +type NeededPermissions struct { + Denied []model.Statement + Unauthorized []permissions.Node +} + // CheckResult - the final result for the authorization is accepted only if it's CheckAllow type CheckResult int @@ -1148,11 +1153,6 @@ func NewAPIAuthServiceWithClient(client ClientWithResponsesInterface, externalPr }, nil } -type NeededPermissions struct { - Denied []model.Statement - Unauthorized []model.Statement -} - func (n *NeededPermissions) String() string { if len(n.Denied) != 0 { deniedStr := "denied from:\n" @@ -1163,16 +1163,17 @@ func (n *NeededPermissions) String() string { } if len(n.Unauthorized) != 0 { permStr := "lacking permissions for:\n" - for _, statement := range n.Unauthorized { - permStr += strings.Join(statement.Action, "\n") + for _, node := range n.Unauthorized { + permStr += node.Permission.Action } return permStr } - return "--____" + return "" } func CheckPermissions(ctx context.Context, node permissions.Node, username string, policies []*model.Policy, perm *NeededPermissions) (CheckResult, *NeededPermissions) { allowed := CheckNeutral + hasPermission := false switch node.Type { case permissions.NodeTypeNode: // check whether the permission is allowed, denied or natural (not allowed and not denied) @@ -1185,23 +1186,24 @@ func CheckPermissions(ctx context.Context, node permissions.Node, username strin for _, action := range stmt.Action { if !wildcard.Match(action, node.Permission.Action) { continue // not a matching action - } else { - perm.Unauthorized = append(perm.Unauthorized, stmt) - - perm.Denied = append(perm.Denied, stmt) - fmt.Println("hihiihi") } if stmt.Effect == model.StatementEffectDeny { // this is a "Deny" and it takes precedence perm.Denied = append(perm.Denied, stmt) return CheckDeny, perm + } else { + hasPermission = true + allowed = CheckAllow } allowed = CheckAllow } } } + if !hasPermission { + perm.Unauthorized = append(perm.Unauthorized, node) + } case permissions.NodeTypeOr: // returns: From 224221855b054215b7e8dcaef7081e81753e686c Mon Sep 17 00:00:00 2001 From: Itamar Yuran Date: Mon, 28 Oct 2024 14:14:37 +0200 Subject: [PATCH 03/15] works --- pkg/auth/service.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/pkg/auth/service.go b/pkg/auth/service.go index bc69130675e..4d6a392c2e3 100644 --- a/pkg/auth/service.go +++ b/pkg/auth/service.go @@ -1196,8 +1196,6 @@ func CheckPermissions(ctx context.Context, node permissions.Node, username strin hasPermission = true allowed = CheckAllow } - - allowed = CheckAllow } } } From b1f9e3a5cb6205aac8227b3c371d5997b148ef59 Mon Sep 17 00:00:00 2001 From: Itamar Yuran Date: Mon, 28 Oct 2024 17:28:31 +0200 Subject: [PATCH 04/15] fixed errors --- pkg/auth/service.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/auth/service.go b/pkg/auth/service.go index 4d6a392c2e3..ca8fce5150f 100644 --- a/pkg/auth/service.go +++ b/pkg/auth/service.go @@ -12,7 +12,6 @@ package auth import ( "context" - "errors" "fmt" "net/http" "strconv" @@ -930,7 +929,7 @@ func (a *APIAuthService) Authorize(ctx context.Context, req *AuthorizationReques if allowed != CheckAllow { return &AuthorizationResponse{ Allowed: false, - Error: errors.New(permissions.String()), + Error: fmt.Errorf("%w\n%s", ErrInsufficientPermissions, permissions.String()), }, nil } From 11aa10a24eb039e53e48e61ff32609f154a9d140 Mon Sep 17 00:00:00 2001 From: Itamar Yuran Date: Mon, 28 Oct 2024 18:22:52 +0200 Subject: [PATCH 05/15] fixed esti --- esti/auth_test.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/esti/auth_test.go b/esti/auth_test.go index dcbc7bdccca..6866271269e 100644 --- a/esti/auth_test.go +++ b/esti/auth_test.go @@ -2,6 +2,7 @@ package esti import ( "context" + "errors" "net/http" "slices" "testing" @@ -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 !errors.Is(err, auth.ErrInsufficientPermissions) { 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 !errors.Is(err, auth.ErrInsufficientPermissions) { 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 !errors.Is(err, auth.ErrInsufficientPermissions) { 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 !errors.Is(err, auth.ErrInsufficientPermissions) { t.Errorf("expected error message %q, got %q", auth.ErrInsufficientPermissions.Error(), resp.JSON401.Message) } }) From ee2986a5d1b55db961057931925df770c265bee0 Mon Sep 17 00:00:00 2001 From: Itamar Yuran Date: Tue, 29 Oct 2024 12:38:17 +0200 Subject: [PATCH 06/15] enhanced, very enhanced --- esti/auth_test.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/esti/auth_test.go b/esti/auth_test.go index 6866271269e..87f296976a1 100644 --- a/esti/auth_test.go +++ b/esti/auth_test.go @@ -235,7 +235,8 @@ func TestCreateRepo_Unauthorized(t *testing.T) { }) require.NoError(t, err) require.NotNil(t, resp.JSON401) - if !errors.Is(err, auth.ErrInsufficientPermissions) { + var authErr error + if errors.As(err, &authErr) && !errors.Is(authErr, auth.ErrInsufficientPermissions) { t.Fatalf("expected error message %q, got %q", auth.ErrInsufficientPermissions.Error(), resp.JSON401.Message) } } @@ -256,7 +257,8 @@ func TestRepoMetadata_Unauthorized(t *testing.T) { }) require.NoError(t, err) require.NotNil(t, resp.JSON401) - if !errors.Is(err, auth.ErrInsufficientPermissions) { + var authErr error + if errors.As(err, &authErr) && !errors.Is(authErr, auth.ErrInsufficientPermissions) { t.Errorf("expected error message %q, got %q", auth.ErrInsufficientPermissions.Error(), resp.JSON401.Message) } }) @@ -264,7 +266,8 @@ 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 !errors.Is(err, auth.ErrInsufficientPermissions) { + var authErr error + if errors.As(err, &authErr) && !errors.Is(authErr, auth.ErrInsufficientPermissions) { t.Errorf("expected error message %q, got %q", auth.ErrInsufficientPermissions.Error(), resp.JSON401.Message) } }) @@ -273,7 +276,8 @@ func TestRepoMetadata_Unauthorized(t *testing.T) { resp, err := clt.GetRepositoryMetadataWithResponse(ctx, repo) require.NoError(t, err) require.NotNil(t, resp.JSON401) - if !errors.Is(err, auth.ErrInsufficientPermissions) { + var authErr error + if errors.As(err, &authErr) && !errors.Is(authErr, auth.ErrInsufficientPermissions) { t.Errorf("expected error message %q, got %q", auth.ErrInsufficientPermissions.Error(), resp.JSON401.Message) } }) From d04f675a3f66266372c029aabe00f9ef1116f698 Mon Sep 17 00:00:00 2001 From: Itamar Yuran Date: Tue, 29 Oct 2024 13:41:21 +0200 Subject: [PATCH 07/15] crossing fingaz --- esti/auth_test.go | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/esti/auth_test.go b/esti/auth_test.go index 87f296976a1..840923315ab 100644 --- a/esti/auth_test.go +++ b/esti/auth_test.go @@ -235,8 +235,7 @@ func TestCreateRepo_Unauthorized(t *testing.T) { }) require.NoError(t, err) require.NotNil(t, resp.JSON401) - var authErr error - if errors.As(err, &authErr) && !errors.Is(authErr, auth.ErrInsufficientPermissions) { + if !errors.Is(errors.New(resp.JSON401.Message), auth.ErrInsufficientPermissions) { t.Fatalf("expected error message %q, got %q", auth.ErrInsufficientPermissions.Error(), resp.JSON401.Message) } } @@ -257,8 +256,7 @@ func TestRepoMetadata_Unauthorized(t *testing.T) { }) require.NoError(t, err) require.NotNil(t, resp.JSON401) - var authErr error - if errors.As(err, &authErr) && !errors.Is(authErr, auth.ErrInsufficientPermissions) { + if !errors.Is(errors.New(resp.JSON401.Message), auth.ErrInsufficientPermissions) { t.Errorf("expected error message %q, got %q", auth.ErrInsufficientPermissions.Error(), resp.JSON401.Message) } }) @@ -266,8 +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) - var authErr error - if errors.As(err, &authErr) && !errors.Is(authErr, auth.ErrInsufficientPermissions) { + if !errors.Is(errors.New(resp.JSON401.Message), auth.ErrInsufficientPermissions) { t.Errorf("expected error message %q, got %q", auth.ErrInsufficientPermissions.Error(), resp.JSON401.Message) } }) @@ -276,8 +273,7 @@ func TestRepoMetadata_Unauthorized(t *testing.T) { resp, err := clt.GetRepositoryMetadataWithResponse(ctx, repo) require.NoError(t, err) require.NotNil(t, resp.JSON401) - var authErr error - if errors.As(err, &authErr) && !errors.Is(authErr, auth.ErrInsufficientPermissions) { + if !errors.Is(errors.New(resp.JSON401.Message), auth.ErrInsufficientPermissions) { t.Errorf("expected error message %q, got %q", auth.ErrInsufficientPermissions.Error(), resp.JSON401.Message) } }) From e883b3c881471577c9d31e0557517cad747e2e56 Mon Sep 17 00:00:00 2001 From: Itamar Yuran Date: Tue, 29 Oct 2024 13:52:29 +0200 Subject: [PATCH 08/15] using string.contains --- esti/auth_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/esti/auth_test.go b/esti/auth_test.go index 840923315ab..9cc40421f9e 100644 --- a/esti/auth_test.go +++ b/esti/auth_test.go @@ -2,9 +2,9 @@ package esti import ( "context" - "errors" "net/http" "slices" + "strings" "testing" "time" @@ -235,7 +235,7 @@ func TestCreateRepo_Unauthorized(t *testing.T) { }) require.NoError(t, err) require.NotNil(t, resp.JSON401) - if !errors.Is(errors.New(resp.JSON401.Message), auth.ErrInsufficientPermissions) { + if !strings.Contains(resp.JSON401.Message, auth.ErrInsufficientPermissions.Error()) { t.Fatalf("expected error message %q, got %q", auth.ErrInsufficientPermissions.Error(), resp.JSON401.Message) } } @@ -256,7 +256,7 @@ func TestRepoMetadata_Unauthorized(t *testing.T) { }) require.NoError(t, err) require.NotNil(t, resp.JSON401) - if !errors.Is(errors.New(resp.JSON401.Message), auth.ErrInsufficientPermissions) { + if !strings.Contains(resp.JSON401.Message, auth.ErrInsufficientPermissions.Error()) { t.Errorf("expected error message %q, got %q", auth.ErrInsufficientPermissions.Error(), resp.JSON401.Message) } }) @@ -264,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 !errors.Is(errors.New(resp.JSON401.Message), auth.ErrInsufficientPermissions) { + if !strings.Contains(resp.JSON401.Message, auth.ErrInsufficientPermissions.Error()) { t.Errorf("expected error message %q, got %q", auth.ErrInsufficientPermissions.Error(), resp.JSON401.Message) } }) @@ -273,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 !errors.Is(errors.New(resp.JSON401.Message), auth.ErrInsufficientPermissions) { + if !strings.Contains(resp.JSON401.Message, auth.ErrInsufficientPermissions.Error()) { t.Errorf("expected error message %q, got %q", auth.ErrInsufficientPermissions.Error(), resp.JSON401.Message) } }) From 5da4f6620a2b88b816a683c9e3282387f51deb0a Mon Sep 17 00:00:00 2001 From: Itamar Yuran Date: Thu, 31 Oct 2024 14:07:01 +0200 Subject: [PATCH 09/15] with changes no tests --- contrib/auth/acl/service.go | 2 +- pkg/auth/service.go | 39 +++++++++++++++++++------------------ 2 files changed, 21 insertions(+), 20 deletions(-) diff --git a/contrib/auth/acl/service.go b/contrib/auth/acl/service.go index bb7c5e4abf6..2e037aa51ac 100644 --- a/contrib/auth/acl/service.go +++ b/contrib/auth/acl/service.go @@ -896,7 +896,7 @@ func (s *AuthService) Authorize(ctx context.Context, req *auth.AuthorizationRequ return nil, err } perm := &auth.NeededPermissions{} - allowed, _ := auth.CheckPermissions(ctx, req.RequiredPermissions, req.Username, policies, perm) + allowed := auth.CheckPermissions(ctx, req.RequiredPermissions, req.Username, policies, perm) if allowed != auth.CheckAllow { return &auth.AuthorizationResponse{ diff --git a/pkg/auth/service.go b/pkg/auth/service.go index ca8fce5150f..fa0a286734b 100644 --- a/pkg/auth/service.go +++ b/pkg/auth/service.go @@ -924,12 +924,12 @@ func (a *APIAuthService) Authorize(ctx context.Context, req *AuthorizationReques return nil, err } perm := &NeededPermissions{} - allowed, permissions := CheckPermissions(ctx, req.RequiredPermissions, req.Username, policies, perm) + allowed := CheckPermissions(ctx, req.RequiredPermissions, req.Username, policies, perm) if allowed != CheckAllow { return &AuthorizationResponse{ Allowed: false, - Error: fmt.Errorf("%w\n%s", ErrInsufficientPermissions, permissions.String()), + Error: fmt.Errorf("%w\n%s", ErrInsufficientPermissions, perm.String()), }, nil } @@ -1153,28 +1153,29 @@ func NewAPIAuthServiceWithClient(client ClientWithResponsesInterface, externalPr } func (n *NeededPermissions) String() string { + w := &strings.Builder{} if len(n.Denied) != 0 { - deniedStr := "denied from:\n" + fmt.Fprintln(w, "denied from:") for _, statement := range n.Denied { - deniedStr += strings.Join(statement.Action, "\n") + for _, action := range statement.Action { + fmt.Fprintln(w, action) + } } - return deniedStr } if len(n.Unauthorized) != 0 { - permStr := "lacking permissions for:\n" + fmt.Fprintln(w, "lacking permissions for:") for _, node := range n.Unauthorized { - permStr += node.Permission.Action + fmt.Fprintln(w, node.Permission.Action) } - return permStr } - return "" + return strings.TrimSpace(w.String()) } -func CheckPermissions(ctx context.Context, node permissions.Node, username string, policies []*model.Policy, perm *NeededPermissions) (CheckResult, *NeededPermissions) { +func CheckPermissions(ctx context.Context, node permissions.Node, username string, policies []*model.Policy, perm *NeededPermissions) CheckResult { allowed := CheckNeutral - hasPermission := false 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 { @@ -1190,7 +1191,7 @@ 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) - return CheckDeny, perm + return CheckDeny } else { hasPermission = true allowed = CheckAllow @@ -1208,9 +1209,9 @@ 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, perm := CheckPermissions(ctx, node, username, policies, perm) + result := CheckPermissions(ctx, node, username, policies, perm) if result == CheckDeny { - return CheckDeny, perm + return CheckDeny } if allowed != CheckAllow { allowed = result @@ -1223,18 +1224,18 @@ 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, perm := CheckPermissions(ctx, node, username, policies, perm) + result := CheckPermissions(ctx, node, username, policies, perm) if result == CheckNeutral || result == CheckDeny { - return result, perm + return result } } - return CheckAllow, perm + return CheckAllow default: logging.FromContext(ctx).Error("unknown permission node type") - return CheckDeny, perm + return CheckDeny } - return allowed, perm + return allowed } func interpolateUser(resource string, username string) string { From 54dc6aa7ff41d55e14b27944afd14aac9cfaaa96 Mon Sep 17 00:00:00 2001 From: Itamar Yuran Date: Sun, 3 Nov 2024 16:10:41 +0200 Subject: [PATCH 10/15] service --- pkg/auth/service.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/auth/service.go b/pkg/auth/service.go index fa0a286734b..605f607361b 100644 --- a/pkg/auth/service.go +++ b/pkg/auth/service.go @@ -1161,12 +1161,14 @@ func (n *NeededPermissions) String() string { fmt.Fprintln(w, action) } } + return strings.TrimSpace(w.String()) } 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()) } From 743f81822b6287b9c4c91b5c179bfa2f6ce8c60b Mon Sep 17 00:00:00 2001 From: Itamar Yuran Date: Sun, 3 Nov 2024 16:24:42 +0200 Subject: [PATCH 11/15] yesh tests --- pkg/api/controller_test.go | 151 +++++++++++++++++++++++++++++++++++++ 1 file changed, 151 insertions(+) diff --git a/pkg/api/controller_test.go b/pkg/api/controller_test.go index 78135dc1c4b..5b6798e90c9 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,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", + }, ///////////////////////////////////////////////////////////////// + { + 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", + }, + { + 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() From 4a054c66d4215616d255774d269dbed5110f69e0 Mon Sep 17 00:00:00 2001 From: Itamar Yuran Date: Mon, 4 Nov 2024 12:36:51 +0200 Subject: [PATCH 12/15] basic changes --- pkg/auth/service.go | 34 +++++++++++++--------------------- 1 file changed, 13 insertions(+), 21 deletions(-) diff --git a/pkg/auth/service.go b/pkg/auth/service.go index 605f607361b..16dbbedf108 100644 --- a/pkg/auth/service.go +++ b/pkg/auth/service.go @@ -45,16 +45,19 @@ type AuthorizationResponse struct { } type NeededPermissions struct { - 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 const ( - InvalidUserID = "" - MaxPage = 1000 + UserNotAllowed = "not allowed" + InvalidUserID = "" + MaxPage = 1000 // CheckAllow Permission allowed CheckAllow CheckResult = iota // CheckNeutral Permission neither allowed nor denied @@ -929,7 +932,7 @@ func (a *APIAuthService) Authorize(ctx context.Context, req *AuthorizationReques if allowed != CheckAllow { return &AuthorizationResponse{ Allowed: false, - Error: fmt.Errorf("%w\n%s", ErrInsufficientPermissions, perm.String()), + Error: fmt.Errorf("%w: %s", ErrInsufficientPermissions, perm.String()), }, nil } @@ -1153,24 +1156,13 @@ func NewAPIAuthServiceWithClient(client ClientWithResponsesInterface, externalPr } 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()) + return fmt.Sprintf("denied permissions to %s", strings.Join(n.Denied, ",")) } 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 fmt.Sprintf("denied permissions to %s", strings.Join(n.Unauthorized, ",")) } - return strings.TrimSpace(w.String()) + return UserNotAllowed } func CheckPermissions(ctx context.Context, node permissions.Node, username string, policies []*model.Policy, perm *NeededPermissions) CheckResult { @@ -1192,7 +1184,7 @@ 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) + perm.Denied = append(perm.Denied, action) return CheckDeny } else { hasPermission = true @@ -1202,7 +1194,7 @@ func CheckPermissions(ctx context.Context, node permissions.Node, username strin } } if !hasPermission { - perm.Unauthorized = append(perm.Unauthorized, node) + perm.Unauthorized = append(perm.Unauthorized, node.Permission.Action) } case permissions.NodeTypeOr: From 803d4a38cc2e859db99f6136eb52cc6960a4592a Mon Sep 17 00:00:00 2001 From: Itamar Yuran Date: Mon, 4 Nov 2024 16:43:42 +0200 Subject: [PATCH 13/15] minor changes --- contrib/auth/acl/service.go | 2 +- pkg/auth/gomock_reflect_4293290132/prog.go | 66 ++++++++++++++++++++++ pkg/auth/service.go | 22 ++++---- 3 files changed, 78 insertions(+), 12 deletions(-) create mode 100644 pkg/auth/gomock_reflect_4293290132/prog.go diff --git a/contrib/auth/acl/service.go b/contrib/auth/acl/service.go index 2e037aa51ac..908e4e03b65 100644 --- a/contrib/auth/acl/service.go +++ b/contrib/auth/acl/service.go @@ -895,7 +895,7 @@ func (s *AuthService) Authorize(ctx context.Context, req *auth.AuthorizationRequ if err != nil { return nil, err } - perm := &auth.NeededPermissions{} + perm := &auth.MissingPermissions{} allowed := auth.CheckPermissions(ctx, req.RequiredPermissions, req.Username, policies, perm) if allowed != auth.CheckAllow { diff --git a/pkg/auth/gomock_reflect_4293290132/prog.go b/pkg/auth/gomock_reflect_4293290132/prog.go new file mode 100644 index 00000000000..fc096224037 --- /dev/null +++ b/pkg/auth/gomock_reflect_4293290132/prog.go @@ -0,0 +1,66 @@ + +package main + +import ( + "encoding/gob" + "flag" + "fmt" + "os" + "path" + "reflect" + + "github.com/golang/mock/mockgen/model" + + pkg_ "github.com/treeverse/lakefs/pkg/auth" +) + +var output = flag.String("output", "", "The output file name, or empty to use stdout.") + +func main() { + flag.Parse() + + its := []struct{ + sym string + typ reflect.Type + }{ + + { "ClientWithResponsesInterface", reflect.TypeOf((*pkg_.ClientWithResponsesInterface)(nil)).Elem()}, + + } + pkg := &model.Package{ + // NOTE: This behaves contrary to documented behaviour if the + // package name is not the final component of the import path. + // The reflect package doesn't expose the package name, though. + Name: path.Base("github.com/treeverse/lakefs/pkg/auth"), + } + + for _, it := range its { + intf, err := model.InterfaceFromInterfaceType(it.typ) + if err != nil { + fmt.Fprintf(os.Stderr, "Reflection: %v\n", err) + os.Exit(1) + } + intf.Name = it.sym + pkg.Interfaces = append(pkg.Interfaces, intf) + } + + outfile := os.Stdout + if len(*output) != 0 { + var err error + outfile, err = os.Create(*output) + if err != nil { + fmt.Fprintf(os.Stderr, "failed to open output file %q", *output) + } + defer func() { + if err := outfile.Close(); err != nil { + fmt.Fprintf(os.Stderr, "failed to close output file %q", *output) + os.Exit(1) + } + }() + } + + if err := gob.NewEncoder(outfile).Encode(pkg); err != nil { + fmt.Fprintf(os.Stderr, "gob encode: %v\n", err) + os.Exit(1) + } +} diff --git a/pkg/auth/service.go b/pkg/auth/service.go index 16dbbedf108..c49fe3e41ed 100644 --- a/pkg/auth/service.go +++ b/pkg/auth/service.go @@ -44,7 +44,7 @@ type AuthorizationResponse struct { Error error } -type NeededPermissions struct { +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. @@ -926,13 +926,13 @@ func (a *APIAuthService) Authorize(ctx context.Context, req *AuthorizationReques if err != nil { return nil, err } - perm := &NeededPermissions{} - allowed := CheckPermissions(ctx, req.RequiredPermissions, req.Username, policies, perm) + permAudit := &MissingPermissions{} + allowed := CheckPermissions(ctx, req.RequiredPermissions, req.Username, policies, permAudit) if allowed != CheckAllow { return &AuthorizationResponse{ Allowed: false, - Error: fmt.Errorf("%w: %s", ErrInsufficientPermissions, perm.String()), + Error: fmt.Errorf("%w: %s", ErrInsufficientPermissions, permAudit.String()), }, nil } @@ -1155,17 +1155,17 @@ func NewAPIAuthServiceWithClient(client ClientWithResponsesInterface, externalPr }, nil } -func (n *NeededPermissions) String() string { +func (n *MissingPermissions) String() string { if len(n.Denied) != 0 { return fmt.Sprintf("denied permissions to %s", strings.Join(n.Denied, ",")) } if len(n.Unauthorized) != 0 { - return fmt.Sprintf("denied permissions to %s", strings.Join(n.Unauthorized, ",")) + return fmt.Sprintf("missing permission to %s", strings.Join(n.Unauthorized, ",")) } return UserNotAllowed } -func CheckPermissions(ctx context.Context, node permissions.Node, username string, policies []*model.Policy, perm *NeededPermissions) CheckResult { +func CheckPermissions(ctx context.Context, node permissions.Node, username string, policies []*model.Policy, permAudit *MissingPermissions) CheckResult { allowed := CheckNeutral switch node.Type { case permissions.NodeTypeNode: @@ -1184,7 +1184,7 @@ 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, action) + permAudit.Denied = append(permAudit.Denied, action) return CheckDeny } else { hasPermission = true @@ -1194,7 +1194,7 @@ func CheckPermissions(ctx context.Context, node permissions.Node, username strin } } if !hasPermission { - perm.Unauthorized = append(perm.Unauthorized, node.Permission.Action) + permAudit.Unauthorized = append(permAudit.Unauthorized, node.Permission.Action) } case permissions.NodeTypeOr: @@ -1203,7 +1203,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, perm) + result := CheckPermissions(ctx, node, username, policies, permAudit) if result == CheckDeny { return CheckDeny } @@ -1218,7 +1218,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, perm) + result := CheckPermissions(ctx, node, username, policies, permAudit) if result == CheckNeutral || result == CheckDeny { return result } From 758d910533366ae517086a509be40caa76b4dbe5 Mon Sep 17 00:00:00 2001 From: Itamar Yuran Date: Tue, 5 Nov 2024 16:02:41 +0200 Subject: [PATCH 14/15] simpler, returning first missing --- pkg/api/controller_test.go | 18 +++++++++--------- pkg/auth/service.go | 2 +- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/pkg/api/controller_test.go b/pkg/api/controller_test.go index 5b6798e90c9..a6a23d0a3bc 100644 --- a/pkg/api/controller_test.go +++ b/pkg/api/controller_test.go @@ -5387,10 +5387,10 @@ func TestCheckPermissions_UnpermittedRequests(t *testing.T) { }, }, }, - expected: "denied from:\nfs:DeleteRepository", + expected: "denied permission to fs:DeleteRepository", }, ///////////////////////////////////////////////////////////////// { - name: "deny multiple actions", + name: "deny multiple actions, one concerning the request", node: permissions.Node{ Type: permissions.NodeTypeNode, Permission: permissions.Permission{ @@ -5410,7 +5410,7 @@ func TestCheckPermissions_UnpermittedRequests(t *testing.T) { }, }, }, - expected: "denied from:\nfs:DeleteRepository\nfs:CreateRepository", + expected: "denied permission to fs:DeleteRepository", }, ///////////////////////////////////////////////////////////////// { name: "neutral action", @@ -5433,10 +5433,10 @@ func TestCheckPermissions_UnpermittedRequests(t *testing.T) { }, }, }, - expected: "lacking permissions for:\nfs:ReadRepository", + expected: "missing permission to fs:ReadRepository", }, ///////////////////////////////////////////////////////////////// { - name: "node and no policy", + name: "nodeAnd no policy, returns first missing one", node: permissions.Node{ Type: permissions.NodeTypeAnd, Nodes: []permissions.Node{ @@ -5457,10 +5457,10 @@ func TestCheckPermissions_UnpermittedRequests(t *testing.T) { }, }, username: "user1", - expected: "lacking permissions for:\nfs:CreateRepository", + expected: "missing permission to fs:CreateRepository", }, { - name: "node and one policy", + name: "nodeAnd one policy, returns first missing policy", node: permissions.Node{ Type: permissions.NodeTypeAnd, Nodes: []permissions.Node{ @@ -5492,12 +5492,12 @@ func TestCheckPermissions_UnpermittedRequests(t *testing.T) { }, }, }, - expected: "lacking permissions for:\nfs:AttachStorageNamespace", + expected: "missing permission to fs:AttachStorageNamespace", }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - perm := &auth.NeededPermissions{} + 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()) diff --git a/pkg/auth/service.go b/pkg/auth/service.go index c49fe3e41ed..493b48e3590 100644 --- a/pkg/auth/service.go +++ b/pkg/auth/service.go @@ -1157,7 +1157,7 @@ func NewAPIAuthServiceWithClient(client ClientWithResponsesInterface, externalPr func (n *MissingPermissions) String() string { if len(n.Denied) != 0 { - return fmt.Sprintf("denied permissions to %s", strings.Join(n.Denied, ",")) + return fmt.Sprintf("denied permission to %s", strings.Join(n.Denied, ",")) } if len(n.Unauthorized) != 0 { return fmt.Sprintf("missing permission to %s", strings.Join(n.Unauthorized, ",")) From 7b304cc24361d3d0075d40c1acfe17ccdbe26e9e Mon Sep 17 00:00:00 2001 From: Itamar Yuran Date: Tue, 5 Nov 2024 16:16:48 +0200 Subject: [PATCH 15/15] simpler, no mock --- pkg/auth/gomock_reflect_4293290132/prog.go | 66 ---------------------- 1 file changed, 66 deletions(-) delete mode 100644 pkg/auth/gomock_reflect_4293290132/prog.go diff --git a/pkg/auth/gomock_reflect_4293290132/prog.go b/pkg/auth/gomock_reflect_4293290132/prog.go deleted file mode 100644 index fc096224037..00000000000 --- a/pkg/auth/gomock_reflect_4293290132/prog.go +++ /dev/null @@ -1,66 +0,0 @@ - -package main - -import ( - "encoding/gob" - "flag" - "fmt" - "os" - "path" - "reflect" - - "github.com/golang/mock/mockgen/model" - - pkg_ "github.com/treeverse/lakefs/pkg/auth" -) - -var output = flag.String("output", "", "The output file name, or empty to use stdout.") - -func main() { - flag.Parse() - - its := []struct{ - sym string - typ reflect.Type - }{ - - { "ClientWithResponsesInterface", reflect.TypeOf((*pkg_.ClientWithResponsesInterface)(nil)).Elem()}, - - } - pkg := &model.Package{ - // NOTE: This behaves contrary to documented behaviour if the - // package name is not the final component of the import path. - // The reflect package doesn't expose the package name, though. - Name: path.Base("github.com/treeverse/lakefs/pkg/auth"), - } - - for _, it := range its { - intf, err := model.InterfaceFromInterfaceType(it.typ) - if err != nil { - fmt.Fprintf(os.Stderr, "Reflection: %v\n", err) - os.Exit(1) - } - intf.Name = it.sym - pkg.Interfaces = append(pkg.Interfaces, intf) - } - - outfile := os.Stdout - if len(*output) != 0 { - var err error - outfile, err = os.Create(*output) - if err != nil { - fmt.Fprintf(os.Stderr, "failed to open output file %q", *output) - } - defer func() { - if err := outfile.Close(); err != nil { - fmt.Fprintf(os.Stderr, "failed to close output file %q", *output) - os.Exit(1) - } - }() - } - - if err := gob.NewEncoder(outfile).Encode(pkg); err != nil { - fmt.Fprintf(os.Stderr, "gob encode: %v\n", err) - os.Exit(1) - } -}