SigV4_GenerateHTTPAuthorization() API Functionality #16

sukhmanm · 2021-06-24T17:15:42Z

Description of changes: Adds API functionality for SigV4_GenerateAuthorizationHeader(). The unit tests for this function will be added in a separate PR, hence the failing check here.

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

source/sigv4.c

gshvang

In General, more comments can be added to the code in the functions to understand easily how they are achieving the required functionality.

source/sigv4.c

cobusve · 2021-07-19T23:35:16Z

source/sigv4.c

+
+/*-----------------------------------------------------------*/
+
+static SigV4Status_t verifySigV4Parameters( const SigV4Parameters_t * pParams )


I have 2 comments here, firstly a lot of error strings like this can end up taking a large amount of memory on smaller systems. Did we consider if it is possible to implement this in such a way that we can minimize the memory footprint on smaller devices?

Second question, it is always useful when you return an error code which represents a the same granularity of knowledge about the problem that you know, that way it is possible for callers to act accordingly when they get an error . In this case we return the same error code for any parameter which is incorrect. If we decide to just return SigV4InvalidParameter then I would get rid of all the duplicated assignments and invert the logic to set the return status to SigV4InvalidParameter at init and just set it to success in a last else?

One further question, this is a static function defined in this file but never called?

That is an interesting point, and something we hadn't given too much thought. Although the logging approach used was modelled a bit after coreHTTP (HTTPClient_ReadHeader, for example), it does seem like there could be much room for optimization here, just given the number of null check repetitions required, as one example.

As you alluded to, the general approach we followed here was to supply detailed error logs explaining the rather vague status values returned from the library, but it might be worth expanding the range and detail of these enums, and maybe providing some sort of generic function to log them and avoid redundancy as much as possible.

I will discuss this in some more detail and if time permits, address any refactoring decided upon independently. Let me know if you disagree with this approach or have any further suggestions on this. Thanks.

source/sigv4.c

emilmelnikov · 2021-07-25T19:33:38Z

source/sigv4.c

