Skip to content

Commit

Permalink
Merge pull request from GHSA-m836-gxwq-j2pm
Browse files Browse the repository at this point in the history 
* Added required permissions on copy operation

* Changed Authorize to support and or operation

* Changed Authorize to support and or operation

* Changed Authorize to support and or operation

* Changed Authorize to support and or operation
  • Loading branch information
@eden-ohana
eden-ohana authored Oct 27, 2021
1 parent 62ebf77 commit f211728
Show file tree
Hide file tree
Showing 16 changed files with 420 additions and 521 deletions.
588 changes: 224 additions & 364 deletions pkg/api/controller.go
View file

Large diffs are not rendered by default.

97 changes: 69 additions & 28 deletions pkg/auth/service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import (

type AuthorizationRequest struct {
Username string
RequiredPermissions []permissions.Permission
RequiredPermissions PermissionNode
}

type AuthorizationResponse struct {
Expand Down Expand Up @@ -852,6 +852,71 @@ func interpolateUser(resource string, username string) string {
return strings.ReplaceAll(resource, "${user}", username)
}

type CheckResult int

const (
CheckAllow CheckResult = iota // Permission allowed
CheckNeutral // Permission neither allowed nor denied
CheckDeny // Permission denied
)

type PermissionNode interface {
CheckPermissions(policies []*model.Policy, req *AuthorizationRequest) CheckResult
}

type OnePermission permissions.Permission
type AndPermission []PermissionNode
type OrPermission []PermissionNode

func (p OnePermission) CheckPermissions(policies []*model.Policy, req *AuthorizationRequest) CheckResult {
allowed := CheckNeutral
for _, policy := range policies {
for _, stmt := range policy.Statement {
resource := interpolateUser(stmt.Resource, req.Username)
if !ArnMatch(resource, p.Resource) {
continue
}
for _, action := range stmt.Action {
if !wildcard.Match(action, p.Action) {
continue // not a matching action
}

if stmt.Effect == model.StatementEffectDeny {
// this is a "Deny" and it takes precedence
return CheckDeny
}

allowed = CheckAllow
}
}
}
return allowed
}

func (p AndPermission) CheckPermissions(policies []*model.Policy, req *AuthorizationRequest) CheckResult {
for _, perm := range p {
result := perm.CheckPermissions(policies, req)
if result == CheckNeutral || result == CheckDeny {
return result
}
}
return CheckAllow
}

func (p OrPermission) CheckPermissions(policies []*model.Policy, req *AuthorizationRequest) CheckResult {
allowed := CheckNeutral
for _, perm := range p {
result := perm.CheckPermissions(policies, req)
if result == CheckDeny {
return CheckDeny
}
if allowed != CheckAllow {
allowed = result
}
}
return allowed
}

func (s *DBAuthService) Authorize(ctx context.Context, req *AuthorizationRequest) (*AuthorizationResponse, error) {
policies, _, err := s.ListEffectivePolicies(ctx, req.Username, &model.PaginationParams{
After: "", // all
Expand All @@ -861,34 +926,10 @@ func (s *DBAuthService) Authorize(ctx context.Context, req *AuthorizationRequest
if err != nil {
return nil, err
}
allowed := false
for _, perm := range req.RequiredPermissions {
for _, policy := range policies {
for _, stmt := range policy.Statement {
resource := interpolateUser(stmt.Resource, req.Username)
if !ArnMatch(resource, perm.Resource) {
continue
}
for _, action := range stmt.Action {
if !wildcard.Match(action, perm.Action) {
continue // not a matching action
}

if stmt.Effect == model.StatementEffectDeny {
// this is a "Deny" and it takes precedence
return &AuthorizationResponse{
Allowed: false,
Error: ErrInsufficientPermissions,
}, nil
}

allowed = true
}
}
}
}

if !allowed {
allowed := req.RequiredPermissions.CheckPermissions(policies, req)

if allowed != CheckAllow {
return &AuthorizationResponse{
Allowed: false,
Error: ErrInsufficientPermissions,
Expand Down
81 changes: 30 additions & 51 deletions pkg/auth/service_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"github.com/treeverse/lakefs/pkg/auth/model"
authparams "github.com/treeverse/lakefs/pkg/auth/params"
"github.com/treeverse/lakefs/pkg/logging"
"github.com/treeverse/lakefs/pkg/permissions"
"github.com/treeverse/lakefs/pkg/testutil"
)

Expand Down Expand Up @@ -190,11 +189,9 @@ func TestDBAuthService_Authorize(t *testing.T) {
request: func(userName string) *auth.AuthorizationRequest {
return &auth.AuthorizationRequest{
Username: userName,
RequiredPermissions: []permissions.Permission{
{
Action: "fs:WriteObject",
Resource: "arn:lakefs:fs:::repository/foo/object/bar",
},
RequiredPermissions: &auth.OnePermission{
Action: "fs:WriteObject",
Resource: "arn:lakefs:fs:::repository/foo/object/bar",
},
}
},
Expand All @@ -217,11 +214,9 @@ func TestDBAuthService_Authorize(t *testing.T) {
request: func(userName string) *auth.AuthorizationRequest {
return &auth.AuthorizationRequest{
Username: userName,
RequiredPermissions: []permissions.Permission{
{
Action: "fs:WriteObject",
Resource: "arn:lakefs:fs:::repository/foo/object/bar",
},
RequiredPermissions: &auth.OnePermission{
Action: "fs:WriteObject",
Resource: "arn:lakefs:fs:::repository/foo/object/bar",
},
}
},
Expand All @@ -244,11 +239,9 @@ func TestDBAuthService_Authorize(t *testing.T) {
request: func(userName string) *auth.AuthorizationRequest {
return &auth.AuthorizationRequest{
Username: userName,
RequiredPermissions: []permissions.Permission{
{
Action: "fs:WriteObject",
Resource: "arn:lakefs:fs:::repository/foo/object/bar",
},
RequiredPermissions: &auth.OnePermission{
Action: "fs:WriteObject",
Resource: "arn:lakefs:fs:::repository/foo/object/bar",
},
}
},
Expand All @@ -271,11 +264,9 @@ func TestDBAuthService_Authorize(t *testing.T) {
request: func(userName string) *auth.AuthorizationRequest {
return &auth.AuthorizationRequest{
Username: userName,
RequiredPermissions: []permissions.Permission{
{
Action: "auth:CreateUser",
Resource: "arn:lakefs:auth:::user/foobar",
},
RequiredPermissions: &auth.OnePermission{
Action: "auth:CreateUser",
Resource: "arn:lakefs:auth:::user/foobar",
},
}
},
Expand All @@ -298,11 +289,9 @@ func TestDBAuthService_Authorize(t *testing.T) {
request: func(userName string) *auth.AuthorizationRequest {
return &auth.AuthorizationRequest{
Username: userName,
RequiredPermissions: []permissions.Permission{
{
Action: "auth:CreateUser",
Resource: fmt.Sprintf("arn:lakefs:auth:::user/%s", userName),
},
RequiredPermissions: &auth.OnePermission{
Action: "auth:CreateUser",
Resource: fmt.Sprintf("arn:lakefs:auth:::user/%s", userName),
},
}
},
Expand All @@ -325,11 +314,9 @@ func TestDBAuthService_Authorize(t *testing.T) {
request: func(userName string) *auth.AuthorizationRequest {
return &auth.AuthorizationRequest{
Username: userName,
RequiredPermissions: []permissions.Permission{
{
Action: "auth:CreateUser",
Resource: fmt.Sprintf("arn:lakefs:auth:::user/%sxxxx", userName),
},
RequiredPermissions: &auth.OnePermission{
Action: "auth:CreateUser",
Resource: fmt.Sprintf("arn:lakefs:auth:::user/%sxxxx", userName),
},
}
},
Expand All @@ -352,11 +339,9 @@ func TestDBAuthService_Authorize(t *testing.T) {
request: func(userName string) *auth.AuthorizationRequest {
return &auth.AuthorizationRequest{
Username: userName,
RequiredPermissions: []permissions.Permission{
{
Action: "auth:CreateUser",
Resource: "arn:lakefs:auth:::user/foobar",
},
RequiredPermissions: &auth.OnePermission{
Action: "auth:CreateUser",
Resource: "arn:lakefs:auth:::user/foobar",
},
}
},
Expand All @@ -379,11 +364,9 @@ func TestDBAuthService_Authorize(t *testing.T) {
request: func(userName string) *auth.AuthorizationRequest {
return &auth.AuthorizationRequest{
Username: userName,
RequiredPermissions: []permissions.Permission{
{
Action: "auth:CreateUser",
Resource: "arn:lakefs:auth:::user/foobar",
},
RequiredPermissions: &auth.OnePermission{
Action: "auth:CreateUser",
Resource: "arn:lakefs:auth:::user/foobar",
},
}
},
Expand All @@ -406,11 +389,9 @@ func TestDBAuthService_Authorize(t *testing.T) {
request: func(userName string) *auth.AuthorizationRequest {
return &auth.AuthorizationRequest{
Username: userName,
RequiredPermissions: []permissions.Permission{
{
Action: "auth:DeleteUser",
Resource: "arn:lakefs:auth:::user/foobar",
},
RequiredPermissions: &auth.OnePermission{
Action: "auth:DeleteUser",
Resource: "arn:lakefs:auth:::user/foobar",
},
}
},
Expand Down Expand Up @@ -442,11 +423,9 @@ func TestDBAuthService_Authorize(t *testing.T) {
request: func(userName string) *auth.AuthorizationRequest {
return &auth.AuthorizationRequest{
Username: userName,
RequiredPermissions: []permissions.Permission{
{
Action: "auth:DeleteUser",
Resource: "arn:lakefs:auth:::user/foobar",
},
RequiredPermissions: &auth.OnePermission{
Action: "auth:DeleteUser",
Resource: "arn:lakefs:auth:::user/foobar",
},
}
},
Expand Down
17 changes: 12 additions & 5 deletions pkg/gateway/handler.go
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package gateway

import (
"errors"
"net/http"
gohttputil "net/http/httputil"
"net/url"
Expand All @@ -18,7 +19,6 @@ import (
"github.com/treeverse/lakefs/pkg/gateway/simulator"
"github.com/treeverse/lakefs/pkg/httputil"
"github.com/treeverse/lakefs/pkg/logging"
"github.com/treeverse/lakefs/pkg/permissions"
"github.com/treeverse/lakefs/pkg/stats"
)

Expand Down Expand Up @@ -195,14 +195,21 @@ func PathOperationHandler(sc *ServerContext, handler operations.PathOperationHan
matchedHost := ctx.Value(ContextKeyMatchedHost).(bool)
o := ctx.Value(ContextKeyOperation).(*operations.Operation)
perms, err := handler.RequiredPermissions(req, repo.Name, refID, path)

if err != nil {
_ = o.EncodeError(w, req, gatewayerrors.ErrAccessDenied.ToAPIErr())
if errors.Is(err, gatewayerrors.ErrInvalidCopySource) {
_ = o.EncodeError(w, req, gatewayerrors.ErrInvalidCopySource.ToAPIErr())
} else {
_ = o.EncodeError(w, req, gatewayerrors.ErrAccessDenied.ToAPIErr())
}
return
}

authOp := authorize(w, req, sc.authService, perms)
if authOp == nil {
return
}

// run callback
operation := &operations.PathOperation{
RefOperation: &operations.RefOperation{
Expand All @@ -225,14 +232,14 @@ func PathOperationHandler(sc *ServerContext, handler operations.PathOperationHan
})
}

func authorize(w http.ResponseWriter, req *http.Request, authService simulator.GatewayAuthService, perms []permissions.Permission) *operations.AuthorizedOperation {
func authorize(w http.ResponseWriter, req *http.Request, authService simulator.GatewayAuthService, perms auth.PermissionNode) *operations.AuthorizedOperation {
ctx := req.Context()
o := ctx.Value(ContextKeyOperation).(*operations.Operation)
username := ctx.Value(ContextKeyUser).(*model.User).Username
authContext := ctx.Value(ContextKeyAuthContext).(sig.SigContext)

if len(perms) == 0 {
// Either no permissions are required, or they will be checked later.
if perms == nil {
// has not provided required permissions
return &operations.AuthorizedOperation{
Operation: o,
Principal: username,
Expand Down
2 changes: 1 addition & 1 deletion pkg/gateway/middleware.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ func EnrichWithRepositoryOrFallback(c catalog.Interface, authService simulator.G
if errors.Is(err, catalog.ErrNotFound) {
authResp, authErr := authService.Authorize(ctx, &auth.AuthorizationRequest{
Username: username,
RequiredPermissions: []permissions.Permission{{Action: permissions.ListRepositoriesAction, Resource: "*"}},
RequiredPermissions: &auth.OnePermission{Action: permissions.ListRepositoriesAction, Resource: "*"},
})
if authErr != nil || authResp.Error != nil || !authResp.Allowed {
_ = o.EncodeError(w, req, gatewayerrors.ErrAccessDenied.ToAPIErr())
Expand Down
12 changes: 6 additions & 6 deletions pkg/gateway/operations/base.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import (
"github.com/treeverse/lakefs/pkg/gateway/simulator"
"github.com/treeverse/lakefs/pkg/httputil"
"github.com/treeverse/lakefs/pkg/logging"
"github.com/treeverse/lakefs/pkg/permissions"
)

const StorageClassHeader = "x-amz-storage-class"
Expand Down Expand Up @@ -208,25 +207,26 @@ func (o *PathOperation) EncodeError(w http.ResponseWriter, req *http.Request, er
}

type OperationHandler interface {
RequiredPermissions(req *http.Request) ([]permissions.Permission, error)
RequiredPermissions(req *http.Request) (auth.PermissionNode, error)
Handle(w http.ResponseWriter, req *http.Request, op *Operation)
}

type AuthenticatedOperationHandler interface {
RequiredPermissions(req *http.Request) ([]permissions.Permission, error)
RequiredPermissions(req *http.Request) (auth.PermissionNode, error)
Handle(w http.ResponseWriter, req *http.Request, op *AuthorizedOperation)
}

type RepoOperationHandler interface {
RequiredPermissions(req *http.Request, repository string) ([]permissions.Permission, error)
RequiredPermissions(req *http.Request, repository string) (auth.PermissionNode, error)
Handle(w http.ResponseWriter, req *http.Request, op *RepoOperation)
}

type BranchOperationHandler interface {
RequiredPermissions(req *http.Request, repository, branch string) ([]permissions.Permission, error)
RequiredPermissions(req *http.Request, repository, branch string) (auth.PermissionNode, error)
Handle(w http.ResponseWriter, req *http.Request, op *RefOperation)
}

type PathOperationHandler interface {
RequiredPermissions(req *http.Request, repository, branch, path string) ([]permissions.Permission, error)
RequiredPermissions(req *http.Request, repository, branch, path string) (auth.PermissionNode, error)
Handle(w http.ResponseWriter, req *http.Request, op *PathOperation)
}
Loading

0 comments on commit f211728

Please sign in to comment.