Fix S3 optional auth #2907
Fix S3 optional auth #2907
Conversation
jdisanti
force-pushed
the
jdisanti-fix-s3-optional-auth
branch
from
93556f4 to
00d0f41
Compare
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
|
|
||
| let result = client | ||
| .list_objects() | ||
| .bucket("gdc-organoid-pancreatic-phs001611-2-open") |
There was a problem hiding this comment.
This is giving me flashbacks to my previous job
| } | ||
| } | ||
| ], | ||
| "docs": "todo docs", |
There was a problem hiding this comment.
| "docs": "todo docs", | |
| "docs": "a traffic recording of optional auth (no AUTHORIZATION header is included)", |
There was a problem hiding this comment.
Fixed docs in 6244621
| .remove_invocation_id_for_tests() | ||
| .user_agent_for_tests() |
There was a problem hiding this comment.
side note: we should add these to with_test_defaults() on the main config
| // TODO(P96049742): Endpoint config doesn't currently have a concept of optional auth or "no auth", so | ||
| // we are short-circuiting lookup of endpoint auth scheme config if that is the selected scheme. | ||
| if scheme_id == NO_AUTH_SCHEME_ID { | ||
| return Ok(AuthSchemeEndpointConfig::from(None)); |
There was a problem hiding this comment.
| return Ok(AuthSchemeEndpointConfig::from(None)); | |
| return Ok(AuthSchemeEndpointConfig::empty()); |
just a little clearer on intention I think. Could also consider naming this from_model() or inherit()
| @@ -124,6 +125,11 @@ fn extract_endpoint_auth_scheme_config( | |||
| endpoint: &Endpoint, | |||
| scheme_id: AuthSchemeId, | |||
| ) -> Result<AuthSchemeEndpointConfig<'_>, AuthOrchestrationError> { | |||
| // TODO(P96049742): Endpoint config doesn't currently have a concept of optional auth or "no auth", so | |||
| // we are short-circuiting lookup of endpoint auth scheme config if that is the selected scheme. | |||
| if scheme_id == NO_AUTH_SCHEME_ID { | |||
There was a problem hiding this comment.
I might consider renaming this—I'm not sure if NO_AUTH_SCHEME_ID means "empty" or "not authenticated". This would probably be clearer as
AuthSchemeId::unauthenticated()
There was a problem hiding this comment.
It's referring to the special auth scheme named "no auth", but I decided to leave out the second "auth" in the name: NO_AUTH_AUTH_SCHEME_ID.
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
In #2907, I created an allow list of S3 operations to add `@optionalAuth` to, but this turns out to be too restrictive, as seen in awslabs/aws-sdk-rust#878. This PR restores the original middleware behavior of allowing optional auth for all S3 operations. ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._
Motivation and Context
This PR implements a short-term solution for aws-sdk-rust#864 while a long-term solution is worked out.
Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.