Skip to content

Commit

Permalink
fix default credential chain not respecting endpoint URL overrides (#…
Browse files Browse the repository at this point in the history
…3873)

## Motivation and Context
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here -->
awslabs/aws-sdk-rust#1193

## Description
This PR fixes a customer reported bug where the default chain doesn't
respect `AWS_ENDPOINT_URL`/`AWS_ENDPOINT_URL_<SERVICE>` environment
variables or the equivalents in AWS shared config (`~/.aws/config`).

This fix is a little nuanced and frankly gross but there isn't a better
option that I can see right now that isn't way more invasive. The crux
of the issue is that when we implemented support for this feature
([1](#3568),
[2](#3493),
[3](#3488)) we really only
made it work for clients created via
[`ConfigLoader::load()`](https://github.com/smithy-lang/smithy-rs/blob/release-2024-10-09/aws/rust-runtime/aws-config/src/lib.rs#L871).
Internally the default chain credential provider constructs `STS` and
`SSO` clients but it does so using
[`ProviderConfig`](https://github.com/smithy-lang/smithy-rs/blob/release-2024-10-09/aws/rust-runtime/aws-config/src/provider_config.rs#L36)
by mapping this to `SdkConfig` via
[`ProviderConfig::client_config()`](https://github.com/smithy-lang/smithy-rs/blob/release-2024-10-09/aws/rust-runtime/aws-config/src/provider_config.rs#L199).
This conversion is used in several places and it doesn't take any of the
required logic into account to setup
[`EnvServiceConfig`](https://github.com/smithy-lang/smithy-rs/blob/release-2024-10-09/aws/rust-runtime/aws-config/src/lib.rs#L859-L862)
which is what generated SDK's ultimately use to figure out the endpoint
URL from either environment/profile ([example
client](https://github.com/awslabs/aws-sdk-rust/blob/release-2024-10-09/sdk/sts/src/config.rs#L1214-L1221)
which ultimately ends up in `EnvServiceConfig`
[here](https://github.com/smithy-lang/smithy-rs/blob/release-2024-10-09/aws/rust-runtime/aws-config/src/env_service_config.rs#L18)).

The fix applied here is nuanced in that we update the conversion to
provide a `EnvServiceConfig` but it relies on the profile to have been
parsed already or else you'll get an empty/default profile. This
generally works for the profile provider since the first thing we do is
load the profile but in isolation it may not work as expected. I've
added tests for STS to cover all cases but SSO credentials and token
providers do NOT currently respect shared config endpoint URL keys.
Fixing this is possible but involved since we require an `async` context
to ensure a profile is loaded already and in many places where we
construct `SdkConfig` from `ProviderConfig` we are in non async
function.

## Testing
Tested repro + additional integration tests

## Future
This does _not_ fix awslabs/aws-sdk-rust#1194
which was discovered as a bug/gap. Fixing it would be outside the scope
of this PR.

SSO/token provider is instantiated sometimes before we have parsed a
profile. This PR definitely fixes the STS provider for all configuration
scenarios but the SSO related client usage may still have some edge
cases when configured via profiles since we often instantiate them
before parsing a profile. When we surveyed other SDKs there were several
that failed to respect these variables and haven't received issues
around this which leads me to believe this isn't likely a problem in
practice (most likely due to SSO being used in local development most
often where redirecting that endpoint doesn't make much sense anyway).

## Checklist
- [X] For changes to the AWS SDK, generated SDK code, or SDK runtime
crates, I have created a changelog entry Markdown file in the
`.changelog` directory, specifying "aws-sdk-rust" in the `applies_to`
key.

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
  • Loading branch information
aajtodd authored Oct 21, 2024
1 parent 4b66264 commit 42751e5
Show file tree
Hide file tree
Showing 34 changed files with 892 additions and 195 deletions.
12 changes: 12 additions & 0 deletions .changelog/1728582276.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
---
applies_to:
- aws-sdk-rust
authors:
- aajtodd
references:
- aws-sdk-rust#1193
breaking: false
new_feature: false
bug_fix: true
---
Fix default credential provider chain not respecting endpoint URL overrides from environment
18 changes: 9 additions & 9 deletions aws/rust-runtime/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

20 changes: 10 additions & 10 deletions aws/rust-runtime/aws-config/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion aws/rust-runtime/aws-config/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "aws-config"
version = "1.5.8"
version = "1.5.9"
authors = [
"AWS Rust SDK Team <[email protected]>",
"Russell Cohen <[email protected]>",
Expand Down
16 changes: 12 additions & 4 deletions aws/rust-runtime/aws-config/src/profile/credentials.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ use aws_credential_types::{
Credentials,
};
use aws_smithy_types::error::display::DisplayErrorContext;
use aws_types::SdkConfig;
use std::borrow::Cow;
use std::collections::HashMap;
use std::error::Error;
Expand Down Expand Up @@ -142,7 +141,6 @@ pub struct ProfileFileCredentialsProvider {
#[derive(Debug)]
struct Config {
factory: exec::named::NamedProviderFactory,
sdk_config: SdkConfig,
provider_config: ProviderConfig,
}

Expand Down Expand Up @@ -493,7 +491,6 @@ impl Builder {
ProfileFileCredentialsProvider {
config: Arc::new(Config {
factory,
sdk_config: conf.client_config(),
provider_config: conf,
}),
inner_provider: ErrorTakingOnceCell::new(),
Expand Down Expand Up @@ -542,9 +539,13 @@ impl ChainProvider {
return Err(CredentialsError::provider_error(e));
}
};

// we want to create `SdkConfig` _after_ we have resolved the profile or else
// we won't get things like `service_config()` set appropriately.
let sdk_config = config.provider_config.client_config();
for provider in chain.chain().iter() {
let next_creds = provider
.credentials(creds, &config.sdk_config)
.credentials(creds, &sdk_config)
.instrument(tracing::debug_span!("load_assume_role", provider = ?provider))
.await;
match next_creds {
Expand Down Expand Up @@ -609,7 +610,14 @@ mod test {
#[cfg(feature = "sso")]
make_test!(sso_credentials);
#[cfg(feature = "sso")]
make_test!(sso_override_global_env_url);
#[cfg(feature = "sso")]
make_test!(sso_token);

make_test!(assume_role_override_global_env_url);
make_test!(assume_role_override_service_env_url);
make_test!(assume_role_override_global_profile_url);
make_test!(assume_role_override_service_profile_url);
}

#[cfg(all(test, feature = "sso"))]
Expand Down
14 changes: 14 additions & 0 deletions aws/rust-runtime/aws-config/src/provider_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

//! Configuration Options for Credential Providers
use crate::env_service_config::EnvServiceConfig;
use crate::profile;
#[allow(deprecated)]
use crate::profile::profile_file::ProfileFiles;
Expand Down Expand Up @@ -196,13 +197,26 @@ impl ProviderConfig {
Self::without_region().load_default_region().await
}

/// Attempt to get a representation of `SdkConfig` from this `ProviderConfig`.
///
///
/// **WARN**: Some options (e.g. `service_config`) can only be set if the profile has been
/// parsed already (e.g. by calling [`ProviderConfig::profile()`]). This is an
/// imperfect mapping and should be used sparingly.
pub(crate) fn client_config(&self) -> SdkConfig {
let profiles = self.parsed_profile.get().and_then(|v| v.as_ref().ok());
let service_config = EnvServiceConfig {
env: self.env(),
env_config_sections: profiles.cloned().unwrap_or_default(),
};

let mut builder = SdkConfig::builder()
.retry_config(RetryConfig::standard())
.region(self.region())
.time_source(self.time_source())
.use_fips(self.use_fips().unwrap_or_default())
.use_dual_stack(self.use_dual_stack().unwrap_or_default())
.service_config(service_config)
.behavior_version(crate::BehaviorVersion::latest());
builder.set_http_client(self.http_client.clone());
builder.set_sleep_impl(self.sleep_impl.clone());
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"HOME": "/home",
"AWS_ENDPOINT_URL": "http://aws.global-env-override"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[default]
region = us-east-1
role_arn = arn:aws:iam::123456789:role/integration-test
source_profile = base

[profile base]
region = us-east-1
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
[base]
aws_access_key_id = AKIAFAKE
aws_secret_access_key = FAKE
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
{
"events": [
{
"connection_id": 0,
"action": {
"Request": {
"request": {
"uri": "http://aws.global-env-override",
"headers": {
"content-type": [
"application/x-www-form-urlencoded"
],
"authorization": [
"AWS4-HMAC-SHA256 Credential=AKIAFAKE/20210810/us-east-1/sts/aws4_request, SignedHeaders=content-length;content-type;host;x-amz-date;x-amz-user-agent, Signature=cd5cb2aa1d20717ca17692bcbda711797ae9eb8bb1130690b021b3952b7ae56e"
],
"user-agent": [
"aws-sdk-rust/0.1.0 os/macos lang/rust/1.55.0-nightly"
],
"content-length": [
"146"
],
"x-amz-date": [
"20210810T003833Z"
],
"host": [
"aws.global-env-override"
],
"x-amz-user-agent": [
"aws-sdk-rust/0.1.0 api/sts/0.0.14-alpha os/macos lang/rust/1.55.0-nightly"
]
},
"method": "POST"
}
}
}
},
{
"connection_id": 0,
"action": {
"Data": {
"data": {
"Utf8": "Action=AssumeRole&Version=2011-06-15&RoleArn=arn%3Aaws%3Aiam%3A%3A123456789%3Arole%2Fintegration-test&RoleSessionName=assume-role-provider-session"
},
"direction": "Request"
}
}
},
{
"connection_id": 0,
"action": {
"Eof": {
"ok": true,
"direction": "Request"
}
}
},
{
"connection_id": 0,
"action": {
"Response": {
"response": {
"Ok": {
"status": 200,
"version": "HTTP/1.1",
"headers": {
"date": [
"Thu, 05 Aug 2021 18:58:02 GMT"
],
"content-length": [
"1491"
],
"content-type": [
"text/xml"
],
"x-amzn-requestid": [
"c2e971c2-702d-4124-9b1f-1670febbea18"
]
}
}
}
}
}
},
{
"connection_id": 0,
"action": {
"Data": {
"data": {
"Utf8": "<AssumeRoleResponse xmlns=\"https://sts.amazonaws.com/doc/2011-06-15/\">\n <AssumeRoleResult>\n <AssumedRoleUser>\n <AssumedRoleId>AROARABCDEFGHIJKLMNOP:assume-role-provider-session</AssumedRoleId>\n <Arn>arn:aws:sts::123456789012:assumed-role/integration-test/assume-role-provider-session</Arn>\n </AssumedRoleUser>\n <Credentials>\n <AccessKeyId>ASIARTESTID</AccessKeyId>\n <SecretAccessKey>TESTSECRETKEY</SecretAccessKey>\n <SessionToken>TESTSESSIONTOKEN</SessionToken>\n <Expiration>2021-08-05T19:58:02Z</Expiration>\n </Credentials>\n </AssumeRoleResult>\n <ResponseMetadata>\n <RequestId>c2e971c2-702d-4124-9b1f-1670febbea18</RequestId>\n </ResponseMetadata>\n</AssumeRoleResponse>\n"
},
"direction": "Response"
}
}
},
{
"connection_id": 0,
"action": {
"Eof": {
"ok": true,
"direction": "Response"
}
}
}
],
"docs": "standard request / response with STS",
"version": "V0"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
"name": "assume-role-override-global-env-url",
"docs": "override AWS_ENDPOINT_URL via environment",
"result": {
"Ok": {
"access_key_id": "ASIARTESTID",
"secret_access_key": "TESTSECRETKEY",
"session_token": "TESTSESSIONTOKEN",
"expiry": 1628193482
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"HOME": "/home"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
[default]
region = us-east-1
role_arn = arn:aws:iam::123456789:role/integration-test
source_profile = base
endpoint_url = http://aws.global-profile-override

[profile base]
region = us-east-1
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
[base]
aws_access_key_id = AKIAFAKE
aws_secret_access_key = FAKE
Loading

0 comments on commit 42751e5

Please sign in to comment.