cdklabs / cdk-nag

Check CDK applications for best practices using a combination of available rule packs
Apache License 2.0
835 stars 65 forks source link

bug: SNSEncryptedKMS rule unexpectedly removed from NIST 800-53 rev 5 pack #1849

Open bhegde82 opened 1 week ago

bhegde82 commented 1 week ago

What is the problem?

In a prior change #1821 , the SNSEncryptedKMS rule was removed from cdk-nag entirely due to updated AWS guidance that retired this control for AWS Foundational Security Best Practices: https://docs.aws.amazon.com/securityhub/latest/userguide/sns-controls.html#sns-1

However, as noted in the above documentation, the rule is still included in the NIST 800-53 Rev.5 standard.

Therefore, the rule should be added back to cdk-nag for the NIST 800-53 rev 5 pack, although it can still be excluded for the AwsSolutions pack.

This also breaks custom nag packs that were importing the premade rule for use, even though the change was introduced in a minor version upgrade.

Reproduction Steps

Sample 1: Using NIST 800-53 rev. 5 rule pack

import { App, Aspects, Stack } from "aws-cdk-lib";
import { Topic } from "aws-cdk-lib/aws-sns";
import { Construct } from "constructs";

import { NIST80053R5Checks } from "cdk-nag";

const app = new App();

Aspects.of(app).add(new NIST80053R5Checks({ verbose: true }));

class InfrastructureStack extends Stack {
  constructor(scope: Construct, id: string) {
    super(scope, id);

    // Unencrypted SNS topic
    new Topic(this, "SNSTopic");
  }
}
new InfrastructureStack(app, "CDKNagTest");

Sample 2: Import SNSEncryptedKMS rule into custom nag packs

import { App, Aspects, CfnResource, Stack } from 'aws-cdk-lib';
import { Construct, IConstruct } from 'constructs';
import {
  NagMessageLevel,
  NagPack,
  NagPackProps,
  rules,
} from 'cdk-nag';
import { Topic } from 'aws-cdk-lib/aws-sns';
import { Key } from 'aws-cdk-lib/aws-kms';

export class ExampleChecks extends NagPack {
  constructor (props?: NagPackProps) {
    super(props);
    this.packName = 'Example';
  }
  public visit (node: IConstruct) {
    if (node instanceof CfnResource) {
      this.applyRule({
        info: 'SNS KMS required.',
        explanation: 'Please enabled SNS KMS.',
        level: NagMessageLevel.ERROR,
        rule: rules.sns.SNSEncryptedKMS,
        node: node,
      });
    }
  }
}

const app = new App();

Aspects.of(app).add(new ExampleChecks({ verbose: true }));

class InfrastructureStack extends Stack {
  constructor(scope: Construct, id: string) {
    super(scope, id);

    const key = new Key(this, "Key", { enableKeyRotation: true });
    new Topic(this, "SNSTopic", { masterKey: key });
  }
}
new InfrastructureStack(app, "CDKNagTest");

What did you expect to happen?

Sample 1

This code is supposed to trigger an Error finding due to the SNS topic not using KMS encryption.

> tsc && cdk synth

[Error at /CDKNagTest/SNSTopic/Resource] NIST.800.53.R5-SNSEncryptedKMS: The SNS topic does not have KMS encryption enabled - (Control IDs: AU-9(3), CP-9d, SC-8(3), SC-8(4), SC-13a, SC-28(1)). To help protect data at rest, ensure that your Amazon Simple Notification Service (Amazon SNS) topics require encryption using AWS Key Management Service (AWS KMS) Because sensitive data can exist at rest in published messages, enable encryption at rest to help protect that data.

Sample 2

This code is supposed to synthesize properly into CloudFormation without a runtime error.

What actually happened?

Sample 1

cdk synth succeeds instead of raising the Error finding.

> tsc && cdk synth

