SigV4_GenerateHTTPAuthorization unit tests #29

yourslab · 2021-08-04T19:57:10Z

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

muneebahmed10 · 2021-08-04T21:34:47Z

test/unit-test/sigv4_utest.c

+        resetFailableHashParams();
+        initHashCallToFail = i;
+        returnStatus = SigV4_GenerateHTTPAuthorization( &params, authBuf, &authBufLen, &signature, &signatureLen );
+        TEST_ASSERT_EQUAL( SigV4HashError, returnStatus );


Would it be useful to know which iteration failed? Perhaps by using TEST_ASSERT_EQUAL_MESSAGE to log the failing iteration?

muneebahmed10 · 2021-08-04T21:35:33Z

test/unit-test/sigv4_utest.c

+    returnStatus = SigV4_GenerateHTTPAuthorization( &params, authBuf, &authBufLen, &signature, &signatureLen );
+    TEST_ASSERT_EQUAL( SigV4MaxQueryPairCountExceeded, returnStatus );
+
+    resetInputParams();


Is this reset needed here for another test that is meant to be written?

Update auto-generated .md files Add test and tool directories (aws#1) Adding test and tools directories, with CMock submodule. CI Actions (aws#2) Add header files + default configurations (aws#3) - Add files sigv4.h and sigv4_config_defaults.h - Add public-facing API elements detailed in design doc Format Date Header for ISO8601 Compliance (aws#4) Add optional utility function to format date header returned from AWS IoT (ex. in temp tokens) for compliance with the ISO8601 format required for authentication Add unit tests for SigV4_AwsIotDateToIso8601() (aws#8) Setup proof infrastructure for CBMC (aws#7) * Add Litani and templates for CBMC * Add sample proof * Implement CBMC proof for SigV4_AwsIotDateToIso8601 * Unwind all loops such that no unwinding errors occur Change submodule to use https rather than ssh for aws-templates-for-cbmc-proofs (aws#12) * CBMC fix test (do not merge) * Change AWS templates to https instead of ssh * Revert README Update README.md and LICENSE files (aws#14) Update README.md and LICENSE files before changing repo's visibility status (to public). Add remaining doxygen + link verification checks (aws#15) Add doxygen + link verifier checks (the library-specific doxygen content will be added in a separate PR for further review). [SigV4] CBMC proof for Sigv4_awsIotdatetoISO8601 API (aws#19) * Sigv4_AWSIOtDateToISO8601 CBMC PROOF * Unit test coverage changes Add release workflow (+ revert to previous license) (aws#18) change permissions of run_cbmc_proofs.py (aws#21) [Sigv4] Doxygen content updates (aws#22) * doxygen doc update * lexicon update Update proof tools (+disable submodule cloning by default) (aws#20) This commit advances Litani to release 1.10.0, and the starter kit to the tip-of-tree. This brings the following improvements: - Profiling - Litani measures the memory usage of the CBMC safety checking and coverage checking jobs - The dashboard includes box-and-whisker diagrams for memory use per proof - The dashboard includes a graph of how many parallel jobs are running over the whole run, making it easy to choose a CI machine with enough parallelism - It is now possible to designate particular proofs as "EXPENSIVE"; Litani runs expensive proofs serially, ensuring that they do not over-consume resources like RAM. - UI improvements - Each pipeline page includes a table of contents - Each pipeline page includes a dependency graph of the pipeline - Each job on the pipeline page has a hyperlink to that job - The terminal output is now less noisy SigV4_GenerateHTTPAuthorization() API Functionality (aws#16) * Squash of outdated aws#13 commits * Hold for checks * Add definitions for sorting structures * Include parsing functions * Fix old commit error * Missing asserts * (temporarily allow warnings) * Spell check + include partial context * More updates to lexicon+doxygen * Add asserts for private func. * Move access after asserts * Clarify pointer increment * Update postfix syntax for correct operator precedence * Feedback changes only * + remove accidental duplicate Implement credential scope Implement generate credential query Validation of parameter count Solution a bit overcomplicated Squash bugs and canonical query parameters should also be sorted by value Finish canonicalize query Fix canonical URI encoding Add hash helper function Add hmac implementation Add newline chars for canonical request Finish writing of canonical request Hex-encoded hash of canonical request matches Write string to sign Fix bug Refactor writeStringToSign for complexity Allow HMAC keys to be passed through separate function calls Add code for generating signing key Fix hmac bug Generate the final signature correctly Fix bug Fix newline not being written Merge Shivangi's code Stylistic changes Link OpenSSL to the test Add unit tests attaining branch coverage of 71% Integrate Shivangi's latest changes Output authBufLen when complete Update logic when headers are precanonicalized. Add additional parameter checks for block/digest len Add documentation Fix test case Get complexity <= 8 for private functions Reduce complexity Remove use of %zu Revert changes to test as it was added to another PR Uncrustify and add doxygen strings. Add docs Resolve doxygen errors and lexicon.txt Document all private functions Fix remaining doxygen errors Update lexicon.txt Remove duplicate declaration Remove assertions on pQuery being NULL Add log messages for insufficient memory errors Uncrustify

…rary

…nd add helpful comments

source/include/sigv4_internal.h

muneebahmed10 · 2021-08-14T17:42:10Z

source/sigv4.c

@@ -1542,6 +1556,7 @@ static SigV4Status_t generateCredentialScope( const SigV4Parameters_t * pSigV4Pa

            if( sigV4Status != SigV4Success )
            {
+                LogError( ( "Unable to write Signed Headers for Canonical Request: Insufficient memory configured in \"SIGV4_PROCESSING_BUFFER_LENGTH\"" ) );


LOG_INSUFFICIENT_MEMORY_ERROR here too?

source/sigv4.c

muneebahmed10 · 2021-08-14T18:00:26Z

source/sigv4.c

+    /* Set the appropriate error code for failures. */
+    if( !isBufferSpaceSufficient )
+    {
+        returnStatus = SigV4InsufficientMemory;
+    }
+    else if( hmacStatus != 0 )
    {
        returnStatus = SigV4HashError;
    }
+    else
+    {
+        /* Empty else for MISRA C:2012 compliance. */
+    }


Since isBufferSpaceSufficient is only ever set once, you could just set the return status at the same time. Then this block could just be

Suggested change

/* Set the appropriate error code for failures. */

if( !isBufferSpaceSufficient )

{

returnStatus = SigV4InsufficientMemory;

}

else if( hmacStatus != 0 )

{

returnStatus = SigV4HashError;

}

else

{

/* Empty else for MISRA C:2012 compliance. */

}

/* Set the error code for hash failures. */

if( hmacStatus != 0 )

{

returnStatus = SigV4HashError;

}

and you wouldn’t need the empty else

muneebahmed10 · 2021-08-14T18:02:44Z

source/sigv4.c

+        /* Default arguments. */
+        if( ( pParams->pAlgorithm == NULL ) || ( pParams->algorithmLen == 0U ) )
+        {
+            /* The default algorithm is AWS4-HMAC-SHA256. */
+            pAlgorithm = SIGV4_AWS4_HMAC_SHA256;
+            algorithmLen = SIGV4_AWS4_HMAC_SHA256_LENGTH;
+        }
+        else
+        {
+            pAlgorithm = pParams->pAlgorithm;
+            algorithmLen = pParams->algorithmLen;
+        }


If the complexity of this function is too high, you can reduce it by moving this code out of the if( returnStatus == SigV4Success ) block while adding an additional condition ( pParams == NULL ). Alternatively, you can remove the if-else and use a ternary for both of them

Because I did this already:

/** * @brief Verify all authorization-related parameters passed to * #SigV4_GenerateHTTPAuthorization. * * @param[in] pParams Complete SigV4 configurations passed by application. * @param[out] pAlgorithm The algorithm used for SigV4 authentication. * @param[out] algorithmLen The length of @p pAlgorithm. */ static void assignDefaultArguments( const SigV4Parameters_t * pParams, char ** pAlgorithm, size_t * algorithmLen );

static void assignDefaultArguments( const SigV4Parameters_t * pParams, char ** pAlgorithm, size_t * algorithmLen ) { if( ( pParams->pAlgorithm == NULL ) || ( pParams->algorithmLen == 0 ) ) { /* The default algorithm is AWS4-HMAC-SHA256. */ *pAlgorithm = SIGV4_AWS4_HMAC_SHA256; *algorithmLen = SIGV4_AWS4_HMAC_SHA256_LENGTH; } else { *pAlgorithm = pParams->pAlgorithm; *algorithmLen = pParams->algorithmLen; } }

muneebahmed10 · 2021-08-14T18:11:39Z

source/sigv4_quicksort.c

+        /* Calculate length of the left partition containing items smaller
+         * than the pivot element.
+         * The length is zero if either:
+         * 1. The pivoted item is the smallest in the
+         * the array before partitioning.
+         *              OR
+         * 2. The left partition is only of single length which can be treated as
+         * sorted, and thus, of zero length for avoided adding to the stack. */


You could alternatively describe this as

Calculate the difference between the first and last indices of the left partition. Partitions with more than one element are not guaranteed to be sorted so must be pushed onto the stack. If the pivot is either the smallest or second smallest item in the array, the left partition is sorted.

source/sigv4_quicksort.c

yourslab · 2021-08-15T00:50:26Z

source/sigv4.c

+                                 SIGV4_HMAC_SIGNING_KEY_PREFIX,
+                                 SIGV4_HMAC_SIGNING_KEY_PREFIX_LEN,
+                                 true /* Is key prefix. */ );
+        assert( hmacStatus == 0 );


Why assert this? That may certainly not be the case depending on how the hash functions are implemented.

Adding the prefix does not involve hashing operations but only involves adding the prefix string to the cache. Thus, I added the assert.

yourslab · 2021-08-15T00:51:15Z

source/sigv4.c

@@ -2741,27 +2787,36 @@ static SigV4Status_t generateSigningKey( const SigV4Parameters_t * pSigV4Params,
    SigV4Status_t returnStatus = SigV4Success;
    int32_t hmacStatus = 0;
    char * pSigningKeyStart = NULL;
+    bool isBufferSpaceSufficient = true;


Is an extra flag really necessary here? One can just check the current value of returnStatus.

test/CMakeLists.txt

yourslab · 2021-08-15T00:54:56Z

source/sigv4.c

            {
+                returnStatus = SigV4InsufficientMemory;
+                LogError( ( "Unable to write canonical query: Insufficient memory configured in \"SIGV4_PROCESSING_BUFFER_LENGTH\"" ) );


Why enclose SIGV4_PROCESSING_BUFFER_LENGTH in quotation marks? Makes it seem like a string. Better to refer to it as a macro and to mention about how much memory is lacking.

yourslab · 2021-08-15T00:56:14Z

source/sigv4.c

-                         size_t dataLen );
+static int32_t hmacIntermediate( HmacContext_t * pHmacContext,
+                                 const char * pData,
+                                 size_t dataLen );


The whole point of this naming scheme was to match the naming schemes of the hash interface.

Also, the original terminology is more commonly used in cryptographic operations

I felt that using terminology that follows the HMAC algorithmic operations is easier for a developer to make sense of the relationship between the hmac* internal functions. As these are internal functions, I prefer prioritize ease of readability

It would be good to rename the hash interface then to match this new naming scheme

test/CMakeLists.txt

yourslab · 2021-08-15T00:59:18Z

test/unit-test/sigv4_utest.c

+    params.pCredentials->pSecretAccessKey = SECRET_KEY_LONGER_THAN_HASH_BLOCK;
+    params.pCredentials->secretAccessKeyLen = strlen( SECRET_KEY_LONGER_THAN_HASH_BLOCK );
+
+    for( i = 0U; i < HAPPY_PATH_HASH_ITERATIONS; i++ )


This is certainly a hack for now. I'd just leave a comment if you run out of time in actually being able to assert what iteration you expect to fail.

yourslab · 2021-08-15T01:12:35Z

source/sigv4.c

        for( i = 0U; i < pCryptoInterface->hashBlockLen; i++ )
        {
-            pHmacContext->key[ i ] ^= 0x6aU;
+            pHmacContext->key[ i ] ^= ( HMAC_INNER_PAD_BYTE ^ HMAC_OUTER_PAD_BYTE );


This may XOR ipad and opad constants on each iteration depending on the compiler. Better to have a macro for 0x6aU.

yourslab · 2021-08-15T01:14:46Z

source/sigv4.c

+static int32_t hmacAddKey( HmacContext_t * pHmacContext,
+                           const char * pKey,
+                           size_t keyLen,
+                           bool isKeyPrefix );


Why bother with another input parameter?

Because we want to add the zero padding only when the key is completely received. I thought it is better to encapsulate the K' operation within this function than distribute it across two functions

source/sigv4.c

test/unit-test/sigv4_utest.c

yourslab · 2021-08-15T01:27:49Z

test/unit-test/sigv4_utest.c

+    params.pHttpParameters->pQuery = QUERY_VALUE_HAS_EQUALS;
+    params.pHttpParameters->queryLen = STR_LIT_LEN( QUERY_VALUE_HAS_EQUALS );
+    returnStatus = SigV4_GenerateHTTPAuthorization( &params, authBuf, &authBufLen, &signature, &signatureLen );
+    TEST_ASSERT_EQUAL( SigV4Success, returnStatus );


So we never assert the actual signature matches what we expect. That's quite useful to have for a test, but I guess we've run out of time at this point.

Or it would be good to have at least one test case that asserts the signature. Otherwise, this is really just a test of return codes for the sake of coverage...

Co-authored-by: Oscar Michael Abrina <[email protected]> Co-authored-by: Muneeb Ahmed <[email protected]>

abhidixi11

I'm approving it based on the decision that unit test code quality related comments will be addressed in subsequent PR.

aggarw13 · 2021-08-15T19:56:46Z

source/sigv4_quicksort.c

@@ -202,31 +201,32 @@ static size_t partition( void * pArray,
                         size_t itemSize,
                         ComparisonFunc_t comparator )
 {
-    void * pivot;
+    uint8_t * pivot;
+    uint8_t * pArrayLocal = ( uint8_t * ) pArray;


This addresses the warnings generated (by gcc-7.5.0) for using arithmetic operations on void *

/home/ubuntu/Repos/SigV4-for-AWS-IoT-embedded-sdk/source/sigv4_quicksort.c:212:20: warning: pointer of type ‘void *’ used in arithmetic [-Wpointer-arith] pivot = pArray + ( high * itemSize ); ^ /home/ubuntu/Repos/SigV4-for-AWS-IoT-embedded-sdk/source/sigv4_quicksort.c:220:32: warning: pointer of type ‘void *’ used in arithmetic [-Wpointer-arith] if( comparator( pArray + ( j * itemSize ), pivot ) < 0 ) ^ /home/ubuntu/Repos/SigV4-for-AWS-IoT-embedded-sdk/source/sigv4_quicksort.c:223:26: warning: pointer of type ‘void *’ used in arithmetic [-Wpointer-arith] swap( pArray + ( i * itemSize ), pArray + ( j * itemSize ), itemSize ); ^ /home/ubuntu/Repos/SigV4-for-AWS-IoT-embedded-sdk/source/sigv4_quicksort.c:223:53: warning: pointer of type ‘void *’ used in arithmetic [-Wpointer-arith] swap( pArray + ( i * itemSize ), pArray + ( j * itemSize ), itemSize ); ^ /home/ubuntu/Repos/SigV4-for-AWS-IoT-embedded-sdk/source/sigv4_quicksort.c:229:18: warning: pointer of type ‘void *’ used in arithmetic [-Wpointer-arith] swap( pArray + ( ( i + 1U ) * itemSize ), pivot, itemSize ); ^

Hygiene Changes Address unresolved feedback from PR SigV4_GenerateHTTPAuthorization unit tests #29 Improve tests to include verification of generated signature in happy-path test cases Bugfixes Fix 2 categories of bugs found from adding signature verification in tests: Fix parsing of query string for cases empty parameter values (with the format <param>=&<param2> OR <param>&<param2>) Fix URI encoding to not encode NULL character.

yourslab added 5 commits August 4, 2021 15:46

Link OpenSSL to the test

cc45898

Add unit tests attaining branch coverage of 71%

5d60836

Output authBufLen when complete

9a688f5

Fix test case

6174e8d

Revert changes to sigv4 sources

fa4a976

muneebahmed10 reviewed Aug 4, 2021

View reviewed changes

yourslab marked this pull request as draft August 4, 2021 21:45

yourslab mentioned this pull request Aug 5, 2021

SigV4_GenerateHTTPAuthorization #27

Merged

amazon-auto and others added 12 commits August 9, 2021 18:30

Merge doxygen

e1d370f

Create SigV4ConstString_t type

5140fa6

Fix checks

7cb9491

Hygiene improvements in URI encoding logic

43258d6

Hygiene improvements in Authorization Header prefix value logic

bcd8e72

Minor improvements

962a2f7

Resolve compiler warning

ab889d5

Fix checks

0a81e65

Address review comments

613363d

More comment changes

2380cab

Merge branch 'generate-auth-header' into sigv4-unit-tests

dea0cc9

aggarw13 force-pushed the sigv4-unit-tests branch 2 times, most recently from 60ede09 to 2d61d9d Compare August 10, 2021 18:29

Fix build errors

098f715

aggarw13 force-pushed the sigv4-unit-tests branch from 2d61d9d to 098f715 Compare August 10, 2021 18:30

Fix unit test run failures

999a91d

aggarw13 force-pushed the sigv4-unit-tests branch from 58d4fd0 to 999a91d Compare August 10, 2021 22:39

aggarw13 added 2 commits August 10, 2021 22:42

Merge remote-tracking branch 'origin/main' into sigv4-unit-tests

080469e

Add test case for sorting corner cases

aebaf44

aggarw13 force-pushed the sigv4-unit-tests branch from 7771717 to aebaf44 Compare August 11, 2021 06:05

Minor coverage increment and hygiene of redundant length check in lib…

412c972

…rary

Quicksort: Remove unnecessary branches of invalid array or elements a…

26b26bd

…nd add helpful comments

aggarw13 force-pushed the sigv4-unit-tests branch 2 times, most recently from b771c24 to 2f1427a Compare August 14, 2021 09:59

100% coverage again

6c04aba

aggarw13 force-pushed the sigv4-unit-tests branch from 0c48f4c to 6c04aba Compare August 14, 2021 10:14

muneebahmed10 suggested changes Aug 14, 2021

View reviewed changes