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: consult x-%s-content-sha256 for payload hash #9804

Closed
wants to merge 5 commits into from

Conversation

cbodley
Copy link
Contributor

@cbodley cbodley commented Oct 26, 2022

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

@cbodley
Copy link
Contributor Author

cbodley commented Oct 26, 2022

will follow up with test cases if the approach looks good

@outscale-mgo
Copy link
Contributor

will follow up with test cases if the approach looks good

It look good to me :)

Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

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

Is this really a method a user would like to use? It seems a rather complicated way to get the checksum set for the content.

This method needs to get documented somewhere.

lib/http_aws_sigv4.c Outdated Show resolved Hide resolved
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);
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@cbodley
Copy link
Contributor Author

cbodley commented Oct 28, 2022

@bagder thanks for the review!

Is this really a method a user would like to use? It seems a rather complicated way to get the checksum set for the content.

ultimately this is for services that already require this header. i agree that it's not user-friendly, so would like for Curl_output_aws_sigv4() to add this header itself when service=s3. we're still discussing the best way to handle this part in #8935. once we decide how to add that header, we'll still need this logic to a) allow the user to pass in itheir own checksum, and b) to make sure that any header values we generate match what we put in the signature - that's why i chose to break this piece out into its own PR

This method needs to get documented somewhere.

sure, is CURLOPT_AWS_SIGV4 the right place to mention this?

@cbodley
Copy link
Contributor Author

cbodley commented Nov 2, 2022

test cases planned:

  • signature without X-Xxx-Content-Sha256 matches signature with X-Xxx-Content-Sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 (hash of empty buffer)
  • signature without X-Xxx-Content-Sha256 does not match signature with other values of X-Xxx-Content-Sha256
  • leading/trailing whitespace in header value doesn't effect signature
  • header value longer than 'char sha_hex[65]'

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

cbodley commented Nov 21, 2022

updated with new test cases:

  • 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()

test cases planned:

  • signature without X-Xxx-Content-Sha256 matches signature with X-Xxx-Content-Sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 (hash of empty buffer)

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>
bagder
bagder approved these changes Nov 25, 2022
@bagder
Copy link
Member

bagder commented Nov 25, 2022

Thanks!

@bagder bagder closed this in 7f8e6da Nov 25, 2022
bagder pushed a commit that referenced this pull request Nov 25, 2022
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants