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

aws-sigv4 signature covers non-existent Expect: header #11664

Closed
apparentorder opened this issue Aug 12, 2023 · 5 comments
Closed

aws-sigv4 signature covers non-existent Expect: header #11664

apparentorder opened this issue Aug 12, 2023 · 5 comments

Comments

@apparentorder
Copy link

apparentorder commented Aug 12, 2023

I did this

curl -v
 --header 'expect:'
 --header 'Content-Type: application/json'
 --data '[ ... foo ... ]'
 --aws-sigv4 aws:amz:eu-central-1:appsync
 --user 'akid:secret'
 'https://xxx.appsync-api.eu-central-1.amazonaws.com/graphql'

I expected the following

Correct AWS SigV4, i.e. expect is not part of curl's generated SignedHeaders, as it is not part of the emitted request headers, like this:

authorization: AWS4-HMAC-SHA256
 Credential=xxx/20230812/eu-central-1/appsync/aws4_request,
 SignedHeaders=content-type;host;x-amz-date,
 Signature=xxx

But what actually happens is this:

authorization: AWS4-HMAC-SHA256
 Credential=xxx/20230812/eu-central-1/appsync/aws4_request,
 SignedHeaders=content-type;expect;host;x-amz-date,
 Signature=xxx

Per man page, using --header 'expect:' should remove that header. The header is indeed not present, but it seems that by forcing the removal of this non-existent header, it somehow gets added to the list of headers to be considered for aws-sigv4.

Additional context:

Some AWS APIs just ignore this (e.g. sts), but others report failure, e.g. Appsync GraphQL with IAM Auth:

  "errors" : [ {
    "errorType" : "IncompleteSignatureException",
    "message" : "'expect' is named as a SignedHeader, but it does not exist in the HTTP request."
  } ]

This seems to happen in libcurl – I've discovered it while playing around with Hurl, which explicitly sets Expect:.

curl/libcurl version

curl 8.0.1 (aarch64-amazon-linux-gnu) libcurl/8.0.1 OpenSSL/3.0.8 zlib/1.2.11 libidn2/2.3.2 nghttp2/1.51.0

operating system

Amazon Linux 2023

@jay
Copy link
Member

jay commented Aug 12, 2023

Try #11668

apparentorder added a commit to apparentorder/hurl that referenced this issue Aug 13, 2023
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 added a commit to apparentorder/hurl that referenced this issue Aug 13, 2023
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
Author

Just tested with 370aabb3ef0dc4fe1bcedd52ce706f4fceed3cea and it works now. Thanks for the quick response!

@jay
Copy link
Member

jay commented Aug 14, 2023

With that commit if you send a no-value header to AWS like -H "foo;" (note the semi-colon which causes libcurl to translate it to 'foo:') and it sends "foo:" does AWS show you any signature error for that?

@apparentorder
Copy link
Author

Note that different AWS endpoints seem to use different implementations (see initial report: sts for example ignores missing Expect header).

Testing against an Appsync GraphQL endpoint:

  • Using expect; causes this request header to be present and empty, and part of SignedHeaders, but the API complains just the same: 'expect' is named as a SignedHeader, but it does not exist in the HTTP request.
  • Using foo; causes the request header to be present and empty, and part of SignedHeaders, but the API responds successfully

Testing against STS:

  • Using expect; causes this request header to be present and empty, and part of SignedHeaders, and STS fails with 417 Expectation Failed
  • Using foo; causes the request header to be present and empty, and part of SignedHeaders, and the API responds successfully

By the way, using "Expect; " (with a trailing space) makes curl ignore that argument completely, i.e. no header is emitted. Should behave the same as "without space", or cause an error, I'd say.

@jay
Copy link
Member

jay commented Aug 18, 2023

Thanks for testing. I tried to get the behavior as close as possible to the way http.c handles these headers. We want SignedHeaders to only include what is actually being sent.

'expect' is named as a SignedHeader, but it does not exist in the HTTP request.

Since the no-value Expect: header actually is sent in the HTTP request, from a libcurl perspective this is ok. The server error message is misleading.

417 Expectation Failed

Yes that is expected since any 'expect' header value other than 100-continue is undefined.

By the way, using "Expect; " (with a trailing space) makes curl ignore that argument completely, i.e. no header is emitted. Should behave the same as "without space", or cause an error, I'd say.

I forget whether that's a bug or intended. I'll take a look.

apparentorder added a commit to apparentorder/hurl that referenced this issue Aug 24, 2023
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.
jay added a commit to jay/curl that referenced this issue Sep 11, 2023
- Handle user headers in format 'name:' and 'name;' with no value.

The former is used when the user wants to remove an internal libcurl
header and the latter is used when the user actually wants to send a
no-value header in the format 'name:' (note the semi-colon is converted
by libcurl to a colon).

Prior to this change the AWS header import code did not special case
either of those and the generated AWS SignedHeaders would be incorrect.

Reported-by: apparentorder@users.noreply.github.com

Fixes curl#11664
Closes #xxxx
@jay jay closed this as completed in b5c65f8 Sep 11, 2023
ptitSeb pushed a commit to wasix-org/curl that referenced this issue Sep 25, 2023
- Handle user headers in format 'name:' and 'name;' with no value.

The former is used when the user wants to remove an internal libcurl
header and the latter is used when the user actually wants to send a
no-value header in the format 'name:' (note the semi-colon is converted
by libcurl to a colon).

Prior to this change the AWS header import code did not special case
either of those and the generated AWS SignedHeaders would be incorrect.

Reported-by: apparentorder@users.noreply.github.com

Ref: https://curl.se/docs/manpage.html#-H

Fixes curl#11664
Closes curl#11668
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

2 participants