Skip to content

Commit db7b699

Browse files
jasdeljasdel
authored
aws/request: Fix for Go 1.8 request incorrectly sent with body (#991)
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 added 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. This change also deprecates the aws.ReaderSeekerCloser type. This type has a bug in its design that hides the fact the underlying reader is not also a seeker. This obfuscation, leads to unexpected bugs. If using this type for operations such as S3.PutObject it is suggested to use s3manager.Upload instead. The s3manager.Upload takes an io.Reader to make streaming to S3 easier. Related golang/go#18257 Fix #984
1 parent d5fd698 commit db7b699

File tree

15 files changed

+363
-12
lines changed

15 files changed

+363
-12
lines changed

‎aws/request/request.go

Lines changed: 76 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -246,7 +246,82 @@ func (r *Request) ResetBody() {
246246
}
247247

248248
r.safeBody = newOffsetReader(r.Body, r.BodyStart)
249-
r.HTTPRequest.Body = r.safeBody
249+
250+
// Go 1.8 tightened and clarified the rules code needs to use when building
251+
// requests with the http package. Go 1.8 removed the automatic detection
252+
// of if the Request.Body was empty, or actually had bytes in it. The SDK
253+
// always sets the Request.Body even if it is empty and should not actually
254+
// be sent. This is incorrect.
255+
//
256+
// Go 1.8 did add a http.NoBody value that the SDK can use to tell the http
257+
// client that the request really should be sent without a body. The
258+
// Request.Body cannot be set to nil, which is preferable, because the
259+
// field is exported and could introduce nil pointer dereferences for users
260+
// of the SDK if they used that field.
261+
//
262+
// Related golang/go#18257
263+
l, err := computeBodyLength(r.Body)
264+
if err != nil {
265+
r.Error = awserr.New("SerializationError", "failed to compute request body size", err)
266+
return
267+
}
268+
269+
if l == 0 {
270+
r.HTTPRequest.Body = noBodyReader
271+
} else if l > 0 {
272+
r.HTTPRequest.Body = r.safeBody
273+
} else {
274+
// Hack to prevent sending bodies for methods where the body
275+
// should be ignored by the server. Sending bodies on these
276+
// methods without an associated ContentLength will cause the
277+
// request to socket timeout because the server does not handle
278+
// Transfer-Encoding: chunked bodies for these methods.
279+
//
280+
// This would only happen if a aws.ReaderSeekerCloser was used with
281+
// a io.Reader that was not also an io.Seeker.
282+
switch r.Operation.HTTPMethod {
283+
case "GET", "HEAD", "DELETE":
284+
r.HTTPRequest.Body = noBodyReader
285+
default:
286+
r.HTTPRequest.Body = r.safeBody
287+
}
288+
}
289+
}
290+
291+
// Attempts to compute the length of the body of the reader using the
292+
// io.Seeker interface. If the value is not seekable because of being
293+
// a ReaderSeekerCloser without an unerlying Seeker -1 will be returned.
294+
// If no error occurs the length of the body will be returned.
295+
func computeBodyLength(r io.ReadSeeker) (int64, error) {
296+
seekable := true
297+
// Determine if the seeker is actually seekable. ReaderSeekerCloser
298+
// hides the fact that a io.Readers might not actually be seekable.
299+
switch v := r.(type) {
300+
case aws.ReaderSeekerCloser:
301+
seekable = v.IsSeeker()
302+
case *aws.ReaderSeekerCloser:
303+
seekable = v.IsSeeker()
304+
}
305+
if !seekable {
306+
return -1, nil
307+
}
308+
309+
curOffset, err := r.Seek(0, 1)
310+
if err != nil {
311+
return 0, err
312+
}
313+
314+
endOffset, err := r.Seek(0, 2)
315+
if err != nil {
316+
return 0, err
317+
}
318+
319+
_, err = r.Seek(curOffset, 0)
320+
if err != nil {
321+
return 0, err
322+
}
323+
324+
return endOffset - curOffset, nil
250325
}
251326

