Skip to content

Commit

Permalink
Fix s3 checksum-mode header in presigned requests (#4000)
Browse files Browse the repository at this point in the history
## 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 -->

Fixes a bug reported in
awslabs/aws-sdk-rust#1253 with flexible
checksum headers sneaking into presigned requests.

## Description
<!--- Describe your changes in detail -->
Short circuit the closure that mutates the `checksum-mode` header when
we detect that we are making a presigned request.

## Testing
<!--- Please describe in detail how you tested your changes -->
<!--- Include details of your testing environment, and the tests you ran
to -->
<!--- see how your change affects other areas of the code, etc. -->
Update our presigned canary tests to do both PUT and GET and have extra
headers included that aren't in the query string that will expose this
issue if there is a regression.


## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [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._
  • Loading branch information
landonxjames authored Feb 7, 2025
1 parent 61007da commit 8039247
Show file tree
Hide file tree
Showing 6 changed files with 59 additions and 20 deletions.
10 changes: 10 additions & 0 deletions .changelog/presigned-request-fix.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
---
applies_to: ["aws-sdk-rust"]
authors: ["landonxjames"]
references: ["aws-sdk-rust#1253"]
breaking: false
new_feature: false
bug_fix: true
---

Fix bug in S3 with flexible checksum headers incorrectly included in presigned GET requests.
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,13 @@ class HttpResponseChecksumCustomization(
.load::<#{ResponseChecksumValidation}>()
.unwrap_or(&#{ResponseChecksumValidation}::WhenSupported);
let is_presigned_req = cfg.load::<#{PresigningMarker}>().is_some();
// For presigned requests we do not enable the checksum-mode header.
if is_presigned_req {
return #{Ok}(())
}
// If validation setting is WhenSupported (or unknown) we enable response checksum
// validation. If it is WhenRequired we do not enable (since there is no way to
// indicate that a response checksum is required).
Expand Down Expand Up @@ -206,6 +213,7 @@ class HttpResponseChecksumCustomization(
CargoDependency.smithyTypes(codegenContext.runtimeConfig).toType()
.resolve("checksum_config::ResponseChecksumValidation"),
"ConfigBag" to RuntimeType.configBag(codegenContext.runtimeConfig),
"PresigningMarker" to AwsRuntimeType.presigning().resolve("PresigningMarker"),
)
}
}
Expand Down
7 changes: 2 additions & 5 deletions aws/sdk/integration-tests/s3/tests/express.rs
Original file line number Diff line number Diff line change
Expand Up @@ -182,11 +182,8 @@ async fn presigning() {
][..],
&query_params
);
// Presigned request has one header and that is the x-amz-checksum-mode
// header with default value ENABLED
assert_eq!(presigned.headers().count(), 1);
let headers = presigned.headers().collect::<Vec<(&str, &str)>>();
assert_eq!(headers.get(0).unwrap(), &("x-amz-checksum-mode", "ENABLED"));
// Presigned request has no headers by default
assert_eq!(presigned.headers().count(), 0);
}

fn operation_request_with_checksum(
Expand Down
4 changes: 2 additions & 2 deletions aws/sdk/integration-tests/s3/tests/presigning.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,13 +100,13 @@ async fn test_presigning() {
][..],
&query_params
);
assert_eq!(presigned.headers().count(), 1);
assert_eq!(presigned.headers().count(), 0);
let headers = presigned.headers().collect::<HashMap<_, _>>();
assert_eq!(headers.get("x-amz-checksum-mode").unwrap(), &"ENABLED");

// Checksum headers should not be included by default in presigned requests
assert_eq!(headers.get("x-amz-sdk-checksum-algorithm"), None);
assert_eq!(headers.get("x-amz-checksum-crc32"), None);
assert_eq!(headers.get("x-amz-checksum-mode"), None);
}

#[tokio::test]
Expand Down
38 changes: 31 additions & 7 deletions tools/ci-cdk/canary-lambda/src/latest/s3_canary.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ use crate::{mk_canary, CanaryEnv};
use anyhow::{Context, Error};
use aws_config::SdkConfig;
use aws_sdk_s3 as s3;
use aws_sdk_s3::types::{CompletedMultipartUpload, CompletedPart, Delete, ObjectIdentifier};
use aws_sdk_s3::types::{
CompletedMultipartUpload, CompletedPart, Delete, ObjectIdentifier, RequestPayer,
};
use s3::config::Region;
use s3::presigning::PresigningConfig;
use s3::primitives::ByteStream;
Expand All @@ -31,6 +33,8 @@ mk_canary!("s3", |sdk_config: &SdkConfig, env: &CanaryEnv| {
/// Runs canary exercising S3 APIs against a regular bucket
pub async fn s3_canary(client: s3::Client, s3_bucket_name: String) -> anyhow::Result<()> {
let test_key = Uuid::new_v4().as_u128().to_string();
let mut presigned_test_key = test_key.clone();
presigned_test_key.push_str("_presigned");

// Look for the test object and expect that it doesn't exist
match client
Expand Down Expand Up @@ -74,21 +78,41 @@ pub async fn s3_canary(client: s3::Client, s3_bucket_name: String) -> anyhow::Re
.await
.context("s3::GetObject[2]")?;

// repeat the test with a presigned url
let uri = client
// Repeat the GET/PUT tests with a presigned url
let reqwest_client = reqwest::Client::new();

let presigned_put = client
.put_object()
.bucket(&s3_bucket_name)
.key(&presigned_test_key)
.presigned(PresigningConfig::expires_in(Duration::from_secs(120)).unwrap())
.await
.unwrap();
let http_put = presigned_put.make_http_1x_request("presigned_test");
let reqwest_put = reqwest::Request::try_from(http_put).unwrap();
let put_resp = reqwest_client.execute(reqwest_put).await?;
assert_eq!(put_resp.status(), 200);

let presigned_get = client
.get_object()
.bucket(&s3_bucket_name)
.key(&test_key)
.key(&presigned_test_key)
// Ensure a header is included that isn't in the query string
.request_payer(RequestPayer::Requester)
.presigned(PresigningConfig::expires_in(Duration::from_secs(120)).unwrap())
.await
.unwrap();
let response = reqwest::get(uri.uri().to_string())
let headers = presigned_get.make_http_1x_request("").headers().clone();
let get_resp = reqwest_client
.get(presigned_get.uri().to_string())
.headers(headers)
.send()
.await
.context("s3::presigned")?
.text()
.await?;
if response != "test" {
return Err(CanaryError(format!("presigned URL returned bad data: {:?}", response)).into());
if get_resp != "presigned_test" {
return Err(CanaryError(format!("presigned URL returned bad data: {:?}", get_resp)).into());
}

let metadata_value = output
Expand Down
12 changes: 6 additions & 6 deletions tools/ci-cdk/canary-runner/src/build_bundle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ tracing-subscriber = { version = "0.3", features = ["fmt", "env-filter"] }
uuid = { version = "0.8", features = ["v4"] }
tokio-stream = "0"
tracing-texray = "0.1.1"
reqwest = { version = "0.11.14", features = ["rustls-tls"], default-features = false }
reqwest = { version = "0.12.12", features = ["rustls-tls"], default-features = false }
edit-distance = "2"
wit-bindgen = { version = "0.16.0", features = ["macros", "realloc"] }
wasmtime = { version = "17.0.1", features = ["component-model"] }
Expand All @@ -153,7 +153,7 @@ arbitrary = "=1.3.2"
lazy_static! {
static ref REQUIRED_SDK_CRATES: Vec<RequiredDependency> = vec![
RequiredDependency::new("aws-config").with_features(["behavior-version-latest"]),
RequiredDependency::new("aws-sdk-s3"),
RequiredDependency::new("aws-sdk-s3").with_features(["http-1x"]),
RequiredDependency::new("aws-sdk-ec2"),
RequiredDependency::new("aws-sdk-transcribestreaming"),
RequiredDependency::new("aws-smithy-wasm"),
Expand Down Expand Up @@ -600,7 +600,7 @@ tracing-subscriber = { version = "0.3", features = ["fmt", "env-filter"] }
uuid = { version = "0.8", features = ["v4"] }
tokio-stream = "0"
tracing-texray = "0.1.1"
reqwest = { version = "0.11.14", features = ["rustls-tls"], default-features = false }
reqwest = { version = "0.12.12", features = ["rustls-tls"], default-features = false }
edit-distance = "2"
wit-bindgen = { version = "0.16.0", features = ["macros", "realloc"] }
wasmtime = { version = "17.0.1", features = ["component-model"] }
Expand All @@ -612,7 +612,7 @@ wasmtime-wasi-http = "17.0.1"
arbitrary = "=1.3.2"
aws-config = { path = "some/sdk/path/aws-config", features = ["behavior-version-latest"] }
aws-sdk-s3 = { path = "some/sdk/path/s3" }
aws-sdk-s3 = { path = "some/sdk/path/s3", features = ["http-1x"] }
aws-sdk-ec2 = { path = "some/sdk/path/ec2" }
aws-sdk-transcribestreaming = { path = "some/sdk/path/transcribestreaming" }
aws-smithy-wasm = { path = "some/sdk/path/aws-smithy-wasm" }
Expand Down Expand Up @@ -663,7 +663,7 @@ tracing-subscriber = { version = "0.3", features = ["fmt", "env-filter"] }
uuid = { version = "0.8", features = ["v4"] }
tokio-stream = "0"
tracing-texray = "0.1.1"
reqwest = { version = "0.11.14", features = ["rustls-tls"], default-features = false }
reqwest = { version = "0.12.12", features = ["rustls-tls"], default-features = false }
edit-distance = "2"
wit-bindgen = { version = "0.16.0", features = ["macros", "realloc"] }
wasmtime = { version = "17.0.1", features = ["component-model"] }
Expand All @@ -675,7 +675,7 @@ wasmtime-wasi-http = "17.0.1"
arbitrary = "=1.3.2"
aws-config = { version = "0.46.0", features = ["behavior-version-latest"] }
aws-sdk-s3 = { version = "0.20.0" }
aws-sdk-s3 = { version = "0.20.0", features = ["http-1x"] }
aws-sdk-ec2 = { version = "0.19.0" }
aws-sdk-transcribestreaming = { version = "0.16.0" }
aws-smithy-wasm = { version = "0.1.0" }
Expand Down

0 comments on commit 8039247

Please sign in to comment.