[SigV4]Payload optimization #51
Conversation
| { | ||
| /* Copy the hashed payload data supplied by the user in the headers data list. */ | ||
| returnStatus = copyHeaderStringToCanonicalBuffer( pCanonicalContext->pHashPayloadLoc, pCanonicalContext->hashPayloadLen, pParams->pHttpParameters->flags, '\n', pCanonicalContext ); | ||
| pCanonicalContext->pBufCur--; |
There was a problem hiding this comment.
What's the decrement for?
There was a problem hiding this comment.
We don't need new line at the end of hash payload in the canonical request but the above function adds new line also so decrementing it.
| <td><center>4.9K</center></td> | ||
| <td><center>4.2K</center></td> | ||
| <td><center>5.1K</center></td> | ||
| <td><center>4.3K</center></td> |
There was a problem hiding this comment.
Is the user likely to have already hashed the payload and encoded it in lowercase hex? Is the extra memory usage worth the speedup gained from not having to hash the payload?
There was a problem hiding this comment.
Yeah, In almost all cases the payload will be hashed and hex encoded by the application. So I think it is worth. Also if there is x-amz-content-sha256 header is there in the HTTP headers passed to the library, then the payload is definitely hashed. And almost all AWS requests include this header.
The x-amz-content-sha256 header is required for all AWS Signature Version 4 requests. It provides a hash of the request payload. If there is no payload, you must provide the hash of an empty string.
This is mentioned in this AWS doc: https://docs.aws.amazon.com/AmazonS3/latest/API/sig-v4-header-based-auth.html
There was a problem hiding this comment.
But is that header something the application is expected to have been added already, or would a user expect the library to add the header for them? If it's the latter case, then the user can save themself time by just telling the library to add it for them
There was a problem hiding this comment.
The application will add that header before calling the Sigv4 API.
There was a problem hiding this comment.
So it's not almost all cases then, it's every case
| @@ -76,7 +76,7 @@ | |||
| * <b>Default value:</b> `1024` | |||
| */ | |||
| #ifndef SIGV4_PROCESSING_BUFFER_LENGTH | |||
| #define SIGV4_PROCESSING_BUFFER_LENGTH 350 | |||
| #define SIGV4_PROCESSING_BUFFER_LENGTH 428 | |||
There was a problem hiding this comment.
This seems very arbitrary. Why 428?
There was a problem hiding this comment.
I changed the buffer for the new header X_AMZ_CONTENT_SHA256 as now the canonical headers and signed headers will include the header too.
Co-authored-by: Oscar Michael Abrina <[email protected]>
| assert( canonicalRequest != NULL ); | ||
|
|
||
| const uint8_t * pHeaderData = ( const uint8_t * ) canonicalRequest->pHeadersLoc[ headerIndex ].key.pData; | ||
| const uint8_t * pHeaderLiteral = ( const uint8_t * ) pAmzSHA256Header; |
There was a problem hiding this comment.
Do we need an intermediate pHeaderLiteral variable here? We can just use pAmzSHA256Header and cast directly?
There was a problem hiding this comment.
Or better yet, just use the macro directly
There was a problem hiding this comment.
Yes we do. MISRA reasons
There was a problem hiding this comment.
This is done to avoid MISRA violation(doing the cast within the function call still triggers 21.16.)
Co-authored-by: Oscar Michael Abrina <[email protected]>
Co-authored-by: Muneeb Ahmed <[email protected]>
|
The CBMC proofs are still failing. I suggest running them locally. |
Yeah, I am investigating that and will raise a separate PR for it. |
This PR contains changes related to optimizing the sigV4 library for the already hashed payload.
By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.