Checksum notes rebase #2826
Checksum notes rebase #2826
Conversation
| @@ -264,6 +265,7 @@ bool AWSAuthV4Signer::SignRequest(Aws::Http::HttpRequest& request, const char* r | |||
| { | |||
| payloadHash = STREAMING_UNSIGNED_PAYLOAD_TRAILER; | |||
| Aws::String trailerHeaderValue = Aws::String("x-amz-checksum-") + request.GetRequestHash().first; | |||
| request.DeleteHeader(trailerHeaderValue.c_str()); | |||
There was a problem hiding this comment.
why is trailer being deleted?
There was a problem hiding this comment.
not a good name for the variable re-naming to "checksumHeaderValue". We are removing the checksum header from being sent as it is part of the trailer in this branch of logic. The bug beforehand was that a user would add the checksum to the request, it would serialized as a header AND calculated as part of the request. thus my comment on the original PR
we should consider not serializing the header x-amz-checksum- to the header in the first place during codegen as we will always end up adding it one way or another if needed. this is fine for now imo opinon, but removing the serialization could perhaps be a better long term solution
so when sent there would be a header and a trailer, which is wrong.
| if (checksumAlgorithmName == "crc32") | ||
| if (request.IsStreaming() && checksumValueAndAlgorithmProvided) | ||
| { | ||
| auto hash = Aws::MakeShared<Crypto::PrecalculatedHash>(AWS_CLIENT_LOG_TAG,request.GetHeaders().find(checksumType)->second); |
There was a problem hiding this comment.
nit: spacing inconsistent, do we need to run lint?
There was a problem hiding this comment.
fixed spacing, we really should have a clang format file. We've discussed it multiple times and i think the only thing stopping us is the one time format the world event. Im very much pro a format the world event though. but we should start thinking about adding one sooner than later.
src/aws-cpp-sdk-core/include/aws/core/utils/crypto/PrecalculatedHash.h
Outdated
Show resolved
Hide resolved
| // Check if user has provided a checksum value for the specified algorithm | ||
| const Aws::String checksumType = "x-amz-checksum-" + checksumAlgorithmName; | ||
| const HeaderValueCollection &headers = request.GetHeaders(); | ||
| bool checksumValueAndAlgorithmProvided = headers.find(checksumType) != headers.end(); |
There was a problem hiding this comment.
are you sure that checksumAlgorithmName always contains a lowered-case string?
There was a problem hiding this comment.
yes because its serialized as such from the request. which actually relates to dmitrys quesiton about removing the header when adding the trailing checksum. Serializing the checksum as part of the request as a header is one of the issues that got us here.
Issue #, if available:
#2644
Description of changes:
Fix checksum behavior when precalculated checksum is added
Check all that applies:
Check which platforms you have built SDK on to verify the correctness of this PR.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.