fix(logs): adding a resource policy statement with AnyPrincipal fails#27787
fix(logs): adding a resource policy statement with AnyPrincipal fails#27787mergify[bot] merged 5 commits intoaws:mainfrom
AnyPrincipal fails#27787Conversation
| const logGroup = new LogGroup(this, 'LogGroup'); | ||
| logGroup.addToResourcePolicy(new iam.PolicyStatement({ | ||
| effect: iam.Effect.ALLOW, | ||
| resources: ['*'], |
There was a problem hiding this comment.
since this int test may be used as an example, maybe this should be resources: [logGroup.logGroupArn] ? The way it's written now, it results in a policy that says any anonymous principal can write to any log group in the account.
There was a problem hiding this comment.
One more thought... is addToResourcePolicy misleading? It's a method on LogGroup, but it actually creates a AWS::Logs::ResourcePolicy which is not tied to a LogGroup at all, but is at the Account/Region level (with the ability for resources to be scoped down in the policy statement). The problem is this is parameterized and as we can see from this example, the policy affects more than the log group the method is called on. I almost think resources for this method should be set to logGroup.logGroupArn and there should be another mechanism to create a resource policy with parameterized resources.
There was a problem hiding this comment.
I have changed this to the suggested value. I'd recommend that if there are larger concerns about the addToResourcePolicy API that they be addressed in a separate issue to make sure that they get the needed attention rather than on a PR targeted at a tangentially related bug fix.
71c191a to
e102df4
Compare
Because `AnyPrincipal` extends `ArnPrincipal` it gets caught up in the checks for parsing the ARN from the principal to get the account. This check should be skipped when the ARN is set to `"*"` because that can't be parsed.
e102df4 to
b63ef98
Compare
lpizzinidev
left a comment
There was a problem hiding this comment.
Looks good to me 👍
Just a super-minor nit (it was annoying 🤓)
| } | ||
|
|
||
| if (principal instanceof iam.ArnPrincipal) { | ||
| if (principal instanceof iam.ArnPrincipal && principal.arn !== '*') { |
There was a problem hiding this comment.
Can you please fix the typo in the function name? convertArnPrincipalToAccountId
There was a problem hiding this comment.
I'll push a change for this in a separate PR. Agreed that it's annoying but I don't want to hold this PR up because of it. Updated: #28416
There was a problem hiding this comment.
Great work, thanks for adding this fix @kylelaker, and thanks for reviewing @lpizzinidev @msambol!
|
Not sure why tests are failing, I'll push an empty commit in a sec to trigger the build again. |
Pull request has been modified.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Because
AnyPrincipalextendsArnPrincipalit gets caught up in the checks for parsing the ARN from the principal to get the account. This check should be skipped when the ARN is set to"*"because that can't be parsed.Closes #27783.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license