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.

Sign up for GitHub

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

Use UNSIGNED-PAYLOAD if sha256 is not calculated #661

Closed
wants to merge 1 commit into from
Closed

Use UNSIGNED-PAYLOAD if sha256 is not calculated #661

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -659,9 +659,7 @@ func (c Client) newRequest(method string, metadata requestMetadata) (req *http.R
// Set sha256 sum for signature calculation only with signature version '4'.
shaHeader := unsignedPayload
if !c.secure {
if metadata.contentSHA256Bytes == nil {
shaHeader = hex.EncodeToString(sum256([]byte{}))
} else {
if len(metadata.contentSHA256Bytes) > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this specifically be for presigned operations? - why is it done for regular signature V4. When there is no data it should be calculated as "sha256" of an empty byte array.

http://docs.aws.amazon.com/AmazonS3/latest/API/sig-v4-header-based-auth.html

Hex(SHA256Hash(""))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, actually this is unrelated to presigned url.

i.e the following code gives error without this fix:

krishna@escape:~/tmp$ cat test.go
package main

import (
	"log"
	"strings"

	minio "github.com/minio/minio-go"
)

func main() {
	client, err := minio.NewCore("localhost:9000", "SXO8VW2OFKKP2OG7AC85", "CKWSSgrUgvfUMTaNBkB63exet4WW+uNhQvi91Bc3", false)
	if err != nil {
		log.Fatal(err)
	}
	bucket := "testbucket"
	object := "testobject"
	content := "hello world"
	size := int64(len(content))
	r := strings.NewReader(content)
	_, err = client.PutObject(bucket, object, size, r, nil, nil, nil)
	if err != nil {
		log.Fatal(err)
	}
}
krishna@escape:~/tmp$ go run test.go
2017/04/23 19:16:13 The provided 'x-amz-content-sha256' header does not match what was computed.
exit status 1
krishna@escape:~/tmp$

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 issue was found when testing S3 gateway. In gateway setup, if client does
presignedPUT the sha256 value is not valid in which case we should use
UNSIGNED-PAYLOAD instead of calculating sha256sum() on empty byte array

meaning, for presignedPUT the gateway calls:

Core.PutObject() with sha256bytes as empty byte array

Now minio-go sets SHA256 header as Hex(SHA256Hash("")) because of the bug instead of UNSIGNED-PAYLOAD

Copy link
Member

Choose a reason for hiding this comment

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

But we are breaking backward compatibility here. The example is wrong it is expected in signature-v4 for PUT you are passing in the payload checksum. So setting it as nil is wrong.

What should be set is perhaps []byte("UNSIGNED-PAYLOAD") is passed in from PutObject() perspective indicating that the payload checksum is going to be unsigned and internal code differentiates this.

Otherwise we are breaking backward compatibility i.e for some servers which do not implement UNSIGNED-PAYLOAD verification for GET, HEAD operations would lead to signature issues.

Copy link
Member

Choose a reason for hiding this comment

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

@krishnasrinivas It is not wrong to compute sha256 sum of []byte{} and set x-amz-content-sha256 header with it. See Important section here http://docs.aws.amazon.com/AmazonS3/latest/API/sig-v4-header-based-auth.html. UNSIGNED-PAYLOAD is an alternate. So, the server should work with requests signed using either of the approaches.

Copy link
Contributor Author

@krishnasrinivas krishnasrinivas Apr 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Otherwise we are breaking backward compatibility i.e for some servers which do not implement UNSIGNED-PAYLOAD verification for GET, HEAD operations would lead to signature issues.

but even without this fix, for secure connections, we are sending UNSIGNED-PAYLOAD for GET/HEAD, take a look at this:

krishna@escape:~$ mc --debug cat s3/krisbuck/passwd
mc: <DEBUG> GET /krisbuck/?location= HTTP/1.1
Host: s3.amazonaws.com
User-Agent: Minio (linux; amd64) minio-go/2.0.4 mc/DEVELOPMENT.GOGET
Authorization: AWS4-HMAC-SHA256 Credential=**REDACTED**/20170424/us-east-1/s3/aws4_request, SignedHeaders=host;x-amz-content-sha256;x-amz-date, Signature=**REDACTED**
X-Amz-Content-Sha256: UNSIGNED-PAYLOAD
X-Amz-Date: 20170424T063041Z
Accept-Encoding: gzip

mc: <DEBUG> HTTP/1.1 200 OK
Transfer-Encoding: chunked
Content-Type: application/xml
Date: Mon, 24 Apr 2017 06:30:42 GMT
Server: AmazonS3
X-Amz-Id-2: tC2i4HFd7VHFDaUQ4gzO37h+PxBAs+yI/zogjuNoMtB7rFEQ0aItQHAt8jmp2W5xJQvWJmY9IYs=
X-Amz-Request-Id: 87492B7834CCCD81

mc: <DEBUG> Response Time:  482.121718ms

mc: <DEBUG> GET /?delimiter=%2F&fetch-owner=true&list-type=2&max-keys=1000&prefix=passwd HTTP/1.1
Host: krisbuck.s3.amazonaws.com
User-Agent: Minio (linux; amd64) minio-go/2.0.4 mc/DEVELOPMENT.GOGET
Authorization: AWS4-HMAC-SHA256 Credential=**REDACTED**/20170424/us-east-1/s3/aws4_request, SignedHeaders=host;x-amz-content-sha256;x-amz-date, Signature=**REDACTED**
X-Amz-Content-Sha256: UNSIGNED-PAYLOAD
X-Amz-Date: 20170424T063041Z
Accept-Encoding: gzip

mc: <DEBUG> HTTP/1.1 200 OK
Transfer-Encoding: chunked
Content-Type: application/xml
Date: Mon, 24 Apr 2017 06:30:43 GMT
Server: AmazonS3
X-Amz-Bucket-Region: us-east-1
X-Amz-Id-2: g4oRn0k5X1eNANYJaF92HPR1WlgrXmWEQQernVrGXxgP0uIUmnEQjUud1bbjorMEE7V5eYnB7/s=
X-Amz-Request-Id: 0D01FE6F31848632

mc: <DEBUG> Response Time:  726.363076ms

mc: <DEBUG> HEAD /passwd HTTP/1.1
Host: krisbuck.s3.amazonaws.com
User-Agent: Minio (linux; amd64) minio-go/2.0.4 mc/DEVELOPMENT.GOGET
Authorization: AWS4-HMAC-SHA256 Credential=**REDACTED**/20170424/us-east-1/s3/aws4_request, SignedHeaders=host;x-amz-content-sha256;x-amz-date, Signature=**REDACTED**
X-Amz-Content-Sha256: UNSIGNED-PAYLOAD
X-Amz-Date: 20170424T063042Z

mc: <DEBUG> HTTP/1.1 200 OK
Content-Length: 2284
Accept-Ranges: bytes
Content-Type: application/octet-stream
Date: Mon, 24 Apr 2017 06:30:43 GMT
Etag: "73c2ee0acb590362c0ac3740f8c39523"
Last-Modified: Mon, 24 Apr 2017 06:30:34 GMT
Server: AmazonS3
X-Amz-Id-2: 0NnKbRgQIE5Kq2yRJ+zdQX0fKijcJ413XDhRhqq42bv6ZbmvykpmcvM8ZleG5AjiPO4uwzAqKCs=
X-Amz-Request-Id: 1E6CE7455F322E3F

mc: <DEBUG> Response Time:  91.483497ms

mc: <DEBUG> GET /passwd HTTP/1.1
Host: krisbuck.s3.amazonaws.com
User-Agent: Minio (linux; amd64) minio-go/2.0.4 mc/DEVELOPMENT.GOGET
Authorization: AWS4-HMAC-SHA256 Credential=**REDACTED**/20170424/us-east-1/s3/aws4_request, SignedHeaders=host;x-amz-content-sha256;x-amz-date, Signature=**REDACTED**
X-Amz-Content-Sha256: UNSIGNED-PAYLOAD
X-Amz-Date: 20170424T063042Z
Accept-Encoding: gzip

mc: <DEBUG> HTTP/1.1 200 OK
Content-Length: 2284
Accept-Ranges: bytes
Content-Type: application/octet-stream
Date: Mon, 24 Apr 2017 06:30:43 GMT
Etag: "73c2ee0acb590362c0ac3740f8c39523"
Last-Modified: Mon, 24 Apr 2017 06:30:34 GMT
Server: AmazonS3
X-Amz-Id-2: dFrgt4Y2e6WL8Y/r0eWFk+pC9wF5C+cjc0w0NKbtn1ksJXNB9zkR16GR9z/M2Ju8s2EjZqprw10=
X-Amz-Request-Id: 0BF985468EB69ADF

mc: <DEBUG> Response Time:  90.616068ms

root:x:0:0:root:/root:/bin/bash
....
...

But I see what you mean here, minio-js was written such that it uses UNSIGNED-PAYLOAD only for "upload object" operations.

What should be set is perhaps []byte("UNSIGNED-PAYLOAD") is passed in from PutObject() perspective indicating that the payload checksum is going to be unsigned and internal code differentiates this.

It can be fixed in a cleaner way if we want to fix it. i.e metadata.contentSHA256Bytes being nil should not indicate whether the body itself is empty. If the body is empty the caller should set metadata.contentSHA256Bytes to Hex(SHA256Hash("")) and If metadata.contentSHA256Bytes is set to nil then we simple use UNSIGNED-PAYLOAD

@krishnasrinivas It is not wrong to compute sha256 sum of []byte{} and set x-amz-content-sha256 header with it. See Important section here http://docs.aws.amazon.com/AmazonS3/latest/API/sig-v4-header-based-auth.html. UNSIGNED-PAYLOAD is an alternate. So, the server should work with requests signed using either of the approaches.

Ofcourse. This commit does not counter that fact.

Copy link
Member

Choose a reason for hiding this comment

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

Ofcourse. This commit does not counter that fact.

Oops. This is completely different from what I was referring to. UNSIGNED-PAYLOAD got me.

shaHeader = hex.EncodeToString(metadata.contentSHA256Bytes)
}
}
Expand Down