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

Use IAM policy documents separately instead of a single template document to manage policies on bucket #20

Merged
merged 10 commits into from Jun 11, 2019

Conversation

@chrisgilmerproj
Copy link
Contributor

commented May 31, 2019

This resolves #17 (troubles with allow_s3) and also helps with #7 (migrating policies to data sources).

The template is too difficult to manage and the actions of allow_s3 don't seem to do what was intended. Here are the changes:

  • allow_s3 removed and default_allow no longer manages the acl. Instead a s3_bucket_acl manages the acl which is a lot easier to understand.
  • the bucket policy relies on terraform data "aws_iam_policy_document". This does some validation on our behalf for each policy and control over if a policy is in effect is done via the effect as either Allow or Deny.
  • There is a dependency between the policy document and the public access block so that creating or destroying doesn't have any conflicts over modifying the s3 bucket resource.
  • added more examples to the docs.

Since this introduces a breaking change I'd like to release this as v3.0.0 when it gets approved as opposed to v2.2.0.

@chrisgilmerproj chrisgilmerproj requested a review from pjdufour-truss May 31, 2019

@chrisgilmerproj chrisgilmerproj self-assigned this May 31, 2019

@chrisgilmerproj chrisgilmerproj force-pushed the cg_use_iam_policy_docs branch 4 times, most recently from 35026be to 0ec5319 May 31, 2019

@chrisgilmerproj chrisgilmerproj marked this pull request as ready for review May 31, 2019

@chrisgilmerproj chrisgilmerproj force-pushed the cg_use_iam_policy_docs branch from a4a2470 to 3c1b5b1 Jun 4, 2019

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

S3 buckets only allow one policy document. So I've decided to take the approach of using Allow or Deny with a ternary to explicitly enable or disable policies. Here's an example module:

module "logs_test" {

  source  = "trussworks/logs/aws"
  version = "~> 2.1.0"

  default_allow    = false
  allow_s3         = true
  allow_cloudtrail = true
  allow_cloudwatch = true
  allow_config     = true
  allow_alb        = true

  s3_log_bucket_retention = "${local.log_retention_days}"
  region                  = "${local.region}"
  s3_bucket_name          = "${local.aws_logs_bucket}-test"
}

With the output of that choice for the S3 bucket:

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Sid": "cloudtrail-logs-get-bucket-acl",
            "Effect": "Allow",
            "Principal": {
                "Service": "cloudtrail.amazonaws.com"
            },
            "Action": "s3:GetBucketAcl",
            "Resource": "arn:aws:s3:::transcom-ppp-aws-logs-test"
        },
        {
            "Sid": "cloudtrail-logs-put-object",
            "Effect": "Allow",
            "Principal": {
                "Service": "cloudtrail.amazonaws.com"
            },
            "Action": "s3:PutObject",
            "Resource": "arn:aws:s3:::transcom-ppp-aws-logs-test/cloudtrail/*",
            "Condition": {
                "StringEquals": {
                    "s3:x-amz-acl": "bucket-owner-full-control"
                }
            }
        },
        {
            "Sid": "cloudwatch-logs-get-bucket-acl",
            "Effect": "Allow",
            "Principal": {
                "Service": "logs.us-west-2.amazonaws.com"
            },
            "Action": "s3:GetBucketAcl",
            "Resource": "arn:aws:s3:::transcom-ppp-aws-logs-test"
        },
        {
            "Sid": "cloudwatch-logs-put-object",
            "Effect": "Allow",
            "Principal": {
                "Service": "logs.us-west-2.amazonaws.com"
            },
            "Action": "s3:PutObject",
            "Resource": "arn:aws:s3:::transcom-ppp-aws-logs-test/cloudwatch/*",
            "Condition": {
                "StringEquals": {
                    "s3:x-amz-acl": "bucket-owner-full-control"
                }
            }
        },
        {
            "Sid": "config-permissions-check",
            "Effect": "Allow",
            "Principal": {
                "Service": "config.amazonaws.com"
            },
            "Action": "s3:GetBucketAcl",
            "Resource": "arn:aws:s3:::transcom-ppp-aws-logs-test"
        },
        {
            "Sid": "config-bucket-delivery",
            "Effect": "Allow",
            "Principal": {
                "Service": "config.amazonaws.com"
            },
            "Action": "s3:PutObject",
            "Resource": "arn:aws:s3:::transcom-ppp-aws-logs-test/config/*",
            "Condition": {
                "StringEquals": {
                    "s3:x-amz-acl": "bucket-owner-full-control"
                }
            }
        },
        {
            "Sid": "elb-logs-put-object",
            "Effect": "Deny",
            "Principal": {
                "AWS": "arn:aws:iam::*:root"
            },
            "Action": "s3:PutObject",
            "Resource": "arn:aws:s3:::transcom-ppp-aws-logs-test/elb/*"
        },
        {
            "Sid": "alb-logs-put-object",
            "Effect": "Allow",
            "Principal": {
                "AWS": "arn:aws:iam::*:root"
            },
            "Action": "s3:PutObject",
            "Resource": "arn:aws:s3:::transcom-ppp-aws-logs-test/alb/*"
        },
        {
            "Sid": "redshift-logs-put-object",
            "Effect": "Deny",
            "Principal": {
                "AWS": "arn:aws:iam::*:user/logs"
            },
            "Action": "s3:PutObject",
            "Resource": "arn:aws:s3:::transcom-ppp-aws-logs-test/redshift/*"
        },
        {
            "Sid": "redshift-logs-get-bucket-acl",
            "Effect": "Deny",
            "Principal": {
                "AWS": "arn:aws:iam::*:user/logs"
            },
            "Action": "s3:GetBucketAcl",
            "Resource": "arn:aws:s3:::transcom-ppp-aws-logs-test"
        }
    ]
}
Show resolved Hide resolved README.md
@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

