Add credentials exposure test & fix STS + SSO #2603

rcoh · 2023-04-19T18:54:35Z

Motivation and Context

credentials providers may leak credentials in the HTTP body at the debug level

Description

This adds a test to aws-config that looks for leaked credentials in all of our provider integration tests—since these test use AWS APIs under the hood, this also serves to test AWS services in general.

To support this, sensitive was added to the ParseHttpResponse trait and code was generated to take action based on this change.

Add environment variable to force logging of the body
consider if we want to suppress request body logging as well

Testing

Checklist

I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
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.

github-actions · 2023-04-19T19:23:57Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

jdisanti

Looks good!

aws/rust-runtime/aws-config/test-data/profile-provider/credential_process/fs/home/.aws/config

aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/customize/sso/SSODecorator.kt

This adds a test to aws-config that looks for leaked credentials in all of our provider integration tests—since these test use AWS APIs under the hood, this also serves to test AWS services in general. To support this, `sensitive` was added to the ParseHttpResponse trait and code was generated to take action based on this change.

…ial_process/fs/home/.aws/config Co-authored-by: John DiSanti <jdisanti@amazon.com>

github-actions · 2023-04-20T13:02:08Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Client Test (ignoring whitespace)
No codegen difference in the Server Test
No codegen difference in the Server Test Python
No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

ysaito1001

Great PR, handling the business of logging sensitive info holistically both in models and AWS runtime crates.

ysaito1001 · 2023-04-20T15:17:33Z

rust-runtime/aws-smithy-http/src/middleware.rs

@@ -132,7 +134,15 @@ where
    };

    let http_response = http::Response::from_parts(parts, Bytes::from(body));
-    trace!(http_response = ?http_response, "read HTTP response body");
+    if !handler.sensitive()
+        || std::env::var(LOG_SENSITIVE_BODIES)


Nice to provide an escape hatch to still be able to print.

Velfi · 2023-04-20T15:58:05Z

aws/rust-runtime/aws-config/src/lib.rs

