-
Notifications
You must be signed in to change notification settings - Fork 31
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
[SigV4]Payload optimization #51
Changes from 25 commits
e1a0478
84d8657
c378949
db85d98
25d97d3
21d8828
56b3606
b7ea230
94c10ec
c6ba116
7da24f8
61f9e0b
063f544
b8ee13a
535ceb3
489b6c2
bacabf1
ad72b7a
84570e3
9959721
a34e799
78bcb7b
43f042b
6f4978b
a10a3ae
096db00
f21cf74
a87474a
7e73f01
7d65564
09cf6c4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -192,6 +192,18 @@ static SigV4Status_t appendCanonicalizedHeaders( size_t headerCount, | |
uint32_t flags, | ||
CanonicalContext_t * canonicalRequest ); | ||
|
||
/** | ||
* @brief Store the location of HTTP request hashed payload in the HTTP request. | ||
* | ||
* @param[in] headerIndex Index of request Header in the list of parsed headers. | ||
* @param[in] pAmzSHA256Header Literal for x-amz-content-sha256 header in HTTP request. | ||
* @param[in,out] canonicalRequest Struct to maintain intermediary buffer | ||
* and state of canonicalization. | ||
*/ | ||
static void storeHashedPayloadLocation( size_t headerIndex, | ||
const char * pAmzSHA256Header, | ||
CanonicalContext_t * canonicalRequest ); | ||
gshvang marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/** | ||
* @brief Parse each header key and value pair from HTTP headers. | ||
* | ||
|
@@ -350,6 +362,20 @@ static void setQueryParameterValue( size_t currentParameter, | |
size_t valueLen, | ||
CanonicalContext_t * pCanonicalRequest ); | ||
|
||
/** | ||
* @brief Write the HTTP request payload hash to the canonical request. | ||
* | ||
* @param[in] pParams The application-defined parameters used for writing | ||
* the payload hash. | ||
* @param[out] pCanonicalContext The canonical context where the payload | ||
* hash should be written. | ||
* | ||
* @return SigV4InsufficientMemory if the length of the canonical request | ||
* buffer cannot write the desired line, #SigV4Success otherwise. | ||
*/ | ||
static SigV4Status_t writePayloadHashToCanonicalRequest( const SigV4Parameters_t * pParams, | ||
CanonicalContext_t * pCanonicalContext ); | ||
|
||
/** | ||
* @brief Generates the key for the HMAC operation. | ||
* | ||
|
@@ -1583,6 +1609,21 @@ static void generateCredentialScope( const SigV4Parameters_t * pSigV4Params, | |
} | ||
|
||
/*-----------------------------------------------------------*/ | ||
static void storeHashedPayloadLocation( size_t headerIndex, | ||
const char * pAmzSHA256Header, | ||
CanonicalContext_t * canonicalRequest ) | ||
{ | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need an intermediate There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or better yet, just use the macro directly There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes we do. MISRA reasons There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is done to avoid MISRA violation(doing the cast within the function call still triggers 21.16.) |
||
|
||
if( memcmp( pHeaderData, pHeaderLiteral, canonicalRequest->pHeadersLoc[ headerIndex ].key.dataLen ) == 0 ) | ||
gshvang marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
canonicalRequest->pHashPayloadLoc = canonicalRequest->pHeadersLoc[ headerIndex ].value.pData; | ||
canonicalRequest->hashPayloadLen = canonicalRequest->pHeadersLoc[ headerIndex ].value.dataLen; | ||
} | ||
} | ||
|
||
static SigV4Status_t appendCanonicalizedHeaders( size_t headerCount, | ||
uint32_t flags, | ||
|
@@ -1670,6 +1711,10 @@ static void generateCredentialScope( const SigV4Parameters_t * pSigV4Params, | |
dataLen = pCurrLoc - pKeyOrValStartLoc; | ||
canonicalRequest->pHeadersLoc[ noOfHeaders ].value.pData = pKeyOrValStartLoc; | ||
canonicalRequest->pHeadersLoc[ noOfHeaders ].value.dataLen = ( size_t ) dataLen; | ||
|
||
/* Storing location of hashed request payload */ | ||
storeHashedPayloadLocation( noOfHeaders, ( const char * ) SIGV4_HTTP_X_AMZ_CONTENT_SHA256_HEADER, canonicalRequest ); | ||
|
||
/* Set starting location of the next header key string after the "\r\n". */ | ||
pKeyOrValStartLoc = pCurrLoc + 2U; | ||
keyFlag = true; | ||
|
@@ -1681,6 +1726,10 @@ static void generateCredentialScope( const SigV4Parameters_t * pSigV4Params, | |
dataLen = pCurrLoc - pKeyOrValStartLoc; | ||
canonicalRequest->pHeadersLoc[ noOfHeaders ].value.pData = pKeyOrValStartLoc; | ||
canonicalRequest->pHeadersLoc[ noOfHeaders ].value.dataLen = ( size_t ) dataLen; | ||
|
||
/* Storing location of hashed request payload */ | ||
storeHashedPayloadLocation( noOfHeaders, ( const char * ) SIGV4_HTTP_X_AMZ_CONTENT_SHA256_HEADER, canonicalRequest ); | ||
gshvang marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/* Set starting location of the next header key string after the "\n". */ | ||
pKeyOrValStartLoc = pCurrLoc + 1U; | ||
keyFlag = true; | ||
|
@@ -2921,6 +2970,39 @@ static SigV4Status_t generateSigningKey( const SigV4Parameters_t * pSigV4Params, | |
return returnStatus; | ||
} | ||
|
||
static SigV4Status_t writePayloadHashToCanonicalRequest( const SigV4Parameters_t * pParams, | ||
CanonicalContext_t * pCanonicalContext ) | ||
{ | ||
size_t encodedLen = 0U; | ||
SigV4Status_t returnStatus = SigV4Success; | ||
|
||
assert( pParams != NULL ); | ||
assert( pCanonicalContext != NULL ); | ||
|
||
if( FLAG_IS_SET( pParams->pHttpParameters->flags, SIGV4_HTTP_PAYLOAD_IS_HASH ) ) | ||
{ | ||
/* 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 ); | ||
/* Remove new line at the end of the payload. */ | ||
pCanonicalContext->pBufCur--; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the decrement for? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
else | ||
{ | ||
encodedLen = pCanonicalContext->bufRemaining; | ||
/* Calculate hash of the request payload. */ | ||
returnStatus = completeHashAndHexEncode( pParams->pHttpParameters->pPayload, | ||
pParams->pHttpParameters->payloadLen, | ||
pCanonicalContext->pBufCur, | ||
&encodedLen, | ||
pParams->pCryptoInterface ); | ||
pCanonicalContext->pBufCur += encodedLen; | ||
pCanonicalContext->bufRemaining -= encodedLen; | ||
} | ||
|
||
return returnStatus; | ||
} | ||
|
||
|
||
SigV4Status_t SigV4_AwsIotDateToIso8601( const char * pDate, | ||
size_t dateLen, | ||
char * pDateISO8601, | ||
|
@@ -3007,7 +3089,7 @@ SigV4Status_t SigV4_GenerateHTTPAuthorization( const SigV4Parameters_t * pParams | |
CanonicalContext_t canonicalContext; | ||
const char * pAlgorithm = NULL; | ||
char * pSignedHeaders = NULL; | ||
size_t encodedLen = 0U, algorithmLen = 0U, signedHeadersLen = 0U, authPrefixLen = 0U; | ||
size_t algorithmLen = 0U, signedHeadersLen = 0U, authPrefixLen = 0U; | ||
HmacContext_t hmacContext = { 0 }; | ||
|
||
SigV4String_t signingKey; | ||
|
@@ -3032,12 +3114,8 @@ SigV4Status_t SigV4_GenerateHTTPAuthorization( const SigV4Parameters_t * pParams | |
/* Hash and hex-encode the canonical request to the buffer. */ | ||
if( returnStatus == SigV4Success ) | ||
{ | ||
encodedLen = canonicalContext.bufRemaining; | ||
returnStatus = completeHashAndHexEncode( pParams->pHttpParameters->pPayload, | ||
pParams->pHttpParameters->payloadLen, | ||
canonicalContext.pBufCur, | ||
&encodedLen, | ||
pParams->pCryptoInterface ); | ||
/* Write HTTP request payload hash to the canonical request. */ | ||
returnStatus = writePayloadHashToCanonicalRequest( pParams, &canonicalContext ); | ||
} | ||
|
||
/* Write the prefix of the Authorizaton header value. */ | ||
|
@@ -3053,8 +3131,6 @@ SigV4Status_t SigV4_GenerateHTTPAuthorization( const SigV4Parameters_t * pParams | |
/* Write string to sign. */ | ||
if( returnStatus == SigV4Success ) | ||
{ | ||
canonicalContext.pBufCur += encodedLen; | ||
canonicalContext.bufRemaining -= encodedLen; | ||
returnStatus = writeStringToSign( pParams, pAlgorithm, algorithmLen, &canonicalContext ); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems very arbitrary. Why 428? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed the buffer for the new header |
||
#endif | ||
|
||
/** | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The application will add that header before calling the Sigv4 API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it's not almost all cases then, it's every case