Skip to content

Commit

Permalink
Fix config::Builder::set_credentials_provider to override what's pr…
Browse files Browse the repository at this point in the history
…evisouly set (#3278)

## Motivation and Context
Fixes awslabs/aws-sdk-rust#973

## Description
Prior to the PR, if a customer explicitly passed a credentials provider
to a client's config `Builder::set_credentials_provider`, what's passed
did not override a credentials provider previously set ([actual use
case](awslabs/aws-sdk-rust#973 (comment))).

While in general, we recommend customers single-source a credentials
provider through
[aws_config::ConfigLoader::credentials_provider](https://docs.rs/aws-config/1.0.1/aws_config/struct.ConfigLoader.html#method.credentials_provider),
we should eliminate the said footgun in case they directly pass a
credentials provider to a client's config `Builder`.

The PR reverts test signature updates in
#3156 (in hindsight, having
to update test signatures in that PR was a sign of regression).

## Testing
Added a Kotlin test to `CredentialProviderConfigTest.kt` to verify the
fix

## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [x] I have updated `CHANGELOG.next.toml` if I made changes to the AWS
SDK, generated SDK code, or SDK runtime crates

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._

---------

Co-authored-by: John DiSanti <jdisanti@amazon.com>
  • Loading branch information
ysaito1001 and jdisanti committed Dec 1, 2023
1 parent 76ae89a commit 81fc83e
Show file tree
Hide file tree
Showing 14 changed files with 181 additions and 23 deletions.
20 changes: 19 additions & 1 deletion CHANGELOG.next.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,22 @@
# message = "Fix typos in module documentation for generated crates"
# references = ["smithy-rs#920"]
# meta = { "breaking" = false, "tada" = false, "bug" = false, "target" = "client | server | all"}
# author = "rcoh"
# author = "rcoh"

[[aws-sdk-rust]]
message = "Fix `config::Builder::set_credentials_provider` to override a credentials provider previously set."
references = ["aws-sdk-rust#973", "smithy-rs#3278"]
meta = { "breaking" = false, "tada" = false, "bug" = true }
author = "ysaito1001"

[[aws-sdk-rust]]
message = "`config::Config::credentials_provider` has been broken since `release-2023-11-15` and is now marked as `deprecated` explicitly."
references = ["smithy-rs#3251", "smithy-rs#3278"]
meta = { "breaking" = false, "tada" = false, "bug" = false }
author = "ysaito1001"

[[smithy-rs]]
message = "`RuntimeComponentsBuilder::push_identity_resolver` is now deprecated since it does not replace the existing identity resolver of a given auth scheme ID. Use `RuntimeComponentsBuilder::set_identity_resolver` instead."
references = ["smithy-rs#3278"]
meta = { "breaking" = false, "tada" = false, "bug" = false, "target" = "client" }
author = "ysaito1001"
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,10 @@ class CredentialProviderConfig(private val codegenContext: ClientCodegenContext)
ServiceConfig.ConfigImpl -> {
rustTemplate(
"""
/// Returns the credentials provider for this service
/// This function was intended to be removed, and has been broken since release-2023-11-15 as it always returns a `None`. Do not use.
##[deprecated(note = "This function was intended to be removed, and has been broken since release-2023-11-15 as it always returns a `None`. Do not use.")]
pub fn credentials_provider(&self) -> Option<#{SharedCredentialsProvider}> {
self.config.load::<#{SharedCredentialsProvider}>().cloned()
#{None}
}
""",
*codegenScope,
Expand Down Expand Up @@ -118,13 +119,13 @@ class CredentialProviderConfig(private val codegenContext: ClientCodegenContext)
if (codegenContext.serviceShape.supportedAuthSchemes().contains("sigv4a")) {
featureGateBlock("sigv4a") {
rustTemplate(
"self.runtime_components.push_identity_resolver(#{SIGV4A_SCHEME_ID}, credentials_provider.clone());",
"self.runtime_components.set_identity_resolver(#{SIGV4A_SCHEME_ID}, credentials_provider.clone());",
*codegenScope,
)
}
}
rustTemplate(
"self.runtime_components.push_identity_resolver(#{SIGV4_SCHEME_ID}, credentials_provider);",
"self.runtime_components.set_identity_resolver(#{SIGV4_SCHEME_ID}, credentials_provider);",
*codegenScope,
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ internal class CredentialProviderConfigTest {
val moduleName = ctx.moduleUseName()
rustTemplate(
"""
let (http_client, _rx) = #{capture_request}(None);
let (http_client, _rx) = #{capture_request}(#{None});
let client_config = $moduleName::Config::builder()
.http_client(http_client)
.build();
Expand Down Expand Up @@ -62,4 +62,71 @@ internal class CredentialProviderConfigTest {
}
}
}

@Test
fun `configuring credentials provider on builder should replace what was previously set`() {
awsSdkIntegrationTest(SdkCodegenIntegrationTest.model) { ctx, rustCrate ->
val rc = ctx.runtimeConfig
val codegenScope = arrayOf(
*RuntimeType.preludeScope,
"capture_request" to RuntimeType.captureRequest(rc),
"Credentials" to AwsRuntimeType.awsCredentialTypesTestUtil(rc)
.resolve("Credentials"),
"Region" to AwsRuntimeType.awsTypes(rc).resolve("region::Region"),
"SdkConfig" to AwsRuntimeType.awsTypes(rc).resolve("sdk_config::SdkConfig"),
"SharedCredentialsProvider" to AwsRuntimeType.awsCredentialTypes(rc)
.resolve("provider::SharedCredentialsProvider"),
)
rustCrate.integrationTest("credentials_provider") {
// per https://github.com/awslabs/aws-sdk-rust/issues/973
tokioTest("configuring_credentials_provider_on_builder_should_replace_what_was_previously_set") {
val moduleName = ctx.moduleUseName()
rustTemplate(
"""
let (http_client, rx) = #{capture_request}(#{None});
let replace_me = #{Credentials}::new(
"replace_me",
"replace_me",
#{None},
#{None},
"replace_me",
);
let sdk_config = #{SdkConfig}::builder()
.credentials_provider(
#{SharedCredentialsProvider}::new(replace_me),
)
.region(#{Region}::new("us-west-2"))
.build();
let expected = #{Credentials}::new(
"expected_credential",
"expected_credential",
#{None},
#{None},
"expected_credential",
);
let conf = $moduleName::config::Builder::from(&sdk_config)
.http_client(http_client)
.credentials_provider(expected)
.build();
let client = $moduleName::Client::from_conf(conf);
let _ = client
.some_operation()
.send()
.await
.expect("success");
let req = rx.expect_request();
let auth_header = req.headers().get("AUTHORIZATION").unwrap();
assert!(auth_header.contains("expected_credential"), "{auth_header}");
""",
*codegenScope,
)
}
}
}
}
}
2 changes: 1 addition & 1 deletion aws/sdk/integration-tests/kms/tests/integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ async fn generate_random() {
.header("content-type", "application/x-amz-json-1.1")
.header("x-amz-target", "TrentService.GenerateRandom")
.header("content-length", "20")
.header("authorization", "AWS4-HMAC-SHA256 Credential=ANOTREAL/20090213/us-east-1/kms/aws4_request, SignedHeaders=content-length;content-type;host;x-amz-date;x-amz-security-token;x-amz-target;x-amz-user-agent, Signature=703f72fe50c310e3ee1a7a106df947b980cb91bc8bad7a4a603b057096603aed")
.header("authorization", "AWS4-HMAC-SHA256 Credential=ANOTREAL/20090213/us-east-1/kms/aws4_request, SignedHeaders=content-length;content-type;host;x-amz-date;x-amz-target;x-amz-user-agent, Signature=53dcf70f6f852cb576185dcabef5aaa3d068704cf1b7ea7dc644efeaa46674d7")
.header("x-amz-date", "20090213T233130Z")
.header("user-agent", "aws-sdk-rust/0.123.test os/windows/XPSP3 lang/rust/1.50.0")
.header("x-amz-user-agent", "aws-sdk-rust/0.123.test api/test-service/0.123 os/windows/XPSP3 lang/rust/1.50.0")
Expand Down
2 changes: 1 addition & 1 deletion aws/sdk/integration-tests/qldbsession/tests/integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ async fn signv4_use_correct_service_name() {
.header("content-type", "application/x-amz-json-1.0")
.header("x-amz-target", "QLDBSession.SendCommand")
.header("content-length", "49")
.header("authorization", "AWS4-HMAC-SHA256 Credential=ANOTREAL/20090213/us-east-1/qldb/aws4_request, SignedHeaders=content-length;content-type;host;x-amz-date;x-amz-security-token;x-amz-target;x-amz-user-agent, Signature=e8d50282fa369adf05f33a5b32e3ce2a7582edc902312c59de311001a97426d9")
.header("authorization", "AWS4-HMAC-SHA256 Credential=ANOTREAL/20090213/us-east-1/qldb/aws4_request, SignedHeaders=content-length;content-type;host;x-amz-date;x-amz-target;x-amz-user-agent, Signature=9a07c60550504d015fb9a2b0f1b175a4d906651f9dd4ee44bebb32a802d03815")
// qldbsession uses the signing name 'qldb' in signature _________________________^^^^
.header("x-amz-date", "20090213T233130Z")
.header("user-agent", "aws-sdk-rust/0.123.test os/windows/XPSP3 lang/rust/1.50.0")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ async fn test_s3_signer_with_naughty_string_metadata() {

// This is a snapshot test taken from a known working test result
let snapshot_signature =
"Signature=733dba2f1ca3c9a39f4eef3a6750a71eff00297cd765408ad3cef5dcdc44d642";
"Signature=a5115604df66219874a9e5a8eab4c9f7a28c992ab2d918037a285756c019f3b2";
assert!(
auth_header .contains(snapshot_signature),
"authorization header signature did not match expected signature: got {}, expected it to contain {}",
Expand Down
2 changes: 1 addition & 1 deletion aws/sdk/integration-tests/s3/tests/normalize-uri-path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ async fn test_operation_should_not_normalize_uri_path() {
let expected_uri = "https://test-bucket-ad7c9f01-7f7b-4669-b550-75cc6d4df0f1.s3.us-east-1.amazonaws.com/a/.././b.txt?x-id=PutObject";
assert_eq!(actual_uri, expected_uri);

let expected_sig = "Signature=404fb9502378c8f46fb83544848c42d29d55610a14b4bed9577542e49e549d08";
let expected_sig = "Signature=2ac540538c84dc2616d92fb51d4fc6146ccd9ccc1ee85f518a1a686c5ef97b86";
assert!(
actual_auth.contains(expected_sig),
"authorization header signature did not match expected signature: expected {} but not found in {}",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ async fn test_s3_signer_query_string_with_all_valid_chars() {

// This is a snapshot test taken from a known working test result
let snapshot_signature =
"Signature=740feb1de3968a643e68fb1a17c415d98dd6a1cc28782fb1ef6157586548c747";
"Signature=9a931d20606f93fa4e5553602866a9b5ccac2cd42b54ae5a4b17e4614fb443ce";
assert!(
auth_header
.contains(snapshot_signature),
Expand Down
2 changes: 1 addition & 1 deletion aws/sdk/integration-tests/s3/tests/signing-it.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use aws_smithy_types::body::SdkBody;
async fn test_signer() {
let http_client = StaticReplayClient::new(vec![ReplayEvent::new(
http::Request::builder()
.header("authorization", "AWS4-HMAC-SHA256 Credential=ANOTREAL/20090213/us-east-1/s3/aws4_request, SignedHeaders=host;x-amz-content-sha256;x-amz-date;x-amz-security-token;x-amz-user-agent, Signature=d8ea22461a59cc1cbeb01fa093423ffafcb7695197ba2409b477216a4be2c104")
.header("authorization", "AWS4-HMAC-SHA256 Credential=ANOTREAL/20090213/us-east-1/s3/aws4_request, SignedHeaders=host;x-amz-content-sha256;x-amz-date;x-amz-user-agent, Signature=27e3f59ec3cffaa10e4f1c92112e8fb62d468a04cd32be39e68215f830404dbb")
.uri("https://test-bucket.s3.us-east-1.amazonaws.com/?list-type=2&prefix=prefix~")
.body(SdkBody::empty())
.unwrap(),
Expand Down
6 changes: 3 additions & 3 deletions aws/sdk/integration-tests/s3control/tests/signing-it.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,10 @@ use aws_smithy_types::body::SdkBody;
async fn test_signer() {
let http_client = StaticReplayClient::new(vec![ReplayEvent::new(
http::Request::builder()
.header("authorization",
.header("authorization",
"AWS4-HMAC-SHA256 Credential=ANOTREAL/20090213/us-east-1/s3/aws4_request, \
SignedHeaders=host;x-amz-account-id;x-amz-content-sha256;x-amz-date;x-amz-security-token;x-amz-user-agent, \
Signature=01a71226e959c7b0b998adf26fa266f9c3612df57a60b187d549822e86d90667")
SignedHeaders=host;x-amz-account-id;x-amz-content-sha256;x-amz-date;x-amz-user-agent, \
Signature=0102a74cb220f8445c4efada17660572ff813e07b524032ec831e8c2514be903")
.uri("https://test-bucket.s3-control.us-east-1.amazonaws.com/v20180820/accesspoint")
.body(SdkBody::empty())
.unwrap(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ private class HttpAuthConfigCustomization(
/// Sets an API key resolver will be used for authentication.
pub fn api_key_resolver(mut self, api_key_resolver: impl #{ResolveIdentity} + 'static) -> Self {
self.runtime_components.push_identity_resolver(
self.runtime_components.set_identity_resolver(
#{HTTP_API_KEY_AUTH_SCHEME_ID},
#{SharedIdentityResolver}::new(api_key_resolver)
);
Expand All @@ -240,7 +240,7 @@ private class HttpAuthConfigCustomization(
/// Sets a bearer token provider that will be used for HTTP bearer auth.
pub fn bearer_token_resolver(mut self, bearer_token_resolver: impl #{ResolveIdentity} + 'static) -> Self {
self.runtime_components.push_identity_resolver(
self.runtime_components.set_identity_resolver(
#{HTTP_BEARER_AUTH_SCHEME_ID},
#{SharedIdentityResolver}::new(bearer_token_resolver)
);
Expand All @@ -260,7 +260,7 @@ private class HttpAuthConfigCustomization(
/// Sets a login resolver that will be used for HTTP basic auth.
pub fn basic_auth_login_resolver(mut self, basic_auth_resolver: impl #{ResolveIdentity} + 'static) -> Self {
self.runtime_components.push_identity_resolver(
self.runtime_components.set_identity_resolver(
#{HTTP_BASIC_AUTH_SCHEME_ID},
#{SharedIdentityResolver}::new(basic_auth_resolver)
);
Expand All @@ -280,7 +280,7 @@ private class HttpAuthConfigCustomization(
/// Sets a login resolver that will be used for HTTP digest auth.
pub fn digest_auth_login_resolver(mut self, digest_auth_resolver: impl #{ResolveIdentity} + 'static) -> Self {
self.runtime_components.push_identity_resolver(
self.runtime_components.set_identity_resolver(
#{HTTP_DIGEST_AUTH_SCHEME_ID},
#{SharedIdentityResolver}::new(digest_auth_resolver)
);
Expand Down
2 changes: 1 addition & 1 deletion rust-runtime/aws-smithy-runtime-api/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ zeroize = { version = "1", optional = true }

[dev-dependencies]
proptest = "1"
tokio = { version = "1.25", features = ["rt", "macros"] }
tokio = { version = "1.25", features = ["macros", "rt", "rt-multi-thread"] }

[package.metadata.docs.rs]
all-features = true
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -570,7 +570,11 @@ impl RuntimeComponentsBuilder {
self
}

/// Adds an identity resolver.
/// This method is broken since it does not replace an existing identity resolver of the given auth scheme ID.
/// Use `set_identity_resolver` instead.
#[deprecated(
note = "This method is broken since it does not replace an existing identity resolver of the given auth scheme ID. Use `set_identity_resolver` instead."
)]
pub fn push_identity_resolver(
&mut self,
scheme_id: AuthSchemeId,
Expand All @@ -583,13 +587,40 @@ impl RuntimeComponentsBuilder {
self
}

/// Sets the identity resolver for a given `scheme_id`.
///
/// If there is already an identity resolver for that `scheme_id`, this method will replace
/// the existing one with the passed-in `identity_resolver`.
pub fn set_identity_resolver(
&mut self,
scheme_id: AuthSchemeId,
identity_resolver: impl ResolveIdentity + 'static,
) -> &mut Self {
let tracked = Tracked::new(
self.builder_name,
ConfiguredIdentityResolver::new(scheme_id, identity_resolver.into_shared()),
);

if let Some(s) = self
.identity_resolvers
.iter_mut()
.find(|s| s.value.scheme_id() == scheme_id)
{
*s = tracked;
} else {
self.identity_resolvers.push(tracked);
}

self
}

/// Adds an identity resolver.
pub fn with_identity_resolver(
mut self,
scheme_id: AuthSchemeId,
identity_resolver: impl ResolveIdentity + 'static,
) -> Self {
self.push_identity_resolver(scheme_id, identity_resolver);
self.set_identity_resolver(scheme_id, identity_resolver);
self
}

Expand Down Expand Up @@ -1144,4 +1175,45 @@ mod tests {
fn building_test_builder_should_not_panic() {
let _ = RuntimeComponentsBuilder::for_tests().build(); // should not panic
}

#[test]
fn set_identity_resolver_should_replace_existing_resolver_for_given_auth_scheme() {
use crate::client::auth::AuthSchemeId;
use crate::client::identity::{Identity, IdentityFuture, ResolveIdentity};
use crate::client::runtime_components::{GetIdentityResolver, RuntimeComponents};
use aws_smithy_types::config_bag::ConfigBag;
use tokio::runtime::Runtime;

#[derive(Debug)]
struct AnotherFakeIdentityResolver;
impl ResolveIdentity for AnotherFakeIdentityResolver {
fn resolve_identity<'a>(
&'a self,
_: &'a RuntimeComponents,
_: &'a ConfigBag,
) -> IdentityFuture<'a> {
IdentityFuture::ready(Ok(Identity::new("doesntmatter", None)))
}
}

// Set a different `IdentityResolver` for the `fake` auth scheme already configured in
// a test runtime components builder
let rc = RuntimeComponentsBuilder::for_tests()
.with_identity_resolver(AuthSchemeId::new("fake"), AnotherFakeIdentityResolver)
.build()
.expect("should build RuntimeComponents");

let resolver = rc
.identity_resolver(AuthSchemeId::new("fake"))
.expect("identity resolver should be found");

let identity = Runtime::new().unwrap().block_on(async {
resolver
.resolve_identity(&rc, &ConfigBag::base())
.await
.expect("identity should be resolved")
});

assert_eq!(Some(&"doesntmatter"), identity.data::<&str>());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ impl<I, O, E> OperationBuilder<I, O, E> {
.push_auth_scheme(SharedAuthScheme::new(NoAuthScheme::default()));
self.runtime_components
.set_identity_cache(Some(IdentityCache::no_cache()));
self.runtime_components.push_identity_resolver(
self.runtime_components.set_identity_resolver(
NO_AUTH_SCHEME_ID,
SharedIdentityResolver::new(NoAuthIdentityResolver::new()),
);
Expand Down

0 comments on commit 81fc83e

Please sign in to comment.