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

New check for S3 bucket visibility #253

Merged
merged 14 commits into from Jan 20, 2018

Conversation

rhussmann
Copy link
Contributor

@rhussmann rhussmann commented Oct 31, 2017

General

  • Update Changelog following the conventions laid out here

  • Update README with any necessary configuration snippets

  • [?] Binstubs are created if needed

  • RuboCop passes

  • Existing tests pass

New Plugins

  • Tests

  • Add the plugin to the README

  • Does it have a complete header as outlined here

Purpose

Adding a new check: check-s3-bucket-visibility.rb. This check is to ensure buckets you assume are not publicly exposed aren't.

Checks a bucket to ensure:

  • there is no website configuration for the bucket, and
  • the bucket policy has no Get*, List* or* statements.

Objects within a bucket can have their own ACL, and IAM roles can set object permissions as well. This check is intended to be a best-effort check to ensure a bucket is not exposed.

Known Compatibility Issues

None.

@rhussmann rhussmann changed the title S3 bucket visibility New check for S3 bucket visibility Oct 31, 2017
Copy link
Collaborator

@eheydrick eheydrick left a comment

Choose a reason for hiding this comment

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

Hi @rhussmann, thank you for your contribution to sensu-plugins! I think this is a great addition and really useful in light of recent issues with exposed buckets. I left some comments mostly about authentication. Can you also include a testing artifact in the PR description - something that shows the plugin working. Would be great to show the output of a critical bucket and an ok bucket. Thanks again!

long: '--aws-secret-access-key AWS_SECRET_KEY',
description: "AWS Secret Access Key. Either set ENV['AWS_SECRET_KEY'] or provide it as an option",
default: ENV['AWS_SECRET_KEY']

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you remove the key and secret options. We're working on removing support for those options because it is insecure to pass credentials on the command line. Instead we support the methods supported by the aws-sdk and document the recommended methods here.

short: '-u',
long: '--use-iam',
description: 'Use IAM role authenticiation. Instance must have IAM role assigned for this to work'

Copy link
Collaborator

Choose a reason for hiding this comment

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

This option isn't needed as the aws-sdk will automatically use an IAM role if there is one assigned to the instance.

short: '-t AWS_SESSION_TOKEN',
long: '--aws-session-token TOKEN',
description: 'AWS Session Token',
default: ENV['AWS_SESSION_TOKEN']
Copy link
Collaborator

Choose a reason for hiding this comment

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

How are you using this? Just wondering since none of the other checks have this option and I'd prefer to be consistent on authentication.

secret_access_key: config[:aws_secret_access_key],
session_token: config[:session_token],
region: config[:aws_region] }
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of this can you include Common and also require sensu-plugins-aws. That way it's consistent with how the other checks here are configured. See https://github.com/sensu-plugins/sensu-plugins-aws/blob/master/bin/check-alb-target-group-health.rb#L38 for example.

aws_config[:access_key_id] = config[:aws_access_key]
aws_config[:secret_access_key] = config[:aws_secret_access_key]
aws_config[:session_token] = config[:aws_session_token]
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can go away when Common is added.

@rhussmann
Copy link
Contributor Author

@eheydrick Thanks for the review. Will act on your suggestions ASAP.

Copy link
Member

@majormoses majormoses left a comment

Choose a reason for hiding this comment

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

This is fantastic! I was contemplating writing the same check myself. Aside from the previous comments we should allow checking multiple buckets. I have over a dozen buckets per environment and would like to run 1 check per environment to reduce the cpu load.

description: 'AWS Session Token',
default: ENV['AWS_SESSION_TOKEN']

option :bucket_name,
Copy link
Member

Choose a reason for hiding this comment

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

might want to include multiple bucket names to avoid needing instantiate multiple checks for multiple buckets. Something like a comma separated list of buckets seems appropriate. See #24 , #27 for more details as to why.

begin
errors = []
if website_configuration?(s3, config[:bucket_name])
errors.push 'Website configuration found'
Copy link
Member

Choose a reason for hiding this comment

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

we should include the bucket name as we ideally should support multiple buckets for a single check.

@rhussmann
Copy link
Contributor Author

@eheydrick @majormoses Sorry for the long delay. I believe I've addressed all your feedback. Please re-review.

Copy link
Member

@majormoses majormoses left a comment

Choose a reason for hiding this comment

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

Overall this looks good, just one more tweak to make the bucket_names option a bit more accurate. After this I am 👍


option :bucket_name,
short: '-b BUCKET_NAME',
long: '--bucket-name',
Copy link
Member

Choose a reason for hiding this comment

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

lets use --bucket-names to indicate is supports multiple buckets.

description: 'AWS Region (defaults to us-east-1).',
default: 'us-east-1'

option :bucket_name,
Copy link
Member

Choose a reason for hiding this comment

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

lets use bucket_names to indicate is supports multiple buckets.


def run
errors = []
buckets = config[:bucket_name].split ','
Copy link
Member

Choose a reason for hiding this comment

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

config[:bucket_names] to imply it is multiple buckets.

ok "#{buckets.join ','} not exposed via website or bucket policy"
end
rescue Aws::S3::Errors::NotFound => _
critical "Bucket #{config[:bucket_name]} not found"
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we want to return the whole list of buckets just the one that failed. Feel like this should be bucket_name and likely should really go within the each do.

Copy link
Member

Choose a reason for hiding this comment

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

I thought about this a bit more and I think the status for a bucket not found should be configurable. I think some people might want to consider it a warning in a dev or qa environment. If one of my devs blows up a non production bucket I would not want to be woken up. If someone deleted a bucket in production that is another story.

* Missing buckets are treated as warnings by default
@rhussmann
Copy link
Contributor Author

rhussmann commented Jan 20, 2018

@majormoses Bucket name config option updated and other feedback incorporated. Please re-review at your convenience.

@rhussmann
Copy link
Contributor Author

rhussmann commented Jan 20, 2018

As for the missing bucket config option, I added a new -m flag that will default to warn rather than raise critical.

@majormoses
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants