cdklabs / cdk-nag

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

feat: Support regexes in resourceSuppressionsByPath #1373

Open cogwirrel opened 1 year ago

cogwirrel commented 1 year ago

Description

It would be really useful to not have to specify a full path (or at least full prefix) to suppress a rule for a particular resource.

ie. NagSuppressions.addResourceSuppressionsByPath(construct, regex, suppressions)

Use Case

One example is for suppressing rules for a bucket deployment, I need to specify the stage name, stack name and then the UUID for the bucket deployment, eg:

NagSuppressions.addResourceSuppressionsByPath(
      this,
      "/StageName/StackName/Custom::CDKBucketDeployment8693BB64968944B69AAFB0CC9EB8756C",
      [
        {
          id: "AwsSolutions-IAM4",
          reason: "Bucket deployment lambda uses basic execution policy for writing to cloudwatch logs",
        },
        {
          id: "AwsSolutions-IAM5",
          reason: "Bucket deployment writes arbitrary data to S3 (ie key not known until deploy time)",
        },
      ],
      true
    );

It would be much more readable if I could do something like this, and I wouldn't have to worry about inferring the stage and stack names.

NagSuppressions.addResourceSuppressionsByPath(
      this,
      "/.*Custom::CDKBucketDeployment.*/",
      [
        {
          id: "AwsSolutions-IAM4",
          reason: "Bucket deployment lambda uses basic execution policy for writing to cloudwatch logs",
        },
        {
          id: "AwsSolutions-IAM5",
          reason: "Bucket deployment writes arbitrary data to S3 (ie key not known until deploy time)",
        },
      ],
      true
    );

Proposed Solution

Have not yet investigated :)

Other information

I'd be equally happy if NagSuppressions exposed a new method for this if there are any concerns around conflicts, eg NagSuppressions.addResourceSuppressionsByPathRegex

Acknowledge

dontirun commented 1 year ago

Thanks for the feature request! I'm somewhat hesitant to implement this as a feature as a poorly created regex expression can lead to unintended suppressions on resources.

Additionally, path based suppressions are highly dependant on order of operations (I.e. the suppression can only be added after the resource has been created). I think it would be very confusing to have to move around a path based suppression based on where the resources are created in your application

cogwirrel commented 1 year ago

Thanks for the speedy response Arun! πŸ˜„

Sure, I understand that a regex could cover more resources than intended so it'd definitely be something to use with caution.

I actually got hit by that issue of order mattering, but I assumed that was just the intended behaviour of the library - I think I'd suppressed a rule on a role before the particular policy that caused the issue had been added, and found that the suppression didn't take effect.

Do you see any other ways we could achieve the same outcome, perhaps less dangerous than a path regex? πŸ˜„ The troublesome cases are usually where CDK adds some construct directly to the stack rather than as a child of the construct that declared it, such as these bucket deployments or AWS custom resources that use singleton lambdas. It'd be great to not need to worry about copy/pasting CDK's UUIDs or having to infer the stage/stack name in a pipeline.

dontirun commented 1 year ago

I actually got hit by that issue of order mattering, but I assumed that was just the intended behaviour of the library - I think I'd suppressed a rule on a role before the particular policy that caused the issue had been added, and found that the suppression didn't take effect.

Currently path based suppressions throw an error of they don't match anything, but that wouldn't be possible when using a regex expression.

Do you see any other ways we could achieve the same outcome, perhaps less dangerous than a path regex? πŸ˜„ The troublesome cases are usually where CDK adds some construct directly to the stack rather than as a child of the construct that declared it, such as these bucket deployments or AWS custom resources that use singleton lambdas. It'd be great to not need to worry about copy/pasting CDK's UUIDs or having to infer the stage/stack name in a pipeline.

I usually use variables in the Suppression path such as this.stackName so that the suppression works across multiple instantiations of the stack with different names.

I also see that at least one UUID is exported in the CDK due to the PR that you made. In that case the UUID can be added as a variable in the path as well

cogwirrel commented 1 year ago

Makes sense!

Ah funny you saw my PR - when I saw that the bucket deployment used a different UUID this time round I thought perhaps addressing it in CDK Nag would be a longer term improvement! But I totally understand the dangers :) At least it's not too tricky to implement as a one-off helper method without adding it to CDK Nag itself :)

When I next get some bandwidth perhaps I'll do a scan through CDK for all of these UUIDs and see if I can get them all exported in one go :)

meniluca commented 3 months ago

If this feature is not implemented, can you help me writing a stack suppression for a AwsSolutions-L1 and IAM-5 which includes a regex to only match the ID provided as example, say: /StageName/StackName/Custom::CDKBucketDeployment8693BB64968944B69AAFB0CC9EB8756C.

At least it should be possible to do that already with this other method but I don't manage to find a way to make the regex correct. (related to: https://github.com/aws/aws-cdk/issues/27210)