Skip to content
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

Merged
merged 31 commits into from
Aug 23, 2021
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
e1a0478
payload optimization changes
gshvang Aug 17, 2021
84d8657
payload optimization changes
gshvang Aug 17, 2021
c378949
lexicon changes
gshvang Aug 17, 2021
db85d98
complexity chanf=ges
gshvang Aug 17, 2021
25d97d3
complexity chanf=ges
gshvang Aug 17, 2021
21d8828
lexicon changes
gshvang Aug 18, 2021
56b3606
memory statistics change
gshvang Aug 18, 2021
b7ea230
comment update
gshvang Aug 18, 2021
94c10ec
Flag changes
gshvang Aug 18, 2021
c6ba116
Comment update
gshvang Aug 18, 2021
7da24f8
utest addition
gshvang Aug 18, 2021
61f9e0b
updating buffer
gshvang Aug 18, 2021
063f544
Adding more unit tests
gshvang Aug 18, 2021
b8ee13a
Update unit tests
gshvang Aug 18, 2021
535ceb3
Removing code
gshvang Aug 18, 2021
489b6c2
Removing code
gshvang Aug 18, 2021
bacabf1
Revert "Update unit tests"
gshvang Aug 18, 2021
ad72b7a
Revert "Revert "Update unit tests""
gshvang Aug 18, 2021
84570e3
Revert "Removing code"
gshvang Aug 18, 2021
9959721
Revert "Removing code"
gshvang Aug 18, 2021
a34e799
Unit test done
gshvang Aug 19, 2021
78bcb7b
Unit test fix 1
gshvang Aug 19, 2021
43f042b
Unit test fix 2
gshvang Aug 19, 2021
6f4978b
Comment update
gshvang Aug 19, 2021
a10a3ae
MISRA fixes
gshvang Aug 19, 2021
096db00
Update source/include/sigv4_internal.h
gshvang Aug 19, 2021
f21cf74
Update source/include/sigv4_internal.h
gshvang Aug 19, 2021
a87474a
Update test/unit-test/sigv4_utest.c
gshvang Aug 19, 2021
7e73f01
Address feedback
gshvang Aug 19, 2021
7d65564
Condition len change
gshvang Aug 19, 2021
09cf6c4
Unit test cverage
gshvang Aug 20, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions docs/doxygen/include/size_table.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@
</tr>
<tr>
<td>sigv4.c</td>
<td><center>4.9K</center></td>
<td><center>4.2K</center></td>
<td><center>5.1K</center></td>
<td><center>4.3K</center></td>
Copy link
Contributor

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?

Copy link
Contributor Author

@gshvang gshvang Aug 18, 2021

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

Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

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

</tr>
<tr>
<td>sigv4_quicksort.c</td>
Expand All @@ -19,7 +19,7 @@
</tr>
<tr>
<td><b>Total estimates</b></td>
<td><b><center>5.3K</center></b></td>
<td><b><center>4.5K</center></b></td>
<td><b><center>5.5K</center></b></td>
<td><b><center>4.6K</center></b></td>
</tr>
</table>
2 changes: 2 additions & 0 deletions lexicon.txt
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ hashblocklen
hashdigestlen
hashfinal
hashinit
hashpayloadlen
hashtofail
hashupdate
headercount
Expand Down Expand Up @@ -156,6 +157,7 @@ pfirstitem
pfirstval
pformat
phashcontext
phashpayloadloc
pheaders
pheadersloc
phexoutput
Expand Down
9 changes: 9 additions & 0 deletions source/include/sigv4.h
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,15 @@
*/
#define SIGV4_HTTP_HEADERS_ARE_CANONICAL_FLAG 0x4U

/**
* @ingroup sigv4_canonical_flags
* @brief Set this flag to indicate that the HTTP request payload is
* already hashed.
*
* This flag is valid only for #SigV4HttpParameters_t.flags.
*/
#define SIGV4_HTTP_PAYLOAD_IS_HASH 0x8U

/**
* @ingroup sigv4_canonical_flags
* @brief Set this flag to indicate that the HTTP request path, query, and
Expand Down
2 changes: 2 additions & 0 deletions source/include/sigv4_internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,8 @@ typedef struct CanonicalContext
uint8_t pBufProcessing[ SIGV4_PROCESSING_BUFFER_LENGTH ]; /**< Internal calculation buffer used during canonicalization. */
char * pBufCur; /**< pBufProcessing cursor. */
size_t bufRemaining; /**< pBufProcessing value used during internal calculation. */
const char * pHashPayloadLoc; /**< Pointer used to store the location of HTTP request hashed payload. */
gshvang marked this conversation as resolved.
Show resolved Hide resolved
size_t hashPayloadLen; /**< Length of HTTP request hashed payload. */
gshvang marked this conversation as resolved.
Show resolved Hide resolved
} CanonicalContext_t;

/**
Expand Down
88 changes: 79 additions & 9 deletions source/sigv4.c
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,16 @@ 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,out] canonicalRequest Struct to maintain intermediary buffer
* and state of canonicalization.
*/
static void storeHashedPayloadLocation( size_t headerIndex,
CanonicalContext_t * canonicalRequest );
gshvang marked this conversation as resolved.
Show resolved Hide resolved

/**
* @brief Parse each header key and value pair from HTTP headers.
*
Expand Down Expand Up @@ -350,6 +360,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.
*
Expand Down Expand Up @@ -1583,6 +1607,17 @@ static void generateCredentialScope( const SigV4Parameters_t * pSigV4Params,
}

/*-----------------------------------------------------------*/
static void storeHashedPayloadLocation( size_t headerIndex,
CanonicalContext_t * canonicalRequest )
{
assert( canonicalRequest != NULL );

if( memcmp( canonicalRequest->pHeadersLoc[ headerIndex ].key.pData, ( const char * ) SIGV4_HTTP_X_AMZ_CONTENT_SHA256_HEADER, 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,
Expand Down Expand Up @@ -1670,6 +1705,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, canonicalRequest );

/* Set starting location of the next header key string after the "\r\n". */
pKeyOrValStartLoc = pCurrLoc + 2U;
keyFlag = true;
Expand All @@ -1681,6 +1720,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, canonicalRequest );

/* Set starting location of the next header key string after the "\n". */
pKeyOrValStartLoc = pCurrLoc + 1U;
keyFlag = true;
Expand Down Expand Up @@ -2921,6 +2964,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--;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the decrement for?

Copy link
Contributor Author

@gshvang gshvang Aug 18, 2021

Choose a reason for hiding this comment

The 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,
Expand Down Expand Up @@ -3007,7 +3083,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;
Expand All @@ -3032,12 +3108,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. */
Expand All @@ -3053,8 +3125,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 );
}

Expand Down