Unverified Commit 733eab7e authored by Landon James's avatar Landon James Committed by GitHub
Browse files

Fix presigned put bug with flex checksums (#3971) 

## 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 -->
Fixing bug reported in
https://github.com/awslabs/aws-sdk-rust/issues/1240#issuecomment-2594822919

## Description
<!--- Describe your changes in detail -->

## 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. -->
Updated presigning integ tests to account for behavior specified in SEP,
added new test for the user provided checksum case

## 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._
parent 41ff31ba

.changelog/presigning-bugfix.md

0 → 100644
+10 −0
Original line number Diff line number Diff line 
---

applies_to: ["aws-sdk-rust"]

authors: ["landonxjames"]

references: ["aws-sdk-rust#1240"]

breaking: false

new_feature: false

bug_fix: true

---



Fix bug with presigned requests introduced by new flexibile checksums functionality

aws/rust-runtime/aws-inlineable/src/http_request_checksum.rs

+11 −13
Original line number Diff line number Diff line
@@ -193,24 +193,22 @@ where 
            .load::<RequestChecksumCalculation>()

            .unwrap_or(&RequestChecksumCalculation::WhenSupported);



        // Determine if we actually calculate the checksum. If the user setting is WhenSupported (the default)

        // we always calculate it (because this interceptor isn't added if it isn't supported). If it is

        // WhenRequired we only calculate it if the checksum is marked required on the trait.

        let calculate_checksum = match request_checksum_calculation {

            RequestChecksumCalculation::WhenRequired => request_checksum_required,

            RequestChecksumCalculation::WhenSupported => true,

        // Need to know if this is a presigned req because we do not calculate checksums for those.

        let is_presigned_req = cfg.load::<PresigningMarker>().is_some();



        // Determine if we actually calculate the checksum. If this is a presigned request we do not

        // If the user setting is WhenSupported (the default) we always calculate it (because this interceptor

        // isn't added if it isn't supported). If it is WhenRequired we only calculate it if the checksum

        // is marked required on the trait.

        let calculate_checksum = match (request_checksum_calculation, is_presigned_req) {

            (_, true) => false,

            (RequestChecksumCalculation::WhenRequired, false) => request_checksum_required,

            (RequestChecksumCalculation::WhenSupported, false) => true,

            _ => true,

        };



        // Calculate the checksum if necessary

        if calculate_checksum {

            let is_presigned_req = cfg.load::<PresigningMarker>().is_some();



            // If this is a presigned request and the user has not set a checksum we short circuit

            if is_presigned_req && checksum_algorithm.is_none() {

                return Ok(());

            }



            // If a checksum override is set in the ConfigBag we use that instead (currently only used by S3Express)

            // If we have made it this far without a checksum being set we set the default (currently Crc32)

            let checksum_algorithm =

aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/HttpRequestChecksumDecorator.kt

+11 −5
Original line number Diff line number Diff line
@@ -119,8 +119,8 @@ private fun HttpChecksumTrait.checksumAlgorithmToStr( 
            // Checksums are required but a user can't set one, so we set crc32 for them

            rust("""let checksum_algorithm = Some("crc32");""")

        } else {

            // Use checksum algo set by user or crc32 if one has not been set

            rust("""let checksum_algorithm = checksum_algorithm.map(|algorithm| algorithm.as_str()).or(Some("crc32"));""")

            // Use checksum algo set by user

            rust("""let checksum_algorithm = checksum_algorithm.map(|algorithm| algorithm.as_str());""")

        }



        // If a request checksum is not required and there's no way to set one, do nothing
@@ -205,6 +205,9 @@ class HttpRequestChecksumCustomization( 
                                    // From the httpChecksum trait

                                    let http_checksum_required = $requestChecksumRequired;



                                    let is_presigned_req = cfg.load::<#{PresigningMarker}>().is_some();



                                    // If the request is presigned we do not set a default.

                                    // If the RequestChecksumCalculation is WhenSupported and the user has not set a checksum value or algo

                                    // we default to Crc32. If it is WhenRequired and a checksum is required by the trait and the user has not

                                    // set a checksum value or algo we also set the default. In all other cases we do nothing.
@@ -212,10 +215,12 @@ class HttpRequestChecksumCustomization( 
                                        request_checksum_calculation,

                                        http_checksum_required,

                                        user_set_checksum_value,

                                        user_set_checksum_algo

                                        user_set_checksum_algo,

                                        is_presigned_req,

                                    ) {

                                        (#{RequestChecksumCalculation}::WhenSupported, _, false, false)

                                        | (#{RequestChecksumCalculation}::WhenRequired, true, false, false) => {

                                        (_, _, _, _, true) => {}

                                        (#{RequestChecksumCalculation}::WhenSupported, _, false, false, _)

                                        | (#{RequestChecksumCalculation}::WhenRequired, true, false, false, _) => {

                                            request.headers_mut().insert(${requestAlgoHeader.dq()}, "CRC32");

                                        }

                                        _ => {},
@@ -254,6 +259,7 @@ class HttpRequestChecksumCustomization( 
                                            codegenContext.model,

                                        ),

                                    ),

                                "PresigningMarker" to AwsRuntimeType.presigning().resolve("PresigningMarker"),

                            )

                        }

                    }

aws/sdk/aws-models/s3.json

+36242 −34087

File changed.

Preview size limit exceeded, changes collapsed.

aws/sdk/integration-tests/s3/tests/presigning.rs

+38 −6
Original line number Diff line number Diff line
@@ -103,6 +103,10 @@ async fn test_presigning() { 
    assert_eq!(presigned.headers().count(), 1);

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

}



#[tokio::test]
@@ -137,8 +141,8 @@ async fn test_presigning_with_payload_headers() { 
            "X-Amz-Date=20090213T233131Z",

            "X-Amz-Expires=30",

            "X-Amz-Security-Token=notarealsessiontoken",

            "X-Amz-Signature=9403eb5961c8af066f2473f88cb9248b39fd61f8eedc33af14ec9c2d26c22974",

            "X-Amz-SignedHeaders=content-length%3Bcontent-type%3Bhost%3Bx-amz-checksum-crc32%3Bx-amz-sdk-checksum-algorithm",

            "X-Amz-Signature=be1d41dc392f7019750e4f5e577234fb9059dd20d15f6a99734196becce55e52",

            "X-Amz-SignedHeaders=content-length%3Bcontent-type%3Bhost",

            "x-id=PutObject"

        ][..],

        &query_params
@@ -150,9 +154,12 @@ async fn test_presigning_with_payload_headers() { 
        Some(&"application/x-test")

    );

    assert_eq!(headers.get(CONTENT_LENGTH.as_str()), Some(&"12345"));

    assert_eq!(headers.get("x-amz-sdk-checksum-algorithm"), Some(&"CRC32"));

    assert_eq!(headers.get("x-amz-checksum-crc32"), Some(&"AAAAAA=="));

    assert_eq!(headers.len(), 4);



    // 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.len(), 2);

}



#[tokio::test]
@@ -168,7 +175,7 @@ async fn test_presigned_upload_part() { 
    })

    .await;

    pretty_assertions::assert_eq!(

        "https://bucket.s3.us-east-1.amazonaws.com/key?x-id=UploadPart&partNumber=0&uploadId=upload-id&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=ANOTREAL%2F20090213%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20090213T233131Z&X-Amz-Expires=30&X-Amz-SignedHeaders=content-length%3Bhost%3Bx-amz-checksum-crc32%3Bx-amz-sdk-checksum-algorithm&X-Amz-Signature=0b5835e056c463d6c0963326966f6cf42c75b7a218057836274d38288e055d36&X-Amz-Security-Token=notarealsessiontoken",

        "https://bucket.s3.us-east-1.amazonaws.com/key?x-id=UploadPart&partNumber=0&uploadId=upload-id&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=ANOTREAL%2F20090213%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20090213T233131Z&X-Amz-Expires=30&X-Amz-SignedHeaders=content-length%3Bhost&X-Amz-Signature=a702867244f0bd1fb4d161e2a062520dcbefae3b9992d2e5366bcd61a60c6ddd&X-Amz-Security-Token=notarealsessiontoken",

        presigned.uri().to_string(),

    );

}
@@ -199,3 +206,28 @@ async fn test_presigned_head_object() { 
        presigned.uri().to_string(),

    );

}



#[tokio::test]

async fn test_presigned_user_provided_checksum() {

    let presigned = presign(|client| {

        client

            .put_object()

            .checksum_crc64_nvme("NotARealChecksum")

            .bucket("test-bucket")

            .key("test-key")

    })

    .await;



    // The x-amz-checksum-crc64nvme header is added to the signed headers

    pretty_assertions::assert_eq!(

        "https://test-bucket.s3.us-east-1.amazonaws.com/test-key?x-id=PutObject&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=ANOTREAL%2F20090213%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20090213T233131Z&X-Amz-Expires=30&X-Amz-SignedHeaders=host%3Bx-amz-checksum-crc64nvme&X-Amz-Signature=40e6ea102769a53f440db587be0b6898893d9a0f8268d2f8d2315ca0abc42fee&X-Amz-Security-Token=notarealsessiontoken",

        presigned.uri().to_string(),

    );



    // Checksum value header is persisted into the request

    let headers = presigned.headers().collect::<HashMap<_, _>>();

    assert_eq!(

        headers.get("x-amz-checksum-crc64nvme"),

        Some(&"NotARealChecksum")

    );

}