Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

provider/aws: validation: Add validation function for IAM Policies #14669

Merged
merged 2 commits into from
May 22, 2017
Merged

provider/aws: validation: Add validation function for IAM Policies #14669

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
96e8381
provider/aws: validation: Add validation function for IAM Policies
grubernaut May 19, 2017
dffa575
add docstring on ignored error
grubernaut May 19, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions builtin/providers/aws/resource_aws_iam_policy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,24 +24,24 @@ func resourceAwsIamPolicy() *schema.Resource {
},

Schema: map[string]*schema.Schema{
"description": &schema.Schema{
"description": {
Type: schema.TypeString,
ForceNew: true,
Optional: true,
},
"path": &schema.Schema{
"path": {
Type: schema.TypeString,
Optional: true,
Default: "/",
ForceNew: true,
},
"policy": &schema.Schema{
"policy": {
Type: schema.TypeString,
Required: true,
ValidateFunc: validateJsonString,
ValidateFunc: validateIAMPolicyJson,
DiffSuppressFunc: suppressEquivalentAwsPolicyDiffs,
},
"name": &schema.Schema{
"name": {
Type: schema.TypeString,
Optional: true,
Computed: true,
Expand Down Expand Up @@ -79,7 +79,7 @@ func resourceAwsIamPolicy() *schema.Resource {
return
},
},
"arn": &schema.Schema{
"arn": {
Type: schema.TypeString,
Computed: true,
},
Expand Down
37 changes: 36 additions & 1 deletion builtin/providers/aws/resource_aws_iam_policy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package aws

import (
"fmt"
"regexp"
"strings"
"testing"

Expand All @@ -19,7 +20,7 @@ func TestAWSPolicy_namePrefix(t *testing.T) {
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSPolicyDestroy,
Steps: []resource.TestStep{
resource.TestStep{
{
Config: testAccAWSPolicyPrefixNameConfig,
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSPolicyExists("aws_iam_policy.policy", &out),
Expand All @@ -31,6 +32,20 @@ func TestAWSPolicy_namePrefix(t *testing.T) {
})
}

func TestAWSPolicy_invalidJson(t *testing.T) {
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSPolicyDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSPolicyInvalidJsonConfig,
ExpectError: regexp.MustCompile("invalid JSON"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit overkill to test validation function on a real resource, but 🤷‍♂️ I guess it does the job 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly wanted to test the HEREDOC evaluation would fail prior to the apply phase.

},
},
})
}

func testAccCheckAWSPolicyExists(resource string, res *iam.GetPolicyOutput) resource.TestCheckFunc {
return func(s *terraform.State) error {
rs, ok := s.RootModule().Resources[resource]
Expand Down Expand Up @@ -94,3 +109,23 @@ resource "aws_iam_policy" "policy" {
EOF
}
`
const testAccAWSPolicyInvalidJsonConfig = `
resource "aws_iam_policy" "policy" {
name_prefix = "test-policy-"
path = "/"
policy = <<EOF
{
"Version": "2012-10-17",
"Statement": [
{
"Action": [
"ec2:Describe*"
],
"Effect": "Allow",
"Resource": "*"
}
]
}
EOF
}
`
14 changes: 8 additions & 6 deletions builtin/providers/aws/resource_aws_iam_role_policy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,25 +26,27 @@ func resourceAwsIamRolePolicy() *schema.Resource {
},

Schema: map[string]*schema.Schema{
"policy": &schema.Schema{
Type: schema.TypeString,
Required: true,
"policy": {
Type: schema.TypeString,
Required: true,
ValidateFunc: validateIAMPolicyJson,
DiffSuppressFunc: suppressEquivalentAwsPolicyDiffs,
},
"name": &schema.Schema{
"name": {
Type: schema.TypeString,
Optional: true,
Computed: true,
ForceNew: true,
ConflictsWith: []string{"name_prefix"},
ValidateFunc: validateIamRolePolicyName,
},
"name_prefix": &schema.Schema{
"name_prefix": {
Type: schema.TypeString,
Optional: true,
ForceNew: true,
ValidateFunc: validateIamRolePolicyNamePrefix,
},
"role": &schema.Schema{
"role": {
Type: schema.TypeString,
Required: true,
ForceNew: true,
Expand Down
64 changes: 60 additions & 4 deletions builtin/providers/aws/resource_aws_iam_role_policy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package aws

import (
"fmt"
"regexp"
"testing"

"github.com/aws/aws-sdk-go/aws"
Expand All @@ -22,7 +23,7 @@ func TestAccAWSIAMRolePolicy_basic(t *testing.T) {
Providers: testAccProviders,
CheckDestroy: testAccCheckIAMRolePolicyDestroy,
Steps: []resource.TestStep{
resource.TestStep{
{
Config: testAccIAMRolePolicyConfig(role, policy1),
Check: resource.ComposeTestCheckFunc(
testAccCheckIAMRolePolicy(
Expand All @@ -31,7 +32,7 @@ func TestAccAWSIAMRolePolicy_basic(t *testing.T) {
),
),
},
resource.TestStep{
{
Config: testAccIAMRolePolicyConfigUpdate(role, policy1, policy2),
Check: resource.ComposeTestCheckFunc(
testAccCheckIAMRolePolicy(
Expand All @@ -53,7 +54,7 @@ func TestAccAWSIAMRolePolicy_namePrefix(t *testing.T) {
Providers: testAccProviders,
CheckDestroy: testAccCheckIAMRolePolicyDestroy,
Steps: []resource.TestStep{
resource.TestStep{
{
Config: testAccIAMRolePolicyConfig_namePrefix(role),
Check: resource.ComposeTestCheckFunc(
testAccCheckIAMRolePolicy(
Expand All @@ -75,7 +76,7 @@ func TestAccAWSIAMRolePolicy_generatedName(t *testing.T) {
Providers: testAccProviders,
CheckDestroy: testAccCheckIAMRolePolicyDestroy,
Steps: []resource.TestStep{
resource.TestStep{
{
Config: testAccIAMRolePolicyConfig_generatedName(role),
Check: resource.ComposeTestCheckFunc(
testAccCheckIAMRolePolicy(
Expand All @@ -88,6 +89,22 @@ func TestAccAWSIAMRolePolicy_generatedName(t *testing.T) {
})
}

func TestAccAWSIAMRolePolicy_invalidJSON(t *testing.T) {
role := acctest.RandString(10)

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckIAMRolePolicyDestroy,
Steps: []resource.TestStep{
{
Config: testAccIAMRolePolicyConfig_invalidJSON(role),
ExpectError: regexp.MustCompile("invalid JSON"),
},
},
})
}

func testAccCheckIAMRolePolicyDestroy(s *terraform.State) error {
iamconn := testAccProvider.Meta().(*AWSClient).iamconn

Expand Down Expand Up @@ -328,3 +345,42 @@ EOF
}
`, role, policy1, policy2)
}

func testAccIAMRolePolicyConfig_invalidJSON(role string) string {
return fmt.Sprintf(`
resource "aws_iam_role" "role" {
name = "tf_test_role_%s"
path = "/"
assume_role_policy = <<EOF
{
"Version": "2012-10-17",
"Statement": [
{
"Action": "sts:AssumeRole",
"Principal": {
"Service": "ec2.amazonaws.com"
},
"Effect": "Allow",
"Sid": ""
}
]
}
EOF
}

resource "aws_iam_role_policy" "foo" {
name = "tf_test_policy_%s"
role = "${aws_iam_role.role.name}"
policy = <<EOF
{
"Version": "2012-10-17",
"Statement": {
"Effect": "Allow",
"Action": "*",
"Resource": "*"
}
}
EOF
}
`, role, role)
}
6 changes: 5 additions & 1 deletion builtin/providers/aws/structure.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1886,7 +1886,11 @@ func normalizeJsonString(jsonString interface{}) (string, error) {
return s, err
}

bytes, _ := json.Marshal(j)
bytes, err := json.Marshal(j)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 wasn't this here to allow empty policies to passthrough? e.g. when it's interpolated from a variable and the only way to make it empty is to pass an empty string (not to omit the argument).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, possibly. Noticed we didn't catch the error returned there, and thought we should. I'll add a comment that it's on purpose.

if err != nil {
return s, err
}

return string(bytes[:]), nil
}

Expand Down
17 changes: 17 additions & 0 deletions builtin/providers/aws/validators.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -605,6 +605,23 @@ func validateJsonString(v interface{}, k string) (ws []string, errors []error) {
return
}

func validateIAMPolicyJson(v interface{}, k string) (ws []string, errors []error) {
// IAM Policy documents need to be valid JSON, and pass legacy parsing
value := v.(string)
if len(value) < 1 {
errors = append(errors, fmt.Errorf("%q contains an invalid JSON policy", k))
return
}
if value[:1] != "{" {
errors = append(errors, fmt.Errorf("%q conatains an invalid JSON policy", k))
return
}
if _, err := normalizeJsonString(v); err != nil {
errors = append(errors, fmt.Errorf("%q contains an invalid JSON: %s", k, err))
}
return
}

func validateCloudFormationTemplate(v interface{}, k string) (ws []string, errors []error) {
if looksLikeJsonString(v) {
if _, err := normalizeJsonString(v); err != nil {
Expand Down
59 changes: 59 additions & 0 deletions builtin/providers/aws/validators_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -799,6 +799,65 @@ func TestValidateJsonString(t *testing.T) {
}
}

func TestValidateIAMPolicyJsonString(t *testing.T) {
type testCases struct {
Value string
ErrCount int
}

invalidCases := []testCases{
{
Value: `{0:"1"}`,
ErrCount: 1,
},
{
Value: `{'abc':1}`,
ErrCount: 1,
},
{
Value: `{"def":}`,
ErrCount: 1,
},
{
Value: `{"xyz":[}}`,
ErrCount: 1,
},
{
Value: ``,
ErrCount: 1,
},
{
Value: ` {"xyz": "foo"}`,
ErrCount: 1,
},
}

for _, tc := range invalidCases {
_, errors := validateIAMPolicyJson(tc.Value, "json")
if len(errors) != tc.ErrCount {
t.Fatalf("Expected %q to trigger a validation error.", tc.Value)
}
}

validCases := []testCases{
{
Value: `{}`,
ErrCount: 0,
},
{
Value: `{"abc":["1","2"]}`,
ErrCount: 0,
},
}

for _, tc := range validCases {
_, errors := validateIAMPolicyJson(tc.Value, "json")
if len(errors) != tc.ErrCount {
t.Fatalf("Expected %q not to trigger a validation error.", tc.Value)
}
}
}

func TestValidateCloudFormationTemplate(t *testing.T) {
type testCases struct {
Value string
Expand Down