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

Usage in pre-commit checks that only flag for brand new resources #1621

Open
1 of 2 tasks
gurudesu opened this issue Mar 8, 2024 · 5 comments
Open
1 of 2 tasks

Usage in pre-commit checks that only flag for brand new resources #1621

gurudesu opened this issue Mar 8, 2024 · 5 comments
Labels
feature-request A feature should be added or improved. other This issue doesn't fit into the other categories

Comments

@gurudesu
Copy link

gurudesu commented Mar 8, 2024

Description

I would like to be able to be able to run some command similar to the following:

bb cdk synth --cdk-nag-pre-commit-error-level WARN

and this will exit non-zero for if resources added or modified in the staged commit.

Use Case

Organizations often have old, legacy projects where a compliance uplift is difficult for all resources. Fixing everything is generally untenable upfront. However, net-new resources are often easy to nip at the bud with pre-commit checks.

Without this feature, our only options are to:

  • Fix everything
  • Make the issue WARN only but this almost always gets ignored.

Proposed Solution

I don't have a great solution but I think we'd need a way to:

  • Map a CDK resource to a line in code.
  • Integrate with git to understand what is staged.
  • Provide a way for users to configure when to elevate resource out of warn for pre-commit checks.

Other information

No response

Acknowledge

  • I may be able to implement this feature request
  • This feature might incur a breaking change
@gurudesu gurudesu added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Mar 8, 2024
@gurudesu
Copy link
Author

gurudesu commented Mar 8, 2024

As mentioned in the ticket, I may be able to implement but I'd like to understand if a change like this would be accepted.

@dontirun
Copy link
Collaborator

I believe this can be addressed using a combination of NagLoggers, Suppressions, and Conditional Suppression Ignores.

Example Scenario

Security creates a new rule in their custom NagPack. The new rule is an error, but security decides that they will give developers a 1 month leeway to fix their violating resources. A condition is added to the rule that Suppressions are only accepted for the rest of the month and when linked to a ticket. Additionally, security decides that suppressed errors still should appear as warnings so they add a custom NagLogger that outputs a "You have X days left to fix this violation" warning when onSuppressedError is triggered for that particular ruleId.

Given this, developers can either fix the violation immediately or temporarily suppress the error given that the suppression meets security's requirements.

@gurudesu
Copy link
Author

Interesting - seems like this would work.

One of the concerns I'd like to find a software solution for is the date based checks are a little fragile and ideally, I could use a condition around whether or the resource in in a staged git commit. It might be good enough but it could be better.

Any thoughts on that functionality?

@dontirun
Copy link
Collaborator

Conditions are open ended with their logic. You should be able to use any libararies/custom code you want to do the git based logic that you're looking for.

However, here are a few "gotchas" that you may run into.

  1. The CDK does not pass line numbers to Aspects (the system which cdk-nag uses), only resource ids
  2. A condition is evaluated every time a rule is evaluated against a resource. You won't be able to persist state between evaluations unless you read/write from/to an external file

@dontirun dontirun added other This issue doesn't fit into the other categories and removed needs-triage This issue or PR still needs to be triaged. labels Mar 11, 2024
@gurudesu
Copy link
Author

gurudesu commented Mar 13, 2024

The CDK does not pass line numbers to Aspects (the system which cdk-nag uses), only resource ids

Interesting. Let me look upstream to see if that's resolvable.

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. other This issue doesn't fit into the other categories
Projects
None yet
Development

No branches or pull requests

2 participants