terraform-aws-modules / terraform-aws-efs

Terraform module to create AWS EFS resources 🇺🇦
https://registry.terraform.io/modules/terraform-aws-modules/efs/aws
Apache License 2.0
23 stars 38 forks source link

deny_nonsecure_transport grants read-write access to all principals #30

Open davejbax opened 3 months ago

davejbax commented 3 months ago

Description

The policy generated by deny_nonsecure_transport grants access to all AWS principals. This makes the use of IAM to control access to the filesystem impossible when this boolean is set, and is an extremely significant side-effect of the boolean (in contrast to having it set to false and using policy_statements) that is not clear in documentation.

The problematic policy was added in https://github.com/terraform-aws-modules/terraform-aws-efs/pull/21, in an attempt to fix #20 and #11. In particular, I think the policy given in #20 is the incorrect policy to fix the ECS issue because it only works because it grants access to all principals -- which is far too broad of a policy, and probably not the intention.

20 mentions that the web console generated the policy. However, I believe the policies generated by the web console are intended to be used where IAM is not used to control access to the filesystem: all of them generate a similar policy granting access to all principals, with specific denies; this is because the 'default' EFS policy is to allow access to all principals, and use firewall rules to control access.

I think this module should support using IAM to selectively control access to the EFS filesystem, instead of firewall rules alone. At the very least, it should be made more explicit that deny_nonsecure_transport precludes the use of IAM. I would suggest creating a new boolean that makes it very explicit as to whether a 'allow all principals' policy will be attached; then, this could be set to false to facilitate the use of IAM to control access.

Versions

Reproduction Code [Required]

module "efs" {
  source = "terraform-aws-modules/efs/aws"

  # File system
  name           = "example"
  creation_token = "example-token"
  encrypted      = true
  kms_key_arn    = "arn:aws:kms:eu-west-1:111122223333:key/1234abcd-12ab-34cd-56ef-1234567890ab"

  lifecycle_policy = {
    transition_to_ia = "AFTER_30_DAYS"
  }

  # File system policy
  attach_policy                      = true
  bypass_policy_lockout_safety_check = false
  deny_nonsecure_transport = true
  policy_statements = [
  # XXX: This policy has no effect, because of the 'grant all' policy added by deny_nonsecure_transport!
    {
      sid     = "Example"
      actions = ["elasticfilesystem:ClientMount"]
      principals = [
        {
          type        = "AWS"
          identifiers = ["arn:aws:iam::111122223333:role/EfsReadOnly"]
        }
      ]
    }
  ]

  # Mount targets / security group
  mount_targets = {
    "eu-west-1a" = {
      subnet_id = "subnet-abcde012"
    }
    "eu-west-1b" = {
      subnet_id = "subnet-bcde012a"
    }
    "eu-west-1c" = {
      subnet_id = "subnet-fghi345a"
    }
  }
  security_group_description = "Example EFS security group"
  security_group_vpc_id      = "vpc-1234556abcdef"
  security_group_rules = {
    vpc = {
      # relying on the defaults provdied for EFS/NFS (2049/TCP + ingress)
      description = "NFS ingress from VPC private subnets"
      cidr_blocks = ["10.99.3.0/24", "10.99.4.0/24", "10.99.5.0/24"]
    }
  }
}

Steps to reproduce the behavior:

  1. Create an EFS filesystem using the module that:
    • Has deny_nonsecure_transport set to true
    • Uses policy_statements to attempt to grant some form of access to a specific IAM principal
  2. Attempt to mount the EFS filesystem with an IAM principal that you did not grant explicit access to
  3. Notice that the EFS filesystem is mounted successfully

Expected behavior

The EFS filesystem should not be able to be mounted by any IAM principal when deny_nonsecure_transport is true

Actual behavior

The EFS filesystem is able to be mounted by any IAM principal when deny_nonsecure_transport is true, regardless of any allows in policy_statements.

lorengordon commented 2 months ago

I'd add that, checking EFS docs, the statement in question has nothing to do with nonsecure transport, and so gating it with deny_nonsecure_transport is rather misleading. The statement is instead just one approach AWS suggests to consider the file system non-public...

github-actions[bot] commented 1 month ago

This issue has been automatically marked as stale because it has been open 30 days with no activity. Remove stale label or comment or this issue will be closed in 10 days

davejbax commented 1 month ago

Not stale

github-actions[bot] commented 3 weeks ago

This issue has been automatically marked as stale because it has been open 30 days with no activity. Remove stale label or comment or this issue will be closed in 10 days

lorengordon commented 3 weeks ago

Still not stale