aws_sigv4: consult x-%s-content-sha256 for payload hash #9804
Conversation
|
will follow up with test cases if the approach looks good |
It look good to me :) |
lib/http_aws_sigv4.c
Outdated
| method, | ||
| data->state.up.path, | ||
| data->state.up.query ? data->state.up.query : "", | ||
| Curl_dyn_ptr(&canonical_headers), | ||
| Curl_dyn_ptr(&signed_headers), | ||
| sha_hex); | ||
| payload_hash_len, payload_hash); |
There was a problem hiding this comment.
the length should be an int according to how *printf() works, but I think using the %.*s construct seems unnecessary here. Can't you just rather do %s and payload_hash_len ? payload_hash: "" as argument?
There was a problem hiding this comment.
Can't you just rather do %s and payload_hash_len ? payload_hash: "" as argument?
parse_content_sha_hdr() trims any trailing whitespace from the header value, so may return a shorter length
payload_hash points to memory returned by Curl_checkheaders(), so i used the explicit payload_hash_len here to avoid mutating that header to move the null terminator
i considered copying this header value into a different buffer to change the null. but that required the choice of a buffer size, so added an extra error path when the user provides a header value that's longer
i found this solution to be the simplest, but would welcome your guidance here
the length should be an int according to how *printf() works
ok, thanks
|
@bagder thanks for the review!
ultimately this is for services that already require this header. i agree that it's not user-friendly, so would like for
sure, is CURLOPT_AWS_SIGV4 the right place to mention this? |
|
test cases planned:
|
`Curl_output_aws_sigv4()` doesn't always have the whole payload in memory to generate a real payload hash. this commit allows the user to pass in a header like `x-amz-content-sha256` to provide their desired payload hash some services like s3 require this header, and may support other values like s3's `UNSIGNED-PAYLOAD` and `STREAMING-AWS4-HMAC-SHA256-PAYLOAD` with special semantics. servers use this header's value as the payload hash during signature validation, so it must match what the client uses to generate the signature Signed-off-by: Casey Bodley <cbodley@redhat.com>
the request path is part of the v4 signature. using the same path for all aws_sigv4 test cases allows us to compare signatures between them Signed-off-by: Casey Bodley <cbodley@redhat.com>
1956 adds the sha256 value corresponding to an empty buffer 1957 adds an arbitrary value and confirms that the signature differs from 1956 1958 adds whitespace to 1957 and confirms that the signature matches 1957 1959 adds a value longer than 'char sha_hex[65]' in Curl_output_aws_sigv4() Signed-off-by: Casey Bodley <cbodley@redhat.com>
cbodley
force-pushed
the
wip-sigv4-content-sha256
branch
from
7885a4a
to
ce7835e
Compare
|
updated with new test cases:
this part didn't quite work out, because the presence/absence of x-xxx-content-sha256 in SignedHeaders changes the signature |
Signed-off-by: Casey Bodley <cbodley@redhat.com>
Signed-off-by: Casey Bodley <cbodley@redhat.com>
cbodley
force-pushed
the
wip-sigv4-content-sha256
branch
from
509f140
to
0c85c84
Compare
|
Thanks! |
1956 adds the sha256 value corresponding to an empty buffer 1957 adds an arbitrary value and confirms that the signature differs from 1956 1958 adds whitespace to 1957 and confirms that the signature matches 1957 1959 adds a value longer than 'char sha_hex[65]' in Curl_output_aws_sigv4() Signed-off-by: Casey Bodley <cbodley@redhat.com> Closes #9804
Curl_output_aws_sigv4()doesn't always have the whole payload in memory to generate a real payload hash. this commit allows the user to pass in a header likex-amz-content-sha256to provide their desired payload hashsome services like s3 require this header, and may support other values like s3's
UNSIGNED-PAYLOADandSTREAMING-AWS4-HMAC-SHA256-PAYLOADwith special semantics. servers use this header's value as the payload hash during signature validation, so it must match what the client uses to generate the signature