From 876e014b5dfe14a068a0c1bfa18c1b6f9fdc9eaf Mon Sep 17 00:00:00 2001 From: Pavel Karpy Date: Thu, 8 Sep 2022 15:44:27 +0300 Subject: [PATCH] [#1628] tree: Make ACL checks the same way as for object requests 1. Do not require a request to be signed by the container owner if a bearer token is missing 2. Do not check the system role since public requests are not expected to be signed by IR or a container node (unlike the object requests) Signed-off-by: Pavel Karpy --- cmd/neofs-node/tree.go | 1 + pkg/services/tree/options.go | 21 ++-- pkg/services/tree/service.go | 14 +-- pkg/services/tree/signature.go | 142 ++++++++++++++++++++-------- pkg/services/tree/signature_test.go | 65 ++++++++----- 5 files changed, 171 insertions(+), 72 deletions(-) diff --git a/cmd/neofs-node/tree.go b/cmd/neofs-node/tree.go index 862f93301..f70502389 100644 --- a/cmd/neofs-node/tree.go +++ b/cmd/neofs-node/tree.go @@ -21,6 +21,7 @@ func initTreeService(c *cfg) { c.treeService = tree.New( tree.WithContainerSource(c.cfgObject.cnrSource), + tree.WithEACLSource(c.cfgObject.eaclSource), tree.WithNetmapSource(c.netMapSource), tree.WithPrivateKey(&c.key.PrivateKey), tree.WithLogger(c.log), diff --git a/pkg/services/tree/options.go b/pkg/services/tree/options.go index 2ee8eb4ed..ef111b477 100644 --- a/pkg/services/tree/options.go +++ b/pkg/services/tree/options.go @@ -11,12 +11,13 @@ import ( ) type cfg struct { - log *zap.Logger - key *ecdsa.PrivateKey - rawPub []byte - nmSource netmap.Source - cnrSource container.Source - forest pilorama.Forest + log *zap.Logger + key *ecdsa.PrivateKey + rawPub []byte + nmSource netmap.Source + cnrSource container.Source + eaclSource container.EACLSource + forest pilorama.Forest // replication-related parameters replicatorChannelCapacity int replicatorWorkerCount int @@ -34,6 +35,14 @@ func WithContainerSource(src container.Source) Option { } } +// WithEACLSource sets a eACL table source for a tree service. +// This option is required. +func WithEACLSource(src container.EACLSource) Option { + return func(c *cfg) { + c.eaclSource = src + } +} + // WithNetmapSource sets a netmap source for a tree service. // This option is required. func WithNetmapSource(src netmap.Source) Option { diff --git a/pkg/services/tree/service.go b/pkg/services/tree/service.go index 4506095ee..a7b87ee09 100644 --- a/pkg/services/tree/service.go +++ b/pkg/services/tree/service.go @@ -7,8 +7,8 @@ import ( "fmt" "github.com/nspcc-dev/neofs-node/pkg/local_object_storage/pilorama" + "github.com/nspcc-dev/neofs-sdk-go/container/acl" cidSDK "github.com/nspcc-dev/neofs-sdk-go/container/id" - "github.com/nspcc-dev/neofs-sdk-go/eacl" netmapSDK "github.com/nspcc-dev/neofs-sdk-go/netmap" "go.uber.org/zap" ) @@ -69,7 +69,7 @@ func (s *Service) Add(ctx context.Context, req *AddRequest) (*AddResponse, error return nil, err } - err := s.verifyClient(req, cid, b.GetBearerToken(), eacl.OperationPut) + err := s.verifyClient(req, cid, b.GetBearerToken(), acl.OpObjectPut) if err != nil { return nil, err } @@ -117,7 +117,7 @@ func (s *Service) AddByPath(ctx context.Context, req *AddByPathRequest) (*AddByP return nil, err } - err := s.verifyClient(req, cid, b.GetBearerToken(), eacl.OperationPut) + err := s.verifyClient(req, cid, b.GetBearerToken(), acl.OpObjectPut) if err != nil { return nil, err } @@ -177,7 +177,7 @@ func (s *Service) Remove(ctx context.Context, req *RemoveRequest) (*RemoveRespon return nil, err } - err := s.verifyClient(req, cid, b.GetBearerToken(), eacl.OperationPut) + err := s.verifyClient(req, cid, b.GetBearerToken(), acl.OpObjectPut) if err != nil { return nil, err } @@ -226,7 +226,7 @@ func (s *Service) Move(ctx context.Context, req *MoveRequest) (*MoveResponse, er return nil, err } - err := s.verifyClient(req, cid, b.GetBearerToken(), eacl.OperationPut) + err := s.verifyClient(req, cid, b.GetBearerToken(), acl.OpObjectPut) if err != nil { return nil, err } @@ -274,7 +274,7 @@ func (s *Service) GetNodeByPath(ctx context.Context, req *GetNodeByPathRequest) return nil, err } - err := s.verifyClient(req, cid, b.GetBearerToken(), eacl.OperationGet) + err := s.verifyClient(req, cid, b.GetBearerToken(), acl.OpObjectGet) if err != nil { return nil, err } @@ -350,7 +350,7 @@ func (s *Service) GetSubTree(req *GetSubTreeRequest, srv TreeService_GetSubTreeS return err } - err := s.verifyClient(req, cid, b.GetBearerToken(), eacl.OperationGet) + err := s.verifyClient(req, cid, b.GetBearerToken(), acl.OpObjectGet) if err != nil { return err } diff --git a/pkg/services/tree/signature.go b/pkg/services/tree/signature.go index 8f97929b6..3500d1b90 100644 --- a/pkg/services/tree/signature.go +++ b/pkg/services/tree/signature.go @@ -8,7 +8,10 @@ import ( "github.com/nspcc-dev/neo-go/pkg/crypto/keys" "github.com/nspcc-dev/neofs-api-go/v2/refs" + core "github.com/nspcc-dev/neofs-node/pkg/core/container" "github.com/nspcc-dev/neofs-sdk-go/bearer" + "github.com/nspcc-dev/neofs-sdk-go/client" + "github.com/nspcc-dev/neofs-sdk-go/container/acl" cidSDK "github.com/nspcc-dev/neofs-sdk-go/container/id" neofscrypto "github.com/nspcc-dev/neofs-sdk-go/crypto" neofsecdsa "github.com/nspcc-dev/neofs-sdk-go/crypto/ecdsa" @@ -23,9 +26,20 @@ type message interface { SetSignature(*Signature) } -// verifyClient verifies that the request for a client operation was either signed by owner -// or contains a valid bearer token. -func (s *Service) verifyClient(req message, cid cidSDK.ID, rawBearer []byte, op eacl.Operation) error { +func basicACLErr(op acl.Op) error { + return fmt.Errorf("access to operation %s is denied by basic ACL check", op) +} + +func eACLErr(op eacl.Operation, err error) error { + return fmt.Errorf("access to operation %s is denied by extended ACL check: %w", op, err) +} + +// verifyClient verifies if the request for a client operation +// was signed by a key allowed by (e)ACL rules. +// Operation must be one of: +// - 1. ObjectPut; +// - 2. ObjectGet. +func (s *Service) verifyClient(req message, cid cidSDK.ID, rawBearer []byte, op acl.Op) error { err := verifyMessage(req) if err != nil { return err @@ -36,53 +50,66 @@ func (s *Service) verifyClient(req message, cid cidSDK.ID, rawBearer []byte, op return fmt.Errorf("can't get container %s: %w", cid, err) } - ownerID := cnr.Value.Owner() + role, err := roleFromReq(cnr, req) + if err != nil { + return fmt.Errorf("can't get request role: %w", err) + } - if len(rawBearer) == 0 { // must be signed by the owner - // No error is expected because `VerifyDataWithSource` checks the signature. - // However, we may use different algorithms in the future, thus this check. - pub, err := keys.NewPublicKeyFromBytes(req.GetSignature().GetKey(), elliptic.P256()) - if err != nil { - return fmt.Errorf("invalid public key: %w", err) - } + basicACL := cnr.Value.BasicACL() - var actualID user.ID - user.IDFromKey(&actualID, (ecdsa.PublicKey)(*pub)) + if !basicACL.IsOpAllowed(op, role) { + return basicACLErr(op) + } - if !actualID.Equals(ownerID) { - return errors.New("request must be signed by a container owner") - } + if !basicACL.Extendable() { return nil } - var bt bearer.Token - if err := bt.Unmarshal(rawBearer); err != nil { - return fmt.Errorf("invalid bearer token: %w", err) - } - if !bearer.ResolveIssuer(bt).Equals(ownerID) { - return errors.New("bearer token must be signed by the container owner") - } - if !bt.AssertContainer(cid) { - return errors.New("bearer token is created for another container") - } - if !bt.VerifySignature() { - return errors.New("invalid bearer token signature") + eaclOp := eACLOp(op) + + var tb eacl.Table + if len(rawBearer) != 0 && basicACL.AllowedBearerRules(op) { + var bt bearer.Token + if err = bt.Unmarshal(rawBearer); err != nil { + return eACLErr(eaclOp, fmt.Errorf("invalid bearer token: %w", err)) + } + if !bearer.ResolveIssuer(bt).Equals(cnr.Value.Owner()) { + return eACLErr(eaclOp, errors.New("bearer token must be signed by the container owner")) + } + if !bt.AssertContainer(cid) { + return eACLErr(eaclOp, errors.New("bearer token is created for another container")) + } + if !bt.VerifySignature() { + return eACLErr(eaclOp, errors.New("invalid bearer token signature")) + } + + tb = bt.EACLTable() + } else { + tbCore, err := s.eaclSource.GetEACL(cid) + if err != nil { + if client.IsErrEACLNotFound(err) { + return nil + } + + return fmt.Errorf("get eACL table: %w", err) + } + + tb = *tbCore.Value } - tb := bt.EACLTable() - - // The default action should be DENY, so we use RoleOthers to allow token issuer - // to restrict everyone not affected by the previous rules. - // This can be simplified after nspcc-dev/neofs-sdk-go#243 . + // The default action should be DENY. action, found := eacl.NewValidator().CalculateAction(new(eacl.ValidationUnit). WithEACLTable(&tb). WithContainerID(&cid). - WithRole(eacl.RoleOthers). + WithRole(eACLRole(role)). WithSenderKey(req.GetSignature().GetKey()). - WithOperation(op)) - if !found || action != eacl.ActionAllow { - return errors.New("operation denied by bearer eACL") + WithOperation(eaclOp)) + if !found { + return eACLErr(eaclOp, errors.New("not found allowing rules for the request")) + } else if action != eacl.ActionAllow { + return eACLErr(eaclOp, errors.New("DENY eACL rule")) } + return nil } @@ -132,3 +159,44 @@ func signMessage(m message, key *ecdsa.PrivateKey) error { return nil } + +func roleFromReq(cnr *core.Container, req message) (acl.Role, error) { + role := acl.RoleOthers + owner := cnr.Value.Owner() + + pub, err := keys.NewPublicKeyFromBytes(req.GetSignature().GetKey(), elliptic.P256()) + if err != nil { + return role, fmt.Errorf("invalid public key: %w", err) + } + + var reqSigner user.ID + user.IDFromKey(&reqSigner, (ecdsa.PublicKey)(*pub)) + + if reqSigner.Equals(owner) { + role = acl.RoleOwner + } + + return role, nil +} + +func eACLOp(op acl.Op) eacl.Operation { + switch op { + case acl.OpObjectGet: + return eacl.OperationGet + case acl.OpObjectPut: + return eacl.OperationPut + default: + panic(fmt.Sprintf("unexpected tree service ACL operation: %s", op)) + } +} + +func eACLRole(role acl.Role) eacl.Role { + switch role { + case acl.RoleOwner: + return eacl.RoleUser + case acl.RoleOthers: + return eacl.RoleOthers + default: + panic(fmt.Sprintf("unexpected tree service ACL role: %s", role)) + } +} diff --git a/pkg/services/tree/signature_test.go b/pkg/services/tree/signature_test.go index 9adfe7864..990e20a64 100644 --- a/pkg/services/tree/signature_test.go +++ b/pkg/services/tree/signature_test.go @@ -7,11 +7,12 @@ import ( "testing" "github.com/nspcc-dev/neo-go/pkg/crypto/keys" - "github.com/nspcc-dev/neofs-api-go/v2/acl" + aclV2 "github.com/nspcc-dev/neofs-api-go/v2/acl" containercore "github.com/nspcc-dev/neofs-node/pkg/core/container" "github.com/nspcc-dev/neofs-node/pkg/core/netmap" "github.com/nspcc-dev/neofs-sdk-go/bearer" "github.com/nspcc-dev/neofs-sdk-go/container" + "github.com/nspcc-dev/neofs-sdk-go/container/acl" cid "github.com/nspcc-dev/neofs-sdk-go/container/id" cidtest "github.com/nspcc-dev/neofs-sdk-go/container/id/test" eaclSDK "github.com/nspcc-dev/neofs-sdk-go/eacl" @@ -63,15 +64,17 @@ func TestMessageSign(t *testing.T) { var ownerID user.ID user.IDFromKey(&ownerID, (ecdsa.PublicKey)(*privs[0].PublicKey())) + cnr := &containercore.Container{ + Value: testContainer(ownerID), + } + s := &Service{ cfg: cfg{ log: zaptest.NewLogger(t), key: &privs[0].PrivateKey, nmSource: dummyNetmapSource{}, cnrSource: dummyContainerSource{ - cid1.String(): &containercore.Container{ - Value: testContainer(ownerID), - }, + cid1.String(): cnr, }, }, } @@ -90,26 +93,43 @@ func TestMessageSign(t *testing.T) { }, } + op := acl.OpObjectPut + cnr.Value.SetBasicACL(acl.PublicRW) + t.Run("missing signature, no panic", func(t *testing.T) { - require.Error(t, s.verifyClient(req, cid2, nil, eaclSDK.OperationUnknown)) + require.Error(t, s.verifyClient(req, cid2, nil, op)) }) require.NoError(t, signMessage(req, &privs[0].PrivateKey)) - require.NoError(t, s.verifyClient(req, cid1, nil, eaclSDK.OperationUnknown)) + require.NoError(t, s.verifyClient(req, cid1, nil, op)) t.Run("invalid CID", func(t *testing.T) { - require.Error(t, s.verifyClient(req, cid2, nil, eaclSDK.OperationUnknown)) + require.Error(t, s.verifyClient(req, cid2, nil, op)) }) + + cnr.Value.SetBasicACL(acl.Private) + + t.Run("extension disabled", func(t *testing.T) { + require.NoError(t, signMessage(req, &privs[0].PrivateKey)) + require.Error(t, s.verifyClient(req, cid2, nil, op)) + }) + t.Run("invalid key", func(t *testing.T) { require.NoError(t, signMessage(req, &privs[1].PrivateKey)) - require.Error(t, s.verifyClient(req, cid1, nil, eaclSDK.OperationUnknown)) + require.Error(t, s.verifyClient(req, cid1, nil, op)) }) t.Run("bearer", func(t *testing.T) { + bACL := acl.PrivateExtended + bACL.AllowBearerRules(op) + cnr.Value.SetBasicACL(bACL) + + bACL.DisableExtension() + t.Run("invalid bearer", func(t *testing.T) { req.Body.BearerToken = []byte{0xFF} require.NoError(t, signMessage(req, &privs[0].PrivateKey)) - require.Error(t, s.verifyClient(req, cid1, req.GetBody().GetBearerToken(), eaclSDK.OperationPut)) + require.Error(t, s.verifyClient(req, cid1, req.GetBody().GetBearerToken(), acl.OpObjectPut)) }) t.Run("invalid bearer CID", func(t *testing.T) { @@ -118,7 +138,7 @@ func TestMessageSign(t *testing.T) { req.Body.BearerToken = bt.Marshal() require.NoError(t, signMessage(req, &privs[1].PrivateKey)) - require.Error(t, s.verifyClient(req, cid1, req.GetBody().GetBearerToken(), eaclSDK.OperationPut)) + require.Error(t, s.verifyClient(req, cid1, req.GetBody().GetBearerToken(), acl.OpObjectPut)) }) t.Run("invalid bearer owner", func(t *testing.T) { bt := testBearerToken(cid1, privs[1].PublicKey(), privs[2].PublicKey()) @@ -126,47 +146,48 @@ func TestMessageSign(t *testing.T) { req.Body.BearerToken = bt.Marshal() require.NoError(t, signMessage(req, &privs[1].PrivateKey)) - require.Error(t, s.verifyClient(req, cid1, req.GetBody().GetBearerToken(), eaclSDK.OperationPut)) + require.Error(t, s.verifyClient(req, cid1, req.GetBody().GetBearerToken(), acl.OpObjectPut)) }) t.Run("invalid bearer signature", func(t *testing.T) { bt := testBearerToken(cid1, privs[1].PublicKey(), privs[2].PublicKey()) require.NoError(t, bt.Sign(privs[0].PrivateKey)) - var bv2 acl.BearerToken + var bv2 aclV2.BearerToken bt.WriteToV2(&bv2) bv2.GetSignature().SetSign([]byte{1, 2, 3}) req.Body.BearerToken = bv2.StableMarshal(nil) require.NoError(t, signMessage(req, &privs[1].PrivateKey)) - require.Error(t, s.verifyClient(req, cid1, req.GetBody().GetBearerToken(), eaclSDK.OperationPut)) + require.Error(t, s.verifyClient(req, cid1, req.GetBody().GetBearerToken(), acl.OpObjectPut)) }) bt := testBearerToken(cid1, privs[1].PublicKey(), privs[2].PublicKey()) require.NoError(t, bt.Sign(privs[0].PrivateKey)) req.Body.BearerToken = bt.Marshal() + cnr.Value.SetBasicACL(acl.PublicRWExtended) t.Run("put and get", func(t *testing.T) { require.NoError(t, signMessage(req, &privs[1].PrivateKey)) - require.NoError(t, s.verifyClient(req, cid1, req.GetBody().GetBearerToken(), eaclSDK.OperationPut)) - require.NoError(t, s.verifyClient(req, cid1, req.GetBody().GetBearerToken(), eaclSDK.OperationGet)) + require.NoError(t, s.verifyClient(req, cid1, req.GetBody().GetBearerToken(), acl.OpObjectPut)) + require.NoError(t, s.verifyClient(req, cid1, req.GetBody().GetBearerToken(), acl.OpObjectGet)) }) t.Run("only get", func(t *testing.T) { require.NoError(t, signMessage(req, &privs[2].PrivateKey)) - require.Error(t, s.verifyClient(req, cid1, req.GetBody().GetBearerToken(), eaclSDK.OperationPut)) - require.NoError(t, s.verifyClient(req, cid1, req.GetBody().GetBearerToken(), eaclSDK.OperationGet)) + require.Error(t, s.verifyClient(req, cid1, req.GetBody().GetBearerToken(), acl.OpObjectPut)) + require.NoError(t, s.verifyClient(req, cid1, req.GetBody().GetBearerToken(), acl.OpObjectGet)) }) t.Run("none", func(t *testing.T) { require.NoError(t, signMessage(req, &privs[3].PrivateKey)) - require.Error(t, s.verifyClient(req, cid1, req.GetBody().GetBearerToken(), eaclSDK.OperationPut)) - require.Error(t, s.verifyClient(req, cid1, req.GetBody().GetBearerToken(), eaclSDK.OperationGet)) + require.Error(t, s.verifyClient(req, cid1, req.GetBody().GetBearerToken(), acl.OpObjectPut)) + require.Error(t, s.verifyClient(req, cid1, req.GetBody().GetBearerToken(), acl.OpObjectGet)) }) }) } -func testBearerToken(cid cid.ID, forPut, forGet *keys.PublicKey) bearer.Token { +func testBearerToken(cid cid.ID, forPutGet, forGet *keys.PublicKey) bearer.Token { tgtGet := eaclSDK.NewTarget() tgtGet.SetRole(eaclSDK.RoleUnknown) - tgtGet.SetBinaryKeys([][]byte{forPut.Bytes(), forGet.Bytes()}) + tgtGet.SetBinaryKeys([][]byte{forPutGet.Bytes(), forGet.Bytes()}) rGet := eaclSDK.NewRecord() rGet.SetAction(eaclSDK.ActionAllow) @@ -175,7 +196,7 @@ func testBearerToken(cid cid.ID, forPut, forGet *keys.PublicKey) bearer.Token { tgtPut := eaclSDK.NewTarget() tgtPut.SetRole(eaclSDK.RoleUnknown) - tgtPut.SetBinaryKeys([][]byte{forPut.Bytes()}) + tgtPut.SetBinaryKeys([][]byte{forPutGet.Bytes()}) rPut := eaclSDK.NewRecord() rPut.SetAction(eaclSDK.ActionAllow)