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

Open
1 of 2 tasks
zsstiers opened this issue Jun 6, 2024 · 1 comment
Open
1 of 2 tasks

feat: Simpler appliesTo syntax for referencing CDK nodes #1715

zsstiers opened this issue Jun 6, 2024 · 1 comment
Labels
feature-request A feature should be added or improved.

Comments

@zsstiers
Copy link

zsstiers commented Jun 6, 2024

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 #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

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

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.

@dontirun dontirun removed the needs-triage This issue or PR still needs to be triaged. label Jun 24, 2024
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