Skip to content
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

Fix config::Builder::set_credentials_provider to override what's previsouly set #3278

Merged
merged 12 commits into from
Dec 1, 2023
Merged
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
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