From 2db22deb93b734383cbc1c02f2afb2cecb253f9a Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Wed, 26 Dec 2018 02:03:28 -0800 Subject: [PATCH] Fix policy bugs Null conditions and canonical names (#7021) This PR fixes two different issues - Null condition implementation - HTTP Canonical request value names This PR fixes handling of null conditions and handle HTTP canonical names in request values. This PR was tested with policies mentioned in the following blog https://aws.amazon.com/blogs/security/how-to-prevent-uploads-of-unencrypted-objects-to-amazon-s3/ Fixes #6955 --- pkg/policy/condition/func_test.go | 6 +++--- pkg/policy/condition/ipaddressfunc.go | 8 +++++++- pkg/policy/condition/nullfunc.go | 21 +++++++++++++-------- pkg/policy/condition/nullfunc_test.go | 22 +++++++++++----------- pkg/policy/condition/stringequalsfunc.go | 7 ++++++- pkg/policy/condition/stringlikefunc.go | 8 +++++++- 6 files changed, 47 insertions(+), 25 deletions(-) diff --git a/pkg/policy/condition/func_test.go b/pkg/policy/condition/func_test.go index 56df25b60..4675e50fd 100644 --- a/pkg/policy/condition/func_test.go +++ b/pkg/policy/condition/func_test.go @@ -53,12 +53,12 @@ func TestFunctionsEvaluate(t *testing.T) { {case1Function, map[string][]string{ "x-amz-copy-source": {"mybucket/myobject"}, "SourceIp": {"192.168.1.10"}, - }, true}, + }, false}, {case1Function, map[string][]string{ "x-amz-copy-source": {"mybucket/myobject"}, "SourceIp": {"192.168.1.10"}, "Refer": {"http://example.org/"}, - }, true}, + }, false}, {case1Function, map[string][]string{"x-amz-copy-source": {"mybucket/myobject"}}, false}, {case1Function, map[string][]string{"SourceIp": {"192.168.1.10"}}, false}, {case1Function, map[string][]string{ @@ -79,7 +79,7 @@ func TestFunctionsEvaluate(t *testing.T) { result := testCase.functions.Evaluate(testCase.values) if result != testCase.expectedResult { - t.Fatalf("case %v: expected: %v, got: %v\n", i+1, testCase.expectedResult, result) + t.Errorf("case %v: expected: %v, got: %v\n", i+1, testCase.expectedResult, result) } } } diff --git a/pkg/policy/condition/ipaddressfunc.go b/pkg/policy/condition/ipaddressfunc.go index 2d108bfa4..3c3087d30 100644 --- a/pkg/policy/condition/ipaddressfunc.go +++ b/pkg/policy/condition/ipaddressfunc.go @@ -19,6 +19,7 @@ package condition import ( "fmt" "net" + "net/http" "sort" ) @@ -46,7 +47,12 @@ type ipAddressFunc struct { // falls in one of network or not. func (f ipAddressFunc) evaluate(values map[string][]string) bool { IPs := []net.IP{} - for _, s := range values[f.k.Name()] { + requestValue, ok := values[http.CanonicalHeaderKey(f.k.Name())] + if !ok { + requestValue = values[f.k.Name()] + } + + for _, s := range requestValue { IP := net.ParseIP(s) if IP == nil { panic(fmt.Errorf("invalid IP address '%v'", s)) diff --git a/pkg/policy/condition/nullfunc.go b/pkg/policy/condition/nullfunc.go index 353d675a5..76bfd888c 100644 --- a/pkg/policy/condition/nullfunc.go +++ b/pkg/policy/condition/nullfunc.go @@ -18,17 +18,19 @@ package condition import ( "fmt" + "net/http" "reflect" "strconv" ) -// nullFunc - Null condition function. It checks whether Key is present in given +// nullFunc - Null condition function. It checks whether Key is not present in given // values or not. // For example, // 1. if Key = S3XAmzCopySource and Value = true, at evaluate() it returns whether -// S3XAmzCopySource is in given value map or not. -// 2. if Key = S3XAmzCopySource and Value = false, at evaluate() it returns whether // S3XAmzCopySource is NOT in given value map or not. +// 2. if Key = S3XAmzCopySource and Value = false, at evaluate() it returns whether +// S3XAmzCopySource is in given value map or not. +// https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition_operators.html#Conditions_Null type nullFunc struct { k Key value bool @@ -37,13 +39,16 @@ type nullFunc struct { // evaluate() - evaluates to check whether Key is present in given values or not. // Depending on condition boolean value, this function returns true or false. func (f nullFunc) evaluate(values map[string][]string) bool { - requestValue := values[f.k.Name()] - - if f.value { - return len(requestValue) != 0 + requestValue, ok := values[http.CanonicalHeaderKey(f.k.Name())] + if !ok { + requestValue = values[f.k.Name()] } - return len(requestValue) == 0 + if f.value { + return len(requestValue) == 0 + } + + return len(requestValue) != 0 } // key() - returns condition key which is used by this condition function. diff --git a/pkg/policy/condition/nullfunc_test.go b/pkg/policy/condition/nullfunc_test.go index 77e218bf7..32b917bea 100644 --- a/pkg/policy/condition/nullfunc_test.go +++ b/pkg/policy/condition/nullfunc_test.go @@ -37,23 +37,23 @@ func TestNullFuncEvaluate(t *testing.T) { values map[string][]string expectedResult bool }{ - {case1Function, map[string][]string{"prefix": {"true"}}, true}, - {case1Function, map[string][]string{"prefix": {"false"}}, true}, - {case1Function, map[string][]string{"prefix": {"mybucket/foo"}}, true}, - {case1Function, map[string][]string{}, false}, - {case1Function, map[string][]string{"delimiter": {"/"}}, false}, - {case2Function, map[string][]string{"prefix": {"true"}}, false}, - {case2Function, map[string][]string{"prefix": {"false"}}, false}, - {case2Function, map[string][]string{"prefix": {"mybucket/foo"}}, false}, - {case2Function, map[string][]string{}, true}, - {case2Function, map[string][]string{"delimiter": {"/"}}, true}, + {case1Function, map[string][]string{"prefix": {"true"}}, false}, + {case1Function, map[string][]string{"prefix": {"false"}}, false}, + {case1Function, map[string][]string{"prefix": {"mybucket/foo"}}, false}, + {case1Function, map[string][]string{}, true}, + {case1Function, map[string][]string{"delimiter": {"/"}}, true}, + {case2Function, map[string][]string{"prefix": {"true"}}, true}, + {case2Function, map[string][]string{"prefix": {"false"}}, true}, + {case2Function, map[string][]string{"prefix": {"mybucket/foo"}}, true}, + {case2Function, map[string][]string{}, false}, + {case2Function, map[string][]string{"delimiter": {"/"}}, false}, } for i, testCase := range testCases { result := testCase.function.evaluate(testCase.values) if result != testCase.expectedResult { - t.Fatalf("case %v: expected: %v, got: %v\n", i+1, testCase.expectedResult, result) + t.Errorf("case %v: expected: %v, got: %v\n", i+1, testCase.expectedResult, result) } } } diff --git a/pkg/policy/condition/stringequalsfunc.go b/pkg/policy/condition/stringequalsfunc.go index 03170c0c0..9c84e73d9 100644 --- a/pkg/policy/condition/stringequalsfunc.go +++ b/pkg/policy/condition/stringequalsfunc.go @@ -18,6 +18,7 @@ package condition import ( "fmt" + "net/http" "sort" "strings" @@ -44,7 +45,11 @@ type stringEqualsFunc struct { // evaluate() - evaluates to check whether value by Key in given values is in // condition values. func (f stringEqualsFunc) evaluate(values map[string][]string) bool { - requestValue := values[f.k.Name()] + requestValue, ok := values[http.CanonicalHeaderKey(f.k.Name())] + if !ok { + requestValue = values[f.k.Name()] + } + return !f.values.Intersection(set.CreateStringSet(requestValue...)).IsEmpty() } diff --git a/pkg/policy/condition/stringlikefunc.go b/pkg/policy/condition/stringlikefunc.go index 99d20dd2b..ca05c688e 100644 --- a/pkg/policy/condition/stringlikefunc.go +++ b/pkg/policy/condition/stringlikefunc.go @@ -18,6 +18,7 @@ package condition import ( "fmt" + "net/http" "sort" "strings" @@ -45,7 +46,12 @@ type stringLikeFunc struct { // evaluate() - evaluates to check whether value by Key in given values is wildcard // matching in condition values. func (f stringLikeFunc) evaluate(values map[string][]string) bool { - for _, v := range values[f.k.Name()] { + requestValue, ok := values[http.CanonicalHeaderKey(f.k.Name())] + if !ok { + requestValue = values[f.k.Name()] + } + + for _, v := range requestValue { if !f.values.FuncMatch(wildcard.Match, v).IsEmpty() { return true }