aws/request: Fix for Go 1.8 request incorrectly sent with body

aws/request/request.go

@@ -245,7 +245,82 @@ func (r *Request) ResetBody() {
 	}
 
 	r.safeBody = newOffsetReader(r.Body, r.BodyStart)
-	r.HTTPRequest.Body = r.safeBody
+
+	// Go 1.8 tightened and clarified the rules code needs to use when building
+	// requests with the http package. Go 1.8 removed the automatic detection
+	// of if the Request.Body was empty, or actually had bytes in it. The SDK
+	// always sets the Request.Body even if it is empty and should not actually
+	// be sent. This is incorrect.
+	//
+	// Go 1.8 did add a http.NoBody value that the SDK can use to tell the http
+	// client that the request really should be sent without a body. The
+	// Request.Body cannot be set to nil, which is preferable, because the
+	// field is exported and could introduce nil pointer dereferences for users
+	// of the SDK if they used that field.
+	//
+	// Related golang/go#18257
+	l, err := computeBodyLength(r.Body)
+	if err != nil {
+		r.Error = awserr.New("SerializationError", "failed to compute request body size", err)
+		return
+	}
+
+	if l == 0 {
+		r.HTTPRequest.Body = noBodyReader
+	} else if l > 0 {
+		r.HTTPRequest.Body = r.safeBody
+	} else {
+		// Hack to prevent sending bodies for methods where the body
+		// should be ignored by the server. Sending bodies on these
+		// methods without an associated ContentLength will cause the
+		// request to socket timeout because the server does not handle
+		// Transfer-Encoding: chunked bodies for these methods.
+		//
+		// This would only happen if a aws.ReaderSeekerCloser was used with
+		// a io.Reader that was not also an io.Seeker.
+		switch r.Operation.HTTPMethod {
+		case "GET", "HEAD", "DELETE":
+			r.HTTPRequest.Body = noBodyReader
+		default:
+			r.HTTPRequest.Body = r.safeBody
+		}
+	}
+}
+
+// Attempts to compute the length of the body of the reader using the
+// io.Seeker interface. If the value is not seekable because of being
+// a ReaderSeekerCloser without an unerlying Seeker -1 will be returned.
+// If no error occurs the length of the body will be returned.
+func computeBodyLength(r io.ReadSeeker) (int64, error) {
+	seekable := true
+	// Determine if the seeker is actually seekable. ReaderSeekerCloser
+	// hides the fact that a io.Readers might not actually be seekable.
+	switch v := r.(type) {
+	case aws.ReaderSeekerCloser:
+		seekable = v.IsSeeker()
+	case *aws.ReaderSeekerCloser:
+		seekable = v.IsSeeker()
+	}
+	if !seekable {
+		return -1, nil
+	}
+
+	curOffset, err := r.Seek(0, 1)
+	if err != nil {
+		return 0, err
+	}
+
+	endOffset, err := r.Seek(0, 2)
+	if err != nil {
+		return 0, err
+	}
+
+	_, err = r.Seek(curOffset, 0)
+	if err != nil {
+		return 0, err
+	}
+
+	return endOffset - curOffset, nil
 }
 
 // GetBody will return an io.ReadSeeker of the Request's underlying

aws/request/request_1_7.go

@@ -0,0 +1,21 @@
+// +build !go1.8
+
+package request
+
+import "io"
+
+// NoBody is an io.ReadCloser with no bytes. Read always returns EOF
+// and Close always returns nil. It can be used in an outgoing client
+// request to explicitly signal that a request has zero bytes.
+// An alternative, however, is to simply set Request.Body to nil.
+//
+// Copy of Go 1.8 NoBody type from net/http/http.go
+type noBody struct{}
+
+func (noBody) Read([]byte) (int, error)         { return 0, io.EOF }
+func (noBody) Close() error                     { return nil }
+func (noBody) WriteTo(io.Writer) (int64, error) { return 0, nil }
+
+// Is an empty reader that will trigger the Go HTTP client to not include
+// and body in the HTTP request.
+var noBodyReader = noBody{}

aws/request/request_1_7_test.go

@@ -0,0 +1,24 @@
+// +build !go1.8
+
+package request
+
+import (
+	"net/http"
+	"strings"
+	"testing"
+)
+
+func TestResetBody_WithEmptyBody(t *testing.T) {
+	r := Request{
+		HTTPRequest: &http.Request{},
+	}
+
+	reader := strings.NewReader("")
+	r.Body = reader
+
+	r.ResetBody()
+
+	if a, e := r.HTTPRequest.Body, (noBody{}); a != e {
+		t.Errorf("expected request body to be set to reader, got %#v", r.HTTPRequest.Body)
+	}
+}

