aws/signer/v4: Fix X-Amz-Content-Sha256 being in to query for presign (#188)

Fixes the bug which would allow the X-Amz-Content-Sha256 header to be
promoted to the query string when presigning a S3 request.

This bug also was preventing users from setting their own sha256 value
for a presigned URL. Presigned requests generated with the custom sha256
would of always failed with invalid signature.

S3 presign requests without a user specified X-Amz-Content-Sha256 will
no longer include the X-Amz-Content-Sha256 in the header nor query. The
X-Amz-Content-Sha256 header only needs to be set if it contains a non
UNSIGNED-PAYLOAD value.

Related to aws/aws-sdk-go#1974

Author: jasdel

aws/signer/v4/functional_test.go

@@ -52,7 +52,7 @@ func TestPresignHandler(t *testing.T) {
 	expectedHost := "bucket.s3.mock-region.amazonaws.com"
 	expectedDate := "19700101T000000Z"
 	expectedHeaders := "content-disposition;host;x-amz-acl"
-	expectedSig := "a46583256431b09eb45ba4af2e6286d96a9835ed13721023dc8076dfdcb90fcb"
+	expectedSig := "2d76a414208c0eac2a23ef9c834db9635ecd5a0fbb447a00ad191f82d854f55b"
 	expectedCred := "AKID/19700101/mock-region/s3/aws4_request"
 
 	u, _ := url.Parse(urlstr)
@@ -75,8 +75,8 @@ func TestPresignHandler(t *testing.T) {
 	if e, a := "300", urlQ.Get("X-Amz-Expires"); e != a {
 		t.Errorf("expect %v, got %v", e, a)
 	}
-	if e, a := "UNSIGNED-PAYLOAD", urlQ.Get("X-Amz-Content-Sha256"); e != a {
-		t.Errorf("expect %v, got %v", e, a)
+	if a := urlQ.Get("X-Amz-Content-Sha256"); len(a) != 0 {
+		t.Errorf("expect no content sha256 got %v", a)
 	}
 
 	if e, a := "+", urlstr; strings.Contains(a, e) { // + encoded as %20
@@ -105,7 +105,7 @@ func TestPresignRequest(t *testing.T) {
 	expectedHost := "bucket.s3.mock-region.amazonaws.com"
 	expectedDate := "19700101T000000Z"
 	expectedHeaders := "content-disposition;host;x-amz-acl"
-	expectedSig := "a46583256431b09eb45ba4af2e6286d96a9835ed13721023dc8076dfdcb90fcb"
+	expectedSig := "2d76a414208c0eac2a23ef9c834db9635ecd5a0fbb447a00ad191f82d854f55b"
 	expectedCred := "AKID/19700101/mock-region/s3/aws4_request"
 	expectedHeaderMap := http.Header{
 		"x-amz-acl":           []string{"public-read"},
@@ -135,8 +135,8 @@ func TestPresignRequest(t *testing.T) {
 	if e, a := "300", urlQ.Get("X-Amz-Expires"); e != a {
 		t.Errorf("expect %v, got %v", e, a)
 	}
-	if e, a := "UNSIGNED-PAYLOAD", urlQ.Get("X-Amz-Content-Sha256"); e != a {
-		t.Errorf("expect %v, got %v", e, a)
+	if a := urlQ.Get("X-Amz-Content-Sha256"); len(a) != 0 {
+		t.Errorf("expect no content sha256 got %v", a)
 	}
 
 	if e, a := "+", urlstr; strings.Contains(a, e) { // + encoded as %20

aws/signer/v4/v4.go

@@ -133,6 +133,7 @@ var requiredSignedHeaders = rules{
 			"X-Amz-Server-Side-Encryption-Customer-Key-Md5":               struct{}{},
 			"X-Amz-Storage-Class":                                         struct{}{},
 			"X-Amz-Website-Redirect-Location":                             struct{}{},
+			"X-Amz-Content-Sha256":                                        struct{}{},
 		},
 	},
 	patterns{"X-Amz-Meta-"},
@@ -643,14 +644,22 @@ func (ctx *signingCtx) buildSignature() {
 func (ctx *signingCtx) buildBodyDigest() {
 	hash := ctx.Request.Header.Get("X-Amz-Content-Sha256")
 	if hash == "" {
-		if ctx.unsignedPayload || (ctx.isPresign && ctx.ServiceName == "s3") {
+		includeSHA256Header := ctx.unsignedPayload ||
+			ctx.ServiceName == "s3" ||
+			ctx.ServiceName == "glacier"
+
+		s3Presign := ctx.isPresign && ctx.ServiceName == "s3"
+
+		if ctx.unsignedPayload || s3Presign {
 			hash = "UNSIGNED-PAYLOAD"
+			includeSHA256Header = !s3Presign
 		} else if ctx.Body == nil {
 			hash = emptyStringSHA256
 		} else {
 			hash = hex.EncodeToString(makeSha256Reader(ctx.Body))
 		}
-		if ctx.unsignedPayload || ctx.ServiceName == "s3" || ctx.ServiceName == "glacier" {
+
+		if includeSHA256Header {
 			ctx.Request.Header.Set("X-Amz-Content-Sha256", hash)
 		}
 	}

aws/signer/v4/v4_test.go

@@ -197,16 +197,36 @@ func TestSignBodyGlacier(t *testing.T) {
 	}
 }
 
-func TestPresignEmptyBodyS3(t *testing.T) {
-	req, body := buildRequest("s3", "us-east-1", "hello")
+func TestPresign_SignedPayload(t *testing.T) {
+	req, body := buildRequest("glacier", "us-east-1", "hello")
 	signer := buildSigner()
-	signer.Presign(req, body, "s3", "us-east-1", 5*time.Minute, time.Now())
+	signer.Presign(req, body, "glacier", "us-east-1", 5*time.Minute, time.Now())
+	hash := req.Header.Get("X-Amz-Content-Sha256")
+	if e, a := "2cf24dba5fb0a30e26e83b2ac5b9e29e1b161e5c1fa7425e73043362938b9824", hash; e != a {
+		t.Errorf("expect %v, got %v", e, a)
+	}
+}
+
+func TestPresign_UnsignedPayload(t *testing.T) {
+	req, body := buildRequest("service-name", "us-east-1", "hello")
+	signer := buildSigner()
+	signer.UnsignedPayload = true
+	signer.Presign(req, body, "service-name", "us-east-1", 5*time.Minute, time.Now())
 	hash := req.Header.Get("X-Amz-Content-Sha256")
 	if e, a := "UNSIGNED-PAYLOAD", hash; e != a {
 		t.Errorf("expect %v, got %v", e, a)
 	}
 }
 
+func TestPresign_UnsignedPayload_S3(t *testing.T) {
+	req, body := buildRequest("s3", "us-east-1", "hello")
+	signer := buildSigner()
+	signer.Presign(req, body, "s3", "us-east-1", 5*time.Minute, time.Now())
+	if a := req.Header.Get("X-Amz-Content-Sha256"); len(a) != 0 {
+		t.Errorf("expect no content sha256 got %v", a)
+	}
+}
+
 func TestSignPrecomputedBodyChecksum(t *testing.T) {
 	req, body := buildRequest("dynamodb", "us-east-1", "hello")
 	req.Header.Set("X-Amz-Content-Sha256", "PRECOMPUTED")