Use UNSIGNED-PAYLOAD if sha256 is not calculated

[krishnasrinivas]

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

Reference: #661

Tested api_functional_v4_test.go on localhost:9000 minio
Tested with mc --debug on aws-s3

[krisis]

why is []byte{} used in favour of nil? The default value for a slice is nil so we can avoid setting metadata.contentSHA256Bytes like we are doing in this PR.

[krishnasrinivas]

I copy/pasted it from the previous code:
shaHeader = hex.EncodeToString(sum256([]byte{}))

let me fix that.

[krisis]

@krishnasrinivas Could you add a functional test case capturing this?

[krishnasrinivas]

@krishnasrinivas Could you add a functional test case capturing this?

@krisis actually I see that we don't have functional tests for the Core functions. I could bring it as a separate commit as it will take time to write all the tests.

[krisis]

@krishnasrinivas see core_test.go. @harshavardhana added one yesterday.

[krisis]

Why can't we check for nil like before? I.e,

if metadata.contentSHA256Bytes != nil {
     // ....
}

[krishnasrinivas]

So that we do the right thing if the caller passes []byte{} like how remco's s3 gateway patch does.

[krisis]

To quote you from #661 (comment)

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

So, I thought metadata.contentSHA256Bytes == nil meant the client wants to send unsigned payload and signed payload otherwise.

[krisis]

I was coming from the point that nil is a sentinel value for []T and makes it easier to recognise as a pattern while reading code. if len(contentSHA256Bytes) > 0 allows users to send []byte{} and nil interchangeably, which is convenient, so this check is fine by me.

[krishnasrinivas]

I had it as nil, but changed it after it failed with remco's patch

[harshavardhana]

The story of

slice := nil 

or 

var slice []byte

slice := []byte{}

is that if slice != nil would work for the former but not the latter but functionally both are of same len and cap

Quite surprisingly this happens with encoding JSON objects (a nil slice encodes to null, while []string{} encodes to the JSON array []).

[krisis]

Is the error message as expected? Shouldn't it be as below?

t.Fatalf(..., minPartSize, size)

[krisis]

for uniformity sake, shall we have one newline between any two code-blocks. Could you fix that in this function?

[vadmeste]

Doesn't this change break something ?
I think it should be:

case c.signature.isV4():
   fallthrough
case c.signature.isStreamingV4() && method != "PUT":
   ..

[krishnasrinivas]

@vadmeste the previous code was not looking correct.

	} else if c.signature.isV4() || c.signature.isStreamingV4() &&
		method != "PUT" {

I reviewed the code and saw that StreamingSignature is enabled only for PUT request. Hence this code was not necessary. Let me know if I am reading anything incorrectly.

[vadmeste]

I reviewed the code and saw that StreamingSignature is enabled only for PUT request. Hence this code was not necessary. Let me know if I am reading anything incorrectly.

yes you are right, a request is stream v4 only when we are going to upload an object or a part, thanks.

[krisis]

@krishnasrinivas the check is still required since the client's signature type is set before we know if the upload is going to be a single PUT object or a multipart upload. Streaming signature applies only to http.MethodPUT. API calls corresponding to get incomplete uploads, initiate new multipart, etc should use signature v4. Having said that, you could see if it can be refactored to be obvious to the reader and maintainable in the long run.

The current PR fails in go test -v -run=TestPutObjectStreaming when we make the following change. I have made the size of object to be uploaded to 64MB + 1 bytes.

diff --git a/api_functional_v4_test.go b/api_functional_v4_test.go
index dacbba8..c7c4a00 100644
--- a/api_functional_v4_test.go
+++ b/api_functional_v4_test.go
@@ -392,7 +392,7 @@ func TestPutObjectStreaming(t *testing.T) {
        }
 
        // Upload an object.
-       sizes := []int64{0, 64*1024 - 1, 64 * 1024}
+       sizes := []int64{0, 64*1024 - 1, 64*1024*1024 + 1}
        objectName := "test-object"
        for i, size := range sizes {
                data := bytes.Repeat([]byte("a"), int(size))

[krishnasrinivas]

@krisis you are right, let me fix that

[krisis]

LGTM. Locally tested with object size 64MB + 1 bytes.