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

GetObject set_checksum_mode does not remove x-amz-checksum-mode header #1246

Open
1 task
rogusdev opened this issue Jan 19, 2025 · 4 comments
Open
1 task
Labels
bug This issue is a bug. needs-triage This issue or PR still needs to be triaged.

Comments

@rogusdev
Copy link

Describe the bug

I need to remove the x-amz-checksum-mode header for talking to an s3 compatible data store. I would expect get_object().set_checksum_mode(None) to achieve this. However, it does not appear to do so. (There is a workaround I give at the bottom, but I would like this SDK to work as I would expect.)

I would happily put up a PR to fix this, except

  1. I don't see anything wrong with the function itself (
    pub fn set_checksum_mode(mut self, input: ::std::option::Option<crate::types::ChecksumMode>) -> Self {
    +
    pub fn set_checksum_mode(mut self, input: ::std::option::Option<crate::types::ChecksumMode>) -> Self {
    )
  2. I don't know how that field gets turned into the actual header in this lib (maybe
    builder = builder.header("x-amz-checksum-mode", header_value);
    but that looks like it should work, since the field should be None so the conditional should never assign that header).

Please correct me if I am simply using the wrong function to achieve my goal, or there is some other point that I am missing, thanks.

Regression Issue

  • Select this option if this issue appears to be a regression.

Expected Behavior

There is no x-amx-checksum-mode header in the request.

Current Behavior

The HTTP request headers include x-amz-checksum-mode = ENABLED

Reproduction Steps

Full repro steps:

cargo new s3checksum
cd s3checksum/
cargo add aws_config -F behavior-version-latest
cargo add aws_sdk_s3
cargo add tokio -F full

With this main.rs

use aws_sdk_s3::Client;

#[tokio::main]
async fn main() {
    let config = aws_config::load_from_env().await;
    let client = Client::new(&config);

    let bucket = "bucket";
    let key = "folder/file";

    client
        .get_object()
        .bucket(bucket)
        .key(key)
        .set_checksum_mode(None)
        .customize()
        .mutate_request(|request| {
            for (key, value) in request.headers().iter() {
                println!("Header: {} => {:?}", key, value);
            }
        })
        .send()
        .await
        .expect(&format!(
            "Failed to get s3 object '{key}' from bucket '{bucket}'"
        ));
}

Will give output like:

Header: x-amz-checksum-mode => "ENABLED"
Header: user-agent => "aws-sdk-rust/1.3.4 os/linux lang/rust/1.84.0"
Header: x-amz-user-agent => "aws-sdk-rust/1.3.4 ua/2.1 api/s3/1.69.0 os/linux lang/rust/1.84.0 m/E,b md/http#hyper-0.x"

Possible Solution

Perhaps somewhere something is overriding the value set_checksum_mode sets?

Additional Information/Context

There is a workaround:

        .mutate_request(|request| {
            request.headers_mut().remove("x-amz-checksum-mode");
        })

But that seems like it is doing only exactly what .set_checksum_mode(None) should accomplish, I would think. And, tbh, figuring out how to do this was quite unintuitive -- I would prefer that the fn that does the thing, actually does the thing.

Version

