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

feat: Support regexes in resourceSuppressionsByPath #1373

Open
1 of 2 tasks
cogwirrel opened this issue Jul 14, 2023 · 4 comments
Open
1 of 2 tasks

feat: Support regexes in resourceSuppressionsByPath #1373

cogwirrel opened this issue Jul 14, 2023 · 4 comments
Labels
feature-request A feature should be added or improved.

Comments

@cogwirrel
Copy link
Contributor

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

  • I may be able to implement this feature request
  • This feature might incur a breaking change
@cogwirrel cogwirrel added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Jul 14, 2023
@dontirun
Copy link
Collaborator

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

@dontirun dontirun removed the needs-triage This issue or PR still needs to be triaged. label Jul 14, 2023
@cogwirrel
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved.
Projects
None yet
Development

No branches or pull requests

2 participants