fix(logs): adding a resource policy statement with AnyPrincipal fails
#27787
Conversation
github-actions
bot
added
bug
aws-cdk-automation
added
the
pr/needs-community-review
| 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.
aws-cdk-automation
removed
the
pr/needs-community-review
kylelaker
force-pushed
the
log-grou-arn-parse
branch
from
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.
kylelaker
force-pushed
the
log-grou-arn-parse
branch
from
e102df4
to
b63ef98
Compare
aws-cdk-automation
added
the
pr/needs-community-review
| @@ -236,7 +236,7 @@ abstract class LogGroupBase extends Resource implements ILogGroup { | |||
| return new iam.ArnPrincipal(principal.principalAccount); | |||
| } | |||
|
|
|||
| 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
aws-cdk-automation
removed
the
pr/needs-community-review
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. |
mergify
bot
dismissed
paulhcsun’s stale review
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