From 52a1d248b20a2baa966d89bfb8f005a455a64fbe Mon Sep 17 00:00:00 2001 From: Anis Elleuch Date: Sun, 10 May 2020 18:55:28 +0100 Subject: [PATCH] policy: Do not return an error for invalid value during parsing (#9442) s3:HardwareInfo was removed recently. Users having that admin action stored in the backend will have an issue starting the server. To fix this, we need to avoid returning an error in Marshal/Unmarshal when they encounter an invalid action and validate only in specific location. Currently the validation is done and in ParseConfig(). --- pkg/bucket/policy/effect.go | 30 ---- pkg/bucket/policy/effect_test.go | 59 ------- pkg/bucket/policy/id.go | 27 ---- pkg/bucket/policy/id_test.go | 57 ------- pkg/iam/policy/action.go | 43 ----- pkg/iam/policy/action_test.go | 56 +------ pkg/iam/policy/actionset.go | 35 +++- pkg/iam/policy/actionset_test.go | 33 ++-- pkg/iam/policy/policy.go | 30 +--- pkg/iam/policy/policy_test.go | 263 +++---------------------------- pkg/iam/policy/statement.go | 50 ++---- pkg/iam/policy/statement_test.go | 115 +++----------- 12 files changed, 115 insertions(+), 683 deletions(-) diff --git a/pkg/bucket/policy/effect.go b/pkg/bucket/policy/effect.go index 67a6e2092..31167587a 100644 --- a/pkg/bucket/policy/effect.go +++ b/pkg/bucket/policy/effect.go @@ -16,10 +16,6 @@ package policy -import ( - "encoding/json" -) - // Effect - policy statement effect Allow or Deny. type Effect string @@ -49,29 +45,3 @@ func (effect Effect) IsValid() bool { return false } - -// MarshalJSON - encodes Effect to JSON data. -func (effect Effect) MarshalJSON() ([]byte, error) { - if !effect.IsValid() { - return nil, Errorf("invalid effect '%v'", effect) - } - - return json.Marshal(string(effect)) -} - -// UnmarshalJSON - decodes JSON data to Effect. -func (effect *Effect) UnmarshalJSON(data []byte) error { - var s string - if err := json.Unmarshal(data, &s); err != nil { - return err - } - - e := Effect(s) - if !e.IsValid() { - return Errorf("invalid effect '%v'", s) - } - - *effect = e - - return nil -} diff --git a/pkg/bucket/policy/effect_test.go b/pkg/bucket/policy/effect_test.go index d01b3e76e..5797aaa15 100644 --- a/pkg/bucket/policy/effect_test.go +++ b/pkg/bucket/policy/effect_test.go @@ -17,8 +17,6 @@ package policy import ( - "encoding/json" - "reflect" "testing" ) @@ -63,60 +61,3 @@ func TestEffectIsValid(t *testing.T) { } } } - -func TestEffectMarshalJSON(t *testing.T) { - testCases := []struct { - effect Effect - expectedResult []byte - expectErr bool - }{ - {Allow, []byte(`"Allow"`), false}, - {Deny, []byte(`"Deny"`), false}, - {Effect(""), nil, true}, - {Effect("foo"), nil, true}, - } - - for i, testCase := range testCases { - result, err := json.Marshal(testCase.effect) - expectErr := (err != nil) - - if expectErr != testCase.expectErr { - t.Fatalf("case %v: error: expected: %v, got: %v\n", i+1, testCase.expectErr, expectErr) - } - - if !testCase.expectErr { - if !reflect.DeepEqual(result, testCase.expectedResult) { - t.Fatalf("case %v: result: expected: %v, got: %v\n", i+1, string(testCase.expectedResult), string(result)) - } - } - } -} - -func TestEffectUnmarshalJSON(t *testing.T) { - testCases := []struct { - data []byte - expectedResult Effect - expectErr bool - }{ - {[]byte(`"Allow"`), Allow, false}, - {[]byte(`"Deny"`), Deny, false}, - {[]byte(`""`), Effect(""), true}, - {[]byte(`"foo"`), Effect(""), true}, - } - - for i, testCase := range testCases { - var result Effect - err := json.Unmarshal(testCase.data, &result) - expectErr := (err != nil) - - if expectErr != testCase.expectErr { - t.Fatalf("case %v: error: expected: %v, got: %v\n", i+1, testCase.expectErr, expectErr) - } - - if !testCase.expectErr { - if result != testCase.expectedResult { - t.Fatalf("case %v: result: expected: %v, got: %v\n", i+1, testCase.expectedResult, result) - } - } - } -} diff --git a/pkg/bucket/policy/id.go b/pkg/bucket/policy/id.go index 4bae2863e..62e48e6c2 100644 --- a/pkg/bucket/policy/id.go +++ b/pkg/bucket/policy/id.go @@ -17,7 +17,6 @@ package policy import ( - "encoding/json" "unicode/utf8" ) @@ -28,29 +27,3 @@ type ID string func (id ID) IsValid() bool { return utf8.ValidString(string(id)) } - -// MarshalJSON - encodes ID to JSON data. -func (id ID) MarshalJSON() ([]byte, error) { - if !id.IsValid() { - return nil, Errorf("invalid ID %v", id) - } - - return json.Marshal(string(id)) -} - -// UnmarshalJSON - decodes JSON data to ID. -func (id *ID) UnmarshalJSON(data []byte) error { - var s string - if err := json.Unmarshal(data, &s); err != nil { - return err - } - - i := ID(s) - if !i.IsValid() { - return Errorf("invalid ID %v", s) - } - - *id = i - - return nil -} diff --git a/pkg/bucket/policy/id_test.go b/pkg/bucket/policy/id_test.go index e84885ef5..b1f7d5dfd 100644 --- a/pkg/bucket/policy/id_test.go +++ b/pkg/bucket/policy/id_test.go @@ -17,8 +17,6 @@ package policy import ( - "encoding/json" - "reflect" "testing" ) @@ -40,58 +38,3 @@ func TestIDIsValid(t *testing.T) { } } } - -func TestIDMarshalJSON(t *testing.T) { - testCases := []struct { - id ID - expectedResult []byte - expectErr bool - }{ - {ID("DenyEncryptionSt1"), []byte(`"DenyEncryptionSt1"`), false}, - {ID(""), []byte(`""`), false}, - {ID("aa\xe2"), nil, true}, // invalid utf-8 - } - - for i, testCase := range testCases { - result, err := json.Marshal(testCase.id) - expectErr := (err != nil) - - if expectErr != testCase.expectErr { - t.Fatalf("case %v: error: expected: %v, got: %v\n", i+1, testCase.expectErr, expectErr) - } - - if !testCase.expectErr { - if !reflect.DeepEqual(result, testCase.expectedResult) { - t.Fatalf("case %v: result: expected: %v, got: %v\n", i+1, string(testCase.expectedResult), string(result)) - } - } - } -} - -func TestIDUnmarshalJSON(t *testing.T) { - testCases := []struct { - data []byte - expectedResult ID - expectErr bool - }{ - {[]byte(`"DenyEncryptionSt1"`), ID("DenyEncryptionSt1"), false}, - {[]byte(`""`), ID(""), false}, - {[]byte(`"aa\xe2"`), ID(""), true}, - } - - for i, testCase := range testCases { - var result ID - err := json.Unmarshal(testCase.data, &result) - expectErr := (err != nil) - - if expectErr != testCase.expectErr { - t.Fatalf("case %v: error: expected: %v, got: %v\n", i+1, testCase.expectErr, expectErr) - } - - if !testCase.expectErr { - if result != testCase.expectedResult { - t.Fatalf("case %v: result: expected: %v, got: %v\n", i+1, testCase.expectedResult, result) - } - } - } -} diff --git a/pkg/iam/policy/action.go b/pkg/iam/policy/action.go index cda186c73..bded00c77 100644 --- a/pkg/iam/policy/action.go +++ b/pkg/iam/policy/action.go @@ -17,8 +17,6 @@ package iampolicy import ( - "encoding/json" - "github.com/minio/minio/pkg/bucket/policy/condition" "github.com/minio/minio/pkg/wildcard" ) @@ -215,47 +213,6 @@ func (action Action) IsValid() bool { return ok } -// MarshalJSON - encodes Action to JSON data. -func (action Action) MarshalJSON() ([]byte, error) { - if action.IsValid() || AdminAction(action).IsValid() { - return json.Marshal(string(action)) - } - - return nil, Errorf("invalid action '%v'", action) -} - -// UnmarshalJSON - decodes JSON data to Action. -func (action *Action) UnmarshalJSON(data []byte) error { - var s string - - if err := json.Unmarshal(data, &s); err != nil { - return err - } - - a := Action(s) - if !a.IsValid() { - return Errorf("invalid action '%v'", s) - } - - *action = a - - return nil -} - -func parseAction(s string) (Action, error) { - adminAction, err := parseAdminAction(s) - if err == nil { - return Action(adminAction), nil - } - action := Action(s) - - if action.IsValid() { - return action, nil - } - - return action, Errorf("unsupported action '%v'", s) -} - // actionConditionKeyMap - holds mapping of supported condition key for an action. var actionConditionKeyMap = map[Action]condition.KeySet{ AllActions: condition.NewKeySet(condition.AllSupportedKeys...), diff --git a/pkg/iam/policy/action_test.go b/pkg/iam/policy/action_test.go index 94f0f3fd3..653ee4a57 100644 --- a/pkg/iam/policy/action_test.go +++ b/pkg/iam/policy/action_test.go @@ -17,8 +17,6 @@ package iampolicy import ( - "encoding/json" - "reflect" "testing" ) @@ -49,6 +47,7 @@ func TestActionIsValid(t *testing.T) { action Action expectedResult bool }{ + {PutObjectAction, true}, {AbortMultipartUploadAction, true}, {Action("foo"), false}, } @@ -61,56 +60,3 @@ func TestActionIsValid(t *testing.T) { } } } - -func TestActionMarshalJSON(t *testing.T) { - testCases := []struct { - action Action - expectedResult []byte - expectErr bool - }{ - {PutObjectAction, []byte(`"s3:PutObject"`), false}, - {Action("foo"), nil, true}, - } - - for i, testCase := range testCases { - result, err := json.Marshal(testCase.action) - expectErr := (err != nil) - - if testCase.expectErr != expectErr { - t.Fatalf("case %v: error: expected: %v, got: %v", i+1, testCase.expectErr, expectErr) - } - - if !testCase.expectErr { - if !reflect.DeepEqual(result, testCase.expectedResult) { - t.Fatalf("case %v: result: expected: %v, got: %v", i+1, testCase.expectedResult, result) - } - } - } -} - -func TestActionUnmarshalJSON(t *testing.T) { - testCases := []struct { - data []byte - expectedResult Action - expectErr bool - }{ - {[]byte(`"s3:PutObject"`), PutObjectAction, false}, - {[]byte(`"foo"`), Action(""), true}, - } - - for i, testCase := range testCases { - var result Action - err := json.Unmarshal(testCase.data, &result) - expectErr := (err != nil) - - if testCase.expectErr != expectErr { - t.Fatalf("case %v: error: expected: %v, got: %v", i+1, testCase.expectErr, expectErr) - } - - if !testCase.expectErr { - if testCase.expectedResult != result { - t.Fatalf("case %v: result: expected: %v, got: %v", i+1, testCase.expectedResult, result) - } - } - } -} diff --git a/pkg/iam/policy/actionset.go b/pkg/iam/policy/actionset.go index e670c7d6c..ef6ad588a 100644 --- a/pkg/iam/policy/actionset.go +++ b/pkg/iam/policy/actionset.go @@ -107,6 +107,16 @@ func (actionSet ActionSet) ToSlice() []Action { return actions } +// ToAdminSlice - returns slice of admin actions from the action set. +func (actionSet ActionSet) ToAdminSlice() []AdminAction { + actions := []AdminAction{} + for action := range actionSet { + actions = append(actions, AdminAction(action)) + } + + return actions +} + // UnmarshalJSON - decodes JSON data to ActionSet. func (actionSet *ActionSet) UnmarshalJSON(data []byte) error { var sset set.StringSet @@ -120,14 +130,29 @@ func (actionSet *ActionSet) UnmarshalJSON(data []byte) error { *actionSet = make(ActionSet) for _, s := range sset.ToSlice() { - action, err := parseAction(s) - if err != nil { - return err - } + actionSet.Add(Action(s)) + } - actionSet.Add(action) + return nil +} + +// ValidateAdmin checks if all actions are valid Admin actions +func (actionSet ActionSet) ValidateAdmin() error { + for _, action := range actionSet.ToAdminSlice() { + if !action.IsValid() { + return Errorf("unsupported admin action '%v'", action) + } } + return nil +} +// Validate checks if all actions are valid +func (actionSet ActionSet) Validate() error { + for _, action := range actionSet.ToSlice() { + if !action.IsValid() { + return Errorf("unsupported action '%v'", action) + } + } return nil } diff --git a/pkg/iam/policy/actionset_test.go b/pkg/iam/policy/actionset_test.go index 37dc26fde..9cb5c6116 100644 --- a/pkg/iam/policy/actionset_test.go +++ b/pkg/iam/policy/actionset_test.go @@ -128,17 +128,18 @@ func TestActionSetToSlice(t *testing.T) { func TestActionSetUnmarshalJSON(t *testing.T) { testCases := []struct { - data []byte - expectedResult ActionSet - expectErr bool + data []byte + expectedResult ActionSet + expectUnmarshalErr bool + expectValidateErr bool }{ - {[]byte(`"s3:PutObject"`), NewActionSet(PutObjectAction), false}, - {[]byte(`["s3:PutObject"]`), NewActionSet(PutObjectAction), false}, - {[]byte(`["s3:PutObject", "s3:GetObject"]`), NewActionSet(PutObjectAction, GetObjectAction), false}, - {[]byte(`["s3:PutObject", "s3:GetObject", "s3:PutObject"]`), NewActionSet(PutObjectAction, GetObjectAction), false}, - {[]byte(`[]`), NewActionSet(), true}, // Empty array. - {[]byte(`"foo"`), nil, true}, // Invalid action. - {[]byte(`["s3:PutObject", "foo"]`), nil, true}, // Invalid action. + {[]byte(`"s3:PutObject"`), NewActionSet(PutObjectAction), false, false}, + {[]byte(`["s3:PutObject"]`), NewActionSet(PutObjectAction), false, false}, + {[]byte(`["s3:PutObject", "s3:GetObject"]`), NewActionSet(PutObjectAction, GetObjectAction), false, false}, + {[]byte(`["s3:PutObject", "s3:GetObject", "s3:PutObject"]`), NewActionSet(PutObjectAction, GetObjectAction), false, false}, + {[]byte(`[]`), NewActionSet(), true, false}, // Empty array. + {[]byte(`"foo"`), nil, false, true}, // Invalid action. + {[]byte(`["s3:PutObject", "foo"]`), nil, false, true}, // Invalid action. } for i, testCase := range testCases { @@ -146,11 +147,17 @@ func TestActionSetUnmarshalJSON(t *testing.T) { err := json.Unmarshal(testCase.data, &result) expectErr := (err != nil) - if expectErr != testCase.expectErr { - t.Fatalf("case %v: error: expected: %v, got: %v\n", i+1, testCase.expectErr, expectErr) + if expectErr != testCase.expectUnmarshalErr { + t.Fatalf("case %v: error during unmarshal: expected: %v, got: %v\n", i+1, testCase.expectUnmarshalErr, expectErr) } - if !testCase.expectErr { + err = result.Validate() + expectErr = (err != nil) + if expectErr != testCase.expectValidateErr { + t.Fatalf("case %v: error during validation: expected: %v, got: %v\n", i+1, testCase.expectValidateErr, expectErr) + } + + if !testCase.expectUnmarshalErr && !testCase.expectValidateErr { if !reflect.DeepEqual(result, testCase.expectedResult) { t.Fatalf("case %v: result: expected: %v, got: %v\n", i+1, testCase.expectedResult, result) } diff --git a/pkg/iam/policy/policy.go b/pkg/iam/policy/policy.go index e150ca200..4b874ef82 100644 --- a/pkg/iam/policy/policy.go +++ b/pkg/iam/policy/policy.go @@ -103,21 +103,9 @@ func (iamp Policy) isValid() error { return err } } - return nil } -// MarshalJSON - encodes Policy to JSON data. -func (iamp Policy) MarshalJSON() ([]byte, error) { - if err := iamp.isValid(); err != nil { - return nil, err - } - - // subtype to avoid recursive call to MarshalJSON() - type subPolicy Policy - return json.Marshal(subPolicy(iamp)) -} - func (iamp *Policy) dropDuplicateStatements() { redo: for i := range iamp.Statements { @@ -153,30 +141,14 @@ func (iamp *Policy) UnmarshalJSON(data []byte) error { } p := Policy(sp) - if err := p.isValid(); err != nil { - return err - } - p.dropDuplicateStatements() - *iamp = p - return nil } // Validate - validates all statements are for given bucket or not. func (iamp Policy) Validate() error { - if err := iamp.isValid(); err != nil { - return err - } - - for _, statement := range iamp.Statements { - if err := statement.Validate(); err != nil { - return err - } - } - - return nil + return iamp.isValid() } // ParseConfig - parses data in given reader to Iamp. diff --git a/pkg/iam/policy/policy_test.go b/pkg/iam/policy/policy_test.go index a981e5186..b082f0972 100644 --- a/pkg/iam/policy/policy_test.go +++ b/pkg/iam/policy/policy_test.go @@ -387,226 +387,7 @@ func TestPolicyIsValid(t *testing.T) { } } -func TestPolicyMarshalJSON(t *testing.T) { - case1Policy := Policy{ - ID: "MyPolicyForMyBucket1", - Version: DefaultVersion, - Statements: []Statement{ - NewStatement( - policy.Allow, - NewActionSet(PutObjectAction), - NewResourceSet(NewResource("mybucket", "/myobject*")), - condition.NewFunctions(), - ), - }, - } - case1Policy.Statements[0].SID = "SomeId1" - case1Data := []byte(`{"ID":"MyPolicyForMyBucket1","Version":"2012-10-17","Statement":[{"Sid":"SomeId1","Effect":"Allow","Action":["s3:PutObject"],"Resource":["arn:aws:s3:::mybucket/myobject*"]}]}`) - - _, IPNet1, err := net.ParseCIDR("192.168.1.0/24") - if err != nil { - t.Fatalf("unexpected error. %v\n", err) - } - func1, err := condition.NewIPAddressFunc( - condition.AWSSourceIP, - IPNet1, - ) - if err != nil { - t.Fatalf("unexpected error. %v\n", err) - } - - case2Policy := Policy{ - Version: DefaultVersion, - Statements: []Statement{ - NewStatement( - policy.Allow, - NewActionSet(PutObjectAction), - NewResourceSet(NewResource("mybucket", "/myobject*")), - condition.NewFunctions(), - ), - NewStatement( - policy.Deny, - NewActionSet(GetObjectAction), - NewResourceSet(NewResource("mybucket", "/yourobject*")), - condition.NewFunctions(func1), - ), - }, - } - case2Data := []byte(`{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["s3:PutObject"],"Resource":["arn:aws:s3:::mybucket/myobject*"]},{"Effect":"Deny","Action":["s3:GetObject"],"Resource":["arn:aws:s3:::mybucket/yourobject*"],"Condition":{"IpAddress":{"aws:SourceIp":["192.168.1.0/24"]}}}]}`) - - case3Policy := Policy{ - ID: "MyPolicyForMyBucket1", - Version: DefaultVersion, - Statements: []Statement{ - NewStatement( - policy.Allow, - NewActionSet(GetObjectAction), - NewResourceSet(NewResource("mybucket", "/myobject*")), - condition.NewFunctions(), - ), - NewStatement( - policy.Allow, - NewActionSet(PutObjectAction), - NewResourceSet(NewResource("mybucket", "/myobject*")), - condition.NewFunctions(), - ), - }, - } - case3Data := []byte(`{"ID":"MyPolicyForMyBucket1","Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["s3:GetObject"],"Resource":["arn:aws:s3:::mybucket/myobject*"]},{"Effect":"Allow","Action":["s3:PutObject"],"Resource":["arn:aws:s3:::mybucket/myobject*"]}]}`) - - case4Policy := Policy{ - ID: "MyPolicyForMyBucket1", - Version: DefaultVersion, - Statements: []Statement{ - NewStatement( - policy.Allow, - NewActionSet(PutObjectAction), - NewResourceSet(NewResource("mybucket", "/myobject*")), - condition.NewFunctions(), - ), - NewStatement( - policy.Allow, - NewActionSet(GetObjectAction), - NewResourceSet(NewResource("mybucket", "/myobject*")), - condition.NewFunctions(), - ), - }, - } - case4Data := []byte(`{"ID":"MyPolicyForMyBucket1","Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["s3:PutObject"],"Resource":["arn:aws:s3:::mybucket/myobject*"]},{"Effect":"Allow","Action":["s3:GetObject"],"Resource":["arn:aws:s3:::mybucket/myobject*"]}]}`) - - case5Policy := Policy{ - ID: "MyPolicyForMyBucket1", - Version: DefaultVersion, - Statements: []Statement{ - NewStatement( - policy.Allow, - NewActionSet(PutObjectAction), - NewResourceSet(NewResource("mybucket", "/myobject*")), - condition.NewFunctions(), - ), - NewStatement( - policy.Allow, - NewActionSet(PutObjectAction), - NewResourceSet(NewResource("mybucket", "/yourobject*")), - condition.NewFunctions(), - ), - }, - } - case5Data := []byte(`{"ID":"MyPolicyForMyBucket1","Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["s3:PutObject"],"Resource":["arn:aws:s3:::mybucket/myobject*"]},{"Effect":"Allow","Action":["s3:PutObject"],"Resource":["arn:aws:s3:::mybucket/yourobject*"]}]}`) - - _, IPNet2, err := net.ParseCIDR("192.168.2.0/24") - if err != nil { - t.Fatalf("unexpected error. %v\n", err) - } - func2, err := condition.NewIPAddressFunc( - condition.AWSSourceIP, - IPNet2, - ) - if err != nil { - t.Fatalf("unexpected error. %v\n", err) - } - - case6Policy := Policy{ - ID: "MyPolicyForMyBucket1", - Version: DefaultVersion, - Statements: []Statement{ - NewStatement( - policy.Allow, - NewActionSet(PutObjectAction), - NewResourceSet(NewResource("mybucket", "/myobject*")), - condition.NewFunctions(func1), - ), - NewStatement( - policy.Allow, - NewActionSet(PutObjectAction), - NewResourceSet(NewResource("mybucket", "/myobject*")), - condition.NewFunctions(func2), - ), - }, - } - case6Data := []byte(`{"ID":"MyPolicyForMyBucket1","Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["s3:PutObject"],"Resource":["arn:aws:s3:::mybucket/myobject*"],"Condition":{"IpAddress":{"aws:SourceIp":["192.168.1.0/24"]}}},{"Effect":"Allow","Action":["s3:PutObject"],"Resource":["arn:aws:s3:::mybucket/myobject*"],"Condition":{"IpAddress":{"aws:SourceIp":["192.168.2.0/24"]}}}]}`) - - case7Policy := Policy{ - ID: "MyPolicyForMyBucket1", - Version: DefaultVersion, - Statements: []Statement{ - NewStatement( - policy.Allow, - NewActionSet(GetBucketLocationAction), - NewResourceSet(NewResource("mybucket", "")), - condition.NewFunctions(), - ), - }, - } - case7Data := []byte(`{"ID":"MyPolicyForMyBucket1","Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["s3:GetBucketLocation"],"Resource":["arn:aws:s3:::mybucket"]}]}`) - - case8Policy := Policy{ - ID: "MyPolicyForMyBucket1", - Version: DefaultVersion, - Statements: []Statement{ - NewStatement( - policy.Allow, - NewActionSet(GetBucketLocationAction), - NewResourceSet(NewResource("*", "")), - condition.NewFunctions(), - ), - }, - } - case8Data := []byte(`{"ID":"MyPolicyForMyBucket1","Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["s3:GetBucketLocation"],"Resource":["arn:aws:s3:::*"]}]}`) - - func3, err := condition.NewNullFunc( - condition.S3XAmzCopySource, - true, - ) - if err != nil { - t.Fatalf("unexpected error. %v\n", err) - } - case9Policy := Policy{ - ID: "MyPolicyForMyBucket1", - Version: DefaultVersion, - Statements: []Statement{ - NewStatement( - policy.Allow, - NewActionSet(GetObjectAction, PutObjectAction), - NewResourceSet(NewResource("mybucket", "myobject*")), - condition.NewFunctions(func1, func2, func3), - ), - }, - } - - testCases := []struct { - policy Policy - expectedResult []byte - expectErr bool - }{ - {case1Policy, case1Data, false}, - {case2Policy, case2Data, false}, - {case3Policy, case3Data, false}, - {case4Policy, case4Data, false}, - {case5Policy, case5Data, false}, - {case6Policy, case6Data, false}, - {case7Policy, case7Data, false}, - {case8Policy, case8Data, false}, - {case9Policy, nil, true}, - } - - for i, testCase := range testCases { - result, err := json.Marshal(testCase.policy) - expectErr := (err != nil) - - if expectErr != testCase.expectErr { - t.Fatalf("case %v: error: expected: %v, got: %v", i+1, testCase.expectErr, expectErr) - } - - if !testCase.expectErr { - if !reflect.DeepEqual(result, testCase.expectedResult) { - t.Fatalf("case %v: result: expected: %v, got: %v", i+1, string(testCase.expectedResult), string(result)) - } - } - } -} - -func TestPolicyUnmarshalJSON(t *testing.T) { +func TestPolicyUnmarshalJSONAndValidate(t *testing.T) { case1Data := []byte(`{ "ID": "MyPolicyForMyBucket1", "Version": "2012-10-17", @@ -973,24 +754,25 @@ func TestPolicyUnmarshalJSON(t *testing.T) { } testCases := []struct { - data []byte - expectedResult Policy - expectErr bool + data []byte + expectedResult Policy + expectUnmarshalErr bool + expectValidationErr bool }{ - {case1Data, case1Policy, false}, - {case2Data, case2Policy, false}, - {case3Data, case3Policy, false}, - {case4Data, case4Policy, false}, - {case5Data, case5Policy, false}, - {case6Data, case6Policy, false}, - {case7Data, case7Policy, false}, - {case8Data, case8Policy, false}, + {case1Data, case1Policy, false, false}, + {case2Data, case2Policy, false, false}, + {case3Data, case3Policy, false, false}, + {case4Data, case4Policy, false, false}, + {case5Data, case5Policy, false, false}, + {case6Data, case6Policy, false, false}, + {case7Data, case7Policy, false, false}, + {case8Data, case8Policy, false, false}, // Invalid version error. - {case9Data, Policy{}, true}, + {case9Data, Policy{}, false, true}, // Duplicate statement success, duplicate statement is removed. - {case10Data, case10Policy, false}, + {case10Data, case10Policy, false, false}, // Duplicate statement success (Effect differs). - {case11Data, case11Policy, false}, + {case11Data, case11Policy, false, false}, } for i, testCase := range testCases { @@ -998,11 +780,18 @@ func TestPolicyUnmarshalJSON(t *testing.T) { err := json.Unmarshal(testCase.data, &result) expectErr := (err != nil) - if expectErr != testCase.expectErr { - t.Errorf("case %v: error: expected: %v, got: %v", i+1, testCase.expectErr, expectErr) + if expectErr != testCase.expectUnmarshalErr { + t.Errorf("case %v: error during unmarshal: expected: %v, got: %v", i+1, testCase.expectUnmarshalErr, expectErr) + } + + err = result.Validate() + expectErr = (err != nil) + + if expectErr != testCase.expectValidationErr { + t.Errorf("case %v: error during validation: expected: %v, got: %v", i+1, testCase.expectValidationErr, expectErr) } - if !testCase.expectErr { + if !testCase.expectUnmarshalErr && !testCase.expectValidationErr { if !reflect.DeepEqual(result, testCase.expectedResult) { t.Errorf("case %v: result: expected: %v, got: %v", i+1, testCase.expectedResult, result) } diff --git a/pkg/iam/policy/statement.go b/pkg/iam/policy/statement.go index 2ca0bc3d4..74f8874f3 100644 --- a/pkg/iam/policy/statement.go +++ b/pkg/iam/policy/statement.go @@ -17,7 +17,6 @@ package iampolicy import ( - "encoding/json" "strings" "github.com/minio/minio/pkg/bucket/policy" @@ -63,11 +62,11 @@ func (statement Statement) IsAllowed(args Args) bool { } func (statement Statement) isAdmin() bool { for action := range statement.Actions { - if !AdminAction(action).IsValid() { - return false + if AdminAction(action).IsValid() { + return true } } - return true + return false } // isValid - checks whether statement is valid or not. @@ -81,6 +80,9 @@ func (statement Statement) isValid() error { } if statement.isAdmin() { + if err := statement.Actions.ValidateAdmin(); err != nil { + return err + } for action := range statement.Actions { keys := statement.Conditions.Keys() keyDiff := keys.Difference(adminActionConditionKeyMap[action]) @@ -91,6 +93,10 @@ func (statement Statement) isValid() error { return nil } + if !statement.SID.IsValid() { + return Errorf("invalid SID %v", statement.SID) + } + if len(statement.Resources) == 0 { return Errorf("Resource must not be empty") } @@ -99,6 +105,10 @@ func (statement Statement) isValid() error { return err } + if err := statement.Actions.Validate(); err != nil { + return err + } + for action := range statement.Actions { if !statement.Resources.objectResourceExists() && !statement.Resources.bucketResourceExists() { return Errorf("unsupported Resource found %v for action %v", statement.Resources, action) @@ -114,38 +124,6 @@ func (statement Statement) isValid() error { return nil } -// MarshalJSON - encodes JSON data to Statement. -func (statement Statement) MarshalJSON() ([]byte, error) { - if err := statement.isValid(); err != nil { - return nil, err - } - - // subtype to avoid recursive call to MarshalJSON() - type subStatement Statement - ss := subStatement(statement) - return json.Marshal(ss) -} - -// UnmarshalJSON - decodes JSON data to Statement. -func (statement *Statement) UnmarshalJSON(data []byte) error { - // subtype to avoid recursive call to UnmarshalJSON() - type subStatement Statement - var ss subStatement - - if err := json.Unmarshal(data, &ss); err != nil { - return err - } - - s := Statement(ss) - if err := s.isValid(); err != nil { - return err - } - - *statement = s - - return nil -} - // Validate - validates Statement is for given bucket or not. func (statement Statement) Validate() error { return statement.isValid() diff --git a/pkg/iam/policy/statement_test.go b/pkg/iam/policy/statement_test.go index 71b19eab1..3b290ff73 100644 --- a/pkg/iam/policy/statement_test.go +++ b/pkg/iam/policy/statement_test.go @@ -265,82 +265,7 @@ func TestStatementIsValid(t *testing.T) { } } -func TestStatementMarshalJSON(t *testing.T) { - case1Statement := NewStatement( - policy.Allow, - NewActionSet(PutObjectAction), - NewResourceSet(NewResource("mybucket", "/myobject*")), - condition.NewFunctions(), - ) - case1Statement.SID = "SomeId1" - case1Data := []byte(`{"Sid":"SomeId1","Effect":"Allow","Action":["s3:PutObject"],"Resource":["arn:aws:s3:::mybucket/myobject*"]}`) - - func1, err := condition.NewNullFunc( - condition.S3XAmzCopySource, - true, - ) - if err != nil { - t.Fatalf("unexpected error. %v\n", err) - } - case2Statement := NewStatement( - policy.Allow, - NewActionSet(PutObjectAction), - NewResourceSet(NewResource("mybucket", "/myobject*")), - condition.NewFunctions(func1), - ) - case2Data := []byte(`{"Effect":"Allow","Action":["s3:PutObject"],"Resource":["arn:aws:s3:::mybucket/myobject*"],"Condition":{"Null":{"s3:x-amz-copy-source":[true]}}}`) - - func2, err := condition.NewNullFunc( - condition.S3XAmzServerSideEncryption, - false, - ) - if err != nil { - t.Fatalf("unexpected error. %v\n", err) - } - case3Statement := NewStatement( - policy.Deny, - NewActionSet(GetObjectAction), - NewResourceSet(NewResource("mybucket", "/myobject*")), - condition.NewFunctions(func2), - ) - case3Data := []byte(`{"Effect":"Deny","Action":["s3:GetObject"],"Resource":["arn:aws:s3:::mybucket/myobject*"],"Condition":{"Null":{"s3:x-amz-server-side-encryption":[false]}}}`) - - case4Statement := NewStatement( - policy.Allow, - NewActionSet(GetObjectAction, PutObjectAction), - NewResourceSet(NewResource("mybucket", "myobject*")), - condition.NewFunctions(func1, func2), - ) - - testCases := []struct { - statement Statement - expectedResult []byte - expectErr bool - }{ - {case1Statement, case1Data, false}, - {case2Statement, case2Data, false}, - {case3Statement, case3Data, false}, - // Invalid statement error. - {case4Statement, nil, true}, - } - - for i, testCase := range testCases { - result, err := json.Marshal(testCase.statement) - expectErr := (err != nil) - - if expectErr != testCase.expectErr { - t.Fatalf("case %v: error: expected: %v, got: %v", i+1, testCase.expectErr, expectErr) - } - - if !testCase.expectErr { - if !reflect.DeepEqual(result, testCase.expectedResult) { - t.Fatalf("case %v: result: expected: %v, got: %v", i+1, string(testCase.expectedResult), string(result)) - } - } - } -} - -func TestStatementUnmarshalJSON(t *testing.T) { +func TestStatementUnmarshalJSONAndValidate(t *testing.T) { case1Data := []byte(`{ "Sid": "SomeId1", "Effect": "Allow", @@ -408,7 +333,7 @@ func TestStatementUnmarshalJSON(t *testing.T) { case4Data := []byte(`{ "Effect": "Allow", - "Action": "s3:PutObjec", + "Action": "s3:PutObjec, "Resource": "arn:aws:s3:::mybucket/myobject*" }`) @@ -450,36 +375,42 @@ func TestStatementUnmarshalJSON(t *testing.T) { }`) testCases := []struct { - data []byte - expectedResult Statement - expectErr bool + data []byte + expectedResult Statement + expectUnmarshalErr bool + expectValidationErr bool }{ - {case1Data, case1Statement, false}, - {case2Data, case2Statement, false}, - {case3Data, case3Statement, false}, + {case1Data, case1Statement, false, false}, + {case2Data, case2Statement, false, false}, + {case3Data, case3Statement, false, false}, // JSON unmarshaling error. - {case4Data, Statement{}, true}, + {case4Data, Statement{}, true, true}, // Invalid effect error. - {case5Data, Statement{}, true}, + {case5Data, Statement{}, false, true}, // Empty action error. - {case7Data, Statement{}, true}, + {case7Data, Statement{}, false, true}, // Empty resource error. - {case8Data, Statement{}, true}, + {case8Data, Statement{}, false, true}, // Empty condition error. - {case9Data, Statement{}, true}, + {case9Data, Statement{}, true, false}, // Unsupported condition key error. - {case10Data, Statement{}, true}, + {case10Data, Statement{}, false, true}, } for i, testCase := range testCases { var result Statement expectErr := (json.Unmarshal(testCase.data, &result) != nil) - if expectErr != testCase.expectErr { - t.Fatalf("case %v: error: expected: %v, got: %v", i+1, testCase.expectErr, expectErr) + if expectErr != testCase.expectUnmarshalErr { + t.Fatalf("case %v: error during unmarshal: expected: %v, got: %v", i+1, testCase.expectUnmarshalErr, expectErr) + } + + expectErr = (result.Validate() != nil) + if expectErr != testCase.expectValidationErr { + t.Fatalf("case %v: error during validation: expected: %v, got: %v", i+1, testCase.expectValidationErr, expectErr) } - if !testCase.expectErr { + if !testCase.expectUnmarshalErr && !testCase.expectValidationErr { if !reflect.DeepEqual(result, testCase.expectedResult) { t.Fatalf("case %v: result: expected: %v, got: %v", i+1, testCase.expectedResult, result) }