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: Simpler appliesTo syntax for referencing CDK nodes #1715

Open zsstiers opened 3 months ago

zsstiers commented 3 months ago

Description

Support passing unresolved strings to appliesTo.

Use Case

This commonly applies to cases where L2 constructs add wildcard resources to roles. For example, the .grantInvoke method on L2 Lambda Function construct adds [this.functionArn, `${this.functionArn}:*`].

The idea is to support the following syntax, which is more consistent with how the rest of CDK works.

NagSuppressions.addResourceSuppressions(
  lambdaInvokeRole,
  [{
    id: 'AwsSolutions-IAM5',
    reason: 'The role to invoke the lambda function is intended to be allowed to also invoke any of its layers',
    appliesTo: [`Resource::${lambda.functionArn}:*`],
  }],
  true,
);

This question did previously come up on https://github.com/cdklabs/cdk-nag/issues/957, but I wanted to propose a simpler syntax for use cases that a direct logicalId suppression would be cumbersome. An example of one of those more cumbersome cases is inside a construct that is used multiple times throughout the stack(s). I am aware that extracting the logicalId as that ticket does would also work, though is not as intuitive as building the string in the manner that would commonly be done for other uses cases within CDK.

Proposed Solution

A quick prototype version that worked for me was to add one more argument to NagSuppressionHelper.doesApply named resolve. resolve is really just Stack.resolve. It was then used like

if (typeof appliesTo === 'string') {
    return flattenCfnReference(resolve(appliesTo)) === findingId;
}
else {
    const regex = NagSuppressionHelper.toRegEx(appliesTo.regex);
    return regex.test(findingId);
}

NagSuppresionHelper.doesApply is only called in one location, NagPack.ignoreRule, so I also updated that call site to pass in the equivalent of

const resourceStack = Stack.of(resource);
const resolve = resourceStack.resolve.bind(resourceStack);

Including the new imports, the prototype only had a 7 line diff.

Other information

There are two drawbacks I'm aware of to supporting this, although I'm not sure that either of them would be very important in practice.

The first is that the computational complexity of ignoring rules has increased deep inside the loops it is iterating through. Even on strings with no tokens there is still work being done to identify that to make both of those function calls return the input.

The second is that the metadata in the CFN templates are larger due to containing the token formatting syntax. In the example above it would contain something like

{
  "Fn::Join": [
    "",
    [
      "Resource::",
      "Fn::GetAtt": ["ExampleFunctionLogicalId12345678", "Arn"],
      ":*"
    ]
  ]
}

I suspect that the second drawback could be mitigated by updating the code that inserts the metadata to use resolve and use flattenCfnReference. I did not attempt this because it seemed too involved for my little prototype to attempt a metadata rewrite. I was also concerned about erroneously flattening before a logical id was overridden, which of course just increases the complexity of trying to identify when it is safe to update the metadata into the flattened format.

Acknowledge

dontirun commented 3 months ago

I like the idea! Some things to think about.

I was also concerned about erroneously flattening before a logical id was overridden

I think an implementation should look further into this. I think its best to use as little template space as possible, and this may not be a problem depending on what app lifecycle phase the overrides take place.