From 9040e48504c03b0e2a0a9c51c02ff849f2c56c71 Mon Sep 17 00:00:00 2001 From: Marina Biryukova Date: Mon, 11 Mar 2024 13:30:28 +0300 Subject: [PATCH] [#57] iam: Add policy validation checks Signed-off-by: Marina Biryukova --- iam/converter_test.go | 1 + iam/policy.go | 11 +++++++++ iam/policy_test.go | 55 ++++++++++++++++++++++++++++++++++++++++--- 3 files changed, 64 insertions(+), 3 deletions(-) diff --git a/iam/converter_test.go b/iam/converter_test.go index 868e63f..350e218 100644 --- a/iam/converter_test.go +++ b/iam/converter_test.go @@ -315,6 +315,7 @@ func TestConverters(t *testing.T) { t.Run("valid mixed iam/s3 actions", func(t *testing.T) { p := Policy{ + Version: "2012-10-17", Statement: []Statement{{ Principal: map[PrincipalType][]string{AWSPrincipalType: {principal}}, Effect: AllowEffect, diff --git a/iam/policy.go b/iam/policy.go index d5649fa..ab8194f 100644 --- a/iam/policy.go +++ b/iam/policy.go @@ -46,6 +46,8 @@ type ( PrincipalType string ) +const policyVersion = "2012-10-17" + const ( GeneralPolicyType PolicyType = iota IdentityBasedPolicyType @@ -222,11 +224,20 @@ func (p Policy) Validate(typ PolicyType) error { } func (p Policy) validate() error { + if p.Version != policyVersion { + return fmt.Errorf("invalid policy version, expected '%s', actual: '%s'", policyVersion, p.Version) + } + if len(p.Statement) == 0 { return errors.New("'Statement' is missing") } + sids := make(map[string]struct{}, len(p.Statement)) for _, statement := range p.Statement { + if _, ok := sids[statement.SID]; ok && statement.SID != "" { + return fmt.Errorf("duplicate 'SID': %s", statement.SID) + } + sids[statement.SID] = struct{}{} if !statement.Effect.IsValid() { return fmt.Errorf("unknown effect: '%s'", statement.Effect) } diff --git a/iam/policy_test.go b/iam/policy_test.go index 20aed3b..ef66d68 100644 --- a/iam/policy_test.go +++ b/iam/policy_test.go @@ -218,6 +218,7 @@ func TestValidatePolicies(t *testing.T) { { name: "valid permission boundaries", policy: Policy{ + Version: policyVersion, Statement: []Statement{{ Effect: AllowEffect, Action: []string{"s3:*", "cloudwatch:*", "ec2:*"}, @@ -230,6 +231,7 @@ func TestValidatePolicies(t *testing.T) { { name: "general invalid effect", policy: Policy{ + Version: policyVersion, Statement: []Statement{{ Effect: "dummy", Action: []string{"s3:*", "cloudwatch:*", "ec2:*"}, @@ -242,6 +244,7 @@ func TestValidatePolicies(t *testing.T) { { name: "general invalid principal block", policy: Policy{ + Version: policyVersion, Statement: []Statement{{ Effect: AllowEffect, Action: []string{"s3:*", "cloudwatch:*", "ec2:*"}, @@ -256,6 +259,7 @@ func TestValidatePolicies(t *testing.T) { { name: "general invalid not principal", policy: Policy{ + Version: policyVersion, Statement: []Statement{{ Effect: AllowEffect, Action: []string{"s3:*", "cloudwatch:*", "ec2:*"}, @@ -269,6 +273,7 @@ func TestValidatePolicies(t *testing.T) { { name: "general invalid principal type", policy: Policy{ + Version: policyVersion, Statement: []Statement{{ Effect: AllowEffect, Action: []string{"s3:*", "cloudwatch:*", "ec2:*"}, @@ -282,6 +287,7 @@ func TestValidatePolicies(t *testing.T) { { name: "general invalid action block", policy: Policy{ + Version: policyVersion, Statement: []Statement{{ Effect: AllowEffect, Action: []string{"s3:*", "cloudwatch:*", "ec2:*"}, @@ -295,6 +301,7 @@ func TestValidatePolicies(t *testing.T) { { name: "general invalid resource block", policy: Policy{ + Version: policyVersion, Statement: []Statement{{ Effect: AllowEffect, Resource: []string{Wildcard}, @@ -307,6 +314,7 @@ func TestValidatePolicies(t *testing.T) { { name: "invalid resource block", policy: Policy{ + Version: policyVersion, Statement: []Statement{{ Effect: AllowEffect, Resource: []string{}, @@ -319,6 +327,7 @@ func TestValidatePolicies(t *testing.T) { { name: "missing resource block", policy: Policy{ + Version: policyVersion, Statement: []Statement{{ Effect: AllowEffect, }}, @@ -332,9 +341,43 @@ func TestValidatePolicies(t *testing.T) { typ: GeneralPolicyType, isValid: false, }, + { + name: "duplicate sid", + policy: Policy{ + Version: policyVersion, + Statement: []Statement{ + { + SID: "sid", + Effect: AllowEffect, + Action: []string{"s3:*"}, + Resource: []string{Wildcard}, + }, + { + SID: "sid", + Effect: AllowEffect, + Action: []string{"cloudwatch:*"}, + Resource: []string{Wildcard}, + }}, + }, + typ: GeneralPolicyType, + isValid: false, + }, + { + name: "missing version", + policy: Policy{ + Statement: []Statement{{ + Effect: AllowEffect, + Action: []string{"s3:*"}, + Resource: []string{Wildcard}, + }}, + }, + typ: GeneralPolicyType, + isValid: false, + }, { name: "identity based valid", policy: Policy{ + Version: policyVersion, Statement: []Statement{{ Effect: AllowEffect, Action: []string{"s3:PutObject"}, @@ -347,7 +390,8 @@ func TestValidatePolicies(t *testing.T) { { name: "identity based invalid because of id presence", policy: Policy{ - ID: "some-id", + ID: "some-id", + Version: policyVersion, Statement: []Statement{{ Effect: AllowEffect, Action: []string{"s3:PutObject"}, @@ -360,6 +404,7 @@ func TestValidatePolicies(t *testing.T) { { name: "identity based invalid because of principal presence", policy: Policy{ + Version: policyVersion, Statement: []Statement{{ Effect: AllowEffect, Action: []string{"s3:PutObject"}, @@ -373,6 +418,7 @@ func TestValidatePolicies(t *testing.T) { { name: "identity based invalid because of not principal presence", policy: Policy{ + Version: policyVersion, Statement: []Statement{{ Effect: AllowEffect, Action: []string{"s3:PutObject"}, @@ -386,6 +432,7 @@ func TestValidatePolicies(t *testing.T) { { name: "resource based valid principal", policy: Policy{ + Version: policyVersion, Statement: []Statement{{ Effect: DenyEffect, Action: []string{"s3:PutObject"}, @@ -399,7 +446,8 @@ func TestValidatePolicies(t *testing.T) { { name: "resource based valid not principal", policy: Policy{ - ID: "some-id", + ID: "some-id", + Version: policyVersion, Statement: []Statement{{ Effect: DenyEffect, Action: []string{"s3:PutObject"}, @@ -413,7 +461,8 @@ func TestValidatePolicies(t *testing.T) { { name: "resource based invalid missing principal", policy: Policy{ - ID: "some-id", + ID: "some-id", + Version: policyVersion, Statement: []Statement{{ Effect: AllowEffect, Action: []string{"s3:PutObject"},