Plugin needlessly requires ListBucketVersions permission

[ghicks-rmn]

    def check_apikeys
      @bucket.empty?
    rescue AWS::S3::Errors::NoSuchBucket
      # ignore NoSuchBucket Error because ensure_bucket checks it.
    rescue => e
      raise "can't call S3 API. Please check your aws_key_id / aws_sec_key or s3_region configuration. error = #{e.inspect}"
    end

This code attempts to run "bucket.empty?" after initializing an S3 connection, ostensibly to check whether the plugin can successfully access the bucket.

What bucket.empty? actually does is perform a ListBucketVersions call, and returns a boolean depending on whether there were any records in the response. As a result, the plugin effectively requires permissions to run ListBucketVersions, even though this permission has nothing to do with the basic task of uploading messages to S3.

Moreover, there's nothing actually in the documentation (or in the code) to indicate that this permission is required. To discover it, I had to open the plugin source, add debug statements, trace my way through to find the failing line, then check the Ruby SDK source to find out what the method was actually trying to do.

The plugin should be updated to simply print error messages to the log when it fails to upload a file due to permissions. Or, at the very least, the docs should explicitly say that the ListBucketVersions permission is required for the plugin to work at all.

[ijin]

I don't know if I would consider it needlessly. That's just the way the aws sdk implements bucket availability, as you've discovered. But thanks for digging through and pointing this out.

Pull requests are always welcome. :grinning:

[ijin]

Looks like I answered this some time ago. https://github.com/fluent/fluent-plugin-s3/issues/34 (Code has changed, so not everything might be fully relevant)

[ghicks-rmn]

First, a correction/clarification (edit: of my first message above): The ListBucketVersions permission is only required if check_apikey_on_start is true. It looks like the SDK returns true if bucket.exists? fails due to AccessDenied, which is ultimately the behavior I want - if it can't tell whether the bucket exists, it should assume that it does and try to write to it. I haven't tested but it looks like ListBucketVersionswould be required for the plugin to actually determine whether the bucket exists before attempting to create it - maybe it would be better to simply attempt to create it without even checking, and note in the log if it fails due to AccessDenied or the bucket already existing, as that would not require any additional permissions.

For check_apikey_on_start though: given that ListBucket is required for the plugin to work in the first place, doesn't it make more sense to test the API keys using ListBucket, rather than requiring an unrelated permission?

I'll see if I have time to make a pull request for these changes - if I do I'll try to add some info about permissions requirements in README.md as well.

[ijin]

Yes, I understand that ListBucketVersions is required for check_apikey_on_start, as I have pointed out in #34.

AFAIK, bucket.exists? returns true if an AccessDenied is returned (meaning the existence is verified by a denial error), so it's not using permissions at all. I believe what you're describing is covered by ensure_bucket.

Note: This method only indicates if there is a bucket in S3, not if you have permissions to work with the bucket or not." http://docs.aws.amazon.com/AWSRubySDK/latest/AWS/S3/Bucket.html#exists%3F-instance_method

The context check_apikeys was introduced is this thread #27 - To check the validity of API keys.

As you mentioned, it would be great if we can limit the checking (the validity) of API keys with just ListBucket, but I am not aware of any methods in AWS::S3::Bucket of the ruby sdk that does this. Perhaps there is. bucket.empty?checks deleted versioned objects so ListBucketVersions is necessary.

I'm open for improvements (without breaking backwards compatibility).

[riquito]

An improvement would be to add it in the documentation, I had an hard time to understand what was happening ("access denied, check your aws key/secret/region" sidetracked me). Maybe a list of the IAM permissions required.

[repeatedly]

It seems good idea.

@ijin example on https://github.com/fluent/fluent-plugin-s3/issues/34#issuecomment-26567229 is minimal configuration? Should we write more example like MultiUpload?

[jcerjak]

Judging from another comment on that issue, this commit message and another comment, it looks like more permissions might be needed.

It would be very beneficial to have a definitive answer on this added to documentation.

[et304383]

Can you at least list the required IAM permissions in the README? This is madness.

I completely disagree with needing any permissions other than PutObject.

[github-actions[bot]]

This issue has been automatically marked as stale because it has been open 90 days with no activity. Remove stale label or comment or this issue will be closed in 30 days

[github-actions[bot]]

This issue has been automatically marked as stale because it has been open 90 days with no activity. Remove stale label or comment or this issue will be closed in 30 days

[ashie]

Add enhancement label to avoid marking stale (we should add more appropriate label).