Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the aws-sigv4 option to generate AWS SigV4 signed requests #1844

Closed
wants to merge 17 commits into from

Conversation

apparentorder
Copy link
Contributor

AWS SigV4 support is available in curl since 7.75.0 (December 2020).

Add the necessary bits for Hurl to understand this option and pass
it on to libcurl, both as a command line option --aws-sigv4 and
as a per-request option aws-sigv4 in Hurlfiles.

Do not emit Authorization: Basic when aws-sigv4 is used, as this
would take priority over the Authorization header generated by libcurl.
Instead, explicitly set username and password options.

Suppress removal of the Expect: header when using aws-sigv4, as a
workaround for curl/curl#11664.

Add a corresponding integration test and documentation.

@apparentorder
Copy link
Contributor Author

Regarding docs, I wasn't sure if I should commit the generated docs files too, because they showed several unrelated changes, e.g. replacing <picture>s in README.md with <img>s 🤷 – so I didn't commit those.

@fabricereix
Copy link
Collaborator

We usually update the doc later on.
There are various parts which are generated.
But this is good you have updated the man.

@apparentorder
Copy link
Contributor Author

apparentorder commented Aug 13, 2023

I'm not sure how to proceed here – apparently the libcurl on those debian, ubuntu and "generic" package tests does not support aws-sigv4 yet.

I could probably check beforehand if curl supports that option, but could we even make those integration tests behave differently for those platforms?

@fabricereix
Copy link
Collaborator

Detecting early on that the option is not supported, and printing a warning would be ideal.
For the integration test, we can always tweek the shell script integration/tests_ok/aws_sigv4.sh to make the test pass on these "old" platforms.
We will remove them once these platforms have been upgraded with more recent libcurl.

@apparentorder
Copy link
Contributor Author

Detecting early on that the option is not supported, and printing a warning would be ideal.

I wasn't sure how to deal with that – the libcurl error seems kinda appropriate. But I figured it might be good for the future to have a generic way to handle this case and added an HttpError entry for that, so we can return exactly which option failed and what the minimum libcurl version is to support that. Was that what you had in mind?

For the integration test, we can always tweek the shell script integration/tests_ok/aws_sigv4.sh to make the test pass on these "old" platforms.

That was kinda hacky to do :-) Let me know if you have a better idea how to handle that. But this seems to work now on my ubuntu 20.04 VM.

@fabricereix
Copy link
Collaborator

For the integration test, we could maybe check the request header value in the server. it would avoid using a possible "fake" output file.
It as a good idea to add a specific error for unknown curl option, that will probably deserve its own integ test.

@fabricereix
Copy link
Collaborator

/accept

@hurl-bot
Copy link
Collaborator

🕗 /accept is running, please wait for completion.

@hurl-bot
Copy link
Collaborator

❌ You have to rebase apparentorder/hurl/aws-sigv4 branch because there are new commits pending on target Orange-OpenSource/hurl/master branch (sorry i can't auto rebase from a fork):

This change allows option names to contain digits, like in `aws-sigv4`.
AWS SigV4 support is available in curl since 7.75.0 (December 2020).

Add the necessary bits for Hurl to understand this option and pass
it on to libcurl, both as a command line option `--aws-sigv4` and
as a per-request option `aws-sigv4` in Hurlfiles.

Do not emit `Authorization: Basic` when aws-sigv4 is used, as this
would take priority over the `Authorization` header generated by libcurl.
Instead, explicitly set `username` and `password` options.

Suppress removal of the `Expect:` header when using aws-sigv4, as a
workaround for curl/curl#11664.

Add a corresponding integration test.
@apparentorder
Copy link
Contributor Author

For the integration test, we could maybe check the request header value in the server. it would avoid using a possible "fake" output file.

I've seen that in other checks, but my intention was to see the actual/expected values in the test output, to easily see what went wrong. I can change it if you prefer.

As for the failing "run curl command" test – I have now simply removed the --aws-sigv4 from the .curl file. Hm...

Maybe having an option to disable some tests on some platforms might be more elegant in the long run.

@fabricereix
Copy link
Collaborator

/accept

@hurl-bot
Copy link
Collaborator

🕗 /accept is running, please wait for completion.

@hurl-bot
Copy link
Collaborator

❌ Some checks are still failing, please fix them before trying to merge this pull request.

@apparentorder
Copy link
Contributor Author

apparentorder commented Aug 25, 2023

Ok, the ubuntu test fails because that libcurl seems to behave differently than on other OS.

The test explicitly sets X-Amz-Date, so the signature result is stable. Normally, libcurl generates that header with the current date and time (signature changes every second).

This happens on ubuntu-latest – it adds two headers, one with the current time:

$ curl -v --aws-sigv4 aws:amz:eu-central-1:hurltest --header 'X-Amz-Date: 20230813T061344Z' [...]  2>&1 |grep Amz-Date
> X-Amz-Date: 20230825T114008Z
> X-Amz-Date: 20230813T061344Z

I guess I'll drop the idea of checking the exact signature and will have the test just check for a correctly-formatted Authorization header, so that there is no need to set X-Amz-Date.

@apparentorder
Copy link
Contributor Author

Heh, i knew someone would slip in another updated package. Updated.

@fabricereix
Copy link
Collaborator

/accept

@hurl-bot
Copy link
Collaborator

🕗 /accept is running, please wait for completion.

@hurl-bot
Copy link
Collaborator

✅ Pull request merged and closed by fabricereix with fast forward merge..

# List of commits merged from apparentorder/hurl/aws-sigv4 branch into Orange-OpenSource/hurl/master branch:

  • 7e476a4 Update crates (again)
  • 12f8bcd Update crates (again)
  • bbb8b48 check only for correct header format instead of exact signature value
  • e5a6a3a Update crates
  • dcbcab1 explicitly set user-agent, as it may be part of the signature
  • a1d7b5e re-enable 'set -Ee' and temp. disable it to capture error code
  • 2ca2c9a remove --aws-sigv4 option to fool integration tests on older systems
  • 1b7ae46 fix formatting
  • 5eae6ed adjust the aws-sigv4 test to fake correct output on machines with too old libcurl
  • 78a45f1 when setting aws_sigv4, explicitly check for and report "unknown option" from libcurl
  • c1063f8 introduce HttpError::LibcurlUnknownOption to report unsupported libcurl options
  • caa29f7 reformat to satisfy check
  • 82e5d01 reformat to satisfy "black" check
  • 70a64f5 reformat to satisfy check
  • a9b8a5a add documentation for aws-sigv4
  • 56d4fa4 Add the aws-sigv4 option to generate AWS SigV4 signed requests
  • 58c4e05 allow alphanumeric instead of alphabetic in option() parsing

@hurl-bot hurl-bot closed this Aug 26, 2023
@fabricereix
Copy link
Collaborator

@apparentorder
We usually force /accept when checks fails only for the crate update

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PR offer: Support for adding AWS SigV4 Authorization headers
3 participants