Sign request using AWS v4 signature #74
Conversation
| } else { | ||
| body, _ := ioutil.ReadAll(s.Request.Body) | ||
| s.Request.Body = ioutil.NopCloser(bytes.NewReader(body)) | ||
| hash = hex.EncodeToString(sha(body)) |
There was a problem hiding this comment.
This is fairly inefficient, but could be avoided if the X-Amz-Content-Sha256 header was pushed earlier in the process. Replacing MD5 for SHA256 in putter.go and getter.go would probably make that easier, but that's another piece of work altogether.
There was a problem hiding this comment.
You're right, this is a ton of copies and will significantly increase memory usage for large transfers.
As you suggested, the X-Amz-Content-Sha256 header can be calculated in putter and getter when the body is seekable.
I'd like to get that change in before merging this to avoid a huge efficiency regression.
|
Thanks for doing this @cyberdelia . I'll review this soon, hopefully in the next few days. |
|
@cyberdelia Other than the issue with copying the body, this looks good. Is that something you can take a look at? |
|
@rlmcpherson I added pre-calculation for sha256 hash when possible or needed. I also slipped some whitespace syntax changes, as before I can removed them. |
|
@cyberdelia Thanks. The changes look fine to me, including the uploadID etc. I’m concerned about the increase in memory usage that the new signing brings. In my tests streaming a large file with default part size and concurrency settings, the memory usage is up from < 250 MB to > 700 and growing more quickly. Is there anything we can do to reduce the impact? I realize it will be impossible to match the existing usage but I’d like to mitigate it at least somewhat. |
|
@rlmcpherson I did some memory profiling, and added some benchmarks functions, and I don't see any memory changes due to the new signing. Feel free to tweak or add benchmark functions to illustrate what you're seeing. |
|
@cyberdelia Sorry for the delay in retesting this. You're right, I retested with your benchmarks, some larger benchmark sizes, as well as running the I'm not sure what caused the results I was seeing before. I may have been testing with an old binary. This all looks good though. Merging! |
Sign request using AWS v4 signature
|
✨ ✨ ✨ |
Use AWS v4 signature for signing requests, this is a port of aws go logic exclusively for s3 and adapted to work against
http.Request. It will also detect region based on s3 domain or using theAWS_REGIONenvironment variable.P.S: I can move out whitespace and other small changes out of this PR if preferred.