Support presigned URL signature "UNSIGNED-PAYLOAD" and expose EncodeURI #89

giuspen · 2023-10-05T20:26:28Z

As discussed in aws/aws-iot-device-sdk-embedded-C#1887 I have made changes to an existing demo http_demo_s3_download to generate a presigned URL instead of directly downloading the file.

I'm planning, if the changes to the submodules are accepted, to then generate a new demo called http_demo_s3_presigned_url or something like that.

You can see/test my work at https://github.com/giuspen/aws-iot-device-sdk-embedded-C/tree/GP_http_demo_s3_download_test_signature (only generate presigned URL) or https://github.com/giuspen/aws-iot-device-sdk-embedded-C/tree/GP_http_demo_s3_download_test_signature2 (download and also generate presigned URL)

Exact same requirements of original demo http_demo_s3_download

By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.

moninom1 · 2023-10-09T03:40:09Z

Hello @giuspen, Thank you for raising the PR! We will be looking further into this issue as soon as we can, thank you for your patience!

giuspen · 2023-10-13T21:54:11Z

/bot run formatting

giuspen · 2023-11-09T08:42:22Z

I see that there are unit tests errors but that doesn't seem to be related to my change.
Can anybody help me reviewing this?

tony-josi-aws · 2023-11-13T18:26:01Z

@giuspen, Thanks for the PR. We will look into the unit test failures and update.

tony-josi-aws · 2023-11-17T11:03:59Z

source/sigv4.c

+                                                pBufCur + 1U,
+                                                &valueBytesWritten,
+                                                true,
+                                                false );


The unit test test_SigV4_GenerateHTTPAuthorization_Happy_Paths is failing because of this change [line 2066]. Can you please update the tests to match the changes in your PR?

Thanks for reviewing and apologies for missing that.
I completely forgot about this change so I excluded that my changes could ever cause a unit test failure.
I will review this now, maybe I'll propose a way to pass false here only with the flag SIGV4_HTTP_PAYLOAD_IS_UNSIGNED as to not upset other use cases.

…rmsValues

giuspen · 2023-11-17T17:15:24Z

I wanted to run the unit tests myself and possibly add one unit test myself to not waste your time and patience but the instructions in https://github.com/aws/SigV4-for-AWS-IoT-embedded-sdk/blob/main/README.md#building-unit-tests don't work for me, I get instead

penoneg@cinnamon:~/git/SigV4-for-AWS-IoT-embedded-sdk/build$ ctest
Test project /home/penoneg/git/SigV4-for-AWS-IoT-embedded-sdk/build
    Start 1: sigv4_utest
1/1 Test #1: sigv4_utest ......................Child aborted***Exception:   0.00 sec

0% tests passed, 1 tests failed out of 1

Total Test time (real) =   0.01 sec

The following tests FAILED:
	  1 - sigv4_utest (Child aborted)
Errors while running CTest

tony-josi-aws · 2023-11-20T04:47:37Z

@giuspen,

Can you try the following script to build and run the unit tests:

cmake -S test -B build/ -G "Unix Makefiles" -DCMAKE_BUILD_TYPE=Debug -DBUILD_UNIT_TESTS=ON -DCMAKE_C_FLAGS='--coverage -Wall -Wextra -DNDEBUG -DLOGGING_LEVEL_DEBUG=1'
make -C build/ all
cd build/ && ctest -E system --output-on-failure --verbose

giuspen · 2023-11-20T10:29:35Z

Thanks @tony-josi-aws I could run the unit tests with your command, I did check that my implementation didn't fail existing unit tests and I also added a unit test to protect the presigned URL

AniruddhaKanhere · 2023-11-22T21:04:07Z

@giuspen, it seems that your changes have increased the memory consumption of the library a bit. It is causing the memory-statistics check to fail. Would you mind updating the size_table.md like so:

  --- docs/doxygen/include/size_table.md	2023-11-22 20:54:32.891598188 +0000
  +++ size_table.html	2023-11-22 20:54:59.319864025 +0000
  @@ -10,5 +10,5 @@
       <tr>
           <td>sigv4.c</td>
  -        <td><center>5.2K</center></td>
  +        <td><center>5.3K</center></td>
           <td><center>4.4K</center></td>
       </tr>
  @@ -20,5 +20,5 @@
       <tr>
           <td><b>Total estimates</b></td>
  -        <td><b><center>5.6K</center></b></td>
  +        <td><b><center>5.7K</center></b></td>
           <td><b><center>4.7K</center></b></td>
       </tr>

