cc @jseabold and @stmarier (who helped me figure this out)

Current coverage is 52.39%

Merging #2126 into master will not affect coverage as of 75f5d0a

@@            master   #2126   diff @@
======================================
  Files           49      49       
  Stmts         6505    6505       
  Branches         0       0       
  Methods          0       0       
======================================
  Hit           3408    3408       
  Partial          0       0       
  Missed        3097    3097       

Review entire Coverage Diff as of 75f5d0a

Powered by Codecov. Updated on successful CI builds.

Overriding the default security here doesn't quite feel right to me. I'd like to make sure there aren't any other alternatives first...

Have to admit I've never actually had a need to use the s3:// protocol. With the right s3 bucket policies, I've always just used http or https. Preferably now https, since AWS is signing certs for free. Way easier than securely managing AWS key pair creds or even ec2 instance profiles. Convince me that I'm wrong though...

I think @stmarier understands this a lot better than I do, but I don't believe this validate argument is security-related. From my limited understanding, it's so you can do things like add an exception handler to create the bucket if it doesn't exist. Also, boto3 effectively has validate=False (docs).

For our particular use case, I think it's easier to use S3 with key pairs than https. But yeah, I agree with you that the https protocol is more generally useful.

I think @mheilman summed it up pretty well. To my knowledge there's nothing inherently secure or insecure about either approach.

Perhaps it might be more appropriate to move the try/except to the key = bucket.get_key(key_string) line, since we would be using validate=False, and then check for S3 errors there. I don't think any functionality would be lost, then.

It's not security related. If you pass validate=True, then boto runs a query on the bucket before creating the object it returns to you. If validate-False, it just returns the object. Relevant part of the boto code: https://github.com/boto/boto/blob/2.39.0/boto/s3/connection.py#L465L550

When setting validate=False, you should also remove bucket = conn.get_bucket(bucket_name, validate=False) from the try/except, since it would now never trigger an exception.

Got it. @mheilman could you amend the PR with @mheilman's suggestion?

Perhaps it might be more appropriate to move the try/except to the key = bucket.get_key(key_string) line, since we would be using validate=False, and then check for S3 errors there. I don't think any functionality would be lost, then.

The inspection completed: 2 updated code elements

Hi there, thank you for your contribution to Conda!

This pull request has been automatically locked since it has not had recent activity after it was closed.

Please open a new issue or pull request if needed.