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

SARIF output support #573

Merged
merged 6 commits into from
Oct 25, 2021
Merged

SARIF output support #573

merged 6 commits into from
Oct 25, 2021

Conversation

arothian
Copy link
Contributor

Adds support for output results in SARIF format #568

  • Pass rule definitions to result formatters
  • Add names to rule definitions/violations
  • Utility method to generate violations from rule classes

@@ -152,17 +153,12 @@ def audit_result(violations)
}
end

def fatal_violation(message)
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 was moved to a class method in Violation

violation(logical_resource_ids)
end

def violation(logical_resource_ids, line_numbers = nil)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Allow rule implementation classes to generate violation objects without needing to run audit. This is useful for unit testing and other locations

@arothian
Copy link
Contributor Author

Open Questions

  • Should the rules list contain all rules in the scan or only those with results?
  • Handling of file paths in results

@NickLiffen
Copy link

Open Questions

  • Should the rules list contain all rules in the scan or only those with results?

Take a look here. There actually isn't a right or wrong answer here. The main thing is people can go and see more information about the rules ran. At a minimum, it should have the rules where is a result. In a perfect world, all rules would be great, but it can just be with results.

Handling of file paths in results

do you mean how to structure to location format so it shows the line path within the GUI?

@NickLiffen
Copy link

Nice work @arothian 💯

This is actually super close to being done 👍 I just tested it and the only problem I am seeing is:

"region": {
    "startLine": -1
 }

This data should be structured like this:

"region": {
    "startLine": 54,
 }

I see it is working in some situations, for example:

              "physicalLocation": {
                "artifactLocation": {
                  "uri": "../GCSMTTR/template.yml",
                  "uriBaseId": "%SRCROOT%"
                },
                "region": {
                  "startLine": 526
                }
              },

However, for quite a few others I am seeing:

                "region": {
                  "startLine": -1
                }

The problem here is the -1. Are you able to replicate this? If not I am happy to send you the file that I am working with. I tried to upload it to GitHub Code Scanning but it failed as the file wasn't valid.

@NickLiffen
Copy link

It looks like it may be rule specific 🤔

Screenshot 2021-10-12 at 17 17 20

@arothian
Copy link
Contributor Author

@NickLiffen Are you using AWS::Serverless::Function in your template? I think the line number is missing in that case as the Lambda function is technically created as part of the transform.

I'll see if we can get a line number that matches back to the pre-transformed resource; otherwise, I can likely drop off the line number entirely for those results.

@NickLiffen
Copy link

@arothian you are indeed correct, this is using SAM, so they are AWS::Serverless::Function set.

It would be amazing if you could grab the line number before pre-transformed resources. Worst comes to worst (not saying we should do this) we see a lot of companies putting line 1 if they don't know where it is coming from. Most customers find it useful (and they prefer the tool if the actual line numbering works (as that's the value of SARIF and the code scanning GUI). However, if it's too complex architectural (which I understand sometimes these solutions can be), then maybe just default to the line 1. but only for lines we can't really find.

@arothian
Copy link
Contributor Author

Alright, I've added some protection to this so that it will default to line 1 if for some reason the finding doesn't have a valid line number detected. That should help protect against this scenario.

I'll open a separate PR around the issue with -1 line numbers for SAM resources, as it's not directly related to SARIF support here.

@NickLiffen
Copy link

@arothian this is good to ship 💯 (IMO)

{
name: 'cfn_nag',
informationUri: 'https://github.com/stelligent/cfn_nag',
semanticVersion: Gem.loaded_specs['cfn-nag'].version.to_s,
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we need to do checks for when this is nil or using .& format because rake:test-all is coming back with failures in these cases:

Failures:

  1) SarifResults#driver with four rules should contain name, informationUri, semanticVersion and rules attributes
     Failure/Error: semanticVersion: Gem.loaded_specs['cfn-nag'].version.to_s,

     NoMethodError:
       undefined method `version' for nil:NilClass
     # ./lib/cfn-nag/result_view/sarif_results.rb:38:in `driver'
     # ./spec/result_view/sarif_results_spec.rb:36:in `block (4 levels) in <top (required)>'

  2) SarifResults#driver with four rules should contain a driver with four rules
     Failure/Error: semanticVersion: Gem.loaded_specs['cfn-nag'].version.to_s,

     NoMethodError:
       undefined method `version' for nil:NilClass
     # ./lib/cfn-nag/result_view/sarif_results.rb:38:in `driver'
     # ./spec/result_view/sarif_results_spec.rb:44:in `block (4 levels) in <top (required)>'

  3) SarifResults#driver with four rules should contain a rule with id, name, fullDescription
     Failure/Error: semanticVersion: Gem.loaded_specs['cfn-nag'].version.to_s,

     NoMethodError:
       undefined method `version' for nil:NilClass
     # ./lib/cfn-nag/result_view/sarif_results.rb:38:in `driver'
     # ./spec/result_view/sarif_results_spec.rb:50:in `block (4 levels) in <top (required)>'

Finished in 12.88 seconds (files took 11.49 seconds to load)
871 examples, 3 failures

Failed examples:

rspec ./spec/result_view/sarif_results_spec.rb:35 # SarifResults#driver with four rules should contain name, informationUri, semanticVersion and rules attributes
rspec ./spec/result_view/sarif_results_spec.rb:43 # SarifResults#driver with four rules should contain a driver with four rules
rspec ./spec/result_view/sarif_results_spec.rb:49 # SarifResults#driver with four rules should contain a rule with id, name, fullDescription

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thegonch Addressed by setting up a version.rb file and updating several code references to use that instead.

Copy link
Contributor

@thegonch thegonch left a comment

Choose a reason for hiding this comment

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

Good fixes, however, during local testing, the end to end test fails on the version_spec test as it expects the hardcoded version 0.0.01 to be in use.

begin end-to-end tests...
Run options: include {:end_to_end=>true}
..F....1
.F......F..

Failures:

  1) cfn_nag --version when ensuring the local gem is installed equals 0.0.01
     Failure/Error:
       expect { system %( cfn_nag --version ) }
         .to output(a_string_matching('0.0.01'))
         .to_stdout_from_any_process

       expected block to output a string matching "0.0.01" to stdout, but output "0.0.0\n"
       Diff:
       @@ -1,2 +1,2 @@
       -(a string matching "0.0.01")
       +0.0.0

     # ./spec/end_to_end/cfn_nag/version_spec.rb:6:in `block (3 levels) in <top (required)>'

  2) cfn_nag_rules --version when ensuring the local gem is installed equals 0.0.01
     Failure/Error:
       expect { system %( cfn_nag_rules --version ) }
         .to output(a_string_matching('0.0.01'))
         .to_stdout_from_any_process

       expected block to output a string matching "0.0.01" to stdout, but output "0.0.0\n"
       Diff:
       @@ -1,2 +1,2 @@
       -(a string matching "0.0.01")
       +0.0.0

     # ./spec/end_to_end/cfn_nag_rules/version_spec.rb:6:in `block (3 levels) in <top (required)>'

  3) cfn_nag_scan --version when ensuring the local gem is installed equals 0.0.01
     Failure/Error:
       expect { system %( cfn_nag_scan --version ) }
         .to output(a_string_matching('0.0.01'))
         .to_stdout_from_any_process

       expected block to output a string matching "0.0.01" to stdout, but output "0.0.0\n"
       Diff:
       @@ -1,2 +1,2 @@
       -(a string matching "0.0.01")
       +0.0.0

     # ./spec/end_to_end/cfn_nag_scan/version_spec.rb:6:in `block (3 levels) in <top (required)>'

Finished in 19.08 seconds (files took 7.03 seconds to load)
18 examples, 3 failures

Failed examples:

rspec ./spec/end_to_end/cfn_nag/version_spec.rb:5 # cfn_nag --version when ensuring the local gem is installed equals 0.0.01
rspec ./spec/end_to_end/cfn_nag_rules/version_spec.rb:5 # cfn_nag_rules --version when ensuring the local gem is installed equals 0.0.01
rspec ./spec/end_to_end/cfn_nag_scan/version_spec.rb:5 # cfn_nag_scan --version when ensuring the local gem is installed equals 0.0.01

@NickLiffen
Copy link

👋 @thegonch / @arothian just wanted to see where we were with this 👍

I have a few developers looking at this PR interesting in consuming the SARIF output with actions. Interested to see if this is something that could get driven home 🚀

Update version spec to reflect the version.rb values
@arothian arothian merged commit 67ae380 into stelligent:master Oct 25, 2021
@arothian arothian deleted the sarif_support branch October 25, 2021 21:34
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.

3 participants