Skip to content

Commit

Permalink
acl: Perceive tombstone saving as delete operation
Browse files Browse the repository at this point in the history
Previously, storage nodes handled `ObjectService.Put` requests
regardless of object types. Since writing tombstones is essentially an
operation of removing objects from their context, this opened up a
security issue: granting the right to write objects into the container
automatically granted the right to delete any objects from it. This, on
the one hand, implicit, on the other hand, strange behavior would raise
obvious questions about practical access control in the system.

From now, `PUT` ops of `TOMBSTONE` objects are treated as `DELETE`. The
only exclusion is intra-container replication that still should be
performed in `PUT` manner (container nodes have no removal rights).

Closes #2261.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
  • Loading branch information
cthulhu-rider committed Feb 27, 2024
1 parent 9756cc4 commit a7eb6ec
Show file tree
Hide file tree
Showing 4 changed files with 21 additions and 3 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ Changelog for NeoFS Node
### Added

### Fixed
- Access to `PUT` objects no longer grants `DELETE` rights (#2261)

### Changed

Expand Down
19 changes: 18 additions & 1 deletion pkg/services/object/acl/v2/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -511,11 +511,28 @@ func (p putStreamBasicChecker) Send(request *objectV2.PutRequest) error {
src: request,
}

reqInfo, err := p.source.findRequestInfo(req, cnr, acl.OpObjectPut)
verb := acl.OpObjectPut
tombstone := part.GetHeader().GetObjectType() == objectV2.TypeTombstone
if tombstone {

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

View check run for this annotation

Codecov / codecov/patch

pkg/services/object/acl/v2/service.go#L514-L516

Added lines #L514 - L516 were not covered by tests
// such objects are specific - saving them is essentially the removal of other
// objects
verb = acl.OpObjectDelete

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

View check run for this annotation

Codecov / codecov/patch

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

Added line #L519 was not covered by tests
}

reqInfo, err := p.source.findRequestInfo(req, cnr, verb)

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

View check run for this annotation

Codecov / codecov/patch

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

Added line #L522 was not covered by tests
if err != nil {
return err
}

if tombstone {

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

View check run for this annotation

Codecov / codecov/patch

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

Added line #L527 was not covered by tests
// the only exception when writing tombstone should not be treated as deletion
// is intra-container replication: container nodes must be able to replicate
// such objects while deleting is prohibited
if reqInfo.requestRole == acl.RoleContainer && request.GetMetaHeader().GetTTL() == 1 {
reqInfo.operation = acl.OpObjectPut

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

View check run for this annotation

Codecov / codecov/patch

pkg/services/object/acl/v2/service.go#L531-L532

Added lines #L531 - L532 were not covered by tests
}
}

reqInfo.obj = obj

if !p.source.checker.CheckBasicACL(reqInfo) || !p.source.checker.StickyBitCheck(reqInfo, idOwner) {
Expand Down
2 changes: 1 addition & 1 deletion pkg/services/object/acl/v2/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ func assertVerb(tok sessionSDK.Object, op acl.Op) bool {
//nolint:exhaustive
switch op {
case acl.OpObjectPut:
return tok.AssertVerb(sessionSDK.VerbObjectPut, sessionSDK.VerbObjectDelete)
return tok.AssertVerb(sessionSDK.VerbObjectPut)
case acl.OpObjectDelete:
return tok.AssertVerb(sessionSDK.VerbObjectDelete)
case acl.OpObjectGet:
Expand Down
2 changes: 1 addition & 1 deletion pkg/services/object/acl/v2/util_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ func testGenerateMetaHeader(depth uint32, b *acl.BearerToken, s *session.Token)
func TestIsVerbCompatible(t *testing.T) {
// Source: https://nspcc.ru/upload/neofs-spec-latest.pdf#page=28
table := map[aclsdk.Op][]sessionSDK.ObjectVerb{
aclsdk.OpObjectPut: {sessionSDK.VerbObjectPut, sessionSDK.VerbObjectDelete},
aclsdk.OpObjectPut: {sessionSDK.VerbObjectPut},
aclsdk.OpObjectDelete: {sessionSDK.VerbObjectDelete},
aclsdk.OpObjectGet: {sessionSDK.VerbObjectGet},
aclsdk.OpObjectHead: {
Expand Down

0 comments on commit a7eb6ec

Please sign in to comment.