From 3128352693fce056958ad1c4626d861fcf48c4b0 Mon Sep 17 00:00:00 2001 From: Denis Kirillov Date: Tue, 19 Dec 2023 10:35:14 +0300 Subject: [PATCH] [#36] iam: Keep s3/iam prefixes in resources Signed-off-by: Denis Kirillov --- iam/converter.go | 38 ++++------- iam/converter_native.go | 51 +++++++++++---- iam/converter_s3.go | 53 ++++++--------- iam/converter_test.go | 141 ++++++++++++++-------------------------- schema/s3/consts.go | 8 +++ 5 files changed, 130 insertions(+), 161 deletions(-) diff --git a/iam/converter.go b/iam/converter.go index d7cd4a0..53b581e 100644 --- a/iam/converter.go +++ b/iam/converter.go @@ -244,50 +244,38 @@ func parsePrincipalAsIAMUser(principal string) (account string, user string, err return account, user, nil } -func parseResourceAsS3ARN(resource string) (bucket string, object string, err error) { +func validateResource(resource string) error { if resource == Wildcard { - return Wildcard, Wildcard, nil + return nil } - if !strings.HasPrefix(resource, s3ResourcePrefix) { - return "", "", ErrInvalidResourceFormat + if !strings.HasPrefix(resource, s3ResourcePrefix) && !strings.HasPrefix(resource, arnIAMPrefix) { + return ErrInvalidResourceFormat } - // iam arn format arn:aws:s3:::/ - s3Resource := strings.TrimPrefix(resource, s3ResourcePrefix) - sepIndex := strings.Index(s3Resource, "/") - if sepIndex < 0 { - return s3Resource, "", nil + index := strings.IndexByte(resource, Wildcard[0]) + if index != -1 && index != utf8.RuneCountInString(resource)-1 { + return ErrInvalidResourceFormat } - bucket = s3Resource[:sepIndex] - object = s3Resource[sepIndex+1:] - if len(object) == 0 { - return bucket, Wildcard, nil - } - - if bucket == Wildcard && object != Wildcard { - return "", "", ErrInvalidResourceFormat - } - - return bucket, object, nil + return nil } -func parseActionAsS3Action(action string) (string, error) { +func validateAction(action string) error { if action == Wildcard { - return Wildcard, nil + return nil } if !strings.HasPrefix(action, s3ActionPrefix) && !strings.HasPrefix(action, iamActionPrefix) { - return "", ErrInvalidActionFormat + return ErrInvalidActionFormat } index := strings.IndexByte(action, Wildcard[0]) if index != -1 && index != utf8.RuneCountInString(action)-1 { - return "", ErrInvalidActionFormat + return ErrInvalidActionFormat } - return action, nil + return nil } func splitGroupedConditions(groupedConditions []GroupedConditions) [][]chain.Condition { diff --git a/iam/converter_native.go b/iam/converter_native.go index 8e6c4a1..7eb7cad 100644 --- a/iam/converter_native.go +++ b/iam/converter_native.go @@ -161,30 +161,50 @@ func formNativeResourceNamesAndConditions(names []string, resolver NativeResolve var combined []string - for i := range names { - bkt, obj, err := parseResourceAsS3ARN(names[i]) - if err != nil { + for _, resource := range names { + if err := validateResource(resource); err != nil { return nil, err } - if bkt == Wildcard { + if resource == Wildcard { res = res[:0] return append(res, GroupedResources{Names: []string{native.ResourceFormatAllObjects}}), nil } + if !strings.HasPrefix(resource, s3ResourcePrefix) { + continue + } + + var bkt, obj string + s3Resource := strings.TrimPrefix(resource, s3ResourcePrefix) + if s3Resource == Wildcard { + res = res[:0] + return append(res, GroupedResources{Names: []string{native.ResourceFormatAllObjects}}), nil + } + + if sepIndex := strings.Index(s3Resource, "/"); sepIndex < 0 { + bkt = s3Resource + } else { + bkt = s3Resource[:sepIndex] + obj = s3Resource[sepIndex+1:] + if len(obj) == 0 { + obj = Wildcard + } + } + cnrID, err := resolver.GetBucketCID(bkt) if err != nil { return nil, err } - resource := fmt.Sprintf(native.ResourceFormatRootContainerObjects, cnrID) + nativeResource := fmt.Sprintf(native.ResourceFormatRootContainerObjects, cnrID) if obj == Wildcard || obj == "" { - combined = append(combined, resource) + combined = append(combined, nativeResource) continue } res = append(res, GroupedResources{ - Names: []string{resource}, + Names: []string{nativeResource}, Conditions: []chain.Condition{ { Op: chain.CondStringLike, @@ -233,14 +253,23 @@ func formPrincipalKey(principal string, resolver NativeResolver) (string, error) func formNativeActionNames(names []string) ([]string, error) { res := make([]string, 0, len(names)) - for i := range names { - action, err := parseActionAsS3Action(names[i]) - if err != nil { + for _, action := range names { + if err := validateAction(action); err != nil { return nil, err } - if action == Wildcard || strings.TrimPrefix(action, s3ActionPrefix) == Wildcard { + + if action == Wildcard { return []string{Wildcard}, nil } + + if !strings.HasPrefix(action, s3ActionPrefix) { + continue + } + + if strings.TrimPrefix(action, s3ActionPrefix) == Wildcard { + return []string{Wildcard}, nil + } + res = append(res, actionToOpMap[action]...) } diff --git a/iam/converter_s3.go b/iam/converter_s3.go index 5380097..d136e50 100644 --- a/iam/converter_s3.go +++ b/iam/converter_s3.go @@ -21,19 +21,17 @@ func ConvertToS3Chain(p Policy, resolver S3Resolver) (*chain.Chain, error) { for _, statement := range p.Statement { status := formStatus(statement) - action, actionInverted := statement.action() - s3Actions, err := formS3ActionNames(action) - if err != nil { + actions, actionInverted := statement.action() + if err := validateS3ActionNames(actions); err != nil { return nil, err } - ruleAction := chain.Actions{Inverted: actionInverted, Names: s3Actions} + ruleAction := chain.Actions{Inverted: actionInverted, Names: actions} - resource, resourceInverted := statement.resource() - s3Resources, err := formS3ResourceNames(resource) - if err != nil { + resources, resourceInverted := statement.resource() + if err := validateS3ResourceNames(resources); err != nil { return nil, err } - ruleResource := chain.Resources{Inverted: resourceInverted, Names: s3Resources} + ruleResource := chain.Resources{Inverted: resourceInverted, Names: resources} groupedConditions, err := convertToS3ChainCondition(statement.Conditions, resolver) if err != nil { @@ -141,33 +139,22 @@ func formPrincipalOwner(principal string, resolver S3Resolver) (string, error) { return address, nil } -func formS3ResourceNames(names []string) ([]string, error) { - res := make([]string, len(names)) +func validateS3ResourceNames(names []string) error { for i := range names { - bkt, obj, err := parseResourceAsS3ARN(names[i]) - if err != nil { - return nil, err - } - - if bkt == Wildcard || obj == "" { - res[i] = bkt - continue - } - - res[i] = bkt + "/" + obj - } - - return res, nil -} - -func formS3ActionNames(names []string) ([]string, error) { - var err error - res := make([]string, len(names)) - for i := range names { - if res[i], err = parseActionAsS3Action(names[i]); err != nil { - return nil, err + if err := validateResource(names[i]); err != nil { + return err } } - return res, nil + return nil +} + +func validateS3ActionNames(names []string) error { + for i := range names { + if err := validateAction(names[i]); err != nil { + return err + } + } + + return nil } diff --git a/iam/converter_test.go b/iam/converter_test.go index b5836fd..000d5cd 100644 --- a/iam/converter_test.go +++ b/iam/converter_test.go @@ -69,7 +69,7 @@ func TestConverters(t *testing.T) { principal := "arn:aws:iam::" + namespace + ":user/" + userName bktName := "DOC-EXAMPLE-BUCKET" objName := "object-name" - resource := bktName + "/*" + resource := fmt.Sprintf(s3.ResourceFormatS3BucketObjects, bktName) s3action := "s3:PutObject" mockResolver := newMockUserResolver([]string{user}, []string{bktName}) @@ -83,7 +83,7 @@ func TestConverters(t *testing.T) { }, Effect: AllowEffect, Action: []string{"s3:PutObject"}, - Resource: []string{"arn:aws:s3:::" + resource}, + Resource: []string{resource}, Conditions: map[string]Condition{ CondStringEquals: { "s3:RequestObjectTag/Department": {"Finance"}, @@ -128,7 +128,7 @@ func TestConverters(t *testing.T) { }, Effect: AllowEffect, Action: []string{"s3:PutObject"}, - Resource: []string{"arn:aws:s3:::" + resource}, + Resource: []string{resource}, }}, } @@ -162,7 +162,7 @@ func TestConverters(t *testing.T) { }, Effect: DenyEffect, NotAction: []string{"s3:PutObject"}, - NotResource: []string{"arn:aws:s3:::" + resource}, + NotResource: []string{resource}, }}, } @@ -196,7 +196,7 @@ func TestConverters(t *testing.T) { }, Effect: DenyEffect, NotAction: []string{"s3:GetObject"}, - NotResource: []string{"arn:aws:s3:::" + bktName + "/" + objName}, + NotResource: []string{fmt.Sprintf(s3.ResourceFormatS3BucketObject, bktName, objName)}, }}, } @@ -892,9 +892,9 @@ func TestComplexS3Conditions(t *testing.T) { principal2 := "arn:aws:iam::" + namespace + ":user/" + userName2 bktName1, bktName2, bktName3 := "bktName", "bktName2", "bktName3" objName1 := "objName1" - resource1 := bktName1 + "/" + objName1 - resource2 := bktName2 + "/*" - resource3 := bktName3 + "/*" + resource1 := fmt.Sprintf(s3.ResourceFormatS3BucketObject, bktName1, objName1) + resource2 := fmt.Sprintf(s3.ResourceFormatS3BucketObjects, bktName2) + resource3 := fmt.Sprintf(s3.ResourceFormatS3BucketObjects, bktName3) action := "s3:PutObject" key1, key2 := "key1", "key2" @@ -910,7 +910,7 @@ func TestComplexS3Conditions(t *testing.T) { }, Effect: DenyEffect, Action: []string{action}, - Resource: []string{"arn:aws:s3:::" + resource1, "arn:aws:s3:::" + resource2, "arn:aws:s3:::" + resource3}, + Resource: []string{resource1, resource2, resource3}, Conditions: map[string]Condition{ CondStringEquals: {key1: {val0, val1}}, CondStringLike: {key2: {val2}}, @@ -1001,7 +1001,7 @@ func TestComplexS3Conditions(t *testing.T) { { name: "bucket resource3, all conditions matched", action: action, - resource: bktName3 + "/some-obj", + resource: fmt.Sprintf(s3.ResourceFormatS3BucketObject, bktName3, "some-obj"), requestMap: map[string]string{ s3.PropertyKeyOwner: mockResolver.users[user1], key1: val0, @@ -1012,7 +1012,7 @@ func TestComplexS3Conditions(t *testing.T) { { name: "bucket resource, user condition mismatched", action: action, - resource: bktName2 + "/some-obj", + resource: fmt.Sprintf(s3.ResourceFormatS3BucketObject, bktName2, "some-obj"), requestMap: map[string]string{ key1: val0, key2: val2, @@ -1022,7 +1022,7 @@ func TestComplexS3Conditions(t *testing.T) { { name: "bucket resource, key2 condition mismatched", action: action, - resource: bktName3 + "/some-obj", + resource: fmt.Sprintf(s3.ResourceFormatS3BucketObject, bktName3, "some-obj"), requestMap: map[string]string{ s3.PropertyKeyOwner: mockResolver.users[user1], key1: val0, @@ -1033,7 +1033,7 @@ func TestComplexS3Conditions(t *testing.T) { { name: "bucket resource, key1 condition mismatched", action: action, - resource: bktName2 + "/some-obj", + resource: fmt.Sprintf(s3.ResourceFormatS3BucketObject, bktName2, "some-obj"), requestMap: map[string]string{ s3.PropertyKeyOwner: mockResolver.users[user1], key2: val2, @@ -1054,7 +1054,7 @@ func TestComplexS3Conditions(t *testing.T) { { name: "bucket/object resource, resource mismatched", action: action, - resource: bktName1 + "/some-obj", + resource: fmt.Sprintf(s3.ResourceFormatS3BucketObject, bktName1, "some-obj"), requestMap: map[string]string{ s3.PropertyKeyOwner: mockResolver.users[user1], key1: val0, @@ -1097,7 +1097,7 @@ func TestComplexS3Conditions(t *testing.T) { { name: "resource mismatched", action: action, - resource: "some-bkt/some-obj", + resource: fmt.Sprintf(s3.ResourceFormatS3BucketObject, "some-bkt", "some-obj"), requestMap: map[string]string{ s3.PropertyKeyOwner: mockResolver.users[user1], key1: val0, @@ -1120,7 +1120,7 @@ func TestS3BucketResource(t *testing.T) { bktName1, bktName2 := "bucket1", "bucket2" chainName := chain.Name("name") - mockResolver := newMockUserResolver([]string{}, []string{bktName1}) + mockResolver := newMockUserResolver([]string{}, []string{}) p := Policy{ Version: "2012-10-17", @@ -1129,13 +1129,13 @@ func TestS3BucketResource(t *testing.T) { Principal: map[PrincipalType][]string{Wildcard: nil}, Effect: DenyEffect, Action: []string{"s3:HeadBucket"}, - Resource: []string{"arn:aws:s3:::" + bktName1}, + Resource: []string{fmt.Sprintf(s3.ResourceFormatS3Bucket, bktName1)}, }, { Principal: map[PrincipalType][]string{Wildcard: nil}, Effect: AllowEffect, Action: []string{"*"}, - Resource: []string{"arn:aws:s3:::*"}, + Resource: []string{s3.ResourceFormatS3All}, }, }, } @@ -1148,19 +1148,19 @@ func TestS3BucketResource(t *testing.T) { require.NoError(t, err) // check we match just "bucket1" resource - req := testutil.NewRequest("s3:HeadBucket", testutil.NewResource(bktName1, nil), nil) + req := testutil.NewRequest("s3:HeadBucket", testutil.NewResource(fmt.Sprintf(s3.ResourceFormatS3Bucket, bktName1), nil), nil) status, _, err := s.IsAllowed(chainName, engine.NewRequestTargetWithNamespace(namespace), req) require.NoError(t, err) require.Equal(t, chain.AccessDenied.String(), status.String()) // check we match just "bucket2" resource - req = testutil.NewRequest("s3:HeadBucket", testutil.NewResource(bktName2, nil), nil) + req = testutil.NewRequest("s3:HeadBucket", testutil.NewResource(fmt.Sprintf(s3.ResourceFormatS3Bucket, bktName2), nil), nil) status, _, err = s.IsAllowed(chainName, engine.NewRequestTargetWithNamespace(namespace), req) require.NoError(t, err) require.Equal(t, chain.Allow.String(), status.String()) // check we also match "bucket2/object" resource - req = testutil.NewRequest("s3:PutObject", testutil.NewResource(bktName2+"/object", nil), nil) + req = testutil.NewRequest("s3:PutObject", testutil.NewResource(fmt.Sprintf(s3.ResourceFormatS3BucketObject, bktName2, "object"), nil), nil) status, _, err = s.IsAllowed(chainName, engine.NewRequestTargetWithNamespace(namespace), req) require.NoError(t, err) require.Equal(t, chain.Allow.String(), status.String()) @@ -1182,58 +1182,46 @@ func TestWildcardConverters(t *testing.T) { func TestActionParsing(t *testing.T) { for _, tc := range []struct { - action string - expected string - err bool + action string + err bool }{ { - action: "withoutPrefix", - expected: "", - err: true, + action: "withoutPrefix", + err: true, }, { - action: "s3:*Object", - expected: "", - err: true, + action: "s3:*Object", + err: true, }, { - action: "*", - expected: "*", + action: "*", }, { - action: "s3:PutObject", - expected: "s3:PutObject", + action: "s3:PutObject", }, { - action: "s3:Put*", - expected: "s3:Put*", + action: "s3:Put*", }, { - action: "s3:*", - expected: "s3:*", + action: "s3:*", }, { - action: "s3:", - expected: "s3:", + action: "s3:", }, { - action: "iam:ListAccessKeys", - expected: "iam:ListAccessKeys", + action: "iam:ListAccessKeys", }, { - action: "iam:*", - expected: "iam:*", + action: "iam:*", }, } { t.Run("", func(t *testing.T) { - actual, err := parseActionAsS3Action(tc.action) + err := validateAction(tc.action) if tc.err { require.Error(t, err) - return + } else { + require.NoError(t, err) } - - require.NoError(t, err) - require.Equal(t, tc.expected, actual) }) } } @@ -1303,55 +1291,24 @@ func TestPrincipalParsing(t *testing.T) { func TestResourceParsing(t *testing.T) { for _, tc := range []struct { - resource string - expectedBucket string - expectedObject string - err bool + resource 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: "*", - }, + {resource: "withoutPrefixAnd", err: true}, + {resource: "arn:aws:s3:::*/obj", err: true}, + {resource: "arn:aws:s3:::bkt/*"}, + {resource: "arn:aws:s3:::bkt"}, + {resource: "arn:aws:s3:::bkt/"}, + {resource: "arn:aws:s3:::*"}, + {resource: "*"}, } { t.Run("", func(t *testing.T) { - bkt, obj, err := parseResourceAsS3ARN(tc.resource) + err := validateResource(tc.resource) if tc.err { require.Error(t, err) - return + } else { + require.NoError(t, err) } - - require.NoError(t, err) - require.Equal(t, tc.expectedBucket, bkt) - require.Equal(t, tc.expectedObject, obj) }) } } diff --git a/schema/s3/consts.go b/schema/s3/consts.go index a8de81f..7159a32 100644 --- a/schema/s3/consts.go +++ b/schema/s3/consts.go @@ -6,4 +6,12 @@ const ( PropertyKeyDelimiter = "s3:delimiter" PropertyKeyPrefix = "s3:prefix" PropertyKeyVersionID = "s3:VersionId" + + ResourceFormatS3All = "arn:aws:s3:::*" + ResourceFormatS3Bucket = "arn:aws:s3:::%s" + ResourceFormatS3BucketObjects = "arn:aws:s3:::%s/*" + ResourceFormatS3BucketObject = "arn:aws:s3:::%s/%s" + + ResourceFormatIAMNamespaceUser = "arn:aws:iam::%s:user/%s" + ResourceFormatIAMNamespaceGroup = "arn:aws:iam::%s:group/%s" )