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(cloudformation): inline ignore support for YAML templates #6358

Merged
merged 3 commits into from
Mar 29, 2024

Conversation

nikpivkin
Copy link
Contributor

@nikpivkin nikpivkin commented Mar 20, 2024

Description

Moved inline ignore handling from terraform to a separate package, which will allow to add support for ignore by inline comments to other iac scanners. Added ignore support for CloudFormation (yaml only).

Related issues

Checklist

  • I've read the guidelines for contributing to this repository.
  • I've followed the conventions in the PR title.
  • I've added tests that prove my fix is effective or that my feature works.
  • I've updated the documentation with the relevant information (if needed).
  • I've added usage information (if the PR introduces new options)
  • I've included a "before" and "after" example to the description (if the PR is a user interface change).

@nikpivkin nikpivkin marked this pull request as ready for review March 20, 2024 13:04
@nikpivkin nikpivkin requested a review from simar7 as a code owner March 20, 2024 13:04
@nikpivkin nikpivkin changed the title feat(cloudformation): support inline ignores feat(cloudformation): inline ignore support for YAML templates Mar 21, 2024
Comment on lines -22 to 20
func NewCFReferenceWithValue(resourceRange iacTypes.Range, resolvedValue Property, logicalId string) CFReference {
return CFReference{
resourceRange: resourceRange,
resolvedValue: resolvedValue,
logicalId: logicalId,
}
}

func (cf CFReference) String() string {
return cf.resourceRange.String()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were they not used?

Copy link
Contributor Author

@nikpivkin nikpivkin Mar 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The resolvedValue field was used in methods that were not used anywhere else, so this constructor was not needed.

ignored int
}{
{
name: "without ignored",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
name: "without ignored",
name: "without ignore",

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed dd26943

Comment on lines -30 to -37
func ScannerWithAlternativeIDProvider(f func(string) []string) options.ScannerOption {
return func(s options.ConfigurableScanner) {
if tf, ok := s.(ConfigurableTerraformScanner); ok {
tf.AddExecutorOptions(executor.OptionWithAlternativeIDProvider(f))
}
}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

were alternativeIDProviders not in use? I'm not sure of the original motivation behind them but just want to be sure they weren't part of any use case that we might have missed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not used in the Trivy. I think it's a legacy from tfsec.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have noticed that there is a lot of logic in the Terraform scanner to process the scan results and a lot of it is not being used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have noticed that there is a lot of logic in the Terraform scanner to process the scan results and a lot of it is not being used.

We should remove such logic, in a separate PR.

// tfsec:ignore:*[secure=false]
// trivy:ignore:*[secure=false]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would still keep the old tfsec tests in so that we remain backwards compatible but add another test case for the trivy ignores.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I seem to have renamed it by accident, fixed it dd26943

@@ -1,131 +0,0 @@
package parser
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where's this logic been moved to?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@simar7 simar7 self-requested a review March 29, 2024 03:56
@simar7 simar7 added this pull request to the merge queue Mar 29, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Mar 29, 2024
@simar7 simar7 enabled auto-merge March 29, 2024 05:11
@simar7 simar7 added this pull request to the merge queue Mar 29, 2024
Merged via the queue into aquasecurity:main with commit df024e8 Mar 29, 2024
17 checks passed
@nikpivkin nikpivkin deleted the iac-inline-ignore branch March 29, 2024 06:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(misconf): Support inline ignores for CloudFormation Templates
2 participants