252327
// GetBody will return an io.ReadSeeker of the Request's underlying

‎aws/request/request_1_7.go

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
// +build !go1.8
2+
3+
package request
4+
5+
import "io"
6+
7+
// NoBody is an io.ReadCloser with no bytes. Read always returns EOF
8+
// and Close always returns nil. It can be used in an outgoing client
9+
// request to explicitly signal that a request has zero bytes.
10+
// An alternative, however, is to simply set Request.Body to nil.
11+
//
12+
// Copy of Go 1.8 NoBody type from net/http/http.go
13+
type noBody struct{}
14+
15+
func (noBody) Read([]byte) (int, error) { return 0, io.EOF }
16+
func (noBody) Close() error { return nil }
17+
func (noBody) WriteTo(io.Writer) (int64, error) { return 0, nil }
18+
19+
// Is an empty reader that will trigger the Go HTTP client to not include
20+
// and body in the HTTP request.
21+
var noBodyReader = noBody{}

‎aws/request/request_1_7_test.go

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
// +build !go1.8
2+
3+
package request
4+
5+
import (
6+
"net/http"
7+
"strings"
8+
"testing"
9+
)
10+
11+
func TestResetBody_WithEmptyBody(t *testing.T) {
12+
r := Request{
13+
HTTPRequest: &http.Request{},
14+
}
15+
16+
reader := strings.NewReader("")
17+
r.Body = reader
18+
19+
r.ResetBody()
20+
21+
if a, e := r.HTTPRequest.Body, (noBody{}); a != e {
22+
t.Errorf("expected request body to be set to reader, got %#v", r.HTTPRequest.Body)
23+
}
24+
}

‎aws/request/request_1_8.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
// +build go1.8
2+
3+
package request
4+
5+
import "net/http"
6+
7+
// Is a http.NoBody reader instructing Go HTTP client to not include
8+
// and body in the HTTP request.
9+
var noBodyReader = http.NoBody

‎aws/request/request_1_8_test.go

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
// +build go1.8
2+
3+
package request
4+
5+
import (
6+
"net/http"
7+
"strings"
8+
"testing"
9+
)
10+
11+
func TestResetBody_WithEmptyBody(t *testing.T) {
12+
r := Request{
13+
HTTPRequest: &http.Request{},
14+
}
15+
16+
reader := strings.NewReader("")
17+
r.Body = reader
18+
19+
r.ResetBody()
20+
21+
if a, e := r.HTTPRequest.Body, http.NoBody; a != e {
22+
t.Errorf("expected request body to be set to reader, got %#v",
23+
r.HTTPRequest.Body)
24+
}
25+
}

‎aws/request/request_resetbody_test.go

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
package request
2+
3+
import (
4+
"bytes"
5+
"net/http"
6+
"strings"
7+
"testing"
8+
9+
"github.com/aws/aws-sdk-go/aws"
10+
)
11+
12+
func TestResetBody_WithBodyContents(t *testing.T) {
13+
r := Request{
14+
HTTPRequest: &http.Request{},
15+
}
16+
17+
reader := strings.NewReader("abc")
18+
r.Body = reader
19+
20+
r.ResetBody()
21+
22+
if v, ok := r.HTTPRequest.Body.(*offsetReader); !ok || v == nil {
23+
t.Errorf("expected request body to be set to reader, got %#v",
24+
r.HTTPRequest.Body)
25+
}
26+
}
27+
28+
func TestResetBody_ExcludeUnseekableBodyByMethod(t *testing.T) {
29+
cases := []struct {
30+
Method string
31+
IsNoBody bool
32+
}{
33+
{"GET", true},
34+
{"HEAD", true},
35+
{"DELETE", true},
36+
{"PUT", false},
37+
{"PATCH", false},
38+
{"POST", false},
39+
}
40+
41+
reader := aws.ReadSeekCloser(bytes.NewBuffer([]byte("abc")))
42+
43+
for i, c := range cases {
44+
r := Request{
45+
HTTPRequest: &http.Request{},
46+
Operation: &Operation{
47+
HTTPMethod: c.Method,
48+
},
49+
}
50+
51+
r.SetReaderBody(reader)
52+
53+
if a, e := r.HTTPRequest.Body == noBodyReader, c.IsNoBody; a != e {
54+
t.Errorf("%d, expect body to be set to noBody(%t), but was %t", i, e, a)
55+
}
56+
}
57+
58+
}

‎aws/request/request_test.go

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,9 @@ import (
77
"io"
88
"io/ioutil"
99
"net/http"
10+
"net/http/httptest"
1011
"runtime"
12+
"strconv"
1113
"testing"
1214
"time"
1315

@@ -18,6 +20,7 @@ import (
1820
"github.com/aws/aws-sdk-go/aws/credentials"
1921
"github.com/aws/aws-sdk-go/aws/request"
2022
"github.com/aws/aws-sdk-go/awstesting"
23+
"github.com/aws/aws-sdk-go/private/protocol/rest"
2124
)
2225

2326
type testData struct {
@@ -377,3 +380,61 @@ func TestRequestRecoverTimeoutWithNilResponse(t *testing.T) {
377380
assert.Equal(t, 1, int(r.RetryCount))
378381
assert.Equal(t, "valid", out.Data)
379382
}
383+
384+
func TestRequest_NoBody(t *testing.T) {
385+
cases := []string{
386+
"GET", "HEAD", "DELETE",
387+
"PUT", "POST", "PATCH",
388+
}
389+
390+
for i, c := range cases {
391+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
392+
if v := r.TransferEncoding; len(v) > 0 {
393+
t.Errorf("%d, expect no body sent with Transfer-Encoding, %v", i, v)
394+
}
395+
396+
outMsg := []byte(`{"Value": "abc"}`)
397+
398+
if b, err := ioutil.ReadAll(r.Body); err != nil {
399+
t.Fatalf("%d, expect no error reading request body, got %v", i, err)
400+
} else if n := len(b); n > 0 {
401+
t.Errorf("%d, expect no request body, got %d bytes", i, n)
402+
}
403+
404+
w.Header().Set("Content-Length", strconv.Itoa(len(outMsg)))
405+
if _, err := w.Write(outMsg); err != nil {
406+
t.Fatalf("%d, expect no error writing server response, got %v", i, err)
407+
}
408+
}))
409+
410+
s := awstesting.NewClient(&aws.Config{
411+
Region: aws.String("mock-region"),
412+
MaxRetries: aws.Int(0),
413+
Endpoint: aws.String(server.URL),
414+
DisableSSL: aws.Bool(true),
415+
})
416+
s.Handlers.Build.PushBack(rest.Build)
417+
s.Handlers.Validate.Clear()
418+
s.Handlers.Unmarshal.PushBack(unmarshal)
419+
s.Handlers.UnmarshalError.PushBack(unmarshalError)
420+
421+
in := struct {
422+
Bucket *string `location:"uri" locationName:"bucket"`
423+
Key *string `location:"uri" locationName:"key"`
424+
}{
425+
Bucket: aws.String("mybucket"), Key: aws.String("myKey"),
426+
}
427+
428+
out := struct {
429+
Value *string
430+
}{}
431+
432+
r := s.NewRequest(&request.Operation{
433+
Name: "OpName", HTTPMethod: c, HTTPPath: "/{bucket}/{key+}",
434+
}, &in, &out)
435+
436+
if err := r.Send(); err != nil {
437+
t.Fatalf("%d, expect no error sending request, got %v", i, err)
438+
}
439+
}
440+
}

