Removed null encryption key vulnerability

[dillonjlee]

Hello! I really like this repo, managing S3 data like pathlib Path's is awesome. I'd like to use your s3path in an internal project. However, a security scan by Fortify revealed this one "vulnerability" so I wanted to contribute to its removal, so our security guys will be happy.

This is what the Fortify had to say about the offending line access_key = secret_key = token = None:

"Null encryption keys can compromise security in a way that is not easy to remedy.

Assigning None to encryption key variables is a bad idea because it can allow attackers to expose sensitive and encrypted information. Not only does using a null encryption key significantly reduce the protection afforded by a good encryption algorithm, but it also makes fixing the problem extremely difficult. After the offending code is in production, a software patch is required to change the null encryption key. If an account protected by the null encryption key is compromised, the owners of the system must choose between security and availability."

I will say, to get to this part in the code, you need to have python<3.12 and smart-open<5.1.0. The latter requirement is already blocked by your setup.py requirements, so if you're okay with more formally removing support for smart-open<5.1.0 (forcibly installing smart-open==5.0.0 causes 1 pytest failure on python 3.10 (test_open_method_with_custom_endpoint_url - AssertionError: assert 'https://s3.amazonaws.com' == 'http://localhost') and 21 on python 3.12 (all are TypeError: open() got an unexpected keyword argument 'compression')) I'm down to make an alternative PR removing this code support for <5.1.0 entirely. However, this proposed change maintains the exact same functionality as before.

I've ran the pytests with these changes and no extra errors occur asides from those inherent to smart-open==5.0.0 as mentioned. Other than with your moto tests not requiring credentials, I've otherwise tested on a bucket requiring credentials, and seen that you will get the same authentication error, whether 'aws_secret_access_key' etc. is to None (current), or not provided at all (this proposed change).

[liormizr]

Hi @dillonjlee
satisfying security guys is important so I'm all with you 😆

OK so lets add this change
Just so I'll understand, you want to add also in the setup.py requirements smart-open>5.1.0 instead of smart-open>=5.1.0?
If so I'm ok with it, just update your PR with this change and will add everything together

[dillonjlee]

Sounds great!

you want to add also in the setup.py requirements smart-open>5.1.0 instead of smart-open>=5.1.0?

No, I was wanting to remove the existence of _smart_open_old_version_kwargs entirely, because it's called like this on line 258:

if smart_open.__version__ >= '5.1.0':
           self._smart_open_new_version_kwargs(
               dummy_object,
               resource,
               config,
               transport_params,
               smart_open_kwargs)
       else:
           self._smart_open_old_version_kwargs(
               dummy_object,
               resource,
               config,
               transport_params,
               smart_open_kwargs)

If smart_open needs to be >= 5.1.0 for future versions of s3path (needed based on pytests), we can just always use "new" one, and remove "old" one. I understand if you are wanting to keep "old" for partial support for older versions though!

[liormizr]

@dillonjlee
make sense, go for it

[dillonjlee]

done