From b1a31281e4901e73d93a5690ccb9ac6ae3e2aa4f Mon Sep 17 00:00:00 2001 From: Evgenii Stratonikov Date: Fri, 8 Nov 2024 09:51:14 +0300 Subject: [PATCH] [#1480] ape: Remove SoftAPECheck flag Previous release was EACL-compatible. Starting from now all EACL should've been migrated to APE chains. Signed-off-by: Evgenii Stratonikov --- pkg/services/common/ape/checker.go | 5 +- pkg/services/object/acl/acl.go | 48 ------------ pkg/services/object/acl/acl_test.go | 89 ----------------------- pkg/services/object/acl/v2/errors.go | 7 -- pkg/services/object/acl/v2/errors_test.go | 10 --- pkg/services/object/acl/v2/request.go | 7 -- pkg/services/object/acl/v2/service.go | 67 ----------------- pkg/services/object/acl/v2/types.go | 11 --- pkg/services/object/ape/checker.go | 4 - pkg/services/object/ape/service.go | 12 --- pkg/services/object/request_context.go | 2 - pkg/services/tree/ape.go | 1 - 12 files changed, 1 insertion(+), 262 deletions(-) delete mode 100644 pkg/services/object/acl/acl_test.go diff --git a/pkg/services/common/ape/checker.go b/pkg/services/common/ape/checker.go index 278f6da31..eb4fd03c7 100644 --- a/pkg/services/common/ape/checker.go +++ b/pkg/services/common/ape/checker.go @@ -44,9 +44,6 @@ type CheckPrm struct { // The request's bearer token. It is used in order to check APE overrides with the token. BearerToken *bearer.Token - - // If SoftAPECheck is set to true, then NoRuleFound is interpreted as allow. - SoftAPECheck bool } // CheckCore provides methods to perform the common logic of APE check. @@ -104,7 +101,7 @@ func (c *checkerCoreImpl) CheckAPE(prm CheckPrm) error { if err != nil { return err } - if !found && prm.SoftAPECheck || status == apechain.Allow { + if found && status == apechain.Allow { return nil } err = fmt.Errorf("access to operation %s is denied by access policy engine: %s", prm.Request.Operation(), status.String()) diff --git a/pkg/services/object/acl/acl.go b/pkg/services/object/acl/acl.go index 921545c8b..53ba652e1 100644 --- a/pkg/services/object/acl/acl.go +++ b/pkg/services/object/acl/acl.go @@ -3,7 +3,6 @@ package acl import ( "context" "crypto/ecdsa" - "crypto/elliptic" "errors" "fmt" "io" @@ -22,7 +21,6 @@ import ( objectSDK "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/object" oid "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/object/id" "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/user" - "github.com/nspcc-dev/neo-go/pkg/crypto/keys" ) // Checker implements v2.ACLChecker interfaces and provides @@ -72,33 +70,6 @@ func NewChecker( } } -// CheckBasicACL is a main check function for basic ACL. -func (c *Checker) CheckBasicACL(info v2.RequestInfo) bool { - // check basic ACL permissions - return info.BasicACL().IsOpAllowed(info.Operation(), info.RequestRole()) -} - -// StickyBitCheck validates owner field in the request if sticky bit is enabled. -func (c *Checker) StickyBitCheck(info v2.RequestInfo, owner user.ID) bool { - // According to FrostFS specification sticky bit has no effect on system nodes - // for correct intra-container work with objects (in particular, replication). - if info.RequestRole() == acl.RoleContainer { - return true - } - - if !info.BasicACL().Sticky() { - return true - } - - if len(info.SenderKey()) == 0 { - return false - } - - requestSenderKey := unmarshalPublicKey(info.SenderKey()) - - return isOwnerFromKey(owner, requestSenderKey) -} - // CheckEACL is a main check function for extended ACL. func (c *Checker) CheckEACL(msg any, reqInfo v2.RequestInfo) error { basicACL := reqInfo.BasicACL() @@ -241,22 +212,3 @@ func isValidBearer(reqInfo v2.RequestInfo, st netmap.State) error { return nil } - -func isOwnerFromKey(id user.ID, key *keys.PublicKey) bool { - if key == nil { - return false - } - - var id2 user.ID - user.IDFromKey(&id2, (ecdsa.PublicKey)(*key)) - - return id.Equals(id2) -} - -func unmarshalPublicKey(bs []byte) *keys.PublicKey { - pub, err := keys.NewPublicKeyFromBytes(bs, elliptic.P256()) - if err != nil { - return nil - } - return pub -} diff --git a/pkg/services/object/acl/acl_test.go b/pkg/services/object/acl/acl_test.go deleted file mode 100644 index d63cb1285..000000000 --- a/pkg/services/object/acl/acl_test.go +++ /dev/null @@ -1,89 +0,0 @@ -package acl - -import ( - "testing" - - "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/core/container" - "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/local_object_storage/engine" - v2 "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/services/object/acl/v2" - "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/container/acl" - cid "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/container/id" - eaclSDK "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/eacl" - "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/user" - usertest "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/user/test" - "github.com/stretchr/testify/require" -) - -type emptyEACLSource struct{} - -func (e emptyEACLSource) GetEACL(_ cid.ID) (*container.EACL, error) { - return nil, nil -} - -type emptyNetmapState struct{} - -func (e emptyNetmapState) CurrentEpoch() uint64 { - return 0 -} - -func TestStickyCheck(t *testing.T) { - checker := NewChecker( - emptyNetmapState{}, - emptyEACLSource{}, - eaclSDK.NewValidator(), - &engine.StorageEngine{}) - - t.Run("system role", func(t *testing.T) { - var info v2.RequestInfo - - info.SetSenderKey(make([]byte, 33)) // any non-empty key - info.SetRequestRole(acl.RoleContainer) - - require.True(t, checker.StickyBitCheck(info, usertest.ID())) - - var basicACL acl.Basic - basicACL.MakeSticky() - - info.SetBasicACL(basicACL) - - require.True(t, checker.StickyBitCheck(info, usertest.ID())) - }) - - t.Run("owner ID and/or public key emptiness", func(t *testing.T) { - var info v2.RequestInfo - - info.SetRequestRole(acl.RoleOthers) // should be non-system role - - assertFn := func(isSticky, withKey, withOwner, expected bool) { - info := info - if isSticky { - var basicACL acl.Basic - basicACL.MakeSticky() - - info.SetBasicACL(basicACL) - } - - if withKey { - info.SetSenderKey(make([]byte, 33)) - } else { - info.SetSenderKey(nil) - } - - var ownerID user.ID - - if withOwner { - ownerID = usertest.ID() - } - - require.Equal(t, expected, checker.StickyBitCheck(info, ownerID)) - } - - assertFn(true, false, false, false) - assertFn(true, true, false, false) - assertFn(true, false, true, false) - assertFn(false, false, false, true) - assertFn(false, true, false, true) - assertFn(false, false, true, true) - assertFn(false, true, true, true) - }) -} diff --git a/pkg/services/object/acl/v2/errors.go b/pkg/services/object/acl/v2/errors.go index 11b9e6e5f..e969d37fa 100644 --- a/pkg/services/object/acl/v2/errors.go +++ b/pkg/services/object/acl/v2/errors.go @@ -26,13 +26,6 @@ const ( accessDeniedEACLReasonFmt = "access to operation %s is denied by extended ACL check: %v" ) -func basicACLErr(info RequestInfo) error { - errAccessDenied := &apistatus.ObjectAccessDenied{} - errAccessDenied.WriteReason(fmt.Sprintf(accessDeniedACLReasonFmt, info.operation)) - - return errAccessDenied -} - func eACLErr(info RequestInfo, err error) error { errAccessDenied := &apistatus.ObjectAccessDenied{} errAccessDenied.WriteReason(fmt.Sprintf(accessDeniedEACLReasonFmt, info.operation, err)) diff --git a/pkg/services/object/acl/v2/errors_test.go b/pkg/services/object/acl/v2/errors_test.go index 2d2b7bc8d..3cc74e6aa 100644 --- a/pkg/services/object/acl/v2/errors_test.go +++ b/pkg/services/object/acl/v2/errors_test.go @@ -8,16 +8,6 @@ import ( "github.com/stretchr/testify/require" ) -func TestBasicACLErr(t *testing.T) { - var reqInfo RequestInfo - err := basicACLErr(reqInfo) - - var errAccessDenied *apistatus.ObjectAccessDenied - - require.ErrorAs(t, err, &errAccessDenied, - "basicACLErr must be able to be casted to apistatus.ObjectAccessDenied") -} - func TestEACLErr(t *testing.T) { var reqInfo RequestInfo testErr := errors.New("test-eacl") diff --git a/pkg/services/object/acl/v2/request.go b/pkg/services/object/acl/v2/request.go index e35cd2e11..8bd34ccb3 100644 --- a/pkg/services/object/acl/v2/request.go +++ b/pkg/services/object/acl/v2/request.go @@ -104,13 +104,6 @@ func (r RequestInfo) RequestRole() acl.Role { return r.requestRole } -// IsSoftAPECheck states if APE should perform soft checks. -// Soft APE check allows a request if CheckAPE returns NoRuleFound for it, -// otherwise it denies the request. -func (r RequestInfo) IsSoftAPECheck() bool { - return r.BasicACL().Bits() != 0 -} - // MetaWithToken groups session and bearer tokens, // verification header and raw API request. type MetaWithToken struct { diff --git a/pkg/services/object/acl/v2/service.go b/pkg/services/object/acl/v2/service.go index e02a3be36..9f5ac5a27 100644 --- a/pkg/services/object/acl/v2/service.go +++ b/pkg/services/object/acl/v2/service.go @@ -123,7 +123,6 @@ func (w *wrappedGetObjectStream) Context() context.Context { ContainerOwner: w.requestInfo.ContainerOwner(), SenderKey: w.requestInfo.SenderKey(), Role: w.requestInfo.RequestRole(), - SoftAPECheck: w.requestInfo.IsSoftAPECheck(), BearerToken: w.requestInfo.Bearer(), }) } @@ -149,7 +148,6 @@ func (w *wrappedRangeStream) Context() context.Context { ContainerOwner: w.requestInfo.ContainerOwner(), SenderKey: w.requestInfo.SenderKey(), Role: w.requestInfo.RequestRole(), - SoftAPECheck: w.requestInfo.IsSoftAPECheck(), BearerToken: w.requestInfo.Bearer(), }) } @@ -175,7 +173,6 @@ func (w *wrappedSearchStream) Context() context.Context { ContainerOwner: w.requestInfo.ContainerOwner(), SenderKey: w.requestInfo.SenderKey(), Role: w.requestInfo.RequestRole(), - SoftAPECheck: w.requestInfo.IsSoftAPECheck(), BearerToken: w.requestInfo.Bearer(), }) } @@ -231,14 +228,6 @@ func (b Service) Get(request *objectV2.GetRequest, stream object.GetObjectStream reqInfo.obj = obj - if reqInfo.IsSoftAPECheck() { - if !b.checker.CheckBasicACL(reqInfo) { - return basicACLErr(reqInfo) - } else if err := b.checker.CheckEACL(request, reqInfo); err != nil { - return eACLErr(reqInfo, err) - } - } - return b.next.Get(request, &getStreamBasicChecker{ GetObjectStream: newWrappedGetObjectStreamStream(stream, reqInfo), info: reqInfo, @@ -309,14 +298,6 @@ func (b Service) Head( reqInfo.obj = obj - if reqInfo.IsSoftAPECheck() { - if !b.checker.CheckBasicACL(reqInfo) { - return nil, basicACLErr(reqInfo) - } else if err := b.checker.CheckEACL(request, reqInfo); err != nil { - return nil, eACLErr(reqInfo, err) - } - } - resp, err := b.next.Head(requestContext(ctx, reqInfo), request) if err == nil { if err = b.checker.CheckEACL(resp, reqInfo); err != nil { @@ -362,14 +343,6 @@ func (b Service) Search(request *objectV2.SearchRequest, stream object.SearchStr return err } - if reqInfo.IsSoftAPECheck() { - if !b.checker.CheckBasicACL(reqInfo) { - return basicACLErr(reqInfo) - } else if err := b.checker.CheckEACL(request, reqInfo); err != nil { - return eACLErr(reqInfo, err) - } - } - return b.next.Search(request, &searchStreamBasicChecker{ checker: b.checker, SearchStream: newWrappedSearchStream(stream, reqInfo), @@ -422,14 +395,6 @@ func (b Service) Delete( reqInfo.obj = obj - if reqInfo.IsSoftAPECheck() { - if !b.checker.CheckBasicACL(reqInfo) { - return nil, basicACLErr(reqInfo) - } else if err := b.checker.CheckEACL(request, reqInfo); err != nil { - return nil, eACLErr(reqInfo, err) - } - } - return b.next.Delete(requestContext(ctx, reqInfo), request) } @@ -475,14 +440,6 @@ func (b Service) GetRange(request *objectV2.GetRangeRequest, stream object.GetOb reqInfo.obj = obj - if reqInfo.IsSoftAPECheck() { - if !b.checker.CheckBasicACL(reqInfo) { - return basicACLErr(reqInfo) - } else if err := b.checker.CheckEACL(request, reqInfo); err != nil { - return eACLErr(reqInfo, err) - } - } - return b.next.GetRange(request, &rangeStreamBasicChecker{ checker: b.checker, GetObjectRangeStream: newWrappedRangeStream(stream, reqInfo), @@ -496,7 +453,6 @@ func requestContext(ctx context.Context, reqInfo RequestInfo) context.Context { ContainerOwner: reqInfo.ContainerOwner(), SenderKey: reqInfo.SenderKey(), Role: reqInfo.RequestRole(), - SoftAPECheck: reqInfo.IsSoftAPECheck(), BearerToken: reqInfo.Bearer(), }) } @@ -546,14 +502,6 @@ func (b Service) GetRangeHash( reqInfo.obj = obj - if reqInfo.IsSoftAPECheck() { - if !b.checker.CheckBasicACL(reqInfo) { - return nil, basicACLErr(reqInfo) - } else if err := b.checker.CheckEACL(request, reqInfo); err != nil { - return nil, eACLErr(reqInfo, err) - } - } - return b.next.GetRangeHash(requestContext(ctx, reqInfo), request) } @@ -605,15 +553,6 @@ func (b Service) PutSingle(ctx context.Context, request *objectV2.PutSingleReque reqInfo.obj = obj - if reqInfo.IsSoftAPECheck() { - if !b.checker.CheckBasicACL(reqInfo) || !b.checker.StickyBitCheck(reqInfo, idOwner) { - return nil, basicACLErr(reqInfo) - } - if err := b.checker.CheckEACL(request, reqInfo); err != nil { - return nil, eACLErr(reqInfo, err) - } - } - return b.next.PutSingle(requestContext(ctx, reqInfo), request) } @@ -679,12 +618,6 @@ func (p putStreamBasicChecker) Send(ctx context.Context, request *objectV2.PutRe reqInfo.obj = obj - if reqInfo.IsSoftAPECheck() { - if !p.source.checker.CheckBasicACL(reqInfo) || !p.source.checker.StickyBitCheck(reqInfo, idOwner) { - return basicACLErr(reqInfo) - } - } - ctx = requestContext(ctx, reqInfo) } diff --git a/pkg/services/object/acl/v2/types.go b/pkg/services/object/acl/v2/types.go index 061cd26b6..6ae80e9c2 100644 --- a/pkg/services/object/acl/v2/types.go +++ b/pkg/services/object/acl/v2/types.go @@ -1,22 +1,11 @@ package v2 -import ( - "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/user" -) - // ACLChecker is an interface that must provide // ACL related checks. type ACLChecker interface { - // CheckBasicACL must return true only if request - // passes basic ACL validation. - CheckBasicACL(RequestInfo) bool // CheckEACL must return non-nil error if request // doesn't pass extended ACL validation. CheckEACL(any, RequestInfo) error - // StickyBitCheck must return true only if sticky bit - // is disabled or enabled but request contains correct - // owner field. - StickyBitCheck(RequestInfo, user.ID) bool } // InnerRingFetcher is an interface that must provide diff --git a/pkg/services/object/ape/checker.go b/pkg/services/object/ape/checker.go index abcd2f4bb..4a3b5ba5e 100644 --- a/pkg/services/object/ape/checker.go +++ b/pkg/services/object/ape/checker.go @@ -64,9 +64,6 @@ type Prm struct { // An encoded container's owner user ID. ContainerOwner user.ID - // If SoftAPECheck is set to true, then NoRuleFound is interpreted as allow. - SoftAPECheck bool - // The request's bearer token. It is used in order to check APE overrides with the token. BearerToken *bearer.Token @@ -109,6 +106,5 @@ func (c *checkerImpl) CheckAPE(ctx context.Context, prm Prm) error { Container: prm.Container, ContainerOwner: prm.ContainerOwner, BearerToken: prm.BearerToken, - SoftAPECheck: prm.SoftAPECheck, }) } diff --git a/pkg/services/object/ape/service.go b/pkg/services/object/ape/service.go index c114f02f6..558c48da8 100644 --- a/pkg/services/object/ape/service.go +++ b/pkg/services/object/ape/service.go @@ -84,8 +84,6 @@ type getStreamBasicChecker struct { role string - softAPECheck bool - bearerToken *bearer.Token } @@ -105,7 +103,6 @@ func (g *getStreamBasicChecker) Send(resp *objectV2.GetResponse) error { SenderKey: hex.EncodeToString(g.senderKey), ContainerOwner: g.containerOwner, Role: g.role, - SoftAPECheck: g.softAPECheck, BearerToken: g.bearerToken, XHeaders: resp.GetMetaHeader().GetXHeaders(), } @@ -142,7 +139,6 @@ func (c *Service) Get(request *objectV2.GetRequest, stream objectSvc.GetObjectSt senderKey: reqCtx.SenderKey, containerOwner: reqCtx.ContainerOwner, role: nativeSchemaRole(reqCtx.Role), - softAPECheck: reqCtx.SoftAPECheck, bearerToken: reqCtx.BearerToken, }) } @@ -174,7 +170,6 @@ func (p *putStreamBasicChecker) Send(ctx context.Context, request *objectV2.PutR SenderKey: hex.EncodeToString(reqCtx.SenderKey), ContainerOwner: reqCtx.ContainerOwner, Role: nativeSchemaRole(reqCtx.Role), - SoftAPECheck: reqCtx.SoftAPECheck, BearerToken: reqCtx.BearerToken, XHeaders: request.GetMetaHeader().GetXHeaders(), } @@ -230,7 +225,6 @@ func (p *patchStreamBasicChecker) Send(ctx context.Context, request *objectV2.Pa SenderKey: hex.EncodeToString(reqCtx.SenderKey), ContainerOwner: reqCtx.ContainerOwner, Role: nativeSchemaRole(reqCtx.Role), - SoftAPECheck: reqCtx.SoftAPECheck, BearerToken: reqCtx.BearerToken, XHeaders: request.GetMetaHeader().GetXHeaders(), } @@ -300,7 +294,6 @@ func (c *Service) Head(ctx context.Context, request *objectV2.HeadRequest) (*obj Role: nativeSchemaRole(reqCtx.Role), SenderKey: hex.EncodeToString(reqCtx.SenderKey), ContainerOwner: reqCtx.ContainerOwner, - SoftAPECheck: reqCtx.SoftAPECheck, BearerToken: reqCtx.BearerToken, XHeaders: request.GetMetaHeader().GetXHeaders(), }) @@ -330,7 +323,6 @@ func (c *Service) Search(request *objectV2.SearchRequest, stream objectSvc.Searc Role: nativeSchemaRole(reqCtx.Role), SenderKey: hex.EncodeToString(reqCtx.SenderKey), ContainerOwner: reqCtx.ContainerOwner, - SoftAPECheck: reqCtx.SoftAPECheck, BearerToken: reqCtx.BearerToken, XHeaders: request.GetMetaHeader().GetXHeaders(), }) @@ -360,7 +352,6 @@ func (c *Service) Delete(ctx context.Context, request *objectV2.DeleteRequest) ( Role: nativeSchemaRole(reqCtx.Role), SenderKey: hex.EncodeToString(reqCtx.SenderKey), ContainerOwner: reqCtx.ContainerOwner, - SoftAPECheck: reqCtx.SoftAPECheck, BearerToken: reqCtx.BearerToken, XHeaders: request.GetMetaHeader().GetXHeaders(), }) @@ -395,7 +386,6 @@ func (c *Service) GetRange(request *objectV2.GetRangeRequest, stream objectSvc.G Role: nativeSchemaRole(reqCtx.Role), SenderKey: hex.EncodeToString(reqCtx.SenderKey), ContainerOwner: reqCtx.ContainerOwner, - SoftAPECheck: reqCtx.SoftAPECheck, BearerToken: reqCtx.BearerToken, XHeaders: request.GetMetaHeader().GetXHeaders(), }) @@ -425,7 +415,6 @@ func (c *Service) GetRangeHash(ctx context.Context, request *objectV2.GetRangeHa Role: nativeSchemaRole(reqCtx.Role), SenderKey: hex.EncodeToString(reqCtx.SenderKey), ContainerOwner: reqCtx.ContainerOwner, - SoftAPECheck: reqCtx.SoftAPECheck, BearerToken: reqCtx.BearerToken, XHeaders: request.GetMetaHeader().GetXHeaders(), } @@ -461,7 +450,6 @@ func (c *Service) PutSingle(ctx context.Context, request *objectV2.PutSingleRequ Role: nativeSchemaRole(reqCtx.Role), SenderKey: hex.EncodeToString(reqCtx.SenderKey), ContainerOwner: reqCtx.ContainerOwner, - SoftAPECheck: reqCtx.SoftAPECheck, BearerToken: reqCtx.BearerToken, XHeaders: request.GetMetaHeader().GetXHeaders(), } diff --git a/pkg/services/object/request_context.go b/pkg/services/object/request_context.go index 95d4c9d93..eb4041f80 100644 --- a/pkg/services/object/request_context.go +++ b/pkg/services/object/request_context.go @@ -20,7 +20,5 @@ type RequestContext struct { Role acl.Role - SoftAPECheck bool - BearerToken *bearer.Token } diff --git a/pkg/services/tree/ape.go b/pkg/services/tree/ape.go index 69cf59405..606044f8e 100644 --- a/pkg/services/tree/ape.go +++ b/pkg/services/tree/ape.go @@ -81,7 +81,6 @@ func (s *Service) checkAPE(ctx context.Context, bt *bearer.Token, ContainerOwner: container.Value.Owner(), PublicKey: publicKey, BearerToken: bt, - SoftAPECheck: false, }) }