/amz

Added support for v4 signing to S3. #27

Merged
merged 3 commits into from Mar 1, 2015

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@kat-co @dimitern
@kat-co
Member

kat-co commented Feb 11, 2015

  • S3 constructor now takes a Signer function. This indicates the preferred signing methodology.
  • Users can still use the recommended type of signing by using the Signer defined on the region.
@kat-co
Member

kat-co commented Feb 11, 2015

Most of this PR is switching the S3 requests over to vanilla http.Request structures. This was done because the new AWS signing operates on these.
@kat-co
Added support for v4 signing to S3. 
- S3 constructor now takes a Signer function. This indicates the preferred signing methodology.
- Users can still use the recommended type of signing by using the Signer defined on the region.
9dd0a80
aws/aws.go
+ if r.S3BucketEndpoint != "" {
+ return strings.Replace(r.S3BucketEndpoint, "${bucket}", bucketName, -1)
+ }
+ return r.S3Endpoint + "/" + bucketName + "/"
@dimitern

dimitern Feb 13, 2015

Member

path.Join here instead?

@kat-co

kat-co Feb 23, 2015

Member

path.Join scrubs the content and will remove one of the slashes in "http://" in the r.S3Endpoint.

s3/multi.go
+ req.URL.RawQuery = query.Encode()
+
+ for attempt := attempts.Start(); attempt.Next(); attempt = attempts.Start() {
+
@dimitern

dimitern Feb 13, 2015

Member

d?

@kat-co

kat-co Feb 23, 2015

Member

Present for readability.

s3/multi.go
}
+
@dimitern

dimitern Feb 13, 2015

Member

d

@kat-co

kat-co Feb 23, 2015

Member

Present for readability.

s3/multi.go
+ query := req.URL.Query()
+ query.Add("upload", "")
+ req.URL.RawQuery = query.Encode()
+
@dimitern

dimitern Feb 13, 2015

Member

d?

@kat-co

kat-co Feb 23, 2015

Member

Present for readability.

s3/s3i_test.go
- _, err := b.Get("non-existent")
+ s := s3.New(s.s3.Auth, r, aws.SignV2)
+ b, err := s.Bucket("goamz-" + s.Auth.AccessKey)
+ c.Assert(err, IsNil)
@dimitern

dimitern Feb 13, 2015

Member

I'd prefer not to use c.Assert in a loop like this, so that a single failure won't stop the test. How about:

if !c.Check(err, IsNil) {
  errs <- err
}
_, err = b.Get("non-existent")
@kat-co

kat-co Feb 23, 2015

Member

Good suggestion, I've implemented as you have it laid out.

s3/s3_test.go
- s.s3 = s3.New(auth, aws.Region{Name: "faux-region-1", S3Endpoint: testServer.URL, Sign: aws.SignV2})
+ s.s3 = s3.New(
+ aws.Auth{"abc", "123"},
+ aws.Region{Name: "faux-region-1", S3Endpoint: testServer.URL, Sign: aws.SignV2},
@dimitern

dimitern Feb 13, 2015

Member

Split aws.Region fields in multiple lines, as it's getting a bit too long.

@kat-co

kat-co Feb 23, 2015

Member

Done

s3/s3.go
+ return resp, nil
+ }
+
+ panic("could not complete the request within the specified retry attempts")
@dimitern

dimitern Feb 13, 2015

Member

Why not just return an error here? This is production code and I'd rather handle the error outside than to crash the whole process.

@kat-co

kat-co Feb 23, 2015

Member

This was the pattern established when I began working on the code, but you're correct. I've switched it to an error.

s3/s3.go
-// URL returns a non-signed URL that allows retriving the
@dimitern

dimitern Feb 13, 2015

Member

So we're dropping URL and SignedURL methods entirely? This is not backwards-compatible and will break existing client. Is it possible to leave these two methods in place and somehow simulate what they do with the new logic?

@kat-co

kat-co Feb 23, 2015

Member

As we discussed:

  • This is proposed against v3-unstable, so it will not break anyone.
  • SignedURL no longer makes sense as an option since v4 utilizes headers for signing.

I have added the URL function back in.

@dimitern
Member

dimitern commented Feb 13, 2015

Most of this looks good, however dropping the 2 methods I can't quite get. Also, have you tried running all the s3 tests with -amazon to make sure both the s3test server still works (I'm assuming tests pass, otherwise travis CI would've reported it) and the changes work against live AWS and s3test the same way?
@dimitern
Member

dimitern commented Feb 13, 2015

