S3 checksum middleware #102

jeskew · 2018-01-19T00:27:57Z

This PR also includes the work described in https://sim.amazon.com/issues/d2307adc-302f-4461-ad6a-81841442c388, as it would have been additional work to support ReadableStream|Blob right before ripping ReadableStream support out.

chrisradek · 2018-01-19T17:18:42Z

packages/apply-body-checksum-middleware/src/index.ts

+                !ArrayBuffer.isView(body) &&
+                !isArrayBuffer(body)
+            ) {
+                body = await streamCollector(body);


We don't actually want to collect an entire stream to calculate a hash though, right? If it helps, the glacier middleware pr I submitted has packages for calculating hashes on streams (throws an error if a non-file stream is encountered in node). I can change it to allow any Hash to be provided, instead of relying on sha256.

It doesn't cover WHATWG ReadableStreams because those aren't valid on requests yet, but we would just want to convert those to a Blob for now anyway, then use the BlobCollector.

I'll take a look at the Glacier PR, but I don't see how we'd be able to entirely consume a stream (to calculate the hash) and then also send it as a request body without putting the bytes somewhere. Throwing when someone uses anything other than an undecorated file stream seems very limiting. (I thought you had wanted to put special code for file handling in a higher-level glacier abstraction, which would let the base API handle common use cases like zlib and crypto streams.)

What if we added a "CollectedStream" type parameter everywhere we currently have a Stream type parameter? You're absolutely right that we should be using Blobs as stream sinks in browsers, and maybe we should be using temp files for the same purpose in node. Another option would be to use separate implementations of this middleware for Node & the browser so that special cases like Blobs and file streams can be handled efficiently.

I think it will make sense to either make this middleware environment specific, or let its MiddlewareCustomizationDefinition define an environment-specific configuration to pass in something that can hash a stream/file.

You're right that we'll need temp files in node if we want to perform this hashing. I realize now that the streamCollector hopefully shouldn't need to be used unless the user turns off unsigned payloads. I originally assumed that wouldn't be the case, since in V2, we calculate the MD5 still even with unsigned payloads (though maybe that is no longer a requirement).

What would the CollectedStream type be? Uint8Array in node, Blob in browsers?

After looking at your Glacier middleware PR, I think you were right that we need an environment-specific stream hasher instead of just reading the whole thing into memory. I would like to see streams transformed into a CollectedStream or RewindableStream before being fed into the hasher, but that can be a future improvement.

chrisradek · 2018-01-19T17:20:16Z

packages/apply-body-checksum-middleware/tsconfig.json

+        "importHelpers": true,
+        "noEmitHelpers": true,
+        "lib": [
+            "dom",


Is "dom" needed?

Oops, think I left that in after putting some debugging console.log statements in a few spots. Will remove.

chrisradek

More than just checksum middleware, looks pretty cool!

chrisradek · 2018-01-24T20:50:50Z

packages/service-types-generator/src/ServiceCustomizations/s3/bodySigning.ts

+            PutBucketReplication: [
+                md5Checksum,
+            ],
+            PutObject: [


I really hope this isn't the case anymore, but I remember with V2, when we used unsignedPayload with sigv4, we had to calculate the md5. Might be worth checking if this is still a requirement.

I verified that there is no technical requirement that we do so (sending PutObject operations with no Content-MD5 header and an X-Amz-Content-Sha256 header of UNSIGNED-PAYLOAD works just fine).

chrisradek · 2018-01-24T20:55:35Z

packages/apply-body-checksum-middleware/src/index.ts

+                !ArrayBuffer.isView(body) &&
+                !isArrayBuffer(body)
+            ) {
+                body = await streamCollector(body);


I think it will make sense to either make this middleware environment specific, or let its MiddlewareCustomizationDefinition define an environment-specific configuration to pass in something that can hash a stream/file.

You're right that we'll need temp files in node if we want to perform this hashing. I realize now that the streamCollector hopefully shouldn't need to be used unless the user turns off unsigned payloads. I originally assumed that wouldn't be the case, since in V2, we calculate the MD5 still even with unsigned payloads (though maybe that is no longer a requirement).

What would the CollectedStream type be? Uint8Array in node, Blob in browsers?

… changes to mw stack

chrisradek · 2018-02-14T23:44:41Z

packages/fetch-http-handler/src/fetch-http-handler.ts

@@ -72,21 +69,11 @@ export class FetchHttpHandler implements HttpHandler<ReadableStream, BrowserHttp
                        transformedHeaders[pair[0]] = pair[1];
                    }

-                    const httpResponse: HttpResponse<ReadableStream> = {
+                    return response.blob().then<HttpResponse<Blob>>(body => ({


I think this is a fair change for now since we're in dev preview and it isn't easy to convert a stream to a blob (and blobs are more useful than streams right now)

Someday, we can have browser streams; hopefully before GA!

* Add body-checksum-applying middleware * Add a universal JS MD5 package * Add S3 checksum middleware bindings * Add SigV4 unsigned payload middleware * Adapt body checksum and unsigned payload middleware to reflect recent changes to mw stack * Add Glacier .gitignores * Incorporate changes made for Glacier middleware addition * Change browser stream type from ReadableStream to Blob * Regenerate browser SDK packages * Fix failing tests

lock · 2019-09-26T20:06:09Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

jeskew requested review from AllanZhengYP and chrisradek January 19, 2018 00:27

chrisradek reviewed Jan 19, 2018

View reviewed changes

chrisradek reviewed Jan 24, 2018

View reviewed changes

jeskew changed the title ~~Feature/s3 checksum middleware~~ [WIP] S3 checksum middleware Feb 9, 2018

jeskew and others added 10 commits February 9, 2018 23:52

Add body-checksum-applying middleware

f6aad5d

Add a universal JS MD5 package

3c18aaa

Add SigV4 unsigned payload middleware

5298809

Add S3 checksum middleware bindings

fdf42b8

Adapt body checksum and unsigned payload middleware to reflect recent…

03674b4

… changes to mw stack

Add Glacier .gitignores

7c3501f

Incorporate changes made for Glacier middleware addition

a51d5c7

Change browser stream type from ReadableStream to Blob

6ccacf0

Regenerate browser SDK packages

47a3b03

Fix failing tests

9a5873d

jeskew changed the title ~~[WIP] S3 checksum middleware~~ S3 checksum middleware Feb 10, 2018

jeskew added the must-ship-with-dev-preview label Feb 14, 2018

chrisradek approved these changes Feb 14, 2018

View reviewed changes

jeskew merged commit 3c642b4 into aws:master Feb 15, 2018

jeskew deleted the feature/s3-checksum-middleware branch February 15, 2018 02:21

lock bot locked as resolved and limited conversation to collaborators Sep 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

S3 checksum middleware #102

S3 checksum middleware #102

jeskew commented Jan 19, 2018 •

edited

Loading

chrisradek Jan 19, 2018

jeskew Jan 19, 2018

chrisradek Jan 24, 2018

jeskew Jan 24, 2018

chrisradek Jan 19, 2018

jeskew Jan 19, 2018

chrisradek left a comment

chrisradek Jan 24, 2018

jeskew Jan 24, 2018

chrisradek Jan 24, 2018

chrisradek Feb 14, 2018

jeskew Feb 15, 2018

lock bot commented Sep 26, 2019

S3 checksum middleware #102

S3 checksum middleware #102

Conversation

jeskew commented Jan 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisradek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lock bot commented Sep 26, 2019

jeskew commented Jan 19, 2018 •

edited

Loading