aws/request/request_1_8.go

@@ -0,0 +1,9 @@
+// +build go1.8
+
+package request
+
+import "net/http"
+
+// Is a http.NoBody reader instructing Go HTTP client to not include
+// and body in the HTTP request.
+var noBodyReader = http.NoBody

aws/request/request_1_8_test.go

@@ -0,0 +1,25 @@
+// +build go1.8
+
+package request
+
+import (
+	"net/http"
+	"strings"
+	"testing"
+)
+
+func TestResetBody_WithEmptyBody(t *testing.T) {
+	r := Request{
+		HTTPRequest: &http.Request{},
+	}
+
+	reader := strings.NewReader("")
+	r.Body = reader
+
+	r.ResetBody()
+
+	if a, e := r.HTTPRequest.Body, http.NoBody; a != e {
+		t.Errorf("expected request body to be set to reader, got %#v",
+			r.HTTPRequest.Body)
+	}
+}

aws/request/request_resetbody_test.go

@@ -0,0 +1,58 @@
+package request
+
+import (
+	"bytes"
+	"net/http"
+	"strings"
+	"testing"
+
+	"github.com/aws/aws-sdk-go/aws"
+)
+
+func TestResetBody_WithBodyContents(t *testing.T) {
+	r := Request{
+		HTTPRequest: &http.Request{},
+	}
+
+	reader := strings.NewReader("abc")
+	r.Body = reader
+
+	r.ResetBody()
+
+	if v, ok := r.HTTPRequest.Body.(*offsetReader); !ok || v == nil {
+		t.Errorf("expected request body to be set to reader, got %#v",
+			r.HTTPRequest.Body)
+	}
+}
+
+func TestResetBody_ExcludeUnseekableBodyByMethod(t *testing.T) {
+	cases := []struct {
+		Method   string
+		IsNoBody bool
+	}{
+		{"GET", true},
+		{"HEAD", true},
+		{"DELETE", true},
+		{"PUT", false},
+		{"PATCH", false},
+		{"POST", false},
+	}
+
+	reader := aws.ReadSeekCloser(bytes.NewBuffer([]byte("abc")))
+
+	for i, c := range cases {
+		r := Request{
+			HTTPRequest: &http.Request{},
+			Operation: &Operation{
+				HTTPMethod: c.Method,
+			},
+		}
+
+		r.SetReaderBody(reader)
+
+		if a, e := r.HTTPRequest.Body == noBodyReader, c.IsNoBody; a != e {
+			t.Errorf("%d, expect body to be set to noBody(%t), but was %t", i, e, a)
+		}
+	}
+
+}

aws/request/request_test.go

@@ -8,7 +8,9 @@ import (
 	"io"
 	"io/ioutil"
 	"net/http"
+	"net/http/httptest"
 	"runtime"
+	"strconv"
 	"testing"
 	"time"
 
@@ -19,6 +21,7 @@ import (
 	"github.com/aws/aws-sdk-go/aws/credentials"
 	"github.com/aws/aws-sdk-go/aws/request"
 	"github.com/aws/aws-sdk-go/awstesting"
+	"github.com/aws/aws-sdk-go/private/protocol/rest"
 )
 
 type testData struct {
@@ -378,3 +381,61 @@ func TestRequestRecoverTimeoutWithNilResponse(t *testing.T) {
 	assert.Equal(t, 1, int(r.RetryCount))
 	assert.Equal(t, "valid", out.Data)
 }
+
+func TestRequest_NoBody(t *testing.T) {
+	cases := []string{
+		"GET", "HEAD", "DELETE",
+		"PUT", "POST", "PATCH",
+	}
+
+	for i, c := range cases {
+		server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			if v := r.TransferEncoding; len(v) > 0 {
+				t.Errorf("%d, expect no body sent with Transfer-Encoding, %v", i, v)
+			}
+
+			outMsg := []byte(`{"Value": "abc"}`)
+
+			if b, err := ioutil.ReadAll(r.Body); err != nil {
+				t.Fatalf("%d, expect no error reading request body, got %v", i, err)
+			} else if n := len(b); n > 0 {
+				t.Errorf("%d, expect no request body, got %d bytes", i, n)
+			}
+
+			w.Header().Set("Content-Length", strconv.Itoa(len(outMsg)))
+			if _, err := w.Write(outMsg); err != nil {
+				t.Fatalf("%d, expect no error writing server response, got %v", i, err)
+			}
+		}))
+
+		s := awstesting.NewClient(&aws.Config{
+			Region:     aws.String("mock-region"),
+			MaxRetries: aws.Int(0),
+			Endpoint:   aws.String(server.URL),
+			DisableSSL: aws.Bool(true),
+		})
+		s.Handlers.Build.PushBack(rest.Build)
+		s.Handlers.Validate.Clear()
+		s.Handlers.Unmarshal.PushBack(unmarshal)
+		s.Handlers.UnmarshalError.PushBack(unmarshalError)
+
+		in := struct {
+			Bucket *string `location:"uri" locationName:"bucket"`
+			Key    *string `location:"uri" locationName:"key"`
+		}{
+			Bucket: aws.String("mybucket"), Key: aws.String("myKey"),
+		}
+
+		out := struct {
+			Value *string
+		}{}
+
+		r := s.NewRequest(&request.Operation{
+			Name: "OpName", HTTPMethod: c, HTTPPath: "/{bucket}/{key+}",
+		}, &in, &out)
+
+		if err := r.Send(); err != nil {
+			t.Fatalf("%d, expect no error sending request, got %v", i, err)
+		}
+	}
+}

aws/signer/v4/v4.go

@@ -171,6 +171,16 @@ type Signer struct {
 	// http://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html
 	DisableURIPathEscaping bool
 
+	// Disales the automatical setting of the HTTP request's Body field with the
+	// io.ReadSeeker passed in to the signer. This is useful if you're using a
+	// custom wrapper around the body for the io.ReadSeeker and want to preserve
+	// the Body value on the Request.Body.
+	//
+	// This does run the risk of signing a request with a body that will not be
+	// sent in the request. Need to ensure that the underlying data of the Body
+	// values are the same.
+	DisableRequestBodyOverwrite bool
+
 	// currentTimeFn returns the time value which represents the current time.
 	// This value should only be used for testing. If it is nil the default
 	// time.Now will be used.
@@ -321,7 +331,7 @@ func (v4 Signer) signWithBody(r *http.Request, body io.ReadSeeker, service, regi
 	// If the request is not presigned the body should be attached to it. This
 	// prevents the confusion of wanting to send a signed request without
 	// the body the request was signed for attached.
-	if !ctx.isPresign {
+	if !(v4.DisableRequestBodyOverwrite || ctx.isPresign) {
 		var reader io.ReadCloser
 		if body != nil {
 			var ok bool
@@ -416,6 +426,10 @@ func signSDKRequestWithCurrTime(req *request.Request, curTimeFn func() time.Time
 			// S3 service should not have any escaping applied
 			v4.DisableURIPathEscaping = true
 		}
+		// Prevents setting the HTTPRequest's Body. Since the Body could be
+		// wrapped in a custom io.Closer that we do not want to be stompped
+		// on top of by the signer.
+		v4.DisableRequestBodyOverwrite = true
 	})
 
 	signingTime := req.Time

aws/signer/v4/v4_test.go

@@ -424,6 +424,49 @@ func TestBuildCanonicalRequest(t *testing.T) {
 	assert.Equal(t, expected, ctx.Request.URL.String())
 }
 
+func TestSignWithBody_ReplaceRequestBody(t *testing.T) {
+	creds := credentials.NewStaticCredentials("AKID", "SECRET", "SESSION")
+	req, seekerBody := buildRequest("dynamodb", "us-east-1", "{}")
+	req.Body = ioutil.NopCloser(bytes.NewReader([]byte{}))
+
+	s := NewSigner(creds)
+	origBody := req.Body
+
+	_, err := s.Sign(req, seekerBody, "dynamodb", "us-east-1", time.Now())
+	if err != nil {
+		t.Fatalf("expect no error, got %v", err)
+	}
+
+	if req.Body == origBody {
+		t.Errorf("expeect request body to not be origBody")
+	}
+
+	if req.Body == nil {
+		t.Errorf("expect request body to be changed but was nil")
+	}
+}
+
+func TestSignWithBody_NoReplaceRequestBody(t *testing.T) {
+	creds := credentials.NewStaticCredentials("AKID", "SECRET", "SESSION")
+	req, seekerBody := buildRequest("dynamodb", "us-east-1", "{}")
+	req.Body = ioutil.NopCloser(bytes.NewReader([]byte{}))
+
+	s := NewSigner(creds, func(signer *Signer) {
+		signer.DisableRequestBodyOverwrite = true
+	})
+
+	origBody := req.Body
+
+	_, err := s.Sign(req, seekerBody, "dynamodb", "us-east-1", time.Now())
+	if err != nil {
+		t.Fatalf("expect no error, got %v", err)
+	}
+
+	if req.Body != origBody {
+		t.Errorf("expeect request body to not be chagned")
+	}
+}
+
 func BenchmarkPresignRequest(b *testing.B) {
 	signer := buildSigner()
 	req, body := buildRequest("dynamodb", "us-east-1", "{}")