Sorry, I've seen this is actually targeted to v3-unstable, so changes to the interface is fine at this point. However, I still think URL is worth keeping as a method. I'd like to have another look when you're done with the tests.
aws/sign.go
+ if req.Body != nil {
+ if b, err := ioutil.ReadAll(req.Body); err != nil {
+ return "", err
+ } else {
@dimitern

dimitern Mar 1, 2015

Member

I'd pull b, err := ioutil.ReadAll(..) before the if and drop the else clause to reduce the indentation level.

aws/sign.go
+ } else {
+ req.Body = ioutil.NopCloser(bytes.NewBuffer(b))
+ return hasher(b), nil
+ }
} else {
@dimitern

dimitern Mar 1, 2015

Member

Drop the else and just return hasher([]byte("")), nil

aws/sign.go
- sort.Strings(sortedHdrVals)
- hdrVals := strings.Join(sortedHdrVals, ",")
+
+ var hdrVals string
@dimitern

dimitern Mar 1, 2015

Member

How about:

hdrVals := host
if hName != "host" {
    canonHdrKey := http.CanonicalHeaderKey(hName)
    sortedHdrVals := hdr[canonHdrKey]
    sort.Strings(sortedHdrVals)
    hdrVals = strings.Join(sortedHdrVals, ",")
}
aws/sign_test.go
)
var _ = Suite(&SigningSuite{})
type SigningSuite struct{}
+const v4skipReason = `"The signing methodology is a "one size fits all" approach. The hashes we check against don't include headers that are added in as requisite parts for S3. That doesn't mean the tests are invalid, or that signing is broken for these examples, but as long as we're adding heads in, it's impossible to know what the new signature should be. We should revaluate these later.`
@dimitern

dimitern Mar 1, 2015

Member

How about:

// TODO(katco-): The signing methodology is a "one size fits all" approach.
// The hashes we check against against don't include headers that are added
// in as requisite parts for S3. That doesn't mean the tests are invalid, or that
// signing is broken for these examples, but as long as we're adding heads in,
// it's impossible to know what the new signature should be. We should reevaluate
// these later. See issue #XYZ.
const v4SkipReason = `Extra headers present - cannot predict generated signature (issue #XYZ)`

This will show nicely as we're running tests (i.e. not splitting the output on multiple lines), but also gives the reader of this source enough context to understand the issue. Please, file an issue for this and replace the #XYZ with the issue number.

@dimitern

dimitern Mar 1, 2015

Member

As for the impossibility of knowing what the hash could be, I guess this is because the hash changes because of the current timestamp (or reqTime). This could be faked for the test servers - setting a fixed time of a request/response. But that's a future improvement. As long as you've actually manually tested the scenarios in the skipped tests I think it's fine.

exp/sdb/sdb_test.go
@@ -25,7 +25,7 @@ var testServer = testutil.NewHTTPServer()
func (s *S) SetUpSuite(c *C) {
testServer.Start()
auth := aws.Auth{"abc", "123"}
- s.sdb = sdb.New(auth, aws.Region{SDBEndpoint: testServer.URL, Sign: aws.SignV2})
@dimitern

dimitern Mar 1, 2015

Member

It's great you took the time to actually fix the exp/ packages as well. However, they're barely working and are not maintained, so if there are test failures here I don't really care.

s3/multi_test.go
multi, err := b.InitMulti("multi", "text/plain", s3.Private)
c.Assert(err, IsNil)
parts, err := multi.PutAll(strings.NewReader("part1part2last"), 5)
- c.Assert(parts, HasLen, 3)
@dimitern

dimitern Mar 1, 2015

Member

Good catch!

s3/s3.go
}
- panic("unreachable")
+ return resp.Body, nil
}
// Put inserts an object into the S3 bucket.
@dimitern

dimitern Mar 1, 2015

Member

s/Put/PutReader/

@dimitern

dimitern Mar 1, 2015

Member

Or actually, just drop the Put doc comment and body only. rather than PutReader's doc comment as well.

s3/s3.go
-}
-
-// PutReader inserts an object into the S3 bucket by consuming data
-// from r until EOF.
func (b *Bucket) PutReader(path string, r io.Reader, length int64, contType string, perm ACL) error {
return b.PutReaderWithHeader(path, r, length, contType, perm, http.Header{})
}
// PutReader inserts an object into the S3 bucket by consuming data
@dimitern

dimitern Mar 1, 2015

Member

s/PutReader/PutReaderWithHeader/

@dimitern

dimitern Mar 1, 2015

Member

Also, restore the two lines below in the doc comment - ... The HTTP PUT request will have...

s3/s3_test.go
@@ -96,27 +104,11 @@ func (s *S) TestGet(c *C) {
c.Assert(string(data), Equals, "content")
}
-func (s *S) TestURL(c *C) {
@dimitern

dimitern Mar 1, 2015

Member

Since URL() is back, should this test stay as well?

@dimitern
Member

dimitern commented Mar 1, 2015

All in all it looks fine, but I'm getting some s3 test failures in both local and live tests. I've sent you a mail with more details.
@dimitern
Member

dimitern commented Mar 1, 2015

LGTM with the patch discussed on IRC: http://pastebin.ubuntu.com/10490009/ which makes s3 live tests to pass reliably. However, a side-effect of that is leaving a bunch of buckets after each live test run - please, add an issue for this as well. When we have time, we'll fix it.
@kat-co
Addressed review issues. 
- Minor formatting issues.
- Changed skip message to a comment. Now referencing bug#.
- Documentation cleaned up.
6f9db5d
@dimitern
Member

dimitern commented Mar 1, 2015

Great, I'm merging this.

dimitern pushed a commit that referenced this pull request Mar 1, 2015

Merge pull request #27 from katco-/v3-unstable 
Added support for v4 signing to S3.
07316ca

@dimitern dimitern merged commit 07316ca into go-amz:v3-unstable Mar 1, 2015
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kat-co kat-co deleted the kat-co:v3-unstable branch Mar 13, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment