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

#2810 - Add check if aws s3 bucket is encrypted. #2937

Merged
merged 3 commits into from
May 3, 2018

Conversation

UranusBytes
Copy link
Contributor

@UranusBytes UranusBytes commented Apr 10, 2018

Fixes #2810 - Add check if aws s3 bucket is encrypted.

Required terraform aws provider >= 1.6

Signed-off-by: Jeremy Phillips github@uranusbytes.com

@UranusBytes UranusBytes requested a review from a team as a code owner April 10, 2018 11:37
@UranusBytes UranusBytes force-pushed the 2810 branch 2 times, most recently from 4083dcb to 6072c99 Compare April 10, 2018 15:41
Required terraform aws provider >= 1.6
Fix indentation issue in aws_s3_bucket.rb

Signed-off-by: Jeremy Phillips <github@uranusbytes.com>
@clintoncwolfe
Copy link
Contributor

clintoncwolfe commented Apr 10, 2018

Was Blocked by #2884, now unblocked

Copy link
Contributor

@TrevorBramble TrevorBramble left a comment

Choose a reason for hiding this comment

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

Hi, thanks for your contribution!

Please consider the following suggestions.


def get_bucket_encryption(query)
buckets = {
'public' => OpenStruct.new({ server_side_encryption_configuration: OpenStruct.new({ rules: [] }) })
Copy link
Contributor

Choose a reason for hiding this comment

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

You can omit these curly braces and lose some of the noise here. OpenStruct.new(server_side_encryption_configuration: OpenStruct.new(rules: []))

buckets = {
'public' => OpenStruct.new({ server_side_encryption_configuration: OpenStruct.new({ rules: [] }) })
}
if query[:bucket].eql?'private'
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a space before the 'private' argument, please. (Or wrap it with parentheses.)

unless buckets.key?(query[:bucket])
raise Aws::S3::Errors::NoSuchBucket.new(nil, nil)
end
buckets[query[:bucket]]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think putting these three cases into an actual case statement would make the purpose of this method more clear.

def get_bucket_encryption(query)
  buckets = {
    'public' => OpenStruct.new(server_side_encryption_configuration: OpenStruct.new(rules: []))
  }

  case
  when query[:bucket].eql? 'private'
    raise Aws::S3::Errors::ServerSideEncryptionConfigurationNotFoundError.new(nil, nil)
  when !buckets.key?(query[:bucket])
    raise Aws::S3::Errors::NoSuchBucket.new(nil, nil)
  else
    buckets[query[:bucket]]
  end
end

In writing that, I realized the second and third cases are actually just a missing use of fetch(), which I think is even better than the case approach.

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.fetch(query[:bucket]) { raise Aws::S3::Errors::NoSuchBucket.new(nil, nil) }
end

@has_default_encryption_enabled = false
end
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

A few things here. =^)

Line 100 is hard to read for how long it is. Going vertical with those method calls off BackendFactory would make it more readable.

We can do a single assignment to @has_default_encryption_enabled from whatever you return in the catch_aws_errors block.

def fetch_bucket_encryption_configuration
  @has_default_encryption_configuration ||= 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

server_side_encryption_configuration {
rule {
apply_server_side_encryption_by_default {
sse_algorithm = "aws:kms"
Copy link
Contributor

Choose a reason for hiding this comment

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

[nitpick] Extra spacing between sse_algorithm and =.

@@ -35,6 +35,11 @@ def public?
bucket_policy.any? { |s| s.effect == 'Allow' && s.principal == '*' }
end

def has_default_encryption_enabled?
return unless @exists
Copy link
Contributor

Choose a reason for hiding this comment

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

[nitpick] Predicate methods should only ever return explicit true or false values. So return false unless @exists would be the more accurate guard statement.

@clintoncwolfe clintoncwolfe added Type: New Feature Adds new functionality Platform: AWS Amazon Web Services-related issues labels Apr 11, 2018
…other methods to align with recommendations (except Terraform nitpick; preference is to keep coding style consistent until full refactor).

Signed-off-by: Jeremy Phillips <github@uranusbytes.com>
@jquick
Copy link
Contributor

jquick commented Apr 19, 2018

Hey @UranusBytes, we merged in the new terraform pins and you will need to rebase your branch. Sorry about that.

Copy link
Contributor

@TrevorBramble TrevorBramble left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

@jquick jquick left a comment

Choose a reason for hiding this comment

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

Thanks @UranusBytes

@jquick jquick merged commit 1407e68 into inspec:master May 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Platform: AWS Amazon Web Services-related issues Type: New Feature Adds new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants