Unverified Commit 745d48a7 authored by Russell Cohen's avatar Russell Cohen Committed by GitHub
Browse files

Add a RuntimePlugin/Interceptor to enforce expected content length (#3491) 

## 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 -->
There is a rarely-triggered bug
(https://github.com/awslabs/aws-sdk-rust/issues/1079) that can occur
when the runtime is dropped between requests. Although this is
definitely the _wrong thing to do_(tm) which should still aim to at
least protect the users from bad data in this case.

This adds an interceptor which validates that the body returned is the
correct length.

## Description
- Adds an interceptor that computes the actual content length of the
body and compares it to the actual length

## Testing
- Integration style test. Note that this is very hard to test using
Hyper because Hyper will attempt to mitigate this issue.


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


----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
parent 1487391b

CHANGELOG.next.toml

+7 −1
Original line number Diff line number Diff line
@@ -10,3 +10,9 @@ 
# references = ["smithy-rs#920"]

# meta = { "breaking" = false, "tada" = false, "bug" = false, "target" = "client | server | all"}

# author = "rcoh"



[[smithy-rs]]

message = "Clients now enforce that the Content-Length sent by the server matches the length of the returned response body. In most cases, Hyper will enforce this behavior, however, in extremely rare circumstances where the Tokio runtime is dropped in between subsequent requests, this scenario can occur."

references = ["smithy-rs#3491", "aws-sdk-rust#1079"]

meta = { "breaking" = false, "bug" = true, "tada" = false }

author = "rcoh"

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

+2 −1
Original line number Diff line number Diff line
@@ -271,7 +271,8 @@ where 
        let fs = Fs::from_test_dir(dir.join("fs"), "/");

        let network_traffic = std::fs::read_to_string(dir.join("http-traffic.json"))

            .map_err(|e| format!("failed to load http traffic: {}", e))?;

        let network_traffic: NetworkTraffic = serde_json::from_str(&network_traffic)?;

        let mut network_traffic: NetworkTraffic = serde_json::from_str(&network_traffic)?;

        network_traffic.correct_content_lengths();



        let metadata: Metadata<O> = serde_json::from_str(

            &std::fs::read_to_string(dir.join("test-case.json"))

aws/sdk/integration-tests/dynamodb/tests/retries-with-client-rate-limiting.rs

+3 −2
Original line number Diff line number Diff line
@@ -21,14 +21,15 @@ fn req() -> http::Request<SdkBody> { 
}



fn ok() -> http::Response<SdkBody> {

    let body = "{ \"TableNames\": [ \"Test\" ] }";

    http::Response::builder()

        .status(200)

        .header("server", "Server")

        .header("content-type", "application/x-amz-json-1.0")

        .header("content-length", "23")

        .header("content-length", body.len().to_string())

        .header("connection", "keep-alive")

        .header("x-amz-crc32", "2335643545")

        .body(SdkBody::from("{ \"TableNames\": [ \"Test\" ] }"))

        .body(SdkBody::from(body))

        .unwrap()

}

aws/sdk/integration-tests/s3/tests/data/no_auth/head-object.json

+0 −3
Original line number Diff line number Diff line
@@ -52,9 +52,6 @@ 
                "server": [

                  "AmazonS3"

                ],

                "content-length": [

                  "386910"

                ],

                "accept-ranges": [

                  "bytes"

                ],

aws/sdk/integration-tests/s3/tests/streaming-response.rs

+41 −0
Original line number Diff line number Diff line
@@ -8,12 +8,51 @@ use aws_credential_types::provider::SharedCredentialsProvider; 
use aws_sdk_s3::config::{Credentials, Region};

use aws_sdk_s3::error::DisplayErrorContext;

use aws_sdk_s3::Client;

use aws_smithy_runtime::assert_str_contains;

use aws_smithy_runtime::client::http::test_util::infallible_client_fn;

use aws_smithy_types::body::SdkBody;

use bytes::BytesMut;

use http::header::CONTENT_LENGTH;

use std::future::Future;

use std::net::SocketAddr;

use std::time::Duration;

use tracing::debug;



#[tokio::test]

async fn test_too_short_body_causes_an_error() {

    // this is almost impossible to reproduce with Hyper—you need to do stuff like run each request

    // in its own async runtime. But there's no reason a customer couldn't run their _own_ HttpClient

    // that was more poorly behaved, so we'll do that here.

    let http_client = infallible_client_fn(|_req| {

        http::Response::builder()

            .header(CONTENT_LENGTH, 5000)

            .body(SdkBody::from("definitely not 5000 characters"))

            .unwrap()

    });



    let client = aws_sdk_s3::Client::from_conf(

        aws_sdk_s3::Config::builder()

            .with_test_defaults()

            .region(Region::new("us-east-1"))

            .http_client(http_client)

            .build(),

    );



    let content = client

        .get_object()

        .bucket("some-test-bucket")

        .key("test.txt")

        .send()

        .await

        .unwrap()

        .body;

    let error = content.collect().await.expect_err("content too short");

    assert_str_contains!(

        format!("{}", DisplayErrorContext(error)),

        "Invalid Content-Length: Expected 5000 bytes but 30 bytes were received"

    );

}



// test will hang forever with the default (single-threaded) test executor

#[tokio::test(flavor = "multi_thread")]

async fn test_streaming_response_fails_when_eof_comes_before_content_length_reached() {
@@ -48,6 +87,8 @@ async fn test_streaming_response_fails_when_eof_comes_before_content_length_reac 
            message.contains(expected),

            "Expected `{message}` to contain `{expected}`"

        );

    } else {

        panic!("response did not error")

    }

}