From b83018cc50b7693a9bbfe3bd91e3fa599ac69730 Mon Sep 17 00:00:00 2001 From: Archit Aggarwal Date: Thu, 19 Aug 2021 22:40:44 +0000 Subject: [PATCH] Address review comments --- source/sigv4.c | 83 +++++++++++++++++++++--------------- test/unit-test/sigv4_utest.c | 13 +++--- 2 files changed, 54 insertions(+), 42 deletions(-) diff --git a/source/sigv4.c b/source/sigv4.c index e464eeff..24e1495c 100644 --- a/source/sigv4.c +++ b/source/sigv4.c @@ -1819,9 +1819,9 @@ static void generateCredentialScope( const SigV4Parameters_t * pSigV4Params, /*-----------------------------------------------------------*/ - static void processAmpersantInQueryString( bool fieldHasValue, + static void processAmpersandInQueryString( bool fieldHasValue, size_t currQueryIndex, - size_t startOfFieldOrValue, + size_t * pStartOfFieldOrValue, size_t * pCurrParamCount, const char * pQuery, CanonicalContext_t * pCanonicalRequest ) @@ -1829,51 +1829,66 @@ static void generateCredentialScope( const SigV4Parameters_t * pSigV4Params, bool previousParamIsValid = true; assert( pCurrParamCount != NULL ); + assert( pStartOfFieldOrValue != NULL ); assert( pQuery != NULL ); assert( pCanonicalRequest != NULL ); - /* Ignore unnecessary '&' that represent empty parameter names. - * Trimmable '&'s can be leading, trailing or repeated as separators between parameter - * pairs. - * For example, this function will prune "&p1=v1&&p2=v2&" to "p1=v1&p2=v2". */ - if( !( fieldHasValue ) && ( ( ( currQueryIndex - startOfFieldOrValue ) == 0U ) ) ) + /* Process scenarios when a value for the previous parameter entry has been detected. */ + if( fieldHasValue ) { - /* Do nothing as previous parameter name is empty. */ - previousParamIsValid = false; - } - - /* Case when parameter has empty value even though the query parameter entry has the form - * "=". */ - else if( ( fieldHasValue ) && ( ( currQueryIndex - startOfFieldOrValue ) == 0U ) ) - { - /* Store the previous parameter's empty value information. Use NULL to represent empty value. */ - setQueryParameterValue( *pCurrParamCount, NULL, 0U, pCanonicalRequest ); - } - - /* Case when a new query parameter pair has begun without the previous parameter having an associated value, - * i.e. of the format "&=" where "". - * In thus case, as the previous parameter does not have a value, we will store an empty value for it. */ - else if( !( fieldHasValue ) ) - { - /* Store information about previous query parameter name. The query parameter has no associated value. */ - setQueryParameterKey( *pCurrParamCount, &pQuery[ startOfFieldOrValue ], currQueryIndex - startOfFieldOrValue, pCanonicalRequest ); + /* Case when parameter has empty value even though the query parameter entry has the form + * "=". */ + if( ( currQueryIndex - *pStartOfFieldOrValue ) == 0U ) + { + /* Store the previous parameter's empty value information. Use NULL to represent empty value. */ + setQueryParameterValue( *pCurrParamCount, NULL, 0U, pCanonicalRequest ); + } - /* Store the previous parameter's empty value information. Use NULL to represent empty value. */ - setQueryParameterValue( *pCurrParamCount, NULL, 0U, pCanonicalRequest ); + /* This represents the case when the previous parameter has a value associated with it. */ + else + { + /* End of value reached, so store a pointer to the previously set value. */ + setQueryParameterValue( *pCurrParamCount, &pQuery[ *pStartOfFieldOrValue ], currQueryIndex - *pStartOfFieldOrValue, pCanonicalRequest ); + } } - /* This represents the case when the previous parameter has a value associated with it. */ + /* A parameter value has not been found for the previous parameter. */ else { - /* End of value reached, so store a pointer to the previously set value. */ - setQueryParameterValue( *pCurrParamCount, &pQuery[ startOfFieldOrValue ], currQueryIndex - startOfFieldOrValue, pCanonicalRequest ); + /* Ignore unnecessary '&' that represent empty parameter names. + * Trimmable '&'s can be leading, trailing or repeated as separators between parameter + * pairs. + * For example, this function will prune "&p1=v1&&p2=v2&" to "p1=v1&p2=v2". */ + if( ( currQueryIndex - *pStartOfFieldOrValue ) == 0U ) + { + /* Do nothing as previous parameter name is empty. */ + previousParamIsValid = false; + } + + /* Case when a new query parameter pair has begun without the previous parameter having an associated value, + * i.e. of the format "&=" where "". + * In thus case, as the previous parameter does not have a value, we will store an empty value for it. */ + else + { + /* Store information about previous query parameter name. The query parameter has no associated value. */ + setQueryParameterKey( *pCurrParamCount, &pQuery[ *pStartOfFieldOrValue ], currQueryIndex - *pStartOfFieldOrValue, pCanonicalRequest ); + + /* Store the previous parameter's empty value information. Use NULL to represent empty value. */ + setQueryParameterValue( *pCurrParamCount, NULL, 0U, pCanonicalRequest ); + } } + /* Increment the parameter count if the previous parameter is not empty + * An empty previous parameter is caused in query strings where the '&' are unnecessary. */ if( previousParamIsValid == true ) { /* As the previous parameter is valid, increment the total parameters * parsed so far from the query string. */ ( *pCurrParamCount )++; } + + /* As '&' is always treated as a separator, update the starting index to represent the next + * query parameter. */ + *pStartOfFieldOrValue = currQueryIndex + 1U; } /*-----------------------------------------------------------*/ @@ -1918,12 +1933,10 @@ static void generateCredentialScope( const SigV4Parameters_t * pSigV4Params, * Short circuit evaluation ensures the string is not dereferenced for that case. */ if( ( i == queryLen ) || ( pQuery[ i ] == '&' ) ) { - processAmpersantInQueryString( fieldHasValue, i, startOfFieldOrValue, + processAmpersandInQueryString( fieldHasValue, i, &startOfFieldOrValue, ¤tParameter, pQuery, pCanonicalRequest ); - /* As '&' is always treated as a separator, update the starting index to represent the next - * query parameter and reset the flag about parameter value state of the new parameter. */ - startOfFieldOrValue = i + 1U; + /* As '&' represents a new parameter, reset the flag about parameter value state.*/ fieldHasValue = false; } else if( ( pQuery[ i ] == '=' ) && !fieldHasValue ) diff --git a/test/unit-test/sigv4_utest.c b/test/unit-test/sigv4_utest.c index e2e30375..68d3bdd7 100644 --- a/test/unit-test/sigv4_utest.c +++ b/test/unit-test/sigv4_utest.c @@ -62,8 +62,8 @@ #define QUERY_WITH_SPECIAL_CHARS "param=/" #define QUERY_STRING_NO_PARAM_VALUE "param=¶m2=" -#define QUERY_STRING_WITH_TRAILING_N_LEADING_AMPERSANTS "&¶m2=&&" -#define QUERY_STRING_WITH_REPEATED_AMPERSANTS "param1=val&¶m2=val" +#define QUERY_STRING_WITH_TRAILING_N_LEADING_AMPERSANT "&¶m2=&&" +#define QUERY_STRING_WITH_REPEATED_AMPERSANT "param1=val&¶m2=val" #define QUERY "Action=ListUsers&Version=2010-05-08" #define QUERY_LENGTH ( sizeof( QUERY ) - 1U ) @@ -634,7 +634,6 @@ void test_SigV4_GenerateHTTPAuthorization_Invalid_Params() TEST_ASSERT_EQUAL( SigV4InvalidParameter, returnStatus ); } -/* TODO - Verify the generated signatures. */ void test_SigV4_GenerateHTTPAuthorization_Happy_Paths() { SigV4Status_t returnStatus; @@ -770,8 +769,8 @@ void test_SigV4_GenerateAuthorization_Query_Strings_Special_Cases() printf( "%.*s\n", authBufLen, authBuf ); TEST_ASSERT_EQUAL_MEMORY( pExpectedSignature, signature, signatureLen ); - params.pHttpParameters->pQuery = QUERY_STRING_WITH_TRAILING_N_LEADING_AMPERSANTS; - params.pHttpParameters->queryLen = strlen( QUERY_STRING_WITH_TRAILING_N_LEADING_AMPERSANTS ); + params.pHttpParameters->pQuery = QUERY_STRING_WITH_TRAILING_N_LEADING_AMPERSANT; + params.pHttpParameters->queryLen = strlen( QUERY_STRING_WITH_TRAILING_N_LEADING_AMPERSANT ); pExpectedSignature = "576a0348d54591e15bed920586936f9263656470197adf7ce79c5fc8ef44d825"; @@ -780,8 +779,8 @@ void test_SigV4_GenerateAuthorization_Query_Strings_Special_Cases() TEST_ASSERT_EQUAL( SIGV4_HASH_MAX_DIGEST_LENGTH * 2U, signatureLen ); TEST_ASSERT_EQUAL_MEMORY( pExpectedSignature, signature, signatureLen ); - params.pHttpParameters->pQuery = QUERY_STRING_WITH_REPEATED_AMPERSANTS; - params.pHttpParameters->queryLen = strlen( QUERY_STRING_WITH_REPEATED_AMPERSANTS ); + params.pHttpParameters->pQuery = QUERY_STRING_WITH_REPEATED_AMPERSANT; + params.pHttpParameters->queryLen = strlen( QUERY_STRING_WITH_REPEATED_AMPERSANT ); pExpectedSignature = "4fcf6c89d5ddb944c0e386817d52835f578769e100d7e92d433bf4a946b7e6c3"; TEST_ASSERT_EQUAL( SigV4Success, SigV4_GenerateHTTPAuthorization(