I shall look at the failing memory safety proofs in the meanwhile.

thanks,
Aniruddha

giuspen · 2023-11-22T21:13:14Z

sure thanks for reviewing @AniruddhaKanhere

AniruddhaKanhere · 2023-11-22T22:35:49Z

@giuspen, thanks for updating the code.

I found the issue with memory safety proofs. We are using mocks to simplify and test implementation of functions. To that end, we have to define the stub of the function we intend to mock out.

The mock of generateCanonicalQuery here is missing the newly added const bool doubleEncodeEqualsInParmsValues, parameter. Can you add it please? You just need to add the parameter to that definition - it will not be used at all - and that is perfectly fine!

I think that should fix the failing check :)

Thanks again,
Aniruddha

giuspen · 2023-11-22T22:53:30Z

Thanks @AniruddhaKanhere , I noticed that in the same file that you pointed out there is also the stub for encodeURI() but that was renamed in my PR as it is now no longer private but exposed to the API. I wonder if I should remove that stub as no longer needed or rename it

AniruddhaKanhere · 2023-11-24T21:56:15Z

Thanks @AniruddhaKanhere , I noticed that in the same file that you pointed out there is also the stub for encodeURI() but that was renamed in my PR as it is now no longer private but exposed to the API. I wonder if I should remove that stub as no longer needed or rename it

Ah! Yes, I overlooked that. Thank you for catching it.
I think we should also rename the stub for encodeURI() to the new API that you have created.

giuspen · 2023-11-24T22:00:16Z

I suspect the stubs are just for private stuff while for the public API there's no need. I have removed completely the encodeURI() for now, would it be possible to try and run the CI and see how it behaves?

giuspen · 2023-11-24T22:01:19Z

If you are sure though I will restore and rename... let me know

AniruddhaKanhere · 2023-11-24T22:11:04Z

Those stubs work in a different way. I think we should rename them.

Those stubs are there so that CBMC does not need to compute everything all at once. Because, if CBMC were to try and prove all the functions in the call graph to be memory safe, it would take exponentially long time and quite a bit more RAM. These stubs reduce the computation that the CBMC SAT solver has to do.

Will keep an eye on the result though :)

Thank you for making that change.

giuspen · 2023-11-24T22:25:15Z

Thanks @AniruddhaKanhere I'm going to restore and rename the encodeURI() stub

giuspen · 2024-02-08T11:42:10Z

Is there any chance that my work will be merged in?
This will allow me to proceed submitting a new demo for the generation of a presigned URL to download a file from an S3 bucket.

tony-josi-aws · 2024-02-19T15:28:29Z

Hi @giuspen, could you help in fixing the failing CBMC proofs?

giuspen · 2024-02-19T15:59:46Z

Hi Tony, 100%, only thing it is not clear that CI error but maybe I can restore the private function name that I suspect the check is looking for and have the public function just call the private one. I'll give that a try this evening Europe time and push so we'll see if that makes the CI happy

giuspen · 2024-02-19T22:47:35Z

/bot run formatting

giuspen · 2024-02-21T07:38:21Z

I'm puzzled but I will try to run the test locally and possibly extract more info, the log of the error is not helping :(

tony-josi-aws · 2024-02-21T07:40:59Z

Hi @giuspen,

Thanks for updating the PR. Could you please apply this attached patch and update the PR again?
It should fix the failing CBMC proof.

0001-Fix-CBMC-proof-for-SigV4_GenerateHTTPAuthorization.patch

giuspen · 2024-02-21T08:02:57Z

Hi @tony-josi-aws thanks for the help, I applied your patch

giuspen added 3 commits October 1, 2023 20:32

Support for Presigned URLs

4be42de

encodeURI exposed to outside world

46d58bb

Merge branch 'aws:main' into GP_SupportForPresignedURLs

49dde4e

archigup previously approved these changes Oct 13, 2023

View reviewed changes

Uncrustify: triggered by comment.

e5abdea

github-actions bot dismissed archigup’s stale review via e5abdea October 13, 2023 21:55

moved #include <stdbool.h> to header to support SigV4_EncodeURI

e647f48

archigup previously approved these changes Oct 19, 2023

View reviewed changes

tony-josi-aws reviewed Nov 17, 2023

View reviewed changes

writeCanonicalQueryParameters() added argument doubleEncodeEqualsInPa…

6861d74

…rmsValues

giuspen dismissed archigup’s stale review via 6861d74 November 17, 2023 17:02

added unit test

bafc0ce

updated size_table.md

7e5286a

updated test cbmc stubs

5cb1d6c

updated test cbmc stubs - SigV4_EncodeURI()

1b383aa

Merge branch 'main' into GP_SupportForPresignedURLs

afc24eb

giuspen added 2 commits February 19, 2024 22:29

rework to minimise changes to not upset CI

cf6da4f

rework to minimise changes to not upset CI

2475223

fix for CBMC proof

5343f8f

AniruddhaKanhere approved these changes Feb 22, 2024

View reviewed changes

tony-josi-aws approved these changes Feb 23, 2024

View reviewed changes

tony-josi-aws merged commit e828353 into aws:main Feb 23, 2024
12 checks passed

giuspen deleted the GP_SupportForPresignedURLs branch February 23, 2024 07:34

kar-rahul-aws mentioned this pull request May 15, 2024

Replace private API encodeURI with public API SigV4_EncodeURI #97

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support presigned URL signature "UNSIGNED-PAYLOAD" and expose EncodeURI #89

Support presigned URL signature "UNSIGNED-PAYLOAD" and expose EncodeURI #89

giuspen commented Oct 5, 2023

moninom1 commented Oct 9, 2023

giuspen commented Oct 13, 2023

giuspen commented Nov 9, 2023

tony-josi-aws commented Nov 13, 2023

tony-josi-aws Nov 17, 2023

giuspen Nov 17, 2023

giuspen commented Nov 17, 2023

tony-josi-aws commented Nov 20, 2023

giuspen commented Nov 20, 2023

AniruddhaKanhere commented Nov 22, 2023 •

edited

Loading

giuspen commented Nov 22, 2023

AniruddhaKanhere commented Nov 22, 2023

giuspen commented Nov 22, 2023

AniruddhaKanhere commented Nov 24, 2023

giuspen commented Nov 24, 2023

giuspen commented Nov 24, 2023

AniruddhaKanhere commented Nov 24, 2023

giuspen commented Nov 24, 2023

giuspen commented Feb 8, 2024

tony-josi-aws commented Feb 19, 2024

giuspen commented Feb 19, 2024

giuspen commented Feb 19, 2024

giuspen commented Feb 21, 2024

tony-josi-aws commented Feb 21, 2024

giuspen commented Feb 21, 2024

Support presigned URL signature "UNSIGNED-PAYLOAD" and expose EncodeURI #89

Support presigned URL signature "UNSIGNED-PAYLOAD" and expose EncodeURI #89

Conversation

giuspen commented Oct 5, 2023

moninom1 commented Oct 9, 2023

giuspen commented Oct 13, 2023

giuspen commented Nov 9, 2023

tony-josi-aws commented Nov 13, 2023

tony-josi-aws Nov 17, 2023

Choose a reason for hiding this comment

giuspen Nov 17, 2023

Choose a reason for hiding this comment

giuspen commented Nov 17, 2023

tony-josi-aws commented Nov 20, 2023

giuspen commented Nov 20, 2023

AniruddhaKanhere commented Nov 22, 2023 • edited Loading

giuspen commented Nov 22, 2023

AniruddhaKanhere commented Nov 22, 2023

giuspen commented Nov 22, 2023

AniruddhaKanhere commented Nov 24, 2023

giuspen commented Nov 24, 2023

giuspen commented Nov 24, 2023

AniruddhaKanhere commented Nov 24, 2023

giuspen commented Nov 24, 2023

giuspen commented Feb 8, 2024

tony-josi-aws commented Feb 19, 2024

giuspen commented Feb 19, 2024

giuspen commented Feb 19, 2024

giuspen commented Feb 21, 2024

tony-josi-aws commented Feb 21, 2024

giuspen commented Feb 21, 2024

AniruddhaKanhere commented Nov 22, 2023 •

edited

Loading