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

eacl: Support new operators #2742

Merged
merged 4 commits into from
Feb 29, 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
7 changes: 7 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,16 @@ Changelog for NeoFS Node
### Added
- Support of numeric object search queries (#2733)
- Support of `GT`, `GE`, `LT` and `LE` numeric comparison operators in CLI (#2733)
- SN eACL processing of NULL and numeric operators (#2742)
carpawell marked this conversation as resolved.
Show resolved Hide resolved
- CLI now allows to create and print eACL with numeric filters (#2742)

### Fixed
- Access to `PUT` objects no longer grants `DELETE` rights (#2261)
- Storage nodes no longer reject GET w/ TTL=1 requests to read complex objects (#2447)

### Changed
- IR now checks format of NULL and numeric eACL filters specified in the protocol (#2742)
- Empty filter value is now treated as `NOT_PRESENT` op by CLI `acl extended create` cmd (#2742)

### Removed
- Object notifications incl. NATS (#2750)
Expand All @@ -37,6 +41,9 @@ raw binaries. All binaries have OS in their names as well now, following
regular naming used throughout NSPCC, so instead of neofs-cli-amd64 you get
neofs-cli-linux-amd64 now.

CLI command `acl extended create` changed and extended input format for filters.
For example, `attr>=100` or `attr=` are now processed differently. See `-h` for details.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"See -h"? see help? use -h flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see help text

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-h prints help text that u can see

Copy link
Member

@carpawell carpawell Feb 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i mean i cant remember i saw somewhere "see -h". it is usually "see help" or "use -h flag for details"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i bet anybody will understand what is this about


## [0.40.0] - 2024-02-09 - Maldo

### Added
Expand Down
9 changes: 6 additions & 3 deletions cmd/neofs-cli/modules/acl/extended/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,11 @@ Filter consists of <typ>:<key><match><value>
Well-known system object headers start with '$Object:' prefix.
User defined headers start without prefix.
Read more about filter keys at github.com/nspcc-dev/neofs-api/blob/master/proto-docs/acl.md#message-eaclrecordfilter
Match is '=' for matching and '!=' for non-matching filter.
Value is a valid unicode string corresponding to object or request header value.
Match is:
'=' for string equality or, if no value, attribute absence;
'!=' for string inequality;
'>' | '>=' | '<' | '<=' for integer comparison.
Value is a valid unicode string corresponding to object or request header value. Numeric filters must have base-10 integer values.

Target is
'user' for container owner,
Expand All @@ -43,7 +46,7 @@ Target is
When both '--rule' and '--file' arguments are used, '--rule' records will be placed higher in resulting extended ACL table.
`,
Example: `neofs-cli acl extended create --cid EutHBsdT1YCzHxjCfQHnLPL1vFrkSyLSio4vkphfnEk -f rules.txt --out table.json
neofs-cli acl extended create --cid EutHBsdT1YCzHxjCfQHnLPL1vFrkSyLSio4vkphfnEk -r 'allow get obj:Key=Value others' -r 'deny put others'`,
neofs-cli acl extended create --cid EutHBsdT1YCzHxjCfQHnLPL1vFrkSyLSio4vkphfnEk -r 'allow get obj:Key=Value others' -r 'deny put others' -r 'deny put obj:$Object:payloadLength<4096 others' -r 'deny get obj:Quality>=100 others'`,
Args: cobra.NoArgs,
Run: createEACL,
}
Expand Down
96 changes: 80 additions & 16 deletions cmd/neofs-cli/modules/util/acl.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
"encoding/hex"
"errors"
"fmt"
"math/big"
"strings"
"text/tabwriter"

Expand Down Expand Up @@ -140,6 +141,16 @@
_, _ = tw.Write([]byte("\t==\t"))
case eacl.MatchStringNotEqual:
_, _ = tw.Write([]byte("\t!=\t"))
case eacl.MatchNumGT:
_, _ = tw.Write([]byte("\t>\t"))
case eacl.MatchNumGE:
_, _ = tw.Write([]byte("\t>=\t"))
case eacl.MatchNumLT:
_, _ = tw.Write([]byte("\t<\t"))
case eacl.MatchNumLE:
_, _ = tw.Write([]byte("\t<=\t"))
case eacl.MatchNotPresent:
_, _ = tw.Write([]byte("\tNULL\t"))

Check warning on line 153 in cmd/neofs-cli/modules/util/acl.go

View check run for this annotation

Codecov / codecov/patch

cmd/neofs-cli/modules/util/acl.go#L144-L153

Added lines #L144 - L153 were not covered by tests
case eacl.MatchUnknown:
}

Expand Down Expand Up @@ -235,24 +246,11 @@
return nil, fmt.Errorf("invalid filter or target: %s", args[i])
}

i := strings.Index(ss[1], "=")
if i < 0 {
return nil, fmt.Errorf("invalid filter key-value pair: %s", ss[1])
key, value, op, err := parseKVWithOp(ss[1])
if err != nil {
return nil, fmt.Errorf("invalid filter key-value pair %s: %w", ss[1], err)

Check warning on line 251 in cmd/neofs-cli/modules/util/acl.go

View check run for this annotation

Codecov / codecov/patch

cmd/neofs-cli/modules/util/acl.go#L249-L251

Added lines #L249 - L251 were not covered by tests
}

var key, value string
var op eacl.Match

if 0 < i && ss[1][i-1] == '!' {
key = ss[1][:i-1]
op = eacl.MatchStringNotEqual
} else {
key = ss[1][:i]
op = eacl.MatchStringEqual
}

value = ss[1][i+1:]

typ := eacl.HeaderFromRequest
if ss[0] == "obj" {
typ = eacl.HeaderFromObject
Expand Down Expand Up @@ -288,6 +286,57 @@
return r, nil
}

func parseKVWithOp(s string) (string, string, eacl.Match, error) {
i := strings.Index(s, "=")
if i < 0 {
if i = strings.Index(s, "<"); i >= 0 {
if !validateDecimal(s[i+1:]) {
return "", "", 0, fmt.Errorf("invalid base-10 integer value %q for attribute %q", s[i+1:], s[:i])
}
return s[:i], s[i+1:], eacl.MatchNumLT, nil
carpawell marked this conversation as resolved.
Show resolved Hide resolved
} else if i = strings.Index(s, ">"); i >= 0 {
if !validateDecimal(s[i+1:]) {
return "", "", 0, fmt.Errorf("invalid base-10 integer value %q for attribute %q", s[i+1:], s[:i])
}
return s[:i], s[i+1:], eacl.MatchNumGT, nil
}

return "", "", 0, errors.New("missing op")
}

if len(s[i+1:]) == 0 {
return s[:i], "", eacl.MatchNotPresent, nil
}

value := s[i+1:]

if i == 0 {
return "", value, eacl.MatchStringEqual, nil
}

switch s[i-1] {
roman-khimov marked this conversation as resolved.
Show resolved Hide resolved
case '!':
return s[:i-1], value, eacl.MatchStringNotEqual, nil
case '<':
if !validateDecimal(value) {
return "", "", 0, fmt.Errorf("invalid base-10 integer value %q for attribute %q", value, s[:i-1])
}
return s[:i-1], value, eacl.MatchNumLE, nil
case '>':
if !validateDecimal(value) {
return "", "", 0, fmt.Errorf("invalid base-10 integer value %q for attribute %q", value, s[:i-1])
}
return s[:i-1], value, eacl.MatchNumGE, nil
default:
return s[:i], value, eacl.MatchStringEqual, nil
}
}

func validateDecimal(s string) bool {
_, ok := new(big.Int).SetString(s, 10)
return ok
}

// eaclRoleFromString parses eacl.Role from string.
func eaclRoleFromString(s string) (eacl.Role, error) {
var r eacl.Role
Expand Down Expand Up @@ -332,12 +381,27 @@
// ValidateEACLTable validates eACL table:
// - eACL table must not modify [eacl.RoleSystem] access.
func ValidateEACLTable(t *eacl.Table) error {
var b big.Int
for _, record := range t.Records() {
for _, target := range record.Targets() {
if target.Role() == eacl.RoleSystem {
return errors.New("it is prohibited to modify system access")
}
}
for _, f := range record.Filters() {
//nolint:exhaustive
switch f.Matcher() {
case eacl.MatchNotPresent:
if len(f.Value()) != 0 {
return errors.New("non-empty value in absence filter")
}
case eacl.MatchNumGT, eacl.MatchNumGE, eacl.MatchNumLT, eacl.MatchNumLE:
_, ok := b.SetString(f.Value(), 10)
if !ok {
return errors.New("numeric filter with non-decimal value")
}
}
}
}

return nil
Expand Down
136 changes: 136 additions & 0 deletions cmd/neofs-cli/modules/util/acl_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
package util

import (
"testing"

"github.com/nspcc-dev/neofs-sdk-go/eacl"
"github.com/stretchr/testify/require"
)

func TestParseKVWithOp(t *testing.T) {
for _, tc := range []struct {
s string
k string
op eacl.Match
v string
}{
{"=", "", eacl.MatchNotPresent, ""},
{"!=", "!", eacl.MatchNotPresent, ""},
{">1234567890", "", eacl.MatchNumGT, "1234567890"},
{"<1234567890", "", eacl.MatchNumLT, "1234567890"},
{">=1234567890", "", eacl.MatchNumGE, "1234567890"},
{"=>", "", eacl.MatchStringEqual, ">"},
{"=<", "", eacl.MatchStringEqual, "<"},
{"key=", "key", eacl.MatchNotPresent, ""},
{"key>=", "key>", eacl.MatchNotPresent, ""},
{"key<=", "key<", eacl.MatchNotPresent, ""},
{"=value", "", eacl.MatchStringEqual, "value"},
{"!=value", "", eacl.MatchStringNotEqual, "value"},
{"key=value", "key", eacl.MatchStringEqual, "value"},
{"key>1234567890", "key", eacl.MatchNumGT, "1234567890"},
{"key<1234567890", "key", eacl.MatchNumLT, "1234567890"},
{"key==value", "key", eacl.MatchStringEqual, "=value"},
{"key=>value", "key", eacl.MatchStringEqual, ">value"},
{"key>=1234567890", "key", eacl.MatchNumGE, "1234567890"},
{"key<=1234567890", "key", eacl.MatchNumLE, "1234567890"},
{"key=<value", "key", eacl.MatchStringEqual, "<value"},
{"key!=value", "key", eacl.MatchStringNotEqual, "value"},
{"key=!value", "key", eacl.MatchStringEqual, "!value"},
{"key!=!value", "key", eacl.MatchStringNotEqual, "!value"},
{"key!!=value", "key!", eacl.MatchStringNotEqual, "value"},
{"key!=!=value", "key", eacl.MatchStringNotEqual, "!=value"},
{"key=value=42", "key", eacl.MatchStringEqual, "value=42"},
{"key!=value!=42", "key", eacl.MatchStringNotEqual, "value!=42"},
{"k e y = v a l u e", "k e y ", eacl.MatchStringEqual, " v a l u e"},
{"k e y != v a l u e", "k e y ", eacl.MatchStringNotEqual, " v a l u e"},
} {
k, v, op, err := parseKVWithOp(tc.s)
require.NoError(t, err, tc)
require.Equal(t, tc.k, k, tc)
require.Equal(t, tc.v, v, tc)
require.Equal(t, tc.op, op, tc)
}

for _, tc := range []struct {
s string
e string
}{
{"", "missing op"},
{"!", "missing op"},
{">", "invalid base-10 integer value \"\" for attribute \"\""},
{">1.2", "invalid base-10 integer value \"1.2\" for attribute \"\""},
{">=1.2", "invalid base-10 integer value \"1.2\" for attribute \"\""},
{"<", "invalid base-10 integer value \"\" for attribute \"\""},
{"<1.2", "invalid base-10 integer value \"1.2\" for attribute \"\""},
{"<=1.2", "invalid base-10 integer value \"1.2\" for attribute \"\""},
{"k", "missing op"},
{"k!", "missing op"},
{"k>", "invalid base-10 integer value \"\" for attribute \"k\""},
{"k<", "invalid base-10 integer value \"\" for attribute \"k\""},
{"k!v", "missing op"},
{"k<v", "invalid base-10 integer value \"v\" for attribute \"k\""},
{"k<=v", "invalid base-10 integer value \"v\" for attribute \"k\""},
{"k>=v", "invalid base-10 integer value \"v\" for attribute \"k\""},
} {
_, _, _, err := parseKVWithOp(tc.s)
require.ErrorContains(t, err, tc.e, tc)
}
}

var allNumMatchers = []eacl.Match{eacl.MatchNumGT, eacl.MatchNumGE, eacl.MatchNumLT, eacl.MatchNumLE}

func anyValidEACL() eacl.Table {
return eacl.Table{}
}

func TestValidateEACL(t *testing.T) {
t.Run("absence matcher", func(t *testing.T) {
var r eacl.Record
r.AddObjectAttributeFilter(eacl.MatchNotPresent, "any_key", "any_value")
tb := anyValidEACL()
tb.AddRecord(&r)

err := ValidateEACLTable(&tb)
require.ErrorContains(t, err, "non-empty value in absence filter")

r = eacl.Record{}
r.AddObjectAttributeFilter(eacl.MatchNotPresent, "any_key", "")
tb = anyValidEACL()
tb.AddRecord(&r)

err = ValidateEACLTable(&tb)
require.NoError(t, err)
})

t.Run("numeric matchers", func(t *testing.T) {
for _, tc := range []struct {
ok bool
v string
}{
{false, "not a base-10 integer"},
{false, "1.2"},
{false, ""},
{true, "01"},
{true, "0"},
{true, "01"},
{true, "-0"},
{true, "-01"},
{true, "1111111111111111111111111111111111111111111111"},
{true, "-1111111111111111111111111111111111111111111111"},
} {
for _, m := range allNumMatchers {
var r eacl.Record
r.AddObjectAttributeFilter(m, "any_key", tc.v)
tb := anyValidEACL()
tb.AddRecord(&r)

err := ValidateEACLTable(&tb)
if tc.ok {
require.NoError(t, err, [2]any{m, tc})
} else {
require.ErrorContains(t, err, "numeric filter with non-decimal value", [2]any{m, tc})
}
}
}
})
}
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ require (
github.com/nspcc-dev/neo-go v0.105.1
github.com/nspcc-dev/neofs-api-go/v2 v2.14.1-0.20240213170208-cfca09b5acbe
github.com/nspcc-dev/neofs-contract v0.19.1
github.com/nspcc-dev/neofs-sdk-go v1.0.0-rc.11.0.20240220193911-24254bf9aebe
github.com/nspcc-dev/neofs-sdk-go v1.0.0-rc.11.0.20240221185518-cbaf23c6aa7a
github.com/nspcc-dev/tzhash v1.7.1
github.com/olekukonko/tablewriter v0.0.5
github.com/panjf2000/ants/v2 v2.8.2
Expand Down
4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -251,8 +251,8 @@ github.com/nspcc-dev/neofs-contract v0.19.1 h1:U1Uh+MlzfkalO0kRJ2pADZyHrmAOroC6K
github.com/nspcc-dev/neofs-contract v0.19.1/go.mod h1:ZOGouuwuHpgvYkx/LCGufGncIzEUhYEO18LL4cWEbyw=
github.com/nspcc-dev/neofs-crypto v0.4.0 h1:5LlrUAM5O0k1+sH/sktBtrgfWtq1pgpDs09fZo+KYi4=
github.com/nspcc-dev/neofs-crypto v0.4.0/go.mod h1:6XJ8kbXgOfevbI2WMruOtI+qUJXNwSGM/E9eClXxPHs=
github.com/nspcc-dev/neofs-sdk-go v1.0.0-rc.11.0.20240220193911-24254bf9aebe h1:VCJyY86/CSMTRjiXAPGGwRlss3FVGSGHZirCjQZzN2E=
github.com/nspcc-dev/neofs-sdk-go v1.0.0-rc.11.0.20240220193911-24254bf9aebe/go.mod h1:icGhc6HFg+yKivBUoP7cut62SASuijDiWD5Txd6vWqY=
github.com/nspcc-dev/neofs-sdk-go v1.0.0-rc.11.0.20240221185518-cbaf23c6aa7a h1:vmN8Sm8Wna5BrgkGBvt5cnPTzU4Fu0JzC6VnwDNiDIA=
github.com/nspcc-dev/neofs-sdk-go v1.0.0-rc.11.0.20240221185518-cbaf23c6aa7a/go.mod h1:icGhc6HFg+yKivBUoP7cut62SASuijDiWD5Txd6vWqY=
github.com/nspcc-dev/rfc6979 v0.2.0 h1:3e1WNxrN60/6N0DW7+UYisLeZJyfqZTNOjeV/toYvOE=
github.com/nspcc-dev/rfc6979 v0.2.0/go.mod h1:exhIh1PdpDC5vQmyEsGvc4YDM/lyQp/452QxGq/UEso=
github.com/nspcc-dev/tzhash v1.7.1 h1:6zmexLqdTF/ssbUAh7XJS7RxgKWaw28kdNpE/4UFdEU=
Expand Down
20 changes: 18 additions & 2 deletions pkg/innerring/processors/container/process_eacl.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import (
"errors"
"fmt"
"math/big"

cntClient "github.com/nspcc-dev/neofs-node/pkg/morph/client/container"
"github.com/nspcc-dev/neofs-node/pkg/morph/event/container"
Expand Down Expand Up @@ -40,7 +41,7 @@
return fmt.Errorf("invalid binary table: %w", err)
}

err = validateEACl(table)
err = validateEACL(table)

Check warning on line 44 in pkg/innerring/processors/container/process_eacl.go

View check run for this annotation

Codecov / codecov/patch

pkg/innerring/processors/container/process_eacl.go#L44

Added line #L44 was not covered by tests
if err != nil {
return fmt.Errorf("table validation: %w", err)
}
Expand Down Expand Up @@ -98,13 +99,28 @@
}
}

func validateEACl(t *eacl.Table) error {
func validateEACL(t *eacl.Table) error {
var b big.Int
for _, record := range t.Records() {
for _, target := range record.Targets() {
if target.Role() == eacl.RoleSystem {
return errors.New("it is prohibited to modify system access")
}
}
for _, f := range record.Filters() {
//nolint:exhaustive
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not two ifs or default?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not two ifs cuz more than 2 values, not default cuz it'd be empty. IMO that's not the case where linter suggests smth meaningful

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not two ifs cuz more than 2 values

every case has return, still can be with ifs

not default cuz it'd be empty

more regular expression for a regular go reader (even more if a comment is just nolint without some real comment). both are one line long (deafult always shorter)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, can be merged, but i do not understand it (not disagree just don't understand why it is easier to do than one of my suggestion)

Copy link
Contributor Author

@cthulhu-rider cthulhu-rider Feb 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more regular expression for a regular go reader

empty default is useless and always skipped, that's what i see as a regular Go reader

switch f.Matcher() {
case eacl.MatchNotPresent:
if len(f.Value()) != 0 {
return errors.New("non-empty value in absence filter")
}
case eacl.MatchNumGT, eacl.MatchNumGE, eacl.MatchNumLT, eacl.MatchNumLE:
_, ok := b.SetString(f.Value(), 10)
if !ok {
return errors.New("numeric filter with non-decimal value")
}
}
}
}

return nil
Expand Down
Loading
Loading