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

fix(logs): adding a resource policy statement with AnyPrincipal fails #27787

Merged
merged 5 commits into from
Dec 18, 2023

Conversation

laurelmay
Copy link
Contributor

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.

Closes #27783.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@aws-cdk-automation aws-cdk-automation requested a review from a team November 1, 2023 02:53
@github-actions github-actions bot added bug This issue is a bug. p2 star-contributor [Pilot] contributed between 25-49 PRs to the CDK labels Nov 1, 2023
@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Nov 1, 2023
const logGroup = new LogGroup(this, 'LogGroup');
logGroup.addToResourcePolicy(new iam.PolicyStatement({
effect: iam.Effect.ALLOW,
resources: ['*'],
Copy link
Contributor

@msambol msambol Nov 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@msambol msambol Nov 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Nov 1, 2023
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.
@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Nov 27, 2023
Copy link
Contributor

@lpizzinidev lpizzinidev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍
Just a super-minor nit (it was annoying 🤓)

@@ -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 !== '*') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please fix the typo in the function name? convertArnPrincipalToAccountId

Copy link
Contributor

@paulhcsun paulhcsun Dec 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 4, 2023
paulhcsun
paulhcsun previously approved these changes Dec 18, 2023
Copy link
Contributor

@paulhcsun paulhcsun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, thanks for adding this fix @kylelaker, and thanks for reviewing @lpizzinidev @msambol!

@paulhcsun
Copy link
Contributor

Not sure why tests are failing, I'll push an empty commit in a sec to trigger the build again.

@mergify mergify bot dismissed paulhcsun’s stale review December 18, 2023 18:51

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 0b7226a
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Copy link
Contributor

mergify bot commented Dec 18, 2023

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).

@mergify mergify bot merged commit 0b2ba1c into aws:main Dec 18, 2023
11 checks passed
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. p2 star-contributor [Pilot] contributed between 25-49 PRs to the CDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws-logs: LogGroup.addToResourcePolicy 'ARNs must start with "arn:" and have at least 6 components: *'
5 participants