Per @pjdufour-truss - add a dependency here to ensure that only the policy happens first before the block. Otherwise you get an issue when applying or deleting.

Show resolved Hide resolved main.tf Outdated
Show resolved Hide resolved main.tf Outdated
@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

Alright, the ticket description has been updated since this ticket now includes a breaking change. Here's a new example:

module "logs_test" {

  source  = "trussworks/logs/aws"
  version = "~> 3.0.0"

  default_allow    = false
  allow_cloudtrail = true
  allow_cloudwatch = true
  allow_config     = true
  allow_alb        = true

  s3_log_bucket_retention = "${local.log_retention_days}"
  region                  = "${local.region}"
  s3_bucket_name          = "${local.aws_logs_bucket}-test"
}

@chrisgilmerproj chrisgilmerproj force-pushed the cg_use_iam_policy_docs branch from 83ea58b to 0740fd5 Jun 6, 2019

@chrisgilmerproj chrisgilmerproj requested a review from pjdufour-truss Jun 6, 2019

@sojeri

sojeri approved these changes Jun 6, 2019

Copy link
Contributor

left a comment

looks good to me. not sure if you want to wait for @pjdufour-truss's feedback, too, since he did your original review.

@@ -12,7 +12,7 @@
* * [RedShift](https://aws.amazon.com/redshift/)

This comment has been minimized.

Copy link
@sojeri

sojeri Jun 6, 2019

Contributor

Since this is already a breaking change, I recommend adding something to the top of these doc comments to explicitly call out whether this module is compatible with Terraform 0.11 or 0.12.

This comment has been minimized.

Copy link
@brainsik

brainsik Jun 7, 2019

Member

Great idea. I just checked and after #23 gets merged in this should be compatible with both.

This comment has been minimized.

Copy link
@sojeri

sojeri Jun 7, 2019

Contributor

approved that ;)

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jun 7, 2019

Author Contributor

Excellent I'll wait to tag until we get both in.

@brainsik
Copy link
Member

left a comment

This is fantastic!!! The only thing I think is missing here is something extra for the README about upgrading from v2.x to v3.x so folks can quickly know what to do in order to adopt this.

@chrisgilmerproj chrisgilmerproj force-pushed the cg_use_iam_policy_docs branch from 0740fd5 to 166ab16 Jun 7, 2019

@pjdufour-truss
Copy link
Contributor

left a comment

I think we can remove cloudtrail_cloudwatch_logs_group variable right?

@chrisgilmerproj chrisgilmerproj requested a review from pjdufour-truss Jun 10, 2019

@chrisgilmerproj chrisgilmerproj merged commit 1519fff into master Jun 11, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@chrisgilmerproj chrisgilmerproj deleted the cg_use_iam_policy_docs branch Jun 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.