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

Endpoint URL Not Respected By AssumeRoleProvider #1193

Closed
tustvold opened this issue Sep 19, 2024 · 3 comments
Closed

Endpoint URL Not Respected By AssumeRoleProvider #1193

tustvold opened this issue Sep 19, 2024 · 3 comments
Labels
bug This issue is a bug. p2 This is a standard priority issue

Comments

@tustvold
Copy link

Describe the bug

The AssumeRoleProvider created as part of loading credentials from a profile does not appear to respect the endpoint URL overrides, and continues to talk to sts.amazonaws.com

Expected Behavior

It should talk to the configured endpoint URL

Current Behavior

It talks to the default STS endpoint

Reproduction Steps

Create a file at /tmp/aws-config with the following content

[profile default]
aws_access_key_id = test
aws_secret_access_key = test

[profile messaging]
role_arn = arn:aws:iam::000000000000:role/messaging
role_session_name = messaging
source_profile = default

Then create a new Rust project with

Cargo.toml

[package]
name = "temp"
version = "0.1.0"
edition = "2021"

[dependencies]
aws-config = "1.5.6"
aws-credential-types = "1.2"
tokio = { version = "1.0", features = ["full"] }

src/main.rs

use aws_credential_types::provider::ProvideCredentials;

#[tokio::main]
async fn main() {
    let aws_config = aws_config::defaults(aws_config::BehaviorVersion::latest()).load().await;
    aws_config.credentials_provider().unwrap().provide_credentials().await.unwrap();
}

Run it with

export AWS_ENDPOINT_URL=http://127.0.0.1:4566
export AWS_CONFIG_FILE=/tmp/aws-config
export AWS_PROFILE=messaging
cargo run

Get the following error

called `Result::unwrap()` on an `Err` value: ProviderError(ProviderError { source: ProviderError(ProviderError { source: ServiceError(ServiceError { source: Unhandled(Unhandled { source: ErrorMetadata { code: Some("InvalidClientTokenId"), message: Some("The security token included in the request is invalid."), extras: Some({"aws_request_id": "64665aaf-2926-429c-999d-6f22f439d5e3"}) }, meta: ErrorMetadata { code: Some("InvalidClientTokenId"), message: Some("The security token included in the request is invalid."), extras: Some({"aws_request_id": "64665aaf-2926-429c-999d-6f22f439d5e3"}) } }), raw: Response { status: StatusCode(403), headers: Headers { headers: {"x-amzn-requestid": HeaderValue { _private: H0("64665aaf-2926-429c-999d-6f22f439d5e3") }, "content-type": HeaderValue { _private: H0("text/xml") }, "content-length": HeaderValue { _private: H0("306") }, "date": HeaderValue { _private: H0("Thu, 19 Sep 2024 16:19:07 GMT") }} }, body: SdkBody { inner: Once(Some(b"<ErrorResponse xmlns=\"https://sts.amazonaws.com/doc/2011-06-15/\">\n  <Error>\n    <Type>Sender</Type>\n    <Code>InvalidClientTokenId</Code>\n    <Message>The security token included in the request is invalid.</Message>\n  </Error>\n  <RequestId>64665aaf-2926-429c-999d-6f22f439d5e3</RequestId>\n</ErrorResponse>\n")), retryable: true }, extensions: Extensions { extensions_02x: Extensions, extensions_1x: Extensions } } }) }) })

Crucially this does not require localstack to be running, as it completely ignores the configured endpoint URL

Possible Solution

No response

Additional Information/Context

#921 is related, but it would appear smithy-lang/smithy-rs#3014 was only a partial fix

Version

aws-config = 1.5.6
aws-credential-types = 1.2.1

Environment details (OS name and version, etc.)

x86_64 GNU/Linux

Logs

No response

@tustvold tustvold added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 19, 2024
@aajtodd aajtodd removed the needs-triage This issue or PR still needs to be triaged. label Sep 19, 2024
@aajtodd
Copy link
Contributor

aajtodd commented Sep 19, 2024

Thanks for the short repro example, I've been able to reproduce this, looking into it.

@aajtodd aajtodd added the p2 This is a standard priority issue label Sep 20, 2024
github-merge-queue bot pushed a commit to smithy-lang/smithy-rs that referenced this issue Oct 21, 2024
…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._
aws-sdk-rust-ci pushed a commit that referenced this issue Oct 25, 2024
…overrides (#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 -->
#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](smithy-lang/smithy-rs#3568),
[2](smithy-lang/smithy-rs#3493),
[3](smithy-lang/smithy-rs#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 #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._
@aajtodd
Copy link
Contributor

aajtodd commented Oct 30, 2024

Fix for this should be available from Oct 25 2024 release and after.

@aajtodd aajtodd closed this as completed Oct 30, 2024
Copy link

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. p2 This is a standard priority issue
Projects
None yet
Development

No branches or pull requests

2 participants