AWSAuthV4Signer::PayloadSigningPolicy::Never not work

[zombee0]

Describe the bug

from the code https://github.com/aws/aws-sdk-cpp/blob/8209a3ed365edb4789d6eb9d5763c08f6bd77d11/src/aws-cpp-sdk-core/source/client/AWSClient.cpp#L561 I see the AWSAuthV4Signer::PayloadSigningPolicy::Never won't work any more, so is this a bug? or by designed? And even in AWSAuthV4Signer https://github.com/aws/aws-sdk-cpp/blob/8209a3ed365edb4789d6eb9d5763c08f6bd77d11/src/aws-cpp-sdk-core/source/auth/signer/AWSAuthV4Signer.cpp#L219 i see singBody has no effect so how does AWSAuthV4Signer::PayloadSigningPolicy::Never work?

Regression Issue

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

Expected Behavior

No signBody

Current Behavior

signBody

Reproduction Steps

Any PutObject

Possible Solution

No response

Additional Information/Context

No response

AWS CPP SDK version used

main

Compiler and Version used

gcc 11.4.0

Operating System and version

ubuntu

[sbiscigl]

I see the AWSAuthV4Signer::PayloadSigningPolicy::Never won't work any more, so is this a bug? or by designed?

it is by design and it relates to the change for default checksum behavior in S3 https://github.com/aws/aws-sdk-cpp/issues/3253

before this change "Payload Signing Policy" essentially meant

if(signBody || !https) {
  request_headers.put("checksum-value", calculate_checksum(request_body))
} else {
  request_headers.put("trailing-checksum", "checksum-alogithm")
  request.notify_http_client_to_chunk_encode()
}

so PayloadSigningPolicy before this change more or less meant Always -> put the checksum in the header RequestDependent -> fallback to what is modeled in the request Never -> put the checksum in the trailer

so to summarize this sign body variable only had a effect if you used checksums which is currently only the s3 client.

the updated logic will always chunk encode requests that support it unless a pre-calculated checksum is provided.

if(streaming_body) {
  request_headers.put("trailing-checksum", "checksum-alogithm")
  request.notify_http_client_to_chunk_sign()
} else {
  request_headers.put("checksum-value", calculate_checksum(request_body))
}

in essence AWSAuthV4Signer::PayloadSigningPolicy::Never is the default behavior now. more details on the behavior can be found in this doc that describes how to disable checksums all together if you want to.

So you are right in that that variable has no effect, we can update the doc on it so notify that it has no effect anymore, but how is it impacting you and how is the current behavior different from what you want?

[zombee0]

@sbiscigl thank you! In my system, I have set AWSAuthV4Signer::PayloadSigningPolicy::Never, and we introduced a new HTTPClient that didn't support chunked upload, it works fine for the prior version, but not now, and now i know why. and we may support chunked upload later in our HTTPClient, but do you think if you split chunked upload code from the HTTPClient that will be better? In my opinion, chunked upload is more related to AWS, and it will be clearer that HTTPClient is only responsible for data transmission.

[sbiscigl]

but do you think if you split chunked upload code from the HTTPClient that will be better? In my opinion, chunked upload is more related to AWS, and it will be clearer that HTTPClient is only responsible for data transmission.

yes you are right and that was a issue in the original implementation where it was made http clients responsibility to handle chunked. we should only be giving http request stream of bytes, and well thats it, just a stream of bytes.

let me change this issue to a feature request and hopefully we can make that adjustment sooner than later

it works fine for the prior version

In the prior version your entire body was checksummed with MD5 in the header by default so this code path was never executed. This entire code path in the prior version was gated off by MD5 being turned on by default. If in the older version you were to set a CRC32 checksum algorithm on the request i.e.

auto put_object_request = PutObjectRequest().WithBucket("bucket_name")
  .WithKey("key")
  .WithChecksumAlgorithm(ChecksumAlgorithm::CRC32);

you would see the same issue i would think.

so for now i would recommend either explicitly setting a checksum, thus settings the objects header, or using one of the ways we document on how to enable "prior" behavior.

[zombee0]

@sbiscigl thank you very much!

[kai-ion]

@zombee0 we were able to ship this new feature PR to allow custom implementation of http clients (we moved the http client chunking logic to an interceptor, now sdk http clients will only receive http request stream of bytes). We also added the option of having a smart default for the interceptor, for user who still want to use their current custom http client to continue handling chunking logic