TrueCloudLab
/
policy-engine
generated from TrueCloudLab/basic
19
0
Fork 9
Code Issues 3 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

[#60] chain: Support numeric conditions #60

Merged
dkirillov merged 1 commits from mbiryukova/policy-engine:feature/numeric_conditions into master 2024-04-08 14:32:14 +00:00
Conversation 0 Commits 1 Files Changed 5
5 changed files with 270 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

31
iam/converter.go
View File

@ -10,6 +10,7 @@ import (
"git.frostfs.info/TrueCloudLab/policy-engine/pkg/chain"
"git.frostfs.info/TrueCloudLab/policy-engine/schema/common"
"github.com/nspcc-dev/neo-go/pkg/encoding/fixedn"
)
const (
@ -233,8 +234,7 @@ func getConditionTypeAndConverter(op string) (chain.ConditionType, convertFuncti
return 0, nil, fmt.Errorf("unsupported condition operator: '%s'", op)
}
case strings.HasPrefix(op, "Numeric"):
// TODO
return 0, nil, fmt.Errorf("currently nummeric conditions unsupported: '%s'", op)
return numericConditionTypeAndConverter(op)
case strings.HasPrefix(op, "Date"):
switch op {
case CondDateEquals:
@ -269,12 +269,39 @@ func getConditionTypeAndConverter(op string) (chain.ConditionType, convertFuncti
}
}
func numericConditionTypeAndConverter(op string) (chain.ConditionType, convertFunction, error) {
switch op {
case CondNumericEquals:
return chain.CondNumericEquals, numericConvertFunction, nil
case CondNumericNotEquals:
return chain.CondNumericNotEquals, numericConvertFunction, nil
case CondNumericLessThan:
return chain.CondNumericLessThan, numericConvertFunction, nil
case CondNumericLessThanEquals:
return chain.CondNumericLessThanEquals, numericConvertFunction, nil
case CondNumericGreaterThan:
return chain.CondNumericGreaterThan, numericConvertFunction, nil
case CondNumericGreaterThanEquals:
return chain.CondNumericGreaterThanEquals, numericConvertFunction, nil
default:
return 0, nil, fmt.Errorf("unsupported condition operator: '%s'", op)
}
}
type convertFunction func(string) (string, error)
func noConvertFunction(val string) (string, error) {
return val, nil
}
func numericConvertFunction(val string) (string, error) {
dstepanov-yadro commented 2024-04-08 08:50:12 +00:00
Outdated
Review

+Inf, inf, NaN, nan are valid float strings, see https://pkg.go.dev/strconv#example-ParseFloat
Does IAM supports it?

`+Inf`, `inf`, `NaN`, `nan` are valid float strings, see https://pkg.go.dev/strconv#example-ParseFloat Does IAM supports it?
dkirillov commented 2024-04-08 09:58:19 +00:00
Outdated
Review

No. But now we use Fixed8 and it doesn't support it.

Probably we should include this +Inf, inf, NaN, nan in test cases

No. But now we use [Fixed8](https://github.com/nspcc-dev/neo-go/blob/master/pkg/encoding/fixedn/fixed8.go) and it doesn't support it. Probably we should include this `+Inf, inf, NaN, nan` in test cases
if _, err := fixedn.Fixed8FromString(val); err == nil {
return val, nil
}
return "", fmt.Errorf("invalid numeric value: '%s'", val)
}
func dateConvertFunction(val string) (string, error) {
if _, err := strconv.ParseInt(val, 10, 64); err == nil {
return val, nil

71
iam/converter_test.go
View File

@ -385,6 +385,12 @@ func TestConvertToChainCondition(t *testing.T) {
CondArnLike: {condKeyAWSPrincipalARN: {principal}},
CondArnNotEquals: {"key18": {"val18"}},
CondArnNotLike: {"key19": {"val19"}},
CondNumericEquals: {"key20": {"-20"}},
dstepanov-yadro commented 2024-04-02 10:38:17 +00:00
Outdated
Review

negative, zero, hex, binary values tests should be added.
also values with spaces, with '+' sign.

negative, zero, hex, binary values tests should be added. also values with spaces, with '+' sign.
dkirillov commented 2024-04-02 12:55:55 +00:00
Outdated
Review

AWS supports only integer and decimals. So here (in converter_test.go) I wouldn't add hex/binary values.
If this is required we can support such values in native condition types and add tests there.

AWS supports only integer and decimals. So here (in `converter_test.go`) I wouldn't add hex/binary values. If this is required we can support such values in native condition types and add tests there.
dstepanov-yadro commented 2024-04-03 12:50:42 +00:00
Outdated
Review

Tests should not be only positive. I was expecting to see a negative binary and hex test. Also we need to support decimals)

Tests should not be only positive. I was expecting to see a negative binary and hex test. Also we need to support decimals)
CondNumericNotEquals: {"key21": {"+21"}},
CondNumericLessThan: {"key22": {"0"}},
CondNumericLessThanEquals: {"key23": {"23.23"}},
CondNumericGreaterThan: {"key24": {"-24.24"}},
CondNumericGreaterThanEquals: {"key25": {"+25.25"}},
}
expectedCondition := []GroupedConditions{
@ -549,11 +555,76 @@ func TestConvertToChainCondition(t *testing.T) {
Value: "val19",
}},
},
{
Conditions: []chain.Condition{{
Op: chain.CondNumericEquals,
Object: chain.ObjectRequest,
Key: "key20",
Value: "-20",
}},
},
{
Conditions: []chain.Condition{{
Op: chain.CondNumericNotEquals,
Object: chain.ObjectRequest,
Key: "key21",
Value: "+21",
}},
},
{
Conditions: []chain.Condition{{
Op: chain.CondNumericLessThan,
Object: chain.ObjectRequest,
Key: "key22",
Value: "0",
}},
},
{
Conditions: []chain.Condition{{
Op: chain.CondNumericLessThanEquals,
Object: chain.ObjectRequest,
Key: "key23",
Value: "23.23",
}},
},
{
Conditions: []chain.Condition{{
Op: chain.CondNumericGreaterThan,
Object: chain.ObjectRequest,
Key: "key24",
Value: "-24.24",
}},
},
{
Conditions: []chain.Condition{{
Op: chain.CondNumericGreaterThanEquals,
Object: chain.ObjectRequest,
Key: "key25",
Value: "+25.25",
}},
},
}
actualCondition, err := convertToChainCondition(conditions)
require.NoError(t, err)
require.ElementsMatch(t, expectedCondition, actualCondition)
invalidConditions := []Condition{
{"key1": {"invalid"}},
{"key2": {"1 2"}},
{"key3": {"0x12f"}},
{"key4": {"0b1010"}},
{"key5": {"+Inf"}},
{"key6": {"-Inf"}},
{"key7": {"inf"}},
{"key8": {"NaN"}},
{"key9": {"nan"}},
}
for _, cond := range invalidConditions {
_, err = convertToChainCondition(Conditions{CondNumericEquals: cond})
require.Error(t, err)
}
}
func TestParsePrincipalARN(t *testing.T) {

33
pkg/chain/chain.go
View File

@ -6,6 +6,7 @@ import (
"git.frostfs.info/TrueCloudLab/policy-engine/pkg/resource"
"git.frostfs.info/TrueCloudLab/policy-engine/util"
"github.com/nspcc-dev/neo-go/pkg/encoding/fixedn"
"golang.org/x/exp/slices"
)
@ -186,6 +187,38 @@ func (c *Condition) Match(req resource.Request) bool {
return val >= c.Value
case CondSliceContains:
return slices.Contains(strings.Split(val, condSliceContainsDelimiter), c.Value)
case CondNumericEquals, CondNumericNotEquals, CondNumericLessThan, CondNumericLessThanEquals, CondNumericGreaterThan,
CondNumericGreaterThanEquals:
return c.matchNumeric(val)
}
}
func (c *Condition) matchNumeric(val string) bool {
valDecimal, err := fixedn.Fixed8FromString(val)
dkirillov commented 2024-04-02 12:57:28 +00:00
Outdated
Review

Probably we should try parse float if parsing integer failed because spec says Numeric condition operators let you construct Condition elements that restrict access based on comparing a key to an integer or decimal value.

Probably we should try parse float if parsing integer failed because [spec](https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition_operators.html#Conditions_Numeric) says `Numeric condition operators let you construct Condition elements that restrict access based on comparing a key to an integer or decimal value.`
mbiryukova commented 2024-04-02 15:15:50 +00:00
Outdated
Review

Can we parse only float in this case?

Can we parse only float in this case?
dkirillov commented 2024-04-03 06:22:37 +00:00
Outdated
Review

No. In general we cannot do this because equals for float can have some error:

	tmp := 0.1
	tmp2 := 0.2

	fmt.Println(tmp+tmp2 == 0.3) // false

Or we have to write custom equal function and not use ==

No. In general we cannot do this because equals for float can have some error: ```golang tmp := 0.1 tmp2 := 0.2 fmt.Println(tmp+tmp2 == 0.3) // false ``` Or we have to write custom equal function and not use `==`
fyrchik commented 2024-04-05 06:42:01 +00:00
Outdated
Review

What is the length of a decimal?
neo-go has a fixed decimal package https://github.com/nspcc-dev/neo-go/blob/master/pkg/encoding/fixedn/decimal.go (and we already depend on neo-go)

What is the length of a decimal? neo-go has a fixed decimal package https://github.com/nspcc-dev/neo-go/blob/master/pkg/encoding/fixedn/decimal.go (and we already depend on neo-go)
fyrchik commented 2024-04-05 06:43:13 +00:00
Outdated
Review

Do we have any compatibility tests here? The description at the link you have provided is vague.

Do we have any compatibility tests here? The description at the link you have provided is vague.
dkirillov commented 2024-04-08 08:43:49 +00:00
Outdated
Review

I couldn't find more precise description of this conditions. So we decided to use 1e-8 precision. We can use decimal from neo-go I think

I couldn't find more precise description of this conditions. So we decided to use `1e-8` precision. We can use decimal from neo-go I think
if err != nil {
return c.Op == CondNumericNotEquals
}
condVal, err := fixedn.Fixed8FromString(c.Value)
if err != nil {
return c.Op == CondNumericNotEquals
}
switch c.Op {
default:
panic(fmt.Sprintf("unimplemented: %d", c.Op))
case CondNumericEquals:
return valDecimal.Equal(condVal)
case CondNumericNotEquals:
return !valDecimal.Equal(condVal)
case CondNumericLessThan:
return valDecimal.LessThan(condVal)
case CondNumericLessThanEquals:
return valDecimal.LessThan(condVal) || valDecimal.Equal(condVal)
case CondNumericGreaterThan:
aarifullin marked this conversation as resolved Outdated
aarifullin commented 2024-04-05 09:37:46 +00:00
Outdated
Review

I suppose the correct way is to compare them like this

valFloat < condVal || equalFloat(valFloat, condVal)
I suppose the correct way is to compare them like this ```go valFloat < condVal || equalFloat(valFloat, condVal) ```
👍 1
return valDecimal.GreaterThan(condVal)
case CondNumericGreaterThanEquals:
return valDecimal.GreaterThan(condVal) || valDecimal.Equal(condVal)
}
}

136
pkg/chain/chain_test.go
View File

@ -6,6 +6,7 @@ import (
"git.frostfs.info/TrueCloudLab/policy-engine/pkg/resource/testutil"
"git.frostfs.info/TrueCloudLab/policy-engine/schema/common"
"git.frostfs.info/TrueCloudLab/policy-engine/schema/native"
"git.frostfs.info/TrueCloudLab/policy-engine/schema/s3"
"github.com/stretchr/testify/require"
)
@ -149,3 +150,138 @@ func TestCondSliceContainsMatch(t *testing.T) {
})
}
}
func TestNumericConditionsMatch(t *testing.T) {
propKey := s3.PropertyKeyMaxKeys
for _, tc := range []struct {
name string
conditions []Condition
value string
status Status
}{
{
name: "value from interval",
conditions: []Condition{
{
Op: CondNumericLessThan,
Object: ObjectRequest,
Key: propKey,
Value: "100",
},
{
Op: CondNumericGreaterThan,
Object: ObjectRequest,
Key: propKey,
Value: "80",
},
{
Op: CondNumericNotEquals,
Object: ObjectRequest,
Key: propKey,
Value: "91",
},
},
value: "90",
status: Allow,
},
{
name: "border value",
conditions: []Condition{
{
Op: CondNumericEquals,
Object: ObjectRequest,
Key: propKey,
Value: "50",
},
{
Op: CondNumericLessThanEquals,
Object: ObjectRequest,
Key: propKey,
Value: "50",
},
{
Op: CondNumericGreaterThanEquals,
Object: ObjectRequest,
Key: propKey,
Value: "50",
},
},
value: "50",
status: Allow,
},
} {
t.Run(tc.name, func(t *testing.T) {
resource := testutil.NewResource(native.ResourceFormatRootContainers, nil)
request := testutil.NewRequest(native.MethodPutObject, resource, map[string]string{propKey: tc.value})
ch := Chain{Rules: []Rule{{
Status: Allow,
Actions: Actions{Names: []string{native.MethodPutObject}},
Resources: Resources{Names: []string{native.ResourceFormatRootContainers}},
Condition: tc.conditions,
}}}
st, _ := ch.Match(request)
require.Equal(t, tc.status.String(), st.String())
})
}
}
func TestInvalidNumericValues(t *testing.T) {
propKey := s3.PropertyKeyMaxKeys
propValues := []string{"", "invalid"}
for _, tc := range []struct {
name string
conditionType ConditionType
match bool
}{
{
name: "NumericEquals condition",
conditionType: CondNumericEquals,
match: false,
},
{
name: "NumericNotEquals condition",
conditionType: CondNumericNotEquals,
match: true,
},
{
name: "NumericLessThan condition",
conditionType: CondNumericLessThan,
match: false,
},
{
name: "NumericLessThanEquals condition",
conditionType: CondNumericLessThanEquals,
match: false,
},
{
name: "NumericGreaterThan condition",
conditionType: CondNumericGreaterThan,
match: false,
},
{
name: "NumericGreaterThanEquals condition",
conditionType: CondNumericGreaterThanEquals,
match: false,
},
} {
t.Run(tc.name, func(t *testing.T) {
resource := testutil.NewResource(native.ResourceFormatRootContainers, nil)
condition := Condition{
Op: tc.conditionType,
Object: ObjectRequest,
Key: propKey,
Value: "50",
}
for _, propValue := range propValues {
request := testutil.NewRequest(native.MethodPutObject, resource, map[string]string{propKey: propValue})
match := condition.Match(request)
require.Equal(t, tc.match, match)
}
})
}
}

1
schema/s3/consts.go
View File

@ -6,6 +6,7 @@ const (
PropertyKeyDelimiter = "s3:delimiter"
PropertyKeyPrefix = "s3:prefix"
PropertyKeyVersionID = "s3:VersionId"
PropertyKeyMaxKeys = "s3:max-keys"
ResourceFormatS3All = "arn:aws:s3:::*"
ResourceFormatS3Bucket = "arn:aws:s3:::%s"