cargo tree | grep aws-
├── aws-config v1.5.14
│   ├── aws-credential-types v1.2.1
│   │   ├── aws-smithy-async v1.2.4
│   │   ├── aws-smithy-runtime-api v1.7.3
│   │   │   ├── aws-smithy-async v1.2.4 (*)
│   │   │   ├── aws-smithy-types v1.2.12
│   │   ├── aws-smithy-types v1.2.12 (*)
│   ├── aws-runtime v1.5.4
│   │   ├── aws-credential-types v1.2.1 (*)
│   │   ├── aws-sigv4 v1.2.7
│   │   │   ├── aws-credential-types v1.2.1 (*)
│   │   │   ├── aws-smithy-eventstream v0.60.6
│   │   │   │   ├── aws-smithy-types v1.2.12 (*)
│   │   │   ├── aws-smithy-http v0.60.12
│   │   │   │   ├── aws-smithy-eventstream v0.60.6 (*)
│   │   │   │   ├── aws-smithy-runtime-api v1.7.3 (*)
│   │   │   │   ├── aws-smithy-types v1.2.12 (*)
│   │   │   ├── aws-smithy-runtime-api v1.7.3 (*)
│   │   │   ├── aws-smithy-types v1.2.12 (*)
│   │   ├── aws-smithy-async v1.2.4 (*)
│   │   ├── aws-smithy-eventstream v0.60.6 (*)
│   │   ├── aws-smithy-http v0.60.12 (*)
│   │   ├── aws-smithy-runtime v1.7.7
│   │   │   ├── aws-smithy-async v1.2.4 (*)
│   │   │   ├── aws-smithy-http v0.60.12 (*)
│   │   │   ├── aws-smithy-runtime-api v1.7.3 (*)
│   │   │   ├── aws-smithy-types v1.2.12 (*)
│   │   ├── aws-smithy-runtime-api v1.7.3 (*)
│   │   ├── aws-smithy-types v1.2.12 (*)
│   │   ├── aws-types v1.3.4
│   │   │   ├── aws-credential-types v1.2.1 (*)
│   │   │   ├── aws-smithy-async v1.2.4 (*)
│   │   │   ├── aws-smithy-runtime-api v1.7.3 (*)
│   │   │   ├── aws-smithy-types v1.2.12 (*)
│   ├── aws-sdk-sso v1.54.0
│   │   ├── aws-credential-types v1.2.1 (*)
│   │   ├── aws-runtime v1.5.4 (*)
│   │   ├── aws-smithy-async v1.2.4 (*)
│   │   ├── aws-smithy-http v0.60.12 (*)
│   │   ├── aws-smithy-json v0.61.2
│   │   │   └── aws-smithy-types v1.2.12 (*)
│   │   ├── aws-smithy-runtime v1.7.7 (*)
│   │   ├── aws-smithy-runtime-api v1.7.3 (*)
│   │   ├── aws-smithy-types v1.2.12 (*)
│   │   ├── aws-types v1.3.4 (*)
│   ├── aws-sdk-ssooidc v1.55.0
│   │   ├── aws-credential-types v1.2.1 (*)
│   │   ├── aws-runtime v1.5.4 (*)
│   │   ├── aws-smithy-async v1.2.4 (*)
│   │   ├── aws-smithy-http v0.60.12 (*)
│   │   ├── aws-smithy-json v0.61.2 (*)
│   │   ├── aws-smithy-runtime v1.7.7 (*)
│   │   ├── aws-smithy-runtime-api v1.7.3 (*)
│   │   ├── aws-smithy-types v1.2.12 (*)
│   │   ├── aws-types v1.3.4 (*)
│   ├── aws-sdk-sts v1.55.0
│   │   ├── aws-credential-types v1.2.1 (*)
│   │   ├── aws-runtime v1.5.4 (*)
│   │   ├── aws-smithy-async v1.2.4 (*)
│   │   ├── aws-smithy-http v0.60.12 (*)
│   │   ├── aws-smithy-json v0.61.2 (*)
│   │   ├── aws-smithy-query v0.60.7
│   │   │   ├── aws-smithy-types v1.2.12 (*)
│   │   ├── aws-smithy-runtime v1.7.7 (*)
│   │   ├── aws-smithy-runtime-api v1.7.3 (*)
│   │   ├── aws-smithy-types v1.2.12 (*)
│   │   ├── aws-smithy-xml v0.60.9
│   │   ├── aws-types v1.3.4 (*)
│   ├── aws-smithy-async v1.2.4 (*)
│   ├── aws-smithy-http v0.60.12 (*)
│   ├── aws-smithy-json v0.61.2 (*)
│   ├── aws-smithy-runtime v1.7.7 (*)
│   ├── aws-smithy-runtime-api v1.7.3 (*)
│   ├── aws-smithy-types v1.2.12 (*)
│   ├── aws-types v1.3.4 (*)
├── aws-sdk-s3 v1.69.0
│   ├── aws-credential-types v1.2.1 (*)
│   ├── aws-runtime v1.5.4 (*)
│   ├── aws-sigv4 v1.2.7 (*)
│   ├── aws-smithy-async v1.2.4 (*)
│   ├── aws-smithy-checksums v0.62.0
│   │   ├── aws-smithy-http v0.60.12 (*)
│   │   ├── aws-smithy-types v1.2.12 (*)
│   ├── aws-smithy-eventstream v0.60.6 (*)
│   ├── aws-smithy-http v0.60.12 (*)
│   ├── aws-smithy-json v0.61.2 (*)
│   ├── aws-smithy-runtime v1.7.7 (*)
│   ├── aws-smithy-runtime-api v1.7.3 (*)
│   ├── aws-smithy-types v1.2.12 (*)
│   ├── aws-smithy-xml v0.60.9 (*)
│   ├── aws-types v1.3.4 (*)

