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

Redacting sensitive HTTP responses lost in orchestrator migration #2926

Closed
jdisanti opened this issue Aug 16, 2023 · 1 comment · Fixed by #2972
Closed

Redacting sensitive HTTP responses lost in orchestrator migration #2926

jdisanti opened this issue Aug 16, 2023 · 1 comment · Fixed by #2972
Assignees
Labels
bug Something isn't working

Comments

@jdisanti
Copy link
Collaborator

jdisanti commented Aug 16, 2023

The middleware implementation had a sensitive() function on the ParseHttpResponse trait that would conditionally redact logging of the HTTP response. This was lost during the orchestrator implementation and needs to be ported over.

Old implementation:
https://github.com/awslabs/smithy-rs/blob/2d61502221d446695f6a0ff9799352ab38ad31cd/rust-runtime/aws-smithy-http/src/response.rs#L64-L69
https://github.com/awslabs/smithy-rs/blob/2d61502221d446695f6a0ff9799352ab38ad31cd/rust-runtime/aws-smithy-http/src/middleware.rs#L137-L145

@jdisanti jdisanti added the bug Something isn't working label Aug 16, 2023
@ysaito1001 ysaito1001 self-assigned this Sep 1, 2023
@ysaito1001
Copy link
Contributor

ysaito1001 commented Sep 1, 2023

The reason credentials exposure detection tests added in #2603 still pass in the orchestrator, even though we haven't ported the pieces of code above to the orcestrator, is that even in the orchestrator, code execution paths still go through aws_smithy_http::middleware::load_response.

As of today, CredentialsProviders in aws_config, such as ImdsCredentialsProvider and HttpCredentialProvider, use a smithy client with middleware (example). This means the pieces of code above are used even in the orchestrator.

Only after middleware has been removed, do credentials exposure detection tests start failing in the orchestrator.

UPDATE
Regarding the statement above

Only after middleware has been removed, do credentials exposure detection tests start failing in the orchestrator.

The tests won't fail because in the orchestrator we do not print request bodies. If we changed the following code in try_attempt in the orchestraor

        match maybe_deserialized {
            Some(output_or_error) => output_or_error,
            None => read_body(response)
                .instrument(debug_span!("read_body"))
                .await
                .map_err(OrchestratorError::response)
                .and_then(|_| {
                    let _span = debug_span!("deserialize_nonstreaming").entered();
                    response_deserializer.deserialize_nonstreaming(response)
                }),
        }

to

        match maybe_deserialized {
            Some(output_or_error) => output_or_error,
            None => read_body(response)
                .instrument(debug_span!("read_body"))
                .await
                .map_err(OrchestratorError::response)
                .and_then(|_| {
                    let _span = debug_span!("deserialize_nonstreaming").entered();
                    trace!(response = ?response); // <--- adds this line to print the body
                    response_deserializer.deserialize_nonstreaming(response)
                }),
        }

then exposure tests will fail (so the tests are working in the orchestrator).

github-merge-queue bot pushed a commit that referenced this issue Sep 8, 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](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
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [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 <[email protected]>
Co-authored-by: John DiSanti <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants