Fix to allow user to provide their own checksum value when checksum algorithm is specified #2644

yasminetalby · 2023-08-29T15:57:28Z

Issue #, if available: #2540

Description of changes:
Fix to allow user to provide their own checksum value when checksum algorithm is specified.

Check all that applies:

Did a review by yourself.
Added proper tests to cover this PR. (If tests are not applicable, explain.)
Checked if this PR is a breaking (APIs have been changed) change.
Checked if this PR will not introduce cross-platform inconsistent behavior.
Checked if this PR would require a ReadMe/Wiki update.

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.

…lgorithm is specified

src/aws-cpp-sdk-core/source/http/curl/CurlHttpClient.cpp

sbiscigl · 2023-08-29T16:05:30Z

src/aws-cpp-sdk-core/source/auth/signer/AWSAuthV4Signer.cpp

@@ -251,6 +257,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());


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

sbiscigl · 2023-08-29T16:10:09Z

src/aws-cpp-sdk-core/include/aws/core/utils/crypto/Hash.h

+                    return m_precalculatedHashSet;
+                }
+
+                const Aws::String &GetPrecalculatedHash() const {


could make these methods inline

SergeyRyabinin · 2023-08-29T16:21:07Z

src/aws-cpp-sdk-core/source/client/AWSClient.cpp

+                httpRequest->SetRequestHash(checksumAlgorithmName, Aws::MakeShared<Crypto::CRC32>(AWS_CLIENT_LOG_TAG));
+            }
+            else if (checksumValueAndAlgorithmProvided){
+                httpRequest->SetHeaderValue(checksum_type, request.GetHeaders().find(checksum_type)->second);


request.GetHeaders().find() will return request.GetHeaders().end() iterator which points beyond the container and it is UB to dereference -> such iterator.

src/aws-cpp-sdk-core/source/client/AWSClient.cpp

SergeyRyabinin · 2023-08-30T16:26:59Z

src/aws-cpp-sdk-core/source/auth/signer/AWSAuthV4Signer.cpp

@@ -238,7 +238,13 @@ bool AWSAuthV4Signer::SignRequest(Aws::Http::HttpRequest& request, const char* r
        if (request.GetRequestHash().second != nullptr)
        {
            Aws::String checksumHeaderKey = Aws::String("x-amz-checksum-") + request.GetRequestHash().first;
-            Aws::String checksumHeaderValue = HashingUtils::Base64Encode(request.GetRequestHash().second->Calculate(*(request.GetContentBody())).GetResult());
+            auto checksumHeaderValue = [](const std::shared_ptr<Utils::Crypto::Hash> &hash,


I know IIFE is cool, but KISS is also cool and ternary operator would work here too:

auto checksumHeaderValue = request.GetRequestHash().second ? request.GetRequestHash().second : HashingUtils::Base64Encode(hash->Calculate(request.GetContentBody().get()).GetResult());

also, because we now dereference GetContentBody, it worth adding a check it is present (and trace error and return false if not)

or keep the IIFE, resulting ternary operator is also looking complicated.

DmitriyMusatkin · 2023-08-30T23:41:21Z

src/aws-cpp-sdk-core/include/aws/core/utils/crypto/Hash.h

@@ -48,6 +48,23 @@ namespace Aws

                // when hashing streams, this is the size of our internal buffer we read the stream into
                static const uint32_t INTERNAL_HASH_STREAM_BUFFER_SIZE = 8192;
+
+                void SetPrecalculatedHash(const Aws::String &precalculatedHash) {


Adding pre calculated to the interface scares me, since now we have to be aware that hash is either in the computing state and we need to pull the computed value or its in precomputed state and we pull that value.

I know its a public interface and we cannot really change it. One thing to consider is a NoopHash implementation - basically it will take precomputed hash and all update methods will do nothing, with GetHash just returning precomputed value. That way we can hide the fact that its precomputed

DmitriyMusatkin · 2023-08-31T00:11:29Z

src/aws-cpp-sdk-core/source/auth/signer/AWSAuthV4Signer.cpp

@@ -238,7 +238,13 @@ bool AWSAuthV4Signer::SignRequest(Aws::Http::HttpRequest& request, const char* r
        if (request.GetRequestHash().second != nullptr)
        {
            Aws::String checksumHeaderKey = Aws::String("x-amz-checksum-") + request.GetRequestHash().first;
-            Aws::String checksumHeaderValue = HashingUtils::Base64Encode(request.GetRequestHash().second->Calculate(*(request.GetContentBody())).GetResult());
+            auto checksumHeaderValue = [](const std::shared_ptr<Utils::Crypto::Hash> &hash,


nit: lets backlog this and put a comment - we should not be computing checksum over the whole body and put it in header. we should keep a running checksum and compute it as we are streaming the body

sbiscigl · 2023-09-13T14:34:53Z

src/aws-cpp-sdk-core/include/aws/core/utils/crypto/PrecalculatedHash.h

+    namespace Utils {
+        namespace Crypto {
+            /**
+             * CRC32 hash implementation.


nit: you copied the comment as well

sbiscigl · 2023-09-13T14:37:09Z

src/aws-cpp-sdk-core/source/utils/crypto/PrecalculatedHash.cpp

+using namespace Aws::Utils::Crypto;
+
+PrecalculatedHash::PrecalculatedHash(const Aws::String &mHash): m_hash(ByteBuffer((unsigned char*)mHash.c_str(),mHash.length())){
+m_hash_string = mHash;


add to the initializer list instead of assigning in the ctor and rename to m_hashString pls

sbiscigl · 2023-09-13T14:39:32Z

src/aws-cpp-sdk-core/include/aws/core/utils/crypto/PrecalculatedHash.h

+             */
+            class AWS_CORE_API PrecalculatedHash : public Hash {
+            public:
+                PrecalculatedHash(const Aws::String &mHash);


nit: call it it hash instead of mHash lol. It really doesnt matter but we call member variables m_variable name to disambiguate from non-members

sbiscigl · 2023-09-13T14:41:15Z

src/aws-cpp-sdk-core/source/client/AWSClient.cpp

-                auto precalculatedValue = request.GetHeaders().find(checksumType)->second;
-                hash->SetPrecalculatedHash(precalculatedValue);
-                httpRequest->SetRequestHash(checksumAlgorithmName, hash); }
+                auto hash = Aws::MakeShared<Crypto::PrecalculatedHash>(AWS_CLIENT_LOG_TAG,request.GetHeaders().find(checksumType)->second);


we could likely factor all of this out into a single if (request.IsStreaming() && checksumValueAndAlgorithmProvided) statement instead of creating one for each checksum algo

sbiscigl · 2023-09-13T14:45:14Z

src/aws-cpp-sdk-core/source/utils/crypto/PrecalculatedHash.cpp

+HashResult PrecalculatedHash::Calculate(Aws::IStream& stream)
+{
+    AWS_UNREFERENCED_PARAM(stream);
+    return m_hash;


do we need m_hash at all anymore? imo whenever we want to return a byte buffer we should be calling HashingUtils::Base64Decode(m_hashString) that way we only are keeping track of one member variable and the constructor becomes a lot cleaner as we are only using one string param and not casting it to a byte array.

also it might be incorrect as we would be creating a byte buffer of the base54 encoding and not the actual hash result

src/aws-cpp-sdk-core/include/aws/core/utils/crypto/Hash.h

… the PrecalculatedHash value

…is general

src/aws-cpp-sdk-core/source/client/AWSClient.cpp

@@ -774,61 +775,76 @@ void AWSClient::AppendHeaderValueToRequest(const std::shared_ptr<Aws::Http::Http
 void AWSClient::AddChecksumToRequest(const std::shared_ptr<Aws::Http::HttpRequest>& httpRequest,


DmitriyMusatkin · 2023-09-20T21:40:05Z

src/aws-cpp-sdk-core/include/aws/core/utils/crypto/Hash.h

+                /**
+                 * Return the base64 encoded checksum value
+                 */
+                virtual Aws::String GetHashBase64(ByteBuffer& m_checksum){


im confused on how this is supposed to work. are the callers expected to retrieve checksum from the class before calling this method?

DmitriyMusatkin · 2023-09-20T21:59:06Z

src/aws-cpp-sdk-core/source/utils/crypto/PrecalculatedHash.cpp

+
+using namespace Aws::Utils::Crypto;
+
+PrecalculatedHash::PrecalculatedHash(const Aws::String &hash) : m_hashString(hash){}


i would recommend decoding b64 in constructor once. and then all functions that return raw value just return that precomputed

DmitriyMusatkin · 2023-09-20T22:01:03Z

src/aws-cpp-sdk-core/source/auth/signer/AWSAuthV4Signer.cpp

@@ -238,7 +238,7 @@ bool AWSAuthV4Signer::SignRequest(Aws::Http::HttpRequest& request, const char* r
        if (request.GetRequestHash().second != nullptr)
        {
            Aws::String checksumHeaderKey = Aws::String("x-amz-checksum-") + request.GetRequestHash().first;
-            Aws::String checksumHeaderValue = HashingUtils::Base64Encode(request.GetRequestHash().second->Calculate(*(request.GetContentBody())).GetResult());
+            auto checksumHeaderValue = request.GetRequestHash().second->GetHashBase64(request.GetRequestHash().second->Calculate(*(request.GetContentBody())).GetResult());


GetHashBase64 should not be taking result of calculate here. Just keep the old code here and let calculate return raw value for precomputed as described below

DmitriyMusatkin · 2023-09-20T22:02:13Z

src/aws-cpp-sdk-core/source/http/curl/CurlHttpClient.cpp

-                if (request->GetRequestHash().second != nullptr)
-                {
+                if (request->GetRequestHash().second != nullptr) {
+                    auto hash = request->GetRequestHash().second->GetHash().GetResult();


this hash can be changed to GetHashBase64()

DmitriyMusatkin · 2023-09-20T22:34:02Z

src/aws-cpp-sdk-core/source/client/AWSClient.cpp

-        if (checksumAlgorithmName == "crc32")
+        if (request.IsStreaming() && checksumValueAndAlgorithmProvided)
+        {
+            auto hash = Aws::MakeShared<Crypto::PrecalculatedHash>(AWS_CLIENT_LOG_TAG,request.GetHeaders().find(checksumType)->second);


I would switch a logic slightly here. Lets decode b64 that customer provided, make sure its valid b64, then construct precalculated hash and give it both b64 and raw formats. precalculated hash can then return b64/raw as specified by method.

we can probably remove the need to decode the b64, but that can be done along with the changes needed to avoid hashing the whole stream for signing purposes

jmklix · 2024-02-13T17:04:17Z

merged here: #2826

sbiscigl and others added 2 commits August 28, 2023 13:48

staging commits to share work

0e497cc

Fix to allow user to provide their own checksum value when checksum a…

d153e04

…lgorithm is specified

yasminetalby marked this pull request as ready for review August 29, 2023 15:57

sbiscigl reviewed Aug 29, 2023

View reviewed changes

SergeyRyabinin reviewed Aug 29, 2023

View reviewed changes

SergeyRyabinin reviewed Aug 30, 2023

View reviewed changes

src/aws-cpp-sdk-core/source/client/AWSClient.cpp Outdated Show resolved Hide resolved

SergeyRyabinin reviewed Aug 30, 2023

View reviewed changes

DmitriyMusatkin reviewed Aug 31, 2023

View reviewed changes

yasminetalby added 4 commits September 5, 2023 15:37

remove debug

cc1cbef

Fix syntax

cd652d9

const

1271851

Allow user to provide their own checksum

9d275ba

sbiscigl reviewed Sep 13, 2023

View reviewed changes

src/aws-cpp-sdk-core/include/aws/core/utils/crypto/Hash.h Show resolved Hide resolved

yasminetalby added 2 commits September 13, 2023 16:55

Correct Precalculated Hash Calculate method to return Base64Decode of…

2ca3619

… the PrecalculatedHash value

Check and behavior for checksum algorithm can be checked first as it …

fd25ca1

…is general

sennhann29 reviewed Sep 17, 2023

View reviewed changes

Naming, constructor and typo comment changes

ccff597

sbiscigl approved these changes Sep 20, 2023

View reviewed changes

yasminetalby added 2 commits September 20, 2023 16:24

naming change

b247868

fix

e940986

SergeyRyabinin approved these changes Sep 20, 2023

View reviewed changes

DmitriyMusatkin reviewed Sep 20, 2023

View reviewed changes

jmklix mentioned this pull request Jan 25, 2024

Checksum notes rebase #2826

Merged

11 tasks

jmklix closed this Feb 13, 2024

jmklix deleted the checksum-notes branch February 13, 2024 17:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix to allow user to provide their own checksum value when checksum algorithm is specified #2644

Fix to allow user to provide their own checksum value when checksum algorithm is specified #2644

yasminetalby commented Aug 29, 2023

sbiscigl Aug 29, 2023 •

edited

Loading

sbiscigl Aug 29, 2023

SergeyRyabinin Aug 29, 2023

SergeyRyabinin Aug 30, 2023

SergeyRyabinin Aug 30, 2023

DmitriyMusatkin Aug 30, 2023

DmitriyMusatkin Aug 31, 2023

sbiscigl Sep 13, 2023

sbiscigl Sep 13, 2023

sbiscigl Sep 13, 2023

sbiscigl Sep 13, 2023

sbiscigl Sep 13, 2023

This comment was marked as spam.

DmitriyMusatkin Sep 20, 2023

DmitriyMusatkin Sep 20, 2023

DmitriyMusatkin Sep 20, 2023

DmitriyMusatkin Sep 20, 2023

DmitriyMusatkin Sep 20, 2023

jmklix commented Feb 13, 2024

		@@ -774,61 +775,76 @@ void AWSClient::AppendHeaderValueToRequest(const std::shared_ptr<Aws::Http::Http
		void AWSClient::AddChecksumToRequest(const std::shared_ptr<Aws::Http::HttpRequest>& httpRequest,


		using namespace Aws::Utils::Crypto;

		PrecalculatedHash::PrecalculatedHash(const Aws::String &hash) : m_hashString(hash){}

Fix to allow user to provide their own checksum value when checksum algorithm is specified #2644

Fix to allow user to provide their own checksum value when checksum algorithm is specified #2644

Conversation

yasminetalby commented Aug 29, 2023

sbiscigl Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as spam.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmklix commented Feb 13, 2024

sbiscigl Aug 29, 2023 •

edited

Loading