Environment details (OS name and version, etc.)

WSL2 Ubuntu in windows 11

Logs

No response

@rogusdev rogusdev added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jan 19, 2025
@rogusdev
Copy link
Author

Perhaps an additional question: why is checksum mode enabled by default? Is that actually undesirable? Or perhaps the field is indeed actually turned off by default, but somehow getting added to the headers anyway, ignoring the actual field?...

@landonxjames
Copy link
Contributor

landonxjames commented Jan 20, 2025

The default behavior around S3 checksums has recently changed in all AWS SDKs, see the announcement for the Rust SDK at #1240

What you likely want is to set the new option response_checksum_calculation to ResponseChecksumValidation::WhenRequired (see documentation on that enum for more specifics) so that checksums are only validated on operations that require it.

@rogusdev
Copy link
Author

rogusdev commented Jan 20, 2025

Thanks, this works:

    let mut builder = aws_config::load_from_env().await.to_builder();
    builder.set_response_checksum_validation(Some(ResponseChecksumValidation::WhenRequired));
    let config = builder.build();

(My original comment was wrong: I was supposed to be using WhenRequired (my brain heard "Required" as mandatory and "Supported" as optional, when in fact it is the reverse here). Switching to that fixed things.)

I am still curious how checksum_mode gets set by set_response_checksum_validation such that it is overriding set_checksum_mode. Because now I want to know what the point of set_checksum_mode is. Or should it be deprecated?

@landonxjames
Copy link
Contributor

There is some codegenerated logic around mutating that value: https://github.com/smithy-lang/smithy-rs/blob/e41f7d7dfa84b38b1524823bdcac56f0dcc9dddc/aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/HttpResponseChecksumDecorator.kt#L155-L176

You can see the closure generated by that code for GetObject below. If checksum_mode is not explicitly enabled by the user we look at the response_checksum_validation value from the config and change it based on that setting.

|input: &mut ::aws_smithy_runtime_api::client::interceptors::context::Input, cfg: &::aws_smithy_types::config_bag::ConfigBag| {
let input = input
.downcast_mut::<crate::operation::get_object::GetObjectInput>()
.ok_or("failed to downcast to crate::operation::get_object::GetObjectInput")?;
let request_validation_enabled = matches!(input.checksum_mode(), Some(crate::types::ChecksumMode::Enabled));
if !request_validation_enabled {
// This value is set by the user on the SdkConfig to indicate their preference
let response_checksum_validation = cfg
.load::<::aws_smithy_types::checksum_config::ResponseChecksumValidation>()
.unwrap_or(&::aws_smithy_types::checksum_config::ResponseChecksumValidation::WhenSupported);
// 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).
#[allow(clippy::wildcard_in_or_patterns)]
match response_checksum_validation {
::aws_smithy_types::checksum_config::ResponseChecksumValidation::WhenRequired => {}
::aws_smithy_types::checksum_config::ResponseChecksumValidation::WhenSupported | _ => {
input.checksum_mode = Some(crate::types::ChecksumMode::Enabled);
}
}
}
::std::result::Result::Ok(())
},

You can see the Smithy spec around this functionality here: https://smithy.io/2.0/aws/aws-core.html#aws-protocols-httpchecksum-trait

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. needs-triage This issue or PR still needs to be triaged.
Projects
None yet
Development

No branches or pull requests

2 participants