Use MD5 to sign S3 SigV4 bodies #920

JordonPhillips · 2016-05-13T22:16:44Z

S3 now supports optional SHA256 body signing, so this switches from SHA256 to MD5 to boost speed while reducing CPU and Memory usage when using sigv4 with S3. SHA256 will still be used on systems which do not support using MD5.

cc @kyleknap @jamesls

jamesls · 2016-05-16T21:51:25Z

botocore/auth.py

+        # impact, we opt to not SHA256 sign the body. Instead, we MD5 it.
+        # We will fall back to SHA256 when MD5 is not available, such as with
+        # FIPS systems.
+        if MD5_AVAILABLE:


Can't we just check if there's a Content-MD5 value in the request headers? Seems more definitive.

jamesls · 2016-05-16T21:57:28Z

I think we need to have a config option to enable/disable this. Especially given that users can reject requests without unsigned payloads via IAM policies (http://docs.aws.amazon.com/AmazonS3/latest/API/bucket-policy-s3-sigv4-conditions.html), we need to give them a way to enable this behavior if needed.

I think I still prefer only doing this for the streaming s3 requests (put_object/upload_part) but I'm interested to hear other's thoughts.

JordonPhillips · 2016-05-17T21:51:33Z

I updated to check for the header and added a config flag. Still need to look into making this exclusive to UploadPart and PutObject

JordonPhillips · 2016-06-03T19:18:52Z

Personally, I think we should go for unsigned payloads regardless. It keeps things simple on our side and provides the performance boost consistently. What do you think @kyleknap

kyleknap · 2016-06-03T22:47:09Z

I talked to you about this, but wanted to capture the conversation. Lets only do unsigned payload for PutObject and UploadPart for the following reasons:

The behavior change will only be scoped to PutObject and UploadPart leaving a lot of the control plane operations of S3 untouched.
We have knowledge upfront if the payload is streaming by looking at the operation model when we create the context so we won't be hard coding PutObject and UploadPart into auth signer if we pass that knowledge to the context and we would probably would want to disable body signing for any future S3 operation that has a streaming body for its input.

jamesls · 2016-06-06T23:00:09Z

I'm going to switch this to incorporating-feedback. Seems like this is going to change based on @kyleknap 's feedback so I'll defer reviewing this until those changes are made.

JordonPhillips · 2016-06-10T20:17:45Z

Rebased on top of #936 to take advantage of context plumbing.

JordonPhillips · 2016-06-10T20:38:31Z

Squashed the commits. Only the top commit needs to be reviewed.

kyleknap · 2016-06-10T21:18:18Z

botocore/handlers.py

+    if s3_config is None:
+        s3_config = {}
+    use_sha256 = s3_config.get('sha256_sign_s3v4_payload', False)
+    if request_signer.signature_version == 's3v4' and \


Questions for everyone: if the users opt-in to sha256_sign_s3v4_payload, should we also do MD5 calculation? Based on the docstring you have it is not clear that you turn off MD5 signing.

You're right, we shouldn't disable MD5. Rather, we should enforce SHA256 if that config isn't explicitly set and MD5 isn't present.

JordonPhillips · 2016-06-10T22:02:58Z

Moved the config value checking into auth so that MD5 is not impacted.

JordonPhillips · 2016-06-13T22:54:28Z

Rebased against the latest redirector and tentatively changed the name to payload_signing_enabled

jamesls · 2016-06-14T00:32:32Z

botocore/auth.py

+            s3_config = {}
+
+        sign_payload = s3_config.get('payload_signing_enabled')
+        if sign_payload in [True, 'True', 'true']:


-1 to this. We shouldn't let the lack of typed parsing in the parser/config objects leak into dependent objects. I would prefer we try to normalize this as high up the stack as we can. Likely in the client creator for now.

JordonPhillips · 2016-06-14T19:28:55Z

Updated

jamesls · 2016-06-14T23:36:49Z

Looks good to me. Thanks for incorporating the feedback.

codecov-io · 2016-06-17T23:17:20Z

Current coverage is 97.31%

Merging #920 into develop will increase coverage by 0.04%

@@            develop       #920   diff @@
==========================================
  Files            43         43          
  Lines          6909       7014   +105   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           6721       6826   +105   
  Misses          188        188          
  Partials          0          0

Powered by Codecov. Last updated by 2db54af...d6d7f1e

jamesls · 2016-06-17T23:59:30Z

da6f25fbf541885a243ad90a3ecee3604aefa444 looks good to me.

Knowing the headers can be useful, especially when the service model does not add them to the actual response.

S3 generally provides enough information for us to redirect requests, so this attempts to do so. To prevent a massive number of additional requests, bucket regions are cached. When a redirect occurs, a warning is printed that tells the customer they should use a client configured to the proper region to avoid additional requests.

jamesls · 2016-06-20T16:36:56Z

@JordonPhillips failing build?

kyleknap · 2016-06-20T16:37:53Z

🚢 Thanks for all the work you put into it.

JordonPhillips · 2016-06-20T16:45:59Z

@jamesls it's codecov. What is our official target percent?

S3 now supports optional SHA256 body signing, so this switches from SHA256 to MD5 to boost speed while reducing CPU and Memory usage when using sigv4 with S3. SHA256 will still be used on systems which do not support using MD5.

JordonPhillips force-pushed the md5-sigv4 branch from 2e5be69 to 2be9f69 Compare May 13, 2016 23:30

JordonPhillips added the pr/needs-review This PR needs a review from a member of the team. label May 13, 2016

JordonPhillips force-pushed the md5-sigv4 branch from 2be9f69 to 2f538cf Compare May 13, 2016 23:31

jamesls reviewed May 16, 2016
View reviewed changes

jamesls added incorporating-feedback and removed pr/needs-review This PR needs a review from a member of the team. labels May 16, 2016

JordonPhillips force-pushed the md5-sigv4 branch 3 times, most recently from e052515 to 3cda647 Compare May 17, 2016 21:50

JordonPhillips force-pushed the md5-sigv4 branch from 3cda647 to 2a9ea1e Compare June 3, 2016 19:13

JordonPhillips added pr/needs-review This PR needs a review from a member of the team. and removed incorporating-feedback labels Jun 3, 2016

jamesls added incorporating-feedback and removed pr/needs-review This PR needs a review from a member of the team. labels Jun 6, 2016

JordonPhillips mentioned this pull request Jun 8, 2016

Convert boolean config values to boolean types #940

Closed

JordonPhillips force-pushed the md5-sigv4 branch from 2a9ea1e to 24da5d3 Compare June 10, 2016 20:16

JordonPhillips added pr/needs-review This PR needs a review from a member of the team. and removed incorporating-feedback labels Jun 10, 2016

JordonPhillips force-pushed the md5-sigv4 branch from 0abf4a6 to 5a3a7f1 Compare June 10, 2016 20:38

kyleknap reviewed Jun 10, 2016
View reviewed changes

jamesls removed the pr/needs-review This PR needs a review from a member of the team. label Jun 13, 2016

JordonPhillips added pr/needs-review This PR needs a review from a member of the team. and removed incorporating-feedback labels Jun 13, 2016

JordonPhillips force-pushed the md5-sigv4 branch from d2ebf1d to 5882277 Compare June 13, 2016 22:51

jamesls reviewed Jun 14, 2016
View reviewed changes

jamesls added incorporating-feedback and removed pr/needs-review This PR needs a review from a member of the team. labels Jun 14, 2016

JordonPhillips added pr/needs-review This PR needs a review from a member of the team. and removed incorporating-feedback labels Jun 14, 2016

JordonPhillips force-pushed the md5-sigv4 branch 2 times, most recently from 10cda43 to 70e16bf Compare June 17, 2016 23:11

JordonPhillips force-pushed the md5-sigv4 branch from 70e16bf to ef0ff52 Compare June 17, 2016 23:25

JordonPhillips added 2 commits June 20, 2016 09:24

Add HTTP headers to ResponseMetadata

9256870

Knowing the headers can be useful, especially when the service model does not add them to the actual response.

JordonPhillips force-pushed the md5-sigv4 branch from ef0ff52 to fac97c8 Compare June 20, 2016 16:25

JordonPhillips mentioned this pull request Jun 20, 2016

Automatically redirect S3 sigv4 requests sent to the wrong region #936

Closed

JordonPhillips added pr/ready-to-merge This PR is ready to be merged. and removed pr/needs-review This PR needs a review from a member of the team. labels Jun 20, 2016

Use MD5 to sign S3 SigV4 bodies

d6d7f1e

S3 now supports optional SHA256 body signing, so this switches from SHA256 to MD5 to boost speed while reducing CPU and Memory usage when using sigv4 with S3. SHA256 will still be used on systems which do not support using MD5.

JordonPhillips force-pushed the md5-sigv4 branch from 061ad69 to d6d7f1e Compare June 20, 2016 19:28

JordonPhillips merged commit ab22547 into boto:develop Jun 20, 2016

JordonPhillips deleted the md5-sigv4 branch August 7, 2017 22:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use MD5 to sign S3 SigV4 bodies #920

Use MD5 to sign S3 SigV4 bodies #920

JordonPhillips commented May 13, 2016 •

edited

Loading

jamesls May 16, 2016

jamesls commented May 16, 2016

JordonPhillips commented May 17, 2016

JordonPhillips commented Jun 3, 2016

kyleknap commented Jun 3, 2016

jamesls commented Jun 6, 2016

JordonPhillips commented Jun 10, 2016

JordonPhillips commented Jun 10, 2016

kyleknap Jun 10, 2016

JordonPhillips Jun 10, 2016

JordonPhillips commented Jun 10, 2016

JordonPhillips commented Jun 13, 2016

jamesls Jun 14, 2016

JordonPhillips commented Jun 14, 2016

jamesls commented Jun 14, 2016

codecov-io commented Jun 17, 2016 •

edited

Loading

jamesls commented Jun 17, 2016

jamesls commented Jun 20, 2016

kyleknap commented Jun 20, 2016

JordonPhillips commented Jun 20, 2016

Use MD5 to sign S3 SigV4 bodies #920

Use MD5 to sign S3 SigV4 bodies #920

Conversation

JordonPhillips commented May 13, 2016 • edited Loading

jamesls May 16, 2016

Choose a reason for hiding this comment

jamesls commented May 16, 2016

JordonPhillips commented May 17, 2016

JordonPhillips commented Jun 3, 2016

kyleknap commented Jun 3, 2016

jamesls commented Jun 6, 2016

JordonPhillips commented Jun 10, 2016

JordonPhillips commented Jun 10, 2016

kyleknap Jun 10, 2016

Choose a reason for hiding this comment

JordonPhillips Jun 10, 2016

Choose a reason for hiding this comment

JordonPhillips commented Jun 10, 2016

JordonPhillips commented Jun 13, 2016

jamesls Jun 14, 2016

Choose a reason for hiding this comment

JordonPhillips commented Jun 14, 2016

jamesls commented Jun 14, 2016

codecov-io commented Jun 17, 2016 • edited Loading

Current coverage is 97.31%

jamesls commented Jun 17, 2016

jamesls commented Jun 20, 2016

kyleknap commented Jun 20, 2016

JordonPhillips commented Jun 20, 2016

JordonPhillips commented May 13, 2016 •

edited

Loading

codecov-io commented Jun 17, 2016 •

edited

Loading