search

awslabs / landing-zone-accelerator-on-aws

Deploy a multi-account cloud foundation to support highly-regulated workloads and complex compliance requirements.
https://aws.amazon.com/solutions/implementations/landing-zone-accelerator-on-aws/
Apache License 2.0
557 stars 444 forks source link

Bucket Policy Generated by AWS LZA Incorrectly References LogArchive Account Instead of ELB Service Account #636

Open chummy-opinion opened 6 days ago

chummy-opinion commented 6 days ago

Describe the bug When using the AWS Landing Zone Accelerator (LZA) with an imported S3 bucket specified for ELB access logs in the global-config.yaml file, the generated S3 bucket policy incorrectly sets the Principal for the “Allow write access for ELB Account principal” statement to the Log Archive account ID instead of the Elastic Load Balancing (ELB) service account ID for the region. This misconfiguration results in ELB access logs failing to be delivered to the S3 bucket with an “Access Denied” error.

To Reproduce

  1. Configure the LZA with an imported S3 bucket for ELB access logs in the global-config.yaml file:
logging:
  elbLogBucket:
    importedBucket:
        name: "{{ ElbAccessLogsBucket }}"
        applyAcceleratorManagedBucketPolicy: true
  1. Deploy the LZA.
  2. Observe that the generated bucket policy includes a statement similar to:
{
    "Sid": "Allow write access for ELB Account principal",
    "Effect": "Allow",
    "Principal": {
        "AWS": "arn:aws:iam::112233445566:root"
    },
    "Action": "s3:PutObject",
    "Resource": [
        "arn:aws:s3:::elb-access-logs-bucket-name",
        "arn:aws:s3:::elb-access-logs-bucket-name/*"
    ]
}

Note:

  1. Configure an Application Load Balancer (ALB) in a different account to write access logs to the S3 bucket.
  2. Observe that access logs are not delivered, and the following error message is received:

Access Denied for bucket: elb-access-logs-bucket-name. Please check S3bucket permission

Expected behavior The bucket policy should grant the ELB service account ID for the region (in this case, 783225319266 for ap-southeast-2) permission to write to the S3 bucket, as per AWS documentation. This would allow the ALB to deliver access logs successfully without “Access Denied” errors.

{
    "Sid": "Allow write access for ELB Account principal",
    "Effect": "Allow",
    "Principal": {
        "AWS": "arn:aws:iam::783225319266:root"
    },
    "Action": "s3:PutObject",
    "Resource": [
        "arn:aws:s3:::elb-access-logs-bucket-name",
        "arn:aws:s3:::elb-access-logs-bucket-name/*"
    ]
}

Please complete the following information about the solution:

Additional context When specifying an imported S3 bucket for ELB access logs in the global-config.yaml file with applyAcceleratorManagedBucketPolicy: true, the LZA generates a bucket policy that incorrectly assigns the Log Archive account ID as the Principal for ELB write access. The correct behavior should be to assign the ELB service account ID for the region.

According to the AWS documentation for Application Load Balancer Access Logs, the bucket policy must grant permission to the ELB service account for the specific region.

List of ELB Service Account IDs by Region:

jfan9 commented 5 days ago

Hi @chummy-opinion, this is NOT a bug,

The bucket you used was incorrect. In your Log Archive account's S3 console, there is a bucket name called: aws-accelerator-elb-access-logs-<log-archive-account-id>-<home-region> which you need to use this bucket name in the importedBucket.

Here is the correct global-config.yaml file:

  elbLogBucket:
    importedBucket:
        name: aws-accelerator-elb-access-logs-<log-archive-account-id>-<home-region>
        applyAcceleratorManagedBucketPolicy: true
chummy-opinion commented 4 days ago

Hi @jfan9, this is not correct and appears to miss the point of this bug report - that the S3 Bucket Policy generated by the LZA for imported buckets is incorrect.

The intention of the importedBucket property is to allow you to provide an S3 bucket that you manage and not have the LZA generate one for you. This allows configuration of other properties such as Notifications etc that are not possible using the LZA yaml config files.

What you have listed is the name of the S3 bucket that is generated by the LZA when the importedBucket property is left undefined.

When provided a bucket name, and applyAcceleratorManagedBucketPolicy set to true, the LZA successfully bootstraps the provided bucket except for the error in the S3 Bucket Policy.

