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: fall back to UNSIGNED-PAYLOAD for sign_as_s3 #9995

Closed
wants to merge 10 commits into from

Conversation

cbodley
Copy link
Contributor

@cbodley cbodley commented Nov 28, 2022

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
Copy link
Contributor Author

cbodley commented Nov 28, 2022

cc @outscale-mgo

lib/http_aws_sigv4.c Outdated Show resolved Hide resolved
@bagder
Copy link
Member

bagder commented Dec 1, 2022

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

Copy link
Contributor

@outscale-mgo outscale-mgo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

cbodley commented Dec 15, 2022

@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
Copy link
Contributor Author

cbodley commented Jan 11, 2023

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

@bagder
Copy link
Member

bagder commented Jan 12, 2023

CURLOPT_MIMEPOST is also a popular way to send POST data.

@cbodley
Copy link
Contributor Author

cbodley commented Feb 1, 2023

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 Outdated Show resolved Hide resolved
@cbodley
Copy link
Contributor Author

cbodley commented Feb 14, 2023

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

Comment on lines 362 to 367
head = Curl_slist_append_nodup(data->set.headers, header);
if(!head) {
free(header);
goto fail;
}
data->set.headers = head;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@cbodley
Copy link
Contributor Author

cbodley commented Feb 14, 2023

@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
Copy link
Contributor

outscale-mgo commented Feb 14, 2023

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

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

@cbodley 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
Copy link
Contributor

@outscale-mgo outscale-mgo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
lib/http_aws_sigv4.c Outdated Show resolved Hide resolved
lib/http_aws_sigv4.c Outdated Show resolved Hide resolved
lib/http_aws_sigv4.c Show resolved Hide resolved
lib/http_aws_sigv4.c Show resolved Hide resolved
lib/http_aws_sigv4.c Outdated Show resolved Hide resolved
lib/http_aws_sigv4.c Show resolved Hide resolved
lib/http_aws_sigv4.c Outdated Show resolved Hide resolved
@cbodley cbodley force-pushed the wip-sigv4-s3-unsigned branch 4 times, most recently from 3f052b9 to 33ff18d Compare February 15, 2023 17:47
@cbodley
Copy link
Contributor Author

cbodley commented Feb 15, 2023

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

@cbodley
Copy link
Contributor Author

cbodley commented Feb 17, 2023

@bagder any more feedback on your end?

@cbodley
Copy link
Contributor Author

cbodley commented Feb 17, 2023

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

Comment on lines 73 to 74
0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@cbodley cbodley Feb 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

@cbodley cbodley force-pushed the wip-sigv4-s3-unsigned branch 2 times, most recently from 78a3ef4 to ff2bc87 Compare February 28, 2023 21:50
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>
Signed-off-by: Casey Bodley <cbodley@redhat.com>
Signed-off-by: Casey Bodley <cbodley@redhat.com>
Signed-off-by: Casey Bodley <cbodley@redhat.com>
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
Copy link
Contributor Author

cbodley commented Mar 5, 2023

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

@cbodley
Copy link
Contributor Author

cbodley commented Mar 14, 2023

@bagder a friendly reminder; i believe this is ready

@bagder bagder closed this in 495d098 Mar 14, 2023
@bagder
Copy link
Member

bagder commented Mar 14, 2023

Thanks!

bch pushed a commit to bch/curl that referenced this pull request Jul 19, 2023
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
hjmallon added a commit to hjmallon/curl that referenced this pull request Nov 2, 2023
bagder pushed a commit that referenced this pull request Nov 5, 2023
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants