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

Port redacting sensitive body to orchestrator #2972

Merged
merged 12 commits into from
Sep 8, 2023

Conversation

ysaito1001
Copy link
Contributor

@ysaito1001 ysaito1001 commented Sep 5, 2023

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) 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

  • 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.

@ysaito1001 ysaito1001 requested a review from a team as a code owner September 5, 2023 20:48
@github-actions
Copy link

github-actions bot commented Sep 5, 2023

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

github-actions bot commented Sep 6, 2023

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

github-actions bot commented Sep 6, 2023

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

github-actions bot commented Sep 8, 2023

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link
Collaborator

@jdisanti jdisanti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cargo-semver-checks is saying this is a breaking change, but I don't see it...

@@ -36,7 +36,8 @@ use tracing::{debug, debug_span, instrument, trace, Instrument};
mod auth;
/// Defines types that implement a trait for endpoint resolution
pub mod endpoints;
mod http;
/// Defines types that work with HTTP types
pub mod http;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this doesn't need to be pub anymore.

@jdisanti jdisanti added this pull request to the merge queue Sep 8, 2023
@github-actions
Copy link

github-actions bot commented Sep 8, 2023

A new generated diff is ready to view.

A new doc preview is ready to view.

Merged via the queue into main with commit 0bd57fe Sep 8, 2023
41 checks passed
@jdisanti jdisanti deleted the ysaito/redact-response-body branch September 8, 2023 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redacting sensitive HTTP responses lost in orchestrator migration
3 participants