aws_sigv4: fall back to UNSIGNED-PAYLOAD for sign_as_s3 #9995

cbodley · 2022-11-28T16:57:13Z

all s3 requests default to UNSIGNED-PAYLOAD and add the required x-amz-content-sha256 header. this allows CURLAUTH_AWS_SIGV4 to correctly sign s3 requests to amazon with no additional configuration

adds a bool sign_as_s3 flag that can be extended with detection/configuration for other s3-compatible services (out of scope)

cbodley · 2022-11-28T17:03:47Z

cc @outscale-mgo

lib/http_aws_sigv4.c

bagder · 2022-12-01T16:34:59Z

@outscale-mgo can we get your comments on this PR?

outscale-mgo

I like how the bolean is use overall.
as we talk here #8935, what I'm not a big fan of is this condition

  sign_as_s3 = (strcasecompare(provider0, "aws") &&
                strcasecompare(service, "s3"));

first because it bind some code to s3, but TBH if we see this code as a first step for allowing other API to use sign_as_s3 it's not a big deal either.

what I find a little more annoying, is that s3 might require the hash calculated here to be set as x-%s-content-sha256. and by using aws:s3 as the condition to set UNSIGNED_PAYLOAD, we remove the future possibility to use curl to calculate the payload for x-%s-content-sha256.
UNSIGNED_PAYLOAD is the 'best' default value for s3 because it's the one that will work most of the time.
but, currently if a user want to have an hash in x-%s-content-sha256, they need to compute it, it's not ideal, especially as the hash they need to compute is already computed in curl code. so ideally we need to find a way for s3 user be able to tell curl to set this as x-%s-content-sha256, because even if it's not the header that will always work, it's the one the user might need the most (as it's the one with the most complex algorithm, so the one you want to avoid the most).
it's not exclusive to keep aws:s3 as condition for defaulting to UNSIGNED_PAYLOAD and then add another condition to use the calculated hash for the header, even if aws:s3 is set, but it might not be very intuitive either.

cbodley · 2022-12-15T22:29:23Z

@outscale-mgo thanks for the feedback!

i went with UNSIGNED-PAYLOAD for all s3 requests because i view the existing payload hashing as unreliable. it only works for certain requests, and it's been hard for me to specify exactly which those are. so i worried that getting this detection logic wrong would lead to signing bugs, where UNSIGNED-PAYLOAD would always work

but maybe it's not so hard to specify which requests can't be signed? the only non-empty payload we currently support is from CURLOPT_POSTFIELDS. @bagder are CURLOPT_READFUNCTION and CURLOPT_READDATA the only other ways to provide a request body? if so, then we would only need to use UNSIGNED-PAYLOAD for those 2 cases

cbodley · 2023-01-11T17:22:33Z

happy new year, @outscale-mgo @bagder. a friendly reminder about this one

bagder · 2023-01-12T13:21:48Z

CURLOPT_MIMEPOST is also a popular way to send POST data.

cbodley · 2023-02-01T23:11:48Z

CURLOPT_MIMEPOST is also a popular way to send POST data.

ok, thanks. it looks like the existing code will generate the wrong signature (using the checksum of an empty buffer) for these requests. if that mimepost data is all in memory, we should be able to calculate a real checksum

for now, i pushed a rebase/update that uses s3's UNSIGNED-PAYLOAD for all 3 cases (CURLOPT_MIMEPOST, CURLOPT_READDATA, CURLOPT_READFUNCTION), and otherwise falls back to the existing code

i still need to update the doc and add test cases with/without each of those CURLOPTs

lib/http_aws_sigv4.c

cbodley · 2023-02-14T00:18:04Z

rebased and updated to use only the request method and infilesize

test case 1960 checks that UPLOAD with INFILESIZE=0 calculates the checksum of an empty buffer

other test cases planned:

non-empty UPLOAD -> UNSIGNED-PAYLOAD
MIMEPOST -> UNSIGNED-PAYLOAD
POSTFIELDS -> real checksum
GET -> empty checksum

cbodley · 2023-02-14T01:19:41Z

lib/http_aws_sigv4.c

+  head = Curl_slist_append_nodup(data->set.headers, header);
+  if(!head) {
+    free(header);
+    goto fail;
+  }
+  data->set.headers = head;


the final test case 1964 doesn't add any headers to the request. in that case only, valgrind complains that this memory is leaked. is there a safe way to do this here?

below, Curl_output_aws_sigv4() adds a x-amz-date header at the end of auth_headers:

auth_headers = curl_maprintf("Authorization: %s4-HMAC-SHA256 " "Credential=%s/%s, " "SignedHeaders=%s, " "Signature=%s\r\n" "%s\r\n", provider0, user, credential_scope, Curl_dyn_ptr(&signed_headers), sha_hex, date_header);

is that the correct way to add x-amz-content-sha256?

i added a commit that does this. the leak is gone and the tests still pass

cbodley · 2023-02-14T13:50:11Z

@outscale-mgo would you please do another round of review?

this successfully interops with ceph's s3 server in its current form

s/sign_as_s3/default_unsigned_payload/g

i chose to stick with the former, since this flag controls both the use of UNSIGNED-PAYLOAD and the addition of a content-sha256 header. i assume that other s3-like services will want both

by using aws:s3 as the condition to set UNSIGNED_PAYLOAD, we remove the future possibility to use curl to calculate the payload for x-%s-content-sha256

it now calculates real checksums where it can. are you happy with the way that calc_s3_payload_hash() and calc_payload_hash() are composed here?

outscale-mgo · 2023-02-14T14:17:44Z

@outscale-mgo would you please do another round of review?

@cbodley on it, I try to give you feedback today, or tomorrow :)

outscale-mgo

