Format Date Header for ISO8601 Compliance #4
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sukhmanm
force-pushed
the
iso-function
branch
2 times, most recently
from
e7ccb51 to
3f57a84
Compare
leegeth
reviewed
Mar 17, 2021
leegeth
reviewed
Mar 17, 2021
leegeth
reviewed
Mar 17, 2021
leegeth
reviewed
Mar 17, 2021
leegeth
reviewed
Mar 17, 2021
leegeth
reviewed
Mar 18, 2021
leegeth
reviewed
Mar 19, 2021
leegeth
reviewed
Mar 19, 2021
aggarw13
reviewed
Mar 22, 2021
aggarw13
reviewed
Mar 22, 2021
aggarw13
reviewed
Mar 22, 2021
aggarw13
reviewed
Mar 22, 2021
aggarw13
reviewed
Mar 22, 2021
aggarw13
reviewed
Mar 26, 2021
aggarw13
reviewed
Mar 26, 2021
aggarw13
reviewed
Mar 26, 2021
aggarw13
reviewed
Mar 26, 2021
aggarw13
reviewed
Mar 26, 2021
aggarw13
previously approved these changes
Mar 27, 2021
source/include/sigv4.h
Outdated
| * https://docs.aws.amazon.com/general/latest/gr/sigv4-date-handling.html. | ||
| * | ||
| * Formatting Overview: | ||
| * - The AWS IoT response date is of the form "YYYY-MM-DD'T'hh:mm:ss'Z'" (ex. |
There was a problem hiding this comment.
Do we know that this will always be returned to us in UTC? Also, since UTC is required for this function, it would be helpful to explicitly state that in these comments instead of inferring it from the format
There was a problem hiding this comment.
Sounds good, I've added a comment clarifying the UTC requirement.
The HTTP specification limits the Date header to UTC/GMT, but I believe there are 3 variations of acceptable (less common) standard formats, which I had planned to revisit in a future PR.
dan4thewin
reviewed
Mar 31, 2021
muneebahmed10
approved these changes
Apr 21, 2021
| returnStatus = SigV4ISOFormattingError; | ||
| } | ||
|
|
||
| return ( returnStatus != SigV4ISOFormattingError ) ? SigV4Success : returnStatus; |
There was a problem hiding this comment.
Looks a bit weird. If the only possible return values are formatting error and success, it would look a little less weird if it was initialized to success, then this line could just be return returnStatus;
There was a problem hiding this comment.
Sure, sounds good -- I'll address both comments in #8 once this has been merged.
| formatIndex++; | ||
|
|
||
| /* Numerical value of length specifier character. */ | ||
| lenToRead = ( ( uint64_t ) pFormat[ formatIndex ] - ( uint64_t ) '0' ); |
There was a problem hiding this comment.
uint64_t seems unnecessary for a value that looks to be a single digit number at most. If it's because of size_t you can just use uint32_t
sukhmanm
referenced
this pull request
in sukhmanm/sigv4-for-aws-iot-embedded-sdk-forked
Apr 22, 2021
aggarg
approved these changes
Apr 23, 2021
| { | ||
| int32_t tm_year; /**< Year (1900 or later) */ | ||
| int32_t tm_mon; /**< Month (1 to 12) */ | ||
| int32_t tm_mday; /**< Day of Month (1 to 28/29/20/31) */ |
There was a problem hiding this comment.
Suggested change
| int32_t tm_mday; /**< Day of Month (1 to 28/29/20/31) */ | |
| int32_t tm_mday; /**< Day of Month (1 to 28/29/30/31) */ |
| * @param[in] dateLen Length of pDate, the date to be formatted. | ||
| * @param[in] pFormat The format string used to extract date pDateElements from pDate. | ||
| * This string, among other characters, may contain specifiers of the form | ||
| * "%LV", where L is the number of characters to be readLoc, and V is one of |
There was a problem hiding this comment.
Suggested change
| * "%LV", where L is the number of characters to be readLoc, and V is one of | |
| * "%LV", where L is the number of characters to be read, and V is one of |
| SigV4Status_t returnStatus = SigV4InvalidParameter; | ||
| const char * pMonthNames[] = MONTH_NAMES; | ||
| const char * pLoc = pDate + readLoc; | ||
| size_t readLen = lenToRead; |
There was a problem hiding this comment.
readLen --> remainingLengthToRead?
| * | ||
| * @param[in] formatChar The specifier identifying the struct member to fill. | ||
| * @param[in] result The value to assign to the specified struct member. | ||
| * @param[in, out] pDateElements The date representation structure to modify. |
| * @param[in] formatChar The format specifier used to interpret characters. | ||
| * @param[in] readLoc The index of pDate to read from. | ||
| * @param[in] lenToRead The number of characters to read. | ||
| * @param[in, out] pDateElements The date representation to modify. |
| formatIndex++; | ||
|
|
||
| /* Numerical value of length specifier character. */ | ||
| lenToRead = ( ( uint64_t ) pFormat[ formatIndex ] - ( uint64_t ) '0' ); |
aggarw13
dismissed
gkwicker’s stale review
Making change to unblock merging PR as approval has been received from PRBR (@aggarg).
yourslab
pushed a commit
to yourslab/SigV4-for-AWS-IoT-embedded-sdk-1
that referenced
this pull request
Aug 6, 2021
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
yourslab
pushed a commit
to yourslab/SigV4-for-AWS-IoT-embedded-sdk-1
that referenced
this pull request
Aug 6, 2021
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
yourslab
pushed a commit
to yourslab/SigV4-for-AWS-IoT-embedded-sdk-1
that referenced
this pull request
Aug 9, 2021
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
yourslab
added a commit
that referenced
this pull request
Aug 15, 2021
* 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]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes: 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.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.