hashicorp / terraform-provider-aws

The AWS Provider enables Terraform to manage AWS resources.
https://registry.terraform.io/providers/hashicorp/aws
Mozilla Public License 2.0
9.77k stars 9.12k forks source link

Feature request: Exclusive policy attachment list for users, groups, and roles #4426

Open jbscare opened 6 years ago

jbscare commented 6 years ago

Terraform Version

+$ terraform -v
Terraform v0.11.7
+ provider.aws v1.16.0

Affected Resource(s)

Expected Behavior

This is a feature request for either (a) an enhancement to aws_iam_user_policy_attachment, aws_iam_group_policy_attachment, and aws_iam_role_policy_attachment; or (b) to create a new trio of resources (perhaps aws_iam_user_policy_list, aws_iam_group_policy_list, and aws_iam_role_policy_list?), to allow for the specification of an exclusive list of policies that should be attached to a group or role, much like aws_iam_group_membership allows for the specification of an exclusive list of users who should be members of a group. If any policies are found attached to the group or role that aren't on the list, they should be removed.

Actual Behavior

There doesn't seem to be a way to accomplish this effect at this time. (You can get sort of a converse of this effect with aws_iam_policy_attachment, which specifies an exclusive list of roles and users and groups to which a policy is attached, but doesn't exclude other policies from being attached to a role, user, or group.)

Use case

We'd like to use Terraform to fully specify the policies that should be attached to a user or group or role, and if someone adds a policy outside of Terraform, for Terraform to detect and correct that.

lorengordon commented 6 years ago

Following. I thought terraform already did this. Yikes. Major security hole here.

FernandoMiguel commented 6 years ago

@lorengordon it's by design. No security risk. If you allow anyone to modify your account, no security can save you

lorengordon commented 6 years ago

It can certainly be one thing to you and another to me and our auditors. Call it whatever you like. We call it a security risk.

We want to use terraform to manage infrastructure as code. We create the IAM role with terraform and we create the IAM policies with terraform. But it turns out we cannot actually guarantee an exact configuration of a role using terraform, since any policies added to the role outside of terraform are basically invisible to terraform right now. That's pretty bad in our book. We'll need to use something else to manage IAM roles until this is fixed.

I see it as similar functionality as what terraform provides with security groups... You can either define the rules as part of the aws_security_group resource, and those rules will remove any other rules, ensuring the exact configuration of the security group. Or you can use the aws_security_group_rule resource to add rules to a security group without removing any externally added rules.

jbscare commented 6 years ago

The only thing I'd quibble with is that using Terraform in this way doesn't create a security hole where there wasn't one before. If you consider users groups and roles that aren't managed by code to be a security hole, then it's true that Terraform doesn't fix that, but Terraform doesn't fix a lot of security holes.

All that said, we of course want this feature, since this seems like a security hole that Terraform could fix. :^) (Or an "issue" that it could "address", if you like.)

YakDriver commented 6 years ago

@jbscare @bflad @lorengordon I'm working on a PR for an exclusive list of role policies (aws_iam_role_policy_list) currently. There are a few design options. I'd like to get your thoughts.

  1. ONLY enforce exclusivity

The new resource would not associate listed policies with the role. It would detach managed or delete inline policies found that weren't listed. If you didn't want the resource to mess with managed policies, you could skip the argument and the role's managed policies would be left unaffected. Same for inline policies.

If you wanted to make sure that no inline and/or managed policies were put/attached to the role, you could include inline_policies = [] or managed_policies = [].

This would provide all the security benefits with no redundant functionality. To associate a policy with a role, you would have to use aws_role_policy_attachment (managed) or aws_role_policy (inline).

resource "aws_iam_role_policy_list" "policy_list" {
  name = "yak_testing_role_policy_list"
  role = "${aws_iam_role.instance_role.name}"

  managed_policies = [
    "${aws_iam_policy.policy_one.arn}",
    "${aws_iam_policy.policy_two.arn}",
  ]

  inline_policies = [
    "${aws_iam_role_policy.inline_test_policy1.name}",
    "${aws_iam_role_policy.inline_test_policy2.name}",
  ]
}
  1. Enforce exclusivity and manage managed policies

The same as the first approach except with the added convenience that you would not need aws_role_policy_attachment to attach managed policies to the role. In other words, the listed managed policies would be attached to the role if they were not already. Inline policies would still be handled as above (inline cannot be managed in the same way because to put an inline policy requires the policy document).

  1. Ultimate management of policies

Redundant functionality but very convenient. You can create and associate managed and inline policies, and an exclusive list, all with the same resource.

resource "aws_iam_role_policy_list" "policy_list" {
  name = "yak_testing_role_policy_list"
  role = "${aws_iam_role.instance_role.name}"

  managed_policy {
  name        = "test_managed_policy"
  path        = "/"
  description = "My test policy"
  policy = <<EOF
{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Action": [
        "ec2:Describe*"
      ],
      "Effect": "Allow",
      "Resource": "*"
    }
  ]
}
EOF
  }

  inline_policy {
  name = "test_inline_policy"
  policy = <<EOF
{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Action": [
        "ec2:Describe*"
      ],
      "Effect": "Allow",
      "Resource": "*"
    }
  ]
}
EOF
  }
}
  1. My view

I think that #2 is probably the best approach but #1 has merit. #3 is the wrong approach IMO.

jbscare commented 6 years ago

I like option 2: I think we want to be able to say "this role/user/group should have exactly this list of policies attached", and Terraform should cause that to happen, whether that means removing policies that are attached but aren't on the list, or adding policies that aren't attached and are on the list.

It seems like option 1 could get you into a weird state where your TF config could define a policy attachment but not have it on the list -- what would Terraform do then?

lorengordon commented 6 years ago

It seems like option 1 could get you into a weird state where your TF config could define a policy attachment but not have it on the list -- what would Terraform do then?

Terraform would first attach it, then detach it! 😂 Don't do that!

Option 2 doesn't resolve that scenario for inline policies... If a user creates and associates the inline policy using aws_role_policy, but does not specify the inline policy in aws_iam_role_policy_list, then terraform would again create the inline policy and then delete it!

Personally, I'm ok with this, as I think it would be pretty apparent what was happening from one apply to the next. User would just need to update their config to match their intent.

I like option 1... I think option 3 could be compelling but only if we can reuse the code from the other modules.

YakDriver commented 6 years ago

whether that means... adding policies that aren't attached and are on the list

That's not a problem for managed policies. The scenario is not possible for inline. The scenarios would be:

Policy type Assoc Listed Outcome
Managed Attached Yes All - No change
Managed Not attached Yes 1 - Error; 2 - Attached; 3 - Attached
Managed Attached No All - Detached
Managed Not attached No All - No change
Inline Added Yes All - No change
Inline Not added Yes 1 & 2 - Error (policy cannot exist); 3 - Created
Inline Added No All - Deleted
Inline Not added No All - No change

Both option 1 and option 2 would do the same thing with inconsistent config (e.g., aws_role_policy defines an inline policy that is not listed in the aws_iam_role_policy_list). But, isn't that the exact point of an exclusive list? If it's not on the list, it can't go to the VIP room.

YakDriver commented 6 years ago

@lorengordon @jbscare @bflad Implementation of the new resource is pretty much ready. (Still working on docs and tests.) But, just to make sure we're on the same page, please consider the following config and outcomes and see if it works for you.

resource "aws_iam_role_policy_list" "rpl" {
  name = "yak_testing_role_policy_list"

  managed_policies = [
    "${aws_iam_policy.policy_one.arn}",
    "${aws_iam_policy.policy_two.arn}",
  ]

  inline_policies = [
    "${aws_iam_role_policy.inline_test_policy.name}",
  ]

  role = "${aws_iam_role.instance_role.name}"
}

resource "aws_iam_role_policy" "inline_test_policy" {
  name = "yak_inline_test_policy"
  role = "${aws_iam_role.instance_role.name}"

  policy = <<EOF
{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Action": [
        "ec2:Describe*"
      ],
      "Effect": "Allow",
      "Resource": "*"
    }
  ]
}
EOF
}

data "aws_iam_policy_document" "instance_assume_role_policy" {
  statement {
    actions = ["sts:AssumeRole"]

    principals {
      type        = "Service"
      identifiers = ["ec2.amazonaws.com"]
    }
  }
}

resource "aws_iam_role" "instance_role" {
  name               = "yak_instance_role"
  path               = "/system/"
  assume_role_policy = "${data.aws_iam_policy_document.instance_assume_role_policy.json}"
}

resource "aws_iam_policy" "policy_one" {
  name        = "yak_test_policy1"
  path        = "/"
  description = "My test policy"

  policy = <<EOF
{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Action": [
        "ec2:Describe*"
      ],
      "Effect": "Allow",
      "Resource": "*"
    }
  ]
}
EOF
}

resource "aws_iam_policy" "policy_two" {
  name        = "yak_test_policy2"
  path        = "/"
  description = "My test policy"

  policy = <<EOF
{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Action": [
        "ec2:Describe*"
      ],
      "Effect": "Allow",
      "Resource": "*"
    }
  ]
}
EOF
}

Assuming, this chain of events, the table lists the outcomes.

  1. Terraform created everything using the config above
  2. Some non-Terraform action takes place
  3. terraform apply is performed again
  4. Outcome as shown below
Non-TF Action On apply
Managed policy detached Policy reattached :ballot_box_with_check:
Managed policy deleted Policy recreated, attached :ballot_box_with_check:
Extra managed policy attached Policy detached :ballot_box_with_check:
Extra inline policy added Policy deleted :ballot_box_with_check:
Inline policy modified Policy reverted to original :ballot_box_with_check:
No inline policy list, inline policy added No changes :ballot_box_with_check:
No managed policy list, managed policy attached No changes :ballot_box_with_check:
Empty inline policy list, inline policy added Inline policy deleted :ballot_box_with_check:
Empty managed policy list, managed policy attached Managed policy detached :ballot_box_with_check:

Let me know if you'd like more tests or different outcomes.

jbscare commented 6 years ago

Just to clarify, in that table in the previous comment, the "Action" column describes what would happen if you did a terraform apply after someone took those actions via some out-of-band non-Terraform method (like the AWS web UI), right?

If so, that all sounds good to me!

One thought: I'm not sure about the aws_iam_role_policy_list name; it's not intuitively clear from the name how this relates to aws_iam_policy_attachment. It's sort of cumbersome, but would aws_iam_exclusive_policy_attachment_list be clearer? Any other ideas?

Also, is this only for roles? Could it be extended to cover users and groups as well?

Thanks for all your work on this!

YakDriver commented 6 years ago

I'll clean up the table to make it a bit clearer.

aws_iam_role_policy_list has 24 characters. What about aws_iam_role_policy_sole_list (29), aws_iam_role_policy_exclusive_list (34), or aws_iam_role_policy_manager (27)?

Right now I'm just working roles. With some lessons learned with roles (with the implementation and merge process), we may have a chance to do users and groups as well.

YakDriver commented 6 years ago

@jbscare @lorengordon Feel free to thumbs up the PR if you have a chance.

YakDriver commented 6 years ago

The acceptance tests for PR #5904 cover the tests in the comment above.

bflad commented 6 years ago

Thanks for all your work here so far. From my design comment on #5904, I would like to introduce some talking points.

Adding more IAM resources (times 3 for group/role/policy) doesn't seem necessary and I think will further add to the confusing landscape that is AWS IAM management in Terraform. Is there a reason to split away this information from the original resource configurations (aws_iam_group/aws_iam_role/aws_iam_user)?

Starting with managed policies, we can introduce a policy_attachments/managed_policies/managed_policy_attachments/managed_policy_arns TypeSet attribute on the existing (e.g. aws_iam_role) resources to enable this type of configuration:

# Design sketch - not currently implemented and details may change during development
resource "aws_iam_role" "example" {
  # ... other configuration ...

  managed_policy_arns = [
    "${aws_iam_policy.example.arn}",
    "arn:aws:iam::aws:policy/AdministratorAccess",
  ]
}

In this model, the aws_iam_role resource knows everything it needs to about maintaining a set list of managed policy attachments to add/update/remove them. The same caveat applies that this configuration conflicts with any aws_iam_role_policy_attachment resources.

Inline policies are slightly trickier because they have a name associated with them, but there are a few potential solutions there, with a TypeMap potentially being the best in my opinion (the other being the introduction of another data source specifically for inline policies, but the additional complexity seems unnecessary). Drawing out how this might look:

# Design sketch - not currently implemented and details may change during development
resource "aws_iam_role" "example" {
  # ... other configuration ...

  inline_policies = {
    policy1 = "${data.aws_iam_policy_document.example.json}"
    policy2 = <<POLICY
{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Action": [
        "ec2:Describe*"
      ],
      "Effect": "Allow",
      "Resource": "*"
    }
  ]
}
POLICY
  }
}

In this model, the aws_iam_role resource knows everything it needs to about maintaining a set list of inline policies (based on name) to add/update/remove them. The same caveat applies that this configuration conflicts with any aws_iam_role_policy resources.

What do you all think?

Aside (and a very personal opinion): I think its extremely important to not include statements in documentation saying this type of functionality "enhances" security. While it does overall improve your security posture by detecting drift from a desired configuration, it does nothing to prevent security issues, is not designed to be a security alerting tool, and only provides point in time remediation (per run).

lorengordon commented 6 years ago

Seems reasonable to me. Could perhaps use the list of maps pattern as well, seems pretty common throughout Terraform. I don't think I have a preference between the two...

managed policies:

# Design sketch - not currently implemented and details may change during development
resource "aws_iam_role" "example" {
  # ... other configuration ...

  managed_policy {
    arn = "${aws_iam_policy.example.arn}"
  }

  managed_policy {
    arn = "arn:aws:iam::aws:policy/AdministratorAccess",
  }
}

inline policies:

# Design sketch - not currently implemented and details may change during development
resource "aws_iam_role" "example" {
  # ... other configuration ...

  inline_policy {
    name = "policy1"
    policy = "${data.aws_iam_policy_document.example.json}"
  }

  inline_policy {
    name = "policy2"
    policy = <<POLICY
{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Action": [
        "ec2:Describe*"
      ],
      "Effect": "Allow",
      "Resource": "*"
    }
  ]
}
POLICY
  }
}
YakDriver commented 6 years ago

I dunno. You're trading one type of mess for another:

  1. (confusing landscape) having an extra IAM resource that's clean and not importable
  2. (complex import) adding inline functionality to an already full resource that is importable
jbscare commented 6 years ago

I don't have a strong opinion either way, other than to note that this seems somewhat analogous to the situation with aws_security_group and aws_security_group_rule, and if it's going to behave in a similar way, it might make sense for its configuration to be similar.

YakDriver commented 6 years ago

After delving into the code more, aws_iam_role is a very clean resource code-wise so I think adding the functionality there is going to work fine. I hear that this approach will make more sense to users.

YakDriver commented 6 years ago

Moved the implementation question to the PR #5904.

github-actions[bot] commented 4 years ago

Marking this issue as stale due to inactivity. This helps our maintainers find and focus on the active issues. If this issue receives no comments in the next 30 days it will automatically be closed. Maintainers can also remove the stale label.

If this issue was automatically closed and you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thank you!

lorengordon commented 4 years ago

Please don't close this, it would be a very important feature for maintaining strict control over iam roles (and users and groups).

soumyajk commented 3 years ago

Hi, This is a very valid problem(surprised to see the request in stale state). Its very annoying to have additional policies attached against iam roles which are created through terraform, which the terraform is completely unaware of. I would have expected terraform to show it as a drift, detach the managed policy if not present in terraform.

This also makes it impossible to catch the drift, which stops the role from being deleted if force_detach_policies value is set true.

I completely agree with the model shared in comment : https://github.com/hashicorp/terraform-provider-aws/issues/4426#issuecomment-421183069

I would appreciate if someone can share the latest update on this bug.

Thanks

lorengordon commented 3 years ago

If anyone subscribed here still really cares about this feature like I do, here are few new issues to please go give a thumbs up, plus the pr that implements exclusive attachment for IAM roles...

AnthonyFoiani-at commented 2 years ago

Hello! It took me a quite a while to figure out what aws_iam_role.managed_policy_arns meant, and why it was added.

It looks like I'm not the only person that was confused: https://discuss.hashicorp.com/t/confusion-about-aws-iam-role-managed-policy-arns-attribute/29728

Could the documentation be expanded / reworded? In particular, the usage of "exclusive" made me think that "oh, this role is the only user of this policy", not "this role is the only thing that controls which policies are attached to itself".

Maybe something along the lines of:

If you specify a list of managed_policy_arns, Terraform apply will attempt to enforce that exactly and only those policies are attached to this role; any out-of-band changes or separate attachment resources will be disregarded and/or reverted (and/or cause resource cycling).

Thank you!

lorengordon commented 2 years ago

@AnthonyFoiani-at I do not believe that is accurate. When you specify this:

resource "aws_iam_role" "sensitive" {
  ...
  # Enforce that we want exactly and only these policies attached to this role:
  managed_policy_arns = [
    aws_iam_policy.sensitive_stuff,
    aws_iam_policy.other_sensitive_stuff,
  ]
}

The aws_iam_role resource fully manages the attachments. You do not need and should not use aws_iam_role_policy_attachment for that role at all.

AnthonyFoiani-at commented 2 years ago

The aws_iam_role resource fully manages the attachments. You do not need and should not use aws_iam_role_policy_attachment for that role at all.

Thank you for the correction -- I will do some experiments!

Hopefully it's still a constructive message that the documentation is confusing?

AnthonyFoiani-at commented 2 years ago

@lorengordon wrote:

The aws_iam_role resource fully manages the attachments. You do not need and should not use aws_iam_role_policy_attachment for that role at all.

Verified! Thank you for the correction.

lorengordon commented 2 years ago

Nice. I think part of understanding the docs comes with the term "exclusive" and what terraform means by that. And then the tri-modal nature of assigning a value.

A few resources have this concept of what I call a "container" resource and an attachment. Like IAM Roles (container) and policies (attachment), and security groups (container) and rules (attachment). "Exclusive" is in relation to how terraform manages the attachments on the container resource.

When you use an attribute on the "container" resource that manages attachments, it is an "exclusive" relationship. I.e. Exactly and only these attachments should be present, and any other attachments other than what is specified will be removed.

When you use a separate attachment resource, it is non-exclusive. Meaning you could have one terraform state that manages the container resource (without specifying the attachment attribute), and then you could have any number of other terraform states managing attachments to that container resource. Or really, the attachments could be created any way at all, including manually in the console or cli. When running terraform on the state that manages the container resource, it will just ignore the attachments.

So, that kinda gets at the tri-modal nature also... Here is what I mean, see the comment on each example:

resource "aws_iam_role" "sensitive" {
  ...
  # Enforce that we want exactly and only these policies attached to this role:
  managed_policy_arns = [
    aws_iam_policy.sensitive_stuff,
    aws_iam_policy.other_sensitive_stuff,
  ]
}
resource "aws_iam_role" "sensitive" {
  ...
  # Enforce that there must be exactly 0 attached policies:
  managed_policy_arns = []
}
resource "aws_iam_role" "sensitive" {
  ...
  # Ignore all attachments, neither add nor remove them
  # managed_policy_arns = null
}
AnthonyFoiani-at commented 2 years ago

@lorengordon Those examples are great! "Tri-modal" isn't a word I hear very often. :-)

And now that I [am pretty sure! I] understand what's going on, the existing wording in the docs makes sense. That's a tough puzzle to crack; hopefully my naive comments above might help us find a starting point.

The only change I'd suggest is to the last one:

resource "aws_iam_role" "sensitive" {
  ...
  # This resource will not manipulate associated policy attachments on its own; you will
  # need to attach policies through either `aws_iam_role_policy_attaachment` resources,
  # or manually via the console / API / CLI.
  managed_policy_arns = null
}

Thank you all again, and have a great weekend!