@@ -417,7 +417,7 @@ mod loader {
        ///
        /// # Example: Using a custom profile name
        ///
-        /// ```
+        /// ```no_run


Why don't we want to run these anymore?

we shouldn't be running doc tests ever, these were misses. They should only be compiled—running them will try to load up an SSL chain etc. and makes them very slow

We should probably do a pass (as a separate PR) and check that we aren't running them elsewhere.

Velfi · 2023-04-20T16:00:28Z

aws/rust-runtime/aws-config/src/test_case.rs

+/// *Why use this instead of traced_test?*
+/// This captures _all_ logs, not just logs produced by the current crate.


ty for adding this comment! It's exactly what I would have asked.

Velfi · 2023-04-20T16:02:45Z

aws/rust-runtime/aws-sigv4/src/http_request/sign.rs

@@ -256,7 +256,7 @@ fn calculate_signing_headers<'a>(
    // Step 4: https://docs.aws.amazon.com/en_pv/general/latest/gr/sigv4-add-signature-to-request.html
    let values = creq.values.as_headers().expect("signing with headers");
    let mut headers = HeaderMap::new();
-    add_header(&mut headers, header::X_AMZ_DATE, &values.date_time);
+    add_header(&mut headers, header::X_AMZ_DATE, &values.date_time, false);


It may be worth creating an enum for is_sensitive in order to make this easier to understand.

jdisanti · 2023-04-20T16:25:19Z

CHANGELOG.next.toml

+[[aws-sdk-rust]]
+message = """Reduce several instances of credential exposure in the SDK logs:
+- IMDS now suppresses the body of the response from logs
+- `aws-sigv4` marks the `x-amz-session-token` header as sensitive
+- STS & SSO credentials have been manually marked as sensitive which suppresses logging of response bodies for relevant operations
+"""


Is this going to render correctly? Seems like the sub bullet points might become top-level bullet points in the rendered changelog.

## Motivation and Context - credentials providers may leak credentials in the HTTP body at the debug level ## Description This adds a test to aws-config that looks for leaked credentials in all of our provider integration tests—since these test use AWS APIs under the hood, this also serves to test AWS services in general. To support this, `sensitive` was added to the ParseHttpResponse trait and code was generated to take action based on this change. - [x] Add environment variable to force logging of the body - [x] consider if we want to suppress request body logging as well ## Testing    ## Checklist  - [x] I have updated `CHANGELOG.next.toml` if I made changes to the smithy-rs codegen or runtime crates - [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>

## Motivation and Context Fixes #2926 ## Description This PR ports logic implemented in #2603. Thankfully, even though we did not port this at the time of the orchestrator launch, the orchestrator has not logged sensitive bodies because we have never logged response bodies in the orchestrator code. The code changes in this PR - now logs response bodies in `try_attempt` - ports the logic from the previous PR in question to the orchestrator, via an interceptor Now, when credentials providers in `aws_config` need to say "I want to redact a response body" ([example](https://github.com/awslabs/smithy-rs/blob/2c27834f90b0585dba60606f9da6246b341227d6/aws/rust-runtime/aws-config/src/http_credential_provider.rs#L48)) when middleware is gone, they can pass an interceptor `SensitiveOutputInterceptor` to `Config` of whatever clients they are using. ## Testing Depends on the existing tests. Without the logic ported over the orchestrator and by logging response bodies unconditionally in `try_attempt`, we got the following failures. After we've ported the logic, they now pass. ``` default_provider::credentials::test::ecs_assume_role default_provider::credentials::test::imds_assume_role default_provider::credentials::test::sso_assume_role default_provider::credentials::test::web_identity_token_env default_provider::credentials::test::web_identity_token_profile default_provider::credentials::test::web_identity_token_source_profile profile::credentials::test::e2e_assume_role profile::credentials::test::region_override profile::credentials::test::retry_on_error ``` ## Checklist  - [x] I have updated `CHANGELOG.next.toml` if I made changes to the smithy-rs codegen or runtime crates - [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: ysaito1001 <awsaito@amazon.com> Co-authored-by: John DiSanti <jdisanti@amazon.com>

rcoh force-pushed the credentials-exposure-test branch from a777342 to e750d89 Compare April 19, 2023 18:55

jdisanti reviewed Apr 20, 2023

View reviewed changes

aws/rust-runtime/aws-config/test-data/profile-provider/credential_process/fs/home/.aws/config Outdated Show resolved Hide resolved

aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/customize/sso/SSODecorator.kt Show resolved Hide resolved

rcoh and others added 3 commits April 20, 2023 08:33

Update aws/rust-runtime/aws-config/test-data/profile-provider/credent…

c800e5c

…ial_process/fs/home/.aws/config Co-authored-by: John DiSanti <jdisanti@amazon.com>

Add changelog entry, enable printing via env var

75461b9

rcoh force-pushed the credentials-exposure-test branch from cd1a864 to 75461b9 Compare April 20, 2023 12:33

rcoh marked this pull request as ready for review April 20, 2023 12:37

rcoh requested review from a team as code owners April 20, 2023 12:37

rcoh requested review from pose and crisidev April 20, 2023 12:37

rcoh added the needs-sdk-review label Apr 20, 2023

ysaito1001 approved these changes Apr 20, 2023

View reviewed changes

ysaito1001 removed the needs-sdk-review label Apr 20, 2023

Velfi approved these changes Apr 20, 2023

View reviewed changes

jdisanti reviewed Apr 20, 2023

View reviewed changes

rcoh added this pull request to the merge queue Apr 20, 2023

Merged via the queue into main with commit 9a41e35 Apr 20, 2023

rcoh deleted the credentials-exposure-test branch April 20, 2023 17:54

This was referenced Sep 1, 2023

Redacting sensitive HTTP responses lost in orchestrator migration #2926

Closed

Port redacting sensitive body to orchestrator #2972

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add credentials exposure test & fix STS + SSO #2603

Add credentials exposure test & fix STS + SSO #2603

rcoh commented Apr 19, 2023 •

edited

Loading

github-actions bot commented Apr 19, 2023

jdisanti left a comment

github-actions bot commented Apr 20, 2023

ysaito1001 left a comment

ysaito1001 Apr 20, 2023

Velfi Apr 20, 2023

rcoh Apr 20, 2023

Velfi Apr 20, 2023

Velfi Apr 20, 2023

Velfi Apr 20, 2023

jdisanti Apr 20, 2023

		/// Why use this instead of traced_test?
		/// This captures _all_ logs, not just logs produced by the current crate.

Add credentials exposure test & fix STS + SSO #2603

Add credentials exposure test & fix STS + SSO #2603

Conversation

rcoh commented Apr 19, 2023 • edited Loading

Motivation and Context

Description

Testing

Checklist

github-actions bot commented Apr 19, 2023

jdisanti left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 20, 2023

ysaito1001 left a comment

Choose a reason for hiding this comment

ysaito1001 Apr 20, 2023

Choose a reason for hiding this comment

Velfi Apr 20, 2023

Choose a reason for hiding this comment

rcoh Apr 20, 2023

Choose a reason for hiding this comment

Velfi Apr 20, 2023

Choose a reason for hiding this comment

Velfi Apr 20, 2023

Choose a reason for hiding this comment

Velfi Apr 20, 2023

Choose a reason for hiding this comment

jdisanti Apr 20, 2023

Choose a reason for hiding this comment

rcoh commented Apr 19, 2023 •

edited

Loading