From 1407e681fcc8d584637decc752633014770a32f2 Mon Sep 17 00:00:00 2001 From: Jeremy Date: Thu, 3 May 2018 09:55:29 -0400 Subject: [PATCH] #2810 - Add check if aws s3 bucket is encrypted. (#2937) * Add check if aws s3 bucket is encrypted. Required terraform aws provider >= 1.6 Fix indentation issue in aws_s3_bucket.rb * Implement most changes recommended by @TrevorBramble, and refactored other methods to align with recommendations (except Terraform nitpick; preference is to keep coding style consistent until full refactor). Signed-off-by: Jeremy Phillips --- docs/resources/aws_s3_bucket.md.erb | 6 ++ lib/resources/aws/aws_s3_bucket.rb | 26 ++++- test/integration/aws/default/build/s3.tf | 26 +++++ .../default/verify/controls/aws_s3_bucket.rb | 12 ++- test/unit/resources/aws_s3_bucket_test.rb | 97 +++++++++++-------- 5 files changed, 122 insertions(+), 45 deletions(-) diff --git a/docs/resources/aws_s3_bucket.md.erb b/docs/resources/aws_s3_bucket.md.erb index ad17475d2..b1c3d8b74 100644 --- a/docs/resources/aws_s3_bucket.md.erb +++ b/docs/resources/aws_s3_bucket.md.erb @@ -132,3 +132,9 @@ Note: This resource does not detect insecure object ACLs. The `have_access_logging_enabled` matcher tests if access logging is enabled for the s3 bucket. it { should have_access_logging_enabled } + +### have\_default\_encryption\_enabled + +The `have_default_encryption_enabled` matcher tests if default encryption is enabled for the s3 bucket. + + it { should have_default_encryption_enabled } diff --git a/lib/resources/aws/aws_s3_bucket.rb b/lib/resources/aws/aws_s3_bucket.rb index c7ee0e6cc..c1786f9e9 100644 --- a/lib/resources/aws/aws_s3_bucket.rb +++ b/lib/resources/aws/aws_s3_bucket.rb @@ -10,7 +10,7 @@ class AwsS3Bucket < Inspec.resource(1) supports platform: 'aws' include AwsSingularResourceMixin - attr_reader :bucket_name, :has_access_logging_enabled, :region + attr_reader :bucket_name, :has_default_encryption_enabled, :has_access_logging_enabled, :region def to_s "S3 Bucket #{@bucket_name}" @@ -35,8 +35,13 @@ class AwsS3Bucket < Inspec.resource(1) bucket_policy.any? { |s| s.effect == 'Allow' && s.principal == '*' } end + def has_default_encryption_enabled? + return false unless @exists + @has_default_encryption_enabled ||= fetch_bucket_encryption_configuration + end + def has_access_logging_enabled? - return unless @exists + return false unless @exists catch_aws_errors do @has_access_logging_enabled ||= !BackendFactory.create(inspec_runner).get_bucket_logging(bucket: bucket_name).logging_enabled.nil? end @@ -89,6 +94,19 @@ class AwsS3Bucket < Inspec.resource(1) end end + def fetch_bucket_encryption_configuration + @has_default_encryption_enabled ||= catch_aws_errors do + begin + !BackendFactory.create(inspec_runner) + .get_bucket_encryption(bucket: bucket_name) + .server_side_encryption_configuration + .nil? + rescue Aws::S3::Errors::ServerSideEncryptionConfigurationNotFoundError + false + end + end + end + # Uses the SDK API to really talk to AWS class Backend class AwsClientApi < AwsBackendBase @@ -110,6 +128,10 @@ class AwsS3Bucket < Inspec.resource(1) def get_bucket_logging(query) aws_service_client.get_bucket_logging(query) end + + def get_bucket_encryption(query) + aws_service_client.get_bucket_encryption(query) + end end end end diff --git a/test/integration/aws/default/build/s3.tf b/test/integration/aws/default/build/s3.tf index c17fdd6f6..4cd7dc9d0 100644 --- a/test/integration/aws/default/build/s3.tf +++ b/test/integration/aws/default/build/s3.tf @@ -85,6 +85,32 @@ output "s3_bucket_access_logging_not_enabled_name" { value = "${aws_s3_bucket.acess_logging_not_enabled.id}" } +resource "aws_s3_bucket" "default_encryption_enabled" { + bucket = "inspec-testing-defencrypt-enabled-${terraform.env}.chef.io" + acl = "private" + + server_side_encryption_configuration { + rule { + apply_server_side_encryption_by_default { + sse_algorithm = "aws:kms" + } + } + } +} + +output "s3_bucket_default_encryption_enabled_name" { + value = "${aws_s3_bucket.default_encryption_enabled.id}" +} + +resource "aws_s3_bucket" "default_encryption_not_enabled" { + bucket = "inspec-testing-defencrypt-not-enabled-${terraform.env}.chef.io" + acl = "private" +} + +output "s3_bucket_default_encryption_not_enabled_name" { + value = "${aws_s3_bucket.default_encryption_not_enabled.id}" +} + #=================================================================# # S3 Bucket Policies #=================================================================# diff --git a/test/integration/aws/default/verify/controls/aws_s3_bucket.rb b/test/integration/aws/default/verify/controls/aws_s3_bucket.rb index b917cc88f..d78262f88 100644 --- a/test/integration/aws/default/verify/controls/aws_s3_bucket.rb +++ b/test/integration/aws/default/verify/controls/aws_s3_bucket.rb @@ -5,6 +5,8 @@ fixtures = {} 's3_bucket_auth_name', 's3_bucket_private_acl_public_policy_name', 's3_bucket_public_region', + 's3_bucket_default_encryption_enabled_name', + 's3_bucket_default_encryption_not_enabled_name', 's3_bucket_access_logging_enabled_name', 's3_bucket_access_logging_not_enabled_name', ].each do |fixture_name| @@ -113,7 +115,15 @@ control 'aws_s3_bucket matchers test' do it { should be_public } end - #----------------- have_access_logging_enabled -----------------# + #----------------- have_default_encryption_enabled -----------------# + describe aws_s3_bucket(bucket_name: fixtures['s3_bucket_default_encryption_enabled_name']) do + it { should have_default_encryption_enabled } + end + describe aws_s3_bucket(bucket_name: fixtures['s3_bucket_default_encryption_not_enabled_name']) do + it { should_not have_default_encryption_enabled } + end + + #----------------- have_access_logging_enabled -----------------# describe aws_s3_bucket(bucket_name: fixtures['s3_bucket_access_logging_enabled_name']) do it { should have_access_logging_enabled } end diff --git a/test/unit/resources/aws_s3_bucket_test.rb b/test/unit/resources/aws_s3_bucket_test.rb index 227728b32..7d627c94d 100644 --- a/test/unit/resources/aws_s3_bucket_test.rb +++ b/test/unit/resources/aws_s3_bucket_test.rb @@ -156,7 +156,15 @@ class AwsS3BucketPropertiesTest < Minitest::Test def test_has_access_logging_enabled_negative refute(AwsS3Bucket.new('private').has_access_logging_enabled?) end - + + def test_has_default_encryption_enabled_positive + assert(AwsS3Bucket.new('public').has_default_encryption_enabled?) + end + + def test_has_default_encryption_enabled_negative + refute(AwsS3Bucket.new('private').has_default_encryption_enabled?) + end + end #=============================================================================# @@ -166,60 +174,58 @@ end module AwsMSBSB class Basic < AwsBackendBase def get_bucket_acl(query) - owner_full_control = OpenStruct.new({ - grantee: OpenStruct.new({ + owner_full_control = OpenStruct.new( + grantee: OpenStruct.new( type: 'CanonicalUser', - }), + ), permission: 'FULL_CONTROL', - }) + ) buckets = { - 'public' => OpenStruct.new({ + 'public' => OpenStruct.new( :grants => [ owner_full_control, - OpenStruct.new({ - grantee: OpenStruct.new({ + OpenStruct.new( + grantee: OpenStruct.new( type: 'Group', uri: 'http://acs.amazonaws.com/groups/global/AllUsers' - }), + ), permission: 'READ', - }), + ), ] - }), - 'auth-users' => OpenStruct.new({ + ), + 'auth-users' => OpenStruct.new( :grants => [ owner_full_control, - OpenStruct.new({ - grantee: OpenStruct.new({ + OpenStruct.new( + grantee: OpenStruct.new( type: 'Group', uri: 'http://acs.amazonaws.com/groups/global/AuthenticatedUsers' - }), + ), permission: 'READ', - }), + ), ] - }), - 'private' => OpenStruct.new({ :grants => [ owner_full_control ] }), - 'private-acl-public-policy' => OpenStruct.new({ :grants => [ owner_full_control ] }), + ), + 'private' => OpenStruct.new(:grants => [ owner_full_control ]), + 'private-acl-public-policy' => OpenStruct.new(:grants => [ owner_full_control ]), } buckets[query[:bucket]] end def get_bucket_location(query) buckets = { - 'public' => OpenStruct.new({ location_constraint: 'us-east-2' }), - 'private' => OpenStruct.new({ location_constraint: 'EU' }), - 'auth-users' => OpenStruct.new({ location_constraint: 'ap-southeast-1' }), - 'private-acl-public-policy' => OpenStruct.new({ location_constraint: 'ap-southeast-2' }), + 'public' => OpenStruct.new(location_constraint: 'us-east-2'), + 'private' => OpenStruct.new(location_constraint: 'EU'), + 'auth-users' => OpenStruct.new(location_constraint: 'ap-southeast-1'), + 'private-acl-public-policy' => OpenStruct.new(location_constraint: 'ap-southeast-2'), } - unless buckets.key?(query[:bucket]) - raise Aws::S3::Errors::NoSuchBucket.new(nil, nil) - end - buckets[query[:bucket]] + + buckets.fetch(query[:bucket]) { raise Aws::S3::Errors::NoSuchBucket.new(nil, nil) } end def get_bucket_policy(query) buckets = { - 'public' => OpenStruct.new({ + 'public' => OpenStruct.new( policy: StringIO.new(<<'EOP') { "Version": "2012-10-17", @@ -234,8 +240,8 @@ module AwsMSBSB ] } EOP - }), - 'private' => OpenStruct.new({ + ), + 'private' => OpenStruct.new( policy: StringIO.new(<<'EOP') { "Version": "2012-10-17", @@ -250,8 +256,8 @@ EOP ] } EOP - }), - 'private-acl-public-policy' => OpenStruct.new({ + ), + 'private-acl-public-policy' => OpenStruct.new( policy: StringIO.new(<<'EOP') { "Version": "2012-10-17", @@ -266,24 +272,31 @@ EOP ] } EOP - }), + ), # No policies for auth bucket } - unless buckets.key?(query[:bucket]) - raise Aws::S3::Errors::NoSuchBucketPolicy.new(nil, nil) - end - buckets[query[:bucket]] + + buckets.fetch(query[:bucket]) { raise Aws::S3::Errors::NoSuchBucketPolicy.new(nil, nil) } end def get_bucket_logging(query) buckets = { - 'public' => OpenStruct.new({ logging_enabled: OpenStruct.new({ target_bucket: 'log-bucket' }) }), - 'private' => OpenStruct.new({ logging_enabled: nil }), + 'public' => OpenStruct.new(logging_enabled: OpenStruct.new(target_bucket: 'log-bucket')), + 'private' => OpenStruct.new(logging_enabled: nil ), } - unless buckets.key?(query[:bucket]) - raise Aws::S3::Errors::NoSuchBucket.new(nil, nil) + + buckets.fetch(query[:bucket]) { raise Aws::S3::Errors::NoSuchBucket.new(nil, nil) } + end + + def get_bucket_encryption(query) + buckets = { + 'public' => OpenStruct.new(server_side_encryption_configuration: OpenStruct.new(rules: [])) + } + if query[:bucket].eql? 'private' + raise Aws::S3::Errors::ServerSideEncryptionConfigurationNotFoundError.new(nil, nil) end - buckets[query[:bucket]] + + buckets.fetch(query[:bucket]) { raise Aws::S3::Errors::NoSuchBucket.new(nil, nil) } end end end