Below is the full bucket policy generated by the LZA, note that the real bucket name, account ID and org ID have been redacted.

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Sid": "Allow get acl access for SSM principal",
            "Effect": "Allow",
            "Principal": {
                "Service": "ssm.amazonaws.com"
            },
            "Action": "s3:GetBucketAcl",
            "Resource": "arn:aws:s3:::elb-access-logs-bucket-name"
        },
        {
            "Sid": "Allow write access for ELB Account principal",
            "Effect": "Allow",
            "Principal": {
                "AWS": "arn:aws:iam::112233445566:root"
            },
            "Action": "s3:PutObject",
            "Resource": [
                "arn:aws:s3:::elb-access-logs-bucket-name",
                "arn:aws:s3:::elb-access-logs-bucket-name/*"
            ]
        },
        {
            "Sid": "Allow write access for delivery logging service principal",
            "Effect": "Allow",
            "Principal": {
                "Service": "delivery.logs.amazonaws.com"
            },
            "Action": "s3:PutObject",
            "Resource": "arn:aws:s3:::elb-access-logs-bucket-name/*",
            "Condition": {
                "StringEquals": {
                    "s3:x-amz-acl": "bucket-owner-full-control"
                }
            }
        },
        {
            "Sid": "Allow read bucket ACL access for delivery logging service principal",
            "Effect": "Allow",
            "Principal": {
                "Service": "delivery.logs.amazonaws.com"
            },
            "Action": "s3:GetBucketAcl",
            "Resource": "arn:aws:s3:::elb-access-logs-bucket-name"
        },
        {
            "Sid": "Allow Organization principals to use of the bucket",
            "Effect": "Allow",
            "Principal": {
                "AWS": "*"
            },
            "Action": [
                "s3:GetBucketLocation",
                "s3:PutObject"
            ],
            "Resource": [
                "arn:aws:s3:::elb-access-logs-bucket-name",
                "arn:aws:s3:::elb-access-logs-bucket-name/*"
            ],
            "Condition": {
                "StringEquals": {
                    "aws:PrincipalOrgID": "o-abcd1234"
                }
            }
        }
    ]
}

I should also note that there is a property in the network-config.yaml file that can be set to manually override the ELB account ID, see network-config.yaml

Setting this value to the to the following should override the ELB account ID in the S3 Bucket Policy, but it doesn't.

elbAccountIds:
  - region: ap-southeast-2
    accountId: '783225319266'

I suspect this issue is related to how the bucket policy is generated for an importedBucket vs an LZA created bucket. landing-zone-accelerator-on-aws/source/packages/@aws-accelerator/accelerator/lib/stacks/logging-stack.ts

SatinderSidhu commented 3 days ago

unsubscribe

On Fri, Nov 8, 2024 at 6:20 PM chummy-opinion @.***> wrote:

Hi @jfan9 https://github.com/jfan9, this is not correct and appears to miss the point of this bug report - that the S3 Bucket Policy generated by the LZA for imported buckets is incorrect.

The intention of the importedBucket property is to allow you to provide an S3 bucket that you manage and not have the LZA generate one for you. This allows configuration of other properties such as Notifications etc that are not possible using the LZA yaml config files.

What you have listed is the name of the S3 bucket that is generated by the LZA when the importedBucket property is left undefined.

When provided a bucket name, and applyAcceleratorManagedBucketPolicy set to true, the LZA successfully bootstraps the provided bucket except for the error in the S3 Bucket Policy.

Below is the full bucket policy generated by the LZA, note that the real bucket name, account ID and org ID have been redacted.

{ "Version": "2012-10-17", "Statement": [ { "Sid": "Allow get acl access for SSM principal", "Effect": "Allow", "Principal": { "Service": "ssm.amazonaws.com" }, "Action": "s3:GetBucketAcl", "Resource": "arn:aws:s3:::elb-access-logs-bucket-name" }, { "Sid": "Allow write access for ELB Account principal", "Effect": "Allow", "Principal": { "AWS": "arn:aws:iam::112233445566:root" }, "Action": "s3:PutObject", "Resource": [ "arn:aws:s3:::elb-access-logs-bucket-name", "arn:aws:s3:::elb-access-logs-bucket-name/" ] }, { "Sid": "Allow write access for delivery logging service principal", "Effect": "Allow", "Principal": { "Service": "delivery.logs.amazonaws.com" }, "Action": "s3:PutObject", "Resource": "arn:aws:s3:::elb-access-logs-bucket-name/", "Condition": { "StringEquals": { "s3:x-amz-acl": "bucket-owner-full-control" } } }, { "Sid": "Allow read bucket ACL access for delivery logging service principal", "Effect": "Allow", "Principal": { "Service": "delivery.logs.amazonaws.com" }, "Action": "s3:GetBucketAcl", "Resource": "arn:aws:s3:::elb-access-logs-bucket-name" }, { "Sid": "Allow Organization principals to use of the bucket", "Effect": "Allow", "Principal": { "AWS": "" }, "Action": [ "s3:GetBucketLocation", "s3:PutObject" ], "Resource": [ "arn:aws:s3:::elb-access-logs-bucket-name", "arn:aws:s3:::elb-access-logs-bucket-name/" ], "Condition": { "StringEquals": { "aws:PrincipalOrgID": "o-abcd1234" } } } ] }