Resources:
  SNSTopicBCCC5DD8:
    Type: AWS::SNS::Topic
    Metadata:
      aws:cdk:path: CDKNagTest/SNSTopic/Resource
  CDKMetadata:
    Type: AWS::CDK::Metadata
    Properties:
      Analytics: v2:deflate64:H4sIAAAAAAAA/yXGzQ5EMBAA4Gdxb0c1Ee7egL1vqq1k/MxsTK2DeHfB6fssFJUBk7ldtA+TnrGHo0vOT8rt8hUSOD78Q6+agZ6c99oovK0+3m+YAiZkOhVxiDBK/i9qsAbKbBREvW6UcInQvl4CUdSScwAAAA==
    Metadata:
      aws:cdk:path: CDKNagTest/CDKMetadata/Default
    Condition: CDKMetadataAvailable
Conditions:
  CDKMetadataAvailable:
    Fn::Or:
      - Fn::Or:
          - Fn::Equals:
              - Ref: AWS::Region
              - af-south-1
          - Fn::Equals:
              - Ref: AWS::Region
              - ap-east-1
          - Fn::Equals:
              - Ref: AWS::Region
              - ap-northeast-1
          - Fn::Equals:
              - Ref: AWS::Region
              - ap-northeast-2
          - Fn::Equals:
              - Ref: AWS::Region
              - ap-northeast-3
          - Fn::Equals:
              - Ref: AWS::Region
              - ap-south-1
          - Fn::Equals:
              - Ref: AWS::Region
              - ap-south-2
          - Fn::Equals:
              - Ref: AWS::Region
              - ap-southeast-1
          - Fn::Equals:
              - Ref: AWS::Region
              - ap-southeast-2
          - Fn::Equals:
              - Ref: AWS::Region
              - ap-southeast-3
      - Fn::Or:
          - Fn::Equals:
              - Ref: AWS::Region
              - ap-southeast-4
          - Fn::Equals:
              - Ref: AWS::Region
              - ca-central-1
          - Fn::Equals:
              - Ref: AWS::Region
              - ca-west-1
          - Fn::Equals:
              - Ref: AWS::Region
              - cn-north-1
          - Fn::Equals:
              - Ref: AWS::Region
              - cn-northwest-1
          - Fn::Equals:
              - Ref: AWS::Region
              - eu-central-1
          - Fn::Equals:
              - Ref: AWS::Region
              - eu-central-2
          - Fn::Equals:
              - Ref: AWS::Region
              - eu-north-1
          - Fn::Equals:
              - Ref: AWS::Region
              - eu-south-1
          - Fn::Equals:
              - Ref: AWS::Region
              - eu-south-2
      - Fn::Or:
          - Fn::Equals:
              - Ref: AWS::Region
              - eu-west-1
          - Fn::Equals:
              - Ref: AWS::Region
              - eu-west-2
          - Fn::Equals:
              - Ref: AWS::Region
              - eu-west-3
          - Fn::Equals:
              - Ref: AWS::Region
              - il-central-1
          - Fn::Equals:
              - Ref: AWS::Region
              - me-central-1
          - Fn::Equals:
              - Ref: AWS::Region
              - me-south-1
          - Fn::Equals:
              - Ref: AWS::Region
              - sa-east-1
          - Fn::Equals:
              - Ref: AWS::Region
              - us-east-1
          - Fn::Equals:
              - Ref: AWS::Region
              - us-east-2
          - Fn::Equals:
              - Ref: AWS::Region
              - us-west-1
      - Fn::Equals:
          - Ref: AWS::Region
          - us-west-2
Parameters:
  BootstrapVersion:
    Type: AWS::SSM::Parameter::Value<String>
    Default: /cdk-bootstrap/hnb659fds/version
    Description: Version of the CDK Bootstrap resources in this environment, automatically retrieved from SSM Parameter Store. [cdk:skip]

Sample 2

Typescript compilation fails since the rule has been deleted

> tsc && cdk synth

lib/test.ts:23:25 - error TS2339: Property 'SNSEncryptedKMS' does not exist on type 'typeof import("/Users/username/CDKTest/node_modules/cdk-nag/lib/rules/sns/index")'.

23         rule: rules.sns.SNSEncryptedKMS,
                           ~~~~~~~~~~~~~~~

Found 1 error in lib/test.ts:23

cdk-nag version

2.34.3

Language

Typescript

Other information

No response

clueleaf commented 4 days ago

@bhegde82 Thank you for reporting. I will add the rule back to NIST 800-53 Rev.5 and NIST 800-53 Rev.4 as well.