From e66ba2ad675a18a4007df31962f983b1d01c7a89 Mon Sep 17 00:00:00 2001 From: Jason Del Ponte Date: Fri, 9 Dec 2016 13:07:14 -0800 Subject: [PATCH 01/11] aws/request: Fix for Go 1.8 request incorrectly sent with body 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 Fix #984 --- aws/request/request.go | 7 ++-- aws/request/request_resetbody.go | 22 ++++++++++++ aws/request/request_resetbody_1.7.go | 8 +++++ aws/request/request_resetbody_1.7_test.go | 41 +++++++++++++++++++++++ aws/request/request_resetbody_test.go | 41 +++++++++++++++++++++++ 5 files changed, 117 insertions(+), 2 deletions(-) create mode 100644 aws/request/request_resetbody.go create mode 100644 aws/request/request_resetbody_1.7.go create mode 100644 aws/request/request_resetbody_1.7_test.go create mode 100644 aws/request/request_resetbody_test.go diff --git a/aws/request/request.go b/aws/request/request.go index 3c8851a112d..b2d31105d72 100644 --- a/aws/request/request.go +++ b/aws/request/request.go @@ -244,8 +244,11 @@ func (r *Request) ResetBody() { r.safeBody.Close() } - r.safeBody = newOffsetReader(r.Body, r.BodyStart) - r.HTTPRequest.Body = r.safeBody + // Resets the body with the offset reader. For Go +1.8 the + // body is only wrapped if it is no empty. Otherwise the + // http.NoBody value will be written to the http.Request.Body + // field. + resetBody(r) } // GetBody will return an io.ReadSeeker of the Request's underlying diff --git a/aws/request/request_resetbody.go b/aws/request/request_resetbody.go new file mode 100644 index 00000000000..fdea974aafa --- /dev/null +++ b/aws/request/request_resetbody.go @@ -0,0 +1,22 @@ +// +build go1.8 + +package request + +import ( + "io" + "net/http" +) + +func resetBody(r *Request) { + curOffset, _ := r.Body.Seek(0, io.SeekCurrent) + endOffset, _ := r.Body.Seek(0, io.SeekEnd) + r.Body.Seek(r.BodyStart, io.SeekStart) + + r.safeBody = newOffsetReader(r.Body, r.BodyStart) + + if endOffset-curOffset == 0 { + r.HTTPRequest.Body = http.NoBody + } else { + r.HTTPRequest.Body = r.safeBody + } +} diff --git a/aws/request/request_resetbody_1.7.go b/aws/request/request_resetbody_1.7.go new file mode 100644 index 00000000000..3eb26355e9b --- /dev/null +++ b/aws/request/request_resetbody_1.7.go @@ -0,0 +1,8 @@ +// +build !go1.8 + +package request + +func resetBody(r *Request) { + r.safeBody = newOffsetReader(r.Body, r.BodyStart) + r.HTTPRequest.Body = r.safeBody +} diff --git a/aws/request/request_resetbody_1.7_test.go b/aws/request/request_resetbody_1.7_test.go new file mode 100644 index 00000000000..8f643031442 --- /dev/null +++ b/aws/request/request_resetbody_1.7_test.go @@ -0,0 +1,41 @@ +// +build !go1.8 + +package request + +import ( + "net/http" + "strings" + "testing" +) + +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_WithEmptyBody(t *testing.T) { + r := Request{ + HTTPRequest: &http.Request{}, + } + + reader := strings.NewReader("") + 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) + } +} diff --git a/aws/request/request_resetbody_test.go b/aws/request/request_resetbody_test.go new file mode 100644 index 00000000000..cb92cdeaef9 --- /dev/null +++ b/aws/request/request_resetbody_test.go @@ -0,0 +1,41 @@ +// +build go1.8 + +package request + +import ( + "net/http" + "strings" + "testing" +) + +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_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) + } +} From 6b1f0ee36eefb6b877191a3efb4ed9991f4121e4 Mon Sep 17 00:00:00 2001 From: Jason Del Ponte Date: Fri, 9 Dec 2016 13:25:20 -0800 Subject: [PATCH 02/11] add docs --- aws/request/request_resetbody.go | 13 +++++++++++++ aws/request/request_resetbody_1.7.go | 3 +++ 2 files changed, 16 insertions(+) diff --git a/aws/request/request_resetbody.go b/aws/request/request_resetbody.go index fdea974aafa..5e2e7ae4a39 100644 --- a/aws/request/request_resetbody.go +++ b/aws/request/request_resetbody.go @@ -7,6 +7,19 @@ import ( "net/http" ) +// 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 func resetBody(r *Request) { curOffset, _ := r.Body.Seek(0, io.SeekCurrent) endOffset, _ := r.Body.Seek(0, io.SeekEnd) diff --git a/aws/request/request_resetbody_1.7.go b/aws/request/request_resetbody_1.7.go index 3eb26355e9b..fa69a8cc4f6 100644 --- a/aws/request/request_resetbody_1.7.go +++ b/aws/request/request_resetbody_1.7.go @@ -2,6 +2,9 @@ package request +// Pre Go 1.8 funcitonality to always set Request.Body to a value regardless +// if it will actually be used. Go 1.8 removed undocumented feature the SDK +// expected where the request body would only be sent if the length was > 0. func resetBody(r *Request) { r.safeBody = newOffsetReader(r.Body, r.BodyStart) r.HTTPRequest.Body = r.safeBody From bdda5b9f37ac432bc7a06c8bc3807474d3ecb4c0 Mon Sep 17 00:00:00 2001 From: Jason Del Ponte Date: Fri, 9 Dec 2016 15:35:01 -0800 Subject: [PATCH 03/11] deprecate aws.ReaderSeekerCloser --- aws/request/request_resetbody.go | 55 +++++++++++++++++++++---- aws/types.go | 8 ++++ private/model/api/param_filler.go | 2 +- private/protocol/rest/unmarshal.go | 14 +++++-- private/protocol/restjson/build_test.go | 2 +- service/s3/statusok_error.go | 5 +-- 6 files changed, 70 insertions(+), 16 deletions(-) diff --git a/aws/request/request_resetbody.go b/aws/request/request_resetbody.go index 5e2e7ae4a39..34643095472 100644 --- a/aws/request/request_resetbody.go +++ b/aws/request/request_resetbody.go @@ -5,6 +5,9 @@ package request import ( "io" "net/http" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/aws/awserr" ) // Go 1.8 tightened and clarified the rules code needs to use when building @@ -21,15 +24,53 @@ import ( // // Related golang/go#18257 func resetBody(r *Request) { - curOffset, _ := r.Body.Seek(0, io.SeekCurrent) - endOffset, _ := r.Body.Seek(0, io.SeekEnd) - r.Body.Seek(r.BodyStart, io.SeekStart) - r.safeBody = newOffsetReader(r.Body, r.BodyStart) - if endOffset-curOffset == 0 { - r.HTTPRequest.Body = http.NoBody + seekable := false + // Determine if the seeker is actually seekable. ReaderSeekerCloser + // hides the fact that a io.Readers might not actually be seekable. + switch v := r.Body.(type) { + case aws.ReaderSeekerCloser: + seekable = v.IsSeeker() + case *aws.ReaderSeekerCloser: + seekable = v.IsSeeker() + default: + seekable = true + } + + if seekable { + curOffset, err := r.Body.Seek(0, io.SeekCurrent) + if err != nil { + r.Error = awserr.New("SerializationError", "failed to seek request body", err) + return + } + endOffset, err := r.Body.Seek(0, io.SeekEnd) + if err != nil { + r.Error = awserr.New("SerializationError", "failed to seek request body", err) + return + } + _, err = r.Body.Seek(r.BodyStart, io.SeekStart) + if err != nil { + r.Error = awserr.New("SerializationError", "failed to seek request body", err) + return + } + + if endOffset-curOffset == 0 { + r.HTTPRequest.Body = http.NoBody + } else { + r.HTTPRequest.Body = r.safeBody + } } else { - r.HTTPRequest.Body = r.safeBody + // 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. + switch r.Operation.HTTPMethod { + case "GET", "HEAD", "DELETE": + r.HTTPRequest.Body = http.NoBody + default: + r.HTTPRequest.Body = r.safeBody + } } } diff --git a/aws/types.go b/aws/types.go index fa014b49e1d..f0729ed0e5b 100644 --- a/aws/types.go +++ b/aws/types.go @@ -6,6 +6,8 @@ import ( ) // ReadSeekCloser wraps a io.Reader returning a ReaderSeekerCloser +// +// Deprecated: Masking reader without valid seeker hides errors. If using for S3 PutObject use s3manager.Uploader instead. func ReadSeekCloser(r io.Reader) ReaderSeekerCloser { return ReaderSeekerCloser{r} } @@ -44,6 +46,12 @@ func (r ReaderSeekerCloser) Seek(offset int64, whence int) (int64, error) { return int64(0), nil } +// IsSeeker returns if the underlying reader is also a seeker. +func (r ReaderSeekerCloser) IsSeeker() bool { + _, ok := r.r.(io.Seeker) + return ok +} + // Close closes the ReaderSeekerCloser. // // If the ReaderSeekerCloser is not an io.Closer nothing will be done. diff --git a/private/model/api/param_filler.go b/private/model/api/param_filler.go index 4626e72469c..eb3edb9d2a5 100644 --- a/private/model/api/param_filler.go +++ b/private/model/api/param_filler.go @@ -55,7 +55,7 @@ func (f paramFiller) paramsStructAny(value interface{}, shape *Shape) string { case "blob": v := reflect.Indirect(reflect.ValueOf(value)) if v.IsValid() && shape.Streaming { - return fmt.Sprintf("aws.ReadSeekCloser(bytes.NewBufferString(%#v))", v.Interface()) + return fmt.Sprintf("bytes.NewReader([]byte(%#v))", v.Interface()) } else if v.IsValid() { return fmt.Sprintf("[]byte(%#v)", v.Interface()) } diff --git a/private/protocol/rest/unmarshal.go b/private/protocol/rest/unmarshal.go index 2cba1d9aa7d..9c00921c09b 100644 --- a/private/protocol/rest/unmarshal.go +++ b/private/protocol/rest/unmarshal.go @@ -1,6 +1,7 @@ package rest import ( + "bytes" "encoding/base64" "fmt" "io" @@ -11,7 +12,6 @@ import ( "strings" "time" - "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/awserr" "github.com/aws/aws-sdk-go/aws/request" ) @@ -70,10 +70,16 @@ func unmarshalBody(r *request.Request, v reflect.Value) { } default: switch payload.Type().String() { - case "io.ReadSeeker": - payload.Set(reflect.ValueOf(aws.ReadSeekCloser(r.HTTPResponse.Body))) - case "aws.ReadSeekCloser", "io.ReadCloser": + case "io.ReadCloser": payload.Set(reflect.ValueOf(r.HTTPResponse.Body)) + case "io.ReadSeeker": + b, err := ioutil.ReadAll(r.HTTPResponse.Body) + if err != nil { + r.Error = awserr.New("SerializationError", + "failed to read response body", err) + return + } + payload.Set(reflect.ValueOf(ioutil.NopCloser(bytes.NewReader(b)))) default: io.Copy(ioutil.Discard, r.HTTPResponse.Body) defer r.HTTPResponse.Body.Close() diff --git a/private/protocol/restjson/build_test.go b/private/protocol/restjson/build_test.go index 55fdf55a002..8e17b74b27c 100644 --- a/private/protocol/restjson/build_test.go +++ b/private/protocol/restjson/build_test.go @@ -3590,7 +3590,7 @@ func TestInputService9ProtocolTestURIParameterQuerystringParamsHeadersAndJSONBod func TestInputService10ProtocolTestStreamingPayloadCase1(t *testing.T) { svc := NewInputService10ProtocolTest(unit.Session, &aws.Config{Endpoint: aws.String("https://test")}) input := &InputService10TestShapeInputService10TestCaseOperation1Input{ - Body: aws.ReadSeekCloser(bytes.NewBufferString("contents")), + Body: bytes.NewReader([]byte("contents")), Checksum: aws.String("foo"), VaultName: aws.String("name"), } diff --git a/service/s3/statusok_error.go b/service/s3/statusok_error.go index ce65fcdaf6c..5a78fd33703 100644 --- a/service/s3/statusok_error.go +++ b/service/s3/statusok_error.go @@ -5,7 +5,6 @@ import ( "io/ioutil" "net/http" - "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/awserr" "github.com/aws/aws-sdk-go/aws/request" ) @@ -17,8 +16,8 @@ func copyMultipartStatusOKUnmarhsalError(r *request.Request) { return } body := bytes.NewReader(b) - r.HTTPResponse.Body = aws.ReadSeekCloser(body) - defer r.HTTPResponse.Body.(aws.ReaderSeekerCloser).Seek(0, 0) + r.HTTPResponse.Body = ioutil.NopCloser(body) + defer body.Seek(0, 0) if body.Len() == 0 { // If there is no body don't attempt to parse the body. From e2d64a023ae871d8707a43893a69d2ccec18b68d Mon Sep 17 00:00:00 2001 From: Jason Del Ponte Date: Mon, 12 Dec 2016 14:36:28 -0800 Subject: [PATCH 04/11] simplified implementation --- aws/request/request.go | 79 +++++++++++++++++++++-- aws/request/request_1_7.go | 21 ++++++ aws/request/request_1_7_test.go | 24 +++++++ aws/request/request_1_8.go | 9 +++ aws/request/request_1_8_test.go | 25 +++++++ aws/request/request_resetbody.go | 76 ---------------------- aws/request/request_resetbody_1.7.go | 11 ---- aws/request/request_resetbody_1.7_test.go | 41 ------------ aws/request/request_resetbody_test.go | 18 ------ 9 files changed, 153 insertions(+), 151 deletions(-) create mode 100644 aws/request/request_1_7.go create mode 100644 aws/request/request_1_7_test.go create mode 100644 aws/request/request_1_8.go create mode 100644 aws/request/request_1_8_test.go delete mode 100644 aws/request/request_resetbody.go delete mode 100644 aws/request/request_resetbody_1.7.go delete mode 100644 aws/request/request_resetbody_1.7_test.go diff --git a/aws/request/request.go b/aws/request/request.go index b2d31105d72..a6273cdc1f2 100644 --- a/aws/request/request.go +++ b/aws/request/request.go @@ -244,11 +244,80 @@ func (r *Request) ResetBody() { r.safeBody.Close() } - // Resets the body with the offset reader. For Go +1.8 the - // body is only wrapped if it is no empty. Otherwise the - // http.NoBody value will be written to the http.Request.Body - // field. - resetBody(r) + r.safeBody = newOffsetReader(r.Body, r.BodyStart) + + // 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. + 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, io.SeekCurrent) + if err != nil { + return 0, err + } + + endOffset, err := r.Seek(0, io.SeekEnd) + if err != nil { + return 0, err + } + + _, err = r.Seek(curOffset, io.SeekStart) + if err != nil { + return 0, err + } + + return endOffset - curOffset, nil } // GetBody will return an io.ReadSeeker of the Request's underlying diff --git a/aws/request/request_1_7.go b/aws/request/request_1_7.go new file mode 100644 index 00000000000..1323af9007b --- /dev/null +++ b/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{} diff --git a/aws/request/request_1_7_test.go b/aws/request/request_1_7_test.go new file mode 100644 index 00000000000..ca6150cb0c0 --- /dev/null +++ b/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) + } +} diff --git a/aws/request/request_1_8.go b/aws/request/request_1_8.go new file mode 100644 index 00000000000..8b963f4de27 --- /dev/null +++ b/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 diff --git a/aws/request/request_1_8_test.go b/aws/request/request_1_8_test.go new file mode 100644 index 00000000000..6a3aba68fa2 --- /dev/null +++ b/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) + } +} diff --git a/aws/request/request_resetbody.go b/aws/request/request_resetbody.go deleted file mode 100644 index 34643095472..00000000000 --- a/aws/request/request_resetbody.go +++ /dev/null @@ -1,76 +0,0 @@ -// +build go1.8 - -package request - -import ( - "io" - "net/http" - - "github.com/aws/aws-sdk-go/aws" - "github.com/aws/aws-sdk-go/aws/awserr" -) - -// 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 -func resetBody(r *Request) { - r.safeBody = newOffsetReader(r.Body, r.BodyStart) - - seekable := false - // Determine if the seeker is actually seekable. ReaderSeekerCloser - // hides the fact that a io.Readers might not actually be seekable. - switch v := r.Body.(type) { - case aws.ReaderSeekerCloser: - seekable = v.IsSeeker() - case *aws.ReaderSeekerCloser: - seekable = v.IsSeeker() - default: - seekable = true - } - - if seekable { - curOffset, err := r.Body.Seek(0, io.SeekCurrent) - if err != nil { - r.Error = awserr.New("SerializationError", "failed to seek request body", err) - return - } - endOffset, err := r.Body.Seek(0, io.SeekEnd) - if err != nil { - r.Error = awserr.New("SerializationError", "failed to seek request body", err) - return - } - _, err = r.Body.Seek(r.BodyStart, io.SeekStart) - if err != nil { - r.Error = awserr.New("SerializationError", "failed to seek request body", err) - return - } - - if endOffset-curOffset == 0 { - r.HTTPRequest.Body = http.NoBody - } else { - 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. - switch r.Operation.HTTPMethod { - case "GET", "HEAD", "DELETE": - r.HTTPRequest.Body = http.NoBody - default: - r.HTTPRequest.Body = r.safeBody - } - } -} diff --git a/aws/request/request_resetbody_1.7.go b/aws/request/request_resetbody_1.7.go deleted file mode 100644 index fa69a8cc4f6..00000000000 --- a/aws/request/request_resetbody_1.7.go +++ /dev/null @@ -1,11 +0,0 @@ -// +build !go1.8 - -package request - -// Pre Go 1.8 funcitonality to always set Request.Body to a value regardless -// if it will actually be used. Go 1.8 removed undocumented feature the SDK -// expected where the request body would only be sent if the length was > 0. -func resetBody(r *Request) { - r.safeBody = newOffsetReader(r.Body, r.BodyStart) - r.HTTPRequest.Body = r.safeBody -} diff --git a/aws/request/request_resetbody_1.7_test.go b/aws/request/request_resetbody_1.7_test.go deleted file mode 100644 index 8f643031442..00000000000 --- a/aws/request/request_resetbody_1.7_test.go +++ /dev/null @@ -1,41 +0,0 @@ -// +build !go1.8 - -package request - -import ( - "net/http" - "strings" - "testing" -) - -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_WithEmptyBody(t *testing.T) { - r := Request{ - HTTPRequest: &http.Request{}, - } - - reader := strings.NewReader("") - 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) - } -} diff --git a/aws/request/request_resetbody_test.go b/aws/request/request_resetbody_test.go index cb92cdeaef9..8202bdd1358 100644 --- a/aws/request/request_resetbody_test.go +++ b/aws/request/request_resetbody_test.go @@ -1,5 +1,3 @@ -// +build go1.8 - package request import ( @@ -23,19 +21,3 @@ func TestResetBody_WithBodyContents(t *testing.T) { r.HTTPRequest.Body) } } - -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) - } -} From 1ef9dd8f60af564f7646a4187e10c4a4cf307045 Mon Sep 17 00:00:00 2001 From: Jason Del Ponte Date: Mon, 12 Dec 2016 15:01:02 -0800 Subject: [PATCH 05/11] Fix usage of io.Seek constances not added until go 1.7 --- aws/request/request.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/aws/request/request.go b/aws/request/request.go index a6273cdc1f2..7550ce3f7d6 100644 --- a/aws/request/request.go +++ b/aws/request/request.go @@ -302,17 +302,17 @@ func computeBodyLength(r io.ReadSeeker) (int64, error) { return -1, nil } - curOffset, err := r.Seek(0, io.SeekCurrent) + curOffset, err := r.Seek(0, 1) if err != nil { return 0, err } - endOffset, err := r.Seek(0, io.SeekEnd) + endOffset, err := r.Seek(0, 2) if err != nil { return 0, err } - _, err = r.Seek(curOffset, io.SeekStart) + _, err = r.Seek(curOffset, 0) if err != nil { return 0, err } From 0a88972c3c60fbda1325db2009e0535cfa58baf9 Mon Sep 17 00:00:00 2001 From: Jason Del Ponte Date: Mon, 12 Dec 2016 17:24:32 -0800 Subject: [PATCH 06/11] ensure the request body is in a good state after signing --- aws/signer/v4/v4.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/aws/signer/v4/v4.go b/aws/signer/v4/v4.go index 90fe1ffaf70..d368dbef11e 100644 --- a/aws/signer/v4/v4.go +++ b/aws/signer/v4/v4.go @@ -432,6 +432,9 @@ func signSDKRequestWithCurrTime(req *request.Request, curTimeFn func() time.Time return } + // Reset the body so that it will be in a good state after signing. + req.ResetBody() + req.SignedHeaderVals = signedHeaders req.LastSignedAt = curTimeFn() } From ebae57f4d8d94509bfef4ca17a753a2e1a0d9121 Mon Sep 17 00:00:00 2001 From: Jason Del Ponte Date: Tue, 13 Dec 2016 10:27:29 -0800 Subject: [PATCH 07/11] Add tests for signer option to not replace body --- aws/signer/v4/v4.go | 19 ++++++++++++++---- aws/signer/v4/v4_test.go | 43 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 4 deletions(-) diff --git a/aws/signer/v4/v4.go b/aws/signer/v4/v4.go index d368dbef11e..b997ec0a88f 100644 --- a/aws/signer/v4/v4.go +++ b/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 @@ -432,9 +446,6 @@ func signSDKRequestWithCurrTime(req *request.Request, curTimeFn func() time.Time return } - // Reset the body so that it will be in a good state after signing. - req.ResetBody() - req.SignedHeaderVals = signedHeaders req.LastSignedAt = curTimeFn() } diff --git a/aws/signer/v4/v4_test.go b/aws/signer/v4/v4_test.go index b225a86e23d..72763fd091c 100644 --- a/aws/signer/v4/v4_test.go +++ b/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", "{}") From 52df8ab0d30531841ba6728f0b60fb527f74c524 Mon Sep 17 00:00:00 2001 From: Jason Del Ponte Date: Tue, 13 Dec 2016 10:57:05 -0800 Subject: [PATCH 08/11] Add test for exlucde body by method --- aws/request/request.go | 3 +++ aws/request/request_resetbody_test.go | 35 +++++++++++++++++++++++++++ aws/types.go | 8 ++++-- 3 files changed, 44 insertions(+), 2 deletions(-) diff --git a/aws/request/request.go b/aws/request/request.go index 7550ce3f7d6..306b23c4ec0 100644 --- a/aws/request/request.go +++ b/aws/request/request.go @@ -275,6 +275,9 @@ func (r *Request) ResetBody() { // 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 diff --git a/aws/request/request_resetbody_test.go b/aws/request/request_resetbody_test.go index 8202bdd1358..4efc5057d10 100644 --- a/aws/request/request_resetbody_test.go +++ b/aws/request/request_resetbody_test.go @@ -1,9 +1,12 @@ package request import ( + "bytes" "net/http" "strings" "testing" + + "github.com/aws/aws-sdk-go/aws" ) func TestResetBody_WithBodyContents(t *testing.T) { @@ -21,3 +24,35 @@ func TestResetBody_WithBodyContents(t *testing.T) { 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) + } + } + +} diff --git a/aws/types.go b/aws/types.go index f0729ed0e5b..9ca685e9040 100644 --- a/aws/types.go +++ b/aws/types.go @@ -5,9 +5,13 @@ import ( "sync" ) -// ReadSeekCloser wraps a io.Reader returning a ReaderSeekerCloser +// ReadSeekCloser wraps a io.Reader returning a ReaderSeekerCloser. Should +// only be used with an io.Reader that is also an io.Seeker. Doing so may +// cause request signature errors, or request body's not sent for GET, HEAD +// and DELETE HTTP methods. // -// Deprecated: Masking reader without valid seeker hides errors. If using for S3 PutObject use s3manager.Uploader instead. +// Deprecated: Should only be used with io.ReadSeeker. If using for +// S3 PutObject to stream content use s3manager.Uploader instead. func ReadSeekCloser(r io.Reader) ReaderSeekerCloser { return ReaderSeekerCloser{r} } From c9f262e048cb7db23b10db6fbdc7574cc56c9ef7 Mon Sep 17 00:00:00 2001 From: Jason Del Ponte Date: Tue, 13 Dec 2016 13:11:52 -0800 Subject: [PATCH 09/11] Add lifecycle test --- aws/request/request_test.go | 48 +++++++++++++++++++++++++++++++++++++ awstesting/client.go | 4 ++++ 2 files changed, 52 insertions(+) diff --git a/aws/request/request_test.go b/aws/request/request_test.go index 16bdd615937..e93c2cb4ab2 100644 --- a/aws/request/request_test.go +++ b/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,48 @@ func TestRequestRecoverTimeoutWithNilResponse(t *testing.T) { assert.Equal(t, 1, int(r.RetryCount)) assert.Equal(t, "valid", out.Data) } + +func TestRequest_NoBody(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if v, ok := r.Header[http.CanonicalHeaderKey("Transfer-Encoding")]; ok { + t.Errorf("expect no body sent with Transfer-Encoding, %q", v) + } + + outMsg := []byte(`{"Value": "abc"}`) + + w.Header().Set("Content-Length", strconv.Itoa(len(outMsg))) + if _, err := w.Write(outMsg); err != nil { + t.Fatalf("expect no error writing server response, got %v", 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: "GET", HTTPPath: "/{bucket}/{key+}", + }, &in, &out) + + if err := r.Send(); err != nil { + t.Fatalf("expect no error sending request, got %v", err) + } +} diff --git a/awstesting/client.go b/awstesting/client.go index ca64a447815..539639d8bfb 100644 --- a/awstesting/client.go +++ b/awstesting/client.go @@ -16,5 +16,9 @@ func NewClient(cfgs ...*aws.Config) *client.Client { def := defaults.Get() def.Config.MergeIn(cfgs...) + if v := aws.StringValue(def.Config.Endpoint); len(v) > 0 { + info.Endpoint = v + } + return client.New(*def.Config, info, def.Handlers) } From d18e22607a9d2cf2834c8d50c4cc44bdcd4ac9d6 Mon Sep 17 00:00:00 2001 From: Jason Del Ponte Date: Tue, 13 Dec 2016 14:01:42 -0800 Subject: [PATCH 10/11] Add round trip test for each request method --- aws/request/request_test.go | 85 +++++++++++++++++++++---------------- 1 file changed, 49 insertions(+), 36 deletions(-) diff --git a/aws/request/request_test.go b/aws/request/request_test.go index e93c2cb4ab2..d047d70572c 100644 --- a/aws/request/request_test.go +++ b/aws/request/request_test.go @@ -383,46 +383,59 @@ func TestRequestRecoverTimeoutWithNilResponse(t *testing.T) { } func TestRequest_NoBody(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if v, ok := r.Header[http.CanonicalHeaderKey("Transfer-Encoding")]; ok { - t.Errorf("expect no body sent with Transfer-Encoding, %q", v) - } - - outMsg := []byte(`{"Value": "abc"}`) + cases := []string{ + "GET", "HEAD", "DELETE", + "PUT", "POST", "PATCH", + } - w.Header().Set("Content-Length", strconv.Itoa(len(outMsg))) - if _, err := w.Write(outMsg); err != nil { - t.Fatalf("expect no error writing server response, got %v", err) + 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"), } - })) - 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 + }{} - out := struct { - Value *string - }{} + r := s.NewRequest(&request.Operation{ + Name: "OpName", HTTPMethod: c, HTTPPath: "/{bucket}/{key+}", + }, &in, &out) - r := s.NewRequest(&request.Operation{ - Name: "OpName", HTTPMethod: "GET", HTTPPath: "/{bucket}/{key+}", - }, &in, &out) - - if err := r.Send(); err != nil { - t.Fatalf("expect no error sending request, got %v", err) + if err := r.Send(); err != nil { + t.Fatalf("%d, expect no error sending request, got %v", i, err) + } } } From 9113cb5046743d30515ac8f3fe91709cfa5e54d7 Mon Sep 17 00:00:00 2001 From: Jason Del Ponte Date: Tue, 13 Dec 2016 14:19:50 -0800 Subject: [PATCH 11/11] Fixup signer disable overwrite body condition --- aws/signer/v4/v4.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aws/signer/v4/v4.go b/aws/signer/v4/v4.go index b997ec0a88f..17738786322 100644 --- a/aws/signer/v4/v4.go +++ b/aws/signer/v4/v4.go @@ -331,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 !v4.DisableRequestBodyOverwrite && !ctx.isPresign { + if !(v4.DisableRequestBodyOverwrite || ctx.isPresign) { var reader io.ReadCloser if body != nil { var ok bool