Canonicalize and Generate A String to Sign #5
Conversation
| */ | ||
| SigV4Status_t SigV4_AwsIotDateToIso8601( const char * pDate, | ||
| size_t dateLen, | ||
| char pDateISO8601[ 17 ] ); |
There was a problem hiding this comment.
I guess this change will be rebased based on the recent updates in PR #3
| * @param[in] pURI The URI string to encode. | ||
| * @param[in] uriLen Length of pURI. | ||
| * @param[out] pCanonicalURI The resulting canonicalized URI. | ||
| * @param[in, out] canonicalURILen input: the length of pCanonicalURI, |
There was a problem hiding this comment.
| * @param[in, out] canonicalURILen input: the length of pCanonicalURI, | |
| * @param[in, out] canonicalURILen input: the length of pCanonicalURI buffer, |
| * | ||
| * @param[in] pURI The URI string to encode. | ||
| * @param[in] uriLen Length of pURI. | ||
| * @param[out] pCanonicalURI The resulting canonicalized URI. |
There was a problem hiding this comment.
Minor: I prefer keeping the output params at the end. Not a fan of mixing the in and out params in the function.
| * encoding should be done once or twice. For example, S3 requires that the | ||
| * URI is encoded only once, while other services encode twice. | ||
| * @param[in, out] canonicalRequest Struct to maintain intermediary buffer | ||
| * and state of canonicalization. |
There was a problem hiding this comment.
It will be better to specify what is the input and output for this param.
| * @param[in] uriLen Length of pURI. | ||
| * @param[in] encodeOnce Service-dependent option to indicate whether | ||
| * encoding should be done once or twice. For example, S3 requires that the | ||
| * URI is encoded only once, while other services encode twice. |
| canonicalContext_t * canonicalRequest ); | ||
|
|
||
| /** | ||
| * @brief Canonicalize the query string HTTP URL, beginning (but not |
There was a problem hiding this comment.
| * @brief Canonicalize the query string HTTP URL, beginning (but not | |
| * @brief Canonicalize the query string in HTTP URL, beginning (but not |
| /*-----------------------------------------------------------*/ | ||
|
|
||
| /* Converts a hex character to its integer value */ | ||
| static char hexToInt( char pHex ) |
There was a problem hiding this comment.
Did you intend to use int/int8_t as the return type here?
| /* Converts a hex character to its integer value */ | ||
| static char hexToInt( char pHex ) | ||
| { | ||
| return isdigit( pHex ) ? pHex - '0' : tolower( pHex ) - 'a' + 10; |
There was a problem hiding this comment.
Don't we have to ensure that the character is only through a-f as well here?
| } | ||
|
|
||
| /* Converts an integer value to its hex character */ | ||
| static char intToHex( char pInt ) |
There was a problem hiding this comment.
Did you intend to use integer as a param here?
| { | ||
| static char hex[] = "0123456789abcdef"; | ||
|
|
||
| return hex[ pInt & 15 ]; |
There was a problem hiding this comment.
Don't we have to make sure that the integer is below 16?
| assert( pURI != NULL ); | ||
| assert( pCanonicalURI != NULL ); | ||
| assert( canonicalURILen != NULL ); | ||
| assert( *canonicalURILen > 0U ); |
| } | ||
| else | ||
| { | ||
| *pBufLoc++ = '%', *pBufLoc++ = intToHex( *pURILoc >> 4 ), *pBufLoc++ = intToHex( *pURILoc & 15 ); |
There was a problem hiding this comment.
What does this case handle? Can you please add comments about the details of the operations to make it more readable.?
What does ',' do here?
| index++; | ||
| } | ||
|
|
||
| *canonicalURILen = index; |
There was a problem hiding this comment.
Please make sure in the loop above, we are not writing out of the bounds of the buffer.
| assert( pURI != NULL ); | ||
| assert( canonicalRequest != NULL ); | ||
|
|
||
| encodeURI( pURI, uriLen, pBufLoc, &encodedLen, false, true ); |
There was a problem hiding this comment.
The documentation of the function mentions about passing the bufer length as the input as well. Does the logic need update to do that?
| if( !encodeOnce ) | ||
| { | ||
| encodeURI( pBufLoc, encodedLen, pBufLoc + encodedLen, &remainingLen, false, true ); | ||
| memmove( canonicalRequest->pBufCur + encodedLen, canonicalRequest->pBufCur, remainingLen ); |
There was a problem hiding this comment.
Why is this memmove required? Is the src and dst pointers reversed here?
| memmove( canonicalRequest->pBufCur + encodedLen, canonicalRequest->pBufCur, remainingLen ); | ||
| } | ||
|
|
||
| canonicalRequest->pBufCur += remainingLen; |
There was a problem hiding this comment.
I think this will be correct only if the encodeOnce is false.
| } | ||
|
|
||
| canonicalRequest->pBufCur += remainingLen; | ||
| *( canonicalRequest->pBufCur++ ) = '\n'; |
There was a problem hiding this comment.
please check for a possible buffer overflow before writing to the buffer.
|
|
||
| int compare = strcmp( token_a, token_b ); | ||
|
|
||
| if( strcmp == 0 ) |
| assert( token_b != NULL ); | ||
|
|
||
| compare = strcmp( token_a, token_b ); | ||
| } |
There was a problem hiding this comment.
Not sure if I understood the meaning of this if condition
There was a problem hiding this comment.
Adding doxygen comments for all the static functions in this file similar to those already present at the top of the file, would help to improve the readability of the code.
|
|
||
| while( tokenQueries != NULL ) | ||
| { | ||
| canonicalRequest->pQueryLoc[ index ] = &tokenQueries[ 0 ]; |
There was a problem hiding this comment.
what is the size of the array pQueryLoc?
| index++; | ||
| } | ||
|
|
||
| qsort( canonicalRequest->pQueryLoc, index, cmpFun ); |
There was a problem hiding this comment.
I think now I understand what the cmpFunc is for. However, more comments would help for that function.
|
|
||
| if( tokenParams != NULL ) | ||
| { | ||
| encodeURI( tokenParams, strlen( tokenParams ), pBufLoc, &remainingLen, true, false ); |
There was a problem hiding this comment.
is tokenParams null terminated? If not, strlen will fail to work.
|
|
||
| static SigV4Status_t verifySigV4Parameters( const SigV4Parameters_t * pParams ) | ||
| { | ||
| SigV4Status_t returnStatus = SigV4Success; |
There was a problem hiding this comment.
Initializing to SigV4InvalidParameter will be better here and code can be reduced that way
| * <b>Possible values:</b> Any positive 32 bit integer. <br> | ||
| * <b>Default value:</b> `1024` | ||
| */ | ||
| #ifndef SIGV4_PROCESSING_BUFFER_LENGTH |
There was a problem hiding this comment.
Is this a fixed size required for the operations it is used in? If yes, should it be a config value?
| * @brief Macro defining the digest length of the specified hash function, used | ||
| * to determine the length of the output buffer. |
There was a problem hiding this comment.
| * @brief Macro defining the digest length of the specified hash function, used | |
| * to determine the length of the output buffer. | |
| * @brief Macro defining the digest length (in bytes) of the specified hash function, used | |
| * to determine the length of the output buffer. |
| * <b>Possible values:</b> Any positive 32 bit integer. <br> | ||
| * <b>Default value:</b> `100` | ||
| */ | ||
| #ifndef SIGV4_MAX_HTTP_HEADER_COUNT |
There was a problem hiding this comment.
How is the default value of 100 chosen?
| @@ -0,0 +1,183 @@ | |||
| /* | |||
| * SigV4 Utility Library v1.0.0 | |||
| * Copyright (C) 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved. | |||
There was a problem hiding this comment.
Change is relevant across the board
| * Copyright (C) 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved. | |
| * Copyright (C) 2021 Amazon.com, Inc. or its affiliates. All Rights Reserved. |
| * to determine the length of the output buffer. | ||
| * | ||
| * This macro should be updated if using a hashing algorithm other than SHA256 | ||
| * (32 byte digest length). For example, SHA512 would require this macro to be |
There was a problem hiding this comment.
Is HMAC-SHA512 algorithm supported in SigV4? If yes, would that require a macro equivalent of the SIGV4_AWS4_HMAC_SHA256 macro for SHA256?
| #define SIGV4_AWS4_HMAC_SHA256 "AWS4-HMAC-SHA256" | ||
|
|
||
| /**< AWS identifier for S3 SigV4 streaming upload. */ | ||
| #define SIGV4_STREAMING_AWS4_HMAC_SHA256_PAYLOAD "STREAMING-AWS4-HMAC-SHA256-PAYLOAD" | ||
|
|
||
| /**< AWS identifiers for Amazon specific HTTP header fields. */ | ||
| #define SIGV4_HTTP_X_AMZ_DATE_HEADER "x-amz-date" | ||
| #define SIGV4_HTTP_X_AMZ_SECURITY_TOKEN_HEADER "x-amz-security-token" | ||
| #define SIGV4_HTTP_X_AMZ_CONTENT_SHA256_HEADER "x-amz-content-sha256" | ||
| #define SIGV4_HTTP_X_AMZ_STORAGE_CLASS_HEADER "x-amz-storage-class" |
There was a problem hiding this comment.
Can these be internal constants instead of exposing in the public API?
| * @brief The date in ISO 8601 format, e.g. "20150830T123600Z". This is | ||
| * always 16 characters long. | ||
| */ | ||
| const char * pDateIso8601; |
There was a problem hiding this comment.
If the date buffer is fixed, having it defined explicitly would be helpful for the user.
| const char * pDateIso8601; | |
| const char pDateIso8601[16]; |
Description of changes: Adding internal functions used in constructing the canonicalized strings matching those created service-side for signature verification.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.