(aws-logs): (Support for KMS encryption on Log Retention Construct)

AlastairMiller commented 1 year ago

Describe the feature

The LogRetentionProps do not allow passing a KMS key unlike LogGroupProps

Use Case

To be compliant with business security requirements, all log groups require encryption. This can be satisfied by using the LogGroup construct but currently, the more convenient LogRetention construct cannot replicate this behaviour.

Proposed Solution

Adding an optional parameter to the props which allows passing of a KMS Key if desired.

export interface LogGroupProps {
  /**
   * The KMS customer managed key to encrypt the log group with.
   *
   * @default Server-side encrpytion managed by the CloudWatch Logs service
   */
  readonly encryptionKey?: kms.IKey;

...

}

This can be then passed (if present) through to the lambda and the SDK accepts kmsKeyId as a parameter


The Amazon Resource Name (ARN) of the CMK to use when encrypting log data. For more information, see [Amazon Resource Names - AWS Key Management Service](https://docs.aws.amazon.com/general/latest/gr/aws-arns-and-namespaces.html#arn-syntax-kms).

Type: String

Length Constraints: Maximum length of 256.

Required: No

### Other Information

Relevant SDK docs. https://docs.aws.amazon.com/AmazonCloudWatchLogs/latest/APIReference/API_CreateLogGroup.html

### Acknowledgements

- [X] I may be able to implement this feature request
- [ ] This feature might incur a breaking change

### CDK version used

v1.180

### Environment details (OS name and version, etc.)

MacOS version 11

peterwoodworth commented 1 year ago

Hey @AlastairMiller,

We should be able to support this if the LogGroup is being created as part of this construct. As part of the LogRetention construct, we use a custom resource. As part of this custom resource, we are making a createLogGroup call, which does allow you to set the encryption key. We would be able to modify this in our custom resource here https://github.com/aws/aws-cdk/blob/4c11af6067b35125781aa590bb33c7990078d1ed/packages/%40aws-cdk/aws-logs/lib/log-retention-provider/index.ts#L32

Unfortunately this might not cover all use cases, as there is no PutLogGroup API call to make. And my understanding of this LogRetention resource is that it may not necessarily be responsible for creating the log groups it is modifying. So, if this custom resource does not create the log group, there might not be a way to add encryption.

AlastairMiller commented 1 year ago

Thanks for your response @peterwoodworth ,

That is the behaviour I would expect. As far as I know you cannot encrypt a Log group post creation anyway. In this case I would expect another encrypted log group to be created rather than replacement of the existing. I have completed a quick POC and it appears to work as I would expect.

wgiddens commented 1 year ago

There are API calls to associate and disassociate a KMS key with an existing log group.

aws / aws-cdk