[#57] iam: Add policy validation checks

Signed-off-by: Marina Biryukova <m.biryukova@yadro.com>

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,

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)
 		}

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"},