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.

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

Remove principal type AWS from IAM policy wildcard normalization #4248

Merged
merged 4 commits into from
May 11, 2018

Conversation

spirius
Copy link
Contributor

@spirius spirius commented Apr 18, 2018

The normalized IAM policies for wildcard principals are not accepted by IAM when used as trust policy for roles. According to AWS IAM doc it should be fine, but in practice IAM returns MalformedPolicyDocument: AssumeRolepolicy contained an invalid principal: "STAR":"*" error.

Basically following terraform code doesn't work, because it normalizes principal to "Principal": "*", and it is not possible to get {"Principal": {"AWS": *}} in rendered policy when using aws_iam_policy_document data source

data "aws_iam_policy_document" "policy" {
  statement {
    effect = "Deny"
    actions = ["sts:AssumeRole"]
    principals {
      type = "AWS"
      identifiers = ["*"]
    }
  }
}

resource "aws_iam_role" "role" {
  name = "test_role"
  assume_role_policy = "${data.aws_iam_policy_document.policy.json}"
}

Without normalization it works as expected:

resource "aws_iam_role" "role" {
  name = "test_role"
  assume_role_policy = <<EOS
{
  "Statement": [
    {
      "Effect": "Deny",
      "Action": "sts:AssumeRole",
      "Principal": {
        "AWS": "*"
      }
    }
  ]
}
EOS
}

The PR removes principal type "AWS" from normalization.

@ghost ghost added the size/S Managed by automation to categorize the size of a PR. label Apr 18, 2018
@spirius
Copy link
Contributor Author

spirius commented Apr 18, 2018

Test results

$ TF_ACC=1 go test -v -run=TestAccAWSDataSourceIAMPolicyDocument_* -timeout 120m
=== RUN   TestAccAWSDataSourceIAMPolicyDocument_basic
--- PASS: TestAccAWSDataSourceIAMPolicyDocument_basic (18.33s)
=== RUN   TestAccAWSDataSourceIAMPolicyDocument_source
--- PASS: TestAccAWSDataSourceIAMPolicyDocument_source (27.55s)
=== RUN   TestAccAWSDataSourceIAMPolicyDocument_sourceConflicting
--- PASS: TestAccAWSDataSourceIAMPolicyDocument_sourceConflicting (17.95s)
=== RUN   TestAccAWSDataSourceIAMPolicyDocument_override
--- PASS: TestAccAWSDataSourceIAMPolicyDocument_override (18.05s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	81.920s

@bflad bflad added the service/iam Issues and PRs that pertain to the iam service. label Apr 20, 2018
@bflad
Copy link
Contributor

bflad commented Apr 20, 2018

I think this is a good idea, but we should add some additional user-facing documentation surrounding the various principal behaviors on the aws_iam_policy_document data source page. Can you include some additional verbiage in the principal(/type) section and/or bottom examples section please? We should call out the support for type = "*" explicitly at least.

MAINTAINER NOTE: This will require a NOTE callout in the CHANGELOG, preferably linking to the above documentation enhancements

@bflad bflad added enhancement Requests to existing resources that expand the functionality or scope. waiting-response Maintainers are waiting on response from community or contributor. labels Apr 20, 2018
@ghost ghost added the size/S Managed by automation to categorize the size of a PR. label May 11, 2018
@spirius
Copy link
Contributor Author

spirius commented May 11, 2018

@bflad, added new section in docs about wildcard principals

@bflad bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label May 11, 2018
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Thanks @spirius! I think this is a good change to get in as it is technically more correct/expected behavior. I will write up a note in the CHANGELOG regarding this change as its bound to cause some initial confusion for those depending on the old behavior.

@bflad bflad added this to the v1.19.0 milestone May 11, 2018
@bflad bflad merged commit d8d67df into hashicorp:master May 11, 2018
bflad added a commit that referenced this pull request May 11, 2018
@spirius spirius deleted the feature/iam-wildcard-pricipal branch May 15, 2018 14:09
@bflad
Copy link
Contributor

bflad commented May 17, 2018

This has been released in version 1.19.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

@ghost
Copy link

ghost commented Apr 6, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Apr 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Requests to existing resources that expand the functionality or scope. service/iam Issues and PRs that pertain to the iam service. size/S Managed by automation to categorize the size of a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants