feat: Adding vpc_flow_log_permissions_boundary

[kelseymok]

Description

Adding vpc_flow_log_permissions_boundary so that developers who are required to specify a permissions boundary on IAM roles can enable the VPC Flow Log feature in this module

Motivation and Context

In many AWS setups, developers are required to assume a role in order to make changes to AWS resources. One of the recommended security measures that is recommended by AWS is to allow roles the ability to create other roles only if a permissions boundary is set directly on the role. This prevents developers (in an assumed-role session) from [unintentionally/intentionally] creating a role with, for example, super powers/administrative rights. Without the ability to set a permissions boundary on the VPC Flow Logs IAM role, those who are bound by this permissions boundary restraint will not be able to enable the VPC Flow Logs feature, but will instead need to create a new module to handle this.

Breaking Changes

None

How Has This Been Tested?

CircleCI pipeline passes. Using the fork in our production code/environment (manual smoke testing/verification) -- the VPC Flow Logs elements (IAM role, IAM cloudwatch log group) are created; other elements are not affected.

module "vpc" {
  source = "git::https://github.com/kelseymok/terraform-aws-vpc.git"

  name = "${local.project-name}-${local.module-name}"
  cidr = local.vpc-cidr

  azs = local.azs
  private_subnets = local.private_subnets
  public_subnets  = local.public_subnets

  enable_nat_gateway = true
  enable_dns_hostnames = true
  enable_dns_support   = true

  enable_flow_log = true
  flow_log_destination_type = "cloud-watch-logs"
  create_flow_log_cloudwatch_iam_role = true
  create_flow_log_cloudwatch_log_group = true
  vpc_flow_log_permissions_boundary = "arn:aws:iam::${data.aws_caller_identity.current.account_id}:policy/my-awesome-boundary"
}

[DrFaust92]

Hey, if we are introducing permission boundaries for VPC flow logs role i think we should consider having a default policy thats based on the role policy we have (the same one can be reused?) instead of an empty value. having said that the empty string does not introduce a change to exiting resources but i would opt to have the addition as it makes the role more "secure"

@bryantbiggs thoughts?

[bryantbiggs]

ya that sounds reasonable. whichever route we go, we should be consistent. I made this comment the other day #497 (comment) but again, I can also get behind setting a default policy rather than an empty value

[DrFaust92]

Good point, but in this case unlike the Endpoint policy i think it makes more sense to go with a specific policy as its a role created for a specific purpose. its hard to think of a default policy for endpoints (probably also different for each type that supports it) i would go with defaulting to null of endpoints but having something stricter here as there is a logic for a naive policy here where there isnt one for endpoints otherwise.

[kelseymok]

Fair point @DrFaust92 @bryantbiggs! I will have a look and add some commits for your review.

[kelseymok]

@DrFaust92 @bryantbiggs I've gone ahead and added a default of null to the new variable per the request and example above.

However, on the topic of a default policy for the permissions boundary (if I understand the conversation above), it would be more native to actually NOT include a default policy for the permissions boundary based on the role that already exists for the reasons that:

1. The operating role running terraform apply might not have the ability to change the permissions boundary. If people are using permissions boundary, it's probably because the policy has been given to them by some sort of AWS account administrator and if their administrator has set up the permissions boundary correctly (and securely), their operation role (the one that runs terraform apply) should NOT be able to modify the permissions boundary.
2. Updating the permissions boundary should happen asynchronously. If the original permissions boundary does not allow their role to do IAM actions required by a VPC Flow Logs role, it was probably by design or even more likely, the permissions boundary policy needs to be updated (again, the person who is running this code likely will not have the permissions to update the permissions boundary - the administrator should do this). Updating the policy of the permissions boundary should happen asynchronously.
3. Updating the existing permissions boundary might not be possible outside of the resource. I am a deep believer that security is in the heart and soul of the human running the terraform apply but the reality is that when we have an AWS setup at an enterprise level, we do have separate admin roles (who manage permissions boundaries) and maybe something like developer roles that can run the terraform apply. Should the human runner of terraform apply be able to update both, then it could be possible to update the permissions boundary to include the VPC flow logs as part of this module... if the terraform IAM policy resource allows defining more than one policy document via async attachment... which I don't think is possible at least according to my reading of the documentation.

To summarise, adding a default permissions boundary is not something that feels natural to the way that users would be using a permissions boundary, and it likely isn't something that they would have permissions to change.

My recommendation here would be to leave this default as null as it's the least common denominator and allows for flexibility of various IAM strategies in both simple and complex enterprise setups. I've proactively added a section in the README about permissions boundaries, linking to the original AWS documentation that specifies the IAM actions that are required.

Thoughts?

[bryantbiggs]

I think this looks like a good route - @DrFaust92 / @antonbabenko thoughts?

[antonbabenko]

Looks good to me but there are a couple of small corrections in the readme still missing. I will be able to merge this next week.

[antonbabenko]

Thanks for this addition! I have updated (made much shorter) the README. Hopefully, it is ok :)

v2.65.0 has been just released.

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.