aws_sigv4: fix headers computation for some unhandled cases. #7966

outscale-mgo · 2021-11-05T15:03:53Z

Up to this PR, canonical header, and signed header was hardcoded
in the aws-sigv4 signature.

Those patch handle canonical headers and signed headers creation
as it is explain here:
https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html

The algorithm tell that signed and canonical must contain at last host and x-amz-date.

So we check whatever they are present in the curl http headers list.
If they are, we use the one enter by curl user, otherwise we generate them.
then we to lower, and remove space from each http headers plus host and x-amz-date,
then sort them all by alphabetical order.

This patch also fix a bug with host header, which was ignoring the port.

Because I now handle the port in the signature,
I had to hardcode the host in the tests, in which the port was random,
which was making impossible to had a signature in the tests data.

bagder · 2021-11-05T22:50:53Z

lib/http_aws_sigv4.c

+  char *date_full_hdr = curl_maprintf("x-%s-date:%s",
+                                      provider1_low, timestamp);
+#ifdef DEBUGBUILD
+  char *force_host = getenv("CURL_FORCETIME");


Why is the environment variable to force host called force time ?

lib/http_aws_sigv4.c

bagder · 2021-11-05T22:51:56Z

lib/http_aws_sigv4.c

+#endif
+  else {
+    char *full_host = data->state.aptr.host ? data->state.aptr.host :
+      curl_maprintf("host:%s", hostname);


No space after the colon? And what if curl_maprintf() returns NULL?

In what situations is data->state.aptr.host NULL when this is called? What then if there's a custom port number or an IPv6 address host name?

I really don't know if not having data->state.aptr.host is possible, I've made this ternary out of fear of having full_host NULL, which would be a bug.
Should I remove the condition ?

It is probably wise to keep it.

CanonicalHeaders must contain Lowercase(HeaderName) + ':' + Trimall(HeaderValue) + '\n'.
so no space before and after the Value.
I don't see why IPv6 wouldn't work with the current code ?
I guess I could try to add manually the port, if the ip is set using conn->host.name, though most case where a port is needed should be handle using data->state.aptr.host.
My guess is that this would be too much code for handling a too niche corner case, maybe add a comment saying we don't handle port if host is set using conn->host.name would be enough ?

Please note that data->state.aptr.host consists of \r\n on its end. You have to trim them in order not to create extra empty lines in CanonicalHeaders.

lib/http_aws_sigv4.c

bagder · 2021-11-05T22:56:28Z

lib/http_aws_sigv4.c

+  }
+
+  if(!Curl_checkheaders(data, date_hdr_key))
+    curl_slist_append(head, date_full_hdr);


No return code check for a function that is fallible.

bagder · 2021-11-05T23:02:51Z

Why does it have to sort the headers?

I had to hardcode the host in the tests, in which the port was random, which was making impossible to had a signature in the tests data.

I think that's fine. Another way would be to replace the port number part only with a fixed number for the test run.

outscale-mgo · 2021-12-09T13:15:43Z

Why does it have to sort the headers?

the "header" have to be sort in the canonical header and the signed header.
but if I'm correct, I don't change the existing headers order in data->set.headers

outscale-mgo · 2021-12-16T15:40:36Z

note that I still need to converts sequential spaces to a single space, in order to be conform to the algorithm.

outscale-mgo · 2021-12-21T10:07:10Z

should be done now

matwey · 2021-12-31T13:03:40Z

lib/http_aws_sigv4.c

+  for(l = head; l; l = l->next) {
+    char *tmp = l->data, *tmp2;
+
+    Curl_strntolower(l->data, l->data, strlen(l->data));


Sorry to bother.

Here you lowercase the whole header while the specs require you to lowercase only the header name:

https://docs.aws.amazon.com/AmazonS3/latest/API/sig-v4-header-based-auth.html

Thanks, now the headers are lower-cased properly.

Thanks for the feedback :)

