diff --git a/bin/cfn_nag_rules b/bin/cfn_nag_rules index 362d20e6..453d4fba 100755 --- a/bin/cfn_nag_rules +++ b/bin/cfn_nag_rules @@ -6,7 +6,7 @@ require 'cfn-nag' require 'rubygems/specification' opts = Optimist.options do - version Gem::Specification.find_by_name('cfn-nag').version + version CfnNagVersion::VERSION opt :rule_directory, 'Extra rule directories', type: :io, required: false, diff --git a/cfn-nag.gemspec b/cfn-nag.gemspec index 69ba0587..5d781cc9 100644 --- a/cfn-nag.gemspec +++ b/cfn-nag.gemspec @@ -1,9 +1,11 @@ # frozen_string_literal: true +require_relative 'lib/cfn-nag/version' + Gem::Specification.new do |s| s.name = 'cfn-nag' s.license = 'MIT' - s.version = ENV['GEM_VERSION'] || '0.0.0' + s.version = CfnNagVersion::VERSION s.bindir = 'bin' s.executables = %w[cfn_nag cfn_nag_rules cfn_nag_scan spcm_scan] s.authors = ['Eric Kascic'] diff --git a/lib/cfn-nag/base_rule.rb b/lib/cfn-nag/base_rule.rb index 84a60076..38737a6f 100644 --- a/lib/cfn-nag/base_rule.rb +++ b/lib/cfn-nag/base_rule.rb @@ -20,10 +20,16 @@ def audit(cfn_model) logical_resource_ids = audit_impl(cfn_model) return if logical_resource_ids.empty? + violation(logical_resource_ids) + end + + def violation(logical_resource_ids, line_numbers = nil) Violation.new(id: rule_id, + name: self.class.name, type: rule_type, message: rule_text, - logical_resource_ids: logical_resource_ids) + logical_resource_ids: logical_resource_ids, + line_numbers: line_numbers) end end end diff --git a/lib/cfn-nag/cfn_nag.rb b/lib/cfn-nag/cfn_nag.rb index 3ea3fe11..f594a97c 100644 --- a/lib/cfn-nag/cfn_nag.rb +++ b/lib/cfn-nag/cfn_nag.rb @@ -8,6 +8,7 @@ require_relative 'result_view/simple_stdout_results' require_relative 'result_view/colored_stdout_results' require_relative 'result_view/json_results' +require_relative 'result_view/sarif_results' require 'cfn-model' # Top-level CfnNag class for running profiles @@ -96,10 +97,10 @@ def audit(cloudformation_string:, parameter_values_string: nil, condition_values violations = filter_violations_by_deny_list_and_profile(violations) violations = mark_line_numbers(violations, cfn_model) rescue RuleRepoException, Psych::SyntaxError, ParserError => fatal_error - violations << fatal_violation(fatal_error.to_s) + violations << Violation.fatal_violation(fatal_error.to_s) rescue JSON::ParserError => json_parameters_error error = "JSON Parameter values parse error: #{json_parameters_error}" - violations << fatal_violation(error) + violations << Violation.fatal_violation(error) end violations = prune_fatal_violations(violations) if @config.ignore_fatal @@ -112,7 +113,7 @@ def prune_fatal_violations(violations) def render_results(aggregate_results:, output_format:) - results_renderer(output_format).new.render(aggregate_results) + results_renderer(output_format).new.render(aggregate_results, @config.custom_rule_loader.rule_definitions) end private @@ -141,7 +142,7 @@ def filter_violations_by_deny_list_and_profile(violations) violations: violations ) rescue StandardError => deny_list_or_profile_parse_error - violations << fatal_violation(deny_list_or_profile_parse_error.to_s) + violations << Violation.fatal_violation(deny_list_or_profile_parse_error.to_s) violations end @@ -152,17 +153,12 @@ def audit_result(violations) } end - def fatal_violation(message) - Violation.new(id: 'FATAL', - type: Violation::FAILING_VIOLATION, - message: message) - end - def results_renderer(output_format) registry = { 'colortxt' => ColoredStdoutResults, 'txt' => SimpleStdoutResults, - 'json' => JsonResults + 'json' => JsonResults, + 'sarif' => SarifResults } registry[output_format] end diff --git a/lib/cfn-nag/cfn_nag_executor.rb b/lib/cfn-nag/cfn_nag_executor.rb index 61ad92d9..ba30c800 100644 --- a/lib/cfn-nag/cfn_nag_executor.rb +++ b/lib/cfn-nag/cfn_nag_executor.rb @@ -74,9 +74,9 @@ def scan_file(cfn_nag, fail_on_warnings) end def validate_options(opts) - unless opts[:output_format].nil? || %w[colortxt txt json].include?(opts[:output_format]) + unless opts[:output_format].nil? || %w[colortxt txt json sarif].include?(opts[:output_format]) Optimist.die(:output_format, - 'Must be colortxt, txt, or json') + 'Must be colortxt, txt, json or sarif') end opts[:rule_arguments]&.each do |rule_argument| diff --git a/lib/cfn-nag/cli_options.rb b/lib/cfn-nag/cli_options.rb index d1ec4ac9..0881c351 100644 --- a/lib/cfn-nag/cli_options.rb +++ b/lib/cfn-nag/cli_options.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require 'optimist' +require_relative 'version' # rubocop:disable Metrics/ClassLength class Options @@ -8,7 +9,7 @@ class Options 'emit the exception without stack trace ' \ 'and keep chugging' - @version = Gem::Specification.find_by_name('cfn-nag').version + @version = CfnNagVersion::VERSION def self.for(type) case type @@ -89,7 +90,7 @@ def self.file_options required: false, default: false opt :output_format, - 'Format of results: [txt, json, colortxt]', + 'Format of results: [txt, json, colortxt, sarif]', type: :string, default: 'colortxt' opt :rule_repository, @@ -132,7 +133,7 @@ def self.scan_options type: :string, required: true opt :output_format, - 'Format of results: [txt, json, colortxt]', + 'Format of results: [txt, json, colortxt, sarif]', type: :string, default: 'colortxt' opt :debug, diff --git a/lib/cfn-nag/custom_rules/base.rb b/lib/cfn-nag/custom_rules/base.rb index 9d0d2267..a5b471fe 100644 --- a/lib/cfn-nag/custom_rules/base.rb +++ b/lib/cfn-nag/custom_rules/base.rb @@ -19,9 +19,15 @@ def audit(cfn_model) logical_resource_ids = audit_impl(cfn_model) return if logical_resource_ids.empty? + violation(logical_resource_ids) + end + + def violation(logical_resource_ids, line_numbers = []) Violation.new(id: rule_id, + name: self.class.name, type: rule_type, message: rule_text, - logical_resource_ids: logical_resource_ids) + logical_resource_ids: logical_resource_ids, + line_numbers: line_numbers) end end diff --git a/lib/cfn-nag/result_view/json_results.rb b/lib/cfn-nag/result_view/json_results.rb index 48fc6d90..52cfc216 100644 --- a/lib/cfn-nag/result_view/json_results.rb +++ b/lib/cfn-nag/result_view/json_results.rb @@ -3,7 +3,7 @@ require 'json' class JsonResults - def render(results) + def render(results, _rule_registry) hashified_results = results.each do |result| result[:file_results][:violations] = result[:file_results][:violations].map(&:to_h) end diff --git a/lib/cfn-nag/result_view/sarif_results.rb b/lib/cfn-nag/result_view/sarif_results.rb new file mode 100644 index 00000000..8d58a319 --- /dev/null +++ b/lib/cfn-nag/result_view/sarif_results.rb @@ -0,0 +1,103 @@ +# frozen_string_literal: true + +require 'json' +require 'pathname' + +class SarifResults + def render(results, rule_registry) + sarif_results = [] + results.each do |file| + # For each file in the results, review the violations + file[:file_results][:violations].each do |violation| + # For each violation, generate a sarif result for each logical resource id in the violation + violation.logical_resource_ids.each_with_index do |_logical_resource_id, index| + sarif_results << sarif_result(file_name: file[:filename], violation: violation, index: index) + end + end + end + + sarif_report = { + version: '2.1.0', + '$schema': 'https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json', + runs: [ + tool: { + driver: driver(rule_registry.rules) + }, + results: sarif_results + ] + } + + puts JSON.pretty_generate(sarif_report) + end + + # Generates a SARIF driver object, which describes the tool and the rules used + def driver(rules) + { + name: 'cfn_nag', + informationUri: 'https://github.com/stelligent/cfn_nag', + semanticVersion: CfnNagVersion::VERSION, + rules: rules.map do |rule_definition| + { + id: "CFN_NAG_#{rule_definition.id}", + name: rule_definition.name, + fullDescription: { + text: rule_definition.message + } + } + end + } + end + + # Given a cfn_nag Violation object, and index, generates a SARIF result object for the finding + def sarif_result(file_name:, violation:, index:) + { + ruleId: "CFN_NAG_#{violation.id}", + level: sarif_level(violation.type), + message: { + text: violation.message + }, + locations: [ + { + physicalLocation: { + artifactLocation: { + uri: relative_path(file_name), + uriBaseId: '%SRCROOT%' + }, + region: { + startLine: sarif_line_number(violation.line_numbers[index]) + } + }, + logicalLocations: [ + { + name: violation.logical_resource_ids[index] + } + ] + } + ] + } + end + + # Line number defaults to 1 unless provided with valid number + def sarif_line_number(line_number) + line_number.nil? || line_number.to_i < 1 ? 1 : line_number.to_i + end + + def sarif_level(violation_type) + case violation_type + when RuleDefinition::WARNING + 'warning' + else + 'error' + end + end + + def relative_path(file_name) + file_pathname = Pathname.new(file_name) + + if file_pathname.relative? + file_pathname.to_s + else + file_pathname.relative_path_from(Pathname.pwd).to_s + end + end +end diff --git a/lib/cfn-nag/result_view/stdout_results.rb b/lib/cfn-nag/result_view/stdout_results.rb index 7cb6009d..fca2ad51 100644 --- a/lib/cfn-nag/result_view/stdout_results.rb +++ b/lib/cfn-nag/result_view/stdout_results.rb @@ -24,7 +24,7 @@ def print_warnings(violations) puts "Warnings count: #{Violation.count_warnings(violations)}" end - def render(results) + def render(results, _rule_definitions) results.each do |result| 60.times { print '-' } puts "\n#{result[:filename]}" diff --git a/lib/cfn-nag/rule_definition.rb b/lib/cfn-nag/rule_definition.rb index 358a2907..802b62de 100644 --- a/lib/cfn-nag/rule_definition.rb +++ b/lib/cfn-nag/rule_definition.rb @@ -4,27 +4,30 @@ class RuleDefinition WARNING = 'WARN' FAILING_VIOLATION = 'FAIL' - attr_reader :id, :type, :message + attr_reader :id, :name, :type, :message def initialize(id:, + name:, type:, message:) @id = id + @name = name @type = type @message = message - [@id, @type, @message].each do |required| + [@id, @type, @name, @message].each do |required| raise 'No parameters to Violation constructor can be nil' if required.nil? end end def to_s - "#{@id} #{@type} #{@message}" + "#{@id} #{name} #{@type} #{@message}" end def to_h { id: @id, + name: @name, type: @type, message: @message } diff --git a/lib/cfn-nag/rule_registry.rb b/lib/cfn-nag/rule_registry.rb index a4c2d8ec..a4c9cb97 100644 --- a/lib/cfn-nag/rule_registry.rb +++ b/lib/cfn-nag/rule_registry.rb @@ -43,6 +43,7 @@ def definition(rule_class) if existing_def.nil? rule_definition = RuleDefinition.new( id: rule.rule_id, + name: rule_class.name, type: rule.rule_type, message: rule.rule_text ) diff --git a/lib/cfn-nag/version.rb b/lib/cfn-nag/version.rb new file mode 100644 index 00000000..822cf656 --- /dev/null +++ b/lib/cfn-nag/version.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true + +module CfnNagVersion + # This is managed at release time via scripts/publish.sh + VERSION = '0.0.0' +end diff --git a/lib/cfn-nag/violation.rb b/lib/cfn-nag/violation.rb index 3a20ae9c..b9b382c7 100644 --- a/lib/cfn-nag/violation.rb +++ b/lib/cfn-nag/violation.rb @@ -6,18 +6,22 @@ class Violation < RuleDefinition attr_reader :logical_resource_ids, :line_numbers + # rubocop:disable Metrics/ParameterLists def initialize(id:, + name:, type:, message:, logical_resource_ids: [], line_numbers: []) super id: id, + name: name, type: type, message: message @logical_resource_ids = logical_resource_ids @line_numbers = line_numbers end + # rubocop:enable Metrics/ParameterLists def to_s "#{super} #{@logical_resource_ids}" @@ -57,6 +61,13 @@ def count_failures(violations) end end + def fatal_violation(message) + Violation.new(id: 'FATAL', + name: 'system', + type: Violation::FAILING_VIOLATION, + message: message) + end + private def empty?(array) diff --git a/scripts/deploy_local.sh b/scripts/deploy_local.sh index f9352126..53980609 100755 --- a/scripts/deploy_local.sh +++ b/scripts/deploy_local.sh @@ -3,5 +3,5 @@ echo "Installing cfn_nag from local source" gem uninstall cfn-nag -x brew gem uninstall cfn-nag -GEM_VERSION=0.0.01 gem build cfn-nag.gemspec -gem install cfn-nag-0.0.01.gem --no-document +gem build cfn-nag.gemspec +gem install cfn-nag-0.0.0.gem --no-document diff --git a/scripts/publish.sh b/scripts/publish.sh index b76c8ed6..af92477e 100755 --- a/scripts/publish.sh +++ b/scripts/publish.sh @@ -45,7 +45,7 @@ else new_version="${minor_version}.$((current_version+1))" fi -sed -i.bak "s/0\.0\.0/${new_version}/g" cfn-nag.gemspec +sed -i.bak "s/0\.0\.0/${new_version}/g" lib/cfn-nag/version.rb # publish rubygem to rubygems.org, https://rubygems.org/gems/cfn-nag gem build cfn-nag.gemspec diff --git a/scripts/setup_and_run_end_to_end_tests.sh b/scripts/setup_and_run_end_to_end_tests.sh index 2853ce89..b48c0ae5 100755 --- a/scripts/setup_and_run_end_to_end_tests.sh +++ b/scripts/setup_and_run_end_to_end_tests.sh @@ -33,7 +33,7 @@ download_and_scan_templates () { fi } -# Build and install gem locally, using version 0.0.01 +# Build and install gem locally, using version 0.0.0 /bin/sh scripts/deploy_local.sh # Install the two gems required to run end-to-end tests diff --git a/spec/cfn_nag_integration/cfn_nag_cloudfront_distribution_spec.rb b/spec/cfn_nag_integration/cfn_nag_cloudfront_distribution_spec.rb index f75607c9..4b3c6ace 100644 --- a/spec/cfn_nag_integration/cfn_nag_cloudfront_distribution_spec.rb +++ b/spec/cfn_nag_integration/cfn_nag_cloudfront_distribution_spec.rb @@ -18,24 +18,9 @@ file_results: { failure_count: 0, violations: [ - Violation.new( - id: 'W10', type: Violation::WARNING, - message: 'CloudFront Distribution should enable access logging', - logical_resource_ids: %w[rDistribution2], - line_numbers: [46] - ), - Violation.new( - id: 'W70', type: Violation::WARNING, - message: 'Cloudfront should use minimum protocol version TLS 1.2', - logical_resource_ids: ["rDistribution1", "rDistribution2"], - line_numbers: [4,46] - ), - Violation.new( - id: 'W51', type: Violation::WARNING, - message: 'S3 bucket should likely have a bucket policy', - logical_resource_ids: %w[S3Bucket], - line_numbers: [81] - ) + CloudFrontDistributionAccessLoggingRule.new.violation(%w[rDistribution2], [46]), + CloudfrontMinimumProtocolVersionRule.new.violation(["rDistribution1", "rDistribution2"], [4,46]), + MissingBucketPolicyRule.new.violation(%w[S3Bucket], [81]) ] } } diff --git a/spec/cfn_nag_integration/cfn_nag_ec2_instance_spec.rb b/spec/cfn_nag_integration/cfn_nag_ec2_instance_spec.rb index c1ec9e1b..d111b992 100644 --- a/spec/cfn_nag_integration/cfn_nag_ec2_instance_spec.rb +++ b/spec/cfn_nag_integration/cfn_nag_ec2_instance_spec.rb @@ -19,12 +19,7 @@ file_results: { failure_count: 0, violations: [ - Violation.new( - id: 'W1', type: Violation::WARNING, - message: 'Specifying credentials in the template itself is probably not the safest thing', - logical_resource_ids: %w[EC2I4LBA1], - line_numbers: [11] - ) + CloudFormationAuthenticationRule.new.violation(%w[EC2I4LBA1], [11]) ] } } diff --git a/spec/cfn_nag_integration/cfn_nag_ec2_volume_spec.rb b/spec/cfn_nag_integration/cfn_nag_ec2_volume_spec.rb index 7f26196d..c05bae2d 100644 --- a/spec/cfn_nag_integration/cfn_nag_ec2_volume_spec.rb +++ b/spec/cfn_nag_integration/cfn_nag_ec2_volume_spec.rb @@ -18,20 +18,8 @@ file_results: { failure_count: 2, violations: [ - Violation.new( - id: 'W37', type: Violation::WARNING, - message: - 'EBS Volume should specify a KmsKeyId value', - logical_resource_ids: %w[NewVolume1 NewVolume2], - line_numbers: [4, 13] - ), - Violation.new( - id: 'F1', type: Violation::FAILING_VIOLATION, - message: - 'EBS volume should have server-side encryption enabled', - logical_resource_ids: %w[NewVolume1 NewVolume2], - line_numbers: [4, 13] - ) + EbsVolumeEncryptionKeyRule.new.violation(%w[NewVolume1 NewVolume2], [4, 13]), + EbsVolumeHasSseRule.new.violation(%w[NewVolume1 NewVolume2], [4, 13]) ] } } @@ -54,13 +42,7 @@ file_results: { failure_count: 0, violations: [ - Violation.new( - id: 'W37', type: Violation::WARNING, - message: - 'EBS Volume should specify a KmsKeyId value', - logical_resource_ids: %w[NewVolume], - line_numbers: [4] - ) + EbsVolumeEncryptionKeyRule.new.violation(%w[NewVolume], [4]) ] } } diff --git a/spec/cfn_nag_integration/cfn_nag_elasticloadbalancing_loadbalancer_spec.rb b/spec/cfn_nag_integration/cfn_nag_elasticloadbalancing_loadbalancer_spec.rb index fe7a6aa9..5d1ad425 100644 --- a/spec/cfn_nag_integration/cfn_nag_elasticloadbalancing_loadbalancer_spec.rb +++ b/spec/cfn_nag_integration/cfn_nag_elasticloadbalancing_loadbalancer_spec.rb @@ -18,13 +18,7 @@ file_results: { failure_count: 0, violations: [ - Violation.new( - id: 'W26', type: Violation::WARNING, - message: - 'Elastic Load Balancer should have access logging enabled', - logical_resource_ids: %w[elb1 elb2], - line_numbers: [4, 19] - ) + ElasticLoadBalancerAccessLoggingRule.new.violation(%w[elb1 elb2], [4, 19]) ] } } diff --git a/spec/cfn_nag_integration/cfn_nag_iam_role_spec.rb b/spec/cfn_nag_integration/cfn_nag_iam_role_spec.rb index bac7f018..02b8a117 100644 --- a/spec/cfn_nag_integration/cfn_nag_iam_role_spec.rb +++ b/spec/cfn_nag_integration/cfn_nag_iam_role_spec.rb @@ -13,11 +13,7 @@ template_name = 'yaml/iam_role/embedded_ref.yml' expected_violations = [ - Violation.new(id: 'W11', - type: Violation::WARNING, - message: 'IAM role should not allow * resource on its permissions policy', - logical_resource_ids: %w[HelperRole], - line_numbers: [7]) + IamRoleWildcardResourceOnPermissionsPolicyRule.new.violation(%w[HelperRole], [7]) ] actual_violations = @cfn_nag.audit( diff --git a/spec/cfn_nag_integration/cfn_nag_iam_user_spec.rb b/spec/cfn_nag_integration/cfn_nag_iam_user_spec.rb index 022da7f4..251d00f0 100644 --- a/spec/cfn_nag_integration/cfn_nag_iam_user_spec.rb +++ b/spec/cfn_nag_integration/cfn_nag_iam_user_spec.rb @@ -19,11 +19,7 @@ # only increment this when Violation::FAILING (vs WARNING) failure_count: 1, violations: [ - Violation.new(id: 'F2000', - type: Violation::FAILING_VIOLATION, - message: 'User is not assigned to a group', - logical_resource_ids: %w[myuser2], - line_numbers: [4]) + UserMissingGroupRule.new.violation(%w[myuser2], [4]) ] } } diff --git a/spec/cfn_nag_integration/cfn_nag_lambda_permission_spec.rb b/spec/cfn_nag_integration/cfn_nag_lambda_permission_spec.rb index 62bbd0a6..54d28019 100644 --- a/spec/cfn_nag_integration/cfn_nag_lambda_permission_spec.rb +++ b/spec/cfn_nag_integration/cfn_nag_lambda_permission_spec.rb @@ -18,31 +18,11 @@ file_results: { failure_count: 3, violations: [ - Violation.new(id: 'F38', - type: Violation::FAILING_VIOLATION, - message: 'IAM role should not allow * resource with PassRole action on its permissions policy', - logical_resource_ids: %w[LambdaExecutionRole], - line_numbers: [50]), - Violation.new(id: 'F3', - type: Violation::FAILING_VIOLATION, - message: 'IAM role should not allow * action on its permissions policy', - logical_resource_ids: %w[LambdaExecutionRole], - line_numbers: [50]), - Violation.new(id: 'W11', - type: Violation::WARNING, - message: 'IAM role should not allow * resource on its permissions policy', - logical_resource_ids: %w[LambdaExecutionRole], - line_numbers: [50]), - Violation.new(id: 'W89', - type: Violation::WARNING, - message: 'Lambda functions should be deployed inside a VPC', - logical_resource_ids: %w[AppendItemToListFunction], - line_numbers: [4]), - Violation.new(id: 'F13', - type: Violation::FAILING_VIOLATION, - message: 'Lambda permission principal should not be wildcard', - logical_resource_ids: %w[lambdaPermission], - line_numbers: [24]) + IamRolePassRoleWildcardResourceRule.new.violation(%w[LambdaExecutionRole], [50]), + IamRoleWildcardActionOnPermissionsPolicyRule.new.violation(%w[LambdaExecutionRole], [50]), + IamRoleWildcardResourceOnPermissionsPolicyRule.new.violation(%w[LambdaExecutionRole], [50]), + LambdaFunctionInsideVPCRule.new.violation(%w[AppendItemToListFunction], [4]), + LambdaPermissionWildcardPrincipalRule.new.violation(%w[lambdaPermission], [24]) ] } } diff --git a/spec/cfn_nag_integration/cfn_nag_rds_instance_spec.rb b/spec/cfn_nag_integration/cfn_nag_rds_instance_spec.rb index dfbd6635..db9160f5 100644 --- a/spec/cfn_nag_integration/cfn_nag_rds_instance_spec.rb +++ b/spec/cfn_nag_integration/cfn_nag_rds_instance_spec.rb @@ -18,12 +18,7 @@ file_results: { failure_count: 1, violations: [ - Violation.new(id: 'F22', - type: Violation::FAILING_VIOLATION, - message: - 'RDS instance should not be publicly accessible', - logical_resource_ids: %w[PublicDB], - line_numbers: [4]) + RDSInstancePubliclyAccessibleRule.new.violation(%w[PublicDB], [4]) ] } } @@ -44,18 +39,8 @@ file_results: { failure_count: 2, violations: [ - Violation.new( - id: 'F23', type: Violation::FAILING_VIOLATION, - message: 'RDS instance master user password must not be a plaintext string or a Ref to a Parameter with a Default value. Can be Ref to a NoEcho Parameter without a Default, or a dynamic reference to a secretsmanager/ssm-secure value.', - logical_resource_ids: %w[BadDb2], - line_numbers: [11] - ), - Violation.new(id: 'F22', - type: Violation::FAILING_VIOLATION, - message: - 'RDS instance should not be publicly accessible', - logical_resource_ids: %w[BadDb2], - line_numbers: [11]) + RDSDBInstanceMasterUserPasswordRule.new.violation(%w[BadDb2], [11]), + RDSInstancePubliclyAccessibleRule.new.violation(%w[BadDb2], [11]) ] } } @@ -77,18 +62,8 @@ file_results: { failure_count: 4, violations: [ - Violation.new( - id: 'F23', type: Violation::FAILING_VIOLATION, - message: 'RDS instance master user password must not be a plaintext string or a Ref to a Parameter with a Default value. Can be Ref to a NoEcho Parameter without a Default, or a dynamic reference to a secretsmanager/ssm-secure value.', - logical_resource_ids: %w[BadDb1 BadDb2], - line_numbers: [14, 30] - ), - Violation.new( - id: 'F24', type: Violation::FAILING_VIOLATION, - message: 'RDS instance master username must not be a plaintext string or a Ref to a Parameter with a Default value. Can be Ref to a NoEcho Parameter without a Default, or a dynamic reference to a secretsmanager value.', - logical_resource_ids: %w[BadDb1 BadDb2], - line_numbers: [14, 30] - ) + RDSDBInstanceMasterUserPasswordRule.new.violation(%w[BadDb1 BadDb2], [14, 30]), + RDSDBInstanceMasterUsernameRule.new.violation(%w[BadDb1 BadDb2], [14, 30]) ] } } diff --git a/spec/cfn_nag_integration/cfn_nag_s3_bucket_policy_spec.rb b/spec/cfn_nag_integration/cfn_nag_s3_bucket_policy_spec.rb index 0984a709..3d11419e 100644 --- a/spec/cfn_nag_integration/cfn_nag_s3_bucket_policy_spec.rb +++ b/spec/cfn_nag_integration/cfn_nag_s3_bucket_policy_spec.rb @@ -18,18 +18,8 @@ file_results: { failure_count: 3, violations: [ - Violation.new(id: 'F15', - type: Violation::FAILING_VIOLATION, - message: - 'S3 Bucket policy should not allow * action', - logical_resource_ids: %w[S3BucketPolicy S3BucketPolicy2], - line_numbers: [61, 86]), - Violation.new(id: 'F16', - type: Violation::FAILING_VIOLATION, - message: - 'S3 Bucket policy should not allow * principal', - logical_resource_ids: %w[S3BucketPolicy2], - line_numbers: [86]) + S3BucketPolicyWildcardActionRule.new.violation(%w[S3BucketPolicy S3BucketPolicy2], [61, 86]), + S3BucketPolicyWildcardPrincipalRule.new.violation(%w[S3BucketPolicy2], [86]) ] } } diff --git a/spec/cfn_nag_integration/cfn_nag_s3_bucket_spec.rb b/spec/cfn_nag_integration/cfn_nag_s3_bucket_spec.rb index 16193f1d..36ea576a 100644 --- a/spec/cfn_nag_integration/cfn_nag_s3_bucket_spec.rb +++ b/spec/cfn_nag_integration/cfn_nag_s3_bucket_spec.rb @@ -18,24 +18,9 @@ file_results: { failure_count: 1, violations: [ - Violation.new( - id: 'W51', type: Violation::WARNING, - message: 'S3 bucket should likely have a bucket policy', - logical_resource_ids: %w[S3BucketRead S3BucketReadWrite], - line_numbers: [4, 24] - ), - Violation.new( - id: 'W31', type: Violation::WARNING, - message: 'S3 Bucket likely should not have a public read acl', - logical_resource_ids: %w[S3BucketRead], - line_numbers: [4] - ), - Violation.new(id: 'F14', - type: Violation::FAILING_VIOLATION, - message: - 'S3 Bucket should not have a public read-write acl', - logical_resource_ids: %w[S3BucketReadWrite], - line_numbers: [24]) + MissingBucketPolicyRule.new.violation(%w[S3BucketRead S3BucketReadWrite], [4, 24]), + S3BucketPublicReadAclRule.new.violation(%w[S3BucketRead], [4]), + S3BucketPublicReadWriteAclRule.new.violation(%w[S3BucketReadWrite], [24]) ] } } diff --git a/spec/cfn_nag_integration/cfn_nag_security_group_spec.rb b/spec/cfn_nag_integration/cfn_nag_security_group_spec.rb index 13d25b1f..93635d98 100644 --- a/spec/cfn_nag_integration/cfn_nag_security_group_spec.rb +++ b/spec/cfn_nag_integration/cfn_nag_security_group_spec.rb @@ -20,10 +20,7 @@ file_results: { failure_count: 1, violations: [ - Violation.new(id: 'FATAL', - type: Violation::FAILING_VIOLATION, - message: "Basic CloudFormation syntax error:[#]", - logical_resource_ids: []) + Violation.fatal_violation("Basic CloudFormation syntax error:[#]") ] } } @@ -48,12 +45,7 @@ file_results: { failure_count: 1, violations: [ - Violation.new( - id: 'F1000', type: Violation::FAILING_VIOLATION, - message: 'Missing egress rule means all traffic is allowed outbound. Make this explicit if it is desired configuration', - logical_resource_ids: %w[sg], - line_numbers: [4] - ) + SecurityGroupMissingEgressRule.new.violation(%w[sg], [4]) ] } } @@ -75,30 +67,10 @@ file_results: { failure_count: 2, violations: [ - Violation.new( - id: 'W9', type: Violation::WARNING, - message: 'Security Groups found with ingress cidr that is not /32', - logical_resource_ids: %w[sg2], - line_numbers: [18] - ), - Violation.new( - id: 'W2', type: Violation::WARNING, - message: 'Security Groups found with cidr open to world on ingress. This should never be true on instance. Permissible on ELB', - logical_resource_ids: %w[sg2], - line_numbers: [18] - ), - Violation.new( - id: 'W27', type: Violation::WARNING, - message: 'Security Groups found ingress with port range instead of just a single port', - logical_resource_ids: %w[sg sg2], - line_numbers: [4, 18] - ), - Violation.new( - id: 'F1000', type: Violation::FAILING_VIOLATION, - message: 'Missing egress rule means all traffic is allowed outbound. Make this explicit if it is desired configuration', - logical_resource_ids: %w[sg sg2], - line_numbers: [4, 18] - ) + SecurityGroupIngressCidrNon32Rule.new.violation(%w[sg2], [18]), + SecurityGroupIngressOpenToWorldRule.new.violation(%w[sg2], [18]), + SecurityGroupIngressPortRangeRule.new.violation(%w[sg sg2], [4, 18]), + SecurityGroupMissingEgressRule.new.violation(%w[sg sg2], [4, 18]) ] } } @@ -144,13 +116,7 @@ file_results: { failure_count: 0, violations: [ - Violation.new( - id: 'W9', type: Violation::WARNING, - message: - 'Security Groups found with ingress cidr that is not /32', - logical_resource_ids: %w[sg], - line_numbers: [9] - ) + SecurityGroupIngressCidrNon32Rule.new.violation(%w[sg], [9]) ] } } @@ -174,13 +140,7 @@ file_results: { failure_count: 0, violations: [ - Violation.new( - id: 'W9', type: Violation::WARNING, - message: - 'Security Groups found with ingress cidr that is not /32', - logical_resource_ids: %w[sg sg2], - line_numbers: [9, 30] - ) + SecurityGroupIngressCidrNon32Rule.new.violation(%w[sg sg2], [9, 30]) ] } } diff --git a/spec/cfn_nag_integration/cfn_nag_serverless_transform_spec.rb b/spec/cfn_nag_integration/cfn_nag_serverless_transform_spec.rb index 55b43ed9..cb46465b 100644 --- a/spec/cfn_nag_integration/cfn_nag_serverless_transform_spec.rb +++ b/spec/cfn_nag_integration/cfn_nag_serverless_transform_spec.rb @@ -21,24 +21,9 @@ file_results: { failure_count: 0, violations: [ - Violation.new( - id: 'W58', type: Violation::WARNING, - message: LambdaFunctionCloudWatchLogsRule.new.rule_text, - logical_resource_ids: %w[SomeFunction2], - line_numbers: [34] - ), - Violation.new( - id: 'W89', type: Violation::WARNING, - message: LambdaFunctionInsideVPCRule.new.rule_text, - logical_resource_ids: ["SomeFunction", "SomeFunction2"], - line_numbers: [20, 34] - ), - Violation.new( - id: 'W92', type: Violation::WARNING, - message: LambdaFunctionReservedConcurrentExecutionsRule.new.rule_text, - logical_resource_ids: ["SomeFunction","SomeFunction2"], - line_numbers: [20, 34] - ) + LambdaFunctionCloudWatchLogsRule.new.violation(%w[SomeFunction2], [34]), + LambdaFunctionInsideVPCRule.new.violation(["SomeFunction", "SomeFunction2"], [20, 34]), + LambdaFunctionReservedConcurrentExecutionsRule.new.violation(["SomeFunction","SomeFunction2"], [20, 34]) ] } } diff --git a/spec/cfn_nag_integration/cfn_nag_sns_policy_spec.rb b/spec/cfn_nag_integration/cfn_nag_sns_policy_spec.rb index 820a4276..e081def1 100644 --- a/spec/cfn_nag_integration/cfn_nag_sns_policy_spec.rb +++ b/spec/cfn_nag_integration/cfn_nag_sns_policy_spec.rb @@ -20,13 +20,7 @@ # only increment this when Violation::FAILING (vs WARNING) failure_count: 4, violations: [ - Violation.new( - id: 'F18', type: Violation::FAILING_VIOLATION, - message: 'SNS topic policy should not allow * principal', - logical_resource_ids: %w[mysnspolicy0 mysnspolicy1 - mysnspolicy2 mysnspolicy3], - line_numbers: [11, 29, 55, 85] - ) + SnsTopicPolicyWildcardPrincipalRule.new.violation(%w[mysnspolicy0 mysnspolicy1 mysnspolicy2 mysnspolicy3], [11, 29, 55, 85]) ] } } diff --git a/spec/cfn_nag_integration/cfn_nag_sqs_policy_spec.rb b/spec/cfn_nag_integration/cfn_nag_sqs_policy_spec.rb index 9b18b2d2..8473cb9f 100644 --- a/spec/cfn_nag_integration/cfn_nag_sqs_policy_spec.rb +++ b/spec/cfn_nag_integration/cfn_nag_sqs_policy_spec.rb @@ -19,13 +19,7 @@ # only increment this when Violation::FAILING (vs WARNING) failure_count: 0, violations: [ - Violation.new( - id: 'W18', type: Violation::WARNING, - message: 'SQS Queue policy should not allow Allow+NotAction', - logical_resource_ids: %w[QueuePolicyWithNotAction - QueuePolicyWithNotAction2], - line_numbers: [20, 37] - ) + SqsQueuePolicyNotActionRule.new.violation(%w[QueuePolicyWithNotAction QueuePolicyWithNotAction2], [20, 37]) ] } } diff --git a/spec/cfn_nag_spec.rb b/spec/cfn_nag_spec.rb index 60c9ce0a..79f29a23 100644 --- a/spec/cfn_nag_spec.rb +++ b/spec/cfn_nag_spec.rb @@ -93,10 +93,7 @@ file_results: { failure_count: 1, violations: [ - Violation.new(id: 'FATAL', - type: Violation::FAILING_VIOLATION, - message: 'Illegal cfn - no Resources', - logical_resource_ids: []) + Violation.fatal_violation('Illegal cfn - no Resources') ] } } @@ -120,10 +117,7 @@ file_results: { failure_count: 1, violations: [ - Violation.new(id: 'FATAL', - type: Violation::FAILING_VIOLATION, - message: 'Illegal cfn - no Resources', - logical_resource_ids: []) + Violation.fatal_violation('Illegal cfn - no Resources') ] } } @@ -147,12 +141,7 @@ file_results: { failure_count: 1, violations: [ - Violation.new( - id: 'F16', type: Violation::FAILING_VIOLATION, - message: 'S3 Bucket policy should not allow * principal', - logical_resource_ids: %w[S3BucketPolicy2], - line_numbers: [86] - ) + S3BucketPolicyWildcardPrincipalRule.new.violation(%w[S3BucketPolicy2], [86]) ] } } @@ -201,24 +190,9 @@ def rule_id file_results: { failure_count: 1, violations: [ - Violation.new( - id: 'W9', type: Violation::WARNING, - message: 'Security Groups found with ingress cidr that is not /32', - logical_resource_ids: %w[sgOpenIngress], - line_numbers: [4] - ), - Violation.new( - id: 'W2', type: Violation::WARNING, - message: 'Security Groups found with cidr open to world on ingress. This should never be true on instance. Permissible on ELB', - logical_resource_ids: %w[sgOpenIngress], - line_numbers: [4] - ), - Violation.new( - id: 'F1000', type: Violation::FAILING_VIOLATION, - message: 'Missing egress rule means all traffic is allowed outbound. Make this explicit if it is desired configuration', - logical_resource_ids: %w[sgOpenIngress], - line_numbers: [4] - ) + SecurityGroupIngressCidrNon32Rule.new.violation(%w[sgOpenIngress], [4]), + SecurityGroupIngressOpenToWorldRule.new.violation(%w[sgOpenIngress], [4]), + SecurityGroupMissingEgressRule.new.violation(%w[sgOpenIngress], [4]) ] } } @@ -253,24 +227,9 @@ def rule_id file_results: { failure_count: 2, violations: [ - Violation.new( - id: 'W9', type: Violation::WARNING, - message: 'Security Groups found with ingress cidr that is not /32', - logical_resource_ids: %w[sgOpenIngress sgOpenIngress2], - line_numbers: [4, 21] - ), - Violation.new( - id: 'W2', type: Violation::WARNING, - message: 'Security Groups found with cidr open to world on ingress. This should never be true on instance. Permissible on ELB', - logical_resource_ids: %w[sgOpenIngress sgOpenIngress2], - line_numbers: [4, 21] - ), - Violation.new( - id: 'F1000', type: Violation::FAILING_VIOLATION, - message: 'Missing egress rule means all traffic is allowed outbound. Make this explicit if it is desired configuration', - logical_resource_ids: %w[sgOpenIngress sgOpenIngress2], - line_numbers: [4, 21] - ) + SecurityGroupIngressCidrNon32Rule.new.violation(%w[sgOpenIngress sgOpenIngress2], [4, 21]), + SecurityGroupIngressOpenToWorldRule.new.violation(%w[sgOpenIngress sgOpenIngress2], [4, 21]), + SecurityGroupMissingEgressRule.new.violation(%w[sgOpenIngress sgOpenIngress2], [4, 21]) ] } } @@ -307,11 +266,7 @@ def rule_id file_results: { failure_count: 1, violations: [ - Violation.new( - id: 'FATAL', - type: Violation::FAILING_VIOLATION, - message: '(): mapping values are not allowed in this context at line 3 column 15' - ) + Violation.fatal_violation('(): mapping values are not allowed in this context at line 3 column 15') ] } } diff --git a/spec/custom_rule_loader_spec.rb b/spec/custom_rule_loader_spec.rb index 858c6277..f37a6b63 100644 --- a/spec/custom_rule_loader_spec.rb +++ b/spec/custom_rule_loader_spec.rb @@ -60,6 +60,7 @@ def audit_impl(cfn_model) actual_violations = @file_base_rule_repo.execute_custom_rules cfn_model, @file_base_rule_repo.rule_definitions expected_violations = [ Violation.new(id: 'W9933', + name: 'FakeRule', type: Violation::WARNING, message: 'this is fake rule text', logical_resource_ids: %w[hardwired1 hardwired2]) diff --git a/spec/custom_rules/base_spec.rb b/spec/custom_rules/base_spec.rb index 01e8a53e..b37e1e02 100644 --- a/spec/custom_rules/base_spec.rb +++ b/spec/custom_rules/base_spec.rb @@ -46,10 +46,7 @@ def rule_text dontcare = double('cfn_model') - expected_violation = Violation.new(id: 'F3333', - type: Violation::FAILING_VIOLATION, - message: 'This is an epic fail!', - logical_resource_ids: %w[r1 r2 r3]) + expected_violation = base_rule.violation(%w[r1 r2 r3]) expect(base_rule.audit(dontcare)).to eq expected_violation end diff --git a/spec/custom_rules/boolean_base_rule_spec.rb b/spec/custom_rules/boolean_base_rule_spec.rb index f94a49ec..7cf8693f 100644 --- a/spec/custom_rules/boolean_base_rule_spec.rb +++ b/spec/custom_rules/boolean_base_rule_spec.rb @@ -39,10 +39,7 @@ def boolean_property cfn_model = CfnParser.new.parse read_test_template 'json/efs/filesystem_with_encryption_false.json' - expected_violation = Violation.new(id: 'F3333', - type: Violation::FAILING_VIOLATION, - message: 'This is an epic fail!', - logical_resource_ids: %w[filesystem]) + expected_violation = base_rule.violation(%w[filesystem]) expect(base_rule.audit(cfn_model)).to eq expected_violation end diff --git a/spec/custom_rules/password_base_rule_spec.rb b/spec/custom_rules/password_base_rule_spec.rb index 4e54c685..08edd06f 100644 --- a/spec/custom_rules/password_base_rule_spec.rb +++ b/spec/custom_rules/password_base_rule_spec.rb @@ -30,10 +30,7 @@ def password_property end before(:all) do - @failing_violation = Violation.new(id: 'F3333', - type: Violation::FAILING_VIOLATION, - message: 'This is an epic fail!', - logical_resource_ids: %w[RedshiftCluster]) + @failing_violation = @base_rule.violation(%w[RedshiftCluster]) end it 'raises an error when properties are not set' do diff --git a/spec/custom_rules/resource_base_rule_spec.rb b/spec/custom_rules/resource_base_rule_spec.rb index fe8aefd7..3c045f84 100644 --- a/spec/custom_rules/resource_base_rule_spec.rb +++ b/spec/custom_rules/resource_base_rule_spec.rb @@ -34,10 +34,7 @@ def resource_type cfn_model = CfnParser.new.parse read_test_template 'yaml/amazon_simpledb/simpledb_domain_resource.yml' - expected_violation = Violation.new(id: 'F3333', - type: Violation::FAILING_VIOLATION, - message: 'This is an epic fail!', - logical_resource_ids: %w[NewSimpleDB]) + expected_violation = base_rule.violation(%w[NewSimpleDB]) expect(base_rule.audit(cfn_model)).to eq expected_violation end diff --git a/spec/custom_rules/sub_property_with_list_password_base_rule_spec.rb b/spec/custom_rules/sub_property_with_list_password_base_rule_spec.rb index 593eef93..5ea65882 100644 --- a/spec/custom_rules/sub_property_with_list_password_base_rule_spec.rb +++ b/spec/custom_rules/sub_property_with_list_password_base_rule_spec.rb @@ -34,10 +34,7 @@ def sub_property_name end before(:all) do - @failing_violation = Violation.new(id: 'F3333', - type: Violation::FAILING_VIOLATION, - message: 'This is an epic fail!', - logical_resource_ids: %w[OpsWorksStack]) + @failing_violation = @base_rule_with_list.violation(%w[OpsWorksStack]) end it 'raises an error when properties are not set' do diff --git a/spec/end_to_end/cfn_nag/version_spec.rb b/spec/end_to_end/cfn_nag/version_spec.rb index f67204c2..4c1c1e3f 100644 --- a/spec/end_to_end/cfn_nag/version_spec.rb +++ b/spec/end_to_end/cfn_nag/version_spec.rb @@ -2,9 +2,9 @@ describe 'cfn_nag --version', end_to_end: true do context 'when ensuring the local gem is installed' do - it 'equals 0.0.01' do + it 'equals 0.0.0' do expect { system %( cfn_nag --version ) } - .to output(a_string_matching('0.0.01')) + .to output(a_string_matching('0.0.0')) .to_stdout_from_any_process end end diff --git a/spec/end_to_end/cfn_nag_rules/version_spec.rb b/spec/end_to_end/cfn_nag_rules/version_spec.rb index c7198164..b87fb9ac 100644 --- a/spec/end_to_end/cfn_nag_rules/version_spec.rb +++ b/spec/end_to_end/cfn_nag_rules/version_spec.rb @@ -4,7 +4,7 @@ context 'when ensuring the local gem is installed' do it 'equals 0.0.01' do expect { system %( cfn_nag_rules --version ) } - .to output(a_string_matching('0.0.01')) + .to output(a_string_matching('0.0.0')) .to_stdout_from_any_process end end diff --git a/spec/end_to_end/cfn_nag_scan/version_spec.rb b/spec/end_to_end/cfn_nag_scan/version_spec.rb index 6e83c001..0e14d0a0 100644 --- a/spec/end_to_end/cfn_nag_scan/version_spec.rb +++ b/spec/end_to_end/cfn_nag_scan/version_spec.rb @@ -4,7 +4,7 @@ context 'when ensuring the local gem is installed' do it 'equals 0.0.01' do expect { system %( cfn_nag_scan --version ) } - .to output(a_string_matching('0.0.01')) + .to output(a_string_matching('0.0.0')) .to_stdout_from_any_process end end diff --git a/spec/result_view/sarif_results_spec.rb b/spec/result_view/sarif_results_spec.rb new file mode 100644 index 00000000..4078fcfd --- /dev/null +++ b/spec/result_view/sarif_results_spec.rb @@ -0,0 +1,176 @@ +require 'spec_helper' +require 'cfn-nag/cfn_nag' + +describe SarifResults do + # Method of suppressing stderr and stdout was found on StackOverflow here: + # https://stackoverflow.com/a/22777806 + original_stderr = nil + original_stdout = nil + + before(:all) do + original_stderr = $stderr # capture previous value of $stderr + original_stdout = $stdout # capture previous value of $stdout + $stderr = StringIO.new # assign a string buffer to $stderr + $stdout = StringIO.new # assign a string buffer to $stdout + # $stderr.string # return the contents of the string buffer if needed + # $stdout.string # return the contents of the string buffer if needed + end + + after(:all) do + $stderr = original_stderr # restore $stderr to its previous value + $stdout = original_stdout # restore $stdout to its previous value + end + + describe '#driver' do + context 'with four rules' do + before(:each) do + @rules = [ + RuleDefinition.new(id: 'F1', name: 'Fail1', type: RuleDefinition::FAILING_VIOLATION, message: 'Bad Security Setting'), + RuleDefinition.new(id: 'F2', name: 'Fail2', type: RuleDefinition::FAILING_VIOLATION, message: 'Missing Security Setting'), + RuleDefinition.new(id: 'F3', name: 'Fail3', type: RuleDefinition::FAILING_VIOLATION, message: 'Invalid Security Setting'), + RuleDefinition.new(id: 'F4', name: 'Fail4', type: RuleDefinition::FAILING_VIOLATION, message: 'Default Security Setting') + ] + end + + it 'should contain name, informationUri, semanticVersion and rules attributes' do + sarif_driver = SarifResults.new.driver(@rules) + + expect(sarif_driver).to include(:name, :informationUri, :semanticVersion, :rules) + expect(sarif_driver[:name]).to eq('cfn_nag') + expect(sarif_driver[:informationUri]).to eq('https://github.com/stelligent/cfn_nag') + end + + it 'should contain a driver with four rules' do + sarif_driver = SarifResults.new.driver(@rules) + + expect(sarif_driver[:rules].length).to eq(4) + end + + it 'should contain a rule with id, name, fullDescription' do + sarif_driver = SarifResults.new.driver(@rules) + + expect(sarif_driver[:rules].first).to include(:id, :name, :fullDescription) + end + end + end + + describe '#sarif_result' do + context 'with single entry violation' do + before(:each) do + @violation = S3BucketPolicyWildcardActionRule.new.violation(['resource1'], [54]) + end + + it 'should return result object with ruleId, level, message and location attributes' do + sarif_results = SarifResults.new.sarif_result(file_name: 'bob.txt', violation: @violation, index: 0) + + expect(sarif_results).to include(:ruleId, :level, :message, :locations) + expect(sarif_results[:ruleId]).to eq("CFN_NAG_#{@violation.id}") + expect(sarif_results[:level]).to eq('error') + expect(sarif_results[:message][:text]).to eq(@violation.message) + end + + it 'should return one location with physical and logical sections' do + sarif_results = SarifResults.new.sarif_result(file_name: 'bob.txt', violation: @violation, index: 0) + + expect(sarif_results[:locations].length).to eq(1) + + location = sarif_results[:locations].first + + expect(location).to include(:physicalLocation, :logicalLocations) + + expect(location[:physicalLocation]).to include(:artifactLocation, :region) + expect(location[:physicalLocation][:artifactLocation]).to include(:uri) + expect(location[:physicalLocation][:artifactLocation][:uri]).to eq('bob.txt') + expect(location[:physicalLocation][:region][:startLine]).to eq(54) + + expect(location[:logicalLocations].length).to eq(1) + expect(location[:logicalLocations].first).to include(:name) + expect(location[:logicalLocations].first[:name]).to eq('resource1') + end + end + + context 'with multiple entry violation' do + before(:each) do + @violation = S3BucketPolicyWildcardActionRule.new.violation(['resource1', 'resource2'], [54, 1039]) + end + + it 'should return result object with ruleId, level, message and location attributes' do + sarif_results = SarifResults.new.sarif_result(file_name: 'bob.txt', violation: @violation, index: 1) + + expect(sarif_results).to include(:ruleId, :level, :message, :locations) + expect(sarif_results[:ruleId]).to eq("CFN_NAG_#{@violation.id}") + expect(sarif_results[:level]).to eq('error') + expect(sarif_results[:message][:text]).to eq(@violation.message) + end + + it 'should return one location with physical and logical sections' do + sarif_results = SarifResults.new.sarif_result(file_name: 'bob.txt', violation: @violation, index: 1) + + expect(sarif_results[:locations].length).to eq(1) + + location = sarif_results[:locations].first + + expect(location).to include(:physicalLocation, :logicalLocations) + + expect(location[:physicalLocation]).to include(:artifactLocation, :region) + expect(location[:physicalLocation][:artifactLocation]).to include(:uri) + expect(location[:physicalLocation][:artifactLocation][:uri]).to eq('bob.txt') + expect(location[:physicalLocation][:region][:startLine]).to eq(1039) + + expect(location[:logicalLocations].length).to eq(1) + expect(location[:logicalLocations].first).to include(:name) + expect(location[:logicalLocations].first[:name]).to eq('resource2') + end + end + end + + describe '#sarif_level' do + it 'should return warning type for warning rules' do + expect(SarifResults.new.sarif_level(RuleDefinition::WARNING)).to eq('warning') + end + it 'should return error type for failing rules' do + expect(SarifResults.new.sarif_level(RuleDefinition::FAILING_VIOLATION)).to eq('error') + end + + it 'should return error type for other input' do + expect(SarifResults.new.sarif_level(nil)).to eq('error') + expect(SarifResults.new.sarif_level('bob')).to eq('error') + end + end + + describe '#relative_path' do + it 'should return relative path for absolute path' do + expect(SarifResults.new.relative_path("#{Dir.pwd}/spec/test_templates/yaml/vulgar_bad_syntax.yml")).to eq('spec/test_templates/yaml/vulgar_bad_syntax.yml') + end + it 'should return relative path for relative path' do + expect(SarifResults.new.relative_path('spec/test_templates/yaml/vulgar_bad_syntax.yml')).to eq('spec/test_templates/yaml/vulgar_bad_syntax.yml') + end + end + + describe '#sarif_line_number' do + context 'with nil line number' do + it 'should return 1' do + expect(SarifResults.new.sarif_line_number(nil)).to eq(1) + end + end + context 'with empty line number' do + it 'should return 1' do + expect(SarifResults.new.sarif_line_number('')).to eq(1) + end + end + context 'with line number less than 1' do + it 'should return 1' do + expect(SarifResults.new.sarif_line_number(0)).to eq(1) + expect(SarifResults.new.sarif_line_number(-1)).to eq(1) + expect(SarifResults.new.sarif_line_number('-1')).to eq(1) + end + end + context 'with positive line number' do + it 'should return line number' do + expect(SarifResults.new.sarif_line_number(1)).to eq(1) + expect(SarifResults.new.sarif_line_number('10')).to eq(10) + expect(SarifResults.new.sarif_line_number(100)).to eq(100) + end + end + end +end \ No newline at end of file diff --git a/spec/rule_definition_spec.rb b/spec/rule_definition_spec.rb index c7fbc2ec..d1b3f0e2 100644 --- a/spec/rule_definition_spec.rb +++ b/spec/rule_definition_spec.rb @@ -5,6 +5,7 @@ it 'raises an error' do expect do RuleDefinition.new(id: nil, + name: 'BadRule', type: RuleDefinition::FAILING_VIOLATION, message: 'EBS volume should have server-side encryption enabled') end.to raise_error 'No parameters to Violation constructor can be nil' @@ -14,9 +15,10 @@ describe '#to_s' do it 'emits attributes' do violation = RuleDefinition.new(id: 'F99', + name: 'F9Rule', type: RuleDefinition::FAILING_VIOLATION, message: 'EBS volume should have server-side encryption enabled') - expect(violation.to_s).to eq 'F99 FAIL EBS volume should have server-side encryption enabled' + expect(violation.to_s).to eq 'F99 F9Rule FAIL EBS volume should have server-side encryption enabled' end end @@ -24,10 +26,12 @@ context 'unequal' do it 'return false' do v1 = RuleDefinition.new(id: 'F1', + name: 'F1Rule', type: RuleDefinition::FAILING_VIOLATION, message: 'EBS volume should have server-side encryption enabled') v2 = RuleDefinition.new(id: 'F2', + name: 'F2Rule', type: RuleDefinition::FAILING_VIOLATION, message: 'EBS volume should have server-side encryption enabled') @@ -38,10 +42,12 @@ context 'equal' do it 'return true' do v1 = RuleDefinition.new(id: 'F1', + name: 'F1Rule', type: RuleDefinition::FAILING_VIOLATION, message: 'EBS volume should have server-side encryption enabled') v2 = RuleDefinition.new(id: 'F1', + name: 'F1Rule', type: RuleDefinition::FAILING_VIOLATION, message: 'EBS volume should have server-side encryption enabled') diff --git a/spec/rule_dumper_spec.rb b/spec/rule_dumper_spec.rb index 6e9d9eed..0721a76b 100644 --- a/spec/rule_dumper_spec.rb +++ b/spec/rule_dumper_spec.rb @@ -69,11 +69,13 @@ [ { "id": "F1", + "name": "EbsVolumeHasSseRule", "type": "FAIL", "message": "EBS volume should have server-side encryption enabled" }, { "id": "F9", + "name": "S3BucketPolicyNotPrincipalRule", "type": "FAIL", "message": "S3 Bucket policy should not allow Allow+NotPrincipal" } diff --git a/spec/rule_registry_spec.rb b/spec/rule_registry_spec.rb index 6ef8676a..b83f59aa 100644 --- a/spec/rule_registry_spec.rb +++ b/spec/rule_registry_spec.rb @@ -68,6 +68,7 @@ def rule_id expected_rules = [ RuleDefinition.new(id: 'F444', + name: 'F444Rule', type: RuleDefinition::WARNING, message: 'you have been warned!!') ] @@ -108,6 +109,7 @@ def rule_id context 'good id' do it 'returns the rule def' do expected_rule = RuleDefinition.new(id: 'W444', + name: 'W444Rule', type: RuleDefinition::WARNING, message: 'you have been warned!!') @@ -120,6 +122,7 @@ def rule_id it 'returns all/only warnings' do expected_rules = [ RuleDefinition.new(id: 'W444', + name: 'W444Rule', type: RuleDefinition::WARNING, message: 'you have been warned!!') ] @@ -131,6 +134,7 @@ def rule_id it 'returns all/only failures' do expected_rules = [ RuleDefinition.new(id: 'F999', + name: 'F999Rule', type: RuleDefinition::FAILING_VIOLATION, message: 'you have failed!!') ] diff --git a/spec/rules_repos/file_base_rule_repo_spec.rb b/spec/rules_repos/file_base_rule_repo_spec.rb index 38d28e02..525a756e 100644 --- a/spec/rules_repos/file_base_rule_repo_spec.rb +++ b/spec/rules_repos/file_base_rule_repo_spec.rb @@ -46,6 +46,7 @@ def audit_impl(cfn_model) # Validate that the rule was loaded by id expected_rule_definition = RuleDefinition.new id: 'W9933', + name: 'ValidCustomRule', message: 'this is fake rule text', type: RuleDefinition::WARNING @@ -74,6 +75,7 @@ def audit_impl(cfn_model) # Validate that the rule was loaded by id expected_rule_definition = RuleDefinition.new id: 'W9933', + name: 'ValidCustomRule', message: 'this is fake rule text', type: RuleDefinition::WARNING diff --git a/spec/violation_filtering_spec.rb b/spec/violation_filtering_spec.rb index d5cf5d42..a36e808a 100644 --- a/spec/violation_filtering_spec.rb +++ b/spec/violation_filtering_spec.rb @@ -4,26 +4,18 @@ require 'cfn-nag/rule_id_set' require 'cfn-nag/profile_loader' require 'cfn-nag/deny_list_loader' +require 'cfn-nag/custom_rules/EbsVolumeHasSseRule' +require 'cfn-nag/custom_rules/SecurityGroupMissingEgressRule' +require 'cfn-nag/custom_rules/SnsTopicPolicyWildcardPrincipalRule' include ViolationFiltering describe ViolationFiltering do before(:all) do @violations = [ - Violation.new(id: 'F1', - type: Violation::FAILING_VIOLATION, - message: 'EBS volume should have server-side encryption enabled', - logical_resource_ids: %w[NewVolume1 NewVolume2]), - - Violation.new(id: 'F2', - type: Violation::FAILING_VIOLATION, - message: 'EBS volume should have server-side encryption enabled2', - logical_resource_ids: %w[NewVolume1 NewVolume2]), - - Violation.new(id: 'F3', - type: Violation::FAILING_VIOLATION, - message: 'EBS volume should have server-side encryption enabled3', - logical_resource_ids: %w[NewVolume1 NewVolume2]) + EbsVolumeHasSseRule.new.violation(%w[NewVolume1 NewVolume2]), + SecurityGroupMissingEgressRule.new.violation(%w[sgOpenIngress sgOpenIngress2]), + SnsTopicPolicyWildcardPrincipalRule.new.violation(%w[topic1 topic2]) ] end @@ -61,7 +53,7 @@ it 'returns violations X,Y' do profile = RuleIdSet.new profile.add_rule 'F1' - profile.add_rule 'F2' + profile.add_rule 'F1000' mocked_profile_loader = double('profile_loader') expect(ProfileLoader).to receive(:new).and_return(mocked_profile_loader) @@ -70,15 +62,8 @@ profile_definition = 'dontcare' dontcare = nil expected_violations = [ - Violation.new(id: 'F1', - type: Violation::FAILING_VIOLATION, - message: 'EBS volume should have server-side encryption enabled', - logical_resource_ids: %w[NewVolume1 NewVolume2]), - - Violation.new(id: 'F2', - type: Violation::FAILING_VIOLATION, - message: 'EBS volume should have server-side encryption enabled2', - logical_resource_ids: %w[NewVolume1 NewVolume2]) + EbsVolumeHasSseRule.new.violation(%w[NewVolume1 NewVolume2]), + SecurityGroupMissingEgressRule.new.violation(%w[sgOpenIngress sgOpenIngress2]), ] actual_violations = filter_violations_by_profile( profile_definition: profile_definition, @@ -92,8 +77,8 @@ context 'deny list with Y,Z' do it 'returns violations X' do deny_list = RuleIdSet.new - deny_list.add_rule 'F2' - deny_list.add_rule 'F3' + deny_list.add_rule 'F1000' + deny_list.add_rule 'F18' mocked_deny_list_loader = double('deny_list_loader') expect(DenyListLoader).to receive(:new).and_return(mocked_deny_list_loader) @@ -102,10 +87,7 @@ deny_list_definition = 'dontcare' dontcare = nil expected_violations = [ - Violation.new(id: 'F1', - type: Violation::FAILING_VIOLATION, - message: 'EBS volume should have server-side encryption enabled', - logical_resource_ids: %w[NewVolume1 NewVolume2]) + EbsVolumeHasSseRule.new.violation(%w[NewVolume1 NewVolume2]) ] actual_violations = filter_violations_by_deny_list( deny_list_definition: deny_list_definition, diff --git a/spec/violation_spec.rb b/spec/violation_spec.rb index b4ad6fed..a12c02c9 100644 --- a/spec/violation_spec.rb +++ b/spec/violation_spec.rb @@ -4,10 +4,11 @@ describe '#to_s' do it 'emits attributes' do violation = Violation.new(id: 'F99', + name: 'MyRule', type: Violation::FAILING_VIOLATION, message: 'EBS volume should have server-side encryption enabled', logical_resource_ids: %w[NewVolume1 NewVolume2]) - expect(violation.to_s).to eq 'F99 FAIL EBS volume should have server-side encryption enabled ["NewVolume1", "NewVolume2"]' + expect(violation.to_s).to eq 'F99 MyRule FAIL EBS volume should have server-side encryption enabled ["NewVolume1", "NewVolume2"]' end end @@ -15,11 +16,13 @@ context 'unequal' do it 'return false' do v1 = Violation.new(id: 'F1', + name: 'MyRule', type: Violation::FAILING_VIOLATION, message: 'EBS volume should have server-side encryption enabled', logical_resource_ids: %w[NewVolume1 NewVolume2]) v2 = Violation.new(id: 'F2', + name: 'MyRule2', type: Violation::FAILING_VIOLATION, message: 'EBS volume should have server-side encryption enabled', logical_resource_ids: %w[NewVolume1 NewVolume2]) @@ -31,11 +34,13 @@ context 'equal' do it 'return true' do v1 = Violation.new(id: 'F1', + name: 'MyRule', type: Violation::FAILING_VIOLATION, message: 'EBS volume should have server-side encryption enabled', logical_resource_ids: %w[NewVolume1 NewVolume2]) v2 = Violation.new(id: 'F1', + name: 'MyRule', type: Violation::FAILING_VIOLATION, message: 'EBS volume should have server-side encryption enabled', logical_resource_ids: %w[NewVolume1 NewVolume2]) @@ -50,10 +55,12 @@ it 'returns 0' do violations = [ Violation.new(id: 'F1', + name: 'MyRule', type: Violation::FAILING_VIOLATION, message: 'EBS volume should have server-side encryption enabled', logical_resource_ids: %w[NewVolume1 NewVolume2]), Violation.new(id: 'F2', + name: 'MyRule2', type: Violation::FAILING_VIOLATION, message: 'Bark at the moon', logical_resource_ids: %w[NewVolume2 NewVolume3]) @@ -66,18 +73,22 @@ it 'returns 3' do violations = [ Violation.new(id: 'F1', + name: 'MyRule', type: Violation::FAILING_VIOLATION, message: 'EBS volume should have server-side encryption enabled', logical_resource_ids: %w[NewVolume1 NewVolume2]), Violation.new(id: 'F2', + name: 'MyRule2', type: Violation::FAILING_VIOLATION, message: 'Bark at the moon', logical_resource_ids: %w[NewVolume2 NewVolume3]), Violation.new(id: 'W2', + name: 'MyRule3', type: Violation::WARNING, message: 'Moo at the moon', logical_resource_ids: %w[NewVolume2 NewVolume3]), Violation.new(id: 'W3', + name: 'MyRule4', type: Violation::WARNING, message: 'Moo at the moon2', logical_resource_ids: %w[NewVolume3]) @@ -91,6 +102,7 @@ it 'returns 1' do violations = [ Violation.new(id: 'W3', + name: 'MyRule', type: Violation::WARNING, message: 'Moo at the moon2', logical_resource_ids: []) @@ -106,10 +118,12 @@ it 'returns 0' do violations = [ Violation.new(id: 'W1', + name: 'MyRule', type: Violation::WARNING, message: 'Bark at the moon', logical_resource_ids: %w[NewVolume2 NewVolume3]), Violation.new(id: 'W2', + name: 'MyRule2', type: Violation::WARNING, message: 'Moo at the moon', logical_resource_ids: %w[NewVolume2 NewVolume3]) @@ -122,14 +136,17 @@ it 'returns 4' do violations = [ Violation.new(id: 'F1', + name: 'MyRule', type: Violation::FAILING_VIOLATION, message: 'EBS volume should have server-side encryption enabled', logical_resource_ids: %w[NewVolume1 NewVolume2]), Violation.new(id: 'F2', + name: 'MyRule2', type: Violation::FAILING_VIOLATION, message: 'Bark at the moon', logical_resource_ids: %w[NewVolume2 NewVolume3]), Violation.new(id: 'W2', + name: 'MyRule3', type: Violation::WARNING, message: 'Moo at the moon', logical_resource_ids: %w[NewVolume2 NewVolume3]) @@ -142,6 +159,7 @@ it 'returns 1' do violations = [ Violation.new(id: 'F3', + name: 'MyRule', type: Violation::FAILING_VIOLATION, message: 'Moo at the moon2', logical_resource_ids: []) @@ -151,4 +169,15 @@ end end end + describe '#fatal_violation' do + context 'generates fatal violation' do + it 'returns violation' do + fatal_violation = Violation.fatal_violation('Bad Moon') + + expect(fatal_violation.id).to eq 'FATAL' + expect(fatal_violation.name).to eq 'system' + expect(fatal_violation.type).to eq Violation::FAILING_VIOLATION + end + end + end end