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.

Sign up for GitHub

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

[SigV4]Payload optimization #51

Show file tree
Hide file tree
Changes from all 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
View file
Open in desktop
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 •
edited
Loading

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
View file
Open in desktop
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
40 changes: 25 additions & 15 deletions source/include/sigv4.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,21 +55,22 @@
/** @addtogroup sigv4_constants
* @{
*/
#define SIGV4_AWS4_HMAC_SHA256 "AWS4-HMAC-SHA256" /**< AWS identifier for SHA256 signing algorithm. */
#define SIGV4_AWS4_HMAC_SHA256_LENGTH ( sizeof( SIGV4_AWS4_HMAC_SHA256 ) - 1U ) /**< AWS identifier for SHA256 signing algorithm. */
#define SIGV4_HTTP_X_AMZ_DATE_HEADER "x-amz-date" /**< AWS identifier for HTTP date header. */
#define SIGV4_HTTP_X_AMZ_SECURITY_TOKEN_HEADER "x-amz-security-token" /**< AWS identifier for security token. */

#define SIGV4_STREAMING_AWS4_HMAC_SHA256_PAYLOAD "STREAMING-AWS4-HMAC-SHA256-PAYLOAD" /**< S3 identifier for chunked payloads. */
#define SIGV4_HTTP_X_AMZ_CONTENT_SHA256_HEADER "x-amz-content-sha256" /**< S3 identifier for streaming requests. */
#define SIGV4_HTTP_X_AMZ_STORAGE_CLASS_HEADER "x-amz-storage-class" /**< S3 identifier for reduced streaming redundancy. */

#define SIGV4_ACCESS_KEY_ID_LENGTH 20U /**< Length of access key ID. */
#define SIGV4_SECRET_ACCESS_KEY_LENGTH 40U /**< Length of secret access key. */

#define SIGV4_ISO_STRING_LEN 16U /**< Length of ISO 8601 date string. */
#define SIGV4_EXPECTED_LEN_RFC_3339 20U /**< Length of RFC 3339 date input. */
#define SIGV4_EXPECTED_LEN_RFC_5322 29U
#define SIGV4_AWS4_HMAC_SHA256 "AWS4-HMAC-SHA256" /**< AWS identifier for SHA256 signing algorithm. */
#define SIGV4_AWS4_HMAC_SHA256_LENGTH ( sizeof( SIGV4_AWS4_HMAC_SHA256 ) - 1U ) /**< Length of AWS identifier for SHA256 signing algorithm. */
#define SIGV4_HTTP_X_AMZ_DATE_HEADER "x-amz-date" /**< AWS identifier for HTTP date header. */
#define SIGV4_HTTP_X_AMZ_SECURITY_TOKEN_HEADER "x-amz-security-token" /**< AWS identifier for security token. */

#define SIGV4_STREAMING_AWS4_HMAC_SHA256_PAYLOAD "STREAMING-AWS4-HMAC-SHA256-PAYLOAD" /**< S3 identifier for chunked payloads. */
#define SIGV4_HTTP_X_AMZ_CONTENT_SHA256_HEADER "x-amz-content-sha256" /**< S3 identifier for streaming requests. */
#define SIGV4_HTTP_X_AMZ_CONTENT_SHA256_HEADER_LENGTH ( sizeof( SIGV4_HTTP_X_AMZ_CONTENT_SHA256_HEADER ) - 1U ) /**< Length of S3 identifier for streaming requests. */
#define SIGV4_HTTP_X_AMZ_STORAGE_CLASS_HEADER "x-amz-storage-class" /**< S3 identifier for reduced streaming redundancy. */

#define SIGV4_ACCESS_KEY_ID_LENGTH 20U /**< Length of access key ID. */
#define SIGV4_SECRET_ACCESS_KEY_LENGTH 40U /**< Length of secret access key. */

#define SIGV4_ISO_STRING_LEN 16U /**< Length of ISO 8601 date string. */
#define SIGV4_EXPECTED_LEN_RFC_3339 20U /**< Length of RFC 3339 date input. */
#define SIGV4_EXPECTED_LEN_RFC_5322 29U
/**< Length of RFC 5322 date input. */

/** @}*/
Expand Down Expand Up @@ -110,6 +111,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
View file
Open in desktop
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 hashed HTTP request payload. */
size_t hashPayloadLen; /**< Length of hashed HTTP request payload. */
} CanonicalContext_t;

/**
Expand Down
98 changes: 89 additions & 9 deletions source/sigv4.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,20 @@ 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] amzSHA256Header Length of @p pAmzSHA256Header.
* @param[in,out] pCanonicalRequest Struct to maintain intermediary buffer
* and state of canonicalization.
*/
static void storeHashedPayloadLocation( size_t headerIndex,
const char * pAmzSHA256Header,
size_t amzSHA256Header,
CanonicalContext_t * pCanonicalRequest );

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

/*-----------------------------------------------------------*/
static void storeHashedPayloadLocation( size_t headerIndex,
const char * pAmzSHA256Header,
size_t amzSHA256HeaderLen,
CanonicalContext_t * pCanonicalRequest )
{
assert( pCanonicalRequest != NULL );

const uint8_t * pHeaderData = ( const uint8_t * ) pCanonicalRequest->pHeadersLoc[ headerIndex ].key.pData;
size_t headerLen = pCanonicalRequest->pHeadersLoc[ headerIndex ].key.dataLen;
const uint8_t * pHeaderLiteral = ( const uint8_t * ) pAmzSHA256Header;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need an intermediate pHeaderLiteral variable here? We can just use pAmzSHA256Header and cast directly?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or better yet, just use the macro directly

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes we do. MISRA reasons

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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( ( headerLen == amzSHA256HeaderLen ) && ( memcmp( pHeaderData, pHeaderLiteral, headerLen ) == 0 ) )
{
pCanonicalRequest->pHashPayloadLoc = pCanonicalRequest->pHeadersLoc[ headerIndex ].value.pData;
pCanonicalRequest->hashPayloadLen = pCanonicalRequest->pHeadersLoc[ headerIndex ].value.dataLen;
}
}

static SigV4Status_t appendCanonicalizedHeaders( size_t headerCount,
uint32_t flags,
Expand Down Expand Up @@ -1670,6 +1715,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, SIGV4_HTTP_X_AMZ_CONTENT_SHA256_HEADER, SIGV4_HTTP_X_AMZ_CONTENT_SHA256_HEADER_LENGTH, canonicalRequest );

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

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

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 +3093,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 +3118,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 +3135,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
2 changes: 1 addition & 1 deletion test/unit-test/sigv4_config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems very arbitrary. Why 428?

Copy link
Contributor Author

@gshvang gshvang Aug 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

#endif

/**
Expand Down
Loading