overall I'm pretty happy with this PR.
I should be able to reuse sign_as_s3 variable to handle URI encoding here:
#7600.
something that I might have to rethink in my PR , is if I should let the user to have a possibility to speficy the number of encoding perself, or just force the URI encoding, depending of sign_as_s3, and lose the "no encoding" option. (or drop the PR overall and expect user to do the URI encoding perself, as it's currently the case.)

lib/http_aws_sigv4.c

cbodley · 2023-02-15T17:51:45Z

@outscale-mgo thanks for the great feedback. i believe i've addressed all of your comments

cbodley · 2023-02-17T16:18:24Z

@bagder any more feedback on your end?

cbodley · 2023-02-17T16:21:08Z

oops, i'm looking through the failed checks, and some of them are due to new test cases. i'll look into those

cbodley · 2023-02-17T16:35:37Z

tests/data/test1961

+0
+


the failing tests 1961 and 1965 are both missing the final two lines of this 'terminating chunk'. the tests succeed in my local build configured with --with-openssl --enable-debug

both libtests rely on a CURLOPT_READFUNCTION that does nothing but return 0. should they be doing something different?

i'm guessing that these test failures didn't see those expected lines of request body because the server <reply> didn't include the expected 100 Continue? i added that to test1961 and test1965 to see if it helps

@bagder the combination of a redirect and 100-continue seemed to be the issue there. does that final SQUASHME commit look sensible? if so i'll squash

otherwise i think the other failed checks are unrelated to this PR

Signed-off-by: Casey Bodley <cbodley@redhat.com>

if a content-sha256 header needs to be added, make_headers() will need to see it for inclusion in SignedHeaders Signed-off-by: Casey Bodley <cbodley@redhat.com>

Signed-off-by: Casey Bodley <cbodley@redhat.com>

6 test cases that set provider:service to aws:s3 and verify that the correct x-amz-content-sha256 header is added 1970: UPLOAD with empty body -> checksum of empty buffer 1971: UPLOAD with unknown body -> UNSIGNED-PAYLOAD 1972: MIMEPOST -> UNSIGNED-PAYLOAD 1973: POSTFIELDS -> checksum of postfields 1974: GET -> checksum of empty buffer 1975: UPLOAD with x-amz-content-sha-256 -> same signature as 1960 Signed-off-by: Casey Bodley <cbodley@redhat.com>

cbodley · 2023-03-05T16:16:07Z

a conflicting test1960 was merged, so i rebased and bumped the new test cases from the range 1960-1965 to 1970-1975

cbodley · 2023-03-14T15:51:29Z

@bagder a friendly reminder; i believe this is ready

bagder · 2023-03-14T16:05:56Z

Thanks!

all s3 requests default to UNSIGNED-PAYLOAD and add the required x-amz-content-sha256 header. this allows CURLAUTH_AWS_SIGV4 to correctly sign s3 requests to amazon with no additional configuration Signed-off-by: Casey Bodley <cbodley@redhat.com> Closes curl#9995

* Fixed in curl#9995

* Remove other mention of hyper memory-leaks from `KNOWN_BUGS`. Should have been removed in 629723e * Remove mention of aws-sigv4 sort query string from `KNOWN_BUGS`. Fixed in #11806 * Remove mention of aws-sigv4 query empty value problems * Remove mention of aws-sigv4 missing amz-content-sha256 Fixed in #9995

This was referenced Nov 28, 2022

--aws-sigv4 is missing the amz-content-sha256 header? #8810

Closed

aws_sigv4: Include and sign X-Amz-Content-Sha256. #8935

Closed

bagder reviewed Nov 29, 2022

View reviewed changes

lib/http_aws_sigv4.c Outdated Show resolved Hide resolved

bagder added the authentication label Nov 29, 2022

cbodley force-pushed the wip-sigv4-s3-unsigned branch from 393c5e4 to 5639089 Compare November 29, 2022 15:26

outscale-mgo reviewed Dec 6, 2022

View reviewed changes

cbodley force-pushed the wip-sigv4-s3-unsigned branch from 5639089 to 3784fbd Compare February 1, 2023 23:06

cbodley force-pushed the wip-sigv4-s3-unsigned branch from 3784fbd to 0b67068 Compare February 1, 2023 23:25

cbodley commented Feb 3, 2023

View reviewed changes

lib/http_aws_sigv4.c Outdated Show resolved Hide resolved

cbodley force-pushed the wip-sigv4-s3-unsigned branch from 0b67068 to 583d50c Compare February 14, 2023 00:13

cbodley force-pushed the wip-sigv4-s3-unsigned branch from 583d50c to a093b47 Compare February 14, 2023 01:14

cbodley commented Feb 14, 2023

View reviewed changes

cbodley force-pushed the wip-sigv4-s3-unsigned branch from a093b47 to fba4a55 Compare February 14, 2023 04:02

cbodley changed the title ~~aws_sigv4: default to UNSIGNED-PAYLOAD for sign_as_s3~~ aws_sigv4: fall back to UNSIGNED-PAYLOAD for sign_as_s3 Feb 14, 2023

outscale-mgo reviewed Feb 15, 2023

View reviewed changes

cbodley force-pushed the wip-sigv4-s3-unsigned branch 4 times, most recently from 3f052b9 to 33ff18d Compare February 15, 2023 17:47

cbodley commented Feb 17, 2023

View reviewed changes

cbodley force-pushed the wip-sigv4-s3-unsigned branch 2 times, most recently from 78a3ef4 to ff2bc87 Compare February 28, 2023 21:50

cbodley added 10 commits March 5, 2023 11:09

aws_sigv4: use defines for sha256 buffer sizes

5fe55a6

Signed-off-by: Casey Bodley <cbodley@redhat.com>

aws_sigv4: calculate payload hash before make_headers()

83885f8

if a content-sha256 header needs to be added, make_headers() will need to see it for inclusion in SignedHeaders Signed-off-by: Casey Bodley <cbodley@redhat.com>

aws_sigv4: move hash calculation to calc_payload_hash()

d48f55e

Signed-off-by: Casey Bodley <cbodley@redhat.com>

aws_sigv4: add service detection for sign_as_s3

3fec832

Signed-off-by: Casey Bodley <cbodley@redhat.com>

aws_sigv4: call Curl_http_method() earlier

e9187b5

Signed-off-by: Casey Bodley <cbodley@redhat.com>

aws_sigv4: add required x-%s-content-sha256 header to s3 requests

62c39d6

Signed-off-by: Casey Bodley <cbodley@redhat.com>

aws_sigv4: use UNSIGNED-PAYLOAD for s3 requests with unknown payload

7c68c16

Signed-off-by: Casey Bodley <cbodley@redhat.com>

doc: CURLOPT_AWS_SIGV4 describes special behavior for aws:s3

95c041c

Signed-off-by: Casey Bodley <cbodley@redhat.com>

SQUASHME: try adding 100 Continue to server responses

e8fa9cd

cbodley force-pushed the wip-sigv4-s3-unsigned branch from ff2bc87 to e8fa9cd Compare March 5, 2023 16:15

bagder closed this in 495d098 Mar 14, 2023

hjmallon added a commit to hjmallon/curl that referenced this pull request Nov 2, 2023

docs: Remove mention of aws-sigv4 missing amz-content-sha256

7654261

* Fixed in curl#9995

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

aws_sigv4: fall back to UNSIGNED-PAYLOAD for sign_as_s3 #9995

aws_sigv4: fall back to UNSIGNED-PAYLOAD for sign_as_s3 #9995

cbodley commented Nov 28, 2022

cbodley commented Nov 28, 2022

bagder commented Dec 1, 2022

outscale-mgo left a comment

cbodley commented Dec 15, 2022

cbodley commented Jan 11, 2023

bagder commented Jan 12, 2023

cbodley commented Feb 1, 2023

cbodley commented Feb 14, 2023 •

edited

Loading

cbodley Feb 14, 2023

cbodley Feb 14, 2023

cbodley commented Feb 14, 2023

outscale-mgo commented Feb 14, 2023 •

edited

Loading

outscale-mgo left a comment •

edited

Loading

cbodley commented Feb 15, 2023

cbodley commented Feb 17, 2023

cbodley commented Feb 17, 2023

cbodley Feb 17, 2023

cbodley Feb 28, 2023 •

edited

Loading

cbodley Mar 1, 2023

cbodley commented Mar 5, 2023

cbodley commented Mar 14, 2023

bagder commented Mar 14, 2023

aws_sigv4: fall back to UNSIGNED-PAYLOAD for sign_as_s3 #9995

aws_sigv4: fall back to UNSIGNED-PAYLOAD for sign_as_s3 #9995

Conversation

cbodley commented Nov 28, 2022

cbodley commented Nov 28, 2022

bagder commented Dec 1, 2022

outscale-mgo left a comment

Choose a reason for hiding this comment

cbodley commented Dec 15, 2022

cbodley commented Jan 11, 2023

bagder commented Jan 12, 2023

cbodley commented Feb 1, 2023

cbodley commented Feb 14, 2023 • edited Loading

cbodley Feb 14, 2023

Choose a reason for hiding this comment

cbodley Feb 14, 2023

Choose a reason for hiding this comment

cbodley commented Feb 14, 2023

outscale-mgo commented Feb 14, 2023 • edited Loading

outscale-mgo left a comment • edited Loading

Choose a reason for hiding this comment

cbodley commented Feb 15, 2023

cbodley commented Feb 17, 2023

cbodley commented Feb 17, 2023

cbodley Feb 17, 2023

Choose a reason for hiding this comment

cbodley Feb 28, 2023 • edited Loading

Choose a reason for hiding this comment

cbodley Mar 1, 2023

Choose a reason for hiding this comment

cbodley commented Mar 5, 2023

cbodley commented Mar 14, 2023

bagder commented Mar 14, 2023

cbodley commented Feb 14, 2023 •

edited

Loading

outscale-mgo commented Feb 14, 2023 •

edited

Loading

outscale-mgo left a comment •

edited

Loading

cbodley Feb 28, 2023 •

edited

Loading