‎aws/signer/v4/v4.go

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,16 @@ type Signer struct {
171171
// http://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html
172172
DisableURIPathEscaping bool
173173

174+
// Disales the automatical setting of the HTTP request's Body field with the
175+
// io.ReadSeeker passed in to the signer. This is useful if you're using a
176+
// custom wrapper around the body for the io.ReadSeeker and want to preserve
177+
// the Body value on the Request.Body.
178+
//
179+
// This does run the risk of signing a request with a body that will not be
180+
// sent in the request. Need to ensure that the underlying data of the Body
181+
// values are the same.
182+
DisableRequestBodyOverwrite bool
183+
174184
// currentTimeFn returns the time value which represents the current time.
175185
// This value should only be used for testing. If it is nil the default
176186
// time.Now will be used.
@@ -321,7 +331,7 @@ func (v4 Signer) signWithBody(r *http.Request, body io.ReadSeeker, service, regi
321331
// If the request is not presigned the body should be attached to it. This
322332
// prevents the confusion of wanting to send a signed request without
323333
// the body the request was signed for attached.
324-
if !ctx.isPresign {
334+
if !(v4.DisableRequestBodyOverwrite || ctx.isPresign) {
325335
var reader io.ReadCloser
326336
if body != nil {
327337
var ok bool
@@ -416,6 +426,10 @@ func signSDKRequestWithCurrTime(req *request.Request, curTimeFn func() time.Time
416426
// S3 service should not have any escaping applied
417427
v4.DisableURIPathEscaping = true
418428
}
429+
// Prevents setting the HTTPRequest's Body. Since the Body could be
430+
// wrapped in a custom io.Closer that we do not want to be stompped
431+
// on top of by the signer.
432+
v4.DisableRequestBodyOverwrite = true
419433
})
420434

421435
signingTime := req.Time

‎aws/signer/v4/v4_test.go

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -424,6 +424,49 @@ func TestBuildCanonicalRequest(t *testing.T) {
424424
assert.Equal(t, expected, ctx.Request.URL.String())
425425
}
426426

427+
func TestSignWithBody_ReplaceRequestBody(t *testing.T) {
428+
creds := credentials.NewStaticCredentials("AKID", "SECRET", "SESSION")
429+
req, seekerBody := buildRequest("dynamodb", "us-east-1", "{}")
430+
req.Body = ioutil.NopCloser(bytes.NewReader([]byte{}))
431+
432+
s := NewSigner(creds)
433+
origBody := req.Body
434+
435+
_, err := s.Sign(req, seekerBody, "dynamodb", "us-east-1", time.Now())
436+
if err != nil {
437+
t.Fatalf("expect no error, got %v", err)
438+
}
439+
440+
if req.Body == origBody {
441+
t.Errorf("expeect request body to not be origBody")
442+
}
443+
444+
if req.Body == nil {
445+
t.Errorf("expect request body to be changed but was nil")
446+
}
447+
}
448+
449+
func TestSignWithBody_NoReplaceRequestBody(t *testing.T) {
450+
creds := credentials.NewStaticCredentials("AKID", "SECRET", "SESSION")
451+
req, seekerBody := buildRequest("dynamodb", "us-east-1", "{}")
452+
req.Body = ioutil.NopCloser(bytes.NewReader([]byte{}))
453+
454+
s := NewSigner(creds, func(signer *Signer) {
455+
signer.DisableRequestBodyOverwrite = true
456+
})
457+
458+
origBody := req.Body
459+
460+
_, err := s.Sign(req, seekerBody, "dynamodb", "us-east-1", time.Now())
461+
if err != nil {
462+
t.Fatalf("expect no error, got %v", err)
463+
}
464+
465+
if req.Body != origBody {
466+
t.Errorf("expeect request body to not be chagned")
467+
}
468+
}
469+
427470
func BenchmarkPresignRequest(b *testing.B) {
428471
signer := buildSigner()
429472
req, body := buildRequest("dynamodb", "us-east-1", "{}")

0 commit comments

Comments
 (0)