Added a check when forbidden is returned from S3 bucket

[mdupras]

…sts in a S3 Bucket.

Prerequisites

• I have written a descriptive pull-request title
• I have verified that there are no overlapping pull-requests open
• I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
• I have provided test coverage for my change (where applicable)

Description

We don't need the ListBucket permission when accessing the S3 bucket. We will always know the key we want to access, but the side effect is we are getting a Forbidden from the S3 bucket, probably to prevent bucket enumeration. I think it's safe to assume that the Key doesn't exist.

[codecov]

Codecov Report

Merging #282 (4f3dcbc) into main (2b1450d) will decrease coverage by 0%.
The diff coverage is 0%.

@@         Coverage Diff         @@
##           main   #282   +/-   ##
===================================
- Coverage    85%    85%   -1%     
===================================
  Files        75     75           
  Lines      2045   2047    +2     
  Branches    298    299    +1     
===================================
- Hits       1744   1741    -3     
- Misses      217    221    +4     
- Partials     84     85    +1     

Flag	Coverage Δ
unittests	85% <0%> (-1%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...oviders.AWS/Providers/AWSS3StorageImageProvider.cs	67% <0%> (-3%)	⬇️
...ching/LruCache/ConcurrentTLruCache{TKey,TValue}.cs	43% <0%> (-3%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b1450d...4f3dcbc. Read the comment docs.

[JimBobSquarePants]

Thanks for this. Is there any way we can test it?

[mdupras]

Thanks for this. Is there any way we can test it?

Yea! If the app is only using those permissions over the S3 bucket :

{
    "Action": [
        "s3:GetObject",
        "s3:GetBucketLocation"
    ],
    "Effect": "Allow",
    "Resource": [
        "arn:aws:s3:::BUCKETNAME/*",
        "arn:aws:s3:::BUCKETNAME"
    ],
    "Sid": ""
}

I figured it didn't really need to list the content of the bucket.

[JimBobSquarePants]

Ah, I meant in our unit tests. If not I'll just trust you. I have no way to test and little experience with S3.

[mdupras]

Ahh I'm sorry, I didn't think it was possible with the current setup.

I have just validated it and I'm getting this error : A parameter you provided implies functionality that is not implemented when I try to set a policy on the bucket. At the very least, s3rver would need to have some permissions support and I don't think it has any right now.

If you want to test the PR, it would be outside the CI, with a real bucket. I can make you some terraform files for a repro env if you want, but you would need a working AWS Account.

[JimBobSquarePants]

No worries. Looks like you've covered everything. Unfortunately the test strategy for AWS isn't fully comprehensive.