From a0a35bf4bf3142054ac44466ae91964af433b9ed Mon Sep 17 00:00:00 2001 From: Denis Kirillov Date: Tue, 28 Nov 2023 17:56:36 +0300 Subject: [PATCH] [#22] iam: Fix converters Validate that actions and resources contain wildcard only at the end Signed-off-by: Denis Kirillov --- iam/converter.go | 28 ++++++ iam/converter_native.go | 22 +++-- iam/converter_s3.go | 38 +++++++-- iam/converter_test.go | 184 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 255 insertions(+), 17 deletions(-) diff --git a/iam/converter.go b/iam/converter.go index 9002d83..b288de3 100644 --- a/iam/converter.go +++ b/iam/converter.go @@ -6,6 +6,7 @@ import ( "strconv" "strings" "time" + "unicode/utf8" "git.frostfs.info/TrueCloudLab/policy-engine/pkg/chain" ) @@ -63,6 +64,9 @@ var ( // ErrInvalidResourceFormat occurs when resource has unknown/unsupported format. ErrInvalidResourceFormat = errors.New("invalid resource format") + + // ErrInvalidActionFormat occurs when action has unknown/unsupported format. + ErrInvalidActionFormat = errors.New("invalid action format") ) type formPrincipalConditionFunc func(string) chain.Condition @@ -240,6 +244,10 @@ func parsePrincipalAsIAMUser(principal string) (account string, user string, err } func parseResourceAsS3ARN(resource string) (bucket string, object string, err error) { + if resource == Wildcard { + return Wildcard, Wildcard, nil + } + if !strings.HasPrefix(resource, s3ResourcePrefix) { return "", "", ErrInvalidResourceFormat } @@ -264,6 +272,26 @@ func parseResourceAsS3ARN(resource string) (bucket string, object string, err er return bucket, object, nil } +func parseActionAsS3Action(action string) (string, error) { + if action == Wildcard { + return Wildcard, nil + } + + if !strings.HasPrefix(action, s3ActionPrefix) { + return "", ErrInvalidActionFormat + } + + // iam arn format :s3: + s3Action := strings.TrimPrefix(action, s3ActionPrefix) + + index := strings.IndexByte(s3Action, Wildcard[0]) + if index != -1 && index != utf8.RuneCountInString(s3Action)-1 { + return "", ErrInvalidActionFormat + } + + return s3Action, nil +} + func splitGroupedConditions(groupedConditions []GroupedConditions) [][]chain.Condition { var orConditions []chain.Condition commonConditions := make([]chain.Condition, 0, len(groupedConditions)) diff --git a/iam/converter_native.go b/iam/converter_native.go index 4399ebe..4abfcdf 100644 --- a/iam/converter_native.go +++ b/iam/converter_native.go @@ -3,7 +3,6 @@ package iam import ( "errors" "fmt" - "strings" "git.frostfs.info/TrueCloudLab/policy-engine/pkg/chain" "git.frostfs.info/TrueCloudLab/policy-engine/schema/native" @@ -46,7 +45,11 @@ func ConvertToNativeChain(p Policy, resolver NativeResolver) (*chain.Chain, erro status := formStatus(statement) action, actionInverted := statement.action() - ruleAction := chain.Actions{Inverted: actionInverted, Names: formNativeActionNames(action)} + nativeActions, err := formNativeActionNames(action) + if err != nil { + return nil, err + } + ruleAction := chain.Actions{Inverted: actionInverted, Names: nativeActions} if len(ruleAction.Names) == 0 { continue } @@ -226,16 +229,19 @@ func formPrincipalKey(principal string, resolver NativeResolver) (string, error) return key, nil } -func formNativeActionNames(names []string) []string { +func formNativeActionNames(names []string) ([]string, error) { res := make([]string, 0, len(names)) for i := range names { - trimmed := strings.TrimPrefix(names[i], s3ActionPrefix) - if trimmed == Wildcard { - return []string{Wildcard} + action, err := parseActionAsS3Action(names[i]) + if err != nil { + return nil, err } - res = append(res, actionToOpMap[trimmed]...) + if action == Wildcard { + return []string{Wildcard}, nil + } + res = append(res, actionToOpMap[action]...) } - return res + return res, nil } diff --git a/iam/converter_s3.go b/iam/converter_s3.go index 7fed390..094c3d3 100644 --- a/iam/converter_s3.go +++ b/iam/converter_s3.go @@ -2,7 +2,6 @@ package iam import ( "fmt" - "strings" "git.frostfs.info/TrueCloudLab/policy-engine/pkg/chain" "git.frostfs.info/TrueCloudLab/policy-engine/schema/s3" @@ -23,10 +22,18 @@ func ConvertToS3Chain(p Policy, resolver S3Resolver) (*chain.Chain, error) { status := formStatus(statement) action, actionInverted := statement.action() - ruleAction := chain.Actions{Inverted: actionInverted, Names: formS3ActionNames(action)} + s3Actions, err := formS3ActionNames(action) + if err != nil { + return nil, err + } + ruleAction := chain.Actions{Inverted: actionInverted, Names: s3Actions} resource, resourceInverted := statement.resource() - ruleResource := chain.Resources{Inverted: resourceInverted, Names: formS3ResourceNamesAndConditions(resource)} + s3Resources, err := formS3ResourceNames(resource) + if err != nil { + return nil, err + } + ruleResource := chain.Resources{Inverted: resourceInverted, Names: s3Resources} groupedConditions, err := convertToS3ChainCondition(statement.Conditions, resolver) if err != nil { @@ -134,20 +141,33 @@ func formPrincipalOwner(principal string, resolver S3Resolver) (string, error) { return address, nil } -func formS3ResourceNamesAndConditions(names []string) []string { +func formS3ResourceNames(names []string) ([]string, error) { res := make([]string, len(names)) for i := range names { - res[i] = strings.TrimPrefix(names[i], s3ResourcePrefix) + bkt, obj, err := parseResourceAsS3ARN(names[i]) + if err != nil { + return nil, err + } + + if bkt == Wildcard { + res[i] = bkt + continue + } + + res[i] = bkt + "/" + obj } - return res + return res, nil } -func formS3ActionNames(names []string) []string { +func formS3ActionNames(names []string) ([]string, error) { + var err error res := make([]string, len(names)) for i := range names { - res[i] = strings.TrimPrefix(names[i], s3ActionPrefix) + if res[i], err = parseActionAsS3Action(names[i]); err != nil { + return nil, err + } } - return res + return res, nil } diff --git a/iam/converter_test.go b/iam/converter_test.go index 1b9d2f7..c3eed2d 100644 --- a/iam/converter_test.go +++ b/iam/converter_test.go @@ -1,6 +1,7 @@ package iam import ( + "encoding/json" "errors" "fmt" "strconv" @@ -1071,6 +1072,189 @@ func TestComplexS3Conditions(t *testing.T) { } } +func TestWildcardConverters(t *testing.T) { + policy := `{"Version":"2012-10-17","Statement":{"Effect":"Allow", "Principal": "*", "Action":"*","Resource":"*"}}` + + var p Policy + err := json.Unmarshal([]byte(policy), &p) + require.NoError(t, err) + + _, err = ConvertToS3Chain(p, newMockUserResolver(nil, nil)) + require.NoError(t, err) + + _, err = ConvertToNativeChain(p, newMockUserResolver(nil, nil)) + require.NoError(t, err) +} + +func TestActionParsing(t *testing.T) { + for _, tc := range []struct { + action string + expected string + err bool + }{ + { + action: "withoutPrefix", + expected: "", + err: true, + }, + { + action: "s3:*Object", + expected: "", + err: true, + }, + { + action: "*", + expected: "*", + }, + { + action: "s3:PutObject", + expected: "PutObject", + }, + { + action: "s3:Put*", + expected: "Put*", + }, + { + action: "s3:*", + expected: "*", + }, + { + action: "s3:", + + expected: "", + }, + } { + t.Run("", func(t *testing.T) { + actual, err := parseActionAsS3Action(tc.action) + if tc.err { + require.Error(t, err) + return + } + + require.NoError(t, err) + require.Equal(t, tc.expected, actual) + }) + } +} + +func TestPrincipalParsing(t *testing.T) { + for _, tc := range []struct { + principal string + expectedAccount string + expectedUser string + err bool + }{ + { + principal: "withoutPrefix", + err: true, + }, + { + principal: "*", + err: true, + }, + { + principal: "arn:aws:iam:::dummy", + err: true, + }, + { + principal: "arn:aws:iam::", + err: true, + }, + { + principal: "arn:aws:iam:::dummy/test", + err: true, + }, + { + principal: "arn:aws:iam:::user/", + err: true, + }, + { + principal: "arn:aws:iam:::user/user/", + err: true, + }, + { + principal: "arn:aws:iam:::user/name", + expectedUser: "name", + }, + { + principal: "arn:aws:iam:::user/path/name", + expectedUser: "name", + }, + { + principal: "arn:aws:iam::root:user/path/name", + expectedAccount: "root", + expectedUser: "name", + }, + } { + t.Run("", func(t *testing.T) { + account, user, err := parsePrincipalAsIAMUser(tc.principal) + if tc.err { + require.Error(t, err) + return + } + + require.NoError(t, err) + require.Equal(t, tc.expectedAccount, account) + require.Equal(t, tc.expectedUser, user) + }) + } +} + +func TestResourceParsing(t *testing.T) { + for _, tc := range []struct { + resource string + expectedBucket string + expectedObject string + err bool + }{ + { + resource: "withoutPrefixAnd", + err: true, + }, + { + resource: "arn:aws:s3:::*/obj", + err: true, + }, + { + resource: "arn:aws:s3:::bkt/*", + expectedBucket: "bkt", + expectedObject: "*", + }, + { + resource: "arn:aws:s3:::bkt", + expectedBucket: "bkt", + expectedObject: "*", + }, + { + resource: "arn:aws:s3:::bkt/", + expectedBucket: "bkt", + expectedObject: "*", + }, + { + resource: "arn:aws:s3:::*", + expectedBucket: "*", + expectedObject: "*", + }, + { + resource: "*", + expectedBucket: "*", + expectedObject: "*", + }, + } { + t.Run("", func(t *testing.T) { + bkt, obj, err := parseResourceAsS3ARN(tc.resource) + if tc.err { + require.Error(t, err) + return + } + + require.NoError(t, err) + require.Equal(t, tc.expectedBucket, bkt) + require.Equal(t, tc.expectedObject, obj) + }) + } +} + func requireChainRulesMatch(t *testing.T, expected, actual []chain.Rule) { require.Equal(t, len(expected), len(actual), "length of chain rules differ")