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

(aws_elasticloadbalancingv2): logAccessLogs(myBucket) doesn't grant correct permissions #18367

Closed
ahammond opened this issue Jan 11, 2022 · 3 comments · Fixed by #18558
Closed
Labels
@aws-cdk/aws-elasticloadbalancingv2 Related to Amazon Elastic Load Balancing V2 bug This issue is a bug. in-progress This issue is being actively worked on. p2

Comments

@ahammond
Copy link
Contributor

What is the problem?

Permissions granted to the bucket policy by applicationLoadBalancer.logAccessLogs(logBucket); do not match permissions required for logging as documented at https://docs.aws.amazon.com/elasticloadbalancing/latest/application/load-balancer-access-logs.html

Specifically, per the docs the policy needs to include

{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Effect": "Allow",
      "Principal": {
        "AWS": "arn:aws:iam::elb-account-id:root"
      },
      "Action": "s3:PutObject",
      "Resource": "arn:aws:s3:::bucket-name/prefix/AWSLogs/your-aws-account-id/*"
    },
    {
      "Effect": "Allow",
      "Principal": {
        "Service": "delivery.logs.amazonaws.com"
      },
      "Action": "s3:PutObject",
      "Resource": "arn:aws:s3:::bucket-name/prefix/AWSLogs/your-aws-account-id/*",
      "Condition": {
        "StringEquals": {
          "s3:x-amz-acl": "bucket-owner-full-control"
        }
      }
    },
    {
      "Effect": "Allow",
      "Principal": {
        "Service": "delivery.logs.amazonaws.com"
      },
      "Action": "s3:GetBucketAcl",
      "Resource": "arn:aws:s3:::bucket-name"
    }
  ]
}

The logAccessLogs() method appears to be adding the first stanza to the bucket policy:

        {
            "Effect": "Allow",
            "Principal": {
                "AWS": "arn:aws:iam::MY_ALB_ACCOUNT_ID:root"
            },
            "Action": [
                "s3:PutObject",
                "s3:Abort*"
            ],
            "Resource": "arn:aws:s3:::app-access-logs.clickup-qa.com/AWSLogs/MY_ALB_ACCOUNT_ID/*"
        }

Workaround is

function grantLogDeliveryPrincipal(account: string, logBucket: aws_s3.Bucket): void {
  const principals = [new aws_iam.ServicePrincipal('delivery.logs.amazonaws.com')];
  // Per https://docs.aws.amazon.com/elasticloadbalancing/latest/application/load-balancer-access-logs.html
  [
    {
      actions: ['s3:PutObject'],
      conditions: { StringEquals: { 's3:x-amz-acl': 'bucket-owner-full-control' } },
      principals,
      resources: [logBucket.arnForObjects(`AWSLogs/${account}/*`)],
    },
    {
      actions: ['s3:GetBucketAcl'],
      principals,
      resources: [logBucket.bucketArn],
    },
  ].forEach((p) => logBucket.addToResourcePolicy(new aws_iam.PolicyStatement(p)));
}

Reproduction Steps

import {
  App,
  aws_ec2,
  aws_elasticloadbalancingv2,
  aws_s3,
  Stack,
  StackProps,
} from 'aws-cdk-lib';
import { Construct } from 'constructs';

export class MyStack extends Stack {
  constructor(scope: Construct, id: string, props: StackProps = {}) {
    super(scope, id, props);

    const vpc = new aws_ec2.Vpc(this, 'Vpc');

    const logBucket = new aws_s3.Bucket(this, 'LogBucket');
    const alb = new aws_elasticloadbalancingv2.ApplicationLoadBalancer(this, 'Alb', { vpc });
    alb.logAccessLogs(logBucket); // Expect this to do everything necessary to get logs in the bucket.
  }
}

const devEnv = {
  account: process.env.CDK_DEFAULT_ACCOUNT,
  region: process.env.CDK_DEFAULT_REGION,
};

const app = new App();

new MyStack(app, 'my-stack-dev', { env: devEnv });

app.synth();

What did you expect to happen?

I expected the policy to be correctly configured for the log bucket.

What actually happened?

Logs did not arrive in the bucket.

CDK CLI Version

2.1.0 (build f4f18b1)

Framework Version

2.1.0

Node.js Version

v16.13.1

OS

MacOS

Language

Typescript

Language Version

4.5.2

Other information

No response

@ahammond ahammond added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jan 11, 2022
@github-actions github-actions bot added the @aws-cdk/aws-elasticloadbalancingv2 Related to Amazon Elastic Load Balancing V2 label Jan 11, 2022
@ahammond
Copy link
Contributor Author

Interestingly that first stanza, which is generated by CDK, is coming up as follows:

        {
            "Effect": "Allow",
            "Principal": {
                "AWS": "arn:aws:iam::797873946194:root"
            },
            "Action": [
                "s3:PutObject",
                "s3:Abort*"
            ],
            "Resource": "arn:aws:s3:::app-access-logs.clickup-qa.com/AWSLogs/187416477703/*"
        },

Note that the account in the principal does not match the account id in the resource. These are both deployed in the account identified by the resource. I'm also not sure where the Abort* comes from.

@ryparker ryparker added the p2 label Jan 11, 2022
@ahammond
Copy link
Contributor Author

Ah, the different account ids is intentional and correct (I read further in the docs).

@ryparker ryparker removed the needs-triage This issue or PR still needs to be triaged. label Jan 11, 2022
njlynch added a commit that referenced this issue Jan 20, 2022
…es not grant all necessary permissions

`ALB.logAccessLogs` today grants the ELB account Put/Abort on the bucket. The
NLB code extends this to also grant permissions to the
`delivery.logs.amazonaws.com` service principal. The ALB documentation now
states that the permissions required for ALB are the same as NLB, so
consolidating the code back into the base.

fixes #18367
@njlynch njlynch added the in-progress This issue is being actively worked on. label Jan 20, 2022
@njlynch njlynch removed their assignment Jan 20, 2022
@mergify mergify bot closed this as completed in #18558 Jan 24, 2022
mergify bot pushed a commit that referenced this issue Jan 24, 2022
…es not grant all necessary permissions (#18558)

`ALB.logAccessLogs` today grants the ELB account Put/Abort on the bucket. The
NLB code extends this to also grant permissions to the
`delivery.logs.amazonaws.com` service principal. The ALB documentation now
states that the permissions required for ALB are the same as NLB, so
consolidating the code back into the base.

fixes #18367


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

LukvonStrom pushed a commit to LukvonStrom/aws-cdk that referenced this issue Jan 26, 2022
…es not grant all necessary permissions (aws#18558)

`ALB.logAccessLogs` today grants the ELB account Put/Abort on the bucket. The
NLB code extends this to also grant permissions to the
`delivery.logs.amazonaws.com` service principal. The ALB documentation now
states that the permissions required for ALB are the same as NLB, so
consolidating the code back into the base.

fixes aws#18367


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this issue Feb 21, 2022
…es not grant all necessary permissions (aws#18558)

`ALB.logAccessLogs` today grants the ELB account Put/Abort on the bucket. The
NLB code extends this to also grant permissions to the
`delivery.logs.amazonaws.com` service principal. The ALB documentation now
states that the permissions required for ALB are the same as NLB, so
consolidating the code back into the base.

fixes aws#18367


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-elasticloadbalancingv2 Related to Amazon Elastic Load Balancing V2 bug This issue is a bug. in-progress This issue is being actively worked on. p2
Projects
None yet
3 participants