lib/http_aws_sigv4.c

matwey · 2021-12-31T16:02:09Z

lib/http_aws_sigv4.c

+          *tmp++ = ' '; /* if tab convert to space */
+          if (*tmp == ' ' || *tmp == '\t')
+            memmove(tmp, tmp2, strlen(tmp2));
+        }


I may be wrong here, but the spec says:

Trim() Remove any leading or trailing white-space.

and this code seems to do extra work on intermediate spaces. Do you know any valid examples with space-containing header values?

Please note, that trailing \r\n (from data->state.aptr.host) also could be trimmed here.

yes, your right, I'm presently working on fixing this, thanks for the feedback.

The Host: bug seems to be fixed now.

matwey · 2022-01-08T10:40:55Z

@outscale-mgo sorry, it seems that this PR should be rebased onto current master, now it is conflicting.

outscale-mgo · 2022-01-11T16:27:23Z

lib/http_aws_sigv4.c

+  }
+
+  for(l = head; l; l = l->next) {
+    char *tmp = *canonical_headers;


note, that I'm presently refactoring this code to use dynbuf instead of curl_maprintf for this chunk.

matwey · 2022-01-11T17:11:06Z

lib/http_aws_sigv4.c

+    head = NULL;
+  }
+#ifdef DEBUGBUILD
+  else if(force_host) {


Just for information. I am not insisting on anything.

Take a look how I overcame the random port issue using CONNECT_TO feature: fc29695
I think this could have advantage that the real code path is tested in this case.

Thanks, I've use your code, to replace my tests. (also keep you as the author of the test commit)

tests/data/test1937

bagder · 2022-09-28T10:57:36Z

lib/http_aws_sigv4.c

+    store = value;
+
+    /* skip leading whitespace */
+    while(*value && ISSPACE(*value))


May I suggest using ISBLANK() instead? Or do you actually think this needs to also skip control codes and more?

bagder · 2022-09-28T10:57:44Z

lib/http_aws_sigv4.c

+
+    while(*value) {
+      int space = 0;
+      while(*value && ISSPACE(*value)) {


bagder · 2022-09-28T11:04:07Z

lib/http_aws_sigv4.c

+    if(data->state.aptr.host) {
+      size_t pos;
+
+      strncpy(full_host, data->state.aptr.host, FULL_HOST_LEN);


This is arbitrarily cutting off the header, which then will make it fail. Shouldn't it rather return an error if the existing header is too long?

bagder · 2022-09-28T11:04:54Z

lib/http_aws_sigv4.c

+    }
+    else {
+      msnprintf(full_host, FULL_HOST_LEN, "host:%s", hostname);
+      full_host[FULL_HOST_LEN - 1] = 0;


msnprintf() null-terminates the string, no need to do it again.

lib/http_aws_sigv4.c

bagder · 2022-09-28T11:13:33Z

also: "This branch has conflicts that must be resolved"

Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com>

Up to this patch, canonical header, and signed header was hardcoded in the aws-sigv4 signature. This patch handle canonical headers and signed headers creation as it is explain here: https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html The algo tell that signed and canonical must contain at last host and x-amz-date. So we check whatever thoses are present in the curl http headers list. If they are, we use the one enter by curl user, otherwise we generate them. then we to lower, and remove space from each http headers plus host and x-amz-date, then sort them all by alphabetical order. This patch also fix a bug with host header, which was ignoring the port. Because I now handle the port in the signature, I had to hardcode the host in the tests, in which the port was random, which was making imposible to had a signature in the tests data. Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com>

Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com>

bagder

I think is is ready for merge. I could only find one nit, and that's in a comment.

bagder · 2022-10-07T12:20:09Z

lib/http_aws_sigv4.c

+/* string been x-PROVIDER-date:TIMESTAMP, I need +1 for ':' */
+#define DATE_FULL_HDR_LEN (DATE_HDR_KEY_LEN + TIMESTAMP_SIZE + 1)
+
+/* strlen(timestamp) need to be inferior to TIMESTAMP_SIZE */


I propose:

/* sizeof(timestamp) needs to be at least TIMESTAMP_SIZE bytes */

maybe it's my english lacking here, but doesn't "need to be at last", mean that strlen(timestamp) must be 17 or more ? which would mean an overflow ?
I've just patch the comment to say:

strlen(timestamp) should be 16, and having it bigger than TIMESTAMP_SIZE could cause an overflow

it's more tedious, but more precise I guess.

strlen() is the length of a string, not the size of a buffer so that is just plain wrong to describe how large a buffer is. Also, the word "inferior" does not mean shorter or smaller, it means "worse" or "less good", so it makes no sense there.

My version of the comment describes how large the buffer needs to be. It needs to be at least 17 bytes. Right?

yes, but no more.

I've change it to that

/* * timestamp should point to a buffer of at last TIMESTAMP_SIZE bytes, * if it point to a buffer bigger than TIMESTAMP_SIZE bytes * code could overflow */

Explain this. How would it "overflow" if the buffer is larger than that? The code assumes 17 bytes, it does not touch the bytes beyond that does it?

Ok, overflow was wrong sorry (I thought I use strcpy, but I use n functions),

msnprintf(date_full_hdr, DATE_FULL_HDR_LEN, "x-%s-date:%s", provider1, timestamp);

If timestamp is too big here, the signature will fail.

on the other hand timestamp been set like this:

if(!strftime(timestamp, sizeof(timestamp), "%Y%m%dT%H%M%SZ", &tm)) {

I don't think it can be something else than 17.
But if the code calling make_headers is modify, then timestamp could be too large for the buffer in which it is copy.

If timestamp is too big here, the signature will fail.

Yes, but the comment speaks of the buffer size for timestamp, now you are talking about the string put into that buffer. They are not the same.

if(!strftime(timestamp, sizeof(timestamp), "%Y%m%dT%H%M%SZ", &tm)) {

Right, and this code makes sure it doesn't overwrite the buffer. It still works perfectly fine with a larger buffer.

I claim your updated comment is wrong. The buffer needs to be at least TIMESTAMP_SIZE bytes big. If it is bigger, the code still works.

ok I've update the comment, thanks for the review.

Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com>

bagder · 2022-10-11T12:34:58Z

Thanks. I did squash this into a single commit before merging.

Handle canonical headers and signed headers creation as explained here: https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html The algo tells that signed and canonical must contain at last host and x-amz-date. So we check whatever thoses are present in the curl http headers list. If they are, we use the one enter by curl user, otherwise we generate them. then we to lower, and remove space from each http headers plus host and x-amz-date, then sort them all by alphabetical order. This patch also fix a bug with host header, which was ignoring the port. Closes curl#7966

bagder reviewed Nov 5, 2021

View reviewed changes

outscale-mgo changed the title ~~Aws sigv4 headers~~ Aws sigv4 headers refacto Nov 8, 2021

outscale-mgo force-pushed the aws-sigv4-headers-rebased-master branch 2 times, most recently from d5cbac7 to a12cd0f Compare November 8, 2021 18:35

bagder added authentication HTTP labels Nov 11, 2021

outscale-mgo force-pushed the aws-sigv4-headers-rebased-master branch 7 times, most recently from 35950a2 to 510b082 Compare November 15, 2021 18:32

outscale-mgo force-pushed the aws-sigv4-headers-rebased-master branch from 510b082 to 72d8a3c Compare December 17, 2021 15:53

outscale-mgo mentioned this pull request Dec 31, 2021

aws_sigv4: Fix "host:" canonical header #8202

Closed

outscale-mgo requested a review from bagder December 31, 2021 10:24

matwey reviewed Dec 31, 2021

View reviewed changes

lib/http_aws_sigv4.c Show resolved Hide resolved

matwey mentioned this pull request Dec 31, 2021

http_aws_sigv4: Improve payload hashing outscale/curl#1

Open

matwey reviewed Dec 31, 2021

View reviewed changes

outscale-mgo force-pushed the aws-sigv4-headers-rebased-master branch from 72d8a3c to b595a41 Compare January 6, 2022 09:57

outscale-mgo force-pushed the aws-sigv4-headers-rebased-master branch from b595a41 to 3ebc602 Compare January 8, 2022 12:17

outscale-mgo commented Jan 11, 2022

View reviewed changes

matwey reviewed Jan 11, 2022

View reviewed changes

outscale-mgo force-pushed the aws-sigv4-headers-rebased-master branch from 3ebc602 to afdfd05 Compare January 13, 2022 14:01

outscale-mgo requested a review from bagder September 27, 2022 12:55

bagder reviewed Sep 28, 2022

View reviewed changes

matwey and others added 5 commits October 3, 2022 17:14

tests: use connect to in test

34238ec

Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com>

aws-sigv4: test header refacto

ce31720

Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com>

aws-sigv4: refactor

909e491

aws-sigv4: use more static allocation

19ab809

outscale-mgo force-pushed the aws-sigv4-headers-rebased-master branch from bde238e to ad0ee7c Compare October 3, 2022 15:19

outscale-mgo added 6 commits October 4, 2022 15:04

aws-sigv4: remove another malloc

957e36a

aws-sigv4: use do while instead of for, and initialize var to NULL

4fce073

Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com>

aws-sigv4: fix comparaison always true

9b11f8c

Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com>

aws-sigv4: try to fix read overflow

e986a31

aws_sigv4: use ISBLANK instead of ISSPACE

405471f

Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com>

aws-sigv4: check if host is longer than a FQDN

cba343c

Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com>

outscale-mgo force-pushed the aws-sigv4-headers-rebased-master branch from ad0ee7c to efecafd Compare October 4, 2022 14:27

outscale-mgo requested a review from bagder October 4, 2022 17:59

bagder reviewed Oct 7, 2022

View reviewed changes

outscale-mgo force-pushed the aws-sigv4-headers-rebased-master branch 3 times, most recently from d513f31 to d11b05f Compare October 10, 2022 13:31

outscale-mgo added 2 commits October 10, 2022 15:49

aws_sigv4: timestamp comment

cf09e58

Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com>

aws-sigv4: use exam.ple.com instead of 127.0.0.1 in test

96eb43c

Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com>

outscale-mgo force-pushed the aws-sigv4-headers-rebased-master branch from d11b05f to 96eb43c Compare October 10, 2022 13:49

bagder approved these changes Oct 11, 2022

View reviewed changes

bagder closed this in 29c4aa0 Oct 11, 2022

bagder mentioned this pull request Oct 13, 2022

--aws-sigv4 doesn't sort query string parameters before signing #9717

Closed

cbodley mentioned this pull request Oct 26, 2022

http: Curl_output_aws_sigv4 compatibility fixes #9588

Closed

aws_sigv4: fix headers computation for some unhandled cases. #7966

aws_sigv4: fix headers computation for some unhandled cases. #7966

Conversation

outscale-mgo commented Nov 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

outscale-mgo Nov 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

outscale-mgo Nov 22, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bagder commented Nov 5, 2021

outscale-mgo commented Dec 9, 2021

outscale-mgo commented Dec 16, 2021

outscale-mgo commented Dec 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matwey commented Jan 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bagder commented Sep 28, 2022

bagder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

outscale-mgo Oct 10, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bagder Oct 10, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bagder commented Oct 11, 2022

outscale-mgo Nov 15, 2021 •

edited

outscale-mgo Nov 22, 2021 •

edited

outscale-mgo Oct 10, 2022 •

edited

bagder Oct 10, 2022 •

edited