Fully implement AWS4 signing support #1
Conversation
| signer := aws.NewV4Signer(as.Auth, "autoscaling", as.Region) | ||
| signer.Sign(hreq) | ||
| err = aws.SignV4(hreq, as.Auth, "autoscaling", as.Region.Name) | ||
| if err != nil { |
There was a problem hiding this comment.
Not going to use if err =... ; err != nil - what's the benefit of doing it this way if you're returning immediately?
stevenh
force-pushed
the
s3-aws4
branch
2 times, most recently
from
59839ce
to
944d07a
Compare
|
@leepa issues addressed and did a few more http status constants that I spotted while there. |
|
Seems sane - I assume tests pass and if they do 👍 |
| } | ||
| reqSignpathSpaceFix := (&url.URL{Path: signpath}).String() | ||
| req.headers["Host"] = []string{u.Host} | ||
| req.headers["Date"] = []string{time.Now().In(time.UTC).Format(time.RFC1123)} | ||
| if s3.Auth.Token() != "" { |
There was a problem hiding this comment.
This header shouldnt be removed.
stevenh
force-pushed
the
s3-aws4
branch
2 times, most recently
from
209d1b5
to
cadc75f
Compare
|
Being such a large change, it's hard to spot anything, but looks fine and the rationale in the commit message makes sense. Hopefully upstream will accept this, though breaking API compatibility is not going to be an easy pill to swallow I imagine. 👍 review done |
| } | ||
| s = &Service{service: service, signer: signer} | ||
| return | ||
| // Create a new AWS service to handle making requests |
There was a problem hiding this comment.
A couple of places we don't follow the official good convention for commenting functions and in others we do. Small nit pick. http://blog.golang.org/godoc-documenting-go-code
There was a problem hiding this comment.
The reason for this is not our code, the methods I've added should be commented, updated old ones not.
|
Aside from the above two comments, review done |
This fully implemeents AWS4 signing support, which was only partially added by 5d9165. This adds the Sign field to Region, which can be used to sign a request with default sign method for that region. In addition we have a single method that implements signing of requests for each supported signing method: * SignV2 * SignV4 * SignRoute53 Fix V4Signer adding headers to the request after calculating the canonicalHeaders, due to this change all the tests have had to be updated with corrected checksums and headers. Unfortunately it was not possible to make this change 100% backwards compatible. The following changes are API incompatible and hence may require users to update their code: * Removes Signature constants, as they are not used any more * Removes ServiceInfo type, as its not used any more * Removes Signer interface, as its not compatible with all signing methods. * Changes the type of Region.RDSEndpoint from ServiceInfo -> string * Renames Region.CloudWatchServicepoint -> CloudWatchEndpoint and changes its type from ServiceInfo -> string. * Changes NewService from (auth Auth, serviceInfo ServiceInfo) -> (auth Auth, endpoint string, region Region). * Changes NewV2Signer from (auth Auth, service ServiceInfo) -> (auth Auth, endpoint string) * Changes NewCloudWatch from (auth aws.Auth, region aws.ServiceInfo) -> NewCloudWatch(auth aws.Auth, region aws.Region) * Changes CanonicalRequest return from string to string, error Also switch to using http status code constants while I'm here to ease comprehension.
Fully implement AWS4 signing support
Fully implement AWS4 signing support
This fully implemeents AWS4 signing support, which was only partially added by 5d9165.
This adds the Sign field to Region, which can be used to sign a request with default sign method for that region.
In addition we have a single method that implements signing of requests for each supported signing method:
Fix V4Signer adding headers to the request after calculating the canonicalHeaders, due to this change all the tests have had to be updated with corrected checksums and headers.
Unfortunately it was not possible to make this change 100% backwards compatible.
The following changes are API incompatible and hence may require users to update their code:
Also switch to using http status code constants while I'm here to ease comprehension.