Add markers encryption_sse_c and encryption_sse_kms #553
Conversation
atta
commented
Mar 1, 2024
atta
commented
- testing of sse-c is now possible without a configured kms
- add header 'X-Forwarded-Proto' = 'https' if is_secure is set to False in the configuration file
* testing of sse-c is now possible without a configured kms * add header 'X-Forwarded-Proto' = 'https' if is_secure is set to False in the configuration file
|
Hi, please let me know if there is anything to change, would love to get it done |
|
Hey @cbodley or @mattbenjamin - would you mind taking a look at this PR? Thank you :) |
| pip install pylint | ||
| - name: Analysing the code with pylint | ||
| run: | | ||
| pylint $(git ls-files '*.py') |
| encryption_sse_c | ||
| encryption_sse_kms |
There was a problem hiding this comment.
there's already a marker for sse_s3. please remove the encryption_ part to be consistent
| @@ -5171,15 +5171,15 @@ def test_list_buckets_invalid_auth(): | |||
| e = assert_raises(ClientError, bad_auth_client.list_buckets) | |||
| status, error_code = _get_status_and_error_code(e.response) | |||
| assert status == 403 | |||
| assert error_code == 'InvalidAccessKeyId' | |||
| assert error_code == 'AccessDenied' | |||
There was a problem hiding this comment.
what's the motivation of this change? afaik this has been passing as-is
| if get_config_is_secure() == False: | ||
| sse_client_headers['X-Forwarded-Proto'] = 'https' |
There was a problem hiding this comment.
this shouldn't be necessary, and servers probably shouldn't accept this header from clients without additional configuration. if you're just testing against ceph rgw, you can set rgw_crypt_require_ssl=false to run these
There was a problem hiding this comment.
In my tests, if the rgw_crypt_require_ssl=false is set, the ceph rgw will require the header in order to accept the request.
There was a problem hiding this comment.
that config option has always worked in our automated testing, ex. https://github.com/ceph/ceph/blob/04416f4/qa/suites/rgw/crypt/3-rgw/rgw.yaml#L7
