Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

node/acl: fix acl checks for non-container nodes #2913

Merged
merged 1 commit into from
Aug 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@
"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 @@
// 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 @@
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) 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 @@

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 @@
}
}

// 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 @@
// 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 @@

irFetcher InnerRingFetcher

nm netmap.Source
nm Netmapper

next object.ServiceServer
}
Expand Down Expand Up @@ -458,6 +467,27 @@

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
Loading