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

Add support to extract rules to skip from terraform comments #434

Merged
merged 7 commits into from
Jan 8, 2021

Conversation

kanchwala-yusuf
Copy link
Contributor

@kanchwala-yusuf kanchwala-yusuf commented Dec 17, 2020

This PR adds support for extracting rule ids to skip policy evaluation. These rules can be set as a comma separated list of rule ids in the comments of terraform resource config in the following pattern:

#ts:skip=AWS.S3Bucket.DS.High.1041, AWS.S3Bucket.DS.High.1042

Example of a terraform resource config with rules to skip policy evaluation:

resource "aws_s3_bucket" "bucket2" {
   #ts:skip=AWS.S3Bucket.DS.High.1041
   region        = var.region
   bucket        = local.bucket_name
   #ts:skip=AWS.S3Bucket.DS.High.1044,AWS.S3Bucket.DS.High.1045
   force_destroy = true
   #ts:skip= AWS.S3Bucket.DS.High.1046   ,   AWS.S3Bucket.DS.High.1047
   acl           = "public-read"
 }

@codecov
Copy link

codecov bot commented Dec 17, 2020

Codecov Report

Merging #434 (08f94ec) into master (c39a76d) will increase coverage by 0.11%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #434      +/-   ##
==========================================
+ Coverage   74.46%   74.57%   +0.11%     
==========================================
  Files          87       88       +1     
  Lines        2005     2014       +9     
==========================================
+ Hits         1493     1502       +9     
  Misses        379      379              
  Partials      133      133              
Impacted Files Coverage Δ
pkg/iac-providers/terraform/v12/resource.go 66.66% <100.00%> (+1.66%) ⬆️
pkg/utils/skip_rules.go 100.00% <100.00%> (ø)


"github.com/accurics/terrascan/pkg/iac-providers/output"
var (
skipRulesPattern = regexp.MustCompile(`#ts:skip=\s*(([A-Za-z0-9]+\.?){5})(\s*,\s*([A-Za-z0-9]+\.?){5})*`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just FYI, I think the rule ID format may go through some changes prior to the next release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, kept that in mind!

williepaul
williepaul previously approved these changes Jan 8, 2021
Copy link
Contributor

@williepaul williepaul left a comment

Choose a reason for hiding this comment

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

Changes look fine, my only thought is about the skip_rules param when it is null. Shall we mark this in the json as omitempty? It seems like that would simplify some of the test data by not having to include it when null. Also, it prevents an empty field from being printed out in the yaml/json/xml output.

@kanchwala-yusuf
Copy link
Contributor Author

Changes look fine, my only thought is about the skip_rules param when it is null. Shall we mark this in the json as omitempty? It seems like that would simplify some of the test data by not having to include it when null. Also, it prevents an empty field from being printed out in the yaml/json/xml output.

@williepaul, I agree with you and I do not have a problem using omitempty flag as well. The reason it is kept as it because skip_rules would be in output of --config-only option which is primarily used for debugging. And the thought that keep the skip_rules field untouched would help in debugging.

@sonarcloud
Copy link

sonarcloud bot commented Jan 8, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@williepaul williepaul merged commit 32ff137 into tenable:master Jan 8, 2021
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.

2 participants