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

Application Load Balancer, LogAccessLogs doesnt follow best practise #2824

Closed
McDoit opened this issue Jun 11, 2019 · 5 comments · Fixed by #2929
Closed

Application Load Balancer, LogAccessLogs doesnt follow best practise #2824

McDoit opened this issue Jun 11, 2019 · 5 comments · Fixed by #2929
Labels
bug This issue is a bug. needs-triage This issue or PR still needs to be triaged.

Comments

@McDoit
Copy link
Contributor

McDoit commented Jun 11, 2019

Describe the bug
The policy generated by the LogAccessLogs method allows too wide of a permission on a prefix of loadbalancer

${log-bucket.Arn}/loadbalancer*
vs
${log-bucket.Arn}/arn:aws:s3:::${log-bucket}/loadbalancer/AWSLogs/${AWS::AccountId}/*

To Reproduce
Create a ALB and call LogAccessLogs on it, with a bucket
Generates a bucket policy with ${log-bucket.Arn}/loadbalancer*

Expected behavior
Generate a bucket policy with ${log-bucket.Arn}/arn:aws:s3:::${log-bucket}/loadbalancer/AWSLogs/${AWS::AccountId}/*

https://docs.aws.amazon.com/elasticloadbalancing/latest/application/load-balancer-access-logs.html

Version:

  • OS - Win 10
  • Programming Language - CSharp
  • CDK Version - 0.28
@McDoit McDoit added the bug This issue is a bug. label Jun 11, 2019
@NGL321 NGL321 added the needs-triage This issue or PR still needs to be triaged. label Jun 17, 2019
@made2591
Copy link
Contributor

I would be happy to work on this if it's ok for you!

@McDoit
Copy link
Contributor Author

McDoit commented Jun 18, 2019

Feel free, havnt digged down anything in it yet

@made2591
Copy link
Contributor

I made the change, but I was thinking if it is maybe too much restrictive then... is there any situation in which you would like to have access to multiple accountId under this?

@McDoit
Copy link
Contributor Author

McDoit commented Jun 19, 2019

I think that can be up to the usre to add a less restrictive policy for those rare cases?

AWS writes it like this:

For Amazon Resource Name (ARN), type the ARN of your S3 bucket in the following format. For aws-account-id, specify the ID of the AWS account that owns the load balancer (for example, 123456789012). Do not specify a wildcard for the account ID, as this would allow any other account to write access logs to your bucket. To use a single bucket to store access logs from load balancers in multiple accounts, specify one ARN per account in the bucket policy, using the corresponding AWS account ID in each ARN.

arn:aws:s3:::bucket/prefix/AWSLogs/aws-account-id/*

Otherwise anyone can send data to the bucket

@made2591
Copy link
Contributor

Yes, you're right. I will open a pull request for this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. needs-triage This issue or PR still needs to be triaged.
Projects
None yet
3 participants