search

aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.63k stars 3.91k forks source link

(core): addUnacknowledgeableWarning() method for Annotations #26914

Open dontirun opened 1 year ago

dontirun commented 1 year ago

Describe the feature

Exactly the addWarning method, but not deprecated

Use Case

cdk-nag uses the cdk Annotation system to emit Warnings and Errors. Currently it uses the addWarning method for Warnings. addWarning was deprecated with the introduction of addWarningV2 in the latest cdk release. All cdk-nag Warnings now emit deprecation messages along with the warning, which adds a lot of clutter the the CLI output. While this could be fixed by switching to addWarningV2, I don't think that addWarningV2 is the correct choice in this case, primarily due to how the cdk-nag warning/error suppression system is intended to function.

Primary Concern acknowledgeWarning vs NagSuppression

NagSuppressions are cdk-nag's way of acknowledging warnings and errors. The key difference between acknowledgeWarning and NagSuppressions are that NagSuppresions write the suppression to the CloudFormation Metadata in a cdk-nag specific format. That Cfn metadata is important when using SAST tooling to look over the generated CloudFormation templates (Anecdotally, I know of several use cases where the cdk-nag Metadata is reviewed in CI/CD pipelines). If cdk-nag were to switch from addWarning to addWarningV2 users would be able to use both acknowledgeWarning and NagSuppresions to silence warnings. While technically not a breaking change giving users the ability to use acknowledgeWarning for cdk-nag isn't a good choice.

Smaller Concerns/Annoyances with addWarningV2 and cdk-nag

if cdk-nag were to switch to addWarningV2 and we were to ignore the primary concern there would be a few other minor annoyances.

  1. The CLI output of warnings and errors would be slightly different from one another. Warnings would have an additional addWarningV2 identifier appended to the end of the message, while errors would not. While that would be the existing RuleId already used for cdk-nag, errors don't have a corresponding addErrorV2 (and in my opinion it doesn't make sense to have this) so this adds a bit of clutter and confusion
  2. Since there is no addErrorV2 method there would a delta between the options to acknowledge/suppress Warnings and Errors

Proposed Solution

I don't think that "undeprecating" addWarning is the correct solution, because in cdk internal uses casesaddWarningV2 is the correct choice and users should use addWarningV2. Having a clone of addWarning with a better name would add some "guardrails"(?) on usage

  1. Add an addUnacknowledgeableWarning() method to Annotations which is a clone of of the current addWarning method
  2. Put a disclaimer message message that users should likely be using addWarningV2 unless they have a specific use case where the warnings should not be acknowledgeable and that unhandled warnings will prohibit users from deploying cdk applications with strict mode.

Other Information

Related to https://github.com/cdklabs/cdk-nag/issues/1418

Acknowledgements

CDK version used

2.93.0

Environment details (OS name and version, etc.)

Not relevant, but macOs 13.4 (Ventura) 😄

indrora commented 1 year ago

I'm torn if this is a bug or a feature request. I want to err on the side of bug.

I'm going to consider this a bug

mrgrain commented 1 year ago

Thanks @dontirun we are undeprecating the method until we have a better solution for you.

We are currently thinking something like an accessible report of acknowledged warnings for plugins to parse. NagSuppressions could then be a thin wrapper around CDK functionality. But we'd also have to address the papercuts you've mentioned.

rix0rrr commented 1 year ago

On the papercuts:

this adds a bit of clutter and confusion

I appreciate this, but I also think it's vitally important that the suppression tag is surfaced in a clear way to users, especially if we intend to go full-hog on warnings (which I want to, with all the tweakable and adjustable things and weird edge cases people can get into, it's much better to overwarn and allow it to be silenced, than to not warn or *gasp* error). I opted for putting the tag into the warning string directly, so there's no way to miss it. Of course, any presentation layer could be aware of this, parse out the tag and display it differently, for example in a separate table column or something.

there would a delta [...] to acknowledge/suppress [...] Errors

Do you have an example of a suppressible error? To my mind the major distinction between warnings and errors is their suppressibility, and so the fact that they cannot be suppressed is a feature.

dontirun commented 1 year ago

Do you have an example of a suppressible error? To my mind the major distinction between warnings and errors is their suppressibility, and so the fact that they cannot be suppressed is a feature.

Yes, but with some context first 😄. cdk-nag has a concept of NagPacks which are sets of guidance that are based on some set of standards. Generally during a security review/audit you provide evidence to compliance and provide/discuss/ document exceptions with the reviewer/auditor.

Given that, let's say that you are using the HIPAA security based NagPack and encounter the HIPAA.Security-LambdaInsideVPC error. If you are strictly following the HIPAA guidelines, you want to prevent a deployment that has a misconfiguration (Error), but there are plenty of valid reasons why this particular guideline doesn't matter in a specific case. For example, maybe your lambda function is calling an external service, your application is completely serverless and doesn't have a VPC otherwise, your lambda function isn't processing any Health related data, etc. In that case you would suppress with your documented reason and continue the deployment.

rix0rrr commented 1 year ago

In CDK Core feature parlance, would it be equivalent to say that message is a warning, and everyone must always run with --strict ? That is, all warnings must either be fixed or acknowledged?

dontirun commented 1 year ago

In CDK Core feature parlance, would it be equivalent to say that message is a warning, and everyone must always run with --strict ? That is, all warnings must either be fixed or acknowledged?

Yes, I would say an error in cdk-nag is equivalent to this