+    for( i = 0; i < pInputStr->dataLen; i++ )
+    {
+        *( hex++ ) = digitArr[ ( pInputStr->pData[ i ] & 0xF0 ) >> 4 ];
+        *( hex++ ) = digitArr[ ( pInputStr->pData[ i ] & 0xF0 ) ];


Suggested change

*( hex++ ) = digitArr[ ( pInputStr->pData[ i ] & 0xF0 ) ];

*( hex++ ) = digitArr[ ( pInputStr->pData[ i ] & 0x0F ) ];

source/include/sigv4_internal.h

source/sigv4.c

aggarg · 2021-07-27T22:03:44Z

source/sigv4.c

+
+/*-----------------------------------------------------------*/
+
+    static int cmpKeyValue( const void * pFirstVal,


why use void* here? Can we not use SigV4KeyValuePair_t?

It was just to match the function signature that's defined for the qsort() comparison functions, which is the intention for this function as well (during canonicalization).

source/sigv4.c

aggarg

The remaining comments will be addressed in a separate PR.

abhidixi11 · 2021-07-29T17:41:26Z

See comment from @aggarg

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

* Link OpenSSL to the test * Add unit tests attaining branch coverage of 71% * Output authBufLen when complete * Fix test case * Revert changes to sigv4 sources * SigV4_GenerateHTTPAuthorization Implementation Update auto-generated .md files Add test and tool directories (#1) Adding test and tools directories, with CMock submodule. CI Actions (#2) Add header files + default configurations (#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 (#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() (#8) Setup proof infrastructure for CBMC (#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 (#12) * CBMC fix test (do not merge) * Change AWS templates to https instead of ssh * Revert README Update README.md and LICENSE files (#14) Update README.md and LICENSE files before changing repo's visibility status (to public). Add remaining doxygen + link verification checks (#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 (#19) * Sigv4_AWSIOtDateToISO8601 CBMC PROOF * Unit test coverage changes Add release workflow (+ revert to previous license) (#18) change permissions of run_cbmc_proofs.py (#21) [Sigv4] Doxygen content updates (#22) * doxygen doc update * lexicon update Update proof tools (+disable submodule cloning by default) (#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 (#16) * Squash of outdated #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 * Merge doxygen * Create SigV4ConstString_t type * Fix checks * Hygiene improvements in URI encoding logic * Hygiene improvements in Authorization Header prefix value logic * Minor improvements * Resolve compiler warning * Fix checks * Address review comments * More comment changes * Fix build errors * Fix unit test run failures * Add test case for sorting corner cases * Minor coverage increment and hygiene of redundant length check in library * Prune API to remove unused members of struct, and add test coverage for input parameter validation * More code coverage on logic of trimmable spaces & header count > threshold * Hygiene improvements in build configuration * Small refactor in implementation and complete testing coverage of encodeURI * Fix bugs in encodeURI implementation when handling special characters or double encoded equals sign * Fix some CI checks * Disable asserts from unit test coverage * Fix some doxygen failures * Address minor review comments * Add error code for invalid HTTP headers and increment test coverage * Hygiene improvement in sigv4.c and test coverage increment * Minor hygiene refactor in implementation and test coverage for canonical query logic * Complete test coverage for canonical functions * Fix some CI check failures * Make more hygiene improvements and increase test coverage * Achieve 100% coverage * Hygiene improvements * Minor README.md update * Address review comments * Quicksort: Remove unnecessary branches of invalid array or elements and add helpful comments * 100% coverage again * Apply suggestions from code review Co-authored-by: Oscar Michael Abrina <[email protected]> Co-authored-by: Muneeb Ahmed <[email protected]> * Fix complexity and hygiene improvements Co-authored-by: Archit Aggarwal <[email protected]> Co-authored-by: Muneeb Ahmed <[email protected]>

Update doxygen in cbmc-proof branch (#275)

sukhmanm force-pushed the authorization branch from d2f8d48 to ed97d58 Compare June 24, 2021 17:45

sukhmanm marked this pull request as ready for review July 2, 2021 20:29

gshvang reviewed Jul 7, 2021

View reviewed changes

source/sigv4.c Show resolved Hide resolved

gshvang reviewed Jul 7, 2021

View reviewed changes

sukhmanm added 4 commits July 15, 2021 12:43

Squash of outdated aws#13 commits

43e1af6

Hold for checks

47f92af

Add definitions for sorting structures

cb6aec8

Include parsing functions

0fa043a

sukhmanm force-pushed the authorization branch 2 times, most recently from 3483e7e to e3f83b5 Compare July 15, 2021 16:57

gshvang suggested changes Jul 15, 2021

View reviewed changes

source/sigv4.c Outdated Show resolved Hide resolved

source/sigv4.c Show resolved Hide resolved

source/sigv4.c Outdated Show resolved Hide resolved

source/sigv4.c Show resolved Hide resolved

Fix old commit error

1a2ac37

sukhmanm force-pushed the authorization branch from e3f83b5 to 1a2ac37 Compare July 15, 2021 21:23

gshvang suggested changes Jul 15, 2021

View reviewed changes

source/sigv4.c Outdated Show resolved Hide resolved

source/sigv4.c Outdated Show resolved Hide resolved

sukhmanm added 4 commits July 15, 2021 20:56

Missing asserts

50ca124

(temporarily allow warnings)

1cd4f83

Spell check + include partial context

8d83543

More updates to lexicon+doxygen

8cad50b

sukhmanm force-pushed the authorization branch from 54d9d50 to 8cad50b Compare July 16, 2021 00:57

gshvang reviewed Jul 16, 2021

View reviewed changes

source/sigv4.c Outdated Show resolved Hide resolved

gshvang reviewed Jul 16, 2021

View reviewed changes

source/sigv4.c Outdated Show resolved Hide resolved

gshvang reviewed Jul 16, 2021

View reviewed changes

source/sigv4.c Outdated Show resolved Hide resolved

source/sigv4.c Outdated Show resolved Hide resolved

Add asserts for private func.

6575a77

sukhmanm force-pushed the authorization branch from 3f17009 to 6575a77 Compare July 16, 2021 20:55

gshvang reviewed Jul 16, 2021

View reviewed changes

source/sigv4.c Outdated Show resolved Hide resolved

source/sigv4.c Outdated Show resolved Hide resolved

sukhmanm added 2 commits July 16, 2021 19:44

Merge branch 'main' into authorization

c29c69f

Move access after asserts

865fa7e

gshvang previously approved these changes Jul 19, 2021

View reviewed changes

sukhmanm changed the title ~~SigV4_GenerateHTTPAuthorization() + Unit Tests~~ SigV4_GenerateHTTPAuthorization() API Functionality Jul 19, 2021

cobusve reviewed Jul 19, 2021

View reviewed changes

Clarify pointer increment

5712aef

cobusve reviewed Jul 20, 2021

View reviewed changes

source/sigv4.c Outdated Show resolved Hide resolved

cobusve reviewed Jul 20, 2021

View reviewed changes

source/sigv4.c Outdated Show resolved Hide resolved

cobusve reviewed Jul 20, 2021

View reviewed changes

source/sigv4.c Outdated Show resolved Hide resolved

cobusve reviewed Jul 20, 2021

View reviewed changes

source/sigv4.c Outdated Show resolved Hide resolved

sukhmanm dismissed gshvang’s stale review via 5712aef July 20, 2021 23:00

Update postfix syntax for correct operator precedence

28bbab5

emilmelnikov reviewed Jul 25, 2021

View reviewed changes

aggarg reviewed Jul 27, 2021

View reviewed changes

Feedback changes only

f81cfe2

sukhmanm force-pushed the authorization branch from 9bc0f97 to e44b7f3 Compare July 29, 2021 01:49

+ remove accidental duplicate

3dfb4ca

sukhmanm force-pushed the authorization branch from e44b7f3 to 3dfb4ca Compare July 29, 2021 02:20

aggarg approved these changes Jul 29, 2021

View reviewed changes

abhidixi11 merged commit bacef82 into aws:main Jul 29, 2021

markrtuttle pushed a commit to markrtuttle/SigV4-for-AWS-IoT-embedded-sdk that referenced this pull request Sep 2, 2021

Merge pull request aws#16 from aws/cbmc-proof

9e7e978

Update doxygen in cbmc-proof branch (#275)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SigV4_GenerateHTTPAuthorization() API Functionality #16

SigV4_GenerateHTTPAuthorization() API Functionality #16

sukhmanm commented Jun 24, 2021 •

edited

Loading

gshvang left a comment

cobusve Jul 19, 2021

cobusve Jul 20, 2021

sukhmanm Jul 20, 2021

emilmelnikov Jul 25, 2021

aggarg Jul 27, 2021

sukhmanm Jul 29, 2021

aggarg left a comment

abhidixi11 commented Jul 29, 2021


		/-----------------------------------------------------------/

		static SigV4Status_t verifySigV4Parameters( const SigV4Parameters_t * pParams )

	*( hex++ ) = digitArr[ ( pInputStr->pData[ i ] & 0xF0 ) ];
	*( hex++ ) = digitArr[ ( pInputStr->pData[ i ] & 0x0F ) ];


		/-----------------------------------------------------------/

		static int cmpKeyValue( const void * pFirstVal,

SigV4_GenerateHTTPAuthorization() API Functionality #16

SigV4_GenerateHTTPAuthorization() API Functionality #16

Conversation

sukhmanm commented Jun 24, 2021 • edited Loading

gshvang left a comment

Choose a reason for hiding this comment

cobusve Jul 19, 2021

Choose a reason for hiding this comment

cobusve Jul 20, 2021

Choose a reason for hiding this comment

sukhmanm Jul 20, 2021

Choose a reason for hiding this comment

emilmelnikov Jul 25, 2021

Choose a reason for hiding this comment

aggarg Jul 27, 2021

Choose a reason for hiding this comment

sukhmanm Jul 29, 2021

Choose a reason for hiding this comment

aggarg left a comment

Choose a reason for hiding this comment

abhidixi11 commented Jul 29, 2021

sukhmanm commented Jun 24, 2021 •

edited

Loading