I should also note that there is a property in the network-config.yaml file that can be set to manually override the ELB account ID, see network-config.yaml https://awslabs.github.io/landing-zone-accelerator-on-aws/latest/typedocs/latest/interfaces/___packages__aws_accelerator_config_dist_lib_models_network_config.INetworkConfig.html#elbAccountIds

Setting this value to the to the following should override the ELB account ID in the S3 Bucket Policy, but it doesn't.

elbAccountIds:

  • region: ap-southeast-2 accountId: '783225319266'

I suspect this issue is related to how the bucket policy is generated for an importedBucket vs an LZA created bucket.

@.***/accelerator/lib/stacks/logging-stack.ts

https://github.com/awslabs/landing-zone-accelerator-on-aws/blob/main/source/packages/%40aws-accelerator/accelerator/lib/stacks/logging-stack.ts

— Reply to this email directly, view it on GitHub https://github.com/awslabs/landing-zone-accelerator-on-aws/issues/636#issuecomment-2465890611, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABZLET6MVZMHJWDPYATGMETZ7VBKXAVCNFSM6AAAAABRJ53YCSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINRVHA4TANRRGE . You are receiving this because you are subscribed to this thread.Message ID: @.*** com>

-- Thanks & Regards Satinder

SatinderSidhu commented 3 days ago

unsubscribe

On Fri, Nov 8, 2024 at 1:31 AM Jacky Fan @.***> wrote:

Hi @chummy-opinion https://github.com/chummy-opinion, this is NOT a bug,

The bucket you used was incorrect. In your Log Archive account's S3 console, there is a bucket name called: aws-accelerator-elb-access-logs-- which you need to use this bucket name in the importedBucket.

Here is the correct global-config.yaml file:

elbLogBucket: importedBucket: name: aws-accelerator-elb-access-logs-- applyAcceleratorManagedBucketPolicy: true

— Reply to this email directly, view it on GitHub https://github.com/awslabs/landing-zone-accelerator-on-aws/issues/636#issuecomment-2463882129, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABZLET4O5OMX6HUWTCL7IN3Z7RLELAVCNFSM6AAAAABRJ53YCSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINRTHA4DEMJSHE . You are receiving this because you are subscribed to this thread.Message ID: @.*** com>

-- Thanks & Regards Satinder

chummy-opinion commented 2 days ago

Upon further inspection, the issue appears to be in put-bucket-policy/index.ts.

In the put-bucket-policy/index.ts file within LZA, the generateBucketPolicy function incorrectly sets the Principal for the ELB logs bucket by assigning sourceAccount rather than elbAccountId:

case AcceleratorImportedBucketType.ELB_LOGS_BUCKET:
  let elbPrincipal: PrincipalOrgIdConditionType = {
    Service: ['logdelivery.elasticloadbalancing.amazonaws.com'],
  };

  if (elbAccountId) {
    elbPrincipal = {
      AWS: [`arn:${partition}:iam::${sourceAccount}:root`],
    };
  }

Suggested Code Correction:

Update the elbPrincipal assignment to use elbAccountId:

if (elbAccountId) {
  elbPrincipal = {
    AWS: [`arn:${partition}:iam::${elbAccountId}:root`],
  };
}
SatinderSidhu commented 2 days ago

Unsubscribe Sent from my iPhoneOn Nov 10, 2024, at 5:18 PM, chummy-opinion @.***> wrote: Upon further inspection, the issue appears to be in put-bucket-policy/index.ts. In the put-bucket-policy/index.ts file within LZA, the generateBucketPolicy function incorrectly sets the Principal for the ELB logs bucket by assigning sourceAccount rather than elbAccountId: case AcceleratorImportedBucketType.ELB_LOGS_BUCKET: let elbPrincipal: PrincipalOrgIdConditionType = { Service: ['logdelivery.elasticloadbalancing.amazonaws.com'], };

if (elbAccountId) { elbPrincipal = { AWS: [arn:${partition}:iam::${sourceAccount}:root], }; }

Suggested Code Correction: Update the elbPrincipal assignment to use elbAccountId: if (elbAccountId) { elbPrincipal = { AWS: [arn:${partition}:iam::${elbAccountId}:root], }; }

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you commented.Message ID: @.***>