Skip to content

Commit

Permalink
node/acl: fix acl checks for non-container nodes
Browse files Browse the repository at this point in the history
Split objects are validated and it required to do some additional requests for
it. Such operation can be restricted for a non-container node, so checking for
it is an unsolvable problem, skip ACL checks in such cases.
Closes #2909.

Signed-off-by: Pavel Karpy <[email protected]>
  • Loading branch information
carpawell committed Aug 9, 2024
1 parent 0cbd3d0 commit 4756b5c
Show file tree
Hide file tree
Showing 4 changed files with 63 additions and 7 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ Changelog for NeoFS Node
- Control service's Drop call does not clean metabase (#2822)
- It was impossible to specify memory amount as "1b" (one byte) in config, default was used instead (#2899)
- Container session token's lifetime was not checked (#2898)
- ACL checks for split objects could be forced by a node than might lack access (#2909)

### Changed
- neofs-cli allows several objects deletion at a time (#2774)
Expand Down
32 changes: 29 additions & 3 deletions cmd/neofs-node/object.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"context"
"errors"
"fmt"
"strings"

lru "github.com/hashicorp/golang-lru/v2"
"github.com/nspcc-dev/neofs-api-go/v2/object"
Expand Down Expand Up @@ -312,11 +313,12 @@ func initObjectService(c *cfg) {
// every object part in every chain will try to refer to the first part, so caching
// should help a lot here
const cachedFirstObjectsNumber = 1000
objNode := newNodeForObjects(cnrNodes, sPut, c.IsLocalKey)

Check warning on line 316 in cmd/neofs-node/object.go

View check run for this annotation

Codecov / codecov/patch

cmd/neofs-node/object.go#L316

Added line #L316 was not covered by tests

aclSvc := v2.New(
v2.WithLogger(c.log),
v2.WithIRFetcher(newCachedIRFetcher(irFetcher)),
v2.WithNetmapSource(c.netMapSource),
v2.WithNetmapper(netmapSourceWithNodes{Source: c.netMapSource, n: objNode}),

Check warning on line 321 in cmd/neofs-node/object.go

View check run for this annotation

Codecov / codecov/patch

cmd/neofs-node/object.go#L321

Added line #L321 was not covered by tests
v2.WithContainerSource(
c.cfgObject.cnrSource,
),
Expand Down Expand Up @@ -350,8 +352,6 @@ func initObjectService(c *cfg) {
firstSvc = objectService.NewMetricCollector(signSvc, c.metricsCollector)
}

objNode := newNodeForObjects(cnrNodes, sPut, c.IsLocalKey)

server := objectTransportGRPC.New(firstSvc, objNode)

for _, srv := range c.cfgGRPC.servers {
Expand Down Expand Up @@ -739,3 +739,29 @@ func (c *cfg) IsLocalNodePublicKey(b []byte) bool { return c.IsLocalKey(b) }
func (c *cfg) GetNodesForObject(addr oid.Address) ([][]netmapsdk.NodeInfo, []uint, error) {
return c.cfgObject.containerNodes.getNodesForObject(addr)
}

type netmapSourceWithNodes struct {
netmap.Source
n *nodeForObjects
}

func (n netmapSourceWithNodes) ServerInContainer(cID cid.ID) (bool, error) {
var serverInContainer bool
err := n.n.ForEachContainerNodePublicKeyInLastTwoEpochs(cID, func(pubKey []byte) bool {
if n.n.isLocalPubKey(pubKey) {
serverInContainer = true
return false

Check warning on line 753 in cmd/neofs-node/object.go

View check run for this annotation

Codecov / codecov/patch

cmd/neofs-node/object.go#L748-L753

Added lines #L748 - L753 were not covered by tests
}
return true

Check warning on line 755 in cmd/neofs-node/object.go

View check run for this annotation

Codecov / codecov/patch

cmd/neofs-node/object.go#L755

Added line #L755 was not covered by tests
})
if err != nil {

Check warning on line 757 in cmd/neofs-node/object.go

View check run for this annotation

Codecov / codecov/patch

cmd/neofs-node/object.go#L757

Added line #L757 was not covered by tests
// https://github.com/nspcc-dev/neofs-sdk-go/pull/615
if strings.Contains(err.Error(), "not enough nodes to SELECT from") {
return false, nil

Check warning on line 760 in cmd/neofs-node/object.go

View check run for this annotation

Codecov / codecov/patch

cmd/neofs-node/object.go#L759-L760

Added lines #L759 - L760 were not covered by tests
}

return false, err

Check warning on line 763 in cmd/neofs-node/object.go

View check run for this annotation

Codecov / codecov/patch

cmd/neofs-node/object.go#L763

Added line #L763 was not covered by tests
}

return serverInContainer, nil

Check warning on line 766 in cmd/neofs-node/object.go

View check run for this annotation

Codecov / codecov/patch

cmd/neofs-node/object.go#L766

Added line #L766 was not covered by tests
}
5 changes: 2 additions & 3 deletions pkg/services/object/acl/v2/opts.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package v2

import (
"github.com/nspcc-dev/neofs-node/pkg/core/container"
"github.com/nspcc-dev/neofs-node/pkg/core/netmap"
objectSvc "github.com/nspcc-dev/neofs-node/pkg/services/object"
"go.uber.org/zap"
)
Expand All @@ -14,9 +13,9 @@ func WithLogger(v *zap.Logger) Option {
}
}

// WithNetmapSource return option to set
// WithNetmapper return option to set
// netmap source.
func WithNetmapSource(v netmap.Source) Option {
func WithNetmapper(v Netmapper) Option {

Check warning on line 18 in pkg/services/object/acl/v2/opts.go

View check run for this annotation

Codecov / codecov/patch

pkg/services/object/acl/v2/opts.go#L18

Added line #L18 was not covered by tests
return func(c *cfg) {
c.nm = v
}
Expand Down
32 changes: 31 additions & 1 deletion pkg/services/object/acl/v2/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,15 @@ type searchStreamBasicChecker struct {
// Option represents Service constructor option.
type Option func(*cfg)

// Netmapper must provide network map information.
type Netmapper interface {
netmap.Source
// ServerInContainer checks if current node belongs to requested container.
// Any unknown state must be returned as `(false, error.New("explanation"))`,
// not `(false, nil)`.
ServerInContainer(cid.ID) (bool, error)
}

type cfg struct {
log *zap.Logger

Expand All @@ -67,7 +76,7 @@ type cfg struct {

irFetcher InnerRingFetcher

nm netmap.Source
nm Netmapper

next object.ServiceServer
}
Expand Down Expand Up @@ -458,6 +467,27 @@ func (p putStreamBasicChecker) Send(request *objectV2.PutRequest) error {

header := part.GetHeader()

cIDV2 := header.GetContainerID().GetValue()
var cID cid.ID
err = cID.Decode(cIDV2)
if err != nil {
return fmt.Errorf("invalid container ID: %w", err)

Check warning on line 474 in pkg/services/object/acl/v2/service.go

View check run for this annotation

Codecov / codecov/patch

pkg/services/object/acl/v2/service.go#L470-L474

Added lines #L470 - L474 were not covered by tests
}

inContainer, err := p.source.nm.ServerInContainer(cID)
if err != nil {
return fmt.Errorf("checking if node in container: %w", err)

Check warning on line 479 in pkg/services/object/acl/v2/service.go

View check run for this annotation

Codecov / codecov/patch

pkg/services/object/acl/v2/service.go#L477-L479

Added lines #L477 - L479 were not covered by tests
}

if header.GetSplit() != nil && !inContainer {

Check warning on line 482 in pkg/services/object/acl/v2/service.go

View check run for this annotation

Codecov / codecov/patch

pkg/services/object/acl/v2/service.go#L482

Added line #L482 was not covered by tests
// skip ACL checks for split objects if it is not a container
// node, since it requires additional object operations (e.g.,
// requesting the other split parts) that may (or may not) be
// prohibited by ACL rules; this node is not going to store such
// objects anyway
return p.next.Send(request)

Check warning on line 488 in pkg/services/object/acl/v2/service.go

View check run for this annotation

Codecov / codecov/patch

pkg/services/object/acl/v2/service.go#L488

Added line #L488 was not covered by tests
}

idV2 := header.GetOwnerID()
if idV2 == nil {
return errors.New("missing object owner")
Expand Down

0 comments on commit 4756b5c

Please sign in to comment.