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

http_aws_sigv4: Improve payload hashing #1

Open
wants to merge 4 commits into
base: aws-sigv4-headers-rebased-master
Choose a base branch
from

Conversation

matwey
Copy link

@matwey matwey commented Dec 31, 2021

Hi,

I think the following could be useful for your PR curl#7966. Unfortunately, AWS SIGv4 implementation in curl is far from ideal currently.

Introduce X-Provider-Content-Sha256 header as it is required by the specs [1]
and we anyway need to know its value to calculate the CanonicalRequest.

Additionally:

  1. Allow the user to provide precomputed SHA256 of the payload (when it is known in
    advance)

  2. Fallback to "Unsigned payload" when postdata is not available instead of
    providing the hash for empty payload and having the wrong signature
    verification. postdata is only (can be) available for POST method and may be
    missed for non-empty payload.

    For instance, the following valid use-case will lead to PUT request with
    empty postdata:

    curl ... --aws-sigv4 "aws:amz" -u accesskey:secretkey -T file_to_upload.dat http://endpoint
    

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

Signed-off-by: Matwey V. Kornilov matwey.kornilov@gmail.com

@outscale-mgo
Copy link

outscale-mgo commented Jan 13, 2022

First thanks you for tackling X-Provider-Content-Sha256 problem, which for now still need to be manually computed.

Introduce X-Provider-Content-Sha256 header as it is required by the specs [1]

kind of, X-Provider-Content-Sha256 is require by s3 v4 signature spec, but not for other API sure as EC2 for instance.
So adding this header by force, seems dangerous, as you force to add this header on some API that doesn't require it.
So I guess we need an option to optionally add X-Provider-Content-Sha256 header.

@outscale-mgo outscale-mgo force-pushed the aws-sigv4-headers-rebased-master branch from 3ebc602 to afdfd05 Compare January 13, 2022 14:01
Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com>
@matwey
Copy link
Author

matwey commented Jan 13, 2022

Thanks. Now I see, you are right.

@matwey
Copy link
Author

matwey commented Jan 13, 2022

Put here for reference: minio/minio#5340

@matwey matwey force-pushed the fix/sigv4 branch 2 times, most recently from f1d8bf9 to 1dc64b4 Compare January 14, 2022 19:26
@matwey
Copy link
Author

matwey commented Jan 14, 2022

So I guess we need an option to optionally add X-Provider-Content-Sha256 header.

Actually, there is already such an option: when service is set to s3.

@matwey
Copy link
Author

matwey commented Jan 15, 2022

I've just double-checked. S3 rejects responses without X-Provider-Content-Sha256:

<?xml version="1.0" encoding="UTF-8"?>
<Error><Code>InvalidRequest</Code><Message>Missing required header for this request: x-amz-content-sha256</Message><RequestId>JK9VRX9RB76M58CR</RequestId><HostId>BtP3ov6WCpGYYf4772w9XEqW7TWGusRFn34r+MXD0/VjD/P8/4LsV50Lka7T1U4lU3PbhA+RaQI=</HostId></Error>⏎

Also, S3 seems to be not validating payload when the payload is empty. In this case X-Provider-Content-Sha256 should be consistent with the canonical request and may be arbitrary.

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>
@outscale-mgo outscale-mgo force-pushed the aws-sigv4-headers-rebased-master branch from afdfd05 to af197f2 Compare January 17, 2022 09:56
 1. Allow the user to provide precomputed SHA256 of the payload by providing
    X-Provider-Content-Sha256 header. If X-Provider-Content-Sha256 header is
    present then its value MUST coincide with payload hash in the canonical
    request text, so we reuse its value for the canonical request.

 2. Fallback to the empty SHA otherwise. There is no reason to calculate payload hash
    when X-Provider-Content-Sha256 is missed, since the server will fail to
    check the signature. Note, that X-Provider-Content-Sha256 is mandatory
    required for S3 API [1] while some implementations are known to rely on
    this header to reconstruct the canonical request [2].

[1] https://docs.aws.amazon.com/AmazonS3/latest/API/sig-v4-header-based-auth.html
[2] minio/minio#5340

Signed-off-by: Matwey V. Kornilov <matwey.kornilov@gmail.com>
@outscale-mgo outscale-mgo force-pushed the aws-sigv4-headers-rebased-master branch 2 times, most recently from 4640e7e to b857d01 Compare January 18, 2022 13:25
@outscale-mgo outscale-mgo force-pushed the aws-sigv4-headers-rebased-master branch from b857d01 to d6bea06 Compare March 8, 2022 15:36
@outscale-mgo outscale-mgo force-pushed the aws-sigv4-headers-rebased-master branch 3 times, most recently from 60d8d52 to acfbf2d Compare April 28, 2022 09:26
@outscale-mgo outscale-mgo force-pushed the aws-sigv4-headers-rebased-master branch 2 times, most recently from 0ebf434 to 8cf770a Compare May 9, 2022 09:39
@outscale-mgo outscale-mgo force-pushed the aws-sigv4-headers-rebased-master branch 4 times, most recently from 62548a0 to 8c6e060 Compare August 18, 2022 13:03
@outscale-mgo outscale-mgo force-pushed the aws-sigv4-headers-rebased-master branch 6 times, most recently from d11b05f to 96eb43c Compare October 10, 2022 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants