fix(logs): missing KMS key policy statement

[lpizzinidev]

Fixes key policy for encrypted log groups.
The generated log groups are missing a statement causing the deployment to fail.

Closes #28304.

---

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

[msambol]

should this be log-group:<log-group-name> instead of * ?

[lpizzinidev]

I tried to use ArnEquals with the log group ARN but it created a circular dependency between the key and the log group.
Not sure if there's a workaround, I agree that this may be too permissive.

[msambol]

If there is no easy workaround, I am OK with this, as the second example in the docs has *.

[kaizencc]

I'm not sure if this will work @lpizzinidev but have you tried using log-group:${this.physicalName}? Instead of using logGroupArn or logGroupName, which are attributes of the log group.

[lpizzinidev]

@kaizencc
I tried log-group:${this.physicalName} and it works.
It should be safe to go with it, let me know if you think otherwise.

[kaizencc]

It works as in you integ tested it and confirmed that the permission boundary works as expected (not just that it deploys successfully)?

If so, I think that's the best path forward, and once thats in I'm happy to approve.

[lpizzinidev]

@kaizencc
I got it wrong.

conditions: {
  ArnEquals: {
    'kms:EncryptionContext:aws:logs:arn': `arn:${this.stack.partition}:logs:${this.env.region}:${this.env.account}:log-group:${this.physicalName}`,
  },
},

Produces this incorrect policy condition:

Condition: {
  ArnEquals: {
    'kms:EncryptionContext:aws:logs:arn': {
      'Fn::Join': [
        '',
        [
          'arn:',
          {
            Ref: 'AWS::Partition',
          },
          ':logs:',
          {
            Ref: 'AWS::Region',
          },
          ':',
          {
            Ref: 'AWS::AccountId',
          },
          ':log-group:',
        ],
      ],
    },
  },
},

It deploys successfully but the log group name is left blank.

[msambol]

If you use Lazy does that fix it ?

[kaizencc]

@lpizzinidev ^. Fingers crossed it does :)

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: cd88c9d
• Result: FAILED
• Build Logs (available for 30 days)

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

[msambol]

this shouldn't change existing log group names will it ?

[lpizzinidev]

@kaizencc
This is my concern as well.
With this approach, we'll probably trigger a replacement of existing log groups that don't have logGroupName specified (unless I'm missing something).
I don't see a workaround, and the previous solution with ArnLike / :log-group:* may be too loose.
Thoughts?

[kaizencc]

Workaround as always is a feature flag, and I agree that the previous solution is probably too loose. Give me a bit to think this one through and see what we should od. I think the safest thing is a feature flag but will ask around to make sure.

[rix0rrr]

Alternative proposal:

If we add a kms:ViaService condition to the key, is that not good enough? It will allow us to avoid naming the log group.

[lpizzinidev]

@rix0rrr
Isn't it the same as

conditions: {
 ArnLike: {
  'kms:EncryptionContext:aws:logs:arn': `arn:${this.stack.partition}:logs:${this.env.region}:${this.env.account}:log-group:*`,
 },
},

?
It looks to me like we restrict permissions to the service in both cases (original problem).

[msambol]

nice

[aws-cdk-automation]

This PR has been in the BUILD FAILING state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

[lpizzinidev]

@rix0rrr #28319 (comment) 👀

[aws-cdk-automation]

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.