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

Support for ReadableStream body #50

Closed
wants to merge 3 commits into from

Conversation

Manouchehri
Copy link
Contributor

This is quite important for us folks using aws4fetch in a CF Worker in bundled billing mode.

Fixes #36

@Manouchehri
Copy link
Contributor Author

For those who realllllly can't wait (like myself), here's a published fork: https://www.npmjs.com/package/@aimoda/aws4fetch

@mhart
Copy link
Owner

mhart commented Feb 9, 2023

To be clear, you can set the X-Amz-Content-Sha256 header to whatever you like (including UNSIGNED-PAYLOAD) – you don't need to use a fork of this library to do this.

@Manouchehri
Copy link
Contributor Author

Agreed, I think this PR makes the behavior a bit more developer friendly. (I think most of us often use ReadableStreams because we don't want to buffer everything into memory.)

@mhart
Copy link
Owner

mhart commented Feb 9, 2023

I'm not convinced this works with every service though – in fact, I suspect it works with very few as, at least in the past, most services required fully hashed bodies. Happy to test it out though. Which services have you tested it on?

@Manouchehri
Copy link
Contributor Author

I'm using it with Lambda Function URLs.

https://docs.aws.amazon.com/lambda/latest/dg/urls-invocation.html#urls-payloads

@mhart
Copy link
Owner

mhart commented Feb 9, 2023

Right, that makes sense for that particular use case. But nearly every other AWS service uses fixed JSON/XML payloads as bodies, so using ReadableStreams will likely just get signature errors. I can test it out when I get some time, but I don't think this should go in as is – at the very least the documentation should be updated to highlight that users might get errors if they use streams.

@Manouchehri
Copy link
Contributor Author

Nope, I am 100% wrong. It does not work with Lambda Function URLs like I claimed. Not sure why I thought it was working.

@Manouchehri Manouchehri closed this Feb 9, 2023
@bcheung
Copy link

bcheung commented Oct 8, 2024

Hi @mhart
I think I have a similar use case as @Manouchehri where my request body could be a ReadableStream and for context, I'm using a cloudflare worker to proxy requests to my lambda function url which requires IAM authorization.

This error is thrown when the request body is a ReadableStream:

aws4fetch/src/main.js

Lines 328 to 330 in 629b108

if (this.body && typeof this.body !== 'string' && !('byteLength' in this.body)) {
throw new Error('body must be a string, ArrayBuffer or ArrayBufferView, unless you include the X-Amz-Content-Sha256 header')
}

I guess this makes sense as this check requires the payload to be signed.

However, I'm wondering:

  1. if the only way to send a ReadableStream request body is to provide the X-Amz-Content-Sha256 header for signing?
    That would mean the client making a request to the proxy must provide a hash of the payload, and my proxy would then need to set that as the X-Amz-Content-Sha256 header. Otherwise, I'd have to read the full payload and hash it on the worker like you seem to do here:

    aws4fetch/src/main.js

    Lines 83 to 89 in 629b108

    async sign(input, init) {
    if (input instanceof Request) {
    const { method, url, headers, body } = input
    init = Object.assign({ method, url, headers }, init)
    if (init.body == null && headers.has('Content-Type')) {
    init.body = body != null && headers.has('X-Amz-Content-Sha256') ? body : await input.clone().arrayBuffer()
    }

  2. Do you know if the request body is always one of either a ReadableStream or null? If not, do you know of a reliable way to detect if the request body is a ReadableStream?

Apologies if some of this is out of scope, but I'm very new to this whole signing concept and haven't really worked withReadableStream much so I appreciate if you could help fill some knowledge gaps!

Also thanks for making this library!

@Manouchehri
Copy link
Contributor Author

I think we ended up just converting to an ArrayBuffer and avoided using ReadableStream objects in our Cloudflare Workers.

@bcheung
Copy link

bcheung commented Oct 10, 2024

Ah appreciate the reply that's what I ended up doing

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.

Support for streaming body via UNSIGNED_PAYLOAD
3 participants