-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement opt-in unsigned requests for clients. #448
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ | |
from tests import unittest | ||
import mock | ||
|
||
import botocore | ||
from botocore import client | ||
from botocore import hooks | ||
from botocore.credentials import Credentials | ||
|
@@ -100,7 +101,7 @@ def test_client_signature_no_override(self, request_signer): | |
mock.ANY, mock.ANY, mock.ANY, 'v4', mock.ANY, mock.ANY) | ||
|
||
@mock.patch('botocore.client.RequestSigner') | ||
def test_client_signature_override(self, request_signer): | ||
def test_client_signature_override_config_file(self, request_signer): | ||
creator = self.create_client_creator() | ||
config = { | ||
'myservice': {'signature_version': 'foo'} | ||
|
@@ -111,6 +112,35 @@ def test_client_signature_override(self, request_signer): | |
request_signer.assert_called_with( | ||
mock.ANY, mock.ANY, mock.ANY, 'foo', mock.ANY, mock.ANY) | ||
|
||
@mock.patch('botocore.client.RequestSigner') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might not be possible, but can we add a test that's more "direct"? I see that this test is essentially checking that we forward the value of config.signature_version to the request signer, but it'd be great if we had something that verified, if you set the signature version to UNSIGNED, then we do not sign the request. I get there's another level of indirection here with the RequestSigner and the auth classes, but it seems possible to regress on this feature without any unit tests failing because we're never actually testing anything that explicitly sets the value to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll look into adding a more comprehensive test for this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So there is another test that covers some of this: https:/boto/botocore/blob/develop/tests/unit/test_signers.py#L117-L127 I've also added a new test to ensure the request is attempted without any auth headers/query args. |
||
def test_client_signature_override_arg(self, request_signer): | ||
creator = self.create_client_creator() | ||
config = botocore.client.Config(signature_version='foo') | ||
service_client = creator.create_client( | ||
'myservice', 'us-west-2', credentials=self.credentials, | ||
client_config=config) | ||
request_signer.assert_called_with( | ||
mock.ANY, mock.ANY, mock.ANY, 'foo', mock.ANY, mock.ANY) | ||
|
||
def test_anonymous_client_request(self): | ||
creator = self.create_client_creator() | ||
config = botocore.client.Config(signature_version=botocore.UNSIGNED) | ||
service_client = creator.create_client( | ||
'myservice', 'us-west-2', client_config=config) | ||
|
||
response = service_client.test_operation(Foo='one') | ||
|
||
# Make sure a request has been attempted | ||
self.assertTrue(self.endpoint.make_request.called) | ||
|
||
# Make sure the request parameters do NOT include auth | ||
# information. The service defined above for these tests | ||
# uses sigv4 by default (which we disable). | ||
params = dict((k.lower(), v) for k, v in \ | ||
self.endpoint.make_request.call_args[0][1].items()) | ||
self.assertNotIn('authorization', params) | ||
self.assertNotIn('x-amz-signature', params) | ||
|
||
def test_client_registers_request_created_handler(self): | ||
event_emitter = mock.Mock() | ||
creator = self.create_client_creator(event_emitter=event_emitter) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this affect the CLI? We set the endpoint's signature version to
None
. You can check real quick by running this test: https:/aws/aws-cli/blob/develop/tests/integration/customizations/s3/test_plugin.py#L1594There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given it's now a sentinel, we should be using identity checks instead of equality check (
signature_version is not botocore.UNSIGNED
)