Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes to AWS construct to support streaming #1496

Draft
wants to merge 5 commits into
base: series/0.19
Choose a base branch
from

Conversation

daddykotex
Copy link
Contributor

TLDR: the design you originally proposed mostly works. the only thing that I needed to fix was to carry along a function to convert from bytes to the StreamingInput or StreamingOutput types. This PR only contains a few changes I'd make to improve things when we want to support streaming payload signature, trailers, etc.

I've opened this PR because theses are changes I made to make s3-restart-wip work.

It does the following:

  1. reorder headers so that they are alphabetically sorted for the signature
  2. include any headers starting with x-amz in the signature
  3. extract payload signing in a middleware of it's own and default to unsigned-payload

Regarding 2), I did that because if we introduce middleware like CRC/SHA checksum, they'll include additional headers that need to be in the signature.

Regarding 3), I did that because generating a sha256 hash and including it in the signature works but is only one way, of several ways, to do payload signature. It does not work well when the payload is large (load everything in memory, it does not support sending the payload in chunks), and it does not work for sigv4a.

For example, signing the payload on a request where the payload is uploaded in chunks is described in the docs. One of the input of the first chunk is the signature computed for the Authorization header, so we'd probably have to rework how we do ( in AWSSigning) to expose it.

In the list of changes in s3-restart-wip, you'll see that I also include the java-sdk. This is for testing and inspecting what the java SDK does when we enable certain things. But I'M UNABLE TO ENABLE THE SIGNATURE OF THE PAYLOAD. The only thing that comes close is setting the CRC32 checksum algorithm which will affect the request (change the body to include trailers, and so it changes the X-Amz-Content-SHA256 header to STREAMING-UNSIGNED-PAYLOAD-TRAILER, add a bunch of headers).

Example:

PUT /david/bytes-todelete.txt HTTP/1.1
Host: som-bucket.s3.amazonaws.com
amz-sdk-invocation-id: 1f70431f-b110-74b5-0962-c64b0f8b4b69
amz-sdk-request: attempt=1; max=4
Authorization: AWS4-HMAC-SHA256 Credential=****************/20240419/us-east-1/s3/aws4_request, SignedHeaders=amz-sdk-invocation-id;amz-sdk-request;content-encoding;content-length;content-type;host;x-amz-content-sha256;x-amz-date;x-amz-decoded-content-length;x-amz-sdk-checksum-algorithm;x-amz-security-token;x-amz-trailer, Signature=ddc0712d2d703eebe2f7dfd90e10addd18d130be24635bf6a779d943cdff8cc9
Content-Type: application/octet-stream
User-Agent: aws-sdk-java/2.25.11 Mac_OS_X....
x-amz-content-sha256: STREAMING-UNSIGNED-PAYLOAD-TRAILER
X-Amz-Date: 20240419T142913Z
x-amz-decoded-content-length: 16
x-amz-sdk-checksum-algorithm: CRC32
X-Amz-Security-Token: *****
x-amz-trailer: x-amz-checksum-crc32
Content-Length: 58
Connection: Keep-Alive

10
my data is in s3
0
x-amz-checksum-crc32:N7SBfA==

To view this request, I've also used mitm and target my AWS java sdk client to it. It makes it easy to see what the SDK does when playing with settings.

PR Checklist (not all items are relevant to all PRs)

  • Added unit-tests (for runtime code)
  • Added bootstrapped code + smoke tests (when the rendering logic is modified)
  • Added build-plugins integration tests (when reflection loading is required at codegen-time)
  • Added alloy compliance tests (when simpleRestJson protocol behaviour is expanded/updated)
  • Updated dynamic module to match generated-code behaviour
  • Added documentation
  • Updated changelog

@daddykotex daddykotex marked this pull request as draft April 19, 2024 17:56
@daddykotex
Copy link
Contributor Author

Updated the branch at s3-restart-wip to support upload. It worked mostly how you'd expect it. The only thing that did not work how of the box was that when using withEntity(stream) where is stream is Stream[F, Byte] it will add a transfer encoding header that leads to a 501.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant