-
Notifications
You must be signed in to change notification settings - Fork 4.5k
[v2] Fix bug with auth_scheme_preference #10169
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
base: v2
Are you sure you want to change the base?
Changes from all commits
527f455
5be9c66
e09d885
77e7f7c
24c9076
b5e9d0b
7a4c270
9a8cd56
0563fe8
8daf219
5e9e1a2
9847e4e
cc0548f
edc7fd5
a40c847
5681b21
cd01f69
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 |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| { | ||
| "type": "bugfix", | ||
| "category": "signing", | ||
| "description": "Fix bug so that configured auth scheme preference is used when auth scheme is resolved from endpoints rulesets, or from operation-level auth trait. Auth scheme preference can be configured using the existing ``auth_scheme_preference`` shared config setting, or the existing ``AWS_AUTH_SCHEME_PREFERENCE`` environment variable." | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -119,6 +119,7 @@ def get_client_args( | |
| s3_disable_express_session_auth = config_kwargs[ | ||
| 's3_disable_express_session_auth' | ||
| ] | ||
| preferred_auth_schemes = config_kwargs['auth_scheme_preference'] | ||
|
Member
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. [Nit, non-blocking]
|
||
|
|
||
| event_emitter = copy.copy(self._event_emitter) | ||
| signer = RequestSigner( | ||
|
|
@@ -169,6 +170,7 @@ def get_client_args( | |
| credentials, | ||
| account_id_endpoint_mode, | ||
| s3_disable_express_session_auth, | ||
| preferred_auth_schemes, | ||
| ) | ||
|
|
||
| # Copy the session's user agent factory and adds client configuration. | ||
|
|
@@ -589,6 +591,7 @@ def _build_endpoint_resolver( | |
| credentials, | ||
| account_id_endpoint_mode, | ||
| s3_disable_express_session_auth, | ||
| preferred_auth_schemes, | ||
| ): | ||
| if endpoints_ruleset_data is None: | ||
| return None | ||
|
|
@@ -645,6 +648,7 @@ def _build_endpoint_resolver( | |
| event_emitter=event_emitter, | ||
| use_ssl=is_secure, | ||
| requested_auth_scheme=sig_version, | ||
| preferred_auth_schemes=preferred_auth_schemes, | ||
| ) | ||
|
|
||
| def compute_endpoint_resolver_builtin_defaults( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -471,6 +471,7 @@ def __init__( | |
| event_emitter, | ||
| use_ssl=True, | ||
| requested_auth_scheme=None, | ||
| preferred_auth_schemes=None, | ||
| ): | ||
| self._provider = EndpointProvider( | ||
| ruleset_data=endpoint_ruleset_data, | ||
|
|
@@ -483,6 +484,7 @@ def __init__( | |
| self._event_emitter = event_emitter | ||
| self._use_ssl = use_ssl | ||
| self._requested_auth_scheme = requested_auth_scheme | ||
| self._preferred_auth_schemes = preferred_auth_schemes | ||
| self._instance_cache = {} | ||
|
|
||
| def construct_endpoint( | ||
|
|
@@ -698,6 +700,17 @@ def auth_schemes_to_signing_ctx(self, auth_schemes): | |
| if self._requested_auth_scheme == UNSIGNED: | ||
| return 'none', {} | ||
|
|
||
| # if a preferred auth schemes list is provided, reorder the auth schemes | ||
| # list based on the preferred ordering. | ||
| if self._preferred_auth_schemes is not None: | ||
|
Member
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. [Change requested] Right now we manually reorder the endpoint ruleset authSchemes list based on auth_scheme_preference. Instead, we can reuse resolve_auth_scheme_preference() to pick the desired auth type and then map that back to the actual ruleset scheme dict. Here's the proof of concept I wrote: We could definitely refactor this and/or the |
||
| prefs = self._preferred_auth_schemes.split(',') | ||
| by_name = {s['name']: s for s in auth_schemes} | ||
| auth_schemes = [ | ||
| by_name[p] for p in prefs if p in by_name | ||
| ] + [ | ||
| s for s in auth_schemes if s['name'] not in prefs | ||
| ] | ||
|
|
||
| auth_schemes = [ | ||
| {**scheme, 'name': self._strip_sig_prefix(scheme['name'])} | ||
| for scheme in auth_schemes | ||
|
|
||
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.
High level note:
Jonathan has a semi-related bugfix that will likely need to apply here as well - https://github.com/boto/botocore/pull/3663/changes.
When we resolve v4a via auth scheme preference, we need to make sure we are also respecting the signing region set